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-103547 Use the common utility function to check error conditions #905

Merged
merged 5 commits into from
Nov 12, 2020
Merged

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

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5ece36c
MAYA-103547 Use the common utility function to check error conditions…
fowlertADSK Nov 9, 2020
05c701b
MAYA-103547 Should not have removed the return of the def primspec
fowlertADSK Nov 9, 2020
dcad078
MAYA-103547 changes requested in code review
fowlertADSK Nov 9, 2020
5ea6329
MAYA-103547 missed a file
fowlertADSK Nov 9, 2020
837f9e3
MAYA-103547 More cleanup of the duplicate cmd.
fowlertADSK Nov 11, 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
16 changes: 3 additions & 13 deletions lib/mayaUsd/ufe/UsdUndoDuplicateCommand.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
#include "UsdUndoDuplicateCommand.h"

#include "private/UfeNotifGuard.h"
#include "private/Utils.h"

#include <mayaUsd/ufe/Utils.h>
#include <mayaUsdUtils/util.h>
Expand Down Expand Up @@ -59,6 +60,8 @@ void UsdUndoDuplicateCommand::primInfo(
SdfPath& usdDstPath,
SdfLayerHandle& srcLayer)
{
ufe::applyCommandRestriction(srcPrim, "duplicate");
Copy link
Contributor

Choose a reason for hiding this comment

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

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()

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

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

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

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


auto parent = srcPrim.GetParent();
TfToken::HashSet childrenNames;
for (auto child : parent.GetFilteredChildren(UsdPrimIsDefined && !UsdPrimIsAbstract)) {
Expand All @@ -69,19 +72,6 @@ void UsdUndoDuplicateCommand::primInfo(
// has a numerical suffix, increment it, otherwise append "1" to it.
auto dstName = uniqueName(childrenNames, srcPrim.GetName());
usdDstPath = parent.GetPath().AppendChild(TfToken(dstName));
Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


// Iterate over the layer stack, starting at the highest-priority layer.
// The source layer is the one in which there exists a def primSpec, not
// an over. An alternative would have beeen to call Sdf.CopySpec for
// each layer in which there is an over or a def, until we reach the
// layer with a def primSpec. This would preserve the visual appearance
// of the duplicate. PPT, 12-Jun-2018.
srcLayer = MayaUsdUtils::defPrimSpecLayer(srcPrim);
if (!srcLayer) {
std::string err
= TfStringPrintf("No prim found at %s", srcPrim.GetPath().GetString().c_str());
throw std::runtime_error(err.c_str());
}
}
Copy link
Contributor

@HamedSabri-adsk HamedSabri-adsk Nov 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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.


/*static*/
Expand Down