godotengine / godot-cpp

C++ bindings for the Godot script API
MIT License
1.76k stars 580 forks source link

How to register a property of type material? #237

Closed mnn closed 5 years ago

mnn commented 5 years ago

I tried many ways, but none is working for me. Is there an example repository, or a tutorial, or docs?

There are virtually no examples beyond extremely basic ones (in GDNative-demos I didn't found any property which isn't of primitive type) and "docs" are redirecting to compilation of the engine, why?

My last attempt with Variant ended with editor crash:

[1] /lib/x86_64-linux-gnu/libc.so.6(+0x354b0) [0x7fd8e8ef44b0] (??:0)
[2] /xxx/Godot_v3.1-beta4_x11_64/Godot_v3.1-beta4_x11.64() [0x214619c] (??:?)
[3] /xxx/Godot_v3.1-beta4_x11_64/Godot_v3.1-beta4_x11.64() [0xca5881] (??:?)
[4] godot::Object* godot::get_wrapper<godot::Object>(void*) (??:0)
[5] void godot::register_property<godot::CPP, godot::Variant>(char const*, godot::Variant godot::CPP::*, godot::Variant, godot_method_rpc_mode, godot_property_usage_flags, godot_property_hint, godot::String) (??:0)
[6] godot::CPP::_register_methods() (??:0)
[7] void godot::register_class<godot::CPP>() (??:0)
[8] /mnt/veracrypt/old_ssd_data/workspace/godot/ugb-godot-3.1.beta3-cpp-2/demo/bin/x11/libgdexample.so(godot_nativescript_init+0x13) [0x7fd8c75a495b] (??:0)
[9] /xxx/Godot_v3.1-beta4_x11_64/Godot_v3.1-beta4_x11.64() [0xf49b6c] (??:?)
[10] /xxx/Godot_v3.1-beta4_x11_64/Godot_v3.1-beta4_x11.64() [0xa71a0f] (<artificial>:?)
[11] /xxx/Godot_v3.1-beta4_x11_64/Godot_v3.1-beta4_x11.64() [0xad649c] (<artificial>:?)
[12] /xxx/Godot_v3.1-beta4_x11_64/Godot_v3.1-beta4_x11.64() [0x20f3bdf] (??:?)
[13] /xxx/Godot_v3.1-beta4_x11_64/Godot_v3.1-beta4_x11.64() [0x219e151] (??:?)
[14] /xxx/Godot_v3.1-beta4_x11_64/Godot_v3.1-beta4_x11.64() [0x1d5cbf6] (<artificial>:?)
[15] /xxx/Godot_v3.1-beta4_x11_64/Godot_v3.1-beta4_x11.64() [0x2367bd7] (??:?)
[16] /xxx/Godot_v3.1-beta4_x11_64/Godot_v3.1-beta4_x11.64() [0xe15eb4] (??:?)
[17] /xxx/Godot_v3.1-beta4_x11_64/Godot_v3.1-beta4_x11.64() [0x2583a0e] (??:?)
[18] /xxx/Godot_v3.1-beta4_x11_64/Godot_v3.1-beta4_x11.64() [0x23463f2] (??:?)
[19] /xxx/Godot_v3.1-beta4_x11_64/Godot_v3.1-beta4_x11.64() [0x1513ea0] (<artificial>:?)
[20] /xxx/Godot_v3.1-beta4_x11_64/Godot_v3.1-beta4_x11.64() [0x1514558] (<artificial>:?)
[21] /xxx/Godot_v3.1-beta4_x11_64/Godot_v3.1-beta4_x11.64() [0xd11250] (??:?)
[22] /xxx/Godot_v3.1-beta4_x11_64/Godot_v3.1-beta4_x11.64() [0x1039c12] (<artificial>:?)
[23] /xxx/Godot_v3.1-beta4_x11_64/Godot_v3.1-beta4_x11.64() [0x20dd3c4] (??:?)
[24] /xxx/Godot_v3.1-beta4_x11_64/Godot_v3.1-beta4_x11.64() [0x160e67c] (<artificial>:?)
[25] /xxx/Godot_v3.1-beta4_x11_64/Godot_v3.1-beta4_x11.64() [0x160e5d3] (<artificial>:?)
[26] /xxx/Godot_v3.1-beta4_x11_64/Godot_v3.1-beta4_x11.64() [0x1693b17] (<artificial>:?)
[27] /xxx/Godot_v3.1-beta4_x11_64/Godot_v3.1-beta4_x11.64() [0x1693c26] (??:?)
[28] /xxx/Godot_v3.1-beta4_x11_64/Godot_v3.1-beta4_x11.64() [0x7ac20b] (??:?)
[29] /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf0) [0x7fd8e8edf830] (??:0)
[30] /xxx/Godot_v3.1-beta4_x11_64/Godot_v3.1-beta4_x11.64() [0x7b9c8e] (??:?)

Probably utterly wrong, but I am starting to lose hope (is anyone even using GDNative with c++? several days without any comment on a very basic common beginner topic).

  register_property<CPP, Variant>("material", &CPP::material, Variant(new Material()));
...
    Variant material;

Linux 64b, 3.1.beta3 and 4 behaves same.

bruvzg commented 5 years ago
mnn commented 5 years ago

Yeah, I was pretty sure using Variant this way is wrong, I tried directly Material, Ref, combinations of what is pointer and what's not, nullptr in Ref and so on.

Your register_property way is behaving exactly same as majority of my other attempts (locked Material prop with text "[null]" and nothing can be changed).

Hopefully tomorrow I'll look at the _get_property_list/_get/_set (looks a bit complex for what I wanted to do).

Thank you for your response.

BastiaanOlij commented 5 years ago

You can look at property list/get/set which is what I do in procmesh but for entirely different reasons: https://github.com/BastiaanOlij/gdprocmesh/blob/master/src/gdprocmesh.cpp#L8

The same should work with register_property as well. Everything is communicated as variants but you have to set the types correctly, I know I've done it before but can't find an example.

Anyways, procmesh atleast gives you an example of how to do it with property_list, get and set :)

mnn commented 5 years ago

Tried the _get_property_list way and it doesn't seem to be doing anything:

  register_method("_get_property_list", &CPP::_get_property_list);
  register_method("_get", &CPP::_get);
  register_method("_set", &CPP::_set);
Array CPP::_get_property_list() {
  std::cout << "_get_property_list ";
  Array arr;

  {
    Dictionary prop;
    prop["name"] = "material";
    prop["type"] = GlobalConstants::TYPE_OBJECT;
    prop["hint"] = GlobalConstants::PROPERTY_HINT_RESOURCE_TYPE;
    prop["hint_string"] = GlobalConstants::PROPERTY_USAGE_SCRIPT_VARIABLE;
    arr.push_back(prop);
  }

  std::cout << arr.size() << std::endl;
  return arr;
}

Variant CPP::_get(String name) {
  return Variant();
}

bool CPP::_set(String name, Variant value) {
  return false;
}

_get_property_list doesn't seem to be ever called (no console output) and that one property is not visible in the editor. I have no clue what things in GlobalConstants are for (or format of items expected from _get_property_list), because there are no comments, no documentation in code...

Any idea why this (or anything related to property objects) is not working for me, but apparently it works fine for others? I don't see any warning or error in editor output tab nor console. I am starting to question if this immature C++ interop is worth it. Even alpha C# support seemed to be much better, at least basics like "export" for editor worked flawlessly in minutes...

bruvzg commented 5 years ago

Done some test, register_property doesn't work since it can't infer type of object from null reference (it's set to GlobalConstants::TYPE_NIL instead of GlobalConstants::TYPE_OBJECT).

_get_property_list/_get/_set works fine for me.

doesn't seem to be ever called (no console output)

You have to register classes as tool class using godot::register_tool_class instead of godot::register_class, otherwise scripts (including _get_property_list) won't be executed in editor!

Here's my code:

class Test2 : public Node2D {
    GODOT_CLASS(Test2, Node2D);

private:

    Ref<Material> material;

public:
    static void _register_methods();

    Array _get_property_list();
    Variant _get(String p_name);
    bool _set(String p_name, Variant p_value);

    void _init() { /*NOP*/ };

    Test2() { /*NOP*/ };
    ~Test2() { /*NOP*/ };
};

/*************************************************************************/

void Test2::_register_methods() {

    register_method("_get_property_list", &Test2::_get_property_list);
    register_method("_get", &Test2::_get);
    register_method("_set", &Test2::_set);
}

Array Test2::_get_property_list() {

    Array arr;

    Dictionary prop;
    prop["name"] = String("test_material");
    prop["type"] = GlobalConstants::TYPE_OBJECT;
    prop["hint"] = GlobalConstants::PROPERTY_HINT_RESOURCE_TYPE;
    prop["hint_string"] = "ShaderMaterial,CanvasItemMaterial";
    prop["usage"] = GlobalConstants::PROPERTY_USAGE_DEFAULT;
    arr.push_back(prop);

    return arr;
}

Variant Test2::_get(String p_name) {

    if (p_name == "test_material") {
        return material;
    } else {
        return Variant();
    }
}

bool Test2::_set(String p_name, Variant p_value) {

    if (p_name == "test_material") {
        material = p_value;
        return true;
    } else {
        return false;
    }
}
mnn commented 5 years ago

@bruvzg Well, your code works (material can be selected), but if I understand it correctly - it must run as an editor plugin (class? node?), so _process is called in editor, instead of during game run (so benchmark code started to run inside editor, freezing it) - I am not sure I can use this approach...

I also tried passing new material to the initial property value, but that didn't really work in editor - new material cannot be easily created from dropdown of property.

  register_property<CPP, Ref<Material>>("material", &CPP::set_material, &CPP::get_material,
                                        Ref<Material>(Material::_new()),
                                        GODOT_METHOD_RPC_MODE_DISABLED, GODOT_PROPERTY_USAGE_DEFAULT,
                                        GODOT_PROPERTY_HINT_RESOURCE_TYPE,
                                        "ShaderMaterial,CanvasItemMaterial");

I guess my next attempt will be just calling method on C++ node from GDScript with exported material (so working in editor) and passing the material from GDScript to C++ part via a method argument. Seems really hacky, but I don't see other options.

Maybe by the time (months) I get to the feature where I will need properties properly working without hacks/workarounds, register_property might be fixed.

bruvzg commented 5 years ago

I also tried passing new material to the initial property value, but that didn't really work in editor - new material cannot be easily created from dropdown of property.

That's another bug, register_property version with getter/getter doesn't set the hint string.

Here's quick fix for register_property (patch can be applied to the header without need to rebuild godot-cpp library), it just assumes null is object, probably there is a better method, but Variant::NIL property doesn't make much sense and shouldn't cause any problems.

diff --git a/include/core/Godot.hpp b/include/core/Godot.hpp
index 4ed48ed..8b1c2aa 100644
--- a/include/core/Godot.hpp
+++ b/include/core/Godot.hpp
@@ -326,8 +326,13 @@ void register_property(const char *name, P(T::*var), P default_value, godot_meth
    godot_string *_hint_string = (godot_string *)&hint_string;

    godot_property_attributes attr = {};
-   attr.type = def_val.get_type();
-   attr.default_value = *(godot_variant *)&def_val;
+   if (def_val.get_type() == Variant::NIL) {
+       attr.type = Variant::OBJECT;
+   } else {
+       attr.type = def_val.get_type();
+       attr.default_value = *(godot_variant *)&def_val;
+   }
+
    attr.hint = hint;
    attr.rset_type = rpc_mode;
    attr.usage = usage;
@@ -356,12 +361,19 @@ template <class T, class P>
 void register_property(const char *name, void (T::*setter)(P), P (T::*getter)(), P default_value, godot_method_rpc_mode rpc_mode = GODOT_METHOD_RPC_MODE_DISABLED, godot_property_usage_flags usage = GODOT_PROPERTY_USAGE_DEFAULT, godot_property_hint hint = GODOT_PROPERTY_HINT_NONE, String hint_string = "") {
    Variant def_val = default_value;

+   godot_string *_hint_string = (godot_string *)&hint_string;
+
    godot_property_attributes attr = {};
-   attr.type = def_val.get_type();
-   attr.default_value = *(godot_variant *)&def_val;
+   if (def_val.get_type() == Variant::NIL) {
+       attr.type = Variant::OBJECT;
+   } else {
+       attr.type = def_val.get_type();
+       attr.default_value = *(godot_variant *)&def_val;
+   }
    attr.hint = hint;
    attr.rset_type = rpc_mode;
    attr.usage = usage;
+   attr.hint_string = *_hint_string;

    _PropertySetFunc<T, P> *wrapped_set = (_PropertySetFunc<T, P> *)godot::api->godot_alloc(sizeof(_PropertySetFunc<T, P>));
    wrapped_set->f = setter;

With this patch following code works:

register_property<Test1, Ref<Material> >("test_material", &Test1::set_test_material, &Test1::get_test_material, Ref<Material>(), GODOT_METHOD_RPC_MODE_DISABLED, GODOT_PROPERTY_USAGE_DEFAULT, GODOT_PROPERTY_HINT_RESOURCE_TYPE, String("ShaderMaterial,CanvasItemMaterial"));
mnn commented 5 years ago

@bruvzg Thank you, the patch seems to work well :smiley:.

Windfisch commented 5 years ago

hi @bruvzg, thank you for this patch. It works great!

After some research, I found that the root cause seems to be c2b59773af525161d4dd3f1dd9baac179f1afda9, where you added the if-else in Variant's constructor as below: https://github.com/GodotNativeTools/godot-cpp/blob/c2b59773af525161d4dd3f1dd9baac179f1afda9/src/core/Variant.cpp#L143-L149

That way, the Variant's type obviously becomes NIL when passing an empty Ref<...>() as default value.

Would it be a better idea to call godot_variant_new_object(..., nullptr) instead of godot_variant_new_nil at this place?

I'd love to see this issue fixed in upstream, btw :)