-
-
Notifications
You must be signed in to change notification settings - Fork 98
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
Automate or reduce the boilerplate when forwarding script properties in GDScript #6750
Comments
This looks like a good use case for custom annotations, when we implement them. Adding an engine built-in for something that appears pretty specific to a particular project architecture doesn't look like a good idea to me. And a feature to auto-generate setters and getters doesn't solve the problem of visual noise that you've outlined. |
Is there any active work on custom annotations? It feels like it could be a very powerful feature. As far as I can tell from a quick glance through #1316, there hasn't been much development on it. If the auto-generated setters and getters showed up towards the bottom of the script, it would at least reduce the noise at the top of the script where all the variables are defined. Finally, I'm surprised that this is considered a specific/niche problem. You're not the first person to have mentioned something like that. To me, it seems like you'll run in to this problem whenever you want to group multiple low-level objects into a single high-level scene/object. In particular, isn't it considered good practice to keep the logical state of a game entity separate from the entity's visual appearance? In some ways, accessing nodes by their Unique Scene Name solved this problem for me at the node/scene level. For example, in a complicated scene tree, I can get any node I want by calling Coming at things from the opposite direction, if the proposed
as syntactic sugar for
|
First we need to understand what features we expect from custom annotations. Should it just be "machine-readable metadata information on declarations in code" or we want custom annotations to allow you to modify the behavior just like the standard ones. Annotations are not Python decorators, they are more like PHP attributes, although the standard annotations have hardcoded behavior. The following things deeply affect the language itself, they cannot be implemented if custom annotations are just static metadata:
Also note that annotation arguments are currently required to be constant values, the following code does not work: @export_range(0, 100) var a: int
@export_range(0, a) var b: int Thus, custom annotations are quite a difficult problem. As for the problem of boilerplate, I have three more ideas. 1. @alias var a = x.a Syntactic sugar for var a:
get:
return x.a
set(value):
x.a = value A variable with the 2. In addition to the var a: int: get = get_a, set = set_a
func get_a() -> int:
return a
func set_a(value: int) -> void:
a = value The problem is that you can't reuse the setter and getter because they don't take a variable name as an argument. But we could add new var a: int: nget = get_prop, nset = set_prop
func get_prop(property: StringName) -> int:
return _data[property]
func set_prop(property: StringName, value: int) -> void:
_data[property] = value You cannot use Or make it so that 3. I don't think this is a good idea in terms of compatibility and performance, but perhaps |
Thanks for the ideas! I had no idea how annotations worked under the hood. I can see why even just determining the scope for what custom annotations should be able to do is a tricky question. I was imagining annotations to be implemented as functional wrappers like in Python. As for your suggestions, I really like the syntax for @alias. I'm confused about |
"nset" is short for "setter with name" or "setter for N properties". I agree that the name is not very good, but we could just use
If you want to use a single function as a setter/getter for heterogeneous properties, then its value argument or return type must be var a: int: set = set_property, get = get_property
var b: String: set = set_property, get = get_property
var _data := { a = 1, b = "test" }
func _ready() -> void:
var x: int = a # Type safe.
var y: String = b # Type safe.
func set_property(property: StringName, value: Variant) -> void:
_data[property] = value
func get_property(property: StringName) -> Variant:
return _data[property]
@alias var a: int = x.a # x.a must be type compatible with int. Note that |
I'm definitely more a fan of this approach than introducing new keywords.
I agree that signal stats_changed(property : StringName)
var stats : Stats #inherits from Resource
var hp : int : set=set_stats_property, get=get_stats_property
var attack : int : set=set_stats_property, get=get_stats_property
var defense : int : set=set_stats_property, get=get_stats_property
var job : String : set=set_stats_property, get=get_stats_property #added this for the sake of type heterogeneity
func set_stats_property(property : StringName, value : Variant) -> void:
if stats.get(property) != value: stats_changed.emit(property)
stats.set(property, value)
func get_stats_property(property : StringName) -> Variant:
return stats.get(property) I suppose if one were incredibly serious about type safety, it should be possible to do something this as well right? var defense : int : set=set_stats_int, get=get_stats_int
var job : String : set=set_stats_string, get=get_stats_string This is starting to sound like a pretty natural extension to the existing setter/getter system. What do people think? |
Another thought I had is about whether this approach would be compatible with Godot's C# API. I don't use C# much, but since Godot "officially" supports it to some extent, it would be great if this feature could be designed in such a way that it works in C# as well. |
Finally (and this might be veering pretty far off topic), it would be great if the set/get syntax had better support for read only properties. I've experimented with the following two approaches using the existing syntax, which each have their own drawbacks. Approach 1: don't write a var hp : int :
get: return stats.hp The problem with this approach is that if I write a line like Approach 2: var hp : int :
get: return stats.hp
set(value): assert(false) This approach at least aborts execution if the program encounters This is probably better suited for its own issue/proposal, but I mention it here because it's worth considering while we are already discussing upgrades to set/get. |
How difficult do you think the expanded set/get functionality would be to implement? Would it be doable for a first-time contributor like me? |
Here are some snippets you will need: 1. You probably won't need to make changes to the parser, but you'll find 2. In the analyzer, you need to check the function signature (number of arguments and their types). 3. Also, some changes in the compiler will be required. I'm not familiar with the later stages, but here's what I found. 4. It is also possible that changes to other files ( 5. At the end, you need to add tests. |
Related:
|
Thanks for pointing me to the relevant source code! As I started thinking about the implementation, a noticed a potential problem with the two-argument set/get approach we discussed. Because the name of the variable would be passed as an argument to the set/get method, the name of the variable becomes more important than usual. For example, consider the code below, which we had discussed previously: var stats : Stats #inherits from Resource
var hp : int : set=set_stats_property, get=get_stats_property This is great if Stats has a property called "hp" AND I want to name my variable "hp", but there are cases where both might not be true. Imagine I want to keep track of a character's stats over time. I would like to be able to write something like var current_stats : Stats
var previous_stats : Stats
var current_hp : int : set_current_stats_property, get=get_current_stats_property
var previous_hp : int : set_previous_stats_property, get=get_previous_stats_property but this might lead to very unexpected results. If Stats only has a property called "hp", not "previous_hp" or "current_hp", the editor could highlight the line as an error. However, if Stats does have a property called "previous_hp", then the user will actually be accessing the previous_hp of previous_stats, whereas they might expect to access hp of previous_stats. If the user understands this, they could in principle modify their set/get with a match statement, eg: func get_previous_stats_property(property : StringName) -> Variant:
match property:
"previous_hp": return previous_stats.get(hp)
_: return previous_stats.get(property) The weird thing about the code above is that What do others think? |
I don't think this is a problem. The name passed to the setter/getter is needed in order to distinguish which property this setter/getter was called for. And since there are no pointers/references in GDScript, the only way to do this is to pass a string with the name of the property. If the object/dictionary has different names, then of course you have to use Another option is to allow binding arguments when specifying a setter/getter: var a: int: set = set_property.bind(&"a"), get = get_property.bind(&"a")
var b: int: set = set_property.bind(&"b1"), get = get_property.bind(&"b1")
func set_property(value: Variant, property: StringName) -> void:
_data[property] = value
func get_property(property: StringName) -> Variant:
return _data[property] But personally, I think this is an unnecessary complication compared to the above solution. As a last resort, you can use existing inline setters/getters, but you end up with 3 or 5 lines instead of one (depending on the style). |
I'm glad to hear you don't think it's a problem! I also thought about a functional approach using I'll go ahead with the implementation we discussed (and edit my original post when I get the chance). If the variable name thing ever becomes confusing for users, we can always combine it with something like the proposed @alias decorator. For example, @alias("hp") var previous_hp : int : set=set_previous_stats_property, get=get_previous_stats_property could be the syntax for accessing PS: The var previous_hp : int : set=@bind("hp") set_previous_stats_property, get=@bind("hp") get_previous_stats_property I would say this is overkill for this particular example (it also doesn't fix the copy-and-pastability), but I mention it because I imagine an |
Annotations are not decorators and not expressions, so this syntax would never be valid I guess.
Yes, that's right. We could make the argument binding part of the syntax, but that would restrict them to constant values and therefore also a bad option.
In my opinion, this is non-obvious, "magic" behavior, which does not correspond to my original idea of @alias var a = x.a
@alias var b = x.b1
Good luck! If you need help, don't hesitate to ask. |
Note that just because this is being discussed it does not mean the proposal was approved. Not to discourage you, but to avoid having you working on something that won't be merged. Honestly, none of the approaches sound like a good idea to me. Not because they don't solve the problem in a good way but because the problem itself is very specific to the way you're structuring your code. I think it's not even a problem: if you want to follow this pattern you will have to provide some boilerplate, that's how those things commonly go. Usually code is written once and read many times, so while it might be tedious to write at first, you only have to do it a handful times.
It might be possible already to write a custom plugin that does this. The only issue is that you don't have access to internals of the GDScript compiler to help, so you would need some manual parsing to make it work. Given your specific use case, I don't think anything general would be helpful for you. Usually generating getter/setter from IDE would just give you the standard: var stats: Stats:
get: return stats
set(value): stats = value Instead of doing what you want. Regarding custom annotations, what @dalexeev said holds: we don't have a good picture yet of what is expected of custom annotations. There's many questions to answers, not everything is fleshed out in the proposal. But it is still on my mind and I want to work on it at some point, as it was part of the original idea for annotations. I'm against any syntactic sugar that is not standard or used a lot (e.g., the Just to be clear: C# is unrelated. This is a language feature so it's only relevant for GDScript and can be solved within the GDScript implementation. If you needed it in C# you would have to ask Microsoft to implement the feature in the language. The setter/getter that receives the name of the property may have some merit to allow reusing the same function with many properties. Still, I'm not sure if those are warranted if they only solve this problem. Are there other actual use cases that would benefit from this? Perhaps it would be the solution to other proposals? This might be "easy" to implement, so maybe it's worth it for the sake of it. Type safety is something to consider as well. If the same setter/getter is used for properties of different types, the type of the parameter must be compatible (or Variant). The input value for the setter and the return value for the getter must be properly converted beforehand, and this needs to be guaranteed by the compiler. Binding arbitrary arguments sounds useful, but it's probably complex to implement. Functions are not Callables, they just create Callables at runtime when referenced. So to properly call the setter/getter with binds those would need to be stored in the script and passed as arguments at runtime. If you want non-constant values it becomes more difficult because it would need to store expressions to be executed when calling. I don't think this is worth the hassle. |
Thanks for the addition. I wanted to advise @nlupugla to first ask for confirmation of this design on the
In my opinion, the option is the best and most compromise of those proposed. It reduces the boilerplate, is quite versatile (it solves not only @nlupugla's specific problem, you can add custom logic in the setter/getter), relatively easy to implement, logically continues an existing feature, rather than introducing a new one (if you want to delegate a setter/getter function with
Yes, it should be handled correctly in the analyzer and compiler, but I think it's relatively easy to implement and shouldn't cause any problems. The only other thing we need to think about is whether we want to use some other setter/getter signature in the future. For example, there is a suggestion (#6107) to allow functions with optional extra arguments to be used as setter/getter (for example, |
Thanks for the clarification! That's kind of what I figured anyway. I'm having fun working on it, even if it doesn't get approved. I'll be sure to discuss the feature a bit on the chat. Plus there's a few details that I need some help with anyway. Is it okay to @ either of you with specific questions about the source code or is that considered bothersome?
I vaguely remember seeing a proposal related to making it easier to make your variables to emit a signal when they change. This change would solve that issue I think. |
You can @ me in the chat if you need to ask something.
True. I feel that the name argument in setter/getter is a good overall addition.
I thought this was already allowed. For what I see now it is supported in 3.x and dropped in 4.0, but this was an oversight. I was already considering this case, the difference between optional and required arguments would be enough for this. |
Could you clarify what you mean here? In 4.0, the only allowed getter signature is my_getter_function() -> some_type With the variable getters we've proposed in this issue, we would allow one additional getter signature: my_getter_function(property:StringName) -> some_type If we allow for optional arguments, what signatures would be supported? Something like my_getter_function(property:StringName, option_1 := default_1, option_2 := default_2) -> some_type looks pretty compatible with variable getter proposal, but what about the following signature? my_getter_function(option_1=default_1) If the compiler determines the getter behavior by simple argument counting, this signature would get confused with the |
PS: I was looking through some of the issues linked to this one and noticed that #3133 looks very related as well. |
When a variable has an "outer" setter/getter (with the For a regular setter and getter, there must be 1 and 0 required arguments, for setter and getter proposed here there must be 2 and 1 required arguments, respectively. Any other options for the number of required arguments will result in an analyzer error. Thus, the following signatures are different and there will be no conflict, because if you use an "outer" setter/getter, then the function must satisfy these requirements. func set_prop(name, value) # A setter with property name.
func set_prop(value, opt_arg = 1) # A regular setter with one optional argument.
func get_prop(name) # A getter with property name.
func get_prop(opt_arg = 1) # A regular getter with one optional argument. You can't have a regular setter with two required arguments or a regular getter with one requred argument (neither in 4.0 nor 3.x), it doesn't make sense. Therefore, it does not break compatibility. |
Should the compiler ensure that the first argument of a 1-argument getter or 2-argument setter is a StringName? Maybe it should only enforce this if the first argument isn't optional, so as not to conflict with any future plans to allow for setters and getters with optional arguments? |
If the first argument is optional, it should be a regular getter/setter, not a getter/setter with name.
If the argument is required and has a specified type (that is, it can be either a weak type or a hard |
An interesting point is that the core also supports setters/getters with additional arguments, there is a macro This is not a change request, just an additional argument in favor of an additional argument. 🙂 |
Describe the project you are working on
A jrpg with stats-heavy combat and visual novel elements.
Describe the problem or limitation you are having in your project
I often find myself having to write a lot of tedious boilerplate code to gracefully navigate through complicated objects.
As a simple example, suppose I have a Character script that contains, among its many variables, a Stats resource (which defines attributes like hp, attack, defense, etc). One way to access the hp of a character would be
character.stats.hp
. However, best practices (eg: The Law of Demeter) dictate that it would be better to writecharacter.get_hp()
or simplycharacter.hp
so that the Character script doesn't need to know anything about the internal structure of the Stats resource. The most straightforward approach I can think of would be to write the Character script like so:and so on for all the attributes I want Character to have direct access to. My problems with this approach are that
Describe the feature / enhancement and how it helps to overcome the problem or limitation
Edit 28 April 2023:
After discussion, the proposed solution is to upgrade the inline set/get syntax to allow for an extra
property:StringName
argument. For example,I see two approaches for overcoming this problem.
Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams
Syntactic Sugar Approach
Something along the lines of
@forward_to could be a new built-in annotation or a custom annotation if #1316 is ever implemented.
Auto-generation Approach
When the user writes
there is some UI within the editor which allows the user to auto-generate code such as
This code could be inserted towards the end of the script to avoid cluttering the script's header with visual noise.
The UI for achieving this could either be built in to the editor or provided by custom tooling/a plug-in.
If this enhancement will not be used often, can it be worked around with a few lines of script?
While a single property can be forwarded in 2-3 lines of code using getters/setters, these lines quickly add up over many properties.
Is there a reason why this should be core and not an add-on in the asset library?
Better built-in support for forwarding encourage the use of good design principles like Composition over Inheritance, Single Responsibility, and the Façade pattern.
The Character script in my example respects these principles because it is composed of a Stats object (among other variables), instead of inheriting (from something like CharacterWithoutStats or EntityWithStats) or directly including the members of Stats (which could make Character have multiple responsibilities).
The text was updated successfully, but these errors were encountered: