Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add onset(...) GDScript function to observe changes in script variables #10909

Open
rune-scape opened this issue Oct 5, 2024 · 11 comments
Open

Comments

@rune-scape
Copy link

rune-scape commented Oct 5, 2024

Describe the project you are working on

same as #4867

Describe the problem or limitation you are having in your project

this is copied from #4867 bc this is an alternative to an annotation

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

a global method that takes a "reference to a member" and returns a Signal that's emitted every time that member is set

var volume := 0.5
var speed := 0.5

func _ready():
    onset(volume).connect(func(old, new): print("volume changed from %s to %s" % [old, new]))
    onset(speed).connect(func(old, new): print("speed changed from %s to %s" % [old, new]))

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

internally the onset(...) method parses the expression passed to it specially, (like a[b] = c) and at runtime resolves the base (either a GDScript or instance of GDScript) and the member name (can be dynamic index, a[b], or attribute a.c)

then adds a special signal to the base, and sets a variable on the base that can be quickly read when setting the member

and when the member is set, the variable is checked to see if anything is even observing, if it is it emits the special signal

this applies to variables set inside their setters (in the gdscript vm) and GDScript::set() GDScriptInstance::set()

my implementation is here: https://github.com/rune-scape/godot/tree/gds-onset

there is a speed impact of 0-15% when assigning any members of scripts depending on the kind of assignment
local vars seem unaffected or even slightly faster (i changed a small part of the gdscript vm)

with this project: set-test.zip

on master:

static var: index without setter: 0.10050392150879s
static var: attr without setter: 0.0556378364563s
static var: index with setter: 0.21009182929993s
static var: attribute with setter: 0.18113780021667s
non-static var: index without setter: 0.09078598022461s
non-static var: attr without setter: 0.03088116645813s
non-static var: index with setter: 0.21073007583618s
non-static var: attribute with setter: 0.19546914100647s
local var: 0.03088903427124s

my branch:

static var: index without setter: 0.10746908187866s
static var: attr without setter: 0.05551791191101s
static var: index with setter: 0.24118113517761s
static var: attribute with setter: 0.2020480632782s
non-static var: index without setter: 0.10572814941406s
non-static var: attr without setter: 0.03479790687561s
non-static var: index with setter: 0.2424590587616s
non-static var: attribute with setter: 0.21048617362976s
local var: 0.03020906448364s

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

@Calinou Calinou changed the title GDScript: Add onset(...) to observe changes in script variables Add onset(...) GDScript function to observe changes in script variables Oct 6, 2024
@Calinou
Copy link
Member

Calinou commented Oct 6, 2024

This looks interesting and the implementation linked in the proposal seems fairly compact considering the scope of the feature. I don't know if the implementation is sound from a technical perspective though, since I haven't worked on the GDScript VM code.

From the syntax/naming side of things, onset() looks strange to me since we generally don't use compound words in functions. We do that for @onready, but that's an annotation and could be considered legacy baggage at this point. I'd probably go for on_set() instead.

Does onset() work on local variables? For instance:

