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

Export enum property with custom int discriminants #764

Open
Tracked by #767
clarfonthey opened this issue Jul 23, 2021 · 10 comments · May be fixed by #775
Open
Tracked by #767

Export enum property with custom int discriminants #764

clarfonthey opened this issue Jul 23, 2021 · 10 comments · May be fixed by #775
Labels
c: core Component: core (mod core_types, object, log, init, ...) feature Adds functionality to the library
Milestone

Comments

@clarfonthey
Copy link

clarfonthey commented Jul 23, 2021

Basically, this simple gdscript example can't quite be replicated:

enum Dir { Top = -1, Bottom = 1 }
export(Dir) var dir = Bottom

Since, when defining IntHint::Enum, you just provide an ordered list of strings, and there's no way to map strings to values. You can still get the result in code with custom setters, but I have a feeling that there's a more direct way to do this by changing the IntHint struct.

@Bogay
Copy link
Contributor

Bogay commented Aug 30, 2021

Hello, I want to help solving this issue.
Does using HashMap to replace Vec inside EnumHint can solve the problem?
I am not familiar to Rust so I am afraid that I miss something should be considered.

@Bromeon
Copy link
Member

Bromeon commented Aug 30, 2021

Here's the current implementation for context:

/// Hints that an integer, float or string property is an enumerated value to pick in a list.
///
///
/// # Examples
///
/// Basic usage:
///
/// ```rust
/// use gdnative_core::nativescript::init::property::hint::EnumHint;
///
/// let hint = EnumHint::new(vec!["Foo".into(), "Bar".into(), "Baz".into()]);
/// ```
#[derive(Clone, Eq, PartialEq, Debug, Default)]
pub struct EnumHint {
    values: Vec<String>,
}

impl EnumHint {
    #[inline]
    pub fn new(values: Vec<String>) -> Self {
        EnumHint { values }
    }
}

To answer your question: yes, HashMap would be one option, Vec<(String, i32)> another. Unless we need properties of a map somewhere, I would keep things simple and stay with Vec.

The internal representation is not that important, but maybe we could add a new method:

    /// Create a list of values to choose from, each with an associated numerical value.
    #[inline]
    pub fn with_numbers(values: Vec<(String, i32)>) -> Self {
        EnumHint { values }
    }

@Bromeon Bromeon added c: core Component: core (mod core_types, object, log, init, ...) feature Adds functionality to the library labels Aug 30, 2021
@Bogay Bogay linked a pull request Aug 30, 2021 that will close this issue
@Bromeon Bromeon added this to the v0.10 milestone Aug 30, 2021
@Bromeon Bromeon linked a pull request Aug 30, 2021 that will close this issue
@Bromeon
Copy link
Member

Bromeon commented Aug 31, 2021

@clarfonthey The code you posted:

enum Dir { Top = -1, Bottom = 1 }
export(Dir) var dir = Dir.Bottom

is only syntactic sugar for:

const Dir: Dictionary = {
    "Top": -1,
    "Bottom": 1,
}

export(int, "Top", "Bottom") var dir = Dir["Bottom"]

There are no real enums in GDScript. So what you suggest is possible today in godot-rust, however it needs a bit of manual work. See also discussion in this PR for details.

So we would need to first design what we concretely want to achieve. Is it exporting Rust enums?

@Bogay
Copy link
Contributor

Bogay commented Aug 31, 2021

I found that #546 had discussed very similar topic to this. If we can serialize/deserialize a Rust enum (at least as int). Maybe it will be easy to export that enum? But I am not sure whether the conversion can be done now.

@clarfonthey
Copy link
Author

So we would need to first design what we concretely want to achieve. Is it exporting Rust enums?

Yes, this is my primary goal -- I'm redesigning some code I wrote in GDScript in Rust, and as a result I'd like to be able to export an enum very similar to the one described. My main issue here is that while I want to internally reference these enum values using integers other than the standard indexed ones, I don't see an easy way to avoid duplicating this code when exporting -- one for the 0, 1 indexing and one for the -1, 1 indexing.

I was under the impression that GDScript preserved the values of the variants when exporting but I guess I was wrong

@Bromeon
Copy link
Member

Bromeon commented Sep 1, 2021

I found that #546 had discussed very similar topic to this. If we can serialize/deserialize a Rust enum (at least as int). Maybe it will be easy to export that enum? But I am not sure whether the conversion can be done now.

