pcp: Fix layer muting bug with file format targets

Pcp did not consider the file format target used for prim
index computations when determining whether a given layer
should be considered muted. So, if a user added a layer
identifier with an embedded file format target to the muted
layer set (e.g., "foo.usda:SDF_FORMAT_ARGS:target=x"), and
"foo.usda" was seen during a prim index computation where
the file format target was set to "x", this layer would
not be considered muted when it should have.

When computing the "canonical" identifier used to check
if a layer is muted, Pcp now strips out the embedded
file format target argument if it's present and matches
the file format target used during prim indexing. In
the example above, layer muting would work as expected
because the canonical identifier added to the muted
layer set would be just "foo.usda".

(Internal change: 2309311)

diff --git a/pxr/usd/pcp/cache.h b/pxr/usd/pcp/cache.h
index e2944bfa31d4f192854d55bee5966e81bdea9c51..a1ca7e6e8bbf6124877f52f08730083689dea9a9 100644 (file)
--- a/pxr/usd/pcp/cache.h
+++ b/pxr/usd/pcp/cache.h
@@ -197,8 +197,11 @@ public:
     ///
     /// A canonical identifier for each layer in \p layersToMute will be
     /// computed using ArResolver::CreateIdentifier using the cache's root
-    /// layer as the anchoring asset. Any layer encountered during composition
-    /// with the same identifier will be considered muted and ignored.
+    /// layer as the anchoring asset. If an identifier contains a file
+    /// format target that matches this cache's file format target, that
+    /// argument will be removed from the identifier. Any layer encountered
+    /// during composition with the same canonical identifier will be
+    /// considered muted and ignored.
     ///
     /// Note that muting a layer will cause this cache to release all
     /// references to that layer.  If no other client is holding on to

diff --git a/pxr/usd/pcp/layerStackRegistry.cpp b/pxr/usd/pcp/layerStackRegistry.cpp
index a86899697b13933ea54978dc5c0d5ee0a8a22ccb..2dd6cdaeec4dc65e053a8ebfe65c12232ff8ccef 100644 (file)
--- a/pxr/usd/pcp/layerStackRegistry.cpp
+++ b/pxr/usd/pcp/layerStackRegistry.cpp
@@ -27,6 +27,7 @@
 #include "pxr/usd/pcp/layerStackRegistry.h"
 #include "pxr/usd/pcp/layerStack.h"
 #include "pxr/usd/pcp/layerStackIdentifier.h"
+#include "pxr/usd/pcp/utils.h"
 
 #include "pxr/usd/ar/resolver.h"
 #include "pxr/usd/sdf/layerUtils.h"
@@ -52,6 +53,7 @@ public:
         : rootLayerStackId(rootLayerStackId_)
         , fileFormatTarget(fileFormatTarget_)
         , isUsd(isUsd)
+        , mutedLayers(fileFormatTarget_)
     { }
 
     typedef SdfLayerHandleVector Layers;
@@ -366,24 +368,46 @@ Pcp_LayerStackRegistry::_GetMutedLayers() const
 
 // ------------------------------------------------------------
 
-namespace
-{
 std::string 
-_GetCanonicalLayerId(const SdfLayerHandle& anchorLayer, 
-                     const std::string& layerId)
+Pcp_MutedLayers::_GetCanonicalLayerId(const SdfLayerHandle& anchorLayer, 
+                                      const std::string& layerId) const
 {
-    if (SdfLayer::IsAnonymousLayerIdentifier(layerId)) {
-        return layerId;
+    // Split out any file format arguments embedded in layerId so we
+    // can anchor the layer path separately.
+    std::string layerPath;
+    SdfLayer::FileFormatArguments args;
+    if (!SdfLayer::SplitIdentifier(layerId, &layerPath, &args)) {
+        return std::string();
     }
-
+    
     // XXX: 
     // We may ultimately want to use the resolved path here but that's
     // possibly a bigger change and there are questions about what happens if
     // the muted path doesn't resolve to an existing asset and how/when to
     // invalidate the resolved paths stored in the Pcp_MutedLayers object.
-    return ArGetResolver().CreateIdentifier(
-        layerId, anchorLayer->GetResolvedPath());
+    const std::string anchoredPath =
+        SdfLayer::IsAnonymousLayerIdentifier(layerPath) ?
+        layerPath :
+        ArGetResolver().CreateIdentifier(
+            layerPath, anchorLayer->GetResolvedPath());
+
+    if (anchoredPath.empty()) {
+        return std::string();
+    }
+
+    // If the layer identifier specified a file format target that matches
+    // our own, we strip it off. This simplifies the matching done in
+    // IsLayerMuted, e.g. in the case where the user specified a muted
+    // layer with a file format target that matches our own, and the
+    // layer identifier being checked has no file format target.
+    Pcp_StripFileFormatTarget(_fileFormatTarget, &args);
+
+    return SdfLayer::CreateIdentifier(anchoredPath, args);
 }
+
+Pcp_MutedLayers::Pcp_MutedLayers(const std::string& fileFormatTarget)
+    : _fileFormatTarget(fileFormatTarget)
+{
 }
 
 const std::vector<std::string>& 
@@ -402,6 +426,9 @@ Pcp_MutedLayers::MuteAndUnmuteLayers(const SdfLayerHandle& anchorLayer,
     for (const auto& layerToMute : *layersToMute) {
         const std::string canonicalId = 
             _GetCanonicalLayerId(anchorLayer, layerToMute);
+        if (canonicalId.empty()) {
+            continue;
+        }
 
         const auto layerIt = std::lower_bound(
             _layers.begin(), _layers.end(), canonicalId);
@@ -414,6 +441,9 @@ Pcp_MutedLayers::MuteAndUnmuteLayers(const SdfLayerHandle& anchorLayer,
     for (const auto& layerToUnmute : *layersToUnmute) {
         const std::string canonicalId = 
             _GetCanonicalLayerId(anchorLayer, layerToUnmute);
+        if (canonicalId.empty()) {
+            continue;
+        }
 
         const auto layerIt = std::lower_bound(
             _layers.begin(), _layers.end(), canonicalId);
@@ -437,6 +467,10 @@ Pcp_MutedLayers::IsLayerMuted(const SdfLayerHandle& anchorLayer,
     }
 
     std::string canonicalId = _GetCanonicalLayerId(anchorLayer, layerId);
+    if (canonicalId.empty()) {
+        return false;
+    }
+
     if (std::binary_search(_layers.begin(), _layers.end(), canonicalId)) {
         if (canonicalLayerId) {
             canonicalLayerId->swap(canonicalId);

diff --git a/pxr/usd/pcp/layerStackRegistry.h b/pxr/usd/pcp/layerStackRegistry.h
index 1eb7139a6b623b7cfccd238f3821b6f040151e7b..3adbf4d0a29536fda731d8096dbac41625ac87ba 100644 (file)
--- a/pxr/usd/pcp/layerStackRegistry.h
+++ b/pxr/usd/pcp/layerStackRegistry.h
@@ -157,6 +157,8 @@ private:
 class Pcp_MutedLayers
 {
 public:
+    explicit Pcp_MutedLayers(const std::string& fileFormatTarget);
+
     const std::vector<std::string>& GetMutedLayers() const;
     void MuteAndUnmuteLayers(const SdfLayerHandle& anchorLayer,
                              std::vector<std::string>* layersToMute,
@@ -166,6 +168,11 @@ public:
                       std::string* canonicalLayerIdentifier = nullptr) const;
 
 private:
+    std::string 
+    _GetCanonicalLayerId(const SdfLayerHandle& anchorLayer, 
+                         const std::string& layerId) const;
+
+    std::string _fileFormatTarget;
     std::vector<std::string> _layers;
 };
 

diff --git a/pxr/usd/pcp/testenv/testPcpLayerMuting.py b/pxr/usd/pcp/testenv/testPcpLayerMuting.py
index 7c42cd16857acdc10bd035694fef32bcac2359da..c3ff8a060318aa6f3075c6955445577330d6880d 100644 (file)
--- a/pxr/usd/pcp/testenv/testPcpLayerMuting.py
+++ b/pxr/usd/pcp/testenv/testPcpLayerMuting.py
@@ -29,10 +29,12 @@ class TestPcpLayerMuting(unittest.TestCase):
     def _LoadLayer(self, layerPath):
         return Sdf.Layer.FindOrOpen(layerPath)
 
-    def _LoadPcpCache(self, layerPath, sessionLayerPath=None):
+    def _LoadPcpCache(self, layerPath, sessionLayerPath=None,
+                      fileFormatTarget=None):
         layer = self._LoadLayer(layerPath)
         sessionLayer = None if not sessionLayerPath else self._LoadLayer(sessionLayerPath)
-        return Pcp.Cache(Pcp.LayerStackIdentifier(layer, sessionLayer))
+        return Pcp.Cache(Pcp.LayerStackIdentifier(layer, sessionLayer),
+                         fileFormatTarget=fileFormatTarget or '')
 
     def test_MutingSublayers(self):
         """Tests muting sublayers"""
@@ -57,6 +59,7 @@ class TestPcpLayerMuting(unittest.TestCase):
                     [layer.GetPrimAtPath('/Root'),
                      sublayer.GetPrimAtPath('/Root'),
                      anonymousSublayer.GetPrimAtPath('/Root')])
+        self.assertEqual(pcp.GetMutedLayers(), [])
         self.assertEqual(pi.rootNode.layerStack.mutedLayers, [])
 
         (pi2, err2) = pcp2.ComputePrimIndex('/Root')
@@ -65,6 +68,7 @@ class TestPcpLayerMuting(unittest.TestCase):
                     [layer.GetPrimAtPath('/Root'),
                      sublayer.GetPrimAtPath('/Root'),
                      anonymousSublayer.GetPrimAtPath('/Root')])
+        self.assertEqual(pcp2.GetMutedLayers(), [])
         self.assertEqual(pi2.rootNode.layerStack.mutedLayers, [])
 
         # Muting the cache's root layer is explicitly disallowed.
@@ -79,6 +83,7 @@ class TestPcpLayerMuting(unittest.TestCase):
         self.assertEqual(pi.primStack, 
                     [layer.GetPrimAtPath('/Root'),
                      anonymousSublayer.GetPrimAtPath('/Root')])
+        self.assertEqual(pcp.GetMutedLayers(), [sublayer.identifier])
         self.assertEqual(pi.rootNode.layerStack.mutedLayers, 
                          [sublayer.identifier])
 
@@ -88,6 +93,7 @@ class TestPcpLayerMuting(unittest.TestCase):
                     [layer.GetPrimAtPath('/Root'),
                      sublayer.GetPrimAtPath('/Root'),
                      anonymousSublayer.GetPrimAtPath('/Root')])
+        self.assertEqual(pcp2.GetMutedLayers(), [])
         self.assertEqual(pi2.rootNode.layerStack.mutedLayers, [])
 
         # Unmute sublayer and verify that it comes back into /Root's
@@ -99,6 +105,7 @@ class TestPcpLayerMuting(unittest.TestCase):
                     [layer.GetPrimAtPath('/Root'),
                      sublayer.GetPrimAtPath('/Root'),
                      anonymousSublayer.GetPrimAtPath('/Root')])
+        self.assertEqual(pcp.GetMutedLayers(), [])
         self.assertEqual(pi.rootNode.layerStack.mutedLayers, [])
 
         (pi2, err2) = pcp2.ComputePrimIndex('/Root')
@@ -107,6 +114,7 @@ class TestPcpLayerMuting(unittest.TestCase):
                     [layer.GetPrimAtPath('/Root'),
                      sublayer.GetPrimAtPath('/Root'),
                      anonymousSublayer.GetPrimAtPath('/Root')])
+        self.assertEqual(pcp2.GetMutedLayers(), [])
         self.assertEqual(pi2.rootNode.layerStack.mutedLayers, [])
 
         # Mute sublayer and verify that change processing has occurred
@@ -118,6 +126,7 @@ class TestPcpLayerMuting(unittest.TestCase):
                     [layer.GetPrimAtPath('/Root'),
                      sublayer.GetPrimAtPath('/Root')])
         self.assertTrue(anonymousSublayer)
+        self.assertEqual(pcp.GetMutedLayers(), [anonymousSublayer.identifier])
         self.assertEqual(pi.rootNode.layerStack.mutedLayers, 
                          [anonymousSublayer.identifier])
 
@@ -127,6 +136,7 @@ class TestPcpLayerMuting(unittest.TestCase):
                     [layer.GetPrimAtPath('/Root'),
                      sublayer.GetPrimAtPath('/Root'),
                      anonymousSublayer.GetPrimAtPath('/Root')])
+        self.assertEqual(pcp2.GetMutedLayers(), [])
         self.assertEqual(pi2.rootNode.layerStack.mutedLayers, [])
 
         # Unmute sublayer and verify that it comes back into /Root's
@@ -138,6 +148,7 @@ class TestPcpLayerMuting(unittest.TestCase):
                     [layer.GetPrimAtPath('/Root'),
                      sublayer.GetPrimAtPath('/Root'),
                      anonymousSublayer.GetPrimAtPath('/Root')])
+        self.assertEqual(pcp.GetMutedLayers(), [])
         self.assertEqual(pi.rootNode.layerStack.mutedLayers, [])
 
         (pi2, err2) = pcp2.ComputePrimIndex('/Root')
@@ -146,6 +157,7 @@ class TestPcpLayerMuting(unittest.TestCase):
                     [layer.GetPrimAtPath('/Root'),
                      sublayer.GetPrimAtPath('/Root'),
                      anonymousSublayer.GetPrimAtPath('/Root')])
+        self.assertEqual(pcp2.GetMutedLayers(), [])
         self.assertEqual(pi2.rootNode.layerStack.mutedLayers, [])
         
     def test_MutingSessionLayer(self):
@@ -169,6 +181,7 @@ class TestPcpLayerMuting(unittest.TestCase):
         self.assertTrue(not err)
         self.assertEqual(pi.primStack,
                     [layer.GetPrimAtPath('/Root')])
+        self.assertEqual(pcp.GetMutedLayers(), [sessionLayer.identifier])
         self.assertEqual(pi.rootNode.layerStack.mutedLayers, 
                          [sessionLayer.identifier])
 
@@ -179,6 +192,7 @@ class TestPcpLayerMuting(unittest.TestCase):
         self.assertEqual(pi.primStack,
                     [sessionLayer.GetPrimAtPath('/Root'),
                      layer.GetPrimAtPath('/Root')])
+        self.assertEqual(pcp.GetMutedLayers(), [])
         self.assertEqual(pi.rootNode.layerStack.mutedLayers, [])
 
     def test_MutingReferencedLayers(self):
@@ -205,6 +219,8 @@ class TestPcpLayerMuting(unittest.TestCase):
         # result in change processing, a composition error when recomposing
         # /Root, and no reference node on the prim index.
         pcp.RequestLayerMuting([refLayer.identifier], [])
+        self.assertEqual(pcp.GetMutedLayers(), [refLayer.identifier])
+
         (pi, err) = pcp.ComputePrimIndex('/Root')
         self.assertTrue(err)
         self.assertEqual(len(pi.rootNode.children), 0)
@@ -217,6 +233,8 @@ class TestPcpLayerMuting(unittest.TestCase):
         # Unmute the layer and verify that the composition error is resolved
         # and the reference is restored to the prim index.
         pcp.RequestLayerMuting([], [refLayer.identifier])
+        self.assertEqual(pcp.GetMutedLayers(), [])
+
         (pi, err) = pcp.ComputePrimIndex('/Root')
         self.assertTrue(not err)
         self.assertEqual(pi.rootNode.children[0].arcType, Pcp.ArcTypeReference)
@@ -227,5 +245,58 @@ class TestPcpLayerMuting(unittest.TestCase):
         self.assertEqual(pi2.rootNode.children[0].arcType, Pcp.ArcTypeReference)
         self.assertEqual(pi2.rootNode.children[0].layerStack.layers[0], refLayer)
 
+    def test_MutingWithFileFormatTarget(self):
+        """Tests layer muting for a Pcp.Cache with a file format target."""
+
+        def _HasInvalidSublayerError(errors):
+            return (Pcp.ErrorType_InvalidSublayerPath
+                    in (e.errorType for e in errors))
+
+        def _TestMuting(mutedId, 
+                        expectMutedLayerId=None,
+                        expectInvalidSublayerError=False):
+
+            expectMutedLayerId = expectMutedLayerId or mutedId
+
+            # Create a Pcp.Cache with a 'bogus' file format target. This
+            # target will be used for loading layers during composition.
+            pcp = self._LoadPcpCache(
+                'sublayers/root.sdf', fileFormatTarget='bogus')
+
+            # Normally, we'd be unable to load the sublayer specified in
+            # root.sdf because there is no file format plugin that handles
+            # the 'bogus' file format target. This would cause an invalid
+            # sublayer error during composition.
+            #
+            # To avoid this, we're going to mute the sublayer before we
+            # compose anything.
+            pcp.RequestLayerMuting([mutedId], [])
+            self.assertEqual(pcp.GetMutedLayers(), [expectMutedLayerId])
+
+            # Compute prim index. We should not get an invalid sublayer error
+            # because the sublayer was muted above.
+            (pi, err) = pcp.ComputePrimIndex('/Root')
+
+            if not expectInvalidSublayerError:
+                self.assertFalse(_HasInvalidSublayerError(err))
+
+        sublayerPath = os.path.join(os.getcwd(), 'sublayers/sublayer.sdf')
+
+        # Test using an identifier with no target specified.
+        _TestMuting(sublayerPath, expectMutedLayerId=sublayerPath)
+
+        # Test using an identifier with a target that matches the target
+        # used by the Pcp.Cache.
+        _TestMuting(
+            Sdf.Layer.CreateIdentifier(sublayerPath, {'target':'bogus'}),
+            expectMutedLayerId=sublayerPath)
+
+        # Test using an identifier with a target that does not match the
+        # target used by the Pcp.Cache. In this case, the sublayer will
+        # not be muted, so we expect an invalid sublayer composition error.
+        _TestMuting(
+            Sdf.Layer.CreateIdentifier(sublayerPath, {'target':'bogus2'}),
+            expectInvalidSublayerError=True)
+
 if __name__ == "__main__":
     unittest.main()

diff --git a/pxr/usd/pcp/utils.cpp b/pxr/usd/pcp/utils.cpp
index f02436bba293cd67b5abd87e8406a074aa86ff35..450ff8121979f9e848c5cead7171d1b6997ec76c 100644 (file)
--- a/pxr/usd/pcp/utils.cpp
+++ b/pxr/usd/pcp/utils.cpp
@@ -147,6 +147,17 @@ Pcp_GetArgumentsForFileFormatTarget(
     return *localArgs;
 }
 
+void
+Pcp_StripFileFormatTarget(
+    const std::string& target,
+    SdfLayer::FileFormatArguments* args)
+{
+    auto targetIt = args->find(SdfFileFormatTokens->TargetArg);
+    if (targetIt != args->end() && targetIt->second == target) {
+        args->erase(targetIt);
+    }
+}
+
 std::pair<PcpNodeRef, PcpNodeRef>
 Pcp_FindStartingNodeOfClassHierarchy(const PcpNodeRef& n)
 {

diff --git a/pxr/usd/pcp/utils.h b/pxr/usd/pcp/utils.h
index 9d1c3f586b995353ff7ed465a6fcc9a4b126f064..1ca22790e6f3b1e687da83c7913ff7113f10e740 100644 (file)
--- a/pxr/usd/pcp/utils.h
+++ b/pxr/usd/pcp/utils.h
@@ -105,6 +105,13 @@ Pcp_GetArgumentsForFileFormatTarget(
     const SdfLayer::FileFormatArguments* defaultArgs,
     SdfLayer::FileFormatArguments* localArgs);
 
+// Removes the "target" argument from \p args if it exists and its value
+// is the same as \p target.
+void
+Pcp_StripFileFormatTarget(
+    const std::string& target,
+    SdfLayer::FileFormatArguments* args);
+
 // Find the starting node of the class hierarchy of which node n is a part.
 // This is the prim that starts the class chain, aka the 'instance' of the
 // class hierarchy. Also returns the node for the first class in the