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 a way to detect a change of specific properties in a script #2837

Open
Killfrra opened this issue Jun 6, 2021 · 9 comments
Open

Add a way to detect a change of specific properties in a script #2837

Killfrra opened this issue Jun 6, 2021 · 9 comments

Comments

@Killfrra
Copy link

Killfrra commented Jun 6, 2021

Describe the project you are working on

2D MOBA game as a hobby and university course project

Describe the problem or limitation you are having in your project

I want to synchronize the object fields on the server with the object fields on the client side and notify the interface of this change. In other words, I want to have a setter for all variables. There are several implementation options, but they all have their pros and cons:

a) Preferred. Inspired by this answer. Does not work (tested in 3.2 and 4.0)

var armor: int

var stats := {
    StringName("armor"): true
}

func _set(prop: StringName, value):
    print("_set(", prop, ", ", value, ')')
    if prop in stats:
        print(prop, " = ", value)
        return true
    return false

func _ready():
    self.visible = true
    self.armor = 1

Expectation: a line will be printed for "visible" and two for "armor".
Reality: only the line for "visible" is printed.
Note: You may notice that this solution uses a dictionary as well as the next one, but I can't avoid this in my code, because I need to store variable attributes somewhere, which is a topic for another proposal

b)

var armor: int:
    set(value):
        print("armor = ", value)
        armor = value

func _ready():
    self.armor = 1

Pros: works as expected
Cons: requires large amounts of code when there are a lot of variables, as is often the case in MOBA

c) Inspired by this post.
The "Why is this useful?" section describes the situation quite accurately.

# in parent class:
var stats := {}

func _get(prop: StringName):
    return stats.get(prop)

func _set(prop: StringName, value):
    print("_set(", prop, ", ", value, ')')
    if prop in stats:
        print(prop, " = ", value)
        # Function "set()" not found in base Dictionary
#		stats.set(prop, value)
        stats[prop] = value
        return true
    return false

# in child classes:
func _init():
    stats[StringName("armor")] = 0

func _ready():
    self.visible = true
    self.armor = 1

Pros: works as expected
Cons: auto-completion does not work, it is not possible to detect the absence of a variable at the compilation stage, there is no typing, additional code is required to display variables from stats for my gd2puml converter (used to build UML class diagrams from existing GDScript code)

d) Same thing, only with a class instead of a dictionary.

# in parent class:
class Stats:
    pass

var stats: Stats

func _get(prop: StringName):
    return stats.get(prop)

func _set(prop: StringName, value):
    print("_set(", prop, ", ", value, ')')
    # Parse Error: Invalid operands "StringName" and "Stats" for "in" operator.
    #if prop in stats:
    if prop == StringName("armor"): # walkaround
        print(prop, " = ", value)
        stats.set(prop, value)
        return true
    
    # set returns void instead of bool
#	if stats.set(prop, value):
#		print(prop, " = ", value)
#		return true
    
    return false

# in child classes:
class ChildStats extends Stats:
    var armor: int

func _init():
    stats = ChildStats.new()

func _ready():
    self.visible = true
    self.armor = 1

Pros: works as expected.
Cons: there is no autocompletion and there is no way to detect a mistake in writing the variable name at the compilation stage.

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

I would like to change the order of checking for the presence of a variable before calling the _set function so that solution a works. I think it will be enough to swap the lines in Object::set (core/object/object.cpp). After that, more variables will be passed through _set, but it already handles the excess, as can be seen in the example with visibility.

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

Code from solution a will work.

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

Yes, quite - there are three working solutions, but all of them are not ideal.

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

The easiest way would be to change this in the core

@Calinou Calinou changed the title Way to detect a change of some properties Add a way to detect a change of specific properties in a script Jun 6, 2021
@dalexeev
Copy link
Member

dalexeev commented Jun 7, 2021

3.3, both options work for me:

func _set(prop: String, value) -> bool:
    print("_set: ", prop, " = ", value)
    return true

func _ready() -> void:
    self.my_var = 42
var my_var: int setget set_my_var

func set_my_var(value: int) -> void:
    print("set_my_var: ", value)

func _ready() -> void:
    self.my_var = 42

@me2beats
Copy link

me2beats commented Jun 7, 2021

3.3, both options work for me

could you pls provide a full script example?

Can't get it working

extends Node2D

var x

func _set(property:String, value)->bool:
	print(property, value)
	return true

func _ready():
	self.x = 10

linux, 3.3, 3.3.2

@dalexeev
Copy link
Member

dalexeev commented Jun 7, 2021

