Support specified z index #24
Conversation
| @@ -91,19 +91,21 @@ function stringifyNode(nodeProperties, contentProperties = {}) { | |||
| } | |||
| str += ']\n'; | |||
| for (const [key, value] of Object.entries(contentProperties)) { | |||
| str += this.stringifyKeyValue(key, value, false, true) + '\n'; | |||
| if (value !== undefined) { | |||
| str += this.stringifyKeyValue(key, value, false, true) + '\n'; | |||
There was a problem hiding this comment.
Added this to have a cleaner set of options above for passing the z_index, but don't know if theres legit cases to include undefined in the scene file.
export_to_godot_tilemap.js
Outdated
| @@ -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")); | |||
| 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)); | |||
There was a problem hiding this comment.
While the change looks fine to me, at this point since we're passing in 4 parameters that are taken from the layer, I'd change the function to take layer itself as parameter instead.
export_to_godot_tilemap.js
Outdated
| groups = splitCommaSeparated(groups); | ||
| getTileMapTemplate(tileMapName, tilesetID, poolIntArrayString, layer, parent = ".") { | ||
| const tileWidth = layer.map.tileWidth || 16; | ||
| const tileHeight = layer.map.tileHeight || 16; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
It was their previous default, i didn't want to impact any functional change.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
#23