Extension bindings for new `RenderingServer::ArrayFormat` are incorrect

[mihe]

Godot version

4.2-rc [b3a0e077e]

System information

Windows 11 (10.0.22621)

Issue description

With the new vertex compression introduced in #81138 the RenderingServer::ArrayFormat enum was changed to use an underlying type of uint64_t as opposed to the default int32_t int, as seen here:

https://github.com/godotengine/godot/blob/eda44bfe109d7f15c5a965db2d65c8a17ce4b7d0/servers/rendering_server.h#L255-L299

As far as I can tell, changing the underlying type for an enum is not something that extension_api.json supports at the moment. This leads to ArrayFormat being generated as such in godot-cpp:

    enum ArrayFormat {
        ARRAY_FORMAT_VERTEX = 1,
        ARRAY_FORMAT_NORMAL = 2,
        ARRAY_FORMAT_TANGENT = 4,
        ARRAY_FORMAT_COLOR = 8,
        ARRAY_FORMAT_TEX_UV = 16,
        ARRAY_FORMAT_TEX_UV2 = 32,
        ARRAY_FORMAT_CUSTOM0 = 64,
        ARRAY_FORMAT_CUSTOM1 = 128,
        ARRAY_FORMAT_CUSTOM2 = 256,
        ARRAY_FORMAT_CUSTOM3 = 512,
        ARRAY_FORMAT_BONES = 1024,
        ARRAY_FORMAT_WEIGHTS = 2048,
        ARRAY_FORMAT_INDEX = 4096,
        ARRAY_FORMAT_BLEND_SHAPE_MASK = 7,
        ARRAY_FORMAT_CUSTOM_BASE = 13,
        ARRAY_FORMAT_CUSTOM_BITS = 3,
        ARRAY_FORMAT_CUSTOM0_SHIFT = 13,
        ARRAY_FORMAT_CUSTOM1_SHIFT = 16,
        ARRAY_FORMAT_CUSTOM2_SHIFT = 19,
        ARRAY_FORMAT_CUSTOM3_SHIFT = 22,
        ARRAY_FORMAT_CUSTOM_MASK = 7,
        ARRAY_COMPRESS_FLAGS_BASE = 25,
        ARRAY_FLAG_USE_2D_VERTICES = 33554432,
        ARRAY_FLAG_USE_DYNAMIC_UPDATE = 67108864,
        ARRAY_FLAG_USE_8_BONE_WEIGHTS = 134217728,
        ARRAY_FLAG_USES_EMPTY_VERTEX_ARRAY = 268435456,
        ARRAY_FLAG_COMPRESS_ATTRIBUTES = 536870912,
        ARRAY_FLAG_FORMAT_VERSION_BASE = 35,
        ARRAY_FLAG_FORMAT_VERSION_SHIFT = 35,
        ARRAY_FLAG_FORMAT_VERSION_1 = 0,
        ARRAY_FLAG_FORMAT_VERSION_2 = 34359738368,
        ARRAY_FLAG_FORMAT_CURRENT_VERSION = 34359738368,
        ARRAY_FLAG_FORMAT_VERSION_MASK = 255,
    };

This seems to result in the compiler (MSVC in this case) truncating any enum flag larger than 2^31 down to 0, while also emitting warnings about this, which means that actually using ARRAY_FLAG_FORMAT_VERSION_2 or ARRAY_FLAG_FORMAT_CURRENT_VERSION, and thus conforming to this new vertex format without Godot emitting warnings, is impossible from godot-cpp without specifying the actual literal numerical value instead.

Steps to reproduce

N/A

Minimal reproduction project

N/A

[AThousandShips]

Cc @clayjohn @dsnopek

[dsnopek]

Hm. GDExtension always encodes all integers as int64_t, so I suspect this isn't a Godot issue, but a godot-cpp one?

I've never used this syntax, I just Google'd it up, but perhaps godot-cpp should be generating all enums as:

enum ArrayFormat : __int64 {
  // ...
}

... to specify that 64-bit integers should be used for enums?

