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

Fixes related to export of skeleton rest-xforms #1130

Merged
merged 5 commits into from
Feb 2, 2021
Merged

Fixes related to export of skeleton rest-xforms #1130

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a83340a
joint translator: fix for warning about joints missing from bind pose
Jan 27, 2021
f8a0c36
joint translator: use logical indices as link between dagPose xformMa…
Sep 12, 2020
fb76e95
author restXforms from bindXforms if dagPose can't be used
Sep 12, 2020
ad03c38
add a debug check to verify logicalIndex exists
Jan 30, 2021
afaea55
address code review notes from kxl-adsk
Feb 1, 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
27 changes: 12 additions & 15 deletions lib/usd/translators/jointWriter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,6 @@ static GfMatrix4d _GetJointWorldBindTransform(const MDagPath& dagPath)
CHECK_MSTATUS_AND_RETURN(status, GfMatrix4d(1));
MPlug plgWorldMatrices = fnNode.findPlug("worldMatrix", false, &status);
CHECK_MSTATUS_AND_RETURN(status, GfMatrix4d(1));
TF_VERIFY(membersIdx < plgWorldMatrices.numElements());
Copy link

Choose a reason for hiding this comment

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

Can you comment on why this check wasn't good?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's because membersIdx is a logical index, while numElements() gives the number of physical plugs... so the two aren't related.

