MAYA-103547 Use the common utility function to check error conditions #905

fowlertADSK · 2020-11-09T14:10:42Z

There is a utility function in Utils that can be shared between some of the commands (e.g. duplicate and rename). Using it to get the correct logic and proper error message.

There is still a question about what the exact logic/message should be for the commands, but if we want to change it then that will happen in a separate PR.

… for the duplicate command

HamedSabri-adsk · 2020-11-09T16:14:32Z

lib/mayaUsd/ufe/UsdUndoDuplicateCommand.cpp

@@ -59,6 +60,8 @@ void UsdUndoDuplicateCommand::primInfo(
    SdfPath&        usdDstPath,
    SdfLayerHandle& srcLayer)
 {
+    ufe::applyCommandRestriction(srcPrim, "duplicate");


1- both static flavors of primInfo() can go away. All the logics in primInfo can be moved directly to the constructor.

maya-usd/lib/mayaUsd/ufe/UsdUndoDuplicateCommand.h

Line 61 in 5ece36c

static void primInfo();

maya-usd/lib/mayaUsd/ufe/UsdUndoDuplicateCommand.h

Line 53 in 5ece36c

static void primInfo(const UsdPrim& srcPrim, SdfPath& usdDstPath, SdfLayerHandle& srcLayer);

2- fStage can go away. We don't need to save it . It can be obtained from fUfeSrcPath.

3- I believe there is a bug around the layer. You should get layer from the srcPrim and store in

fLayer = GetStage()->GetEditTarget().GetLayer()

Nit-pick: these includes can go away.

#include <mayaUsd/ufe/UsdSceneItem.h>
#include <pxr/usd/usd/stage.h>

all the fxxx member variables should follow the coding standard.

primInfo() doesn't even have an implementation as far as I can tell, so I'll just get rid of it. The other primInfo static is used by Scene Item Ops so can't be moved to the constructor.

Yes, we can get the stage when needed. I think I'd have to use _srcPrim.GetStage() though, fUfeSrcPath doesn't know anything about Usd.

Hmm. fLayer is currently set by passing it to primInfo where it's set to the whatever layer the def primspec is on. What you're suggesting is to change this to always grab the current edit target layer? With the new restriction we want to enforce, I think those two should always be the same, but there is a comment in the next function down that says we chose to not just grab the edit target layer.

I removed one include that didn't look like it was needed. I only see stage.h in the .cpp though, which is needed.

Yes, I can switch to the new naming convention for variables while I'm here.

HamedSabri-adsk · 2020-11-09T16:14:34Z

lib/mayaUsd/ufe/UsdUndoDuplicateCommand.cpp

    auto dstName = uniqueName(childrenNames, srcPrim.GetName());
    usdDstPath = parent.GetPath().AppendChild(TfToken(dstName));


line 66-73 can be factored out. See uniqueChildName utility function:

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

Line 123 in 91994c0

std::string uniqueChildName(const UsdPrim& usdParent, const std::string& name)

HamedSabri-adsk

It would be nice to clean up this class and also back this new change with a test case.

Also putting back in the original error message, which will now only run when the def can't be found. Most of the error conditions will still now be caught by using the new common function though and provide proper error messages.

fowlertADSK · 2020-11-09T18:27:31Z

It would be nice to clean up this class and also back this new change with a test case.
The existing test cases in testDuplicateCmd.py should cover the cases we're checking right now. The only real difference at the moment is that the error message itself should give more details.

The requested changes in MAYA-107582 would be where we will probably want to add some new coverage. At least that's what I think, unless you have a specific case that isn't covered right now?

HamedSabri-adsk · 2020-11-09T20:33:55Z

lib/mayaUsd/ufe/UsdUndoDuplicateCommand.cpp

+        std::string err
+            = TfStringPrintf("No prim found at %s", srcPrim.GetPath().GetString().c_str());
+        throw std::runtime_error(err.c_str());
+    }
 }


Hey @fowlertADSK There is no need to call MayaUsdUtils::defPrimSpecLayer . This routine can be removed all together later. It is currently called in some Ufe1.0 but I believe this can be cleaned up as well.

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

Line 149 in 91994c0

SdfLayerHandle layer = MayaUsdUtils::defPrimSpecLayer(childPrim);

What you're suggesting is to change this to always grab the current edit target layer? With the new restriction we want to enforce, I think those two should always be the same, but there is a comment in the next function down that says we chose to not just grab the edit target layer.

By calling applyCommandRestriction(), you are guaranteed to be in the correct layer. That's the whole idea, this restrict function puts you in a correct layer to perform your change. This layer can be retrieved from the edit target:

_layer = srcPrim.GetStage()->GetEditTarget().GetLayer();

The other primInfo static is used by Scene Item Ops so can't be moved to the constructor.

The name primInfo is confusing and can be simplified. This is how I would imagine the constructor to be

{ ufe::applyCommandRestriction(srcPrim, "duplicate"); _layer = _srcPrim.GetStage()->GetEditTarget().GetLayer(); auto parentPrim = srcPrim.GetParent(); auto dstName = uniqueChildName(parentPrim, srcPrim.GetName()); _usdDstPath = parentPrim.GetPath().AppendChild(TfToken(dstName)); }

Similar to UsdUndoRenameCommand, UsdUndoDuplicateCommand can return the duplicate Scene item which can be returned by UsdSceneItemOps::duplicateItem()

