Set instance and instance binding in `Wrapped` constructor.

[Daylily-Zeleen]

Fixes https://github.com/godotengine/godot-cpp/issues/1039

Refer to this comment. This is my solution to advance the timing of setting instance and instance binding.

There may be some potential problems that I haven't found, please review it carefully.

This is my test class:

class MyLineEdit : public LineEdit {
    GDCLASS(MyLineEdit, LineEdit)
protected:
    static void _bind_methods() {}

public:
    void _on_text_submitted(const String &p_test) {
        UtilityFunctions::print("Submitted: ", p_test);
    }

    MyLineEdit() {
        connect("text_submitted", callable_mp(this, &MyLineEdit::_on_text_submitted));
    }
};

Before this pr, it will push an error:

ERROR: Condition "_instance_bindings != nullptr && _instance_bindings[0].binding != nullptr" is true.
   at: set_instance_binding (core/object/object.cpp:1824)

[dsnopek]

As pointed out in a comment on a similar issue, moving this setup to the Wrapped constructor is tricky because there are two paths for extension classes to be created: from within the extension, and from Godot.

I haven't had a chance to really review the code here, but I think we need automated tests that ensure both paths work correctly. This should either be part of this PR or in a PR that's merged before it, since this is one of the things that has gotten broken and fixed in godot-cpp a couple of times.

[Daylily-Zeleen]

As pointed out in a comment on a similar issue, moving this setup to the Wrapped constructor is tricky because there are two paths for extension classes to be created: from within the extension, and from Godot.

I don't know if there are other ways to create extension class objects that I have neglected. I found that create extension class from extension and create from godot are the same way. In extension, we use memnew() directly. In godot, create extension classe through GDExtensionClassCreationInfo3::create_instance_func, it is point to ClassDB::_create_instance_func in godot-cpp, and use memnew(), too.

Wrapped have another constructor Wrapped(GodotObject *p_godot_object), it can only be used to godot classes' create_callback, I'm not sure about its function. We don't use this constructor in godot-cpp, so this constructor should not relate about this pr.

I haven't had a chance to really review the code here, but I think we need automated tests that ensure both paths work correctly. This should either be part of this PR or in a PR that's merged before it, since this is one of the things that has gotten broken and fixed in godot-cpp a couple of times.

About test, I think the content in "test" folder already done this thing. Both creating extensions node(Example) in godot editor, and creating extension object by memnew(ExampleRef) in extension self.

If there have other situations need to be tested, please point them out.

[dsnopek]

About test, I think the content in "test" folder already done this thing. Both creating extensions node(Example) in godot editor, and creating extension object by memnew(ExampleRef) in extension self.

Yeah, we're already creating objects in both ways, but we aren't really testing it, ie. the tests won't necessarily fail if we get this wrong.

Looking over old issues/PRs where we broke this previously, we commonly got particular error messages, for example:

ERROR: Condition "_instance_bindings != nullptr" is true

In the current code, I think that message would actually be different, but perhaps making the CI fail if we got those sort of error messages would help here?

Anyway, given that this is tricky, and we have a history of breaking and fixing this multiple times in the past, I'd really like to have some support from the automated tests to help us get this right. :-)

[Daylily-Zeleen]

I found the reson of this error ERROR: Condition "_instance_bindings != nullptr && _instance_bindings[0].binding != nullptr" is true.:

godot::internal::get_object_instance_binding() will pass the binding_callbacks to Object::get_instnce_binding() in godot, and set instance binding if it has not been set.

In my example, connect will check the custom callble's validity, and godot::internal::get_object_instance_binding() will be called for cheacking. Before this pr, instance binding still not be set at this time, and Object::get_instnce_binding() will set it. After construction of extension object, instance binding will be set again (by Wrapped::_postinitialize()), and the error will be printed.

We can draw a conclusion, the key point of the new test is to check whether the binding is set many times unexpectedly, instead of creating extension class in godot-cpp and godot.

But I can't find a elegant way to do this check currently, maybe we need some new things which provided by godot.

---

godot::internal::get_object_instance_binding() has been used in many type conversion situations. To ensure users can use them in extension class constructors, we must let instance binding have been set before extension class constructors.

So I added a new test to check whether the binding was set before the constructor of extension class.

[dsnopek]

Thanks for your work on this PR!

