utopia-rise / godot-kotlin-jvm

Godot Kotlin JVM Module
MIT License
576 stars 39 forks source link

feat(api-gen): Generate bitfields using interface, value class and statics #524

Closed piiertho closed 10 months ago

piiertho commented 10 months ago

This resolves https://github.com/utopia-rise/godot-kotlin-jvm/issues/497

CedNaru commented 10 months ago

Do we really need those XTrait interfaces ? It seems everything could be directly moved inside the sealed classes (which are just abstract classes already) and would bloat the generated code less.

Something like this, taking KeyModifiedMask as example:

public sealed class KeyModifierMask(
    public val flag: Int,
){
    public data object KEY_CODE_MASK : KeyModifierMask(8388607)
    public data object KEY_MODIFIER_MASK : KeyModifierMask(532676608)
    public data object KEY_MASK_CMD_OR_CTRL : KeyModifierMask(16777216)
    public data object KEY_MASK_SHIFT : KeyModifierMask(33554432)
    public data object KEY_MASK_ALT : KeyModifierMask(67108864)
    public data object KEY_MASK_META : KeyModifierMask(134217728)
    public data object KEY_MASK_CTRL : KeyModifierMask(268435456)
    public data object KEY_MASK_KPAD : KeyModifierMask(536870912)
    public data object KEY_MASK_GROUP_SWITCH : KeyModifierMask(1073741824)
    public class CustomFlag internal constructor(flag: Int) : KeyModifierMask(flag)

    public infix fun or(other: KeyModifierMask): KeyModifierMask = CustomFlag(flag or other.flag)
    public infix fun or(other: Int): KeyModifierMask = CustomFlag(flag or other)
    public infix fun xor(other: KeyModifierMask): KeyModifierMask = CustomFlag(flag xor other.flag)
    public infix fun xor(other: Int): KeyModifierMask = CustomFlag(flag xor other)
    public infix fun and(other: KeyModifierMask): KeyModifierMask = CustomFlag(flag and other.flag)
    public infix fun and(other: Int): KeyModifierMask = CustomFlag(flag and other)
}

We lose the value class for Custom flag with this implementation but the "value" was already useless anyway. The moment you send it as a KeyModifierMask parameter and not a CustomFlag parameter, it's going to be autoboxed. Any polymorphic context removes the value and transforms it into a regular object, and there is no case where it's not going to happen in the context of Godot bitfields.

Also careful about the use of either Long or Int to carry the value. Right now the Int values are sent alongside a VariantType.Long when written to the buffer. Regular enums are using Long to store their values if I remember. I don't know if Godot ever go above the limits of 32 bits for enums. Depending on that, we could just move everything to Int. But regardless, we need consistency on that front.

piiertho commented 10 months ago

Do we really need those XTrait interfaces ? It seems everything could be directly moved inside the sealed classes (which are just abstract classes already) and would bloat the generated code less.

Something like this, taking KeyModifiedMask as example:

public sealed class KeyModifierMask(
    public val flag: Int,
){
    public data object KEY_CODE_MASK : KeyModifierMask(8388607)
    public data object KEY_MODIFIER_MASK : KeyModifierMask(532676608)
    public data object KEY_MASK_CMD_OR_CTRL : KeyModifierMask(16777216)
    public data object KEY_MASK_SHIFT : KeyModifierMask(33554432)
    public data object KEY_MASK_ALT : KeyModifierMask(67108864)
    public data object KEY_MASK_META : KeyModifierMask(134217728)
    public data object KEY_MASK_CTRL : KeyModifierMask(268435456)
    public data object KEY_MASK_KPAD : KeyModifierMask(536870912)
    public data object KEY_MASK_GROUP_SWITCH : KeyModifierMask(1073741824)
    public class CustomFlag internal constructor(flag: Int) : KeyModifierMask(flag)

    public infix fun or(other: KeyModifierMask): KeyModifierMask = CustomFlag(flag or other.flag)
    public infix fun or(other: Int): KeyModifierMask = CustomFlag(flag or other)
    public infix fun xor(other: KeyModifierMask): KeyModifierMask = CustomFlag(flag xor other.flag)
    public infix fun xor(other: Int): KeyModifierMask = CustomFlag(flag xor other)
    public infix fun and(other: KeyModifierMask): KeyModifierMask = CustomFlag(flag and other.flag)
    public infix fun and(other: Int): KeyModifierMask = CustomFlag(flag and other)
}

We lose the value class for Custom flag with this implementation but the "value" was already useless anyway. The moment you send it as a KeyModifierMask parameter and not a CustomFlag parameter, it's going to be autoboxed. Any polymorphic context removes the value and transforms it into a regular object, and there is no case where it's not going to happen in the context of Godot bitfields.

Also careful about the use of either Long or Int to carry the value. Right now the Int values are sent alongside a VariantType.Long when written to the buffer. Regular enums are using Long to store their values if I remember. I don't know if Godot ever go above the limits of 32 bits for enums. Depending on that, we could just move everything to Int. But regardless, we need consistency on that front.

After testing with bytecode viewer it seems it's sometime inline, sometime not.
Interface has the counterpart to enable user implem, which we don't want.
So we'll lose value class but not a big deal.

CedNaru commented 10 months ago

The issue with the way it's implemented is that each enum value is a kotlin singleton, which ends up as its own class. The solution would be to discard the use of sealed class containing objects and use a sealed interface instead:

public sealed interface KeyModifierMask {
    public val flag: Int

   @JvmInline
   value class CustomFlag internal constructor(override val flag: Int) : KeyModifierMask 
}

enum class KeyModifierMaskConstants(override val flag: Int) : KeyModifierMask {
    KEY_CODE_MASK(8388607),
    KEY_MODIFIER_MASK(532676608),
    KEY_MASK_CMD_OR_CTRL(16777216),
    KEY_MASK_SHIFT(33554432),
    KEY_MASK_ALT(67108864),
    KEY_MASK_META(134217728),
    KEY_MASK_CTRL(268435456),
    KEY_MASK_KPAD(536870912),
    KEY_MASK_GROUP_SWITCH(1073741824);
}

With this we are back to enums with the exception of a custom implementation in the parent sealed interface. But we still introduce 2 different names. All instances users will manipulate are in the "Contant" enum, but api methods will require an instance of the parent interface, which can lead to confusion. If we want to go even further, we could entirely get rid of the enum and only use values inside a companion object for the sealed interface.


public sealed interface KeyModifierMask {
    public val flag: Int

    @JvmInline
    value class BitFlagValue internal constructor(override val flag: Int) : KeyModifierMask

    companion object {
        val KEY_CODE_MASK: KeyModifierMask = BitFlagValue(8388607)
        val KEY_MODIFIER_MASK: KeyModifierMask = BitFlagValue(532676608)
        val KEY_MASK_CMD_OR_CTRL: KeyModifierMask = BitFlagValue(16777216)
        val KEY_MASK_SHIFT: KeyModifierMask = BitFlagValue(33554432)
        val KEY_MASK_ALT: KeyModifierMask = BitFlagValue(67108864)
        val KEY_MASK_META: KeyModifierMask = BitFlagValue(134217728)
        val KEY_MASK_CTRL: KeyModifierMask = BitFlagValue(268435456)
        val KEY_MASK_KPAD: KeyModifierMask = BitFlagValue(536870912)
        val KEY_MASK_GROUP_SWITCH: KeyModifierMask = BitFlagValue(1073741824)
    }
}

We lose pattern matching but it shouldn't matter for bitfields that are supposed to be composite values anyway.