Automatic custom enum parsing doesn't work with "Save as String"

[differenceclouds]

I'm working from the "dev" branch.
To reproduce:

• Make a custom enum in the Custom Types Editor, add some values, and set it to Save as String.
• Add an object like a point, and add this enum as a custom property, and set the value.
• If editing an existing object, when changing from "Save as String" to "Save as Int" or vice-versa, you have to make a change in the map (like moving the object a couple pixels or something) and then save the map for the storage type to take effect.
• in C#, import a map with the default loader and no arguments.
• I get the error "System.FormatException: 'The input string 'North' was not in a correct format.'"

So, for thoroughness, this is what is in the .tiled-project:

    "propertyTypes": [
        {
            "id": 5,
            "name": "CardinalDirection",
            "storageType": "string",
            "type": "enum",
            "values": [
                "North",
                "East",
                "South",
                "West"
            ],
            "valuesAsFlags": false
        },

This is what is in the .tmx:

  <object id="6" name="Tim" x="630.695" y="180.202">
   <properties>
    <property name="cardinal" propertytype="CardinalDirection" value="North"/>
   </properties>
   <point/>
  </object>

[dcronqvist]

Great detail here, nice! Yeah, I'm aware of this limitation regarding enums. Is there any way to have enum properties without having them being defined as custom types? Because if not, then it feels more natural to have a hard requirement in DotTiled that those custom enum types are also defined properly with CustomEnumDefinition.

However, if there's a way to have enum properties without having them defined (like how you can input arbitrary strings in class) as a custom type, then we're going to have to find a way to deal with them without them needing to be defined in DotTiled.

What do you think?

[differenceclouds]

It's a great feature, and the enums are much easier than classes to implement, so it's definitely manageable as a requirement. My personal preference would be to have strings as a backup option however, because why not? The string is right there. From my perspective in this case, I get a crash on load with one storage type and not the other, so it feels more like a bug than a limitation. So I'm curious what's going on data-wise with the "Save As Integer" option selected. Is that integer available? If so, why make the string unavailable?

I think part of what i was getting at with my original issue about classes has to do with the process of iteratively coding a game. You make some maps, put some data into the maps, and then you might want to launch the game and see some fraction of that working before you implement all of the data you have in the map file. It's iterative, and not being able to load and view a map before a ton of stuff is perfectly in place can be frustrating. This is especially true if you're learning to use all these tools simultaneously. Not to rag on the custom classes again, but in that case that would have been a total deal-breaker for me using this library a couple years ago.

[dcronqvist]

You make a good point. I agree that it should be as frictionless as possible to use DotTiled in an iterative development process. Let's go over the 4 combinations of enums that you can have in the .tmx format.

1. String storage, no flags

<property name="customenumstringprop" propertytype="CustomEnumString" value="CustomEnumString_2"/>

In this case, there's no problem with providing just the string value with a StringProperty.

2. String storage, flags

<property name="customenumstringflagsprop" propertytype="CustomEnumStringFlags" value="CustomEnumStringFlags_1,CustomEnumStringFlags_2"/>

Can provide this string too as a StringProperty, but it might be confusing to some extent. But that would allow developers to do their own e.g. .Contains("CustomEnumStringFlags_1") to check if a flag is set.

3. Integer storage, no flags

<property name="customenumintprop" type="int" propertytype="CustomEnumInt" value="3"/>

Can provide as IntProperty. If users want to know which string value "3" corresponds to in Tiled, there is no other solution than also providing a custom enum definition.

4. Integer storage, flags

<property name="customenumintflagsprop" type="int" propertytype="CustomEnumIntFlags" value="6"/>

We can provide this as an IntProperty, and users would have to do their own bitwise & to check if a flag is set.

Essentially, this behaviour would be to use whatever type is set to by default, and only ever map a property like the ones above to an EnumProperty if there is a custom enum definition for that propertytype. The same behaviour should be possible to implement for the .tmj format.

The only problem that I can see is that we will be providing different types of properties (IntProperty and StringProperty) depending on what the user has configured for that custom enum type. We won't get the exact same thing unfortunately, and I'm not sure that I see a way past that. There will be completely different data in the map file so we can't provide the same things. That is the advantage of using a custom enum type - we can provide an EnumProperty that will look the same regardless of storage type (as long as it is configured the same in DotTiled).

Do you think this is an acceptable way to change the behaviour? It would allow users to freely use custom enums, load maps that use them without crashing, and check their values without having to define them as custom enum definitions. The only caveat is the different storage type -> different property type in DotTiled.

[differenceclouds]

Yes, this sounds straightforward enough! I think getting the exact same data out regardless of setup is a noble goal but limiting in this case. You could add a warning in these cases I suppose.

[dcronqvist]

Great, there's a PR up with a fix for this issue. Is it possible for you to verify it against something you were experiencing a problem with? #65

[differenceclouds]

Ah, apologies, I thought I responded already. The PR seems to work as expected! Tested with several permutations.