Fix bug where adding an empty sublayer to a layer stack

could cause errors and incorrect results with value clips.

Usd_Clip and Usd_ClipSet previously stored the index of
the layer in the layer stack where the clip set was
authored. When a non-empty sublayer was added to the
layer stack, UsdStage would end up resync'ing prims,
which would ultimately recompute the clip sets and
update that index. However, when an empty sublayer
was added, this resync would not occur and the index
would be invalidated if the sublayer was added before
that position in the layer stack.

Once in this state, UsdStage would incorrectly identify
the layer where the value clips were authored. This could
lead to issues including errors opening value clips
specified via relative asset paths and incorrect results
when looking at an attribute's property stack.

This fix modifies the objects to store the handle of
the layer where the clip set was authored instead of the
index, avoiding the need to update that index on any
sublayer changes.

Fixes #2014

(Internal change: 2256833)

pxr/usd/usd/clip.cpp

@@ -104,7 +104,11 @@ Usd_Clip::Usd_Clip(
     const std::shared_ptr<TimeMappings> &timeMapping)
     : sourceLayerStack(clipSourceLayerStack)
     , sourcePrimPath(clipSourcePrimPath)
-    , sourceLayerIndex(clipSourceLayerIndex)
+    , sourceLayer(
+        TF_VERIFY(clipSourceLayerIndex 
+                      < clipSourceLayerStack->GetLayers().size()) ?
+            SdfLayerHandle(clipSourceLayerStack->GetLayers()[clipSourceLayerIndex]) :
+            SdfLayerHandle())
     , assetPath(clipAssetPath)
     , primPath(clipPrimPath)
     , authoredStartTime(clipAuthoredStartTime)
