fix: handle non-contiguous xmodel root bones (#1009)

* fix: handle non-contiguous xmodel root bones

* chore: add comment about invalid parent index fix

---------

Co-authored-by: Jan Laupetin <[email protected]>

src/ObjWriting/XModel/Gltf/GltfWriter.cpp

@@ -189,8 +189,8 @@ namespace
             for (auto meshIndex = 0u; meshIndex < meshCount; meshIndex++)
                 rootNode.children->emplace_back(m_first_mesh_node + meshIndex);
 
-            for (auto rootBoneIndex = 0u; rootBoneIndex < m_root_bone_count; rootBoneIndex++)
-                rootNode.children->emplace_back(m_first_bone_node + rootBoneIndex);
+            for (const auto rootBoneNode : m_root_bone_nodes)
+                rootNode.children->emplace_back(rootBoneNode);
 
             m_root_node = static_cast<unsigned>(gltf.nodes->size());
             gltf.nodes->emplace_back(std::move(rootNode));
@@ -303,7 +303,7 @@ namespace
 
             const auto boneCount = common.m_bones.size();
             m_first_bone_node = static_cast<unsigned>(gltf.nodes->size());
-            m_root_bone_count = 0;
+            m_root_bone_nodes.clear();
             for (auto boneIndex = 0u; boneIndex < boneCount; boneIndex++)
             {
                 JsonNode boneNode;
@@ -337,10 +337,7 @@ namespace
                     rotation = inverseParentRotation * rotation;
                 }
                 else
-                {
-                    assert(m_root_bone_count == boneIndex);
-                    m_root_bone_count++;
-                }
+                    m_root_bone_nodes.emplace_back(m_first_bone_node + boneIndex);
                 rotation.normalize();
 
                 boneNode.name = bone.name;
@@ -380,7 +377,8 @@ namespace
             if (!xmodel.m_bone_weight_data.weights.empty())
                 skin.inverseBindMatrices = m_inverse_bind_matrices_accessor;
 
-            skin.skeleton = m_first_bone_node;
+            if (m_root_bone_nodes.size() == 1u)
+                skin.skeleton = m_root_bone_nodes[0];
 
             gltf.skins->emplace_back(std::move(skin));
         }
@@ -751,7 +749,7 @@ namespace
         unsigned m_first_mesh_node = 0u;
         unsigned m_root_node = 0u;
         unsigned m_first_bone_node = 0u;
-        unsigned m_root_bone_count = 0u;
+        std::vector<unsigned> m_root_bone_nodes;
         unsigned m_position_accessor = 0u;
         unsigned m_normal_accessor = 0u;
         unsigned m_color_accessor = 0u;

src/ObjWriting/XModel/XModelToCommonConverter.cpp.template

@@ -37,6 +37,7 @@
 #include <cassert>
 #include <format>
 #include <string_view>
+#include <utility>
 
 using namespace GAME;
 
@@ -258,7 +259,19 @@ namespace
                 bone.name = "INVALID_BONE_NAME";
 
             if (boneNum >= model.numRootBones)
-                bone.parentIndex = static_cast<int>(boneNum - static_cast<unsigned int>(model.parentList[boneNum - model.numRootBones]));
+            {
+                const auto parentIndex = static_cast<int>(boneNum) - static_cast<int>(model.parentList[boneNum - model.numRootBones]);
+                if (parentIndex >= 0 && std::cmp_less(parentIndex, model.numBones))
+                {
+                    bone.parentIndex = static_cast<unsigned>(parentIndex);
+                }
+                else
+                {
+                    // This means that the parent index is invalid. Which should never happen... but it does.
+                    // For some custom maps... We will pretend it is a root bone in that case.
+                    bone.parentIndex = std::nullopt;
+                }
+            }
             else
                 bone.parentIndex = std::nullopt;
 

test/ObjWritingTests/Game/T4/XModel/XModelGltfWriterT4Test.cpp

