Support duplicate blendshape target names #1179
Conversation
…not get exported 2. blendshapes with the same names across multiple target meshes did not get exported
35907c8 to
8cd41ed
Compare
lib/mayaUsd/fileio/writeJobContext.h
Outdated
| /// This is cached at the writeJob-level because the state needs to persist across instances of | ||
| /// the meshWriter->Write() function. | ||
| MAYAUSD_CORE_PUBLIC | ||
| MPlugArray mBlendShapesAnimWeightPlugs; |
There was a problem hiding this comment.
It's rather surprising to see translator-specific data in this generic context. What do others think? Should we make it easier to attach any blind data to the context instead of hardcoding something specific each time a translator needs it?
There was a problem hiding this comment.
Yes; I foresee a lot of future needs for this where a translator will need to retain state between invocations or similar, especially for certain types of Maya nodes.
There was a problem hiding this comment.
@kxl-adsk Based on our discussion, I've gone ahead and made the change to move this into the meshWriter translator instead, along with updating the unit test as also discussed. Please let me know if there are other questions/concerns.
There was a problem hiding this comment.
Sounds good. I re-requested the review from @williamkrick and dismissed the stale review since it was no longer applicable based on what we discussed. I also started a new PF.
There was a problem hiding this comment.
@ysiewappl I am trying to build this PR on my Windows machine and getting C2071 error:
maya-usd/lib/mayaUsd/fileio/writeJobContext.h(163): error C2071: 'pxrInternal_v0_21__pxrReserved__::UsdMayaWriteJobContext::mBlendShapesAnimWeightPlugs': illegal storage class
Hi @HamedSabri-adsk: That's...odd. For one, the code is just: Which on w32 should just expand to |
|
@ysiewappl I was able to build the PR by removing the "MAYAUSD_CORE_PUBLIC" before your public member variable. I haven't had time to investigate why VS compiler is not happy. I also don't see anything that jumps up under "Selective Member Import/Export" criteria: https://docs.microsoft.com/en-us/cpp/cpp/using-dllimport-and-dllexport-in-cpp-classes?view=msvc-160 |
Ah, I think I know what the issue is; I haven't defined the specific symbol. Clang doesn't seem to treat this as an error, oddly enough; I think MSVC is making the right call on this one.
There is no reason why this member needs to be made an exportable symbol, so I've gone ahead and removed that macro prefix (as of 01d62a7). Previously I had just added in there in keeping with the surrounding code. |
lib/usd/translators/meshWriter.h
Outdated
| @@ -75,12 +74,12 @@ class PxrUsdTranslators_MeshWriter : public UsdMayaPrimWriter | |||
| bool writeBlendShapeAnimation(const UsdTimeCode& usdTime); | |||
| bool writeAnimatedMeshExtents(const MObject& deformedMesh, const UsdTimeCode& usdTime); | |||
|
|
|||
| /// Used to cache the animated blend shape weight plugs that need to be sampled per-frame. | |||
There was a problem hiding this comment.
We should add the explanation here of why this needs to be a static member.
There was a problem hiding this comment.
This request has been accommodated for in 2ab03a5.
- adding more documentation clang-format
Hi:
This PR fixes some bugs introduced with the newly-added blendshape export functionality, related to exporting multiple targets across multiple meshes.
The following are of significance in this PR:
Previously, exporting multiple targets across multiple meshes resulted in some targets not having their animation written out at all.
Exporting targets that share the same name across multiple meshes is now supported; we now namespace each target name (while leaving the prim name untouched to act as a "nice" name) to ensure that there is no risk of collision.
We also now write out the default weights for blendshape targets, if they exist. This (unfortunately) only applies when animation export is enabled for now.
Please review and raise any concerns.