Emit "Resource.changed" for custom Resources

mattismoel commented 2 months ago

Describe the project you are working on

A driving game, where a script is responsible for creating a RoadLayout (Custom Resource) in the inspector. Other nodes should be able to listen to changes to any property on the layout.

This is very much related to #10263, though implementation details differ.

Describe the problem or limitation you are having in your project

Currently, one would have to create setters for each exported Resource variable, that emits the signal itself. This is very cumbersome, if many variables are exported, and need to emit this signal.

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

Emit the Resource.changed signal for any Resource - also custom Resources.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

From a custom Resource:

# some_custom_resource.gd
class_name SomeCustomResource
extends Resource

@export var var1: Type
@export var var2: Type
@export var var3: Type

The resource is exported from some script

## exporter.gd
extends Node

@export var resource: SomeCustomResource

func _ready() -> void:
    resource.changed.connect(func(): 
        print("changed")
        print(resource.var1)
        print(resource.var2)
        print(resource.var3)
    )

When any property on the exported SomeCustomResource is changed in the editor or through code, the changed signal is emitted.

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

This can be worked around by doing setters for each exported member:

# some_custom_resource.gd
class_name SomeCustomResource
extends Resource

@export var var1: Type:
    set(v):
        var1 = v
        changed.emit()

@export var var2: Type
    set(v):
        var1 = v
        changed.emit()

@export var var3: Type
    set(v):
        var1 = v
        changed.emit()

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

Improves workflow tremendously, and is part of GDScript itself. It also avoids boilerplate and code repetition.

Mickeon commented 2 months ago

Emitting the changed signal under the hood for every change can lead to big performance concerns (even if the signal is not connected to anything), and seemingly unpredictable, "quirky" behavior. This is mostly because what would happen would be different than the written code. As an existing example, var my_node = $MyNode does not automatically assign the node on _ready. Instead, you need to use the @onready annotation, which keeps the intent more obvious without "compiler magic".

sockeye-d commented 2 months ago

This may not be the best way to do it but maybe if there was a Resource-specific @emit_changed annotation that could be applied to exported variables like this:

# some_custom_resource.gd
class_name SomeCustomResource
extends Resource

@emit_changed @export var var1: Type
@emit_changed @export var var2: Type
@emit_changed @export var var3: Type

As a workaround, I think

func _set(property: StringName, value: Variant) -> bool:
    if property in ["var1", "var2", "var3"]:
        emit_changed()
    return false

should work

Mickeon commented 2 months ago

See also:

In a nutshell, something similar to @observable with a custom signal name would also solve this proposal, because changed could then be passed directly and be emitted accordingly.

WhalesState commented 2 months ago

@emit_changed @export var var1: Type

I think annotations should work with all classes starting from Object.

Edit: "@onready" can only be used in classes that inherit "Node"

So maybe this will be possible but it may be better to use only one annotation for both of them, @notify_changed as a separation for the below exported variables.

This may be useful to avoid creating setters for all the variables that calls one method, and we replace the method with a connection to changed.

Something like this can be written in just 7 lines.

@export var start_position := Vector2(4, 4):
    set(value):
        if value != start_position:
            start_position = value
            emit_changed()

@export var end_position := Vector2(32, 32):
    set(value):
        if value != end_position:
            end_position = value
            emit_changed()

@export_range(0.0, 1.0, 0.01) var opacity := 1.0:
    set(value):
        if value != opacity:
            opacity = value
            emit_changed()

@export var stroke_color := Color(1.0, 1.0, 1.0, 1.0):
    set(value):
        if value != stroke_color:
            stroke_color = value
            emit_changed()

@export var stroke_width := 4.0:
    set(value):
        if value != stroke_width:
            stroke_width = value
            emit_changed()

@export var stroke_linecap := LineCap.BUTT:
    set(value):
        if value != stroke_linecap:
            stroke_linecap = value
            emit_changed()

@export var stroke_dash_array := PackedFloat32Array([0]):
    set(value):
        stroke_dash_array = value
        emit_changed()

# This doesn's call `changed`.
@export var my_int: int

# This calls `changed`.
@notify_changed
@export var start_position := Vector2(4, 4)
@export var end_position := Vector2(32, 32)
@export_range(0.0, 1.0, 0.01) var opacity := 1.0
@export var stroke_color := Color(1.0, 1.0, 1.0, 1.0)
@export var stroke_width := 4.0
@export var stroke_linecap := LineCap.BUTT
@export var stroke_dash_array := PackedFloat32Array([0])

FabriceCastel commented 1 month ago

@WhalesState emitting a broad "something in object X changed" from an object instead of a more precise "Y property in object X changed" makes very little sense to me. Typically different listeners will care about different subsets of properties (eg. object X is a player character, then the HP UI would care about listening to properties related to health) and it would be better practice to connect listeners only to the specific properties they should care about.

Typically in observable patterns, there's a way to group multiple observables together to effectively do what you're suggesting, but I don't think that setting up a broad "something changed in this object" should be the default/out of the box way that it works.

godotengine / godot-proposals