Remove _tilesStillNotUnloadedCount, rename _doNotUnloadCount, review …

…comments

Cesium3DTilesSelection/include/Cesium3DTilesSelection/Tile.h

@@ -25,8 +25,9 @@
 namespace Cesium3DTilesSelection {
 class TilesetContentLoader;
 
+#define CESIUM_DEBUG_TILE_UNLOADING 1
 #ifdef CESIUM_DEBUG_TILE_UNLOADING
-class TileDoNotUnloadCountTracker {
+class TileDoNotUnloadSubtreeCountTracker {
 private:
   struct Entry {
     std::string reason;
@@ -523,7 +524,7 @@ class CESIUM3DTILESSELECTION_API Tile final {
    * This function is not supposed to be called by clients.
    */
   int32_t getDoNotUnloadCount() const noexcept {
-    return this->_doNotUnloadCount;
+    return this->_doNotUnloadSubtreeCount;
   }
 
   /**
@@ -532,47 +533,36 @@ class CESIUM3DTILESSELECTION_API Tile final {
    *
    * This function is not supposed to be called by clients.
    */
-  void incrementDoNotUnloadCount(const char* reason) noexcept;
+  void incrementDoNotUnloadSubtreeCount(const char* reason) noexcept;
 
   /**
    * @brief Decrements the internal count denoting that the tile and its
    * ancestors should not be unloaded.
    *
    * This function is not supposed to be called by clients.
    */
-  void decrementDoNotUnloadCount(const char* reason) noexcept;
+  void decrementDoNotUnloadSubtreeCount(const char* reason) noexcept;
 
   /**
-   * @brief Marks this tile as having content that has not yet been unloaded,
-   * preventing a parent external tileset from cleaning up. This count will be
-   * propagated to any ancestors.
+   * @brief Increments the internal count denoting that the tile and its
+   * ancestors should not be unloaded starting with this tile's parent.
    *
    * This function is not supposed to be called by clients.
    */
-  void incrementTilesStillNotUnloadedCount() noexcept;
+  void incrementDoNotUnloadSubtreeCountOnParent(const char* reason) noexcept;
 
   /**
-   * @brief Unmarks this tile as having content that has not yet been unloaded,
-   * allowing a parent external tileset to clean up. This count will be
-   * propagated to any ancestors.
+   * @brief Decrements the internal count denoting that the tile and its
+   * ancestors should not be unloaded starting with this tile's parent.
    *
    * This function is not supposed to be called by clients.
    */
-  void decrementTilesStillNotUnloadedCount() noexcept;
-
-  /**
-   * @brief Obtains the number of tiles at or below this tile (that is, the tile
-   * itself and its children) that still have content that has not yet been
-   * unloaded, preventing a parent external tileset from cleaning up.
-   */
-  int32_t getTilesStillNotUnloadedCount() const noexcept {
-    return this->_tilesStillNotUnloadedCount;
-  }
+  void decrementDoNotUnloadSubtreeCountOnParent(const char* reason) noexcept;
 
 private:
-  void incrementDoNotUnloadCount(const std::string& reason) noexcept;
+  void incrementDoNotUnloadSubtreeCount(const std::string& reason) noexcept;
 
-  void decrementDoNotUnloadCount(const std::string& reason) noexcept;
+  void decrementDoNotUnloadSubtreeCount(const std::string& reason) noexcept;
 
   struct TileConstructorImpl {};
   template <
@@ -638,9 +628,7 @@ class CESIUM3DTILESSELECTION_API Tile final {
 
   // Number of existing claims on this tile preventing it and its parent
   // external tileset (if any) from being unloaded from the tree.
-  int32_t _doNotUnloadCount = 0;
-
-  int32_t _tilesStillNotUnloadedCount = 0;
+  int32_t _doNotUnloadSubtreeCount = 0;
 
   friend class TilesetContentManager;
   friend class MockTilesetContentManagerTestFixture;

Cesium3DTilesSelection/src/Tile.cpp

@@ -27,22 +27,25 @@ using namespace std::string_literals;
 
 namespace Cesium3DTilesSelection {
 #ifdef CESIUM_DEBUG_TILE_UNLOADING
-std::unordered_map<std::string, std::vector<TileDoNotUnloadCountTracker::Entry>>
-    TileDoNotUnloadCountTracker::_entries;
+std::unordered_map<
+    std::string,
+    std::vector<TileDoNotUnloadSubtreeCountTracker::Entry>>
+    TileDoNotUnloadSubtreeCountTracker::_entries;
 
-void TileDoNotUnloadCountTracker::addEntry(
+void TileDoNotUnloadSubtreeCountTracker::addEntry(
     uint64_t id,
     bool increment,
     const std::string& reason,
     int32_t newCount) {
   const std::string idString = fmt::format("{:x}", id);
-  const auto foundIt = TileDoNotUnloadCountTracker::_entries.find(idString);
-  if (foundIt != TileDoNotUnloadCountTracker::_entries.end()) {
+  const auto foundIt =
+      TileDoNotUnloadSubtreeCountTracker::_entries.find(idString);
+  if (foundIt != TileDoNotUnloadSubtreeCountTracker::_entries.end()) {
     foundIt->second.push_back(Entry{reason, increment, newCount});
   } else {
     std::vector<Entry> entries{Entry{reason, increment, newCount}};
 
-    TileDoNotUnloadCountTracker::_entries.insert(
+    TileDoNotUnloadSubtreeCountTracker::_entries.insert(
         {idString, std::move(entries)});
   }
 }
@@ -92,7 +95,7 @@ Tile::Tile(
       _mightHaveLatentChildren{true} {
   if (_loadState != TileLoadState::Unloaded && !_content.isUnknownContent() &&
       !_content.isEmptyContent()) {
-    incrementTilesStillNotUnloadedCount();
+    incrementDoNotUnloadSubtreeCountOnParent("Tile constructor");
   }
 }
 
@@ -111,8 +114,7 @@ Tile::Tile(Tile&& rhs) noexcept
       _content(std::move(rhs._content)),
       _pLoader{rhs._pLoader},
       _loadState{rhs._loadState},
-      _mightHaveLatentChildren{rhs._mightHaveLatentChildren},
-      _tilesStillNotUnloadedCount{rhs._tilesStillNotUnloadedCount} {
+      _mightHaveLatentChildren{rhs._mightHaveLatentChildren} {
   // since children of rhs will have the parent pointed to rhs,
   // we will reparent them to this tile as rhs will be destroyed after this
   for (Tile& tile : this->_children) {
@@ -144,11 +146,10 @@ Tile& Tile::operator=(Tile&& rhs) noexcept {
     this->_pLoader = rhs._pLoader;
     this->_loadState = rhs._loadState;
     this->_mightHaveLatentChildren = rhs._mightHaveLatentChildren;
-    this->_tilesStillNotUnloadedCount = rhs._tilesStillNotUnloadedCount;
-    // We deliberately do *not* copy the _doNotUnloadCount of rhs here.
-    // This is because the _doNotUnloadCount is a count of instances of the
-    // *pointer* to the tile, denoting the number of active pointers that would
-    // be invalidated if the Tile were to be deleted. Because the memory
+    // We deliberately do *not* copy the _doNotUnloadSubtreeCount of rhs here.
+    // This is because the _doNotUnloadSubtreeCount is a count of instances of
+    // the *pointer* to the tile, denoting the number of active pointers that
+    // would be invalidated if the Tile were to be deleted. Because the memory
     // location of the tile will have changed as a result of the move operation,
     // the new Tile object will not have any pointers referencing it, so copying
     // over the count would be incorrect and could result in a Tile not being
@@ -163,19 +164,9 @@ void Tile::createChildTiles(std::vector<Tile>&& children) {
     throw std::runtime_error("Children already created.");
   }
 
-  int32_t prevLoadedContentsCount = this->_tilesStillNotUnloadedCount;
   this->_children = std::move(children);
   for (Tile& tile : this->_children) {
     tile.setParent(this);
-    this->_tilesStillNotUnloadedCount += tile._tilesStillNotUnloadedCount;
-  }
-
-  // Propagate new loaded contents count up the chain
-  Tile* pParent = this->_pParent;
-  while (pParent != nullptr) {
-    pParent->_tilesStillNotUnloadedCount -= prevLoadedContentsCount;
-    pParent->_tilesStillNotUnloadedCount += this->_tilesStillNotUnloadedCount;
-    pParent = pParent->_pParent;
   }
 }
 
@@ -295,80 +286,80 @@ void Tile::setMightHaveLatentChildren(bool mightHaveLatentChildren) noexcept {
 }
 
 void Tile::clearChildren() noexcept {
-  CESIUM_ASSERT(this->_doNotUnloadCount == 0);
+  CESIUM_ASSERT(this->_doNotUnloadSubtreeCount == 0);
   this->_children.clear();
 }
 
-void Tile::incrementDoNotUnloadCount(
+void Tile::incrementDoNotUnloadSubtreeCount(
     [[maybe_unused]] const char* reason) noexcept {
 #ifdef CESIUM_DEBUG_TILE_UNLOADING
   const std::string reasonStr = fmt::format(
       "Initiator ID: {:x}, {}",
       reinterpret_cast<uint64_t>(this),
       reason);
-  this->incrementDoNotUnloadCount(reasonStr);
+  this->incrementDoNotUnloadSubtreeCount(reasonStr);
 #else
-  this->incrementDoNotUnloadCount(std::string());
+  this->incrementDoNotUnloadSubtreeCount(std::string());
 #endif
 }
 
-void Tile::decrementDoNotUnloadCount(
+void Tile::decrementDoNotUnloadSubtreeCount(
     [[maybe_unused]] const char* reason) noexcept {
 #ifdef CESIUM_DEBUG_TILE_UNLOADING
   const std::string reasonStr = fmt::format(
       "Initiator ID: {:x}, {}",
       reinterpret_cast<uint64_t>(this),
       reason);
-  this->decrementDoNotUnloadCount(reasonStr);
+  this->decrementDoNotUnloadSubtreeCount(reasonStr);
 #else
-  this->decrementDoNotUnloadCount(std::string());
+  this->decrementDoNotUnloadSubtreeCount(std::string());
 #endif
 }
 
-void Tile::incrementDoNotUnloadCount(
-    [[maybe_unused]] const std::string& reason) noexcept {
-  ++this->_doNotUnloadCount;
-#ifdef CESIUM_DEBUG_TILE_UNLOADING
-  TileDoNotUnloadCountTracker::addEntry(
-      reinterpret_cast<uint64_t>(this),
-      true,
-      std::string(reason),
-      this->_doNotUnloadCount);
-#endif
+void Tile::incrementDoNotUnloadSubtreeCountOnParent(
+    const char* reason) noexcept {
   if (this->getParent() != nullptr) {
-    this->getParent()->incrementDoNotUnloadCount(reason);
+    this->getParent()->incrementDoNotUnloadSubtreeCount(reason);
   }
 }
 
-void Tile::decrementDoNotUnloadCount(
-    [[maybe_unused]] const std::string& reason) noexcept {
-  CESIUM_ASSERT(this->_doNotUnloadCount > 0);
-  --this->_doNotUnloadCount;
-#ifdef CESIUM_DEBUG_TILE_UNLOADING
-  TileDoNotUnloadCountTracker::addEntry(
-      reinterpret_cast<uint64_t>(this),
-      false,
-      std::string(reason),
-      this->_doNotUnloadCount);
-#endif
+void Tile::decrementDoNotUnloadSubtreeCountOnParent(
+    const char* reason) noexcept {
   if (this->getParent() != nullptr) {
-    this->getParent()->decrementDoNotUnloadCount(reason);
+    this->getParent()->decrementDoNotUnloadSubtreeCount(reason);
   }
 }
 
-void Tile::incrementTilesStillNotUnloadedCount() noexcept {
-  CESIUM_ASSERT(this->_tilesStillNotUnloadedCount >= 0);
-  this->_tilesStillNotUnloadedCount++;
-  if (this->getParent() != nullptr) {
-    this->getParent()->incrementTilesStillNotUnloadedCount();
+void Tile::incrementDoNotUnloadSubtreeCount(
+    [[maybe_unused]] const std::string& reason) noexcept {
+  Tile* pTile = this;
+  while (pTile != nullptr) {
+    ++pTile->_doNotUnloadSubtreeCount;
+#ifdef CESIUM_DEBUG_TILE_UNLOADING
+    TileDoNotUnloadSubtreeCountTracker::addEntry(
+        reinterpret_cast<uint64_t>(pTile),
+        true,
+        std::string(reason),
+        pTile->_doNotUnloadSubtreeCount);
+#endif
+    pTile = pTile->getParent();
   }
 }
 
-void Tile::decrementTilesStillNotUnloadedCount() noexcept {
-  CESIUM_ASSERT(this->_tilesStillNotUnloadedCount > 0);
-  --this->_tilesStillNotUnloadedCount;
-  if (this->getParent() != nullptr) {
-    this->getParent()->decrementTilesStillNotUnloadedCount();
+void Tile::decrementDoNotUnloadSubtreeCount(
+    [[maybe_unused]] const std::string& reason) noexcept {
+  CESIUM_ASSERT(this->_doNotUnloadSubtreeCount > 0);
+  Tile* pTile = this;
+  while (pTile != nullptr) {
+    --pTile->_doNotUnloadSubtreeCount;
+#ifdef CESIUM_DEBUG_TILE_UNLOADING
+    TileDoNotUnloadSubtreeCountTracker::addEntry(
+        reinterpret_cast<uint64_t>(pTile),
+        false,
+        std::string(reason),
+        pTile->_doNotUnloadSubtreeCount);
+#endif
+    pTile = pTile->getParent();
   }
 }
 

Cesium3DTilesSelection/src/Tileset.cpp

@@ -252,7 +252,7 @@ void Tileset::_updateLodTransitions(
         // This tile is done fading out and was immediately kicked from the
         // cache.
         tileIt = result.tilesFadingOut.erase(tileIt);
-        (*tileIt)->decrementDoNotUnloadCount(
+        (*tileIt)->decrementDoNotUnloadSubtreeCount(
             "Tileset::_updateLodTransitions done fading out");
         continue;
       }
@@ -265,7 +265,7 @@ void Tileset::_updateLodTransitions(
         // This tile will already be on the render list.
         pRenderContent->setLodTransitionFadePercentage(0.0f);
         tileIt = result.tilesFadingOut.erase(tileIt);
-        (*tileIt)->decrementDoNotUnloadCount(
+        (*tileIt)->decrementDoNotUnloadSubtreeCount(
             "Tileset::_updateLodTransitions in render list");
         continue;
       }
@@ -278,7 +278,7 @@ void Tileset::_updateLodTransitions(
         // last frame.
         pRenderContent->setLodTransitionFadePercentage(0.0f);
         tileIt = result.tilesFadingOut.erase(tileIt);
-        (*tileIt)->decrementDoNotUnloadCount(
+        (*tileIt)->decrementDoNotUnloadSubtreeCount(
             "Tileset::_updateLodTransitions done fading out");
         continue;
       }
@@ -330,7 +330,7 @@ Tileset::updateViewOffline(const std::vector<ViewState>& frustums) {
   }
 
   for (Tile* pTile : this->_updateResult.tilesFadingOut) {
-    pTile->decrementDoNotUnloadCount(
+    pTile->decrementDoNotUnloadSubtreeCount(
         "Tileset::updateViewOffline clear tilesFadingOut");
   }
 
@@ -346,7 +346,7 @@ Tileset::updateViewOffline(const std::vector<ViewState>& frustums) {
       if (pRenderContent) {
         pRenderContent->setLodTransitionFadePercentage(1.0f);
         this->_updateResult.tilesFadingOut.insert(tile);
-        tile->incrementDoNotUnloadCount(
+        tile->incrementDoNotUnloadSubtreeCount(
             "Tileset::updateViewOffline start fading out");
       }
     }
@@ -382,7 +382,7 @@ Tileset::updateView(const std::vector<ViewState>& frustums, float deltaTime) {
 
   if (!_options.enableLodTransitionPeriod) {
     for (Tile* pTile : this->_updateResult.tilesFadingOut) {
-      pTile->decrementDoNotUnloadCount(
+      pTile->decrementDoNotUnloadSubtreeCount(
           "Tileset::updateView clear tilesFadingOut");
     }
     result.tilesFadingOut.clear();
@@ -647,7 +647,7 @@ void markTileNonRendered(
       (lastResult == TileSelectionState::Result::Refined &&
        tile.getRefine() == TileRefine::Add)) {
     result.tilesFadingOut.insert(&tile);
-    tile.incrementDoNotUnloadCount("markTileNonRendered fading out");
+    tile.incrementDoNotUnloadSubtreeCount("markTileNonRendered fading out");
     TileRenderContent* pRenderContent = tile.getContent().getRenderContent();
     if (pRenderContent) {
       pRenderContent->setLodTransitionFadePercentage(0.0f);
@@ -1650,7 +1650,6 @@ void Tileset::_clearChildrenRecursively(Tile* pTile) noexcept {
     }
     CESIUM_ASSERT(child.getState() == TileLoadState::Unloaded);
     CESIUM_ASSERT(child.getDoNotUnloadCount() == 0);
-    CESIUM_ASSERT(child.getTilesStillNotUnloadedCount() == 0);
     CESIUM_ASSERT(child.getContent().isUnknownContent());
     this->_loadedTiles.remove(child);
     _clearChildrenRecursively(&child);
@@ -1714,12 +1713,8 @@ void Tileset::_unloadCachedTiles(double timeBudget) noexcept {
   }
 
   if (!tilesNeedingChildrenCleared.empty()) {
-    // Because we iterated over the tiles list backwards, the
-    // `tilesNeedingChildrenCleared` vector is in order from bottom to top of
-    // the tree.
     for (Tile* pTileToClear : tilesNeedingChildrenCleared) {
       CESIUM_ASSERT(pTileToClear->getDoNotUnloadCount() == 0);
-      CESIUM_ASSERT(pTileToClear->getTilesStillNotUnloadedCount() == 0);
       _clearChildrenRecursively(pTileToClear);
     }
   }