From b88a7faf642e102282b45021d91ea50d7ef18cc3 Mon Sep 17 00:00:00 2001 From: AMZN-stankowi <4838196+AMZN-stankowi@users.noreply.github.com> Date: Mon, 16 Aug 2021 10:18:45 -0700 Subject: [PATCH] Fixed issues with blend shape animations (#3080) Duplicate blend shape animations are now handled correctly. Invalid animation targets are now an error instead of a crash in the builder. Signed-off-by: stankowi <4838196+AMZN-stankowi@users.noreply.github.com> --- .../SDKWrapper/AssImpSceneWrapper.cpp | 3 + .../Importers/AssImpAnimationImporter.cpp | 32 ++++++- .../Importers/AssImpBlendShapeImporter.cpp | 87 ++++++++++++++++--- 3 files changed, 107 insertions(+), 15 deletions(-) diff --git a/Code/Tools/SceneAPI/SDKWrapper/AssImpSceneWrapper.cpp b/Code/Tools/SceneAPI/SDKWrapper/AssImpSceneWrapper.cpp index 0680b41eca..12aefca00c 100644 --- a/Code/Tools/SceneAPI/SDKWrapper/AssImpSceneWrapper.cpp +++ b/Code/Tools/SceneAPI/SDKWrapper/AssImpSceneWrapper.cpp @@ -70,6 +70,9 @@ namespace AZ // This results in the loss of the offset matrix data for nodes without a mesh which is required for the Transform Importer. m_importer.SetPropertyBool(AI_CONFIG_IMPORT_FBX_PRESERVE_PIVOTS, false); m_importer.SetPropertyBool(AI_CONFIG_IMPORT_FBX_OPTIMIZE_EMPTY_ANIMATION_CURVES, false); + // The remove empty bones flag is on by default, but doesn't do anything internal to AssImp right now. + // This is here as a bread crumb to save others times investigating issues with empty bones. + // m_importer.SetPropertyBool(AI_CONFIG_IMPORT_REMOVE_EMPTY_BONES, false); m_sceneFileName = fileName; m_assImpScene = m_importer.ReadFile(fileName, aiProcess_Triangulate //Triangulates all faces of all meshes diff --git a/Code/Tools/SceneAPI/SceneBuilder/Importers/AssImpAnimationImporter.cpp b/Code/Tools/SceneAPI/SceneBuilder/Importers/AssImpAnimationImporter.cpp index 150a50138e..97b5960a8d 100644 --- a/Code/Tools/SceneAPI/SceneBuilder/Importers/AssImpAnimationImporter.cpp +++ b/Code/Tools/SceneAPI/SceneBuilder/Importers/AssImpAnimationImporter.cpp @@ -147,7 +147,9 @@ namespace AZ SerializeContext* serializeContext = azrtti_cast(context); if (serializeContext) { - serializeContext->Class()->Version(5); // [LYN-4226] Invert PostRotation matrix in animation chains + // Revision 5: [LYN-4226] Invert PostRotation matrix in animation chains + // Revision 6: Handle duplicate blend shape animations + serializeContext->Class()->Version(6); } } @@ -631,6 +633,15 @@ namespace AZ for (const auto& [meshIdx, keys] : valueToKeyDataMap) { + + if (static_cast(meshIdx) >= mesh->mNumAnimMeshes) + { + AZ_Error( + "AnimationImporter", false, + "Mesh %s has an animation mesh index reference of %d, but only has %d animation meshes. Skipping importing this. This is an error in the source scene file that should be corrected.", + mesh->mName.C_Str(), meshIdx, mesh->mNumAnimMeshes); + continue; + } AZStd::shared_ptr morphAnimNode = AZStd::make_shared(); @@ -656,12 +667,29 @@ namespace AZ morphAnimNode->AddKeyFrame(weight); } + // Some DCC tools, like Maya, include a full path separated by '.' in the node names. + // For example, "cone_skin_blendShapeNode.cone_squash" + // Downstream processing doesn't want anything but the last part of that node name, + // so find the last '.' and remove anything before it. const size_t dotIndex = nodeName.find_last_of('.'); nodeName = nodeName.substr(dotIndex + 1); morphAnimNode->SetBlendShapeName(nodeName.data()); - AZStd::string animNodeName(AZStd::string::format("%s_%s", s_animationNodeName, nodeName.data())); + // Duplicates can exist if an anim mesh had a name with a suffix like .001, in that case + // AssImp will strip off that suffix. Note that this behavior is separate from the + // scan for a period in the node name that came before this. + AZStd::string originalNodeName(AZStd::string::format("%s_%s", s_animationNodeName, nodeName.data())); + AZStd::string animNodeName(originalNodeName); + if (RenamedNodesMap::SanitizeNodeName( + animNodeName, context.m_scene.GetGraph(), context.m_currentGraphPosition, originalNodeName.c_str())) + { + AZ_Warning( + "AnimationImporter", false, + "Duplicate animations were found with the name %s on mesh %s. The duplicate will be named %s.", + originalNodeName.c_str(), mesh->mName.C_Str(), animNodeName.c_str()); + } + Containers::SceneGraph::NodeIndex addNode = context.m_scene.GetGraph().AddChild( context.m_currentGraphPosition, animNodeName.c_str(), AZStd::move(morphAnimNode)); context.m_scene.GetGraph().MakeEndPoint(addNode); diff --git a/Code/Tools/SceneAPI/SceneBuilder/Importers/AssImpBlendShapeImporter.cpp b/Code/Tools/SceneAPI/SceneBuilder/Importers/AssImpBlendShapeImporter.cpp index e8eb5ecf68..8e447f9d79 100644 --- a/Code/Tools/SceneAPI/SceneBuilder/Importers/AssImpBlendShapeImporter.cpp +++ b/Code/Tools/SceneAPI/SceneBuilder/Importers/AssImpBlendShapeImporter.cpp @@ -40,7 +40,9 @@ namespace AZ SerializeContext* serializeContext = azrtti_cast(context); if (serializeContext) { - serializeContext->Class()->Version(3); // LYN-2576 + // Revision 3: Fixed an issue where jack.fbx was failing to process + // Revision 4: Handle duplicate blend shape animations + serializeContext->Class()->Version(4); } } @@ -80,7 +82,32 @@ namespace AZ // AssImp separates meshes that have multiple materials. // This code re-combines them to match previous FBX SDK behavior, // so they can be separated by engine code instead. - AZStd::map>> animToMeshToAnimMeshIndices; + // Can't de-dupe nodes in the first loop because we can't generate names until we create nodes later. + // Because meshes are split on material at this point and need to be recombined, we can be in a position where + // There is a legit duped anim mesh that needs to be combined based on the outer non-anim mesh, + // or this is a duplicately named anim mesh that needs to be de-duped. There is also the case where both are true, + // it's a duplicate name and the non-anim mesh has to be deduped. + + // Helper struct to track an anim mesh and its associated mesh. + struct AnimMeshAndSceneMeshIndex + { + AnimMeshAndSceneMeshIndex(const aiAnimMesh* aiAnimMesh, const aiMesh* aiMesh) + : m_aiAnimMesh(aiAnimMesh) + , m_aiMesh(aiMesh) + { + } + const aiAnimMesh* m_aiAnimMesh = nullptr; + const aiMesh* m_aiMesh = nullptr; + }; + + // Helper struct to track all anim meshes at an index for all scene meshes. + struct AnimMeshAndSceneMeshes + { + AZStd::vector m_animMeshAndSceneMeshIndex; + }; + + // Map the animation index to the list of anim meshes at that index, and the mesh associated with those anim meshes. + AZStd::map animMeshIndexToSceneMeshes; for (int nodeMeshIdx = 0; nodeMeshIdx < numMesh; nodeMeshIdx++) { int sceneMeshIdx = context.m_sourceNode.GetAssImpNode()->mMeshes[nodeMeshIdx]; @@ -88,20 +115,57 @@ namespace AZ for (unsigned int animIdx = 0; animIdx < aiMesh->mNumAnimMeshes; animIdx++) { aiAnimMesh* aiAnimMesh = aiMesh->mAnimMeshes[animIdx]; - animToMeshToAnimMeshIndices[aiAnimMesh->mName.C_Str()].emplace_back(nodeMeshIdx, animIdx); + + // This code executes if: + // A mesh in the FBX file had multiple materials and blend shapes. + // This means that AssImp splits that mesh to one material per mesh. + // AssImp creates a set of anim meshes for each mesh based on that split. + // This verifies that those anim mesh arrays are in the same order across all split meshes, if it fails + // it means this logic needs to be updated, but it also catches that here earlier in an obvious way, + // instead of failing later in a harder to track way. + if (animMeshIndexToSceneMeshes.contains(animIdx)) + { + const AnimMeshAndSceneMeshIndex& firstExistingAnim( + animMeshIndexToSceneMeshes[animIdx].m_animMeshAndSceneMeshIndex[0]); + if (strcmp( + firstExistingAnim.m_aiAnimMesh->mName.C_Str(), + aiAnimMesh->mName.C_Str()) != 0) + { + AZ_Error( + Utilities::ErrorWindow, false, + "Meshes %s and %s on node %s have mismatched animations %s and %s at index %d. This can be resolved by " + "either manually separating meshes by material in the source scene file, or by updating this logic to " + "handle out of order animation indices.", + firstExistingAnim.m_aiMesh->mName.C_Str(), + aiMesh->mName.C_Str(), + context.m_sourceNode.GetName(), + firstExistingAnim.m_aiAnimMesh->mName.C_Str(), + aiAnimMesh->mName.C_Str(), animIdx); + return Events::ProcessingResult::Failure; + } + } + + animMeshIndexToSceneMeshes[animIdx].m_animMeshAndSceneMeshIndex.emplace_back( + AnimMeshAndSceneMeshIndex(aiAnimMesh, aiMesh)); } } - for (const auto& animToMeshIndex : animToMeshToAnimMeshIndices) + for (const auto& animMeshToSceneMeshes : animMeshIndexToSceneMeshes) { AZStd::shared_ptr blendShapeData = AZStd::make_shared(); + if (animMeshToSceneMeshes.second.m_animMeshAndSceneMeshIndex.size() == 0) + { + AZ_Error(Utilities::ErrorWindow, false, "Blend shape animations were expected but missing on node %s.", + context.m_sourceNode.GetName()); + return Events::ProcessingResult::Failure; + } // Some DCC tools, like Maya, include a full path separated by '.' in the node names. // For example, "cone_skin_blendShapeNode.cone_squash" // Downstream processing doesn't want anything but the last part of that node name, // so find the last '.' and remove anything before it. - AZStd::string nodeName(animToMeshIndex.first); + AZStd::string nodeName(animMeshToSceneMeshes.second.m_animMeshAndSceneMeshIndex[0].m_aiAnimMesh->mName.C_Str()); size_t dotIndex = nodeName.rfind('.'); if (dotIndex != AZStd::string::npos) { @@ -109,12 +173,11 @@ namespace AZ } int vertexOffset = 0; RenamedNodesMap::SanitizeNodeName(nodeName, context.m_scene.GetGraph(), context.m_currentGraphPosition, "BlendShape"); - AZ_TraceContext("Blend shape name", nodeName); - for (const auto& meshIndex : animToMeshIndex.second) + + for (const auto& animMeshAndSceneIndex : animMeshToSceneMeshes.second.m_animMeshAndSceneMeshIndex) { - int sceneMeshIdx = context.m_sourceNode.GetAssImpNode()->mMeshes[meshIndex.first]; - const aiMesh* aiMesh = context.m_sourceScene.GetAssImpScene()->mMeshes[sceneMeshIdx]; - const aiAnimMesh* aiAnimMesh = aiMesh->mAnimMeshes[meshIndex.second]; + const aiAnimMesh* aiAnimMesh = animMeshAndSceneIndex.m_aiAnimMesh; + const aiMesh* aiMesh = animMeshAndSceneIndex.m_aiMesh; AZStd::bitset uvSetUsedFlags; for (AZ::u8 uvSetIndex = 0; uvSetIndex < SceneData::GraphData::BlendShapeData::MaxNumUVSets; ++uvSetIndex) @@ -199,6 +262,7 @@ namespace AZ face.mNumIndices); continue; } + for (unsigned int idx = 0; idx < face.mNumIndices; ++idx) { blendFace.vertexIndex[idx] = face.mIndices[idx] + vertexOffset; @@ -207,11 +271,8 @@ namespace AZ blendShapeData->AddFace(blendFace); } vertexOffset += aiMesh->mNumVertices; - - } - // Report problem if no vertex or face converted to MeshData if (blendShapeData->GetVertexCount() <= 0 || blendShapeData->GetFaceCount() <= 0) {