Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix transform changes in _integrate_forces being overwritten #84799

Merged
merged 1 commit into from Nov 12, 2023

Conversation

mihe
Copy link
Contributor

@mihe mihe commented Nov 12, 2023

Fixes #83412.

This adds a force_update_transform after every _integrate_forces call, in order to flush any pending NOTIFICATION_TRANSFORM_CHANGED that might be needed and thereby push those transform changes to the physics server before we request it back from the physics server again at the next _sync_body_state that happens right after.

Note that this results in a new NOTIFICATION_TRANSFORM_CHANGED notification that wasn't being emitted before, even when the user was explicitly modifying the transform. I would perhaps argue that the lack of this notification was also a bug and something that should have been emitted, seeing as how transform changes in any other method would emit that notification.

@mihe
Copy link
Contributor Author

mihe commented Nov 12, 2023

There is an alternative implementation to this as well, which is to not use force_update_transform and instead just push the new transform to the physics server explicitly through PhysicsServer*D::body_set_state.

This has the benefit of not emitting NOTIFICATION_TRANSFORM_CHANGED right away, as it would instead be queued up and flushed by the SceneTree like normal. It would then also not be emitted as part of the physics tick, which could otherwise potentially happen multiple times per drawn frame.

The drawback of this is that it would likely need to look something like this:

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);
}

... meaning every single RigidBody3D that implements _integrate_forces would now be paying the cost for potentially multiple updates to global_transform regardless of whether or not the transform is modified within _integrate_forces.

force_update_transform on the other hand is just a nullptr check in the case where no changes to the node transform have been made. Unfortunately the SelfList that Node3D uses for this check is private, and strikes me as an implementation detail anyway, otherwise it could've potentially been used by the physics bodies to quickly check whether the transform was dirtied or not.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

scene/2d/physics_body_2d.cpp Outdated Show resolved Hide resolved
scene/3d/physics_body_3d.cpp Outdated Show resolved Hide resolved
scene/3d/physics_body_3d.cpp Outdated Show resolved Hide resolved
@mihe mihe marked this pull request as draft November 12, 2023 18:48
@mihe mihe marked this pull request as ready for review November 12, 2023 19:36
@mihe
Copy link
Contributor Author

mihe commented Nov 12, 2023

It seems like this PR has unearthed another bug related to CanvasItem::block_transform_notify, that manifests in ways where if you set the transform to be a constant value in _integrate_forces, like this:

extends RigidBody2D

func _integrate_forces(_state):
	rotation_degrees = 45

... you will never actually see that transformation happen until something changes the transform somewhere else. But if you do something like this instead:

extends RigidBody2D

func _integrate_forces(state):
	rotation_degrees += 45 * state.step

... then everything is fine.

Interestingly this last snippet doesn't seem to have worked even before #79977, so there's been some kind of give and take here at least.

I'll see about making a separate PR to fix this new issue with CanvasItem::block_transform_notify.

@akien-mga akien-mga merged commit 6415006 into godotengine:master Nov 12, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

look_at() doesn't work with RigidBody3D
3 participants