godotengine / godot-proposals

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

Add `Node.get_unique_node()` as a way to get Scene Unique Nodes #5048

Open Mickeon opened 2 years ago

Mickeon commented 2 years ago

Describe the project you are working on

A Typical 2D Godot game, dipping my toes with Scene Unique Nodes.

Describe the problem or limitation you are having in your project

Scene Unique Nodes are a positively received feature, but I do personally find the way this feature is accessed in via code to be potentially troubling in the long run:

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

Add Node.get_unique_node(). This function would behave exactly like $%Node or get_node("%Node"), but what lacks in conciseness makes up for readability. It's much, much more self-explanatory, especially when utilised in other languages Godot supports. But as for GDScript, like "$" is to get_node(), the previously mentioned "$%" syntax could be said to be the shorthand of get_unique_node().

get_unique_node() should support autocompletion, and its description should serve as plenty of space to summarise what Scene Unique Nodes are and how to access them.

Despite the criticism, the current proposal is not about removing the "%" accessor/operator and everything related to it, internally. As such, get_unique_node() would practically prefix the NodePath with "%", or something along those lines.

Although potentially controversial, I would love if get_node("%Node") were to be replaced by get_unique_node("Node"). But unfortunately, it would be a rather drastic change at this point in time. As such, this may not be a breaking change and can be considered later down the line, as well as being portable to 3.x.

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


var sprite := get_unique_node("Sprite")

func _ready():
    var other_node := sprite.get_unique_node("OtherNode")

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

One could manually add a similar function to their own Node, but for aforementioned reasons, it's not what the proposal would want to accomplish.

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

Node is a core class.

RedMser commented 2 years ago

I personally don't see the need for this new method, as you most often can work with the %Node syntax instead of resorting to get_node("%Node").

Also it would probably require a corresponding get_unique_node_or_null() variation of the function.

I would be fine with a different symbol than % in GDScript, however I don't see this happening any time soon.

Although potentially controversial, I would love if get_node("%Node") were to be replaced by get_unique_node("Node"). But unfortunately, it would be a rather drastic change at this point in time.

Be aware that you currently can call get_node("abc/%def/ghi") to quickly dig into another scene's unique node hierarchy, which would then have to be replaced with the much longer get_node("abc").get_unique_node("def").get_node("ghi") if we did not have the % prefix syntax in node paths.

MewPurPur commented 2 years ago

It's a bit confusing, it feels as though % is part of a name in some scenarios when used for scene-unique nodes. I'm in favor of an alternative way.

I also had an idea for slightly adjusting the highlighting color of parts of noderefs that aren't part of names (so $, %, /, and maybe even ". So it's easier to read the node names themselves.

Mickeon commented 2 years ago

I personally don't see the need for this new method, as you most often can work with the %Node syntax instead of resorting to get_node("%Node").

Other languages that Godot also supports should be taken in account. Taking C#, for example, their only option would still be to "resort" to getNode("%Node").

I would be fine with a different symbol than % in GDScript, however I don't see this happening any time soon.

I have an idea cooking which is going to be controversial, but I'll give my best shot on in the upcoming day.

Be aware that you currently can call get_node("abc/%def/ghi") to quickly dig into another scene's unique node hierarchy, which would then have to be replaced with the much longer get_node("abc").get_unique_node("def").get_node("ghi") if we did not have the % prefix syntax in node paths.

This is true. However, and be it a matter of personal taste and habit, I do find that the most important Nodes are typically referenced at the beginning of the Scripts, as variables already. This is ideal, as they act as a general list of dependencies the Script relies upon to function properly. Thus, the simplest, likely more common way to dig down a hierarchy of important Nodes is by typing a much more familiar syntax, such as var node = abc.def.ghi. It's really worth thinking about how common a long chain of unique accessors ("%") may actually be. It can be argued that sacrificing "typing" speed over clarity, readability and unambiguity is beneficial for the integrity of the language, in the long run.

seppoday commented 2 years ago

I personally don't see the need for this new method, as you most often can work with the %Node syntax instead of resorting to get_node("%Node").

For me this is more reasonable:

get_node("Node")
$Node

get_unique_node("Node")
%Node

Than this:

get_node("Node")
$Node

get_node("%Node")  - (it is kinda mix betwen method and syntatic sugar)
%Node

That is why I totally agree with that idea.

h0lley commented 2 years ago

Be aware that you currently can call get_node("abc/%def/ghi") to quickly dig into another scene's unique node hierarchy, which would then have to be replaced with the much longer get_node("abc").get_unique_node("def").get_node("ghi") if we did not have the % prefix syntax in node paths.

that's true, but I think the usefulness of this feature is questionable in the first place, or if it should be encouraged to write paths with nested unique nodes like that at all. as @Mickeon already pointed out, a property referencing the node in a scene's root can and probably should be used instead. I think it wouldn't be a significant loss if it were to be dropped if it meant more uniform usage like in the example @seppoday gave one comment above.

I would be fine with a different symbol than % in GDScript,

yea, whenever I put % as a shorthand for getting an unique node it looks messy to me - $%Node looks especially unwieldy. plus, as OP points out, % already has several other special uses in GDScript like string formatting - both as a operator as well as a placeholder.

next, I feel that the fact that there's currently two variants of the shorthand for no apparent usability reason - % and $% - adds to why this feature feels so unrefined. I definitely see users being confused like "when $%Node does this, then what does %Node do??".

instead of introducing an entirely new shorthand character, I believe we should just stick to $ as the universally understood shorthand for getting a node, and that this character should take a second modifier character to indicate that we are not just getting any node, but an unique node. so that would bring us back to $%Node - but with % replaced for something that's more pleasant to the eye.

here's an idea I posted in the original PR:

$*MyNode would read as follows: $ - get a node * - by unique MyNode - name

this was initially liked, but then was criticized for being easy to confuse with the wildcard * in a string passed to find_node(). with this proposal however - get_unique_node("Node") replacing get_node("%Node") - that would no longer be an issue.

as unique nodes are a kind of "favorite" nodes, the star would fit, and I hope it's not just me that feels that this is significantly more pleasant to read.

however I don't see this happening any time soon.

huh, why is that? nobody is supposed to be using this feature yet in any sort of serious project. changing it would not be a breaking change (breaking changes are between stable versions). so if a good argument can be made, changing the symbol shouldn't be a big deal.

KoBeWi commented 2 years ago

nobody is supposed to be using this feature yet in any sort of serious project

How naive. I'm using this in production in a released game since the day it was backported xd 3.5 stable is around the corner, so if this is changed in 4.0, it will cause disparity in a recently added feature.

That said, there was a long discussion before the feature was merged and lots of bikeshedding about the symbol, but nobody proposed a better alternative. It's kinda too late to discuss that now, % is going to stay.

btw it's possible to autocomplete unique nodes without any dedicated method (just as editor is able to suggest partial paths), there is even a proposal for it: #4593

h0lley commented 2 years ago

How naive. I'm using this in production in a released game since the day it was backported xd 3.5 stable is around the corner, so if this is changed in 4.0, it will cause disparity in a recently added feature.

not naive, I entirely forgot that it was backported, my bad.

% is going to stay.

should you really speak with that kind of authority?

as said already, I did propose a better alternative early on in the PR, which was liked too, but because of the wildcard thing (which btw I never understood why that was a good argument as % pretty much suffers from the same) it wasn't considered.

both in terms of source code that needs to be adjusted (I believe there's a constant for the shorthand character that can easily be adjusted), as well as in terms of user code that needs to be adjusted (one quick replace all), the change should be rather trivial, so what exactly is the huge blocker here?

plus, I'd pose the portion of Godot users who already started actively using unique nodes is likely minuscule.

KoBeWi commented 2 years ago

Keep in mind that the % symbol is used also in %Node shorthand and as an icon in scene tree dock, so any replacement needs to fit everywhere. *Node doesn't look as good IMO; it's also kind of similar to a pointer, if you know C++ 🤔

should you really speak with that kind of authority?

My point is that it's easier to decide something before it's implemented than change it afterwards. The feature is new, so it's probably easier, but still there needs to be a good reason for the change. If a replacement gets considerable support, it can be done, but most users will just use what is available and don't care if it's % or *. btw symbol change should be discussed in a new proposal.

h0lley commented 2 years ago

Keep in mind that the % symbol is used also in %Node shorthand

yes everything I wrote was about exactly that ^^'

*Node doesn't look as good IMO

it would be $*Node, the second variation without the $ would no longer be a thing. the point was that just like with the change of the methods proposed by OP, the shorthand too should be more uniform if the feature is to feel solid in stable.

but most users will just use what is available and don't care if it's % or *.

I fail to see how that's relevant at all. when users don't care then those who do should decide / be heard all the more.

btw symbol change should be discussed in a new proposal.

right, although unfortunately I really don't feel like putting in the effort now for a separate proposal after having gotten shut down like this, especially since it's apparent my comment hasn't actually been read attentively.

YuriSizov commented 2 years ago

@h0lley I think you've mistaken KoBeWi's intent. He wasn't trying to shut down the discussion, but was pointing out that this has already been discussed at length on and off GitHub before the feature was implemented, so it's very unlikely that we're going to change it again quickly after committing to it.

We can revisit the syntax in the future, and for 4.0 it would've been relatively safe to change still. But it's better to give it some time so we have more feedback from other users. Also, the backport to 3.x makes it harder to arrange a change without diverging the branches too much for a new feature that is supposed to be added for parity. We can't as easily change it in 3.5 anymore, and making a change in 3.6+ would likely be a bad idea too.

AttackButton commented 2 years ago

Just saying that after the https://github.com/godotengine/godot/pull/61440 solution and as @KoBeWi said, seeing the % icon in the scene tree dock and in green color in the script editor is very clean, I don't think this symbol needs to be changed anymore. The editor auto-completion makes this very easy to follow too.

But a method to get all the unique nodes from the scene can also be very useful.