Getting global property while Setting local property causes _integrate_physics to malfunction

[awardell]

Godot version

v4.1.2.stable.custom_build [399c9dc]

UPDATE: Issue still occurs in v4.2.dev.custom_build [42425ba]

System information

Godot v4.1.2.stable (399c9dc) - Windows 10.0.19045 - Vulkan (Compatibility) - NVIDIA GeForce GTX 1080 Ti (NVIDIA; 31.0.15.3623) - AMD Ryzen Threadripper 1950X 16-Core Processor (32 Threads)

Issue description

When moving a RigidyBody2D via _integrate_forces, a combination of accessing a global transform-related property while setting a local transform-related property will cause modifications to PhysicsDirectBodyState2D to be ineffectual.

Oddly, it has to be EXACTLY that arrangement of at least 1 global get and at least 1 local set for the bug to occur. The get and set can be done from anywhere, not just the script attached to the RigidBody2D. Global sets and local gets have no effect.

UPDATE: This issue may be related to #79835, but it does not seem to be solved by #79977

Steps to reproduce

Set up the tree hierarchy as shown in the reproduction. Create a script attached to the RigidBody2D with the code below.

Run the game, and observe that the character moves fine when rotation is being set.

Uncomment one of the variables accessing the global properties, and comment out setting the rotation.
Again, observe that the character moves fine when a global property is being read.

Have at least one global get and one local set both uncommented.
Observe when you attempt to move it, the object jitters and is locked in place.

Minimal reproduction project

┖╴Node2D
┖╴RigidBody2D
┠╴Sprite2D
┖╴CollisionShape2D

extends RigidBody2D

func _process(delta: float) -> void:

	#getting any of these
	#var x = global_position
	#var y = global_rotation
	#var z = global_scale

	#while setting either of these
	rotation = 0
	#scale = Vector2(1, 1)


func _integrate_forces(state: PhysicsDirectBodyState2D) -> void:

	var move = Input.get_axis("ui_left", "ui_right") * 256

	#interferes with both of these
	state.linear_velocity = Vector2(move, 0)
	#state.apply_central_force(Vector2(move, 0))

UPDATE: I have been asked for a repro project, so here one is:
ReproProject.zip

[yosoyfreeman]

Hi! Please, attach the minimum reproduction project so the people in charge can debug it.

Based on the text, this seems similar to #79835

[awardell]

Hi! Please, attach the minimum reproduction project so the people in charge can debug it.

Based on the text, this seems similar to #79835

Thank you. I've tested again in 4.2 latest and this specific bug is still occurring. I've updated the description accordingly and uploaded a demo project zip per your request.

[mihe]

This is not related to #79835, nor #79977, and doesn't necessarily have to involve _integrate_forces either.

Something as seemingly innocent as this will also break things in the same way:

extends RigidBody2D

func _process(_delta):
	global_position = global_position

(You'll probably want to leave gravity enabled though, so you can actually see it not working.)

Here's a somewhat simplified view of what a frame ends up looking like in this case:

• Fetch the physics server's position and assign it to RigidBody2D
• Call any implemented _integrate_forces
• Call any implemented _physics_process
• Step the physics server
  • Integrate velocities/gravity into physics server's position
• Process the scene tree
  • Call any implemented _process
    • Get global_position, which is the previous position, since we haven't synced after we stepped the physics server
    • Set global_position to this value (i.e. the one it already had)
    • Queue a NOTIFICATION_TRANSFORM_CHANGED
  • Flush notifications
    • RigidBody2D handles this notification
    • RigidBody2D sends its "changed" (but still previous) position to the physics server
    • The physics server overwrites its more current position with this previous one, which is bad
• Do rendering, audio, input, etc.
• Go back to the beginning

So the problematic part here seems to be that this NOTIFICATION_TRANSFORM_CHANGED gets sent out, and is the thing that differs from how this all normally behaves when you don't make use of any global transform components in _process.

More specifically, the divergence seems to happen here, which seemingly gets run whenever you tinker with any part of the transform, and this whole problem seems to hinge on that check to see whether the global transform is dirty/invalid or not. When you fetch any part of the global transform you will force an update of it, leading to it not being dirty/invalid any longer and thus allowed to go past that check and actually queue up this NOTIFICATION_TRANSFORM_CHANGED.

That's about as far as I made it for now. It's not immediately obvious to me what the fix would be. I'll have to take another look at this at some other point.

[mihe]

Although, it struck me now that maybe changing the transform of a physics body in _process (as opposed to _physics_process) is sort of "undefined behavior" to begin with? Simply switching out _process for _physics_process in any of the examples resolves the issue. The documentation sort of hints towards that as well I guess.

I'll defer to someone with more experience with the node processing.

[Occalepsus]

I've just looked at this issue and indeed I agree that modifying a physic body transform in _process instead of _physics_process is an undefined behaviour. I think this is not an issue, and I propose to add warnings if a user is trying to do that. (I have no idea how to do that though, but I would be pleased to implement it ;) )

[awardell]

@Occalepsus While I agree generally, I do believe my use case which incurred this behavior is valid. I had a Rigidbody with a locked rotation which occasionally I wanted to supply a new rotation. I had no desire for the physics server to have any control over rotation, and I assumed that locking rotation would ensure that. However, that is clearly not the case.

Another valid use case would be scaling the object. Object scale is not something the physics server affects, only something it is affected by. Scaling an object outside of _integrate_forces would seem to be reasonable use.

Moreover, these issues occur even if the transformation or the global property access occurs from a completely unrelated script. This can lead to this sort of undefined behavior occurring when the user is not even aware that they're "mixing" two different forms of interaction.

If this is an issue not worth fixing, then I agree that warnings should be added. I actually think it merits errors rather than warnings, as it is an active malfunction.

[Occalepsus]

@awardell I agree with you. I have never used Godot only for bug testing and fix, but from Unity I know that it is "forbidden" to change a Rigidbody's transform (you can do it, but this is bad), even for the rotation or the scale. Now I don't know how the physics team designed it, but as far as I saw in the code, it is not done for it even in Godot.
That was why I do not think the fix is worth, I'll leave that to the physics team.