RigidBody2D overwrites most transform changes done in _integrate_forces

[mihe]

Godot version

4.2-beta [d5217b6]

System information

Windows 11 (10.0.22621)

Issue description

This is a formal issue for the partial regression caused by #84799, which in turn was meant to solve regression #83412 caused by #79977.

The summary of this issue is pretty much what the title says: Most transform changes done to a RigidBody2D from _integrate_forces will have no effect due to them being overwritten by the RigidBody2D::_sync_body_state call that happens after _integrate_forces has been called.

The one notable exception is if you first read some part of the global transform before modifying it within _integrate_forces, in which case the global_invalid flag on the RigidBody2D will be cleared and the transform change notification will actually be queued up and sent out by the eventual force_update_transform from RigidBody2D::_body_state_changed.

See #84856 for a more in-depth breakdown of the problem.

Steps to reproduce

• Open the main scene of the MRP
• Take note of the intended behavior described on the labels
• Run the main scene
• Note the fact that they don't do as described

Minimal reproduction project

overwritten_transforms.zip

[mihe]

One fairly simple and risk-free fix, in terms of breaking functionality, would be to simply remove these lines here:

godot/scene/main/canvas_item.cpp

Lines 888 to 896 in d5217b6

	/* This check exists to avoid re-propagating the transform
	* notification down the tree on dirty nodes. It provides
	* optimization by avoiding redundancy (nodes are dirty, will get the
	* notification anyway).
	*/
	if (/*p_node->xform_change.in_list() &&*/ p_node->_is_global_invalid()) {
	return; //nothing to do
	}

That would prevent the global_invalid flag from blocking notifications that it has no business blocking, as talked about in #84856, while also technically making things more consistent with the 3D equivalent to this method, which would be Node3D::_propagate_transform_changed.

The only real downside, given that this is purely an optimization, is that you run the risk of introducing a potentially significant performance regression. It's hard to judge how severe exactly, but with a deep enough CanvasItem tree attached to the node you're messing with it could be significant and pervasive I suppose.

The only other solution I can think of is the alternative solution mentioned here, where you instead replace the force_update_transform introduced by #84799 with this stuff:

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

if (new_transform != old_transform) {
	PhysicsServer3D::get_singleton()->body_set_state(get_rid(), PhysicsServer3D::BODY_STATE_TRANSFORM, new_transform);
}

This would however also introduce a performance regression, since you're now adding the cost of updating the global transform for every single RigidBody2D that implements _integrate_forces, potentially twice even, but seeing as how this is still localized to only RigidBody2D that implement _integrate_forces it would probably be the lesser evil compared to the CanvasItem change mentioned above.

[mihe]

I've created a PR for the second solution mentioned in my previous comment, as #84924.

I don't love it, but I'm running out of ideas.