_init() is required with exact name in C++, but not documented or enforced by API

[Bromeon]

Versions: https://github.com/godotengine/godot/commit/245c99175c242bdc60a212cc84986b1a9ad5aa08, godot-cpp 123d9f0e9264dcc7206888fc96419b32feef00c8

Lately I tried to bind a simple C++ class to Godot, very similar to the one in the official example:

class MyClass : public godot::Node2D
{
GODOT_CLASS(MyClass, godot::Node2D)

public:
	static void _register_methods()
	{
		godot::register_method("_init", &MyClass::Init);
		godot::register_method("_process", &MyClass::Process);
	}

	void Init()
	{
		printf("_init()\n");
	}

	void Process(float delta)
	{
		printf("_process(%f)\n", delta);
	}
};

and register as follows:

extern "C" void GDN_EXPORT godot_nativescript_init(void* handle)
{
	godot::Godot::nativescript_init(handle);
	godot::register_class<MyClass>();
}

Note that unlike the official example, I'm using a different naming convention, and my method is called Init instead of _init. Since I'm explicitly binding a member function reference via &MyClass::Init, and C++ generally has no concept of method names at runtime, a different naming convention should not be a problem.

Except the little problem that blows up the application when launching from Godot (expand):

CrashHandlerException: Program crashed
Dumping the backtrace. Please include this when reporting the bug on https://github.com/godotengine/godot/issues
[0] godot_method_bind_ptrcall (c:\godot\modules\gdnative\gdnative\gdnative.cpp:69)
[1] godot_method_bind_ptrcall (c:\godot\modules\gdnative\gdnative\gdnative.cpp:69)
[2] godot::___godot_icall_void (c:\godot-cpp\include\gen\__icalls.hpp:2503)
[3] godot::Object::_init (c:\godot-cpp\src\gen\object.cpp:84)
[4] godot::_godot_class_instance_func<GdBind::NavApi> (c:\c++libs\include\godot\core\godot.hpp:104)
[5] NativeScript::instance_create (c:\godot\modules\gdnative\nativescript\nativescript.cpp:217)
[6] Object::set_script (c:\godot\core\object.cpp:998)
[7] Object::set (c:\godot\core\object.cpp:432)
[8] SceneState::instance (c:\godot\scene\resources\packed_scene.cpp:212)
[9] PackedScene::instance (c:\godot\scene\resources\packed_scene.cpp:1692)
[10] Main::start (c:\godot\main\main.cpp:1808)
[11] widechar_main (c:\godot\platform\windows\godot_windows.cpp:160)
[12] _main (c:\godot\platform\windows\godot_windows.cpp:184)
[13] main (c:\godot\platform\windows\godot_windows.cpp:196)
[14] __scrt_common_main_seh (f:\dd\vctools\crt\vcstartup\src\startup\exe_common.inl:253)
[15] BaseThreadInitThunk
-- END OF BACKTRACE --

I debugged for at least half an hour, always searching the error in the binding, I even inspected the exported DLL functions to make sure their signature is correct. Only when moving step-by-step from the official example, I could reduce the code, so that no other difference than the method names remained. Thinking "this sure can't be it", the naming was it to my surprise. Later I found out, that even an empty _register_methods() would trigger the same crash.

With the attached VC++ debugger, I tracked down the issue to find this GDNative function template:

template <class T>
void *_godot_class_instance_func(godot_object *p, void *method_data) {
	T *d = new T();
	d->_owner = p;
	d->_type_tag = typeid(T).hash_code();
	d->_init();
	return d;
}

Without me defining my own _init() function, it would call the base class Object::_init():

void Object::_init() {
	___godot_icall_void(___mb.mb__init, (const Object *) this);
}

And this crashes, because the function pointer ___mb.mb__init is a null pointer.

---

So far so well, fixing this is easy by naming the function appropriately. However, some thoughts related to the current implementation:

1. I first thought I forgot to override a function in a base class, however Object::_init() is not virtual. Due to the static polymorphism at d->_init() (see above), GDNative still calls the most derived function, but silently falls back to the base function. This design is rather unusual, if not very confusing. A pure virtual function would have had the same effect, but could enforce overriding through C++'s type system. I understand that dynamic polymorphism may have been avoided for performance reasons though.
2. In my opinion, public and internal APIs are ideally separated in a way that it's not that easy for a user to accidentally mess with the internals. There are various ways to achieve this; apart from the obvious private/public/friend access specifiers, it's also possible to add static checks to the resolving function template, to cause a compile error if no user-defined method is provided.
3. It is unclear to me, where the Godot-site _init() (the equivalent in GDScript) comes into play. Why do I even need to register the function pointer, if GDNative already uses a hardcoded call to that method? Or is it just coincidence that Object::_init() and my _init() have the same name?
4. More generally, is there a reason why the de-facto default implementation of _init() -- i.e. Object::_init() -- is broken or is this just a bug?

In case this behavior is by design, I would recommend to make it a bit harder to use wrong, and add a big warning to the documentation -- even for C++ programmers this is quite a pitfall 😉

Nevertheless, I've been amazed how easy it is to get simple things up and running with both Godot and GDScript, keep up the great work!

[Bromeon]

It's been a while, any feedback on this? 🙂

[Zylann]

I had a bit more digging on this recently, and here is what I found so far:

• _init is null because in the Godot API, _init is a """""virtual""""" function. Not exactly in the C++ sense though: this is a function that exists in the API because Godot expects us to be able to implement it optionally, but it doesn't actually define a default implementation itself. The function does not even exists in the engine (or sometimes it does under a different definition), it's purely a script API artifact. I have yet to find a class of the API with a virtual method you can actually call.

• Usually, such functions can be called by the engine (litterally by name, like here or there), but _init is not even called that way. If I look in the implementation of Object, it binds it with BIND_VMETHOD(MethodInfo("_init")), but I searched _init in the whole cpp file, I could not find any use of it.

• I suspect it is present because GDScript constructors are defined with an _init function and the author wanted it to appear nicely in the generated docs. But it looks like outside of this language, it makes no sense. In the Mono module, the C# binding generator even excludes it in BindingsGenerator::_initialize_blacklisted_methods().

• In the C++ bindings, it still has a use, but it's a different story: as you saw in _godot_class_instance_func, this is where custom class instances get created when Godot asks to. But these classes are based on fake wrappers, since we have to use a C API. To call methods, that C API requires us to send the real Godot object pointer, which is a member of _Wrapped that T inherits, called _owner. This means the constructor of T cannot call Godot methods, and would crash, because _owner is not assigned at this point. This is why we defined a second method, called right after assigning _owner, so that you can actually access Godot methods right on construction. And to be in line with what people were used to with GDScript and the docs, we named it _init.

Now, seeing this, there are a few things I considered:

• Could we remove the need to define _init, and use C++ constructors without changing anything? I tried doing a hack to make it so _Wrapped members are assigned even before the constructor is called, like this:

    void *mem = malloc(sizeof(T));
    _Wrapped *w = reinterpret_cast<_Wrapped *>(mem);
    w->_owner = p;
    w->_type_tag = T::___get_id();

    T *d = new (mem) T();
    return d;

That didn't work, because the compiler-generated default constructor of _Wrapped still gets called, and such constructor in turn calls the default constructor of _owner, godot_object*(), which makes it nullptr. There are other, even more hacky ways to do this I think (global thread_locals, forcing definition of a constructor to prevent assignment...) but that's really dirty.

• Could we enforce the presence of _init? One way to do it might be to simply not generate any binding for those """"virtual"""" methods, since their pointer ends up nullptr (I still have to verify if it's true for all of them, but I think it is, because Godot provides pointers nowhere in their definition in the engine). This way, the absence of the method would cause a compile error in the registration of the class since T::_init would not be defined. It has the drawback of forcing its implementation though, which is not often needed.

• On the other hand, we could leave an empty implementation instead, making _init optional. But at the same time, we don't want users to think they can use the regular C++ constructor, because of the inability to use Godot methods like I described above (cuz it would crash as well).

• A variant of this would be to enforce the definition of a constructor taking an argument to initialize _Wrapped, instead of asking to define an _init function. It is more idiomatic, although it's a bit more to type (and correctly calling the base constructor). This would still require users to define it, and required on all the binding classes since we may inherit from them. Basically parameterless constructor wouldnt be possible.

I havent tried the last two things yet.