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

Implement and test setMatrixCmd() for single matrix op prims. #1102

Merged
merged 5 commits into from
Feb 5, 2021
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
1 change: 0 additions & 1 deletion lib/mayaUsd/python/wrapUsdUndoManager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@

#include <mayaUsd/undo/UsdUndoBlock.h>
#include <mayaUsd/undo/UsdUndoManager.h>
#include <mayaUsd/undo/UsdUndoableItem.h>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Cleanup.


#include <pxr/base/tf/pyContainerConversions.h>
#include <pxr/base/tf/pyNoticeWrapper.h>
Expand Down
15 changes: 11 additions & 4 deletions lib/mayaUsd/ufe/UsdTransform3dBase.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -99,15 +99,22 @@ Ufe::Vector3d UsdTransform3dBase::scalePivotTranslation() const { return Ufe::Ve
#ifdef UFE_V2_FEATURES_AVAILABLE
Ufe::SetMatrix4dUndoableCommand::Ptr UsdTransform3dBase::setMatrixCmd(const Ufe::Matrix4d& m)
{
// TODO: HS Aug25,2020 dummy code to pass the compiler errors
return nullptr;
}

Ufe::Matrix4d UsdTransform3dBase::matrix() const
{
// TODO: HS Aug25,2020 dummy code to pass the compiler errors
Ufe::Matrix4d m {};
return m;
UsdGeomXformable xformable(prim());
bool unused;
auto ops = xformable.GetOrderedXformOps(&unused);

GfMatrix4d m(1);
if (!UsdGeomXformable::GetLocalTransformation(&m, ops, getTime(path()))) {
TF_FATAL_ERROR(
"Local transformation computation for prim %s failed.", prim().GetPath().GetText());
}

return toUfe(m);
}
#endif

Expand Down
74 changes: 68 additions & 6 deletions lib/mayaUsd/ufe/UsdTransform3dMatrixOp.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@

#include <mayaUsd/ufe/UsdSceneItem.h>
#include <mayaUsd/ufe/Utils.h>
#include <mayaUsd/undo/UsdUndoBlock.h>
#include <mayaUsd/undo/UsdUndoableItem.h>

#include <pxr/base/gf/rotation.h>
#include <pxr/base/gf/transform.h>
Expand All @@ -30,6 +32,7 @@

namespace {

using namespace MayaUsd;
using namespace MayaUsd::ufe;

VtValue getValue(const UsdAttribute& attr, const UsdTimeCode& time)
Expand Down Expand Up @@ -76,6 +79,42 @@ computeLocalTransform(const UsdPrim& prim, const UsdGeomXformOp& op, const UsdTi
auto computeLocalInclusiveTransform = computeLocalTransform<true>;
auto computeLocalExclusiveTransform = computeLocalTransform<false>;

// Class for setMatrixCmd() implementation. UsdUndoBlock data member and
// undo() / redo() should be factored out into a future command base class.
class UsdSetMatrix4dUndoableCmd : public Ufe::SetMatrix4dUndoableCommand
{
public:
UsdSetMatrix4dUndoableCmd(const Ufe::Path& path, const Ufe::Matrix4d& newM)
: Ufe::SetMatrix4dUndoableCommand(path)
, _newM(newM)
{
}

~UsdSetMatrix4dUndoableCmd() override { }

bool set(const Ufe::Matrix4d&) override
{
// No-op: Maya does not set matrices through interactive manipulation.
TF_WARN("Illegal call to UsdSetMatrix4dUndoableCmd::set()");
return true;
}

void execute() override
{
UsdUndoBlock undoBlock(&_undoableItem);

auto t3d = Ufe::Transform3d::transform3d(sceneItem());
t3d->setMatrix(_newM);
}

void undo() override { _undoableItem.undo(); }
void redo() override { _undoableItem.redo(); }

private:
UsdUndoableItem _undoableItem;
const Ufe::Matrix4d _newM;
};

// Helper class to factor out common code for translate, rotate, scale
// undoable commands.
class UsdTRSUndoableCmdBase
Expand Down Expand Up @@ -351,7 +390,7 @@ Ufe::ScaleUndoableCommand::Ptr UsdTransform3dMatrixOp::scaleCmd(double x, double

Ufe::SetMatrix4dUndoableCommand::Ptr UsdTransform3dMatrixOp::setMatrixCmd(const Ufe::Matrix4d& m)
{
return nullptr;
return std::make_shared<UsdSetMatrix4dUndoableCmd>(path(), m);
}

void UsdTransform3dMatrixOp::setMatrix(const Ufe::Matrix4d& m) { _op.Set(toUsd(m)); }
Expand Down Expand Up @@ -402,11 +441,34 @@ UsdTransform3dMatrixOpHandler::create(const Ufe::Transform3dHandler::Ptr& nextHa
Ufe::Transform3d::Ptr
UsdTransform3dMatrixOpHandler::transform3d(const Ufe::SceneItem::Ptr& item) const
{
// This method can be used to edit the 3D transform of the argument, but at
// time of writing this is not implemented in UsdTransform3dMatrixOp, and
// our UsdTransform3dBaseHandler base class does not know how to edit the
// argument either. Simply delegate to the next handler in the list.
return _nextHandler->transform3d(item);
// Remove code duplication with editTransform3d(). PPT, 21-Jan-2021.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy-paste from editTransform3d() a bit of a shame, hopefully can fix later.

UsdSceneItem::Ptr usdItem = std::dynamic_pointer_cast<UsdSceneItem>(item);
#if !defined(NDEBUG)
assert(usdItem);

Choose a reason for hiding this comment

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

I haven't really heard any good reason yet why to use these naked asserts over a rich number of possibilities to raise an error/warning via Tf diagnostic macros. Please switch over.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed, this is simply a copy-paste of older code.

#endif

auto opName = getMatrixOp();
UsdGeomXformable xformable(usdItem->prim());
bool unused;
auto xformOps = xformable.GetOrderedXformOps(&unused);
auto i = std::find_if(xformOps.begin(), xformOps.end(), [opName](const UsdGeomXformOp& op) {
return (op.GetOpType() == UsdGeomXformOp::TypeTransform)
&& (!opName || std::string(opName) == op.GetOpName());
});
bool foundMatrix = (i != xformOps.end());

bool moreLocalNonMatrix = foundMatrix
? (std::find_if(
i,
xformOps.end(),
[](const UsdGeomXformOp& op) {
return op.GetOpType() != UsdGeomXformOp::TypeTransform;
})
!= xformOps.end())
: false;

return (foundMatrix && !moreLocalNonMatrix) ? UsdTransform3dMatrixOp::create(usdItem, *i)
: _nextHandler->transform3d(item);
}

Ufe::Transform3d::Ptr UsdTransform3dMatrixOpHandler::editTransform3d(
Expand Down
40 changes: 16 additions & 24 deletions lib/mayaUsd/ufe/UsdTransform3dMayaXformStack.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -162,21 +162,13 @@ createTransform3d(const Ufe::SceneItem::Ptr& item, NextTransform3dFn nextTransfo
return stackOps.empty() ? nextTransform3dFn() : UsdTransform3dMayaXformStack::create(usdItem);
}

// Class for setMatrixCmd() implementation. Should be rolled into a future
// command class with full undo / redo support
// Class for setMatrixCmd() implementation. UsdUndoBlock data member and
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed to use new undo / redo framework.

// undo() / redo() should be factored out into a future command base class.
class UsdSetMatrix4dUndoableCmd : public Ufe::SetMatrix4dUndoableCommand
{
public:
UsdSetMatrix4dUndoableCmd(
const Ufe::Path& path,
const Ufe::Vector3d& oldT,
const Ufe::Vector3d& oldR,
const Ufe::Vector3d& oldS,
const Ufe::Matrix4d& newM)
UsdSetMatrix4dUndoableCmd(const Ufe::Path& path, const Ufe::Matrix4d& newM)
: Ufe::SetMatrix4dUndoableCommand(path)
, _oldT(oldT)
, _oldR(oldR)
, _oldS(oldS)
{
// Decompose new matrix to extract TRS. Neither GfMatrix4d::Factor
// nor GfTransform decomposition provide results that match Maya,
Expand Down Expand Up @@ -206,20 +198,21 @@ class UsdSetMatrix4dUndoableCmd : public Ufe::SetMatrix4dUndoableCommand
return true;
}

void undo() override { perform(_oldT, _oldR, _oldS); }
void redo() override { perform(_newT, _newR, _newS); }

private:
void perform(const Ufe::Vector3d& t, const Ufe::Vector3d& r, const Ufe::Vector3d& s)
void execute() override
{
UsdUndoBlock undoBlock(&_undoableItem);

auto t3d = Ufe::Transform3d::transform3d(sceneItem());
t3d->translate(t.x(), t.y(), t.z());
t3d->rotate(r.x(), r.y(), r.z());
t3d->scale(s.x(), s.y(), s.z());
t3d->translate(_newT.x(), _newT.y(), _newT.z());
t3d->rotate(_newR.x(), _newR.y(), _newR.z());
t3d->scale(_newS.x(), _newS.y(), _newS.z());
}
Ufe::Vector3d _oldT;
Ufe::Vector3d _oldR;
Ufe::Vector3d _oldS;

void undo() override { _undoableItem.undo(); }
void redo() override { _undoableItem.redo(); }

private:
UsdUndoableItem _undoableItem;
Ufe::Vector3d _newT;
Ufe::Vector3d _newR;
Ufe::Vector3d _newS;
Expand Down Expand Up @@ -698,8 +691,7 @@ UsdTransform3dMayaXformStack::pivotCmd(const TfToken& pvtOpSuffix, double x, dou
Ufe::SetMatrix4dUndoableCommand::Ptr
UsdTransform3dMayaXformStack::setMatrixCmd(const Ufe::Matrix4d& m)
{
return std::make_shared<UsdSetMatrix4dUndoableCmd>(
path(), translation(), rotation(), scale(), m);
return std::make_shared<UsdSetMatrix4dUndoableCmd>(path(), m);
}

UsdTransform3dMayaXformStack::SetXformOpOrderFn
Expand Down
2 changes: 2 additions & 0 deletions lib/mayaUsd/ufe/UsdUndoSetKindCommand.h
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@

#include <memory>

PXR_NAMESPACE_USING_DIRECTIVE
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Was relying on UsdUndoableItem.h to define this, but UsdUndoableItem.h doesn't need this definition and was removed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @ppt-adsk! I noticed this pattern in some of the other UFE classes and I actually intentionally avoided doing the same here when I added this command in #1094. This macro effectively resolves to a using namespace ... statement which it seems like we would want to avoid in headers:
https://github.com/PixarAnimationStudios/USD/blob/dc710925675f7f58f7a37f6c18d046a687b09271/pxr/pxr.h.in#L52

Instead I had attempted to resolve the symbols by prefixing all of the USD types with PXR_NS::, but looks like I probably missed a few. Reading through that class again, I would guess it's at least these in the header:


And maybe this in the implementation:

UsdModelAPI(_prim).SetKind(_kind);

Though I think using the PXR_NAMESPACE_USING_DIRECTIVE macro in a .cpp file would be fair game if we wanted to do that instead.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Makes perfect sense, fully agree with you. Let me wait until a bit more feedback builds up and I'll fix this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good, thanks! And sorry for leaving that trap for you. :)

Choose a reason for hiding this comment

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

Are we ready to set a course on getting rid of all using directives from the header files in the core modules? That would be fantastic.


namespace MAYAUSD_NS_DEF {
namespace ufe {

Expand Down
3 changes: 1 addition & 2 deletions lib/mayaUsd/undo/UsdUndoBlock.h
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,8 @@
#ifndef MAYAUSD_UNDO_UNDOBLOCK_H
#define MAYAUSD_UNDO_UNDOBLOCK_H

#include "UsdUndoableItem.h"

#include <mayaUsd/base/api.h>
#include <mayaUsd/undo/UsdUndoableItem.h>

#include <pxr/base/tf/declarePtrs.h>

Expand Down
4 changes: 2 additions & 2 deletions lib/mayaUsd/undo/UsdUndoableItem.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@

#include "UsdUndoableItem.h"

#include "UsdUndoBlock.h"
#include <mayaUsd/undo/UsdUndoBlock.h>

#include <pxr/usd/sdf/changeBlock.h>

Expand Down Expand Up @@ -44,4 +44,4 @@ void UsdUndoableItem::doInvert()
}
}

} // namespace MAYAUSD_NS_DEF
} // namespace MAYAUSD_NS_DEF
8 changes: 3 additions & 5 deletions lib/mayaUsd/undo/UsdUndoableItem.h
Original file line number Diff line number Diff line change
Expand Up @@ -14,16 +14,14 @@
// limitations under the License.
//

#ifndef MAYAUSD_UNDO_UNDOABLE_BLOCK_H
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed copy-paste damage... Was causing really, really weird compilation errors for my local changes...

#define MAYAUSD_UNDO_UNDOABLE_BLOCK_H
#ifndef MAYAUSD_UNDO_UNDOABLE_ITEM_H
#define MAYAUSD_UNDO_UNDOABLE_ITEM_H

#include <mayaUsd/base/api.h>

#include <functional>
#include <vector>

PXR_NAMESPACE_USING_DIRECTIVE
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No Pixar namespace declarations or definitions here, removed.

Choose a reason for hiding this comment

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

love it


namespace MAYAUSD_NS_DEF {

//! \brief UsdUndoableItem
Expand Down Expand Up @@ -62,4 +60,4 @@ class MAYAUSD_CORE_PUBLIC UsdUndoableItem

} // namespace MAYAUSD_NS_DEF

#endif // MAYAUSD_UNDO_UNDOABLE_BLOCK_H
#endif // MAYAUSD_UNDO_UNDOABLE_ITEM_H
Loading