Set instance and instance binding in Wrapped constructor.

[Daylily-Zeleen]

Fixes #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]

Thanks again for working on this!

I've finally gotten around to doing some testing and deeper review - I have a couple of notes.

However, overall, I think I'm personally in favor of going forward with this. While C++ doesn't make this easy for us, I feel like the added complexity isn't too bad, and this does fix a big compatibility issue between what developers can do in modules vs GDExtensions.

[dsnopek]

Thanks, this is looking really great!

All the important bits are good - I only have some small nitpicks :-)

[dsnopek]

Thanks!