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

Derive GodotClass for enums #334

Open
Tracked by #697
lilizoey opened this issue Jul 7, 2023 · 8 comments
Open
Tracked by #697

Derive GodotClass for enums #334

lilizoey opened this issue Jul 7, 2023 · 8 comments
Labels
c: register Register classes, functions and other symbols to GDScript feature Adds functionality to the library hard Opposite of "good first issue": needs deeper know-how and significant design work.

Comments

@lilizoey
Copy link
Member

lilizoey commented Jul 7, 2023

It should be possible to derive GodotClass for rust enums by having it create an abstract base class, and make each enum variant a subclass of that abstract class.

This would be very useful to model things like. You want to export a resource to the editor, and the user can pick between 3 different variants that export should be, which each have different exports.

One example of what this may look like:

#[derive(GodotClass)]
// Creature inherits from `Resource`
// Every variant has an automatically generated `init` function
#[class(init, base = Resource)]
pub enum Creature {
  Enemy {
    #[base]
    base: Base<Resource>,
    #[export]
    damage: f64,
  },
  Villager {
    #[export]
    happiness: f64,
  },
}

#[godot_api]
impl Creature {
  // function defined on the base class of `Creature`
  #[func]
  fn some_function(&self) {
    ..
  }
}

I've found myself wanting something like this several times before. There isn't really a good workaround for it, especially in the case where we want this to inherit from Resource. Since we'd want some way to specify that an exported field should be one of a limited set of types.

Some questions

What would happen if someone makes a new subclass of that abstract base class, is that possible? and if so how should we handle it?

Should we add some way to add exported fields/variants to the base class that will be shared among all variants?

Should there be a way to automatically add a base field to all variants, or should that be explicit?

In GDScript, we could specify that a function can only take/returns one of the variants of the enum, does this cause any issues?

@lilizoey lilizoey added feature Adds functionality to the library c: register Register classes, functions and other symbols to GDScript hard Opposite of "good first issue": needs deeper know-how and significant design work. labels Jul 7, 2023
@Bromeon
Copy link
Member

Bromeon commented Jul 8, 2023

This is quite a cool idea!

Apart from the things you mentioned, one problem I see is that the "sub-classes" are not proper types in Rust. You cannot have fn do_sth(enemy: &Creature::Enemy) or similar. I know this is a possibly planned feature in Rust, but that doesn't help us today.

People typically use this idiom to work around this:

pub enum Creature {
  Enemy(Enemy),
  ...
}

#[..??..]
struct Enemy {
    #[base]
    base: Base<Resource>,
    #[export]
    damage: f64,
}

@lilizoey
Copy link
Member Author

lilizoey commented Jul 8, 2023

I dont think these being different types should be an issue for us, but we could consider something like that yes. I'm not entirely sure about the syntax for it but i think it shouldn't be included in an initial implementation. Since it'd border on being actual inheritance, and i'm a bit unclear on whether we should include actual inheritance to any extent. and if we should then to what extent.

The initial idea is very controlled, but with this we could create hierarchies of base classes w/ inherited concrete classes.

This might be fine tho, since they're all abstract classes. And we can limit how much you can use inheritance-like features, such as overriding code and stuff like that. So it could end up being a useful way to group together related classes in the class-explorer too.

my first idea for the syntax is something like:

#[derive(GodotClass)]
#[class(init, base = Resource)]
pub enum Creature {
  #[transparent] // similar to for instance #[serde(transparent)]
  Enemy(Enemy),
  ..
}

#[derive(GodotClass)]
#[class(bikeshed_parent_path = Creature)]
struct Enemy {
  #[base]
  base: Base<Resource>,
  #[export]
  damage: f64,
}

@lilizoey
Copy link
Member Author

lilizoey commented Jul 9, 2023

Just did a bit of a hacky manual job of this, and it seems like it is doable. But there are some issues:

We have a single init method for each type, how do we ensure that the right variant gets generated when godot wants to create each subclass?

We can make the init function take an extra argument representing the variant (like a string maybe?). In that case, where do we get that value from? We usually get values from GodotClass, and we can't implement that for each variant.

What should happen if we do:

impl Creature {
  #[func]
  fn foo(&mut self) {
    *self = Enemy { .. };
  }
}

In general what should happen if the wrong data is contained in something godot believes to contain some specific type? Can we prevent this from happening somehow?

I think we kind of have to do something like what @Bromeon suggested, requiring a newtype for each variant. Otherwise we'll have issues like the above ones. And probably somehow prevent users from getting a mutable reference to the enum, while still allowing mutable references to each variant-newtype.

It is a bit unfortunate if we can't allow the initial syntax i proposed. Though i guess we could generate the types automatically, since we just dont allow you to use the normal struct syntax. So

enum Creature {
  Enemy {
    #[export]
    damage: f64
  },
}

// becomes

enum Creature {
  Enemy(Enemy),
}

struct Enemy {
  #[export]
  damage: f64,
}

We should probably only include one syntax in an initial implementation though.

@Bromeon
Copy link
Member

Bromeon commented Jul 9, 2023

Maybe taking a step back, what is the motivation behind enum? I guess code brevity and having exclusive variants representing the same type? But as you mentioned, those are not exclusive on GDScript side (someone else can inherit the base class).

Or what is the original motivation of not using

#[derive(GodotClass)]
pub struct Creature {
   ...
}

#[derive(GodotClass)]
#[class(base=Creature)]
pub struct Enemy {
    #[base]
    base: Base<Resource>,
    #[export]
    damage: f64,
};

#[derive(GodotClass)]
#[class(base=Creature)]
pub struct Villager {
    #[export]
    happiness: f64,
}

If it's declaration verbosity, it could still use enum syntax that the proc-macro would split apart.
And common methods could either be defined on Creature directly, or as polymorphic global functions:

fn do_sth<T>(creature: Gd<T>)
where T: Inherits<Creature> {
    ...
}

@lilizoey
Copy link
Member Author

lilizoey commented Jul 9, 2023

A big reason to avoid adding inheritance of custom classes is that rust doesn't support it. We've managed to make a system that works for godot's classes, but those classes are effectively static. And adding full-on inheritance like this would add so many new cases to consider (and we might turn #23 into straight up UB, whereas it is currently probably sound).

This idea could be a much more controlled kind of inheritance that plays better with rust's type system.

@Bromeon
Copy link
Member

Bromeon commented Jul 10, 2023

I think the main problem is Deref/DerefMut, which would then need to become explicit (upcast() or so).

But representing inheritance as enum also has some issues:

  • There can be other derived classes outside the explicitly listed ones
  • It's not possible to have shared base state, every enumerator needs to redefine it
  • The derived classes are not proper types

But it may still be useful for a subset of inheritance cases like the pattern you mentioned. Or the other way around: we have a Rust enum and want to map it to something strongly typed in GDScript. Approached from this angle, it might be a nice addition, if we find a way to handle above cases.

@awestlake87
Copy link

awestlake87 commented Aug 12, 2023

FWIW, I managed to make something that works in gdext today. Sounds like you guys are talking more about a custom derive to make it more ergonomic, but I figured I'd share this since I found this thread when I was just looking for a way to do it.

Basically, I'm messing around with making a Mujoco module, and I wanted to define collision shapes for Mujoco bodies in the inspector just like I can do for Godot's default CollisionShape3D. An enum would be perfect for this since there's only a few known variants you can use for geoms and they each have unique properties that define the size of the shape.

I managed to get something basic working without the enum by deriving a couple of custom resources and using downcasting to inspect the properties.

// Deriving GodotClass makes the class available to Godot
#[derive(GodotClass)]
#[class(base=Node3D)]
pub struct MjBody {
    #[base]
    base: Base<Node3D>,

    #[export]
    geom: Option<Gd<Resource>>,
}

#[godot_api]
impl MjBody {}

#[godot_api]
impl NodeVirtual for MjBody {
    fn init(base: Base<Node3D>) -> Self {
        MjBody { base, geom: None }
    }

    fn ready(&mut self) {
        godot_print!("MjBody ready");
        if let Some(geom) = self.geom.as_ref() {
            if let Some(s) = Gd::try_cast::<MjSphereGeom>(geom.share()) {
                godot_print!("MjBody with sphere {}", s.bind().radius);
            } else if let Some(b) = Gd::try_cast::<MjBoxGeom>(geom.share()) {
                godot_print!("MjBody with box {}", b.bind().size);
            }
        }
    }
}

#[derive(GodotClass)]
#[class(base=Resource)]
pub struct MjSphereGeom {
    #[base]
    base: Base<Resource>,

    #[export]
    radius: f64,
}

#[godot_api]
impl MjSphereGeom {}

#[godot_api]
impl NodeVirtual for MjSphereGeom {
    fn init(base: Base<Resource>) -> Self {
        MjSphereGeom { base, radius: 1.0 }
    }
}

#[derive(GodotClass)]
#[class(base=Resource)]
pub struct MjBoxGeom {
    #[base]
    base: Base<Resource>,

    #[export]
    size: Vector3,
}

#[godot_api]
impl MjBoxGeom {}

#[godot_api]
impl NodeVirtual for MjBoxGeom {
    fn init(base: Base<Resource>) -> Self {
        MjBoxGeom {
            base,
            size: Vector3::new(1.0, 1.0, 1.0),
        }
    }
}

With that, MjBodys have an exported 'geom' property that can be set to a resource:
image

And you can fill out properties for different geom types:
image
image

Then at runtime, use Gd::try_cast to downcast the geom shape:
image

The only thing I don't like about it is that I can't create a custom base class for these resources, so I have to pick from a very long list every time I want to define my geom. But that's more of a nitpick, I think it works well enough for what I want to do with it.
image

@astrale-sharp
Copy link
Contributor

Another way to approach the problem :

We've been thinking about mapping rust enums to godot by using an existing godot core functionality: inheritance.
But we have bindings power that can extend the engine, what if we create a more specialized abstraction for rust enums?
We could register a RustEnum Class that exposes methods that makes it easy to work with.
Then when deriving GodotClass for enums, the new generated data structure would inherit from RustEnum.
We could also (harder?) have the inspector use custom logic for RustEnums so has to be able to edit rust enums from godot.

Drawbacks

Well, if the exported class doesn't have subclasses for each of it's variant, we can't rely on the neat syntax if var is EnumVariantX and we just have helper functions instead.

From gdscript we'd still have something like this possibly which is not awful

pub enum OptionFloat{
    Some(f32),
    None
}
match value.enum_variant:
    value.VARIANTS.SOME:
        [var id]  = value.get_variant_some() // I'm not sure this syntax works outside a match stmt and I really don't like it either way
    value.VARIANTS.NONE:
        assert(null == value.get_variant_none()

Of course the problem with this is what happens if get_variant_some() gets called when value.variant != value.VARIANTS.SOME, I think an error would be good.

Why map rust enums to godot in the first place?

Bindings are useful for bringing rust power/expressiveness to godot, rust enums are one of the reasons I fell in love with the language, they allow to think about problems in a typed manner which is very neat.

Alternatives

If godot-egui is ported to godot4, we could instead create another tool, depending on gdext using https://github.com/matthewjberger/enum2egui (proc macros to generate egui ui for rust structs and enums) and integrate that with a tool script or editor plugin, it would be the responsibility of the user to initiate it (probably could be an addon).

This would be more cumbersome and also, separate from godot gui but I would try to make it happen anyway cause I really want to game design by editing rust enums.

This would also loose the advantage of autocomplete and help with enum from godot.

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 hard Opposite of "good first issue": needs deeper know-how and significant design work.
Projects
None yet
Development

No branches or pull requests

4 participants