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

Improve RigidDynamicBody force and torque API #55736

Merged
merged 1 commit into from
Dec 11, 2021

Conversation

pouleyKetchoupp
Copy link
Contributor

@pouleyKetchoupp pouleyKetchoupp commented Dec 8, 2021

Makes the API for forces and impulses more flexible, easier to understand and harmonized between 2D and 3D.

Rigid bodies now have 3 sets of methods for forces and impulses:
-apply_impulse() for impulses (one-shot and time independent)
-apply_force() for forces (time dependent) applied for the current step
-add_constant_force() for forces that keep being applied each step

Also updated the documentation to clarify the different methods and parameters in rigid body nodes, body direct state and physics servers.


Simple script examples to show usage for different cases:

Apply Impulse

if Input.is_action_just_pressed("ui_accept"):
	# Apply one-shot impulse to jump when pressing the button.
	apply_central_impulse(Vector2(0, -300))

Apply Force

if Input.is_action_pressed("ui_accept"):
	# Apply continuous force to levitate while pressing the button.
	apply_central_force(Vector2(0, -1000))

Add Constant Force

if Input.is_action_just_pressed("ui_accept"):
	# Toggle continuous force to set levitation on/off when pressing the button.
	if constant_force == Vector2():
		add_constant_force(Vector2(0, -1000))
	else:
		constant_force = Vector2()

Fixes #38646
Closes godotengine/godot-proposals#3601
Supersedes #42850

@madmiraal
Copy link
Contributor

I do not see how this PR makes "forces and impulses more flexible, easier to understand." Adding more methods and having two do the same thing is not making things easier to understand or more flexible. The way 2D works at the moment is already flexible and easy to understand. #42850 makes 3D the same as 2D (harmonizing 2D and 3D.").

As stated previously in the proposal:

I'm not convinced that the arguments:

-Applying one-shot forces wouldn't be possible directly anymore, it would require using impulses with a * delta factor which can be difficult to understand for users not familiar with physics systems.
-When multiple forces need to be set on and off, it would require to clear all forces and re-apply the desired ones.

justify adding a third set of methods presented in this PR:

void add_constant_central_force ( Vector2 force )
void add_constant_force ( Vector2 force, Vector2 offset )
void add_constant_torque ( float torque )

I addressed each argument individually there, but I'll repeat them here to ensure they're captured:

Applying one-shot forces wouldn't be possible directly anymore, it would require using impulses with a * delta factor which can be difficult to understand for users not familiar with physics systems.

A one-shot force is an impulse, which can be applied directly using apply_impulse(). I think requiring the use of a * delta time factor (which is standard physics i.e. impulse = force x time) is far less difficult to understand than trying to explain why we have three sets of methods all basically doing the same thing.

When multiple forces need to be set on and off, it would require to clear all forces and re-apply the desired ones.

With #42850 this would simply require a single line of code: applied_force = Vector3(). The user is already "re-apply the desired ones." Again, I think this is a lot less difficult to understand than trying to explain when to use add_constant_force() and when to use add_force(); especially when we also have apply_impulse().

Note: This is how it works in 2D at the moment! #42850 is simply making 3D the same as 2D. It's just making them consistent. It's not introducing anything new.

As per the best practices, we need one solution for each problem. We should not have multiple methods doing the same thing.

@pouleyKetchoupp pouleyKetchoupp force-pushed the physics-apply-forces branch 2 times, most recently from 3ec6898 to 094049f Compare December 8, 2021 21:59
@Diddykonga
Copy link
Contributor

Diddykonga commented Dec 9, 2021

I quite frankly disagree with @madmiraal.

I believe in 99% of use cases that most people would need to include your line of applied_force = Vector3() which is unintuitive.
Also, since this PR provides add_constant_force() your use case of adding a constant force which requires reset if needed, is still provided, and is more clear to the user as to the intended use.

As for the removal of apply_force() in favor of apply_impulse( * delta) this is equivalent to the argument of removing move_and_slide() since technically move_and_collide() already exists, one could simply implement their own, but it is provided to display to users the intended use.

I agree with this PR as implemented. +1

@madmiraal
Copy link
Contributor

I've previously provided demo projects showing that add_force() and apply_impulse(* delta) are the same.

As per the best practices, there needs to be a problem before providing a solution. So far I have not seen anyone provide a practical example that demonstrates a problem with using apply_impulse(* delta) instead of the current 3D add_force().

@pouleyKetchoupp
Copy link
Contributor Author

@madmiraal Copy-pasting the same arguments over and over again in different places without adding to the discussion is not helping.
This is not the first time this happens.

Your concerns have been previously addressed, in the original PR, here and in the proposal.

Please discontinue.

@madmiraal
Copy link
Contributor

Please show me where anyone has provide a practical example that demonstrates a problem with using apply_impulse(* delta) instead of the current 3D add_force().

I cut and paste my arguments, because they have not been incorporated into the discussion and my concerns have not been addressed. I specifically provide references to the previous comments to highlight this.

@AwayB
Copy link

AwayB commented Dec 10, 2021

Please show me where anyone has provide a practical example that demonstrates a problem with using apply_impulse(* delta) instead of the current 3D add_force().

As a non-physics expert, allow me:
In English, an impulse is a one-time push, for ex. the explosion of a cartridge's powder to shoot a bullet out of a muzzle. It simply makes no sense to call a continuous energy an "impulse".
A force on the other hand implies a continuous force being applied, for example a rocket that keeps getting pushed forward by its fuel's continuous explosion.

The point of an API is to provide a simple to understand system that requires the least knowledge of its inner workings. For a person competent with physics and the associated maths, having only apply_impulse(* delta) might make more sense, but for a person that doesn't have the time to learn the details of how physics work, apply_force() and apply_impulse() are much easier to understand since they use the proper wording of force and impulse rather than mathematical applications that the underlying system uses.

If you believe that clearer wording should come second to logical correctness, I would agree with you for the internals, but that is simply not applicable to an API. I'm quite confident as a user that 99% of other users of Godot do not want to have to learn how the physics work but just to get it working ASAP, and using proper English is the simplest way to do it.

@madmiraal
Copy link
Contributor

@KingArrkinian I agree. This is exactly the point I'm trying to make.

@Diddykonga
Copy link
Contributor

@KingArrkinian I agree. This is exactly the point I'm trying to make.

There is a difference between:
one-time, frame-dependent velocity, an Impulse.
one-time, frame-independent velocity, an Force.
continuous, frame-independent velocity, an Continuous Force.

This PR provides an API for all three uses cases, and are now arguing over semantics, which I believe this PR with its wording makes very clear each use case.

There are already plenty examples, as I gave one in my previous post with move_and_slide() being a higher-level implementation of move_and_collide(), therefore there is already precedent for having the API as described, not to mention even though it is frowned upon to say, every other game engine I have used has the same API, with the exception of a continuous force, but if anything that API was specifically implemented for your use case.

At the very least, let this PR be merged, and create a new Proposal with your suggested changes to discuss it there.

@pouleyKetchoupp
Copy link
Contributor Author

@KingArrkinian I agree. This is exactly the point I'm trying to make.

Great, this proves enough information was provided and you agree with me on this PR.

Makes the API for forces and impulses more flexible, easier to
understand and harmonized between 2D and 3D.

Rigid bodies now have 3 sets of methods for forces and impulses:
-apply_impulse() for impulses (one-shot and time independent)
-apply_force() for forces (time dependent) applied for the current step
-add_constant_force() for forces that keeps being applied each step

Also updated the documentation to clarify the different methods and
parameters in rigid body nodes, body direct state and physics servers.
@madmiraal
Copy link
Contributor

Great, this proves enough information was provided and you agree with me on this PR.

No. @KingArrkinian describes 99% of users' expectation of the API, which is the current 2D API, and which I agree is the correct one.

To convince me of this PR, all I ask for is a practical example (i.e. a sample Godot project) that demonstrates a problem with using apply_impulse(* delta) instead of the current 3D add_force(). This would justify the need to keep 3D's current add_force() methods i.e. they cannot be made to be the same as 2D's add_force() methods, and hence justify creating the third set of methods this PR proposes.

@pouleyKetchoupp pouleyKetchoupp merged commit f1ca14c into godotengine:master Dec 11, 2021
@pouleyKetchoupp pouleyKetchoupp deleted the physics-apply-forces branch December 11, 2021 00:16
@multimike
Copy link

multimike commented Dec 12, 2021

@madmiraal
I would say it is bad practice to misuse a function to fulfill a similar result.
add_force may have the same effect as add_impulse(* delta) today, but look at this as a mere "coincidence".
If the need arise to change in the underlying implementation, it would be very unfortunate to have misled people to use impulse when they actually meant force.

Also, the reason I found this issue is that I was bitten by forces not being cleared as I expected in 2D, which makes me doubt your claim that 99% of the users expect forces not to clear. Can you present the numbers pointing to that fact?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants