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.

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

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

Merged
merged 3 commits into from
Aug 31, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
19 changes: 14 additions & 5 deletions lib/mayaUsd/ufe/Utils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ bool stringBeginsWithDigit(const std::string& inputString)

// This function calculates the position index for a given layer across all
// the site's local LayerStacks
uint32_t findLayerIndex(const UsdPrim& prim, const SdfLayerHandle& layer)
uint32_t findLayerIndex(const UsdPrim& prim, const SdfLayerHandle& layer, bool& foundLayer)
{
uint32_t position { 0 };

Expand All @@ -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) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

I think that's a good idea. Thanks!

foundLayer = true;
return position;
}
++position;
Expand Down Expand Up @@ -451,8 +452,15 @@ bool isAttributeEditAllowed(const PXR_NS::UsdAttribute& attr, std::string* errMs
return false;
}

bool foundLayer = false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

Makes sense. Will do.


// get the index to edit target layer
const auto targetLayerIndex = findLayerIndex(prim, editTarget.GetLayer());
const auto targetLayerIndex = findLayerIndex(prim, editTarget.GetLayer(), foundLayer);

// 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)
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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:

SdfLayerHandle getStrongerLayer(

Copy link
Author

@NickWu NickWu Jul 12, 2022

Choose a reason for hiding this comment

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

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Author

@NickWu NickWu Jul 14, 2022

Choose a reason for hiding this comment

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

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!

Copy link
Author

Choose a reason for hiding this comment

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

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!

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

return true;

// HS March 22th,2021
// TODO: "Value Clips" are UsdStage-level feature, unknown to Pcp.So if the attribute in
Expand All @@ -470,14 +478,15 @@ bool isAttributeEditAllowed(const PXR_NS::UsdAttribute& attr, std::string* errMs
if (!propertyStack.empty()) {
// get the strongest layer that has the attr.
auto strongestLayer = attr.GetPropertyStack().front()->GetLayer();
foundLayer = false;

// compare the calculated index between the "attr" and "edit target" layers.
if (findLayerIndex(prim, strongestLayer) < targetLayerIndex) {
if (findLayerIndex(prim, strongestLayer, foundLayer) < targetLayerIndex) {
if (errMsg) {
*errMsg = TfStringPrintf(
"Cannot edit [%s] attribute because there is a stronger opinion in [%s].",
attr.GetBaseName().GetText(),
strongestLayer->GetDisplayName().c_str());
attr.GetName().GetText(),
strongestLayer->GetIdentifier().c_str());
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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.

}

return false;
Expand Down
81 changes: 80 additions & 1 deletion test/lib/ufe/testAttributeBlock.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
import ufeUtils
import usdUtils

from pxr import UsdGeom, Vt, Gf
from pxr import UsdGeom, Vt, Gf, Usd, Sdf

from maya import cmds
from maya import standalone
Expand All @@ -32,6 +32,8 @@
import ufe
import os
import unittest
import textwrap


class AttributeBlockTestCase(unittest.TestCase):

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

# authoring new transformation edit is allowed.
self.assertTrue(mayaUsdUfe.isAttributeEditAllowed(transformAttr))

def testIsAttributeEditAllowedFunction(self):
rootLayerStr = """\
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

Will do, thanks.

#sdf 1.4.32
()
def "root"
{
}
"""
subLayerAStr = """\
#sdf 1.4.32
over "root"
{
def "a"
{
float foo = 0.0
}

def "b"
{
float foo = 0.0
}
}
"""
subLayerBStr = """\
#sdf 1.4.32
over "root"
{
}
"""
subLayerCStr = """\
#sdf 1.4.32
over "root"
{
over "a"
{
float foo = 10.0
}
}
"""

rootLayer = Sdf.Layer.CreateAnonymous()
rootLayer.ImportFromString(textwrap.dedent(rootLayerStr))

subLayerA = Sdf.Layer.CreateAnonymous()
subLayerA.ImportFromString(textwrap.dedent(subLayerAStr))

subLayerB = Sdf.Layer.CreateAnonymous()
subLayerB.ImportFromString(textwrap.dedent(subLayerBStr))

subLayerC = Sdf.Layer.CreateAnonymous()
subLayerC.ImportFromString(textwrap.dedent(subLayerCStr))

rootLayer.subLayerPaths = [
subLayerC.identifier,
subLayerB.identifier,
subLayerA.identifier,
]
stage = Usd.Stage.Open(rootLayer)

primB = stage.GetPrimAtPath("/root/b")
self.assertTrue(primB.IsValid())

fooattrB = primB.GetAttribute("foo")
self.assertTrue(fooattrB.IsValid())

stage.SetEditTarget(Usd.EditTarget(subLayerB))
self.assertTrue(mayaUsdUfe.isAttributeEditAllowed(fooattrB))

primA = stage.GetPrimAtPath("/root/a")
self.assertTrue(primA.IsValid())

fooattrA = primA.GetAttribute("foo")
self.assertTrue(fooattrA.IsValid())

self.assertFalse(mayaUsdUfe.isAttributeEditAllowed(fooattrA))


if __name__ == '__main__':
unittest.main(verbosity=2)