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

Jump to bottom

Fallback, multi-matrix parent absolute. #1201

Merged
merged 6 commits into from
Feb 24, 2021
Merged

Fallback, multi-matrix parent absolute. #1201

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
bc6f29c
Fallback, multi-matrix parent absolute.
ppt-adsk Feb 17, 2021
c8e1e83
Fix pre-flight errors.
ppt-adsk Feb 22, 2021
5c281ba
Fix more pre-flight errors.
ppt-adsk Feb 22, 2021
fe35686
Fix clang-format error.
ppt-adsk Feb 22, 2021
b31c057
Restore initial conditions after multi-matrix parent test.
ppt-adsk Feb 23, 2021
3892d75
Addressed code review feedback.
ppt-adsk Feb 23, 2021
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
2 changes: 2 additions & 0 deletions lib/mayaUsd/ufe/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ if(CMAKE_UFE_V2_FEATURES_AVAILABLE)
UsdTransform3dFallbackMayaXformStack.cpp
UsdTransform3dMatrixOp.cpp
UsdTransform3dMayaXformStack.cpp
UsdTransform3dSetObjectMatrix.cpp
UsdUIInfoHandler.cpp
UsdUndoAddNewPrimCommand.cpp
UsdUndoCreateGroupCommand.cpp
Expand Down Expand Up @@ -107,6 +108,7 @@ if(CMAKE_UFE_V2_FEATURES_AVAILABLE)
UsdTransform3dFallbackMayaXformStack.h
UsdTransform3dMatrixOp.h
UsdTransform3dMayaXformStack.h
UsdTransform3dSetObjectMatrix.h
UsdUIInfoHandler.h
UsdUndoAddNewPrimCommand.h
UsdUndoCreateGroupCommand.h
Expand Down
60 changes: 53 additions & 7 deletions lib/mayaUsd/ufe/UsdTransform3dFallbackMayaXformStack.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
#include "UsdTransform3dFallbackMayaXformStack.h"

#include <mayaUsd/ufe/RotationUtils.h>
#include <mayaUsd/ufe/UsdTransform3dSetObjectMatrix.h>
#include <mayaUsd/ufe/Utils.h>
#include <mayaUsd/ufe/XformOpUtils.h>

Expand Down Expand Up @@ -143,7 +144,16 @@ void setXformOpOrder(const UsdGeomXformable& xformable)
xformable.SetXformOpOrder(newOrder, resetsXformStack);
}

Ufe::Transform3d::Ptr createTransform3d(const Ufe::SceneItem::Ptr& item)
// Create a Ufe::Transform3d interface to edit the Maya fallback transform
// stack. This engine method is used in the implementation of
// createTransform3d() and createEditTransform3d(). To avoid having the caller
// repeat these calls for its own use, the prim's transform ops are returned in
// xformOps, along with an iterator to the first Maya fallback transform op in
// firstFallbackOp.
Ufe::Transform3d::Ptr createEditTransform3dImp(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Factored out common code between createTransform3d() and editTransform3d().

Copy link

Choose a reason for hiding this comment

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

Would be good to add documentation for parameters. As it is now, it's unclear without reading the implementation to know which arguments are input vs output. Adding a prefix is another nice way of helping developers that have to work with your code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will add documentation. As for input versus output, I usually stick to
http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#f17-for-in-out-parameters-pass-by-reference-to-non-const
so that non-const ref parameters will be modified inside the function, but I will add that information in the documentation.

Copy link

Choose a reason for hiding this comment

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

I would prefer to see more usage of Doxygen formatted documentation with proper usage of direction [in] and [out] to make this clear - https://www.doxygen.nl/manual/commands.html#cmdparam. I won't block the review on it.

const Ufe::SceneItem::Ptr& item,
std::vector<UsdGeomXformOp>& xformOps,
std::vector<UsdGeomXformOp>::const_iterator& firstFallbackOp)
{
UsdSceneItem::Ptr usdItem = std::dynamic_pointer_cast<UsdSceneItem>(item);
#if !defined(NDEBUG)
Expand All @@ -160,7 +170,7 @@ Ufe::Transform3d::Ptr createTransform3d(const Ufe::SceneItem::Ptr& item)
return nullptr;
}
bool resetsXformStack = false;
auto xformOps = xformSchema.GetOrderedXformOps(&resetsXformStack);
xformOps = xformSchema.GetOrderedXformOps(&resetsXformStack);

