godot-kotlin-jvm icon indicating copy to clipboard operation
godot-kotlin-jvm copied to clipboard

Adding missing group Godot annotations

Open gabryon99 opened this issue 1 year ago • 8 comments

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
}

gabryon99 avatar Apr 10 '24 14:04 gabryon99

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

MartinHaeusler avatar Apr 10 '24 20:04 MartinHaeusler

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 avatar Apr 10 '24 20:04 chippmann

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

chippmann avatar Apr 10 '24 20:04 chippmann

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.

MartinHaeusler avatar Apr 10 '24 21:04 MartinHaeusler

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

chippmann avatar Apr 11 '24 05:04 chippmann

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.

CedNaru avatar Apr 11 '24 10:04 CedNaru

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).

MartinHaeusler avatar Apr 11 '24 16:04 MartinHaeusler

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

chippmann avatar Apr 12 '24 12:04 chippmann