search

CesiumGS / 3d-tiles-tools

Apache License 2.0
315 stars 47 forks source link

Invalid path handling for `combine` command #138

Closed javagl closed 5 months ago

javagl commented 5 months ago

This is a follow-up of https://github.com/CesiumGS/3d-tiles-tools/issues/137 , with the actual issue being isolated and carved out into a test case.

There are configurations where the combine command generates invalid paths/URIs for the tile content. The (distressingly simple) example to reproduce the error is here:

externalInSameDirectory-2024-06-19.zip

The structure is

tileset.json refers to
  sub0/externalA.json

sub0/externalA.json refers to
  tileA.b3dm
  externalB.json

sub0/externalB.json refers to
  tileB.b3dm

The combine command has to combine external tilesets recursively. This means that it will...

If I got this right, then the issue is related to the fact that the root of the processed external tileset is put into the surrounding tileset, and therefore traversed again, updating the URI for the tile content twice.

There is no trivial or obvious way of detecting whether a tile structure that is encountered during the traversal already had been there from the beginning, or whether it occurred due to "inlining" another external tileset - so it is not clear which content URIs have to be updated.

However, I think that there is a trivial way of solving this.

(I have no idea how. "Obvious" approaches of ~"not traversing the inlined parts" will break in other configurations. But I'm pretty sure that there is a trivial way, and I'll try to find it...)

javagl commented 5 months ago

There may be other issues with the combine command, related to external tilesets and multiple contents.

(I thought that I already brought this up, somewhere, but didn't find it right now. There had been a restriction that external tilesets are not allowed when multiple contents are used, but this only applied to 3D Tiles 1.0 + extensions, via https://github.com/CesiumGS/3d-tiles/pull/583 . But in 3D Tiles 1.1, this is allowed, according to the specification).

Specifically, the current approach for combining tilesets is revolving around the key operation:

When a tile content is an external tileset, then the content and children of the tile containing this content are replaced with that of the root of the external tileset.

This was the original approach. And even though it looks embarassingly obvious in hindsight, I didn't properly handle the case of multiple contents during the 3D Tiles Tools rewrite. When there are multiple contents, then the contents/children of the tile may not be replaced. Instead, the root of the external tileset will have to be added to the children of the tile.

I'll try to generalize that, fixing this issue along the way, and increase the test coverage here.

javagl commented 5 months ago

A minor clarification/constraint: The specification says

When a tile points to an external tileset, the tile:

  • Cannot have any children; tile.children shall be omitted

So the children do not require an explicit special handling. But multiple contents in general still do: When a tile has contents [tile.b3dm, external.json], and the combine command is applied, then this tile shuld afterwards have

(One could think about "optimizations" here for special cases - e.g. some form of collapsing/compacting for the case where the externa.json root only has one content, or only one child. But iff these are addressed, they should be an explicit operation. The goal of combine should be to combine, and not to optimizeInSomeWay...)