Storm: Clarifying that HdRepr is owning the draw items by using std::…

…unique_ptr. (Internal change: 2069901)
PixarAnimationStudios · May 21, 2020 · 8a8ab0b · 8a8ab0b
1 parent ed3c85b
commit 8a8ab0b
Show file tree

Hide file tree

Showing 10 changed files with 36 additions and 37 deletions.
diff --git 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
@@ -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
@@ -148,29 +148,30 @@ class HdReprSelector
 class HdRepr final
 {
 public:
-    typedef std::vector<HdDrawItem*> DrawItems;
+    using DrawItemUniquePtr = std::unique_ptr<HdDrawItem>;
+    using DrawItemUniquePtrVector = std::vector<DrawItemUniquePtr>;
 
     HD_API
     HdRepr();
     HD_API
     ~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<HdDrawItem> &&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
@@ -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
@@ -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<HdDrawItem*>;
     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
@@ -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<HdDrawItem>
+#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
@@ -368,9 +368,10 @@ HdStBasisCurves::_InitRepr(TfToken const &reprToken, HdDirtyBits *dirtyBits)
                 continue;
             }
 
-            HdDrawItem *drawItem = new HdStDrawItem(&_sharedData);
+            HdRepr::DrawItemUniquePtr drawItem =
+                std::make_unique<HdStDrawItem>(&_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
@@ -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<HdStDrawItem>(&_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
@@ -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<HdStDrawItem>(&_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
@@ -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<HdRepr>();
+        _volumeRepr->AddDrawItem(
+            std::make_unique<HdStDrawItem>(&_sharedData));
         *dirtyBits |= HdChangeTracker::NewRepr;
     }