Less holes, and improvements to "forbid holes" mode

CHANGES.md

@@ -1,5 +1,18 @@
 # Change Log
 
+### ? - ?
+
+##### Additions :tada:
+
+- Improved the load performance when `TilesetOptions::forbidHoles` is enabled by only loading child tiles when their parent does not meet the necessary screen-space error requirement.
+
+##### Fixes :wrench:
+
+- Fixed a bug that could cause the same tiles to be continually loaded and unloaded when `TilesetOptions::forbidHoles` was enabled.
+- Fixed a bug that could sometimes cause tilesets to fail to show their full detail when making changes to raster overlays.
+- Fixed a bug that could cause holes even with `TilesetOptions::forbidHoles` enabled, particularly when using external tilesets.
+- Tiles will no longer be selected to render when they have no content and they have a higher "geometric error" than their parent. In previous versions, this situation could briefly lead to holes while the children of such tiles loaded.
+
 ### v0.14.1 - 2022-04-14
 
 ##### Fixes :wrench:

Cesium3DTilesSelection/include/Cesium3DTilesSelection/Tileset.h

@@ -461,26 +461,23 @@ class CESIUM3DTILESSELECTION_API Tileset final {
 
   /**
    * @brief Queues load of tiles that are _required_ to be loaded before the
-   * given tile can be refined.
+   * given tile can be refined in "Forbid Holes" mode.
    *
-   * If {@link TilesetOptions::forbidHoles} is false (the default), any tile can
-   * be refined, regardless of whether its children are loaded or not. So in
-   * that case, this method immediately returns `false`.
+   * The queued tiles may include descedents, too, if any children are set to
+   * Unconditionally Refine ({@link Tile::getUnconditionallyRefine}).
    *
-   * When `forbidHoles` is true, however, and some of this tile's children are
-   * not yet renderable, this method returns `true`. It also adds those
-   * not-yet-renderable tiles to the load queue.
+   * This method should only be called if {@link TilesetOptions::forbidHoles} is enabled.
    *
    * @param frameState The state of the current frame.
    * @param tile The tile that is potentially being refined.
    * @param implicitInfo The implicit traversal info.
    * @param distance The distance to the tile.
-   * @return true Some of the required children are not yet loaded, so this tile
-   * _cannot_ yet be refined.
-   * @return false All of the required children (if there are any) are loaded,
-   * so this tile _can_ be refined.
+   * @return true Some of the required descendents are not yet loaded, so this
+   * tile _cannot_ yet be refined.
+   * @return false All of the required descendents (if there are any) are
+   * loaded, so this tile _can_ be refined.
    */
-  bool _queueLoadOfChildrenRequiredForRefinement(
+  bool _queueLoadOfChildrenRequiredForForbidHoles(
       const FrameState& frameState,
       Tile& tile,
       const ImplicitTraversalInfo& implicitInfo,

Cesium3DTilesSelection/src/Tile.cpp

@@ -846,7 +846,6 @@ void Tile::update(
     }
   }
 
-  // TODO: if there's no model, we can actually free any existing overlays.
   if (this->getState() == LoadState::Done &&
       this->getTileset()->supportsRasterOverlays() && this->getContent() &&
       this->getContent()->model) {
@@ -904,6 +903,12 @@ void Tile::update(
     if (moreRasterDetailAvailable && this->_children.empty()) {
       createQuadtreeSubdividedChildren(*this);
     }
+  } else if (
+      this->getState() == LoadState::Done && !this->_rasterTiles.empty()) {
+    // We can't hang raster images on a tile without geometry, and their
+    // existence can prevent the tile from being deemed done loading. So clear
+    // them out here.
+    this->_rasterTiles.clear();
   }
 }
 

Cesium3DTilesSelection/src/Tileset.cpp

@@ -701,18 +701,21 @@ Tileset::TraversalDetails Tileset::_renderLeaf(
   return traversalDetails;
 }
 
-bool Tileset::_queueLoadOfChildrenRequiredForRefinement(
+bool Tileset::_queueLoadOfChildrenRequiredForForbidHoles(
     const FrameState& frameState,
     Tile& tile,
     const ImplicitTraversalInfo& implicitInfo,
     const std::vector<double>& distances) {
-  if (!this->_options.forbidHoles) {
-    return false;
-  }
+  // This method should only be called in "Forbid Holes" mode.
+  assert(this->_options.forbidHoles);
+
+  bool waitingForChildren = false;
+
   // If we're forbidding holes, don't refine if any children are still loading.
   gsl::span<Tile> children = tile.getChildren();
-  bool waitingForChildren = false;
   for (Tile& child : children) {
+    this->_markTileVisited(child);
+
     if (!child.isRenderable() && !child.isExternalTileset()) {
       waitingForChildren = true;
 
@@ -727,7 +730,6 @@ bool Tileset::_queueLoadOfChildrenRequiredForRefinement(
             childInfo);
       }
       child.update(frameState.lastFrameNumber, frameState.currentFrameNumber);
-      this->_markTileVisited(child);
 
       // We're using the distance to the parent tile to compute the load
       // priority. This is fine because the relative priority of the children is
@@ -738,8 +740,22 @@ bool Tileset::_queueLoadOfChildrenRequiredForRefinement(
           frameState.frustums,
           child,
           distances);
+    } else if (child.getUnconditionallyRefine()) {
+      // This child tile is set to unconditionally refine. That means refining
+      // _to_ it will immediately refine _through_ it. So we need to make sure
+      // its children are renderable, too.
+      // The distances are not correct for the child's children, but once again
+      // we don't care because all tiles must be loaded before we can render any
+      // of them, so their relative priority doesn't matter.
+      ImplicitTraversalInfo childInfo(&child, &implicitInfo);
+      waitingForChildren |= this->_queueLoadOfChildrenRequiredForForbidHoles(
+          frameState,
+          child,
+          childInfo,
+          distances);
     }
   }
+
   return waitingForChildren;
 }
 
@@ -1000,14 +1016,23 @@ Tileset::TraversalDetails Tileset::_visitTile(
 
   const bool unconditionallyRefine = tile.getUnconditionallyRefine();
   const bool meetsSse = _meetsSse(frameState.frustums, tile, distances, culled);
-  const bool waitingForChildren = _queueLoadOfChildrenRequiredForRefinement(
-      frameState,
-      tile,
-      implicitInfo,
-      distances);
 
-  if (!unconditionallyRefine &&
-      (meetsSse || ancestorMeetsSse || waitingForChildren)) {
+  bool wantToRefine = unconditionallyRefine || (!meetsSse && !ancestorMeetsSse);
+
+  // In "Forbid Holes" mode, we cannot refine this tile until all its children
+  // are loaded. But don't queue the children for load until we _want_ to
+  // refine this tile.
+  if (wantToRefine && this->_options.forbidHoles) {
+    const bool waitingForChildren =
+        this->_queueLoadOfChildrenRequiredForForbidHoles(
+            frameState,
+            tile,
+            implicitInfo,
+            distances);
+    wantToRefine = !waitingForChildren;
+  }
+
+  if (!wantToRefine) {
     // This tile (or an ancestor) is the one we want to render this frame, but
     // we'll do different things depending on the state of this tile and on what
     // we did _last_ frame.
@@ -1372,6 +1397,38 @@ static bool anyRasterOverlaysNeedLoading(const Tile& tile) noexcept {
       // state if needed.
       if (tile.getState() == Tile::LoadState::Unloaded) {
         tile.setState(Tile::LoadState::ContentLoaded);
+
+        // 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 as (2).
+        //
+        // For a tile with no parent there's no difference between the
+        // behaviors.
+        double myGeometricError = tile.getNonZeroGeometricError();
+
+        Tile* pAncestor = tile.getParent();
+        while (pAncestor && pAncestor->getUnconditionallyRefine()) {
+          pAncestor = pAncestor->getParent();
+        }
+
+        double parentGeometricError =
+            pAncestor ? pAncestor->getNonZeroGeometricError()
+                      : myGeometricError * 2.0;
+        if (myGeometricError >= parentGeometricError) {
+          tile.setEmptyContent();
+        }
       }
     } else if (shouldLoad) {
       loadQueue.push_back({&tile, highestLoadPriority});