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 recursive Node.find_children #75459

Merged
merged 1 commit into from
Jun 19, 2023

Conversation

kleonc
Copy link
Member

@kleonc kleonc commented Mar 29, 2023

In Node.find_children method when the name of a child node was not matching the passed in non-empty pattern it resulted in not reaching if (recursive) block because of the continue to the next iteration:

godot/scene/main/node.cpp

Lines 1417 to 1419 in 23394be

if (!p_pattern.is_empty()) {
if (!cptr[i]->data.name.operator String().match(p_pattern)) {
continue;

This PR fixes this logic to always get to the if (recursive) block so no children will be unintentionally skipped.

Fixes #75446. Fixes #63757.

Supersedes #63785.

@kleonc kleonc added this to the 4.1 milestone Mar 29, 2023
@kleonc kleonc requested a review from a team as a code owner March 29, 2023 12:58
@hawkerm
Copy link

hawkerm commented Apr 9, 2023

@kleonc does this PR need anything else like a test or something before it can be reviewed? Anything I can do to help?

@kleonc
Copy link
Member Author

kleonc commented Apr 9, 2023

@kleonc does this PR need anything else like a test or something before it can be reviewed? Anything I can do to help?

It can be reviewed as is (feel free to do so). But I guess I could add some test case with nested nodes for find_children, won't hurt. 🙃

@kleonc kleonc requested a review from a team as a code owner April 9, 2023 09:00
@Maran23
Copy link
Contributor

Maran23 commented Apr 17, 2023

And this may supersedes #63785 and therefore fixes issue #63757 as well?

@kleonc
Copy link
Member Author

kleonc commented Apr 17, 2023

And this may supersedes #63785 and therefore fixes issue #63757 as well?

@Maran23 Yeah, it should fix #63757 as well. But it doesn't change anything for the nodes without an owner, gonna fix this in here too. Actually, reading the in-docs description for the owned parameter:

If [param owned] is [code]true[/code], this method only finds nodes who have an assigned [member Node.owner]. This is especially important for scenes instantiated through a script, because those scenes don't have an owner.

I think ignoring whole subtrees for the nodes without an owner is intentional. Otherwise (by not ignoring the whole subtree like in #63785) the children of the dynamically instantiated scenes would still be checked, only the root nodes of such scenes would be ignored. Changing this behavior seems like a compat breaking change to me, hence I'll leave it as is. Could always be tweaked in another PR (if it's actually not intended).

@akien-mga akien-mga merged commit cce100a into godotengine:master Jun 19, 2023
@akien-mga
Copy link
Member

Thanks!

@kleonc kleonc deleted the node-fix-find-children branch June 19, 2023 20:54
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.

FindChildren not finding nested children find_children returns [] if match string is given
4 participants