Merge 612352eee9 into 3e68c7a1c2

Co-authored-by: Janine Liu <32226860+j9liu@users.noreply.github.com>

CHANGES.md

@@ -7,7 +7,6 @@
 - `RasterOverlay::createTileProvider` now receives a reference to `CreateRasterOverlayTileProviderParameters` instead of a large number of individual parameters.
 - The constructor parameters for `RasterOverlayTileProvider` and `QuadtreeRasterOverlayTileProvider` have changed.
 - The `getCredit` method has been removed from `RasterOverlayCreditProvider`. Use `getCredits` instead.
-- Removed unused property `RasterOverlayOptions::subTileCacheBytes`.
 
 ##### Additions :tada:
 

CesiumRasterOverlays/include/CesiumRasterOverlays/CreateRasterOverlayTileProviderParameters.h

@@ -22,7 +22,8 @@ struct CESIUMRASTEROVERLAYS_API CreateRasterOverlayTileProviderParameters {
   RasterOverlayExternals externals;
 
   /**
-   * @brief The overlay that owns this overlay.
+   * @brief The overlay that owns the overlay that is creating the tile
+   * provider.
    *
    * If nullptr, this overlay is not aggregated, and the owner of the tile
    * provider is the overlay that created it.

CesiumUtility/include/CesiumUtility/CreditReferencer.h

@@ -109,7 +109,7 @@ public:
   /**
    * @brief Tests if a credit is referenced by this referencer.
    *
-   * @param credit to test if it is referenced.
+   * @param credit The credit to test if it is referenced.
    */
   bool isCreditReferenced(Credit credit) const noexcept;
 

CesiumUtility/include/CesiumUtility/CreditSystem.h

@@ -180,18 +180,7 @@ public:
       std::string&& html,
       bool showOnScreen = false);
 
-  /**
-   * @brief Inserts a credit string.
-   *
-   * @param source The source of the credit. This should be an instance created
-   * and owned by a tileset, raster overlay, or other data source.
-   * @param html The HTML string for the credit.
-   * @param showOnScreen Whether or not the credit should be shown on screen.
-   * Credits not shown on the screen should be shown in a separate popup window.
-   * @return If this string already exists from the same source, returns a
-   * Credit handle to the existing entry. Otherwise returns a Credit handle to a
-   * new entry.
-   */
+  /** @copydoc createCredit */
   Credit createCredit(
       const CreditSource& source,
       const std::string& html,

CesiumUtility/src/CreditSystem.cpp

@@ -97,11 +97,19 @@ Credit CreditSystem::createCredit(
   record.pSource = &source;
 
   if (lastCreditWithSameHtml != INVALID_CREDIT_INDEX) {
-    record.previousCreditWithSameHtml = lastCreditWithSameHtml;
-    record.nextCreditWithSameHtml = INVALID_CREDIT_INDEX;
+    // Even though `lastCreditWithSameHtml` is the credit with the same HTML
+    // that has the largest index, it is not necessarily last in the linked
+    // list.
     CreditRecord& lastRecord = this->_credits[size_t(lastCreditWithSameHtml)];
-    CESIUM_ASSERT(lastRecord.nextCreditWithSameHtml == INVALID_CREDIT_INDEX);
+    record.previousCreditWithSameHtml = lastCreditWithSameHtml;
+    record.nextCreditWithSameHtml = lastRecord.nextCreditWithSameHtml;
     lastRecord.nextCreditWithSameHtml = uint32_t(creditIndex);
+
+    if (record.nextCreditWithSameHtml != INVALID_CREDIT_INDEX) {
+      CreditRecord& nextRecord =
+          this->_credits[size_t(record.nextCreditWithSameHtml)];
+      nextRecord.previousCreditWithSameHtml = uint32_t(creditIndex);
+    }
   }
 
   return Credit(uint32_t(creditIndex), record.generation);
@@ -292,18 +300,18 @@ void CreditSystem::destroyCreditSource(CreditSource& creditSource) noexcept {
             this->_credits[record.nextCreditWithSameHtml];
         nextRecord.previousCreditWithSameHtml =
             record.previousCreditWithSameHtml;
-        record.nextCreditWithSameHtml = INVALID_CREDIT_INDEX;
       }
 
       if (record.previousCreditWithSameHtml != INVALID_CREDIT_INDEX) {
         CreditRecord& previousRecord =
             this->_credits[record.previousCreditWithSameHtml];
         previousRecord.nextCreditWithSameHtml = record.nextCreditWithSameHtml;
-        record.previousCreditWithSameHtml = INVALID_CREDIT_INDEX;
       }
 
       record.referenceCount = 0;
       record.shownLastSnapshot = false;
+      record.nextCreditWithSameHtml = INVALID_CREDIT_INDEX;
+      record.previousCreditWithSameHtml = INVALID_CREDIT_INDEX;
     }
   }
 
@@ -327,9 +335,10 @@ uint32_t CreditSystem::filterCreditForSnapshot(
       const CreditRecord& otherRecord =
           this->_credits[size_t(currentCreditIndex)];
 
-      // If the other credit has the same showOnScreen value, filter this one
-      // out in favor of it.
-      if (otherRecord.showOnScreen == record.showOnScreen) {
+      // If the other credit is referenced and has the same showOnScreen value,
+      // filter this one out in favor of it.
+      if (otherRecord.referenceCount > 0 &&
+          otherRecord.showOnScreen == record.showOnScreen) {
         return currentCreditIndex;
       }
 
@@ -340,32 +349,35 @@ uint32_t CreditSystem::filterCreditForSnapshot(
   } else {
     CESIUM_ASSERT(filteringMode == CreditFilteringMode::UniqueHtml);
 
-    // In this filtering mode, we need to find the first credit in the linked
-    // list with showOnScreen=true. Or if they're all showOnScreen=false, return
-    // the first one. Unlike `UniqueHtmlAndShowOnScreen`, the Credit we want may
-    // occur after the current one.
+    // In this filtering mode, we need to find the first referenced credit in
+    // the linked list with showOnScreen=true. Or if they're all
+    // showOnScreen=false, return the first one. Unlike
+    // `UniqueHtmlAndShowOnScreen`, the Credit we want may occur after the
+    // current one.
 
-    // Walk backwards to find the first credit in the linked list.
+    // Walk backwards to find the first referenced credit in the linked list.
     uint32_t previousCreditIndex = uint32_t(&record - this->_credits.data());
-    uint32_t firstCreditIndex;
+    uint32_t firstReferencedCreditIndex = INVALID_CREDIT_INDEX;
     do {
-      firstCreditIndex = previousCreditIndex;
       const CreditRecord& otherRecord =
-          this->_credits[size_t(firstCreditIndex)];
+          this->_credits[size_t(previousCreditIndex)];
+      if (otherRecord.referenceCount > 0) {
+        firstReferencedCreditIndex = previousCreditIndex;
+      }
       previousCreditIndex = otherRecord.previousCreditWithSameHtml;
     } while (previousCreditIndex != INVALID_CREDIT_INDEX);
 
-    // Walk forward from the first credit to find one with showOnScreen=true (if
-    // any).
-    uint32_t currentCreditIndex = firstCreditIndex;
+    // Walk forward from the first referenced credit to find one with
+    // showOnScreen=true (if any).
+    uint32_t currentCreditIndex = firstReferencedCreditIndex;
     while (currentCreditIndex != INVALID_CREDIT_INDEX) {
       const CreditRecord& otherRecord =
           this->_credits[size_t(currentCreditIndex)];
 
-      if (otherRecord.showOnScreen) {
-        // Found the first credit with showOnScreen=true. Filter this credit out
-        // in favor of it. Unless the currentCreditIndex points to the same
-        // credit we started with!
+      if (otherRecord.showOnScreen && otherRecord.referenceCount > 0) {
+        // Found the first referenced credit with showOnScreen=true. Filter this
+        // credit out in favor of it. Unless the currentCreditIndex points to
+        // the same credit we started with!
         return currentCreditIndex == uint32_t(&record - this->_credits.data())
                    ? CreditSystem::INVALID_CREDIT_INDEX
                    : currentCreditIndex;
@@ -375,11 +387,13 @@ uint32_t CreditSystem::filterCreditForSnapshot(
     }
 
     // Reached the end of the linked list without finding any credit with
-    // showOnScreen=true. So return the first credit in the list. Unless the
-    // firstCreditIndex points to the same credit we started with!
-    return firstCreditIndex == uint32_t(&record - this->_credits.data())
+    // showOnScreen=true. So return the first referenced credit in the list.
+    // Unless the firstReferencedCreditIndex points to the same credit we
+    // started with!
+    return firstReferencedCreditIndex ==
+                   uint32_t(&record - this->_credits.data())
                ? CreditSystem::INVALID_CREDIT_INDEX
-               : firstCreditIndex;
+               : firstReferencedCreditIndex;
   }
 }
 

CesiumUtility/test/TestCreditSystem.cpp

@@ -253,12 +253,12 @@ TEST_CASE("Test CreditSystem with CreditSources") {
     CHECK(snapshot0.currentCredits.size() == 1);
     CHECK(snapshot0.currentCredits[0] == credit0);
 
-    // Remove the credit reference, which will add it to the list of "no longer
-    // shown" credits.
+    // Remove the credit reference. If we were to ask for a snapshot after this,
+    // this credit would show up in the `removedCredits`.
     creditSystem.removeCreditReference(credit0);
 
-    // Destroy the source.
-    // The credit should no longer be reported in the next snapshot.
+    // Destroy the source. Now, the removed credit should no longer be reported
+    // in the next snapshot.
     pTempSourceA.reset();
 
     const CreditsSnapshot& snapshot1 = creditSystem.getSnapshot();
@@ -371,6 +371,26 @@ TEST_CASE("Test CreditSystem with CreditSources") {
         CHECK(snapshot.removedCredits.empty());
       }
 
+      SUBCASE("does not filter in favor of unreferenced credits") {
+        CreditSource sourceC(creditSystem);
+        [[maybe_unused]] Credit credit0 =
+            creditSystem.createCredit(sourceA, html0, true);
+        Credit credit1 = creditSystem.createCredit(sourceB, html0, true);
+        Credit credit2 = creditSystem.createCredit(sourceC, html0, false);
+
+        creditSystem.addCreditReference(credit1);
+        creditSystem.addCreditReference(credit2);
+        // Note: credit0 is not referenced.
+
+        const CreditsSnapshot& snapshot = creditSystem.getSnapshot(
+            CreditFilteringMode::UniqueHtmlAndShowOnScreen);
+
+        REQUIRE(snapshot.currentCredits.size() == 2);
+        CHECK(snapshot.currentCredits[0] == credit1);
+        CHECK(snapshot.currentCredits[1] == credit2);
+        CHECK(snapshot.removedCredits.empty());
+      }
+
       SUBCASE("reference count is the sum of collapsed credits") {
         CreditSource sourceC(creditSystem);
         Credit credit0 = creditSystem.createCredit(sourceA, html0, false);
@@ -452,6 +472,25 @@ TEST_CASE("Test CreditSystem with CreditSources") {
         CHECK(snapshot.removedCredits.empty());
       }
 
+      SUBCASE("does not filter in favor of unreferenced credits") {
+        CreditSource sourceC(creditSystem);
+        Credit credit0 = creditSystem.createCredit(sourceA, html0, false);
+        Credit credit1 = creditSystem.createCredit(sourceB, html0, false);
+        [[maybe_unused]] Credit credit2 =
+            creditSystem.createCredit(sourceC, html0, true);
+
+        creditSystem.addCreditReference(credit0);
+        creditSystem.addCreditReference(credit1);
+        // Note: credit2 is not referenced.
+
+        const CreditsSnapshot& snapshot =
+            creditSystem.getSnapshot(CreditFilteringMode::UniqueHtml);
+
+        REQUIRE(snapshot.currentCredits.size() == 1);
+        CHECK(snapshot.currentCredits[0] == credit0);
+        CHECK(snapshot.removedCredits.empty());
+      }
+
       SUBCASE("reference count is the sum of collapsed credits") {
         CreditSource sourceC(creditSystem);
         Credit credit0 = creditSystem.createCredit(sourceA, html0);
@@ -478,6 +517,6 @@ TEST_CASE("Test CreditSystem with CreditSources") {
     }
   }
 
-  // Refeference count for sorting is the sum of collapsed credit reference
+  // Reference count for sorting is the sum of collapsed credit reference
   // counts.
 }