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

Virtual Function Dispatch #134

Closed
Mercerenies opened this issue Mar 2, 2023 · 5 comments · Fixed by #136
Closed

Virtual Function Dispatch #134

Mercerenies opened this issue Mar 2, 2023 · 5 comments · Fixed by #136
Labels
c: register Register classes, functions and other symbols to GDScript feature Adds functionality to the library

Comments

@Mercerenies
Copy link
Contributor

Currently, aside from a few special cases for common virtual functions like _ready and _process (which exist on a significant percentage of Godot objects), godot-rust does not currently support overriding virtual functions from built-in Godot classes. For instance, CanvasItem._draw cannot be overridden in Rust currently. There are 1159 virtual functions in Godot, split across 48 classes.

Virtual functions go through a different dispatch mechanism than other functions in Godot. Regular functions are registered with classdb_register_extension_class_method, while virtual functions are queried at runtime via GDExtensionClassCreationInfo.get_virtual_func. Currently, godot-rust special-cases a few virtual functions in GodotExt but does not provide a mechanism for general overriding.

This has been discussed a bit before, such as in the comments of #76 and #102. From what I can tell, there are two main ideas that have been floated for this.

  • One idea is to write virtual functions in a struct's inherent impl, and to mark them with a special annotation such as #[func(override)] rather than #[func]. This would be fairly simple to implement. The downside is that we lose out of type safety; there's nothing on the Rust side that stops us from overriding a function _draw() with one that takes arguments, and we'll only discover our mistake at runtime.
  • The other idea is to generate a trait for each built-in Godot class. Then, rather than having users impl GodotExt for CustomStruct, we would have them impl the one particular trait that matches the type they're subclassing, such as impl GodotNode2DExt for CustomStruct. Then we have some magic (probably in #[godot_api]) that generates GodotExt and dynamically dispatches to the correct methods. This is more complex to implement (and requires some significant code generation to get all of the traits), but it has the advantage of being completely type-safe. Our generated traits would probably default-implement all trait methods with an appropriate default, so that users only have to override the methods they actually want from the class (It's possible they'll all just panic!, which is not ideal but is at least consistent).
@Bromeon Bromeon added feature Adds functionality to the library c: register Register classes, functions and other symbols to GDScript labels Mar 2, 2023
@Bromeon
Copy link
Member

Bromeon commented Mar 2, 2023

  • One idea is to write virtual functions in a struct's inherent impl, and to mark them with a special annotation such as #[func(override)] rather than #[func]. This would be fairly simple to implement. The downside is that we lose out of type safety; there's nothing on the Rust side that stops us from overriding a function _draw() with one that takes arguments, and we'll only discover our mistake at runtime.

This would appear in an impl block as follows:

#[godot_api]
impl MyClass {
    #[func(override)]
    fn virtual_fn(some_int: i64) {...}
}

Thinking about that, the macro #[godot_api] sees the following things:

  • MyClass
  • the signature of virtual_fn
  • the fact that virtual_fn is virtual (through override)

However, it does not see the base class. That one is passed in to the struct definition's #[derive(GodotClass)] macro, and at the macro resolution stage, we don't have access to that information. So if we want type safety, we might need to provide the base class name explicitly, which could become a bit ugly...

Or we directly use the second approach.


  • The other idea is to generate a trait for each built-in Godot class. Then, rather than having users impl GodotExt for CustomStruct, we would have them impl the one particular trait that matches the type they're subclassing, such as impl GodotNode2DExt for CustomStruct.

This seems quite straightforward and intuitive in the way that traits work in Rust. The only drawback I see is that one might end up with lots of impl blocks.

If you look at the EditorProperty class, you'll see virtual functions in many layers of the class hierarchy:

  • EditorProperty::_set_read_only
  • Container::_get_allowed_size_flags_horizontal
  • Control::_gui_input
  • CanvasItem::_draw
  • Node::_process
  • Object::_to_string (while technically not a virtual, it behaves exactly like one in GDScript).

There are three ways to deal with this:

  1. We require 6 separate impl blocks. Annoying, but realistically rare in practice (most classes can live with the Node virtuals, a few need more).
  2. The generated traits already have cumulative methods. So CanvasItem would also have the _process() method, since it inherits from Node. So implementing a single impl block would be enough, no matter the class.
  3. One impl block expands to multiple trait impls. However, then the user again needs to manually annotate per-method, to which class it belongs, somewhat negating the point.

We also need to consider what to do with the "quasi-virtual methods" currently in GodotExt, like init (default constructor) or register_class (for later with the builder). Should there still be a separate GodotExt trait, or should those also be available in all the virtual traits?

@lilizoey
Copy link
Member

lilizoey commented Mar 2, 2023

For a trait-based implementation, i'd at least advocate for having virtual traits accumulate. Though if a class isn't adding any new virtual methods we can just have it reuse the old parent's trait. Like we'd have (names pending):

// node.rs
pub struct Node { ... } 
pub trait NodeVirtualMethods { ... }
// node_3d.rs
pub struct Node3D { ... }
pub use NodeVirtualMethods as Node3DVirtualMethods;

which would make it easy for the user to either name the trait themselves, or for a macro to auto-generate the name, while avoiding repeating every virtual method for every single class that has a virtual method.

I'd also like advocate for keeping all virtual-ish methods in the generated traits too.

@Mercerenies
Copy link
Contributor Author

Yes, if we go the trait way I was thinking of having it cumulative. So, from the user's perspective, they always have to write exactly two impl blocks: One that's inherent to their type for all non-virtual functions, and one for the most specific subtype trait, which has all of the virtual methods that type (and its hierarchy) requires. Internally, we take the second impl and use it to generate an impl GodotExt for MyType. I think GodotExt is still important, because Godot needs one centralized place to look for virtual methods, though I don't think we would ever expect the user to implement GodotExt anymore; it'll just be something we generate on the backend when they impl one of the more specific traits.

@Bromeon
Copy link
Member

Bromeon commented Mar 2, 2023

The current GodotExt however has some methods that are not "virtual functions" in Godot's terminology:

  • init() -- default construction
  • register_class() -- builder API (not yet supported)
  • possibly more in the future

But they can probably be integrated into the "cumulative trait" as well. Conceptually, they are close to the virtuals in the sense that they are invoked at fixed time points in the object lifecycle.

@Mercerenies
Copy link
Contributor Author

I think they can be integrated into the cumulative trait well enough. to_string and notification are also special-cased, I believe (though godot-rust doesn't have notification yet, if I recall correctly).

@Bromeon Bromeon linked a pull request Mar 5, 2023 that will close this issue
bors bot added a commit that referenced this issue Mar 19, 2023
136: Virtual function dispatch r=Bromeon a=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.

Co-authored-by: Silvio Mayolo <mercerenies@comcast.net>
@bors bors bot closed this as completed in #136 Mar 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: register Register classes, functions and other symbols to GDScript feature Adds functionality to the library
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants