Allowing a new prim spec to be written in edit target layers #2390

NickWu · 2022-05-30T06:17:21Z

This PR is for addressing the issue discussed in here #2212. In short, at Animal Logic, we found isAttributeEditAllowed function in UFE a bit confusing and was blocking our pipeline so we updated its logic in our internal code base. This PR is to promote our internal solution.

pierrebai-adsk · 2022-05-30T16:19:59Z

I will have to understand well the change. We had internal discussions about loosening the restrictions, but we don't have a design yet. Getting this wrong could break reparent / renaming for example, which could leave prim stranded if they are build from opinions from different layers and a single one gets renamed.

pierrebai-adsk · 2022-05-31T12:31:24Z

lib/mayaUsd/ufe/Utils.cpp

@@ -96,6 +96,7 @@ uint32_t findLayerIndex(const UsdPrim& prim, const SdfLayerHandle& layer)
        // to find the layer
        for (SdfLayerRefPtr const& l : layerStack->GetLayers()) {
            if (l == layer) {


I would prefer an explicit set to false at line 105 when not found, for clarity.

This would also avoid the need for the explicit reset of the variable to false in the caller, when it tries to call this a 2nd time, below.

I think that's a good idea. Thanks!

pierrebai-adsk · 2022-05-31T12:38:14Z

lib/mayaUsd/ufe/Utils.cpp

@@ -451,8 +452,15 @@ bool isAttributeEditAllowed(const PXR_NS::UsdAttribute& attr, std::string* errMs
        return false;
    }

+    bool foundLayer = false;


If I'm going to nit-pick: I'd prefer this variable declaration below the comment, right next to the target index layer, since they go together.

Makes sense. Will do.

pierrebai-adsk · 2022-05-31T12:44:13Z

lib/mayaUsd/ufe/Utils.cpp

-                    attr.GetBaseName().GetText(),
-                    strongestLayer->GetDisplayName().c_str());
+                    attr.GetName().GetText(),
+                    strongestLayer->GetIdentifier().c_str());


Maybe display both the display name and identifier? (IIRC, the identifier is the filename?)

identifier can be an a full URI also. I updated this message because we found displayName, which is the base filename of the identifier, was not helpful enough to troubleshoot issues for us.

pierrebai-adsk · 2022-05-31T12:48:07Z

lib/mayaUsd/ufe/Utils.cpp

+
+    // If the edit target layer not found in the prim's layer stack, we return true to allow
+    // new future edits go to the edit target layer.
+    if (!foundLayer)


This would allow authoring below the stronger opinion. Is the goal to allow new opinion on layers without opinion only above the strongest one or anywhere? Currently for us anywhere would go against the current design, so I'll have to talk to the designer and PO/PM about it.

If your use case is that you want to edit in a layer that currently has no opinion but is above the layer, then you could add a check when the layer is not found: search the top opinion layer, iterating through its ancestors and if you find the target layer then allow the edit.

The code already retrieve the strongestLayer a bit below, so move that variable up and use if when foundLayer is false and check that the target is above.

If your goal is to author a weaker opinion, then we may have to make that desired behaviour optional somehow through a stage option or environment variable or some similar mechanism.

I just look ed at your test and it does seem to already only allow stronger opinions, so the code as it is now would be fine?

I thought that primIndex.GetNodeRange() would only return layers where there are authored opinions, but it returns all intermediary layers too? I guess I'd need to trace in the code with your test layers to confirm.

I just recreated your layer stack and tested it... and the edition of the foo attribute on prim B is already working without your changes. So it is the case that primIndex.GetNodeRange() return all layers, even those without opinions.

So, I think your test is not testing what you hoped it would? You can confirm this by commenting out the early return here and run your test and see if it indeed passes without your changes.

It also means your changes might not be doing what you think it would?

To start, I would need the answer to the first question: is the goal to only allow new opinions on stronger layers or on any layer, even weaker ones?

For an example of checking stronger layer, here is a bit of code we added recently to allow creating child prim on a stronger layer. The function returns which layer is stronger and that fact is used when allowing the parent command:

maya-usd/lib/mayaUsd/ufe/private/Utils.cpp

Line 124 in bd2e08f

