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

Rework the TMX implementation #194

Merged
merged 10 commits into from Dec 17, 2018

Conversation

Projects
None yet
2 participants
@TheRamenChef
Copy link
Collaborator

TheRamenChef commented Dec 2, 2018

This pull request makes a number of changes, mainly to the TMX implementation, including:

  • Adding separation between tileset tiles and tile layer tiles
  • Allowing the Resources.load method to throw checked exceptions
  • Adding a checked TmxException to indicate a problem with loading a TMX file
  • Making ICustomPropertyProvider backed by a Map instead of a List (we may need to rename the Map class to avoid collisions; I would recommend TmxMap)
  • equals and hashCode implementations for various components
  • Support for multiline properties

This will most likely cause incompatibilities with previous versions, but they should be simple to fix.

TheRamenChef added some commits Dec 1, 2018

Seperate tiles and tileset entries
Quoth the Tiled documentation:

> Not to be confused with the `tile` element inside a `tileset`, this
element defines the value of a single tile on a tile layer.

Tiles still have custom properties, because there are a lot of things
depending on them and I'm not ready to remove them just yet.
Change the way custom properties are stored
Store them in a Map instead of a List
Remove needless XmlRootElement annotations
They are supposed to indicate ROOT elements, such as a map or tileset.
Chunks need it so that they can be used with @XmlMixed.

@steffen-wilke steffen-wilke self-requested a review Dec 9, 2018

@steffen-wilke
Copy link
Collaborator

steffen-wilke left a comment

Overall I think these are great changes.
The only thing I don't like is the changed behavior for when no default value is specified when calling CustomPropertyProvider.get...(). A lot of code relies on this. Instead of "I'll give you the property value if it's present or the default" it's now "If no such value is present, I'll throw an exception". This requires to always specify a defaultValue which is an overhead I'd rather like to avoid (e.g. see the PropMapObjecLoader). If I'm interested in whether there is a custom property value set or not, I can still use the new hasCustomProperty method but in most cases, I think it should be sufficient to return the default value for the requested type.

@TheRamenChef

This comment has been minimized.

Copy link
Collaborator

TheRamenChef commented Dec 14, 2018

Alright, changed as requested. Though, I would like to have custom property method that throws an exception if the property is missing, as a sort of defensive programming. Have it get in my face if I forget to configure the properties on an object.

@TheRamenChef

This comment has been minimized.

Copy link
Collaborator

TheRamenChef commented Dec 14, 2018

That's weird. Why does one CI build work, but not the other?

@steffen-wilke

This comment has been minimized.

Copy link
Collaborator

steffen-wilke commented Dec 17, 2018

I think this is due to the master currently having failing tests.

@steffen-wilke steffen-wilke merged commit 533e7b3 into master Dec 17, 2018

1 of 4 checks passed

Codacy/PR Quality Review Not up to standards. This pull request quality could be better.
Details
codeclimate 19 issues to fix
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@steffen-wilke steffen-wilke deleted the tmx-rework branch Dec 17, 2018

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