Skip to content

Commit

Permalink
Merge pull request #964 from CesiumGS/shared-assets-lifetime
Browse files Browse the repository at this point in the history
Fix asset lifetime, avoid circular reference counting.
  • Loading branch information
azrogers authored Oct 10, 2024
2 parents 924b000 + a230812 commit 3e5eb71
Show file tree
Hide file tree
Showing 47 changed files with 476 additions and 402 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ struct Rectangle;
}

namespace CesiumGltf {
struct ImageCesium;
struct ImageAsset;
struct Model;
} // namespace CesiumGltf

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -184,10 +184,11 @@ class CESIUM3DTILESSELECTION_API Tileset final {
/**
* @brief Returns the {@link SharedAssetDepot} of this tileset.
*/
CesiumGltf::SharedAssetSystem& getSharedAssetSystem() noexcept;
CesiumGltfReader::GltfSharedAssetSystem& getSharedAssetSystem() noexcept;

/** @copydoc Tileset::getSharedAssetSystem() */
const CesiumGltf::SharedAssetSystem& getSharedAssetSystem() const noexcept;
const CesiumGltfReader::GltfSharedAssetSystem&
getSharedAssetSystem() const noexcept;

/**
* @brief Updates this view but waits for all tiles that meet sse to finish
Expand Down
4 changes: 2 additions & 2 deletions Cesium3DTilesSelection/src/TileContentLoadInfo.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@ TileContentLoadInfo::TileContentLoadInfo(
const std::shared_ptr<IPrepareRendererResources>&
pPrepareRendererResources_,
const std::shared_ptr<spdlog::logger>& pLogger_,
const CesiumUtility::IntrusivePointer<CesiumGltf::SharedAssetSystem>
pAssetDepot_,
const CesiumUtility::IntrusivePointer<
CesiumGltfReader::GltfSharedAssetSystem> pAssetDepot_,
const TilesetContentOptions& contentOptions_,
const Tile& tile)
: asyncSystem(asyncSystem_),
Expand Down
9 changes: 5 additions & 4 deletions Cesium3DTilesSelection/src/TileContentLoadInfo.h
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
#include <CesiumAsync/AsyncSystem.h>
#include <CesiumAsync/IAssetAccessor.h>
#include <CesiumGeometry/Axis.h>
#include <CesiumGltf/SharedAssetSystem.h>
#include <CesiumGltfReader/GltfSharedAssetSystem.h>

#include <gsl/span>
#include <spdlog/fwd.h>
Expand All @@ -25,8 +25,8 @@ struct TileContentLoadInfo {
const std::shared_ptr<IPrepareRendererResources>&
pPrepareRendererResources,
const std::shared_ptr<spdlog::logger>& pLogger,
const CesiumUtility::IntrusivePointer<CesiumGltf::SharedAssetSystem>
maybeAssetDepot,
const CesiumUtility::IntrusivePointer<
CesiumGltfReader::GltfSharedAssetSystem> maybeAssetDepot,
const TilesetContentOptions& contentOptions,
const Tile& tile);

Expand All @@ -43,7 +43,8 @@ struct TileContentLoadInfo {
BoundingVolume tileBoundingVolume;

std::optional<BoundingVolume> tileContentBoundingVolume;
CesiumUtility::IntrusivePointer<CesiumGltf::SharedAssetSystem> pAssetDepot;
CesiumUtility::IntrusivePointer<CesiumGltfReader::GltfSharedAssetSystem>
pAssetDepot;

TileRefine tileRefine;

Expand Down
5 changes: 3 additions & 2 deletions Cesium3DTilesSelection/src/Tileset.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -149,11 +149,12 @@ const RasterOverlayCollection& Tileset::getOverlays() const noexcept {
return this->_pTilesetContentManager->getRasterOverlayCollection();
}

CesiumGltf::SharedAssetSystem& Tileset::getSharedAssetSystem() noexcept {
CesiumGltfReader::GltfSharedAssetSystem&
Tileset::getSharedAssetSystem() noexcept {
return *this->_pTilesetContentManager->getSharedAssetSystem();
}

const CesiumGltf::SharedAssetSystem&
const CesiumGltfReader::GltfSharedAssetSystem&
Tileset::getSharedAssetSystem() const noexcept {
return *this->_pTilesetContentManager->getSharedAssetSystem();
}
Expand Down
12 changes: 6 additions & 6 deletions Cesium3DTilesSelection/src/TilesetContentManager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -666,7 +666,7 @@ TilesetContentManager::TilesetContentManager(
_tileLoadsInProgress{0},
_loadedTilesCount{0},
_tilesDataUsed{0},
_pAssetDepot(new CesiumGltf::SharedAssetSystem()),
_pSharedAssets(CesiumGltfReader::GltfSharedAssetSystem::getDefault()),
_destructionCompletePromise{externals.asyncSystem.createPromise<void>()},
_destructionCompleteFuture{
this->_destructionCompletePromise.getFuture().share()},
Expand Down Expand Up @@ -696,7 +696,7 @@ TilesetContentManager::TilesetContentManager(
_tileLoadsInProgress{0},
_loadedTilesCount{0},
_tilesDataUsed{0},
_pAssetDepot(new CesiumGltf::SharedAssetSystem()),
_pSharedAssets(CesiumGltfReader::GltfSharedAssetSystem::getDefault()),
_destructionCompletePromise{externals.asyncSystem.createPromise<void>()},
_destructionCompleteFuture{
this->_destructionCompletePromise.getFuture().share()},
Expand Down Expand Up @@ -848,7 +848,7 @@ TilesetContentManager::TilesetContentManager(
_tileLoadsInProgress{0},
_loadedTilesCount{0},
_tilesDataUsed{0},
_pAssetDepot(new CesiumGltf::SharedAssetSystem()),
_pSharedAssets(CesiumGltfReader::GltfSharedAssetSystem::getDefault()),
_destructionCompletePromise{externals.asyncSystem.createPromise<void>()},
_destructionCompleteFuture{
this->_destructionCompletePromise.getFuture().share()},
Expand Down Expand Up @@ -994,7 +994,7 @@ void TilesetContentManager::loadTileContent(
this->_externals.pAssetAccessor,
this->_externals.pPrepareRendererResources,
this->_externals.pLogger,
this->_pAssetDepot,
this->_pSharedAssets,
tilesetOptions.contentOptions,
tile};

Expand Down Expand Up @@ -1237,9 +1237,9 @@ TilesetContentManager::getTilesetCredits() const noexcept {
return this->_tilesetCredits;
}

const CesiumUtility::IntrusivePointer<CesiumGltf::SharedAssetSystem>&
const CesiumUtility::IntrusivePointer<CesiumGltfReader::GltfSharedAssetSystem>&
TilesetContentManager::getSharedAssetSystem() const noexcept {
return this->_pAssetDepot;
return this->_pSharedAssets;
}

int32_t TilesetContentManager::getNumberOfTilesLoading() const noexcept {
Expand Down
7 changes: 4 additions & 3 deletions Cesium3DTilesSelection/src/TilesetContentManager.h
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
#include <Cesium3DTilesSelection/TilesetLoadFailureDetails.h>
#include <Cesium3DTilesSelection/TilesetOptions.h>
#include <CesiumAsync/IAssetAccessor.h>
#include <CesiumGltf/SharedAssetDepot.h>
#include <CesiumUtility/CreditSystem.h>
#include <CesiumUtility/ReferenceCounted.h>

Expand Down Expand Up @@ -116,7 +115,8 @@ class TilesetContentManager

const std::vector<CesiumUtility::Credit>& getTilesetCredits() const noexcept;

const CesiumUtility::IntrusivePointer<CesiumGltf::SharedAssetSystem>&
const CesiumUtility::IntrusivePointer<
CesiumGltfReader::GltfSharedAssetSystem>&
getSharedAssetSystem() const noexcept;

int32_t getNumberOfTilesLoading() const noexcept;
Expand Down Expand Up @@ -172,7 +172,8 @@ class TilesetContentManager
int64_t _tilesDataUsed;

// Stores assets that might be shared between tiles.
CesiumUtility::IntrusivePointer<CesiumGltf::SharedAssetSystem> _pAssetDepot;
CesiumUtility::IntrusivePointer<CesiumGltfReader::GltfSharedAssetSystem>
_pSharedAssets;

CesiumAsync::Promise<void> _destructionCompletePromise;
CesiumAsync::SharedFuture<void> _destructionCompleteFuture;
Expand Down
5 changes: 3 additions & 2 deletions Cesium3DTilesSelection/src/TilesetJsonLoader.h
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
#include <Cesium3DTilesSelection/TilesetExternals.h>
#include <CesiumAsync/Future.h>
#include <CesiumAsync/IAssetAccessor.h>
#include <CesiumGltf/SharedAssetSystem.h>
#include <CesiumGltfReader/GltfSharedAssetSystem.h>

#include <rapidjson/fwd.h>

Expand Down Expand Up @@ -56,7 +56,8 @@ class TilesetJsonLoader : public TilesetContentLoader {
private:
std::string _baseUrl;
CesiumGeospatial::Ellipsoid _ellipsoid;
CesiumUtility::IntrusivePointer<CesiumGltf::SharedAssetSystem> _pAssetDepot;
CesiumUtility::IntrusivePointer<CesiumGltfReader::GltfSharedAssetSystem>
_pSharedAssets;

/**
* @brief The axis that was declared as the "up-axis" for glTF content.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ class SimplePrepareRendererResource
}

virtual void* prepareRasterInLoadThread(
CesiumGltf::ImageCesium& /*image*/,
CesiumGltf::ImageAsset& /*image*/,
const std::any& /*rendererOptions*/) override {
return new AllocationResult{totalAllocation};
}
Expand Down
6 changes: 3 additions & 3 deletions Cesium3DTilesSelection/test/TestTilesetContentManager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1298,8 +1298,8 @@ TEST_CASE("Test the tileset content manager's post processing for gltf") {

CesiumAsync::Future<LoadedRasterOverlayImage>
loadTileImage(RasterOverlayTile& overlayTile) override {
CesiumUtility::IntrusivePointer<CesiumGltf::ImageCesium> pImage;
CesiumGltf::ImageCesium& image = pImage.emplace();
CesiumUtility::IntrusivePointer<CesiumGltf::ImageAsset> pImage;
CesiumGltf::ImageAsset& image = pImage.emplace();
image.width = 1;
image.height = 1;
image.channels = 1;
Expand Down Expand Up @@ -1682,7 +1682,7 @@ TEST_CASE("Test the tileset content manager's post processing for gltf") {
CHECK(images.size() == 1);
}

CHECK(pManager->getSharedAssetSystem()->image().getDistinctCount() == 2);
CHECK(pManager->getSharedAssetSystem()->pImage->getDistinctCount() == 2);

// unload the tile content
for (auto& child : containerTile.getChildren()) {
Expand Down
152 changes: 152 additions & 0 deletions CesiumAsync/include/CesiumAsync/SharedAsset.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,152 @@
#pragma once

#include <CesiumAsync/SharedAssetDepot.h>
#include <CesiumUtility/ExtensibleObject.h>
#include <CesiumUtility/IntrusivePointer.h>

#include <atomic>

namespace CesiumAsync {

/**
* @brief An asset that is potentially shared between multiple objects, such as
* an image shared between multiple glTF models. This is intended to be the base
* class for such assets.
*
* The lifetime of instances of this class should be managed by reference
* counting with {@link IntrusivePointer}.
*
* @tparam T The type that is _deriving_ from this class. For example, you
* should declare your class as
* `class MyClass : public SharedAsset<MyClass> { ... };`
*
* @remarks @parblock
* A `SharedAsset` can be in one of three possible states:
*
* **Independent Asset**
* An independent asset isn't affiliated with an asset depot at all.
* Its lifetime is controlled exclusively by IntrusivePointer / reference
* counting. When the asset's reference count goes to zero, it deletes itself.
* An independent asset's `_pDepot` is nullptr.
*
* **Active Depot Asset**
* This is an asset that is owned by an asset depot and that is in use, meaning
* it has a reference count greater than zero. The asset depot owns the asset
* via an `std::unique_ptr`, not via adding to the reference count. So when the
* reference count goes to zero, only the asset depot itself still has a
* reference to it, so it becomes an inactive depot asset.
*
* **Inactive Depot Asset**
* This is also an asset that is owned by the asset depot, but there are no
* other references to it (it has a reference count of zero). It is found in the
* asset depot's `deletionCandidates` list. When a reference to it is added, it
* is removed from `deletionCandidates` and it becomes an active depot asset.
* @endparblock
*/
template <typename T>
class CESIUMASYNC_API SharedAsset : public CesiumUtility::ExtensibleObject {
public:
/**
* Assets can be copied, but the fresh instance has no references and is not
* in the asset depot.
*/
SharedAsset(const SharedAsset& rhs)
: ExtensibleObject(rhs),
_referenceCount(0),
_pDepot(nullptr),
_uniqueAssetId() {}

/**
* After a move construction, the content of the asset is moved to the new
* instance, but the asset depot still references the old instance.
*/
SharedAsset(SharedAsset&& rhs)
: ExtensibleObject(std::move(rhs)),
_referenceCount(0),
_pDepot(nullptr),
_uniqueAssetId() {}

/**
* Assignment does not affect the asset's relationship with the depot, but is
* useful to assign the data in derived classes.
*/
SharedAsset& operator=(const SharedAsset& rhs) {
CesiumUtility::ExtensibleObject::operator=(rhs);
return *this;
}

SharedAsset& operator=(SharedAsset&& rhs) {
CesiumUtility::ExtensibleObject::operator=(std::move(rhs));
return *this;
}

/**
* @brief Adds a counted reference to this object. Use
* {@link CesiumUtility::IntrusivePointer} instead of calling this method
* directly.
*/
void addReference() const /*noexcept*/ {
const int32_t prevReferences = this->_referenceCount++;
if (this->_pDepot && prevReferences <= 0) {
this->_pDepot->unmarkDeletionCandidate(static_cast<const T*>(this));
}
}

/**
* @brief Removes a counted reference from this object. When the last
* reference is removed, this method will delete this instance. Use
* {@link CesiumUtility::IntrusivePointer} instead of calling this method
* directly.
*/
void releaseReference() const /*noexcept*/ {
CESIUM_ASSERT(this->_referenceCount > 0);
const int32_t references = --this->_referenceCount;
if (references == 0) {
SharedAssetDepot<T>* pDepot = this->_pDepot;
if (pDepot) {
// Let the depot manage this object's lifetime.
pDepot->markDeletionCandidate(static_cast<const T*>(this));
} else {
// No depot, so destroy this object directly.
delete static_cast<const T*>(this);
}
}
}

/**
* @brief Gets the shared asset depot that owns this asset, or nullptr if this
* asset is independent of an asset depot.
*/
const SharedAssetDepot<T>* getDepot() const { return this->_pDepot; }

/**
* @brief Gets the shared asset depot that owns this asset, or nullptr if this
* asset is independent of an asset depot.
*/
SharedAssetDepot<T>* getDepot() { return this->_pDepot; }

/**
* @brief Gets the unique ID of this asset, if it {@link isShareable}.
*
* If this asset is not shareable, this method will return an empty string.
*/
const std::string& getUniqueAssetId() const { return this->_uniqueAssetId; }

protected:
SharedAsset() = default;
~SharedAsset() { CESIUM_ASSERT(this->_referenceCount == 0); }

private:
mutable std::atomic<std::int32_t> _referenceCount{0};
SharedAssetDepot<T>* _pDepot{nullptr};
std::string _uniqueAssetId{};

// The size of this asset when it was counted by the depot. This is stored so
// that the exact same size can be subtracted later.
int64_t _sizeInDepot{0};

// To allow the depot to modify _pDepot, _uniqueAssetId, and _sizeInDepot.
friend class SharedAssetDepot<T>;
};

} // namespace CesiumAsync
Loading

0 comments on commit 3e5eb71

Please sign in to comment.