Godot-proposals: Remove `Node.find_node()` and `Node.find_parent()`

[TheDuriel]

Bugsquad edit: Changed to match the template.

Describe the project you are working on: This applies to any Godot project, since it's about removing an uncommonly used API.

Describe the problem or limitation you are having in your project:
Node.find_node() suffers from many issues, while solving very little.

Points against find_node():

• Not used in Engine. Added in 2015.
• Not used in any of the official demo projects.
• No warnings or errors. Only able to be caught via game crash.
• Slow recursive search.
• Manual refactor. Made more difficult by lack of warnings.
• Encourages bad practices. NodePath export, and static cached NodePaths via $ should always be preferable.

Discussion encouraged.

An argument made _for_ find_node() was that it technically allows you to write shorter onready variables.

My counter argument is that if you have long NodePaths, you can use string concatenation to clean that up, while preserving a single place to edit in case of a refactor.

Describe the feature / enhancement and how it helps to overcome the problem or limitation: Delete find_node for 4.0.

Describe how your proposal will work, with code, pseudocode, mockups, and/or diagrams: grep and the backspace key.

If this enhancement will not be used often, can it be worked around with a few lines of script?: Yes, it can be worked around with get_node, which is an argument in favor of this proposal of removing find_node.

Is there a reason why this should be core and not an add-on in the asset library?: find_node is an existing core API.

[SIsilicon]

The proposal sounds great, but unfortunately it doesn't adhere to the first rule of making a proposal.

1. Only proposals that properly fill out the template will be considered. If the template is not filled out or is filled out improperly, __it will be closed__.

I'm no moderator, but it might be best of you restructure your proposal. Again, I'm not a moderator; this is just a suggestion.

[aaronfranke]

For some evidence of usage, $ grep -RIn "find_node" in the demo projects repository returns exactly zero results.

@SIsilicon You are correct that all proposals must follow the template, thanks for notifying the author by replying here. In this particular case, I just modified the post myself, since all other sections for this proposal are very trivial.

[Error7Studios]

Strongly disagree.

I use find_node() for everything and never use get_node() or $.
I don't want my node-getting function to break whenever I change the SceneTree hierarchy.

All of my node-getters look like this:

func get_some_node() -> Control:
    var __some_node: Control = find_node("some_node")
    assert(__some_node)
    return __some_node

If I have more than one node of the same name that I may want to get, I create a function that uses a different parent/root.

For example, if my hierarchy is like this:

    UI
PlayerB
    UI

I would do this to get the UI node of either Player:

const PlayerID := {
    A = "A",
    B = "B",    
}

func get_Player_UI(__player_id: String) -> Control:
    assert(PlayerID.values().has(__player_id))
    var __Player_node: Control = find_node("Player" + __player_id)
    assert(__Player_node)
    var __Player_UI_node: Control = __Player_node.find_node("UI")
    assert(__Player_UI_node)
    return __Player_UI_node

The assertions make this easy to debug.

---

Not used in any of the official demo projects.

I can create a demo project for this if you want.

No warnings or errors. Only able to be caught via game crash.

Assertions will catch a null find_node() return value.
Yes, find_node() should return a warning or error, but that doesn't warrant completely removing it.
Instead, warnings and errors for find_node() should be added if possible.
Even if it's not possible to add warnings/errors to find_node(), assert already handles this just fine.
You also already get an error if you try to call a function or get a property on a null reference to a node.

Slow recursive search

I've had no problems with performance even in complex SceneTrees.
If I want to save the node in an onready var, I can do that as shown below with find_node().

How many milliseconds of optimization is worth the headache of manually changing a bunch of now incorrect NodePath strings each time you add an extra node somewhere in the middle of the node's path?

If I'm that concerned about performance, I can switch my find_node() calls to $ or get_node() when the game is completely finished and I know for certain that I'm no longer going to be changing the SceneTree.

Manual refactor. Made more difficult by lack of warnings

I can change the SceneTree by adding extra containers, etc, and don't have to worry about the node-getters not working.
If they stop working, the assertions will catch it and I can easily figure out what I did wrong.
Typing out the entire node path will require you to change every affected path whenever you add an extra parent/container in the SceneTree.

Encourages bad practices.

Please explain how the example I provided above is bad practice.

NodePath export

Exported NodePath variables can't be used for nodes generated procedurally in the _ready() function.

static cached NodePaths via $ should always be preferable.

How is the following example using $ better than the one using find_node()?

onready var my_control: Control = $MyControl

func _ready():
    assert(my_control)
    print(my_control.rect_size)

onready var my_control := get_MyControl()

func get_MyControl() -> Control:
    var __MyControl: Control = find_node("MyControl")
    assert(__MyControl); return __MyControl

