Problem with "17/17:RichTextEffect" type in generated api.json

[piiertho]

Hi there,

I got a problem parsing generated api.json for kotlin GDNative.
Here is the branch used on project: https://github.com/utopia-rise/kotlin-godot-wrapper/tree/feature/update-to-godot-3.2
Got a problem with the type of property custom_effects in RichTextLabel:
"type": "17/17:RichTextEffect"
Does someone know what 17/17 stands for ?

[CedNaru]

"17/17:RichTextEffect" is originally the hint_string registered with the property but somehow it ends up exported as the type in the api.json.

ADD_PROPERTY(PropertyInfo(Variant::ARRAY, "custom_effects", (PropertyHint)(PROPERTY_USAGE_DEFAULT | PROPERTY_USAGE_SCRIPT_VARIABLE), "17/17:RichTextEffect", PROPERTY_USAGE_DEFAULT, "RichTextEffect"), "set_effects", "get_effects");

I don't know enough about the flags to know which one does what. But it seems that PropertyUsageFlags are used instead of PropertyHint.

In the api_generator.cpp file, the type returned for a given class (get_type_name function) depends on several if-cases. The one in cause seems to be:

if (info.hint == PROPERTY_HINT_RESOURCE_TYPE) { return info.hint_string; }

[BastiaanOlij]

I'd like to close this issue. The export is correct, we just export what is supplied to the ClassDB.

That said, this requires an explanation of how to parse the string. Looking at godot-cpp it pretty much ignores it because it only parses the getter and setter and they are correctly classified as Array.
That really is a question to be asked in more general Godot channels as obviously Godot has an internal way of dealing with this.

[akien-mga]

This hint is correct but it's indeed the only one using this feature, and it's pretty cryptic.

See godotengine/godot#40383 which aimed at fixing it, but introduced a regression in the actual feature that this enables for inspector hints.

This was later fixed by godotengine/godot#49906 which gives additional information on the format, basically this is a hint that this property is an Object (Variant::Type::OBJECT is 17) with a PROPERTY_HINT_RESOURCE_TYPE (also 17), and the resource type is RichTextEffect. So when you try to set this property in the inspector, it properly lets you select RichTextEffects only.

[CedNaru]

So where is the responsability ? Should the way we register it to the ClassDB change or should the generation of the json be adapted ?
In both case, on the master branch, the json generation is now directly in core if I remember so I guess we can remove that issue from this repo.
I didn't check if that issue is still present in the new api_dump file.
And just to be sure. The issue is not that we didn't know how to parse that string. It was that the hint_string replaced the type of the property in the api.json. We did manage to have a workaround by using the type of the getters/setters, but it's still a workaround made because of a legit error in the json.

[akien-mga]

I think the JSON generation should change, indeed it shouldn't use this hint as a type.

It's an awkward hint and we might also work on improving this at some point, but for the JSON I guess the type should be Array?

[vnen]

The generation should not use property hints. It should actually get the return type info from the getter method, which is much more reliable and it shows what the property actually returns when used.

[CedNaru]

It seems that Vnen already fixed the line that caused the export of the hint_string a while ago.
godotengine/godot@0b3819d#diff-f40ea62cd905de12599e52a3a692bf9ca374918b13e2bb5722acb94413e9fe92

Edit: But looking at the api.json in the 3.x branch, the hint_string seems to still be exported so it's not fixed.

[akien-mga]

It seems that Vnen already fixed the line that caused the export of the hint_string a while ago.
godotengine/godot@0b3819d#diff-f40ea62cd905de12599e52a3a692bf9ca374918b13e2bb5722acb94413e9fe92

So the issue should be fixed by now, but need to check.

That fix looks weird though:

if (info.class_name != StringName()) {
	return info.class_name;
}
if (info.hint == PROPERTY_HINT_RESOURCE_TYPE) {
	return info.class_name;
}

info.class_name will always be empty then given that it did not pass the first if.

I think it's better to outright remove the special case here and let it fall back to return Variant::get_type_name(info.type);.

This should also be done in 3.x.

Edit: Well strangely enough I see no change to the API in 3.x with this diff:

diff --git a/modules/gdnative/nativescript/api_generator.cpp b/modules/gdnative/nativescript/api_generator.cpp
index 9f83095f00..82dc7f219d 100644
--- a/modules/gdnative/nativescript/api_generator.cpp
+++ b/modules/gdnative/nativescript/api_generator.cpp
@@ -126,9 +126,6 @@ static String get_type_name(const PropertyInfo &info) {
        if (info.class_name != StringName()) {
                return info.class_name;
        }
-       if (info.hint == PROPERTY_HINT_RESOURCE_TYPE) {
-               return info.hint_string;
-       }
        if (info.type == Variant::NIL && (info.usage & PROPERTY_USAGE_NIL_IS_VARIANT)) {
                return "Variant";
        }
@@ -351,8 +348,6 @@ List<ClassAPI> generate_c_api_classes() {
                                        if (arg_info.name.find(":") != -1) {
                                                arg_type = arg_info.name.get_slice(":", 1);
                                                arg_name = arg_info.name.get_slice(":", 0);
-                                       } else if (arg_info.hint == PROPERTY_HINT_RESOURCE_TYPE) {
-                                               arg_type = arg_info.hint_string;
                                        } else if (arg_info.type == Variant::NIL) {
                                                arg_type = "Variant";
                                        } else if (arg_info.type == Variant::OBJECT) {

But I can confirm that the api.json is fine in master:

                        {
                                "name": "custom_effects",
                                "type": "Array",
                                "getter": "get_effects",
                                "setter": "set_effects",
                                "index": -1
                        },