Add observer functions #32815

creikey · 2019-10-14T05:33:22Z

When there is a setter and an observer, value is not changed
before calling the observer
Partially fixes Observable properties in gdscript #6491

creikey · 2019-10-14T05:38:00Z

Fixed formatting

realkotob · 2019-10-14T07:00:01Z

This does not seem like it will help a lot with #6491 (unless I misunderstood, if so I apologise).

If the setter takes no argument, then trying to do onchange_value = 45 + 123 would not set the value and instead only do the extra action you described in #6491 (comment), so it will only print("updated") but nothing will actually be updated

creikey · 2019-10-14T07:27:48Z

@asheraryam this PR will update the value independently of the setter function call, so the value is updated then the method is ran. This allows you to bind multiple variables to the same set method, as the set method doesn't change the underlying data, the engine does.

This PR has to do with #6491 as it allows a method to be called when a variable is changed without having to make a new method per variable.

modules/gdscript/gdscript.cpp

bojidar-bg · 2019-10-14T08:31:57Z

I'm not sure about the idea -- on one hand, it allows reusing a function in many setget-s, which is a pretty cool thing; but on the other, it makes it possible to confuse a getter for a setter or vice-versa.

creikey · 2019-10-14T08:41:53Z

@bojidar-bg I'm a little uncertain as well, perhaps to make it more readable, a new onchange keyword could be added, so the syntax would look as such:

var foo = 0 setget set_foo , get_foo onchange foo_changed
var bar = 0 setget set_bar onchange bar_changed
var bee = 0 onchange bee_changed

Calinou · 2019-10-14T11:51:35Z

This could end up being confusing for people, so I'm against it as well. An onchange keyword sounds better to me, as it'd make the programmer's intent more explicit.

groud · 2019-10-14T11:55:45Z

This could end up being confusing for people, so I'm against it as well. An onchange keyword sounds better to me, as it'd make the programmer's intent more explicit.

I completely agree, allowing setters with no arguments makes no sense.

Calinou · 2019-10-14T11:57:46Z

One downside I can see with an onchange keyword is that you can no longer have an associated getter, unless onchange update_something setget ,get_some_property is allowed. (Having both onchange and a setter probably doesn't make sense, though.)

realkotob · 2019-10-14T12:20:13Z

this PR will update the value independently of the setter function call, so the value is updated then the method is ran.

The issue is that this is not always wanted, in many cases we want to perform some operation on the input before setting it, and removing that functionality would be against the standard implementation of setters that most languages have.

bojidar-bg · 2019-10-14T18:22:26Z

@asheraryam What this PR does applies only to zero-parameter setters, which were previously disallowed. It won't break setters with more than one parameter -- those would still get called, without the variable being set before or after them.

creikey · 2019-10-15T04:29:16Z

I'm currently in the process of updating this PR to use a new observer keyword to signify a function that is called when the property is changed as such:

var bar = 0 observer something_changed setget bar_set, bar_get
var fee = 0 setget ,fee_get observer something_changed

In terms of the other half of the issue, maybe you can prepend the var with broadcasting to automatically make a signal for when that property is changed? Like this:

broadcasting var my_var = 0

Then it can be seen and manipulated like any other signal.

- When there is a setter and an observer, value is not changed before calling the observer - Partially fixes godotengine#6491

creikey · 2019-10-15T05:48:37Z

This pull request is now updated to include the observer syntax.

bojidar-bg · 2019-10-15T08:06:10Z

modules/gdscript/gdscript_tokenizer.cpp

@@ -196,6 +197,7 @@ static const _kws _keyword_list[] = {
 	{ GDScriptTokenizer::TK_PR_TOOL, "tool" },
 	{ GDScriptTokenizer::TK_PR_STATIC, "static" },
 	{ GDScriptTokenizer::TK_PR_EXPORT, "export" },
+	{ GDScriptTokenizer::TK_PR_OBSERVER, "observer" },


(non-blocking) OBSERVER is after SETGET in all other lists.

bojidar-bg · 2019-10-15T08:07:17Z

modules/gdscript/gdscript.cpp

@@ -951,13 +951,28 @@ bool GDScriptInstance::set(const StringName &p_name, const Variant &p_value) {
 				Variant::CallError err;
 				call(E->get().setter, &val, 1, err);
 				if (err.error == Variant::CallError::CALL_OK) {
-					return true; //function exists, call was successful
+					if (E->get().observer) {
+						// call observer without changing data beforehand


Given that setgets are typically used to control whether or not a value is changed, especially now with the addition of observers ( no reason to otherwise ) , when there is a setget functuon and an observer function attatched I'm assuming that the script writer wants the setget to control whether or not the value is changed and not have it implicitly done by the observer.

bojidar-bg

Code looks good overall

bojidar-bg · 2019-10-15T08:08:12Z

I guess the keyword could be called just observe instead of observer, not sure about the exact naming though.

vnen · 2019-10-15T15:43:54Z

Honestly I'd like this to through a proposal first so we can flesh out the usage and syntax. I dislike adding new keywords and the variable declaration syntax is already complex enough.

I know there has been discussion in the original issue, but I don't think we reached a consensus.

I also think this could be simplified with annotations (or "attributes" or "decorators", depending on what we decide to do at the end). It would avoid the stream of keywords to declare a variable.

creikey · 2019-10-15T17:24:36Z

@vnen I feel as if given the large amount of information ( both setter/getter methods and observer methods ), the syntax will either be too verbose for a scripting language ( a long list of decorators prefixing a variable ) or too unreadable, as the dense information will have been hidden with a "simplified" syntax ( to avoid the long list of keywords after a variable ). In this PR I went with the simplified syntax, as I don't think many variables will have both a setter/getter method and an observer method. Perhaps the more verbose decorator syntax should be a requirement if there's more than one type of attached method ( observer and a setter/getter ) to increase readability.

Regardless, you're probably right in that there should be more discussion in the original issue, especially from more core contributors. What should be done with this PR until then? I am free to continue to update this code as the discussion converges onto something acceptable as I've been annoyed at the lack of this feature for awhile now.

bojidar-bg · 2019-10-15T20:16:25Z

I'd suggest leaving the PR open, but not changing it; the other option is to close it and mark it "salvageable". I guess you might use the feature in your projects, and when the consensus decision comes, update the PR and projects.

In any cases a proposal should be opened over at https://github.com/godotengine/godot-proposals/

vnen · 2019-10-15T21:21:14Z

This PR can be kept open for now.

What I really want to do is flesh out #18698 into a proper document, including the stuff we plan to do and what's nice to have. Of course, proposals are always welcome but we need a guideline of what we want with GDScript.

creikey · 2019-10-15T21:26:35Z

@vnen Yes, a GDScript language spec seems like it would be pretty useful. I feel like it would be best as a .md file that explicitly describes all the grammar and syntax in gdscript and what it does. I could start typing it up now and open a PR for it if that is desirable.

aaronfranke · 2020-07-01T04:47:10Z

@creikey Is this still desired? If so, it needs to be rebased on the latest master branch.

If not, we can close this and mark it as "salvageable".

creikey · 2020-07-03T21:02:44Z

@aaronfranke For 4.0 the parser is being rewritten by @vnen , and for future 3.2 releases @vnen stated that the proposal workflow is necessary to add a new keyword like this, so this work is not useful. Maybe mark as salvageable for those who want this feature in their custom fork?

Calinou · 2021-03-25T17:04:20Z

Closing, as this work needs to be discussed on the Godot proposals repository then redone against the new GDScript implementation in master. Thanks for the contribution nonetheless 🙂

creikey requested review from bojidar-bg and vnen as code owners October 14, 2019 05:33

creikey force-pushed the observable-functions branch from 7da9902 to 8f0cabb Compare October 14, 2019 05:37

Chaosus added feature proposal topic:gdscript labels Oct 14, 2019

Chaosus added this to the 4.0 milestone Oct 14, 2019

bojidar-bg reviewed Oct 14, 2019

View reviewed changes

modules/gdscript/gdscript.cpp Outdated Show resolved Hide resolved

creikey force-pushed the observable-functions branch from 8f0cabb to a422ce8 Compare October 14, 2019 08:38

creikey force-pushed the observable-functions branch from a422ce8 to ff6077e Compare October 15, 2019 04:17

Add observer functions

6f5b4d1

- When there is a setter and an observer, value is not changed before calling the observer - Partially fixes godotengine#6491

creikey force-pushed the observable-functions branch from ff6077e to 6f5b4d1 Compare October 15, 2019 05:47

creikey changed the title ~~Allow setter functions with no argument~~ Add observer functions Oct 15, 2019

creikey requested a review from bojidar-bg October 15, 2019 05:48

bojidar-bg reviewed Oct 15, 2019

View reviewed changes

bojidar-bg approved these changes Oct 15, 2019

View reviewed changes

creikey mentioned this pull request Jul 23, 2020

Properties in GDScript godotengine/godot-proposals#844

Closed

Calinou closed this Mar 25, 2021

Calinou added archived salvageable labels Mar 25, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add observer functions #32815

Add observer functions #32815

creikey commented Oct 14, 2019 •

edited

Loading

creikey commented Oct 14, 2019

realkotob commented Oct 14, 2019 •

edited

Loading

creikey commented Oct 14, 2019 •

edited

Loading

bojidar-bg commented Oct 14, 2019

creikey commented Oct 14, 2019

Calinou commented Oct 14, 2019 •

edited

Loading

groud commented Oct 14, 2019

Calinou commented Oct 14, 2019 •

edited

Loading

realkotob commented Oct 14, 2019

bojidar-bg commented Oct 14, 2019

creikey commented Oct 15, 2019 •

edited

Loading

creikey commented Oct 15, 2019

bojidar-bg Oct 15, 2019

bojidar-bg Oct 15, 2019

creikey Oct 15, 2019 •

edited

Loading

bojidar-bg left a comment

bojidar-bg commented Oct 15, 2019

vnen commented Oct 15, 2019

creikey commented Oct 15, 2019 •

edited

Loading

bojidar-bg commented Oct 15, 2019

vnen commented Oct 15, 2019

creikey commented Oct 15, 2019

aaronfranke commented Jul 1, 2020

creikey commented Jul 3, 2020

Calinou commented Mar 25, 2021 •

edited

Loading

Add observer functions #32815

Add observer functions #32815

Conversation

creikey commented Oct 14, 2019 • edited Loading

creikey commented Oct 14, 2019

realkotob commented Oct 14, 2019 • edited Loading

creikey commented Oct 14, 2019 • edited Loading

bojidar-bg commented Oct 14, 2019

creikey commented Oct 14, 2019

Calinou commented Oct 14, 2019 • edited Loading

groud commented Oct 14, 2019

Calinou commented Oct 14, 2019 • edited Loading

realkotob commented Oct 14, 2019

bojidar-bg commented Oct 14, 2019

creikey commented Oct 15, 2019 • edited Loading

creikey commented Oct 15, 2019

bojidar-bg Oct 15, 2019

Choose a reason for hiding this comment

bojidar-bg Oct 15, 2019

Choose a reason for hiding this comment

creikey Oct 15, 2019 • edited Loading

Choose a reason for hiding this comment

bojidar-bg left a comment

Choose a reason for hiding this comment

bojidar-bg commented Oct 15, 2019

vnen commented Oct 15, 2019

creikey commented Oct 15, 2019 • edited Loading

bojidar-bg commented Oct 15, 2019

vnen commented Oct 15, 2019

creikey commented Oct 15, 2019

aaronfranke commented Jul 1, 2020

creikey commented Jul 3, 2020

Calinou commented Mar 25, 2021 • edited Loading

creikey commented Oct 14, 2019 •

edited

Loading

realkotob commented Oct 14, 2019 •

edited

Loading

creikey commented Oct 14, 2019 •

edited

Loading

Calinou commented Oct 14, 2019 •

edited

Loading

Calinou commented Oct 14, 2019 •

edited

Loading

creikey commented Oct 15, 2019 •

edited

Loading

creikey Oct 15, 2019 •

edited

Loading

creikey commented Oct 15, 2019 •

edited

Loading

Calinou commented Mar 25, 2021 •

edited

Loading