Added Blendshape export functionality #1016
Conversation
There was a problem hiding this comment.
Just a few cosmetic notes from me, but otherwise looks good.
I noticed that there are still a fair number of TF_VERIFYs that execution will plow right through where maybe we'd want to check its return value and return/continue/etc if the check fails.
I'll defer again to @williamkrick to approve though, since he's far more knowledgeable on the details of this than me.
Thanks @ysiewappl!
| #include <unordered_map> | ||
| #include <vector> | ||
|
|
||
| #define MAYA_BLENDSHAPE_EVAL_HOTFIX 1 |
There was a problem hiding this comment.
constexpr int or bool here?
I see this gets used to compile out the call to mayaBlendShapeTriggerAllTargets() below, but if we set this to 0/false, I would guess we'd see complaints from the compiler about the function being unused. Any reason not to make that just a regular if()?
There was a problem hiding this comment.
I just realized that this isn't my intention; it's meant to actually make sure the code isn't included at all in the final plugin if the #define is on. I've gone ahead and enclosed the function under that #define as well.
| unsigned int numInTgtGrps = plgInTgtGrps.numElements(); | ||
| if (numInTgtGrps == 0) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
I think what you have written here will work, but please test a scene were you have a weight value for a non-existent target group. Maya can handle that case (by ignoring the entry which I think you are successfully doing here) so there could be scenes out there with more weights then targets.
| // TODO: (yliangsiew) For multiple blend shape deformers, what do we do? | ||
| // Do we collapse the shapes from multiple blend targets together, or just | ||
| // write out only the "closest" blendshape deformer's targets? Or just write all | ||
| // of them and print this warning to end-users? |
There was a problem hiding this comment.
I think eventually we'll want a 1 to 1 mapping between deformer nodes in Maya and UsdSkel compute items written to USD. Doing a special case to merge two blend shapes in a row won't help when there's a wrap deformer between them. We'll eventually have to support writing a graph of UsdSkel compute items to the USD file.
I think this change, even though it has a bunch of limitations, is a good step towards that final goal.
| // NOTE: (yliangsiew) Because UsdSkelBlendShape does not | ||
| // support animated targets (the `normalOffsets` and | ||
| // `offsets` attributes are defined as uniforms), we cannot | ||
| // fully support it in the exporter either. |
There was a problem hiding this comment.
Ah that is an interesting limitation. We'd need UsdSkelBlendShape targets to accept the same data type that the input & output geometry are to have a more general solution where we can connect a graph of computes together.
|
@ysiewappl there is some discussion we need to have before this change is merged. It was my understanding from in December that after the original commit for blend shape support you would be continuing on with more work. Is all the work you have planned now included in this pull request or is more work coming? If you are planning to do more work then I am fine with the minor style comments I've made here being fixed in the next change. If this is the end of the work then I'd like to see those loose ends tied off before this is merged. After the changes get merged our MayaUSD QAs will test this functionality and be logging bugs for things that don't work. I need you to provide a list of what is expected to work and what is not expected to work for our QAs to validate. Clearly everything in Maya is not supported, so we need to clearly state the limitations of the blend shape export so we know what to expect, and so that other users of MayaUSD know what to expect. Something like what are the supported deformation setups (blendShape->skinCluster->mesh ?) would be perfect. |
|
@williamkrick just as a heads up, Yi Liang is out for a little bit longer so I'll wait for him to be back to respond accurately. I believe all of our intended changes are represented in this PR. |
…BlendShape does not support animated targets
…cking of supported deformer types when exporting blendshapes
…Also now account for tweaks happening on the mesh node itself
…them, replaced asserts with TF_VERIFY instead, addressed null plug logic check, general housekeeping
…le to allow for more efficient reading of the target data in general w/o. Also increased verbosity of warning/errors
return MString() for error clang-format
|
@ysiewappl yes a test scene with examples would be fantastic. Can provide a brief text overview of what is expected to work vs. not work as well? You don't have to include everything but just give a high level idea of what to expect. After that I approve of this PR being merged. |
I've updated the unit test for the blendshapes a tiny bit, and uploaded one of
|
… combinations of meshes with and without blendshapes at the same time. made error messages more verbose
There was a problem hiding this comment.
@ysiewappl I tested your build against older versions of USD (the minimum version we support is 19.11) and I'm getting build failure. We don't have it yet covered by PF, so this is why you didn't see it in reports. Here is what I did locally to pass the build error (and it's what will have to be done before we can merge the change)
index 0e6b2081d..9da727ff7 100644
--- a/lib/usd/translators/meshWriter_BlendShapes.cpp
+++ b/lib/usd/translators/meshWriter_BlendShapes.cpp
@@ -397,7 +397,7 @@ MStatus mayaGetBlendShapeInfosForMesh(
plgInComponentsTgt, indices);
CHECK_MSTATUS_AND_RETURN_IT(stat);
for (unsigned int m = 0; m < indices.length(); ++m) {
- meshTargetDatum.indices.emplace_back(indices[m]);
+ meshTargetDatum.indices.push_back(indices[m]);
}
unsigned int numComponentIndices = meshTargetDatum.indices.size();
@@ -446,7 +446,7 @@ MStatus mayaGetBlendShapeInfosForMesh(
MPointArray ptDeltas = fnPtArrayData.array();
for (unsigned int m = 0; m < numComponentIndices; ++m) {
MPoint pt = ptDeltas[m];
- meshTargetDatum.ptOffsets.emplace_back(GfVec3f(pt.x, pt.y, pt.z));
+ meshTargetDatum.ptOffsets.push_back(GfVec3f(pt.x, pt.y, pt.z));
}
}
weightInfo.targets.emplace_back(meshTargetDatum);
In addition, I'm seeing the new test (testUsdExportBlendshapes) failing. I will re-trigger the PF once you fix the build error reported above to confirm.
…+17 support for older usd 19.11 version
|
There's several points where I call |
…bust to fix issue where exporting sparse frame ranges writes meshes with wrong offsets
|
@kxl-adsk I'm not sure why the Windows builds failed, but I made a commit based on a guess (regarding the filename passed to |
|
FWIW. You didn't have to replace all emplac_back. Just the two I mentioned in my comment was enough. |
|
I'm going to merge this change and pass it to our QA. I will share the files and short description you provided. If you can think of anything else to mention when testing, please don't hesitate to let me know. Finally, thank you for your hard work on this one. |
|
Awesome! Thank you everyone for your feedback on this, and @ysiewappl for putting this all together. |
This repairs some minor damage from the merge of PR Autodesk#1016 where the new testUsdExportBlendshapes test replaced testUsdExportCamera rather than just being added.
Hi all:
This is a continuation of the PR originally submitted at #952. Please refer to that one for more comments and previous discussion. This update fixes both "baked" blendshape targets and also an issue that sees blendshape offsets being doubled-up if the first frame being exported has any of the blendshape targets activated as well.
This PR adds support for exporting blendshapes out of Maya in line with the UsdSkelBlendShape schema for a variety of scenarios, including single base - single target, single base - multiple targets (incl. inbetween), multiple base - multiple targets, "baked" blendshape targets (i.e. no explicit target meshes in the scene), writing out combined extents, and writing out animation. Some limitations include:
Currently no support for "chained" blendshape deformations or other intermediate deformers in the deform stack.
"Layered" blendshape deformers will end up having individual shapes baked out individually.
Combined extents only take blendshapes into account and no other deformers.
I ran into an issue while implementing this, which can be summarized by the following:
Maya does not evaluate certain plugs on a blendshape node until evaluation of the node is forced. This is a problem because certain information like the indices of components on a blendshape cannot be determined unless the blendshape node itself is evaluated.
Steps to Reproduce:
Create a cube and a blendshape for it with one vertex moved.
Run the following code:
You should have gotten an error when attempting to attach the function set. Now twiddle the weight on the blendshape deformer and twiddle it back and then run the same code again.
The code suddenly works without any other modifications to the scene or its data.
We're currently working around this by basically twiddling the blendshape node's weights through OpenMaya prior to export, which is not ideal since that could potentially be expensive for denser meshes. Hopefully someone from ADSK could help advise here on any possible remedies.
Please review and raise any concerns.