Enum property with custom int discriminants

[Bogay]

Fix #764

[Bromeon]

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]

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]

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]

@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)

[GodotRust]

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdnative/pr-775

[Bogay]

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

[bors[bot]]

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

[bors[bot]]

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

[bors[bot]]

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]

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]

Interesting. I think there are two options here:

1. 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).
2. 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]

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.

https://github.com/godot-rust/gdnative/blob/6dc972190f6cefb15eef6588c81a015983718df5/gdnative-core/src/export/property/hint.rs#L363-L367

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]

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]

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]

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.