CesiumGS / 3d-tiles-tools

Apache License 2.0
312 stars 47 forks source link

merge creates boundingVolume.sphere #69

Closed bertt closed 1 year ago

bertt commented 1 year ago

Hi, I'm trying to merge (using 3.0) 2 tileset's with implicit tiling. When viewing the result in Cesium the following error occurs: "Only box, region, 3DTILES_bounding_volume_S2, and 3DTILES_bounding_volume_cylinder are supported for implicit tiling". The resulting tileset.json file has boundingVolume.Sphere, in the input tileset.json files I've used boundingVolume.region.

Is it possible the merge tool creates boundingVolume.region instead of boundingVolume.sphere?

javagl commented 1 year ago

Just a short 'ack' for now:

The core of the merge functionality was taken from another branch (see comments in TilesetMerger). This code originally created a bounding sphere, and has not been refactored or generalized while being ported to the "new 3D Tiles Tools".

It would be preferable to have better bounding volumes for the output. So this issue is somewhat related to https://github.com/CesiumGS/3d-tiles-tools/issues/58 - at least, on the level that there should be more functions for creating "good" bounding volumes of different types, from different types of input data.

It is not entirely trivial in all cases. You can see that the functions that have been ported from the merge-tilesets branch all have the shape of createBoundingSphereFrom<OtherType>(...). And this bears the potential for some combinatorial explosion, if there should be create X from Y functions for all types X and Y. But we'll have to see in how far some of the functionality that is related to bounding volumes (and that currently is a bit scattered in the repo, with parts being ported from older branches, taken from CesiumJS, or implemented from scratch) can be consolidated.


Remotely related: I've never seen or heard of 3DTILES_bounding_volume_cylinder - I'll have to investigate what that is...

javagl commented 1 year ago

This should be fixed with https://github.com/CesiumGS/3d-tiles-tools/pull/79 :

The merge command (via its implementation with the TilesetMerger) now ...

There is still some legacy code in the TilesetMerger (and the test coverage should probably be increased a bit...), but the main issue here should be resolved.