MPlug plgWorldMatrix = plgWorldMatrices.elementByLogicalIndex(membersIdx);
MObject plgWorldMatrixData = plgWorldMatrix.asMObject();
MFnMatrixData fnMatrixData(plgWorldMatrixData, &status);
Expand Down Expand Up @@ -222,16 +221,16 @@ static bool _FindDagPoseMembers(
indices->resize(numDagPaths);

std::vector<uint8_t> visitedIndices(numDagPaths, 0);
for (unsigned int i = 0; i < membersPlug.numElements(); ++i) {
for (unsigned int i = 0; i < membersPlug.numConnectedElements(); ++i) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't mind so much because I think this makes sense (this connection needs to exist anyway to be meaningful so the change will work), but what catches my eye is the reasoning; your comment seems suspicious that somehow connectedTo created a new element. Might be worth trying using evaluateNumElements to see if this is somehow an evaluation caching issue and if so worth reporting as a separate Maya bug.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, the thing where it was somehow creating an element was odd. Unfortunately, I didn't have time to fully investigate at the time, and I don't know if I'd be able to replicate now. I guess I also didn't feel too motivated too, since I feel like using .numConnectedElements() is conceptually better anyway.
If you want, I can edit the commit message, and remove the bit about the crash / element being created, or throw a bunch more caveats around it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not that big of a deal; if it's not something that you cannot reliably reproduce already, this code is fine as is (IMHO).

Copy link
Contributor Author

@pmolodo pmolodo Jan 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

To be clear - does the thumbs up mean you're ok with this as is? Or that you'd like me to edit the commit message?
Oops - guess you wrote a message that I didn't see while I was typing this. Will leave the commit as is.


MPlug memberPlug = membersPlug[i];
MPlug memberPlug = membersPlug.connectionByPhysicalIndex(i);
memberPlug.connectedTo(inputs, /*asDst*/ true, /*asSrc*/ false);

for (unsigned int j = 0; j < inputs.length(); ++j) {
MObjectHandle connNode(inputs[j].node());
auto it = pathIndexMap.find(connNode);
if (it != pathIndexMap.end()) {
(*indices)[it->second] = i;
(*indices)[it->second] = memberPlug.logicalIndex();
visitedIndices[it->second] = 1;
}
}
Expand All @@ -253,25 +252,23 @@ static bool _FindDagPoseMembers(

bool _GetLocalTransformForDagPoseMember(
const MFnDependencyNode& dagPoseDep,
unsigned int index,
unsigned int logicalIndex,
GfMatrix4d* xform)
{
MStatus status;

MPlug xformMatrixPlug = dagPoseDep.findPlug("xformMatrix");
if (index < xformMatrixPlug.numElements()) {
MPlug xformPlug = xformMatrixPlug[index];
MPlug xformPlug = xformMatrixPlug.elementByLogicalIndex(logicalIndex, &status);
Copy link
Contributor

@ysiewappl ysiewappl Jan 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Right now I know there's only one call site and the only indices stuffed in here should be the logical ones, but just in case, might be worth having debug assert to check if the logical index actually exists already via getExistingArrayAttributeIndices or similar.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My understanding is that if somehow that logical index doesn't exist yet, it will be created... which, as a fallback, seems ok to me.
Though I guess it might make sense to just put in a debug-only check / warning message, since it WOULD be an odd/unexpected occurence, which might warrent further investigation...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, added a check...

CHECK_MSTATUS_AND_RETURN(status, false);

MObject plugObj = xformPlug.asMObject(MDGContext::fsNormal, &status);
CHECK_MSTATUS_AND_RETURN(status, false);
MObject plugObj = xformPlug.asMObject(MDGContext::fsNormal, &status);
CHECK_MSTATUS_AND_RETURN(status, false);

MFnMatrixData plugMatrixData(plugObj, &status);
CHECK_MSTATUS_AND_RETURN(status, false);
MFnMatrixData plugMatrixData(plugObj, &status);
CHECK_MSTATUS_AND_RETURN(status, false);

*xform = GfMatrix4d(plugMatrixData.matrix().matrix);
return true;
}
return false;
*xform = GfMatrix4d(plugMatrixData.matrix().matrix);
return true;
}

/// Get local-space bind transforms to use as rest transforms.
Expand Down
74 changes: 74 additions & 0 deletions test/lib/usd/translators/UsdExportSkeletonTest/UsdExportSkeletonBindPoseSparseIndices.ma
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
//Maya ASCII 2020 scene
//Name: UsdExportSkeletonBindPoseSparseIndicesTest.ma
//Last modified: Wed, Jan 27, 2021 02:16:36 PM
//Codeset: UTF-8
requires maya "2020";
requires "stereoCamera" "10.0";
requires "stereoCamera" "10.0";
currentUnit -l centimeter -a degree -t film;
fileInfo "application" "maya";
fileInfo "product" "Maya 2020";
fileInfo "version" "2020";
fileInfo "cutIdentifier" "202011110415-b1e20b88e2";
fileInfo "osv" "Linux 3.10.0-1062.18.1.el7.x86_64 #1 SMP Tue Mar 17 23:49:17 UTC 2020 x86_64";
fileInfo "UUID" "CA508C80-0000-5159-6011-E644000002A4";
createNode transform -n "joint_grp";
rename -uid "716A8C80-0000-676C-6011-C976000006DF";
createNode joint -n "joint1" -p "joint_grp";
rename -uid "716A8C80-0000-676C-6011-C839000006BD";
setAttr ".t" -type "double3" -0.13387224879650539 0.21395404220941927 0 ;
setAttr ".mnrl" -type "double3" -360 -360 -360 ;
setAttr ".mxrl" -type "double3" 360 360 360 ;
setAttr ".jo" -type "double3" 0 0 2.5352931362020645 ;
setAttr ".radi" 0.66868990097983616;
createNode joint -n "joint2" -p "joint1";
rename -uid "716A8C80-0000-676C-6011-C83A000006BE";
setAttr ".t" -type "double3" 4.2613380856101655 8.8262730457699945e-15 0 ;
setAttr ".mnrl" -type "double3" -360 -360 -360 ;
setAttr ".mxrl" -type "double3" 360 360 360 ;
setAttr ".jo" -type "double3" 0 0 -50.479340742609303 ;
setAttr ".radi" 0.60456769613120953;
createNode joint -n "joint3" -p "joint2";
rename -uid "716A8C80-0000-676C-6011-C83A000006BF";
setAttr ".t" -type "double3" 3.0216421252033836 6.6613381477509392e-16 0 ;
setAttr ".mnrl" -type "double3" -360 -360 -360 ;
setAttr ".mxrl" -type "double3" 360 360 360 ;
setAttr ".jo" -type "double3" 0 0 47.94404760640726 ;
setAttr ".radi" 0.60456769613120953;
createNode dagPose -n "dagPose1";
rename -uid "075DCC80-0000-2CD0-6011-F36F000002EA";
setAttr -s 3 ".wm";
setAttr ".wm[120]" -type "matrix" 1 0 0 0 0 1 0 0
0 0 1 0 0 0 2 1;
setAttr ".wm[480]" -type "matrix" 1 0 0 0 0 1 0 0
0 0 1 0 0 3 2 1;
setAttr ".wm[801]" -type "matrix" 1 0 0 0 0 1 0 0
0 0 1 0 5 3 2 1;
setAttr -s 4 ".xm";
setAttr ".xm[0]" -type "matrix" "xform" 1 1 1 0 0 0 0 777
888 999 0 0 0 0 0 0 0 0 0 0
0 0 0 0 0 0 0 0 1 0 0 0 1 1
1 1 yes;
setAttr ".xm[120]" -type "matrix" "xform" 1 1 1 0 0 0 0 0
0 2 0 0 0 0 0 0 0 0 0 0
0 0 0 0 0 0 0 0 1 0 0 0 1 1
1 1 yes;
setAttr ".xm[480]" -type "matrix" "xform" 1 1 1 0 0 0 0 0
3 0 0 0 0 0 0 0 0 0 0 0
0 0 0 0 0 0 0 0 1 0 0 0 1 1
1 1 yes;
setAttr ".xm[801]" -type "matrix" "xform" 1 1 1 0 0 0 0 5
0 0 0 0 0 0 0 0 0 0 0 0
0 0 0 0 0 0 0 0 1 0 0 0 1 1
1 1 yes;
setAttr -s 3 ".m";
setAttr -s 3 ".p";
connectAttr "joint1.s" "joint2.is";
connectAttr "joint2.s" "joint3.is";
connectAttr "joint1.msg" "dagPose1.m[120]";
connectAttr "joint2.msg" "dagPose1.m[480]";
connectAttr "joint3.msg" "dagPose1.m[801]";
connectAttr "dagPose1.w" "dagPose1.p[120]";
connectAttr "dagPose1.m[120]" "dagPose1.p[480]";
connectAttr "dagPose1.m[480]" "dagPose1.p[801]";
// End of UsdExportSkeletonBindPoseSparseIndicesTest.ma
25 changes: 25 additions & 0 deletions test/lib/usd/translators/testUsdExportSkeleton.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,6 +264,31 @@ def testSkelMissingJointFromDagPose(self):
self.assertIn("Node 'joint4' is not a member of dagPose 'dagPose1'",
missingJointWarnings[0])

def testSkelBindPoseSparseIndices(self):
"""
Check that if a dagPose has sparse indices on some of it's attributes,
with differing number of created indices, that things still work.
"""
mayaFile = os.path.join(self.inputPath, "UsdExportSkeletonTest", "UsdExportSkeletonBindPoseSparseIndices.ma")
cmds.file(mayaFile, force=True, open=True)

usdFile = os.path.abspath('UsdExportBindPoseSparseIndicesTest.usda')

cmds.select('joint_grp')
cmds.mayaUSDExport(mergeTransformAndShape=True, file=usdFile, shadingMode='none',
exportSkels='auto', selection=True)

stage = Usd.Stage.Open(usdFile)

skeleton = UsdSkel.Skeleton.Get(stage, '/joint_grp/joint1')
self.assertEqual(skeleton.GetRestTransformsAttr().Get(),
Vt.Matrix4dArray(
# If we're not correlating using logical indices correctly, we may get this
# matrix in here somehwere (which we shouldn't):
# Gf.Matrix4d( (1, 0, 0, 0), (0, 1, 0, 0), (0, 0, 1, 0), (777, 888, 999, 1) ),
[Gf.Matrix4d( (1, 0, 0, 0), (0, 1, 0, 0), (0, 0, 1, 0), (0, 0, 2, 1) ),
Gf.Matrix4d( (1, 0, 0, 0), (0, 1, 0, 0), (0, 0, 1, 0), (0, 3, 0, 1) ),
Gf.Matrix4d( (1, 0, 0, 0), (0, 1, 0, 0), (0, 0, 1, 0), (5, 0, 0, 1) )]))

if __name__ == '__main__':
unittest.main(verbosity=2)