Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow GDExtensions to register virtual methods and call them on scripts #87758

Merged

Conversation

dsnopek
Copy link
Contributor

@dsnopek dsnopek commented Jan 30, 2024

Currently, GDExtension classes can implement virtual methods registered in Godot. However, GDExtensions classes can't register their own virtual methods, which scripts attached to them can implement.

This PR adds the necessary changes to the GDExtension interface to allow this!

PR godotengine/godot-cpp#1377 adds support for this to godot-cpp.

Fixes #82267

Production edit: closes godotengine/internal-team-priorities#37

Copy link
Contributor

@paddy-exe paddy-exe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked the code to the best of my understanding. Reading the source code the changes makes sense to me. I haven't tested it though.

Copy link
Contributor

@Bromeon Bromeon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very cool feature! 😊

core/extension/gdextension_interface.h Outdated Show resolved Hide resolved
core/extension/gdextension_interface.h Outdated Show resolved Hide resolved
core/extension/gdextension_interface.cpp Show resolved Hide resolved
@dsnopek dsnopek force-pushed the gdextension-register-virtual-method branch from d699cfd to 724a471 Compare February 9, 2024 11:42
@dsnopek
Copy link
Contributor Author

dsnopek commented Feb 9, 2024

@Bromeon Thanks for the review! I've attempted to address your points in my latest push :-)

@Bromeon
Copy link
Contributor

Bromeon commented Feb 9, 2024

Thanks, responded! Will also try to test this with godot-rust this or next week.

Copy link
Contributor

@Bromeon Bromeon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

General question: I assume there is no way to require GDScript to implement a certain method, right? As in abstract/pure virtual methods.

To implement such semantics, user code should probably resort to a default implementation that causes an error?

core/extension/gdextension_interface.h Show resolved Hide resolved
@dsnopek
Copy link
Contributor Author

dsnopek commented Feb 10, 2024

General question: I assume there is no way to require GDScript to implement a certain method, right? As in abstract/pure virtual methods.

Not via this API. However, the godot-cpp PR adds a GDVIRTUAL_REQUIRED_CALL() (which is based on a macro of the same name in Godot) which will print an error if the virtual method isn't implemented. This is in contrast to the GDVIRTUAL_CALL() macro which just evaluates to false if the method isn't implemented. So, I'd expect other bindings to do something similar, generating errors when needed.

To implement such semantics, user code should probably resort to a default implementation that causes an error?

Yep, something like that! In godot-cpp we're mimicking how Godot does it with macros and templates, but other bindings can do whatever makes sense for that particular binding.

@Bromeon
Copy link
Contributor

Bromeon commented Feb 12, 2024

So, I implemented this in Rust, seems to work well! 🦀

Two questions that came up during implementation -- please let me know if that's by design, or I missed something:

  1. It doesn't seem possible to register both a regular (early-binding) and a virtual function with the same name. So while the binding itself can initiate a dynamic dispatch (calling the script method or a C++/Rust fallback), the Godot engine cannot (via Object.call, or from GDScript code). It will always call the script method, or fail if there is no script.

    • The workaround would be to have a separate method that calls the dynamic-dispatch mechanism from the binding. This method could then be registered with Godot, making the dispatch available to GDScript.
  2. Overridden virtual functions in scripts cannot call their super implementation, i.e. C++/Rust code (again because it doesn't exist, from the Godot perspective).

    • Also here, this could be addressed by having a separate method that contains only the fallback code.

Copy link
Contributor

@Bromeon Bromeon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only thing left might be a short TODO on the conventions (see here), so we avoid proliferating even more styles 😉

None of the points in my last post (super calls, script-side dynamic dispatch) is a blocker in my opinion -- even in case we eventually plan to change anything in that regard, this PR is a great foundation either way 🙂 Once merged, bindings can follow up with their implementation, too 🚀

@dsnopek
Copy link
Contributor Author

dsnopek commented Feb 12, 2024

@Bromeon:

So, I implemented this in Rust, seems to work well! 🦀

Great, thanks!

  1. It doesn't seem possible to register both a regular (early-binding) and a virtual function with the same name. So while the binding itself can initiate a dynamic dispatch (calling the script method or a C++/Rust fallback), the Godot engine cannot (via Object.call, or from GDScript code). It will always call the script method, or fail if there is no script.

This is just how virtual methods in Godot work (including in C++ modules): you can't really have a default implementation of a virtual method.

The usual pattern is to have a non-virtual method named without an underscore which calls a virtual method named with an underscore. Then if the class wants to do some default behavior if the virtual method isn't implemented (checked via GDVIRTUAL_IS_OVERRIDDEN() or the return value of GDVIRTUAL_CALL()), or process the return value of the virtual method in some way, it's done in the non-virtual method. And it's the non-virtual method that's the API that folks are expected to call directly, not the virtual method.

This is, of course, different than how virtual methods work in C++, but it's how the binding layer does them.

2. Overridden virtual functions in scripts cannot call their super implementation, i.e. C++/Rust code (again because it doesn't exist, from the Godot perspective).

Yes, but this sort of makes sense in the context of virtual methods not having default implementations.

However, I do think we should add the ability to call super implementations (I have the start of a branch that does this - I probably won't finish it in time for Godot 4.3, though), but not to call the default implementation (because there isn't one). Instead, this is needed for when a GDExtension implements a virtual method from a parent class, and the script also wants to implement that virtual method, but it doesn't want to completely wipe out the GDExtensions implementation. I think this will be especially important when we get around to implementing the ability for a GDExtension class to descend from a parent class in another GDExtension.

But we don't need to account for that in this PR - that's for a different PR in the future. :-)

@dsnopek dsnopek force-pushed the gdextension-register-virtual-method branch from 724a471 to be11002 Compare February 12, 2024 19:29
@dsnopek
Copy link
Contributor Author

dsnopek commented Feb 12, 2024

Thanks!

The only thing left might be a short TODO on the conventions (see here), so we avoid proliferating even more styles 😉

I've added the TODO's in my latest push :-)

@Bromeon
Copy link
Contributor

Bromeon commented Feb 12, 2024

This is just how virtual methods in Godot work (including in C++ modules): you can't really have a default implementation of a virtual method.

Thanks for the elaboration, that makes sense. I will then document this accordingly in Rust and see if we can also establish some convention.


However, I do think we should add the ability to call super implementations [...], but not to call the default implementation (because there isn't one). Instead, this is needed for when a GDExtension implements a virtual method from a parent class, and the script also wants to implement that virtual method, but it doesn't want to completely wipe out the GDExtensions implementation.

I see, so this would work for extensions overriding Godot virtual methods (which are then further overridden by scripts), but not for virtual methods declared by the extension. This might (?) cause some confusion but if it's well documented (including the patterns to work around it), should be OK.


I've added the TODO's in my latest push :-)

Looks great, thanks a lot! From my side, this PR is good to go 🙂

@akien-mga akien-mga merged commit 306dd5b into godotengine:master Feb 12, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@limbonaut
Copy link

limbonaut commented Feb 13, 2024

Thanks! 🤩 Can it be cherry-picked for 4.2.2, maybe? 🙏

@Bromeon
Copy link
Contributor

Bromeon commented Feb 13, 2024

@limbonaut The GDExtension API generally targets the next minor Godot version (4.3 here), binary compatibility gets very complex otherwise. So this won't be backported.

You can however either build Godot yourself from master or wait for the next 4.3-dev release. There are also build artifacts from the CI pipeline that you can use.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BIND_VIRTUAL_METHOD in GDextensions doesn't create a virtual function in GDscript
5 participants