Implement vararg methods of builtin classes.

[Daylily-Zeleen]

Implement builtin classes' vararg methods.

Need #76047.

Now, call(), call_deferred(), rpc(), rpc_id(), bind() in Callable, and Signal::emit() can be called with any count of arguments.

Fixes #802
Fixes #1093

[dsnopek]

Thanks!

I tested this (using Callable::bind()) and it worked! Attached to this comment is a patch that adds an automated test for it. Feel free to add it to your PR (after rebasing, since your PR is from before the tests), otherwise I'll make a follow up later.

The one thing I'm unsure about is putting these implementations into the builtin_vararg_methods.hpp header. Why not put them in the header of the class they belong to? So, for example, putting Callable::bind() in callable.hpp?

gdextension-callable-bind-test.txt

[dsnopek]

Discussed at the GDExtension meeting, and agreed that the generated functions would be better in the *.hpp files associated with the class that the methods are on.

[Daylily-Zeleen]

Discussed at the GDExtension meeting, and agreed that the generated functions would be better in the *.hpp files associated with the class that the methods are on.

I had tried to put implements in theirs *.hpp file, it will occur some compile issue.
If I recall correctly, if I put implements in theirs *.hpp file, they need include variant.hpp.
However, variant.hpp already include these builtin classes through including builtin_types.hpp.

[dsnopek]

Hm. How does Godot itself workaround this problem? The same issue should exist there, because variant.h includes callable.h. callable.h is forward declaring Variant, which I actually had thought would only work if all references are Variant * or Variant &, but Callable::callv() returns Variant, and the vararg template functions seem to look very similar to the ones generated in this PR.

[Daylily-Zeleen]

Hm. How does Godot itself workaround this problem?

I roughly skimmed the implement of Callable::bind() in godot source code.
It is declared in callable.h, but is implemented in variant.h.
For other methods, they are not templates in godot.

So, I think this pr is reasonable.

[dsnopek]

I just attempted to modify this PR to put the template implementations in the same header file as the their classes (so, callable.hpp and signal.hpp). It was able to get it to work for all methods except for Callable::call() because it returns a Variant, and I'm not aware of a way to workaround the problem that return types can't be forward declared.

Looking at the Godot code, it doesn't have this problem, because it doesn't have a Callable::call() method, only Callable::callv() and Callable::callp() which aren't templates (so completely defining Variant can wait until later).

I'm not sure on the best way forward. This PR does include the new header it creates at the end of variant.hpp, so if a developer includes variant.hpp then they're good. But I'm worried about developers that include only callable.hpp or signal.hpp, then try to call any of the vararg methods and it won't be obvious to them that they need to include the builtin_vararg_methods.hpp header.

Maybe we could include the definition of all the methods except for Callable::call() in their respective header files? That way the problem only comes up for Callable::call(), but not Signal::emit(), for example?

Probably worth discussing again at a GDExtension meeting!

[mihe]

Probably worth pointing out that variant.hpp is included by object.hpp (and maybe some even "more core" header file) and as such it would likely be difficult to find yourself in a situation where you're including callable.hpp/signal.hpp without already having variant.hpp included.

[dsnopek]

That's a good point! In a GDExtension, you are very, very likely to be declaring a child class of Object and so you would have included object.hpp.

So, perhaps this PR is fine as-is?

[mihe]

Some minor typos here, like Craete, builin and a redundant backtick at the end.

[Daylily-Zeleen]

Thanks for your warning.

[Daylily-Zeleen]

@dsnopek , @mihe
I think in some single files, user still have possibility of including callable.hpp without using any thing of Object, for example, to create a series of utility functions.

If user include variant.hpp, everything is fine.
But if user include callable.hpp only, for example:

// callable_test.h

#pragma once

namespace godot {
class Callable;
}

class Utilities {
public:
	static void bind_extra_int(godot::Callable p_callable);
};

//  callable_test.cpp

#include "callable_test.h"

#include <godot_cpp/variant/callable.hpp>

void Utilities::bind_extra_int(godot::Callable p_callable) {
    p_callable.bind(1);
}

It wiil occur a compile error "LNK2019" about godot::Callable::bind<int>.

So, we still need to deal this problem.

[Daylily-Zeleen]

I found that I have omitted a template in "callable.h" in godot sourc code:

// callable.h
	template <typename... VarArgs>
	void call_deferred(VarArgs... p_args) const {
		Variant args[sizeof...(p_args) + 1] = { p_args..., 0 }; // +1 makes sure zero sized arrays are also supported.
		const Variant *argptrs[sizeof...(p_args) + 1];
		for (uint32_t i = 0; i < sizeof...(p_args); i++) {
			argptrs[i] = &args[i];
		}
		return call_deferredp(sizeof...(p_args) == 0 ? nullptr : (const Variant **)argptrs, sizeof...(p_args));
	}

Dose it means that if user want to call Callable::call_deferred, they should include "variant.h" instead of "callable.h" in godot modulde?

I written a simple test for this in godot module:

// callable_test.h
#pragma once

class Callable;

class TestUtilities {
public:
	static void callable_call_defferred(Callable &p_callable);
};

// callable_test.cpp
#include "callable_test.h"

// Everything is fine.
#include "core/variant/variant.h"

// Compile error!!
// #include "core/variant/callable.h"

void TestUtilities::callable_call_defferred(Callable &p_callable){
    p_callable.call_deferred("Test");
}

As the code comment show, if I include "callable.h" only and call Callable::call_deferred with argument. it will occur compile errors about "'args' using undefined class 'Variant'".

The conclusion is that, if we want to using these templates, we must include "variant.h"/"variant.hpp" both in GDExtension and godot module.

But there has a problem confuse me: why Callable::call_deferred() define in "callable.h", but Callable::bind() define in "variant.h"?
And where should we define these templates? In their "*.hpp" or in "variant.hpp" like current implement in this pr?

[mihe]

why Callable::call_deferred() define in "callable.h", but Callable::bind() define in "variant.h"?

That is indeed a bit strange, but seeing as how you will likely run into compile errors with either of the two solutions (if you fail to include variant.h) then I guess there is no correct way of doing it in this case.

Clearly there's some precedence for this circular dependency in Godot as well, as you point out, so I would put my vote on just going with the pull request as-is. The case for utility functions creating problems seems niche enough to not be a major concern.

[dsnopek]

There's a PR to add a vararg Callable::call() in the engine: godotengine/godot#79341

Looking at the CI, it appears to be encountering the same problem with Variant being an incomplete type that we encountered here. :-) Let's see how this gets resolved upstream, and maybe we can mimick that

[Daylily-Zeleen]

There's a PR to add a vararg Callable::call() in the engine: godotengine/godot#79341

Looking at the CI, it appears to be encountering the same problem with Variant being an incomplete type that we encountered here. :-) Let's see how this gets resolved upstream, and maybe we can mimick that

In godotengine/godot#79341, Callable::call() is defined in "variant.h" like Callable::bind(), it has the same result like this pr.

I think if we keep the "variant.hpp" include "callable.hpp", we have not a perfect solution to use Callable's vararg templates by including "callable.hpp" only.

What about to generate a comment line before these templates to warn users? like this:

	// Please including "variant.hpp" instead of including "callable.hpp" only if you need to call this function.
	template <class... Args>
	Callable bind(const Args &...args) const;

If user encounter this compile issue and go to function definitions, they can get the solution by this comment.

[dsnopek]

We could add a comment, however, I'm not sure we're really encouraging users of godot-cpp to read the generated source code.

I've personally come around to liking this PR as-is. IMO, if we see users having issues with this, we can re-evaluate then.

But that's just my opinion! We have this PR on the agenda for discussion at an upcoming GDExtension meeting.

[dsnopek]

Discussed at GDExtension meeting, and we think this fine - templates are tricky, and it looks like core is going to solve the problem in a similar way.

[dsnopek]

I had forgotten that this on depends on Godot PR godotengine/godot#76047

I've just re-reviewed that one (which is looking great!) but we'll have to wait for it to be merged before this one can be

[dsnopek]

PR godotengine/godot#76047 has been merged, so we can finally merge this one :-)