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

Add String-based Enum support to GDScript #20988

Closed
willnationsdev opened this issue Aug 14, 2018 · 15 comments
Closed

Add String-based Enum support to GDScript #20988

willnationsdev opened this issue Aug 14, 2018 · 15 comments

Comments

@willnationsdev
Copy link
Contributor

willnationsdev commented Aug 14, 2018

Godot version:
Godot 3.1

Issue description:

  1. GDScript enums are not allowed to be string values.
  2. You can have exported string values with the PROPERTY_HINT_ENUM flag that are string values.

ergo, if strings are types that support enum representation in the editor, then GDScript should logically support me creating enums of that type.

Steps to reproduce:

enum Types {
    # This line currently creates an error because enums must be integers
    TYPE_FIRE = "Fire",
    TYPE_ICE = "Ice",
}
var type
func _get_property_list():
    return [
        {
            "name": "type",
            "type": TYPE_STRING,
            "hint": PROPERTY_HINT_ENUM,
            # Rather than re-iterating the list or creating an explicit mapping const dictionary,
            # I should be able to just directly use the values array of a string enum
            "hint_string": PoolStringArray(Types.values()).join(",")
        }
    ]

What do you guys think?

@nobuyukinyuu
Copy link
Contributor

Yes. Right now I use dictionaries for this.

@hilfazer
Copy link
Contributor

Aren't dictionaries good enough?

@nobuyukinyuu
Copy link
Contributor

@hilfazer they're probably less efficient at lookups, and don't have IDE autocomplete support (one main advantage of enums for me, anyway). I'm unsure if a dictionary can be const, either, but I've never tried. The syntax is different than typical enum syntax, both in initialization and in lookup.

@willnationsdev
Copy link
Contributor Author

willnationsdev commented Aug 14, 2018

@nobuyukinyuu @hilfazer Dictionaries can be const. The bigger problem is the fact that if what you want all along is simply strings and that's it (like in my use case, I just want to display categories in the editor and have no need for the integers since the strings get passed to a python script via OS.execute), then you end up having to use a separate data structure altogether (the enum) just to find the string you want, which leaves open the opportunity for errors in one's code via unintended index offsets or something. There's just no point to store one thing in two disjoint data structures if the one data structure is merely a means to an end. Especially when we can easily add string support with only a few lines of code, as evidenced in the PR I submitted.

@NathanLovato
Copy link
Contributor

NathanLovato commented Aug 15, 2018

I also often wished I could have string enums and benefit from autocompletion + easy drop-down in the Inspector. For common things like a stateful object that lets the user pick a starting state, or giving a command option to some UI widget... I currently use export(String, "option_1", "option_2", ...) or other replacements but it would be a lot more convenient with enums
Would definitely love to see this happen!

@bojidar-bg
Copy link
Contributor

IMHO, I would prefer that enums remain with integer values only. They are only sugar for constant dictionaries, and adding more features to them would just make for complicated sugar.

That being said, if string support is implemented, there should probably be support for other types as well (Floats, Vectors, Arrays, etc.). At which point, it just becomes a glorified constant dictionary syntax, which doesn't use the const keyword and increments numbers automatically.

The code example from the OP can be rewritten as:

const Types = {
    TYPE_FIRE = "Fire",
    TYPE_ICE = "Ice",
}
# And, for quick access:
const TYPE_FIRE = Types.TYPE_FIRE
const TYPE_ICE = Types.TYPE_ICE

But it should be more performant by just doing:

enum Types {Fire, Ice}

@ghost
Copy link

ghost commented Aug 22, 2018

@hilfazer they're probably less efficient at lookups

please show an example of how getting a value from an enum like dictionary is detrimental towards the game and is causing a bottleneck.

@willnationsdev
Copy link
Contributor Author

willnationsdev commented Aug 22, 2018

@nobuyukinyuu @girng There's no difference in lookup time. You're either doing a property lookup when accessing the const value directly, or you're doing a const property lookup followed by a Dictionary lookup when going through the Dictionary/enum. The concern in the issue is merely to improve the usability so that string values can easily be associated with enum values (for printing purposes, passing to external code, whatever).

@bojidar-bg

They are only sugar for constant dictionaries, and adding more features to them would just make for complicated sugar.

Except that allowing users to use strings doesn't "add more features" to them for more "complicated sugar" so much as it just takes away an arbitrary restriction on the permitted values. There's nothing stopping them from being strings except a single if statement that forces only INT variants through. All of the other logic in the related PR is just about circumventing the assumption that all values will be integers.

Also, can't really do enum Types {Fire, Ice} for strings unless you want to replace the assumption that such an enum would be { Fire=0, Ice=1 }, which I don't want to change.

@willnationsdev
Copy link
Contributor Author

willnationsdev commented Sep 17, 2018

So, upon reconsidering this, I feel like there's a mis-solution I'm applying. I'm wanting to not have to generate the strings I want for an exported enum, but in MOST cases (although not my original case for creating this Issue), that's the only basis I would have for wanting string support. Implementation-wise, it's still ideal for the enum itself to use integer values and not support anything else. Integer comparisons are faster and take up less memory. Given this, I have another proposition:

When annotations for GDScript are implemented (#20318), we could add an annotation for an enum that allows the user to specify an enum prefix (e.g. "TYPE_"). Then, when that enum is exported, the script would know to generate a list of strings by grabbing all of the string keys for the enum, stripping away the prefix, and converting it to title case. So...

extends Node
@enum_prefix TYPE_
enum Types {
    TYPE_NONE,
    TYPE_FIRE,
    TYPE_ICE
}
export(Types) var type = TYPE_NONE

# equivalent to

tool
extends Node
enum Types {
    TYPE_NONE,
    TYPE_FIRE,
    TYPE_ICE
}
var type = 0
func _set(p_name, p_value):
    match p_name:
        "type": type = p_value
func _get(p_name):
    match p_name:
        "type": return type
func _get_property_list():
    return [
        {
            "name": "type",
            "type": TYPE_INT,
            "hint": PROPERTY_HINT_ENUM,
            "hint_string": "None,Fire,Ice"
        {
    ]

I think this would be a much cleaner solution.

@aaronfranke
Copy link
Member

aaronfranke commented Jan 22, 2019

Are there any other languages that do this? I'm not against this feature, just curious.

@samgreen
Copy link
Contributor

samgreen commented Feb 11, 2019

Swift has beautiful enum system.

It lets you do whacky and useful things like this:

enum ASCIIControlCharacter: Character {
    case tab = "\t"
    case lineFeed = "\n"
    case carriageReturn = "\r"
}

They get around using pretty much any values by providing a rawValue initializer and accessor to let you go back and forth without writing (or seeing) boilerplate.

@OvermindDL1
Copy link

It lets you do whacky and useful things like this:

That looks more like namespaced variables though.

If anyone is going to put a proper 'enum' in the language then it should be a full (G)ADT type so it is actually more useful than just a basic 'dictionary'. I could imagine something like this:

enum PhysicalAttack_Types:
    Blunt
    Sharp
    Etc

enum MagicAttack_Types:
    Fire
    Nature
    Etc

enum Attack_Types:
    Unknown
    Physical : PhysicalAttack_Types
    Magic : MagicAttack_Types

Used like (pretend that open ... is like importing the scope of the stated thing into the current scope, can be import or whatever):

open PhysicalAttack_Types
open Attack_Types
attack = Physical(Sharp)

An ADT is just an Enum that you can attack extra data to if a specific head/case so wishes. The type of all related heads can be defined at the call site as well. A GADT is just an ADT that can define the type of the 'enum'/ADT itself at the call site, this is important it the typed section of gdscript, less useful with the untyped. The traditional example is just a simple calculator, like you might do this with just normal ADT-style Enum's:

enum Value:
    Bool : bool
    Int : int

enum Expr:
    Value : Value
    If : (Expr, Expr, Expr)
    Eq : (Expr, Expr)
    Lt : (Expr, Expr)

So it's a value and an expression type, a simple interpreter type in other words, an AST. We can define such an ast like:

open Value
open Expr
ast = If(LT(Value(Int(2)), Value(Int(4))), Value(Int(42)), Value(Int(0)))

This is basically just if 2<4 then 42 else 0 is pseudocode.
Making an interpreter for this would be like this, this is what you'd have to do in gdscript 'now' whether via pretending a tuple/dict is an ADT or in the untyped world or so (this might be more python'y than gdscript'y, but close enough):

def eval(expr): # `Expr -> Value` is the type of this function
    match expr: # Just matching on the enum/adt head
        case Value(v): v
        case If(b, l, r):
            match eval(b):
                case Bool(true): eval(l)
                case Bool(false): eval(r)
                default: raise("Invalid AST")
        case Lt(x, y):
            match (eval(x), eval(y)):
                case (Int(x), Int(y)): Bool(x<y)
                default: raise("Invalid AST")
        case Eq(a, b):
            match (eval(a), eval(b)):
                case (Int(x), Int(y)): Bool(x==y)
                case (Bool(x), Bool(y)): Bool(x==y)
                default: raise("Invalid AST")

Notice how wordy it is, how verbose it is, how I'm doing something really stupid by encoding the validity of the AST into the runtime representation instead of making it impossible to construct an invalid AST to begin with. This is what GADT's are for, for making impossible states unrepresentable. An example of running this would be like:

eval(ast)
# Returns Int(42)

And trying to 'eval' an invalid 'expr:

bad_ast = Eq(Value(Int(42)), Value(Bool(false))
eval(bad_ast)
# Raises an exception of "Invalid AST"

Aaaand that is not the kind of error you want to see at runtime. You want to catch such errors when the AST is made, whether that is at compile-time or via some user input at runtime both, not just one or the other. Well let's encode the above Enum/ADT as a GADT instead:

enum Value : t:
    Bool : bool -> Value(bool)
    Int : int -> Value(int

enum Expr : t:
    Value : Value(t) -> Expr(t)
    If : (Expr(bool), Expr(t), Expr(t)) -> Expr(t)
    Eq : (Expr(t), Expr(t)) -> Expr(bool)
    Lt : (Expr(int), Expr(int)) -> Expr(bool)

Using an OCaml'y syntax here for the types, but in essence it types each head individual so they can only accept either a certain value type or anything and thus it types the overall Expr for that specific head in a certain way, 'constraining' it, and trying to use it in a way that doesn't match the constraint will fail to compile. Now the eval function is so much more simple!

def eval(expr): # `Expr(t) -> t` is the type of this function
    match expr: # Just matching on the enum/adt head
        case Value(Bool(b)): b
        case Value(Int(i)): i
        case If(b, l, r):
            if eval(b):
                eval(l)
            else:
                eval(r):
        case Lt(a, b): eval(a) < eval(b)
        case Eq(a, b): eval(a) == eval(b)

Notice how it directly maps and all to constructs as you'd expect it to be. It can be used like:

eval(ast)
# Returns 42, no wrapping needed in a value or anything

And it can return another type, it knows what to return because of the constrains of the ast type passed in:

astb = If(Eq(Value(Int(2)), Value(Int(2))), Value(Bool(true)), Value(Bool(false)))
eval(astb)
# Returns true, all type safe

And trying to mis-use it won't even compile:

bad_ast = Eq(Value(Int(42)), Value(Bool(false))
# Won't even compile in typed mode as bad_ast is improperly formed, won't even reach eval
eval(bad_ast)

I'd expect an error message like:

bad_ast = Eq(Value(Int(42)), Value(Bool(false))
                   ^^^^^^^
Error: This expression has type Value(int)
       But an expression was expected of type Value(bool)
       Type int is not compatible with type bool

As is common in other languages with GADT's (this one is OCaml'y, but I use OCaml a lot so I know what to expect from it). You wouldn't even be able to even craft runtime code that parsed a user input to be able to generate an invalid AST as lacking the enforcement checks makes the type more generic than what the constraint allows for, so you'd be forced to check that the user input is valid before even generating the part of the AST structure that you are currently trying to craft.

TL;DR: ADT's are a more generic and useful Enum, should do them and they work fantastically and fine in the untyped world as well. If you want to lean more on the typed world then should go with GADT's as they can add a whole useful hoard of improvements that allow for significantly smaller code and compile-time instead of runtime checks.

@Reneator
Copy link

Reneator commented Apr 9, 2019

Im using this a lot in java and think its a good comfort-method.
It makes debugging easier when you see a String-value thats the same name as the enum-value instead of just a number

@akien-mga akien-mga added this to the 3.2 milestone Sep 2, 2019
@akien-mga
Copy link
Member

Closing as the proposal #21014 was rejected.

There are however still valid concerns/use cases raised in this issue, which should be discussed further in a new issue as mentioned on #21014 (comment) (will link it once it's open).

@akien-mga
Copy link
Member

Follow-up issue: #31896

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment