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

HamedSabri-adsk · 2020-09-21T16:50:23Z

This PR makes a major improvement in the way "Rename" restriction behaves in Maya.

… cover more edge cases.

test/lib/ufe/testRename.py

HamedSabri-adsk · 2020-09-21T16:56:12Z

lib/usd/utils/util.h

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


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.

HamedSabri-adsk · 2020-09-21T17:08:40Z

lib/mayaUsd/ufe/private/Utils.cpp

+                                 commandName.c_str(),
+                                 prim.GetName().GetString().c_str(), 
+                                 layerDisplayName.c_str());
+        throw std::runtime_error(err.c_str());
    }
 }


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.

HamedSabri-adsk · 2020-09-21T17:18:29Z

lib/mayaUsd/ufe/private/Utils.cpp

+            if(spec->GetSpecifier() == SdfSpecifierOver && spec->HasReferences()) {
+                break;
+            }
+
+            // if the over exist in the session layer
+            if (spec->GetSpecifier() == SdfSpecifierOver) {
+                auto sessionLayer = prim.GetStage()->GetSessionLayer();
+                if (sessionLayer == spec->GetLayer()){
+                    layerDisplayName.append("[" + spec->GetLayer()->GetDisplayName() + "]" + ",");
+                }
+            }
+
+            // 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;
+            }


Handle different cases around "over" specifier.

…he session layer.

…mSpec will be found and instead USD returns an "over".

HamedSabri-adsk · 2020-09-21T17:43:14Z

lib/mayaUsd/ufe/UsdUndoInsertChildCommand.cpp

+    if(!parentPrim.IsPseudoRoot()){
+        ufe::applyCommandRestriction(childPrim, "reparent");
+        ufe::applyCommandRestriction(parentPrim, "reparent");
+    }


I was unconditionally applying the restriction rules here but realized that If parent prim is the pseudo-root, no def primSpec will be found and instead USD returns the "over".

This case happens when one tries to parent a prim under proxy shape ('/').

So does that mean there are no restrictions for children of the pseudo-root? That would be surprising! If I have a child of the pseudo-root with opinions on a stronger layer, I shouldn't be able to rename it, no?

HamedSabri-adsk · 2020-09-21T21:33:41Z

lib/mayaUsd/ufe/private/Utils.cpp

    }
-    else
+
+    if(!layerDisplayName.empty())


Simplified the logic by removing the redundant primSpec check and loop in primStack.

HamedSabri-adsk · 2020-09-21T21:41:20Z

lib/mayaUsd/ufe/private/Utils.cpp

-                if (sessionLayer == spec->GetLayer()){
-                    layerDisplayName.append("[" + spec->GetLayer()->GetDisplayName() + "]" + ",");
-                }
+                layerDisplayName.append("[" + spec->GetLayer()->GetDisplayName() + "]" + ",");


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

- remove unnecessary primSpec->GetLayer() == spec->GetLayer() check - comment clean up

…ready exist.

ppt-adsk · 2020-09-22T13:47:54Z

lib/mayaUsd/ufe/UsdUndoInsertChildCommand.cpp

+    if(!parentPrim.IsPseudoRoot()){
+        ufe::applyCommandRestriction(childPrim, "reparent");
+        ufe::applyCommandRestriction(parentPrim, "reparent");
+    }


So does that mean there are no restrictions for children of the pseudo-root? That would be surprising! If I have a child of the pseudo-root with opinions on a stronger layer, I shouldn't be able to rename it, no?

ppt-adsk · 2020-09-22T13:59:44Z

lib/mayaUsd/ufe/private/Utils.cpp

-                                             prim.GetName().GetString().c_str(), 
-                                             layerDisplayNames.c_str());
-            throw std::runtime_error(err.c_str());
+            // if exists a def sepc


Typo: 'sepc'.

ppt-adsk · 2020-09-22T14:00:54Z

lib/mayaUsd/ufe/private/Utils.cpp

        }
    }
+
+    if(!layerDisplayName.empty()) {
+        layerDisplayName.pop_back();


Why the pop_back()?

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]

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

ppt-adsk · 2020-09-22T14:01:40Z

