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

Full support for multiple-b3dm-in-cmpt tiles, and some other fixes #109

Merged
merged 24 commits into from
Feb 2, 2021

Conversation

kring
Copy link
Member

@kring kring commented Jan 25, 2021

Builds on #102, #103, and #106, so merge those first.

  • Add full support for cmpt files that contain multiple b3dms. This works by merging the multiple glTFs together, a reasonably straightforward operation. I considered an alternate designs where a tile can contain multiple glTFs, but this forced more multiplicity (and thus more code) on clients (e.g. cesium-unreal) for no discernible benefit, because glTF already has all the multiplicity we need. I also considered allowing a single 3D Tiles "content" payload to produce multiple sub-tiles, but that got awkward with tiles that also have actual child tiles. Merging glTFs was clean, easy, and performant.
  • Fixed a bug that could cause runaway refinement when zooming close to a tileset with raster overlays (i.e. Cesium World Terrain). This also reduces the number of unnecessarily-upsampled tiles, making cesium-native on par with CesiumJS.
  • Improved the approach for calculting the bounds of upsampled tiles so that it doesn't compound floating-point error. The compounded error could cause too many raster overlays to be mapped to geometry tiles, which is not great in general but is especially bad for cesium-unreal with its low max number of raster overlays.
  • Added the tile URL to the extras of loaded glTFs. cesium-unreal uses this to name the ActorComponents created from the glTFs, so that when you click on a tile in the editor, the URL of the tile is printed in the console.
  • Update: this now also includes the ability to upgrade the data type of indices decoded from Draco when necessary, fixing a rendering problem in the San Miguel tileset. Thanks to @baothientran. (see Upgrade draco index #110)

Fixes #104

@kring
Copy link
Member Author

kring commented Jan 25, 2021

This is currently a PR into the gltf branch to make it easier to see what changed. It should be retargeted to master once #106 is merged.

Copy link
Contributor

@baothientran baothientran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kring this looks good to me. I only has a small comment below. After that, I think I can merge it into gltf branch


template <typename T>
struct MAT4 {
T value[9];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems to be a typo. Is it supposed to be 16?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! Fixed.

Base automatically changed from gltf to master February 2, 2021 00:17
@baothientran
Copy link
Contributor

thanks @kring! San-Miguel model works correctly

@baothientran baothientran merged commit 82361b3 into master Feb 2, 2021
@baothientran baothientran deleted the cmpt branch February 2, 2021 05:05
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

Successfully merging this pull request may close these issues.

Support composite (cmpt) tile content
2 participants