godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.05k stars 65 forks source link

Allow Binding Methods from godot-cpp with Enum Arguments #9659

Closed kromenak closed 2 weeks ago

kromenak commented 2 weeks ago

Describe the project you are working on

A 2D platformer with online multiplayer.

Describe the problem or limitation you are having in your project

In C++, I have a few functions that take an enum argument (AudioType) for different audio buses. These functions are used to play sounds on different audio buses, change bus volume, or mute/unmute a bus.

When I call these functions from C++, they work great, all good:

mAudioManager->SetMuted(AUDIO_TYPE_MUSIC, true);

However, I wanted to write a settings/options UI in GDScript and have a UI checkbox toggle muting. To do that, I need to expose the "SetMuted" function to GDScript in the _bind_methods function:

ClassDB::bind_method(D_METHOD("SetMuted", "type", "muted"), &AudioManager::SetMuted);

This doesn't compile, since the enum (or enum class) can't be converted to Variant.

As a result, a workaround is for me to enumerate all AudioTypes to create separate mute functions (SetMusicMuted, SetSfxMuted). Not the end of the world, but not totally user-friendly.

Alternatively, I can create a function that doesn't use an enum, and just uses an integer, but THAT is not ideal because I lose that type info from the enum (potentially error prone and confusing).

The end result is a feeling that "ugh exposing stuff for GDScript is such a pain and/or limited in annoying ways."

Describe the feature / enhancement and how it helps to overcome the problem or limitation

From the perspective of someone using godot-cpp, it would be nice if binding a method with an enum parameter "just worked." Since enums can be represented with integers, and integers are supported by Variant, it seems reasonable.

In practice though, it seems two things must be done: 1) Define type info for the enum or enum class (ideally via a simple macro). 2) Update VariantCasters to detect enums and perform the appropriate conversion of Variant => int => enum.

Once this is done, the bind_method function happily accepts functions with enum parameters and exposes them successfully in GDScript.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

I discovered that enabling enum support for method binding doesn't seem very complex. I was able to get a working example on my end doing this.

First, for the enum in question, make use of some existing macros to generate needed type info boilerplate:

enum AudioType
{
    AUDIO_TYPE_MUSIC = 1,
    AUDIO_TYPE_SFX = 2
};
MAKE_TYPE_INFO(AudioType, GDEXTENSION_VARIANT_TYPE_INT);
MAKE_PTRARG(AudioType);

(it might be nice to define a single macro that does this, like MAKE_TYPE_INFO_ENUM(AudioType)).

Then, in binder_common.hpp, cases for enums can be added to VariantCaster:

template <class T>
struct VariantCaster {
    static _FORCE_INLINE_ T cast(const Variant &p_variant) {
        using TStripped = std::remove_pointer_t<T>;
        if constexpr (std::is_base_of<Object, TStripped>::value) {
                    return Object::cast_to<TStripped>(p_variant);
                }
                else if constexpr(std::is_enum<T>()) {
                    int64_t val = p_variant;
                    return static_cast<T>(val);
        } else {
               return p_variant;
        }
    }
};

After doing this, I am able to bind the method (and enum values) in _bind_methods:

ClassDB::bind_method(D_METHOD("SetMuted", "type", "muted"), &AudioManager::SetMuted);
BIND_CONSTANT(AUDIO_TYPE_MUSIC);
BIND_CONSTANT(AUDIO_TYPE_SFX);

And then can use them from GDScript successfully:

func OnMusicMuteToggled(on: bool):
    GlobalAudioManager.SetMuted(GlobalAudioManager.AUDIO_TYPE_MUSIC, on)

If this enhancement will not be used often, can it be worked around with a few lines of script?

I'm not sure how often this will be used, but I think it does work around an annoying limitation in the binding between C++ and GDScript. It can be worked around with some script, but supporting it doesn't seem too complex or add much overhead.

Is there a reason why this should be core and not an add-on in the asset library?

To support this, a change is required in the godot-cpp headers.

dsnopek commented 2 weeks ago

You should be able to setup your enum to cast to variant by adding something like this to your header file (below your class definition):

VARIANT_ENUM_CAST(MyAudioManager::AudioType);

Does that not work for some reason in your case?

kromenak commented 2 weeks ago

@dsnopek, argh, you're right, it did work! Even better, it shows the enum type in GDScript.

Sorry for the unnecessary proposal, but I overlooked that this was available. There are a lot of helpful macros in godot-cpp, but discovery and learning can be a bit of a challenge.

dsnopek commented 2 weeks ago

Glad it worked! :tada:

Yeah, we could certainly use more documentation.