Expose is_required in extension API for virtual methods

[TitanNano]

The engine currently does not expose if a virtual method is required to be implemented or not. The only indicator for a method to be required is if there is any call to it via GDVIRTUAL_REQUIRED_CALL at runtime.

It would be beneficial to GDExtensions if virtual methods were marked as required and only then GDVIRTUAL_REQUIRED_CALL could be called on them. The engine should then also expose a is_required field in extension_api.json.

Bindings like godot-rust/gdext could then handle these required methods appropriately, e.g. fail the compilation if a method has not been implemented.

EDIT: Related issue in godot-rust/gdext#192 (comment)

[dsnopek]

This is an interesting idea!

I guess we could include whether or not it's required in the definition (maybe via an extension to the GDVIRTUAL*() macros?), and then get rid of GDVIRTUAL_REQUIRED_CALL() and just have GDVIRTUAL_CALL() do the right thing depending on the definition. This could use constexpr to avoid adding any extra overhead. So, I don't think there's any technical problems with doing it.

Then I guess it'd be a question of whether or not we think it's worth changing all the GDVIRTUAL*() declarations in the code base in order to add this extra information.

@Bromeon What do you think about this?

[AThousandShips]

I don't think it's feasible to detect it via the calls, and that if we want it to be automated we should change the declaration macros, but that adds quite a bit of extra complexity

I.e. like GDVIRTUAL macros with a required part along with the argument and return

However there could be times when a method is required but wouldn't always be called with it, so I'd keep the call part separate

And we could have a check so that a bind must be marked required if called that way

I'll look at the code when I have some time and energy for it and see if I can come up with some ideas

[dsnopek]

For the implementation, I was thinking we could change the declaration macros, so we'd have like GDVIRTUAL*_REQUIRED() (ex GDVIRTUAL1RC_REQUIRED(int, int) or whatever), and add something like static constexpr bool _gdvirtual_##m_name##_required = true; // or false, for the non-required one.

This value could be used by the GDVIRTUAL_BIND() macro to add the required information to ClassDB. And, we could either have GDVIRTUAL_CALL() look at that or just generate the call method from the declaration macro to either be required or not rather than using a template argument.

Anyway, I think the implementation should be fairly easy.

But if you believe it's reasonably possible, I'd say we could give it a try 🙂 or do you see any downsides?

The biggest challenge that I personally see is just the work of changing all the GDVIRTUAL*() and GDVIRTUAL_CALL() usages throughout the code base, and not making any mistakes. :-)

However there could be times when a method is required but wouldn't always be called with it, so I'd keep the call part separate

Perhaps. If there isn't a case like that already, I'm not sure we should go out of our way to account for it, though.

[TitanNano]

The biggest challenge that I personally see is just the work of changing all the GDVIRTUAL*() and GDVIRTUAL_CALL() usages throughout the code base, and not making any mistakes. :-)

Does it make sense to keep GDVIRTUAL_REQUIRED_CALL and make it only compatible with GDVIRTUAL*_REQUIRED() so that the right pair has to always be used? I don't know if that is possible.

[dsnopek]

Does it make sense to keep GDVIRTUAL_REQUIRED_CALL and make it only compatible with GDVIRTUAL*_REQUIRED() so that the right pair has to always be used?

Personally, I don't see the sense in keeping it if the right pair always has to be used.

[dsnopek]

I did a quick prototype implementation in draft PR godotengine/godot#93311

I didn't end up adding the static constexpr bool _gdvirtual_##m_name##_required variable, and instead added a new METHOD_FLAG_VIRTUAL_REQUIRED, since we're storing the virtual methods in ClassDB as MethodInfo.

[TitanNano]

This has been implemented in godotengine/godot#93311