func _ready():
    print(my_control.rect_size)

If there's a bunch of nodes that you want to get, you can create a generic getter for them.

func find_Control(__control_name: String) -> Control:
    var __control: Control = find_node(__control_name)
    assert(__control); return __control

onready var my_control := find_Control("MyControl")
onready var my_control_2 := find_Control("MyControl_2")

func _ready():
    print(my_control.rect_size)
    print(my_control_2.rect_size)

Also, I'm not sure what you mean by _"static cached NodePaths via $"_.
Are you talking about saving the NodePath into a const?
Because you can't use a const with $.
You can use get_node() with them, but you can do the exact same with find_node()

const PathTo := {
    MyControl = NodePath("MyControl"),
}

const NameOf := {
    MyControl_2 = "MyControl_2",
}

onready var my_control: Control = get_node(PathTo.MyControl)
onready var my_control_2: Control = find_node(NameOf.MyControl_2)

[jonbonazza]

@aaronfranke i think the original proposal was actually about removing the find_node method from the Node class entirely, do that it isn't even an option yo be used any more. This would be a backwards incompatible change in core, which id why it's being suggested for 4.0.

[jonbonazza]

To be clear I don't really have strong feelings either way about this proposal, I was simply trying to help clarify what I believe was @TheDuriel intent.

[aaronfranke]

That doesn't need clarification, the meaning of "removing the find_node method" is perfectly understood from the title of "Remove find_node()". The compatibility breakage is why I gave it the 4.0 milestone.

[jonbonazza]

Oh okay. The edits you made, specifically around mention of the backspace key and what-not, confused me, but i understand your meaning now. Sorry for the confusion!

[Xrayez]

Well I think that's again a debate on "usefulness" vs "feature completeness".

If beginners do __find__ this method useful, let them use it if this allows to concentrate better on more difficult concepts at first. From what I've seen, Godot's development philosophy promotes tools which are easy to use for beginners, but the existence of a single method doesn't really hurt anyone. In any case, it's always possible to recommend better ways of navigating the scene tree via documentation.

It's like, lets remove / operator because * is enough and more efficient. and / makes it too easy to divide by zero. Even better: lets prevent bloat and remove * operator because it's redundant and can be replaced with +. 😃

Sorry for cheesy example but that's just to demonstrate, I'm personally more or less neutral about this.

In fact, this proposal has given the idea I wasn't even aware before: independent onready referencing of nodes. Of course I think that's only suitable for prototyping, I wouldn't rely on this "in production" as they say (and you better have this method in core rather than plugin, otherwise it would defeat the purpose of quick prototyping).

[Xrayez]

I guess the same goes for find_parent()? reduz wasn't against that back in the days, but maybe he changed his opinion by now https://github.com/godotengine/godot/pull/22115#discussion_r219542604.

[TheDuriel]

Omg how did that ever get in.

Yeah I'd say we expand this request to find_parent() as well.

[KoBeWi]

I don't see why we should remove these methods. I mean, yes, people misuse them like doing get_tree().get_root().find_node("Player") every frame, but get_node() has similar complexity and it's also misused a lot (even more probably, because $ operator just makes it very convenient).

I think we should just improve the docs with example when to use these methods and when don't. Correctly used, they are convenient (although personally I never used either of them xd).

How is the following example using $ better than the one using find_node()?

It's shorter 🙃

Exported NodePath variables can't be used for nodes generated procedurally in the _ready() function.

When node is created from code, you already have the reference, so you don't need its path, nor even name.

[groud]

I mostly agree with OP's statements, as I agree find_node() is a really bad practice performance-wise. find_parent() is mostly ok though, as you only have one parent at a time, the algorithm complexity is only proportional to the depth of the hierarchy (and not the depth times the number of children per node).

However, there was a lot of people requesting this function, even if it was refused a number of times due to it's performance problem, we finally ended up accepting it under popular pressure. And I think that's mostly ok, if people want to trade their game performances for not thinking about their node hierarchy beforehand, that's ok. I can understand that some people want to freely move their nodes around while not breaking paths all the time.

What's not ok though, is that the reference does not clearly indicates that using find_node() everywhere is discouraged for performances reasons. This kind of limitations should be stated there.

[iamacup]

I don’t know that the removal is the solution because I don’t think this really addresses the underlying issue:

I see the issue being there is no satisfactory way of finding nodes out of the box without either:

1. Becoming tied to the node hierarchy which, if ever changes, breaks all your code or:
2. Being inefficient
3. Building your own / using third party solutions

This does not matter as much in small project, but once a project gets beyond a certain size moving a node in the component tree is a huge chore and the paths themselves virtually become non-human-readable due to length.

I would say that ‘Node Path Hell’ is worse than JavaScript ‘Callback Hell’ used to be before the language spec caught up with how it was being used.

We have built a bunch of helper functions in our projects that find nodes based on names, ID’s and all sorts, some global, some scene limited, etc.

There are also libraries like NodeFinder: https://github.com/Tearfalas/node-finder which do provide solutions, but obviously as they are not part of Godot core new users may not stumble across them.

It’s almost like it should ship with some O(1) access structure (which is what one of our plugins does) singleton to which you can reference nodes by ID from, adding an operator similar to $, like @GloballyUniqueName would also help in this regard

i.e.

@Player1
@Player2
@GUI
@redux_store

And you could do things like…

@Enemies.get_enemy(‘boss’)

[KoBeWi]

I see the issue being there is no satisfactory way of finding nodes out of the box without either:
Becoming tied to the node hierarchy which, if ever changes, breaks all your code or:
Being inefficient
Building your own / using third party solutions

I think #1048 would be a good solution. You can already export NodePaths which automatically update when nodes are moved within tree (in most cases at least) and that proposal would make accessing the node more convenient.

[TheDuriel]

exporting NodePath properties solves nearly all concerns with regards to "node hell", as do groups.

These solutions are efficient, clean to code, and reliable in all cases.

See also https://github.com/godotengine/godot-proposals/issues/1048 which would make it even better.

[TheDuriel]

By removing poor options we also make it easier for people to naturally encounter and employ preferable approaches. Instead of requiring people to constantly remind everyone "don't use this". Which is already the case with find_node() if you ever post code with it in the Discord.

[iamacup]

if #1048 does happen and fixes node path hell then i fully support removing these two methods, thanks for pointing it out!

I don't think these methods can be removed in isolation though and would be strongly against that - they provide a valuable function that without something else makes using one of the very compelling selling points of Godot a complete pain in the ass.

[Xrayez]

By removing poor options we also make it easier for people to naturally encounter and employ preferable approaches. Instead of requiring people to constantly remind everyone "don't use this". Which is already the case with find_node() if you ever post code with it in the Discord.

Hehe, I've been wondering whether you may have some personal agenda for removing those methods: so that you don't have to constantly teach people to stop using those methods on various community channels. 😛

Again, I think that can be solved with proper documentation.

I'm not really against removing methods and stuff if those are not that useful. I'm more against changes which make the life of beginners a breeze, but a hell for more experienced developers. Same goes for usability features, preferring beginner use cases without taking into account advanced ones is a crime in my eyes.

So I'd prefer if those methods are left untouched as they do not hurt anything by themselves (compare with being able to delete the entire project, for instance).

[dalexeev]

find_node is used in various projects, but often "incorrectly". Example:
https://github.com/TerryCavanagh/diceymodgeons/blob/dd9a0fe6b874b509413ca573af4c8a121a595438/Main.gd#L3-L8

[regakakobigman]

The changes to exporting nodes seem like they will solve the most common need for find_node. I don't think we should get rid of it completely, though. find_node is a loaded gun if anyone decides to use it in a frequent callback like _process, but if we remove it, users will probably just re-implement it or find some other way to continue using it. It's just too convenient, even if it is dangerous.

find_node is used in various projects, but often "incorrectly". Example:
https://github.com/TerryCavanagh/diceymodgeons/blob/dd9a0fe6b874b509413ca573af4c8a121a595438/Main.gd#L3-L8

That looks like a normal use case for find_node to me. It's used in initializing main.gd, so it will not be repeatedly called, and there should be few instances of main. It would be better to use node exports if those are implemented, but currently, that code is taking an acceptable tradeoff by using find_node over get_node.

[Xrayez]

Stumbled upon the Sketchfab Godot Plugin which uses find_node to write shorter onready variables as well:

https://github.com/sketchfab/godot-plugin/blob/379ad4da6e3f9297ff82efec64fe148a2e2f1017/addons/sketchfab/Main.gd#L29-L46

Even find_node().find_node() chains are used. 👀

So, I wouldn't bother removing those methods if they prove to be useful in those cases where these methods are intended to be used, it's just a matter of documenting the usage scope for these methods (with performance caveats already documented in godotengine/godot#41077).

[nathanfranke]

One use case I find in find_node is traversing multiple nested Nodes, especially with containers.

For example, given this setup:

| HBoxContainer
| | VBoxContainer
| | | PanelContainer
| | | | Label

One can access Label from Control with find_node("Label") or get_node("HBoxContainer/VBoxContainer/PanelContainer/Label").

One way to simplify the class signature of Node would be to remove find_node and improve the format of get_node. For example, there could be a glob-like syntax where multiple children can be traversed until a match is made. For example:
get_node("**/Label") or with $ syntax, $**/Label