pcp: Fix bug when muting layers/removing sublayers with sublayers

When computing incremental changes, Pcp needs to ignore the layers being muted/removed *and* any of the sublayers they bring in when recomputing state because these layers should no longer contribute opinions to the composed scene. The latter was not happening, which caused prim indexes to not be invalidated as expected. This fix just ensures we do track all of those layers. Fixing this issue revealed a separate bug when invalidating indexes for prims that reference a muted layer. The logic for determining when this invalidation was necessary was flipped and also did not account for culled dependencies. This has been fixed and new unit tests added to cover this case. (Internal change: 2345458)
PixarAnimationStudios · Oct 23, 2024 · 85c7f10 · 85c7f10
1 parent 7eb1af8
commit 85c7f10
Show file tree

Hide file tree

Showing 23 changed files with 409 additions and 123 deletions.
diff --git a/pxr/usd/pcp/cache.cpp b/pxr/usd/pcp/cache.cpp
@@ -1037,8 +1037,8 @@ PcpCache::Apply(const PcpCacheChanges& changes, PcpLifeboat* lifeboat)
                 // we may have blown the prim index so check that it exists.
                 if (PcpPrimIndex* primIndex = _GetPrimIndex(path)) {
                     Pcp_RescanForSpecs(primIndex, IsUsd(),
-                                       /* updateHasSpecs */ true, 
-                                       changes.layersToMute);
+                                       /* updateHasSpecs */ true,
+                                       &changes);
 
                     // If there are no specs left then we can discard the
                     // prim index.

diff --git a/pxr/usd/pcp/changes.cpp b/pxr/usd/pcp/changes.cpp
@@ -1411,30 +1411,6 @@ void PcpChanges::DidMuteAndUnmuteLayers(
     const std::vector<std::string>& layersToMute,
     const std::vector<std::string>& layersToUnmute)
 {
-    PcpCacheChanges& changes = _GetCacheChanges(cache);
-
-    // We first want to grab all layers that will be muted and unmuted and
-    // store them off.  We may need to refer to these vectors when processing
-    // changes to account for a layer's future state once these changes have
-    // been applied
-    for (const auto& layerId : layersToMute) { 
-        SdfLayerRefPtr layerToMute =
-            _LoadSublayerForChange(cache, layerId, _SublayerRemoved);
-        if (layerToMute) {
-            _lifeboat.Retain(layerToMute);
-            changes.layersToMute.emplace_back(layerToMute);
-        }
-    }
-
-    for (const auto& layerId : layersToUnmute) { 
-        SdfLayerRefPtr layerToUnmute =
-            _LoadSublayerForChange(cache, layerId, _SublayerAdded);
-        if (layerToUnmute) {
-            _lifeboat.Retain(layerToUnmute);
-            changes.layersToUnmute.emplace_back(layerToUnmute);
-        }
-    }
-
     // Register changes for all computed layer stacks that are
     // affected by the newly muted/unmuted layers.
     for (const auto& layerToMute : layersToMute) {
@@ -1455,14 +1431,11 @@ PcpChanges::_MarkReferencingSitesAsSignificantlyChanged(
 
     const PcpCacheChanges& cacheChanges = _GetCacheChanges(cache);
 
-    const auto refOrPayloadChangeFunc = 
-        [this, cache](const SdfPath &depIndexPath, 
-                        const PcpNodeRef &node)
+    const auto refOrPayloadChangeFunc =
+        [this, cache](const SdfPath& depIndexPath, PcpArcType arcType)
         {
-            PcpArcType arcType = node.GetArcType();
-            if (arcType == PcpArcTypeReference || 
-                arcType == PcpArcTypePayload) 
-            {
+            if (arcType == PcpArcTypeReference ||
+                arcType == PcpArcTypePayload) {
                 DidChangeSignificantly(cache, depIndexPath);
             }
         };
@@ -1477,15 +1450,26 @@ PcpChanges::_MarkReferencingSitesAsSignificantlyChanged(
             /* filter */ true);
 
         for(const PcpDependency &dep: deps) {
-            // This ensures that all sites which reference this layer are
-            // also marked as having changed significantly.
+            // If this layer stack no longer has any opinions at dep.sitePath,
+            // composed prims that reference this site must be recomputed to
+            // detect the broken reference.
             if (PcpComposeSiteHasPrimSpecs(
-                layerStack, dep.sitePath, cacheChanges.layersToMute))
-            {
-                Pcp_ForEachDependentNode(dep.sitePath, layerStack,
-                                        dep.indexPath,
-                                        *cache, refOrPayloadChangeFunc);
+                layerStack, dep.sitePath, 
+                cacheChanges.layersAffectedByMutingOrRemoval)) {
+                continue;
             }
+
+            Pcp_ForEachDependentNode(
+                dep.sitePath, layerStack, dep.indexPath, *cache,
+                [&](const SdfPath &depIndexPath, const PcpNodeRef &node)
+                {
+                    refOrPayloadChangeFunc(depIndexPath, node.GetArcType());
+                },
+                [&](const SdfPath& depIndexPath, const PcpCulledDependency& dep)
+                {
+                    refOrPayloadChangeFunc(depIndexPath, dep.arcType);
+                }
+            );
         }
     }
 }
@@ -1534,13 +1518,25 @@ PcpChanges::_DidMuteLayer(
     std::string summary;
     std::string* debugSummary = TfDebug::IsEnabled(PCP_CHANGES) ? &summary : 0;
 
-    PcpCacheChanges& cacheChanges = _GetCacheChanges(cache);
-
     const SdfLayerRefPtr mutedLayer = 
         _LoadSublayerForChange(cache, layerId, _SublayerRemoved);
     const PcpLayerStackPtrVector& layerStacks = 
         cache->FindAllLayerStacksUsingLayer(mutedLayer);
 
+    PcpCacheChanges& cacheChanges = _GetCacheChanges(cache);
+    if (mutedLayer) {
+        _lifeboat.Retain(mutedLayer);
+        cacheChanges.didMuteOrUnmuteNonEmptyLayer |= !mutedLayer->IsEmpty();
+
+        // Track sublayers that have been muted separately. These
+        // layers should no longer contribute opinions to the composed
+        // scene; during change processing, clients that need to
+        // recompute state (e.g. prim stacks) must explicitly ignore
+        // these layers. This is because these layers won't be removed
+        // from layer stacks until change processing is complete.
+        cacheChanges.layersAffectedByMutingOrRemoval.insert(mutedLayer);
+    }
+
     PCP_APPEND_DEBUG("  Did mute layer @%s@\n", layerId.c_str());
 
     // XXX: Computing proper changes for layers containing relocates at this 
@@ -1568,15 +1564,11 @@ PcpChanges::_DidMuteLayer(
         DidChange(cache, changes);
         cacheChanges.layerChangeListVec.emplace_back(
             std::move(changes.front()));
-        _lifeboat.Retain(mutedLayer);
 
         _ProcessLayerStackAndDependencyChanges(cache, layerStacks);
         _MarkReferencingSitesAsSignificantlyChanged(cache, layerStacks);
     }
 
-    cacheChanges.didMuteOrUnmuteNonEmptyLayer |= 
-        mutedLayer ? !mutedLayer->IsEmpty() : false;
-
     if (debugSummary && !debugSummary->empty()) {
         TfDebug::Helper().Msg("PcpChanges::_DidMuteLayer\n%s",
                             debugSummary->c_str());
@@ -1592,13 +1584,17 @@ PcpChanges::_DidUnmuteLayer(
     std::string summary;
     std::string* debugSummary = TfDebug::IsEnabled(PCP_CHANGES) ? &summary : 0;
 
-    PcpCacheChanges& cacheChanges = _GetCacheChanges(cache);
-
     const SdfLayerRefPtr unmutedLayer = 
         _LoadSublayerForChange(cache, layerId, _SublayerAdded);
     const PcpLayerStackPtrVector& layerStacks = 
         cache->_layerStackCache->FindAllUsingMutedLayer(layerId);
 
+    PcpCacheChanges& cacheChanges = _GetCacheChanges(cache);
+    if (unmutedLayer) {
+        _lifeboat.Retain(unmutedLayer);
+        cacheChanges.didMuteOrUnmuteNonEmptyLayer |= !unmutedLayer->IsEmpty();
+    }
+
     PCP_APPEND_DEBUG("  Did unmute layer @%s@\n", layerId.c_str());
 
     // XXX: Computing proper changes for layers containing relocates at this 
@@ -1631,15 +1627,11 @@ PcpChanges::_DidUnmuteLayer(
         DidChange(cache, changes);
         cacheChanges.layerChangeListVec.emplace_back(
             std::move(changes.front()));
-        _lifeboat.Retain(unmutedLayer);
 
         _ProcessLayerStackAndDependencyChanges(cache, layerStacks);
         _MarkReferencingSitesAsSignificantlyChanged(cache, layerStacks);
     }
 
-    cacheChanges.didMuteOrUnmuteNonEmptyLayer |= 
-        unmutedLayer ? !unmutedLayer->IsEmpty() : false;
-
     if (debugSummary && !debugSummary->empty()) {
         TfDebug::Helper().Msg("PcpChanges::_DidUnmuteLayer\n%s",
                               debugSummary->c_str());
@@ -1783,7 +1775,9 @@ static bool
 _NoLongerHasAnySpecs(const PcpCacheChanges& changes, const PcpPrimIndex& primIndex)
 {
     for (const PcpNodeRef &node: primIndex.GetNodeRange()) {
-        if (PcpComposeSiteHasPrimSpecs(node.GetLayerStack(), node.GetPath(), changes.layersToMute)) {
+        if (PcpComposeSiteHasPrimSpecs(
+                node.GetLayerStack(), node.GetPath(), 
+                changes.layersAffectedByMutingOrRemoval)) {
             return false;
         }
     }
@@ -2203,8 +2197,21 @@ PcpChanges::_DidAddOrRemoveSublayer(
             _SublayerChangeType sublayerChange)
         {
             PcpCacheChanges& cacheChanges = _GetCacheChanges(cache);
-            cacheChanges.didAddOrRemoveNonEmptySublayer |= 
-                sublayer && !sublayer->IsEmpty();
+            if (sublayer) {
+                _lifeboat.Retain(sublayer);
+                cacheChanges.didAddOrRemoveNonEmptySublayer |= !sublayer->IsEmpty();
+
+                // Track sublayers that have been removed separately. These
+                // layers should no longer contribute opinions to the composed
+                // scene; during change processing, clients that need to
+                // recompute state (e.g. prim stacks) must explicitly ignore
+                // these layers. This is because these layers won't be removed
+                // from layer stacks until change processing is complete.
+                if (sublayerChange == _SublayerRemoved) {
+                    cacheChanges.layersAffectedByMutingOrRemoval
+                        .insert(sublayer);
+                }
+            }
 
             if (!TfGetEnvSetting(
                     PCP_ENABLE_MINIMAL_CHANGES_FOR_LAYER_OPERATIONS) ||
@@ -2245,7 +2252,6 @@ PcpChanges::_DidAddOrRemoveSublayer(
 
             cacheChanges.layerChangeListVec.emplace_back(
                 std::move(changes.front()));
-            _lifeboat.Retain(sublayer);
 
             return true;
         };

diff --git a/pxr/usd/pcp/changes.h b/pxr/usd/pcp/changes.h
@@ -21,6 +21,7 @@
 
 #include <map>
 #include <set>
+#include <unordered_set>
 
 PXR_NAMESPACE_OPEN_SCOPE
 
@@ -134,8 +135,9 @@ class PcpCacheChanges {
     /// Will be true if a non empty sublayer was added or removed.
     bool didAddOrRemoveNonEmptySublayer = false;
 
-    /// Lists of layers that will be muted or unmuted during change processing.
-    std::vector<SdfLayerHandle> layersToMute, layersToUnmute;
+    /// Set of layers that were explicitly muted or removed from a sublayer
+    /// list and all sublayers of those layers, recursively.
+    std::unordered_set<SdfLayerHandle, TfHash> layersAffectedByMutingOrRemoval;
 
     // Holds all the diff changelists that were computed when adding/removing
     // sublayers or muting/unmuting layers.

diff --git a/pxr/usd/pcp/composeSite.cpp b/pxr/usd/pcp/composeSite.cpp
@@ -187,16 +187,14 @@ PcpComposeSitePermission(PcpLayerStackRefPtr const &layerStack,
 bool
 PcpComposeSiteHasPrimSpecs(PcpLayerStackRefPtr const &layerStack,
                            SdfPath const &path,
-                           const std::vector<SdfLayerHandle>& layersToIgnore)
+                           const std::unordered_set<SdfLayerHandle, TfHash>& 
+                               layersToIgnore)
 {
     for (auto const &layer: layerStack->GetLayers()) {
         // if a spec was found in this layer, ensure that it is currently not
         // being ignored.
-        if (layer->HasSpec(path)) {
-            if (std::find( layersToIgnore.begin(), layersToIgnore.end(), layer) 
-                == layersToIgnore.end()) {
-                return true;
-            }
+        if (layer->HasSpec(path) && layersToIgnore.count(layer) == 0) {
+            return true;
         }
     }
     return false;

diff --git a/pxr/usd/pcp/composeSite.h b/pxr/usd/pcp/composeSite.h
@@ -206,8 +206,10 @@ PCP_API
 bool
 PcpComposeSiteHasPrimSpecs(PcpLayerStackRefPtr const &layerStack,
                            SdfPath const &path,
-                           const std::vector<SdfLayerHandle>& layersToIgnore);
+                           const std::unordered_set<SdfLayerHandle, TfHash>& 
+                               layersToIgnore);
 
+PCP_API
 bool
 PcpComposeSiteHasPrimSpecs(PcpLayerStackRefPtr const &layerStack,
                            SdfPath const &path);

diff --git a/pxr/usd/pcp/dependencies.cpp b/pxr/usd/pcp/dependencies.cpp
@@ -537,9 +537,10 @@ Pcp_AddCulledDependency(
 
     PcpCulledDependency dep;
     dep.flags = depFlags;
+    dep.arcType = node.GetArcType();
     dep.layerStack = node.GetLayerStack();
     dep.sitePath = node.GetPath();
-    if (node.GetArcType() == PcpArcTypeRelocate) {
+    if (dep.arcType == PcpArcTypeRelocate) {
         // See _ProcessDependentNode in pcp/cache.cpp for the similar code
         // we use to handle non-culled dependency
         //

diff --git a/pxr/usd/pcp/dependency.h b/pxr/usd/pcp/dependency.h
@@ -10,6 +10,7 @@
 #include "pxr/pxr.h"
 #include "pxr/usd/pcp/api.h"
 #include "pxr/usd/pcp/mapFunction.h"
+#include "pxr/usd/pcp/types.h"
 #include "pxr/usd/sdf/path.h"
 
 #include "pxr/base/tf/declarePtrs.h"
@@ -121,6 +122,8 @@ struct PcpCulledDependency
 {
     /// Flag representing the type of dependency.
     PcpDependencyFlags flags = PcpDependencyTypeNone;
+    /// Arc type for this dependency.
+    PcpArcType arcType = PcpArcTypeRoot;
     /// Layer stack containing the specs the prim index depends on.
     PcpLayerStackRefPtr layerStack;
     /// Path of the dependency specs in the layer stack.

diff --git a/pxr/usd/pcp/primIndex.cpp b/pxr/usd/pcp/primIndex.cpp
@@ -9,6 +9,7 @@
 #include "pxr/usd/pcp/primIndex.h"
 #include "pxr/usd/pcp/arc.h"
 #include "pxr/usd/pcp/cache.h"
+#include "pxr/usd/pcp/changes.h"
 #include "pxr/usd/pcp/dynamicFileFormatContext.h"
 #include "pxr/usd/pcp/composeSite.h"
 #include "pxr/usd/pcp/debugCodes.h"
@@ -4718,19 +4719,12 @@ _EnforcePermissions(
     }
 }
 
-void
-Pcp_RescanForSpecs(PcpPrimIndex *index, bool usd, bool updateHasSpecs)
-{
-    const std::vector<SdfLayerHandle> layersToIgnore;
-    Pcp_RescanForSpecs(index, usd, updateHasSpecs, layersToIgnore);
-}
-
 void
 Pcp_RescanForSpecs(
     PcpPrimIndex *index,
     bool usd,
     bool updateHasSpecs,
-    const std::vector<SdfLayerHandle>& layersToIgnore)
+    const PcpCacheChanges *cacheChanges = nullptr)
 {
     TfAutoMallocTag2 tag("Pcp", "Pcp_RescanForSpecs");
 
@@ -4741,7 +4735,8 @@ Pcp_RescanForSpecs(
             TF_FOR_ALL(nodeIt, index->GetNodeRange()) {
                 auto node = *nodeIt;
                 nodeIt->SetHasSpecs(PcpComposeSiteHasPrimSpecs(
-                    node.GetLayerStack(), node.GetPath(), layersToIgnore));
+                    node.GetLayerStack(), node.GetPath(), 
+                    cacheChanges->layersAffectedByMutingOrRemoval));
             }
         }
     } else {
@@ -4755,7 +4750,10 @@ Pcp_RescanForSpecs(
                     node.GetLayerStack()->GetLayers();
                 const SdfPath& path = node.GetPath();
                 for (size_t i = 0, n = layers.size(); i != n; ++i) {
-                    if (layers[i]->HasSpec(path)) {
+                    if (layers[i]->HasSpec(path) &&
+                        (!cacheChanges ||
+                          cacheChanges->layersAffectedByMutingOrRemoval
+                            .count(layers[i]) == 0)) {
                         nodeHasSpecs = true;
                         primSites.push_back(node.GetCompressedSdSite(i));
                     }

diff --git a/pxr/usd/pcp/primIndex.h b/pxr/usd/pcp/primIndex.h
@@ -40,6 +40,7 @@ TF_DECLARE_WEAK_AND_REF_PTRS(PcpPrimIndex_Graph);
 
 class ArResolver;
 class PcpCache;
+class PcpCacheChanges;
 class PcpPrimIndex;
 class PcpPrimIndexInputs;
 class PcpPrimIndexOutputs;
@@ -281,7 +282,7 @@ class PcpPrimIndex
     friend void Pcp_RescanForSpecs(
                     PcpPrimIndex*, bool usd,
                     bool updateHasSpecs,
-                    const std::vector<SdfLayerHandle>& layersToIgnore);
+                    const PcpCacheChanges *cacheChanges);
 
     // The node graph representing the compositional structure of this prim.
     PcpPrimIndex_GraphRefPtr _graph;