godotengine / godot-proposals

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

Add shorthand `expr.$NodePath` for `expr.get_node("NodePath")` #1776

Open dalexeev opened 3 years ago

dalexeev commented 3 years ago

Describe the project you are working on: A game

Describe the problem or limitation you are having in your project: When I want to create onready variables for nested nodes, I have to repeat the path prefix:

onready var item1 = $Scroll/Grid/Item1
onready var item2 = $Scroll/Grid/Item2
onready var item3 = $Scroll/Grid/Item3

Obviously, this is not good. For example, the path to the Grid node may change and must be changed in all variables.

I can replace this with

onready var grid = $Scroll/Grid
onready var item1 = grid.get_node("Item1")
onready var item2 = grid.get_node("Item2")
onready var item3 = grid.get_node("Item3")

But I would like to use a more compact syntax.

Describe the feature / enhancement and how it helps to overcome the problem or limitation: Add shorthand expr$NodePath for expr.get_node("NodePath"). Then we can do this:

onready var grid = $Scroll/Grid
onready var item1 = grid$Item1
onready var item2 = grid$Item2
onready var item3 = grid$Item3

or:

onready var grid = $Scroll/Grid
onready var item1 = grid.$Item1
onready var item2 = grid.$Item2
onready var item3 = grid.$Item3

Describe how your proposal will work, with code, pseudocode, mockups, and/or diagrams: I haven't looked at how this is implemented, so I don't know.

If this enhancement will not be used often, can it be worked around with a few lines of script?: It is possible to attach the script directly to the Grid node, but this option is not always acceptable.

Is there a reason why this should be core and not an add-on in the asset library?: This cannot be done with an add-on, because it affects the GDScript syntax.

YuriSizov commented 3 years ago

Not sure if that's the best possible syntax, but I agree with this being a problem.

Calinou commented 3 years ago

If we were to allow this, I think grid.$Item1 (with a .) would make more sense. That said, I can imagine this being difficult to parse.

KoBeWi commented 3 years ago

Obviously, this is not good. For example, the path to the Grid node may change and must be changed in all variables.

This is a matter of using Replace All, so it's not really a big problem.

dalexeev commented 3 years ago

it's not really a big problem

Yes, you're right, this is just syntactic sugar. But this is a logical continuation of the $NodePath syntax, which is also syntactic sugar.

Tyrannosaurus1234 commented 3 years ago

You can do $"parentNode/childNode" to provide the $ operator with a nodePath that includes slashes. Doing $parentNode/childNode, to me, sounds like asking for trouble from a syntax standpoint. To reduce boilerplate and increase portability in your paths, you can, theoretically, do something like:

onready var gridPath = "thisNode/grid"
onready var item1 = $gridPath+"/item1"
onready var item1 = $gridPath+"/item2"
onready var item1 = $gridPath+"/item3"
dalexeev commented 3 years ago

You can do $"parentNode/childNode" to provide the $ operator with a nodePath that includes slashes.

@Tyrannosaurus1234 You may not have known this, but it is not necessary, it even works without the quotes.

var x = "path/to/node"

# In 3.2 these options are equivalent:
$path/to/node
$"path/to/node"
get_node(x)

# In an alternate reality:
$"path/to/node"
$x
$(x)
$("path/to/" + "node")

Basically, I like the option with parentheses because it doesn't break anything.

var grid_path = "Scroll/Grid/"
onready var item1 = $(grid_path + "Item1")

But my option is still more concise:

onready var grid = $Scroll/Grid
onready var item1 = grid$Item1
Calinou commented 3 years ago

For reference, quotes with $ are only required for relative paths that use .. or absolute paths:

$"../Player"
$"/root/Game/Level"
vnen commented 3 years ago

For reference, quotes with $ are only required for relative paths that use .. or absolute paths

