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

Second TMX Overhaul, Part 2 #243

Merged
merged 35 commits into from
Apr 13, 2019
Merged

Second TMX Overhaul, Part 2 #243

merged 35 commits into from
Apr 13, 2019

Conversation

TheRamenChef
Copy link
Collaborator

@TheRamenChef TheRamenChef commented Mar 25, 2019

For part 1, see #232.

This pull request adds even more missing features, including:

  • All resources except spritesheets are now cached using URLs as keys (they still work with strings)
  • Resources can now be loaded asynchronously
  • Most entities loaded from map objects are now rendered as part of their respective layers
  • Tilesets that use individual images for each tile are now supported
  • Tile types are now supported
  • File custom properties are now supported, though only tentatively
  • Light sources now use an enum for their type
  • Various refinements for several features

This pull request was going to have an implementation of JSON maps, but I got stuck on the tile layer data. Actually removing 451348d from the branch proved to be too much of a hassle; if you can do it, go ahead, or don't.

steffen-wilke and others added 21 commits March 13, 2019 16:31
Conflicts:
	build.gradle
Add equals, hashCode, and toString, and don't make it extend
CustomPropertyProvider
Mainly, I had to do these all at once.
Also, "false" is the default value for "required" on @xmlelement and
@XmlAttribute
The "as determined by" already clears up how to resolve tiles for edges
using the definition of insideness.
Conflicts:
	src/de/gurkenlabs/litiengine/environment/tilemap/MapUtilities.java
	src/de/gurkenlabs/litiengine/environment/tilemap/xml/GroupLayer.java
	src/de/gurkenlabs/litiengine/physics/PhysicsEngine.java
	src/de/gurkenlabs/litiengine/resources/ResourcesContainerListener.java
	utiliti/src/de/gurkenlabs/utiliti/EditorScreen.java
Also, implement file properties. It's important to note that file
properties are still somewhat WIP. They work for loading a map made in
Tiled, but additions would need to be made for actually setting such a
property.
@steffen-wilke steffen-wilke self-requested a review March 26, 2019 12:16
Copy link
Contributor

@steffen-wilke steffen-wilke left a comment

Choose a reason for hiding this comment

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

Entities don't have to implement IRenderable but can still have an AnimationController that renders them. This commit effectively forces every Entity implementation to also implement IRenderable which is a breaking change in the way we render our entities. This needs to be reverted. I'm also not sure yet, how the changes related to #172 can be seen in the grand scheme of things, which is why I originally tagged it with "needs-discussion".

From the commit messages and the code that changed, I think most of the changes are reasonable but this pull-request contains so many different pieces including various breaking changes and also just some minor adjustments, it's hard for me to judge the impact that this PR would have on existing code and the engine in general. We need to split it up into various smaller PR such that we can properly test the individual changes. Also, as far as I could see (maybe I overlooked it) there were no unit tests added that check the changed behavior, I think this would be a great addition.

@TheRamenChef
Copy link
Collaborator Author

TheRamenChef commented Apr 1, 2019

For most of the changes, either a unit test would be infeasible (such as graphics changes) or the unit tests were already there (such as moving resource storage to URLs).

With regards to the layer entities, I didn't know that IRenderable wasn't required for drawing an entity; I will need to look into that and implement it for AnimationControllers.

@steffen-wilke
Copy link
Contributor

For most of the changesm either a unit test would be infeasible (such as graphics changes) or the unit tests were already there (such as moving resource storage to URLs).

Okay, since this part was also just something additional from my side, I guess the topic of unit tests is done for this PR.

@TheRamenChef
Copy link
Collaborator Author

Looking at the code again, it looks like it was just the sorting optimization that was made. I've reverted that change.

Also, with entities bound to layers, it doesn't change the way it's rendered if you put the map object layer on the top, or bottom if you’re using the BACKGROUND render type.

@steffen-wilke steffen-wilke self-assigned this Apr 10, 2019
TheRamenChef and others added 4 commits April 12, 2019 16:56
Conflicts:
	src/de/gurkenlabs/litiengine/environment/tilemap/xml/Animation.java
	src/de/gurkenlabs/litiengine/environment/tilemap/xml/GroupLayer.java
	src/de/gurkenlabs/litiengine/environment/tilemap/xml/Map.java
	src/de/gurkenlabs/litiengine/environment/tilemap/xml/TilesetEntry.java
Fix a bug where the serialization process assumed that a MapObject always has to have a MapObjectLayer as parent.
Add method overload to load a game resource file from a String to stay backward compatible.
Make use of "getEnumValue" to retrieve the light shape.
Extend the getEnumValue process to be case insensitive as a fallback.
@steffen-wilke
Copy link
Contributor

I did some additions to the new feature of rendering Entities with their layers. From the code, this PR looks good now in my opinion. But when testing the branch with our game DR.LEPUS, the layers of the map don't seem to be rendered anymore. I did't find the reason for this yet, but I think once we've fixed that issue, we can merge this PR.

@TheRamenChef Are there any changes to the map format that would result in the engine version ofthis branch being incompatible with previous maps? Maybe you can also have a look at this.

2019-04-13-14-17-20

@steffen-wilke steffen-wilke self-requested a review April 13, 2019 14:10
Copy link
Contributor

@steffen-wilke steffen-wilke left a comment

Choose a reason for hiding this comment

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

We need to fix the mentioned issue with the layers not being rendered at all in this branch and then we can merge the PR.

@TheRamenChef
Copy link
Collaborator Author

I think I found the problem. Maps with these changes have a finish method that resolves external resources, among other things. This method would not be called for TMX resources from a ResourceBundle.

Does it work now?

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.

2 participants