[AThousandShips]

~Enum might be int depending on platform I'm not sure, but it should generate some warning if it truncates I believe~

Apparently, based on the standard (according to cppreference):

this type is not larger than int unless the value of an enumerator cannot fit in an int or unsigned int

If the underlying type is not fixed, the value is convertible to the first type from the following list able to hold their entire value range: int, unsigned int, long, unsigned long, long long, or unsigned long long, extended integer types with higher conversion rank (in rank order, signed given preference over unsigned)

This could of course be an MSVC issue as MSVC is infamous for the weird ways it fails to conform to the standard at times

Can you confirm this on other compilers such as clang? In testing clang does correctly handle enums of this scale, but can't test msvc right now

[akien-mga]

See https://github.com/godotengine/godot/pull/61757 where @lawnjelly dealt with a similar MSVC issue with enums.

[AThousandShips]

Then this seems to be godot-cpp specific, should we transfer? As the bindings aren't directly affected AFAIK

[akien-mga]

There's three options I can see:

• Either bindings like godot-cpp should default everything to a big enough type, e.g. int64_t as we sometime used signed enum values, but then what about if we actually start using uint64_t enums in Godot with values beyond INT64_MAX?
• Or we need to pass that information in the JSON, and thus figure out how to register it. Could be manually added when registering the binding, but that could be prone to user errors, or maybe inferred from the enum itself?
• Or a variant of the second point, which would be to only register that info manually when the enum differs from the default size. Which is fairly rare, so shouldn't be too hard to keep up to date. This list could alternatively be handled downstream, but there's more risk of desync.

[mihe]

MSVC does seem to be the outlier here. Clang and GCC both seem to handle this gracefully, whereas MSVC emits C4309 for both ARRAY_FLAG_FORMAT_VERSION_2 and ARRAY_FLAG_FORMAT_CURRENT_VERSION and truncates to 0.

Either bindings like godot-cpp should default everything to a big enough type, e.g. int64_t as we sometime used signed enum values, but then what about if we actually start using uint64_t enums in Godot with values beyond INT64_MAX?

While enums for the most part seem to be transported across the ABI as int64_t, thanks to VARIANT_ENUM_CAST and such, I've found at least one exception, which is the CaretInfo struct, which happens to have TextServer::Direction in it, as seen here:

https://github.com/godotengine/godot/blob/eda44bfe109d7f15c5a965db2d65c8a17ce4b7d0/servers/register_server_types.cpp#L128-L128

https://github.com/godotengine/godot/blob/eda44bfe109d7f15c5a965db2d65c8a17ce4b7d0/servers/text_server.h#L65-L70

So if Godot and godot-cpp start to differ on how big this particular enum is you'll run into trouble when reading/writing those variables.

I'm noticing now though that RenderingServer::ArrayFormat is bound through VARIANT_BITFIELD_CAST as opposed to VARIANT_ENUM_CAST, which leads it to getting marked as "is_bitfield": true in extension_api.json. Maybe the pragmatic solution for this particular case is as simple as just adding : uint64_t in godot-cpp's code generation when the enum is marked as a bit field?

[mihe]

I took the liberty of creating a PR (godotengine/godot-cpp#1320) for the solution mentioned in my previous comment, in case you just want to resolve the more immediate problem here without any changes to Godot itself.

There's obviously still a bigger discussion to be had though, with regards to being more explicit about the underlying types of enums.

[lawnjelly]

I was just looking to see if there was a compiler switch to change the MSVC behaviour, but that doesn't protect against future compilers that might default to signed enum.

One option is to enforce all enums being unsigned via the static checks (in Godot core) and then force either uint32_t or uint64_t in godot cpp.

It of course only tends to be a problem when they are used as bitfields, but there's no super obvious automatic way of detecting this, so the same bug is likely to come up again and again periodically.

[mihe]

One option is to enforce all enums being unsigned via the static checks (in Godot core) and then force either uint32_t or uint64_t in godot cpp.

It of course only tends to be a problem when they are used as bitfields, but there's no super obvious automatic way of detecting this, so the same bug is likely to come up again and again periodically.

Seeing as how bitfields are bound through a separate macro this would be fairly easy to enforce with the help of std::underlying_type and static_assert.

Here's a quick proof-of-concept: https://github.com/godotengine/godot/compare/master...mihe:godot:enforced-enum-types

This will trigger a compilation error if something bound through VARIANT_ENUM_CAST isn't of underlying type int and if something bound through VARIANT_BITFIELD_CAST isn't of underlying type uint64_t.

The ideal solution here would however (in my opinion) be for this information to explicitly be part of extension_api.json instead of being a handshake agreement between Godot and any extension language bindings. This information should be fairly easy to extract/enforce automatically thanks to std::underlying_type a couple of if constexpr checks that check to see whether the enum is int or one of the fixed-width integer types.

[mihe]

I was just looking to see if there was a compiler switch to change the MSVC behaviour, but that doesn't protect against future compilers that might default to signed enum.

For future reference, here is the compiler flag.

It seems like they had to leave it out of the standard-conformance flag (/permissive-) due to it being a breaking change with the Windows SDK.

[dsnopek]

The ideal solution here would however (in my opinion) be for this information to explicitly be part of extension_api.json instead of being a handshake agreement between Godot and any extension language bindings.

Personally, I think enums should be part of the GDExtension ABI simply as integers, rather than trying to bind them too closely to C++ enums.

Technically, GDExtension is supposed to be a "C API" (although, I think it's really more like a "C ABI", since we don't require that a C compiler is used to interact with GDExtension, just that a C compatible ABI is used), and in C the size of an enum is even more ambiguous: the best I can tell via Google, C enums are always int-sized and we have no way to explicitly specify the storage.

So, I think GDExtension should simply be specifying that an enum value is passed to/from GDExtension as a 64-bit integer (like all integers presently), and how that's actually realized in a particular language is up to the language binding. So, in godot-cpp we've decided to use C++ enums which means we have to contend with their quirks. In a theoretical C binding, maybe C enums would be used, or maybe #defines? Each language would figure out how best to represent it.

The one issue is with GDREGISTER_NATIVE_STRUCT(), but given that it's supposed to hold a C struct and C is ambiguous about the size of enums, I think we should probably have a rule against using enums in native structs.

Something good to discuss at the next GDExtension meeting! (I've already put it on the agenda :-))

[mihe]

and in C the size of an enum is even more ambiguous: the best I can tell via Google, C enums are always int-sized and we have no way to explicitly specify the storage.

I'm not sure I agree with it being ambiguous. cppreference.com, which despite the name also provides a reference for the C language standard, seems quite clear about it using int as its underlying type (up until C23, but I assume GDExtension targets something like C99). While int isn't strictly guaranteed to be 32-bit, it should be 32-bit on any platform and architecture relevant to Godot that I'm aware of.

There are popular and widely used C libraries that expose enums as part of their interface just fine. libclang springs to mind as one that also has bindings for many other languages.

With that said, the fact that enums are already being transported across the ABI as int64_t in every case except for in GDREGISTER_NATIVE_STRUCT() might be reason enough to just rip the CaretInfo compatibility breakage band-aid and explicitly declare them all to be 64-bit, as you say. Having the enum size provided as part of extension_api.json and then still always have them be passed around as int64_t would be somewhat strange, now that I think about it.

If the compatbility breakage is off the table, then the only other solution I can see is to enforce that all VARIANT_ENUM_CAST enums are of underlying type int (or int32_t I guess) and enforce that all VARIANT_BITFIELD_CAST enums are of underlying type uint64_t, or that they're only ever used in GDREGISTER_NATIVE_STRUCT() as BitField<T>.

[dsnopek]

While int isn't strictly guaranteed to be 32-bit, it should be 32-bit on any platform and architecture relevant to Godot that I'm aware of.

Yeah, the undefined size of int is what I meant by it being ambiguous. But, you're right, it's effectively always 32-bit :-)