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

Correct the doc about linear damping #42604

Merged
merged 1 commit into from
Oct 13, 2020
Merged

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Oct 6, 2020

Resolves #33676

Seems like the doc was wrong, because there is visible difference between 1 damp and 100 damp. Also the values in Project Settings and RigidBody go from 0 to 100 too.

@KoBeWi KoBeWi added bug documentation topic:physics cherrypick:3.x Considered for cherry-picking into a future 3.x release labels Oct 6, 2020
@KoBeWi KoBeWi added this to the 4.0 milestone Oct 6, 2020
@KoBeWi KoBeWi requested a review from a team as a code owner October 6, 2020 18:03
doc/classes/Area3D.xml Outdated Show resolved Hide resolved
@KoBeWi KoBeWi force-pushed the da100mp branch 2 times, most recently from 6ac4ede to 58e088a Compare October 6, 2020 18:41
@KoBeWi
Copy link
Member Author

KoBeWi commented Oct 6, 2020

Ok updated the descriptions to mention the physics fps, but I wonder if it can't be written better (and somehow mention the default value).

@bruvzg
Copy link
Member

bruvzg commented Oct 6, 2020

Maybe we should actually use fixed 0...1 or 0...100 range for the properties and remap values to 0...phy_fps in the dumping calculations.

@KoBeWi
Copy link
Member Author

KoBeWi commented Oct 6, 2020

So should I do it in this PR? Where is it calculated?

@bruvzg
Copy link
Member

bruvzg commented Oct 6, 2020

Where is it calculated?

servers/physics_server_2d.cpp in PhysicsDirectBodyState2D::integrate_forces
servers/physics_server_3d.cpp in PhysicsDirectBodyState3D::integrate_forces
servers/physics_2d/body_2d_sw.cpp in Body2DSW::integrate_forces
servers/physics_3d/body_3d_sw.cpp in Body3DSW::integrate_forces

@akien-mga
Copy link
Member

Maybe we should actually use fixed 0...1 or 0...100 range for the properties and remap values to 0...phy_fps in the dumping calculations.

That's an option, but it breaks compat so it should likely be in a separate PR.

CC @AndreaCatania @madmiraal @pouleyKetchoupp

@bruvzg
Copy link
Member

bruvzg commented Oct 6, 2020

And somewhere else for the Bullet server, it's using the same calculation, since we have BT_USE_OLD_DAMPING_METHOD defined, but I'm not sure where to remap the values.

@pouleyKetchoupp
Copy link
Contributor

Mapping the values from a 0..1 scale seems like a good idea for 4.0.

In Bullet the mapping only needs to happen here, when we're passing the values to Bullet:

btBody->setDamping(newLinearDamp, newAngularDamp);

doc/classes/Area3D.xml Outdated Show resolved Hide resolved
@madmiraal
Copy link
Contributor

First, it's worth pointing out that Godot damping, as it is currently implemented, is not physically accurate. If it was physically accurate, only values between 0 and 1 would be valid. However, as #19182 demonstrated, physically accurate damping is not intuitive. Accurate physical damping increases exponentially as values approach 1. Instead, to make Godot damping more intuitive, Godot damping is linear. Values less than 0.5 are pretty close to accurate, but noticeably less severe as they approach and pass 1.0. See the table I created here for a comparison.

Godot damping is, as the documentation states: "the linear velocity lost per second", or perhaps more accurately, "the proportion of linear velocity lost per second". Although this can never be greater than 100%, values greater than 1 make sense, because the physics iteration rate is less than a second. Setting the damping to 2 will aim to decrease the velocity to 0 in half a second. Although, it's worth noting that at the next iteration, the velocity will be less; so the actual velocity lost with each iteration (being proportional to the velocity) will be less too. Setting damping to 100 will aim to decrease velocity to 0 in 10ms.

Damping is not limited by the physics iteration rate. It is limited by the accuracy of the physics engine, which is limited by the physics iteration rate. In other words, setting the damping equal to the physics iteration rate (by default 60) will reduce the velocity by 100% in one physics iteration. Values greater than the physics iteration rate won't make a difference, because the body will be stopped regardless.

Bearing all that in mind. I think a range of 0 - 100 is right. Any more would not be noticeable - even at higher physics iteration rates. There is definitely no need to scale it - just to fit into the notion that it should only be between 0 and 1. It is currently both meaningful and accurate at normal, low values less than 0.5.

All that been said, my suggestion for the wording is therefore:

The rate at which objects stop moving in this area. Represents the proportion of [linear|angular] velocity lost per second. Values range from [code]0[/code] (no damping) to [code]100[/code].
Note: Good values are in the range 0 to 1. Values greater than 1 will aim to reduce the velocity to 0 in less than 1 second e.g. a value of 2 will aim to reduce the velocity to 0 in half a second. A value equal to or greater than the physics frame rate ([code]ProjectSettings.physics/common/physics_fps[/code]) will bring the object to a stop in one iteration.

@KoBeWi
Copy link
Member Author

KoBeWi commented Oct 7, 2020

Soo this long node should be for all four properties? Since it's relevant for any damping, maybe it should be documented in one place and we could just link to it?

@bruvzg
Copy link
Member

bruvzg commented Oct 7, 2020

Bearing all that in mind. I think a range of 0 - 100 is right. Any more would not be noticeable - even at higher physics iteration rates.

Since 100 is not hard limit and not a special value, I would remove range at all and only add note, it's describing it better than arbitrary values.

The rate at which objects stop moving in this area. Represents the proportion of [linear|angular] velocity lost per second.

Note: Good values are in the range [code]0[/code] to [code]1[/code]. At value [code]0[/code] objects will keep moving with the same velocity. Values greater than [code]1[/code] will aim to reduce the velocity to [code]0[/code] in less than a second e.g. a value of [code]2[/code] will aim to reduce the velocity to [code]0[/code] in half a second. A value equal to or greater than the physics frame rate ([member ProjectSettings.physics/common/physics_fps]) will bring the object to a stop in one iteration.

@KoBeWi KoBeWi force-pushed the da100mp branch 2 times, most recently from d7c2600 to ae084fb Compare October 11, 2020 19:47
@KoBeWi
Copy link
Member Author

KoBeWi commented Oct 11, 2020

Ok so I copied bruvzg's note to ProjectSettings.physics/2d/default_linear_damp and Area/RigidBody docs link to it. The mention of ranges in the Area descriptions is removed.

doc/classes/Area3D.xml Outdated Show resolved Hide resolved
@akien-mga
Copy link
Member

Last nitpicks, then it's good to go. Good call on also adding the note for both 2D and 3D in the ProjectSettings.

@akien-mga akien-mga merged commit 7baefe8 into godotengine:master Oct 13, 2020
@akien-mga
Copy link
Member

Thanks!

@KoBeWi KoBeWi deleted the da100mp branch October 13, 2020 09:54
@akien-mga
Copy link
Member

Cherry-picked for 3.2.4.

@akien-mga akien-mga removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Oct 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

linear_damp and angular_damp should be 0 to 1
5 participants