lib/mayaUsd/ufe/private/Utils.cpp

+        }
+        else {
+            // if exist a primSpec with reference
+            if(spec->HasReferences()) {


Should comment on why we abort the loop here.

The main reason for the skipping the reference is to not clash with the over that may be created in the stage's local sessioLayer or other layers.

Consider the case, where we have a stage that has opened a USD file called RefExample.usda and in that file we have
a prim with over which has an external reference to another Usd file called HelloWorld.usda.

RefExample.usda

over "refSphere2" ( prepend references = @./HelloWorld.usda@ )

HelloWorld.usda

#usda 1.0 ( defaultPrim = "hello" ) def Xform "hello" { ........... }

All PrimStack knows at this point is that "refSphere2" has two specs in 2 different places in order from strong to weak:

RefExample.usda ---> over HelloWorld.usda ---> def

Now in line 161, I unconditionally check if there is an override and then record it's layer name which then results in error message to be displayed which is not desirable:

// if exists an over sepc if (spec->GetSpecifier() == SdfSpecifierOver) { layerDisplayName.append("[" + layerName + "]" + ","); }

one should be able to rename "refSphere2" since it is created in "RefExample.usda" and itself doesn't have any other "over" in the stage's local layers.

This is why I previously had a condition to check if the "over" was created in sessioLayer or not.

Hope this makes sense.

It does. Can you distill this into a code comment somehow? I know it's a bit challenging, you don't want to have a 2-page code comment, but it would still be nice.

Ugh yes of course! I was planning to do it after the planning meeting.

test/lib/ufe/testRename.py

HamedSabri-adsk · 2020-09-22T20:32:36Z

lib/mayaUsd/ufe/private/Utils.cpp

                                 commandName.c_str(),
-                                 prim.GetName().GetString().c_str(), 
+                                 prim.GetName().GetString().c_str(),
+                                 message.c_str(),
                                 layerDisplayName.c_str());
        throw std::runtime_error(err.c_str());
    }


@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 ) :

Cannot rename something because It is defined on another layer

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..............

HamedSabri-adsk · 2020-09-23T15:42:33Z

lib/mayaUsd/ufe/UsdUndoInsertChildCommand.cpp

-    _parentLayer = parentPrim.IsPseudoRoot() 
-        ? parent->prim().GetStage()->GetEditTarget().GetLayer() 
-        : MayaUsdUtils::defPrimSpecLayer(parentPrim);
+    _childLayer = childPrim.GetStage()->GetEditTarget().GetLayer();


@ppt-adsk clean up the legacy old code for getting the appropriate layer for _parentLayer .

ufe::applyCommandRestriction in line 71 and 72 will always put us in a position to use appropriate layer to use for performing the parent operation.

HamedSabri-adsk · 2020-09-23T15:45:29Z

lib/mayaUsd/ufe/private/Utils.cpp

+    if (prim.IsPseudoRoot()){
+        return;
+    }
+


@ppt-adsk per out offline discussion, PseudoRoot() is a special case and should be treated differently.

After some thinking, it makes more sense to simply return early if we are dealing with the root layer.

HamedSabri-adsk · 2020-09-23T21:50:06Z

lib/mayaUsd/ufe/private/Utils.cpp

@@ -25,6 +25,8 @@

 #include <mayaUsdUtils/util.h>

+#include <iostream>


just noticed I left out to clean up this iostream. Will have it remove in another PR.

MAYA-105175: Rewrote the rename restriction algorithm from scratch to…

6c2a798

… cover more edge cases.

HamedSabri-adsk added ufe-usd Related to UFE-USD plugin in Maya-Usd workflows Related to in-context workflows labels Sep 21, 2020

HamedSabri-adsk requested a review from ppt-adsk September 21, 2020 16:50

HamedSabri-adsk commented Sep 21, 2020

View reviewed changes

test/lib/ufe/testRename.py Show resolved Hide resolved

HamedSabri-adsk commented Sep 21, 2020

View reviewed changes

Hamed Sabri added 2 commits September 21, 2020 13:25

Remove redundant check to distinguish if the "over" specifier is in t…

e50457c

…he session layer.

Fix testParentCmd test. If parent prim is the pseudo-root, no def pri…

a9226cd

…mSpec will be found and instead USD returns an "over".

HamedSabri-adsk assigned ecp-maya-devops-adsk Sep 21, 2020

HamedSabri-adsk commented Sep 21, 2020

View reviewed changes

MAYA-105175: Simplify the logic to avoid code redundancy.

52d7585

HamedSabri-adsk commented Sep 21, 2020

View reviewed changes

HamedSabri-adsk assigned ecp-maya-devops-adsk and unassigned ecp-maya-devops-adsk Sep 21, 2020

HamedSabri-adsk commented Sep 21, 2020

View reviewed changes

Hamed Sabri added 4 commits September 21, 2020 18:54

Minor clean up:

92f1c04

- remove unnecessary primSpec->GetLayer() == spec->GetLayer() check - comment clean up

MAYA-105175: minor fix in the logic: Break out of loop if the spec al…

8038469

…ready exist.

Covering more edge cases.

5adec49

Simplify the algorithm further.

eac389a

ppt-adsk reviewed Sep 22, 2020

View reviewed changes

MAYA-105175: published the algorithm and added proper messaging.

2472eec

HamedSabri-adsk commented Sep 22, 2020

View reviewed changes

HamedSabri-adsk assigned ecp-maya-devops-adsk and unassigned ecp-maya-devops-adsk Sep 22, 2020

Hamed Sabri added 2 commits September 22, 2020 21:14

MAYA-105175: clean up some comments.

b10a2b4

MAYA-105175: clean up the logic around pseudo-root.

9985175

HamedSabri-adsk commented Sep 23, 2020

View reviewed changes

HamedSabri-adsk assigned ecp-maya-devops-adsk and unassigned ecp-maya-devops-adsk Sep 23, 2020

ppt-adsk approved these changes Sep 23, 2020

View reviewed changes

HamedSabri-adsk added the ready-for-merge Development process is finished, PR is ready for merge label Sep 23, 2020

HamedSabri-adsk commented Sep 23, 2020

View reviewed changes

kxl-adsk merged commit 91d0bfe into dev Sep 24, 2020

kxl-adsk deleted the sabri/MAYA-105175/rewrite_rename_restriction branch September 24, 2020 13:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

HamedSabri-adsk commented Sep 21, 2020

HamedSabri-adsk Sep 21, 2020

HamedSabri-adsk Sep 21, 2020 •

edited

Loading

HamedSabri-adsk Sep 21, 2020

HamedSabri-adsk Sep 21, 2020

ppt-adsk Sep 22, 2020

HamedSabri-adsk Sep 21, 2020 •

edited

Loading

HamedSabri-adsk Sep 21, 2020

ppt-adsk Sep 22, 2020

ppt-adsk Sep 22, 2020

ppt-adsk Sep 22, 2020

HamedSabri-adsk Sep 22, 2020 •

edited

Loading

ppt-adsk Sep 22, 2020

ppt-adsk Sep 22, 2020

HamedSabri-adsk Sep 22, 2020

ppt-adsk Sep 22, 2020

HamedSabri-adsk Sep 22, 2020

HamedSabri-adsk Sep 22, 2020 •

edited

Loading

HamedSabri-adsk Sep 23, 2020

HamedSabri-adsk Sep 23, 2020

HamedSabri-adsk Sep 23, 2020

		@@ -25,6 +25,8 @@

		#include <mayaUsdUtils/util.h>

		#include <iostream>

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

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

Conversation

HamedSabri-adsk commented Sep 21, 2020

Choose a reason for hiding this comment

HamedSabri-adsk Sep 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HamedSabri-adsk Sep 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HamedSabri-adsk Sep 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HamedSabri-adsk Sep 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HamedSabri-adsk Sep 21, 2020 •

edited

Loading

HamedSabri-adsk Sep 21, 2020 •

edited

Loading

HamedSabri-adsk Sep 22, 2020 •

edited

Loading

HamedSabri-adsk Sep 22, 2020 •

edited

Loading