Support Tiled 1.9 class property

[barthap]

Why

Closes #234

In Tiled 1.9 the "type" property was renamed to "class". My PR addresses this.

How

In addition to "type", the "class" property is parsed. If the former is not present, the latter takes its place. This way, the solution is compatible with both 1.9+ and older maps.

After the below conversation, this is what I did:

• Deprecated obj_type for Objects and tile_type for Tiles in favor of user_type. Added deprecation notices
• Added user_type to Maps, Tilesets and Layers.
• Both class and type XML params now resolve to user_type everywhere.

Support for type/class is now implemented for the following entities:

• Tiles in tileset (Tile)
• Objects
• Maps
• Tilesets
• Layers

These are all places that I'm aware of.

Test plan

Tested locally on both old and new maps.
I didn't add tests as I haven't found any existing ones (nor test fixtures) which have the "type" or "class" property set. Eventually, I can create/modify one if requested.

[aleokdev]

Thank you! Could you also update the changelog (On the "Unreleased" section)?

[barthap]

Ouch, actually skipping CI blocked the required workflow from running 🙄 Force-push might help Fixed

[aleokdev]

@bjorn Could you check this out? I'm still not 100% familiar with Tiled 1.9 changes

[barthap]

Actually, I found out, that layers (all kinds of them?) and maps can have a class property too.

So my PR only fixes this for existing entities which previously had type. I can add it to the others, but how the property should be named? I thought about map_type etc.. to keep the convention, but for layers there is already a (private) layer_type so I'm not sure how to name it. I don't know your breaking change policy, but maybe let's change this to property named {obj,map,tileset,layer}_class: String? 🤔

[aleokdev]

Bjorn plans to rename class back to type on 1.10, so I don't think the rename is worth it:

[...] that rename didn't go very well, everybody hates their project breaking due to some arbitrary format change. So I plan to rename "class" back to "type", [...]

[barthap]

Bjorn plans to rename class back to type on 1.10, so I don't think the rename is worth it

Ok, in this case it would be good to just add support for the type/class property to all entities, without renaming it. I pushed a proposal commit.

[bjorn]

If we go for user_type then I think we can consistently use this name rather than map_type and tileset_type and such. I realize we already have obj_type, which made sense when it only applied to objects, but maybe it could be renamed to user_type as well now.

And indeed, I plan to rename class back to type for Tiled 1.10, in which case it would be type everywhere else too. While we're at it, it might be good to already support this.

[barthap]

Yes, in this case, using the user_type everywhere makes the most sense. I've updated my PR to:

• Deprecated obj_type for Objects and tile_type for Tiles in favor of user_type. Added deprecation notices
• Added user_type to Maps, Tilesets and Layers.
• Both class and type XML params now resolve to user_type everywhere.

I have found one inconsistency. For tiles, the tile_type was of type Option<String>, and for Objects it was just String and optional unwrapped to empty string.
Which way should be preferred? I'd go with the optional type instead of empty strings everywhere, but for now I left it as-is for objects.

[aleokdev]

I have found one inconsistency. For tiles, the tile_type was of type Option<String>, and for Objects it was just String and optional unwrapped to empty string.

This inconsistency also exists in the TMX docs, so I guess we should leave it as is. @bjorn Thoughts?

[aleokdev]

Eh, it's good enough 🙂

[aleokdev]

Thank you! 🙏

[bjorn]

This inconsistency also exists in the TMX docs, so I guess we should leave it as is. @bjorn Thoughts?

Hmm, I guess the docs have to be fixed. While this property is optional in the file, storage wise I've always defaulted to an empty string rather than having an explicit "type has not been specified" state. Is it helpful to have such a state in Rust?

[aleokdev]

Hmm, I guess the docs have to be fixed. While this property is optional in the file, storage wise I've always defaulted to an empty string rather than having an explicit "type has not been specified" state. Is it helpful to have such a state in Rust?

Is it helpful to distinguish between having an empty text attribute vs not having it at all? I don't know 🙂 Depends on the format and editor, not the language really

[barthap]

Regarding Option vs empty string, I have mixed feelings about that:

• From my perspective, the option seems to be more "correct" from the language point of view, which means the value hasn't been set. An empty string suggests that a value exists.
• On the other hand, in this particular case it's more convenient to compare against empty string than option:

  // option
  if tile.user_type.as_deref() == Some("light_source") { /* ... */ }
  
  // empty string
  if tile.user_type == "light_source" { /* ... */ }

[bjorn]

Right, then I think we should not use Option. Since it is both less convenient, and actually in the editor a "type has not been set" state does not exist either, or equals the value being an empty string.