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

Adding missing group Godot annotations #605

Open
gabryon99 opened this issue Apr 10, 2024 · 8 comments
Open

Adding missing group Godot annotations #605

gabryon99 opened this issue Apr 10, 2024 · 8 comments

Comments

@gabryon99
Copy link
Contributor

While working on manager classes, it is useful to group related properties, using the @export_group (https://docs.godotengine.org/en/stable/tutorials/scripting/gdscript/gdscript_exports.html#grouping-exports) annotation.

However, at the moment, this annotation is not available for Kotlin. It'd be nice to have it!

@RegisterClass
class Simple : Node() {
    @ExportGroup("Bonus Properties")
    var bonusMultiplier: Double
    var bonusPoints: Int
}
@MartinHaeusler
Copy link
Contributor

+1, this would be really nice for structuring the editor.

@chippmann
Copy link
Contributor

Atm I'm thinking about integrating it into the existing @Export annotation as an optional parameter instead of a new one:
@Export(group = "My Custom Group")

This would have the benefit of not having to rely on property ordering as well as being explicit while having the drawback of needing to write the same group name for possibly multiple properties.

Any objections or different suggestions?

@CedNaru & @piiertho what are your opinions on this matter?

@chippmann
Copy link
Contributor

Being more explicit would fit our current approach where every member needs to be explicitly registered instead of implicit

@MartinHaeusler
Copy link
Contributor

This would have the benefit of not having to rely on property ordering as well as being explicit while having the drawback of needing to write the same group name for possibly multiple properties.

A couple of considerations I would like to add here:

  • When it comes to annotations, I'd rather have two separate ones than one with all features squeezed in. It's the general consensus in annotation-driven framworks.
  • Repeating the group name over and over is a bigger issue than it may appear at first. There may be typos, and linters may complain about the repeated string literals. Which leads to constants being defined for this sole purpose. I would argue the other way around: nobody randomly reorders their properties just for fun. And it makes sense to have the properties which belong to the same group closer together in the code file as well.
  • Users switching over from GDScript or ex-unity-users are already used to the pattern of having one annotation define a group, and all subsequent properties in the source code belonging to that group. It's a well established pattern.

@chippmann
Copy link
Contributor

True good point.
Will do some tests with ksp regarding retention of property order

@CedNaru
Copy link
Member

CedNaru commented Apr 11, 2024

Given that GDScript got annotations too since the 4.0.0 version, I'd rather follow the same model as them when possible.
So it means using the separate annotation and having to write it only once, letting the property ordering do the rest.

@MartinHaeusler
Copy link
Contributor

I've got two more points in favor of using the option proposed by @gabryon99 :

  1. The godot-kotlin-jvm project already relies on property ordering for the editor, because their order in the file also determines their order in the editor (which is a very good thing). So it's not introducing any new obscure dependency, it builds on what's already there.

  2. Explicit group names per property can introduce a problem. Consider this:

@Export(group = "A")
@RegisterProperty
lateinit var property1: String

@Export(group = "A")
@RegisterProperty
lateinit var property2: String

@Export(group = "B")
@RegisterProperty
lateinit var property3: String

@Export(group = "A")
@RegisterProperty
lateinit var property4: String

It's very ambiguous what would happen in the editor for this case. Will we produce two groups named A? (Is that even possible in the Godot editor?) Would property4 get reordered to appear after property2 just to group them? By relying on the property order, we say "every following property hereafter goes into this group, until another group is declared". This is unambiguous, quite user friendly, and as @CedNaru pointed out, it matches what GDScript is doing (and what Unity does as well).

@chippmann
Copy link
Contributor

Yeah i see these points and agree. Let the property ordering do it's job then.

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