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

merge creates boundingVolume.sphere #69

Closed
bertt opened this issue Sep 25, 2023 · 2 comments
Closed

merge creates boundingVolume.sphere #69

bertt opened this issue Sep 25, 2023 · 2 comments

Comments

@bertt
Copy link

bertt commented Sep 25, 2023

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
Copy link
Contributor

javagl commented Sep 29, 2023

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 #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
Copy link
Contributor

javagl commented Nov 9, 2023

This should be fixed with #79 :

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

  • takes the bounding volumes of the input tilesets
  • creates an oriented bounding box from each of them
  • creates a merged oriented bounding box from them, and uses this for the merged tileset

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.

@javagl javagl closed this as completed Nov 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants