quads chunk 1: EditableFace foundation - #327
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 |
76b13a3 to
ff5cc8f
Compare
![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: 76b13a3514
ℹ️ 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 (!sub.faces.empty()) { | ||
| for (const auto& face : sub.faces) { |
There was a problem hiding this comment.
This branch unconditionally ignores sub.triangles whenever sub.faces is non-empty, but existing mesh mutators like EditableMesh::weldByPosition and collapseToSingleSubmeshAndWeld still rewrite only triangles. After a quad submesh goes through one of those paths, faces can contain pre-edit indices while triangles contain post-edit indices; rebuilding from this code then replays stale geometry (or skips faces as out-of-range), silently dropping the edit. A guarded fallback to triangles when a face stream is invalid/stale would avoid this data loss.
Useful? React with 👍 / 👎.
| for (size_t i = 1; i + 1 < idx.size(); ++i) { | ||
| EditableTriangle t; | ||
| t.indices[0] = idx[0]; | ||
| t.indices[1] = idx[i]; | ||
| t.indices[2] = idx[i + 1]; |
There was a problem hiding this comment.
triangulateFaces emits triangles directly from face.indices without checking that each index is < sub.vertices.size(). If a caller passes a face with an out-of-range index, this function mirrors invalid indices into sub.triangles (the legacy path used by buffer upload), which can produce invalid index buffers and undefined rendering behavior. buildFromEditableMesh already treats these faces as invalid, so this helper should enforce the same bound check before emitting triangles.
Useful? React with 👍 / 👎.
Introduces the n-gon (quad-aware) data model that subsequent chunks
will wire through GPU upload, importers, topology ops, and exporters.
No behavior change for triangle-only meshes — every existing test
still passes.
Data model:
- New EditableFace struct (n-vertex polygon) with isValid() guard.
- EditableSubMesh gains a `faces` field alongside the existing
`triangles`. Invariant: when `faces` is non-empty it is the
canonical face storage and `triangles` mirrors it as a
fan-triangulation; when `faces` is empty, `triangles` is canonical
(legacy triangle-only mode).
- Free helpers `triangulateFaces(sub)` and `promoteTrianglesToFaces(sub)`
keep the two representations in sync. Documented with the same
fan-triangulation rule HalfEdgeMesh::appendFace uses, so HE
round-trips don't change face shape.
HalfEdgeMesh:
- buildFromEditableMesh prefers `faces` when non-empty; falls back to
`triangles` for legacy submeshes. Out-of-range and duplicate-vertex
inputs are silently skipped.
- toEditableMesh writes any HE n-gon face into both `faces` and a
fan-triangulated `triangles`. Submeshes that turn out all-triangle
leave `faces` empty so legacy consumers see no diff.
- validate() relaxed from "exactly 3 half-edges per face" to "at
least 3"; n-gons are now first-class.
Tests (+11):
- Quad EditableFace round-trips as a single 4-valence HE face.
- toEditableMesh preserves quad in `faces` and emits 2 fan triangles
in `triangles`.
- Triangle-only meshes leave `faces` empty (legacy invariant).
- Mixed tri+quad submesh handling.
- EditableFace::isValid guard cases.
- Out-of-range index in EditableFace silently skipped.
- triangulateFaces / promoteTrianglesToFaces unit coverage.
Towards #326.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ff5cc8f to
ba49a76
Compare
|
1 participant
Summary
First chunk of the quad migration tracked by #326. Adds the n-gon data model (
EditableFace) and the round-trip plumbing throughHalfEdgeMesh. No user-visible behavior change — every existing path still uses triangles.Changes
EditableFacestruct (n-vertex polygon) withisValid()guard.EditableSubMeshgains afacesfield with a clear canonical-vs-mirror invariant: whenfacesis non-empty it is canonical andtrianglesis the fan-triangulation; when empty,trianglesis canonical (legacy mode).triangulateFaces()andpromoteTrianglesToFaces()that maintain the invariant.HalfEdgeMesh::buildFromEditableMeshprefersfaceswhen non-empty, falls back totriangles;toEditableMeshwrites both representations and only populatesfacesif the HE mesh actually contains n-gons (so legacy callers see no diff).validate()relaxed from "exactly 3 half-edges per face" to "at least 3".Tests
+11 HalfEdgeMesh standalone tests covering:
faces+ emits 2 fan triangles intriangles.facesempty.isValid()guard cases.triangulateFaces/promoteTrianglesToFacescorrectness.Test plan
Targeting
feat/quadsPer the rollout plan in #326, chunks land on the long-lived
feat/quadsbranch and only merge to master once the whole migration is working. This PR is chunk 1 of 7.Closes nothing yet; tracks #326.
🤖 Generated with Claude Code