Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

apply_impulse is not intuitive #21634

Closed
Tracked by #45333
HugoSlt opened this issue Aug 31, 2018 · 10 comments
Closed
Tracked by #45333

apply_impulse is not intuitive #21634

HugoSlt opened this issue Aug 31, 2018 · 10 comments

Comments

@HugoSlt
Copy link

HugoSlt commented Aug 31, 2018

Godot version:
3.1, 9eb4d4a

Issue description:
The function void apply_impulse( Vector3 position, Vector3 impulse ) is not intuitive to use.
What we would expect is :

  • We specify the position of the impulse in world coordinates
  • We specify the force vector in world coordinates

So let's say we have a rigid body and we want to apply an impulse at an offset of 2 on the Y axis of the rigid body (i.e the impulse point is at (0,2,0) in local coordinates) with a force along the X axis of the rigid body (i.e the force vector is for eg (1,0,0) in local coordinates).

We should be able to do :

apply_impulse( to_global(Vector3(0,2,0)) , to_global(Vector(1,0,0) )
Or equivalently :
apply_impulse( to_global(Vector3(0,2,0)), transform.basis.x )

But this is not correct, instead we currently have to do :

apply_impulse(transform.basis.xform(Vector3(0,2,0)), transform.basis.x)

For e.g in Unity AddForceAtPosition seems much clearer and easier to understand : Force and Position are in global coordinates and that's it.
Same in Unreal Engine, Same in Lumberyard, etc...

@HugoSlt
Copy link
Author

HugoSlt commented Aug 31, 2018

This happens because to_global(pos) is equivalent to transform.xform(pos) but apply_impulse() seems to expect transform.basis.xform(pos) instead.

@raymoo
Copy link
Contributor

raymoo commented Sep 1, 2018

It's because it uses local position but world rotation

@HugoSlt
Copy link
Author

HugoSlt commented Sep 1, 2018

@raymoo Indeed, but do you find it intuitive to have a position that is neither a World Position nor a Local Position ? I find " local position but world rotation" un-intuitive when we could just have a world position.

@raymoo
Copy link
Contributor

raymoo commented Sep 1, 2018

It's intuitive in the case that the impulse comes from an external source. In that case you don't want the rotation to affect the direction of the force

@HugoSlt
Copy link
Author

HugoSlt commented Sep 1, 2018

Hum, here is what I understand :

  • The direction of the force (impulse) is a Vector3 in world space. Here no problem, we are aligned with other engine. So if you don't want the rotation of the rigid body to influence the direction of the force you pass for eg Vector(0,1,0) (and it will be attracted to the sky whatever rotation the rigid body has). On the contrary, if you have a rocket, then you pass to_global(Vector(0,1,0)) (or transform.axis.y) and it will push the rocket forward (the force depends on the rotation of the rigid body)

  • Now the position where the impulse is applied (position) is actually not a position. And this is where we are not aligned with other engines. In other engines it is just a position in World Space, in Godot this is actually "a world space offset from the rigidbody's origin".

So on a drawing :
drawingrigidbody

For eg in Unreal Engine AddForceAtLocation will take the force direction in World Space (like in Godot) and the impulse position in World Space (in Pink on the drawing). Then AddForceAtLocationLocal will take the force direction in Local Space and the impulse position in Local Space (in Yellow on the drawing), the equivalent in Godot would be #20031.

But for apply_impulse, in Godot we don't take the Pink point, but the Blue one.

Maybe I completely misunderstood, but in case I got this right, then this Blue point does not correspond to the position of anything in any space, but corresponds to the offset between the rigidbody center and the impulse position.

In that case I think we should :

  • Either align with other engines, and say we simply give a position in World Space.
  • Or keep the current definition but then change the arguments name of apply_impulse ( Vector3 position, Vector3 impulse ) to apply_impulse ( Vector3 offset, Vector3 impulse ) and change the doc description from :

Both the impulse and the position are in global coordinates, and the position is relative to the object’s origin.

To maybe something like :

impulse is a (global coordinate) vector representing the force applied. offset is the (global coordinate) vector between the RigidBody center and the impulse position.

@aaronfranke
Copy link
Member

aaronfranke commented Sep 1, 2018

Related: #16863 (comment)

We may have something like apply_impulse_local being local and apply_impulse being global. This is how many methods work, by adding _local. Or, we could add a _global suffix if the local one is more common.

@HugoSlt
Copy link
Author

HugoSlt commented Sep 3, 2018

For apply_impulse_local there wouldn't be any confusion as the offset from the rigidbody's center in Local Coor is the same as the Local Coor position (both are the Yellow point on the drawing). So you could name the function apply_impulse_local(position, impulse) or apply_impulse_local(offset, impulse) both names would be correct.

The problem is for apply_impulse as the offset from the rigidbody's center in Global Coor (Blue point) is not the same as from the Global Coor position (Pink point). That's why here I think we should either rename position to offset or actually use the global position like all other engines.

If we choose to use the global position like the other engines, it would be a good idea to do it at the same time as #16863 as this would also break compat

@Oranjoose
Copy link

Oranjoose commented Dec 17, 2018

Why is offset the first argument in apply_impulse and apply_force? Isn't the magnitude (currently 2nd argument) practically always supplied, but offset is only sometimes supplied? Wouldn't it make more sense to make the offset and optional argument where the default is vector.zero?

@aaronfranke
Copy link
Member

@pouleyKetchoupp
Copy link
Contributor

This was fixed in #37350, closing.

@pouleyKetchoupp pouleyKetchoupp added this to the 4.0 milestone May 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants