godotengine / godot-proposals

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

Add `@observable` annotation for properties in GDScript #4867

Open nathanfranke opened 1 year ago

nathanfranke commented 1 year ago

Describe the project you are working on

This would be very useful for my music player since I basically had a signal for every one of my properties which triggered UI updates

Describe the problem or limitation you are having in your project

https://github.com/godotengine/godot/issues/6491

This is cumbersome:

signal volume_changed(volume)

var volume := 0.5:
    set(value):
        volume = value
        volume_changed.emit(volume)

signal speed_changed(volume)

var speed := 0.5:
    set(value):
        speed = value
        speed_changed.emit(speed)

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

Add an annotation that automatically generates a listening signal

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

# Implicitly generates volume_changed and speed_changed signals.
@observable(volume_changed) var volume := 0.5
@observable(speed_changed) var speed := 0.5

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

Used often, and the workaround is many lines of repeated code

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

GDScript is core


If you have annotation keyword suggestions, let me know and I will set up a vote.

Currently what needs to be discussed is:

Mickeon commented 1 year ago

I like the idea a lot but I don't know how complex it'd be to implement, in practice. Plus, not convinced on the name of the annotation itself.

me2beats commented 1 year ago

Add an annotation that automatically generates a listening signal

If the observable property is x then will the signal named x_changed be generated?

Mickeon commented 1 year ago

Perhaps something like this could be done, to make it more flexible while still reducing the boilerplate code and making it more explicit.


signal volume_changed
signal speed_changed

observe(volume_changed) var volume := 0.5
observe(speed_changed) var speed := 0.5
me2beats commented 1 year ago

Perhaps something like this could be done, to make it more flexible while still reducing the boilerplate code and making it more explicit

What makes it more flexible tho

Also observe(prop) looks a bit strange for gdscript syntax (I mean calling a function outside of _ready() or any other function)

2plus2makes5 commented 1 year ago

What about something like this?:

export var x:=10.0 emits("x_changed")

otherwise:

export var x:=10.0 emits_change_signal #or some other keyword

and then it emits a signal simply called "x" like the variable.

YuriSizov commented 1 year ago

An annotation with a signal name would probably work just fine and should be relatively easy to implement. The name is needed to avoid naming conflicts and have some point of reference in code. But we probably don't need to create any signal separately, so it can just be

@keyword(signal_name) var property

I agree that this pattern is sometimes useful and does require boilerplate currently to work around.

nathanfranke commented 1 year ago

Should the signal be called if the property is set, but not modified (e.g. running x = 1.0 twice)? My vote is to not call the signal, but it does mean an extra check...

Edit: This will also prevent the multiplayer synchronizer from emitting the signal every time an update happens (which might not modify the value)

Diddykonga commented 1 year ago

I like this a lot, provides for a common use case with little boilerplate and its an annotation so it doesn't eat up namespace, although a small change that would make this a lot more useful:

@obvservable(v_c) var v

func _v_c_callback(old_value, new_value): #provide the old_value, as well as the new_value, so independent lambdas/methods are easier to make
    #code

v_c.connect(_v_c_callback)
Diddykonga commented 1 year ago

Should the signal be called if the property is set, but not modified (e.g. running x = 1.0 twice)? My vote is to not call the signal, but it does mean an extra check...

I think that is preferable, although the annotation could potentially take a bool, as a second parameter, which could default to true for checking?

nathanfranke commented 1 year ago

Another idea, what if observable will also notify on array and dictionary changes? There is currently no clean way to do this (implement your own array class, poll changes every frame, manually call the signal)

nathanfranke commented 1 year ago

Can we have the signal be implicitly generated if no argument is passed?

signal x_changed(x)
@keyword(x_changed) var x: int
@keyword var x: int

I understand 1) it will need a bit more work and 2) the signal's existence isn't immediately clear BUT 3) Let's be honest, what pecentage of usages are going to be varname_changed? 99%? Why should we force the user to repeat the variable name in two places and also the variable's type when typed signals are added.

YuriSizov commented 1 year ago

the signal's existence isn't immediately clear

That's a good reason why, in my book. I don't think you need to explicitly add a signal beforehand and then reference it, but I think we should require a name to be passed to the annotation itself, which would implicitly create the signal but with a clear intent instead.

nathanfranke commented 1 year ago

I don't think you need to explicitly add a signal beforehand and then reference it

Why not? I don't think passing an argument to an annotation is much more clear than the default annotation with no arguments. I feel like by the same logic, the user should be required to explictly declare the signal and pass it to the annotation.

Edit: Perhaps the editor could help out? (Edit again: Personally not even a fan of this) image

YuriSizov commented 1 year ago

Well, to me passing a name argument is good balance between being explicit where it matters and not being too verbose 🤷

dalexeev commented 1 year ago

Let's be honest, what pecentage of usages are going to be varname_changed? 99%? Why should we force the user to repeat the variable name in two places and also the variable's type when typed signals are added.

For the same reason that you must write the argument name in the setter:

var my_var:
    set(value):
        # ...
        my_var = value

Even if in 99% the name is value, the presence of a name helps to understand what is happening, makes it more obvious, removes the "magic".

Edit: Perhaps the editor could help out?

Everyone uses different editors. The code is read not only in the editor (for example, on GitHub).

nathanfranke commented 1 year ago

Okay, I am happy with @observable(signal_name) var ..., and I admit @observable by itself is VERY implicit. Still not entirely convinced that we shouldn't require the user to declare the signal, though:

by the same logic, the user should be required to explictly declare the signal and pass it to the annotation

Mickeon commented 1 year ago

I was struggling to re-find this Proposal again, perhaps the title should be changed to note the annotation?

YuriSizov commented 1 year ago

I was struggling to re-find this Proposal again, perhaps the title should be changed to note the annotation?

What exactly do you have in mind? The main keyword here is "observable" whether it is with the annotation syntax or not. The OP body is also included in the search, btw.

Mickeon commented 1 year ago

"Add @observable annotation for properties in GDScript" I thought right off the bat, but you are right.

lostminds commented 5 months ago

I agree with the original issue and would like to see something like this. Perhaps a different approach could be to instead add additional getter/setter-style syntax to define what signal should be emitted for the variable?

For example:

signal volume_changed(volume)
var volume:float = 0.5:
    set(new_volume):
        if (new_volume != volume):
            volume = new_volume
            volume_changed.emit(volume)

Could then be:

signal volume_changed(volume)
var volume:float = 0.5:
    signal_on_change=volume_changed

This would work similar to how you can use set=setter_function syntax-wise. While not quite as short as the original @keyword annotation it would simplify it a lot, retain existing signal definition syntax and allow setting multiple vars to emit the same signal on change.

Another thing I'd like if something like this was introduced would be to distinguish between emitting a signal on changing the variable (as in new value is different, like above) and emitting the signal on setting the variable (as in just assigning a value, even if it's the same). Both have their usecases, and ideally I'd like to then see both a signal_on_change and signal_on_set syntax.

crystalthoughts commented 5 months ago

I also would like to see this. The obvious analogy to draw here is Excel. You are building a dependency graph on variables that trigger updates automatically, which has been vastly popular in that realm and feels very natural. I would imagine it would reduce bugs as you don't have to keep track of where you are pushing updates. Also consider immediate mode guis.

m21-cerutti commented 3 months ago

The use case describe is exactly the same as mine (issue mentionned above), I have make a workaround with the add_user_signal but would be so much convenient to have the @observable annotation. For the name however we could have something like:

@onchanged("optional_name_signal_changed") @onset("optional_name_signal_setted") var property

We use like that the onready syntax, and is more explicit than observable (https://docs.godotengine.org/fr/4.x/tutorials/scripting/gdscript/gdscript_basics.html#onready-annotation)

But I would very like then to just call it like property_changed.connect(...) or optional_name_signal_changed.connect(...) (depending on if you put a signal name or not, could be also directly an optionnal signal as parameter but would be redundant)

mihe commented 3 months ago

I made an attempt at implementing this (godotengine/godot#90071) in hopes of at least getting the ball rolling here.

It's not very rigorously tested or anything, but it does seem to work.

Let me know what you think in the PR.

(Naming and all that is of course still up for debate, along with any other bikeshedding.)

rune-scape commented 2 months ago

maybe to avoid the problem with naming, since the name of the signal isnt rlly important a function to fetch the signal internally:

@observable
var health: float = 100.0

func _ready():
    onchanged(health).connect(something)
    onchanged(FMOD.master_volume).connect(something_else)

-- @observable would silently create a special signal named @health.changed -- onchanged() is a special func that understands the syntax of the arg and errors if its not a GDScript member variable marked as @observable -- onchanged() would just be listed in the documentation of @observable and vice versa

seems easy enough to read even if it looks a little magical, but its kinda a magic feature

onchanged() could actually exist without the annotation even and just silently create the signal and add the check at runtime,

Cryszon commented 2 months ago

onchanged() could actually exist without the annotation even and just silently create the signal and add the check at runtime, seems easy enough to read even if it looks a little magical, but its kinda a magic feature

This reminds me of reactivity in frontend JS frameworks like Vue where you just use ref(myVariable) (or reactive(...)). Whether it is feasible to implement or even needed in Godot is another question. Simple @observable annotation already goes a long way.

theraot commented 1 month ago
FabriceCastel commented 1 month ago

In a lot of cases, the pattern my code follow is roughly:

  1. declare some var foo in one class and a signal on_foo_update(foo)
  2. define a setter for foo that emits on_foo_update when a DIFFERENT value gets assigned (ie. it does a == check and just omits any assignments that re-assign the current value)
  3. in some other class, in the _ready() function I'll connect some callbacks to on_foo_update and also call them once, passing in the current value of foo

The GodotSx library offers an.. okay ISH solution to this problem, but ultimately I ended up rolling my own set of IntObservable, FloatObservable, StringObservable..... classes to wrap that behaviour, like:

class_name IntObservable extends RefCounted

signal on_change(val: int)

var value: int:
  set(new_val):
    if value != new_val:
      value = new_val
      on_change.emit(value)

func _init(val: int) -> void:
  value = val

func observe(callable: Callable, emit_on_connection: bool = true) -> void
  callable.connect(on_change)
  if emit_on_connection:
    callable.call(value) # or whatever the syntax is, I don't remember off the top of my head

and then to use it,

val something := IntObservable.new(10)

# ....elsewhere

func _ready() -> void:
  something.observe(on_some_update)

func on_some_update(foo: int) -> void:
  print("foo is: " + str(foo))

This is how I hook up basically every bit of UI; I want to have it set its initial value and update itself anytime the underlying state changes, and this does that as simply as possible. BUT... it kinda blows, because I need to define a new class for EACH type I want to be able to use this with, if I care at all about type safety (which I do), and that's a lot of annoying boilerplate.

It would be a huuuuuuge improvement if it were possible to do:

@observable var my_integer: int

# ...elsewhere

func _ready() -> void:
  something.observe(on_some_update)

func on_some_update(foo: int) -> void:
  print("foo is: " + str(foo))

With @observable essentially being a shorthand to handle defining that entire per-type class definition I need to currently roll by hand.

I do want to stress that fundamentally, an "observable" value is NOT THE SAME as connecting to a signal. For any value that you want to listen to changes for, you pretty much always want to invoke the listener for every change, and also, once during the initial setup/connection with the initial value. It's not the same pattern as listening to the signal of a button click, or some other user interaction.

I feel like this is a long shot, but having this functionality out of the box would be sooooooooooooo nice, though of course as I've described it, it has some non-trivial design problems like "what does it mean to define an @observable int if you can call some magical observe() method on it? that variable is clearly not really an int then, is it?" but I'm still putting this out there with a sliver of a hope that it gains traction, or at least that someone offers me a less boilerplate-hell-ish way of doing this, if it exists.