Exported variables break setget methods that are "ready" dependent

[henriiquecampos]

Godot version: 3.1.6

OS/device including version: Solus Gnome 4.0

Issue description:

Alright, this sounds kinda complex, but the issue is simple, so I'll give context first.

I have this script:

extends Node

export (NodePath) var node_path = ".."
onready var node = get_node(node_path)

export (bool) var enalbed = true setget set_enabled

func set_enabled(enable):
    enabled = enable
    node.set_process(enabled)

The thing is, if I change the Enabled value in the inspector, the set_enabled method crashes when running the scene, because seems like the method is called before the _ready callback, or at least before the onready variable being set. So it reports that node is null, therefore it is unabled to call set_process.

I think that this could be fixed by changing the order in which setget methods are called, making them wait for idle process at least?

Steps to reproduce:

1. Create a Node
2. Add a child Node
3. In the child Node add a script
4. Copy paste the script above mentioned
5. In the inspector set Enabled to false
6. Test the scene

[Zylann]

This has been the case since forever, it's not a bug. The rule is, properties of a node with USAGE_STORAGE are always set right when it is deserialized for instancing. There is no simple way to defer this to the moment it is added to the tree: unless I'm missing something, you could do something like var scene = packed_scene.instance(), but add_child() might be called later, or never. This applies to every node, scripted, or builtin. It means you can't implement the setter like you did, you have to check if the node is in tree first, and initialize in _ready as well. _ready is the deferring mechanic.

[henriiquecampos]

So, basically, the rule is = don't use exported and setget for setter/getter methods that are instance dependant?

[KoBeWi]

Wouldn't this work in that case?

func set_enabled(enable):
    enabled = enable
    yield(get_tree(), "idle_frame")
    node.set_process(enabled)

[Zylann]

@KoBeWi get_tree() might return null since the node is not yet in the tree.

[KoBeWi]

Ah, right. An ugly hack would be adding a Timer node and waiting for a very short timeout, but I doubt you'd need to do this THAT much.

[Zylann]

@KoBeWi won't work either because Timer works using _process and _process is not called when the node is not in the tree ¯\_(ツ)_/¯
Really all you can do in such case is set the variable, eventually modify the current node, but anything external to self needs to wait for _ready() or _enter_tree().
It's not even guaranteed that the user of that node will add it to the tree. And after that occurs it can still be removed and re-added several times.

[henriiquecampos]

@KoBeWi get_tree() might return null since the node is not yet in the tree.

Exactly what happened.

[santouits]

func _ready():
    node.set_process(enabled)

[KoBeWi]

won't work either because Timer works using _process and process is not called when the node is not in the tree ¯_(ツ)/¯

Yeah, but it will work AFTER it's added to tree. The idea is basically to wait until everything is set up. set_process is useless outside tree anyways.

EDIT:
Or what @santouits wrote plus this:

var ready

func _ready():
    node.set_process(enabled)
    ready = true

func set_enabled(enable):
    enabled = enable
    if ready:
       node.set_process(enabled)

[henriiquecampos]

func _ready():
    node.set_process(enabled)

This breaks the idea of the setter, since it is meant to keep both nodes in sync whenever one changed the other should change as well, not only when one is ready. I think that what I can do is to instead of setting this directly, connect a signal and emit this signal when this enabled setter is called.

Basically my script is:

func set_enabled(enable):
	.set_enabled(enable)
	
	if enable:
		emit_signal("started")
		platform_actor.velocity = Vector2.ZERO
	else:
		emit_signal("finished")

So in my specific case I think I can connect the started signal to the platform_actor.set_velocity method passing Vector2.ZERO as extra argument. But well, this is a very ad hoc solution if you asked me.

[henriiquecampos]

won't work either because Timer works using process and process is not called when the node is not in the tree ¯(ツ)/¯

Yeah, but it will work AFTER it's added to tree. The idea is basically to wait until everything is set up. set_process is useless outside tree anyways.

EDIT:
Or what @santouits wrote plus this:

var ready

func _ready():
    node.set_process(enabled)
    ready = true

func set_enabled(enable):
    enabled = enable
    if ready:
       node.set_process(enabled)

There is the Node.is_inside_tree() method for that, this fixed the problem by the way!

func set_enabled(enable):
        .set_enabled(enable)
	if not is_inside_tree():
		return
	
	if enable:
		emit_signal("started")
		platform_actor.velocity = Vector2.ZERO
	else:
		emit_signal("finished")

[bojidar-bg]

I usually do this in my scripts like so:

export var x = 5 setget set_x

func _ready():
  set_x(x)

func set_x(new_x):
  x = new_x
  if is_inside_tree():
    # do something, like:
    $sprite.frame = x

Note that #6491 will help remove a few lines of that boilerplate, even more if it is made to work with onready nicely.

[Xrayez]

For this I figured I should update such properties deferred most of the time:

extends Node

export (NodePath) var node_path = ".."
onready var node = get_node(node_path)

export (bool) var enabled = true setget set_enabled

func set_enabled(enable):
    enabled = enable
    call_deferred('_update_properties') # updated on idle (next) frame

func _update_properties():
    node.set_process(enabled)

The above mechanism may cause _update_properties called multiple times in a single frame. Here's more a sophisticated and optimized mechanism if you need to change properties that update often (picked this up from engine internals):

extends Node

export (NodePath) var node_path = ".."
onready var node = get_node(node_path)

export (bool) var enabled = true setget set_enabled

var _update_queued = false

func set_enabled(enable):
    enabled = enable
   _queue_update()

func _update_properties():
    node.set_process(enabled)
    # might as well do more stuff
    _update_queued = false

func _queue_update():
    if not is_inside_tree():
        return
    if _update_queued:
        return

    _update_queued = true

    call_deferred("_update_properties")

[DleanJeans]

What I always do is

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

This will delay the setters instead of returning and not setting anything at all.
Still I don't like typing it out every time.

[henriiquecampos]

Seems like this isn't a bug, just a general misunderstanding that is quite logical and the engine provides tools to achieve the desired behaviors quite simply.

I'll close it.

[cgbeutler]

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

Warning: Doesn't work right in tool scripts, as 'ready' is... off.

I was just playing around with this in a tool script and it has a big flaw. The below script will highlight the issue:

tool
extends Control

onready var crect := $ColorRect2

export var crect_color :Color = Color.green  setget set_crect_color
func set_crect_color( value :Color ):
	crect_color = value
	print("pre check")
	print(get_signal_connection_list('ready'))
	if not is_inside_tree():  yield(self, 'ready')
	print("post check")
	print(get_signal_connection_list('ready'))
	crect.color = value

Put that on a Control node with a ColorRect child node and set the exported variable in the editor. Then when you edit/save the tool script you will see that the ready signal keeps getting more and more binds with "pre check" printing way more often than "post check".

Edit: This may be related to the fact that the script is erroring out. Seems like if I change crect.color ... to $ColorRect2 ... then I don't see the memory leak. I'll dig a bit more and maybe open an issue if I can narrow it.

Edit Edit: Looks like it's only an issue cuz the tool script keeps the old object around if there's an error. As soon as you clear up the error, the old obj is deleted and the leaked signals along with it... pretty sure. As for the onready vars, that's more this issue: #16974

[nathanfranke]

Two simple workarounds: godotengine/godot-proposals#325 (comment)