godot-rust / gdnative

Rust bindings for Godot 3
https://godot-rust.github.io
MIT License
3.63k stars 211 forks source link

Export enum property with custom int discriminants #764

Open clarfonthey opened 3 years ago

clarfonthey commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 https://github.com/godot-rust/godot-rust/issues/544.

Concerns:

Bogay commented 3 years ago

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?