TrenchBroom crashes when loading a model with animation

[HaileyEira]

System Information

TrenchBroom Win64 v2024.1 on Windows 11

Expected Behavior

Load entity and add into .map like in the previous version of TrenchBroom.

Steps to Reproduce

Include the model in the .fgd, start TrenchBroom, navigate to the Entity tab or open a .map file that references the model, and TrenchBroom will crash.

The issue appears to be related to loading bones for animation. TrenchBroom crashes in AssimpParser.cpp with the error "vector subscript out of range," https://github.com/TrenchBroom/TrenchBroom/blob/master/common/src/IO/AssimpParser.cpp#L578

The model worked in the previous version of TrenchBroom. Its format is correct, and it loads correctly in other applications.

Crash Info

Crash files are attached.

TrenchBroom_SleepwalkR_Crash.zip

[kduske]

@LogicAndTrick can you take a look at this?

[LogicAndTrick]

Looks like it could be an upstream issue, Assimp's built-in viewer also cannot load this model:

Work around would be to use a model without animations, such as this version which doesn't crash: https://github.com/eGax/TrenchBroom_xtras_plus/blob/main/games_wip/MarbleBlast/assets/models/hazards/trapdoor.fbx

However this particular crash can be prevented by checking the range when loading the animation:

diff --git a/common/src/IO/AssimpParser.cpp b/common/src/IO/AssimpParser.cpp
index 642519e67..96287989f 100644
--- a/common/src/IO/AssimpParser.cpp
+++ b/common/src/IO/AssimpParser.cpp
@@ -575,7 +575,10 @@ std::vector<Assets::EntityModelVertex> computeMeshVertices(
     {
       for (unsigned int j = 0; j < bone.mNumWeights; ++j)
       {
-        weightsPerVertex[bone.mWeights[j].mVertexId].push_back(
+        const auto vertexIndex = bone.mWeights[j].mVertexId;
+        if (vertexIndex < 0 || vertexIndex >= mesh.mNumVertices)
+          continue;
+        weightsPerVertex[vertexIndex].push_back(
           {*index, bone.mWeights[j].mWeight, bone});
       }
     }

[HaileyEira]

Thank you. I'll use the workaround for now. 🩵

[eGax]

Explains why mine wasn't crashing. Thank you @LogicAndTrick for the info.

[HumanGamer]

However this particular crash can be prevented by checking the range when loading the animation:

diff --git a/common/src/IO/AssimpParser.cpp b/common/src/IO/AssimpParser.cpp
index 642519e67..96287989f 100644
--- a/common/src/IO/AssimpParser.cpp
+++ b/common/src/IO/AssimpParser.cpp
@@ -575,7 +575,10 @@ std::vector<Assets::EntityModelVertex> computeMeshVertices(
     {
       for (unsigned int j = 0; j < bone.mNumWeights; ++j)
       {
-        weightsPerVertex[bone.mWeights[j].mVertexId].push_back(
+        const auto vertexIndex = bone.mWeights[j].mVertexId;
+        if (vertexIndex < 0 || vertexIndex >= mesh.mNumVertices)
+          continue;
+        weightsPerVertex[vertexIndex].push_back(
           {*index, bone.mWeights[j].mWeight, bone});
       }
     }

Tested this myself and it causes the model to look wrong. You need to break out, not continue, and if you do break out you also need to fail the if check here https://github.com/TrenchBroom/TrenchBroom/blob/master/common/src/IO/AssimpParser.cpp#L598

That will work correctly.