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

Proposal: Rework exported enums and bitfields #692

Open
CedNaru opened this issue Sep 18, 2024 · 0 comments
Open

Proposal: Rework exported enums and bitfields #692

CedNaru opened this issue Sep 18, 2024 · 0 comments

Comments

@CedNaru
Copy link
Member

CedNaru commented Sep 18, 2024

Currently, the implementation of Enum and BitField we have is clunky and error-prone.
Here how it's done:

enum class TestEnum {
	ENUM_1,
	ENUM_2
}

@Export
@RegisterProperty
var enumList = listOf(TestEnum.ENUM_1) 

@Export
@RegisterProperty
@EnumTypeHint
var p6 = TestEnum.ENUM_1

@Export
@RegisterProperty
@EnumFlag
var p9 = setOf(TestEnum.ENUM_1)

The first is interpreted as an Array of Enum:
image

The second as a simple enum:
image

The third as a BitField:
image

This is not intuitive. It confused a few people who thought there was some automatic conversion between Kotlin collections and Godot. On top of that, the way to use a BitField as a set of Enum entries is not straightforward. On top of that, we only use the ordinal to set the values (directly the ordinal for enums and 2^Ordinal for flags), users can't set their own like this.

We need to rework that whole system and definitely ban the registration of Kotlin collections for such purpose.
Before coming with a new design, let's check what GDScript provides, as it's our main reference.

What's in GDScript ?

First, let's say that there is no real enum type in Godot, not in the Kotlin meaning of it. At best, they are like C enum, a simple namespace for a limited set of Integer constants. Godot 4 with GDScript 2 finally added a bit of type safety by allowing the use of enums directly by name, but when it comes to registering properties and function parameters to Godot, they are simple integers with a hint property.

Now a bit of copying and pasting from the official doc:

enum {TILE_BRICK, TILE_FLOOR, TILE_SPIKE, TILE_TELEPORT}

# Is the same as:
const TILE_BRICK = 0
const TILE_FLOOR = 1
const TILE_SPIKE = 2
const TILE_TELEPORT = 3

enum State {STATE_IDLE, STATE_JUMP = 5, STATE_SHOOT}

# Is the same as:
const State = {STATE_IDLE = 0, STATE_JUMP = 5, STATE_SHOOT = 6}
# Access values with State.STATE_IDLE, etc.

Now, when it comes to exporting properties, enums have kind of dual existence. They can act like proper type, but are also just a shortcut for hints:

Here are the two ways to export an "Enum".

enum NamedEnum {THING_1, THING_2, ANOTHER_THING = -1}
@export var x: NamedEnum   

@export_enum("Warrior", "Magician", "Thief") var character_class: int

The enum can be properly declared as its own type, or be declared directly inside the annotation.

Now here where thing are getting a bit more complex. So far, we only talked about Int, but it's not the whole story. When enums are used as a hint and not as a type, they can be used on String, Array and several PackedArrays (Byte, Int, Long, String):

@export_enum("Rebecca", "Mary", "Leah") var character_name: String
@export_enum("Espresso", "Mocha", "Latte", "Capuccino") var barista_suggestions: Array[String] = []

BitFlags are similar. The main difference is that flags are not a set of unique values, but any integer obtained from bitwise operations between it's predefined values, which make them incompatible with String.

@export_flags("Fire", "Water", "Earth", "Wind") var spell_elements = 0
@export_flags("Fire", "Water", "Earth", "Wind") var phase_elements: Array[int]
@export_flags("Fire", "Water", "Earth", "Wind") var phase_elements: PackedByteArray

In a nutshell:
Enum can be used both as a type alias for Int and as a Hint String for more compatible types.
Flags can only be used as a hint.

I can see 2 solutions forward for Kotlin. As usual, a simple and complex proposals.

Solution 1:

The simple approach is to not allow using enums/flag as type at all, and only use them for hints in the annotation.
It would look like this:

enum class TestEnum: GodotEnum {
	ENUM_1(0),
	ENUM_2(1)
}

// Enum
@ExportEnum(TestEnum::class)
var p1 = 0

@ExportEnum(TestEnum::class)
var p2 = VariantArray<Int>() 

@ExportEnum(TestEnum::class)
var p3 = VariantArray<String>() 

@ExportEnum(TestEnum::class)
var p4 = PackedByteArray() 

enum class TestFlag: GodotFlag {
	FLAG1(1),
	FLAG2(2),
	FLAG3(4),
}

@ExportFlag(TestFlag::class)
var p5 = 0

@ExportFlag(TestFlag::class)
var p6 = VariantArray<Int>() 

A mandatory interface GodotEnum and GodotFlag would have to be implemented to provide easier Flag operations (bitwise operations for flags) and to allow the user to set its own value for the enum if they don't want to follow the natural ordinal ordering (optional for enum but mandatory for flags).

Solution 2:

This solution is basically an extension of the first, assume that everything said previously is valid unless said otherwise.
The additional complexity comes from treating Enum and Flag as proper types.
One major flaw of the first solution is that converting back values is cumbersome and error-prone:

Let's take the simple example given previously:

enum class TestEnum: GodotEnum {
	ENUM_1(0),
	ENUM_2(1)
}

// Enum
@ExportEnum(TestEnum::class)
var p1 = 3

Because p1 is still an Int, the user can easily set a value that doesn't fit any of the Enum values defined.
Here I assigned the value 3 when only 0 and 1 are supposed to be possible.
On top of that, if you want to convert it back to an enum for other Kotlin operations you need to write things like

val myEnum = TestEnum.entries.map{ it.id }.find(p1)  // Would fail if the error is not within the range of the Enum

You first need to get the list of all enum, map them to their ID (because remember we can't use ordinal here) and finally find the right one there. It's boilerplate, error-prone and inefficient.

Everything would be much nicer if we could simply write something like:

enum class TestEnum: GodotEnum {
	ENUM_1(0),
	ENUM_2(1)
}

@Export
var p1 = TestEnum.ENUM_1

The conversion to/from Int would be internally handled by the registration code and no error is possible any more
Now, why is that more complex to implement ? That would be simple enough if the enum and flag to use were only the ones part of the Godot API because we could handle the generation and do whatever we want with it. But here we have to deal with user defined Enums and Flags. I won't give all the issues, but here 2 I can think of:

If we want to push the logic further then something like this should be allowed

enum class TestEnum: GodotEnum {
	ENUM_1(0),
	ENUM_2(1)
}

@Export
var p1 = VariantArray<TestEnum>

The challenge here is that the conversion wouldn't be done by the registration code itself but would need to be handled by the VariantArray, or at least by the VariantConverters. Not saying, it's not possible, we already add VariantConverters for users scripts so they are able to insert their scripts into VariantArray and Dictionary, but it's still extra complexity to handle.

Now the coup de grâce. Let simply switch to a Flag:

enum class TestFlag: GodotFlag {
	FLAG1(1),
	FLAG2(2),
	FLAG3(4),
}

@Export
var p1 = VariantArray<TestFlag>

This one is not easy.
The actual Integer stored inside this array can be any bit combination of FLAG1, FLAG2, FLAG3. Basically, we can convert a flag back to an Integer, but we have no way to convert it back to a Flag.

In the API generation, we handle flag with the following strategy (simplified for the example):

sealed interface TestFlag: GodotFlag {
	object FLAG1(1): TestFlag,
	object FLAG2(2): TestFlag,
	object FLAG3(4): TestFlag,
        class CustomFlag(value: Int): TestFlag,

    // Lot of bitwise function
}

This allows to write things like Flag1 OR FLAG4 OR 128 and still get a value that is considered to be of the type TestFlag. It's simple enough to do with the API generation, but I have my doubts about forcing users to write such a convoluted class. On top of that, it's hard to check if it's properly implemented beforehand for the compiler plugin.

Should we still go along such complexity or only allow Enum to be fully typed and let flags be Hint only ?

Going with the latter would be that Enum would work like in Solution 1, but Flag would work like in Solution 2:

@Export
var p1 = TestEnum.ENUM_1

@ExportFlag(TestFlag::class)
var p2 = 0

What do you think ?

@CedNaru CedNaru changed the title Rework exported Enum and BitField Proposal: rework exported enums and bitfields Sep 18, 2024
@CedNaru CedNaru changed the title Proposal: rework exported enums and bitfields Proposal: Rework exported enums and bitfields Sep 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant