godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.14k stars 93 forks source link

Default implementations for setter/getter in GDScript. #2983

Open iiiCpu opened 3 years ago

iiiCpu commented 3 years ago

As things are right now, programmer is forced to write lots of unnecessary code for properties. Just adding a signal is a challenge itself.

var some_property # Oh snap, a signal! Here we go again...

# Puff!!!

var some_property setget set_some_property, get_some_property 
signal some_property_changed(new_some_property_value)

func set_some_property(_v):
  if some_property != _v:
    some_property = _v
    emit_signal('some_property_changed', some_property)
    # emit_changed() # for Resource 
func get_some_property():
  return some_property

Now imagine Resource class with 100+ different properties because of

Note: This signal is not emitted automatically for custom resources, which means that you need to create a setter and emit the signal yourself.

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

Add support for properties to use default implementations without\with signal, so programmers would write only necessary code. Maybe also make it possible to call default property method from reimplemented code.

Totally default property

var some_property setget default, default 

# is equivalent to

var some_property setget set_some_property , get_some_property 

func set_some_property(_v):
  if some_property != _v:
    some_property = _v
    emit_changed() # only for Resource 
func get_some_property():
  return some_property

Property with default signal

var some_property setget default, default, default

# is equivalent to

var some_property setget set_some_property , get_some_property 
signal some_property_changed(new_some_property_value)

func set_some_property(_v):
  if some_property != _v:
    some_property = _v
    emit_signal('some_property_changed', some_property)
    emit_changed() # only for Resource 
func get_some_property():
  return some_property

Property with custom setter and default getter and signal

var some_property setget set_some_property , default, default
func set_some_property(_v):
  if _v >= 0 and _v <= 99:
    ##default(_v)
    super.set_some_property(_v)

# is equivalent to

var some_property setget set_some_property , get_some_property 
signal some_property_changed(new_some_property_value)

func set_some_property(_v):
  if _v >= 0 and _v <= 99:
    if some_property != _v:
      some_property = _v
      emit_signal('some_property_changed', some_property)
func get_some_property():
  return some_property

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

As GDScript does not currently support any type of metaprogramming the only way is the brute force way.

Yet using _set and _get for mass checks or signals is possible, it doesn't really help in the case. Instead of making code simpler and shorter it makes it more complicated and forces developer to duplicate code. And also moves most checks from core to user code in runtime. So, definitely, not a workaround to use in real code.

It is also possible for simple scripts to be generated from other scripts. And it works like charm except it doesn't. The code does not become simpler to read and modify.

Error7Studios commented 3 years ago

Now imagine Resource class with 100+ different properties

If you need to emit a signal for 100+ property changes, you should probably just use a dictionary instead of 100+ setters funcs

Thing class

extends Node

class_name Thing

signal value_changed(_self, key, old_value, new_value)

const KEY := { # dict keys
    foo = "foo",
    bar = "bar",
}

var dict := { # used instead of variables
    KEY.foo: 100,
    KEY.bar: 25,
}

func _init() -> void:
    assert(KEY.values() == dict.keys())

func fetch(key: String):
    assert(dict.has(key), str("Invalid key: ", key))
    return dict[key]

func change(key: String, new_value, assert_same_type := true) -> void:
    assert(dict.has(key), str("Invalid key: ", key))
    var old_value = dict[key]

    if assert_same_type:
        assert(typeof(old_value) == typeof(new_value))

    var accepted := true

    match key:
        KEY.foo:
            if new_value < 0 or new_value > 99:
                accepted = false
        _:
            pass # no condition checks for other keys

    if accepted and old_value != new_value:
        dict[key] = new_value   
        emit_signal("value_changed", self, key, old_value, new_value)
#       emit_changed() # if Resource

Thing parent

func _on_thing_value_changed(thing: Thing, key: String, old_value, new_value) -> void:
    assert(thing)
    print("Thing ", key, ":", old_value, "->", new_value, ", ", thing)
    # other code here

func _ready() -> void:
    var thing: Thing = $Thing
    thing.connect("value_changed", self, "_on_thing_value_changed")
    thing.change(Thing.KEY.foo, 150) # ignored. fails condition check
    thing.change(Thing.KEY.foo, 75)

prints: Thing foo:100->75, [Node:1199]

Calinou commented 3 years ago

setget has been removed in favor of a new syntax in the GDScript rewrite for Godot 4.0. See also https://github.com/godotengine/godot-proposals/issues/2979.

iiiCpu commented 3 years ago

Now imagine Resource class with 100+ different properties

If you need to emit a signal for 100+ property changes, you should probably just use a dictionary instead of 100+ setters funcs

As i wrote,

Yet using _set and _get for mass checks or signals is possible, it doesn't really help in the case. Instead of making code simpler and shorter it makes it more complicated and forces developer to duplicate code. And also moves most checks from core to user code in runtime. So, definitely, not a workaround to use in real code.

Sure thing, Dictionary is designed for data. But... 1) Worse performance. Even after replacing String keys with Integer keys, dictionary is still a bit slower than property. And to mention additional string comparation in signal-slot... You might think "dis is but a scrath", but in my case there would be 41 slot calls instead of 3. And it's not a final number. It might be decreased but for a price.

2) No integration with Editor If I want properties to be visible, I have to overload _set and _get. And this leads to

3) Code duplication. Even in your code:

const KEY var dict match key:

Then, if i would like to make it real properties, i'll have to store each property metadata in another list. Also, assert_same_type should be in said metadata dictionary. So, it's possible to implement, yet it's still hella bunch of code instead of simple near-free syntax.

UPD1: Due to #24534 it's impossible to mimic normal properties from within of gdscript. set() and get() works fine, but you need to either use them directly in code or use temporary variables for formulas. Both cases are worse. The funniest part is that needed method is called as it should be BUT gdscript fails to finish the opperation.

iiiCpu commented 3 years ago

@Calinou

setget has been removed in favor of a new syntax in the GDScript rewrite for Godot 4.0. See also #2979.

Well, it's still here in 3.3.2. And as for new syntax, it fits just fine:

var my_prop: int: get = default, set = default, signal = default

# instead of
signal my_prop_changed(new_value)
var my_prop: int: 
  get:
    return my_prop
  set(value):
    if my_prop != value:
      my_prop = value
      my_prop_changed.emit(my_prop )

With this some other parts will become nicer

Edit: I think the proposal I made in #844 (comment) could be a compromise for people who still wants actual functions as setter/getter:

So, I guess we can provide a special syntax if you still want a separate function:


var my_prop:
   get = get_my_prop, set = set_my_prop

func get_my_prop():
   return my_prop

func set_my_prop(value):
   my_prop = value

turns into

var my_prop:
    get = default, set = set_my_prop

func set_my_prop(value):
    my_prop = value
iiiCpu commented 3 years ago

If https://github.com/godotengine/godot/pull/50414 will pass without problems, the following code could be somewhat like workaround for #2983 If not, object.property syntax would not work and properties would be accessible only trough common setter object.set('property', value) and getter object.get('property'), which is sad.

tool
extends Resource

