Make the $ operator in GDScript cache node references (or add a $$ operator)

[aaronfranke]

EDIT: If #1048 is implemented, it may make this proposal obsolete.

EDIT 2: As @h0lley noted, #1048 doesn't actually make this proposal obsolete. It provides a clean alternative but it still results in boilerplate and a lack of green syntax highlighting unlike this proposal.

Describe the project you are working on: https://github.com/godotengine/godot-demo-projects/ but this applies to any project made in GDScript.

Describe the problem or limitation you are having in your project:

Here is a code snippet from Dodge the Creeps:

The problem is that this code looks elegant, but it isn't very performant. Every time this method is called, it has to call get_node() 6 times, and 2 of those times are getting the same node (HUD). We can reduce this from 6 to 5 by caching the HUD reference like this:

However, this method is still very inefficient, since it calls get_node() 5 times every time the method is ran. The best simple option that I can see is to use onready var like this:

This means that get_node() is only called 5 times once when this node is created, and this method does not have to call get_node() at all when the method is ran. However, now this code is ugly, since it is much longer than it could be and we've lost the green syntax highlighting in new_game() that tells us we're working with node children.

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

The proposal is to make the $ operator cache references on ready, so that user code looks like the top image, but behaves like the bottom image.

• For projects currently caching references via onready var, this would make code more elegant.

• For projects currently using $ everywhere, this would increase performance.

In cases where get_node() needs to be called every time, such as if nodes are created and deleted, users can just use get_node() instead of $. As such, $ would have different behavior from get_node().

Another option, described below, would be to add a $$ operator with this behavior.

Describe how your proposal will work, with code, pseudocode, mockups, and/or diagrams:

See above for examples of user code, but @vnen would be the one handling the implementation.

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

It can be worked around, but this is something that will be used often.

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

Yes, because it will be used often.

[nathanfranke]

A few notes/questions:

1. This would technically break compatibility so any PR should definitely note that.
2. By how much does the performance improve with this workflow?
3. This actually makes a lot of sense since it is extremely unlikely to do this with dynamically added nodes like $Node@@2. The $ should be used for accessing nodes that are already in the scene before ready.

[groud]

I don't think it is a good idea. Some users might rely on dynamically changing sub-nodes, so this behavior would lead to undefined references.

The thing is that I am not even sure this would lead to a significant performance improvement for several reasons:

1. internally the get_node functions uses NodePath (a list of StringName) instead of Strings, so everything is using reference and the string is not re-analyzed everytime,
2. accessing the node following the path is a very fast operation, which should not be long to execute at all. Most of the time it's a matter of iterating over the few components the path contains
3. adding a cache system requires checking when the cached values becomes invalid, as we don't want the engine to crash if a reference becomes wrong. This requires additional processing which might not be worth the time gained by caching the node.

Anyway, before trying to improve performance in a given part of the code, we have to make sure this is a bottleneck for some users, or that the gain will be really significant for everyone. So unless someone make a benchmark proving that this part of the code is slow, it's not worth making the engine code significantly more complex.

[Zireael07]

I store node references in vars and it does make a performance difference, at least in functions that are running often (e.g. physics_process and process() ).
I don't have a benchmark on hand, though.

[groud]

I store node references in vars and it does make a performance difference

If you access the node several times per frame it surely likely does. But I believe such case is an optimization to be done on the user's side, as he can know if the node is eventually going to be replaced or removed from the hierarchy during the processing.
I believe the most common usage is to access a node once per frame, so in such situation I am not sure is it makes a big difference

[vnen]

I feel this is too much magic. If the script is long, you're now adding a lot of stuff on the user's _ready() function behind their back. While it's hard to make it too slow, there is an added penalty. Maybe the user don't have a _ready() function at all and will be wondering why the profiler says there is some time spent in _ready().

And the more obvious: what do you do when a node is freed? Because if you call get_node() you'll get null but if you are using a cache then you just have pointer to an invalid object.

Also, the user might remove a node and replace with another using the same name. If you keep a cache, then the $Node shorthand will only point to the old node.

It's also possible that you make some cases worse. If the user is using _ready() to connect signals:

func _ready():
    $Button.button_up.connect(_on_Button_button_up)

Now it has a cached value that will never be used again.

Of course we can be clever about usage, but I do think this will lead to bugs into the user code that will be hard to pinpoint. The only way to get around those would be using get_node() instead of $. What would happen is that the shorthand would get bad rap and people would stop using it, making this optimization pointless.

---

If we are going to be clever about usage, then we can consider adding warnings on cases where this is used inside functions called often. Like this:

func _process(delta):
    $Sprite2D.rotate(PI * delta) # Warning: Consider caching the reference to "$Sprite2D" to avoid getting the node from tree every frame.

[Zireael07]

Hm, good point. Could we get a caching equivalent of get_node() (cache_get_node() or some such), or would that require a new shorthand?

[aaronfranke]

And the more obvious: what do you do when a node is freed?

As stated, if you are working with a non-static node tree you would just use get_node() instead of $. I really don't see the problem, since in most cases I've seen, $ is used to access nodes that don't change, and you can just use get_node() otherwise.

[groud]

As stated, if you are working with a non-static node tree you would just use get_node() instead of $.

You are talking about the user point of view, but the problem is not there at all.

What is difficult to do is implementing the caching system while guaranteeing that the reference is reset to null when the node is replaced, moved or removed from the tree. This would require adding checks in every move or delete operation to make sure that we cannot have a wrong pointer somewhere in the code (as the engine should not crash at all). This will, as a consequence, slow down those other operations.

[aaronfranke]

If a node is able to be replaced, moved, or removed, users should use get_node() instead of $. It would be documented that $ doesn't work for nodes that can be replaced, moved, or removed. This way it is clear to users who would need to call get_node() every time that a method is being called, and would make the more common case where $ is used faster.

@vnen: you're now adding a lot of stuff on the user's _ready() function behind their back.

Conversely: Users who might assume $ has this behavior have get_node() called behind their back.

[groud]

I think you don't get the point. This does not changes anything. Even if the user know that, the engine should not crash at all. And this is not negotiable.

Even if the user is not supposed to move or remove the node because "it's not supposed to work", we still have to add checks to simply avoid crashes. Checks that are going to slow down everything else.

[aaronfranke]

If that is true, wouldn't crashes also exist with onready var x = $X? I haven't noticed any. EDIT: This is both a reply to @groud and @vnen when @vnen stated this:

And the more obvious: what do you do when a node is freed? Because if you call get_node() you'll get null but if you are using a cache then you just have pointer to an invalid object.

[groud]

Ah yes you are right, I did not think about that. I am not sure about how this is handled in the engine code, it's true that variable referencing nodes should not cause the engine crash too.

[nathanfranke]

onready var x = $X

would translate to

onready var internal_x = get_node("X")
onready var x = internal_x

[aaronfranke]

@nathanfranke I meant onready var x = $X with the current behavior.

[Zylann]

I don't really like this either, notably because caching is only valid if you know your branch will be static. It might not crash if you just move the node elsewhere in the hierarchy so the ref is still valid, but then the code no longer makes sense. Now from my experience, static nodes is very often the case, and I use onready caching pretty much all the time (which is why at first it sounds like a good idea). However I'm not only doing it for performance: in my mindset, references to external stuff (assets, directories, other scripts, node paths) must appear on top of the script, and only once, instead of being spread and repeated throughout the script. It also makes it easier to re-arrange the tree or rename nodes.

onready var x = $X

would translate to

onready var internal_x = get_node("X")
onready var x = internal_x

Besides, I wouldnt want this to happen, it would make my use case worse. I don't like this kind of magic. Other than that, I feel I'm not the target audience :p

[Mantissa-23]

Here's a thought:

var x = $X - Maintain current behaviour so as to not break compatibility

var x = $$X - Cache variable implicitly. Any time $$X is accessed again, the cached reference will be used.

This doesn't break compatibility, adds a shorthand that both new and experienced developers can take advantage of, and still allows for developers to explicitly decide whether or not they want a node cached.

[nathanfranke]

My proposal which also endorses better code readability with multiple scripts and nodes

vnen has a good point I think there are better alternatives.

[aaronfranke]

I still stand by my original proposal. For any given edge case where the proposed behavior of $ is not desired, you can just use get_node() instead. We can break compat in Godot 4.0.

[Zylann]

@aaronfranke I want to use $ because it makes query faster (better loading and instancing times), but I don't want it to dupe all my onready vars in which I already do caching.

[vnen]

My proposal which also endorses better code readability with multiple scripts and nodes

# shorthand for onready var label = $Label
some_keyword label, "Label"

That's not much different of what you can do today:

@onready var label = $Label

Edit: just noticed the "shorthand" comment. It's not much of a shorthand if you need to type almost the same amount of tokens.

[BeayemX]

I don't see the problem with storing everything in variables. I always store nodes in variables in onready. In my opinion it makes the code cleaner. You see all node-dependencies in one place.
And if the hierarchy changes you have one place where you can adjust everything.

When using the $ throughout your code, you have to find every instance where you used it and change it, which is quite error prone.

Edit:
And you also have full control if you want to cache it or if you want to overwrite it at runtime.

[aaronfranke]

@BeayemX You can just do onready var x = get_node(@"X") in your case with this proposal. EDIT: Added @

[Zylann]

@aaronfranke and that's precisely not good, because get_node() has to build a nodepath to do this, while $ makes it at compilation time, in addition to be shorter to write. And it's been that way since it was introduced, I don't want to revert everything back to get_node if $ still duplicates vars in this use case.

[aaronfranke]

@Zylann Interesting, I didn't know that. You can just do const x = NodePath("X") to make a compile-time NodePath, then get_node(x). The use case you are describing can be worked around with lines at the top of scripts just like the use case I'm describing with static nodes, but personally I have never had a need to have a compile-time NodePath with a dynamic node, so I think it's a less common case. For example, look at the demo projects.

EDIT: Just use onready var x = get_node(@"X"), the @ sign means NodePath literal.

EDIT 2: In Godot 4, use ^ instead, like ^"X".

In the end we both want better performance. Storing references to non-changing nodes is arguably the most common case, which is why I want $ to do that. I don't want to have many lines at the top of my script, and lose the special green syntax highlighting, if I can avoid it. I want the most common operations to be high-performance and elegant.

[Zylann]

You can just do const x = NodePath("X") to make a compile-time NodePath

And write even more code, sorry^^"

I have never had a need to have a compile-time NodePath with a dynamic node

The case I described isn't dynamic, and I use it in basically every script that accesses child nodes, in all my projects (I'm not the only one doing that). Also demo projects aren't representative of that.

In the end we both want better performance

In my case (and some others) this is not just about performance. I also want more maintainable code through decoupling, which happens to be done by explicit caching for fixed nodes, and const paths for dynamic ones (very rare, tho, I give you that). Which means implicit caching gets in the way by either duping vars needlessly, or forcing to downgrade my code and write more lines than before. The idea of auto-caching $ comes from good intention, but I can't agree to have it because of that use case conflict. That's encouraging an existing pattern by stepping over another existing pattern, and I don't quite like the idea that a cached state is implicitely being written to a script wihout using var.
Maybe if you can guarantee member vars assigned from $ won't get a dupe, that would solve it, thought it's exceptional behavior which you could only guarantee if these vars were marked read-only.

[aaronfranke]

Maybe if you can guarantee member vars assigned from $ won't get a dupe

That should be possible to implement if the only usage is in an onready var, but I think that $ would still have to make a duplicate variable if there is also other uses of it elsewhere in the script. This should be implementable without any look-ahead if done right.

Also, if removing the existing behavior of $ is deemed unacceptable, I would be fine with @Mantissa-23 's suggestion of $$, but do note that I would end up using $$ basically everywhere and it does seem a bit weird to make the more common operation longer in characters. It would be like JavaScript's ===.

