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

Don't access Node3D/Node2D/Control global transform in reparent unless needed #89003

Merged

Conversation

akien-mga
Copy link
Member

Fixes #89002.

Marked as co-authored by @elementbound who found what the problem was. The solution needs to be slightly different to the one suggested in #89002, as the global transform needs to be fetched before reparenting, otherwise it won't work.

@akien-mga akien-mga added bug topic:3d cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels Feb 29, 2024
@akien-mga akien-mga added this to the 4.3 milestone Feb 29, 2024
@akien-mga akien-mga requested a review from a team as a code owner February 29, 2024 11:49
@Sauermann
Copy link
Contributor

Changes are looking good.

An alternative solution for the referenced bug report would be to make Node3D::get_global_transform() accessible while not in the scene tree, just like it is possible with CanvasItem::get_global_transform(), so that 2D and 3D behave in the same way.

godot/scene/3d/node_3d.cpp

Lines 345 to 346 in df78c06

Transform3D Node3D::get_global_transform() const {
ERR_FAIL_COND_V(!is_inside_tree(), Transform3D());

@akien-mga
Copy link
Member Author

An alternative solution for the referenced bug report would be to make Node3D::get_global_transform() accessible while not in the scene tree, just like it is possible with CanvasItem::get_global_transform(), so that 2D and 3D behave in the same way.

godot/scene/3d/node_3d.cpp

Lines 345 to 346 in df78c06

Transform3D Node3D::get_global_transform() const {
ERR_FAIL_COND_V(!is_inside_tree(), Transform3D());

This has significant implications though, see #70443 and #74659.

But more importantly, there is no reason to request the global transform if we're not going to use it when p_keep_global_transform = false, so this is the correct fix for #89002.

Copy link
Contributor

@Sauermann Sauermann left a comment

Choose a reason for hiding this comment

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

Thanks for the links to the other PRs - I was not aware of them.
I agree that this solution is correct .

Perhaps the same change should also be done for Node2D

godot/scene/2d/node_2d.cpp

Lines 144 to 151 in df78c06

void Node2D::reparent(Node *p_parent, bool p_keep_global_transform) {
ERR_THREAD_GUARD;
Transform2D temp = get_global_transform();
Node::reparent(p_parent);
if (p_keep_global_transform) {
set_global_transform(temp);
}
}

Edit: ... and Control::reparent

…ess needed

Fixes godotengine#89002.

Co-authored-by: Tamás Gálffy <ezittgtx@gmail.com>
@akien-mga akien-mga force-pushed the node3d-reparent-no-global-transform branch from 005b2fb to 810a0db Compare February 29, 2024 13:07
@akien-mga akien-mga requested review from a team as code owners February 29, 2024 13:07
@akien-mga akien-mga changed the title Don't access Node3D global transform in reparent unless enabled Don't access Node3D/Node2D/Control global transform in reparent unless needed Feb 29, 2024
@akien-mga
Copy link
Member Author

Perhaps the same change should also be done for Node2D

Edit: ... and Control::reparent

Good catch! Done.

@akien-mga akien-mga merged commit 5c38a9f into godotengine:master Feb 29, 2024
16 checks passed
@akien-mga akien-mga deleted the node3d-reparent-no-global-transform branch February 29, 2024 14:19
@akien-mga
Copy link
Member Author

Cherry-picked for 4.2.2.

@akien-mga akien-mga removed the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Mar 11, 2024
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.

Node3D::reparent accesses global transform, even if it's not needed
2 participants