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

RemoteTransform2D may fail to update AnimatableBody2D's position or rotation #75486

Closed
Rindbee opened this issue Mar 30, 2023 · 1 comment · Fixed by #75487
Closed

RemoteTransform2D may fail to update AnimatableBody2D's position or rotation #75486

Rindbee opened this issue Mar 30, 2023 · 1 comment · Fixed by #75487

Comments

@Rindbee
Copy link
Contributor

Rindbee commented Mar 30, 2023

Godot version

v4.1.dev.custom_build [c29866d], v4.0.1.stable.official [cacf499]

System information

Linux Mint 21.1, Vulkan API 1.3.205, NVIDIA GeForce GTX 1050 Ti

Issue description

When update_position, update_rotation, update_scale are not enabled at the same time, for an AnimatableBody2D with sync_to_physics enabled, RemoteTransform2D may not be able to update the position or rotation of the AnimatableBody2D.

0

This is related to the following code:

n->set_global_transform(our_trans);
if (update_remote_scale) {
n->set_scale(get_global_scale());
} else {
n->set_scale(n_scale);
}

For an AnimatableBody2D with sync_to_physics enabled, after setting the transform, and then calling a method (here, it's set_scale()) that may issue a NOTIFICATION_LOCAL_TRANSFORM_CHANGED notification, the following code will be called twice.

case NOTIFICATION_LOCAL_TRANSFORM_CHANGED: {
// Used by sync to physics, send the new transform to the physics...
Transform2D new_transform = get_global_transform();
PhysicsServer2D::get_singleton()->body_set_state(get_rid(), PhysicsServer2D::BODY_STATE_TRANSFORM, new_transform);
// ... but then revert changes.
set_notify_local_transform(false);
set_global_transform(last_valid_transform);
set_notify_local_transform(true);
} break;

In the second call, line 302 sets the physics state with an outdated value. It may cause jitter or even failure.

It will become a duplicate of #58269 when use_global_coordinates is disabled. It may be appropriate to use NOTIFICATION_TRANSFORM_CHANGED instead of NOTIFICATION_LOCAL_TRANSFORM_CHANGED in AnimatableBody2D.

2

Steps to reproduce

  1. Download the MRP project and import it;
  2. Run the scene and observe;
  3. You can use other combinations, then run the scene and observe.

0

Minimal reproduction project

fail-to-update.zip

@Rindbee
Copy link
Contributor Author

Rindbee commented Mar 30, 2023

Disabling use_global_coordinates in RemoteTransform2D will cause the following code not to be called, not sure why. 🤔

case NOTIFICATION_TRANSFORM_CHANGED: {
if (!is_inside_tree()) {
break;
}
if (cache.is_valid()) {
_update_remote();
}
} break;

Can be related to the following code.

if (/*p_node->xform_change.in_list() &&*/ p_node->global_invalid) {
return; //nothing to do
}
p_node->global_invalid = true;
if (p_node->notify_transform && !p_node->xform_change.in_list()) {
if (!p_node->block_transform_notify) {
if (p_node->is_inside_tree()) {
get_tree()->xform_change_list.add(&p_node->xform_change);
}
}
}

global_invalid = false;

global_invalid will be reset to false only after get_global_transform() is called. This may not be appropriate.

The condition on line 787 is commented out, is there any reason? Adding this condition makes it more complete.

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

Successfully merging a pull request may close this issue.

2 participants