Expected values removed from tiledmap data format version 1.10.2

[twje]

The parse fails due to values removed from tiledmap data format in version 1.10.2.

For image tilesets, the json values imagewidth, imageheight and image are not present. These are only available for spritesheet tilesets denoted by when the number of columns is greater than 0.

In addition, Tile::performDataCalculations() throws a division by zero exception for image tilesets when columns is 0.

[SSBMTonberry]

Hello, @twje, and thanks for your issue 🙂
The Tiled changelog does not, as far as I can see, mention any of the changes you are listing in the format, but then again: I've encountered quite a few undocumented changes in the past, so you may still be right (and I believe you are).

Could you post a bare minimum map that can be used to reproduce the error described in the issue? 🙂

[twje]

Hi @SSBMTonberry. Attached is a zip file that consists of a tiled map with a single embedded tileset based on a collection of images and one tile layer.

Unlike tilesets based on a single image, spritesheet, where imagewidth, imageheight, and image values apply to the entire tileset, in this case, these values are specific to each individual tile.

It's also curious to note that columns is 0 when a tileset is based on a collection of images.

map.zip

[SSBMTonberry]

Thanks, @twje 🙂
That you are using collection of images is significant.

As stated in the Readme:

Issue:

• Add support for "Collection of Images" tilsets #30

Sounds like I should prioritize getting that issue done soon...
Alternatively: Feel free to have a look at it yourself and push a PR. It's best done if done by someone who are actively using that feature, though it shouldn't be a problem implementing either way. Not looked at your zip content, but I'm sure that data is good enough to use as eventual verirfications for the tests 🙂

If you are interested in looking into this, just tell me, and I'll assign the #30 task to you :)
If not: I'll have a look at this issue next when I get the time.

[twje]

The fix has been applied in a personal project. I am happy to clean up the code and push a pull request. However, it might be until next week before I find the time to complete it.

[SSBMTonberry]

The fix has been applied in a personal project. I am happy to clean up the code and push a pull request. However, it might be until next week before I find the time to complete it.

Feel free to do it whenever you have the time 🙂
Also: I can be a good idea to read the contribution guidelines before proceeding.

[twje]

Do you have a preference on updating the demo maps to showcase this feature? The options are, add a tileset based on a collection of images, to an exisiting map or create a new map.

[SSBMTonberry]

Do you have a preference on updating the demo maps to showcase this feature? The options are, add a tileset based on a collection of images, to an exisiting map or create a new map.

I would prefer if you created a new map to showcase this feature. That way it cannot affect any of the previous demos, and I think it's a bit cleaner that way 🙂

[twje]

There is a bug when adding missing tiles that do not have property data. Tileset::generateMissingTiles adds missing tiles based on the Gid. However, two Tile instance is added for a given tile if it has properties. Similarly, the same also applies to tiles from an image collection tileset.

One caveat to note is that this bug only manifests for tilesets with a first Gid that is not 1.

Attached is a bar minimum map to reproduce this bug.

For example, consider a tileset with a first gid of 10 and and single tile that has property data:

• instance 1: id=1, gid=10
• instance 2: id=10, gid=10

There should be one Tile instance:

• instance 1: id=1, gid=10

The solution is to amend Tileset::generateMissingTiles and the second Tile constructor responsible for instantiating missing tiles:

Exisiting Code

void tson::Tileset::generateMissingTiles()
{
	...
	for(uint32_t i = m_firstgid; i < m_firstgid + (uint32_t) m_tileCount; ++i)
	{
		if(std::count(tileIds.begin(), tileIds.end(), i) == 0)
		{
			m_tiles.emplace_back(Tile(i, this, m_map));
		}
	}
}

tson::Tile::Tile(uint32_t id, tson::Tileset *tileset, tson::Map *map) 
	: m_id {id}
	, m_gid {id}
{  ... }

Revised Code

void tson::Tileset::generateMissingTiles()
{
	...
	for (uint32_t i = 1; i < (uint32_t)m_tileCount + 1; ++i)
	{
		if(std::count(tileIds.begin(), tileIds.end(), i) == 0)
		{
			m_tiles.emplace_back(Tile(i, this, m_map));
		}
	}
}

tson::Tile::Tile(uint32_t id, tson::Tileset *tileset, tson::Map *map) 
	: m_id{ id }
	, m_gid{ tileset->getFirstgid() + id - 1}
{ ... }

The code change does not break the exisiting unit tests or demo test maps.

As this bug is on the critical path of implementing the image collection tileset feature, I can fix it as part of this feature or raise a separate issue for it.

Somewhat related, I am curious why you chose to represent tile ids starting at 1 and not 0?

map.json

[SSBMTonberry]

@twje : If it's critical for the feature to work, you are welcome to include it in the same PR, but if it were unrelated to the feature, I would have liked it to be in a separate issue 🙂
There is another bug that was reported recently that was caused by something that seems to be pretty similar to what you are describing, and is likely caused by the same thing:

• Animated tile IDs, and nullptr returned by tileset->getTile(tileId), local vs global #110

The main reason why Tileson uses 1 as the first tile id is consistency. In a map 0 means no tile, but in Tiled a tile definition starts at 0. To be able to map a tile against the map you will have to +1 the tile ID at some point anyways, so I chose to simply say that both uses the same id in Tileson. That makes sure you can always expect one tile id to always be the same, no matter which context you are in.

[twje]

Hi @SSBMTonberry,

This fix would also address issue #110. In a tileset, the IDs of tiles are local, which allows them to reference one another within the same tileset, for example, animation frames. At the moment, missing tiles are being assigned a global ID for their local ID.

It's an interesting and somewhat tricky bug because it only shows up under specific conditions: when the first global ID (gid) of a tileset isn't set to 1, and there is no metadata for the tile.

I will push a PR for #110 instead of creating a new issue.

[twje]

A PR for #110 has been pushed

[twje]

PR #117 to add support for Image Collecton Tilesets has been pushed