Change Node*D::reparent to set its new transform before reparenting
#113287
+20
−10
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #113058.
The current implementation of
Node*D::reparent, when called withkeep_global_transformat its default oftrue, does the following:NOTIFICATION_EXIT_WORLD.NOTIFICATION_ENTER_WORLD, but with a potentially new (incorrect) global transform, since the local transform is what's actually preserved in the transition.This creates problems for nodes where the value of the global transform is important at the time of
NOTIFICATION_ENTER_WORLD, like for example kinematic physics bodies, such asCharacterBody3D1.This PR "fixes" that by changing
Node*D::reparentto instead set the new local transform before we doremove_child/add_child, so that the node already has the correct global transform by the timeadd_childis called.This of course means that now we won't have the correct local/global transform when
NOTIFICATION_EXIT_WORLD/NOTIFICATION_EXIT_TREEis emitted instead, so arguably just trades one bug for another, but I figured this might prompt some discussion for better a solution.The proper solution to this (at least in my mind) would be to instead restore the previous global transform inbetween these two lines, as opposed to before/after them:
godot/scene/main/node.cpp
Lines 2077 to 2078 in 7ed0b61
... but I struggle to come up with a clean way of doing so.
Footnotes
This seems to have "worked" with Godot Physics due to what to me looks like a bug in that implementation, but which is not present in the Jolt Physics implementation. ↩