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

Multiple #[godot_api] impl blocks #925

Closed
0x53A opened this issue Oct 22, 2024 · 4 comments · Fixed by #927
Closed

Multiple #[godot_api] impl blocks #925

0x53A opened this issue Oct 22, 2024 · 4 comments · Fixed by #927
Labels
c: register Register classes, functions and other symbols to GDScript feature Adds functionality to the library

Comments

@0x53A
Copy link
Contributor

0x53A commented Oct 22, 2024

At the moment only one #[godot_api] impl block is allowed (and second one to implement the base class interface).

The error message with multiple blocks is

conflicting implementations of trait `ImplementsGodotApi` for type `knight::Knight`
conflicting implementation for `knight::Knight`

Of course, the workaround is trivial, just put everything into one block, so this isn't important, but it would be really nice to have.

For code organization, I have been experimenting with a pattern where I define and implement a relative fine-grained trait in a node then create a godot function to wrap that trait.

Example what I'd like to do:

// pub struct Knight derives from Area2D


// --
// Hittable 
// --

impl Hittable for Knight {
    fn on_hit( /* ... */) {
        // ...
    }
}

#[godot_api]
impl Knight  {
    #[func]
    pub fn get_hittable_trait(&mut self) -> Gd<HittableWrapper> {
        let wrapped = HittableWrapper { other: Box::new(self.to_gd()) };
        return Gd::from_object(wrapped);
    }
}

// --
// Healthy
// --
impl Healthy for Knight {
    fn get_health() -> u32 {
        return 42;
    }
}

#[godot_api]
impl Knight  {
    #[func]
    pub fn get_healthy_trait(&mut self) -> Gd<HealthyWrapper> {
        let wrapped = HealthyWrapper{ other: Box::new(self.to_gd()) };
        return Gd::from_object(wrapped);
    }
}

I don't yet know if this system of code organization makes sense, but that's beside the point.

My arguments why I would like to have multiple #[godot_api] blocks:

  1. I'd like to have the matching godot function right next to the trait implementation, instead of multiple pages below it, disjointed
  2. Later I'd like to generate some of this wrapping code by macro, which would also be much easier if it could be split into different blocks
  3. having multiple blocks would also allow to split the Node implementation into multiple files, if one were inclined to do so. (Personally I tend to prefer fewer larger files over many smaller ones, but taste differs)
@Bromeon
Copy link
Member

Bromeon commented Oct 22, 2024

#[godot_api] annotated impl blocks are very different from regular Rust impl blocks. They define several extra traits and register all contained symbols. You can check the generated code with cargo expand to get an idea 🙂

Supporting multiple impl blocks is probably quite hard to implement with the current trait-based approach, and has the potential to make global registration more complex. The benefit however is "only" cosmetic (code organization) -- while important, I'm not sure if it stands in a good balance with implementation and maintenance effort.

Are you interested in investigating the feasibility? We could of course give you some guidance in the code.

@Bromeon Bromeon added the feature Adds functionality to the library label Oct 22, 2024
@Bromeon Bromeon changed the title feature request: allow multiple >#[godot_api]< blocks Multiple #[godot_api] impl blocks Oct 22, 2024
@Bromeon Bromeon added the c: register Register classes, functions and other symbols to GDScript label Oct 22, 2024
@mivort
Copy link
Contributor

mivort commented Oct 26, 2024

The benefit however is "only" cosmetic (code organization) -- while important, I'm not sure if it stands in a good balance with implementation and maintenance effort.

I'd like to add one practical use case which comes to mind - I think it makes significantly easier to add stuff to multiple Godot classes with declarative macros, as it makes possible to keep the main #[godot_api] block, and then add macro_rules! which will be able to expand independently into #[godot_api(secondary)] blocks. The biggest advantage would be if there's no limit for the amount of #[godot_api(secondary) blocks and no requirement to enumerate them.

This can help to simplify project-specific declarative macros which add traits to Godot classes and automatically expose trait methods to the engine, and apply such macros selectively. This case might be covered by builder API in future, though.

@0x53A
Copy link
Contributor Author

0x53A commented Nov 28, 2024

The biggest advantage would be if there's no limit for the amount of #[godot_api(secondary) blocks and no requirement to enumerate them.

yep that's exactly how it works, only requirement ist for all of them to be in the same file, so you can't split it up over multiple files.

Since it has now been merged you can try it out by referencing gdext from github instead of crates.io.

@mivort
Copy link
Contributor

mivort commented Dec 2, 2024

@0x53A Thank you a lot for this change!
I've refactored the code to use inline macros defined for each trait similar to this:

macro_rules! expose_trait_foo {
  ($id:ident) => {
    #[godot_api(secondary)]
    impl $id {
      #[func]
      fn on_foo(base: Gd<Self>, arg: i32) {
        Self::foo(base, arg) # <- ensures 'foo()' has consistent signature
      }
    }
  }
}
expose_trait_foo!(SomeStruct);
expose_trait_foo!(OtherStruct);

It's really helpful to be able to apply consistent function signatures to different engine-exported types. It feels like a much nicer alternative to multi-inheritance, as it allows to mix and match different interfaces needed for a specific cases while keeping the entirely flat hierarchy, similar to mixins. With full dynamic capabilities of Godot's runtime reflection, but also with compile-time checked signatures.

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