Skip to content

Conversation

chippmann
Copy link
Contributor

@chippmann chippmann commented Aug 8, 2025

Resolves #835

Abstract

This bug is twofold:

  • enums returned from functions were wrongly registered as objects in all cases
  • registered enums (doesn't matter where) using Godot Enums which use custom ordinal ordering (represented with the id property on the kotlin side) were wrongly interpreted as we always used the kotlin ordinal for all manipulations

Possible solution

This PR fixes both of these issues and provide convenience functions for handling enum ordinals like godotOrdinal as an extension property on Enum<*> which is also used internally.

A new interface GodotEnum is introduced to be able to implement these ordinal shenanigans (basically just a common interface for generated godot enum to define a common api for the common id property we generated anyways for all of them).

A new Variant caster for enum types is introduced, which resolved these issues and makes the cutom enum registration obsolete (still required for enum flags and enum lists for now though).

@CedNaru Not sure if the VariantCaster is really the right tool for the job but sure is convenient here :-)

Needed discussion points

2 Things i don't like in my approach currently and until these are discussed, the PR stays in Draft mode:

  • The new variant caster is a class rather than an object. Meaning we allocate as many instances of it as there are enum registrations. Sometimes even multiple
  • We do a type check on each call to/from kotlin to/from godot for all enum registrations in order to get the "proper" godot ordinal of the enum

I personally can live with both of them, but they are not ideal. Maybe you have some ideas here.


sample of generated code:

before:

function(EnumRegistration::provideEnumValue, OBJECT, KtFunctionArgument(OBJECT, "godot.tests.TestEnum"), KtRpcConfig(DISABLED.id.toInt(), false, RELIABLE.id.toInt(), 0))
function(EnumRegistration::provideGodotErrorEnumValue, OBJECT, KtFunctionArgument(OBJECT, "godot.core.Error"), KtRpcConfig(DISABLED.id.toInt(), false, RELIABLE.id.toInt(), 0))
function(EnumRegistration::provideGodotEnumValue, OBJECT, KtFunctionArgument(OBJECT, "godot.api.Tween.EaseType"), KtRpcConfig(DISABLED.id.toInt(), false, RELIABLE.id.toInt(), 0))
enumProperty(EnumRegistration::enumValue, godot.core.PropertyUsageFlags.DEFAULT.flag, "ENUM_1,ENUM_2")
enumProperty(EnumRegistration::godotErrorEnumValue, godot.core.PropertyUsageFlags.DEFAULT.flag, "OK,FAILED,UNAVAILABLE,UNCONFIGURED,UNAUTHORIZED,PARAMETER_RANGE,OUT_OF_MEMORY,FILE_NOT_FOUND,FILE_BAD_DRIVE,FILE_BAD_PATH,FILE_NO_PERMISSION,FILE_ALREADY_IN_USE,FILE_CANT_OPEN,FILE_CANT_WRITE,FILE_CANT_READ,FILE_UNRECOGNIZED,FILE_CORRUPT,FILE_MISSING_DEPENDENCIES,FILE_EOF,CANT_OPEN,CANT_CREATE,QUERY_FAILED,ALREADY_IN_USE,LOCKED,TIMEOUT,CANT_CONNECT,CANT_RESOLVE,CONNECTION,CANT_ACQUIRE_RESOURCE,CANT_FORK,INVALID_DATA,INVALID_PARAMETER,ALREADY_EXISTS,DOES_NOT_EXIST,DATABASE_CANT_READ,DATABASE_CANT_WRITE,COMPILATION_FAILED,METHOD_NOT_FOUND,LINK_FAILED,SCRIPT_FAILED,CYCLIC_LINK,INVALID_DECLARATION,DUPLICATE_SYMBOL,PARSE,BUSY,SKIP,HELP,BUG,PRINTER_ON_FIRE")
enumProperty(EnumRegistration::godotEnumValue, godot.core.PropertyUsageFlags.DEFAULT.flag, "IN,OUT,IN_OUT,OUT_IN")

after:

function(EnumRegistration::provideEnumValue, ENUM<TestEnum>(TestEnum.entries.toTypedArray()), KtFunctionArgument(ENUM<TestEnum>(TestEnum.entries.toTypedArray()), "Int"), KtRpcConfig(DISABLED.id.toInt(), false, RELIABLE.id.toInt(), 0))
function(EnumRegistration::provideGodotErrorEnumValue, ENUM<Error>(Error.entries.toTypedArray()), KtFunctionArgument(ENUM<Error>(Error.entries.toTypedArray()), "Int"), KtRpcConfig(DISABLED.id.toInt(), false, RELIABLE.id.toInt(), 0))
function(EnumRegistration::provideGodotEnumValue, ENUM<EaseType>(EaseType.entries.toTypedArray()), KtFunctionArgument(ENUM<EaseType>(EaseType.entries.toTypedArray()), "Int"), KtRpcConfig(DISABLED.id.toInt(), false, RELIABLE.id.toInt(), 0))
property(EnumRegistration::enumValue, ENUM<TestEnum>(TestEnum.entries.toTypedArray()), ENUM<TestEnum>(TestEnum.entries.toTypedArray()), "Int", godot.core.PropertyHint.ENUM, "ENUM_1,ENUM_2", godot.core.PropertyUsageFlags.DEFAULT.flag)
property(EnumRegistration::godotErrorEnumValue, ENUM<Error>(Error.entries.toTypedArray()), ENUM<Error>(Error.entries.toTypedArray()), "Int", godot.core.PropertyHint.ENUM, "OK,FAILED,UNAVAILABLE,UNCONFIGURED,UNAUTHORIZED,PARAMETER_RANGE,OUT_OF_MEMORY,FILE_NOT_FOUND,FILE_BAD_DRIVE,FILE_BAD_PATH,FILE_NO_PERMISSION,FILE_ALREADY_IN_USE,FILE_CANT_OPEN,FILE_CANT_WRITE,FILE_CANT_READ,FILE_UNRECOGNIZED,FILE_CORRUPT,FILE_MISSING_DEPENDENCIES,FILE_EOF,CANT_OPEN,CANT_CREATE,QUERY_FAILED,ALREADY_IN_USE,LOCKED,TIMEOUT,CANT_CONNECT,CANT_RESOLVE,CONNECTION,CANT_ACQUIRE_RESOURCE,CANT_FORK,INVALID_DATA,INVALID_PARAMETER,ALREADY_EXISTS,DOES_NOT_EXIST,DATABASE_CANT_READ,DATABASE_CANT_WRITE,COMPILATION_FAILED,METHOD_NOT_FOUND,LINK_FAILED,SCRIPT_FAILED,CYCLIC_LINK,INVALID_DECLARATION,DUPLICATE_SYMBOL,PARSE,BUSY,SKIP,HELP,BUG,PRINTER_ON_FIRE", godot.core.PropertyUsageFlags.DEFAULT.flag)
property(EnumRegistration::godotEnumValue, ENUM<EaseType>(EaseType.entries.toTypedArray()), ENUM<EaseType>(EaseType.entries.toTypedArray()), "Int", godot.core.PropertyHint.ENUM, "IN,OUT,IN_OUT,OUT_IN", godot.core.PropertyUsageFlags.DEFAULT.flag)

@chippmann chippmann requested review from CedNaru and piiertho August 8, 2025 09:36
@CedNaru
Copy link
Member

CedNaru commented Aug 13, 2025

I named VariantCaster that way literally because Godot use that name to properly convert Enum to the right variant in the Godot sourcecode, so it's quite fitting.

@CedNaru
Copy link
Member

CedNaru commented Aug 13, 2025

The new variant caster is a class rather than an object. Meaning we allocate as many instances of it as there are enum registrations. Sometimes even multiple

This shouldn't be an issue. And actually, a bunch more are going to end up as instantiable in the future to solve some bugs we have currently with Variant that have generic types (necessary to fully support lambdas), so this is simply the first use for it.

One thing I would do is that id/ordinal never felt right for those enums so can't we rename it to just "value" ? Some of them are really just meaningless number but many also have true meaning behind them (that's why they are not purely incremented 1 by 1).

@CedNaru
Copy link
Member

CedNaru commented Aug 13, 2025

One optimization you could do is to use the new enum variant caster to take the different enum value as an array like you did, but then convert it to a map instead of storing the array directly.
That's way there is no need to search the entire list each time.

@chippmann chippmann force-pushed the bugfix/GH-835_fix-enum-registration branch from 1122fa0 to 1f602c1 Compare August 14, 2025 15:51
@chippmann chippmann marked this pull request as ready for review August 14, 2025 16:07
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

Successfully merging this pull request may close these issues.

[Bug] Registrator types the enum incorrectly when used as function return type and when using Godot Enums with custom ordinals
2 participants