Yes, it would involve:

  1. export an integer (i.e. Variant::from_i64(value))
  2. create a string hint ("value0,value1,...")

So it would be mostly automating the two. You're right, having enum convertible to Variant might be a good start for this. It could also determine under which type it is exported (int or string).


My main issue here is that while I want to internally reference these enum values using integers other than the standard indexed ones

Does that mean you wouldn't want this in Rust:

#[derive(...)]
enum Dir { 
    Top = -1,
    Bottom = 1,
}

but rather something like:

#[derive(...)]
enum Dir {
    #[export_as = -1]
    Top,

    #[export_as = 1]
    Bottom,
}

As written above, we could maybe start with a ToVariant impl, which could maybe come in two presets (exporting as int/string) but also customized arbitrarily.


I was under the impression that GDScript preserved the values of the variants when exporting but I guess I was wrong

GDScript only exports an int. The hint is what makes it look like an enum in the editor (string keys and limited set of values), but in theory you can assign any integer to it.

@clarfonthey
Copy link
Author

Actually, funnily enough, I am using explicit discriminants in this case, but I think allowing for both explicit discriminants and an attribute would be best for extensibility.

@Bogay
Copy link
Contributor

Bogay commented Sep 6, 2021

I found that no matter the enum's mapping value, godot editor only serialize enum's value according to the hint string.
For example, if I declare a enum like this:

pub enum Num {
    THREE = 3,
    FOUR = 4,
}

and export a Resource that hold a Num::THREE in its field. After exporting I can see that field has 3, but after editing, it becomes 0 and 1. (mapping to THREE, FOUR)

Maybe we need to always serialize enum as string if editing it by godot editor is required? But deserialization may be broken if enum's member is renamed. If we actually need to serialize it as int, the only way I can think of now may be (de)serialize it according to its declaring order. (i.e. THREE = 0 and FOUR = 1 in .tres file)

@Bromeon Do you have other ideas about how to implement this? or any information I should provide?

@Bromeon
Copy link
Member

Bromeon commented Sep 6, 2021

We should probably look at the most minimal problem first, and take it from there.
Let's stay with this Rust enum:

pub enum Num {
    THREE = 3,
    FOUR = 4,
}

// in a NativeClass
#[property]
my_num: Num

A user would expect that the My Num property have a drop-down list with THREE and FOUR to select, which would correspond to the two variants. In GDScript, it looks like integers 3 and 4; in Rust it would be the type-safe enum.

This was also discussed in #544.

Concerns:

  • As noted in the other issue, #[property] alone cannot deduce the enum's names; this would need to be explicitly provided or done by a macro on the enum itself.
  • In GDScript we only export an int, so the int -> Rust mapping happens on Rust side. We would need to define what happens if an invalid value is assigned by GDScript. Maybe we need Option<Num> or so?
  • Looks like we need to add an extra dependency (strum) to get the enum's name. Not sure if this is directly usable though, or if a custom macro needs to be written.

@Bromeon Bromeon modified the milestones: v0.10, v0.10.1 Nov 1, 2021
@Bogay
Copy link
Contributor

Bogay commented Nov 16, 2021

Sorry for the late reply, I have tried to implement that, and I think there are some points need to discuss.

pub enum Num {
    THREE = 3,
    FOUR = 4,
}

// in a NativeClass
#[property]
my_num: Num

In above example, users actually can see the drop-down list, but in GDScript side, they would see 0 and 1 instead of 3 and 4 because the export hint cannot include the mapping info (e.g. THREE map to 3). I am not sure how godot can export that with GDscript enum.

Update: I found that the enum hint can accept hint string like "key0:val0,key1:val1", which might be undocumented. So add a method to EnumHint (maybe with_numbers) may help.

* In GDScript we only export an int, so the int -> Rust mapping happens on Rust side. We would need to define what happens if an invalid value is assigned by GDScript. Maybe we need `Option<Num>` or so?

Agree, I think Option is a good choice.

* Looks like we need to add an extra dependency (strum) to get the enum's name. Not sure if this is directly usable though, or if a custom macro needs to be written.

strum can be used to serialize string, but I am not sure should I handle the strum attribute? (it will change the value mapping of enum, e.g. make a enum can be deserialize from multiple choice of string) or just let strum to control the serialization/deserialization?

@Bromeon Bromeon modified the milestones: v0.10.1, v0.11 Jul 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: core Component: core (mod core_types, object, log, init, ...) feature Adds functionality to the library
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants