Allow setters and getters to take additional arguments

[nlupugla]

Please see the original pull request #76665 for summary of changes and discussion.

Edit 2023/Sept/18 8:43: Copied some of the examples over from the linked PR for convenience and expanded on them a bit.

Examples

extends Node2D

signal sprite_property_set(property : StringName, value : Variant)
signal label_property_set(property : StringName, value : Variant)

@onready var sprite := $Icon as Sprite2D
@export var offset : Vector2 : set=set_in_sprite, get=get_in_sprite
@export var centered : bool : set=set_in_sprite, get=get_in_sprite

@onready var label := $Label as Label
@export var text : String : set=set_in_label, get=get_in_label
@export var language : String : set=set_in_label, get=get_in_label


func get_in_sprite(property : StringName) -> Variant:
    return sprite.get(property)
	
	
func set_in_sprite(property : StringName, value : Variant, emit_set := true) -> void:
    sprite.set(property, value)
    if emit_set: sprite_property_set.emit(property, value)


func get_in_label(property : StringName) -> Variant:
    return label.get(property)
	
	
func set_in_label(property : StringName, value : Variant, emit_set := true) -> void:
    label.set(property, value)
    if emit_set: label_property_set.emit(property, value)


func _ready() -> void:
    centered = true
    set(&"text", "The text was set without emitting a signal", false)

• Closes Automate or reduce the boilerplate when forwarding script properties in GDScript godot-proposals#6750.
• Closes Improve access to variables in set / get properties GDScript 2.0 godot-proposals#3096.
• Closes Getter & setter functions - allow extra "optional" parameters (i.e. parameters with default assignments) godot-proposals#6107 (discussion).
• Closes Setters/Getters don't allow functions with default arguments #74144.

addresses the following proposals:

• Add synchronized variables / pointers without functions godot-proposals#2817.
• Default implementations for setter/getter in GDScript. godot-proposals#2983.
• Add @observable annotation for properties in GDScript godot-proposals#4867.
• Add an elegant way to listen to property changes godot-proposals#4868.

Bugsquad edit: Add lists for issues/PRs links.

[dalexeev]

1. You accidentally created a commit in which the author is akien-mga, and you are only the committer. Please fix this or the PR will not be counted as your contribution.
2. Copy "Closes ..." from the old PR to the first post.
3. I think the clarification is not needed in the title, you can add a link to the old PR in the first post.

[nlupugla]

1. You accidentally created a commit in which the author is akien-mga, and you are only the committer. Please fix this or the PR will not be counted as your contribution.
2. Copy "Closes ..." from the old PR to the first post.
3. I think the clarification is not needed in the title, you can add a link to the old PR in the first post.

Done! Thanks for the help :)

[dalexeev]

1. Here you also need to add the check.

https://github.com/godotengine/godot/blob/da4c810fcea9c2000e30e991d55b41328329efc6/modules/gdscript/gdscript.cpp#L1595-L1619

2. After changing the error messages, the tests fail, you must also update the text in them.

Log

modules/gdscript/tests/gdscript_test_runner.cpp:213: ERROR: CHECK( result.passed ) is NOT correct!
  values: CHECK( false )
  logged: modules/gdscript/tests/scripts/analyzer/errors/setter_with_wrong_value_arg.gd
          GDTEST_ANALYZER_ERROR
Value argument of type "Array" in setter "set_named" is not compatible with a property of type "int".


modules/gdscript/tests/gdscript_test_runner.cpp:213: ERROR: CHECK( result.passed ) is NOT correct!
  values: CHECK( false )
  logged: modules/gdscript/tests/scripts/analyzer/errors/property_function_get_type_error.gd
          GDTEST_ANALYZER_ERROR
Return type of getter "get_prop" is not compatible with a property of type "int".


modules/gdscript/tests/gdscript_test_runner.cpp:213: ERROR: CHECK( result.passed ) is NOT correct!
  values: CHECK( false )
  logged: modules/gdscript/tests/scripts/analyzer/errors/property_function_set_type_error.gd
          GDTEST_ANALYZER_ERROR
Value argument of type "int" in setter "set_prop" is not compatible with a property of type "String".


modules/gdscript/tests/gdscript_test_runner.cpp:213: ERROR: CHECK( result.passed ) is NOT correct!
  values: CHECK( false )
  logged: modules/gdscript/tests/scripts/analyzer/errors/setter_with_wrong_name_arg.gd
          GDTEST_ANALYZER_ERROR
