[codex] Fix scene-save n-gon round-trip and bump 3.1.0 - #503
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughPR 503 preserves n-gon polygon topology through the scene export/import pipeline by encoding per-submesh cached face data into Assimp node metadata, then decoding and reconstructing that topology on reimport. Project version, README CI snippets, and website action-ref fallback are updated to 3.1.0. ChangesN-gon Topology Preservation and Version Release
Sequence Diagram(s)sequenceDiagram
participant SceneExport as buildSceneAiScene
participant ReadNgon as readNgonFacesFromMesh
participant Encode as encodeNgonMetadata
participant AiMeta as aiNode::mMetaData
participant SceneImport as SceneImporter
participant Decode as decodeNgonMetadata
participant WriteNgon as writeNgonFacesToMesh
participant OgreMesh as Ogre::Mesh
SceneExport->>ReadNgon: probe cached n-gon faces per submesh
ReadNgon-->>SceneExport: return face index vectors
SceneExport->>Encode: encode faces into compact aiString (qtme.faces.*)
Encode-->>SceneExport: aiString metadata
SceneExport->>AiMeta: store encoded metadata on meshOwnerNode
Note over AiMeta: Assimp scene serialized with aiNode::mMetaData
SceneImport->>AiMeta: read qtme.faces.<subIndex> metadata
AiMeta-->>SceneImport: return encoded aiString(s)
SceneImport->>Decode: decode into EditableFace index lists
Decode-->>SceneImport: ordered face index vectors
SceneImport->>WriteNgon: apply reconstructed polygons
WriteNgon->>OgreMesh: set n-gon face bindings on created mesh
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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: 4307ecb182
ℹ️ 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".
| std::vector<std::vector<unsigned int>> ngonFaces; | ||
| if (readNgonFacesFromMesh(mesh.get(), si, ngonFaces) && !ngonFaces.empty()) | ||
| ngonMetadataEntries.emplace_back(si, encodeSceneNgonFacesMetadata(ngonFaces)); |
There was a problem hiding this comment.
buildSceneAiScene() snapshots qtme.faces from the Ogre mesh before calling compactAiMesh(aiM), but compaction rewrites vertex indices whenever a submesh uses only a subset of its vertex buffer (very common with shared-vertex submeshes). The metadata is then written with stale indices, so on scene import writeNgonFacesToMesh() restores face bindings that no longer match the compacted geometry, producing incorrect topology (and potentially out-of-range face indices during later edit operations).
Useful? React with 👍 / 👎.
| const QByteArray json = QJsonDocument(outer).toJson(QJsonDocument::Compact); | ||
| return aiString(json.constData()); |
There was a problem hiding this comment.
encodeSceneNgonFacesMetadata() stores the entire JSON face list in a single aiString, but Assimp aiString is fixed-size (AI_MAXLEN), so non-trivial meshes can overflow/truncate this payload during export. When that happens, decodeSceneNgonFacesMetadata() fails to parse the truncated JSON and drops all n-gon bindings, meaning scene round-trip preservation only works for very small meshes.
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: 1
🧹 Nitpick comments (1)
src/MeshImporterExporter_test.cpp (1)
747-793: ⚡ Quick winCover the
.scene.glbround-trip too.This regression only exercises the text glTF path, but the feature contract and exporter branch also include
glb2. A small parameterized.scene.gltf/.scene.glbvariant would close the remaining coverage gap for the new n-gon metadata path. As per coding guidelines,src/**/*_test.cpp: Add Google Test unit tests for new functionality.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/MeshImporterExporter_test.cpp` around lines 747 - 793, Extend the test RoundTrip_QuadMesh_PreservesNgonFaceBinding to also exercise the binary glTF path by repeating the export/import assertions with a .scene.glb file (or parameterize the test over formats). After creating the OBJ via writeQuadObjForScene, invoke MeshImporterExporter::sceneExporter with sceneFile set to tmpDir.filePath("ngon.scene.glb"), then call MeshImporterExporter::sceneImporter on that .scene.glb and re-run the same checks that use readNgonFacesFromMesh, entity retrieval via Manager::getSingleton()/getSceneMgr()/getEntity, and EditableMesh::loadFromEntity to verify faces, indices and triangles are preserved; you can factor shared assertions into a helper to avoid duplication. Ensure the test still removes the objPath at the end.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/MeshImporterExporter.cpp`:
- Around line 3096-3103: ngonFaces are captured from the original mesh before
compactAiMesh() may renumber vertices, which can make stored qtme.faces.* refer
to stale indices; to fix, ensure you encodeSceneNgonFacesMetadata() from the
post-compaction index space by either: (A) call compactAiMesh(aiM) first and
then readNgonFacesFromMesh()/encodeSceneNgonFacesMetadata() using the compacted
aiM, or (B) change compactAiMesh(aiM) to return the vertex remapping (e.g. a
vector<int> oldToNew) and then apply that remap to the existing ngonFaces before
pushing into ngonMetadataEntries; update the code around readNgonFacesFromMesh,
ngonFaces, encodeSceneNgonFacesMetadata, and compactAiMesh to perform one of
these approaches so stored n-gon indices match the compacted aiM.
---
Nitpick comments:
In `@src/MeshImporterExporter_test.cpp`:
- Around line 747-793: Extend the test
RoundTrip_QuadMesh_PreservesNgonFaceBinding to also exercise the binary glTF
path by repeating the export/import assertions with a .scene.glb file (or
parameterize the test over formats). After creating the OBJ via
writeQuadObjForScene, invoke MeshImporterExporter::sceneExporter with sceneFile
set to tmpDir.filePath("ngon.scene.glb"), then call
MeshImporterExporter::sceneImporter on that .scene.glb and re-run the same
checks that use readNgonFacesFromMesh, entity retrieval via
Manager::getSingleton()/getSceneMgr()/getEntity, and
EditableMesh::loadFromEntity to verify faces, indices and triangles are
preserved; you can factor shared assertions into a helper to avoid duplication.
Ensure the test still removes the objPath at the end.
🪄 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: 1016ae56-93d3-46c0-a326-dfa5e4cecac3
📒 Files selected for processing (5)
CMakeLists.txtREADME.mdsrc/MeshImporterExporter.cppsrc/MeshImporterExporter_test.cppwebsite/src/hooks/useQtmeshActionRef.js
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/MeshImporterExporter_test.cpp`:
- Around line 674-683: The helper writeQuadObjForScene currently writes to a
deterministic path in the global temp directory which can clash across tests and
leave stale files; change writeQuadObjForScene to create a per-test temporary
file/dir (using QTemporaryFile or QTemporaryDir), write the OBJ contents into
that temporary file, ensure the file is kept alive for the duration of the test
(returning the QTemporaryFile/QTemporaryDir or its file path along with
ownership semantics so callers don't rely on manual deletion), and update tests
that call writeQuadObjForScene to accept the new return type or take ownership
so cleanup happens automatically even on failures or in parallel runs.
In `@src/MeshImporterExporter.cpp`:
- Around line 495-520: decodeSceneNgonFaceMetadata (and the other face-decoding
function handling the face index suffix) currently casts the unsigned long
returned by std::strtoul directly to unsigned int; add overflow checks after
each std::strtoul call by testing errno == ERANGE and comparing the parsed value
against std::numeric_limits<unsigned int>::max(), and return false if either
condition is true before performing the static_cast to unsigned int so values >
UINT_MAX are rejected instead of truncated.
- Around line 3625-3645: The imported N-gon indices are applied against the
post-processed aiMesh and don't account for the vertex/submesh remapping done
during export via compactAiMesh; fix decodeSceneNgonFacesMetadata handling by
applying the inverse remap before storing faces (or skip topology-changing
postprocess when importing). Concretely, when processing metadata in the loop
that calls decodeSceneNgonFacesMetadata and then writeNgonFacesToMesh, obtain
the remap that was used on export (or compute the inverse of the compactAiMesh
remap for the current aiMesh/submesh) and transform each decoded face index
through that inverse remap before populating importedNgonFaces[subIdx].faces, so
indices refer to the current ogreMesh topology; alternatively detect/guard
against aiProcess_JoinIdenticalVertices/aiProcess_OptimizeMeshes being active
and skip remapped metadata restoration in that case.
🪄 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: 3da74f7f-c723-4729-83f9-1357e14bf233
📒 Files selected for processing (2)
src/MeshImporterExporter.cppsrc/MeshImporterExporter_test.cpp
c376f5f to
ab9c3af
Compare
|
1 participant
Summary
Fix scene save/load so
.scene.gltf/.scene.glbpreserves n-gon topology metadata for edited quad meshes, and bump the app version to3.1.0with aligned docs examples.Root Cause
Scene export already knew how to preserve
qtme.faceson live Ogre meshes, but the scene-level glTF export path relied on Assimp face emission alone. glTF import returns triangulated geometry, so without carryingqtme.facesthrough the scene file, a saved quad could come back with only triangle topology data available to edit mode.What Changed
qtme.faces.<i>metadata on the mesh-bearing Assimp node.EditableMeshcan rehydrate quads/n-gons after scene round-trip.project(QtMeshEditor VERSION ...)to3.1.0.README.mdandwebsite/src/hooks/useQtmeshActionRef.jsto match3.1.0.Validation
./scripts/sync-doc-versions-from-cmake.sh --checkQT_QPA_PLATFORM=offscreen ./bin/UnitTests --gtest_filter='SceneSaveLoadTest.RoundTrip_QuadMesh_PreservesNgonFaceBinding:SceneSaveLoadTest.RoundTrip_TwoEntities_PreservesTransforms:SceneSaveLoadTest.RoundTrip_MixedSkeletalAndNonSkeletal:AboutTest.VersionTextIsCorrect:CLIPipelineSmoke.PrintVersionDoesNotCrash:CLIPipelineRun.VersionFlag:CLIPipelineRun.VersionFlagShort'Notes
src/dependencies/ogre-proceduralworktree content untouched.Summary by CodeRabbit
New Features
Tests
Chores