search

czaloj / bullet

Automatically exported from code.google.com/p/bullet
0 stars 0 forks source link

Incorrect damping formula in btRigidBody::applyDamping #74

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago 
What steps will reproduce the problem?
1. set a damping
2.
3.

What is the expected output? What do you see instead?

incorrect velocity change applied to object

What version of the product are you using? On what operating system?

2.70, linux + win32

Please provide any additional information below.

This problem applies identically to linear and angular damping so I will
just say "damping" here.

Damping is specified as a value between 0 and 1, which is the proportion of
velocity lost per second.

btRigidBody::applyDamping has a harder job, however, as it does not update
the velocity every second.  It takes as parameter the amount of time in
seconds since the last update (usually 1/60) and has to apply part of the
damping.

The obvious correctness property here is that if you divide 1 second into n
equal parts, 1 call of applyDamping(1) should be the same as n calls of
applyDamping(1/n).

This is not the case, as the top two lines of the function are using the
wrong formula to apply "partial" damping.  Rather than multiplying the
amount of damping by the timestep, we need to raise to the power.  My
attempt at the correct formula is below:

m_linearVelocity *= powf(GEN_clamped(1.0 - m_linearDamping, 0.0,
1.0),timeStep);
m_angularVelocity *= powf(GEN_clamped(1.0 - m_angularDamping, 0.0,
1.0),timeStep);

powf is probably not the right function as it assumes btScalar is a float.  

Note that since m_linearDamping etc are already clamped betwen 0 and 1, the
additional clamp here is probably not necessary.  The clamp is needed in
the original formula only because it computes the wrong amount of damping,
and can thus invert the velocity if the timeStep is larger than 1.

Also note that when timeStep is 1 second, the correct damping is applied.
and when timestep is 0.5 second, two calls of the function will result in

velocity *= powf(1-damping,0.5) * powf(1-damping,0.5)

which is the correct result, equivalent to:

velocity *= 1-damping

I assume the rest of the function is correct as it doesn't use timeStep.

Thanks.

Original issue reported on code.google.com by d...@doc.ic.ac.uk on 8 Aug 2008 at 6:37

GoogleCodeExporter commented 9 years ago 
Fix also tested by this person

http://www.bulletphysics.com/Bullet/phpBB3/viewtopic.php?f=9&t=2507&p=9920#p9920

Original comment by d...@doc.ic.ac.uk on 21 Aug 2008 at 5:08

GoogleCodeExporter commented 9 years ago 
Here is the essence of the patch:

-       m_linearVelocity *= GEN_clamped((btScalar(1.) - timeStep * 
m_linearDamping),
(btScalar)btScalar(0.0), (btScalar)btScalar(1.0));
-       m_angularVelocity *= GEN_clamped((btScalar(1.) - timeStep *
m_angularDamping), (btScalar)btScalar(0.0), (btScalar)btScalar(1.0));
+       m_linearVelocity *= btPow(btScalar(1)-m_linearDamping, timeStep);
+       m_angularVelocity *= btPow(btScalar(1)-m_angularDamping, timeStep);

Original comment by d...@doc.ic.ac.uk on 28 Aug 2008 at 11:20

GoogleCodeExporter commented 9 years ago 
sorry for the line breaks, next time I'll attach a proper patch...

Original comment by d...@doc.ic.ac.uk on 28 Aug 2008 at 11:38

GoogleCodeExporter commented 9 years ago 
All those damping models are approximations, with a trade-off of 
accuracy/realism 
versus performance.

powf likely degrades performance, so we better allow the user to choose between 
the 
old and new method, and document both approximations.

Original comment by erwin.coumans on 10 Sep 2008 at 7:13

GoogleCodeExporter commented 9 years ago

Original comment by erwin.coumans on 14 Sep 2008 at 6:29

GoogleCodeExporter commented 9 years ago 

It has been fixed and committed in the trunk:
http://code.google.com/p/bullet/source/detail?r=1343

Thanks for the patch/feedback!
Erwin

Original comment by erwin.coumans on 30 Sep 2008 at 12:38