Add custom FBX Binary exporter with comprehensive test coverage - #169
Conversation
Replace Assimp's broken FBX exporter with a custom implementation that writes correct FBX binary format directly from Ogre data. This fixes skeleton data corruption and animation playback issues on reimport. Key changes: - New src/FBX/ module: FBXExporter writes FBX v7300 binary with proper geometry, skeleton (LimbNode hierarchy), skin deformers, animations, and materials - Fix Euler angle decomposition to match Assimp's R=Rz*Ry*Rx convention - Add Euler angle unrolling to prevent keyframe discontinuities - Call skeleton->setBindingPose() after Assimp import so animation deltas are applied relative to correct base transforms - Route "FBX Binary (*.fbx)" export through new exporter in MeshImporterExporter - Add FBX format to MCP server export_mesh tool - Comprehensive unit tests for Euler math, continuity, and FBX output Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The separate test targets in tests/ have their own source file lists and were missing FBXExporter.cpp, causing undefined reference errors. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
When a submesh was skipped in writeGeometryObjects (empty vertex data), m_geomIds had fewer entries than the raw submesh count, causing writeSkinDeformers to map skin data to the wrong geometry. Track the actual submesh index for each geometry entry and iterate over geometry entries instead of raw submesh indices. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
MeshManager::create() leaves meshes unloaded, causing FileNotFoundException when createEntity() tries to load from disk in CI. Switch to createManual() with minimal vertex/index data so meshes are fully loaded in memory. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add 36 new test cases in FBXExporterCoverageTest that use in-memory Ogre meshes (no file loading) to exercise ~98% of FBXExporter.cpp. Includes a lightweight FBX binary parser for output verification and 8 mesh helper functions covering: geometry (Z-mirror, winding, normals, UVs, 32-bit indices, non-shared vertices), materials, skeleton (bone transforms, deforming/non-deforming, hierarchy), skin deformers, animations (stacks, curves, Euler continuity), bind pose, textures, and connections. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe pull request adds a self-contained lightweight FBX binary parser and a comprehensive test suite to Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7b6898d73e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (use32) { | ||
| i0 = static_cast<int32_t>(reinterpret_cast<const uint32_t*>(ibase)[f * 3 + 0]); | ||
| i1 = static_cast<int32_t>(reinterpret_cast<const uint32_t*>(ibase)[f * 3 + 1]); | ||
| i2 = static_cast<int32_t>(reinterpret_cast<const uint32_t*>(ibase)[f * 3 + 2]); |
There was a problem hiding this comment.
The exporter reads triangle indices from the start of the hardware index buffer (f * 3 + ...) and never applies IndexData::indexStart. For meshes that share index buffers or use sliced ranges (e.g., LOD/submesh packing), this serializes the wrong triangles and can produce out-of-range polygon references in the FBX output. Offset the read by iData->indexStart before decoding each face.
Useful? React with 👍 / 👎.
| posElem->baseVertexPointerToElement( | ||
| const_cast<unsigned char*>(base + j * vbuf->getVertexSize()), &p); |
There was a problem hiding this comment.
Vertex attribute reads are indexed as base + j * vertexSize without VertexData::vertexStart, so submeshes that reference an offset into a shared vertex buffer export positions/normals/UVs from the wrong region. This leads to scrambled geometry data even when indices are valid. The per-vertex pointer arithmetic should include (vData->vertexStart + j).
Useful? React with 👍 / 👎.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/FBX/FBXExporter_test.cpp (1)
70-72:⚠️ Potential issue | 🟠 MajorReplace
M_PIusage with a portable constantLines 70–72 and 79–81 rely on
M_PI, which is not guaranteed by standard C++ and requires_USE_MATH_DEFINESon MSVC (not set in this project), causing compilation failures on Windows with default toolchain settings.Proposed fix
+constexpr double kPi = 3.14159265358979323846; + void testQuaternionToEulerXYZ(const Ogre::Quaternion& q, double& rx, double& ry, double& rz) { @@ - rx *= 180.0 / M_PI; - ry *= 180.0 / M_PI; - rz *= 180.0 / M_PI; + rx *= 180.0 / kPi; + ry *= 180.0 / kPi; + rz *= 180.0 / kPi; } @@ Ogre::Quaternion eulerToQuatAssimp(double rxDeg, double ryDeg, double rzDeg) { - double rx = rxDeg * M_PI / 180.0; - double ry = ryDeg * M_PI / 180.0; - double rz = rzDeg * M_PI / 180.0; + double rx = rxDeg * kPi / 180.0; + double ry = ryDeg * kPi / 180.0; + double rz = rzDeg * kPi / 180.0;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/FBX/FBXExporter_test.cpp` around lines 70 - 72, The test uses the non-portable M_PI to convert radians to degrees (see rx, ry, rz conversions and the similar uses later); replace M_PI with a project-local portable constant (e.g. add a constexpr double PI = 3.14159265358979323846; at the top of FBXExporter_test.cpp or in a common header) and use PI in the rotations math (replace M_PI occurrences with PI) so the code compiles on MSVC without _USE_MATH_DEFINES.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/FBX/FBXExporter_test.cpp`:
- Around line 473-503: Validate the read of encoded arrays by checking the local
variables encoding and byteLen before resizing/reading into p.doubleArray,
p.intArray, p.floatArray and p.longArray: if encoding != 0 treat the block as
compressed (use in.seekg(byteLen, std::ios::cur) to skip the bytes and set the
stream failbit) and do not attempt to read into the vector; otherwise verify
byteLen == count * sizeof(element_type) (e.g., sizeof(double)/int/float/long)
and only then resize the corresponding vector and call in.read; on mismatch set
failbit and avoid the read to prevent buffer overrun.
- Around line 1299-1301: The test dereferences objects returned by
findTopLevel("Objects") without checking for nullptr; add an assertion like
ASSERT_NE(objects, nullptr) immediately after retrieving objects to ensure the
test fails instead of crashing, then proceed to call
objects->findAll("Geometry") and ASSERT_EQ(geomNodes.size(), 1u) as before;
reference the findTopLevel call, the objects variable, geomNodes, and use
ASSERT_NE for the null-check.
- Around line 639-645: The exportAndParse helper currently hardcodes "/tmp" and
doesn't treat parseFBX failures as export failures; update exportAndParse (and
use ExportResult) to create a unique portable temp path via QTemporaryFile (or
QDir::tempPath() + QTemporaryFile) instead of "/tmp" and meshCounter to avoid
collisions, ensure the temp file is open/written/closed so
FBXExporter::exportFBX can write to it and parseFBX can read it, call
parseFBX(path) and if parseFBX returns an empty vector set r.success = false
(propagate the parse failure) before returning; reference functions:
exportAndParse, FBXExporter::exportFBX, parseFBX, ExportResult, meshCounter.
---
Outside diff comments:
In `@src/FBX/FBXExporter_test.cpp`:
- Around line 70-72: The test uses the non-portable M_PI to convert radians to
degrees (see rx, ry, rz conversions and the similar uses later); replace M_PI
with a project-local portable constant (e.g. add a constexpr double PI =
3.14159265358979323846; at the top of FBXExporter_test.cpp or in a common
header) and use PI in the rotations math (replace M_PI occurrences with PI) so
the code compiles on MSVC without _USE_MATH_DEFINES.
| case 'd': { | ||
| uint32_t count; in.read(reinterpret_cast<char*>(&count), 4); | ||
| uint32_t encoding; in.read(reinterpret_cast<char*>(&encoding), 4); | ||
| uint32_t byteLen; in.read(reinterpret_cast<char*>(&byteLen), 4); | ||
| p.doubleArray.resize(count); | ||
| in.read(reinterpret_cast<char*>(p.doubleArray.data()), byteLen); | ||
| break; | ||
| } | ||
| case 'i': { | ||
| uint32_t count; in.read(reinterpret_cast<char*>(&count), 4); | ||
| uint32_t encoding; in.read(reinterpret_cast<char*>(&encoding), 4); | ||
| uint32_t byteLen; in.read(reinterpret_cast<char*>(&byteLen), 4); | ||
| p.intArray.resize(count); | ||
| in.read(reinterpret_cast<char*>(p.intArray.data()), byteLen); | ||
| break; | ||
| } | ||
| case 'f': { | ||
| uint32_t count; in.read(reinterpret_cast<char*>(&count), 4); | ||
| uint32_t encoding; in.read(reinterpret_cast<char*>(&encoding), 4); | ||
| uint32_t byteLen; in.read(reinterpret_cast<char*>(&byteLen), 4); | ||
| p.floatArray.resize(count); | ||
| in.read(reinterpret_cast<char*>(p.floatArray.data()), byteLen); | ||
| break; | ||
| } | ||
| case 'l': { | ||
| uint32_t count; in.read(reinterpret_cast<char*>(&count), 4); | ||
| uint32_t encoding; in.read(reinterpret_cast<char*>(&encoding), 4); | ||
| uint32_t byteLen; in.read(reinterpret_cast<char*>(&byteLen), 4); | ||
| p.longArray.resize(count); | ||
| in.read(reinterpret_cast<char*>(p.longArray.data()), byteLen); | ||
| break; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check the file exists and view the relevant lines
wc -l src/FBX/FBXExporter_test.cppRepository: fernandotonon/QtMeshEditor
Length of output: 103
🏁 Script executed:
# Read the specific lines mentioned in the review (473-503)
sed -n '473,503p' src/FBX/FBXExporter_test.cppRepository: fernandotonon/QtMeshEditor
Length of output: 1507
🏁 Script executed:
# Get broader context around these lines to understand the function
sed -n '450,520p' src/FBX/FBXExporter_test.cppRepository: fernandotonon/QtMeshEditor
Length of output: 2807
🏁 Script executed:
# Search for any handling of 'encoding' variable elsewhere in the codebase
rg -n 'encoding' src/FBX/ -A 2 -B 2Repository: fernandotonon/QtMeshEditor
Length of output: 2850
🏁 Script executed:
# Check if there are any comments about FBX format or compression in the file
rg -n 'FBX|compress|deflate' src/FBX/FBXExporter_test.cppRepository: fernandotonon/QtMeshEditor
Length of output: 6186
🏁 Script executed:
# Look for size validation patterns in the codebase
rg -n 'byteLen|expected.*size' src/FBX/Repository: fernandotonon/QtMeshEditor
Length of output: 1799
Validate encoding and byteLen before array reads
The parser reads encoding and byteLen at lines 476/484/492/500 but never validates them. If encoding != 0 (compressed arrays), the code reads compressed data as raw bytes, corrupting the parse. Additionally, byteLen is not verified against the expected size (count * sizeof(element)), creating a buffer overflow risk: the vector is resized by count, but in.read() attempts to write byteLen bytes. For untrusted binary input, both must be validated before the read.
Add an encoding check (e.g., skip compressed arrays with in.seekg(byteLen, std::ios::cur) and set failbit) and verify byteLen == count * sizeof(T) before resizing and reading.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/FBX/FBXExporter_test.cpp` around lines 473 - 503, Validate the read of
encoded arrays by checking the local variables encoding and byteLen before
resizing/reading into p.doubleArray, p.intArray, p.floatArray and p.longArray:
if encoding != 0 treat the block as compressed (use in.seekg(byteLen,
std::ios::cur) to skip the bytes and set the stream failbit) and do not attempt
to read into the vector; otherwise verify byteLen == count *
sizeof(element_type) (e.g., sizeof(double)/int/float/long) and only then resize
the corresponding vector and call in.read; on mismatch set failbit and avoid the
read to prevent buffer overrun.
| ExportResult exportAndParse(Ogre::Entity* entity) { | ||
| ExportResult r; | ||
| r.path = QString("/tmp/fbx_coverage_%1.fbx").arg(meshCounter); | ||
| r.success = FBXExporter::exportFBX(entity, r.path); | ||
| if (r.success) | ||
| r.nodes = parseFBX(r.path.toStdString()); | ||
| return r; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/FBX/FBXExporter_test.cpp | sed -n '630,650p'Repository: fernandotonon/QtMeshEditor
Length of output: 789
🏁 Script executed:
# Check the parseFBX function definition
rg -n 'std::vector<FBXNode>\s+parseFBX' src/FBX/FBXExporter_test.cpp -A10Repository: fernandotonon/QtMeshEditor
Length of output: 387
🏁 Script executed:
# Check includes at the top of the file
head -50 src/FBX/FBXExporter_test.cppRepository: fernandotonon/QtMeshEditor
Length of output: 1587
🏁 Script executed:
# Verify that parseFBX returns empty vector on file read failure
rg -n 'return nodes' src/FBX/FBXExporter_test.cpp | head -20Repository: fernandotonon/QtMeshEditor
Length of output: 132
Use portable temp-file handling and propagate parse failures
Line 641 hardcodes /tmp which is non-portable (Windows uses a different temp directory) and pairs it with a deterministic filename using meshCounter, risking collisions in parallel test runs. Additionally, r.success remains true even when parseFBX() fails—returning an empty vector on file-open errors (line 551). This logic bug allows downstream tests to operate on empty node lists, causing potential crashes.
Use QTemporaryFile and QDir::tempPath() for portable temp handling, and propagate parse failure by checking if parseFBX() returns an empty vector before returning success.
Proposed fix
+#include <QTemporaryFile>
+#include <QDir>
@@
ExportResult exportAndParse(Ogre::Entity* entity) {
ExportResult r;
- r.path = QString("/tmp/fbx_coverage_%1.fbx").arg(meshCounter);
+ QTemporaryFile tmp(QDir::tempPath() + "/fbx_coverage_XXXXXX.fbx");
+ tmp.setAutoRemove(false);
+ if (!tmp.open())
+ return r;
+ r.path = tmp.fileName();
+ tmp.close();
+
r.success = FBXExporter::exportFBX(entity, r.path);
- if (r.success)
- r.nodes = parseFBX(r.path.toStdString());
+ if (!r.success)
+ return r;
+
+ r.nodes = parseFBX(r.path.toStdString());
+ r.success = !r.nodes.empty();
return r;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/FBX/FBXExporter_test.cpp` around lines 639 - 645, The exportAndParse
helper currently hardcodes "/tmp" and doesn't treat parseFBX failures as export
failures; update exportAndParse (and use ExportResult) to create a unique
portable temp path via QTemporaryFile (or QDir::tempPath() + QTemporaryFile)
instead of "/tmp" and meshCounter to avoid collisions, ensure the temp file is
open/written/closed so FBXExporter::exportFBX can write to it and parseFBX can
read it, call parseFBX(path) and if parseFBX returns an empty vector set
r.success = false (propagate the parse failure) before returning; reference
functions: exportAndParse, FBXExporter::exportFBX, parseFBX, ExportResult,
meshCounter.
| auto* objects = findTopLevel(r.nodes, "Objects"); | ||
| auto geomNodes = objects->findAll("Geometry"); | ||
| ASSERT_EQ(geomNodes.size(), 1u); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check the file size and read the relevant lines
wc -l src/FBX/FBXExporter_test.cppRepository: fernandotonon/QtMeshEditor
Length of output: 103
🏁 Script executed:
# Read the specific lines mentioned in the review comment
sed -n '1295,1305p' src/FBX/FBXExporter_test.cppRepository: fernandotonon/QtMeshEditor
Length of output: 464
🏁 Script executed:
# Search for findTopLevel definition to understand its return type
rg -n 'findTopLevel' src/FBX/FBXExporter_test.cpp | head -20Repository: fernandotonon/QtMeshEditor
Length of output: 1294
🏁 Script executed:
# Check for the pattern of Objects usage throughout the file
rg -n 'findTopLevel.*"Objects"' src/FBX/FBXExporter_test.cppRepository: fernandotonon/QtMeshEditor
Length of output: 1544
🏁 Script executed:
# Look for other patterns where findTopLevel results are dereferenced
rg -B2 -A2 'findTopLevel.*->findAll' src/FBX/FBXExporter_test.cpp | head -40Repository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
# Read the findTopLevel function definition
sed -n '567,585p' src/FBX/FBXExporter_test.cppRepository: fernandotonon/QtMeshEditor
Length of output: 829
🏁 Script executed:
# Check a few other instances of Objects usage to see the pattern
sed -n '1269,1280p' src/FBX/FBXExporter_test.cppRepository: fernandotonon/QtMeshEditor
Length of output: 446
🏁 Script executed:
# Check if there are ASSERT_NE checks after findTopLevel calls elsewhere
rg -A3 'findTopLevel.*Objects' src/FBX/FBXExporter_test.cpp | head -50Repository: fernandotonon/QtMeshEditor
Length of output: 1573
Add null-check assertion before dereferencing objects
Line 1300 dereferences objects without verifying it's not null. The findTopLevel() function explicitly returns nullptr if the node is not found, so a missing "Objects" node would cause a null-pointer crash rather than a test failure. This pattern appears at line 1271 with proper ASSERT_NE(objects, nullptr) check and should be applied consistently.
Proposed fix
auto* objects = findTopLevel(r.nodes, "Objects");
+ ASSERT_NE(objects, nullptr);
auto geomNodes = objects->findAll("Geometry");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| auto* objects = findTopLevel(r.nodes, "Objects"); | |
| auto geomNodes = objects->findAll("Geometry"); | |
| ASSERT_EQ(geomNodes.size(), 1u); | |
| auto* objects = findTopLevel(r.nodes, "Objects"); | |
| ASSERT_NE(objects, nullptr); | |
| auto geomNodes = objects->findAll("Geometry"); | |
| ASSERT_EQ(geomNodes.size(), 1u); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/FBX/FBXExporter_test.cpp` around lines 1299 - 1301, The test dereferences
objects returned by findTopLevel("Objects") without checking for nullptr; add an
assertion like ASSERT_NE(objects, nullptr) immediately after retrieving objects
to ensure the test fails instead of crashing, then proceed to call
objects->findAll("Geometry") and ASSERT_EQ(geomNodes.size(), 1u) as before;
reference the findTopLevel call, the objects variable, geomNodes, and use
ASSERT_NE for the null-check.
|
1 participant
Summary
FBXExporterCoverageTest) with a lightweight FBX binary parser that verifies exported output. Covers geometry (Z-mirror, winding reversal, normals, UVs, 32-bit indices, non-shared vertices), materials, skeleton transforms (deforming/non-deforming bones, hierarchy), skin deformers, animations (stacks, curves, Euler continuity), bind pose, textures, and connections.Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit