godotengine / godot-cpp

C++ bindings for the Godot script API
MIT License
1.74k stars 576 forks source link

Bug when using the String module operator for Variant type. #1605

Open YooRarely opened 1 month ago

YooRarely commented 1 month ago

Tested versions

System information

Windows 11

Issue description

I found that the module_Variant method for the String type in GDExtension did not return the correct result. They all returned an incorrect result <null>. Could this be a bug? I haven't found any posts mentioning a similar issue.

Steps to reproduce

I tried the following code. The output on the left is correct, while the result on the right is incorrect.

        Variant iv = (int64_t) 10;
        Variant dv = 99.9;
        Variant bv = true;
        GD::print(iv," ",String("%s") % (int64_t)10, "  ", String("%s") % iv);
        GD::print(dv," ",String("%s") % 99.9, "  ", String("%s") % dv);
        GD::print(bv," ",String("%s") % true, "  ", String("%s") % bv);
10 10  <null>
99.9 99.9  <null>
true true  <null>

They each used different overloaded operators, as shown below.

// [incorrect]
String operator%(const Variant &p_other) const;
// [correct]
String operator%(bool p_other) const;
String operator%(int64_t p_other) const;
String operator%(double p_other) const;

source_code

String String::operator%(const Variant &p_other) const {
    return internal::_call_builtin_operator_ptr<String>(_method_bindings.operator_module_Variant, (GDExtensionConstTypePtr)&opaque, (GDExtensionConstTypePtr)&p_other);
}
String String::operator%(bool p_other) const {
    int8_t p_other_encoded;
    PtrToArg<bool>::encode(p_other, &p_other_encoded);
    return internal::_call_builtin_operator_ptr<String>(_method_bindings.operator_module_bool, (GDExtensionConstTypePtr)&opaque, (GDExtensionConstTypePtr)&p_other_encoded);
}

String String::operator%(int64_t p_other) const {
    int64_t p_other_encoded;
    PtrToArg<int64_t>::encode(p_other, &p_other_encoded);
    return internal::_call_builtin_operator_ptr<String>(_method_bindings.operator_module_int, (GDExtensionConstTypePtr)&opaque, (GDExtensionConstTypePtr)&p_other_encoded);
}

String String::operator%(double p_other) const {
    double p_other_encoded;
    PtrToArg<double>::encode(p_other, &p_other_encoded);
    return internal::_call_builtin_operator_ptr<String>(_method_bindings.operator_module_float, (GDExtensionConstTypePtr)&opaque, (GDExtensionConstTypePtr)&p_other_encoded);
}

So, I had to resort to the most cumbersome way, using a switch statement to solve the issue where the Variant type couldn't be handled correctly.

switch (p_value.get_type())
        {
        case Variant::NIL:
            s = str % nullptr;
            break;
        case Variant::BOOL:
            s = str % bool(p_value);
            break;
        case Variant::INT:
            s = str % int64_t(p_value);
            break;
        case Variant::FLOAT:
            s = str % double(p_value);
            break;
.....
.....
.....
        case Variant::PACKED_COLOR_ARRAY:
            s = str % PackedColorArray(p_value);
            break;
        case Variant::PACKED_VECTOR4_ARRAY:
            s = str % PackedVector4Array(p_value);
            break;
        default:
            s = str % p_value;
            break;
        }

Minimal reproduction project (MRP)

1

akien-mga commented 1 month ago

Moving to the godot-cpp as it's related to its own implementation of String.

dsnopek commented 2 weeks ago

I'm really surprised to learn we have these operator%(...) methods in godot-cpp!

Since this isn't supported in Godot itself, having them goes against one of godot-cpp design goals, which is to support the same API as Godot. I think it might be better to remove these operators, rather than fix them. We appear to be automatically generating them based on the extension_api.json, so it'd be a matter of filtering them out in binding_generator.py.

Using vformat() would be the recommended way to do this kind of string formatting in godot-cpp