From 715ccf7bff63bd4a7ddf897ded87c02889ce9875 Mon Sep 17 00:00:00 2001 From: sunyab Date: Mon, 4 Dec 2023 09:44:38 -0800 Subject: [PATCH] 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 | 65 +++++++-- .../testenv/testPcpDynamicFileFormatPlugin.py | 127 ++++++++++++++++++ .../root.sdf | 40 ++++++ pxr/usd/pcp/utils.cpp | 55 ++++++++ pxr/usd/pcp/utils.h | 11 ++ 5 files changed, 288 insertions(+), 10 deletions(-) diff --git a/pxr/usd/pcp/dynamicFileFormatContext.cpp b/pxr/usd/pcp/dynamicFileFormatContext.cpp index 7504e6c878..eefa0b987d 100644 --- a/pxr/usd/pcp/dynamicFileFormatContext.cpp +++ b/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 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 + 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; diff --git a/pxr/usd/pcp/testenv/testPcpDynamicFileFormatPlugin.py b/pxr/usd/pcp/testenv/testPcpDynamicFileFormatPlugin.py index 0857772818..09913007ea 100644 --- a/pxr/usd/pcp/testenv/testPcpDynamicFileFormatPlugin.py +++ b/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@ + ) + { + 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 diff --git a/pxr/usd/pcp/testenv/testPcpDynamicFileFormatPlugin.testenv/root.sdf b/pxr/usd/pcp/testenv/testPcpDynamicFileFormatPlugin.testenv/root.sdf index f8f7fd00d9..688f0efff5 100644 --- a/pxr/usd/pcp/testenv/testPcpDynamicFileFormatPlugin.testenv/root.sdf +++ b/pxr/usd/pcp/testenv/testPcpDynamicFileFormatPlugin.testenv/root.sdf @@ -26,3 +26,43 @@ references = [@./params.sdf@, @./payload.sdf@] { } +def "Variant" ( + TestPcp_depth = 5 + variantSets = "x" + variants = { + string x = "a" + } +) +{ + int TestPcp_depth = 5 + + variantSet "x" = { + "a" ( + TestPcp_num = 4 + references = [@./params.sdf@, @./payload.sdf@] + ) + { + int TestPcp_num = 4 + } + } +} + +def "SubrootReference" ( + TestPcp_depth = 1 + TestPcp_num = 1 + references = +) +{ + int TestPcp_depth = 1 + int TestPcp_num = 1 +} + +def "SubrootReferenceAndVariant" ( + TestPcp_depth = 1 + TestPcp_num = 1 + references = +) +{ + int TestPcp_depth = 1 + int TestPcp_num = 1 +} \ No newline at end of file diff --git a/pxr/usd/pcp/utils.cpp b/pxr/usd/pcp/utils.cpp index 65f8c3cc25..f02436bba2 100644 --- a/pxr/usd/pcp/utils.cpp +++ b/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 +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 diff --git a/pxr/usd/pcp/utils.h b/pxr/usd/pcp/utils.h index f86f2a49bb..9d1c3f586b 100644 --- a/pxr/usd/pcp/utils.h +++ b/pxr/usd/pcp/utils.h @@ -193,6 +193,17 @@ Pcp_GetArgumentsForFileFormatTarget( std::pair 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 +Pcp_TranslatePathFromNodeToRootOrClosestNode( + const PcpNodeRef& node, + const SdfPath& path); + PXR_NAMESPACE_CLOSE_SCOPE #endif // PXR_USD_PCP_UTILS_H