godotengine / godot-proposals

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

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

Open aaronfranke opened 4 years ago

aaronfranke commented 4 years ago

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:

76044779-4e4a2000-5f29-11ea-96c3-161619caa301

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:

76044779-4e4a2000-5f29-11ea-96c3-161619caa301

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.

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.

EchoingLogos commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

To be a bit more clear on the idea, I wrote up a proposal: https://github.com/godotengine/godot-proposals/discussions/7572