Add state sync after call to _integrate_forces in _body_state_changed

[Owl-A]

in issue #79835 , the root cause is that _physics_process is called after _integrate_forces (called from RigidBody3D::_body_state_changed) while _integrate_forces modifies GodotBody3D's linear_velocity, _physics_process reads from now obsolete RigidBody3D::linear_velocity and when we update it using RigidBody3D::set_linear_velocity which also updates GodotBody3D::linear_velocity.

This can be prevented by updating RigidBody3D's mutable state as soon as we return from _integrate_forces, which is what I have added.

• Bugsquad edit, fixes: [GodotPhysics] Unexpected behavior while interacting with linear_velocity and state.linear_velocity. #79835

[mihe]

It might be a good idea to move all this to a new private method instead, like a void _sync_body_state(PhysicsDirectBodyState3D *p_state), so that you can't accidentally miss one or the other when adding to it. Same goes for the other classes as well.

[mihe]

I realize you copied the code from above the _integrate_forces call, but this looks odd to me. I don't see why this shouldn't synchronize as much mutable state as possible both before and after _integrate_forces, meaning velocities, sleep state and transform.

As it is right now, it looks like PhysicalBone3D::_integrate_forces doesn't actually get an up-to-date transform like it should be getting.

[Owl-A]

I was also confused as to why the transform and sleeping were not synced here, should I sync them too?

[Owl-A]

Also, PhysicalBone3D doesn't seem to have sleeping or inverse_inertia_tensor, we need to only sync transform or am I missing something?

[mihe]

You're right, there is no sleeping (or inverse_inertia_tensor) in PhysicalBone3D, despite there being a can_sleep, so that simplifies things a bit I guess, since it's only the cached state we're interested in syncing.

I can't see a reason why the transform shouldn't be synced, and the set_ignore_transform_notification "scope" should probably span across the _integrate_forces call as well, similar to how it's done in RigidBody3D.

[Owl-A]

Cool, I'll do that

[mihe]

I can't seem to add a comment to these lines, since they're not in proximity of the actual changes, but these lines in PhysicalBone3D::_body_state_changed are now redundant after your latest changes and should be removed:

https://github.com/godotengine/godot/blob/1dbfd2ac5c154c300d44adc9959a895ec28aa75e/scene/3d/physics_body_3d.cpp#L2946-L2948

[mihe]

If I can nitpick one last thing it would be to move this _on_transform_changed() in PhysicalBone3D::_body_state_changed up to right below set_ignore_transform_notification(false), similar to how it was before, since they go hand-in-hand.

[mihe]

I just gave the ragdoll from GDQuest's godot-4-new-features repo a try, and I can confirm that the transform for PhysicalBone3D was indeed stale in _integrate_forces before these changes, and is now as expected.

So this all looks good to me.

(I assume someone in a more official capacity will need to also look over these changes though.)

[rburing]

This makes sense to me, and fixes the bug.

The behavior change should be mentioned in the release notes. To get the old behavior back (which probably nobody really wants) you would have to do your own caching.

Note that the force integration callback is currently only called using

godot/scene/3d/physics_body_3d.cpp

Line 503 in dcd187d

GDVIRTUAL_CALL(_integrate_forces, p_state);

in the scene/ code here, and it doesn't go through the physics server, i.e. body_set_force_integration_callback is currently never called from scene/ code, and even if it were, the callback is never used (at least in Godot Physics). I documented this when I noticed it, but I think this should be fixed as well (in another PR), so that bodies created in the physics server can have a force integration callback too.

[YuriSizov]

The behavior change should be mentioned in the release notes. To get the old behavior back (which probably nobody really wants) you would have to do your own caching.

@rburing Could you quickly summarize what kind of behavior is affected?

[mihe]

and even if it were, the callback is never used (at least in Godot Physics)

@rburing Not to derail this PR too much, but this is not correct. I can't speak for 2D, but the code looks very similar to the 3D one, where the callback that's set using body_set_force_integration_callback is invoked as part of GodotBody3D::call_queries.

The Godot Physics implementation does however only invoke the callback if you provide a piece of userdata along with the callback, as seen in the link there, which is likely a bug and might be why you thought it wasn't used.

