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

Libtiled tile read fix #2006

Merged
merged 5 commits into from Jul 22, 2019

Conversation

@Sam-Manflame
Copy link
Contributor

Sam-Manflame commented Sep 11, 2018

Hello hello. We have met some problems while trying to use libtiled in our project: reading of flipped tiles and reading of images. Here is our solution(we also added a test for flipped tiles).

@bjorn

This comment has been minimized.

Copy link
Owner

bjorn commented Sep 11, 2018

First of all thank you for contributing to libtiled-java! Step by step it will support TMX features added after I stopped coding in Java, like tile flipping. :-)

However, the current implementation is not going to be very memory-friendly, since it allocates a new Tile object for each flipped instance of a tile. Flip your entire map and each individual cell will end up pointing to its own uniquely allocated Tile instance, and due to for example its properties not being immutable, they will get cloned for each instance as well (even though tile properties really should be shared between flipped instances of a tile).

In Tiled I have solved this with a Cell structure, which points to a Tile and stores the flags. Maybe something similar would work in this case? Alternatively, a special cloning method could be used to avoid duplicating the properties, and a cache could be introduced to avoid creating multiple Tile instances with the same flags.

The image layer part of the change looks fine.

@Sam-Manflame

This comment has been minimized.

Copy link
Contributor Author

Sam-Manflame commented Sep 23, 2018

I have two ideas:

  1. Having Cell[][] along with Tile[][] in TileLayer(cause replacing Tile with Cell will cause a lot of changes)
  2. We also can save gid in tileInstanceProperties.
@bjorn

This comment has been minimized.

Copy link
Owner

bjorn commented Sep 23, 2018

If you're going to put the flipping information alongside the tiles array, I'd go for int[][] and store the flip states as bitflags.

Sam-Manflame and others added 2 commits Oct 12, 2018
…ws system. Replacing path separator when reading tileset or map file.
throw new IllegalArgumentException("path cannot be null.");
if (path.isEmpty() || path.lastIndexOf(File.separatorChar) >= 0)
return path;
return path.replace("/", File.separator);

This comment has been minimized.

Copy link
@bjorn

bjorn Oct 19, 2018

Owner

Why is this necessary? I thought on Windows both / and \ worked as path separators.

This comment has been minimized.

Copy link
@pvlbndrnk

pvlbndrnk Nov 21, 2018

Contributor

Hello! The problem occurs during the initialization of xmlPath.
Before you take a substring from the filename, you need to replace the separator. Otherwise, the xmlPath will not be initialized, or it will not be initialized correctly.

@iarkhanhelsky

This comment has been minimized.

Copy link

iarkhanhelsky commented Feb 12, 2019

Hi @bjorn! It's been a while since last activity on this PR but we hope we can get this merged. If you have concerns about any changes we would be happy to receive your feedback and resolve them.

@bjorn bjorn merged commit 261df78 into bjorn:master Jul 22, 2019
3 checks passed
3 checks passed
WIP Ready for review
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bjorn

This comment has been minimized.

Copy link
Owner

bjorn commented Jul 22, 2019

@iarkhanhelsky I'm very sorry, I forgot about this change for a long time despite your reminder. I just looked at it again and saw no reason not to merge these great improvements. Many thanks to @Sam-Manflame and @pvlbndrnk for the patches!

I see the last patch also includes the optimization discussed at #2089, so we can close that pull request. I still think that rather than a cache, we could create just a single Unmarshaller instance that handles all the needed types, like I suggested in #2089 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.