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

Prevent some internal nodes being duplicated in Controls #88114

Merged
merged 1 commit into from
Feb 10, 2025

Conversation

AThousandShips
Copy link
Member

Caused by duplication of on-demand created children, for example:

var cpb := ColorPickerButton.new()
print(cpb.get_child_count(true))
print(cpb.get_popup())
print(cpb.get_child_count(true))

var cpb2 := cpb.duplicate()
print(cpb2.get_child_count(true))
print(cpb2.get_popup())
print(cpb2.get_child_count(true))

Will print:

0
@PopupPanel@...:<PopupPanel#...>
1
1
@PopupPanel@...:<PopupPanel#...>
2

This fixes these

There are probably some other potential cases but unsure how to apply each, but keeping these specific menu cases for now

@AThousandShips AThousandShips added bug topic:gui cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels Feb 8, 2024
@AThousandShips AThousandShips added this to the 4.3 milestone Feb 8, 2024
@AThousandShips AThousandShips requested a review from a team as a code owner February 8, 2024 20:10
@AThousandShips AThousandShips changed the title Prevent internal nodes from being duplicated Prevent some internal nodes from being duplicated Feb 8, 2024
@KoBeWi
Copy link
Member

KoBeWi commented Feb 8, 2024

I think #84824 fixes the same issue, no?

@AThousandShips
Copy link
Member Author

AThousandShips commented Feb 8, 2024

It does not, I tried it with at least some cases, will confirm but that is for instanced nodes I believe

Edit: It does not solve this issue

@Rindbee
Copy link
Contributor

Rindbee commented Feb 9, 2024

#84824 solves the case when copying sub-scenes. The case here is the case of internal nodes in the same scene.

Internal nodes are usually added when the parent node is initialized or under certain conditions. They can be treated as part of the parent node.

godot/scene/main/node.cpp

Lines 2599 to 2607 in 94dbf69

for (int i = 0; i < get_child_count(); i++) {
if (get_child(i)->data.parent_owned) {
continue;
}
if (instantiated && get_child(i)->data.owner == this) {
continue; //part of instance
}
Node *dup = get_child(i)->_duplicate(p_flags, r_duplimap);

-       for (int i = 0; i < get_child_count(); i++) {
+       for (int i = 0; i < get_child_count(false); i++) {

It may be better to directly exclude internal nodes when copying.

@AThousandShips
Copy link
Member Author

Some internal nodes might not be automatically added though and might be data independent, unsure, but it'd also break compat to change it for internal nodes IMO as scripts may use it

Note that if they're added in the constructor they are already automatically excluded

@Rindbee
Copy link
Contributor

Rindbee commented Feb 9, 2024

Well, you are right, the script can manage the entire tree, possibly adding additional cases.

@AThousandShips AThousandShips changed the title Prevent some internal nodes from being duplicated Prevent some internal nodes being duplicated Feb 13, 2024
@akien-mga akien-mga requested a review from KoBeWi March 6, 2024 12:33
KoBeWi
KoBeWi previously approved these changes Mar 12, 2024
@AThousandShips
Copy link
Member Author

AThousandShips commented Mar 13, 2024

This is still relevant for 4.2 and would be best to get in first IMO, #89442 isn't necessarily cherry-pickable, but I can always retarget it for 4.2

@AThousandShips
Copy link
Member Author

AThousandShips commented Feb 10, 2025

Haven't looked for any new cases of this since this was opened, there might be some cases will try and search for them

Edit: Seems at least all relevant cases of internal nodes being handled

@akien-mga akien-mga changed the title Prevent some internal nodes being duplicated Prevent some internal nodes being duplicated in Controls Feb 10, 2025
@akien-mga akien-mga added the cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release label Feb 10, 2025
@akien-mga akien-mga modified the milestones: 4.x, 4.4 Feb 10, 2025
@akien-mga
Copy link
Member

Haven't looked for any new cases of this since this was opened, there might be some cases will try and search for them

There's at least #92880 which would be fixed by #89442, unless the internal node in GraphEdit that triggers it is one of those covered here.

@AThousandShips
Copy link
Member Author

Unsure about that case, there's no unhandled internal children there, and the connections_layer node is added in the constructor so it isn't duplicated, so don't think it is covered by this specific case

@Repiteo Repiteo merged commit 6a3bf28 into godotengine:master Feb 10, 2025
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Feb 10, 2025

Thanks!

@AThousandShips AThousandShips deleted the dupli_fix branch February 10, 2025 18:29
@AThousandShips
Copy link
Member Author

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release topic:gui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants