-
-
Notifications
You must be signed in to change notification settings - Fork 20k
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
Revert removal of find_node
#58412
Comments
This makes sense considering that |
For a moment I thought that
This will actually lead to an error if array is empty, and the purpose of So yeah, I support returning The current |
yes please, as it is not just this:
but considering that generally you'd need to check first if there's at least one match, this
would become this:
|
Please bring it back. Godot is already a Nodepath fixer-up hell on larger scenes and projects. Iterating on complex, nested GUI is basically unbearable in Godot without a simple "find node recursive by name" function like In an ideal scenario we can keep both functions. |
You can use exported NodePath properties, which will be fixed automatically when you move nodes around. You'll have to use (This also applies to |
Yes, please bring it back.
It's so good. by
by NodePath, I need more code:
|
My take on this is that we should probably do this in a different way and take the chance to do it properly in 4.0 I was thinking that we could have an option for a node to make it unique named within the scene you are editing. This is, for this scene, it is warranted that the node name is unique. If you try to make it non-unique the editor will error. Then, from any node within the scene, if you do $Name you obtain this node. This should be relatively easy to add and I think its a lot more usefriendly than having to use find_node, which a lot of users end up relying to and which also requires an unique name. It can also be autocompleted. |
@reduz: Very good idea because I often end up with a single node groups (e.g. "Root" or "player") for exactly this purpose... |
@BeayemX @smix8 Would this proposal work for your use case? Instead of finding by name, you could just export a reference to that node, which should be fast and easy godotengine/godot-proposals#1048 |
@aaronfranke That would be great and fit my mentioned usecase pretty well. But I still think that |
I can setup a PR to restore the old find_node, in fact I will probably do it in a bit, I don't see why we can't have both. |
How is find_nodes() better than find_node()? Better performance? How much better? If there is no better advantage, then please revert and put an end to ugly code. |
@GiantArtisan: From what I understand, performance is one of the advantages. |
If there is test data to illustrate, it might convince a bunch of people |
more so than performance, I think it is about the greater utility, now being able to find multiple nodes and mask not just for the node name, but also being able to specify the desired class name. furthermore, I think the intention is for but "forcefully" getting users to adopt different patterns for their project (when there is no real cost to letting them stick to the patterns they are used to) is probably not the way to go. |
@GiantArtisan: Change was made so recently that I expect not many people have used it before and after (very few people seem to be using Godot 4 alphas, mostly due to veeery incomplete documentation for many things changed/introduced) |
I don't see anything wrong with having both |
Coming to Godot from other engines, I was using find_node as an alternative to finding a child node by type, which is not a built-in function of Node. The reason I wasn't just using $nodepath was that I didn't want to enforce a fixed path, so the script would be useable in different scenes, although by using find_node I was enforcing a name on the node. I couldn't figure out how to expose the node as a parameter either, but maybe I didn't try hard enough on that front. Maybe my thinking was too component-based, but I do think it would be useful to have find_child_by_name(name) and find_child_by_type(class) functions that simply return the first match if possible/agreeable. I understand not having such a function if you think that users of other engines would just bring their programming style to Godot and not learn the Godot way :) EDIT: I didn't realise that get_children() had no recursive option. find_nodes is quite useful, but I agree that it's a little clunkier due to the need to check the returned array is not empty before the node can be safely accessed. |
I agree. |
I propose restoring it. The idea that one would remove oft-used language features to discourage "bad practices" is somewhat problematic for me. What is a "bad practice" can be considered subjective, and if that rationale were evenly applied along the entire engine, how many other features could be pruned? In some sense, the usage of node literal paths ($/path/to/node) is bad practice because they tend to make changing scene structure difficult. |
This is not directly related to the find_node() function but part of the problem why NodePath's don't work as a replacement for it currently. The problem with the exported NodePath's (or Nodetypes like in the linked proposal) is that they share the same issue that the filesystem rename function has. The pathupdate functionality breaks reliably on large projects with heavy, nested scene assets when files are moved or renamed corrupting entire scene files. The "fix broken filepaths" dialog in Godot editor stops working on them. If the nested subfile of the subfile breaks you not even get a realiable error msg. Before this is not fixed the current NodePath solution only works for small sized projects. On a different note, having so many NodePaths as export variables in the Editor Inspector for simple scenes would be incredibly clumsy to work with even when grouping them. |
Just want to give my +1 for renaming |
hm I'm of the opposite opinion.
I think the distinction by plural only is just fine. |
Another vote to bring back |
I use find_node quite a lot in 3.0 for complex scenes (mostly gui) which I refactor the node hierarchy alot. While I would support the return of |
I would also like to request this feature to come back, find_node was very helpful in my opinion. |
So, #60298 implements this same functionality in a cleaner (and more efficient) way, but some users prefer to keep to My proposal here would be to not revert |
I like that idea. |
Done with #60511. |
Godot version
v4.0.alpha.custom_build [256069e]
System information
Linux Mint - Vulkan API 1.2.131 - Using Vulkan Device #0: NVIDIA - NVIDIA GeForce GTX 1080
Issue description
The function
find_node()
(returning a node) was replaced withfind_nodes()
(returning an array of nodes).The commit 78dc608 made the changes.
While I think that the new functionality can be useful I think we should also keep the old implementation.
Previous usage
I used the old implementation mostly to reference nodes that were subject to be moved in the hierarchy later on.
It would look something like this:
Drawbacks
Ugly code
With the new implementation I would have to access the first element of the returned array every time.
This
would become this:
Possible performance gains?
I am not sure if this is correct, but could it be possible for
find_node
to have performance benefits because the traversal could break early when finding the first match?Steps to reproduce
Not applicable
Minimal reproduction project
No response
The text was updated successfully, but these errors were encountered: