-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
#2933 Set the firstgid property on Tilesets #2934
base: master
Are you sure you want to change the base?
Conversation
Hmm, why do you need the tileset's firstGid attribute set? What part of the code relies on it? I'd prefer if we could remove the firstGid member from the Tileset class instead, because I don't think it belongs there. It is only set when saving a map and only used while loading a map, and for that we have |
@bjorn isn't the firstgid required to map a global id back to a tileset? I am using this library in a server-side setting btw to parse out maps/tilesets. |
@csueiras Normally that happens while loading the map, here: which is used from: which is used from: So in the end, you would be working with |
@csueiras With the clarifications above, do you still see a use for exposing the |
384d5ac
to
2e9a0fb
Compare
More so than setting the firstgid to an specific value, For my use case, it would be enough to reorder each tileset when they are embedded into the map. I have an exclusive tileset for "non-colliding" tiles, setting that to the first tileset allows me to check for collision with |
@duarm This PR is a proposed change affecting the Java library for loading Tiled maps. I don't think it has anything to do with your concern, which appears to be about manually setting the firstgid or manually ordering the tilesets. The latter is covered by issue #1613. Note however, that the firstgids for the tilesets will always be incremental, as defined in the file format documentation. So, manually placing a tileset at the start of the list would change its firstgid to 1:
So I don't think your plan would work as such. You'd have to put that tileset first, and then check against the |
Sorry for the wrong issue, yeah, I meant to check with the second tileset, wrong pseudo code |
Fixes issue #2933
I've also added test coverage for the property being set.
@bjorn I know there's some on going work to release the latest version of the library to Maven central, let me know how can I help out to get these changes into master and released. Thanks!