godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.15k stars 97 forks source link

Implement `onready` setters #325

Open KoBeWi opened 4 years ago

KoBeWi commented 4 years ago

Describe the project you are working on: Game with "interactive" nodes, which use tool script with setters and stuff.

Describe the problem or limitation you are having in your project: Real use case here. I have a node called Door. It has two exported properties with setters: right and texture. When you set right to true, the sprite gets flipped, but there's also a collider that changes position to opposite side. texture is a... well, sprite texture.

The Door script is a tool script. I recently noticed that I can't just use _process() in every tool script that reacts to variable changes, because it kills my GPU due to constant scene redrawing. So I switched to setters and ran into another problem. As you might guess from above description, my tool script affects two underlying nodes by editing their properties. The problem is initialization. When I run the game, the setters are called instantly and error out, because they rely on using get_node() before the children are initialized. Right now I'm using a workaround where in the setter I check if the node is inside tree and return if not and also inside _ready() I call the setters directly with current value to initialize them properly. tbh it feels like a hack and also I will have to do this for every "interactive" object, which is meh.

Related issues: https://github.com/godotengine/godot/issues/30460 https://github.com/godotengine/godot/issues/28014

Describe how this feature / enhancement will help you overcome this problem or limitation: My idea is being able to defer the call of setters. Consider this syntax:

export var right: bool onready setget set_right

Notice the onready before setget (this might look ugly, but I don't have idea for better syntax rn). It means that when initializing the object, the engine should use call_deferred instead of call on the setter. This way it will be called after the node is inside tree and have its children properly initialized, so it can modify them.

Show a mock up screenshots/video or a flow diagram explaining how your proposal will work: image maybe

Describe implementation detail for your proposal (in code), if possible: Hopefully it should be possible to call the setter in deferred mode. However this might happen after _ready() (unless we use some new logic, but that makes it complicated), so maybe the property could have its value set bypassing the setter and then call the setter on idle frame. Sounds perfectly possible and solves the problem.

If this enhancement will not be used often, can it be worked around with a few lines of script?: It's a change to GDScript, so non-pluginable.

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

non-pluginable

willnationsdev commented 4 years ago

I have encountered this myself when writing tool scripts. It is definitely frustrating to have to deal with, and I wrote a hack very similar to yours.

Toshiwoz commented 4 years ago

EDIT: Hey, I did have also problems with setters getters execution order, and I actually have a doubt. If you write: export onready var right: bool onready setget set_right In my opinion should do what you mention.

I was trying that but it really does not work as I expect, I guess this feature is needed, also a tutorial or a good description+examples on the official docs would be helpful.

Calinou commented 4 years ago

I recently noticed that I can't just use _process() in every tool script that reacts to variable changes, because it kills my GPU due to constant scene redrawing.

This sounds like a bug to me. Redrawing should only be requested if the property's value actually changes. Can you post the script that triggers constant redrawing?

KoBeWi commented 4 years ago

This sounds like a bug to me. Redrawing should only be requested if the property's value actually changes. Can you post the script that triggers constant redrawing?

$Sprite.texture = texture
$Sprite.flip_h = !right
$Solid.position.x = abs($Solid.position.x) * (-1 if right else 1) #Solid is a StaticBody2D

And few similar others.

YuriSizov commented 4 years ago

I'm not sure what's the intended behavior here when working with such property right after it was supposedly set? There are two proposed ways to deal with setters: deferring the call to them internally or bypassing them entirely until the node receives an "idle_frame" signal. The first one means to me that if we try to access the value immediately, it's going to be stale:

some_node.right = true
print(some_node.right) # <-- Value did not visibly change

At least it's going to be stale until we enter the tree here. This is completely hidden from the user of the node, because the only indicator here is the new keyword inside the node's script. This would be extremely confusing to deal with.

The second one suggests that we store externally provided value without passing it through the setter and possibly sanitizing it? That's even worse, in my opinion. A setter function can do a million things alongside accessing child nodes, thus bypassing it completely is going to put any internal logic in there in danger. And here as well this behavior is not expected by the user of the node.

And the same problem applies to getters in both suggestions: are they bypassed as well until the node is inside the tree? They definitely cannot be deferred... In the end, to me these proposed changes allow for bad code to happen and it puts a lot of faith into the developer who creates these nodes.

We are talking about properties exposed at the top of some scene that abstracts a whole bunch of other nodes. Setters give you an opportunity to take care of these properties in a seamless way and handle various cases the end-user may not be aware about. Such as the node needing to be inside the tree to have the value applied. I don't see what's so hacky or limited about our current capabilities. Frankly, I do this all the time and don't feel like it's a hack:

onready var right : bool = false setget set_right

func _ready() -> void:
  _update_sprite()

func _update_sprite() -> void:
  if (!is_inside_tree()):
    return

  $Sprite.flip_h = !right

func set_right(value : bool) -> void:
  right = value
  _update_sprite()

I guess it's essentially the same as you've described your approach, but in some different words. Yet using this you can implement your deferred behavior right now, by deferring the call to _update_sprite, if that's what you want. This may be wordy, but it's robust. And does not cause any unexpected behavior on exported/public properties.

Please correct me if I've misunderstood what's on the table here.

Shadowblitz16 commented 3 years ago

I honestly think onready, and _enter_tree and _exit_tree should be made obsolete. I don't see a point in having the ability to do things before the tree is ready you can literally do everything in ready

Instead make everything run once the tree is completely ready and then add a _removed hook and removed signal for when nodes are removed or queue_freed

Calinou commented 3 years ago

@Shadowblitz16 _exit_tree() is useful to run code when your project exits. For instance, this can be used to save the current game or user configuration automatically:

extends Node

func _exit_tree():
    # Assuming we have a Settings singleton with a "save" method:
    Settings.save()
Shadowblitz16 commented 3 years ago

I mean a _removed would be the same thing wouldn't it? the only difference is it would be ran after the whole tree is ready

red1939 commented 3 years ago

Wouldn't it make sense to treat all properties as onready? Currently, it seems that the setget is ran before _ready so accessing any nested nodes will fail (as probably everyone knows). If we were do delay them, no ifs would be needed.

dalexeev commented 3 years ago

@red1939 No, this is an artificial limitation, especially if you remember that there are scripts that are not inherited from Node.

nathanfranke commented 3 years ago

3.x

Workaround for Godot 3.x which I've used over 20 times so far:

export var my_var: int setget _set_my_var
func _set_my_var(value: int) -> void:
    my_var = value

    if not is_inside_tree():
        yield(self, "ready")

    # Custom behaviour

Note that this doesn't guarantee the node is ready, only inside the tree, since if you call this from _enter_tree, is_inside_tree will be true.

Edit: This version solves the previous problem, albeit a bit more verbose:

onready var _is_ready := true

export var my_var: int setget _set_my_var
func _set_my_var(value: int) -> void:
    my_var = value

    if not _is_ready:
        yield(self, "ready")

    # Custom behaviour

Edit 2:

4.0

@export var my_var: int:
    set(value):
        my_var = value

        if not is_inside_tree():
            await ready

        # Custom behaviour

I suggest that if for whatever reason this feature isn't desired, we should at least add an is_ready() method. This will fix the issue I noted above and also make the await code way more readable.

cgbeutler commented 3 years ago

For folks using @pycbouh or @nathanfranke's solution, be aware that you can accidentally double-save data if you aren't careful and use it to set fields exported by child nodes. When you do export var foo setget set_foo, the export still creates/uses a getter to save stuff to disk, even though you didn't put one there.

I recommend actually opening your tscn files when creating exports in order to make sure you are saving what you think you are. Doing so has saved me lots of mistakes. Those files are not as scary as they sound. If you open the scene for the example export above, you'll see it saved the data like so:

[node name="Node2D" type="Node2D"]
script = ExtResource( 1 )
foo = 3                            <---- got exported

So if you export a var, then forward that to a child's exported value, you will see it saved in both places. When the scene is later loaded, it will set it in the child, then again in the parent, lengthening load time and your total file size for no reason.

Sadly, there isn't a quick way to export without PROPERTY_USAGE_STORAGE right now. Upvote https://github.com/godotengine/godot-proposals/issues/2539 if you'd like it 😉. To get "no storage", you'd have to export the long way using _get_property_list(). Though, if you are sharing data across the tree, I'd recommend a Custom Resource instead. They are tricky, but they are the mechanism designed for cross-tree data.

YuriSizov commented 2 years ago

@vnen Can you give us an opinion here? The problem is likely real, but is that the best solution? Maybe with more modern GDScript syntax we can provide a better approach?

KoBeWi commented 2 years ago

A better syntax would be maybe

@onready @export var something

and the variable would be initialized like onready variable. Right now when doing this it doesn't affect when the setter is called.

LakshayaG73 commented 2 years ago

Hi all.

Any idea how to circumvent this?

Is the best solution still to use:

if not is_inside_tree(): await ready

inside setter functions?

KoBeWi commented 2 years ago

Yes.

OscarPindaro commented 2 years ago

I'm having the same identical problem. I have a Sprite node with an exported variable called collision_visible that handles the visibilitty of a child node that contains various collision shapes. I use a setter that allows to change the value of the visibility without having to interact with the children of the Sprite node. A tool script is attached to the sprite node.

export(bool) onready var collision_visible : bool = false setget set_collision_visible, get_collision_visible

func set_collision_visible(new_value : bool):
    collision_visible = new_value
        get_node(COLLISIONS_NODE_NAME).set_visible(new_value)

As you can see, the default value of collision_visible is false, and if i set it to true through the game engine interface, the setter method is called before the child collision node exists, causing an error. This is VERY not intuitive. I think that it's pretty normal to expect that setters will try to access lower level nodes and their property. I don't think that is reasonable to expect that for each setter one should implement a series of checks that control if the child nodes are present in the tree or not.

feelingsonice commented 1 year ago

Hey will this be supported in the 4.0 release or nah?

YuriSizov commented 1 year ago

No. This is an open proposal, that has not been worked on, let alone merged. And it still has open questions about its implementation.

sairam4123 commented 1 year ago

Workaround for Godot 3.x which I've used over 20 times so far:

4.0

@export var my_var: int:
  set(value):
      my_var = value

      if not is_inside_tree():
          await ready

      # Custom behaviour

I suggest that if for whatever reason this feature isn't desired, we should at least add an is_ready() method. This will fix the issue I noted above and also make the await code way more readable.

There is is_node_ready() function right?

This would become:

4.0

@export var my_var: int:
    set(value):
        my_var = value

            if not is_node_ready():
            await ready

        # Custom behaviour
dalexeev commented 1 year ago

See also:

ttencate commented 1 month ago

Great trick, @sairam4123! In C#, we can't use await in setters, but we can use an extension method on Node like this:

    public static async void InvokeOnceReady(this Node node, Action action) {
        if (!node.IsNodeReady()) {
            await node.ToSignal(node, Node.SignalName.Ready);
        }
        action.Invoke();
    }

Usage:

    private Color _color;
    [Export] public Color Color {
        get => _color;
        set {
            _color = color;
            this.InvokeOnceReady(() => GetNode<Polygon2D>("$Polygon2D").Color = Color);
        }
    }