This change addresses a crash that resulted when adding / removing a …

…sublayer results in a cycle. In order to prevent this, a cache of visited layer/sublayer path pairs is used to terminate recursive processing of sublayer paths.

Fixes #3493

(Internal change: 2355098)

pxr/usd/pcp/changes.cpp

@@ -2197,17 +2197,27 @@ PcpChanges::_DidAddOrRemoveSublayer(
     std::string* debugSummary,
     std::vector<bool>* significant)
 {
+    PcpCacheChanges& cacheChanges = _GetCacheChanges(cache);
+
+    // Before processing any sublayer paths first check if we have encountered
+    // this layer / sublayer path before.  If we have, it indicates that there
+    // is a cycle in this layer stack.
+    const auto key = std::make_pair(layer, sublayerPath);
+    if (!cacheChanges._processedLayerSublayerPathPairs.insert(key).second) {
+        significant->resize(layerStacks.size(), false);
+        return;
+    }
+
     PCP_APPEND_DEBUG(
         "  Layer @%s@ changed sublayers\n",
         layer ? layer->GetIdentifier().c_str() : "invalid");
 
     const auto& processChanges = 
-        [this, &cache, &sublayerPath, &debugSummary, &layer](
+        [this, &cache, &sublayerPath, &debugSummary, &layer, &cacheChanges](
             const SdfLayerRefPtr sublayer,
             const PcpLayerStackPtrVector& layerStacks,
             _SublayerChangeType sublayerChange)
         {
-            PcpCacheChanges& cacheChanges = _GetCacheChanges(cache);
             if (sublayer) {
                 _lifeboat.Retain(sublayer);
                 cacheChanges.didAddOrRemoveNonEmptySublayer |= !sublayer->IsEmpty();

pxr/usd/pcp/changes.h

@@ -147,6 +147,15 @@ class PcpCacheChanges {
     friend class PcpCache;
     friend class PcpChanges;
 
+    using _ProcessedLayerSublayerPathPairsKey = 
+        std::pair<SdfLayerHandle, std::string>;
+
+    // Set of hashed layer / sublayer path pairs that have been processed in
+    // in this round of changes.  These values are checked in order to avoid
+    // recursively processing cycles created in layer stacks.
+    std::unordered_set<_ProcessedLayerSublayerPathPairsKey, TfHash> 
+        _processedLayerSublayerPathPairs;
+
     // Must rebuild the prim/property stacks at each path due to a change
     // that only affects the internal representation of the stack and
     // not its contents.  Because this causes no externally-observable

pxr/usd/usd/testenv/testUsdChangeProcessing.py

@@ -5,10 +5,8 @@
 # Licensed under the terms set forth in the LICENSE.txt file available at
 # https://openusd.org/license.
 
-from __future__ import print_function
-
 import sys
-from pxr import Sdf,Usd,Tf
+from pxr import Sdf,Usd,Pcp
 
 def RenamingSpec():
     '''Test renaming a SdfPrimSpec.'''
@@ -39,9 +37,62 @@ def ChangeInsignificantSublayer():
         prim = stage.DefinePrim("/Foo")
         assert prim
 
+def _TestStageErrors(stageAndErrorCounts):
+    for stage, errorCount in stageAndErrorCounts:
+        errors = stage.GetCompositionErrors()
+        assert len(errors) == errorCount
+
+        for i in range(errorCount):
+            assert isinstance(errors[i], Pcp.ErrorSublayerCycle)
+
+def AddSublayerWithCycle():
+    '''Tests that adding a sublayer resulting in a cycle produces warnings'''
+
+    root = Usd.Stage.CreateNew("root.usda")
+    a = Usd.Stage.CreateNew("a.usda")
+    b = Usd.Stage.CreateNew("b.usda")
+
+    root.GetRootLayer().subLayerPaths.append("b.usda")
+    b.GetRootLayer().subLayerPaths.append("a.usda")
+
+    # Initial sanity test, there should be no errors on any stage
+    _TestStageErrors([(root, 0), (a, 0), (b, 0)])
+
+    # Add the sublayer which creates a cycle in all stages
+    a.GetRootLayer().subLayerPaths.append("b.usda")
+    _TestStageErrors([(root, 1), (a, 1), (b, 1)])
+
+def UnmuteWithCycle():
+    '''Tests that unmuting a sublayer resulting in a cycle produces warnings'''
+
+    root = Usd.Stage.CreateNew("root.usda")
+    a = Usd.Stage.CreateNew("a.usda")
+    b = Usd.Stage.CreateNew("b.usda")
+
+    # Mute layer b on the root stage, this will prevent a cycle from being
+    # created while the sublayers are assembled
+    root.MuteLayer(b.GetRootLayer().identifier)
+
+    root.GetRootLayer().subLayerPaths.append("b.usda")
+    b.GetRootLayer().subLayerPaths.append("a.usda")
+    a.GetRootLayer().subLayerPaths.append("b.usda")
+
+    # Initial sanity test, there should be no errors on root
+    _TestStageErrors([(root, 0)])
+
+    # Unmute layer b creating a cycle on root
+    root.UnmuteLayer(b.GetRootLayer().identifier)
+    _TestStageErrors([(root, 1)])
+
+    # Mute the layer ensuring that the cycle has been removed
+    root.MuteLayer(b.GetRootLayer().identifier)
+    _TestStageErrors([(root, 0)])
+
 def Main(argv):
     RenamingSpec()
     ChangeInsignificantSublayer()
+    AddSublayerWithCycle()
+    UnmuteWithCycle()
 
 if __name__ == "__main__":
     Main(sys.argv)