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

Change find_node to find_nodes and Add an type parameter. #56032

Merged
merged 1 commit into from
Feb 17, 2022

Conversation

Diddykonga
Copy link
Contributor

@Diddykonga Diddykonga commented Dec 17, 2021

Changed find_node to find_nodes which now returns an TypedArray<Node>, and Added an type parameter to match against specific node types, which supports inheritance. find_nodes can be used with just mask or just type or both.

type: Can be any native, class_name, or resource_path type, similarly to the extends keyword. (supports inheritance)

3.x PR: #56085
Closes: godotengine/godot-proposals#3661
Example Project: FindNodesTest.zip

@Diddykonga Diddykonga requested a review from a team as a code owner December 17, 2021 18:13
@Calinou Calinou added this to the 4.0 milestone Dec 17, 2021
@Diddykonga Diddykonga requested a review from a team as a code owner December 17, 2021 18:34
@Diddykonga Diddykonga force-pushed the get_children_of_type branch 9 times, most recently from b08ca30 to c74fcf9 Compare December 19, 2021 19:27
@me2beats
Copy link

Should it be named get_children_of_class() instead?

@nathanfranke
Copy link
Contributor

Should it be named get_children_of_class() instead?

type makes more sense in my opinion.

@Diddykonga Diddykonga requested review from a team as code owners January 19, 2022 11:10
@Diddykonga
Copy link
Contributor Author

All suggested changes made, and rebased to current Master.

Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

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

Still needs a review from core team probably.

@lawnjelly
Copy link
Member

lawnjelly commented Jan 22, 2022

Still needs a review from core team probably.

Yes added PR meeting label as I think it needs broader review before going further, I think things like the naming and parameters are things that the reviewers like to bikeshed on and come to agreement.

On the naming at least I would be considering something like "find_nodes_of_type" rather than "get_children_of_type".

The difference between get and find seems already set by precedent in the get_node() and find_node() functions ("get" looking for a specific thing known in advance, and "find" implying a check on many nodes and potentially being more expensive .. this is often used as a convention for naming, with "get" used for accessors and "find" and "calculate" used for more expensive operations).

And children could be confusing if you are using the function recursively, which may end up being a common use case.

@akien-mga
Copy link
Member

We discussed this in a PR review meeting, the added feature seems fine, but we were wondering if it wouldn't be worth refactoring this API to unify the existing find_node and the new functionality to restrict only to a specific type (i.e. make it find_nodes that can take an optional type parameter).

@Zireael07
Copy link
Contributor

@akien-mga: Maybe as another PR? This is a feature that would be welcome sooner rather than later

@akien-mga
Copy link
Member

@akien-mga: Maybe as another PR? This is a feature that would be welcome sooner rather than later

Godot 4.0 is in alpha, and the further changes required mean breaking compatibility. There's no point merging a feature now in a pre-release branch to break compatibility right after.

@Zireael07
Copy link
Contributor

Ah, good point.

@Diddykonga Diddykonga changed the title Add Node.get_children_of_type() Change find_node to find_nodes and Add an type parameter. Feb 14, 2022
@Diddykonga
Copy link
Contributor Author

Diddykonga commented Feb 14, 2022

Suggested changes from the PR Review Meeting have been implemented. I will wait for confirmation that the changes are correct/desired, before pushing the same changes to the 3.x PR.

doc/classes/Node.xml Outdated Show resolved Hide resolved
doc/classes/Node.xml Outdated Show resolved Hide resolved
doc/classes/Node.xml Outdated Show resolved Hide resolved
Changed 'find_node' to 'find_nodes' which now returns an 'TypedArray<Node>', as well as Added a 'type' parameter to match against specific node types, which supports inheritance.
@akien-mga akien-mga merged commit b89b168 into godotengine:master Feb 17, 2022
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

@codecat
Copy link
Contributor

codecat commented Mar 7, 2022

I'm seeing the recursive option not working as intended on 4.0.alpha3. For example, find_nodes("*") only returns the top child nodes for me, not any subsequent child nodes. Similarly, searching for a class with either find_nodes("*", "MyClass") or find_nodes("", "MyClass") returns nothing, while there definitely are nodes of that type (nested within the child nodes).

Should I open a new issue for this?

@bitbotzgames
Copy link

Why isn't there also a find_parents function?

@nathanfranke
Copy link
Contributor

Why isn't there also a find_parents function?

You can benefit from type hints with your own function, although I recognize the use case.

func get_map(n: Node = self) -> Map:
    if n == null:
        return null
    if n is Map:
        return n
    return get_map(n.get_parent())

Any other use cases that really shine?

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.

Add a Node.get_children_of_type() method to get child nodes inheriting from a specific type