Skip to content

Support specified z index #24

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Jan 22, 2021
Merged

Support specified z index #24

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e0e3ae3
exports z-index order
agmcleod Nov 25, 2020
0ff734d
Merge branch 'master' into support-specified-z-index
agmcleod Jan 5, 2021
2901d04
refactor of getTileMapTemplate()
agmcleod Jan 7, 2021
5c03471
better handling of defaults
agmcleod Jan 11, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions export_to_godot_tilemap.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ class GodotTilemapExporter {
if (!ld.isEmpty) {
const tileMapName = idx === 0 ? layer.name || "TileMap " + i : ld.tileset.name || "TileMap " + i + "_" + idx;
this.mapLayerToTileset(layer.name, ld.tilesetID);
this.tileMapsString += this.getTileMapTemplate(tileMapName, ld.tilesetID, ld.poolIntArrayString, ld.parent, layer.map.tileWidth, layer.map.tileHeight, layer.property("groups"), parseInt(layer.properties()['z_index'], 10));
this.tileMapsString += this.getTileMapTemplate(tileMapName, ld.tilesetID, ld.poolIntArrayString, layer, ld.parent);
}
}
} else if (layer.isObjectLayer) {
Expand Down Expand Up @@ -446,8 +446,11 @@ ${this.tileMapsString}
* Template for a tilemap node
* @returns {string}
*/
getTileMapTemplate(tileMapName, tilesetID, poolIntArrayString, parent = ".", tileWidth = 16, tileHeight = 16, groups = undefined, zIndex) {
groups = splitCommaSeparated(groups);
getTileMapTemplate(tileMapName, tilesetID, poolIntArrayString, layer, parent = ".") {
const tileWidth = layer.map.tileWidth || 16;
const tileHeight = layer.map.tileHeight || 16;
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, not sure what these default values of "16" were for, but I don't think they make sense here. I guess this code currently means that when the tile size is somehow set to 0, it will take 16 instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was their previous default, i didn't want to impact any functional change.

Copy link
Member

@bjorn bjorn Jan 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

But function parameter defaults don't work this way, right? The previous code said "if nothing is passed, take 16" (though actually, there was always something passed) while the new code says "if it's something that evaluates to false, take 16".

Not that it is likely to happen, but it appears to me that it would be better to just remove the || 16 parts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah fair point, i added ternary to check for undefined specifically. If @MikeMnD is fine with removing the default aspect of it though, I can.

Copy link
Member

Choose a reason for hiding this comment

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

I just learned that the behavior of default parameter values in JS is not what I had expected. Indeed, it appears to take this value even when you explicitly pass in a value, but that value happens to be undefined. So yeah, with your latest code it indeed behaves the same way. :-)

Still, a value of undefined is basically impossible for the expression layer.map.tileWidth, so unless @MikeMnD had a particular reason to have these defaults I still think they can just be removed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

As far as I remember the 16 was just cause I've had 16x16px tiles for the my tilemap case. So it's fine to drop them now.

const groups = splitCommaSeparated(layer.property("groups"));
const zIndex = parseInt(layer.properties()['z_index'], 10);
return stringifyNode({
name: tileMapName,
type: "TileMap",
Expand Down