Fix deriving a custom class with virtual methods

[Zylann]

Fixes #854

[MGilleronFJ]

Actually, I have a doubt. Back to the case:

class TestNode : public godot::Node {
	GDCLASS(TestNode, godot::Node)
public:
	void _ready() override;
private:
	static void _bind_methods();
};

class DerivedNode : public TestNode {
	GDCLASS(DerivedNode, TestNode)
private:
	static void _bind_methods();
};

With this PR, registering DerivedNode will no longer BIND_VIRTUAL_METHOD(_ready) (because it doesnt directly override it, and it doesnt compile). But how will it be bound then? Because it needs to be bound in some way for the base class, I think. How does Godot's GDExtension API handle this? Does registering TestNode take care of registering _ready for the DerivedNode class? Or does the registration of DerivedNode somehow has to do it again?

(on a side note, I'm confused at how BIND_VIRTUAL_METHOD is used here to implement a virtual method rather than declaring one. Related to another issue I have where I have no idea how to define GDVIRTUAL methods when porting a module)

[bruvzg]

With this PR, registering DerivedNode will no longer BIND_VIRTUAL_METHOD(_ready) (because it doesnt directly override it, and it doesnt compile).

Why should it bind _ready if there's no _ready in DerivedNode?

[bruvzg]

Seems fine to me.

[MGilleronFJ]

@bruvzg

Why should it bind _ready if there's no _ready in DerivedNode?

Because there is, in the base class. If one creates a DerivedNode instance, Godot still has to call TestNode::_ready(). Hence my question, does GDExtension require to do something about it when registering a derived class, or is it handled by having registered the base class separately? If it is, then yeah I think it should be fine. Probably need to test further

[Zylann]

Gave it a go, it worked. Added a print inside TestNode::_ready and it prints when instantiating both TestNode and DerivedNode. I'm tired, DerivedNode didnt actually print anything, but it should have.

[aaronfranke]

Does it work with multiple levels of derived nodes?

[Zylann]

Which combination do you want to try?

[aaronfranke]

I wanted to check this:

class A : public godot::Node {
	GDCLASS(A, godot::Node);
	static void _bind_methods();
public:
	void _ready() override;
};

class B : public A {
	GDCLASS(B, A);
	static void _bind_methods();
};

class C : public B {
	GDCLASS(C, B);
	static void _bind_methods();
};

[Zylann]

That works too.
Hmm actually I expected _ready to be printed 3 times here. It only printed for A (TestNode). Same happened with my previous test. Which means... back to my question about GDExtension. Cuz that API is unique (doesnt exist in modules AFAIK cuz they dont need it) and has no documentation, without that info it's not possible to figure out the right thing to do, or even if that's a bug or not.

To recap here, in that setup, when registered in ClassDB, A calls bind_virtual_method(_ready), B and C don't.

Actually, I see bind_virtual_method doesn't directly call into Godot. So maybe something could be changed here too? I wonder how Godot gets those methods. There is a get_virtual_func provided to Godot so I guess Godot asks the library if a given class has a certain virtual method (cached on first call, per object instance), but given the results, it doesn't lookup through the hierarchy. Maybe our get_virtual_func could do that then?

GDNativeExtensionClassCallVirtual ClassDB::get_virtual_func(void *p_userdata, const char *p_name) {
	const char *class_name = (const char *)p_userdata;

	std::unordered_map<std::string, ClassInfo>::iterator type_it = classes.find(class_name);
	ERR_FAIL_COND_V_MSG(type_it == classes.end(), nullptr, "Class doesn't exist.");

	const ClassInfo *type = &type_it->second;

	while (type != nullptr) {
		std::unordered_map<std::string, GDNativeExtensionClassCallVirtual>::const_iterator method_it = type->virtual_methods.find(p_name);

		if (method_it != type->virtual_methods.end()) {
			return method_it->second;
		}

		type = type->parent_ptr;
	}

	return nullptr;
}

On the other hand, if every method of the hierarchy had to be registered via template calls, it's annoying because the code may have to become significantly more complex. Because I could start by changing this:

	static void register_own_virtuals() {
		m_inherits::register_virtuals<m_class, m_inherits>();
	}

	template <class T, class B>
	static void register_virtuals() {
		m_inherits::register_own_virtuals(); // First register base class virtuals
		m_inherits::register_virtuals<T, B>(); // Then attempt to register virtuals of the caller on top
	}

This goes recursively such that all the inheritance hierarchy gets to register its virtual functions, and child classes would do so "on top" of it.
So when B is registered, the calls would be:

B::register_virtuals<B, A>()
	A::register_own_virtuals();
		Node::register_virtuals<A, Node>();
			bind_virtual_method(A::_ready)
	A::register_virtuals<B, A>();
		Node::register_own_virtuals();
			// Nothing, empty function
		Node::register_virtuals<B, A>();
			// Nothing, B doesn't override `_ready`

However with C, it would start binding A twice:

C::register_virtuals<C, B>()
	B::register_own_virtuals()
		A::register_own_virtuals();
			Node::register_virtuals<A, Node>();
				bind_virtual_method(A::_ready)
		A::register_virtuals<B, A>();
			Node::register_own_virtuals();
				// Nothing
			Node::register_virtuals<B, A>();
				// Nothing, B doesn't override `_ready`

	B::register_virtuals<C, B>();
		A::register_own_virtuals();
			Node::register_virtuals<A, Node>();
				bind_virtual_method(A::_ready)
		A::register_virtuals<C, B>();
			Node::register_own_virtuals();
				// Nothing
			Node::register_virtuals<C, B>();
				// Nothing, `C` doesn't override `_ready`

And with more overriden virtuals it would keep calling multiple times bind_virtual_method for the same method, as the hierarchy of calls "unfolds" from parent to child. It might work but that's a bit messy, hard to track and still no idea if that's what GDExtension expects. In fact it would even throw errors here, for every derived class and every duplicate calls:

godot-cpp/src/core/class_db.cpp

Lines 296 to 306 in 8670305

	void ClassDB::bind_virtual_method(const char *p_class, const char *p_method, GDNativeExtensionClassCallVirtual p_call) {
	std::unordered_map<std::string, ClassInfo>::iterator type_it = classes.find(p_class);
	ERR_FAIL_COND_MSG(type_it == classes.end(), "Class doesn't exist.");
	ClassInfo &type = type_it->second;
	ERR_FAIL_COND_MSG(type.method_map.find(p_method) != type.method_map.end(), "Method already registered as non-virtual.");
	ERR_FAIL_COND_MSG(type.virtual_methods.find(p_method) != type.virtual_methods.end(), "Virtual method already registered.");
	type.virtual_methods[p_method] = p_call;
	}

So modifying get_virtual_func sounds better?

[Zylann]

Made ClassDB::get_virtual_func get virtuals from parent classes too. Had to also fix an issue with parent_ptr, it was always null.

Now it works:

Note:
I also notice a few things, which could be worked on in future PRs:

• It looks like if someone defines _ready(int some_arguments, int not_expected), it could be seen by the old (and new) code as an overriden method. I gave a quick try at doing that, but it didnt compile, so at least it shouldnt blow up at runtime.
• virtual or even override aren't required to have _ready be registered as an overriden method. At least, when called by Godot. You could write _ready without either, and it would work. But for the C++ side to work correctly and avoid confusion, it's better to use virtual anyways. I don't know which template magic could be used to pick up on that.

[aaronfranke]

I gave this a test with my attempt to port 1D to GDExtension and it fixes one of the critical issues there.

[akien-mga]

Thanks!