From 8a8ab0b274dccfe335737dff611593dadbfb6a40 Mon Sep 17 00:00:00 2001 From: unhyperbolic Date: Thu, 21 May 2020 10:53:59 -0700 Subject: [PATCH] Storm: Clarifying that HdRepr is owning the draw items by using std::unique_ptr. (Internal change: 2069901) --- pxr/imaging/hd/renderIndex.cpp | 10 +++------- pxr/imaging/hd/repr.cpp | 7 +------ pxr/imaging/hd/repr.h | 15 ++++++++------- pxr/imaging/hd/rprim.cpp | 14 +++++++++----- pxr/imaging/hd/rprim.h | 3 +-- pxr/imaging/hd/version.h | 3 ++- pxr/imaging/hdSt/basisCurves.cpp | 5 +++-- pxr/imaging/hdSt/mesh.cpp | 5 +++-- pxr/imaging/hdSt/points.cpp | 5 +++-- pxr/imaging/hdSt/volume.cpp | 6 +++--- 10 files changed, 36 insertions(+), 37 deletions(-) diff --git a/pxr/imaging/hd/renderIndex.cpp b/pxr/imaging/hd/renderIndex.cpp index d956780928..6651de5de8 100644 --- a/pxr/imaging/hd/renderIndex.cpp +++ b/pxr/imaging/hd/renderIndex.cpp @@ -1844,13 +1844,9 @@ HdRenderIndex::_AppendDrawItems( if (reprSelector.IsActiveRepr(i)) { TfToken const& reprToken = reprSelector[i]; - const HdRprim::HdDrawItemPtrVector* rprimDrawItems = - rprim->GetDrawItems(reprToken); - - if (TF_VERIFY(rprimDrawItems)) { - drawItems.insert(drawItems.end(), - rprimDrawItems->begin(), - rprimDrawItems->end() ); + for (const HdRepr::DrawItemUniquePtr &rprimDrawItem + : rprim->GetDrawItems(reprToken)) { + drawItems.push_back(rprimDrawItem.get()); } } } diff --git a/pxr/imaging/hd/repr.cpp b/pxr/imaging/hd/repr.cpp index 6c875d4f7c..6579dd8970 100644 --- a/pxr/imaging/hd/repr.cpp +++ b/pxr/imaging/hd/repr.cpp @@ -132,12 +132,7 @@ HdReprSelector::operator[](size_t topologyIndex) const HdRepr::HdRepr() = default; -HdRepr::~HdRepr() -{ - for (HdDrawItem* item : _drawItems) { - delete item; - } -} +HdRepr::~HdRepr() = default; PXR_NAMESPACE_CLOSE_SCOPE diff --git a/pxr/imaging/hd/repr.h b/pxr/imaging/hd/repr.h index c310c5dde9..74752ec03f 100644 --- a/pxr/imaging/hd/repr.h +++ b/pxr/imaging/hd/repr.h @@ -148,7 +148,8 @@ class HdReprSelector class HdRepr final { public: - typedef std::vector DrawItems; + using DrawItemUniquePtr = std::unique_ptr; + using DrawItemUniquePtrVector = std::vector; HD_API HdRepr(); @@ -156,21 +157,21 @@ class HdRepr final ~HdRepr(); /// Returns the draw items for this representation. - const DrawItems& GetDrawItems() { + const DrawItemUniquePtrVector& GetDrawItems() const { return _drawItems; } /// Transfers ownership of a draw item to this repr. - void AddDrawItem(HdDrawItem *item) { - _drawItems.push_back(item); + void AddDrawItem(std::unique_ptr &&item) { + _drawItems.push_back(std::move(item)); } /// Returns the draw item at the requested index. /// /// Note that the pointer returned is owned by this object and must not be /// deleted. - HdDrawItem* GetDrawItem(size_t index) { - return _drawItems[index]; + HdDrawItem* GetDrawItem(size_t index) const { + return _drawItems[index].get(); } private: @@ -179,7 +180,7 @@ class HdRepr final HdRepr& operator=(const HdRepr&) = delete; private: - DrawItems _drawItems; + DrawItemUniquePtrVector _drawItems; }; diff --git a/pxr/imaging/hd/rprim.cpp b/pxr/imaging/hd/rprim.cpp index b1188d9538..fdf825c09e 100644 --- a/pxr/imaging/hd/rprim.cpp +++ b/pxr/imaging/hd/rprim.cpp @@ -152,14 +152,18 @@ HdRprim::InitRepr(HdSceneDelegate* delegate, // -------------------------------------------------------------------------- // /// Rprim Hydra Engine API : Execute-Phase // -------------------------------------------------------------------------- // -const HdRprim::HdDrawItemPtrVector* +const HdRepr::DrawItemUniquePtrVector & HdRprim::GetDrawItems(TfToken const& reprToken) const { - HdReprSharedPtr repr = _GetRepr(reprToken); - if (repr) { - return &(repr->GetDrawItems()); + if (HdReprSharedPtr const repr = _GetRepr(reprToken)) { + return repr->GetDrawItems(); } - return nullptr; + + static HdRepr::DrawItemUniquePtrVector empty; + + TF_CODING_ERROR("Rprim has draw items for repr %s", reprToken.GetText()); + + return empty; } // -------------------------------------------------------------------------- // diff --git a/pxr/imaging/hd/rprim.h b/pxr/imaging/hd/rprim.h index da19534295..8c20ef1f55 100644 --- a/pxr/imaging/hd/rprim.h +++ b/pxr/imaging/hd/rprim.h @@ -153,9 +153,8 @@ class HdRprim { /// These draw items should be constructed and cached beforehand by Sync(). /// If no draw items exist, or reprToken cannot be found, nullptr will be /// returned. - using HdDrawItemPtrVector = std::vector; HD_API - const HdDrawItemPtrVector* + const HdRepr::DrawItemUniquePtrVector & GetDrawItems(TfToken const& reprToken) const; // ---------------------------------------------------------------------- // diff --git a/pxr/imaging/hd/version.h b/pxr/imaging/hd/version.h index c06095fd19..096c1f72fc 100644 --- a/pxr/imaging/hd/version.h +++ b/pxr/imaging/hd/version.h @@ -41,7 +41,8 @@ // 31 -> 32: renamed HdShader{Param} to HdMaterial{Param} // 32 -> 33: Deleted GetPathForInstanceIndex; added GetScenePrimPath. // 32 -> 34: Added HdInstancerContext to GetScenePrimPath. -#define HD_API_VERSION 34 +// 34 -> 35: HdRepr is using std::unique_ptr +#define HD_API_VERSION 35 // 1 -> 2: SimpleLighting -> FallbackLighting #define HD_SHADER_API 2 diff --git a/pxr/imaging/hdSt/basisCurves.cpp b/pxr/imaging/hdSt/basisCurves.cpp index 257030bb2f..2512b4f362 100644 --- a/pxr/imaging/hdSt/basisCurves.cpp +++ b/pxr/imaging/hdSt/basisCurves.cpp @@ -368,9 +368,10 @@ HdStBasisCurves::_InitRepr(TfToken const &reprToken, HdDirtyBits *dirtyBits) continue; } - HdDrawItem *drawItem = new HdStDrawItem(&_sharedData); + HdRepr::DrawItemUniquePtr drawItem = + std::make_unique(&_sharedData); HdDrawingCoord *drawingCoord = drawItem->GetDrawingCoord(); - repr->AddDrawItem(drawItem); + repr->AddDrawItem(std::move(drawItem)); if (desc.geomStyle == HdBasisCurvesGeomStyleWire) { // Why does geom style require this change? drawingCoord->SetTopologyIndex(HdStBasisCurves::HullTopology); diff --git a/pxr/imaging/hdSt/mesh.cpp b/pxr/imaging/hdSt/mesh.cpp index 2780b60c35..8c11597c57 100644 --- a/pxr/imaging/hdSt/mesh.cpp +++ b/pxr/imaging/hdSt/mesh.cpp @@ -1915,9 +1915,10 @@ HdStMesh::_InitRepr(TfToken const &reprToken, HdDirtyBits *dirtyBits) if (numDrawItems == 0) continue; for (size_t itemId = 0; itemId < numDrawItems; itemId++) { - HdDrawItem *drawItem = new HdStDrawItem(&_sharedData); - repr->AddDrawItem(drawItem); + HdRepr::DrawItemUniquePtr drawItem = + std::make_unique(&_sharedData); HdDrawingCoord *drawingCoord = drawItem->GetDrawingCoord(); + repr->AddDrawItem(std::move(drawItem)); switch (desc.geomStyle) { case HdMeshGeomStyleHull: diff --git a/pxr/imaging/hdSt/points.cpp b/pxr/imaging/hdSt/points.cpp index d883c1bb27..a4f8f17fb5 100644 --- a/pxr/imaging/hdSt/points.cpp +++ b/pxr/imaging/hdSt/points.cpp @@ -346,9 +346,10 @@ HdStPoints::_InitRepr(TfToken const &reprToken, HdDirtyBits *dirtyBits) const HdPointsReprDesc &desc = descs[descIdx]; if (desc.geomStyle != HdPointsGeomStyleInvalid) { - HdDrawItem *drawItem = new HdStDrawItem(&_sharedData); + HdRepr::DrawItemUniquePtr drawItem = + std::make_unique(&_sharedData); HdDrawingCoord *drawingCoord = drawItem->GetDrawingCoord(); - _smoothHullRepr->AddDrawItem(drawItem); + _smoothHullRepr->AddDrawItem(std::move(drawItem)); // Set up drawing coord instance primvars. drawingCoord->SetInstancePrimvarBaseIndex( diff --git a/pxr/imaging/hdSt/volume.cpp b/pxr/imaging/hdSt/volume.cpp index 206ac76f3d..4719147df0 100644 --- a/pxr/imaging/hdSt/volume.cpp +++ b/pxr/imaging/hdSt/volume.cpp @@ -106,9 +106,9 @@ HdStVolume::_InitRepr(TfToken const &reprToken, HdDirtyBits* dirtyBits) { // All representations point to _volumeRepr. if (!_volumeRepr) { - _volumeRepr = HdReprSharedPtr(new HdRepr()); - HdDrawItem * const drawItem = new HdStDrawItem(&_sharedData); - _volumeRepr->AddDrawItem(drawItem); + _volumeRepr = std::make_shared(); + _volumeRepr->AddDrawItem( + std::make_unique(&_sharedData)); *dirtyBits |= HdChangeTracker::NewRepr; }