-
Notifications
You must be signed in to change notification settings - Fork 20
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
Fixing loading tilesets with spacing #368
Conversation
…maps with spacing
Also I bumped the package version, but that might be done as part of the build process...just lemme know, I can roll that back or make any other changes! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ttay24 This is great, thanks for the PR! We can keep the package.json rev, this is definitely something I want to publish soon 👍
2 small changes
- Update the spacing coalesce to prevent NaN's
- Update integration test for this example
test spacing
toex-tests.ts
this does image diff testing to make sure this new feature will work in the future 👍
// ex-tests.ts
// note the mapTypes array all compare against the same expected image
await selectMap(page, 'test spacing');
await expectLoaded();
await expectPage('test spacing', `./test/integration/images/actual-test-spacing.png`).toBe('./test/integration/images/expected-test-spacing.png');
You can run the integration tests in "interactive mode" and it will ask to generate new expected test images that can be checked in.
npm run test:integration -- -i
src/tiled-map-resource.ts
Outdated
@@ -545,15 +545,21 @@ export class TiledMapResource implements Loadable<TiledMap> { | |||
private _createTileMap() { | |||
// register sprite sheets for each tileset in map | |||
for (const tileset of this.data.rawMap.tilesets) { | |||
const cols = Math.floor(tileset.imagewidth / tileset.tilewidth); | |||
const rows = Math.floor(tileset.imageheight / tileset.tileheight); | |||
const cols = Math.floor((tileset.imagewidth + tileset.spacing) / (tileset.tilewidth + tileset.spacing)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to null coalesce here as well otherwise the addition will produce NaN
's if spacing is null/undefined.
Perhaps into a constant?
const spacing = tileset.spacing ?? 0;
const cols = Math.floor((tileset.imagewidth + spacing) / (tileset.tilewidth + spacing));
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that makes sense! Just pushed these changes
I'm having trouble with the integration test. Any ideas? I'm getting an unhandled promise rejection:
|
I am very dumb lol. I had another webpack dev server running with my game jam attempt, so it failed. Once I stopped that, it seems like it's working normally :D should have an update shortly Edit: should be good to go now! |
@ttay24 The integration tests should really pick a free port an run sorry about that! I have an open issue to fix it excaliburjs/excalibur-testing#2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ttay24 Thanks again! This is fantastic!
@ttay24 New version is live |
Awesome, thanks! I can get rid of my hacky packaged solution in my project lol |
Explanation on the fix:
The number of columns and rows were not being calculated correctly, which threw off which sprite it was pulling from the sprite sheet.
If we had a tileset with 3 tiles, 16px wide and 1px spacing, the width of the image would be 50px. We don't get clean numbers if we do
50 / (16 + 1)
because there are only spaces between the tiles, but not before/after the first/last tiles. So we can just add the spacing to the height/width, and then divide by the tile width (or height) + spacing.Ex:
Before
After
Tiled Map