func _ready():
	var hello := "world"
	hello = "world 2"
	onset(hello).connect(func(old, new): printt(old, new)
	hello = "world 3"
	set_deferred("hello", "world 4")

I'm also curious what this would look like on the C# side. Does C# have anything in place for observable properties? We should try to make it possible for C# to achieve reasonable feature parity here, if it doesn't provide this already. (The question also applies to cross-language scripting: if a C# script sets a GDScript's property, is the signal emitted?)

cc @godotengine/gdscript

@zodywoolsey
Copy link

zodywoolsey commented Oct 7, 2024

I would love to see this feature be implemented and allow binding the onset to any variable not just script vars. could serve for a powerful optimization utility as well as useful shorthand.

imo this would resolve my proposal: #10539

@dalexeev
Copy link
Member

dalexeev commented Oct 7, 2024

Note that if you try to change the changing variable in the callback (for example to limit the value), then it will lead to an infinite recursion. So this feature is more suitable for reacting after a variable has changed, while standard setters can be used to implement the change.

Also, the feature has the advantage and disadvantage that signals have more control. You can disconnect a method from a signal or block the signal emission by an object, unlike standard setters.

Personally, I like another feature #6750 / godotengine/godot#78991 that allows you to reuse the same setter/getter function for several variables, with the ability to distinguish for which variable the setter/getter is called. It still requires a small boilerplate, but I think it's okay.

However, if we don't consider this as a replacement for "shared setters" (and getters? do you want to introduce onget too?), then the feature makes some sense as an ability to react to accessing class variables. But I'm not sure if this is good from a language design perspective. Being able to achieve the same thing in multiple ways (if we approve shared setters/getters) can be redundant. Having similar features with slightly different semantics (implementing vs reacting) can be confusing. Also see Calinou's concerns above.

Does onset() work on local variables?

No, I think it would be bad for local variables, as this makes it hard to understand the control flow and its potential optimizations.

@Meorge
Copy link

Meorge commented Oct 7, 2024

Overall I really like the idea of this - game elements reacting to changes in the underlying game state is a common problem to have to tackle.

However, I am concerned about the proposed syntax. Currently, as far as I know, when a variable is passed to a function, it's always just the value currently contained by that variable, not a reference to the space allocated for the variable. Having on_set() interpret its argument differently than other functions in GDScript could end up being confusing.

The overall solution I would see to this would be introducing a way to pass a reference to the variable itself, rather than the variable's value - maybe something like so:

var volume := 0.5

func _ready():
    on_set(&volume).connect(func(old, new): print("volume changed from %s to %s" % [old, new]))

However, I recognize that this would be a pretty massive change to GDScript, which would be beyond the scope of this specific issue/proposal.

Perhaps there would be another way the syntax could make it clearer that we're referencing the variable itself, rather than passing it as a value to a function/method?

@nlupugla
Copy link

nlupugla commented Oct 7, 2024

Cool idea! There absolutely should be a more convenient way to react to variables changing than what we have in GDScript now, but there are aspects of this proposal that have some design smell to me.

At face value, onset looks like a function that takes in a variable reference, creates a signal and returns that signal. I don't think there are any existing GDScript functions that take in a variable reference in this way. An alternative might be something like onset.volume.connect. In this version, one could imagine onset as a database which associates set signals with variable names. That said, I do prefer dalexeev and my approach here godotengine/godot#78991, but I'm obviously biased since we wrote it xD

@cj0oste
Copy link

cj0oste commented Oct 11, 2024

An alternative syntax is adding this as a property esque thing into the language syntax rather than a method call:

var variable: int:
	get:
		return variable
	on_set:
		do_stuff()
	set(value):
		variable = value

The get and set behaviour could be defaulted as above when neither are specified such that this code can be simplified to the following:

var variable: int:
	on_set:
		do_stuff()

@tracefree
Copy link

Perhaps there would be another way the syntax could make it clearer that we're referencing the variable itself, rather than passing it as a value to a function/method?

Strings could be used, which would be consistent with the way that they are used for referencing properties in Tweens, i.e. on_set("volume").

@Meorge
Copy link

Meorge commented Oct 20, 2024

Strings could be used, which would be consistent with the way that they are used for referencing properties in Tweens, i.e. on_set("volume").

I imagine this would take less work to develop than the alternative of some sort of pseudo-pointer. But I admit, at least for myself the prospect of using strings corresponding to variable/property names feels rather icky. I'd probably rather that a pseudo-pointer feature be adopted, and then Tweens could use that instead, i.e.

var tw = create_tween()
tw.tween_property(&my_sprite.position.x, 1.0, 0.5)

@dalexeev
Copy link
Member

I'd probably rather that a pseudo-pointer feature be adopted, and then Tweens could use that instead, i.e.

var tw = create_tween()
tw.tween_property(&my_sprite.position.x, 1.0, 0.5)

@Meorge
Copy link

Meorge commented Oct 21, 2024

Good catch on StringNames! I pretty much never use them in my code, so I'd forgotten about the & prefix syntax entirely. Even if quote-less StringNames didn't make it into Godot, I feel like &something referring to the getter-setter pair for the variable something, but &"something" referring to a StringName with the string "something" would be very confusing. So I don't think & would be a good prefix to use.

@cj0oste
Copy link

cj0oste commented Oct 22, 2024

StringNames do seem to be the most compatible with Godot's current semantics as they're used in many similar cases.

If some sort of nameof or get_name method got added, then it would gel very well with this, i.e. onset(nameof(my_var), func(): print(":)"))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants