Skip to content

Commit

Permalink
Fix errors that would occur when the callback passed to
Browse files Browse the repository at this point in the history
UsdUtilsModifyAssetPaths returned an empty path.

UsdUtilsModifyAssetPaths now removes the original asset
path if the modification callback returns an empty asset
path.

Fixes #1282

(Internal change: 2254050)
  • Loading branch information
sunyab authored and pixar-oss committed Nov 4, 2022
1 parent 41ac55f commit 2598536
Show file tree
Hide file tree
Showing 5 changed files with 121 additions and 65 deletions.
2 changes: 1 addition & 1 deletion pxr/usd/usdUtils/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -441,7 +441,7 @@ pxr_register_test(testUsdUtilsIntrospection
pxr_register_test(testUsdUtilsModifyAssetPaths
PYTHON
COMMAND "${CMAKE_INSTALL_PREFIX}/tests/testUsdUtilsModifyAssetPaths layer.usda modified.usda"
DIFF_COMPARE modified.usda duplicates.usda
DIFF_COMPARE modified.usda duplicates.usda removal.usda
EXPECTED_RETURN_CODE 0
)

Expand Down
146 changes: 83 additions & 63 deletions pxr/usd/usdUtils/dependencies.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -241,32 +241,37 @@ _FileAnalyzer::_UpdateAssetValue(const VtValue &val)
{
if (val.IsHolding<SdfAssetPath>()) {
auto assetPath = val.UncheckedGet<SdfAssetPath>();
std::string rawAssetPath = assetPath.GetAssetPath();
const std::string& rawAssetPath = assetPath.GetAssetPath();
if (!rawAssetPath.empty()) {
return VtValue(SdfAssetPath(
_ProcessDependency(rawAssetPath, _DepType::Reference)));
const std::string remappedPath =
_ProcessDependency(rawAssetPath, _DepType::Reference);
return remappedPath.empty() ?
VtValue() : VtValue(SdfAssetPath(remappedPath));
}
} else if (val.IsHolding<VtArray<SdfAssetPath>>()) {
VtArray<SdfAssetPath> updatedVal;
VtArray<SdfAssetPath> updatedArray;
for (const SdfAssetPath& assetPath :
val.UncheckedGet< VtArray<SdfAssetPath> >()) {
std::string rawAssetPath = assetPath.GetAssetPath();
const std::string& rawAssetPath = assetPath.GetAssetPath();
if (!rawAssetPath.empty()) {
updatedVal.push_back(SdfAssetPath(
_ProcessDependency(rawAssetPath, _DepType::Reference)));
} else {
// Retain empty paths in the array.
updatedVal.push_back(assetPath);
const std::string remappedPath =
_ProcessDependency(rawAssetPath, _DepType::Reference);
if (!remappedPath.empty()) {
updatedArray.push_back(SdfAssetPath(remappedPath));
}
}
}
return VtValue(updatedVal);
return updatedArray.empty() ? VtValue() : VtValue::Take(updatedArray);

This comment has been minimized.

Copy link
@marktucker

marktucker Apr 25, 2023

Contributor

This line results in a small change in behavior... If I have an attribute
custom asset[] AssetArray = []
and I run it through this function to modify paths, the old code would leave the attribute unchanged. But the new code results in
custom asset[] AssetArray
I think these two are equivalent, so it doesn't really matter, but wanted to verify this...

Also it seems like an unnecessary change in behavior. Is there some reason why this was done intentionally?

This comment has been minimized.

Copy link
@sunyab

sunyab Apr 26, 2023

Author Contributor

Hey @marktucker , thanks for calling this out! This is an unintentional change in behavior and the above difference could be significant. Your first example would override any opinions in weaker layers so that the value of AssetArray would be an empty array. Your second example would allow opinions in weaker layers to come through.

I'll file something internally to get a fix for this into place. Thanks!

}
else if (val.IsHolding<VtDictionary>()) {
VtDictionary updatedVal;
VtDictionary updatedDict;
for (const auto& p : val.UncheckedGet<VtDictionary>()) {
updatedVal[p.first] = _UpdateAssetValue(p.second);
VtValue updatedVal = _UpdateAssetValue(p.second);
if (!updatedVal.IsEmpty()) {
updatedDict[p.first] = std::move(updatedVal);
}
}
return VtValue(updatedVal);
return updatedDict.empty() ? VtValue() : VtValue::Take(updatedDict);
}

return val;
Expand All @@ -277,8 +282,12 @@ _FileAnalyzer::_ProcessSublayers()
{
if (_remapPathFunc) {
_layer->GetSubLayerPaths().ModifyItemEdits(
[this](const std::string& path) {
return _ProcessDependency(path, _DepType::Sublayer);
[this](const std::string& path) {
std::string remappedPath =
_ProcessDependency(path, _DepType::Sublayer);
return remappedPath.empty() ?
boost::optional<std::string>() :
boost::optional<std::string>(std::move(remappedPath));
});
} else {
for (const auto &subLayer: _layer->GetSubLayerPaths()) {
Expand All @@ -300,6 +309,12 @@ _FileAnalyzer::_RemapRefOrPayload(const RefOrPayloadType &refOrPayload)
std::string remappedPath =
_ProcessDependency(refOrPayload.GetAssetPath(), DEP_TYPE);

// If the remapped path was empty, return none to indicate this reference
// or payload should be removed.
if (remappedPath.empty()) {
return boost::none;
}

// If the path was not remapped to a different path, then return the
// incoming payload unmodified.
if (remappedPath == refOrPayload.GetAssetPath()) {
Expand Down Expand Up @@ -353,58 +368,58 @@ _FileAnalyzer::_ProcessProperties(const SdfPrimSpecHandle &primSpec)
const VtValue propertyNames =
primSpec->GetField(SdfChildrenKeys->PropertyChildren);

if (propertyNames.IsHolding<vector<TfToken>>()) {
for (const auto& name :
propertyNames.UncheckedGet<vector<TfToken>>()) {
// For every property
// Build an SdfPath to the property
const SdfPath path = primSpec->GetPath().AppendProperty(name);

// Check property metadata
for (const TfToken& infoKey : _layer->ListFields(path)) {
if (infoKey != SdfFieldKeys->Default &&
infoKey != SdfFieldKeys->TimeSamples) {
if (!propertyNames.IsHolding<vector<TfToken>>()) {
return;
}

for (const auto& name : propertyNames.UncheckedGet<vector<TfToken>>()) {
// For every property
// Build an SdfPath to the property
const SdfPath path = primSpec->GetPath().AppendProperty(name);

// Check property metadata
for (const TfToken& infoKey : _layer->ListFields(path)) {
if (infoKey != SdfFieldKeys->Default &&
infoKey != SdfFieldKeys->TimeSamples) {

VtValue value = _layer->GetField(path, infoKey);
VtValue updatedValue = _UpdateAssetValue(value);
if (_remapPathFunc && value != updatedValue) {
_layer->SetField(path, infoKey, updatedValue);
}
VtValue value = _layer->GetField(path, infoKey);
VtValue updatedValue = _UpdateAssetValue(value);
if (_remapPathFunc && value != updatedValue) {
_layer->SetField(path, infoKey, updatedValue);
}
}
}

// Check property existence
const VtValue vtTypeName =
_layer->GetField(path, SdfFieldKeys->TypeName);
if (!vtTypeName.IsHolding<TfToken>())
continue;
// Check property existence
const VtValue vtTypeName =
_layer->GetField(path, SdfFieldKeys->TypeName);
if (!vtTypeName.IsHolding<TfToken>()) {
continue;
}

const TfToken typeName =
vtTypeName.UncheckedGet<TfToken>();
if (typeName == SdfValueTypeNames->Asset ||
typeName == SdfValueTypeNames->AssetArray) {

// Check default value
VtValue defValue = _layer->GetField(path,
SdfFieldKeys->Default);
VtValue updatedDefValue = _UpdateAssetValue(defValue);
if (_remapPathFunc && defValue != updatedDefValue) {
_layer->SetField(path, SdfFieldKeys->Default,
updatedDefValue);
}
const TfToken typeName = vtTypeName.UncheckedGet<TfToken>();
if (typeName == SdfValueTypeNames->Asset ||
typeName == SdfValueTypeNames->AssetArray) {

// Check default value
VtValue defValue = _layer->GetField(path, SdfFieldKeys->Default);
VtValue updatedDefValue = _UpdateAssetValue(defValue);
if (_remapPathFunc && defValue != updatedDefValue) {
_layer->SetField(path, SdfFieldKeys->Default, updatedDefValue);
}

// Check timeSample values
for (double t : _layer->ListTimeSamplesForPath(path)) {
VtValue timeSampleVal;
if (_layer->QueryTimeSample(path, t, &timeSampleVal)) {
VtValue updatedVal = _UpdateAssetValue(timeSampleVal);
if (_remapPathFunc && timeSampleVal != updatedVal) {

// Check timeSample values
for (double t : _layer->ListTimeSamplesForPath(path)) {
VtValue timeSampleVal;
if (_layer->QueryTimeSample(path,
t, &timeSampleVal)) {

VtValue updatedTimeSampleVal =
_UpdateAssetValue(timeSampleVal);
if (_remapPathFunc &&
timeSampleVal != updatedTimeSampleVal) {
_layer->SetTimeSample(path, t,
updatedTimeSampleVal);
if (updatedVal.IsEmpty()) {
_layer->EraseTimeSample(path, t);
}
else {
_layer->SetTimeSample(path, t, updatedVal);
}
}
}
Expand All @@ -421,7 +436,12 @@ _FileAnalyzer::_ProcessMetadata(const SdfPrimSpecHandle &primSpec)
VtValue value = primSpec->GetInfo(infoKey);
VtValue updatedValue = _UpdateAssetValue(value);
if (_remapPathFunc && value != updatedValue) {
primSpec->SetInfo(infoKey, updatedValue);
if (updatedValue.IsEmpty()) {
primSpec->ClearInfo(infoKey);
}
else {
primSpec->SetInfo(infoKey, updatedValue);
}
}
}
}
Expand Down
3 changes: 2 additions & 1 deletion pxr/usd/usdUtils/dependencies.h
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,8 @@ UsdUtilsComputeAllDependencies(const SdfAssetPath &assetPath,

/// Callback that is used to modify asset paths in a layer. The \c assetPath
/// will contain the string value that's authored. The returned value is the
/// new value that should be authored in the layer.
/// new value that should be authored in the layer. If the function returns
/// an empty string, that value will be removed from the layer.
using UsdUtilsModifyAssetPathFn = std::function<std::string(
const std::string& assetPath)>;

Expand Down
10 changes: 10 additions & 0 deletions pxr/usd/usdUtils/testenv/testUsdUtilsModifyAssetPaths.py
Original file line number Diff line number Diff line change
Expand Up @@ -47,3 +47,13 @@ def fn(s):
Test('layer.usda', 'duplicates.usda', fn)

TestDuplicates()

def TestRemoval():
# Tests behavior when modify callback returns empty asset
# paths.
def fn(s):
return ''

Test('layer.usda', 'removal.usda', fn)

TestRemoval()
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
#usda 1.0

def Xform "Model" (
payload = None
references = None
)
{
asset[] bar
asset foo
asset[] varBar
asset varFoo
}

def Xform "Variants" (
add variantSets = "standin"
)
{
variantSet "standin" = {
"render" {
asset foo

}
}
}

0 comments on commit 2598536

Please sign in to comment.