quads chunk 2: GPU upload n-gon triangulation - #328
Closed
fernandotonon wants to merge 2 commits into
Closed
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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 |
6 tasks
fernandotonon
added a commit
that referenced
this pull request
Apr 28, 2026
Chunk 2 (#328) targets feat/quads-1-foundation (chunk 1) so the diff shows just chunk 2's own work rather than stacking onto the previous chunk's noise. That bypasses the workflow's branch trigger because the previous list only allowed `master` and `feat/quads`. Add the `feat/quads-*` glob to cover stacked chunk PRs. Towards #326. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
fernandotonon
force-pushed
the
feat/quads-1-foundation
branch
from
April 28, 2026 06:48
ff5cc8f to
ba49a76
Compare
Wires the EditableSubMesh::faces (n-gon canonical) field through the
GPU upload path so quad-bearing meshes render correctly via fan-
triangulation. Triangle-only meshes are byte-identical before/after.
Changes:
- buildSubMeshBuffers (the GPU upload entry point used by
resizeEntityBuffers and createNewMesh) re-triangulates faces into
triangles defensively when faces is non-empty, so the index buffer
always matches the live face data even if the caller forgot to
sync. Triangle-only submeshes pay zero overhead — no copy, no
extra work.
- recalculateNormals / recalculateNormalsFlat call triangulateFaces
first when faces is canonical, so vertex normals always reflect
the current polygon geometry.
- New EditableMesh::syncTriangulation() helper for callers that
mutate faces directly and want to publish the change to triangles.
- New EditableMesh::totalFaceCount() for the n-gon-aware caller —
reports faces.size() when n-gons are present, falls back to
triangles.size() for legacy submeshes.
Tests (+5):
- syncTriangulation fan-triangulates a quad face into 2 triangles.
- syncTriangulation is a no-op on triangle-only submeshes.
- totalFaceCount falls back to triangle count for legacy submeshes
and reports n-gon count when faces is canonical.
- recalculateNormals re-syncs triangles from faces and produces
correct vertex normals on a quad-only mesh.
No behavior change for triangle-only meshes — chunk 1 + chunk 2 keep
the existing path byte-identical. Quad meshes can now render correctly
end-to-end (limited by importers, which still triangulate at load —
that's chunk 3).
Towards #326.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
fernandotonon
force-pushed
the
feat/quads-2-gpu-upload
branch
from
April 28, 2026 06:48
7c6a063 to
e28245d
Compare
- src/EditableMesh.cpp:355,433: explicit `static_cast<uint32_t>` for `size_t` → Ogre's `vertexCount` / `indexCount` (uint32). The conversion was implicit pre-existing, but Sonar treats my edits to those lines as new and flags the precision-loss warning. - src/EditableMesh.h: convert the new `totalFaceCount()` and `syncTriangulation()` from member methods to free functions taking `std::vector<EditableSubMesh>&`. The class hit Sonar's 35-method ceiling; free functions are the natural pattern here anyway since they mirror the existing `triangulateFaces(EditableSubMesh&)` and `promoteTrianglesToFaces(EditableSubMesh&)` helpers. - Tests updated to call the free-function versions. No behavior change; same 191 tests still pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
3 tasks
10 tasks
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
Chunk 2 of the quad migration (#326). Builds on chunk 1 (#327). Wires the
EditableSubMesh::facesfield through the GPU upload path so quad-bearing meshes render correctly via fan-triangulation. No behavior change for triangle-only meshes — same paths, byte-identical output.Changes
buildSubMeshBuffers(the GPU upload used byresizeEntityBuffers+createNewMesh) defensively re-triangulates faces into triangles whenfacesis non-empty, so the GPU buffer is always consistent with the live face data.recalculateNormals/recalculateNormalsFlatre-sync triangles from faces before accumulating, so vertex normals reflect current polygon geometry.EditableMesh::syncTriangulation()for callers that mutatefacesdirectly.EditableMesh::totalFaceCount()reporting n-gon count when present, triangle count for legacy submeshes.Tests (+5 standalone)
syncTriangulationfan-triangulates a quad → 2 tris.syncTriangulationis a no-op on triangle-only submeshes.totalFaceCountsemantics for legacy and n-gon submeshes.recalculateNormalsre-syncs from faces and produces correct +Z normals on a planar quad.Test plan
Targeting
feat/quads-1-foundationStacked on chunk 1 (#327). When chunk 1 merges into
feat/quads, this PR's base will auto-update tofeat/quads.Towards #326.
🤖 Generated with Claude Code