The documentation for body_set_force_integration_callback also incorrectly states that this callback is conditionally invoked based on body_set_omit_force_integration, which is also not the case. I'm not sure what the original intent was, but I would argue they shouldn't affect eachother, since RigidBody3D._integrate_forces is called even when not using body_set_omit_force_integration (a.k.a. RigidBody3D.custom_integrator).

However, your overall point about these custom integration callbacks being left out of the synchronization that this PR improves upon is a good one. I'm not sure how you would fix this to be honest, since the custom integration callback is invoked before the body_state_callback, which is what calls _body_state_changed, meaning you'll always end up with stale cached state in any such custom integration callback.

[YuriSizov]

Thanks!

[rburing]

@YuriSizov

Could you quickly summarize what kind of behavior is affected?

A physics body node has state (of the corresponding body in the physics server) and properties (on the node). The order of processing in one physics tick used to be: (1) the equivalent of _sync_body_state, (2) _integrate_forces, (3) _physics_process. So when _integrate_forces changed the body's state on tick number n, that state would be copied to the body node's properties by (the equivalent of) _sync_body_state on tick n+1, too late to be seen in the _physics_process call on tick n. Moreover, incremental updates to properties like linear_velocity += ... (i.e. depending on the "current" value) in _physics_process would be using a stale value, causing bugs like the one this PR fixes. This PR adds another _sync_body_state after (2) _integrate_forces, so that the body node's properties are up-to-date in _physics_process on tick n, even if the state was modified by _integrate_forces.

So it affects the behavior of physics body scripts overriding both _physics_process and _integrate_forces.

---

By the way, writing the above explanation makes me think _sync_body_state should just only be after _integrate_forces and not before it, since inside that method you should just use the state parameter and not the node's properties.

---

@mihe Sorry, I got mixed up when writing that doc, I'll fix it (plus the other documentation errors you mentioned). The callback that can be set using body_set_force_integration_callback does get called as you show, but body_set_force_integration_callback itself never gets called by scene/ code. I would expect the rigid body scene/ code to call it with a callback that calls _integrate_forces. But maybe the current timing is better? I don't know.

meaning you'll always end up with stale cached state in any such custom integration callback

The custom integration callback takes the physics server body's state as input, so I don't think there's an issue with stale state there.

[mihe]

The order of processing in one physics tick used to be: (1) _physics_process, (2) the equivalent of _sync_body_state, (3) _integrate_forces

Maybe I'm misunderstanding your frame numbering, but _integrate_forces (and thus also _sync_body_state) comes before _physics_process, as seen here, where flush_queries is what ends up calling _body_state_changed and thus _integrate_forces.

So a frame used to be:

1. Call any custom force integration callback
2. _body_state_changed
   • Update cached state in RigidBody3D
   • _integrate_forces
3. _physics_process
4. Step the simulation
5. Maybe go back to 1 a couple of times
6. _process and whatever else

Meaning if _integrate_forces made any changes through PhysicsDirectBodyState3D (thereby bypassing RigidBody3D) then the cached state in RigidBody3D would be stale in _physics_process.

Now instead a frame is:

1. Call any custom force integration callback
2. _body_state_changed
   • Update cached state in RigidBody3D
   • _integrate_forces
   • Update cached state in RigidBody3D
3. _physics_process
4. Step the simulation
5. Maybe go back to 1 a couple of times
6. _process and whatever else

I would expect the rigid body scene/ code to call it with a callback that calls _integrate_forces. But maybe the current timing is better? I don't know.

I guess if one considers body_set_force_integration_callback to be a scene-independent way of achieving the equivalent of _integrate_forces then it should probably remain the responsibility of the physics server to call it.

The custom integration callback takes the physics server body's state as input, so I don't think there's an issue with stale state there.

I guess the scenario I had in mind was if somebody used this together with a RigidBody3D, and passed the body as the callback userdata, in which case that body would not have had the chance to update its cached state yet. I've seen someone mention this use-case before, where they had some manager-type object that dealt with some collection of bodies, and for whatever reason wanted to do the force-integration from the manager, and thus reached for body_set_force_integration_callback. But I suppose if you start mixing scene stuff with server stuff then you'll just have to accept whatever sharp edges there might be.