I haven't had a chance to test this yet, but skimming the code I think this could work. I worry a bit about the extra complexity, and if this is going to lead to more function calls during object construction. A few of the new functions are marked as _ALWAYS_INLINE_ which is good. Do we really need the Wrapped::_initialize() function? It seems like the only place its called is in Wrapped::Wrapped(const StringName)?

Anyway, I still need to spend some more time to try this out.

[Daylily-Zeleen]

Yes, Wrapped::_initialize() is redundant, thanks for your reminder.

[dsnopek]

Discussed at the GDExtension meeting: I was concerned about when to merged this (for 4.3 or 4.4) and if to cherry-pick to older versions. We decided that merging for 4.3 makes sense because we still have time to test and find regressions, but that we'll hold off on cherry-picking for now. Later, after this has gotten some testing, we can consider cherry-picking again.

[YuriSizov]

Hey, that's nice: it looks like after this change we get a crash when using new instead of memnew on Godot objects, which is great for catching bugs. Thanks for your work!

[dsnopek]

Heh, I'm not entirely sure if you're being serious or sarcastic, but I wonder if it makes sense to say something like "Did you forgot to use memnew()?" to the error message this PR added to Wrapped::Wrapped()?

New users not knowing that they need to use memnew() is a very common cause for support requests, and it'd be great if we could eliminate some of those.

[dsnopek]

Just created PR https://github.com/godotengine/godot-cpp/pull/1510 to add that to the message :-)

[YuriSizov]

@dsnopek Haha, I can see how this may read as sarcastic, but I'm being genuine :) Good idea to add a more explicit suggestion in the error message, because while tracing back the issue to the offending line is not that hard, it still requires some deductive thinking!

[Naros]

@dsnopek so I'm working through the pains of this specific PR change. One thing I don't particularly care for is that previously we were able to define a Mutex in a GDExtension class just like we do in C++ modules, i.e.:

Mutex _my_mutex;

After this change, it's mandatory that the mutex must be used as:

Ref<Mutex> _my_mutex;

// then somewhere in the object's ctor
_my_mutex.instantiate();

I'm also getting this error with something related to a RefCounted object; however the only place that new is used is on 2 or 3 non-Godot objects in our plug-in. I'm trying to find the specific class or call site where this fails specifically.

UPDATE The RefCounted issue was likely a mistake of mine where we had included a member variable in a class that extended RefCounted but was not defined using Ref<>, similar to the Mutex use case.

[Naros]

Another obscure use case I found was my use of RandomNumberGenerator. In several places where I did not want to reallocate the instance on the heap, I had used a stack-based variable, i.e. RandomNumberGenerator _random and this worked fine. But with this change, these use cases also cause this crash.

My concern is this does not necessarily align with the principle between the usage of GDExtension vs C++ modules. Even in the engine's tests, there are use cases where RandomNumberGenerator is allocated using a Ref<> wrapper, while others its a free-standing stack-allocated object in a class or function call.

[dsnopek]

Well, creating bound Godot classes in godot-cpp without memnew() really shouldn't ever have worked; doing so would mean not using the GDExtension API as intended. So, if it worked in the past, it was entirely accidental. I suspect there are cases where that would lead to unexpected behavior that you just didn't manage to encounter.

Mutex is an interesting case: inside Godot there is a Mutex class that is not bound and can be used by modules without memnew(), and then another core_bind::Mutex class which is bound. Unfortunately, it's the 2nd one that we have in godot-cpp via the usual code generation for bound classes.

In this case, I could see an argument for making a custom Mutex class in godot-cpp that allows mimicking the usage of the Mutex class that Godot modules have access to? That could get tricky if you wanted to pass an object of that class to GDScript or something, but I suspect that's not something that folks really need to do...

Anyway, this isn't the case for RandomNumberGenerator. If a Godot module wanted to use that class, it should also use Ref<>, and if it isn't, I suspect that could also lead to unexpected behavior in some cases. Those uses in the tests should probably be fixed!

[Naros]

Hi @dsnopek, always appreciate the in-depth analysis. Regarding Mutex, I would say that unless this comes up from someone, it's probably not worth the cycles on it until it's needed.

[dmlary]

Building off of @dsnopek's comment, this now triggers a crash for stack allocated Godot class instances. I'm using a CylinderMesh on the stack to generate a vertex array, and drop the instance. Now in 4.3 it crashes. I get the feeling that a lot of godot classes, not just Mutex are going to have this same problem.