Or when the node name contains spaces or special characters (i.e. when it's not a valid identifier).

That said, I would prefer Calinou's suggestion of using node.$ChildPath.

dalexeev commented 3 years ago

That said, I would prefer Calinou's suggestion of using node.$ChildPath.

I'm satisfied with both options. If you think the second is better, I'll agree with you.

Bromeon commented 3 years ago

Is this a frequent use case?

Personally I've made the experience that most of the time, I'm requesting a node relative to the current one. If I wanted to access a sub-node of an unrelated node directly, it would often indicate a lack of encapsulation (my node needs to know the inner structure of the other node, and changing that breaks unrelated code).

dalexeev commented 3 years ago

Is this a frequent use case?

Quite often in UI scenes, because many intermediate nodes are used, especially containers. In other types of scenes, too, there are sometimes many levels of nesting. I don't use 3D, but I saw from the screenshots how many nodes people have.

Plus:

Yes, you're right, this is just syntactic sugar. But this is a logical continuation of the $NodePath syntax, which is also syntactic sugar.

h0lley commented 2 years ago

is this really purely syntactic sugar?

I grabbed https://github.com/Zylann/gdscript_performance added a couple more $ tests, ran it in the current alpha, and here is the ouput:

-------------------
The following times are in microseconds, for one occurence of a test.

For time: 0.035us
Running micro benchmarks...
get_node() where only one child...
    0.508us (1000000 iterations)
get_node() where only one child with $...
    0.165us (1000000 iterations)
get_node() where 10 children...
    0.522us (1000000 iterations)
get_node() where 10 children with $...
    0.167us (1000000 iterations)
get_node() 3 parents deep having 10 children...
    0.984us (1000000 iterations)
get_node() 3 parents deep having 10 children with $...
    0.206us (1000000 iterations)
-------------------
Saving results...
Done.

feel free to test yourself: project4.zip

as you can see, the $ versions have significantly better performance. I suspect this is due to the fact that $ does not take variable node paths, and so perhaps they are being indexed?

so my conclusion is unless node paths are variable, get_node() should generally be avoided, and so it is really unfortunate that right now, $ cannot be called on a node reference.

my_node.$ChildNode would be cool. if syntax highlighting still works with the node path portion being distinctly colored, I think readability will be just fine.

dalexeev commented 2 years ago

is this really purely syntactic sugar?

as you can see, the $ versions have significantly better performance.

I suspect this is due to the fact that $ does not take variable node paths

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).

Yes, it's not exactly syntactic sugar, but it's more shorthand and syntactic sugar than an optimization tool.

h0lley commented 2 years ago

didn't knew about the ^ thing! yes, get_node(^"NodePath") and $NodePath have exactly the same performance.

still very much in favor of this proposal though.

BaddRadish commented 1 year ago

two things here:

  1. $() could simply /not/ take indexed path, so that wouldnt break anything. in fact get_node("some_literal") should smartly index but oh well.
  2. $/path works fine for me, so really you only need quotes for ../ which i find deeply disturbing.
LuisCarli commented 1 year ago

The more I’m using Godot, the more I’m relying on nodes, get_node and on the $ shortcut.

I’ve tried to use variable.$NodeName before, because I thought $NodeName worked as if it was being replaced by a get_node(^”NodeName”) call when the script was parsed. So it should work anywhere a get_node call was being used.

I think this proposal is a logical generalization of the current $ shortcut. And I think the $ shortcut is useful enough for this generalization to be important.

h0lley commented 1 year ago

meanwhile scene unique nodes where introduced with their own shorthand, so one would now expect variable.%NodeName to work in the same way.

Jean-Low commented 1 month ago

This is useful syntax clarity for using heavy composition with nodes. I have a damage system that all creatures in my game have, this is represented by a node with the same script inside each creature scene. Using if(body.$DamageSystem): inside collision signals is really intuitive, instead i need to rely on get_node() for now.

crotron commented 1 week ago

I would also like to add a use case that I'm currently working with. I have a bunch of code in various places that is doing more or less the same bunch of function calls on the children of different child nodes. For example:

# Instance 1
$"Menu/Items".is_focused = false
$"Menu/Items/Clickbox".mouse_filter = MOUSE_FILTER_IGNORE
$"Menu/Items/Sprite2D".use_parent_material = false # etc

# Instance 2
$"Monster".is_focused = false
$"Monster/Clickbox".mouse_filter = MOUSE_FILTER_IGNORE
$"Monster/Sprite2D".use_parent_material = false # etc

(there are a bunch more lines similar in structure to the above that I removed for brevity)

I'm looking to refactor my code to implement the common behavior in a helper function, in the spirit of DRY. Something like the following:

func unfocus(thing):
    thing.is_focused = false
    thing.get_node('Clickbox').mouse_filter = MOUSE_FILTER_IGNORE
    thing.get_node('Sprite2D').use_parent_material = false #etc

I can then call it with thing = $"Menu/Items" for Instance 1 and thing = $"Monster" for Instance 2. This works, but because what I'm trying to work on is now a variable and not self, I can't call $ inside the helper function and must instead use get_node as above. I think that being able to use .$ in this context would improve readability.