Fix combine with multiple contents #139
Merged
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #138
The issue originally referred to an aspect of the URI handling that was not specific for multiple contents, but has been extended to cover some related shortcomings of the current implementation of the
combine
command.Fixes #43
We talked about whether the (legacy)
url
property should be accepted in general, or whether updating this touri
should be done with a dedicatedupgrade
pass. But thecombine
command has to twiddle with (and modify) theuri
properties of contents anyhow, so I think that this is a case where accepting the legacy form (and printing a warning) is OK...The original implementation did not properly update the URIs of contents that had been found in external tilesets. This was caused by the fact that the 'combine' command can add new children to a tile, while the tile is being traversed. These new children had already been traversed during the recursive combination of nested external tilesets, causing some tiles to be traversed twice.
(There are some potentially deep, subtle intricacies when it comes to structural modifications of a tile hierarchy during the traversal: What should be allowed, and what should the behavior be? Originally, the implementation used the
Tiles.traverseExplicit
utility funciton, which does not anticipate structural changes. Now, the traversal has just been moved into theTilesetCombiner
, and is implemented there in a way that handles the modifications that it does)A broader issue with the
combine
command was that of handling multiple contents. Until now, when an external tileset was found, then the properties (content
/contents
) of the tile containing this external tileset had been replaced with the properties of the root of the external one. This breaks when there is a tile with contents[tile.b3dm, external.json]
(which is obvious... in hindsight...).Now, the command properly handles this case, and the specs have been extended with a test case for this (as explained in the README of that spec data):