From 29a46f7752485351cb9ffa9ef27218d20fbeaca4 Mon Sep 17 00:00:00 2001
From: tgvarik <tgvarik@users.noreply.github.com>
Date: Fri, 8 Nov 2024 14:46:48 -0800
Subject: [PATCH] [hdPrman] Fix legacyGeomSubsetSceneIndex prim removal

legacyGeomSubsetSceneIndex was not properly cleaning up its cache of subsets upon ancestor removal.

(Internal change: 2347373)
---
 pxr/imaging/hd/legacyGeomSubsetSceneIndex.cpp | 46 ++++++++-----------
 pxr/imaging/hd/legacyGeomSubsetSceneIndex.h   | 20 ++++----
 2 files changed, 29 insertions(+), 37 deletions(-)

diff --git a/pxr/imaging/hd/legacyGeomSubsetSceneIndex.cpp b/pxr/imaging/hd/legacyGeomSubsetSceneIndex.cpp
index 9b615aa0ac..495308f751 100644
--- a/pxr/imaging/hd/legacyGeomSubsetSceneIndex.cpp
+++ b/pxr/imaging/hd/legacyGeomSubsetSceneIndex.cpp
@@ -99,32 +99,32 @@ class _HdDataSourceLegacyGeomSubset
 {
 public:
     HD_DECLARE_DATASOURCE(_HdDataSourceLegacyGeomSubset);
-    
+
     TfTokenVector GetNames() override;
     HdDataSourceBaseHandle Get(const TfToken& name) override;
-    
+
 private:
     _HdDataSourceLegacyGeomSubset(
         const SdfPath& id,
         const SdfPath& parentId,
         const TfToken& parentType,
         HdSceneDelegate* sceneDelegate);
-    
+
     struct _Subset
     {
         TfToken type;
         VtIntArray indices;
         bool visibility = false;
         SdfPath materialBinding;
-        
+
         operator bool() const {
             return (!type.IsEmpty() && !indices.empty());
         }
     };
-    
+
     _Subset
     _FindSubset() const;
-    
+
     SdfPath _parentId;
     TfToken _parentType;
 };
@@ -166,7 +166,7 @@ _HdDataSourceLegacyGeomSubset::Get(const TfToken& name)
                 .Build();
         }
     }
-    
+
     // We must intercept visibility and materialBindings because the
     // base class does not know how to compute these for geom subsets.
     if (name == HdVisibilitySchema::GetSchemaToken()) {
@@ -202,14 +202,14 @@ _HdDataSourceLegacyGeomSubset::Get(const TfToken& name)
             HdPrimvarsSchema::BuildRetained(0, nullptr, nullptr);
         return emptyPrimvarsDs;
     }
-    
+
     // To block everything else, and so prevent calling something on the
     // sceneDelegate for a geom subset path about which it knows nothing, we
     // only defer to base class for sceneDelegate.
     if (name == HdSceneIndexEmulationTokens->sceneDelegate) {
         return HdDataSourceLegacyPrim::Get(name);
     }
-    
+
     return nullptr;
 }
 
@@ -313,7 +313,7 @@ void
 HdLegacyGeomSubsetSceneIndex::_PrimsAdded(
     const HdSceneIndexBase&  /*sender*/,
     const HdSceneIndexObserver::AddedPrimEntries& entries)
-{                
+{
     HdSceneIndexObserver::AddedPrimEntries newEntries;
     for (const HdSceneIndexObserver::AddedPrimEntry& entry : entries) {
         if (!HdPrimTypeSupportsGeomSubsets(entry.primType)) {
@@ -326,7 +326,7 @@ HdLegacyGeomSubsetSceneIndex::_PrimsAdded(
             continue;
         }
         // Only add prims with subsets to _parentPrims to save on memory.
-        _parentPrims.insert({ entry.primPath, paths });        
+        _parentPrims.insert({ entry.primPath, paths });
         for (const SdfPath& path : paths) {
             newEntries.push_back({ path, HdPrimTypeTokens->geomSubset });
         }
@@ -343,21 +343,13 @@ HdLegacyGeomSubsetSceneIndex::_PrimsRemoved(
     const HdSceneIndexBase&  /*sender*/,
     const HdSceneIndexObserver::RemovedPrimEntries& entries)
 {
-    HdSceneIndexObserver::RemovedPrimEntries removedEntries;
     for (const HdSceneIndexObserver::RemovedPrimEntry& entry : entries) {
-        auto it = _parentPrims.find(entry.primPath);
-        if (it != _parentPrims.end()) {
-            for (const SdfPath& path : it->second) {
-                removedEntries.push_back({ path });
-            }
-            _parentPrims.erase(it);
+        auto it = _parentPrims.lower_bound(entry.primPath);
+        while (it != _parentPrims.end() &&
+            it->first.HasPrefix(entry.primPath)) {
+            it = _parentPrims.erase(it);
         }
     }
-    if (!removedEntries.empty()) {
-        removedEntries.insert(
-            removedEntries.begin(), entries.cbegin(), entries.cend());
-        return _SendPrimsRemoved(removedEntries);
-    }
     _SendPrimsRemoved(entries);
 }
 
@@ -378,11 +370,11 @@ HdLegacyGeomSubsetSceneIndex::_PrimsDirtied(
         HdMeshTopologySchema::GetDefaultLocator() };
     static const HdDataSourceLocatorSet emptyLocatorSet {
         HdDataSourceLocator::EmptyLocator() };
-        
+
     HdSceneIndexObserver::AddedPrimEntries addedEntries;
     HdSceneIndexObserver::RemovedPrimEntries removedEntries;
     HdSceneIndexObserver::DirtiedPrimEntries dirtiedEntries;
-    
+
     for (const HdSceneIndexObserver::DirtiedPrimEntry& entry : entries) {
         if (!entry.dirtyLocators.Intersects(topologyLocators)) {
             // If the change didn't affect topology, we can continue. This is
@@ -453,7 +445,7 @@ HdLegacyGeomSubsetSceneIndex::_PrimsDirtied(
 SdfPathVector
 HdLegacyGeomSubsetSceneIndex::_ListDelegateSubsets(
     const SdfPath& parentPath,
-    const HdSceneIndexPrim& parentPrim) 
+    const HdSceneIndexPrim& parentPrim)
 {
     SdfPathVector paths;
     if (!parentPrim.dataSource ||
@@ -487,7 +479,7 @@ HdLegacyGeomSubsetSceneIndex::_ListDelegateSubsets(
                 paths.push_back(parentPath.AppendChild(
                     _tokens->invisiblePoints));
             }
-            
+
         }
     }
     return paths;
diff --git a/pxr/imaging/hd/legacyGeomSubsetSceneIndex.h b/pxr/imaging/hd/legacyGeomSubsetSceneIndex.h
index 6db85e6f45..804c668997 100644
--- a/pxr/imaging/hd/legacyGeomSubsetSceneIndex.h
+++ b/pxr/imaging/hd/legacyGeomSubsetSceneIndex.h
@@ -18,7 +18,7 @@
 
 #include "pxr/pxr.h"
 
-#include <unordered_map>
+#include <map>
 
 PXR_NAMESPACE_OPEN_SCOPE
 
@@ -45,42 +45,42 @@ class HdLegacyGeomSubsetSceneIndex
     HD_API
     static HdLegacyGeomSubsetSceneIndexRefPtr New(
         const HdSceneIndexBaseRefPtr& inputSceneIndex);
-    
+
     HD_API
     ~HdLegacyGeomSubsetSceneIndex() override;
-    
+
     HD_API
     HdSceneIndexPrim GetPrim(const SdfPath& primPath) const override;
-    
+
     HD_API
     SdfPathVector GetChildPrimPaths(const SdfPath& primPath) const override;
 
 protected:
     HdLegacyGeomSubsetSceneIndex(
         const HdSceneIndexBaseRefPtr& inputSceneIndex);
-    
+
     void _PrimsAdded(
         const HdSceneIndexBase& sender,
         const HdSceneIndexObserver::AddedPrimEntries& entries) override;
-    
+
     void _PrimsRemoved(
         const HdSceneIndexBase& sender,
         const HdSceneIndexObserver::RemovedPrimEntries& entries) override;
-    
+
     void _PrimsDirtied(
         const HdSceneIndexBase& sender,
         const HdSceneIndexObserver::DirtiedPrimEntries& entries) override;
 
-private:    
+private:
     static SdfPathVector
     _ListDelegateSubsets(
         const SdfPath& parentPath,
         const HdSceneIndexPrim& parentPrim);
-    
+
     // Unordered map of parent path -> [subset paths...]
     // XXX: Do not use SdfPathTable because we do not want it
     // to implicitly include the extra ancestor paths.
-    std::unordered_map<SdfPath, SdfPathVector, SdfPath::Hash> _parentPrims;
+    std::map<SdfPath, SdfPathVector> _parentPrims;
 };
 
 PXR_NAMESPACE_CLOSE_SCOPE