Update to newer UFE interface for Create Group #593
Conversation
| @@ -17,6 +17,7 @@ | |||
|
|
|||
| #include <ufe/hierarchy.h> | |||
| #include <ufe/path.h> | |||
| #include <ufe/selection.h> | |||
There was a problem hiding this comment.
Not needed, fwd decl sufficient.
lib/mayaUsd/ufe/UsdHierarchy.cpp
Outdated
|
|
||
| UsdUndoCreateGroupCommand::Ptr cmd = UsdUndoCreateGroupCommand::create(fItem, selection, name.string()); | ||
| if (cmd) { | ||
| cmd->redo(); |
There was a problem hiding this comment.
Not that it makes much difference in your case, because the base class UndoableCommand::execute() calls redo(), but in case we ever implement UsdUndoCreateGroupCommand::execute() it would be future-proof to call cmd->execute() here.
| void undo() override; | ||
| void redo() override; | ||
|
|
||
| const Ufe::Path& newUfePath() const; | ||
|
|
||
| static UsdUndoAddNewPrimCommand::Ptr create(const UsdSceneItem::Ptr& usdSceneItem, |
|
|
||
| const Ufe::Path& newUfePath() const; | ||
|
|
||
| static UsdUndoAddNewPrimCommand::Ptr create(const UsdSceneItem::Ptr& usdSceneItem, |
There was a problem hiding this comment.
Actually I'm voting to leave as-is. All the other command classes have the full namespace listed here (so this one would look a bit odd if it was the only one that didn't), and I actually like it because there are multiple "Ptr" typedefs being used (e.g. this class has a Ptr and there's also the scene item Ptr's being used in the same functions).
| @@ -0,0 +1,100 @@ | |||
| // | |||
| // Copyright 2019 Autodesk | |||
There was a problem hiding this comment.
I guess we have to update the copyright year?
There was a problem hiding this comment.
Yeah, this is the new file and I should have 2020 here (it's there in the .h). We don't update these dates in all files though.
| auto stage = prim.GetStage(); | ||
| auto usdPath = prim.GetPath(); | ||
| stage->RemovePrim(usdPath); | ||
|
|
||
| fGroup.reset(); | ||
| _group.reset(); | ||
| #else |
There was a problem hiding this comment.
I think you should really consider deriving from Ufe::CompositeUndoableCommand, also in undoableCommand.h. It does this for you; in fact, you would not even need to implement undo() or redo().
|
|
||
| _group = std::dynamic_pointer_cast<UsdSceneItem>(newParentSceneItem); | ||
| } | ||
| else { |
There was a problem hiding this comment.
Same comment as for undo(): if you derived from CompositeUndoableCommand, you would not need to implement redo().
| Ufe::SceneItem::Ptr group = hierarchy->createGroup(_name); | ||
| _group = std::dynamic_pointer_cast<UsdSceneItem>(group); | ||
| #else | ||
| if (_ufeUndoCommands.empty()) { |
There was a problem hiding this comment.
I think you should implement this "first time" behavior by overriding UndoableCommand::execute() rather than redo(); that's what it's for.
| _ufeUndoCommands.push_back(addPrimCmd); | ||
| addPrimCmd->redo(); | ||
|
|
||
| auto newParentSceneItem = Ufe::Hierarchy::createItem(addPrimCmd->newUfePath()); |
There was a problem hiding this comment.
This is nice, but since we're all in USD-land here, if we could just get the UsdPrim out of the addPrimCmd (and we know it's there), we could just call UsdSceneItem::create() directly, get a derived class UsdSceneItem out of it, and assign directly into _group, saving the dynamic_pointer_cast at line 122. What do you think? All you need is a prim accessor in UsdUndoAddNewPrimCommand, which sounds reasonable to me.
| UsdUndoCreateGroupCommand(const UsdSceneItem::Ptr& parentItem, const Ufe::PathComponent& name); | ||
| #else | ||
| typedef std::list<std::shared_ptr<Ufe::UndoableCommand>> UfeUndoCommands; |
There was a problem hiding this comment.
As per other comments, if you derived from CompositeUndoableCommand, you wouldn't need this.
| // First get the stage from the proxy shape. | ||
| auto ufePath = usdSceneItem->path(); | ||
| auto segments = ufePath.getSegments(); | ||
| auto dagSegment = segments[0]; | ||
| _stage = MayaUsd::ufe::getStage(Ufe::Path(dagSegment)); |
There was a problem hiding this comment.
Since you have a UsdSceneItem, to get the stage you can just use: usdSceneItem->prim().GetStage()
|
|
||
| Ufe::SceneItem::Ptr ProxyShapeHierarchy::createGroup(const Ufe::Selection& selection, const Ufe::PathComponent& name) const | ||
| { | ||
| Ufe::SceneItem::Ptr createdItem = nullptr; |
There was a problem hiding this comment.
Since these are shared_ptr, you don't really need '= nullptr'
No description provided.