Virtual function dispatch

[Mercerenies]

See #134.

This MR replaces GodotExt with a new trait for each built-in Godot class. The trait is the name of the Godot class followed by the word "Virtual". So, for instance, a Rust class that extends Node2D would implement the trait called godot::engine::Node2DVirtual. This trait contains all of the virtual methods belonging to Node2D and its ancestors, as well as a few special-cased methods that used to be on GodotExt (init, to_string, and register_class).

All methods are optional. Any unimplemented methods call unimplemented!() on the Rust side and never get registered as virtual methods on the Godot side.

Internally, ImplementsGodotExt has been split out into ImplementsGodotVirtual (which only contains real Godot virtual methods), GodotToString, and GodotRegisterClass. All of these are automatically implemented when you implement the ClassNameVirtual trait, and only the ones that you actually define are implemented (so, for example, GodotToString is only implemented if you actually define a to_string function in your trait impl). The user never sees any of this. From the user's perspective, the expectation used to be "define an inherent impl and implement GodotExt". Now the expectation is "define an inherent impl and implement ClassNameVirtual for your class".

The tests and dodge-the-creeps example have been updated to reflect these changes.

[Bromeon]

Very nice addition, thanks a lot! I'm surprised how well you got around in the codegen source 😅 was it difficult to understand the existing code?

bors try

[Bromeon]

You removed the condition about pointers, why that?

[Mercerenies]

I removed the blanket ban on all method names starting with _. All virtual methods have a Godot name that starts with an underscore, so that would exclude all of the methods I intend to add with this MR. The pointer condition is still present (line 508). Looks like the Rust formatter just re-indented it a bit, which is why it shows as a diff.

[Bromeon]

Oh, I see. This is where semantic diff like difftastic might come in really handy 😎

All good then, thanks for explanation!

[Bromeon]

Do these need to be public traits? (Note that "public" doesn't necessarily mean pub; as you see with many symbols, #[doc(hidden)] allows them to be used in proc-macros without making them part of the public APIs).

GodotInit is public because it's used as a generic bound, to determine whether an object is default-constructible from Godot or not. I don't think to_string and register_class contribute to the type in such a way (maybe this could be discussed for GodotToString, similar to Display). I think they are more like ImplementsGodotVirtual below. However, I see that you use them as bounds in the builder API -- possibly something to revisit once we get to that.

One issue with the current approach is that every time we add another method to what was previously in GodotExt, we need to code-generate a new trait and adjust the bounds in several places. But I'm also fine to leave it for now and address that problem, once we have it.

---

What I would definitely change is to remove the "virtual" from the method name, as those method are not technically related to Godot virtuals. The 2 underscores prefix can stay.

[Mercerenies]

My logic here was to make these other traits like GodotInit, since they follow the same principles: Can be used as a Rust-side bound, intended to be implemented automatically based on #[godot_api], and is a special case in Godot to begin with. Open to other suggestions for how to handle that though. Will remove __virtual.

[Bromeon]

Regarding __register_class, I don't know if this abstraction would be necessary in user-facing code? No one is ever going to invoke the class registration method manually, and I also don't think whether a class has a registration method or not should be part of its public API.

__to_string is more reasonable, but also here, I'm not sure if the Rust-side bound fully reflects reality: in Godot, it's possible to invoke to_string() on any object, independently of whether it implements such a method or not. So we may constrain implementations more than necessary by requiring such a bound.

Maybe you could mark them #[doc(hidden)] for now and add a // TODO -- we probably have to revisit this anyway once we get to builder APIs. Until then, I don't really see a use case for having these traits public.

[Mercerenies]

Sounds good. I don't have a strong opinion on whether they're public or not, so I'm happy to hide them away for now and have the discussion in the future.

[bors]

try

Build succeeded:

• full-ci

[Mercerenies]

Made some changes. Thanks for the quick review, Bromeon! The code generation is actually very straightforward and well-designed. You've done a good job here.

[Bromeon]

Thanks for the update!

One small disadvantage compared to GodotExt is that the user has to repeat the base class for a third time:

#[derive(GodotClass)]
#[class(base=CanvasLayer)] // 1
pub struct Hud {
    #[base]
    base: Base<CanvasLayer>, // 2 (could use Self::Base)
}

#[godot_api]
impl CanvasLayerVirtual for Hud { // 3
    fn init(base: Base<Self::Base>) -> Self {
        Self { base }
    }
}

So if you change the base of a class, you have one more place to update now -- even if you use only init, no base-specific virtual functions.

That's probably fine though -- it could also be seen as a feature that you immediately know to which Godot class the virtual methods belong.

---

Merge conflicts arised during a refactoring in the proc-macro handling, sorry about that 🙂

You now have 24 commits -- could you squash them into a single one?
Or if you want multiple commits, make sure that each combines logically related changes and compiles on its own (probably it's easier to have a single one).

[Mercerenies]

I believe I've fixed the merge conflicts, and the commits are down to one.

[Bromeon]

In mod prelude, we currently have this:

    pub use super::engine::{
        load, try_load, utilities, AudioStreamPlayer, Camera2D, Camera3D, Input, Node, Node2D,
        Node3D, Object, PackedScene, RefCounted, Resource, SceneTree,
    };

What do you think about adding the *Virtual trait for each class * that's already present?

Those could then be used inside the dodge-the-creeps example.

---

Also, do you think it would be possible to test that virtual methods are actually invoked, inside itest/rust/src/virtual_methods_test.rs? 2-3 examples should be more than enough, maybe something like _ready, _process and another one? Let me know if you need help.

[Bromeon]

#164 should allow to test lifecycle functions directly on the the scene tree (ready, process etc).

[Mercerenies]

Added tests for _ready, _enter_tree, and _exit_tree using the new dependency injection in #164. I think we'll need access to Godot coroutines to test _process, as it doesn't run instantaneously when a node is added to the scene tree (so we need a way to wait for "idle_frame", for instance, or just sleep our test for a few frames while Godot continues processing). If you have any ideas on that, I'm happy to hear them.

[Bromeon]

Thanks for updating! Regarding _process, I think that's fine for now. We can think about it outside of this PR, if needed 👍

bors try

[bors]

try

Build succeeded:

• full-ci

[Bromeon]

Let's merge this, so everyone can start using it 🚀
Thanks a lot for the great effort! I have some follow-up ideas already 🙂

bors r+

[bors]

Build succeeded:

• full-ci