enum _PLF {
    PROPERTY_ARRAY = 1,
    PROPERTY_MAP   = 2,
}
enum _PAF {
    GETTER         = 0,
    SET_CAST       = 1,
    SET_VALIDATOR  = 2,
    SET_BEFORE     = 3,
    SET_AFTER      = 4,
    CUSTOM_SIGNAL  = 5,
    EMIT_CHANGED   = 6,
    DEFAULT_VALUE  = 7,
}
const paf_map_from = {
    _PAF.GETTER        : "getter"       ,
    _PAF.SET_CAST      : "set_cast"     ,
    _PAF.SET_VALIDATOR : "set_validator",
    _PAF.SET_BEFORE    : "set_before"   ,
    _PAF.SET_AFTER     : "set_after"    ,
    _PAF.CUSTOM_SIGNAL : "custom_signal",
    _PAF.EMIT_CHANGED  : "emit_changed" ,
    _PAF.DEFAULT_VALUE : "default_value",    
}
const paf_map_to = {
    "getter"       : _PAF.GETTER       ,
    "set_cast"     : _PAF.SET_CAST     ,
    "set_validator": _PAF.SET_VALIDATOR,
    "set_before"   : _PAF.SET_BEFORE   ,
    "set_after"    : _PAF.SET_AFTER    ,
    "custom_signal": _PAF.CUSTOM_SIGNAL,
    "emit_changed" : _PAF.EMIT_CHANGED ,
    "default_value": _PAF.DEFAULT_VALUE,
}
# Optionally, it can also include hint: int (see PropertyHint), hint_string: String, and usage: int (see PropertyUsageFlags).

const property_prototype = {
    'name'        : "",
    'type'        : TYPE_NIL,
    'hint'        : PROPERTY_HINT_NONE,
    'hint_string' : "",
    'usage'       : PROPERTY_USAGE_DEFAULT,
    _PAF.GETTER        : "",
    _PAF.SET_VALIDATOR : "",
    _PAF.SET_CAST      : "",
    _PAF.SET_BEFORE    : "",
    _PAF.SET_AFTER     : "",
    _PAF.CUSTOM_SIGNAL : "",                        #custom_signal
    _PAF.EMIT_CHANGED  : true,                      #emit_changed
    _PAF.DEFAULT_VALUE : null,                      #default_value
}
const property_list_prototype = {
        _PLF.PROPERTY_ARRAY: [],
        _PLF.PROPERTY_MAP: {},
}

var pl: Dictionary = property_list_prototype setget __set_properties, __get_properties
var pv: Dictionary = {}

func __set_properties(_v : Dictionary) -> void:
    pl = parse_property_list(_v)
    for _i in pv:
        if not pl[_PLF.PROPERTY_MAP].has(_i):
            var _b = pv.erase(_i)
    return

func __get_properties() -> Dictionary:
    return pl

func parse_property(data : Dictionary) -> Dictionary:
    if not data.has('name') or not data.has('type'):
        return {}
    var ret = property_prototype
    for _i in ret:
        ret[_i] = data.get(_i, ret[_i])
        if _i is int:
            ret[_i] = data.get(paf_map_from[_i], ret[_i])
    return ret

func parse_property_list(data : Dictionary) -> Dictionary:
    var ret = property_list_prototype
    for _i in data:
        var prop = parse_property(data[_i])
        if not prop.empty() and not ret[_PLF.PROPERTY_MAP].has(prop['name']):
            ret[_PLF.PROPERTY_MAP][prop['name']] = ret[_PLF.PROPERTY_ARRAY].size()
            ret[_PLF.PROPERTY_ARRAY].append(prop.duplicate(true))
    return ret

func merge_property_list(old_list : Dictionary, new_list : Dictionary) -> Dictionary:
    if old_list.empty():
        return new_list
    if new_list.empty():
        return old_list
    var ret = old_list
    for _i in new_list[_PLF.PROPERTY_MAP]:
        if ret[_PLF.PROPERTY_MAP].has(_i):
            ret[_PLF.PROPERTY_ARRAY][ret[_PLF.PROPERTY_MAP][_i]] = new_list[_PLF.PROPERTY_ARRAY][new_list[_PLF.PROPERTY_MAP][_i]]
        else:
            ret[_PLF.PROPERTY_MAP][_i] = ret[_PLF.PROPERTY_ARRAY].size()
            ret[_PLF.PROPERTY_ARRAY].append(new_list[_PLF.PROPERTY_ARRAY][new_list[_PLF.PROPERTY_MAP][_i]])
    return ret