Can't get it working

Either use _set and don't declare a property, or declare a property and use setget. But don't mix the two ways.

This will work:

extends Node2D

# var x

func _set(property:String, value)->bool:
	print(property, value)
	return true

func _ready():
	self.x = 10

@me2beats
Copy link

me2beats commented Jun 7, 2021

This will work:

Thanks, this works
But why this doesn't work with properties declared in the script (like var x above)?
This makes it limited.

As for setters, using them is not a good idea for example when having many script properties.
You need to create a new setter method for each property that uses setget.
Also I had a bad experience with setget in plugins, especially when using export vars.

@dalexeev
Copy link
Member

dalexeev commented Jun 7, 2021

You need to create a new setter method for each property that uses setget.

It's okay.

@me2beats
Copy link

me2beats commented Jun 7, 2021

This will work

Oh but I can't set the property then?

extends Node2D

func _set(property:String, value):
	return true
	# how to set the property?

func _ready():
	self.visible = false
	print(self.visible)

--> true

@Killfrra
Copy link
Author

Killfrra commented Jun 7, 2021

  • The recommended by @dalexeev option with setters is good, because there are no problems with auto-completion, error checking, type-specific optimizations, and parsing with custom utilities. But more lines means a greater chance of mistake, worse navigation through the code, and difficulties in making changes when necessary. Not to mention the fact that having a large number of extremely similar functions in the code feels somehow wrong - we are not writing a miner.
  • Also, as a user, the behavior of the _set function does not seem very intuitive to me: based on the documentation, it allows you to "customize the return value of of set", but it does not mention that it is called only if the field does not exist. And why then is it called when setting existing fields of built-in types? As someone who once read an article about the internals of the engine, I understand that scripts and built-in types are simply handled differently, but this is not something that you usually have to focus on. I'm not sure if this is a bug or an undocumented feature, so I created this proposal in the hope that the behavior of the function will be brought into line with my expectations at least in the next version of Godot, which I plan to wait for ;)
  • But beyond my expectations, there are things to think about, such as whether _set should take precedence over regular setters, as it currently does for field setters of built-in types (thanks to @me2beats for discovering this), or not, as it does for user-defined fields now, or should be replaced with the "property_changed" signal (ala "texture_changed" in "Sprite", "script_changed" in "Object", "property_changed" in "EditorProperty", etc.) and be done with it. upd.: Or let's talk about macroses or support for custom preprocessors to solve all problems of this kind, as well as some proposals about the GDScript syntax.
var armor: int:
	set(value):
		print("armor setter triggered")
		armor = value

func _set(prop: StringName, value):
	print(prop, " catched by global setter")
	return true

func _ready():
	self.armor = 1
	self.visible = false
	print("armor = ", self.armor)
	print("visible = ", self.visible)
armor setter triggered
visible catched by global setter
armor = 1
visible = True

@dalexeev
Copy link
Member

dalexeev commented Jun 8, 2021

Oh but I can't set the property then?

Use a different variable or store the value in a dictionary.

But more lines means a greater chance of mistake, worse navigation through the code, and difficulties in making changes when necessary.

Yes, this adds some boilerplate code, but it doesn't really pose any particular problems. This approach is used in many other programming languages. In 4.0, we decided to reduce the boilerplate code by replacing getters and setters with properties that are written after a variable declaration, without creating a function.

based on the documentation, it allows you to "customize the return value of of set", but it does not mention that it is called only if the field does not exist

Yes, we should probably improve the _set documentation to better reflect the current state of affairs.

It is better to ask @reduz and @vnen whether to change the behavior of _set and add the property_changed signal. Personally, it seems to me that you should think of the _set/_get more as service methods. It is better to handle different properties in separate methods and create separate signals for them than to handle everything in one method using if/match. The _set/_get methods should only be used if you want to make an array/dictionary look like an object in order to handle a large number of properties of the same type. Then it makes perfect sense that already declared properties are not processed in _set/_get. In other languages, this also usually happens.

I don't know why built-in properties are handled differently, but in 3.x there is a similar problem with virtual methods, which was fixed in 4.0-dev, and now you need to explicitly call the method in the parent class using the super keyword. Perhaps this was also fixed?

@me2beats
Copy link

me2beats commented Jun 8, 2021

Use a different variable or store the value in a dictionary.

I mean when using _set() it seems all "built-in" properties like visible, position etc become kinda protected when I try to set them "directly" from another script.
node.visible=false doesn't set the property if _set() is in the node script.

It seems node.set("visible",false) works tho.

Weird.

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

4 participants