First mandatory argument of setter "set_named" with 2 mandatory arguments should be a property name and must be a String or StringName.

3. Probably, tests should be duplicated for static variables, since their implementation is also largely duplicated. This does not please me, but I think it makes sense, taking into account point 1.

[nlupugla]

I have some questions about a few details.

1. I just realized that in this version of the code, we don't seem to be allowing for extra optional arguments with getters/setters. For example, in the commit, line 903 of GDScript.cpp, we call callp(member->getter, &args, 1, ce) rather than, say callp(member->getter, &args, args.size(), ce). Is this intentional? I suppose it is because GDScript::set only has 2 arguments. Should these be modified to have a variable number of optional args?
2. I'd like to introduce a helper function like call_getter and call_setter which would look roughly like

Callable::CallError GDScript::call_getter(const MemberInfo &p_member, const StringName &p_name, Variant &r_ret) const {
	Callable::CallError ce;
	if (p_member.getter_with_name) {
		Variant property_name = p_name;
		const Variant *args = &property_name;
		r_ret = const_cast<GDScript *>(this)->callp(p_member.getter, &args, 1, ce);
	} else {
		r_ret = const_cast<GDScript *>(this)->callp(p_member.getter, nullptr, 0, ce);
	}
	return ce;
}

Where should I stick this method in the class? Should it be private? public? protected? Do I need to make a copy of this method for GDScriptInstance?

[dalexeev]

1. I just realized that in this version of the code, we don't seem to be allowing for extra optional arguments with getters/setters.

No, I think setters/getters with optional arguments are handled correctly. 1. Optional arguments are not required to be passed when called, so this works. 2. If you want to pass optional arguments, then you should call the setter/getter function instead:

my_var = value # The syntax itself doesn't allow additional arguments.
set("my_var", value) # The core doesn't support additional arguments.
set_my_var(value, opt_arg_1, opt_arg_2) # It works.

In fact, #74144 reports the issue that you cannot use such functions as setters/getters. But we're not talking about setters/getters supporting extra arguments. Just now you can combine these two things in one function.

2. I'd like to introduce a helper function like call_getter and call_setter

Yes, for GDScriptInstance this should be a separate method, because for non-static variables there is a difference on which instance the setter/getter method is called (for static variables, the method is called on the script, not on the instance). Alternatively, you can use a macro with m_base argument. I'm not sure if we need to follow DRY in this case, but let's try it and see what happens.

[nlupugla]

That makes total sense about the optional arguments, thanks for explaining that :)

I'll be honest, I'm kind of scared of macros, so I'll just stick to functions :). One thing I thought of for the GDScriptInstance helpers was to call script.ptr()->call_getter, but when I compared the code for GDScript::callp and GDScriptInstance::callp, they looked to have some non-trivial differences. For the modifiers (eg: public/protected/private, const, virtual, etc), I'm planning to just copy whatever I see in callp. I'll write the methods directly above wherever callp appears in the source. Finally, for an internal-use-only function like call_getter, is it more appropriate to name it _call_getter to signify that it's private, or would that imply that it is intended to be overwritten?

Thanks!

[nlupugla]

Apologies for the multiple commits. I'm working on rebasing them into 1 commit right now.

[nlupugla]

Some of my changes got screwed up when I squashed. Thankfully I made a backup branch :) This last commit should be good to go now!

[nlupugla]

My bad, forgot to add tests for static variables. I'll write those now.

[nlupugla]

I ran the GDScript tests properly now instead of manually filling in the .out files, so the CI should no longer complain about getting the wrong error messages in the test with this latest commit.

[dalexeev]

• Some of the old comments (about the static_member name, about the removal of blank lines) have not yet been addressed.
• There also seems to be a lack of error tests for too many or too few mandatory parameters of setter/getter functions.

[nlupugla]

• Some of the old comments (about the static_member name, about the removal of blank lines) have not yet been addressed.
• There also seems to be a lack of error tests for too many or too few mandatory parameters of setter/getter functions.

Thanks for the review again! Sorry about forgetting those old comments. I'll try getting into the habit of leaving Todos for myself as soon as I read a review.

Good point about the error tests, I'll add those in.

[nlupugla]

I think I've addressed all the comments now except for the minor change in logic related to the getter always returning true versus only returning true if there was no call error.

[nlupugla]

Finally had some time to make the requested changes! Hopefully the PR is ready to ship now :)

[nlupugla]

Darn, co close to passing the CI...

I don't really understand the error message. Any tips on how to fix?

[AThousandShips]

It's a sporadic error, unrelated to PR

[nlupugla]

Passed!! 🥳

[dalexeev]

The code looks good to me (but I haven't tested it again). There are only 2 points left in which we cannot reach consensus over several iterations:

1. Error messages. Don't underestimate this, they are important to users. I think my suggestions are consistent, but you don't apply them consistently.
2. Inconsistent error handling when calling setters/getters in gdscript.cpp. I'm not asking you to fix it in this PR, you could leave it as it is if you didn't change the logic (by wrapping return into if). But since you're changing this, I suggest an option that's closer to GDScript: Fix some methods still can use multi-level calls #81139.

I really like this PR. I hope we reach a consensus soon!

[dalexeev]

Suggested change

	Callable::CallError ce = call_setter(*static_member, p_name, value);
	if (ce.error == Callable::CallError::CALL_OK) {
	return true; // TODO: should this just be return ce.error == Callable::CallError::CALL_OK?
	}
	call_setter(*static_member, p_name, value); // TODO: returned call error not used.
	return true;

[dalexeev]

Suggested change

	Callable::CallError ce = call_setter(*member, p_name, value);
	if (ce.error == Callable::CallError::CALL_OK) {
	return true; // TODO: should this just be return ce.error == Callable::CallError::CALL_OK?
	}
	call_setter(*member, p_name, value); // TODO: returned call error not used.
	return true;

[dalexeev]

Suggested change

	Callable::CallError ce = call_setter(*static_member, p_name, value);
	if (ce.error == Callable::CallError::CALL_OK) {
	return true; // TODO: should this just be return ce.error == Callable::CallError::CALL_OK?
	call_setter(*static_member, p_name, value); // TODO: returned call error not used.
	return true;

[dalexeev]

Suggested change

	Callable::CallError ce = call_getter(*member, p_name, r_ret);
	if (ce.error == Callable::CallError::CALL_OK) {
	return true; // TODO: should this just be return ce.error == Callable::CallError::CALL_OK?
	}
	call_getter(*member, p_name, r_ret); // TODO: returned call error not used.
	return true;

[dalexeev]

Suggested change

	Callable::CallError ce = sptr->call_getter(*static_member, p_name, r_ret);
	if (ce.error == Callable::CallError::CALL_OK) {
	return true; // TODO: should this just be return ce.error == Callable::CallError::CALL_OK?
	}
	sptr->call_getter(*static_member, p_name, r_ret); // TODO: returned call error not used.
	return true;

[dalexeev]

Suggested change

	push_error(R"(First mandatory argument of a getter must be a property name of type String or StringName.)", member.variable);
	push_error(R"(First mandatory argument of a getter is a property name and must be of type String or StringName.)", member.variable);

[dalexeev]

Suggested change

	push_error(vformat(R"(Setters must have 1 or 2 mandatory arguments.)"), member.variable);
	push_error(vformat(R"(A setter must have 1 or 2 mandatory arguments.)"), member.variable);

For consistency with the getter error message.

[dalexeev]

The argument type is incompatible, not the argument itself. This is important information. You can remove all other names, but the argument type and variable type must be present in the error message, in my opinion.

My previous suggestion:

Suggested change

	push_error(vformat(R"(Setter argument "%s" is not compatible with variable type "%s".)", value_arg->identifier->name, member.variable->datatype.to_string()), member.variable);
	push_error(vformat(R"(Type "%s" of argument "%s" in setter "%s" is not compatible with type "%s" of property "%s".)", value_arg->datatype.to_string(), value_arg->identifier->name, setter_function->identifier->name, member.variable->datatype.to_string(), member.variable->identifier->name), member.variable);

[nlupugla]

Lol, I think we've come full circle :)

I think that's how I wrote the error message before, but we wanted the error message to be shorter.

How about Setter argument "%s" of type "%s" is not compatible with variable type "%s"?

Side note: should I change all the instances of "not compatible" to "incompatible" to save a few characters?

[dalexeev]

Suggested change

	push_error(R"(First mandatory argument of a setter with 2 mandatory arguments must be of type String or StringName.)", member.variable);
	push_error(R"(First argument of a setter with 2 mandatory arguments must be of type String or StringName.)", member.variable);

Or this, for consistency with getter:

Suggested change

	push_error(R"(First mandatory argument of a setter with 2 mandatory arguments must be of type String or StringName.)", member.variable);
	push_error(R"(First argument of a setter with 2 mandatory arguments is a property name and must be of type String or StringName.)", member.variable);

Please either add it here or remove it there.

[nlupugla]

The code looks good to me (but I haven't tested it again). There are only 2 points left in which we cannot reach consensus over several iterations:

1. Error messages. Don't underestimate this, they are important to users. I think my suggestions are consistent, but you don't apply them consistently.
2. Inconsistent error handling when calling setters/getters in gdscript.cpp. I'm not asking you to fix it in this PR, you could leave it as it is if you didn't change the logic (by wrapping return into if). But since you're changing this, I suggest an option that's closer to GDScript: Fix some methods still can use multi-level calls #81139.

I really like this PR. I hope we reach a consensus soon!

1. Thanks for your patience on the error messages! Consistency is definitely not my strong suit 😅 Side note: has Godot ever considered collecting all the error strings into constants in one or several files ? It might make localization easier and might make it easier to ensure consistency across error messages because they all show up in one place instead of scattered throughout the codebase.
2. I've opted to not change the behavior as the CI seems to be fine with not using the return value.

Thanks for all the help :) I'm excited to start using this feature in my projects!

[nlupugla]

@adamscott Recently brought up whether the signature for a setter should require the property name to be the first or second argument.

The more I think about it, the more it makes sense for me that the property name should be the second argument. I was at first thinking that it would make sense to have the property name come first, so that users could specify a default value for the setter, but that actually wouldn't work because the setter must have exactly 2 mandatory arguments for the name to be passed in to the argument.

I can modify the PR with the new signature if we agree that is the way to go.

[dalexeev]

The more I think about it, the more it makes sense for me that the property name should be the second argument.

I don't think it makes sense, since the setter/getter type (normal vs named) is determined by the number of manatory parameters (you can use optional parameters as it is allowed in Godot 3, but they are not taken into account at all).

This might make sense if you wanted to have one of the arguments optional, but I don't think it matters compared to the problem below. This hasn't been allowed before anyway, so it doesn't break compatibility.

In my opinion, the order "value, name" doesn't make sense, it's not what you would intuitively expect and contradicts the rest of the Godot APIs (set, _set, ConfigFile, etc.) and the API that your project is likely to have. I can easily imagine a bug due to the fact that you accidentally forgot to reverse the order of the arguments in code like:

func set_property(value, name):
    _data.set(value, name)

[nlupugla]

That is a really good point dalexeev. Let's keep the signature as is then. Less work for me too :)

[nlupugla]

Updated the OP with an example to clarify the new feature. Let me know if there's any thing else I can help with to get the PR across the finish line :)

[MarianoGnu]

I personally don't like how this feature is meant to work from UX perspective, i dont think the amount of boilerplate is reduced.
i think a syntax i would expect is:

@export_proxy_property(NodePath("ChildSprite:centered")) var sprite_centered_or_any_name = true
@export_proxy_property(NodePath("ChildSprite:self_modulate:a")) var sprite_opacity = 0.5

and then you dont need any setter or getter at all

[nlupugla]

I personally don't like how this feature is meant to work from UX perspective, i dont think the amount of boilerplate is reduced. i think a syntax i would expect is:

@export_proxy_property(NodePath("ChildSprite:centered")) var sprite_centered_or_any_name = true
@export_proxy_property(NodePath("ChildSprite:self_modulate:a")) var sprite_opacity = 0.5

and then you dont need any setter or getter at all

Thanks for the comment MarianoGnu! Your idea is very similar to what I originally proposed a few months ago: godotengine/godot-proposals#6750. The current approach is a compromise that doesn't achieve quite the same level of boilerplate reduction, but is also much more flexible and aligned with GDScript's current syntax. For example, what if you want the proxy property to emit a signal when it changes but the original property doesn't emit a signal? The @export_proxy_property approach doesn't provide a clean way to do this, whereas the setter/getter approach does.