func __set(caller : Object, property : String, value):
    if pl[_PLF.PROPERTY_MAP].has(property):
        var prop = pl[_PLF.PROPERTY_ARRAY][pl[_PLF.PROPERTY_MAP][property]]
        var val = value
        if prop[_PAF.SET_CAST] != '':
            val = caller.call(prop[_PAF.SET_CAST], val)
        if prop[_PAF.SET_VALIDATOR] != '':
            if not caller.call(prop[_PAF.SET_VALIDATOR], val):
                return
        if typeof(val) != prop['type']:
            return
        if prop[_PAF.SET_BEFORE] != '':
            val = caller.call(prop[_PAF.SET_BEFORE], val)
        if val == prop[_PAF.DEFAULT_VALUE]:
            var _b = pv.erase(property)
        else:
            pv[property] = val
        if prop[_PAF.SET_AFTER] != '':
            caller.call(prop[_PAF.SET_AFTER], val)
        if prop[_PAF.CUSTOM_SIGNAL] != '':
            caller.emit_signal(prop[_PAF.CUSTOM_SIGNAL], val)
        if prop[_PAF.EMIT_CHANGED]:
            caller.emit_changed()
    return

func __get(caller : Object, property):
    if pl[_PLF.PROPERTY_MAP].has(property):
        var prop = pl[_PLF.PROPERTY_ARRAY][pl[_PLF.PROPERTY_MAP][property]]
        if pv.has(property):
            if prop[_PAF.GETTER] != '':
                return caller.call(prop[_PAF.GETTER], pv[property])
            return pv[property]
        else:
            if prop[_PAF.GETTER] != '':
                return caller.call(prop[_PAF.GETTER], prop[_PAF.DEFAULT_VALUE])
            return prop[_PAF.DEFAULT_VALUE]
    return null

func __get_property_list() -> Array:
    return pl[_PLF.PROPERTY_ARRAY]

It is used like this

extends Resource

const DRT = preload("res://DynamicResourceTool.gd")

var props = DRT.new()
var lol: bool = false

signal bar_changed(new_val)
func _init():
    var ps = {
        's': {
            'name': 'foo',
            'type': TYPE_BOOL,
            DRT._PAF.DEFAULT_VALUE: false,
        },
        't': {
            'name': 'bar',
            'type': TYPE_INT,
            DRT._PAF.CUSTOM_SIGNAL: 'bar_changed',
            DRT._PAF.DEFAULT_VALUE: 0,
        }
    }
    props.__set_properties(ps)

func _set(property, value):
    props.__set(self, property, value)

func _get(property):
    return props.__get(self, property)

func _get_property_list() -> Array:
    return props.__get_property_list()

func __set_property_list(_v : Dictionary) -> void:
    props.__set_properties(_v)

func __get_property_list() -> Dictionary:
    return props.__get_properties()

With this bugfix dynamic properties would become almost like real ones (still inaccessible inside class by name yet accessible with self.name). Still worse than proposed default keyword but oh fine. image

Strangely enough for my code, it works just as planed. The properties are accessible from editor if root class is tool. image

OeilDeLance commented 8 months ago

I have an impression that the workaround for a C# custom resource would be something like that:

[GlobalClass]
public partial class MyResource : Godot.Resource
{
    private int _health;    

    [Export]
    public int Health
    {
        get
        {
            return _health;
        }
        set
        {
            GD.Print($"Setting: {value}");
                if(_health != value)
                {
              _health = value;
              OnValidateAssetParameters();
                }
        }
    }
}

It seems the property gets serialized and the setter called modifying via inspector, I'm not sure how that works at init time though. An EmitChange or your how custom virtual can be called from the setter. I noticed that it is also called for asset initialisation which is great.

However by the looks of that it is obvious that this is not ideal for large projects ! Any chance a fix is coming soon ?