Skip to content

Commit

Permalink
usd: Fix up value resolution for composed value types not to compose
Browse files Browse the repository at this point in the history 
over the initial out-parameter value.

sdf: Fix a bug in SdfPathExpression where the Nothing expression would
compose as-if it was no-opinion (i.e. '%_').

(Internal change: 2343335)
  • Loading branch information
@gitamohr @pixar-oss
gitamohr authored and pixar-oss committed Oct 3, 2024
1 parent 1b1f20d commit a9c7195
Show file tree
Hide file tree
Showing 4 changed files with 104 additions and 77 deletions.
1 change: 0 additions & 1 deletion pxr/usd/sdf/pathExpression.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -368,7 +368,6 @@ SdfPathExpression
SdfPathExpression::ComposeOver(SdfPathExpression const &weaker) &&
{
if (IsEmpty()) {
*this = weaker;
return std::move(*this);
}
auto resolve = [&weaker](ExpressionReference const &ref) {
Expand Down
7 changes: 7 additions & 0 deletions pxr/usd/sdf/testenv/testSdfPathExpression.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -221,13 +221,20 @@ TestBasics()
TF_AXIOM(!composed.ContainsExpressionReferences());
TF_AXIOM(!composed.ContainsWeakerExpressionReference());
TF_AXIOM(composed.IsComplete());
TF_AXIOM(composed == SdfPathExpression { "/a /b /c" });

auto eval = MatchEval { composed };

TF_AXIOM(eval.Match(SdfPath("/a")));
TF_AXIOM(eval.Match(SdfPath("/b")));
TF_AXIOM(eval.Match(SdfPath("/c")));
TF_AXIOM(!eval.Match(SdfPath("/d")));

// Nothing over something should be Nothing.
const auto nothing = SdfPathExpression::Nothing();
TF_AXIOM(nothing.ComposeOver(a) == nothing);
TF_AXIOM(nothing.ComposeOver(b) == nothing);
TF_AXIOM(nothing.ComposeOver(c) == nothing);
}

{
Expand Down
144 changes: 68 additions & 76 deletions pxr/usd/usd/stage.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6068,24 +6068,30 @@ _TryResolvePathExprs(Storage storage, UsdObject const &obj,
{
// Resolve.
if (_IsHolding<SdfPathExpression>(storage)) {
SdfPathExpression expr;
_UncheckedSwap(storage, expr);
expr = _MapPathExpressionToPrim(
expr, node.GetMapToRoot().Evaluate(),
Usd_StageImplAccess::GetPrimProtoToInstancePathMap(obj.GetPrim()));
_UncheckedSwap(storage, expr);
if (node) {
SdfPathExpression expr;
_UncheckedSwap(storage, expr);
expr = _MapPathExpressionToPrim(
expr, node.GetMapToRoot().Evaluate(),
Usd_StageImplAccess::GetPrimProtoToInstancePathMap(
obj.GetPrim()));
_UncheckedSwap(storage, expr);
}
return true;
}
else if (_IsHolding<VtArray<SdfPathExpression>>(storage)) {
VtArray<SdfPathExpression> exprs;
_UncheckedSwap(storage, exprs);
auto protoToInstMap =
Usd_StageImplAccess::GetPrimProtoToInstancePathMap(obj.GetPrim());
PcpMapFunction const &mapFn = node.GetMapToRoot().Evaluate();
for (SdfPathExpression &expr: exprs) {
expr = _MapPathExpressionToPrim(expr, mapFn, protoToInstMap);
}
_UncheckedSwap(storage, exprs);
if (node) {
VtArray<SdfPathExpression> exprs;
_UncheckedSwap(storage, exprs);
auto protoToInstMap =
Usd_StageImplAccess::GetPrimProtoToInstancePathMap(
obj.GetPrim());
PcpMapFunction const &mapFn = node.GetMapToRoot().Evaluate();
for (SdfPathExpression &expr: exprs) {
expr = _MapPathExpressionToPrim(expr, mapFn, protoToInstMap);
}
_UncheckedSwap(storage, exprs);
}
return true;
}
return false;
Expand Down Expand Up @@ -6172,6 +6178,7 @@ struct ValueComposerBase
bool forFlattening = false)
: _value(s)
, _object(object)
, _foundAComposingValue(false)
, _done(false)
, _forFlattening(forFlattening)
{}
Expand Down Expand Up @@ -6224,10 +6231,13 @@ struct ValueComposerBase
_object, _value, { &stage, layer, specPath, node },
context, &layerOffsetAccess, _forFlattening)) {
// Merge the resolved dictionary.
VtDictionaryOverRecursive(
&tmpDict, _UncheckedGet<VtDictionary>(_value));
_UncheckedSwap(_value, tmpDict);
}
if (_foundAComposingValue) {
VtDictionaryOverRecursive(
&tmpDict, _UncheckedGet<VtDictionary>(_value));
_UncheckedSwap(_value, tmpDict);
}
}
_foundAComposingValue = true;
return true;
}
return false;
Expand All @@ -6249,22 +6259,21 @@ struct ValueComposerBase
if (_GetFallbackValue(primDef, propName, fieldName, keyPath)) {
// Always done after reading the fallback value.
_done = true;
if (_IsHolding<VtDictionary>(_value)) {
if (_IsHolding<VtDictionary>(_value) && _foundAComposingValue) {
// Merge dictionaries: _value is weaker, tmpDict stronger.
VtDictionaryOverRecursive(&tmpDict,
_UncheckedGet<VtDictionary>(_value));
_UncheckedSwap(_value, tmpDict);
}
_foundAComposingValue = true;
}
}

// Consumes an authored pathExpression value and merges it into the current
// strongest pathExpression value.
bool _ConsumeAndMergeAuthoredPathExpressions(const PcpNodeRef &node,
const SdfLayerRefPtr &layer,
const SdfPath &specPath,
const TfToken &fieldName,
const TfToken &keyPath) {
// Consumes an authored or fallback pathExpression value and merges it into
// the current strongest pathExpression value.
template <class GetValueFn>
bool _ConsumeAndMergePathExpressionsImpl(GetValueFn &&getValue,
PcpNodeRef node) {
SdfPathExpression tmpExpr;
VtArray<SdfPathExpression> tmpExprs;
bool array = false;
Expand All @@ -6280,7 +6289,7 @@ struct ValueComposerBase
}

// Try to read value from scene description.
if (_GetValue(layer, specPath, fieldName, keyPath)) {
if (std::forward<GetValueFn>(getValue)()) {
// If this is a value block, set _done to stop composing, and
// swap back the composed value so far.
if (Usd_ValueContainsBlock(_value)) {
Expand All @@ -6296,7 +6305,7 @@ struct ValueComposerBase
// Merge the resolved expr.
if (array) {
// If the arrays are the same size, merge index-wise.
// Otherwise just take the strongest?
// Otherwise take the strongest.
VtArray<SdfPathExpression> weaker =
_UncheckedGet<VtArray<SdfPathExpression>>(_value);
if (weaker.size() == tmpExprs.size()) {
Expand All @@ -6311,70 +6320,50 @@ struct ValueComposerBase
_UncheckedSwap(_value, tmpExprs);
}
else {
tmpExpr = std::move(tmpExpr)
.ComposeOver(_UncheckedGet<SdfPathExpression>(_value));
_UncheckedSwap(_value, tmpExpr);
if (_foundAComposingValue) {
tmpExpr = std::move(tmpExpr).ComposeOver(
_UncheckedGet<SdfPathExpression>(_value));
_UncheckedSwap(_value, tmpExpr);
}
}
}
_foundAComposingValue = true;
return true;
}
return false;
}

// Consumes an authored pathExpression value and merges it into the current
// strongest pathExpression value.
bool _ConsumeAndMergeAuthoredPathExpressions(const PcpNodeRef &node,
const SdfLayerRefPtr &layer,
const SdfPath &specPath,
const TfToken &fieldName,
const TfToken &keyPath) {

return _ConsumeAndMergePathExpressionsImpl(
[&]() {
return _GetValue(layer, specPath, fieldName, keyPath);
}, node);
}

// Consumes the fallback pathExpression value and merges it into the current
// pathExpression value.
void _ConsumeAndMergeFallbackPathExpressions(
const UsdPrimDefinition &primDef,
const TfToken &propName,
const TfToken &fieldName,
const TfToken &keyPath)
{
SdfPathExpression tmpExpr;
VtArray<SdfPathExpression> tmpExprs;
bool array = false;

// Copy to the side since we'll have to merge if the next opinion is
// also an expression.
if (_IsHolding<SdfPathExpression>(_value)) {
tmpExpr = _UncheckedGet<SdfPathExpression>(_value);
}
else {
array = true;
tmpExprs = _UncheckedGet<VtArray<SdfPathExpression>>(_value);
}
const TfToken &keyPath) {

// Try to read value from scene description.
if (_GetFallbackValue(primDef, propName, fieldName, keyPath)) {
// Always done after reading fallback value.
_done = true;
// No need to resolve a fallback value...
// Merge the resolved expr.
if (array) {
// If the arrays are the same size, merge index-wise.
// Otherwise just take the strongest?
VtArray<SdfPathExpression> weaker =
_UncheckedGet<VtArray<SdfPathExpression>>(_value);
if (weaker.size() == tmpExprs.size()) {
std::transform(
tmpExprs.begin(), tmpExprs.end(), weaker.begin(),
tmpExprs.begin(),
[](SdfPathExpression const &stronger,
SdfPathExpression const &weaker) {
return stronger.ComposeOver(weaker);
});
}
_UncheckedSwap(_value, tmpExprs);
}
else {
tmpExpr = std::move(tmpExpr)
.ComposeOver(_UncheckedGet<SdfPathExpression>(_value));
_UncheckedSwap(_value, tmpExpr);
}
}
_ConsumeAndMergePathExpressionsImpl(
[&]() {
return _GetFallbackValue(primDef, propName, fieldName, keyPath);
}, /*node=*/{});
}

Storage _value;
UsdObject _object;
bool _foundAComposingValue;
bool _done;
bool _forFlattening;
};
Expand Down Expand Up @@ -6416,7 +6405,10 @@ struct UntypedValueComposer : public ValueComposerBase<VtValue *>
// We're done if we got value and it's not a dictionary or path
// expressions. For those types we'll continue to merge in
// weaker opinions.
if (!_IsHoldingDictionary() && !_IsHoldingPathExpressions()) {
if (_IsHoldingDictionary() || _IsHoldingPathExpressions()) {
this->_foundAComposingValue = true;
}
else {
this->_done = true;
}
_ResolveValue(stage, node, layer, specPath);
Expand Down
29 changes: 29 additions & 0 deletions pxr/usd/usd/testenv/testUsdHardToReach.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@
#include "pxr/usd/usd/relationship.h"
#include "pxr/usd/usd/stage.h"

#include "pxr/usd/sdf/pathExpression.h"

PXR_NAMESPACE_USING_DIRECTIVE

namespace
Expand Down Expand Up @@ -231,6 +233,32 @@ TestOpaqueValueFileIO()
_CheckNoSpecForOpaqueValues("usdc");
}

void
TestOutParamterIgnoredForComposingValues()
{
const UsdStageRefPtr stage = UsdStage::CreateInMemory();
const SdfLayerHandle layer = stage->GetRootLayer();

const UsdPrim prim = stage->DefinePrim(SdfPath {"/Prim"});

// Test that a PathExpression out-paramter's value is ignored, and not
// composed over authored opinions.
const UsdAttribute pathExprAttr = prim.CreateAttribute(
TfToken("pathExpr"), SdfValueTypeNames->PathExpression);

SdfPathExpression outPathExpr { "/out %_" };

// Calling Get() with no authored value should leave the out parameter
// untouched.
TF_AXIOM(!pathExprAttr.Get(&outPathExpr));
TF_AXIOM(outPathExpr == SdfPathExpression { "/out %_" });

// Calling Get() should ignore the value in the out parameter.
pathExprAttr.Set(SdfPathExpression {"/authored"});
TF_AXIOM(pathExprAttr.Get(&outPathExpr));
TF_AXIOM(outPathExpr == SdfPathExpression { "/authored" });
}

}

int
Expand All @@ -239,5 +267,6 @@ main(int argc, char** argv)
TestTargetSpecs();
TestGetTargetsAndConnections();
TestOpaqueValueFileIO();
TestOutParamterIgnoredForComposingValues();
return 0;
}

0 comments on commit a9c7195

Please sign in to comment.