[Mantissa-23]

I agree it's a bit weird but maintaining backwards compatibility is important. Breaking backwards compatibility too frequently can bleed users pretty easily. Breaking compatibility like that might make sense for Godot 4.0, but I also think a lot of people are expecting 4.0 to be largely a "backend" upgrade that just makes their game better without really affecting the way they make it.

[PetePete1984]

I vaguely recall var x = @"/some/node" being usable in place of var x = NodePath("/some/node"), so maybe onready var x = get_node(@"/some/node") is the secret sauce?

[nathanfranke]

I wonder if it would be a better practice to encourage users to use export nodes (once they are supported) rather than hard-coding node names.

export(Label) var my_label

func _process(delta):
    my_label.text = "Bad FPS Estimation: " + str(1/delta)

[HeartoLazor]

A change I would like to propose to this system is cache the $ calls BEFORE ready time, so when you instance the node, the $ calls are already cached and available in init or other before ready method.
With this approach the user can preinstance the levels in a menu thread for example and move heavy processing algorithms that requires child node references before ready.
I already did something similar as optimization using NOTIFICATION_INSTANCED.

[Calinou]

I think #3695 is a better way to address the problem at hand for two reasons:

• It can add type hints for nodes automatically, further improving performance thanks to GDScript 2.0's typed instructions. $ or $$ will never be able to do this on its own, simply because the node type can't be known at compile-time (or even checked for actual presence). However, the editor side does know this when performing autocompletion.
• It avoids some usability pitfalls by making caching an explicit action rather than an implicit one.

Most programming IDEs that deal with importing packages (Python, Java, PHP, …) also have implemented a similar kind of autocompletion.

[Zylann]

#1048 is another option which doesn't even need to hardcode the path in a script, on top of being meant to run at instancing time.

[FlykeSpice]

This is part of GDScript compile time optimization, it doesn't makes sense for me add a different operation just to cache the node because this feature proposal is chiefly improving the behavior of an existing operator($).

However, there may be corner cases where the compiler can't resolve to cache a node (and it can be cached but the code flow is difficult to compute) and the user want to explicit cache it, so in that case it makes sense to add the explicit $$ cache operator.

Therefore, for me the best solution is: Enhance the gdscript compiler to cache $ node references where it can resolve and add the explicit $$ operator that [tries to] caches the node regardless whether the compiler optimized it and throw an error if it wasn't possible.

[h0lley]

alright I just read through the entire thread and while the critique made sense to me, I did not see a single comment arguing that $$ was a bad idea.

absolutely; let the programmer decide where it makes sense to use the current behavior and where caching would be useful. but that doesn't mean we can't have this.

it seems like there are two camps of us:
those who like to declare a lot of node references as onready, and those of us who prefer skipping this and making use of $ instead. personally I belong to the second camp as I dislike having to add a property to class scope for every node I am going to use in code (or even two variables when the reference is derived from an exported NodePath). I am also quite fond of the green syntax highlighting communicating clearly that we are dealing with a node. but this shouldn't be about which one of these two patterns is superior, of course. let's support both, please.

by the way, here is an up to date performance test I just ran in the current alpha:

# 1 mil iterations each
236ms # direct node reference access
278ms # $NodePath
278ms # get_node(^"LiteralNodePath")
533ms # get_node("NodePathAsString")

source

extends Node

const ITERATIONS = 1000000


func test(code: Callable):
	var time_before = Time.get_ticks_msec()

	for i in ITERATIONS:
		code.call()

	var time_taken = Time.get_ticks_msec() - time_before

	print(str(time_taken) + "ms - " + str(code.get_method()))


@onready var node = $Node2D

func access_node_reference():
	node.position.x = randf()

func use_get_node_dollar():
	$Node2D.position.x = randf()

func use_get_node_with_str():
	get_node("Node2D").position.x = randf()

func use_get_node_with_np():
	get_node(^"Node2D").position.x = randf()


func _ready():
	print("Running tests with " + str(ITERATIONS) + " iterations each...")

	test(access_node_reference)
	test(use_get_node_dollar)
	test(use_get_node_with_str)
	test(use_get_node_with_np)

	await get_tree().create_timer(0.5).timeout
	get_tree().quit()

as you can see, performance of get_node() with a string passed is poor, however with a constant NodePath (which is always the case when using $), it's pretty good. an argument can be made that this proposal isn't needed as performance is already good enough. I think it would still be a nice to have though.

as for the type hint, couldn't it added implicitly the moment the caching happens?

edit: also while I support #1048, I don't agree that it renders this proposal obsolete. again, I don't want to declare variables at class scope unless I have to and I have no desire to expose anything other than things that make sense as configuration to the inspector.

[aaronfranke]

as for the type hint, couldn't it added implicitly the moment the caching happens?

The caching happens at runtime, since the script can be used in multiple places and may have different children.

[dalexeev]

alright I just read through the entire thread and while the critique made sense to me, I did not see a single comment arguing that $$ was a bad idea.

There is the following comment and I agree with it:

You guys know how I think about these things, I think that most of the time, explicitness is better, and things that hide what happens under the hood to the programmer in a non-obvious way, even if it means writing a bit less code, is generally bad.

If something is used frequently, then you need to store it somewhere (in a variable). This is a universal solution that works not only with Node references. $$ is too narrow a solution, which is also confusing.

$$ is purely for optimizations. A similar situation was with iniline (#3760). It seems to me that at the language level there should not be things intended only for optimizations.

278ms # get_node(^"LiteralNodePath")
533ms # get_node("NodePathAsString")

In theory, we can make the GDScript compiler smarter and optimize the get_node(<constant string expression>) case. This optimization will not add confusion.

[h0lley]

while I think it's sufficiently explicit when people learn about $$ as "the $ with caching", I see that it's probably not worth it since in your own words:

This proposal is 100% beneficial only for people coding without onready declarations,

the thing is we have no straight forward way of implementing our own explicit caching without using onready.
$$ would essentially be an onready for people preferring $.

in that sense, it can be seen as more of an syntactic sugar thing than a performance thing.

In theory, we can make the GDScript compiler smarter and optimize the get_node() case. This optimization will not add confusion.

had the same thought. it's a waste that get_node() calls that have no variable component passed into them perform so poorly because the user did not think about explicitly using the literal node path syntax. after learning about this thanks to your comment yesterday in the node$path proposal, I had some searching & replacing to do.

[smeans]

I'm completely new to Godot, so I'm trying to decide if I want to be in the onready/get_node or $ camp. My first concern was the performance implication of the $ operator, so I found this thread. I understand not wanting to break backwards compatibility, but what if there were a source file or (preferably) project-level directive to enable caching (like "option explicit" in JavaScript)? It would let those who know what they're doing take advantage of caching right away without breaking old projects. Then, after a few releases the default value of the option could be flipped to cache by default.

[Calinou]

I understand not wanting to break backwards compatibility, but what if there were a source file or (preferably) project-level directive to enable caching (like "option explicit" in JavaScript)?

We prefer not making GDScript behavior dependent on directives by this (or worse, project settings). Scripts may be copy-pasted from a project to another, and this should never cause them to break because a project uses different settings from another.

That said, breaking compatibility for 4.0 is not a concern as compatibility was already broken there a long time ago.

[h0lley]

EDIT: If #1048 is implemented, it may make this proposal obsolete.

I disagree that #1048 makes this proposal obsolete.

I thought this was at least in part about reducing the ugly boiler plate code of onready declarations. having to declare a bunch of @export var instead doesn't help with that.

However, now this code is ugly, since it is much longer than it could be and we've lost the green syntax highlighting in new_game() that tells us we're working with node children.

or did you perhaps mean to link the unique node proposal instead? that would make a bit more sense, but still, performance of %Node and $Node are currently identical, so no, still not really addressed.

[EchoingLogos]

An argument that I'd like to stress further in favor of this is in terms of code readability. Currently, using $ or get_node() to access nodes provides the benefit to the reader of immediately knowing that the object being referenced is 1. A Node 2. Somewhere in the child hierarchy 3. How far down in the child hierarchy it is being directly proportional to the length of the path. This is a ton of information provided to the reader thanks to the use of $ or get_node().

In the case of onready var, the user is now forced to cache the node into a run-of-the-mill variable indistinguishable at a glance from other global scope variables losing the previously described advantages, unless the user specifically breaks variable naming styles to obtain these advantages. This is run of the mill for programming, and there's "no reason" why I should expect one type of variable to be highlighted like this; but the way things are in Godot, I find myself using $ not because I don't want to cache the node, but simply because it makes the code far more readable.

I acknowledge that altering behavior for $ would break compatibility in very obscure ways amongst other issues, but I strongly support the implementation of $$ as a cached $.

[nlupugla]

I think Godot does have a genuine need for more static scene information, but I don't think this is the way to accomplish it.

The better way to solve the issues of node caching is actually with a change to the editor, not with GDScript. The idea would be the editor lets you mark nodes in a scene as static. Then $my_static_node gains several benefits.

1. If the node doesn't exist in the scene tree, the code will highlighted in red at compile time.
2. The node will be typed.
3. The node will be cached.

Furthermore, this approach has no risk of breaking compatibility because all nodes would be marked as dynamic (ie: non-static) by default.

[aaronfranke]

@nlupugla Keep in mind that a script may be used for multiple scenes, so there is no way to find the static typing information ahead of time. Well, it can be done just in the script editor (it already does this for autocomplete hints, but doing it in GDScript in general at runtime is tricky.

@h0lley You are correct. To clarify, with exporting node references, that is a feature I personally would use over the $ or $$ operators, but cached $/$$ would still be a nice feature to have. (I say "would use" because at the moment exporting node references is buggy so unfortunately I do not use it yet).

[nlupugla]

Fair point about a script being reused for multiple scenes. I think there are multiple good workarounds though. For example, there could be a way to declare at the top of a script what scene it is statically attached to.

extends Node2D in "res://my_scene.tscn" on "%MyNode2D"

[dalexeev]

For example, there could be a way to declare at the top of a script what scene it is statically attached to.

See #1935. But I'm still skeptical about static node validation. The node can still be deleted at runtime, its name and/or path can be changed.

[nlupugla]

Thanks for linking to that proposal dalexeev :)

In my mind, the whole point of marking a node as static within a scene would be that any attempts to delete, rename, or move it would fail at compile time. The node would be treated like a const.

[dalexeev]

In my mind, the whole point of marking a node as static within a scene would be that any attempts to delete, rename, or move it would fail at compile time.

This is difficult or even impossible to guarantee. You can always cheat the static analyzer, sometimes unintentionally. Or the forbidden operation can be performed by external code or the engine.

var t: Variant = static_node
t.queue_free()

[nlupugla]

Fair enough. The program could abort upon trying to delete a static node though, if static analysis isn't enough to catch everything right?

By the way, I don't mean to digress too far from the original topic. On the subject of cached node references, my main problem with the idea is that it makes a bunch of implicit assumptions about the structure of your scene, whereas the static scene approach makes these assumptions explicit. In both approaches, there will be a problem if the node in question moves.

[nlupugla]

To be a bit more clear on the idea, I wrote up a proposal: #7572

[MadeScientist]

Personally, I adhere to the GDScript Style Guide and use an underscore prefix combined with PascalCase to distinguish node reference variables from regular variables. I'm not sure if others appreciate this style, but if it's possible to implement highlighting for _PascalCase variables in green, it would be quite good for me.

Like this:

[Zireael07]

This is about $ and/or caching, what does PascalCase have anything to do with it?

[MadeScientist]

This is about $ and/or caching, what does PascalCase have anything to do with it?

Sorry for my misunderstanding.