_integrate_forces / KinematicCollision3D bug

[prominentdetail]

Godot version

4.2 rc 6 and newer

System information

windows 10

Issue description

I've attached a minimum reproduction project. The KinematicCollision3D seems to not work the same after 4.2 rc 5.. In rc 6 and newer version it is broken.

rc5:

rc6:

Steps to reproduce

Run example project in rc5 or older, compare to rc6 and newer.

Minimal reproduction project

RigidBody Character Bug.zip

[prominentdetail]

I narrowed the issue down to this recent change that causes the issue: https://github.com/godotengine/godot/commit/a3278c772eccedd035d2880cc33a11cab00fd0fc When I revert that change, it works fine.

[akien-mga]

CC @mihe

[mihe]

Yeah, I've spoken to @prominentdetail elsewhere already. I'm looking at it.

(Also, the mentions of "rc5" and "rc6" are meant to say "beta5" and "beta6" respectively.)

[mihe]

@prominentdetail Are you sure that a3278c772eccedd035d2880cc33a11cab00fd0fc is the offending commit? Reverting that commit shouldn't make any difference as far as 3D physics is concerned, since the force_update_transform() that was replaced in that commit had largely the same behavior anyway. It was also merged between 4.2-beta6 and 4.2-rc1, which wouldn't line up with you seeing a difference between 4.2-beta5 and 4.2-beta6.

When bisecting I'm finding #84799 (aka 64150060f89677eaf11229813ae6c5cf8a873802) is where this behavior changed, which makes more sense, since that one was merged between 4.2-beta5 and 4.2-beta6.

I can't speak to whether the regression itself is a valid one just yet though. I need to take a closer look at why this would affect body_test_motion of all things.

[prominentdetail]

@mihe

I'm not sure- maybe I am not referencing the right commit, but when I commented out these lines from the latest version, it works:

void RigidBody3D::_body_state_changed(PhysicsDirectBodyState3D *p_state) {
    lock_callback();

    if (GDVIRTUAL_IS_OVERRIDDEN(_integrate_forces)) {
        _sync_body_state(p_state);

        //Transform3D old_transform = get_global_transform();
        GDVIRTUAL_CALL(_integrate_forces, p_state);
        //Transform3D new_transform = get_global_transform();

        /*if (new_transform != old_transform) {
            // Update the physics server with the new transform, to prevent it from being overwritten at the sync below.
            PhysicsServer3D::get_singleton()->body_set_state(get_rid(), PhysicsServer3D::BODY_STATE_TRANSFORM, new_transform);
        }*/
    }

    _sync_body_state(p_state);
    _on_transform_changed();

    if (contact_monitor) {
        contact_monitor->locked = true;

[mihe]

Ah, right. Removing the code is different from a revert. Then you effectively end up reverting #84799 (aka 64150060f89677eaf11229813ae6c5cf8a873802) in this case, and not #84924 (aka a3278c772eccedd035d2880cc33a11cab00fd0fc).

[mihe]

Just to give a minor update here: I've come to understand what the problem is here, and it breaks down to something quite fundamental about the intended purpose of _integrate_forces.

I've been pondering over this for a few days now, trying to think of how to best fix this, but I think I'll just end up submitting multiple PRs and start some kind of discussion about which might be the best approach.

As far as your specific problem is concerned, @prominentdetail, by commenting out that code you showed above you are effectively turning your "body will sink into floor when landing" workaround into a no-op, since any transform changes you make in _integrate_forces will be overwritten in that _sync_body_state below. So if commenting out that code works for you in your actual project, then you might as well just remove that workaround altogether, which should let you use Godot 4.2-stable as-is.

I also get the feeling that your workaround maybe wasn't doing what you hoped it would, as you (in 4.1) would end up setting the linear velocity for this physics step, but you'd actually end up setting the transform (in the physics server) on the next physics step, which is why your character keeps falling down in that repro (in 4.1) when it rightfully should come to a dead stop as soon as it touches the ground. You can achieve the proper effect (in 4.1) by changing state.transform instead of transform.

[prominentdetail]

@mihe , thanks- I can confirm that 4.2stable as-is works if I just remove the workaround in my main project. I think I remember that the workaround didn't really solve the issue entirely anyway, and I had to do some other things to make stuff work how I wanted it to. That is interesting to know about the difference between state.transform and transform. Is that still the case with 4.2? So if I understand- state.transform will change the current step whereas transform will change the next step?

[mihe]

Is that still the case with 4.2? So if I understand- state.transform will change the current step whereas transform will change the next step?

Yes and no. This is what unfortunately changed/broke in 4.2, and which requires further discussion on how to fix properly.

The physics engine in Godot is always one step ahead of the idle tick (and the rendering), and prior to #79977 the state parameter in _integrate_forces essentially served as the input for this next physics frame, without affecting the current physics frame. But now since #79977 we are pulling that state input back into the current physics frame and essentially mixing the two physics frames together, causing these weird problems.

So reverting #79977 and everything that followed from that is one solution to all this, but it re-introduces other weird and unintuitive behaviors, hence why some bigger discussion needs to be had.

I'll try to have it written up properly within the next couple of days.