@@ -119,12 +123,11 @@ Usd_Clip::Usd_Clip(
     // This is important for change processing. Clip layers will be kept
     // alive during change processing, so any clips that are reconstructed
     // will have the opportunity to reuse the already-opened layer.
-    if (TF_VERIFY(sourceLayerIndex < sourceLayerStack->GetLayers().size())) {
+    if (sourceLayer) {
         const ArResolverContextBinder binder(
             sourceLayerStack->GetIdentifier().pathResolverContext);
         _layer = SdfLayer::FindRelativeToLayer(
-            sourceLayerStack->GetLayers()[sourceLayerIndex],
-            assetPath.GetAssetPath());
+            sourceLayer, assetPath.GetAssetPath());
     }
 
     _hasLayer = (bool)_layer;
@@ -737,12 +740,11 @@ Usd_Clip::_GetLayerForClip() const
 
     SdfLayerRefPtr layer;
 
-    if (TF_VERIFY(sourceLayerIndex < sourceLayerStack->GetLayers().size())) {
+    if (TF_VERIFY(sourceLayer)) {
         const ArResolverContextBinder binder(
             sourceLayerStack->GetIdentifier().pathResolverContext);
         layer = SdfLayer::FindOrOpenRelativeToLayer(
-            sourceLayerStack->GetLayers()[sourceLayerIndex],
-            assetPath.GetAssetPath());
+            sourceLayer, assetPath.GetAssetPath());
     }
 
     if (!layer) {

pxr/usd/usd/clip.h

@@ -192,11 +192,10 @@ struct Usd_Clip
     /// open, it will generally be kept open for the life of the stage.
     SdfLayerHandle GetLayerIfOpen() const;
 
-    /// Layer stack, prim spec path, and index of layer where this clip
-    /// was introduced.
+    /// Layer stack, prim spec path, and layer where this clip was introduced.
     PcpLayerStackPtr sourceLayerStack;
     SdfPath sourcePrimPath;
-    size_t sourceLayerIndex;
+    SdfLayerHandle sourceLayer;
 
     /// Asset path for the clip and the path to the prim in the clip
     /// that provides data.

pxr/usd/usd/clipSet.cpp

@@ -30,6 +30,8 @@
 #include "pxr/usd/usd/usdaFileFormat.h"
 #include "pxr/usd/usd/valueUtils.h"
 
+#include "pxr/usd/pcp/layerStack.h"
+
 #include "pxr/base/tf/staticTokens.h"
 
 #include <algorithm>
@@ -336,7 +338,9 @@ Usd_ClipSet::Usd_ClipSet(
     : name(name_)
     , sourceLayerStack(clipDef.sourceLayerStack)
     , sourcePrimPath(clipDef.sourcePrimPath)
-    , sourceLayerIndex(clipDef.indexOfLayerWhereAssetPathsFound)
+    , sourceLayer(
+        clipDef.sourceLayerStack->GetLayers()[
+            clipDef.indexOfLayerWhereAssetPathsFound])
     , clipPrimPath(SdfPath(*clipDef.clipPrimPath))
     , interpolateMissingClipValues(false)
 {

pxr/usd/usd/clipSet.h

@@ -103,7 +103,7 @@ class Usd_ClipSet
     std::string name;
     PcpLayerStackPtr sourceLayerStack;
     SdfPath sourcePrimPath;
-    size_t sourceLayerIndex;
+    SdfLayerHandle sourceLayer;
     SdfPath clipPrimPath;
     Usd_ClipRefPtr manifestClip;
     Usd_ClipRefPtrVector valueClips;

pxr/usd/usd/resolver.cpp

@@ -124,13 +124,6 @@ Usd_Resolver::_SkipEmptyNodes()
     }
 }
 
-size_t 
-Usd_Resolver::GetLayerStackIndex() const 
-{
-    return std::distance(
-        _curNode->GetLayerStack()->GetLayers().begin(), _curLayer);
-}
-
 void 
 Usd_Resolver::NextNode()
 {

pxr/usd/usd/resolver.h

@@ -118,13 +118,6 @@ class Usd_Resolver {
         return *_curLayer;
     }
 
-    /// Returns the index of the current layer in the current node's layer 
-    /// stack for a valid resolver.
-    ///
-    /// The behavior is undefined if the resolver is not valid.
-    USD_API
-    size_t GetLayerStackIndex() const;
-
     /// Returns a translated path for the current PcpNode and Layer for a valid
     /// resolver.
     ///

pxr/usd/usd/stage.cpp

@@ -7599,10 +7599,9 @@ struct UsdStage::_PropertyStackResolver {
                 if (_withLayerOffsets) {
                     // The layer offset for the clip is the layer offset of the
                     // source layer of the clip set.
-                    const auto layer = clipSet->sourceLayerStack->GetLayers()[
-                        clipSet->sourceLayerIndex];
                     propertyStackWithLayerOffsets.emplace_back(
-                        propertySpec, _GetLayerToStageOffset(node, layer)); 
+                        propertySpec,
+                        _GetLayerToStageOffset(node, clipSet->sourceLayer)); 
                 } else {
                     propertyStack.push_back(propertySpec);
                 }
@@ -7979,11 +7978,10 @@ _GetResolvedValueAtTimeWithClipsImpl(
             }
         }
 
-        const size_t layerStackIndex = res->GetLayerStackIndex();
         for (const Usd_ClipSetRefPtr& clipSet : clips) {
             // We only care about clips that were introduced at this
             // position within the LayerStack.
-            if (clipSet->sourceLayerIndex == layerStackIndex) {
+            if (clipSet->sourceLayer == res->GetLayer()) {
                 // Look through clips to see if they have a time sample for
                 // this attribute. If a time is given, examine just the clips
                 // that are active at that time.

pxr/usd/usd/testenv/testUsdValueClips.py

@@ -2530,5 +2530,98 @@ def test_TemplateFileFormatArguments(self):
         self.assertTrue(layer)
         self.assertEqual(layer.GetFileFormatArguments(), {'a': '1', 'b': 'str'})
 
+    def test_SublayerChanges(self):
+        """Test that clip layers are loaded successfully when sublayers
+        are added or removed before the clip layers are pulled on."""
+
+        def _test(stage):
+            # Query our test attribute's property stack and verify that it
+            # contains the opinions we expect. This will open the clip layer.
+            a = stage.GetAttributeAtPath('/SingleClip.attr_1')
+            propertyStack = a.GetPropertyStack(0)
+
+            rootLayer = stage.GetRootLayer()
+            sublayerWithClip = Sdf.Layer.FindRelativeToLayer(
+                rootLayer, 'layers/sublayer.usda')
+            self.assertTrue(sublayerWithClip)
+
+            clipLayer = Sdf.Layer.FindRelativeToLayer(
+                sublayerWithClip, 'clip.usda')
+            self.assertTrue(clipLayer)
+
+            refLayer = Sdf.Layer.FindRelativeToLayer(
+                rootLayer, 'layers/ref.usda')
+            self.assertTrue(refLayer)
+
+            self.assertEqual(
+                propertyStack,
+                [sublayerWithClip.GetAttributeAtPath('/SingleClip.attr_1'),
+                 clipLayer.GetAttributeAtPath('/Model.attr_1'),
+                 refLayer.GetAttributeAtPath('/Model.attr_1')])
+
+        # Test combinations of inserting and removing sublayers prior to
+        # pulling on attributes and opening clip layers. Clip layers are
+        # opened the first time the _test function is called, so these
+        # tests are separated into insert-first and remove-first to cover
+        # both cases. Empty and non-empty sublayers are also tested 
+        # separately since there's an optimization that avoids significant
+        # resyncs in the former case.
+
+        def _TestInsertAndRemoveEmptySublayer():
+            dummySublayer = Sdf.Layer.CreateAnonymous('.usda')
+            rootLayer = Sdf.Layer.FindOrOpen('sublayerChanges/root.usda')
+
+            stage = Usd.Stage.Open(rootLayer)
+            rootLayer.subLayerPaths.insert(0, dummySublayer.identifier)
+            _test(stage)
+
+            del rootLayer.subLayerPaths[0]
+            _test(stage)
+
+        def _TestRemoveAndInsertEmptySublayer():
+            dummySublayer = Sdf.Layer.CreateAnonymous('.usda')
+
+            rootLayer = Sdf.Layer.FindOrOpen('sublayerChanges/root.usda')
+            rootLayer.subLayerPaths.insert(0, dummySublayer.identifier)
+
+            stage = Usd.Stage.Open(rootLayer)
+            del rootLayer.subLayerPaths[0]
+            _test(stage)
+
+            rootLayer.subLayerPaths.insert(0, dummySublayer.identifier)
+            _test(stage)
+
+        def _TestInsertAndRemoveNonEmptySublayer():
+            dummySublayer = Sdf.Layer.CreateAnonymous('.usda')
+            Sdf.CreatePrimInLayer(dummySublayer, '/Dummy')
+
+            rootLayer = Sdf.Layer.FindOrOpen('sublayerChanges/root.usda')
+
+            stage = Usd.Stage.Open(rootLayer)
+            rootLayer.subLayerPaths.insert(0, dummySublayer.identifier)
+            _test(stage)
+
+            del rootLayer.subLayerPaths[0]
+            _test(stage)
+
+        def _TestRemoveAndInsertNonEmptySublayer():
+            dummySublayer = Sdf.Layer.CreateAnonymous('.usda')
+            Sdf.CreatePrimInLayer(dummySublayer, '/Dummy')
+
+            rootLayer = Sdf.Layer.FindOrOpen('sublayerChanges/root.usda')
+            rootLayer.subLayerPaths.insert(0, dummySublayer.identifier)
+
+            stage = Usd.Stage.Open(rootLayer)
+            del rootLayer.subLayerPaths[0]
+            _test(stage)
+
+            rootLayer.subLayerPaths.insert(0, dummySublayer.identifier)
+            _test(stage)
+
+        _TestInsertAndRemoveNonEmptySublayer()
+        _TestRemoveAndInsertNonEmptySublayer()
+        _TestInsertAndRemoveEmptySublayer()
+        _TestRemoveAndInsertEmptySublayer()
+
 if __name__ == "__main__":
     unittest.main()

pxr/usd/usd/testenv/testUsdValueClips/sublayerChanges/layers/clip.usda

@@ -0,0 +1,9 @@
+#usda 1.0
+
+def "Model"
+{
+    float attr_1.timeSamples = {
+        0: 10,
+    }
+}
+

pxr/usd/usd/testenv/testUsdValueClips/sublayerChanges/layers/clip_manifest.usda

@@ -0,0 +1,7 @@
+#usda 1.0
+
+def "Model"
+{
+    float attr_1
+}
+

pxr/usd/usd/testenv/testUsdValueClips/sublayerChanges/layers/ref.usda

@@ -0,0 +1,7 @@
+#usda 1.0
+
+def "Model"
+{
+    float attr_1 = 1
+}
+

pxr/usd/usd/testenv/testUsdValueClips/sublayerChanges/layers/sublayer.usda

@@ -0,0 +1,19 @@
+#usda 1.0
+
+over "SingleClip" (
+    clips = {
+        dictionary default = {
+            double2[] active = [(0, 0)]
+            asset[] assetPaths = [@./clip.usda@]
+            asset manifestAssetPath = @./clip_manifest.usda@
+            string primPath = "/Model"
+        }
+    }
+    references = @./ref.usda@</Model>
+)
+{
+    float attr_1.timeSamples = {
+        0: 100,
+    }
+}
+

pxr/usd/usd/testenv/testUsdValueClips/sublayerChanges/root.usda

@@ -0,0 +1,6 @@
+#usda 1.0
+(
+    subLayers = [
+        @./layers/sublayer.usda@
+    ]
+)