-
Notifications
You must be signed in to change notification settings - Fork 202
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
Fixes related to export of skeleton rest-xforms #1130
Conversation
…trix and members ...as the physicalIndex for the two attrs may not line up. Also, use members.numConnectedElements() instead of .numElements(), as using numElements() would sometimes cause a plug to get created when memberPlug.connectedTo() was subsequently called, which for some reason triggered a crash.
@dgovil recently touched some of this code, if he wants to take a look over... |
Also CC'ing @ysiewappl since he touched it too. (we treat my personal fork as a submission staging area) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this makes sense to fallback on the bind transforms, and might solve a problem we're seeing here as well on our end.
@@ -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) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
GfMatrix4d* xform) | ||
{ | ||
MStatus status; | ||
|
||
MPlug xformMatrixPlug = dagPoseDep.findPlug("xformMatrix"); | ||
if (index < xformMatrixPlug.numElements()) { | ||
MPlug xformPlug = xformMatrixPlug[index]; | ||
MPlug xformPlug = xformMatrixPlug.elementByLogicalIndex(logicalIndex, &status); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, added a check...
addresses code review note: Autodesk#1130 (comment)
@kxl-adsk - are PF builds a little wonky again? I checked one of the failed builds, and the tests all passed - just some sort of permission error running a groovy script afterwards? |
Yes, I got the same and reported it internally. Stay tuned. |
@@ -136,7 +137,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()); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
"The Skeleton's 'restTransforms' property will not be " | ||
"authored.", | ||
skelPath.GetText()); | ||
TF_DEBUG(PXRUSDMAYA_TRANSLATORS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interested to know why we want to switch this warning to a debug trace? (and same for the one below)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because we now have an additional fallback - we set the restTransforms
from the bindPose - whereas before this would mean that we would "give up", and not author any restTransforms
.
I don't mind changing either this or the next one back to TF_WARN
if you want though.
Actually - on looking again, I think I'll definitely change the next one back to TF_WARN
, because the inability to read the local transform shouldn't happen. (Whereas with this one, it merely indicates that there was no dagPose... which, while not standard, is still kosher.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed the second to TF_WARN
, left this one as TF_DEBUG
for now - let me know if you want this one back to TF_WARN
as well:
afaea55
@@ -205,6 +207,49 @@ def testSkelWithoutBindPose(self): | |||
animSource.GetTranslationsAttr().Get(5.0), | |||
Vt.Vec3fArray([Gf.Vec3f(5.0, 5.0, 0.0)])) | |||
|
|||
def testSkelRestXformsWithNoDagPose(self): | |||
""" | |||
Tests export of of rest xforms when there is no dagPose node at all. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: of of
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in afaea55
- Switch TF_DEBUG back to TF_WARN if unable to read local transform from dagPose Autodesk#1130 (comment) - "of of" typo Autodesk#1130 (comment)
Some bugfixes for edge cases, and made it so that we export restXforms whenever we export bindXforms (ie, the restXforms are calculated from the bindXforms if present).
We were getting some errors in other tools if rest xforms weren't present (as well as an annoying warning/error from usdview, IIRC).