godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.12k stars 69 forks source link

Add scene-unique nodes to make accessing nodes from code more convenient #4096

Closed reduz closed 2 years ago

reduz commented 2 years ago

Describe the project you are working on

Godot

Describe the problem or limitation you are having in your project

Currently in Godot, finding specific nodes that have different accesses across the project is done the following way:

This suffices for a lot of cases, but it often happens that developers want to access a specific node in the scene tree, but this node is situated behind layers of containers or other parent nodes, so typing the full path such as:

$VBoxContainer/HBoxContainer/VBoxContainer/Option1/Label1.text = " hello"

Can be very cumbersome and annoying, even with code completion.

What many users do to work around this is to put a unique name to it and use find_node with an autoload to find it:

@onready var label1 : Label = find_node("Label1")

But this is messy. If there are many nodes around, having to create local variables for each makes code difficult to read. Also the solution is just not obvious.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

The way to solve this is by implementing Scene Unique Nodes.

These nodes would only exist once per scene and you will be able to access them by name by ensuring their name is unique.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

The idea is like this:

RMB any node and set it as unique:

image

This node will be, thus, unique in this local scene. It does not mean that any other node can't be named the same, but it ensures that no unique nodes can't be named the same:

image

Then, accessing it from script would be simply, just type the name:


$@Label1.text = "Hello"
# Same as:
$VBoxContainer/HBoxContainer/Label1.text = "Hello"

And that's it.

So. some questions may arise such as:

Q: What is the difference with find? A: More strict and largely more performant, as access will be instant because this node is unique (no actual searching takes place). Simpler to type and to understand when reading the code.

Q: What if I want to make the parent unique but access a child node? A: $@Label1/Child.say_hello()

Q: What if I want to access it in a sub-scene? A: The idea is that here we want to satisfy the most common case which is searching within a scene, but you can work around this rather easily. If the scene in the example above was a subecene you could do:

$Path/To/SubScene1/@Label1.text = "Hello" # Still works 
# Instead of
$Path/To/SubScene1/VBoxContainer/HBoxContainer/Label1.text = "Hello"

But additionally, you can make the subscene also a scene unique node, so you can do:

$@SubScene1/@Label1.text = "Hello" # Still works 
# Instead of
$Path/To/SubScene1/VBoxContainer/HBoxContainer/Label1.text = "Hello"

Q: What about non GDScript languages? A: get_node (GetNode in C#) will work the same:

GetNode<Label>("@Label1").Text = "Hello";

Thanks @pycbouh and @groud for giving feedback on how to implement this to make this proposal more solid.

If this enhancement will not be used often, can it be worked around with a few lines of script?

N/A

Is there a reason why this should be core and not an add-on in the asset library?

N/A

aaronfranke commented 2 years ago

@starry-abyss Nodes with auto-registering global aliases are already available, in the form of AutoLoads.

h0lley commented 2 years ago

so the gist of it is that we want to be able to rename and move around nodes in our scenes without having to check each time whether that breaks hardcoded nodes paths in our scripts, yes?

find_node() is not a sufficent solution because of potential performance issues and also it only allows moving nodes within the scene - not renaming them (unless the name mask is super loose, but then it may pick up the wrong node).

then there is NodePath and the ability to expose it to the inspector which auto-corrects it, but this is also in need of improvement as:

  1. it's cumbersome to basically keep two variables for each node used in code around; its NodePath and its reference obtained from get_node().
  2. when exported but the node path is yet to be specified in the inspector, and it is attempted to get_node() without a check (which you can't do in an @oneady line), then the game will crash. I think it should be the nature of exported variables that they are optional to be changed/adjusted/set in the inspector. I forgot about get_node_or_null() which can resolve this.
  3. I also dislike having to declare class properties that may only be used in one method.

in conclusion, what I'd like to see is a version of this:

@export_node_path(Sprite2D) var my_node_path
@onready var my_node: Sprite2D = get_node(my_node_path)

that can be collapsed into one line and variable declaration and that does not error when the field in the inspector isn't set yet (should default to an invalid/null reference). there may then also be fallback logic for the case where no node has been specified, as that node may be intentionally optional. I don't know how to solve the third point of my list of issues, but I'll live.

I am probably pretty much imagining this. if that were possible, then I think there is little need to introduce a new concept like unique/shortcut nodes.

I've thought about it some more and now I want both (this proposal and a collapsed NodePath+get_node()). they cover different use cases.

markdibarry commented 2 years ago

Tbh, I'm pretty for this one.

Recently, in my current project while playing around with Godot 4.0, I reverted all Menus in my game from using NodePath to just hard-coding the strings in my script. I liked the idea of being able to assign option containers via the editor, but when I'd make a change to anything in Menu.cs, any scene that used it would get confused and not be able to use any of the assigned export values in the .tscn, effectively clearing all values in every scene. After about the fifth time of going through every menu in the game and reassigning the NodePaths, I decided it was less fragile to rely on magic strings over exported values. Having an easy to assign and maintain hard reference to these nodes available in script would make my workflow much easier.

girng commented 2 years ago

This suffices for a lot of cases, but it often happens that developers want to access a specific node in the scene tree, but this node is situated behind layers of containers or other parent nodes, so typing the full path such as:

These nodes would only exist once per scene and you will be able to access them by name by ensuring their name is unique.

What I've been doing ( for half a decade ) is simply right click and click copy node path and create a onready variable. The "Make Unique" might cause confusion with the Make Unique option for resources as well. Just food for thought and my opinion/experience

Byteron commented 2 years ago

I'm not sure I like this proposal. You can already achieve a similar effect with just creating an onready var with the path.

onready var label1 = $VBoxContainer/HBoxContainer/Label1

func _ready():
    label1.text = "foo"

What I am afraid of is that by giving users a shorthand to nodes, they will just drop using onready vars entirely and just do stuff like

func _process():
    $@Label1.text = Engine.get_frames_per_second()

everywhere. and if the shorthand amounts to get_node("Path/To/Node") that is significantly slower than using an onready var. it's a pitfall for gdscript performing significantly worse than it has to.

darrylryan commented 2 years ago

I am really happy this is being worked on because it's a major headache - but I agree with some people the solution might not be quite right.

We need to be able to do this without saving anything in the scene we're referencing itself, because we need to be able to dynamically load scenes. As time goes on we ideally want scene streaming, lazy loading etc. for large worlds. Also think in terms of imported FBX files - to some extent the FBX is a black box and we don't want to have to make the FBX nodes local in order to make them unique.

I think a much more flexible and useful thing would be to have a C# LINQ-like query syntax on scenes something on the lines of:

var label = myscene.nodes.Where(node => node.name == "Label1" && node is Label).FirstOrDefault() 
label.text = "foo"

or:

var camera = myscene.nodes.Where(node => node is Camera3D && node.parent.name == "CameraRig").FirstOrDefault()

to get the first camera attached to the camera rig.

or a more advanced example:

var materialusers = myscene.nodes.Where(node => node is MeshInstance3D && node.SurfaceMaterialOverride.contains("treematerial01")

could be used to get a list of all the mesh instances in the scene that use the tree material, which you could then update or replace really easily.

samsface commented 2 years ago

What about keep find_node but have a setting on the node in editor 'index for fast find' the same way make unique is done. Then this node would be added to a map that would be the first search attempt for find_node calls.

This would give same performance as proposal. Keeps backwards compatibility with Godot 3.x. Doesn't introduce new syntax and stuff to learn.

dreamsComeTrue commented 2 years ago

As for C#, maybe instead of prepending '@' symbol, provide method like: GetUniqueNode<T>("Label1") ? - it describes exactly what it does, and doesn't introduce cryptic syntax.

OscarCookeAbbott commented 2 years ago

Apologies for commenting out of the blue for the first time, and if my comment or understanding should miss something important or obvious.

I agree with some previous responses in that I think the issue this proposal looks to fix is the result of lacking functionality in existing systems. I think improving exported nodes and singletons would make node references via unique names unnecessary.

For exporting nodes/nodepaths, I think the ability to specify the class of node, and the ability to more easily assign them is necessary. Something like: export var node : NodeType or if that syntax is not specific enough to distinguish nodes in scenes specifically, something like export(NodePath) var node : NodeType instead of export(NodePath) onready var node = get_node(node) as NodeType as it currently stands (in its most concise form)

For singletons, I think the option to add singletons inside a scene instead of on autoload, or the addition of a new complementary system that achieves this, would be an excellent addition.

The result would be using singletons throughout scenes for important, unique objects and exported nodes on those singletons for important children, ie. HUD.health_text.text = "100". This is how Unity and other engines allow you to achieve this sort of functionality, and I've never found it limiting in my extensive Unity experience.

I think it's also easier to follow a chain of references via such a system versus uniquely named nodes in any number of scenes.

h0lley commented 2 years ago

@jasonwinterpixel

Godot should also allow type hints on NodePath exports like this:

@OscarCookeAbbott

For exporting nodes/nodepaths, I think the ability to specify the class of node, and the ability to more easily assign them is necessary.

as I've seen this coming up multiple times now, just wanted to let you know that this is already implemented. you can do this Godot 4 alpha: @export_node_path(YourNodeClass) var my_node_path nodes of other types will then be grayed-out and not selectible from the inspector.

OscarCookeAbbott commented 2 years ago

@h0lley Ah yes great point, I'd forgotten that new feature. One thing to tick off the list!

AttackButton commented 2 years ago

Maybe call it quick or fast node instead of unique, which is already quite used.

Something like create fast node or create quick node instead of make unique.

theraot commented 2 years ago

@AttackButton

Something like create fast node or create quick node instead of make unique.

Although I agree make unique is not great in that the frase has other meaning already when it comes to resources. This isn't great either. I can imagine somebody unfamiliar with the feature thinking this makes the code of the script attached to the node execute faster. Something similar to increasing the script execution priority.

markdibarry commented 2 years ago

Other verbage I like better than unique: make local node (similar to "Make local to scene"... but that's poorly named too) make seek node make alias node make shortcut node make fold node (cause it folds in the tree)

pchasco commented 2 years ago

I don't think the term "alias" should apply because the node already has a name, and I think it would be confusing to grant a node an additional name through an alias. If there isn't a new name being assigned, then "alias" wouldn't be accurate.

"Pinned?" "Scene scoped?"

This could be a checkbox in the property list rather than forcing a user to find the feature behind the RMB click.

markdibarry commented 2 years ago

Oooh. I like "pinned node"

theraot commented 2 years ago

@pycbouh

find_node has been removed in master. So there is currently no solution for non-designers. What reduz is proposing is a solution that would fill find_node's shoes better instead of restoring that potentially misused method.

If this is the solution for non designers. Then why is it something that you do and modify from the editor? If we will define it from the editor then NodePath does that as it has been pointed out by others.

That reminds me, the proposal does not say how we "unmake unique". But I'll assume it is from the same menu.


I believe the way this compares to NodePath is not whether or not it is for developers or designers, but this is producer solution, but NodePath is a consumer solution (and yes, the designer is often the consumer of the code the developer created). To be clear:


Something that has come time and time again is the ability to set or get properties of the children Nodes on their owner, or something along those lines. (Something that can be served with Editable Children and NodePath on the consumer side, or with getter and setters on the producer side).

See https://github.com/godotengine/godot-proposals/issues/361 and https://github.com/godotengine/godot-proposals/issues/3056 and https://github.com/godotengine/godot-proposals/issues/3147 - It is also sometimes the motivation or at least a possible use case of multi scripts or trait/mixin systems, where we could do the boilerplate to do the delegation or to handle a NodePath once and include it in the Nodes that need it, perhaps even multiple times.

I believe that is ultimately what we want. A "make this node easily available form the outside" button. And please notice that this feature does not have to piggyback on get_node.


For example, we could open the context menu in the scene editor, click the "make this node easily available form the outside" option (with whatever name it ends up having), and what it does is create a property on the root of the currently edited scene (or on the owner of the Node if that is preferible) that references the Node. And you would use it like any other property.

So instead of doing this:

$Path/To/SubScene1/@Label1.text = "Hello" # Still works 

You do this:

$Path/To/SubScene1.Label1.text = "Hello" # Still works 

And, of course, there would be UI to remove them. I can even imagine UI that lists them.


And if we are adding custom properties, why do they have have to be Nodes? There is already something similar already: meta. Except meta does not have an UI, nor a convenient way to set Nodes. But if it had, we could do this:

$Path/To/SubScene1.get_meta("Label1").text = "Hello" # Still works 

And the code for that is already in the Godot. Ah, except inferring the type, and of course we will want that part.


Anyway, I'm highlighting the fact this does not need to be built on top of get_node.

dalexeev commented 2 years ago

I don't like this proposal, because at the moment the scenes "dissolve" into the general tree, all nodes are equal. And with this proposal, special nodes appear in the scene tree - the roots of sub-scenes, which cease to "completely dissolve" in the general tree.

This means we may need to at least highlight them in the remote scene tree. IMO this is not obvious and worsens the existing architecture. Even global nodes seem like a better idea than scene-unique ones.

I believe there are better solutions to this problem than this one.

pchasco commented 2 years ago

To me, this feature is not for designers at all. Do designers care if their nodes are easily accessible via a short node path? Wouldn't a designer simply use the GUI to find the node?

I do see the convenience of this feature, however I do think it is blurring the lines between what is a scripting concern and what is a designer concern.

To my mind, the correct solution is already available using find_nodeS at ready, or by exporting a nodepath from the script that can be set by the designer in the UI. This clearly communicates to the designer that the script requires the path to a node, and frees the designer to bury the node as deeply as necessary. The script writer does not need to be concerned about its actual path, and everyone is happy.

YuriSizov commented 2 years ago

@theraot

I agree, that having steps that you need to take in the editor can be considered a downside, but I don't know what alternative way of selecting a node in a scene can be there, other than using the UI and the scene dock. Ultimately, you still compose scenes in the editor, so it's not too big of a disconnect to also mark nodes there before you use them in code.


Personally, I don't use find_node myself, in my own work. But I do use it in projects of others where it's an established pattern. So personally, I am unaffected by the removal itself. However, with this proposal I may start using this feature, as it will be optimized for performance.

And I don't use node paths. Just like I don't connect signals in the editor, and do it in code instead. I find that using these features splits the logic that should be in code into two places, and hides it from the developer. Besides, you often have 10-15 node references in a scene, and have them shown as configurable properties creates a lot of visual noise. But I do see the benefit of this for "consumers" of components that I make, and it's a totally valid use.

Hope this helps see my workflow and workflow of projects that I am part of that aren't mine.


I also agree it shouldn't rely on get_node. I proposed to reduz that instead it should be a separate method that only works with the hash table of unique nodes in a scene, but he wasn't a fan of the idea.

nonunknown commented 2 years ago

Just like I don't connect signals in the editor, and do it in code instead

This can be easily solved by: when user connects signal via editor, the engine places the following line in the script:

connect(...)

theraot commented 2 years ago

@pycbouh

Yes, I compose scenes in the editor. In fact, I don't have a problem with connecting things in the editor most of the time. I don't think it is a downside, just something a designer can do. Thus I don't think this features is simply for non designers. It is for people making reusable scenes, who might or might not be designers.

By the way, trying to think why reduz didn't like the idea of this being separate from get_node… I can see a benefit of having it on get_node: you could use it with NodePath. The consumer can use NodePaths to refer to these "made unique" Nodes. So this feature could actually enhance NodePath instead of competing with it.

Perhaps presenting this feature as an alternative to find_node is what makes us think of NodePath as an alternative. Since both find_node and NodePath are on the consumer side.


@nonunknown If the editor inserts code for the signal connection… does it do it on the source of the signal or on the destination? What if they don't have a script? What if the script is from an addon? Edit: or simply what if the script is used elsewhere where it should not be connected. I don't think it is an easy solution.

nonunknown commented 2 years ago

does it do it on the source of the signal or on the destination?

this is defined by the user in the popup as always has been

What if they don't have a script?

there's no way, every signal connected via editor is to a script, you can try RN!

What if the script is from an addon?

Same as above answer

theraot commented 2 years ago

there's no way, every signal connected via editor is to a script, you can try RN!

The source does not have to have a script attached to be able to connect signals from it. So this won't be true:

this is defined by the user in the popup as always has been

Edit: I mean, the user won't have the choice, it would be on the target always.

And I would not want it inserting connect on addons, or any other script that is meant to be reused in multiple places. And, yes, I can connect a signal to Node with an addon script without it inserting any code there. Sadly this recently requires enabling advanced options, otherwise I can't select the node. It has been an annoyance, thank you.

boukew99 commented 2 years ago

"Rename" a Node and "Make Unique" have the same functionality for me. Could they be combined into one step? This makes sense functionality wise. Access a node directly by giving it a unique name in the Scene. This also already works to ease auto completion for get_node().

Mickeon commented 2 years ago

I personally would love to see the brought up issues solved in a more... "natural" way.

Like many have suggested for very good reasons, I'd love, more importantly, to allow exporting Nodes. It is simple and intuitive, and it's particularly useful for anyone messing around with the tree structure, especially designers and first-hand users trying out Control Nodes.

But along with that, I believe a few extra features on top of it would be nice:

cybereality commented 2 years ago

So I was initially against the idea, but after reading all the comments, I think I support the proposal. It does solve a real problem, particularly when coding UI with 6 or 7 layers deep. Though auto-complete can help, in some cases, it doesn't work well if you have 7 nested nodes with long string names. So this would solve that, since you can access the node directly by the unique name and not the full path (and please, I hope this will work with auto-completion).

I think the NodePath improvements are interesting, but we should discuss them in another proposal. I don't think that is the relevant topic here, and we can still do both (or neither) depending on what people decide. I also don't think the artist / designer workflows are relevant here either, since this is a code / syntax improvement, meaning for programmers (and, in any case, it will make designers job much easier as well). And you can already use NodePaths just fine, I have used them before, they are fully working and don't need improvement. That's all I have to say there.

To give you an idea how I work, I don't deal with long paths. I create a variable for each major nest in the path, that way it saves me typing long strings, allows things to be more flexible, and code-completion still works. For example:

onready var game_ui = get_node("/root/Game/UI")
onready var text_box = game_ui.get_node("TextBox")
onready var title_label = text_box.get_node("TitleLabel")

This is great for me, as if I rename the "UI" node, I only have to change one line. I can also move the whole "UI" node into another parent container, again, only change one line of code. And this works now, but requires a lot of typing. So this proposal would be an elegant solution and make my life much easier.

So I am 100% on-board, and I think using the at symbol and the syntax and making them local scope are all perfect and make sense to me. So we should do it (and if people don't want to use it, the old way still works fine, including using NodePaths). My only concern is the label "Make Unique". This is already used for a completely different purpose (for resources) and I believe this nomenclature is overly confusing. We can decide what the name should be, but I would suggest:

I think "Make Unique" is too ambiguous, but "Set Unique Name" I think is a lot more clear. Because "Make" sounds like you a potentially copying something or generating a resource or file. "Set" seems more clear to me, because you are not "making" anything, you are simply giving something another name (the thing itself is not changing). I would be okay with "Set Alias" or "Set Shortcut" as well, but my vote would be for "Set Unique Name"

dalexeev commented 2 years ago

To give you an idea how I work, I don't deal with long paths. I create a variable for each major nest in the path, that way it saves me typing long strings, allows things to be more flexible, and code-completion still works. For example:

onready var game_ui = get_node("/root/Game/UI")
onready var text_box = game_ui.get_node("TextBox")
onready var title_label = text_box.get_node("TitleLabel")

See also: #1776

rambda commented 2 years ago

I support this proposal because it is convenient. More convenient than exporting NodePath. I only need to click once in the editor and it's good to go. This is a piece of code of "EventEditor" from an add-on I made for the gameplay designer of my game (Godot 3.x). Performance doesn't matter because it's smooth and I just don't want to waste time on an un-reusable add-on.

tool
extends "ResEditor.gd"

onready var Tags: ItemList = find_node("Tags")
onready var Involvers: ItemList = find_node("Involvers")
onready var PreEventsUIDArray := find_node("PreEventsUIDArray")
onready var LocationUID := find_node("LocationUID")
onready var MaxOccurenceButton := find_node("MaxOccurence")
onready var DeferredTimeCheckButton := find_node("DeferredTimeCheck")
onready var PreconditionList := find_node("Preconditions")
onready var PreEvents := find_node("PreEvents")
onready var SubEvents := find_node("SubEvents")
onready var ResultList := find_node("Results")
onready var SelectedEventEditor := find_node("SelectedEventEditor")
onready var StoryFileSearch := find_node("StoryFileSearch")
onready var MissionEligibleLevelBox := find_node("MissionEligibleLevelBox ")
onready var TeamSize := find_node("TeamSize")

Using find_node requires 0 click in editor, 14 lines of code.


Now that find_node got removed in Godot 4. If I use export NodePath.

const UIDSearch = preload("res://Global/Control/UIDSearch.gd")
const FileSearch = preload("res://Global/Control/FileSearch.gd")
const ResEditor = preload("res://SimLogic/Editor/ResEditor.gd")

@export_node_path(ItemList) var Tags
# Assume exporting NodePath gets a syntactic sugar,
# so that it will automatically do @onready var Tags = get_node(Tags_path)
@export_node_path(ItemList) var Involvers
@export_node_path(Container) var PreEventsUIDArray
@export_node_path(UIDSearch) var LocationUID
@export_node_path(Button) var MaxOccurenceButton
@export_node_path(Button) var DeferredTimeCheckButton
@export_node_path(Container) var PreconditionList
@export_node_path(TextEdit) var PreEvents 
@export_node_path(TextEdit) var SubEvents
@export_node_path(Container) var ResultList
@export_node_path(ResEditor) var SelectedEventEditor
@export_node_path(FileSearch) var StoryFileSearch
@export_node_path(Container) var MissionEligibleLevelBox
@export_node_path(SpinBox) var TeamSize

This is 17 lines of code and 42 clicks (assume "exporting NodePath and onready var" gets a syntactic sugar) (14 clicks to open the exporting window, ? presses in the search filter (only in 4.x), 14 clicks for selecting node, 14 clicks for "OK").

And I have tons of sub-components like (EventEditor) in this add-on. How many clicks do I have to do?


But if "scene-unique nodes" gets implemented, it's 14 clicks in the editor, 0 line of code. And this, $@LocationUID, is so *** convenient.


If I will only be able to use export, then I would like to be able to export a node in the editor with one click and have the code automatically generated in the script, with @onready var node = get_node(node_path). Then I'll be able to just use LocationUID directly. If so, it's actually quite nice. But $@LocationUID/LineEdit is still better than LocationUID.get_node("LineEdit")

cybereality commented 2 years ago

Correct, in addition to my original comment, the proposal seems to be the least amount of code and clicks than any of the further suggestions.

PoisonousGame commented 2 years ago

I just want to know. After setting a unique node, if the node is moved at design time or runtime, will it be accessible via $@?

jordo commented 2 years ago

I really don't support this proposal the more I think about it, here's an recap:

Apparently find_node was removed and that removal was merged into master 8 days ago. (https://github.com/godotengine/godot/pull/56032). It was 'replaced' with find_nodes which returns an Array of nodes. Whats the difference between the two? Well presumably, find_node does a breadth-first search from a target node on a name comparison (still exits early on each strcmp), but importantly it exits it's entire search early when a match is found.

The decision was to replace that api with a more flexible api of find_nodes, which returns an Array of all potential matches.

Outcomes from that:

The proposal in my opinion is a knee-jerk reaction to solving an issue that doesn't need to be solved right now, especially when 4.0 alpha/beta/GM need to get out the door as soon as possible. The simplest solution right now is to keep find_node in addition to the new find_nodes on master. How much weight is being thrown behind the idea that '4.0 is our only opportunity to break things and introduce new workflows` is not balanced right now.

Rational 1): _find_node was removed to discourage bad practices._ Well that's not true because it wasn't removed, it was refactored to find_nodes.

Rational 2): @ syntax will be faster because it uses LUTs that can be optimized at "compile"time.

find_node accepts any string input. As such, the nodes it not guaranteed to be there at "compile" time. It can also be a variable. This cannot be optimized, it's purpose is to do a full tree lookup. Which is why it was a long standing desire to remove the method, and so it was done recently.

Again not really true, because the proposed @ syntax lookups will be parsed exactly in the same way the current $ syntax is. $ literally gets parsed into get_node which is then evaluated at runtime. The @ syntax lookup may be faster yes, but there is nothing compile time guaranteed about the new proposed @ syntax. @ will most likely get parsed into another runtime constuct, say get_node_uniquely_named or something, which ends up being a runtime map lookup (which may or may not fail).

Rational 3): This is quick and easier for developers 'working with everything as code'. I don't think quite true. Will auto-complete work? No. Not unless you make your nodes 'unique' before your write accessor. How do you use this feature? You have to interact with the editor! How do you know what nodes are 'unique' or not? You have to inspect the scene-tree, in the editor, and look at the color of the node name's. How do you make a node unique? You have to, in the editor, RMB on a node and click 'make unique'. Using this proposal means doing GUI things in the editor. This is a editor GUI + code thing, as much as looking at and typing node name in the editor is.

With this proposal, instead of 3 there now will be 4 different ways to get references to nodes:

I can say right now that there is going to be a ton of edge cases and scenarios implementing the last one. find_node is really simple to understand and implement because it's purely a dynamic runtime search. With the @ synatax, is there runtime/api support for 'make unique' outside of an editor RMB? If i'm creating a node tree (outside the editor), say purely in code, and a make a node 'unique' what map is the lookup inserted into? How can I tell what map it's in and if it potentially collides with something else? If it's a global map, then sure I guess it would work. But this proposal somehow includes 'locality'...

I think it's going to have edge cases with subscenes , be potentially fragile and error prone, and add a lot of unnecessary complexity to the engine and codebase. Almost every developer in this thread that wants to replace something like this with this proposal:

onready var game_ui = get_node("/root/Game/UI")
onready var text_box = game_ui.get_node("TextBox")
onready var title_label = text_box.get_node("TitleLabel")

would be much better served with just having a construct to popup a Editor GUI selector to click the node in the current scene they want an onready var attached to. Finding and clicking 'make unique' is the same amount of editor clicks as clicking on a node itself.

cybereality commented 2 years ago

I agree with your points and concerns, but I still like this proposal. I frequently have nodes with the same name, such as "Camera" or "Shape". It makes it easier for me. I will still name the parent "Player" or "SpaceShip" or whatever, but there is no need for a unique name of "Shape" because it is clear, from the context, that it is a collision shape. So I never use find_node(). I also don't like the idea that it only returns the first match. If I have 16 nodes in the whole tree with the name "Shape" then I have no clue what was returned. Yes, you could do a local search for just the node you want, but in that case, you already basically know the full path, so you might as well just type it and gain some performance. So the original proposal here does seem useful, even if I can accept it is a band-aid solution.

pchasco commented 2 years ago

would be much better served with just having a construct to popup a Editor GUI selector to click the node in the current scene they want an onready var attached to.

Initially convenient but doesn’t do anything for refactoring the scene hierarchy.

I still believe the best solution is to export a node path in the script and populate it from the editor.

YuriSizov commented 2 years ago

Will auto-complete work? No. Not unless you make your nodes 'unique' before your write accessor. How do you use this feature? You have to interact with the editor! How do you know what nodes are 'unique' or not? You have to inspect the scene-tree, in the editor, and look at the color of the node name's. How do you make a node unique? You have to, in the editor, RMB on a node and click 'make unique'. Using this proposal means doing GUI things in the editor. This is a editor GUI + code thing, as much as looking at and typing node name in the editor is.

This is in no way different to how many people work with "code only" today, when they type out whole node paths in code. Like, at all. It's the same flow for those people. And it's perfectly fine. Please, don't invalidate people's workflow just because it's not your workflow, just because you can afford to have dedicated designers who don't touch code at all. For smaller teams using hardcoded node paths is a normal practice.

And the amount of setup in the editor in the flow introduced by this proposal is the same as it is when you want to hardcode a node path. After all, "code only" doesn't mean we create our scene entirely in code. Sometimes we do, as it is necessary, but most of the time we do set everything up in the editor. We define the scene, its nodes, their properties in the UI, in the scene dock and in the inspector. That doesn't invalidate that then we go to the code editor and write most everything else in code, because that's what is usable and suitable and easy to understand for us. The editor UI in that case assist with that approach. Like you can drag and drop node paths in code from the scene dock! And yes, you'd mark the node as unique in the editor UI before creating a reference in code. That's normal too.

The UX of the proposed feature still needs work, it's not reduz's strongest side, but we can improve on it later. And it would depend on how much people use it too, because if it happens a lot, we can make it as easily togglable as visibility.


Please stop arguing like implementing this somehow blocks another feature that you really want with exported nodes. Most of the contributors and maintainers who spoke on this proposal and in related discussions have supported the idea of exporting nodes. Making it a competition, an ultimative choice that we have to choose one or the other is not productive. Both features have their uses, even if you don't see a use for one of them yourself. There are many other users of the engine who do.

If anything, we welcome anyone to contribute a solution to exporting nodes.

h0lley commented 2 years ago

what perplexed me a bit is the amount of people who are fine with littering the scope of their classes with a bunch of node references that may only be needed in specific methods. declaring a "header" of get_node() calls in each class seems really unwieldy to me.

the export(NodePath) method is even worse, as your get_node() header requires even more code and you have to maintain it in the inspector on top of that! of course, if these nodes are actually something that makes sense to be adjustable - i.e. where a designer may choose out of several different targets - then export(NodePath) is the way to go, but using it as the general method of obtaining node references seems horrible, as in most cases, these node assignments are not something that is subject to change, and so they just bloat the inspector UI.

that is why I mostly opt for the traditional $ within methods. the performance of $ is great - almost on par with references (it is significantly better than get_node(string)). however it is prone to breakage and lengthy written-out paths messing with readability... which is exactly what this proposal addresses.

nonunknown commented 2 years ago

that is why I mostly opt for the traditional $ within methods. the performance of $ is great - almost on par with references (it is significantly better than get_node(string)). however it is prone to breakage and lengthy written-out paths messing with readability... which is exactly what this proposal addresses.

Actually $'s performance is worse than get_node(), I saw a twitter user did the performance tests, and get_node() was faster!!!

Zireael07 commented 2 years ago

what perplexed me a bit is the amount of people who are fine with littering the scope of their classes with a bunch of node references that may only be needed in specific methods. declaring a "header" of get_node() calls in each class seems really unwieldy to me.

It's unwieldy, but necessary if you're using the references every frame or more often (process() or physics_process() )

groud commented 2 years ago

Actually $'s performance is worse than get_node(), I saw a twitter user did the performance tests, and get_node() was

Internally, the $ operator calls get_node, so performances should be similar. $ might have some overhead, but in theory, it should not change much.

h0lley commented 2 years ago

@nonunknown @Zireael07 @groud I always assumed that $ is nothing but a shorthand to get_node(), but of course I wouldn't make claims like this without doing a basic test myself prior.

10 mil iterations in current Godot 4 alpha:

my_node: ~ 1400 ms $MyNode: ~ 1800 ms get_node("MyNode"): 4000+ ms

Using $ within process functions is fine.

edit: learned something new: get_node() has identical performance with $ when it takes a constant NodePath. if it takes a string though, then its performance is much worse.

dalexeev commented 2 years ago

If this proposal was not about scenes, but about branches (all descendants of some node), then I could probably support it. But, by induction, unique names in a branch mean unique names in the entire tree.

I really think that it is a bad idea to somehow separate scenes in the tree. The scenes should completely dissolve into the tree, as it is now, there should not be special nodes in the tree.


More hyperlinks:

markdibarry commented 2 years ago

Rational 1): _find_node was removed to discourage bad practices._ Well that's not true because it wasn't removed, it was refactored to find_nodes.

This point is jumping to conclusions. While it's true find_node was removed to discourage bad practices, find_nodes doesn't restore those bad practices, because they do two different things and return two different results. find_node was doing a full search of the tree for one node, and due to the bad performance, as long as you're searching the entire tree, you might as well get a collection of nodes rather than just one. Basically, don't use a large brush to paint fine detail. The collection result also encourages the user to use it when they need a collection.

The scenes should completely dissolve into the tree

I'm not sure why the proposal needs to fulfill your "dissolve" analogy, unless there is some core-tenant of what the scene tree should be and cannot be written in the documentation that I totally missed (and I totally might have). Either way, I'm pretty sure that changing a node's color doesn't make it "dissolve" any better or worse than a CanvasLayer or Viewport.

I know we need to stop talking about node exports, but the reason I like this proposal is specifically because exports (while more dynamic) have been too fragile for me to rely on, and so even if we had it implemented, I very much still would need a solution similar to this.

dalexeev commented 2 years ago

I'm not sure why the proposal needs to fulfill your "dissolve" analogy

By "dissolving scenes in a tree" I mean that in the scene tree (in runtime) all nodes must be equivalent, there should not be special nodes. Yes, in the editor you split the tree into separate scenes, which you save in separate files. But after the scenes are combined into one tree, the scenes cease to exist, only the tree of nodes remains. At least that's what's happening now. Including when viewing a remote scene tree, scene roots and other nodes are not highlighted in any way.

This point may not seem very significant to you, but IMO it is very important and blocks this proposal. The fact that the tree can be edited at runtime (at runtime, you can add nodes to the tree that will not belong to the scene, and you can add them right in the middle of the scene) goes against the idea that scenes can stay in the tree at runtime. This proposal, if not completely introducing the idea, at least makes the root nodes of scenes special at runtime.

I know about the owner property, but I think we should avoid increasing its value for users. Many do not even know about the existence of this property, and this is actually a good thing. I would hate to see this complication added to one of the main Godot systems when there are other suggestions that solve this problem in the best way IMO.

The introduction of scene-relative addressing will make the owner property more important, which means at the very least an interface needs to be added to represent the ownership relationship (for example, in a view of a remote scene tree).

Now when you use get_node (or $), it behaves the same for both scene nodes and nodes added at runtime. This proposal introduces addressing depending on whether the node was added in the editor or at runtime. Inconsistency is bad.

dalexeev commented 2 years ago

Comment:

Yes, I read somewhere that the difference between get_node and $ is that in the second case, constant precompiled NodePaths are used, and in the first case, the String -> NodePath conversion is additionally performed each time.

Try also comparing with the get_node(@"NodePath") option (get_node(^"NodePath") in 4.0-dev).

Comment:

yes, get_node(^"NodePath") and $NodePath have exactly the same performance.

Riteo commented 2 years ago

I don't get nor agree with the general idea of adding a "quick path" system for the children of a node, but if this must be done, IMO it should not be scene dependent, instead allowing to set which node should keep it, with some sane default behaviour (eg. add "quick path" to the root node), as scenes are nodes AFAIK and I find that adding this extra distinction is unnecessary. Also, what if someone wants to set the "quick path" to another node without making a new scene? That's were allowing to set it to an arbitrary parent would make more sense to me. It could perhaps be implemented, UX wise, by using the active selection to set the rest of the selected nodes as "quick nodes".

markdibarry commented 2 years ago

@dalexeev Your critique of the proposal is much clearer to me now. So, for this functionality, the question we'd need to solve is: if the specific node has a list of nodes inherently associated with it, what happens when those nodes are moved out of the "scene" during runtime? Of course you have the references, and they'd still work, but it would be confusing to have nodes that are specifically associated with an ancestor and accessed through it that can be moved from one branch to another (or even become the parent itself). Of course, you'd need to understand what you're doing enough to get to that point and would be shooting yourself in the foot by using the wrong tool for the wrong job, but it's definitely a possibility, so it's worth saying it's an inevitability.

Funny enough, I didn't really see myself using this outside of a script's node reference setup, and not in the logic itself, so it feels like more of an edge case, but definitely one to consider and solve before moving forward. I imagine there are a lot of new users that use get_node() everywhere instead of assigning the result to a variable, so it's definitely a pitfall.

h0lley commented 2 years ago

Funny enough, I didn't really see myself using this outside of a script's node reference setup, and not in the logic itself, so it feels like more of an edge case, but definitely one to consider and solve before moving forward. I imagine there are a lot of new users that use get_node() everywhere instead of assigning the result to a variable, so it's definitely a pitfall.

what introduced you to the concept of having to do a "node reference setup"?

using $ within methods is entirely fine - not something like a beginner mistake.

markdibarry commented 2 years ago

@h0lley This seems off-topic, but I thought it was common to have a method or section in your script to get all your node references for use in the logic, rather than using $ or get_node() to perform the lookup each time? These longer chains and having to rely on NodePath was what this proposal hoped to make easier.

var myNode = get_node("nodeA/nodeB/nodeC")
var myOtherNode = get_node("nodeD/nodeE/nodeF")
pchasco commented 2 years ago

using $ within methods is entirely fine - not something like a beginner mistake.

Getting a little off-topic here, but hard coding node paths is a code smell. This is akin to the "magic string" antipattern. It is more maintainable to never repeat the path to the node in your code, but instead place it in a variable which is checked at compile-time.

Mickeon commented 2 years ago

I strongly feel about this proposal at its core being a more elaborate implementation of using a Dictionary of NodePaths and their onready referenced Nodes.

export var ui_paths :={
   "health_bar" = NodePath(),
   "mana_bar" = NodePath(),
   ...
}

onready var ui := {
    "health_bar" : get_node(ui_paths.health_bar),
    "mana_bar" :  get_node(ui_paths.mana_bar),
    ...
}

# Referring to them in code
ui.health_bar.value = 20
ui.mana_bar.max_value = 20

Once again and of course, if https://github.com/godotengine/godot-proposals/issues/1048 were to be integrated to somehow work with Dictionaries as well, declaring another Dictionary for just paths would no longer be necessary, it'd be rather tidy, as well as being pretty performant.