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

Less holes, and improvements to "forbid holes" mode #488

Merged
merged 9 commits into from
Apr 19, 2022

Conversation

kring
Copy link
Member

@kring kring commented Apr 17, 2022

This PR has a few significant improvements to tile selection, especially when "Forbid Holes" is enabled:

Improved performance with "Forbid Holes"

When "Forbid Holes" is enabled, all of a parent tile's children must be loaded before the parent can be refined. This forces us to "refine down through the tree." But previously (before this PR), the algorithm also loaded all children of a tile that met the SSE. That was super wasteful; in a quadtree it would end up loading 4x more tiles than necessary! "Forbid Holes" enabled is always going to be slower, but now it's much closer to peformance parity and therefore much more useable.

Fixed ping-ping loading with "Forbid Holes"

With "Forbid Holes" enabled, the algorithm would often get stuck in a cycle of loading and unloading the same tiles repeatedly. 😱 This PR fixes it.

Essentially the problem was that already-loaded child tiles would not be marked as visited while waiting for other children to finish loading. Non-visited tiles are eligible to be unloaded. With a full cache, we'd unload one child tile while waiting for a second to load, then unload the second while waiting for the first. And so on.

Don't get stuck

When using raster overlays and a tileset where some tiles don't have any content, the algorithm could sometimes get stuck and fail to refine tiles to their proper detail. It's easy to reproduce with such a tileset, but unfortunately I can't share the one I've been testing with here. Here's how it's done:

  1. Create a tileset and enable "Forbid Holes"
  2. Attach a raster overlay, such as DebugColorizeTilesRasterOverlay.
  3. Zoom in close and let the detail load.
  4. Change the maximum SSE of the tileset, even slightly.
  5. All the detail disappears, and doesn't come back.

Changing the SSE back doesn't help. Recreating the Tileset (e.g. pressing Refresh Tileset in Cesium for Unreal) does fix it.

Setting the SSE in Cesium for Unreal removes and re-adds the raster overlay. When we add a raster overlay, we add a placeholder for it to all tiles. In the Tile::update, we turn the placeholder into a real overlay tile, if necessary. But we only turn the placeholders into real overlays if the tile has geometry. For tiles without content, before this PR, the placeholders would simply block refinement forever, because they would never be deemed ready.

The solution in this PR is to remove overlays from tiles that are in the Done state but still do not have any geometry.

Forbid Holes should be aware of unconditional refinement

Some tiles are marked "unconditionally refine". Such tiles should never be shown, but are only used for culling and then their children are considered in their place. This PR makes the "Forbid Holes" option aware of this, so that, when waiting for children to load, if one of those children is marked "unconditionally refine", then that tile's children will be waited on, too.

This fixes some holes that can appear even in forbid holes mode.

Better handling of tiles without content

cesium-native has long treated tiles without any content exactly the same way as it does tiles that do have content. If the tile meets the SSE, it's "rendered". In a case A -> B -> C where a tile without content (B) appears between two tiles that do have content (A and C), this can cause a hole to briefly appear in between when A is refined but C is not yet loaded, even if B's geometric error is huge and so it immediately refines anytime A refines.

This is arguably correct according to the 3D Tiles spec, but it's inconsistent with CesiumJS, and it causes problems when a tile without content is used for structural reasons (e.g. an external tileset must have a single root tile).

So with this PR, a tile without content will now be handled differently depending on whether it's geometric error is less than its parent's geometric error. From the code:

        // There are two possible ways to handle a tile with no content:
        //
        // 1. Treat it as a placeholder used for more efficient culling, but
        //    never render it. Refining to this tile is equivalent to refining
        //    to its children. To have this behavior, the tile _should_ have
        //    content, but that content's model should be std::nullopt.
        // 2. Treat it as an indication that nothing need be rendered in this
        //    area at this level-of-detail. In other words, "render" it as a
        //    hole. To have this behavior, the tile should _not_ have content at
        //    all.
        //
        // We distinguish whether the tileset creator wanted (1) or (2) by
        // comparing this tile's geometricError to the geometricError of its
        // parent tile. If this tile's error is greater than or equal to its
        // parent, treat it as (1). If it's less, treat it was (2).
        //
        // For a tile with no parent there's no difference between the
        // behaviors.

CC CesiumGS/3d-tiles#609

@nithinp7 nithinp7 self-requested a review April 19, 2022 13:44
Comment on lines 849 to 851
if (this->getState() == LoadState::Done &&
this->getTileset()->supportsRasterOverlays() && this->getContent() &&
this->getContent()->model) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not directly related to this PR, but "supportsRasterOverlays" doesn't need to exist anymore right? Removing that would make this and the "else if" logic a little clearer to read. Opened this issue for it: #489

@nithinp7
Copy link
Contributor

Thanks for investigating this @kring, this looks solid to me!

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.

2 participants