@@ -0,0 +1,96 @@
+#include "Game/T4/XModel/XModelToCommonConverterT4.h"
+#include "XModel/Gltf/GltfWriter.h"
+
+#include <array>
+#include <catch2/catch_test_macros.hpp>
+#include <nlohmann/json.hpp>
+#include <optional>
+#include <string>
+
+using namespace T4;
+
+namespace
+{
+    class MockGltfOutput final : public gltf::Output
+    {
+    public:
+        std::optional<std::string> CreateBufferUri(const void* buffer, size_t bufferSize) const override
+        {
+            return std::nullopt;
+        }
+
+        void EmitJson(const nlohmann::ordered_json& json) const override
+        {
+            m_json = json;
+        }
+
+        void EmitBuffer(const void* buffer, size_t bufferSize) const override {}
+
+        void Finalize() const override {}
+
+        mutable nlohmann::ordered_json m_json;
+    };
+
+    TEST_CASE("XModelGltfWriterT4: Can write a non-contiguous root bone", "[t4][xmodel][gltf]")
+    {
+        constexpr std::array boneNameValues{
+            "tag_weapon",
+            "tag_brass",
+            "tag_flash",
+            "tag_sights",
+            "tag_sights_front",
+            "tag_clip",
+        };
+
+        Zone zone("MockZone", 0, GameId::T4, GamePlatform::PC);
+
+        std::array<ScriptString, boneNameValues.size()> boneNames{};
+        for (auto boneIndex = 0u; boneIndex < boneNameValues.size(); boneIndex++)
+            boneNames[boneIndex] = zone.m_script_strings.AddOrGetScriptString(boneNameValues[boneIndex]);
+
+        // The final offset is malformed in the source model: bone 5 minus 6 would be -1.
+        // Treating the subtraction as unsigned used to turn this into UINT_MAX and crash the GLTF writer.
+        std::array<unsigned char, boneNameValues.size() - 1> parentList{1, 2, 3, 4, 6};
+        std::array<XModelQuat, boneNameValues.size() - 1> quats{};
+        std::array<float, (boneNameValues.size() - 1) * 3> trans{};
+        std::array<DObjAnimMat, boneNameValues.size()> baseMats{};
+        for (auto& baseMat : baseMats)
+            baseMat.quat.w = 1.0f;
+
+        XModel model{};
+        model.name = "bo2_worldmodel_an94";
+        model.numBones = static_cast<unsigned char>(boneNames.size());
+        model.numRootBones = 1;
+        model.boneNames = boneNames.data();
+        model.parentList = parentList.data();
+        model.quats = quats.data();
+        model.trans = trans.data();
+        model.baseMat = baseMats.data();
+        model.numLods = 1;
+
+        XAssetInfo<XModel> assetInfo(ASSET_TYPE_XMODEL, model.name, &model);
+        assetInfo.m_zone = &zone;
+
+        xmodel::ToCommonConverterT4 converter;
+        const auto common = converter.Convert(assetInfo, 0);
+
+        REQUIRE(common.has_value());
+        REQUIRE(common->m_bones.size() == boneNameValues.size());
+        REQUIRE(common->m_bones[0].parentIndex == std::nullopt);
+        REQUIRE(common->m_bones[1].parentIndex == 0u);
+        REQUIRE(common->m_bones[2].parentIndex == 0u);
+        REQUIRE(common->m_bones[3].parentIndex == 0u);
+        REQUIRE(common->m_bones[4].parentIndex == 0u);
+        REQUIRE(common->m_bones[5].parentIndex == std::nullopt);
+
+        MockGltfOutput output;
+        const auto writer = gltf::Writer::CreateWriter(&output, "T4", zone.m_name);
+        writer->Write(*common);
+
+        REQUIRE(output.m_json["nodes"].size() == 7);
+        REQUIRE(output.m_json["nodes"][0]["children"] == nlohmann::ordered_json::array({1, 2, 3, 4}));
+        REQUIRE(output.m_json["nodes"][6]["children"] == nlohmann::ordered_json::array({0, 5}));
+        REQUIRE(output.m_json["skins"].size() == 1);
+        REQUIRE_FALSE(output.m_json["skins"][0].contains("skeleton"));
+    }
+} // namespace