SdfLayerHandle getStrongerLayer(

I found another alternative solution other than using foundLayer boolean, which is to replace prim.GetPrimIndex() with prim.ComputeExpandedPrimIndex() at the following line:

maya-usd/lib/mayaUsd/ufe/Utils.cpp

Line 85 in 3fb772d

const PcpPrimIndex& primIndex = prim.GetPrimIndex();

From the doc of GetPrimIndex https://graphics.pixar.com/usd/release/api/class_usd_prim.html#ae6ae4259af3ac06b8a9898a787202a32.
It says:

The prim index returned by this function is optimized and may not include sites that do not contribute opinions to this prim. Use UsdPrim::ComputeExpandedPrimIndex to compute a prim index that includes all possible sites that could contribute opinions.

So by using ComputeExpandedPrimIndex function instead of GetPrimIndex, we'll be able to return an index number from findLayerIndex function even if the edit target layer doesn't have opinions on the target prim yet (but might contribute opinions in future).

Which one do you prefer - using ComputeExpandedPrimIndex or foundLayer boolean?

*EDIT: Also, from the doc of ComputeExpandedPrimIndex https://graphics.pixar.com/usd/release/api/class_usd_prim.html#ad642cd45c3326bb0a75c209fcb0ec9eb. It does say it could be potentially slower then GetPrimIndex though.

Your new suggestion seems like a clean answer. I would have a slight preference for it, but either ways would be fine.

At the risk of repeating myself, my main concern is having a unit unit test that replicates the problems you are seeing that would fail without your changes and pass with your changes applied.

Ok, I'll update the code to use ComputeExpandedPrimIndex instead of foundLayer boolean as it seems to be cleaner and less code changes to me. I'll also update the unit test when I get a chance. Thanks!

Hey @pierrebai-adsk , sorry for the late reply. I finally got some time to update this PR. I have updated the unit tests to mimic the issue we were having in our production and use ComputeExpandedPrimIndex function instead of foundLayer boolean for the solution. The test should pass with my change and fail without my change. Please have a look, thanks!

Hi, I was on vacations until today. I will review your changes today or tomorrow when I'm fully back up to speed.

pierrebai-adsk · 2022-05-31T13:41:12Z

test/lib/ufe/testAttributeBlock.py

@@ -347,6 +349,83 @@ def testAttributeBlocking3dMatrixOps(self):

        # authoring new transformation edit is allowed.
        self.assertTrue(mayaUsdUfe.isAttributeEditAllowed(transformAttr))
+
+    def testIsAttributeEditAllowedFunction(self):
+        rootLayerStr = """\


Please add a function doc explaining the goal of the test.

Will do, thanks.

NickWu

Thanks @pierrebai-adsk for taking a look. I'll be working on addressing your comments next.

NickWu · 2022-06-01T04:36:08Z

lib/mayaUsd/ufe/Utils.cpp

@@ -96,6 +96,7 @@ uint32_t findLayerIndex(const UsdPrim& prim, const SdfLayerHandle& layer)
        // to find the layer
        for (SdfLayerRefPtr const& l : layerStack->GetLayers()) {
            if (l == layer) {


I think that's a good idea. Thanks!

NickWu · 2022-06-01T04:37:11Z

lib/mayaUsd/ufe/Utils.cpp

@@ -451,8 +452,15 @@ bool isAttributeEditAllowed(const PXR_NS::UsdAttribute& attr, std::string* errMs
        return false;
    }

+    bool foundLayer = false;


Makes sense. Will do.

NickWu · 2022-06-01T07:15:45Z

lib/mayaUsd/ufe/Utils.cpp

-                    attr.GetBaseName().GetText(),
-                    strongestLayer->GetDisplayName().c_str());
+                    attr.GetName().GetText(),
+                    strongestLayer->GetIdentifier().c_str());


identifier can be an a full URI also. I updated this message because we found displayName, which is the base filename of the identifier, was not helpful enough to troubleshoot issues for us.

NickWu · 2022-06-01T07:16:10Z

test/lib/ufe/testAttributeBlock.py

@@ -347,6 +349,83 @@ def testAttributeBlocking3dMatrixOps(self):

        # authoring new transformation edit is allowed.
        self.assertTrue(mayaUsdUfe.isAttributeEditAllowed(transformAttr))
+
+    def testIsAttributeEditAllowedFunction(self):
+        rootLayerStr = """\


Will do, thanks.

NickWu · 2022-06-01T07:29:00Z

lib/mayaUsd/ufe/Utils.cpp

+
+    // If the edit target layer not found in the prim's layer stack, we return true to allow
+    // new future edits go to the edit target layer.
+    if (!foundLayer)


I just recreated your layer stack and tested it... and the edition of the foo attribute on prim B is already working without your changes. So it is the case that primIndex.GetNodeRange() return all layers, even those without opinions.

So, I think your test is not testing what you hoped it would? You can confirm this by commenting out the early return here and run your test and see if it indeed passes without your changes.

It also means your changes might not be doing what you think it would?

As I found that isAttributeEditAllowed was getting used in many places so my intention of my test was to confirm it's not breaking the previous behaviour and at the same time allowed our pipeline continue to work as it was in maya 2020.

To start, I would need the answer to the first question: is the goal to only allow new opinions on stronger layers or on any layer, even weaker ones?

The goal is to allow new opinions on stronger layers and this was not possible without the change.

NickWu · 2022-06-01T07:32:02Z

lib/mayaUsd/ufe/Utils.cpp

+
+    // If the edit target layer not found in the prim's layer stack, we return true to allow
+    // new future edits go to the edit target layer.
+    if (!foundLayer)


If your use case is that you want to edit in a layer that currently has no opinion but is above the layer, then you could add a check when the layer is not found: search the top opinion layer, iterating through its ancestors and if you find the target layer then allow the edit.

The code already retrieve the strongestLayer a bit below, so move that variable up and use if when foundLayer is false and check that the target is above.

That's a good idea. I'll give it a try. Thanks!

This reverts commit 3fb772d.

allowed edit target layer to be written a new prim spec

3fb772d

csyshing assigned ecp-maya-devops-adsk and unassigned ecp-maya-devops-adsk May 30, 2022

seando-adsk requested a review from pierrebai-adsk May 30, 2022 15:41

seando-adsk added ufe Related to UFE component in Maya unit test Related to unit tests (both python or c++) labels May 30, 2022

pierrebai-adsk reviewed May 31, 2022

View reviewed changes

NickWu commented Jun 1, 2022

View reviewed changes

Nick Wu added 2 commits August 24, 2022 11:45

Revert "allowed edit target layer to be written a new prim spec"

b7e6e09

This reverts commit 3fb772d.

addressed comments

a533165

pierrebai-adsk assigned ecp-maya-devops-adsk Aug 30, 2022

pierrebai-adsk approved these changes Aug 31, 2022

View reviewed changes

seando-adsk added the ready-for-merge Development process is finished, PR is ready for merge label Aug 31, 2022

seando-adsk merged commit b483f61 into Autodesk:dev Aug 31, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allowing a new prim spec to be written in edit target layers #2390

Allowing a new prim spec to be written in edit target layers #2390

NickWu commented May 30, 2022 •

edited

Loading

pierrebai-adsk commented May 30, 2022

pierrebai-adsk May 31, 2022

pierrebai-adsk May 31, 2022

NickWu Jun 1, 2022

pierrebai-adsk May 31, 2022

NickWu Jun 1, 2022

pierrebai-adsk May 31, 2022

NickWu Jun 1, 2022

pierrebai-adsk May 31, 2022

pierrebai-adsk May 31, 2022

pierrebai-adsk May 31, 2022

pierrebai-adsk May 31, 2022

pierrebai-adsk May 31, 2022

NickWu Jul 12, 2022 •

edited

Loading

pierrebai-adsk Jul 14, 2022

NickWu Jul 14, 2022 •

edited

Loading

NickWu Aug 24, 2022

pierrebai-adsk Aug 29, 2022

pierrebai-adsk May 31, 2022

NickWu Jun 1, 2022

NickWu left a comment

NickWu Jun 1, 2022

NickWu Jun 1, 2022

NickWu Jun 1, 2022

NickWu Jun 1, 2022

NickWu Jun 1, 2022

NickWu Jun 1, 2022

Allowing a new prim spec to be written in edit target layers #2390

Allowing a new prim spec to be written in edit target layers #2390

Conversation

NickWu commented May 30, 2022 • edited Loading

pierrebai-adsk commented May 30, 2022

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

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

NickWu Jul 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickWu Jul 14, 2022 • 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

NickWu left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickWu commented May 30, 2022 •

edited

Loading

NickWu Jul 12, 2022 •

edited

Loading

NickWu Jul 14, 2022 •

edited

Loading