When using a setter for a field, if the backing field is only updated after a signal, then values are not correct

[ntsik]

Godot version: 3.1.2.stable.official

Issue description: This one's a bit convoluted, so I think it's easier to describe via an example.

Say that we have two scripts, Parent and Child. Both of them have an exported field, Parent.parent_value := false and Child.child_value := false, and we want to keep them in sync. (Parent essentially exposes the public API for this scene.)

Parent.parent_value has a setter. This setter does not set the backing field directly and instead sets Child.child_value with the new value. Child then sends a signal back up to Parent that its child_value has changed. Parent then from the signal callback sets its backing field from Child.child_value.

When Parent.parent_value is set to true in the scene and then launched, this value saved in the scene is not propagated to Child.child_value as expected. (The scene-saved value is passed through in _ready.)

Additionally, if both Parent and Child are set as tool scripts, then when setting Parent.parent_value to true in the scene and the scene is closed and reopened, the value is not saved in the scene at all. (It's unclear if these two scenarios are the same bug or even related, so I didn't bother opening two issues.)

Steps to reproduce: This one's probably easier to look at the repro project.

Minimal reproduction project:
test.zip

[Zylann]

I believe it's not a bug.
First of all, if you want to use a signal to do this, you have to use tool, otherwise your code won't run.

Now that tool is there, another problem comes in:

func _ready() -> void:
	var error := Child.connect('child_value_set', self, '_on_child_value_set')
	assert(error == OK)
	self.parent_value = parent_value

When adding your node to the scene or opening it, this will create a connection to the child node.
However, node connections are persisted. If you save the scene, it will be saved with that connection. The next time you open the scene, _ready will be called again as expected, but the connection already exist, so an error will occur, and self.parent_value = parent_value will not run. You should then check if the signal exists with is_connected.

There is a third problem:
When the scene is loaded, exported properties are set before your node is added to the tree. This means, indeed, the child node is not accessible yet:

func _set_parent_value(new_parent_value: bool) -> void:
	# Uncommenting this makes it work, yes.
	# but it should be fine as long as the values were in sync
	# when the scene was last saved.
	# The scene loader will have the same value to set in the child node,
	# because you decided to export it too.
	parent_value = new_parent_value

	if Child != null:
		Child.child_value = new_parent_value

---

About the present use case, here is a personal suggestion: if the goal is to expose the child API in the parent node as a shortcut, you don't need a signal to make this work. This should be enough:

# Parent.gd

tool
extends Node2D

export(bool) parent_value setget set_value, get_value

onready var Child = $Child

func get_value():
	if Child != null:
		parent_value = Child.child_value
	return parent_value


func set_value(v):
	if Child != null:
		Child.child_value = v
	parent_value = Child.child_value

However the fact this would cause a duplication of the value in the scene file (and duplication of setter calls) may not be ideal, and it can be fixed using property hints. There is no reason to save the parent value (because the child already is), and no reason for it to even have a backing field. Unfortunately there is no "easy" way to specify hints in GDScript (see #20318), but there is a way to create such binding:

tool
extends Node2D

onready var Child = $Child

func _get_property_list():
	return [
		{
			# This creates a property which shows up in the inspector,
			# but is not actually saved in the scene and has no backing field.
			"name": "parent_value",
			"type": TYPE_BOOL,
			# `export` is equivalent to PROPERTY_USAGE_EDITOR | PROPERTY_USAGE_STORAGE.
			# Since all we want is to expose a shortcut, we can omit storage.
			"usage": PROPERTY_USAGE_EDITOR
		}
	]

func _get(property_name):
	if property_name == "parent_value":
		# No need to check for existence anymore,
		# because it will only be accessed in inspector
		# once the node will be in the scene already.
		# The child node BECOMES the backing field,
		# it is then always in sync.
		return Child.parent_value

func _set(property_name, value):
	if property_name == "parent_value":
		Child.parent_value = value

[ntsik]

Using tool isn't necessary to experience this bug, but it does make the behavior slightly different.

Without tool, if you launch the scene with Parent.parent_value = true set in the editor, I would expect that after _ready then Label.text == 'True'. However, it always seems to be 'False' instead, indicating that setting the value from _ready is not working quite right.

With tool, the above also happens, but also Parent.parent_value's saved value is lost when closing and reopening the scene.

---

Are you sure that connections are persisted? I'd expect that they're persisted if they're declared in the editor, but my tests indicate that connections set through code are not persisted in the scene.

if !Child.is_connected('child_value_set', self, '_on_child_value_set'):
	print('connecting') # Always prints
	var error := Child.connect('child_value_set', self, '_on_child_value_set')
	assert(error == OK)
else:
	print('already connected') # Never prints

Whether the script is tool or not, we never hit the already-connected code path. Regardless of adding this check, the behavior still remains the same for me.

---

This repro project is a greatly simplified version of the original version. In the original, I'm instancing scenes and changing their properties before they enter the SceneTree. So the original code is essentially doing:

var parent_scene := Parent.instance()
parent_scene.parent_value = true

add_child(parent_scene)

Doing this means that we have to wait until _ready to propagate to Child, which is why the code in the repro project is structured as such.

This also complicates the example given where we use a getter. I could use a getter, but IMO it's actually bad practice in this instance as calling the getter before adding to the SceneTree and after can produce different results, especially since in the original Child can clamp values before setting its field. (Plus, I personally find it a pain to have to remember to call self.parent_value every time rather than just parent_value.)

---

I wasn't aware of _get_property_list or that it could work like that. Thanks for the tip - I'll definitely use that in the original!

[Zylann]

Without tool, if you launch the scene with Parent.parent_value = true set in the editor, I would expect that after _ready then Label.text == 'True'. However, it always seems to be 'False' instead, indicating that setting the value from _ready is not working quite right.

If your goal is to keep the two values in sync, then there is a small problem from the beginning: your scene begins with the values NOT in sync.

• parent is true
• child is false

I added prints to double-check, and here is what happens when the scene is instanced:

Parent setter called with value True, child is Null. Backing field is not set and remains False
Parent _ready is called, calling its own setter to False
Parent setter called with value False, child is [Node2D:1168]. Backing field is not set and remains False
Child value is set to False, emitting signal
Parent reacts to child and sets backing field to False

So in this design, it sounds like the parent node is the one "owning" the true value the node should have (important concept, single source of truth). So the backing field of the parent must be set, otherwise you are loosing the information that it is initially set to True.

---

Are you sure that connections are persisted?

My mistake, I'm confusing this with a slightly different situation. Signals are not persisted unless they are flagged as such. Ignore what I said about it.

[ntsik]

Ah you're right, that was the root issue. It's obvious looking back on it now but if Child is not yet ready, such as when scene values are set, then we have to save those values on the Parent otherwise they are lost.

Here is the correct code:

func _set_parent_value(new_parent_value: bool) -> void:
	if Child == null:
		# If Child is not yet ready,
		# then persist parent_value so that
		# it can be set correctly in _ready
		parent_value = new_parent_value
	else:
		Child.child_value = new_parent_value

Thank you for your help and sorry for the false report!