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

Default implementations for setter/getter in GDScript. #2983

Open
iiiCpu opened this issue Jul 12, 2021 · 6 comments
Open

Default implementations for setter/getter in GDScript. #2983

iiiCpu opened this issue Jul 12, 2021 · 6 comments

Comments

@iiiCpu
Copy link

iiiCpu commented Jul 12, 2021

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.

@iiiCpu iiiCpu changed the title Default implementations for setget setter/getter. topic:gdscript Default implementations for setget setter/getter in GDScript. Jul 12, 2021
@Error7Studios
Copy link

Error7Studios commented Jul 12, 2021

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
Copy link
Member

Calinou commented Jul 12, 2021

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

@iiiCpu
Copy link
Author

iiiCpu commented Jul 12, 2021

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
Copy link
Author

iiiCpu commented Jul 12, 2021

@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 iiiCpu changed the title Default implementations for setget setter/getter in GDScript. Default implementations for setter/getter in GDScript. Jul 12, 2021
@iiiCpu
Copy link
Author

iiiCpu commented Jul 13, 2021

If godotengine/godot#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
Copy link

OeilDeLance commented Jan 29, 2024

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 ?

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