Refactor physics force and impulse code to use (force, position) order

[aaronfranke]

Implements the proposal in this comment: #16863 (comment)

add_force now uses (force, position) and apply_impulse now uses (impulse, position). This very much breaks compatibility, in a way that isn't visible in projects until runtime (the worst kind).

The position parameter is now optional, therefore I removed add_central_force and apply_central_impulse since they are now redundant (the code still has checks and optimizations).

In addition to the above, I also enforced a consistent naming scheme. Previously position could either be offset or pos, now it's position everywhere, also impulse could either be j or impu, now it's impulse everywhere.

[AndreaCatania]

What's the reason to remove the query with add central force/impulse?
To me this just make the other API integration more complex, and if it uses interally the add central impulse why would you remove the possibility to use that directly from GDScript?

[aaronfranke]

@AndreaCatania Because add_central_force(my_impulse) is the same as add_force(my_impulse).

[AndreaCatania]

Well, this is not true. Add force performs more thing than just add a vector and this matters when you have to use a lot of times per each frame this function.

What's the benefit that we get removing such API?

[aaronfranke]

I don't understand what you mean. This is the old code:

_FORCE_INLINE_ void add_central_force(const Vector2 &p_force) {
	applied_force += p_force;
}

_FORCE_INLINE_ void add_force(const Vector2 &p_offset, const Vector2 &p_force) {
	applied_force += p_force;	
	applied_torque += p_offset.cross(p_force);	
}

applied_force += p_force; is the exact same between the two, the only difference is with applied_torque += p_offset.cross(p_force); being present or absent, so I changed the code to this:

_FORCE_INLINE_ void add_force(const Vector2 &p_force, const Vector2 &p_position = Vector2()) {
	applied_force += p_force;
	if (p_position != Vector2()) {
		applied_torque += p_position.cross(p_force);
	}
}

The new add_force(my_impulse) is the exact same as the old add_central_force(my_impulse).

The benefit is not having two functions that do the exact same thing.

[AndreaCatania]

These are not the same, otherwise would not be there. The reason is that when you don't want to add an offsetted force (most likely) you don't want to also perform:

	if (p_position != Vector2()) {
		applied_torque += p_position.cross(p_force);
	}

nor send useless position vectors around.

I don't think this API is unclear or confusing so I don't understand what is the real benefit.

[realkotob]

I don't disagree with @AndreaCatania, it might be a good idea to keep them separate.

However, I think add_central_force should be renamed to add_force and add_force should be renamed to add_force_offset, since adding a central impulse is the default use case so it should be made very clear.

[aaronfranke]

What about if add_central_force is kept internally, but we only expose add_force to GDScript and have only the exposed methods check if (p_position == Vector3())? That way we can avoid passing around unnecessary position vectors internally, but there is no duplicate exposed API.

If not, we could also keep add_central_force exposed and deprecate it, or we could keep it forever and still allow add_force to check if the position is a zero vector and switch over to add_central_force internally.

[aaronfranke]

@AndreaCatania I removed all the controversial changes from this PR, now it's just the (force, position) etc ordering, and using the full words "force", "impulse", and "position". I'd appreciate a review. The docs are updated, so you can look at them to see the publicly-facing API changes.

[AndreaCatania]

Modify all the functions were not needed but is cool to have all with the same order, Thanks.

[akien-mga]

Thanks!