Variant from PtrToArg

[YakoYakoYokuYoku]

Required for the godotengine/godot-proposals#14507 proposal. Supersedes #1953 due to out of scope.

[dsnopek]

Thanks!

This is a small enough addition, that I think it can be considered. This is something we'll still need to discuss on RocketChat or at a GDExtension team meeting, though

[dsnopek]

If we do this, let's gate it with an #ifdef, because it could lead to some unintentional incompatibilities with Godot, which should probably be disabled by default

Something like:

Suggested change

	template <typename T, typename = std::void_t<decltype(PtrToArg<T>::encode_arg(std::declval<T>()))>>
	Variant(T v) :
	Variant(PtrToArg<T>::encode_arg(v)) {}
	#ifdef VARIANT_OPEN_CONVERSION_ENABLED
	template <typename T, typename = std::void_t<decltype(PtrToArg<T>::encode_arg(std::declval<T>()))>>
	Variant(T v) :
	Variant(PtrToArg<T>::encode_arg(v)) {}
	#endif

I don't particularly like that name, though - naming is hard :-)

[YakoYakoYokuYoku]

I've chosen to use an explicit here, although said incompatibilities should not appear due to this templated constructor appearing last while the rest of candidates are picked first.

I don't particularly like that name

I share the sentiment, probably some PRs could be made in the engine repo.

[YakoYakoYokuYoku]

Wait, that was already renamed in that repo with godotengine/godot#105231 PR. I'll put this on hold until that is synced here.

[Ivorforce]

This was brought up in the GDExtension meeting.

While there would be a way to make it less "compat breaking" (as suggested by david, or by having a bespoke specializable trait instead of using PtrToArg for it), I think this would be an uphill battle since it goes against godot-cpp's central tenet of being as close to Godot's API as possible. Adding this feature would solve your problem, but it wouldn't solve anybody else's, so we'd have to add more solutions for their problems...

What this points to, really, is that you make it your own, instead. Some ideas:

• Fork godot-cpp with this change and use it for your own GDExtension(s)
• Add the converter as operator Variant to your T, if you own it yourself.

We also discussed adding the ability to add a hook to modify the files on master as metaprogramming (like #2023 but for static files), but that didn't sit right with us since it doesn't sound like it would be much more convenient or powerful than just a fork.

[YakoYakoYokuYoku]

While there would be a way to make it less "compat breaking" (as suggested by david, or by having a bespoke specializable trait instead of using PtrToArg for it), I think this would be an uphill battle since it goes against godot-cpp's central tenet of being as close to Godot's API as possible.

What this points to, really, is that you make it your own, instead. Some ideas:

• Fork godot-cpp with this change and use it for your own GDExtension(s)
• Add the converter as operator Variant to your T, if you own it yourself.

I consider a better option to implement a helper interface library separate to Godot and just do punctual PRs to both the engine and the bindings if needed. By the way I've noticed that some changes to the main project that could be synced here, perhaps it could fulfill what is being addressed by this. I'm willing to do the task.

[dsnopek]

By the way I've noticed that some changes to the main project that could be synced here, perhaps it could fulfill what is being addressed by this. I'm willing to do the task.

If sync'ing Godot changes here would resolve this, then that would great, because that's something we'd want to do anyway!

[Naros]

So I ran into a similar issue today around Variant's alignment on Android devices, and perhaps Dave/Lukas know of a way to do this with SCons, but at least on CMake you can inject a shim to add this without needing to fork:

For us we needed to change variant.hpp from:

class Variant {
	uint8_t opaque[GODOT_CPP_VARIANT_SIZE]{ 0 };

to look like:

class Variant {
	alignas(8) uint8_t opaque[GODOT_CPP_VARIANT_SIZE]{ 0 };

godot-cpp-variant-alignment.cmake:

SET(VARIANT_HEADER "${CMAKE_CURRENT_SOURCE_DIR}/extern/godot-cpp/include/godot_cpp/variant/variant.hpp")
SET(VARIANT_SHIM_DIR "${CMAKE_BINARY_DIR}/godot-cpp-align-shim")

# Re-run configuration when the submodule's header changes, so a submodule bump
# regenerates the copy instead of silently reusing a stale one.
SET_PROPERTY(DIRECTORY APPEND PROPERTY CMAKE_CONFIGURE_DEPENDS "${VARIANT_HEADER}")

IF (NOT EXISTS "${VARIANT_HEADER}")
    MESSAGE(FATAL_ERROR "Cannot apply the Variant alignment shim, ${VARIANT_HEADER} does not exist. "
            "Has the godot-cpp submodule been checked out?")
ENDIF ()

FILE(READ "${VARIANT_HEADER}" VARIANT_SOURCE)

STRING(FIND "${VARIANT_SOURCE}" "alignas(8) uint8_t opaque[GODOT_CPP_VARIANT_SIZE]" VARIANT_ALREADY_ALIGNED)
IF (VARIANT_ALREADY_ALIGNED GREATER -1)
    MESSAGE(STATUS "godot-cpp aligns Variant upstream, alignment shim skipped - cmake/godot-cpp-variant-alignment.cmake can be removed")
ELSE ()
    STRING(REPLACE
            "uint8_t opaque[GODOT_CPP_VARIANT_SIZE]"
            "alignas(8) uint8_t opaque[GODOT_CPP_VARIANT_SIZE]"
            VARIANT_PATCHED "${VARIANT_SOURCE}")

    # Fail loudly rather than silently building an unshimmed, crash-prone binary
    # if godot-cpp ever restructures the declaration.
    IF (VARIANT_PATCHED STREQUAL VARIANT_SOURCE)
        MESSAGE(FATAL_ERROR "The Variant alignment shim did not match anything in ${VARIANT_HEADER}. "
                "The declaration changed upstream, so cmake/godot-cpp-variant-alignment.cmake must be "
                "updated or removed.")
    ENDIF ()

    FILE(WRITE "${VARIANT_SHIM_DIR}/variant.hpp.staged" "${VARIANT_PATCHED}")
    EXECUTE_PROCESS(COMMAND "${CMAKE_COMMAND}" -E copy_if_different
            "${VARIANT_SHIM_DIR}/variant.hpp.staged"
            "${VARIANT_SHIM_DIR}/godot_cpp/variant/variant.hpp")

    # BEFORE, and at directory scope prior to godot-cpp being added, so that the
    # shim precedes godot-cpp's own include directory for every target. godot-cpp
    # itself must see it too, otherwise the static library and the extension
    # disagree on the layout of anything holding a Variant.
    INCLUDE_DIRECTORIES(BEFORE "${VARIANT_SHIM_DIR}")

    MESSAGE(STATUS "Applied godot-cpp Variant alignment shim (GH-954)")
ENDIF ()

And then inside CMakeLists.txt, before godot-cpp:

# Correct the alignment of godot-cpp's Variant before godot-cpp is added, so that
# the shim's include directory is inherited by godot-cpp's targets as well.
INCLUDE(godot-cpp-variant-alignment)

Now we get the proper Android alignment without a godot-cpp fork. I suspect you should be able to do exactly the same in your environment for your constructor needs.