frame #0: 0x000000010d98b434 libhexmap.macos.template_debug`godot::Wrapped::Wrapped(this=0x000000016fdfe4e0, p_godot_class=(opaque = "\xe0\x96\a\U00000001\0`")) at wrapped.cpp:87:3
   84           godot::internal::gdextension_interface_object_set_instance_binding(_owner, godot::internal::token, this, _constructing_class_binding_callbacks);
   85           _constructing_class_binding_callbacks = nullptr;
   86       } else {
-> 87           CRASH_NOW_MSG("BUG: Godot Object created without binding callbacks. Did you forget to use memnew()?");
   88       }
   89   }
   90
(lldb) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BREAKPOINT (code=1, subcode=0x10d98b434)
    frame #0: 0x000000010d98b434 libhexmap.macos.template_debug`godot::Wrapped::Wrapped(this=0x000000016fdfe4e0, p_godot_class=(opaque = "\xe0\x96\a\U00000001\0`")) at wrapped.cpp:87:3
    frame #1: 0x000000010d911928 libhexmap.macos.template_debug`godot::Object::Object(this=0x000000016fdfe4e0, p_godot_class="CylinderMesh") at object.hpp:51:2
    frame #2: 0x000000010d9118b0 libhexmap.macos.template_debug`godot::RefCounted::RefCounted(this=0x000000016fdfe4e0, p_godot_class="CylinderMesh") at ref_counted.hpp:46:2
    frame #3: 0x000000010d911864 libhexmap.macos.template_debug`godot::Resource::Resource(this=0x000000016fdfe4e0, p_godot_class="CylinderMesh") at resource.hpp:50:2
    frame #4: 0x000000010d911788 libhexmap.macos.template_debug`godot::Mesh::Mesh(this=0x000000016fdfe4e0, p_godot_class="CylinderMesh") at mesh.hpp:58:2
    frame #5: 0x000000010d95e378 libhexmap.macos.template_debug`godot::PrimitiveMesh::PrimitiveMesh(this=0x000000016fdfe4e0, p_godot_class="CylinderMesh") at primitive_mesh.hpp:50:2
    frame #6: 0x000000010d95e528 libhexmap.macos.template_debug`godot::CylinderMesh::CylinderMesh(this=0x000000016fdfe4e0, p_godot_class="CylinderMesh") at cylinder_mesh.hpp:46:2
    frame #7: 0x000000010d95e4e4 libhexmap.macos.template_debug`godot::CylinderMesh::CylinderMesh(this=0x000000016fdfe4e0, p_godot_class="CylinderMesh") at cylinder_mesh.hpp:46:2
    frame #8: 0x000000010d95ab08 libhexmap.macos.template_debug`godot::CylinderMesh::CylinderMesh(this=0x000000016fdfe4e0) at cylinder_mesh.hpp:46:2
  * frame #9: 0x000000010d985e4c libhexmap.macos.template_debug`TestNodeEditorPlugin::TestNodeEditorPlugin(this=0x0000600007f3ce20) at test_node_editor_plugin.cpp:116:15
    frame #10: 0x000000010d985eb4 libhexmap.macos.template_debug`TestNodeEditorPlugin::TestNodeEditorPlugin(this=0x0000600007f3ce20) at test_node_editor_plugin.cpp:114:46
    frame #11: 0x000000010d983938 libhexmap.macos.template_debug`void* godot::ClassDB::_create_instance_func<TestNodeEditorPlugin>(data=0x000000010dbecb09) at class_db.hpp:121:20
    frame #12: 0x0000000101639c6c godot.macos.editor.arm64`EditorNode::add_extension_editor_plugin(p_class_name=0x0000600003e8a668) at editor_node.cpp:3580:55 [opt]
    frame #13: 0x0000000101659e20 godot.macos.editor.arm64`EditorNode::EditorNode(this=0x000000012c46da10) at editor_node.cpp:7639:3 [opt]
    frame #14: 0x000000010043ecc8 godot.macos.editor.arm64`Main::start() at main.cpp:3743:18 [opt]
    frame #15: 0x000000010040c3d4 godot.macos.editor.arm64`main(argc=<unavailable>, argv=<unavailable>) at godot_main_macos.mm:81:9 [opt]
    frame #16: 0x000000019a307154 dyld`start + 2476
(lldb) up 9
frame #9: 0x000000010d985e4c libhexmap.macos.template_debug`TestNodeEditorPlugin::TestNodeEditorPlugin(this=0x0000600007f3ce20) at test_node_editor_plugin.cpp:116:15
   113
   114  TestNodeEditorPlugin::TestNodeEditorPlugin() {
   115      editor_shortcut("test_node/test_setting", "TestNode Test Setting", (Key)(KEY_C | godot::KEY_CTRL), true);
-> 116      CylinderMesh m;
   117      m.set_cap_bottom(true);
   118  }
   119
(lldb)

[dsnopek]

@dmlary Godot objects descending from Object aren't meant to be allocated on the stack. Also, in your case, you're using a RefCounted object, but not using the Ref<T> template, which means it won't actually be reference counted.

Instead of:

CylinderMesh m;

... you should use:

Ref<CylinderMesh> m;
m.instantiate();

[dmlary]

Out of curiosity, why not? And why is a stack allocated instance ref counted behind the scenes? The object instance isn’t going into the scene; I’m only creating the instance because CylinderMesh:: create_mesh_array() isn’t exposed.

Generally, is there a place I can read more about what is going on here? I would expect to be able to declare classes on the stack and have the initializer do the proper thing. I’d like to learn more so I can avoid any other footguns.

[dsnopek]

Out of curiosity, why not?

One of godot-cpp's core design pillars is that code should match (as closely as possible) code in the engine itself, such that (ideally) you can copy C++ code between Godot and godot-cpp, and have it work the same.

In Godot, objects aren't fully setup just by their constructor, you need to use memnew() to do the full process, which we also do in godot-cpp (although, we do different setup than in Godot itself).

And why is a stack allocated instance ref counted behind the scenes?

Ref<T> is a smart pointer - it does the memnew() internally for new objects, but also is in charge of incrementing and decrementing the reference count. Again, this is the same as in the engine.

If we weren't trying to match Godot's internal API, we could perhaps bake the incrementing/decrementing of the reference count into the classes constructor/destructor.

Generally, is there a place I can read more about what is going on here?

Unfortunately, there isn't a lot of documentation on how Godot's internal C++ API are designed. Personally, my primary reference is Godot's source code itself, but I know that isn't ideal. :-)

[Daylily-Zeleen]

@dsnopek It seems that it is not uncommon to instantiate a godot object without using memnew(). May we remove ERR_PRINT() and CRASH_NOW_MSG() in Wrapped::Wrapped(const StringName p_godot_class)? If I remember correctly, define a godot object without memnew() in the old logic will not set instance binding and binding callbacks, too.

In most cases, a stack allocated godot object like Mutex, we are not use its signals, so define at stack (has not instance binding) is reasonable. But if we remove the crashing expression, beginer users may improperly use signals of a stack allocated object. This seems like a dilemma.

[dsnopek]

@dsnopek It seems that it is not uncommon to instantiate a godot object without using memnew().

Yeah, it's interesting how many people were apparently doing things that should never have worked! And, even if they did sort of work, they were probably not hitting other issues by luck, and/or causing memory leaks they just weren't aware of.

May we remove ERR_PRINT() and CRASH_NOW_MSG() in Wrapped::Wrapped(const StringName p_godot_class)?

Personally, I think we should keep it as-is. We do need to improve our documentation to better educate developers in the first place, but I think this is primarily a transition thing, and after a couple weeks folks will have adjusted.

In most cases, a stack allocated godot object like Mutex, we are not use its signals, so define at stack (has not instance binding) is reasonable.

I don't think signals are the only thing we have to worry about. There's potentially a lot of things that could go wrong if the Godot objects aren't fully setup, since everything in Godot is going to assume that all objects are.

But if we remove the crashing expression, beginer users may improperly use signals of a stack allocated object. This seems like a dilemma.

Having a loud error with a clue, is better than silently "succeeding" with future hidden traps. :-)

[Daylily-Zeleen]

Could we find out all situations which need instance binding and add some check for them?

[dsnopek]

Could we find out all situations which need instance binding and add some check for them?

Is your thought that we could allow users to use not-fully-setup objects, but then add checks to the situations where we know the result would be particularly bad?

Aside from that being tricky to do (I don't know how we'd know an instance binding should exist to check for it being missing), I don't think that's a very sustainable solution. I personally think it would be better to focus on how we can guide users to use the API correctly.

However, just to for clarity (for those who don't know the technical details), I think the main situation we need an instance binding is any time an object is passed from GDExtension to Godot and back to GDExtension. Otherwise, multiple redundant wrapper objects could be created. Those won't necessarily cause crashes (although, they could if you delete more than one of the wrappers), but it would leak memory, and cause unexpected situations, for example, where you pass an object to Godot, but get a different one when it passes it back to you (even if its the same object on the Godot side), making == not do what you think it should.

[Daylily-Zeleen]

If we can find and add check for all situations which need instance binding, we can add a method to setup binding manually. When users use a stack allocated godot object uncorrectly, they will get an error, and they can setup binding manually or just use memnew() instead of allocating at stack.

I think some simple godot objects to be allocated at stack is reasonable, like Mutex, RegEx and RandomNumberGenerator.

[dsnopek]

I think some simple godot objects to be allocated at stack is reasonable, like Mutex, RegEx and RandomNumberGenerator.

I disagree.

Setting the instance binding when creating an object is part of correctly using the GDExtension interface. I don't think we should be selectively saying that sometimes for some classes we don't do the complete process of creating an object.

[Daylily-Zeleen]

The requirement is to support allocating godot objects at stack. The idea result is to fully setup godot object both at stack and heap.

I had a sudden inspiration, to add a macro, for example, defiene it as #define stack_alloc() xxxx, let it do the same thing like memnew(), but not use new and the return value is not a pointer.

But this plan will break the code style which similar to godot source (because godot side support allocate objects at stack naturally ).

I think I have exhausted my all solutions on this issue, no one is perfect😌.

[dsnopek]

But this plan will break the code style which similar to godot source (because godot side support allocate objects at stack naturally ).

Even within Godot itself, classes that descend from Object should not be created on the stack - they need to be created with memnew() to get fully setup. The potential issues are different because the setup is different, but it's still a situation where it may sort of work in some circumstances, but can lead to unexpected problems or crashes in others.

[Daylily-Zeleen]

Even within Godot itself, classes that descend from Object should not be created on the stack

Almost RegEx are allocate at stack in godot, here is an example. I had met a unexpected behavior when useing Ref<RefEx> in godot module, and use RegEx can get a correct result. Anyway, the issue between Ref<RegEx> and RegEx may not occur in godot-cpp, here I just point out that, sometime godot will allocate godot objects at stack, too.

[dmlary]

In Godot, objects aren't fully setup just by their constructor, you need to use memnew() to do the full process, which we also do in godot-cpp (although, we do different setup than in Godot itself).

I'm going to document a knock-on effect of this, and propose an option.

I'm working on a GDExtension conversion of https://github.com/godotengine/godot/pull/85890 to implement a GridMap with hex-shaped cells. One of the big changes is converting from Vector3i to a CellId class to abstract away the internal coordinate system (important for hex cell coordinates). This is a pile-of-data class that just holds three ints, but with a lot of helper methods such as neighbors, distance, etc.

Internally the C++ code uses CellId to calculate all manner of things: distance between cells, global position for drawing meshes, selecting regions of cells, etc. There are dozens of places where I create instances of CellId on the stack, and use it to generate some result. I also use static consts such as CellId::Origin for other calculations. Here's an explicit example where the class is simply used as a calculator; this is a common pattern throughout the c++ code.

// pick a point on the middle of an edge to find the closest edge of the cells
float max = CellId(GRID_RADIUS, -(int)GRID_RADIUS / 2, 0)
    .unit_center()
    .length_squared();

The HexMap class has GDScript bindings that want to return CellId, but that's only possible if CellId is a RefCounted godot::Object (or return *CellId and leak memory). If I do that, I have to change all of the existing uses of the class to dynamically allocate and initialize a pile of bindings that will never be used. So the example code above becomes:

CellId *edge_cell = memnew(CellId(GRID_RADIUS, -(int)GRID_RADIUS / 2, 0));
float max = edge_cell->unit_center().length_squared();
memdelete(edge_cell);

I'm not doing this for both performance reasons and it gets ungodly ugly in the more complex places like cell iteration.

(ugly) Work-around

The work-around I'm using right now is that for any type I use natively in C++, that also needs to be accessible in GDScript, I create a wrapper class such as CellIdRef that only holds a CellId. Then I return Ref<CellIdRef> in GDScript bound functions. Then I have to bind every method in CellId in CellIdRef. This is a pretty boilerplate-heavy way to operate.

Proposal

Would it be possible to add a compiler flag to turn off this guard rail?

There are cases such as for CellId where it is completely legitimate to use a class in C++ without ever initializing the bindings. And to expose the class to GDScript, it's correctly bound by the call to Ref<T>.instantiate().

I understand the guardrail was added to catch a common problem of passing uninitialized godot::Objects into the engine, but the side effect is that you're barring common use of C++ classes that just happen to be godot::Object to be passed into GDScript.

[YuriSizov]

If your purpose is to specifically expose such types to the scripting API, then it would be an extremely bad idea to make those types incomplete.

Instead, consider exposing setters and getters on the parent class for a proxied access to the properties of this data structure that you want to expose. This would also be a more conventional way to design the API in Godot.

PS. You don't actually need the set method in your example. You can pass arguments to the constructor in memnew. Although with so many arguments it may look too heavy on the reader.

[dmlary]

If your purpose is to specifically expose such types to the scripting API, then it would be an extremely bad idea to make those types incomplete.

@YuriSizov I called out in my comment that I was not exposing incomplete types to the GDScript layer. The classes will be properly bound when they reach the scripting api because they can only reach GDScript by being returned via Ref<T>, which will call memnew() during Ref<T>.instantiate().

The purpose of this is to use the types in C++, because they’re safe to use in C++ without setting up the engine bindings. There are a sufficient number of instances where these types are created, used, and discarded without crossing the boundary into the core engine to consider allowing the behavior.

You can pass arguments to the constructor in memnew.

EDIT: I found the proper syntax; it's not an argument to memnew() it's passing the class constructor memnew(CellId(1,2,3)). I did initially try this, but didn't properly create the needed constructor. I'll update the code sample in my previous comment, to reflect this capability.

This still leaves the performance hit associated with dynamically allocating and releasing the type when it is safe to use on the stack.

ORIGINAL (incorrect): That may be the case in the godot engine c++ code, but it does not look to be the case for godot-cpp. The memnew macro doesn’t take any additional args beyond class name, and I don’t see any macro args in the call to new.

https://github.com/godotengine/godot-cpp/blob/4131b7f95f5a4ec660484431b175e15d95aed8db/include/godot_cpp/core/memory.hpp#L100

[Daylily-Zeleen]

@dmlary In my opinion, only some native godot objects worth to be used without memnew().

If you don't want to expose a class to scripting, the class don't need to inherit from godot classes. If your class need godot features such as signals, set() and call() and so on, you must inherit from native godot classes and use memnew() for safety.

In some case, you can consider to combine them by using multiple Inheritance.

---

Proposal

Would it be possible to add a compiler flag to turn off this guard rail?

There are cases such as for CellId where it is completely legitimate to use a class in C++ without ever initializing the bindings. And to expose the class to GDScript, it's correctly bound by the call to Ref<T>.instantiate().

I understand the guardrail was added to catch a common problem of passing uninitialized godot::Objects into the engine, but the side effect is that you're barring common use of C++ classes that just happen to be godot::Object to be passed into GDScript.

As my previous comment show, some classes in godot source code are used in stack allocated. To force require users use memnew() is break the principle that make godot-cpp similar to godot source. So I think this is a proper way to allow use stack allocated godot object, and we can enable this check and crash by default for safety.

[dmlary]

So I think this is a proper way to allow use stack allocated godot object, and we can enable this check and crash by default for safety.

@Daylily-Zeleen I can open a PR with this change.

[YuriSizov]

I called out in my comment that I was not exposing incomplete types to the GDScript layer. The classes will be properly bound when they reach the scripting api because they can only reach GDScript by being returned via Ref\u003CT>, which will call memnew() during Ref\u003CT>.instantiate().

Okay, I did not understand that initially. Then using two different types, plain one and a wrapper, makes sense for how Godot is designed.

[dmlary]

[...] using two different types, plain one and a wrapper, makes sense for how Godot is designed.

@YuriSizov the problem with using two different types is that you end up having to write a lot of boilerplate for that wrapper class. Any function you expose that doesn't require wrapping or unwrapping a type in Ref<T> is going to end up being a straight pass-through function to the wrapped type. This is an unnecessary maintenance cost as classes grow.

I've opened PR #1585 to allow GDExtension creators to use a compiler flag to disable the guard that prevents the use of godot::Object subclasses from being used on the stack. The default is to leave the guard in place so people learn about the need for memnew(), but it allows advanced usage for people who don't want to pay the performance & maintenance cost.

@YuriSizov on a separate note, I found Ref<T> doesn't have the same support for a constructor to call similar to memnew(). This makes the examples below ugly when we need to copy an existing class instance into Ref<T>.

Full example using a wrapper class

I've been going down this path in my repo, and I wanted to provide a concrete example of what using a separate wrapper class looks like.

I've cleaned up one of the C++ classes I'm working with. This class CellId can be thought of as the equivalent of Vector3 but in a different coordinate system. It provides many calculation functions, but only contains three integers.

class CellId {
public:
    // axial hex coordinates, and y coordinate
    int q, r, y;

    CellId() : q(0), r(0), y(0){};
    CellId(int q, int r, int y) : q(q), r(r), y(y){};

    // get the position of the center of this cell, assuming cell radius=1,
    // height=1
    Vector3 unit_center() const;

    // check if a point falls within this cell
    bool contains_point(Vector3 &point) const;

    // calculate the distance from another hex cell
    unsigned distance(const CellId &) const;

    // get the CellId of the neighbor in the specified direction
    CellId get_neighbor(int direction) const;
};

I use this class in the following ways:

• Allocate on the stack in C++ and use only in C++ without passing to godot scripting layer
  • The class is used as a calculator, similar to Vector3
• Allocate on the heap and return to GDScript
  • this allows GDScripts to make use of all the calculation functions in the class
  • if there were a way to expose the class to GDScript without inheriting Object, I would do it

For the CellId class I cherry-picked four functions that are a representative sample of the types of functions that would be exposed to GDScript:

• unit_center() godot native return type, no args
• contains_point() native return type, godot native arg
• distance() native return type, custom type arg
• get_neighbor() custom return type, native arg

The wrapper class for CellId that exposes these four functions looks like this:

class CellIdWrapper : public RefCounted {
    GDCLASS(CellIdWrapper, RefCounted)

    CellId cell_id;

public:
    CellIdWrapper(const CellId &cell_id) : cell_id(cell_id){};
    CellIdWrapper(){};

    Vector3 unit_center() const { return cell_id.unit_center(); };

    bool contains_point(Vector3 point) const {
        return cell_id.contains_point(point);
    }

    unsigned distance(const Ref<CellIdWrapper> other) const {
        return cell_id.distance(other->cell_id);
    }

    Ref<CellIdWrapper> get_neighbor(int direction) const {
        Ref<CellIdWrapper> ref;
        ref.instantiate();
        ref->cell_id = cell_id.get_neighbor(direction);
        return ref;
    };

protected:
    static void _bind_methods();
};

Of the four types of functions, only two of them require GDScript specific code. unit_center() and contains_point() are pure proxy functions that shouldn't need to be written. Only functions that need to wrap/unwrap a Ref<T> need customized code versus their C++ specific implementations.

Example with guard disabled, allowing stack allocation

When the guard is disabled, we gain the ability to use CellId as a stack-allocated class in C++, but also can set up the bindings when it is passed into GDScript. Here is the CellId class when the guard is disabled. The class is correctly bound in the functions that support GDScript.

class CellId : public RefCounted {
    GDCLASS(CellId, RefCounted)

public:
    // axial hex coordinates, and y coordinate
    int q, r, y;

    CellId() : q(0), r(0), y(0){};
    CellId(int q, int r, int y) : q(q), r(r), y(y){};

    // get the position of the center of this cell, assuming cell radius=1,
    // height=1
    Vector3 unit_center() const;

    // check if a point falls within this cell
    bool contains_point(Vector3 &point) const;

    // calculate the distance from another hex cell
    unsigned distance(const CellId &) const;
    unsigned _distance(const Ref<CellId> other) const {
        return distance(**other);
    }

    // get the CellId of the neighbor in the specified direction
    CellId get_neighbor(int direction) const;
    Ref<CellId> _get_neighbor(int direction) const {
        CellId neighbor = get_neighbor(direction);
        Ref<CellId> ref;
        ref.instantiate();
        // this part is ugly, but it's the most direct method I could find to keep comparison clear
        ref->q = neighbor.q;
        ref->r = neighbor.r;
        ref->y = neighbor.y;
        return ref;
    };

protected:
    static void _bind_methods();
};

The key change is that the GDScript-specific functions get renamed to _distance() and _get_neighbor(). Renaming is necessary for _distance() because ClassDB::bind_method() has issues with overloaded functions. _get_neighbor() has to be renamed because overloaded functions with the same arguments must return the same type.

Overall the difference lies in that we can bind the existing unit_center() and contains_point() functions without creating extranious wrappers. We also don't have to maintain an additional wrapper class.

Another possible solution

If ClassDB::bind_method() supported binding to a method on a instance variable, then it would be significantly more reasonable to maintain a wrapper class. For example binding unit_center() to CellIdWrapper.cell_id.unit_center(). In this case only Ref<T> wrapper functions would need to be written, with the rest bound to the methods on the wrapped type.

This approach works well for my case, but doesn't help anyone who wants to use native godot types like RegEx on the stack.

If this path is taken, it would be nice if we could also override the name of the class used in GDScript. I'm already getting tired of typing CellIdRef in my gdscript tests.

[YuriSizov]

on a separate note, I found Ref<T> doesn't have the same support for a constructor to call similar to memnew(). This makes the examples below ugly when we need to copy an existing class instance into Ref<T>.

Sorry, didn't have a chance to read through the whole thing, but wanted to quickly point out that you are allowed to assign memnew'd instances to Ref<T>, e.g. from my own code:

Ref<SiOPMWavePCMData> pcm_data = memnew(SiOPMWavePCMData(p_data, (int)(p_sampling_note * 64), p_src_channel_count, p_channel_count));

I don't think this introduces any performance differences, as it should simply use the assigned instance in place of the internally initialized one.

And also, yes, I understand the boilerplate argument. I am of an opinion that the style of the godot-cpp project should stay self-consistent, and one of the goals of this project is to let you code in a way that can be migrated between extensions and modules. This is by no means the only way you can use the GDExtension API of the engine, and if the chosen style and considerations that godot-cpp has to deal with don't let you code comfortably, you can definitely roll out your own bindings.

But for the official project I'd prefer if it would stick to an authoritative approach rather than flexibility to accommodate different styles, approaches, and targets. I understand that your main argument is performance, but the engine itself seems to exist fine within the same limitations, so it doesn't strike me as such a critical problem as to address it in the general purpose binding that is godot-cpp.

[dmlary]

[...] you are allowed to assign memnew'd instances to Ref [...]

@YuriSizov oh, that's nice! Thank you for the tip.

I understand that your main argument is performance, but the engine itself seems to exist fine within the same limitations, [...]

@YuriSizov But it actually doesn't. Godot explicitly made Vector3, Vector2, and all similar classes to CellId native godot types and not Godot Objects for performance & usability reasons. Imagine if every Vector3 had to be memnew'd; the engine would slow to a crawl.

[dsnopek]

I agree with a lot of what @YuriSizov wrote above:

Okay, I did not understand that initially. Then using two different types, plain one and a wrapper, makes sense for how Godot is designed.

I agree, that this would be a good way to handle this.

You can have plain C++ classes that you use internally, which you can use however you want. But if you want to bind a class with Godot via godot-cpp, then there are some rules that need to be followed. Separating those into separate classes (internal ones vs bound Godot wrappers) makes a lot of sense to me.

I am of an opinion that the style of the godot-cpp project should stay self-consistent, and one of the goals of this project is to let you code in a way that can be migrated between extensions and modules.

I agree with this as well. One of godot-cpp's design goals is to mimick Godot's internal APIs as much as possible, and that includes the rules for how to use classes that descend from Object and RefCounted.

I understand that there are a few places in Godot where it isn't presently following these rules. However, as I've said previously, I'm of the opinion that those are bugs that should be fixed in Godot, not something we should be attempting to allow from godot-cpp.

This is by no means the only way you can use the GDExtension API of the engine, and if the chosen style and considerations that godot-cpp has to deal with don't let you code comfortably, you can definitely roll out your own bindings.

Yes, godot-cpp isn't the only possible way to build a C++ binding for GDExtension. It has a particular set of goals and that has influenced its design choices.

[dmlary]

Ok, it seems y'all are pretty fixed on preserving the guard. I can accept that.

It seems like the accepted solution for performance is a C++-only class, and a wrapper class for sharing values to GDScript.

To reduce the maintenance overhead on the wrapper class, is there any way to add the ability to bind a GDScript method to an instance variable in the godot object? In concrete terms from the example^1 binding CellIdWrapper.get_unit_center() to CellIdWrapper.cell_id.unit_center().