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

MAYA-125897 support duplicate xform ops #2695

Merged
merged 3 commits into from
Nov 3, 2022
Merged

MAYA-125897 support duplicate xform ops #2695

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
71ad000
MAYA-125897 support duplicate xform ops
pierrebai-adsk Nov 1, 2022
05296dd
MAYA-125897 add the newly generated name to the list of existing ops.
pierrebai-adsk Nov 1, 2022
df50c1a
MAYA-125897 add a unit test
pierrebai-adsk Nov 2, 2022
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
30 changes: 30 additions & 0 deletions lib/mayaUsd/fileio/transformWriter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,6 +260,34 @@ bool UsdMayaTransformWriter::_GatherAnimChannel(
return false;
}

void UsdMayaTransformWriter::_MakeAnimChannelsUnique(const UsdGeomXformable& usdXformable)
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you move the uniqueName() function from lib\mayaUsd\ufe\Utils.h to lib\mayaUsd\utils\util.h and use that instead? You can call it as you go by keeping the existingOps set up to date after each rename.

This will make sure we have a unique way of making names unique in the app, and properly deal with names that already have a numeric suffix.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, I just checked uniqueName... unfortunately, the API is not quite compatible. uniqueName generates the "full name", but what we need to change is the piece used to generate the full name. (The suffix, to be precise.)

I will add the newly generated xform op to the existingOps set though as suggested, in case the newly added op collides with another one later on while iterating over all ops.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps a simple-minded question: why do we append a separate translation? Do we need to recover it separately? Why not combine it with the existing translation?

Copy link
Collaborator Author

@pierrebai-adsk pierrebai-adsk Nov 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Well, the translation comes from different pieces of the rig, as far as I could understand it. One came from a "foot" rig control, the other from a "foot-roll" rig-control. Given that they can control things is arbitrary ways, it seems hard to know how to merge them since there could be rotation, scale etc between two translations.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Those are all good points. I do wonder how this round-trips, if at all.

Copy link
Collaborator Author

@pierrebai-adsk pierrebai-adsk Nov 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Once the rig has been cached to USD, there is no longer a rig when edited-as-Maya, it is pure mesh/transform Maya nodes, so I expect they round-trip as part of our general support for generic xformop:order. If not, that code will need to be fixed, but we have no reason to think it should fail.

If we ever somehow support rig round-trip throughUSD (via maybe a future more complex USD skeleton or somesuch) then we will have to revisit the issue, but that is not even on the horizon.

using OpName = TfToken;
std::set<OpName> existingOps;
bool xformReset = false;
for (const UsdGeomXformOp& op : usdXformable.GetOrderedXformOps(&xformReset)) {
existingOps.emplace(op.GetOpName());
}

if (existingOps.size() == 0)
return;

for (_AnimChannel& channel : _animChannels) {
// We will put a upper limit on the number of similar transform operations
// that a prim can use. Having 1000 separate translations on a single prim
// seems both generous. Having more is highly improbable.
for (int suffixIndex = 1; suffixIndex < 1000; ++suffixIndex) {
TfToken channelOpName
= UsdGeomXformOp::GetOpName(channel.usdOpType, channel.opName, channel.isInverse);
if (existingOps.count(channelOpName) == 0)
break;
std::ostringstream oss;
oss << "channel" << suffixIndex;
channel.opName = TfToken(oss.str());
}
}
}

void UsdMayaTransformWriter::_PushTransformStack(
const MFnTransform& iTrans,
const UsdGeomXformable& usdXformable,
Expand Down Expand Up @@ -477,6 +505,8 @@ void UsdMayaTransformWriter::_PushTransformStack(
}
}

_MakeAnimChannelsUnique(usdXformable);

// Loop over anim channel vector and create corresponding XFormOps
// including the inverse ones if needed
TF_FOR_ALL(iter, _animChannels)
Expand Down
5 changes: 5 additions & 0 deletions lib/mayaUsd/fileio/transformWriter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,11 @@ class UsdMayaTransformWriter : public UsdMayaPrimWriter
const bool isWritingAnimation,
const bool setOpName);

// Change the channel suffix so that the USD XformOp becomes unique.
// This is to deal with complex rigs that can have multiple transforms
// affecting the same transform operation on the same UsdGeomXformable.
void _MakeAnimChannelsUnique(const UsdGeomXformable& usdXformable);

/// Populates the AnimChannel vector with various ops based on
/// the Maya transformation logic. If scale and/or rotate pivot are
/// declared, creates inverse ops in the appropriate order.
Expand Down