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 GodotExt syntax to be used for virtuals #188

Closed
wants to merge 2 commits into from

Conversation

Bromeon
Copy link
Member

@Bromeon Bromeon commented Mar 19, 2023

#136 introduced virtual function dispatch, and with it a new syntax for init, ready etc:

#[derive(GodotClass)]
#[class(base=Area2D)]   // <- 1
pub struct Player {
    ...

    #[base]
    base: Base<Area2D>,   // <- 2
}

#[godot_api]
impl Area2DVirtual for Player {   // <- 3
    fn init(base: Base<Area2D>) -> Self { ... }
    fn ready(&mut self) { ... }
    fn process(&mut self, delta: f64) { ... }
}

As you can see, there is now quite a bit of repetition, although (2) could use Base<Self::Base>.


I took it as a challenge to see if it's possible to still use GodotExt in place of the *Virtual name:

#[godot_api]
impl GodotExt for Player {
    ...
}

This turned out to be not straightforward, because #[godot_api] does not know the base class of Player at that point. But trick 77, which I had already used in a variation for the Inherits declarations, came in handy: I'd define a declarative macro during the #[derive(GodotClass)], which would help resolve Player to its base name Area2D. So the proc-macro could eventually replace GodotExt with the true trait.

This works as demonstrated by this PR. But there are a few trade-offs to keep in mind:

  1. It's now not necessary to repeat the base class' name anywhere, just mention it once in #[class].
  2. The declarative macros add a bit of implementation complexity (and possibly compile-time overhead?). It's also a bit brittle if someone declares impls in a different module, or chooses qualified identifiers like engine::NodeVirtual.
  3. It may be confusing to users that GodotExt is not a real trait, but a magic placeholder substituted by the proc-macro. I added a doc-alias so one would find GodotExt in the docs (under #[godot_api]), but still.
  4. There are now two ways to implement virtuals: GodotExt and *Virtual. While identical in semantics, it's not clear to the user which one they should choose or if there's a difference. I'm also not sure if the repetition is a problem in the first place, or rather makes code clearer by expressing which functionality is inherited.

Thoughts? This is a draft, and we may end up not merging it.

@Bromeon Bromeon added quality-of-life No new functionality, but improves ergonomics/internals c: register Register classes, functions and other symbols to GDScript labels Mar 19, 2023
@Bromeon Bromeon force-pushed the feature/godot-ext-syntax branch from be2cb28 to fdb5ff9 Compare March 19, 2023 22:56
@bluenote10
Copy link
Contributor

Personally I find the FooVirtual quite nice and clean, because it reminds me what interface exactly I'm implementing. I can even <CTRL>+<click> on the trait and see which functions it has, which I assume would not work with GodotExt?

I use Base<Self::Base> in the (2) position, so I only need to make two code changes if I want to change the type, which is perfectly fine to me, and in both the (1) and (3) position the type name conveys valuable information. It is also not such a super frequent change to make anyway. Isn't that already simpler compared to GDScript, where changing the node type requires also two steps, but with more mouse clicks in between (changing the node type in the script, but also changing the node type in the scene tree)?

@Bromeon
Copy link
Member Author

Bromeon commented Apr 13, 2023

There doesn't seem to be interest in preserving this syntax, so I'll close this.
Maybe some of the code/tricks used can be useful elsewhere.

@Bromeon Bromeon closed this Apr 13, 2023
@Bromeon Bromeon deleted the feature/godot-ext-syntax branch June 19, 2023 19:31
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 quality-of-life No new functionality, but improves ergonomics/internals
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants