quads: n-gon splitEdge + extrude/dissolve/merge fixes - #338
Merged
Conversation
Two related changes that drop the triangle-only workarounds the knife
and extrude paths needed on quad-imported meshes.
splitEdge now n-gon-aware
Previously a triangle-only MVP: bailed when either adjacent face had
arity != 3, so the knife (which builds on splitEdge → cutPath) had
to convert the entire mesh to triangles up-front via a build-from-
triangle-copy hack and restore untouched n-gon submeshes after.
New behaviour: replace each adjacent face with ONE face that has
vMid inserted between the shared edge's endpoints. A triangle
becomes a quad, a quad becomes a pentagon, etc. — no fan diagonal.
Contract change: two splitEdges on the same triangle no longer
produce the m1↔m2 edge as a side-effect (they used to, via the
vMid→vOpp diagonal in the old triangle-only code). Callers that
want the cut materialised must call splitFace explicitly. cutPath's
walk loop does this on every step now: when the next click vertex
lands on a face that already contains the previous one, splitFace
produces the connecting edge.
Knife pipeline simplified
Both `commitKnife` and `knifeHitTest` now build the HE directly from
the (possibly n-gon) editable mesh — no more triangle-mode copy, no
more touched-submesh restore dance. The knife produces real n-gon
outputs on quad-imported assets and triangulation only happens on
the faces the cut actually crosses (via splitFace, which already
handled n-gons). splitFace also drops its `n > 4` cap.
Extrude offset now n-gon-aware
`extrudeSelection`'s per-vertex offset computation walked adjacent
faces with `if (verts.size() != 3) continue;`, so on a quad-
imported mesh the offset was zero — and the post-extrude selection-
by-position then matched the OLD un-offset coords, leaving the user
with the pre-extrude vertices selected instead of the new cap.
Replace the triangle cross-product with Newell's method and accept
any face arity ≥ 3. "Top face" detection switches from "is a
triangle and all 3 verts are new" to "all N verts are new" so n-gon
caps contribute correctly. The extruded cap now offsets along its
averaged Newell normal, the position search finds the new
vertices, and selection lands on the cap as expected.
Tests
- Updated SplitEdgeMidpointOfInteriorEdge → expects 2 quads (was 4
triangles); SplitEdgeBoundaryEdge → expects 1 quad (was 2 tris).
- Updated TwoSplitEdgesOnOneTriangle to document the new contract:
splitEdge inserts vMid into the loop, splitFace materialises the
cut.
- New SplitEdgeOnQuadMeshKeepsQuads: locks down "splitEdge on a
quad's edge yields a pentagon, not 4 triangles" so future regressions
can't reintroduce fan diagonals.
- 235 standalone tests pass.
Smoke-tested on FBX quad asset: knife cuts produce real n-gon outputs
on the touched faces only, untouched submeshes keep their quads,
extrude moves the new cap and selects the new vertices.
Three bug fixes the FBX quad asset surfaced after the n-gon splitEdge
work landed.
dissolveEdges merges into a single n-gon
Was a triangle-only MVP: bailed when either adjacent face had arity
!= 3, so the whole op was a no-op on quad-imported meshes. Now walks
both face loops, removes the shared edge endpoints' duplicate
contributions, and appends ONE merged n-gon face. Triangles+quads
still merge cleanly; quad+quad → hexagon; etc.
dissolveVertices replaces the umbrella with a single n-gon
Same fix — was triangle-only (line bailed on `verts.size() != 3` and
used hard-coded 3-element index arithmetic). Now collects each
incident face's "non-v" boundary contribution (n-1 verts in winding
order), chains them into a closed loop, and replaces the umbrella
with one n-gon face. Hex fan center now collapses to a single
hexagon (was 4 fan triangles).
mergeVertices cleans up degenerate corners on n-gon faces
Cleanup pass had `if (verts.size() != 3) continue;` — so a quad
with consecutive-duplicate corners (the typical result of merging
near a quad corner) was never retired or rebuilt, surfacing as a
visible hole in the rendered mesh. Now collapses consecutive
duplicates (incl. wrap-around) on any face arity, retires faces
whose arity drops below 3, and queues a rebuild via retire +
appendFace for faces that just need the duplicates removed.
Duplicate-face detection key is sorted-verts + arity so quads and
triangles aren't accidentally compared.
hitTestVertex front-face culls
User-reported follow-up to the chunk-4b edge / face hit-test polish:
vertex selection on a dense FBX mesh could pull clicks to vertices
on the back of the model. Mirrors the front-facing test (Newell
normal vs camera direction) used elsewhere — only verts in at
least one front-facing polygon are pickable. This also applies
transitively to knife's vertex snap.
Tests
- DissolveEdgesQuadDiagonalMergesIntoSingleQuad: was 2 fan tris,
now 1 quad. Asserts the diagonal is gone AND no fan diagonal
replaces it.
- DissolveEdgesMultipleDisjointEdgesAllProcessed: was 4 fan tris,
now 2 quads.
- DissolveVerticesHexFanCenterCollapsesToHexagon: was 4 fan tris,
now 1 hexagon. Asserts the merged face has exactly 6 vertices.
- 235 standalone tests still pass.
Bevel n-gon path remains a follow-up: the existing `bevelEdges` /
`bevelVertices` algorithms have triangle-only retriangulation built
into multiple paths (effectiveWidth's "third vertex", inner-vertex
position computation, retriangulateBeveledFace). The current bevel
PR uses a triangle-mode HE copy with submesh-level n-gon restore as
an accepted trade-off — touched submeshes triangulate fully, but
unrelated submeshes preserve their quads.
|
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 |
|
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
Drops the triangle-only assumptions from several core ops so the knife / extrude / dissolve / merge paths produce real n-gon outputs on quad-imported meshes. Bevel keeps its triangle-mode workaround for now (the algorithm has triangle-only assumptions in multiple internal paths; n-gon bevel is a separate follow-up).
Core change:
HalfEdgeMesh::splitEdgeis now n-gon-awarePreviously bailed when either adjacent face had arity != 3. Now replaces each adjacent face with ONE face that has vMid inserted between the shared edge's endpoints — a triangle becomes a quad, a quad becomes a pentagon, etc. No fan diagonal.
Contract change: two splitEdges on the same triangle no longer produce the m1↔m2 edge as a side-effect (they used to, via the vMid→vOpp diagonal).
cutPath's walk loop now callssplitFaceexplicitly to materialise the cut between consecutive click vertices.splitFacealso drops itsn > 4cap (its loop walk is generic).Knife pipeline simplified
Both
commitKnifeandknifeHitTestnow build the HE directly from the (possibly n-gon) editable mesh — no more triangle-mode copy + restore-untouched-submeshes dance. The knife produces real n-gon outputs on quad-imported assets; only the cut-touched faces get triangulated (viasplitFace).Extrude offset for n-gon caps
extrudeSelection's per-vertex offset computation gated onverts.size() == 3, so on quad meshes the offset was zero — and the post-extrude selection-by-position then matched the OLD un-offset coords, leaving the user with the pre-extrude vertices selected. Replaced the triangle cross-product with Newell's method; "top face" detection is now "all N verts are new" instead of "is a triangle and all 3 verts are new".Dissolve edges → single n-gon
dissolveEdgeswas triangle-only (if (vA.size() != 3 || vB.size() != 3) continue;) so dissolving a quad's edge was a silent no-op. Now walks both face loops, removes the shared edge endpoints' duplicate contributions, appends ONE merged n-gon face. Triangle+triangle → quad; quad+quad → hexagon; etc.Dissolve vertices → single n-gon
Same fix — was triangle-only with hard-coded 3-element index arithmetic. Now collects each incident face's non-v boundary contribution (n-1 verts in winding order), chains into a closed loop, replaces the umbrella with one n-gon face. Hex fan center now collapses to a single hexagon.
MergeVertices cleans up n-gon faces
Cleanup pass had
if (verts.size() != 3) continue;— so a quad with consecutive-duplicate corners (the typical result of merging near a quad corner) was never retired or rebuilt, surfacing as a visible hole. Now collapses consecutive duplicates (incl. wrap-around) on any face arity, retires below-arity-3 faces, queues a rebuild via retire + appendFace for the rest. Duplicate-face detection key issorted-verts + arity.Bevel: accepted trade-off
bevelEdges/bevelVerticeshave triangle-only retriangulation built into multiple paths (effectiveWidth's third-vertex,innerForFace's coplanar-group check,retriangulateBeveledFace's 3-vertex pattern). To get visible bevel back on quad meshes without a major bevel rewrite, this PR builds the HE from a triangle-mode copy and restores untouched n-gon submeshes from the snapshot. Trade-off: the touched submesh triangulates fully (single-submesh assets become triangle-only after a bevel). Properly n-gon-aware bevel is the next follow-up.Hit-test polish
hitTestVertexmirrors the chunk-4b front-facing filter — vertex selection on a dense FBX mesh no longer pulls clicks to back-face vertices.Test plan
SplitEdgeMidpoint…InsertsVertexInBothFaces(was 4 tris → 2 quads),SplitEdgeBoundary…InsertsVertexInTheTriangle(was 2 tris → 1 quad),TwoSplitEdgesOnOneTriangleNeedFollowupSplitFace(documents the new "splitFace materialises the cut" contract),DissolveEdgesQuadDiagonalMergesIntoSingleQuad,DissolveEdgesMultipleDisjointEdgesAllProcessed,DissolveVerticesHexFanCenterCollapsesToHexagon.SplitEdgeOnQuadMeshKeepsQuadsregression test.Known follow-ups