// We are the fallback Transform3d handler: there must be transform ops to
// match.
Expand All @@ -172,19 +182,19 @@ Ufe::Transform3d::Ptr createTransform3d(const Ufe::SceneItem::Ptr& item)
// fallback component token. If no transform op matches the fallback
// component token, we start a new Maya transform stack at the end of the
// existing stack.
auto i = findFirstFallbackOp(xformOps);
firstFallbackOp = findFirstFallbackOp(xformOps);

// No transform op matched: start a new Maya transform stack at the end.
if (i == xformOps.end()) {
if (firstFallbackOp == xformOps.end()) {
return UsdTransform3dFallbackMayaXformStack::create(usdItem);
}

// Otherwise, copy ops starting at the first fallback op we found. If all
// is well, from the first fallback op onwards, we have a sub-stack that
// matches the fallback Maya transform stack.
std::vector<UsdGeomXformOp> candidateOps;
candidateOps.reserve(std::distance(i, xformOps.cend()));
std::copy(i, xformOps.cend(), std::back_inserter(candidateOps));
candidateOps.reserve(std::distance(firstFallbackOp, xformOps.cend()));
std::copy(firstFallbackOp, xformOps.cend(), std::back_inserter(candidateOps));

// We're the last handler in the chain of responsibility: if the candidate
// ops support the Maya transform stack, create a Maya transform stack
Expand All @@ -193,6 +203,42 @@ Ufe::Transform3d::Ptr createTransform3d(const Ufe::SceneItem::Ptr& item)
: nullptr;
}

Ufe::Transform3d::Ptr createTransform3d(const Ufe::SceneItem::Ptr& item)
{
// This Transform3d interface is for editing the whole object, e.g. setting
// the local transformation matrix for the complete object. We do this
// by wrapping an edit transform 3d interface into a
// UsdTransform3dSetObjectMatrix object.
std::vector<UsdGeomXformOp> xformOps;
std::vector<UsdGeomXformOp>::const_iterator firstFallbackOp;

auto editTransform3d = createEditTransform3dImp(item, xformOps, firstFallbackOp);
if (!editTransform3d) {
return nullptr;
}

// Ml is the transformation before the Maya fallback transform stack.
std::vector<UsdGeomXformOp> mlOps(std::distance(xformOps.cbegin(), firstFallbackOp));
mlOps.assign(xformOps.cbegin(), firstFallbackOp);

GfMatrix4d ml { 1 };
if (!UsdGeomXformable::GetLocalTransformation(&ml, mlOps, getTime(item->path()))) {
TF_FATAL_ERROR(
"Local transformation computation for item %s failed.", item->path().string().c_str());
}

// The Maya fallback transform stack is the last group of transform ops in
// the complete transform stack, so Mr and thus inv(Mr), are the identity.
return UsdTransform3dSetObjectMatrix::create(editTransform3d, ml.GetInverse(), GfMatrix4d(1));
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

transform3d() interface is used to set the object position (under parent -absolute) using the complete transform stack. To do this, use the new UsdTransform3dSetObjectMatrix wrapper that takes an editTransform3d() interface, along with the matrices to perform the matrix algebra to change only the fallback transform ops.

}

Ufe::Transform3d::Ptr createEditTransform3d(const Ufe::SceneItem::Ptr& item)
{
std::vector<UsdGeomXformOp> xformOps;
std::vector<UsdGeomXformOp>::const_iterator firstFallbackOp;
return createEditTransform3dImp(item, xformOps, firstFallbackOp);
}

} // namespace

UsdTransform3dFallbackMayaXformStack::UsdTransform3dFallbackMayaXformStack(
Expand Down Expand Up @@ -325,7 +371,7 @@ UsdTransform3dFallbackMayaXformStackHandler::transform3d(const Ufe::SceneItem::P
Ufe::Transform3d::Ptr UsdTransform3dFallbackMayaXformStackHandler::editTransform3d(
const Ufe::SceneItem::Ptr& item UFE_V2(, const Ufe::EditTransform3dHint& hint)) const
{
return createTransform3d(item);
return createEditTransform3d(item);
}

} // namespace ufe
Expand Down
146 changes: 104 additions & 42 deletions lib/mayaUsd/ufe/UsdTransform3dMatrixOp.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
#include "UsdTransform3dMatrixOp.h"

#include <mayaUsd/ufe/UsdSceneItem.h>
#include <mayaUsd/ufe/UsdTransform3dSetObjectMatrix.h>
#include <mayaUsd/ufe/Utils.h>
#include <mayaUsd/ufe/XformOpUtils.h>
#include <mayaUsd/undo/UsdUndoBlock.h>
Expand Down Expand Up @@ -45,6 +46,53 @@ VtValue getValue(const UsdAttribute& attr, const UsdTimeCode& time)

const char* getMatrixOp() { return std::getenv("MAYA_USD_MATRIX_XFORM_OP_NAME"); }

std::vector<UsdGeomXformOp>::const_iterator
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Factor out common code between the editTransform3d() and transform3d() interfaces.

findMatrixOp(const std::vector<UsdGeomXformOp>& xformOps)
{
auto opName = getMatrixOp();
return std::find_if(xformOps.cbegin(), xformOps.cend(), [opName](const UsdGeomXformOp& op) {
return (op.GetOpType() == UsdGeomXformOp::TypeTransform)
&& (!opName || std::string(opName) == op.GetOpName());
});
}

// Given a starting point i (inclusive), is there a non-matrix transform op in
// the vector?
bool findNonMatrix(
const std::vector<UsdGeomXformOp>::const_iterator& i,
const std::vector<UsdGeomXformOp>& xformOps)
{
return std::find_if(
i,
xformOps.cend(),
[](const UsdGeomXformOp& op) {
return op.GetOpType() != UsdGeomXformOp::TypeTransform;
})
!= xformOps.cend();
}

// Compute the inverse of the cumulative transform for the argument xform ops.
GfMatrix4d xformInv(
const std::vector<UsdGeomXformOp>::const_iterator& begin,
const std::vector<UsdGeomXformOp>::const_iterator& end,
const Ufe::Path& path)
{
auto nbOps = std::distance(begin, end);
if (nbOps == 0) {
return GfMatrix4d { 1 };
}
std::vector<UsdGeomXformOp> ops(nbOps);
ops.assign(begin, end);

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

return m.GetInverse();
}

// 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
Expand All @@ -69,7 +117,10 @@ class UsdSetMatrix4dUndoableCmd : public Ufe::SetMatrix4dUndoableCommand
{
UsdUndoBlock undoBlock(&_undoableItem);

auto t3d = Ufe::Transform3d::transform3d(sceneItem());
// Use editTransform3d() to set a single matrix transform op.
// transform3d() returns a whole-object interface, which may include
// other transform ops.
auto t3d = Ufe::Transform3d::editTransform3d(sceneItem());
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

editTransform3d() sets the matrix for a single matrix op, which is what we want.

t3d->setMatrix(_newM);
}

Expand Down Expand Up @@ -407,32 +458,48 @@ UsdTransform3dMatrixOpHandler::create(const Ufe::Transform3dHandler::Ptr& nextHa
Ufe::Transform3d::Ptr
UsdTransform3dMatrixOpHandler::transform3d(const Ufe::SceneItem::Ptr& item) const
{
// Remove code duplication with editTransform3d(). PPT, 21-Jan-2021.
// We must create a Transform3d interface to edit the whole object,
// e.g. setting the local transformation matrix for the complete object.
UsdSceneItem::Ptr usdItem = std::dynamic_pointer_cast<UsdSceneItem>(item);
TF_AXIOM(usdItem);

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;
// If there is a single matrix transform op in the transform stack, then
// transform3d() and editTransform3d() are equivalent: use that matrix op.
if (xformOps.size() == 1 && xformOps.front().GetOpType() == UsdGeomXformOp::TypeTransform) {
return UsdTransform3dMatrixOp::create(usdItem, xformOps.front());
}

// Find the matrix op to be transformed.
auto i = findMatrixOp(xformOps);

// If no matrix was found, pass on to the next handler.
if (i == xformOps.cend()) {
return _nextHandler->transform3d(item);
}

// If we've found a matrix op, but there is a more local non-matrix op in
// the stack, the more local op should be used. This will happen e.g. if a
// pivot edit was done on a matrix op stack. Since matrix ops don't
// support pivot edits, a fallback Maya stack will be added, and from that
// point on the fallback Maya stack must be used.
if (findNonMatrix(i, xformOps)) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Use factored-out code --- no change in behavior.

return _nextHandler->transform3d(item);
}

// At this point we know we have a matrix op to transform, and that it is
// not alone on the transform op stack. Wrap a matrix op Transform3d
// interface for that matrix into a UsdTransform3dSetObjectMatrix object.
// Ml is the transformation before the matrix op, Mr is the transformation
// after the matrix op.
auto mlInv = xformInv(xformOps.cbegin(), i, item->path());
auto mrInv = xformInv(i + 1, xformOps.cend(), item->path());

return (foundMatrix && !moreLocalNonMatrix) ? UsdTransform3dMatrixOp::create(usdItem, *i)
: _nextHandler->transform3d(item);
return UsdTransform3dSetObjectMatrix::create(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We want to return a transform3d() interface, to set the object's transformation, but we have a multi-matrix stack. Create the new UsdTransform3dSetObjectMatrix wrapper that does the matrix algebra to set the single matrix op we are targeting. The wrapper is given the matrices that represent the aggregate less-local (Ml) and more-local (Mr) transformations, to perform the matrix algebra.

UsdTransform3dMatrixOp::create(usdItem, *i), mlInv, mrInv);
}

Ufe::Transform3d::Ptr UsdTransform3dMatrixOpHandler::editTransform3d(
Expand All @@ -453,37 +520,32 @@ Ufe::Transform3d::Ptr UsdTransform3dMatrixOpHandler::editTransform3d(
// has not been specified, we edit the first matrix op in the stack. If
// the matrix op is not found, or there is no matrix op in the stack, let
// the next Transform3d handler in the chain handle the request.
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());

// If we've found a matrix op, but there is a more local non-matrix op in
// the stack, the more local op should be used to handle the edit.
bool moreLocalNonMatrix = foundMatrix
? (std::find_if(
i,
xformOps.end(),
[](const UsdGeomXformOp& op) {
return op.GetOpType() != UsdGeomXformOp::TypeTransform;
})
!= xformOps.end())
: false;
// Find the matrix op to be transformed.
auto i = findMatrixOp(xformOps);

// We can't handle pivot edits, so in that case pass on to the next handler.
return (foundMatrix && !moreLocalNonMatrix
// If no matrix was found, pass on to the next handler.
if (i == xformOps.cend()) {
return _nextHandler->editTransform3d(item UFE_V2(, hint));
}

// If we've found a matrix op, but there is a more local non-matrix op in
// the stack, the more local op should be used. This will happen e.g. if a
// pivot edit was done on a matrix op stack. Since matrix ops don't
// support pivot edits, a fallback Maya stack will be added, and from that
// point on the fallback Maya stack must be used. Also, pass pivot edits
// on to the next handler, since we can't handle them.
return (findNonMatrix(i, xformOps)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Use factored-out code, no change in behavior.

#ifdef UFE_V2_FEATURES_AVAILABLE
&& (hint.type() != Ufe::EditTransform3dHint::RotatePivot)
&& (hint.type() != Ufe::EditTransform3dHint::ScalePivot)
|| (hint.type() == Ufe::EditTransform3dHint::RotatePivot)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Awkward negative logic changed, but no change in behavior.

|| (hint.type() == Ufe::EditTransform3dHint::ScalePivot)
#endif
)
? UsdTransform3dMatrixOp::create(usdItem, *i)
: _nextHandler->editTransform3d(item UFE_V2(, hint));
? _nextHandler->editTransform3d(item UFE_V2(, hint))
: UsdTransform3dMatrixOp::create(usdItem, *i);
}

} // namespace ufe
Expand Down
15 changes: 12 additions & 3 deletions lib/mayaUsd/ufe/UsdTransform3dMayaXformStack.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,10 @@ class UsdSetMatrix4dUndoableCmd : public Ufe::SetMatrix4dUndoableCommand
{
UsdUndoBlock undoBlock(&_undoableItem);

auto t3d = Ufe::Transform3d::transform3d(sceneItem());
// transform3d() and editTransform3d() are equivalent for a normal Maya
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Objects where we wish to edit only a part of the transform stack (e.g. Maya fallback stack or a single matrix in a stack of multiple matrices) need to use editTransform3d(). For common API or normal Maya transform stack, editTransform3d() and transform3d() are equivalent, so simply always use editTransform3d() when we know we're editing.

// transform stack, but not for a fallback Maya transform stack, and
// both can be edited by this command.
auto t3d = Ufe::Transform3d::editTransform3d(sceneItem());
t3d->translate(_newT.x(), _newT.y(), _newT.z());
t3d->rotate(_newR.x(), _newR.y(), _newR.z());
t3d->scale(_newS.x(), _newS.y(), _newS.z());
Expand Down Expand Up @@ -466,6 +469,9 @@ Ufe::Vector3d UsdTransform3dMayaXformStack::rotation() const
}
UsdGeomXformOp r = getOp(NdxRotate);
TF_AXIOM(r);
if (!r.GetAttr().HasValue()) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Corner case found when writing automated tests: an added transform op has no value until one is set, so return a sensible default in those cases.

Copy link

Choose a reason for hiding this comment

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

This pattern is possibly repeated in every transform 3d implementation. Have you looked for it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point, I will.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Here are my findings:

  • UsdTransform3d.cpp: HasAttribute() checks in place.

  • UsdTransform3dBase.cpp: default implementations only, no attribute access.

  • UsdTransform3dCommonAPI.cpp: no direct attribute access, only accessing
    through UsdGeomXformCommonAPI object, which returns the identity matrix
    if no value has been authored, which is fine.

  • UsdTransform3dFallbackMayaXformStack.cpp: no value attribute access.

  • UsdTransform3dMatrixOp.cpp: getValue() utility is a problem, it ignores the
    UsdAttribute::Get() boolean return value, which may be false. Entered as MAYA-110061, assigned to me.

    Otherwise, uses UsdGeomXformOp::GetOpTransform(), which returns the
identity matrix if no value has been authored, which is fine.

  • UsdTransform3dMayaXformStack.cpp: fixed in this pull request.

  • UsdTransform3dSetObjectMatrix.cpp: no direct attribute access.

return Ufe::Vector3d(0, 0, 0);
}

CvtRotXYZFromAttrFn cvt = getCvtRotXYZFromAttrFn(r.GetOpName());
return cvt(getValue(r.GetAttr(), getTime(path())));
Expand All @@ -478,6 +484,9 @@ Ufe::Vector3d UsdTransform3dMayaXformStack::scale() const
}
UsdGeomXformOp s = getOp(NdxScale);
TF_AXIOM(s);
if (!s.GetAttr().HasValue()) {
return Ufe::Vector3d(1, 1, 1);
}

GfVec3f v;
s.Get(&v, getTime(path()));
Expand Down Expand Up @@ -621,9 +630,9 @@ Ufe::Vector3d UsdTransform3dMayaXformStack::scalePivotTranslation() const
template <class V>
Ufe::Vector3d UsdTransform3dMayaXformStack::getVector3d(const TfToken& attrName) const
{
// If the attribute doesn't exist yet, return a zero vector.
// If the attribute doesn't exist or have a value yet, return a zero vector.
auto attr = prim().GetAttribute(attrName);
if (!attr) {
if (!attr || !attr.HasValue()) {
return Ufe::Vector3d(0, 0, 0);
}

Expand Down
Loading