Skip to content
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

Fix combine with multiple contents #139

Merged
merged 9 commits into from
Jun 24, 2024
Merged

Conversation

javagl
Copy link
Contributor

@javagl javagl commented Jun 22, 2024

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 to uri should be done with a dedicated upgrade pass. But the combine command has to twiddle with (and modify) the uri 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 the TilesetCombiner, 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):

  • A tile refers to a SINGLE content, which is an external tileset
    • In this case, the properties of the tile will be "replaced" with the properties of the external root
  • A tile refers to an multiple contents, including external tilesets
    • In this case, the external roots will be added as children to the tile

@javagl
Copy link
Contributor Author

javagl commented Jun 22, 2024

I tried the new functionality with the tileset that was provided in #137 and originally sparked this issue. It seems to work. And I'll probably try it out by running the resulting tileset in CesiumJS and the validator. But ... not now. The resulting tileset.json has 200MB (!), which is... impractical in many ways...

@javagl javagl marked this pull request as ready for review June 22, 2024 14:08
@lilleyse lilleyse merged commit 05c3ab1 into main Jun 24, 2024
2 checks passed
@lilleyse lilleyse deleted the fix-combine-with-multiple-contents branch June 24, 2024 13:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants