pcp: Fixes for dynamic payload argument composition

When handling ancestral payloads during recursive prim indexing,
PcpDynamicFileFormatContext did not account for differences in
namespace depth when traversing across stack frames. This
caused incorrect behavior where arguments authored on a prim
would affect ancestral payloads.

For example, if Pcp were composing prim /Root, and that prim
referenced /DynamicPayload/Child, Pcp would recursively compose
/DynamicPayload/Child to incorporate ancestral opinions from
/DynamicPayload. While composing /DynamicPayload, Pcp would
incorrectly consider arguments authored on /Root. This is
inconsistent with Pcp's handling of composition arcs like
variants in the same situation: variant selections authored
on /Root would not apply to variant sets authored on
/DynamicPayload.

Instead of using each node's exact path, we now map paths across
composition arcs to maintain the correct namespace depth as we
traverse the prim index.

This change also fixes a bug where composition graph subtrees
would be redundantly traversed when performing non-strongest-first
argument composition.

(Internal change: 2306710)

pxr/usd/pcp/dynamicFileFormatContext.cpp

@@ -27,6 +27,7 @@
 #include "pxr/usd/pcp/layerStack.h"
 #include "pxr/usd/pcp/node_Iterator.h"
 #include "pxr/usd/pcp/primIndex_StackFrame.h"
+#include "pxr/usd/pcp/utils.h"
 #include "pxr/usd/sdf/layer.h"
 #include "pxr/base/vt/value.h"
 
@@ -81,13 +82,14 @@ class PcpDynamicFileFormatContext::_ComposeValueHelper
     // composition should stop.
     template <typename ComposeFunc>
     bool _ComposeOpinionInSubtree(const PcpNodeRef &node,
+                                  const SdfPath &pathInNode,
                                   const TfToken &propName,
                                   const TfToken &fieldName, 
                                   const ComposeFunc &composeFunc)
     {
         // Get the prim or property path within the node's spec.
         const SdfPath &path = propName.IsEmpty() ? 
-            node.GetPath() : node.GetPath().AppendProperty(propName);
+            pathInNode : pathInNode.AppendProperty(propName);
 
         // Search the node's layer stack in strength order for the field on
         // the spec.
@@ -105,8 +107,27 @@ class PcpDynamicFileFormatContext::_ComposeValueHelper
         }
 
         TF_FOR_ALL(childNode, Pcp_GetChildrenRange(node)) {
+            // Map the path in this node to the next child node, also applying
+            // any variant selections represented by the child node.
+            SdfPath pathInChildNode = 
+                childNode->GetMapToParent().MapTargetToSource(
+                    pathInNode.StripAllVariantSelections());
+            if (pathInChildNode.IsEmpty()) {
+                continue;
+            }
+
+            if (const SdfPath childNodePathAtIntro =
+                    childNode->GetPathAtIntroduction();
+                childNodePathAtIntro.ContainsPrimVariantSelection()) {
+
+                pathInChildNode = pathInChildNode.ReplacePrefix(
+                    childNodePathAtIntro.StripAllVariantSelections(),
+                    childNodePathAtIntro);
+            }
+
             if (_ComposeOpinionInSubtree(
-                    *childNode, propName, fieldName, composeFunc)) {
+                    *childNode, pathInChildNode, 
+                    propName, fieldName, composeFunc)) {
                 return true;
             }
         }
@@ -123,21 +144,45 @@ class PcpDynamicFileFormatContext::_ComposeValueHelper
         const TfToken &fieldName, 
         const ComposeFunc &composeFunc)
     {
-        PcpNodeRef currentNode = _iterator.node;
+        return _ComposeOpinionFromAncestors(
+            _iterator.node, _iterator.node.GetPath(),
+            propName, fieldName, composeFunc);
+    }
+
+    template <typename ComposeFunc>
+    bool _ComposeOpinionFromAncestors(
+        const PcpNodeRef &node,
+        const SdfPath &pathInNode,
+        const TfToken &propName,
+        const TfToken &fieldName, 
+        const ComposeFunc &composeFunc)
+    {
+        // Translate the path from the given node's namespace to
+        // the root of the node's prim index.
+        const auto [rootmostPath, rootmostNode] = 
+            Pcp_TranslatePathFromNodeToRootOrClosestNode(node, pathInNode);
+
+        // If we were able to translate the path all the way to the root
+        // node, and we're in the middle of a recursive prim indexing
+        // call, map across the previous frame and recurse.
+        if (rootmostNode.IsRootNode() && _iterator.previousFrame) {
+            PcpNodeRef parentNode = _iterator.previousFrame->parentNode;
+            SdfPath parentNodePath = _iterator.previousFrame->arcToParent->
+                mapToParent.MapSourceToTarget(
+                    rootmostPath.StripAllVariantSelections());
+
+            _iterator.NextFrame();
 
-        // Try parent node.
-        _iterator.Next();
-        if (_iterator.node) {
-            // Recurse on parent node's ancestors.
             if (_ComposeOpinionFromAncestors(
-                    propName, fieldName, composeFunc)) {
+                    parentNode, parentNodePath, propName, fieldName,
+                    composeFunc)) {
                 return true;
             }
         }
 
-        // Otherwise compose from the current node and its subtrees.
+        // Compose opinions in the subtree.
         if (_ComposeOpinionInSubtree(
-                currentNode, propName, fieldName, composeFunc)) {
+                rootmostNode, rootmostPath, propName, fieldName, composeFunc)) {
             return true;
         }
         return false;

pxr/usd/pcp/testenv/testPcpDynamicFileFormatPlugin.py

@@ -333,6 +333,133 @@ def test_BasicRead(self):
 
         print("test_BasicRead Success!\n")
 
+    def test_Variants(self):
+        # Test dynamic payloads and arguments authored in variants.
+
+        rootLayerFile = 'root.sdf'
+        rootLayer = Sdf.Layer.FindOrOpen(rootLayerFile)
+        self.assertTrue(rootLayer)
+        cache = self._CreatePcpCache(rootLayer)
+
+        # /Variant overrides the TestPcp_depth and TestPcp_num values that are
+        # originally defined in params.sdf.
+        payloads = self._GeneratePrimIndexPaths("/Variant", 5, 4, 341)
+        cache.RequestPayloads(payloads, [])
+
+        self._ComputeAndVerifyDynamicPayloads(cache, payloads, 
+             ["TestPcp_depth", "TestPcp_height", "TestPcp_num", "TestPcp_radius"])
+
+    def test_AncestralPayloads(self):
+        # Test that loading a dynamic payload when composing ancestral
+        # opinions picks up the right arguments.
+
+        rootLayerFile = 'root.sdf'
+        rootLayer = Sdf.Layer.FindOrOpen(rootLayerFile)
+        self.assertTrue(rootLayer)
+        cache = self._CreatePcpCache(rootLayer)
+
+        # Request payload for /RootCone.
+        # 
+        # XXX:
+        # This is a bug. This is currently needed for the payload to be loaded
+        # when we compose /SubrootReference below. However, /RootCone is an
+        # ancestral payload to /SubrootReference and should be automatically
+        # included.
+        cache.RequestPayloads([Sdf.Path("/RootCone")], [])
+
+        # Compute the prim index for /SubrootReference, which references
+        # /RootCone/Xform__3_2. When composing that prim index to incorporate
+        # ancestral opinions, Pcp should ignore the parameters authored on
+        # /SubrootReference because they do not map to the payload on
+        # /RootCone. If those parameters were *not* ignored, the prim being
+        # referenced would not be found leading to incorrect unresolved prim
+        # path errors.
+        pi, err = cache.ComputePrimIndex("/SubrootReference")
+        self.assertFalse(err)
+        self.assertTrue(
+            (Sdf.Layer.Find("cone.testpcpdynamic", 
+                            args={ "TestPcp_depth" : "4",
+                                   "TestPcp_num" : "3",
+                                   "TestPcp_radius" : "50" })
+             .GetPrimAtPath("/Root/Xform__3_2"))
+            in pi.primStack)
+
+    def test_AncestralPayloads2(self):
+        # Similar to test_AncestralPayloads but adds a non-internal reference
+        # arc to further exercise path translation logic during argument
+        # composition.
+
+        rootLayer = Sdf.Layer.CreateAnonymous()
+        rootLayer.ImportFromString("""
+        #sdf 1.4.32
+
+        def "Root" (
+            TestPcp_depth = 1
+            TestPcp_num = 1
+            references = @./root.sdf@</SubrootReference>
+        )
+        {
+            int TestPcp_depth = 1
+            int TestPcp_num = 1
+        }
+        """.strip())
+
+        primSpec = Sdf.CreatePrimInLayer(rootLayer, "/Root")
+        primSpec.referenceList.explicitItems = [ 
+            Sdf.Reference("root.sdf", "/SubrootReference") 
+        ]
+        cache = self._CreatePcpCache(rootLayer)
+
+        # Compute the prim index for /Root. We should not see any composition
+        # errors for the same reasons mentioned in test_AncestralPayloads.
+        # 
+        # XXX: Note that requesting the payload for /Root is not necessary
+        # since this case does not run into the same bug mentioned above.
+        pi, err = cache.ComputePrimIndex("/Root")
+        self.assertFalse(err)
+        self.assertTrue(
+            (Sdf.Layer.Find("cone.testpcpdynamic", 
+                            args={ "TestPcp_depth" : "4",
+                                   "TestPcp_num" : "3",
+                                   "TestPcp_radius" : "50" })
+             .GetPrimAtPath("/Root/Xform__3_2"))
+            in pi.primStack)
+
+    def test_AncestralPayloadsAndVariants(self):
+        # Test that loading a dynamic payload when composing ancestral
+        # opinions within variants picks up the right arguments.
+
+        rootLayerFile = 'root.sdf'
+        rootLayer = Sdf.Layer.FindOrOpen(rootLayerFile)
+        self.assertTrue(rootLayer)
+        cache = self._CreatePcpCache(rootLayer)
+
+        # Request payload for /Variant.
+        # 
+        # XXX:
+        # This is a bug. This is currently needed for the payload to be loaded
+        # when we compose /SubrootReference below. However, /Variant is an
+        # ancestral payload to /SubrootReferenceAndVariant and should be
+        # automatically included.
+        cache.RequestPayloads([Sdf.Path("/Variant")], [])
+
+        # Compute the prim index for /SubrootReferenceAndVariant, which
+        # references /Variant/Xform__4_3. When composing that prim index to
+        # incorporate ancestral opinions, Pcp should ignore the parameters
+        # authored on /SubrootReferenceAndVariant because they do not map to the
+        # payload on /Variant. If those parameters were *not* ignored, the prim
+        # being referenced would not be found leading to incorrect unresolved
+        # prim path errors.
+        pi, err = cache.ComputePrimIndex("/SubrootReferenceAndVariant")
+        self.assertFalse(err)
+        self.assertTrue(
+            (Sdf.Layer.Find("cone.testpcpdynamic", 
+                            args={ "TestPcp_depth" : "5",
+                                   "TestPcp_num" : "4",
+                                   "TestPcp_radius" : "50" })
+             .GetPrimAtPath("/Root/Xform__4_3"))
+            in pi.primStack)
+
     def test_Changes(self):
         # Change processing behavior can be different for Pcp caches in USD mode
         # vs not (especially in regards to property change processing). Run the

pxr/usd/pcp/testenv/testPcpDynamicFileFormatPlugin.testenv/root.sdf

@@ -26,3 +26,43 @@ references = [@./params.sdf@</Params>, @./payload.sdf@</PayloadMulti>]
 {
 }
 
+def "Variant" (
+    TestPcp_depth = 5
+    variantSets = "x"
+    variants = {
+        string x = "a"
+    }
+)
+{
+    int TestPcp_depth = 5
+
+    variantSet "x" = {
+       "a" (
+           TestPcp_num = 4
+           references = [@./params.sdf@</Params>, @./payload.sdf@</PayloadCone>]
+       )
+       {
+           int TestPcp_num = 4
+       }
+    }
+}
+
+def "SubrootReference" (
+    TestPcp_depth = 1
+    TestPcp_num = 1
+    references = </RootCone/Xform__3_2>
+)
+{
+    int TestPcp_depth = 1
+    int TestPcp_num = 1
+}
+
+def "SubrootReferenceAndVariant" (
+    TestPcp_depth = 1
+    TestPcp_num = 1
+    references = </Variant/Xform__4_3>
+)
+{
+    int TestPcp_depth = 1
+    int TestPcp_num = 1
+}

pxr/usd/pcp/utils.cpp

@@ -28,6 +28,7 @@
 #include "pxr/usd/pcp/utils.h"
 
 #include "pxr/usd/pcp/expressionVariables.h"
+#include "pxr/usd/pcp/mapExpression.h"
 #include "pxr/usd/sdf/fileFormat.h"
 #include "pxr/usd/sdf/layer.h"
 #include "pxr/usd/sdf/variableExpression.h"
@@ -165,4 +166,58 @@ Pcp_FindStartingNodeOfClassHierarchy(const PcpNodeRef& n)
     return std::make_pair(instanceNode, classNode);
 }
 
+std::pair<SdfPath, PcpNodeRef>
+Pcp_TranslatePathFromNodeToRootOrClosestNode(
+    const PcpNodeRef& node,
+    const SdfPath& path)
+{
+    if (node.IsRootNode()) {
+        // If the given node is already the root node, nothing to do.
+        return std::make_pair(path, node);
+    }
+
+    // Start at the given node and path. We strip all variant selections
+    // from the path because namespace mappings never include them.
+    PcpNodeRef curNode = node;
+    SdfPath curPath = path.StripAllVariantSelections();
+
+    // First, try translating directly to the root node. If that fails,
+    // walk up from the given node to the root node, translating at each
+    // step until the translation fails.
+    if (SdfPath pathInRootNode = node.GetMapToRoot().MapSourceToTarget(path); 
+        !pathInRootNode.IsEmpty()) {
+        curNode = node.GetRootNode();
+        curPath = std::move(pathInRootNode);
+    }
+    else {
+        while (!curNode.IsRootNode()) {
+            SdfPath pathInParentNode = curNode.GetMapToParent()
+                .MapSourceToTarget(curPath);
+            if (pathInParentNode.IsEmpty()) {
+                break;
+            }
+
+            curNode = curNode.GetParentNode();
+            curPath = std::move(pathInParentNode);
+        }
+    }
+
+    // If curNode's path contains a variant selection, do a prefix
+    // replacement to apply that selection to the translated path.
+    //
+    // We don't check curNode.GetArcType() == PcpArcTypeVariant
+    // because curNode may be the root node of a prim index that
+    // is being recursively computed to pick up ancestral opinions.
+    // In that case, curNode's "real" arc type once it's added to
+    // main prim index being computed isn't available here.
+    if (const SdfPath pathAtIntro = curNode.GetPathAtIntroduction();
+        pathAtIntro.ContainsPrimVariantSelection()) {
+
+        curPath = curPath.ReplacePrefix(
+            pathAtIntro.StripAllVariantSelections(), pathAtIntro);
+    }
+
+    return std::make_pair(curPath, curNode);
+}
+
 PXR_NAMESPACE_CLOSE_SCOPE

pxr/usd/pcp/utils.h

@@ -193,6 +193,17 @@ Pcp_GetArgumentsForFileFormatTarget(
 std::pair<PcpNodeRef, PcpNodeRef>
 Pcp_FindStartingNodeOfClassHierarchy(const PcpNodeRef& n);
 
+// Translate the given path (which must be a prim or prim variant selection
+// path) from the namespace of the given node to the namespace of the root node
+// of the prim index that node belongs to. If that translation succeeds, returns
+// the translated path and the root node. If that translation fails, translate
+// the path to the ancestor node closest to the root node where the mapping is
+// successful and return the translated path and the ancestor node.
+std::pair<SdfPath, PcpNodeRef>
+Pcp_TranslatePathFromNodeToRootOrClosestNode(
+    const PcpNodeRef& node,
+    const SdfPath& path);
+
 PXR_NAMESPACE_CLOSE_SCOPE
 
 #endif // PXR_USD_PCP_UTILS_H