feat(quads): preserve n-gons through export (chunk 6) - #349
Merged
Conversation
Stash per-submesh n-gon face data on the live Ogre::Mesh via
UserObjectBindings (qtme.faces.<i>) so exporters recover the source
polygon structure without re-reading the file — even after edits
clear the qtme.source_path tag.
- writeNgonFacesToMesh / readNgonFacesFromMesh helpers in
EditableMesh.{h,cpp} marshal a per-submesh
std::vector<std::vector<unsigned int>> through Ogre::Any.
- MeshImporterExporter import path runs a one-shot
EditableMesh::loadFromAssimpFile to extract source faces and write
the binding alongside the existing chunk-3 source_path tag.
- commitToEntity / resizeEntityBuffers refresh the binding on every
edit so the post-edit n-gon structure persists.
- Assimp scene builder (readSubmeshGeometry) reads the binding and
emits aiFace with N indices when present; falls back to the
triangle index buffer for legacy .mesh assets.
- Custom FBX exporter (writeGeometryObjects) reads the binding,
emits n-gon PolygonVertexIndex with the FBX last-index-bitwise-NOT
end marker, and per-PV layers (Normal / UV / Color) now expand via
a single polyVertexOrder traversal that supports both n-gon and tri
paths — eliminates ~80 LOC of duplicated triangle-walking code.
+4 Ogre-bound unit tests covering helper round-trip, binding-erase
on tri-only flip, missing-binding read, and commit-time refresh.
Skipped on macOS (Ogre init bypass per CLAUDE.md), exercised on
Linux CI.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughPersists per-submesh N-gon face topology onto live Changes
Sequence Diagram(s)sequenceDiagram
participant Assimp as Assimp Importer
participant Editable as EditableMesh (probe)
participant Ogre as Ogre::Mesh
participant Exporter as Exporter (FBX/Assimp)
participant File as Output File
Assimp->>Editable: loadFromAssimpFile(path, flags...)
Editable->>Ogre: writeNgonFacesToMesh(qtme.faces.<i> = polygons)
Note over Ogre: qtme.faces.0, qtme.faces.1, ...
Exporter->>Ogre: readNgonFacesFromMesh(subMeshIndex)
alt ngon data present
Ogre-->>Exporter: polygon vertex lists
Exporter->>Exporter: build PolygonVertexIndex (reversed winding)
Exporter->>Exporter: generate polyVertexOrder, expand normals/UVs/colors
else fallback to triangles
Ogre-->>Exporter: triangle index buffers
Exporter->>Exporter: build triangle-based polyVertexOrder and expand attributes
end
Exporter->>File: write geometry (polygons or triangles)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.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: 5ca71f67e2
ℹ️ 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".
Comment on lines
+1228
to
+1231
| if (ngonProbe.loadFromAssimpFile( | ||
| sourcePath, convertLH, | ||
| importer.getSceneUpAxis() == 2)) { | ||
| writeNgonFacesToMesh(mesh.get(), ngonProbe.subMeshes()); |
There was a problem hiding this comment.
Build the qtme.faces.<i> cache from the exact mesh-processing path used to create mesh, not a second EditableMesh::loadFromAssimpFile pass. AssimpToOgreImporter::loadModel uses a broader/different post-process flag set (including aiProcess_SortByPType, aiProcess_OptimizeMeshes, and additionalFlags), while this probe call does not, so submesh ordering and vertex indices can diverge. When exporters later trust the cached polygons, they can emit wrong topology (and in downstream per-vertex expansion paths, potentially index past valid vertex arrays).
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/EditableMesh.cpp`:
- Around line 757-762: loadFromMesh()/loadFromEntity() currently rebuild
submeshes solely from the triangle index buffer and thus discards the ngons
cached by writeNgonFacesToMesh() once qtme.source_path is cleared; modify
loadFromMesh() (and loadFromEntity() if it delegates) to call
readNgonFacesFromMesh() after loading the basic mesh, iterate each
EditableSubMesh and if readNgonFacesFromMesh() returned faces for that submesh,
restore EditableSubMesh::faces from that data and regenerate/“fan” its triangles
buffer from the restored faces so subsequent edit sessions preserve n-gon
structure.
In `@src/FBX/FBXExporter.cpp`:
- Around line 868-923: readNgonFacesFromMesh only guarantees binding/type; you
must validate all indices before using polyVertexOrder/polyIndices to avoid
out-of-bounds reads. After calling readNgonFacesFromMesh (in the block that
fills ngonFaces) iterate every polygon and every vertex index and if any vi >=
vData->vertexCount (or otherwise invalid) treat the ngon data as unusable: skip
using ngonFaces and fall back to the triangle/index-buffer path (the else branch
that reads iData/indexBuffer). Update the logic around readNgonFacesFromMesh,
polyVertexOrder, and polyIndices so the fallback happens per-submesh when any
invalid index is found; apply the same index-range validation anywhere else you
consume polygon indices before indexing normals/uvs/colors.
In `@src/MeshImporterExporter.cpp`:
- Around line 353-364: The code unconditionally sets aiM->mPrimitiveTypes =
aiPrimitiveType_POLYGON even though ngonFaces can contain triangles and mixed
faces; compute the bitmask instead by scanning the emitted faces (ngonFaces) and
OR-ing aiPrimitiveType_TRIANGLE when any face has 3 indices and
aiPrimitiveType_POLYGON when any face has !=3 indices, then assign that combined
bitmask to aiM->mPrimitiveTypes before allocating aiM->mFaces; keep the existing
face allocation/assignment (aiM->mNumFaces, aiM->mFaces, aiFace.mNumIndices,
aiFace.mIndices) but ensure the computed bitmask reflects both triangle and
polygon presence.
- Around line 1227-1231: The ngonProbe is being re-imported without the same
import flags, so update calls to EditableMesh::loadFromAssimpFile (used on
ngonProbe) to accept and forward the same additionalFlags passed to
importer.loadModel; then propagate that additionalFlags parameter into
EditableMesh::loadFromAssimpFile implementation so topology-affecting flags
(e.g., aiProcess_FindDegenerates, aiProcess_SortByPType) are applied
consistently, ensuring writeNgonFacesToMesh uses subMeshes() that match the
actual imported mesh topology.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e3b813b0-db77-4fa2-bade-7e2881399f65
📒 Files selected for processing (5)
src/EditableMesh.cppsrc/EditableMesh.hsrc/EditableMesh_test.cppsrc/FBX/FBXExporter.cppsrc/MeshImporterExporter.cpp
Four CodeRabbit Major findings on the n-gon export PR. All real:
1. Rehydrate cached faces in loadFromMesh (EditableMesh.cpp).
Once qtme.source_path is wiped after the first commit, the
qtme.faces.<i> binding is the only carrier for the n-gon
structure. loadFromMesh ignored the binding and rebuilt
submeshes triangle-only — the next commit would then re-erase
the cache, losing n-gons forever. Now reads the binding into
EditableSubMesh::faces and resyncs the triangle mirror.
2. Validate cached polygon indices in FBX exporter
(FBX/FBXExporter.cpp). readNgonFacesFromMesh only checks the
payload type, not index ranges. Stale or out-of-range indices
would cause OOB reads when the per-PV layers expand
normals/uvs/colors via polyVertexOrder. Now reject any polygon
with size < 3 or vi >= vData->vertexCount and fall back to the
triangle path for that submesh.
3. Compute mPrimitiveTypes as a bitmask (MeshImporterExporter.cpp).
Hard-coded aiPrimitiveType_POLYGON was wrong: Assimp's
mPrimitiveTypes is a bitmask of all kinds present, and a mixed
tri+quad submesh must declare both bits or downstream Assimp
exporters mis-gate format-specific emission (e.g. STL drops
non-triangle primitives by checking the bitmask).
4. Pass additionalFlags to the n-gon probe
(EditableMesh.{h,cpp} + MeshImporterExporter.cpp). The probe
used a hardcoded flag set while the rendered import passes
`additionalFlags` through. Topology-affecting flags like
aiProcess_FindDegenerates / aiProcess_SortByPType can split
submeshes — without matching flags, qtme.faces.<i> would attach
to a layout that doesn't match the live Ogre mesh and exporter
lookups by submesh index would silently mis-target.
All 232 standalone tests pass. Ogre-bound chunk-6 tests skipped on
macOS, exercised on Linux CI.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
EnterEditModeAfterEditDoesNotReimport asserted that re-entering Edit Mode after a commit produced an empty .faces (legacy triangle-only path). Pre-chunk-6 that was correct: commit erased qtme.source_path and loadFromMesh had no n-gon source. Chunk 6 wrote qtme.faces.<i> on commit and rehydrates from it on loadFromMesh. The new contract: n-gon faces SURVIVE commit → re-enter, so n-gon-aware ops keep working post-edit. Flip the assertion (faces.empty() → !faces.empty()) and explain the contract change in the comment. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PR #349 (chunk 6) introduces user-visible quad preservation through the FBX and OBJ export paths — that's a minor version bump. Also documented the per-format n-gon support matrix on writeNgonFacesToMesh: FBX and OBJ preserve n-gons end-to-end, while glTF/GLB triangulate at export because the spec only allows TRIANGLES/POINTS/LINES (no native polygon primitive). Preserving quads through glTF requires the FB_ngon_encoding vendor extension, which Assimp doesn't support — out of scope for chunk 6, deferred. .mesh and other Assimp formats follow the same triangulation rule they always have. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/EditModeController_test.cpp (1)
2067-2070: ⚡ Quick winAssert the preserved face arity on re-entry.
Right now this only proves that some face data came back. A regression that rehydrates the mesh as triangle faces would still pass, even though chunk 6 is specifically about preserving the original quad/n-gon structure.
Suggested assertion
ASSERT_TRUE(ctrl->enterEditMode()); EXPECT_FALSE(ctrl->currentMesh()->subMeshes()[0].faces.empty()) << "second entry (post-edit) should rehydrate n-gons from " "the qtme.faces.<i> binding"; + ASSERT_EQ(ctrl->currentMesh()->subMeshes()[0].faces[0].indices.size(), 4u) + << "post-edit re-entry should preserve the original quad, not just any face list";As per coding guidelines,
src/**/*_test.cpp: Add Google Test unit tests for new functionality.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/EditModeController_test.cpp` around lines 2067 - 2070, The test currently only verifies that faces are non-empty after re-entry (ASSERT_TRUE(ctrl->enterEditMode()) and EXPECT_FALSE(ctrl->currentMesh()->subMeshes()[0].faces.empty())), but must assert that face arity is preserved (i.e. quads/n-gons, not all triangles). After calling ctrl->enterEditMode(), inspect ctrl->currentMesh()->subMeshes()[0].faces and add assertions that at least one face has the expected number of indices (e.g. size()==4 for the quad) or that the set of faceSizes contains a value >3; use these checks instead of or in addition to the non-empty assertion so the test fails if faces were rehydrated as triangles.
🤖 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/MeshImporterExporter.cpp`:
- Around line 348-399: The ngon export path must validate cached polygon
bindings before emitting aiFace arrays: in the branch handling
readNgonFacesFromMesh (symbols: ngonFaces, aiM, readNgonFacesFromMesh, and the
later compactAiMesh reference), check each poly has size() >= 3 and each vertex
index < aiM->mNumVertices before allocating/assigning mIndices; if any polygon
is invalid (too few verts or an out-of-range index) fall back to the
triangle/index buffer path used in the non-ngon branch (reuse the logic that
reads Ogre::IndexData) instead of emitting the malformed aiFace, and ensure you
unlock/free any allocated buffers on early exit.
---
Nitpick comments:
In `@src/EditModeController_test.cpp`:
- Around line 2067-2070: The test currently only verifies that faces are
non-empty after re-entry (ASSERT_TRUE(ctrl->enterEditMode()) and
EXPECT_FALSE(ctrl->currentMesh()->subMeshes()[0].faces.empty())), but must
assert that face arity is preserved (i.e. quads/n-gons, not all triangles).
After calling ctrl->enterEditMode(), inspect
ctrl->currentMesh()->subMeshes()[0].faces and add assertions that at least one
face has the expected number of indices (e.g. size()==4 for the quad) or that
the set of faceSizes contains a value >3; use these checks instead of or in
addition to the non-empty assertion so the test fails if faces were rehydrated
as triangles.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fcd15c8f-7cc1-46dd-802e-519c1f51d9d4
📒 Files selected for processing (6)
CMakeLists.txtsrc/EditModeController_test.cppsrc/EditableMesh.cppsrc/EditableMesh.hsrc/FBX/FBXExporter.cppsrc/MeshImporterExporter.cpp
✅ Files skipped from review due to trivial changes (1)
- CMakeLists.txt
🚧 Files skipped from review as they are similar to previous changes (2)
- src/EditableMesh.cpp
- src/FBX/FBXExporter.cpp
Comment on lines
+348
to
399
| std::vector<std::vector<unsigned int>> ngonFaces; | ||
| const bool hasNgon = readNgonFacesFromMesh( | ||
| entity->getMesh().get(), subIndex, ngonFaces); | ||
| if (hasNgon) | ||
| { | ||
| aiM->mNumFaces = static_cast<unsigned int>(iData->indexCount / 3); | ||
| // Assimp's mPrimitiveTypes is a bitmask of all primitive kinds | ||
| // present. A submesh with mixed quads + triangles must declare | ||
| // both bits — Assimp's exporters check the bitmask to gate | ||
| // format-specific emission (e.g. STL emits TRIANGLE only). | ||
| // (CodeRabbit follow-up on PR #349.) | ||
| aiM->mPrimitiveTypes = 0; | ||
| aiM->mNumFaces = static_cast<unsigned int>(ngonFaces.size()); | ||
| aiM->mFaces = new aiFace[aiM->mNumFaces]; | ||
| auto ibuf = iData->indexBuffer; | ||
| auto* ibase = static_cast<const unsigned char*>(ibuf->lock(Ogre::HardwareBuffer::HBL_READ_ONLY)); | ||
| bool use32 = ibuf->getType() == Ogre::HardwareIndexBuffer::IT_32BIT; | ||
|
|
||
| const unsigned int indexStart = static_cast<unsigned int>(iData->indexStart); | ||
| for (unsigned int f = 0; f < aiM->mNumFaces; ++f) | ||
| { | ||
| aiM->mFaces[f].mNumIndices = 3; | ||
| aiM->mFaces[f].mIndices = new unsigned int[3]; | ||
| for (unsigned int v = 0; v < 3; ++v) | ||
| const auto& poly = ngonFaces[f]; | ||
| aiM->mPrimitiveTypes |= (poly.size() == 3) | ||
| ? aiPrimitiveType_TRIANGLE | ||
| : aiPrimitiveType_POLYGON; | ||
| aiM->mFaces[f].mNumIndices = static_cast<unsigned int>(poly.size()); | ||
| aiM->mFaces[f].mIndices = new unsigned int[poly.size()]; | ||
| for (size_t v = 0; v < poly.size(); ++v) { | ||
| aiM->mFaces[f].mIndices[v] = poly[v]; | ||
| } | ||
| } | ||
| } | ||
| else | ||
| { | ||
| const Ogre::IndexData* iData = subMesh->indexData; | ||
| if (iData && iData->indexCount > 0) | ||
| { | ||
| aiM->mNumFaces = static_cast<unsigned int>(iData->indexCount / 3); | ||
| aiM->mFaces = new aiFace[aiM->mNumFaces]; | ||
| auto ibuf = iData->indexBuffer; | ||
| auto* ibase = static_cast<const unsigned char*>(ibuf->lock(Ogre::HardwareBuffer::HBL_READ_ONLY)); | ||
| bool use32 = ibuf->getType() == Ogre::HardwareIndexBuffer::IT_32BIT; | ||
|
|
||
| const unsigned int indexStart = static_cast<unsigned int>(iData->indexStart); | ||
| for (unsigned int f = 0; f < aiM->mNumFaces; ++f) | ||
| { | ||
| unsigned int idx = use32 | ||
| ? reinterpret_cast<const uint32_t*>(ibase)[indexStart + f * 3 + v] | ||
| : reinterpret_cast<const uint16_t*>(ibase)[indexStart + f * 3 + v]; | ||
| aiM->mFaces[f].mIndices[v] = idx; | ||
| aiM->mFaces[f].mNumIndices = 3; | ||
| aiM->mFaces[f].mIndices = new unsigned int[3]; | ||
| for (unsigned int v = 0; v < 3; ++v) | ||
| { | ||
| unsigned int idx = use32 | ||
| ? reinterpret_cast<const uint32_t*>(ibase)[indexStart + f * 3 + v] | ||
| : reinterpret_cast<const uint16_t*>(ibase)[indexStart + f * 3 + v]; | ||
| aiM->mFaces[f].mIndices[v] = idx; | ||
| } | ||
| } | ||
| ibuf->unlock(); | ||
| } |
There was a problem hiding this comment.
Validate cached polygon bindings before emitting aiFaces.
Line 351 trusts qtme.faces.<i> as-is, but compactAiMesh() later remaps every face index without bounds checks. A stale binding with a <3 vertex polygon or an index >= aiM->mNumVertices can therefore turn into an out-of-bounds access or a corrupt export. The FBX path already defends against this; this Assimp path needs the same validation and should fall back to the triangle buffer when the cache is invalid.
Suggested guard
std::vector<std::vector<unsigned int>> ngonFaces;
const bool hasNgon = readNgonFacesFromMesh(
entity->getMesh().get(), subIndex, ngonFaces);
- if (hasNgon)
+ const bool hasValidNgon =
+ hasNgon
+ && !ngonFaces.empty()
+ && std::all_of(ngonFaces.begin(), ngonFaces.end(),
+ [aiM](const std::vector<unsigned int>& poly) {
+ if (poly.size() < 3) return false;
+ for (unsigned int idx : poly) {
+ if (idx >= aiM->mNumVertices) return false;
+ }
+ return true;
+ });
+ if (hasValidNgon)
{
// Assimp's mPrimitiveTypes is a bitmask of all primitive kinds
// present. A submesh with mixed quads + triangles must declare
// both bits — Assimp's exporters check the bitmask to gate
// format-specific emission (e.g. STL emits TRIANGLE only).
@@
- else
+ else
{
const Ogre::IndexData* iData = subMesh->indexData;
if (iData && iData->indexCount > 0)
{🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/MeshImporterExporter.cpp` around lines 348 - 399, The ngon export path
must validate cached polygon bindings before emitting aiFace arrays: in the
branch handling readNgonFacesFromMesh (symbols: ngonFaces, aiM,
readNgonFacesFromMesh, and the later compactAiMesh reference), check each poly
has size() >= 3 and each vertex index < aiM->mNumVertices before
allocating/assigning mIndices; if any polygon is invalid (too few verts or an
out-of-range index) fall back to the triangle/index buffer path used in the
non-ngon branch (reuse the logic that reads Ogre::IndexData) instead of emitting
the malformed aiFace, and ensure you unlock/free any allocated buffers on early
exit.
|
This was referenced May 9, 2026
This was referenced May 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes the chunk-6 acceptance criterion of #326: FBX/glTF/OBJ I/O preserves quad structure end-to-end. Both the Assimp-based exporter (glTF/OBJ/Collada/STL/etc.) and the custom binary FBX writer now emit source n-gons instead of fan-triangulated triangles.
Approach: cache n-gons on the Ogre::Mesh
`Ogre::Mesh::UserObjectBindings` was already the chunk-3/4 carrier for `qtme.source_path` / `qtme.source_convert_lh` / `qtme.source_up_axis`. Add a fourth: per-submesh `qtme.faces.` holding `std::vector<std::vector>` of polygon vertex indices.
What's NOT in this PR
Test plan
Acceptance criterion status (#326)
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores