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

No easy way to #[export] custom types #227

Closed
gg-yb opened this issue Apr 12, 2023 · 10 comments · Fixed by #309
Closed

No easy way to #[export] custom types #227

gg-yb opened this issue Apr 12, 2023 · 10 comments · Fixed by #309
Labels
c: register Register classes, functions and other symbols to GDScript feature Adds functionality to the library

Comments

@gg-yb
Copy link
Contributor

gg-yb commented Apr 12, 2023

I do not know if this is a bug, but at the very least it is very unintuitive.

This works:

#[derive(Copy, Clone)]
#[repr(i32)]
pub enum Alignment {
    Left = 0,
    Center = 1,
    Right = 2,
}

impl VariantMetadata for Alignment {
    fn variant_type() -> VariantType {
        VariantType::Int
    }

    fn param_metadata() -> GDExtensionClassMethodArgumentMetadata {
        GDEXTENSION_METHOD_ARGUMENT_METADATA_INT_IS_INT32
    }
}

#[derive(GodotClass)]
#[class(base=Control)]
pub struct Foo {
    #[export]
    bar: Array<Alignment>,
}

However, this does not (compiler error attached):

#[derive(Copy, Clone)]
#[repr(i32)]
pub enum Alignment {
    Left = 0,
    Center = 1,
    Right = 2,
}

impl VariantMetadata for Alignment {
    fn variant_type() -> VariantType {
        VariantType::Int
    }

    fn param_metadata() -> GDExtensionClassMethodArgumentMetadata {
        GDEXTENSION_METHOD_ARGUMENT_METADATA_INT_IS_INT32
    }
}

#[derive(GodotClass)]
#[class(base=Control)]
pub struct Foo {
    #[export]
    bar: Alignment,
}

The only difference is that the former uses Array<Alignment>, while the latter uses raw Alignment.

The error:
out.txt

@Bromeon
Copy link
Member

Bromeon commented Apr 12, 2023

VariantMetadata is very low-level, it also uses sys types directly (from crate godot-ffi). Why that API Is public, let alone in the prelude, I don't know. Possibly needed for inner workings of some other stuff.

As the error message says, the trait Export must be implemented additionally. That trait is already implemented for Array, but not for custom types.

TLDR: we should design a proper API for #[export] of user-defined types.
I would thus classify it as a missing feature.

@Bromeon Bromeon added feature Adds functionality to the library c: register Register classes, functions and other symbols to GDScript labels Apr 12, 2023
@Bromeon Bromeon changed the title Different Requirements on Plain Exports vs. Exports in Arrays No easy way to #[export] custom types Apr 12, 2023
@gg-yb
Copy link
Contributor Author

gg-yb commented Apr 13, 2023

Thank you for your response. If I understand it correctly, it currently is possible to export custom types, but only by implementing low-level / internal traits? I.e. implementing Export is not enough

@Bromeon
Copy link
Member

Bromeon commented Apr 13, 2023

If I understand it correctly, it currently is possible to export custom types, but only by implementing low-level / internal traits?

The thing is, we haven't designed for this feature yet. It may work or not, there's no documentation, stuff may suddenly break and it will be hard to find someone that can support in this case.

If you can use other ways of exporting variables, e.g. through getters returning integers, I'd highly recommend to use those for the moment.

@gg-yb
Copy link
Contributor Author

gg-yb commented Apr 13, 2023

The fact that specifying custom getter/setter that take & return values for the property as a supported type (e.g. i32) works is already very helpful. Thank you!

Of course, things are subject to change, but I'm willing to work around that :)

@lilizoey
Copy link
Member

It is now possible to declare default export info for custom types (variant type, property hint, and property hint string). using the Export trait. This trait is likely to change somewhat in the future, but it is at least less low level than VariantMetadata et al. Though you still need to use custom setters/getters.

as an example:

pub enum GenLayer {
    Surface = 0,
    Underground = 1,
}

impl Export for GenLayer {
    fn export(&self) -> Self {
        *self
    }

    fn default_export_info() -> godot::export::ExportInfo {
        ExportInfo {
            variant_type: VariantType::Int,
            hint: PropertyHint::PROPERTY_HINT_ENUM,
            hint_string: "Surface,Underground".into(),
        }
    }
}

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

    #[export(get = get_kind, set = set_kind)]
    kind: GenLayer,
}

@gg-yb
Copy link
Contributor Author

gg-yb commented May 24, 2023

Small question for my understanding: this currently wraps and unwraps the value (e.g. GenLayer enum) in a variant, right?
Now my question is: Correctly implementing GodotFfi for GenLayer would result in optimizations where this wrapping/unwrapping could be skipped, right?

@lilizoey
Copy link
Member

lilizoey commented May 24, 2023

Small question for my understanding: this currently wraps and unwraps the value (e.g. GenLayer enum) in a variant, right? Now my question is: Correctly implementing GodotFfi for GenLayer would result in optimizations where this wrapping/unwrapping could be skipped, right?

well this uses a custom setter/getter that takes an i64 (since the type is VariantType::Int), i dont think implementing godotffi would really provide much benefit in terms of performance. Keep in mind that everything that crosses FFI needs to use one of the types godot accepts anyway. And enums are just integers in godot.

@gg-yb
Copy link
Contributor Author

gg-yb commented May 25, 2023

I see. So the example above would actually need the following code, too?

#[godot_api]
impl MapGenLayer {
    #[func]
    pub fn get_kind(&self) -> i64 { ... }
    #[func]
    pub fn set_kind(&mut self, v: i64) { ... }
}

@lilizoey
Copy link
Member

I see. So the example above would actually need the following code, too?

#[godot_api]
impl MapGenLayer {
    #[func]
    pub fn get_kind(&self) -> i64 { ... }
    #[func]
    pub fn set_kind(&mut self, v: i64) { ... }
}

yeah

@lilizoey
Copy link
Member

lilizoey commented Jun 21, 2023

#309 should fix this, with that PR you can now implement Property and optionally Export for any type, and that will allow you to use that type in #[var]/#[export] annotations. So your example could become

#[derive(Copy, Clone)]
#[repr(i32)]
pub enum Alignment {
    Left = 0,
    Center = 1,
    Right = 2,
}

impl Property for Alignment {
  type Intermediate = i32;

  fn get_property(&self) -> Self::Intermediate {
    self as i32
  }
  
  fn set_property(&mut self, value: Self::Intermediate) {
    match value {
      0 => *self = Self::Left,
      1 => *self = Self::Center,
      2 => *self = Self::Right,
      other => panic!("incorrect value {other}")
    }
  }
}

impl Export for Alignment {
  fn default_export_info() -> ExportInfo {
        ExportInfo {
            hint: PropertyHint::PROPERTY_HINT_ENUM,
            hint_string: "Left,Center,Right".into(),
        }
    }
}

#[derive(GodotClass)]
#[class(base=Control)]
pub struct Foo {
    #[var]
    unexported_bar: Alignment,
    #[export]
    bar: Alignment,
}

#[godot_api]
impl Foo {}

bors bot added a commit that referenced this issue Jun 28, 2023
309: Distinguish between exported and non-exported properties r=Bromeon a=lilizoey

Built on top of #311 

## Split `#[export]` into `#[var]` and `#[export]`
`var` is now for creating properties, and `export` for exporting to the editor. `export` implies `var`, meaning that the following two field declarations are equivalent:
```rs
#[var]
#[export]
field: SomeType

// and

#[export]
field: SomeType
```

`FieldVar` and `FieldExport` are now the types used to store the information about the `var` and `export` attributes.

## Add support for the various export annotation
things like ``@export_range`` and such

## Add a list parser to godot-macros

`ListParser` is used to parse values from the `KvParser` into ordered lists of items. 

We could've reused `KvParser`, but that would require for instance ``@export_range(0.0,` 10.0, or_greater)` to look something like:
`#[export(range = (min = 0.0, max = 10.0, or_greater))]`

Which to me doesn't seem ideal. I dont think we want people to look up the attribute names of the fields of an export annotation to figure out how to rewrite it in rust. With the list parser the above can instead just become:

`#[export(range = (0.0, 10.0, or_greater))]`

## Split up `Export` trait into `Property` and `Export`
`Property` has a setter and getter function for generating setters/getters. In addition to an associated type for the intermediate value used to pass objects to and from godot.

`Export` is now only for default export info, and functioning as a marker trait for exportable types.

## Add in functions to create `ExportInfo` structs based on each export annotation

Used as a typed api to ensure that the user passes in the right values and can in the future be used by the builder api too to ensure compatibility between the two APIs.

These are all placed in `godot-core::property::export_info_functions`.

## Add `ExportableObject` for exportable `GodotClass` objects.

An object can be exported if it inherits from either `Node` or `Resource`. Unfortunately this means there is no really nice way to create a bound for that using the existing traits. So i made `ExportableObject`, which is automatically implemented by all objects inheriting from one of those two classes.

fixes #227 

Co-authored-by: Lili Zoey <lili.andersen@nrk.no>
@bors bors bot closed this as completed in 7b2f106 Jun 28, 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