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

Making tile.type and layer.class_ compatible with Tiled 1.9's new Unified Custom Types ("class") #50

Merged
merged 11 commits into from Jul 18, 2022

Conversation

kurtome
Copy link
Collaborator

@kurtome kurtome commented Jul 9, 2022

ImageLayer repeats

Adding ImageLayer.repeatX and ImageLayer.repeatY

<imagelayer id="3" name="Image Layer 2" repeaty="1" repeatx="1">
  <image source="image2.png" width="32" height="32"/>
</imagelayer>

Unified Custom Types

Tiled 1.9 changelog:
https://www.mapeditor.org/2022/06/25/tiled-1-9-released.html

Layer

Adding layer.class_ for all for layer types.

Tile

Adding tile.class_ and making tile.type forward compatible with "class" attribute.

New in tiled 1.9:

  <tile id="98" class="Slope">
   <properties>
    <property name="LeftTop" type="int" value="0"/>
    <property name="RightTop" type="int" value="8"/>
   </properties>
  </tile>

Previously:

  <tile id="98" type="Slope">
   <properties>
    <property name="LeftTop" type="int" value="0"/>
    <property name="RightTop" type="int" value="8"/>
   </properties>
  </tile>

…es ("class")

https://www.mapeditor.org/2022/06/25/tiled-1-9-released.html

New in tiled 1.9:
```xml
  <tile id="98" class="Slope">
   <properties>
    <property name="LeftTop" type="int" value="0"/>
    <property name="RightTop" type="int" value="8"/>
   </properties>
  </tile>
```

Previously:
```xml
  <tile id="98" type="Slope">
   <properties>
    <property name="LeftTop" type="int" value="0"/>
    <property name="RightTop" type="int" value="8"/>
   </properties>
  </tile>
```
@kurtome kurtome changed the title Making tile.type compatible with Tiled 1.9's new Unified Custom Typ… Making tile.type compatible with Tiled 1.9's new Unified Custom Types ("class") Jul 9, 2022
CHANGELOG.md Outdated
## 0.8.3
* Downgrade meta dependency

## 0.8.2
* Add support for class, which is replacing type in tiled 1.9
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should never remove entries from the changelog, if the entry was wrong it should be explained in the new entry.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my bad, I got the versions mixed up because when I forked this wasn't here, I'll undelete this

@@ -1,5 +1,5 @@
name: tiled
version: 0.8.3
version: 0.8.4
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should normally not be bumped in this PR, but since I'll do a release after this is merged it doesn't matter this time.

@spydon
Copy link
Member

spydon commented Jul 11, 2022

Didn't #48 already solve this?
@ufrshubham

@ufrshubham
Copy link
Collaborator

Didn't #48 already solve this? @ufrshubham

No, #48 addressed this problem only for the tilemap files. This PR seems to fix it for tileset files as well. I didn't know that tilesets also had type property.

type: parser.getStringOrNull('type'),

/// Tiled 1.9 "type" has been moved to "class"
type: parser.getStringOrNull('type') ?? parser.getStringOrNull('class'),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to make it consistent with #48, you can make it check for class first, and then type. As more and more people will migrate to Tiled 1.9, chances of finding class property instead of type are more likely.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed the ordering.

I think with Tiled 1.9 you can add class to nearly anything, which is why they did the rename. We'd probably need to got through and test this for a bunch of other objects, but Layers for one could be useful. Although it raises the question, should we add a tiledClass attribute or keep calling them type even though they property in the new Tiled UI is "Class", I guess they named it that because layers already have a "type" which means something else.

In addition, the “Type” property previously available only for objects and tiles is now available for all data types as the new “Class” property. For consistency, this value is written out as “class” also for objects and tiles, but a project-wide compatibility option is provided to make it still write out as “type”

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For TiledObject class I kept type attribute as it was and just added a getter called class_ which returns type. That way it won't break anyone's existing code and if someone using tiled with newer map files, tries to find class attribute, class_ will pop-up in auto-complete list.

@kurtome kurtome changed the title Making tile.type compatible with Tiled 1.9's new Unified Custom Types ("class") Making tile.type and layer.class_ compatible with Tiled 1.9's new Unified Custom Types ("class") Jul 12, 2022
@kurtome
Copy link
Collaborator Author

kurtome commented Jul 12, 2022

@ufrshubham I added the class_ getter to Tile the same as you did for Object. I also added support for class_ on all 4 of the Layer types and added some unit tests.

@kurtome
Copy link
Collaborator Author

kurtome commented Jul 12, 2022

Also adding ImageLayer.repeatX and ImageLayer.repeatY, but I promise I'll stop tinkering with this now

@spydon spydon merged commit 6e7656a into flame-engine:main Jul 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants