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 duplicating sub-scene may get two copies of internal node #84824

Merged
merged 1 commit into from
Feb 9, 2024

Conversation

Rindbee
Copy link
Contributor

@Rindbee Rindbee commented Nov 13, 2023

Previously, internal node might be mistaken for hidden_root and be duplicated again. Exclude those internal nodes to avoid this case.

Fix #84803.
Bugsquad edit: Fixes #74406.

@Rindbee Rindbee requested a review from a team as a code owner November 13, 2023 02:01
@AThousandShips AThousandShips added this to the 4.3 milestone Nov 13, 2023
@akien-mga akien-mga added the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Nov 24, 2023
@YuriSizov YuriSizov requested a review from KoBeWi December 4, 2023 13:50
@KoBeWi
Copy link
Member

KoBeWi commented Dec 12, 2023

Shouldn't this be determined by owner instead of internal children? I don't really get why the internal nodes even get duplicated.

Then again, I doubt anyone would be intentionally owning the internal children, so the fix is probably fine anyway.

@Rindbee Rindbee force-pushed the fix-double-internal-node branch from b152e41 to 730458a Compare December 13, 2023 00:18
@Rindbee
Copy link
Contributor Author

Rindbee commented Dec 13, 2023

The owner of internal nodes is usually not set, and the following code does not check if the owner is not set.

godot/scene/main/node.cpp

Lines 2516 to 2518 in 41365c6

if (descendant->get_parent() && descendant->get_parent() != this && descendant->data.owner != descendant->get_parent()) {
hidden_roots.push_back(descendant);
}

Nodes added through tool scripts may not have an owner set. 🤔

@KoBeWi
Copy link
Member

KoBeWi commented Dec 20, 2023

What I mean is that internal mode's only purpose is to "hide" nodes from the user (get_children() etc. ignores them by default). Duplicating instances should only be affected by node's owners, not their internal mode.

Nodes added through tool scripts may not have an owner set.

Then they don't appear in the scene tree. I think it's more expected (or at least consistent) if they don't get duplicated.

Previously, internal node might be mistaken for `hidden_root` and be duplicated again.
Exclude those internal nodes to avoid this case, unless the owner is set intentionally.
@Rindbee Rindbee force-pushed the fix-double-internal-node branch from 730458a to f19c419 Compare December 20, 2023 10:31
@akien-mga
Copy link
Member

CC @KoBeWi - does this look good to go now?

@akien-mga akien-mga merged commit 58a8eb8 into godotengine:master Feb 9, 2024
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@akien-mga
Copy link
Member

Cherry-picked for 4.2.2.

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