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 calls panic with not implemented if they are conditionally excluded from compilation #399

Closed
jrockett6 opened this issue Sep 5, 2023 · 2 comments · Fixed by #444
Labels
bug c: register Register classes, functions and other symbols to GDScript

Comments

@jrockett6
Copy link
Contributor

jrockett6 commented Sep 5, 2023

E.g. this will panic with 'not implemented' if the "client" feature is not included.

#[godot_api]
impl CharacterBody3DVirtual for MyCharacter {
    #[cfg(feature = "client")]
    fn ready(&mut self) {
             // Do stuff
    }
}

This is not a huge deal if there isn't any overhead from including the attribute inside the function body - but I'm not familiar with this, e.g.

#[godot_api]
impl CharacterBody3DVirtual for MyCharacter {
    fn process(&mut self, delta: f64) {
             #[cfg(feature = "client")]
             {
                  // Do our client only stuff
             }

             // Is there overhead here from calling into an empty process block for potentially every character that is not part of client?
    }
}
@Bromeon
Copy link
Member

Bromeon commented Sep 5, 2023

This looks a bit like a special-case of #379 🤔

@PgBiel
Copy link
Contributor

PgBiel commented Sep 28, 2023

If this helps, I noticed that cfg will also cause problems in other places, e.g.

#[godot_api]
impl Main {
    #[cfg(feature = "no")]
    #[func]
    fn game_over(&mut self) {

results in the error (seemingly from the codegen result):
image

...but changing the order of the attribute macros seems to make it work fine (the function isn't even picked up by #[func]):

#[godot_api]
impl Main {
    #[func]
    #[cfg(feature = "no")]
    fn game_over(&mut self) {

Edit: The rest of this reply (below) has been superseded by #379 (comment)


Original contents (outdated):

Perhaps a possible solution could be special-cased for cfg, such that it would be applied to the generated FFI code block as well? For other attribute macros, one could, with some degree of certainty, assume that they won't modify the function's name or make it disappear, as is somewhat done today; however, there should certainly be some sort of "escape hatch" to ensure that a given attribute macro would be applied to both the original function impl and its generated FFI block, e.g. through something like (note: prone to bikeshedding etc.)

#[godot_api]
impl Main {
    #[func(
      cond_attrs = (cfg(feature = "no"), something(else), ...)
    )] // #[cfg(...)] could definitely be special-cased and supported though
    fn game_over(&mut self) {

(The above idea could be excessive for non-trait impls as in the example, however, as using #[func] means we can just tell users to always place it on the top of other attrs and it will likely just work.)

I'm not sure, however, how we would extend this thought to the functions in Virtual traits, which don't have #[func]; we could end up needing a separate top-level attribute for them, e.g. (for the example in the OP)

#[godot_api]
impl CharacterBody3DVirtual for MyCharacter {
    #[cond_attrs(cfg(feature = "client"))]
    fn ready(&mut self) {
             // Do stuff
    }
}

but I also think that #[cfg(...)] in particular, due to how common it is, should also just work out of the box here (should be viable to just check the attribute's name and, if equal to cfg, then duplicate it on top of the generated FFI block).

As a matter of fact, I believe that an initial fix for this problem could just consider cfg, and then the cond_attrs proposal could be left for further discussion. But this needs further thoughts/opinions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug c: register Register classes, functions and other symbols to GDScript
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants