Kinematic friction broken after recent changes to getOrInitSolverBody.

[AccessViolationEnjoyer]

Friction is no longer applied when a dynamic rigid body is resting on a moving kinematic body. I believe that this is caused by the recent change to btSequentialImpulseConstraintSolver::getOrInitSolverBody. Note that this is broken with or without BT_THREADSAFE set. Using the old version of the function fixes the problem, although this is obviously not threadsafe.

[erwincoumans]

Do you have a minimal patch for btSequentialImpulseConstraintSolver::getOrInitSolverBody that reverts the behavior (even if it breaks the multithreaded solver)?

[erwincoumans]

and we should add some unit test, so it doesn't regress next time.

[AccessViolationEnjoyer]

Kinematic bodies are allowed to have CF_STATIC_OBJECT set right?
If so, the kinematic conditional will never be reached. To clarify, getOrInitSolverBody contains the following structure:

if (!body.isStaticOrKinematicObject()) {
    // dynamic body.
}
else if (body.isStaticObject())
{
    // static body.
}
else
{
    // kinematic body.
}

Suggest reordering so that we check for kinematic body before static body (or similar):

if (!body.isStaticOrKinematicObject()) {
    // dynamic body.
}
else if (body.isKinematicObject())
{
    // kinematic body.
}
else
{
    // static body.
}

[erwincoumans]

Why do you need to set the CF_STATIC_OBJECT for kinematic objects? Isn't using CF_KINEMATIC_OBJECT sufficient?

Aside from that, it looks OK to check for kinematic first.

[AccessViolationEnjoyer]

If you set a rigid body's mass to zero then CF_STATIC_OBJECT is set. To be honest, I don't know whether it is necessary or not. I don't think that it is documented anywhere whether or not CF_STATIC_OBJECT should be set when CF_KINEMATIC_OBJECT is set. I have seen users set both of these flags when creating a kinematic bodies.

[erwincoumans]

When BT_THREADSAFE is not set, the old logic seems to check for kinematic object first, right?

https://github.com/bulletphysics/bullet3/blob/master/src/BulletDynamics/ConstraintSolver/btSequentialImpulseConstraintSolver.cpp#L778

How is that code path still broken?

[erwincoumans]

The current Bullet version doesn't seem broken by default (no BT_THREADSAFE defined).
See attached modified BasicDemo with a kinematic animated floor (linear velocity), use premake to create your build files.
BasicExample.zip

[lunkhound]

In the no-BT_THREADSAFE case, the code for that function should be exactly what it was before I changed it. So I'd be very surprised if that case is broken.

In the threadsafe case, I agree that it makes sense to check for kinematic before static. I didn't realise that kinematic objects could have the static flag set.

Shall I create a PR for that?

[erwincoumans]

Yes, please create a PR for that. And in my test, the no-BT_THREADSAFE is working just fine (see my previous reply above here), so no need to worry about that.

[lunkhound]

See PR #894
I modified the MultiThreadedDemo example to reproduce this bug. After verifying the bug was reproduced, I applied the suggested fix and verified the bug no longer occuring.
Also I double checked that the no-BT_THREADSAFE case did not reproduce the bug.

[AccessViolationEnjoyer]

Yes, sorry about that. It does work with BT_THREADSAFE not set. I must have not unset this properly.