Preparation to fix memory leak when using Ref in NativeScript

[PhilWun]

Preparation to fix godotengine/godot-cpp#215.

[Zylann]

Some context is needed here.
Currently discussing that issue with @sheepandshepherd and the thing is, that fix is a workaround to "undo" an unwanted reference initialization.

In C++ bindings, we instanciate new classes with T::_new(), which returns a pointer.
With a Godot class, it works fine as we just invoke the constructor, giving us a godot_object* directly, just like in engine.

The problem arises when we want to instance custom classes, from our C++ library. First, we need to get a hand on the NativeScript resource... which is a .gdns file that our framework has no idea where it is. And it's a file because in Godot, scripts are resources, and resources must be files. So we gave up on unicity, and worked around that by creating a new instance of the NativeScript resource every time (not clean, but worked so far), just so we could call its new_() function https://docs.godotengine.org/en/stable/classes/class_nativescript.html#class-nativescript-method-new.
But because this is a script API call, it returns us a Variant with an already-initialized refcount_init. Because of that, when that temporary Variant goes out of scope to become a pointer in our API, that sole reference is lost and the object is deleted. That would not have been the case if it had been a simple pointer all the way. So initially we thought of "undoing" that by adding an extra reference here: https://github.com/GodotNativeTools/godot-cpp/pull/401/files
However that means we create a leak specifically on custom Reference classes due to that extra reference. Which then, led to the present PR, to keep "undoing" the mess.

So in my opinion, those PRs solve the problem in the wrong way. They rely on the existence of complicated workarounds, which should not have been there in the first place.
What we should have instead, is a way to setup the object and its script instance without involving a Variant or Ref wrapper, just like Godot classes can be instanced already. This way, we could continue treating custom classes inheriting Reference the same as any other class.

[ghost]

It's possible to setup the script instance of an Object using Object::set_script. No Variants are involved that way.

[Zylann]

@toasteater interesting... but then how do you get a hand on your custom class instance pointer?

[ghost]

@Zylann Call godot_nativescript_get_userdata?

[Zylann]

@toasteater like so?

template <class T>
T *get_custom_class_instance(const Object *obj) {
	return (obj) ? (T *)godot::nativescript_api->godot_nativescript_get_userdata(obj->_owner) : nullptr;
}

template <class T>
inline T *create_custom_class_instance(const char *class_name) {
	godot::NativeScript *script = godot::NativeScript::_new();
	script->set_library(get_wrapper<godot::GDNativeLibrary>((godot_object *)godot::gdnlib));
	script->set_class_name(class_name);

	// This makes Godot create the custom class instance and attach it to a new instance of the base class
	Object *base_obj = T::___new_godot_base();
	base_obj->set_script(script);
	T *instance = get_custom_class_instance(base_obj);

	return instance;
}

I need to check if it works properly. Notably, I wonder if the base_obj I create here will be freed (does it need to?), or if I could even use some more direct function without requiring its wrapper,

[ghost]

@Zylann Yes. The Rust bindings do something like that and it seems to work.

I don't think you want to free base_obj. Freeing a object will also free the script instance attached to it, so you'll end up with a dangling pointer.

[Zylann]

@toasteater what I mean is like I explained earlier, base_obj is a wrapper around the real Godot object. It's a heap-allocated object which then contains an _owner pointer field to the actual Godot object (unless I miss something?). I wonder if that wrapper specifically is going to leak or if it's actually needed.

[ghost]

@Zylann The wrapper itself is probably not needed, since whatever magic in your godot_instance_create_func won't be able to see it at all, and will be settings the _owner field of a "fresh" instance allocated as part of the new object. You just need to find a way to deallocate it without running its destructor, in case it frees _owner or decrements the reference count there.

[aaronfranke]

@PhilWun Is this still desired? If so, it needs to be rebased on the latest master branch.

The commits you pushed are merge commits, which are not good to have in a PR. See this article to learn how to rebase: https://docs.godotengine.org/en/latest/community/contributing/pr_workflow.html#mastering-the-pr-workflow-the-rebase

[Zylann]

The bug that led to this PR has been fixed, so it's probably no longer necessary.

[akien-mga]

Closing then. Please comment if it's still necessary and we can reopen it.