Enum property with custom int discriminants #775

Bogay · 2021-08-30T08:54:35Z

Bromeon

Thank you for your contribution! I added a few comments.

Once your version is final, you can run cargo fmt to auto-format everything. 🧹

Bromeon · 2021-08-30T09:01:53Z

gdnative-core/src/nativescript/init/property/hint.rs

+        let keys: Vec<String> = self.values.keys().cloned().collect();
+        keys.join(",").into()


It looks like you're not using the values (i.e. associated numbers), are you?

Great how you reduced the code to be much more idiomatic 🙂

It looks like you're not using the values (i.e. associated numbers), are you?

I didn't aware of that before, so I read the code about export a property in gdnative, and found this line at godot-headers, it seems that I can't export information more than enum names? If so, maybe I should find a workaround to export enum name and its corresponding value?

https://github.com/godotengine/godot-headers/blob/4bae481fc305639a076a69485bed37679149bd32/nativescript/godot_nativescript.h#L56

edit: I don't know why I can't see the preview...code is here

typedef enum { GODOT_PROPERTY_HINT_NONE, ///< no hint provided. GODOT_PROPERTY_HINT_RANGE, ///< hint_text = "min,max,step,slider; //slider is optional" GODOT_PROPERTY_HINT_EXP_RANGE, ///< hint_text = "min,max,step", exponential edit GODOT_PROPERTY_HINT_ENUM, ///< hint_text= "val1,val2,val3,etc" <--- this line

I'll reply to the whole issue, as this affects how we continue.

Bromeon · 2021-08-30T09:03:46Z

gdnative-core/src/nativescript/init/property/hint.rs

+impl From<HashMap<String, i32>> for EnumHint {
+    #[inline]
+    fn from(values: HashMap<String, i32>) -> Self {
+        EnumHint { values }
+    }
 }


I would rather add a method in EnumHint, e.g. the suggested with_numbers() taking tuples. It allows to create EnumHint from a literal syntax and is consistent with the existing new() constructor -- there's no From implementation for Vec<String> either.

Construction could look as follows (this can even be added to documentation):

EnumHint::with_numbers(vec![ ("one".into(), 1), ("two".into(), 2), ("seven".into(), 7), ])

Bromeon · 2021-08-30T09:05:31Z

gdnative-core/src/nativescript/init/property/hint.rs

@@ -116,30 +119,31 @@ where
 /// ```
 #[derive(Clone, Eq, PartialEq, Debug, Default)]
 pub struct EnumHint {
-    values: Vec<String>,
+    values: HashMap<String, i32>,


My edit on the post came a bit late, maybe you missed it:

Unless we need properties of a map somewhere, I would keep things simple and stay with Vec.

It's probably better for such little-used things, as Vec is much cheaper (performance- and memory-wise).

Bromeon · 2021-08-30T11:02:35Z

Your discovery means that Godot doesn't use numbers in the hint at all. Which probably makes sense, since it uses enum names (strings) to abstract from these very numbers.

But it also means we cannot implement this PR as it is now -- there's no point having private numeric values inside EnumHint, when we don't use them anywhere.

We should probably take a step back and ask what problem we exactly want to solve. In the issue, we had this GDScript code:

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

but this does three things:

declares an enum Dir -- not a real type in GDScript 3, but syntax sugar for mapping named constants to numbers (if named, this is a dictionary)
exports a number (print dir to verify that)
associate a GODOT_PROPERTY_HINT_ENUM to this export, so the user has a nice drop-down in the editor

In other words, using the enum definition and export syntax, the previous code is equivalent to this:

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

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

By the way, you can verify this easily:

print("Selected value: ", dir)                     # 1
print("Enum: ", Dir)                               # {Bottom:1, Top:-1}
print("Is Dict: ", typeof(Dir) == TYPE_DICTIONARY) # true

In other words, the desired functionality is already today possible with godot-rust, by exporting an int and using a list of strings as enum hint.

What might add value however is if this could be done in a more convenient way -- maybe by allowing to use a Rust enum?

Bogay · 2021-08-31T05:55:23Z

Wow, that's more complicated than my excepted before. I need more time to learn the usage of hint and export to complete this.

Bromeon · 2021-08-31T08:13:07Z

Sure, don't feel forced to address this now, I think we both underestimated it 😉

Maybe I can copy the important findings to the actual issue. So maybe before doing more implementation work, let's figure out if there's something to be done at all, and if so, what.

Bogay · 2022-08-06T16:48:15Z

@Bromeon
I think this PR is ready to review. You can use my test project to quickly verify whether it works in Godot editor.
Just build the lib, and open the project to see the dir property of Move node inside Move.tscn.

I will rebase these changes into 2 ~ 3 commits after it's ready to merge.

BTW, derive ExportEnum can add editor support to the enum, but it can't be directly access from GDScript side (e.g. call Dir.Up in GDScript). Should I implement it in this PR? (If so, I might need to discuss about the design)

chitoyuu

Reminder that as of #964, there is now a more comprehensive solution for ToVariant and FromVariant of numeric enums, integrated into the ordinary derive macros, with #[variant(enum = "repr")]. Please see if you can fit your work into the existing framework, so that we don't have to maintain two separate code paths for the same thing, one being a subset of another.

chitoyuu · 2022-12-02T12:33:22Z

gdnative-core/src/export/property/hint.rs

+        self.values
+            .iter()
+            .map(|(key, val)| match val {
+                Some(val) => format!("{}:{}", key, val),
+                None => key.clone(),
+            })
+            .collect::<Vec<_>>()
+            .join(",")
+            .into()


The original code is iterative for a reason. While format!, collect and join are indeed convenient and pretty, they perform many allocations under the hood. It's often said that premature optimization is the root of all evil, but that does not mean we need to needlessly slow things down just because.

chitoyuu · 2022-12-02T12:35:39Z

gdnative-derive/src/export_enum.rs

+    use super::*;
+
+    #[test]
+    fn deny_non_unit_variant() -> syn::Result<()> {


Tests for proc-macros are better modeled as UI tests, as they not only allow us to check if the behavior is correct (with a real compiler, too), but also whether the error messages we show to users are up to par.

chitoyuu · 2022-12-02T12:45:18Z

gdnative-derive/src/lib.rs

+#[proc_macro_derive(ExportEnum)]
+pub fn derive_export_enum(input: TokenStream) -> TokenStream {


I'm not sure if this is the best way we can present this in the API. On one hand, naming the macro Export would be too general when it only really works on primitive enums at the present time. On the other hand, users are very unlikely to think of typing derive(ExportEnum) by themselves when the compiler is telling them that they're missing Export.

I don't have a concrete suggestion, but personally I'm leaning toward using Export as the name, and relying on error messages to point users to documentation when they try to use it on something else like a struct (in which case they probably want a custom resource instead).

Agree, I've rename it to Export.

currently the implementaion is dirty. I will refactor it after verifying it can work.

the repr part should be added to change the serialization behavior.

Bogay · 2023-10-11T15:09:08Z

Sorry for the very late reply. I got some time these days and fixed it. If it's OK to merge, I'll reorg it into a few commits.

Reminder that as of #964, there is now a more comprehensive solution for ToVariant and FromVariant of numeric enums, integrated into the ordinary derive macros, with #[variant(enum = "repr")]. Please see if you can fit your work into the existing framework, so that we don't have to maintain two separate code paths for the same thing, one being a subset of another.

The ToVariant / FromVariant derive is indeed what I need in the export impl. I switched to using them instead of implementing them in the export derive.

FYI, The demo project is also updated: https://github.com/Bogay/export-enum-demo

chitoyuu

Thanks for all the effort! The general design looks good to me, although there are still a number of minor concerns on the implementation details. See below.

I understand that it has been a long journey by now, so if you don't see yourself working any further on this PR, feel free to let me know if you'd like me to carry out any of the changes on your behalf.

bors try

gdnative-core/src/export/property/hint.rs

gdnative-derive/src/export.rs

test/src/test_export_enum.rs

bors · 2023-10-12T09:03:38Z

try

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

full-ci

chitoyuu

A few further things. Now all that's left should be the attribute trigger discussed in the open thread above.

bors try

gdnative-core/src/export/property/hint.rs

bors · 2023-10-14T03:06:02Z

try

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

full-ci

Bogay

This PR should be almost done. If the code is OK, I would update the docs.

gdnative-derive/src/export.rs

chitoyuu

Looks good to me now. The added UI tests are nice too! Please do whatever else you meant to do and squash the remaining commits. It should be good to merge after that.

Thanks for following through this entire time!

bors try

bors · 2023-10-17T01:00:52Z

try

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

full-ci

Bogay · 2023-10-17T09:04:17Z

Looks good to me now. The added UI tests are nice too! Please do whatever else you meant to do and squash the remaining commits. It should be good to merge after that.

Thanks for following through this entire time!

bors try

@chitoyuu Sorry, I found that if EnumHint is used for string hint, it shouldn't have explicit mapping, or the conversion might be broken. While that's a misuse, StringHint::Enum with mapping is a valid object in Rust. Perhaps we should implement two types to support these two scenarios? I haven't tested for StringHint::File and StringHint::GlobalFile, but my guess is that they should be similar to StringHint::Enum.

chitoyuu · 2023-10-17T14:08:07Z

Interesting. I think there are two options here:

Have ClassBuilder complain about it when a user tries to register a property with StringHint and explicit mappings. The function can strip (ignore) the values, ignore the whole property (leaving library somewhat usable but likely not behaving as expected), or just panic during init (rendering the resulting library completely unusable).
Revert EnumHint and introduce a new type (EnumEntryint?) for just the IntHint version, as a new variant.

I think option 1 with the soft warning would be the most friendly to users and developers, even though it doesn't adhere strictly to the correctness-by-construction principle. I think this is a case where the input is "correct enough" to be acceptable, and thus not worth the increased complexity and duplicated code paths to reject at compile time.

Which one of the options do you think is best, or maybe something else?

Bogay · 2023-10-18T07:41:59Z

Which one of the options do you think is best, or maybe something else?

Currently I prefer to warn the user when explicit mappings are given to StringHint, but I am not sure where the check should be inserted. The only option I see is StringHint::export_info when extracting the hint string.

gdnative/gdnative-core/src/export/property/hint.rs

Lines 363 to 367 in 6dc9721

    
           let hint_string = match self { 
        
               SH::Enum(e) | SH::File(e) | SH::GlobalFile(e) => e.to_godot_hint_string(), 
        
               SH::Placeholder { placeholder } => placeholder.into(), 
        
               _ => GodotString::new(), 
        
           };

Maybe something like this? But that looks like a hack...

        let hint_string = match self {
            SH::Enum(e) | SH::File(e) | SH::GlobalFile(e) => {
                if e.has_value() {
                    godot_warn!("StringHint should not be used with EnumHint with mappings");
                }
                e.to_godot_hint_string()
            }
            SH::Placeholder { placeholder } => placeholder.into(),
            _ => GodotString::new(),
        };

Other options (strip mappings, ignore property, or separate types) all seem to require some additional API changes and more complexity. If we allow breaking changes, I would prefer to introduce a separate type for enum hint with mapping. However, I think it is better to maintain backwards compatibility in this case.

chitoyuu · 2023-10-18T10:05:00Z

I suppose StringHint::export_info isn't the best place to emit a warning from, but it should be acceptable. Not pretty, but acceptable. You can frame it as sanitation during conversion to make the idea more palatable if you want, I guess. The function does take self by ownership so I think you should be able to implement value stripping without making a breaking change.

If we allow breaking changes, I would prefer to introduce a separate type for enum hint with mapping.

That shouldn't be a more breaking a change than what's already in tree since all the public enums now have #[non_exhaustive] attached to them:

enum IntHint {
    Enum(EnumHint), // old/current type without value
    EnumBikeshed(EnumBikeshedHint), // new type with EnumHintEntry
    // ...
}

enum StringHint {
    // Keep as is
}

However, I think it is better to maintain backwards compatibility in this case.

I agree.

Bogay · 2023-10-18T16:32:56Z

I suppose StringHint::export_info isn't the best place to emit a warning from, but it should be acceptable. Not pretty, but acceptable. You can frame it as sanitation during conversion to make the idea more palatable if you want, I guess.

Do you have a suggestion? I'm sorry that I don't know how to move this check to somewhere like PropertyBuilder::done (which might be more natural?), because it only gets the Hint object, which I can't access the EnumHint that may be in it.

The function does take self by ownership so I think you should be able to implement value stripping without making a breaking change.

Do you mean something like this?

        let hint_string = match self {
            SH::Enum(e) | SH::File(e) | SH::GlobalFile(e) => {
                if e.has_value() {
                    godot_warn!("StringHint should not be used with EnumHint with mappings");
                }
                // strip all values stored inside `EnumHint`
                e.strip();
                e.to_godot_hint_string()
            }
            SH::Placeholder { placeholder } => placeholder.into(),
            _ => GodotString::new(),
        };

I am not sure if it would be better to implement some general purpose APIs in this case. What do you think?

        let hint_string = match self {
            SH::Enum(e) | SH::File(e) | SH::GlobalFile(e) => {
                if e.iter().any(|ent| ent.value.is_some()) {
                    godot_warn!("StringHint should not be used with EnumHint with mappings");
                }

                // strip all values stored inside `EnumHint`
                for ent in e.iter_mut() {
                    ent.value = None;
                }

                e.to_godot_hint_string()
            }
            SH::Placeholder { placeholder } => placeholder.into(),
            _ => GodotString::new(),
        };

chitoyuu · 2023-10-19T01:26:54Z

Do you have a suggestion? I'm sorry that I don't know how to move this check to somewhere like PropertyBuilder::done (which might be more natural?), because it only gets the Hint object, which I can't access the EnumHint that may be in it.

I guess we can also add a sanitize(&mut self, errors: &mut SanitizeErrors) function to the Hint trait and give it an empty default implementation so it isn't a breaking change for external implementations. SanitizeErrors would be something the implementer can push std::error::Errors to, which ~~internally can simply be a String for now~~ can simply be a type that keeps track of the amount and emits the errors immediately. This would allow for a better separation between the two steps logically, if emitting an error message from export_hint somehow feels sacrosanct.

I am not sure if it would be better to implement some general purpose APIs in this case. What do you think?

I think the second example is better. I don't sense any use cases for the functions outside this one case. If they ever become necessary we can always extract the code into functions later.

Bromeon requested changes Aug 30, 2021

View reviewed changes

Bromeon added c: core Component: core (mod core_types, object, log, init, ...) feature Adds functionality to the library labels Aug 30, 2021

Bromeon linked an issue Aug 30, 2021 that may be closed by this pull request

Export enum property with custom int discriminants #764

Open

Bromeon mentioned this pull request Aug 31, 2021

Export enum property with custom int discriminants #764

Open

Bromeon added this to the v0.10.1 milestone Nov 1, 2021

Bromeon modified the milestones: v0.10.1, v0.10.2 Jul 16, 2022

Bogay force-pushed the custom-enum-int branch 2 times, most recently from 7b8deb9 to ef9fb61 Compare August 5, 2022 16:11

Bogay marked this pull request as ready for review August 6, 2022 15:49

Bogay changed the title ~~[WIP] Enum property with custom int discriminants~~ Enum property with custom int discriminants Aug 6, 2022

Bogay requested a review from Bromeon August 6, 2022 16:48

Bromeon modified the milestones: v0.10.2, v0.11.x Oct 1, 2022

Bromeon mentioned this pull request Oct 3, 2022

Enums godot-rust/gdext#7

Closed

Bromeon modified the milestones: v0.11.x, unplanned Oct 18, 2022

Bromeon added the status: postponed label Oct 18, 2022

Bogay marked this pull request as draft October 25, 2022 15:31

chitoyuu reviewed Dec 2, 2022

View reviewed changes

Bogay added 5 commits October 8, 2023 15:15

feat(EnumHint): store optional mapping value information

0f558c6

feat(derive): add ExportEnum

89cbefc

currently the implementaion is dirty. I will refactor it after verifying it can work.

test(EnumHint): test gto_godot_hint_string

d464e63

fix(ExportEnum): handle non-enum input

29458ae

test(ExportEnum): correctly deny invalid input

2754088

Bogay force-pushed the custom-enum-int branch from 70eaf52 to 7d7bdd7 Compare October 8, 2023 12:23

Bogay added 3 commits October 8, 2023 22:03

feat(Export): report compile error for all non-fieldless enum variants

a12fd4d

test(Export): add UI tests

c33e89b

docs(Export): fix example code

825ad68

the repr part should be added to change the serialization behavior.

Bogay marked this pull request as ready for review October 11, 2023 14:42

Bogay requested a review from chitoyuu October 11, 2023 15:14

chitoyuu reviewed Oct 12, 2023

View reviewed changes

bors bot added a commit that referenced this pull request Oct 12, 2023

Try #775:

c2f6666

chitoyuu reviewed Oct 14, 2023

View reviewed changes

gdnative-core/src/export/property/hint.rs Outdated Show resolved Hide resolved

gdnative-core/src/export/property/hint.rs Outdated Show resolved Hide resolved

bors bot added a commit that referenced this pull request Oct 14, 2023

Try #775:

006adca

Bogay force-pushed the custom-enum-int branch from 1f9c20c to f46c163 Compare October 14, 2023 17:44

Bogay added 2 commits October 15, 2023 01:45

feat(EnumHint): add EnumHintEntry to store entry info

98e0f4f

test: remove duplicated tests

e055ea2

Bogay force-pushed the custom-enum-int branch from f46c163 to e055ea2 Compare October 14, 2023 17:45

feat(Export): add #[export(...)] attribute

7159dd9

Bogay force-pushed the custom-enum-int branch from 76b7773 to 7159dd9 Compare October 16, 2023 16:20

test(ui): use glob to match tests for #[derive(Export)]

348bac2

Bogay commented Oct 16, 2023

View reviewed changes

gdnative-derive/src/export.rs Show resolved Hide resolved

chitoyuu approved these changes Oct 17, 2023

View reviewed changes

bors bot added a commit that referenced this pull request Oct 17, 2023

Try #775:

83eccfd

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enum property with custom int discriminants #775

Enum property with custom int discriminants #775

Bogay commented Aug 30, 2021 •

edited

Bromeon left a comment

Bromeon Aug 30, 2021

Bogay Aug 30, 2021 •

edited

Bromeon Aug 30, 2021

Bromeon Aug 30, 2021 •

edited

Bromeon Aug 30, 2021

Bromeon commented Aug 30, 2021

Bogay commented Aug 31, 2021

Bromeon commented Aug 31, 2021

Bogay commented Aug 6, 2022 •

edited

chitoyuu left a comment

chitoyuu Dec 2, 2022

Bogay Oct 11, 2023

chitoyuu Dec 2, 2022

Bogay Oct 11, 2023

chitoyuu Dec 2, 2022 •

edited

Bogay Oct 11, 2023

Bogay commented Oct 11, 2023 •

edited

chitoyuu left a comment

bors bot commented Oct 12, 2023

chitoyuu left a comment

bors bot commented Oct 14, 2023

Bogay left a comment

chitoyuu left a comment

bors bot commented Oct 17, 2023

Bogay commented Oct 17, 2023

chitoyuu commented Oct 17, 2023 •

edited

Bogay commented Oct 18, 2023

chitoyuu commented Oct 18, 2023 •

edited

Bogay commented Oct 18, 2023

chitoyuu commented Oct 19, 2023 •

edited

		let keys: Vec<String> = self.values.keys().cloned().collect();
		keys.join(",").into()

		#[proc_macro_derive(ExportEnum)]
		pub fn derive_export_enum(input: TokenStream) -> TokenStream {

Enum property with custom int discriminants #775

Are you sure you want to change the base?

Enum property with custom int discriminants #775

Conversation

Bogay commented Aug 30, 2021 • edited

Bromeon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bogay Aug 30, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bromeon Aug 30, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bromeon commented Aug 30, 2021

Bogay commented Aug 31, 2021

Bromeon commented Aug 31, 2021

Bogay commented Aug 6, 2022 • edited

chitoyuu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chitoyuu Dec 2, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bogay commented Oct 11, 2023 • edited

chitoyuu left a comment

Choose a reason for hiding this comment

bors bot commented Oct 12, 2023

try

chitoyuu left a comment

Choose a reason for hiding this comment

bors bot commented Oct 14, 2023

try

Bogay left a comment

Choose a reason for hiding this comment

chitoyuu left a comment

Choose a reason for hiding this comment

bors bot commented Oct 17, 2023

try

Bogay commented Oct 17, 2023

chitoyuu commented Oct 17, 2023 • edited

Bogay commented Oct 18, 2023

chitoyuu commented Oct 18, 2023 • edited

Bogay commented Oct 18, 2023

chitoyuu commented Oct 19, 2023 • edited

Bogay commented Aug 30, 2021 •

edited

Bogay Aug 30, 2021 •

edited

Bromeon Aug 30, 2021 •

edited

Bogay commented Aug 6, 2022 •

edited

chitoyuu Dec 2, 2022 •

edited

Bogay commented Oct 11, 2023 •

edited

chitoyuu commented Oct 17, 2023 •

edited

chitoyuu commented Oct 18, 2023 •

edited

chitoyuu commented Oct 19, 2023 •

edited