UsdSceneItem::Ptr duplicatedItem() const; { return _ufeDuplicateItem; } Ufe::SceneItem::Ptr UsdSceneItemOps::duplicateItem() { auto duplicateCmd = UsdUndoDuplicateCommand::create(prim(), fItem->path()); duplicateCmd->execute(); return duplicateCmd->duplicatedItem(); } Ufe::Duplicate UsdSceneItemOps::duplicateItemCmd() { auto duplicateCmd = UsdUndoDuplicateCommand::create(prim(), fItem->path()); duplicateCmd->execute(); return Ufe::Duplicate(duplicateCmd->duplicatedItem(), duplicateCmd); }

I hope it makes sense.

ppt-adsk · 2020-11-10T02:03:06Z

lib/mayaUsd/ufe/UsdUndoDuplicateCommand.cpp

    // MAYA-92264: Pixar bug prevents redo from working.  Try again with USD
    // version 0.8.5 or later.  PPT, 28-May-2018.


Well clearly redo() is working, so the comment is obsolete!

@HamedSabri-adsk This is not the comment I was talking about, I meant the comment on lines 82-84:
// We use the source layer as the destination. An alternate workflow
// would be the edit target layer be the destination:
// layer = self._stage.GetEditTarget().GetLayer()
The static duplicate function assumes that they should be on the same layer, but the constructor of the command assumes that it's the edit target.

And for the redo comment... I originally missed this comment, but I did see a similar comment in the test. I removed that comment and reenabled the test for redo.

Yeah, that comment can also be clean up.

I believe both srcLayer and dstLayer that are being passed to SdfCopySpec should be the same. This is the _layer that is populated in the constructor.

My thinking is that the duplicate always happen in one single layer. Kind of similar to how rename works.

- Changed the input to be a single UsdSceneItem instead of Prim + Path. - Changed it to keep around a Ufe::Path and a SdfPath for src/dst objects instead of the various objects it had before (prim/stage/layer/two paths). - Create the new UsdSceneItem on-demand instead of keeping it around through undo/redo.

HamedSabri-adsk

LGTM! Thank you for the extra clean up. It certainly makes it easier to follow the logics.

HamedSabri-adsk · 2020-11-11T23:26:08Z

lib/mayaUsd/ufe/UsdUndoDuplicateCommand.cpp

 #include <ufe/scene.h>
 #include <ufe/sceneNotification.h>

+#include <iostream>


Nit-pick: this include can be removed.

MAYA-103547 Use the common utility function to check error conditions…

5ece36c

… for the duplicate command

fowlertADSK requested a review from HamedSabri-adsk November 9, 2020 14:10

fowlertADSK assigned ecp-maya-devops-adsk Nov 9, 2020

HamedSabri-adsk reviewed Nov 9, 2020

View reviewed changes

HamedSabri-adsk suggested changes Nov 9, 2020

View reviewed changes

fowlertADSK added 2 commits November 9, 2020 13:28

MAYA-103547 changes requested in code review

dcad078

MAYA-103547 missed a file

5ea6329

fowlertADSK assigned ecp-maya-devops-adsk and unassigned ecp-maya-devops-adsk Nov 9, 2020

HamedSabri-adsk reviewed Nov 9, 2020

View reviewed changes

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

ppt-adsk reviewed Nov 10, 2020

View reviewed changes

HamedSabri-adsk approved these changes Nov 11, 2020

View reviewed changes

HamedSabri-adsk assigned ecp-maya-devops-adsk and unassigned ecp-maya-devops-adsk Nov 11, 2020

HamedSabri-adsk reviewed Nov 11, 2020

View reviewed changes

fowlertADSK added the ready-for-merge Development process is finished, PR is ready for merge label Nov 12, 2020

kxl-adsk merged commit 7bc35c3 into dev Nov 12, 2020

kxl-adsk deleted the fowlert/MAYA-103547/provide_better_error_messages branch November 12, 2020 16:40

samuelliu-adsk mentioned this pull request Apr 24, 2023

Remove/replace all UFE_PREVIEW_VERSION_NUM #3020

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MAYA-103547 Use the common utility function to check error conditions #905

MAYA-103547 Use the common utility function to check error conditions #905

fowlertADSK commented Nov 9, 2020

HamedSabri-adsk Nov 9, 2020

HamedSabri-adsk Nov 9, 2020

fowlertADSK Nov 9, 2020

HamedSabri-adsk Nov 9, 2020

fowlertADSK Nov 9, 2020

HamedSabri-adsk left a comment •

edited

Loading

fowlertADSK commented Nov 9, 2020

HamedSabri-adsk Nov 9, 2020 •

edited

Loading

ppt-adsk Nov 10, 2020

fowlertADSK Nov 10, 2020

fowlertADSK Nov 10, 2020

HamedSabri-adsk Nov 10, 2020

HamedSabri-adsk left a comment

HamedSabri-adsk Nov 11, 2020

		auto dstName = uniqueName(childrenNames, srcPrim.GetName());
		usdDstPath = parent.GetPath().AppendChild(TfToken(dstName));

		// MAYA-92264: Pixar bug prevents redo from working. Try again with USD
		// version 0.8.5 or later. PPT, 28-May-2018.

MAYA-103547 Use the common utility function to check error conditions #905

MAYA-103547 Use the common utility function to check error conditions #905

Conversation

fowlertADSK commented Nov 9, 2020

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 left a comment • edited Loading

Choose a reason for hiding this comment

fowlertADSK commented Nov 9, 2020

HamedSabri-adsk Nov 9, 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 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HamedSabri-adsk left a comment •

edited

Loading

HamedSabri-adsk Nov 9, 2020 •

edited

Loading