Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MAYA-105175: Rewrote the rename restriction algorithm from scratch to cover more edge cases. #786

Merged
merged 11 commits into from
Sep 24, 2020
Merged

MAYA-105175: Rewrote the rename restriction algorithm from scratch to cover more edge cases. #786

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
6c2a798
MAYA-105175: Rewrote the rename restriction algorithm from scratch to…
Sep 21, 2020
e50457c
Remove redundant check to distinguish if the "over" specifier is in t…
Sep 21, 2020
a9226cd
Fix testParentCmd test. If parent prim is the pseudo-root, no def pri…
Sep 21, 2020
52d7585
MAYA-105175: Simplify the logic to avoid code redundancy.
Sep 21, 2020
92f1c04
Minor clean up:
Sep 21, 2020
8038469
MAYA-105175: minor fix in the logic: Break out of loop if the spec al…
Sep 22, 2020
5adec49
Covering more edge cases.
Sep 22, 2020
eac389a
Simplify the algorithm further.
Sep 22, 2020
2472eec
MAYA-105175: published the algorithm and added proper messaging.
Sep 22, 2020
b10a2b4
MAYA-105175: clean up some comments.
Sep 23, 2020
9985175
MAYA-105175: clean up the logic around pseudo-root.
Sep 23, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
85 changes: 45 additions & 40 deletions lib/mayaUsd/ufe/private/Utils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,53 +129,58 @@ UsdGeomXformCommonAPI convertToCompatibleCommonAPI(const UsdPrim& prim)

void applyCommandRestriction(const UsdPrim& prim, const std::string& commandName)
{
// early check to see if a particular node has any specs to contribute
// to the final composed prim. e.g (a node in payload)
if(!MayaUsdUtils::hasSpecs(prim)){
auto primSpec = MayaUsdUtils::getPrimSpecAtEditTarget(prim);
auto primStack = prim.GetPrimStack();
std::string layerDisplayName;

auto layers = MayaUsdUtils::layerInCompositionArcsWithSpec(prim);
std::string layerDisplayNames;
for (auto layer : layers) {
layerDisplayNames.append("[" + layer->GetDisplayName() + "]" + ",");
// check to see if there is a spec at the edit target layer.
if (primSpec)
{
for (const auto& spec : primStack)
{
// skip if the spec already exist
if (primSpec == spec && spec->GetSpecifier() == SdfSpecifierDef && primSpec->GetLayer() == spec->GetLayer()) {
continue;
}

// break out if we are dealing with an over that has a reference.
if(spec->GetSpecifier() == SdfSpecifierOver && spec->HasReferences()) {
break;
}

// if the over exist in the session layer
if (spec->GetSpecifier() == SdfSpecifierOver) {
layerDisplayName.append("[" + spec->GetLayer()->GetDisplayName() + "]" + ",");
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove redundant check to distinguish if the "over" specifier is in the session layer.

}

// if the def primspec is in another layer other than current stage's local layer.
if (spec->GetSpecifier() == SdfSpecifierDef && primSpec->GetLayer() != spec->GetLayer()) {
layerDisplayName.append("[" + spec->GetLayer()->GetDisplayName() + "]" + ",");
break;
}
}
layerDisplayNames.pop_back();
std::string err = TfStringPrintf("Cannot %s [%s]. It does not make any contributions in the current layer "
"because its specs are in an external composition arc. Please open %s to make direct edits.",
commandName.c_str(),
prim.GetName().GetString().c_str(),
layerDisplayNames.c_str());
throw std::runtime_error(err.c_str());
}

// if the current layer doesn't have any contributions
if (!MayaUsdUtils::doesEditTargetLayerContribute(prim)) {
auto strongestContributingLayer = MayaUsdUtils::strongestContributingLayer(prim);
std::string err = TfStringPrintf("Cannot %s [%s]. It is defined on another layer. Please set [%s] as the target layer to proceed.",
commandName.c_str(),
prim.GetName().GetString().c_str(),
strongestContributingLayer->GetDisplayName().c_str());
throw std::runtime_error(err.c_str());
if(!layerDisplayName.empty())
{
layerDisplayName.pop_back();
std::string err = TfStringPrintf("Cannot %s [%s]. It is defined on another layer. Please set %s as the target layer to proceed.",
commandName.c_str(),
prim.GetName().GetString().c_str(),
layerDisplayName.c_str());
throw std::runtime_error(err.c_str());
}
}
else
{
auto layers = MayaUsdUtils::layersWithContribution(prim);
// if we have more than 2 layers that contributes to the final composed prim
if (layers.size() > 1) {
std::string layerDisplayNames;

// skip the the first arc which is PcpArcTypeRoot
// we are interested in all the arcs after root
std::for_each(std::next(layers.begin()),layers.end(), [&](const auto& it) {
layerDisplayNames.append("[" + it->GetDisplayName() + "]" + ",");
});

layerDisplayNames.pop_back();
std::string err = TfStringPrintf("Cannot %s [%s]. It has definitions or opinions on other layers. Opinions exist in %s",
commandName.c_str(),
prim.GetName().GetString().c_str(),
layerDisplayNames.c_str());
throw std::runtime_error(err.c_str());
for (const auto& spec : primStack) {
layerDisplayName.append("[" + spec->GetLayer()->GetDisplayName() + "]" + ",");
}
layerDisplayName.pop_back();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the pop_back()?

Copy link
Contributor Author

@HamedSabri-adsk HamedSabri-adsk Sep 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the pop_back is just for make up purposes in the error message. It just removes the last "," in the case where there are multiple layers:

[bob],[foo],

I pop back the last "," so it looks

[bob],[foo]

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, you've explained this to me once before... A shortened version of your explanation would make a great code comment.

std::string err = TfStringPrintf("Cannot %s [%s]. It is defined on another layer. Please set %s as the target layer to proceed.",
commandName.c_str(),
prim.GetName().GetString().c_str(),
layerDisplayName.c_str());
throw std::runtime_error(err.c_str());
}
Copy link
Contributor Author

@HamedSabri-adsk HamedSabri-adsk Sep 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ppt-adsk

  • polished the algorithm slightly to make it easier to reason about the code and added appropriate comments.
  • added proper messaging. we now only need to deal with two messages ( no more fancy words ) :
  1. Cannot rename something because It is defined on another layer
  2. Cannot rename something because It has a stronger opinion on another layer

please see 2472eec .

Also ignore my typo in the commit message: I meant "polished" not "published". Sigh..............

}
Copy link
Contributor Author

@HamedSabri-adsk HamedSabri-adsk Sep 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This algorithm heavily relies on using "Prim's PrimStack" rather than "Stage's LayerStack" which makes it much easier to interrogate about the SdfPrimSpec(s) in different composition arcs.


Expand Down
81 changes: 0 additions & 81 deletions lib/usd/utils/util.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,53 +93,6 @@ defPrimSpecLayer(const UsdPrim& prim)
return defLayer;
}

std::vector<SdfLayerHandle>
layersWithContribution(const UsdPrim& prim)
{
UsdPrimCompositionQuery query(prim);

std::vector<SdfLayerHandle> layersWithContribution;

for (const auto& arc : query.GetCompositionArcs()) {
layersWithContribution.emplace_back(arc.GetTargetNode().GetLayerStack()->GetIdentifier().rootLayer);
}

return layersWithContribution;
}

bool
doesEditTargetLayerContribute(const UsdPrim& prim)
{
auto editTarget = prim.GetStage()->GetEditTarget();
auto layer = editTarget.GetLayer();
auto primSpec = layer->GetPrimAtPath(prim.GetPath());

// to know whether the target layer can contribute to the
// final composed prim, there must be a primSpec for that prim
if (!primSpec) {
return false;
}

return true;
}

SdfLayerHandle
strongestContributingLayer(const UsdPrim& prim)
{
SdfLayerHandle targetLayer;
auto layerStack = prim.GetStage()->GetLayerStack();
for (auto layer : layerStack)
{
// to know whether the target layer can contribute to the
// final composed prim, there must be a primSpec for that prim
auto primSpec = layer->GetPrimAtPath(prim.GetPath());
if (primSpec) {
targetLayer = layer;
break;
}
}
return targetLayer;
}

SdfPrimSpecHandle
getPrimSpecAtEditTarget(const UsdPrim& prim)
Expand All @@ -165,40 +118,6 @@ isInternalReference(const SdfPrimSpecHandle& primSpec)
return isInternalRef;
}

bool
hasSpecs(const UsdPrim& prim)
{
bool found{true};

UsdPrimCompositionQuery query(prim);

for (const auto& compQueryArc : query.GetCompositionArcs()) {
if (!compQueryArc.GetTargetNode().HasSpecs()) {
found = false;
break;
}
}

return found;
}

std::vector<SdfLayerHandle>
layerInCompositionArcsWithSpec(const UsdPrim& prim)
{
UsdPrimCompositionQuery query(prim);

std::vector<SdfLayerHandle> layersWithContribution;

for (const auto& compQueryArc : query.GetCompositionArcs()) {
if (compQueryArc.GetTargetNode().HasSpecs()) {
layersWithContribution.emplace_back(
compQueryArc.GetTargetNode().GetLayerStack()->GetIdentifier().rootLayer);
}
}

return layersWithContribution;
}

void
printCompositionQuery(const UsdPrim& prim, std::ostream& os)
{
Expand Down
20 changes: 0 additions & 20 deletions lib/usd/utils/util.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,18 +29,6 @@ namespace MayaUsdUtils {
MAYA_USD_UTILS_PUBLIC
SdfLayerHandle defPrimSpecLayer(const UsdPrim&);

//! Return a list of layers in no strength order that can contribute to the argument prim.
MAYA_USD_UTILS_PUBLIC
std::vector<SdfLayerHandle> layersWithContribution(const UsdPrim&);

//! Check if a layer has any contributions towards the argument prim.
MAYA_USD_UTILS_PUBLIC
bool doesEditTargetLayerContribute(const UsdPrim&);

//! Return the strongest layer that can contribute to the argument prim.
MAYA_USD_UTILS_PUBLIC
SdfLayerHandle strongestContributingLayer(const UsdPrim&);

//! Return a PrimSpec for the argument prim in the layer containing the stage's current edit target.
MAYA_USD_UTILS_PUBLIC
SdfPrimSpecHandle getPrimSpecAtEditTarget(const UsdPrim&);
Expand All @@ -49,14 +37,6 @@ namespace MayaUsdUtils {
MAYA_USD_UTILS_PUBLIC
bool isInternalReference(const SdfPrimSpecHandle&);

//! Returns true if the target node has any specs to contribute to the composed prim.
MAYA_USD_UTILS_PUBLIC
bool hasSpecs(const UsdPrim&);

//! Returns the layer in composition arc where HasSpecs is set to true
MAYA_USD_UTILS_PUBLIC
std::vector<SdfLayerHandle> layerInCompositionArcsWithSpec(const UsdPrim& prim);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cleaned up the usd utility functions. We no longer use them.

UsdPrimCompositionQuery is a nice utility but doesn't deal with SubLayer comp arcs. For the sake of this task, I stopped using it altogether.

//! Convenience function for printing the list of queried composition arcs in order.
MAYA_USD_UTILS_PUBLIC
void printCompositionQuery(const UsdPrim&, std::ostream&);
Expand Down
23 changes: 0 additions & 23 deletions test/lib/ufe/testRename.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,29 +270,6 @@ def testRenameRestrictionSameLayerDef(self):
newName = 'Ball_35_Renamed'
cmds.rename(newName)

def testRenameRestrictionOtherLayerOpinions(self):
'''Restrict renaming USD node. Cannot rename a prim with definitions or opinions on other layers.'''

# select a USD object.
mayaPathSegment = mayaUtils.createUfePathSegment('|world|transform1|proxyShape1')
usdPathSegment = usdUtils.createUfePathSegment('/Room_set/Props/Ball_35')
ball35Path = ufe.Path([mayaPathSegment, usdPathSegment])
ball35Item = ufe.Hierarchy.createItem(ball35Path)

ufe.GlobalSelection.get().append(ball35Item)

# get the USD stage
stage = mayaUsd.ufe.getStage(str(mayaPathSegment))

# set the edit target to Assembly_room_set.usda
stage.SetEditTarget(stage.GetLayerStack()[2])
self.assertEqual(stage.GetEditTarget().GetLayer().GetDisplayName(), "Assembly_room_set.usda")

# expect the exception happens
with self.assertRaises(RuntimeError):
newName = 'Ball_35_Renamed'
cmds.rename(newName)
ppt-adsk marked this conversation as resolved.
Show resolved Hide resolved

def testRenameRestrictionHasSpecs(self):
'''Restrict renaming USD node. Cannot rename a node that doesn't contribute to the final composed prim'''

Expand Down