feat(quads): n-gon mesh representation foundation + edit-mode ops - #347
Merged
Conversation
The quad migration (#326) lands chunked PRs onto a long-lived `feat/quads` branch before the final merge to master. Without including `feat/quads` in the workflow's branch list, those chunked PRs run no build/test CI and the rollout has no safety net. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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>
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>
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>
- 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>
Adds the n-gon-aware re-import path discussed on #326. The existing AssimpToOgreImporter pipeline is untouched; this chunk only: 1. Caches the source file path on the imported Ogre::Mesh via UserObjectBindings("qtme.source_path"). MeshImporterExporter attaches it after every Assimp-backed import. 2. Adds EditableMesh::loadFromAssimpFile(path) which spins up an independent Assimp::Importer with aiProcess_Triangulate deliberately OFF, so source quads survive into aiMesh::mFaces and get recorded as EditableFace entries. Vertex attributes (positions/normals/UVs/colors/bone weights) are read out of the same scene; skeleton, animation, material, and tangent processing are skipped — Edit Mode operates on geometry only. Notes & deferred work: - Not yet wired into EditModeController::enterEditMode. Doing so safely needs a "user has modified this mesh" flag so re-importing doesn't discard prior edits — that's a follow-up chunk. - Re-import cost (~10–100ms typical, more on huge FBX) is acceptable as a one-time Tab-into-Edit-Mode cost. Tests (+6, standalone): - empty path / missing file rejected - OBJ quad → 1 EditableFace (4 verts), 2 fan triangles - triangle-only OBJ leaves `faces` empty (chunk-1 invariant) - mixed tri+quad OBJ produces both face types - non-empty mesh is replaced 197 standalone tests green (was 191; +6). Towards #326. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
quads chunk 1: EditableFace foundation
quads chunk 2: GPU upload n-gon triangulation (re-targeted)
quads chunk 3: importer quad detection (re-targeted)
Hooks Edit Mode into the loadFromAssimpFile path added in chunk 3, so
fresh-imported assets enter Edit Mode with their source quads intact
in EditableSubMesh::faces.
Modification tracking:
- commitToEntity (same-count edits) and resizeEntityBuffers (topology
edits) wipe the qtme.source_path cached on Ogre::Mesh. After any
user edit, the live GPU buffers diverge from the source file, so
re-importing would discard the edit.
- This makes "is this mesh modified?" a single-bit invariant: if
qtme.source_path is set, the mesh matches the source; if not, the
mesh has diverged (or was never imported, e.g. procedural primitives).
enterEditMode flow:
1. If qtme.source_path is set → loadFromAssimpFile (n-gon path).
2. Else (or on re-import failure) → loadFromEntity (legacy path).
Procedural primitives, .scene.glb sub-entities, post-edit re-entries,
and any path that doesn't carry a source path all fall back cleanly.
Tests (+5; Ogre-bound, run on Linux CI):
- commitToEntity wipes the cached path.
- resizeEntityBuffers wipes the cached path.
- enterEditMode uses the n-gon path when source_path is set
(faces non-empty for a quad OBJ).
- enterEditMode falls back to legacy when source_path absent
(faces empty — chunk-1 invariant preserved).
- After commit-and-exit, re-entering uses the legacy path.
Towards #326. Smoke-testable end-to-end: import a quad asset, Tab into
Edit Mode, the inspector should report n-gon submeshes with face count
matching the source file. Catmull-Clark and loop cut land in chunks 5
and 7 on top of this.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The original AssimpToOgreImporter applies aiProcess_ConvertToLeftHanded
to every non-.x asset, which flips X (and inverts UV V) so Ogre's
left-handed coordinate system gets correct geometry. My chunk-4
loadFromAssimpFile re-imported the same source file without that flag,
so EditableMesh ended up in the original (right-handed) coordinate
space while the rendered Ogre buffers were in the flipped (left-handed)
space. The vertex / edge / face overlays in Edit Mode therefore drew
mirrored relative to the on-screen mesh. (Reported manually on a Tom &
Jerry asset — overlay points appeared reflected through the Y-Z plane.)
Fix:
- MeshImporterExporter::importer caches the convert-to-left-handed
decision alongside the source path on Ogre::Mesh
(qtme.source_convert_lh).
- EditableMesh::loadFromAssimpFile gains a `convertToLeftHanded`
parameter (default true to match the importer's typical behaviour).
Applies aiProcess_ConvertToLeftHanded when set.
- EditModeController::enterEditMode reads the cached flag and passes
it through, so the editable mesh ends up in the same coordinate
system as the rendered Ogre buffers.
- commitToEntity / resizeEntityBuffers now wipe both
qtme.source_path AND qtme.source_convert_lh on user edits — the
pair is conceptually one cache entry.
Existing 6 LoadFromAssimpFile tests still green (default param keeps
their behaviour identical).
Towards #326.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds the classic Catmull-Clark subdivision-surface operation on the
half-edge mesh. Always produces an all-quad output regardless of
input topology — a triangle becomes 3 quads, a quad becomes 4, an
N-gon becomes N. Output geometry approaches a C¹-continuous limit on
closed manifolds via the standard rule:
- Face point Fp = average of corner positions.
- Edge point Ep = (a + b + Fp1 + Fp2) / 4 on interior edges,
(a + b) / 2 on boundary or cross-submesh edges.
- Smoothed vertex V' = (F + 2R + (n-3) V) / n on interior vertices,
chord rule on boundary vertices. F = avg of adjacent face points,
R = avg of adjacent edge midpoints (NOT edge points — spec).
Cross-submesh edges are treated as boundaries so material seams stay
sharp through the subdivision. Bone weights on smoothed vertices keep
the original assignment (the smoothed position is still mostly "near
V"); other attributes (UV, normal, color, tangent) blend with the
same weights as positions.
UI:
- The Subdivide toolbar button becomes a dropdown:
1. Standard — the existing 1-to-4 triangle split on selected
faces / edges. Unchanged behaviour.
2. Catmull-Clark — whole-mesh subdivide-surface step. Selection
is cleared after the op (partial-CC with selection
preservation needs a more sophisticated boundary blend that
isn't in this MVP).
- Button is enabled whenever in edit mode; menu items self-gate.
Tests (+7 standalone):
- 1 quad → 4 sub-quads.
- 1 triangle → 3 sub-quads.
- Closed cube stays closed (12 → 36 quads, no boundary edges).
- 2x2 quad grid → 16 sub-quads.
- Planar quad face point lands at arithmetic centre.
- Empty mesh is a no-op.
- Round-trip through EditableMesh preserves the all-quad output.
204 standalone tests pass (was 197; +7).
Towards #326.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`findEdgeIdxByVerts` was a linear scan over `m_edges` (size = origEdgeCount), called for every side of every face during the rebuild step of Catmull-Clark subdivision. On typical quad meshes this turned the rebuild phase into roughly O(F·E) (effectively O(F²)) and would stall on larger assets. Fix: pre-build an undirected (min(va,vb), max(va,vb)) → edgeIdx hash inside the existing edge-walk loop, then look up by vertex pair in O(1). The hash is populated only for live edges (`halfEdge >= 0`) with valid vertex indices, so behaviour matches the previous lambda exactly. All 7 Catmull-Clark standalone tests still pass.
Selection in Edit Mode now operates over n-gon polygons rather than
the artificial fan-triangulation:
- Click on a quad → highlights all of its triangles (the whole
polygon), not just one half. Visual selection matches the
user's mental model of "I clicked a face".
- Edge mode hit-test ignores fan diagonals (the artificial edge
between two halves of a quad) and only picks real polygon
perimeter edges. Backface culling added so backside edges
aren't pickable from the front, matching face-selection
behaviour.
- Topology ops (delete / dissolve / extrude / subdivide) consume
the selection through `selectedFacesAsHEFaceIndices()`, which
deduplicates triangle picks down to unique HE face indices —
so a quad selected via either of its triangles is processed
as a single face.
Subdivide-on-quad now actually does something:
- New `HalfEdgeMesh::subdivideFacesToQuads` splits each selected
n-gon into N sub-quads (face point + edge midpoints + corner),
sharing edge midpoints between adjacent selected faces in the
same submesh so the result stays manifold.
- `subdivideSelection` dispatches by face arity: triangles → the
existing 1-to-4 split; n-gons → the new quad split.
Plumbing:
- `EditableSubMesh` gains a `faceIndexForTriangle()` free helper
that maps a fan triangle back to its source face index, used
everywhere the controller needs to dilate triangle selections.
- `EditableMesh::loadFromAssimpFile` reads tangents from the
source file (when present) so the editable mesh carries them
end-to-end. We deliberately do NOT request
aiProcess_CalcTangentSpace because that flag implicitly
triangulates the mesh, defeating the n-gon path.
- `MeshImporterExporter::applyNormalMapsToEntity` is now public
so future fixes can re-attach RTSS bump-map state from any
Edit-Mode op.
Tests (+9 standalone + 4 HE):
- `faceIndexForTriangle` — legacy / quad / mixed / out-of-range /
null-output cases.
- `subdivideFacesToQuads` — empty, single quad → 4 sub-quads,
triangle → 3 sub-quads, two adjacent quads share midpoints.
Known issue (deferred to a follow-up fix-PR):
Bump-mapped meshes loaded through the n-gon import path lose
their bump map (and sometimes basic per-pixel lighting) after a
topology op. Triangle-only and procedural assets are unaffected.
Tracked separately because the fix needs proper RTSS / shader
pipeline instrumentation rather than continued blind permutation.
None of the various invalidate / validate / re-attach
permutations attempted reproduce the import-time behaviour
reliably; the real fix likely lives in tangent / vertex-
declaration handling on the round-trip path.
Towards #326.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two related findings, same root cause: `faceIndexForTriangle` and `selectedFacesAsHEFaceIndices` were counting *all* faces with at least three indices, but `triangulateFaces` and `HalfEdgeMesh::buildFromEditableMesh` only consume faces that pass `EditableFace::isValid()` (which additionally rejects consecutive duplicate indices). On meshes with any invalid face entry — e.g. a 4-vertex face like `[0,0,1,2]` — the triangle-to-face mapping and the per-submesh HE face base-offset would drift, so face-mode ops (extrude / delete / dissolve / subdivide) could mutate the wrong polygon. Fix: align both call sites to skip faces by `!isValid()` (matching what buildFromEditableMesh actually appends), and base the HE face offset on the count of valid faces only. Adds a regression test (FaceIndexForTriangleSkipsInvalidFaces) covering the consecutive-duplicate-index case explicitly. All 190 standalone tests still pass.
Stacked chunk landing on feat/quads. Known issues (deferred to follow-up fix-PR with the lighting/RTSS regression): n-gon-path transforms not applied at import; bone indices use mesh-local not skeleton-global numbering for skinned meshes. Both surface as wrong selection overlays / bone bindings on affected assets.
quads chunk 5a: Catmull-Clark subdivide
quads chunk 4b: n-gon-aware face/edge selection
Addresses both Codex P1 findings on PR #332 (chunk 4) before they reach master. Issue 1 — bone-handle drift on skinned meshes loadFromAssimpFile stored aiBone mesh-local indices in EditableBoneAssignment::boneIndex. The GUI import path (MeshProcessor) instead resolves aiBone->mName against the loaded Ogre::Skeleton and stores Ogre::Bone::getHandle(). When a topology op re-emitted VertexBoneAssignments via resizeEntityBuffers, those mesh-local indices were re-interpreted as Ogre handles, so vertices rebound to whichever bones happened to occupy those handle slots. Fix: add an optional Ogre::Skeleton* parameter to loadFromAssimpFile. When non-null, aiBone->mName is resolved against it (matching MeshProcessor) and the resulting handle is stored. Bones that don't resolve are skipped so we never emit wild handles. EditModeController passes the live entity's skeleton when entering edit mode. Issue 2 — Z-up overlay rotation on FBX/glTF assets MeshProcessor bakes a +90°-around-X rotation into rendered buffers for assets declared Z-up (FBX UpAxis = 2), so the Ogre scene-graph stays Y-up without a node rotation. loadFromAssimpFile read raw aiMesh vertices unchanged, so on Z-up assets the editable representation lived in pre-bake space while rendered buffers were post-bake — vertex/edge/face overlays appeared rotated 90° relative to the on-screen geometry, and a commit would write the rotated positions back, silently rotating the entity. Fix: add an `isZup` parameter to loadFromAssimpFile. When true, apply the same +90°-around-X bake to position, normal, and tangent before storing them. MeshImporterExporter caches the source up-axis under "qtme.source_up_axis" alongside the existing source-path / convert-LH caches, and EditModeController reads it back when re-entering edit mode. EditableMesh::commitToEntity / resizeEntityBuffers now also erase this cache key when the live buffers diverge from the source. Tests: two new standalone regression tests cover the Z-up bake math and the unskinned-mesh shape of the new bone-skeleton parameter. The skinned-mesh skeleton-lookup case is exercised by EditModeController integration tests at run time.
Addresses the deferred lighting/RTSS regression flagged on PR #334. Root cause After any Edit-Mode topology op (subdivide, extrude, bevel, knife, merge, delete/dissolve, undo/redo, …) bump-mapped meshes loaded through the n-gon import path went dark and lost their normal map. Two compounding issues: 1. EditableMesh::buildSubMeshBuffers checks only `vertices[0] .hasTangent` to decide whether to add a VES_TANGENT element to the rebuilt declaration. New vertices created by the op default- construct EditableVertex (zero-valued Vector4 tangent), so: (a) if the first vertex retained tangents the declaration kept VES_TANGENT but new vertices wrote (0,0,0,0), making RTSS's SRS_NORMALMAP TBN math collapse to zeros; (b) if the asset came in via loadFromAssimpFile (which deliberately omits aiProcess_CalcTangentSpace because that flag forces triangulation), every vertex has hasTangent=false and the declaration drops VES_TANGENT entirely. Either way the bump map was effectively gone. 2. Each topology op had its OWN inline _deinitialise/_initialise + invalidateMaterial block; the undo/redo path (EditMeshTopologyCommand::applyMeshState) had a third copy that skipped the RTSS hook entirely. So a Subdivide-then-Ctrl-Z lost lighting even when the redo would have restored it. Fix Centralise post-topology-op refresh into a new static method EditModeController::rewriteEntityAfterTopologyChange(Entity*). It: a) Detects bump-map intent by scanning every subentity's material for a `normal_map`/`NormalMap` TUS. If any subentity is bump- mapped, force `Mesh::buildTangentVectors` (storeParityInW=true) BEFORE _deinitialise/_initialise. Order matters: calling it AFTER _initialise is too late — the SubEntity already linked against the old (no-tangent) declaration and the RTSS shaders compile against that stale layout. b) Saves per-subentity material overrides before the deinit/init (Ogre resets them to the SubMesh default), restores after. c) Re-runs MeshImporterExporter::applyNormalMapsToEntity so RTSS re-attaches its SRS_NORMALMAP sub-render-state against the fresh tangents. invalidateMaterial alone only drops cached shader programs; the SRS_NORMALMAP gets dropped on removeShaderBasedTechnique inside applyNormalMap, so we must call it again to re-add it. d) Final invalidateMaterial pass to keep behaviour identical to the old per-op blocks for materials that aren't bump-mapped. All five inline copies in EditModeController.cpp (extrude, bevel commit, bevel cancel, knife commit, generic post-op via applyTopologyMutationNoSurvivor) now call this helper, and so does EditMeshTopologyCommand::applyMeshState in TransformCommands.cpp — so undo/redo gets the same treatment. Tests Standalone regression test confirms the helper is reachable as a static method (so TransformCommands.cpp's qualified call survives a refactor that might shove it back into an anonymous namespace) and null-tolerant. Full bump-map / RTSS exercise needs a GL context which the test infra doesn't provide on macOS; coverage there is via hand smoke tests on the bump-mapped Mixamo asset (subdivide / extrude / undo / redo all confirmed visually preserving the bump map and per-pixel lighting).
Two regressions surfaced after chunks 4 / 4b / 5a landed on feat/quads.
1. Fill produced fan triangles instead of an n-gon
`HalfEdgeMesh::fillSelection` always called `appendTriangle` in a
fan loop and emitted N-2 triangles for N inputs. After the n-gon
round-trip, `toEditableMesh` saw N-2 separate triangles and wrote
them as N-2 EditableFaces, so a 4-vertex fill ended up as 2
triangles with a visible fan diagonal — and Standard Subdivide
on the result treated them as triangles, not as a quad.
Fix: for n=3 still call appendTriangle (matches existing output);
for n>=4 call `appendFace` once, creating a single n-gon HEFace.
The result round-trips through toEditableMesh as one EditableFace
with N indices. Return value semantics changes from "triangles
created" to "polygons created" (always 1 here); callers used the
value as a success/fail flag, so the surface contract holds.
2. Knife silently failed on quad-imported meshes
`splitEdge` (the primitive `cutPath` uses) is a triangle-only MVP
— it bails immediately if either adjacent face has != 3 vertices.
On a quad-imported mesh, every face is an n-gon and the very
first split fails, so knife appears to do nothing.
Workaround until a proper n-gon-aware splitEdge lands: in the
knife commit path, build the HE from a triangle-mode COPY of the
editable mesh (clear `.faces` so buildFromEditableMesh falls back
to the fan-triangulated `.triangles` mirror). The cost is
materialising the fan diagonals on every submesh the cut touches;
the benefit is a working knife. Tracked as a follow-up.
Tests
- 4-vertex fill now expects `1` polygon, not `2`, AND asserts the
result round-trips as a single quad EditableFace.
- 5-vertex fill same: `1` polygon, asserts a pentagon survives the
HE round-trip.
- 4-orphan fill updated for the new return value.
- Existing `FillSelectionRejectsLargerFanThatDuplicatesExistingTri`
still passes — the fan-vs-existing-triangle dedup check at the
head of fillSelection still walks fan triangles, so duplicate
rejection is unchanged.
Codex P2 on PR #335: applyNormalMapsToEntity is now called from rewriteEntityAfterTopologyChange on every topology op AND every undo/redo, so any single broken/unresolvable material would now abort the entire edit op via an unhandled `mat->load()` throw — a regression from the old path that only invalidated RTSS without forcing a load. Wrap the load call in a try/catch and skip the offending sub-entity on failure. Logs the material name + Ogre exception description so we don't lose the diagnostic.
…it-test Two correctness fixes on top of the earlier knife/fill work. 1. Fill produced inward-facing normals `fillSelection` walked the input vertices in user-supplied order (typically `std::set` ascending), which is whatever order the selection produced — nothing guaranteed it matched the winding of the surrounding mesh. On a hole's boundary loop, the new face often ended up oriented INTO the volume. Fix: before building the new face, compute its Newell normal and compare against the average Newell normal of every existing face that shares at least one of the selected vertices. If the dot product is negative the winding is inverted; reverse it. n=3 triangles and n>=4 n-gons go through the same orientation step. 2. Knife was a no-op on quad-imported meshes `knifeHitTest` built the HE from the live `*m_editableMesh` (n-gon path, since `.faces` is populated for quad-imported assets), but `commitKnife` builds the HE from a triangle-mode copy (clears `.faces` so the splitEdge MVP — triangle-only — can run). The `edgeIndex` recorded at click time pointed at edges in the n-gon HE; the commit-time HE had different edge numbering, so cutPath tried to split an unrelated edge and either no-op'd or mutated the wrong region. Fix: build the SAME triangle-mode HE in `knifeHitTest`, mirroring the commit pipeline. Edge indices now line up between hit-test and commit. Triangle-only meshes (welded cube, primitives) were unaffected since their `.faces` is already empty — the existing knife tests still pass.
Symptom: on the FBX quad asset the knife appeared to do nothing — clicks went through, the commit ran, but cutPath was never called. Root cause: knifeHitTest has a Priority-1 vertex snap (10px radius). On a dense imported mesh (Mixamo character ≈ 19k edges), almost any click also lands within 10px of a vertex, so points came back as KnifePoint::OnVertex. commitKnife only accepted OnEdge — the kind check rejected silently via a Sentry breadcrumb. Fix: at commit time, translate OnVertex points into edge clicks by finding any incident edge to that vertex in the (triangle-mode) HE and using t=0 or t=1. cutPath is unchanged — its splitEdge primitive clamps t away from the endpoints by 1e-4 to keep faces non-degenerate, so the resulting cut vertex sits ≈ 1e-4 of an edge length off the original. Fine for an MVP; a follow-up can teach cutPath to start / end at an existing vertex without splitting at all. OnFace clicks are still rejected (no edge-walk path can represent them).
On dense imported meshes the knife was picking edges/vertices on the far side of the model — invisible to the user but inside the screen- space pixel radius. Mirrors the chunk-4b front-facing fix that hitTestEdge / face selection already use. Implementation Build front-facing vertex / edge sets once at the top of knifeHitTest, using the n-gon `m_editableMesh` so quad meshes are filtered correctly. A polygon is front-facing when its Newell normal (rotated into world space) points toward the camera. A vertex is front-facing iff at least one incident polygon does; an edge iff at least one of its two adjacent polygons does. Priority 1 (vertex snap): only return OnVertex if `snapVert` is in the front-facing set. Priority 2 (edge snap): skip edges not in the front-facing set. Edge keys use (min,max) global-vertex pairs which line up directly with the triangle-mode HE the snap loop walks. Fan-triangulation diagonals — which the triangle-mode HE generates internally for quad faces — are NOT in `frontEdges` (it's built from polygon perimeters only), so the knife also can't snap to fake interior edges that don't exist on the n-gon mesh. Trade-off note Knife still cuts as triangles on quad meshes (the build-from- triangle-copy workaround materialises fan diagonals at commit). The proper fix is an n-gon-aware splitEdge, tracked as a follow-up before loop cut since both ops will share that infrastructure.
PR #335's SonarCloud quality gate failed on cognitive complexity: - rewriteEntityAfterTopologyChange: 47 (limit 25) - enterEditMode: 32 (limit 25) plus a deprecation warning on the old buildTangentVectors overload. Refactor (no behaviour change): - File-scope `entityWantsTangents`, `rebuildMeshTangents`, `invalidateEntityRtssMaterials` factor out the three loops inside `rewriteEntityAfterTopologyChange`. Switches to the non-deprecated buildTangentVectors signature. - File-scope `tryLoadEditableMeshNGonPath` factors out the four- nested-try-block n-gon-import attempt from `enterEditMode`. `rewriteEntityAfterTopologyChange` and `enterEditMode` now read top- to-bottom as plain sequences of named steps. All 234 standalone tests still pass.
PR #335's SonarCloud quality gate failed on cognitive complexity: - rewriteEntityAfterTopologyChange: 47 (limit 25) - enterEditMode: 32 (limit 25) plus a deprecation warning on the old buildTangentVectors overload. Refactor (no behaviour change): - File-scope `entityWantsTangents`, `rebuildMeshTangents`, `invalidateEntityRtssMaterials` factor out the three loops inside `rewriteEntityAfterTopologyChange`. Switches to the non-deprecated buildTangentVectors signature. - File-scope `tryLoadEditableMeshNGonPath` factors out the four- nested-try-block n-gon-import attempt from `enterEditMode`. `rewriteEntityAfterTopologyChange` and `enterEditMode` now read top- to-bottom as plain sequences of named steps. All 234 standalone tests still pass.
quads follow-up: bones/transforms/lighting after n-gon import
quads follow-up: knife works on quad meshes; fill produces n-gons
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.
quads: n-gon splitEdge + extrude/dissolve/merge fixes
New `HalfEdgeMesh::bevelEdgesNgon` handles arbitrary face arity.
`applyBevelTopology` dispatches to it when the editable mesh actually
has n-gon canonical faces; triangle-only meshes still go through the
existing `bevelEdges` so we don't lose its quality features (crease
detection, segments, profile curves).
Algorithm
Per beveled edge (v1, v2) with adjacent faces f1, f2:
1. Compute four "inner" vertices, each a perpendicular-in-face
offset from one endpoint at distance w. Width is clamped per
edge to 0.4 × shortest perimeter edge (walking the actual
face perimeter; the triangle bevel guessed a "third vertex"
which is the diagonal on a quad and gave wrong clamps).
2. Replace f1's loop: substitute v1 → innerV1F1, v2 → innerV2F1.
A triangle stays a triangle, a quad stays a quad — no fan
diagonals introduced. Same for f2.
3. Emit chamfer strip:
- One central quad bridging f1's inner pair to f2's
inner pair.
- Two corner triangles, one at each endpoint, joining
the inner vertices via the original endpoint.
4. Neighbor (non-beveled) faces aren't touched — v1/v2 stay
valid for them; the corner triangle bridges the gap.
This drops the previous "build from triangle copy + restore
untouched submeshes" workaround: the n-gon path produces real
quads + chamfer faces directly, no global triangulation of the
touched submesh.
MVP scope
- Isolated bevels only — input edges sharing an endpoint are
rejected (chained bevels need ring-aware logic; matches the
triangle bevel's same restriction).
- Single-segment flat chamfer; `segments > 1` and `profilePoints`
are reserved for a future extension.
- Vertex bevel (`bevelVertices`) keeps its triangle-mode
workaround for now — will be the next follow-up.
Tests
- BevelEdgesNgonOnQuadEdgeKeepsQuads: two adjacent quads, bevel
the shared edge → 4 inner vertices, 5 faces (2 modified quads +
1 chamfer + 2 corner caps), no fan diagonals introduced.
- BevelEdgesNgonRejectsBoundaryEdge: boundary edge skipped.
- BevelEdgesNgonRejectsChainedSelection: 4-quad cross arrangement,
two interior edges sharing the center vertex; both rejected.
- 238 standalone tests pass.
Extends the n-gon bevel work to (a) the vertex-bevel API and (b)
multi-segment chamfers on the edge bevel.
bevelVerticesNgon
Single-segment flat-cap implementation. Per beveled vertex v of
valence ≥ 3:
- For each incident face f, create one inner vertex at distance
`width` from v along the direction toward f's centroid. Width
clamps to half the shortest incident edge.
- Replace each incident face's loop: substitute v → inner_f.
Original arity preserved (triangle stays triangle, quad stays
quad — only the v-corner moves inward).
- Cap the corner with a single n-gon face walking the inner
vertices in ring order.
applyBevelVertexTopology dispatches to the n-gon variant when the
editable mesh has n-gon canonical faces, falls back to the
existing triangle-only bevelVertices otherwise.
bevelEdgesNgon segments support
Per beveled edge, build a chain of N+1 vertices at each endpoint
spanning innerVF1 → innerVF2 with N-1 intermediates. Each
intermediate is a linear blend along the chord plus a profile-
controlled bulge along the "outward" axis (toward the original
endpoint, projected perpendicular to the chord). Chamfer becomes
N segment quads instead of one; corner caps become N-triangle
fans per endpoint. profilePoints / profile drive the bulge curve
exactly as for the triangle bevel — sin-envelope synthesis when
no per-point vector is supplied.
Tests
- BevelEdgesNgonSegments3ProducesRoundedChamfer: 4 inner + 4
intermediate verts, 11 active faces (2 quads + 3 chamfer
segments + 6 corner-cap triangles).
- BevelVerticesNgonOnQuadCornerKeepsQuads: 4 quads in a + cross
around a valence-4 vertex; bevel produces 4 modified quads + 1
cap quad with no fan diagonals.
- 240 standalone tests pass.
…tions Per-iteration the function does retireFace + appendFace + rebuildEdgesAndTwins, which renumbers edge slots and may invalidate the captured face indices. The cached EdgeInfo's `edgeIdx`, `f1`, `f2` then no longer match the live mesh, so the second edge in a multi-edge selection used to operate on stale references and corrupt the topology. Fix: re-resolve the edge by (v1, v2) vertex pair against the live mesh at the top of each iteration. Vertex indices are append-only and stable across rebuilds. Refresh f1, f2, subMeshIndex, and the face loops from the resolved live edge before computing the bevel. Mirrors the same pattern cutPath / dissolveEdges already use for this class of staleness bug.
Higher-valence endpoints (vertex on more than two faces) left a non-
manifold gap with the previous implementation: the corner cap
triangle bridged innerVF1 → v → innerVF2, but the OTHER incident
faces (e.g. the cube's left/right faces when beveling its top edge)
still terminated at v with the original v-edges. The new chamfer's
side edges had no twin in those neighbor faces, so the result was
non-manifold around v.
Fix: for each neighbor face g of v1 / v2 (not f1 / f2), splice the
appropriate inner vertex into g's loop right next to v. Walk g's
loop; when an outgoing edge is the (v, sideNeighbor) edge that's
shared with f1 or f2, insert the corresponding inner vertex
between v and sideNeighbor. The neighbor's arity grows by one per
splice, but the manifold stays closed: each new edge has exactly
two adjacent faces.
The standalone corner-cap triangle is now emitted ONLY when v is
"isolated" (incident only to f1 and f2, no neighbor faces) — that's
the 2-face local topology the previous version handled correctly.
For higher valence the splicing replaces it.
Tests
- New BevelEdgesNgonOnQuadCubeProducesManifoldOutput: quad cube
with valence-3 endpoints. Without the splicing fix the result
was non-manifold; now it's clean.
- 241 standalone tests pass.
Document at the top of bevelEdges and bevelVertices that they are the triangle-only path, that controllers should dispatch to the n-gon variant when the editable mesh has n-gon canonical faces, and that splitting these big functions into phase-sized helpers is now lower priority since the common case (quad-imported assets) takes the simpler n-gon path. Pure docs change — no behaviour difference.
P1 — n-gon edge bevel left boundary edges at endpoints with
neighbors. The previous "skip caps when v has neighbors"
optimization meant the chamfer's v-side edge (innerVF1, innerVF2)
had no twin face — neighbor-face splicing inserts those inner
verts AROUND v in the neighbor loop, but never the
innerVF1↔innerVF2 edge itself. Closed meshes (cube interior
edges, valence ≥ 3) ended up with boundary seams at the chamfer
endpoints.
Fix: emit corner caps unconditionally. The cap closes the chamfer
side-edge AND its other two edges twin against the neighbor
splice's (v, innerVF) edges. Cap winding flipped to
(v, bV1, aV1) (resp. (v, aV1, bV1)) so it walks edges in the
opposite direction from the neighbor splice — twins match,
no inverted-tri non-manifold.
P2 — n-gon vertex bevel partially mutated state on failure. The
inner-vertex append happened inline with the per-face validation
loop, so a mid-loop `continue` could leave orphan vertices in
m_vertices and report them in the return value, making callers
think the bevel succeeded.
Fix: two-phase. Validate every incident face FIRST and stash each
inner position in a temporary vector. Only if all faces validate
do we actually push the new HEVertex slots and grow newVertices.
Tests
- BevelEdgesNgonOnQuadCubeProducesManifoldOutput: previously
passed by isManifold's permissive boundary check; with the
cap-restore fix the result is now properly closed (no
boundary edges at all on a closed input).
- 241 standalone tests still pass.
quads: n-gon-aware bevel (edges + vertices)
* fix(quads): n-gon bevel chamfer no longer twists on imported meshes Two coupled bugs surfaced when beveling edges on the user's FBX character asset: the chamfer quad twisted (one corner flipped to the wrong side), and the profile bulge inverted between v1 and v2 chains so concave/convex profiles produced saddle-shaped chamfers instead of smooth curves. faceInwardDir was winding-dependent Original formula: `inward = NewellNormal × edge`. The Newell normal flips sign on CW-wound faces, which flips the cross product result. FBX imports can deliver mixed face winding, so some inner vertices ended up on the WRONG side of the beveled edge — causing the chamfer quad to fold across the original edge (visible as a "twist" connecting v2's pair to v1's pair through the mesh interior). Fix: use `(faceCentroid - v) projected perpendicular to edge` instead. The centroid is always inside a convex face regardless of winding direction, so (centroid - v) reliably points "into the face" from any corner. No dependence on Newell-normal sign. Profile bulge axis flipped between v1 and v2 The per-endpoint outward direction `(v - chordMidpoint)` lands on opposite sides of the chamfer surface at v1 vs v2 on non- axis-aligned meshes. The bulge sign then flipped: convex profile pushed v1's chain outward but v2's chain inward → saddle. Fix: anchor sign-consistency to the chamfer centroid (mean of the four inner vertices). The centroid sits inside the original corner; the correct outward direction has positive dot with `(v - centroid)` at BOTH endpoints. If a local outward computation flips, negate it. Both chains now bulge the same way in world space. This anchor is mesh-orientation-independent — no reliance on face-normal bisectors or per-endpoint reference passing. The centroid is geometrically meaningful: it's where the chamfer surface "centers" relative to the original corner. * fix(quads): use loop-neighbor anchor for faceInwardDir on concave n-gons Codex P2 on PR #340: my centroid-based `faceInwardDir` assumed the vertex-average centroid lies in the interior half-plane of every boundary edge, which is false for concave n-gons (the centroid can sit outside the local edge half-plane). On concave faces my inward vector flipped, pushing inner vertices outward — chamfer folded / self-intersected. Fix: replace the centroid anchor with v's non-edge LOOP NEIGHBOR. In any simple polygon (convex or concave), the third vertex of the corner at v topologically sits in the face's interior relative to the (v, otherEndpoint) edge. (toAnchor = nonEdgeNeighbor - v), projected perpendicular to the edge, gives a correct inward direction without depending on: - winding sign (Newell-normal-based formulas flip on CW), - convexity (centroid-based formulas mis-fire on concave). Tests still pass (241).
Loop cut walks the perpendicular ring of quads adjacent to a
selected edge, bisecting each one with a new midpoint chain. Quad-
only operation — that's the geometric definition: triangles have
no opposite-edge correspondence, so a "loop" through tri-tri pairs
is ambiguous and not implemented in this MVP. Matches Blender's
behaviour (loop cut on a triangulated mesh is also a no-op there).
HalfEdgeMesh::loopCut(startEdgeIdx)
Walk plan:
- Start: the two faces adjacent to startEdgeIdx.
- Each step: in the current quad, find the opposite edge (the
side two positions away in the loop) and record the rail
pair (entry edge + opposite). Cross opposite into the next
face.
- Stop on: closed loop (back to start edge), boundary edge
(no second face), non-quad face (no opposite-edge
correspondence).
Walk both directions from the start edge so an interior edge of
an open mesh produces cuts on both sides. visitedFaces guards
against re-entry once a closed ring closes.
Materialisation: collect rails up-front (vertex-pair-keyed since
edge indices shift across splitEdge). Per step, ensure midpoint
on each rail (shared between consecutive steps via map cache),
then splitFace bisects the now-hexagonal face along the new
diagonal. Returns the new midpoint vertex indices in walk order.
EditModeController::loopCutSelection()
Edge-mode only. Uses the FIRST selected edge as start (multi-
edge loop cuts aren't in MVP scope — each cut is independent).
Pushes one undo command labeled "Loop Cut".
UI
Toolbar button (‖ double vertical line) next to Fill, with
Ctrl+R shortcut. The Ctrl modifier disambiguates from R = Scale
mode (Unity convention). Falls through to Scale when the loop
cut returns 0 (e.g. nothing selected, non-quad neighborhood),
so Ctrl+R remains harmless outside the loop-cut context.
Tests
- LoopCutOnQuadStripCutsEachQuadOnce: 3-quad strip → 4 rail
midpoints, 6 quads after cut.
- LoopCutClosedRingOnQuadCubeReturnsToStart: quad cube → 4
rail midpoints (closed ring), 10 quads (6 + 4 cuts).
- LoopCutFailsOnNonQuadAdjacency: triangle pair → empty.
- LoopCutFailsOnInvalidEdgeIndex: -1 / out-of-range → empty.
- 244 standalone tests pass.
- mergeCoplanarTrianglesToQuads(EditableSubMesh&, angleDeg): walks edge adjacency, merges coplanar triangle pairs into 4-vert n-gon faces. Greedy first-fit, convexity-checked. +5 standalone tests. - EditModeController::convertToQuads exposed as Q_INVOKABLE; promotes legacy triangle-only submeshes into the n-gon path even when no merges happen, so downstream features start working. Pushes one "Convert to Quads" undo command. - New ▦ toolbar button in Edit Mode; auto-disables once the mesh is already n-gon-canonical (isMeshQuadBased predicate). meshDataChanged signal now refreshes the topology toolbar so undo flips state. - Loop cut hint: when the start edge has triangle adjacency the op short-circuits and emits editHintMessage so the status bar shows "Loop cut needs a quad mesh — try Mesh → Convert to Quads." - Quad-aware wireframe: when wireframeEnabled AND any submesh has .faces, render a separate ManualObject overlay along n-gon boundaries (skipping fan-triangulation diagonals) instead of PM_WIREFRAME. Pure-tri meshes keep the legacy material override. - Selected vertex/edge overlays bumped to RENDER_QUEUE_OVERLAY+1 and edge selection lines widened to 3px so selection still reads on top of the 2px boundary wireframe. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
# Conflicts: # src/EditableMesh.cpp # src/mainwindow.cpp
📝 WalkthroughWalkthroughAdds full n-gon/quad-aware editing across import, mesh model, half-edge topology, edit-mode controller, UI/tooling, undo path, and tests; exposes RTSS normal-map refresh API and caches Assimp import metadata; adjusts CI workflow triggers to include feat/quads branches. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MainWindow
participant EditModeController
participant EditableMesh
participant Assimp
participant OgreEntity
participant MeshImporterExporter
User->>MainWindow: enterEditMode()
MainWindow->>EditModeController: enterEditMode()
EditModeController->>OgreEntity: check qtme.source_path
alt cached source_path (n-gon)
EditModeController->>EditableMesh: loadFromAssimpFile(path)
EditableMesh->>Assimp: import (triangulation disabled)
Assimp-->>EditableMesh: polygonal faces + attributes
EditableMesh-->>EditModeController: faces + triangles populated
else legacy path
EditModeController->>EditableMesh: loadFromEntity()
EditableMesh-->>EditModeController: triangle-only mesh
end
EditModeController->>MainWindow: enter edit mode UI ready
sequenceDiagram
participant User
participant MainWindow
participant EditModeController
participant HalfEdgeMesh
participant EditableMesh
participant OgreEntity
participant MeshImporterExporter
User->>MainWindow: trigger convertToQuads()
MainWindow->>EditModeController: convertToQuads(angle)
EditModeController->>EditModeController: selectedFacesAsHEFaceIndices()
EditModeController->>HalfEdgeMesh: subdivideFacesToQuads(faceIndices)
HalfEdgeMesh-->>EditModeController: updated topology
EditModeController->>EditableMesh: toEditableMesh()
EditableMesh-->>EditModeController: updated faces/triangles
EditModeController->>OgreEntity: rewriteEntityAfterTopologyChange(entity)
OgreEntity->>MeshImporterExporter: applyNormalMapsToEntity(entity)
MeshImporterExporter-->>OgreEntity: rebuild tangents/RTSS states
EditModeController-->>MainWindow: emit editHintMessage
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 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: aa8910b619
ℹ️ 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
+4985
to
+4987
| const auto [f1, f2] = edgeFaces(startEdgeIdx); | ||
| walkDirection(f1, startA, startB); | ||
| walkDirection(f2, startA, startB); |
There was a problem hiding this comment.
loopCut is documented to return empty unless the start edge has two quad neighbors, but this implementation always walks both sides independently. If one side is a quad and the other is a triangle/non-quad, walk still becomes non-empty and the operation mutates topology (splitting the shared start edge and cutting the quad side) instead of failing. This produces one-sided cuts on mixed topology and bypasses the expected “quad mesh required” failure path.
Useful? React with 👍 / 👎.
Comment on lines
+5036
to
+5039
| if (meshHasNgonFaces()) { | ||
| m_savedMaterials.clear(); | ||
| updateBoundaryEdgeOverlay(); | ||
| return; |
There was a problem hiding this comment.
Wireframe mode is switched globally to boundary-overlay mode as soon as any submesh has faces, but the overlay renderer only emits edges for sub.faces and skips triangle-only submeshes. On assets that mix quad/ngon and triangle submeshes, the triangle-only parts lose wireframe entirely (render solid) while wireframe is enabled, which is a functional regression in edit visibility.
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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/HalfEdgeMesh_test.cpp (1)
2243-2300:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winRename this duplicate Google Test case.
TEST(HalfEdgeMeshStandalone, SmoothSurfaceBevelProducesManifold)is already defined earlier in this file, so this second definition will generate the same test class/symbols and fail the test target build.Suggested fix
-TEST(HalfEdgeMeshStandalone, SmoothSurfaceBevelProducesManifold) { +TEST(HalfEdgeMeshStandalone, SmoothCharacterBevelProducesManifold) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/HalfEdgeMesh_test.cpp` around lines 2243 - 2300, This test defines a duplicate Google Test symbol; rename the TEST macro instance to a unique test name to avoid the duplicate-test-name build failure: change the TEST(HalfEdgeMeshStandalone, SmoothCharacterBevelProducesManifold) identifier to a distinct name (for example SmoothCharacterBevelProducesManifold_Unique or BevelProducesManifold_Character) while leaving the test body (calls to makeSmoothCharacterMesh, HalfEdgeMesh::buildFromEditableMesh, findEdge, bevelEdges, toEditableMesh, isManifold, etc.) unchanged.src/EditModeController.cpp (2)
583-596:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDon't populate
m_selectedEdgesfrom triangulation on n-gon meshes.
selectAll()still walkssub.triangles, so quad/n-gon submeshes add fan diagonals to the edge selection. That makes Edge Mode select-all inconsistent withhitTestEdge()/selectFace()and draws non-existent edges in the overlay.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/EditModeController.cpp` around lines 583 - 596, selectAll() currently fills m_selectedEdges by iterating subMeshes[si].triangles which adds fan diagonals for n-gons; instead, iterate the actual polygon/facet vertex lists (e.g. subMeshes[si].polygons or faces) and insert only the consecutive edge pairs (and the closing last->first) using localToGlobal(si, v) to compute global indices so edges reflect true mesh edges; update the loop that references subMeshes[si].triangles to use the polygon vertex arrays and ensure behavior matches hitTestEdge() and selectFace().
2216-2243:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDispatch bevel implementation from the selected region, not from
any(sub.faces)across the mesh.One n-gon submesh currently forces
bevelEdgesNgon()for the entire entity, even when the selected edges live on triangle-only submeshes. On mixed meshes that silently drops the triangle path's segment/profile behavior. The same dispatch bug is duplicated inapplyBevelVertexTopology()below.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/EditModeController.cpp` around lines 2216 - 2243, The code currently sets meshHasNGons from any(sub.faces) across the whole mesh and dispatches heMesh.bevelEdgesNgon or bevelEdges for all selected edges; instead, determine which selected edges belong to n-gon submeshes and dispatch per-edge (or per-group of edges by submesh type): use originalSubMeshes (and the same vertex-pair -> submesh mapping used to build edges) to check each edge's source submesh faces for ngons, split the input edges/edgeIndices into two lists (ngonEdges and triEdges), call heMesh.bevelEdgesNgon(...) for ngonEdges and heMesh.bevelEdges(...) for triEdges, then merge the returned newHEVertices appropriately; apply the same per-edge/submesh dispatch fix to applyBevelVertexTopology() where the same meshHasNGons check is used.
🧹 Nitpick comments (2)
src/EditableMesh.cpp (1)
322-356: Consider storing mesh identity metadata to robustify re-import for edge cases.
loadFromAssimpFilehas no mechanism to track which aiMesh corresponds to which submesh when re-importing; it loads allscene->mNumMeshesand rebuilds the EditableMesh structure uniformly. This works correctly for single-file imports (whereAssimpToOgreImporter::loadModelalready processed all nodes), but if a source file contained unused meshes or transformed mesh instances, a future refactor that selectively loads subsets could silently mismatch.Consider storing
qtme.source_mesh_indexor similar metadata alongsideqtme.source_pathto explicitly track which aiMesh indices were used during the original import, and verify them on re-load. This would also future-proof against multi-entity scenarios that currently bypass this path entirely (they fall back toloadFromEntity).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/EditableMesh.cpp` around lines 322 - 356, Add and persist per-submesh source-mesh identity so re-imports map aiMesh -> submesh deterministically: when building EditableMesh in loadFromAssimpFile, record the aiMesh index (e.g., store qtme.source_mesh_index alongside existing qtme.source_path) for each created submesh entry, and on subsequent loads validate scene->mNumMeshes and the stored source_mesh_index(s) exist and still refer to the same mesh (or fall back to matching by name/geometry if they don’t); update serialization/deserialization of the EditableMesh metadata to include source_mesh_index and add a runtime check in loadFromAssimpFile to warn/handle mismatches.src/EditModeController.cpp (1)
398-414: ⚡ Quick winAdd a
file.importbreadcrumb around the Assimp re-import.This path performs real asset I/O, but it only emits an
edit_modebreadcrumb after success. Please log attempt/failure/success with thefile.importcategory so re-imports are searchable in Sentry.As per coding guidelines: "All user-facing actions and significant operations must be tracked with SentryReporter::addBreadcrumb(category, message). Use 'file.import'/'file.export' for I/O operations."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/EditModeController.cpp` around lines 398 - 414, Wrap the Assimp re-import path with file.import breadcrumbs: before calling tryLoadEditableMeshNGonPath(m_editEntity, m_editableMesh.get()) emit SentryReporter::addBreadcrumb("file.import", "Attempting n-gon re-import for Edit Mode"), on success emit SentryReporter::addBreadcrumb("file.import", "Succeeded n-gon re-import for Edit Mode") (in addition to the existing edit_mode breadcrumb), and on failure emit SentryReporter::addBreadcrumb("file.import", "Failed n-gon re-import for Edit Mode") before falling back to m_editableMesh->loadFromEntity(m_editEntity); ensure the same category/messages are used so Sentry searches can find import attempts, failures, and successes.
🤖 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 781-805: The early returns in EditableMesh::buildSubMeshBuffers
leave old GPU geometry attached to subMesh; instead of returning when
editSubIn.vertices.empty() or editSub->triangles.empty(), explicitly clear the
submesh so it stops rendering: reset/clear subMesh's vertex and index
attachments and counts and set its render operation to an empty/no-op state
(e.g., null/cleared vertexData and indexData, zeroed index/vertex counts, and
appropriate useSharedVertices/operationType) before returning; do this when you
detect no geometry after triangulateFaces(local) so the GPU buffers reflect the
empty topology.
In `@src/EditModeController_test.cpp`:
- Around line 2049-2054: The test currently uses
translateSelectedVertices(Ogre::Vector3::ZERO) which is a no-op; change it to
perform a real mutation so the commit path that clears qtme.source_path is
exercised: after calling ctrl->setSelectionMode(EditModeController::VertexMode)
and ctrl->selectVertex(0), call ctrl->translateSelectedVertices with a tiny
non-zero vector (e.g. Ogre::Vector3(1e-3f,0,0) or similar) or another guaranteed
edit, then keep ctrl->exitEditMode(true) to ensure the commitToEntity path runs
and qtme.source_path is cleared.
In `@src/EditModeController.cpp`:
- Around line 751-782: deselectFace currently only erases triangle entries from
m_selectedFaces, leaving perimeter edges/vertices added by selectFace behind;
update deselectFace to also compute the face perimeter for the same submesh
(using faceFirstTri/faceTriCount and existing topology helpers) and erase
corresponding global edge indices from m_selectedEdges and global vertex indices
from m_selectedVertices (use local->global helpers like
localEdgeToGlobal/localVertexToGlobal or the same mapping used elsewhere), track
if any edges/vertices were removed in addition to triangles, and only call
updateSelectionOverlay() and emit editSelectionChanged() if any removal
occurred.
- Around line 3666-3695: The current flow calls hm.subdivideFacesToQuads(...) on
a partial n-gon selection which leaves adjacent unselected faces untouched and
produces T-junctions; to fix, detect selection boundary neighbors and
retriangulate adjacent faces (same strategy as subdivideSelection for triangles)
so all edges along the selection perimeter are split consistently: (1) compute
boundary edges/neighbor face IDs from targetFaces, (2) include those neighbor
faces (or their affected triangles) in the retriangulation set or invoke the
existing retriangulation routine after hm.subdivideFacesToQuads, and (3)
preserve the current order (run tri splits first via hm.subdivideFaces, then run
the n-gon quad-split, then run the adjacency retriangulation step) so
subdivideFaces, subdivideFacesToQuads and the new adjacency-retriangulate pass
produce a crack-free result while still collecting newVertHE.
- Around line 3850-3909: The method currently returns totalMerges, which yields
0 when only promoteTrianglesToFaces() changed representation (no merges) even
though the mesh was mutated and undo was pushed; change the final return to
return a non-zero success code when topologyChanged occurred but totalMerges==0
(e.g. compute a return value like: return (totalMerges == 0 ? 1 : totalMerges)).
Locate the tail of the routine after UndoManager::getSingleton()->push(cmd) /
validateMesh() where the code currently does "return totalMerges" and replace it
with the conditional non-zero return so callers see a successful conversion when
promotion-only changes happened.
- Around line 2941-2979: The OnVertex handling picks the first incident edge and
exact t=0/1 which splitEdge then clamps, producing nondeterministic near-edge
splits; instead, gather all incident edges for p.vertexIndex using
hm.edgeCount()/edgeVertices, pick a deterministic one (e.g. smallest edge id)
and push a t nudged slightly off the endpoint (use a small epsilon like 1e-6 or
1-1e-6 depending on whether ev0 or ev1 matched) into the HalfEdgeMesh::CutPoint
so the commit is deterministic and consistently places the split adjacent to the
clicked vertex (update the loop that sets incidentEdge/incidentT for
KnifePoint::OnVertex to implement this deterministic selection and epsilon
nudging).
- Around line 860-894: The per-submesh HE base (heBaseBySub) counts only valid
faces but when faceIndexForTriangle returns a raw face index (faceK) you must
compact that index to skip invalid faces before it; otherwise later HE indices
are shifted. In the loop over m_selectedFaces (use globalTriToLocal,
faceIndexForTriangle, heBaseBySub, uniq), replace inserting heBaseBySub[subIdx]
+ faceK with computing adjustedFaceK = number of f in subs[subIdx].faces with
isValid() and index < faceK, then insert heBaseBySub[subIdx] + adjustedFaceK;
keep the legacy triangle branch unchanged.
In `@src/mainwindow.cpp`:
- Around line 175-180: Replace the transient statusBar()->showMessage call for
EditModeController::instance()->editHintMessage with a dedicated status widget
so frameEnded() can't overwrite it: add a QLabel (e.g. editHintLabel) to the
status bar via statusBar()->addPermanentWidget(...) or addWidget(...), connect
EditModeController::instance()->editHintMessage to a slot/lambda that sets
editHintLabel->setText(msg) and (re)starts a QTimer::singleShot(5000, ...) to
clear/hide the label, and ensure frameEnded() continues writing to the normal
showMessage slot but does not touch the new editHintLabel.
- Around line 1056-1058: The Convert to Quads button is being disabled using
c->isMeshQuadBased(), which returns true for any submesh with canonical faces
and thus hides the action on mixed tri/quad meshes; change the logic that sets
convertToQuadsButton->setEnabled(...) so the action remains available for mixed
meshes (e.g., always enable the button for now by removing the isMeshQuadBased()
check) and add a TODO to switch to a proper canConvertToQuads() query when
available; update references around convertToQuadsButton and isMeshQuadBased()
accordingly.
- Around line 1579-1590: The Ctrl+R branch only accepts the event when
EditModeController::loopCutSelection() > 0, allowing failed loop-cut attempts to
fall through to the 'R' Scale handler; update the Ctrl+R handling in
mainwindow.cpp so that once EditModeController::instance() reports
isEditModeActive(), selectionMode() == EditModeController::EdgeMode and
selectedEdgeCount() > 0 you always consume the event (call event->accept() and
return) even if loopCutSelection() <= 0, and still emit the SentryReporter
breadcrumb when appropriate (you can keep the breadcrumb only on successful
loopCutSelection()>0 if desired) so the shortcut is never passed to the 'R'
handler.
---
Outside diff comments:
In `@src/EditModeController.cpp`:
- Around line 583-596: selectAll() currently fills m_selectedEdges by iterating
subMeshes[si].triangles which adds fan diagonals for n-gons; instead, iterate
the actual polygon/facet vertex lists (e.g. subMeshes[si].polygons or faces) and
insert only the consecutive edge pairs (and the closing last->first) using
localToGlobal(si, v) to compute global indices so edges reflect true mesh edges;
update the loop that references subMeshes[si].triangles to use the polygon
vertex arrays and ensure behavior matches hitTestEdge() and selectFace().
- Around line 2216-2243: The code currently sets meshHasNGons from
any(sub.faces) across the whole mesh and dispatches heMesh.bevelEdgesNgon or
bevelEdges for all selected edges; instead, determine which selected edges
belong to n-gon submeshes and dispatch per-edge (or per-group of edges by
submesh type): use originalSubMeshes (and the same vertex-pair -> submesh
mapping used to build edges) to check each edge's source submesh faces for
ngons, split the input edges/edgeIndices into two lists (ngonEdges and
triEdges), call heMesh.bevelEdgesNgon(...) for ngonEdges and
heMesh.bevelEdges(...) for triEdges, then merge the returned newHEVertices
appropriately; apply the same per-edge/submesh dispatch fix to
applyBevelVertexTopology() where the same meshHasNGons check is used.
In `@src/HalfEdgeMesh_test.cpp`:
- Around line 2243-2300: This test defines a duplicate Google Test symbol;
rename the TEST macro instance to a unique test name to avoid the
duplicate-test-name build failure: change the TEST(HalfEdgeMeshStandalone,
SmoothCharacterBevelProducesManifold) identifier to a distinct name (for example
SmoothCharacterBevelProducesManifold_Unique or BevelProducesManifold_Character)
while leaving the test body (calls to makeSmoothCharacterMesh,
HalfEdgeMesh::buildFromEditableMesh, findEdge, bevelEdges, toEditableMesh,
isManifold, etc.) unchanged.
---
Nitpick comments:
In `@src/EditableMesh.cpp`:
- Around line 322-356: Add and persist per-submesh source-mesh identity so
re-imports map aiMesh -> submesh deterministically: when building EditableMesh
in loadFromAssimpFile, record the aiMesh index (e.g., store
qtme.source_mesh_index alongside existing qtme.source_path) for each created
submesh entry, and on subsequent loads validate scene->mNumMeshes and the stored
source_mesh_index(s) exist and still refer to the same mesh (or fall back to
matching by name/geometry if they don’t); update serialization/deserialization
of the EditableMesh metadata to include source_mesh_index and add a runtime
check in loadFromAssimpFile to warn/handle mismatches.
In `@src/EditModeController.cpp`:
- Around line 398-414: Wrap the Assimp re-import path with file.import
breadcrumbs: before calling tryLoadEditableMeshNGonPath(m_editEntity,
m_editableMesh.get()) emit SentryReporter::addBreadcrumb("file.import",
"Attempting n-gon re-import for Edit Mode"), on success emit
SentryReporter::addBreadcrumb("file.import", "Succeeded n-gon re-import for Edit
Mode") (in addition to the existing edit_mode breadcrumb), and on failure emit
SentryReporter::addBreadcrumb("file.import", "Failed n-gon re-import for Edit
Mode") before falling back to m_editableMesh->loadFromEntity(m_editEntity);
ensure the same category/messages are used so Sentry searches can find import
attempts, failures, and successes.
🪄 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: 5962a905-2503-42e0-851c-5c068e909831
📒 Files selected for processing (14)
.github/workflows/deploy.ymlsrc/EditModeController.cppsrc/EditModeController.hsrc/EditModeController_test.cppsrc/EditableMesh.cppsrc/EditableMesh.hsrc/EditableMesh_test.cppsrc/HalfEdgeMesh.cppsrc/HalfEdgeMesh.hsrc/HalfEdgeMesh_test.cppsrc/MeshImporterExporter.cppsrc/MeshImporterExporter.hsrc/commands/TransformCommands.cppsrc/mainwindow.cpp
Comment on lines
781
to
+805
| void EditableMesh::buildSubMeshBuffers(Ogre::SubMesh* subMesh, | ||
| const EditableSubMesh& editSub) | ||
| const EditableSubMesh& editSubIn) | ||
| { | ||
| if (!subMesh || editSub.vertices.empty() || editSub.triangles.empty()) | ||
| return; | ||
| if (!subMesh || editSubIn.vertices.empty()) return; | ||
|
|
||
| // n-gon synchronisation: if the caller populated `faces`, that's | ||
| // canonical and `triangles` is meant to be a fan-triangulation | ||
| // mirror. Re-triangulate defensively here so the GPU buffer always | ||
| // matches the live face data even if the caller forgot to call | ||
| // `triangulateFaces()` after mutating `faces`. | ||
| // | ||
| // We work on a local copy when re-triangulating is needed so the | ||
| // input EditableSubMesh stays untouched (this method takes the | ||
| // submesh by const&). Triangle-only submeshes incur no copy. | ||
| EditableSubMesh local; | ||
| const EditableSubMesh* editSub = &editSubIn; | ||
| if (!editSubIn.faces.empty()) { | ||
| local.vertices = editSubIn.vertices; // shallow-but-fine — we don't write | ||
| local.faces = editSubIn.faces; | ||
| local.materialName = editSubIn.materialName; | ||
| local.usesSharedVertices = editSubIn.usesSharedVertices; | ||
| triangulateFaces(local); | ||
| editSub = &local; | ||
| } | ||
| if (editSub->triangles.empty()) return; |
There was a problem hiding this comment.
Clear the submesh when triangulation produces no geometry.
Both early returns leave the previous vertexData/indexData attached to subMesh. If a topology op deletes the last face in a submesh, the old geometry will keep rendering because nothing resets the GPU-side counts or buffers. Handle the empty case by explicitly clearing the submesh data instead of returning immediately.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/EditableMesh.cpp` around lines 781 - 805, The early returns in
EditableMesh::buildSubMeshBuffers leave old GPU geometry attached to subMesh;
instead of returning when editSubIn.vertices.empty() or
editSub->triangles.empty(), explicitly clear the submesh so it stops rendering:
reset/clear subMesh's vertex and index attachments and counts and set its render
operation to an empty/no-op state (e.g., null/cleared vertexData and indexData,
zeroed index/vertex counts, and appropriate useSharedVertices/operationType)
before returning; do this when you detect no geometry after
triangulateFaces(local) so the GPU buffers reflect the empty topology.
Comment on lines
+2049
to
+2054
| // Commit any change (translate one vertex by zero — still triggers | ||
| // the commitToEntity path that wipes the source path). | ||
| ctrl->setSelectionMode(EditModeController::VertexMode); | ||
| ctrl->selectVertex(0); | ||
| ctrl->translateSelectedVertices(Ogre::Vector3::ZERO); | ||
| ctrl->exitEditMode(/*commitChanges*/ true); |
There was a problem hiding this comment.
Make this test perform a real mutation.
translateSelectedVertices(Ogre::Vector3::ZERO) is a no-op. If the controller ever short-circuits zero-delta transforms, this test stops covering the commit path that clears qtme.source_path. Use a tiny non-zero move or another guaranteed edit here.
Suggested change
- // Commit any change (translate one vertex by zero — still triggers
- // the commitToEntity path that wipes the source path).
+ // Commit a real change so this test definitely exercises the
+ // commitToEntity path that wipes the source path.
ctrl->setSelectionMode(EditModeController::VertexMode);
ctrl->selectVertex(0);
- ctrl->translateSelectedVertices(Ogre::Vector3::ZERO);
+ ctrl->translateSelectedVertices(Ogre::Vector3(0.001f, 0.0f, 0.0f));
ctrl->exitEditMode(/*commitChanges*/ true);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Commit any change (translate one vertex by zero — still triggers | |
| // the commitToEntity path that wipes the source path). | |
| ctrl->setSelectionMode(EditModeController::VertexMode); | |
| ctrl->selectVertex(0); | |
| ctrl->translateSelectedVertices(Ogre::Vector3::ZERO); | |
| ctrl->exitEditMode(/*commitChanges*/ true); | |
| // Commit a real change so this test definitely exercises the | |
| // commitToEntity path that wipes the source path. | |
| ctrl->setSelectionMode(EditModeController::VertexMode); | |
| ctrl->selectVertex(0); | |
| ctrl->translateSelectedVertices(Ogre::Vector3(0.001f, 0.0f, 0.0f)); | |
| ctrl->exitEditMode(/*commitChanges*/ true); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/EditModeController_test.cpp` around lines 2049 - 2054, The test currently
uses translateSelectedVertices(Ogre::Vector3::ZERO) which is a no-op; change it
to perform a real mutation so the commit path that clears qtme.source_path is
exercised: after calling ctrl->setSelectionMode(EditModeController::VertexMode)
and ctrl->selectVertex(0), call ctrl->translateSelectedVertices with a tiny
non-zero vector (e.g. Ogre::Vector3(1e-3f,0,0) or similar) or another guaranteed
edit, then keep ctrl->exitEditMode(true) to ensure the commitToEntity path runs
and qtme.source_path is cleared.
Comment on lines
751
to
782
| void EditModeController::deselectFace(int triIndex) | ||
| { | ||
| if (m_selectedFaces.erase(triIndex) > 0) { | ||
| if (!m_editableMesh) return; | ||
| auto [subIdx, localTri] = globalTriToLocal(triIndex); | ||
| if (subIdx >= m_editableMesh->subMeshes().size()) { | ||
| // Out of range — try the legacy single-triangle erase as a | ||
| // best-effort fallback so callers with stale indices don't | ||
| // silently no-op. | ||
| if (m_selectedFaces.erase(triIndex) > 0) { | ||
| updateSelectionOverlay(); | ||
| emit editSelectionChanged(); | ||
| } | ||
| return; | ||
| } | ||
| const auto& sub = m_editableMesh->subMeshes()[subIdx]; | ||
|
|
||
| // Mirror selectFace's dilation: deselect every fan triangle of | ||
| // the clicked face so the user sees the whole face de-highlighted. | ||
| size_t faceFirstTri = localTri, faceTriCount = 1; | ||
| faceIndexForTriangle(sub, localTri, &faceFirstTri, &faceTriCount); | ||
|
|
||
| bool anyErased = false; | ||
| for (size_t t = 0; t < faceTriCount; ++t) { | ||
| const size_t tri = faceFirstTri + t; | ||
| if (tri >= sub.triangles.size()) continue; | ||
| const int gi = localTriToGlobal(subIdx, tri); | ||
| if (m_selectedFaces.erase(gi) > 0) anyErased = true; | ||
| } | ||
| if (anyErased) { | ||
| updateSelectionOverlay(); | ||
| emit editSelectionChanged(); | ||
| } |
There was a problem hiding this comment.
Mirror face deselection into the edge/vertex mirrors.
deselectFace() only removes entries from m_selectedFaces. The perimeter edges and vertices added by selectFace() stay behind, so ctrl-deselect leaves stale edge/vertex overlays and stale selection state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/EditModeController.cpp` around lines 751 - 782, deselectFace currently
only erases triangle entries from m_selectedFaces, leaving perimeter
edges/vertices added by selectFace behind; update deselectFace to also compute
the face perimeter for the same submesh (using faceFirstTri/faceTriCount and
existing topology helpers) and erase corresponding global edge indices from
m_selectedEdges and global vertex indices from m_selectedVertices (use
local->global helpers like localEdgeToGlobal/localVertexToGlobal or the same
mapping used elsewhere), track if any edges/vertices were removed in addition to
triangles, and only call updateSelectionOverlay() and emit
editSelectionChanged() if any removal occurred.
Comment on lines
+860
to
+894
| const auto& subs = m_editableMesh->subMeshes(); | ||
| std::vector<int> heBaseBySub(subs.size(), 0); | ||
| int running = 0; | ||
| for (size_t s = 0; s < subs.size(); ++s) { | ||
| heBaseBySub[s] = running; | ||
| if (subs[s].faces.empty()) { | ||
| running += static_cast<int>(subs[s].triangles.size()); | ||
| } else { | ||
| // Match HalfEdgeMesh::buildFromEditableMesh, which only | ||
| // appends faces that pass isValid(). Counting raw faces | ||
| // here would over-shoot the offset and shift later | ||
| // submeshes' HE face indices. | ||
| int valid = 0; | ||
| for (const auto& f : subs[s].faces) { | ||
| if (f.isValid()) ++valid; | ||
| } | ||
| running += valid; | ||
| } | ||
| } | ||
|
|
||
| // Walk the selection, dedup via a small set keyed on HE face idx. | ||
| std::set<int> uniq; | ||
| for (int gi : m_selectedFaces) { | ||
| const auto [subIdx, localTri] = globalTriToLocal(gi); | ||
| if (subIdx >= subs.size()) continue; | ||
| const auto& sub = subs[subIdx]; | ||
| const int faceK = faceIndexForTriangle(sub, localTri, nullptr, nullptr); | ||
| if (faceK >= 0) { | ||
| // n-gon submesh: HE face index = base + faceK. | ||
| uniq.insert(heBaseBySub[subIdx] + faceK); | ||
| } else { | ||
| // Legacy triangle-only submesh: HE face index = base + | ||
| // localTri (one HE face per triangle). | ||
| uniq.insert(heBaseBySub[subIdx] + static_cast<int>(localTri)); | ||
| } |
There was a problem hiding this comment.
Compact invalid faces in the per-face mapping too.
The submesh base offset counts only f.isValid(), but the selected face itself is still encoded as raw faceK. If an invalid face appears earlier in the same sub.faces, every later HE-face index shifts, so face-mode extrude/delete/dissolve/subdivide can hit the wrong face or no-op.
Possible fix
- std::vector<int> heBaseBySub(subs.size(), 0);
+ std::vector<int> heBaseBySub(subs.size(), 0);
+ std::vector<std::vector<int>> heFaceIndexByFace(subs.size());
int running = 0;
for (size_t s = 0; s < subs.size(); ++s) {
heBaseBySub[s] = running;
if (subs[s].faces.empty()) {
running += static_cast<int>(subs[s].triangles.size());
} else {
- int valid = 0;
- for (const auto& f : subs[s].faces) {
- if (f.isValid()) ++valid;
- }
- running += valid;
+ heFaceIndexByFace[s].assign(subs[s].faces.size(), -1);
+ for (size_t fi = 0; fi < subs[s].faces.size(); ++fi) {
+ if (!subs[s].faces[fi].isValid()) continue;
+ heFaceIndexByFace[s][fi] = running++;
+ }
}
}
...
- if (faceK >= 0) {
- uniq.insert(heBaseBySub[subIdx] + faceK);
+ if (faceK >= 0) {
+ const int heFace = heFaceIndexByFace[subIdx][faceK];
+ if (heFace >= 0) uniq.insert(heFace);
} else {
uniq.insert(heBaseBySub[subIdx] + static_cast<int>(localTri));
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/EditModeController.cpp` around lines 860 - 894, The per-submesh HE base
(heBaseBySub) counts only valid faces but when faceIndexForTriangle returns a
raw face index (faceK) you must compact that index to skip invalid faces before
it; otherwise later HE indices are shifted. In the loop over m_selectedFaces
(use globalTriToLocal, faceIndexForTriangle, heBaseBySub, uniq), replace
inserting heBaseBySub[subIdx] + faceK with computing adjustedFaceK = number of f
in subs[subIdx].faces with isValid() and index < faceK, then insert
heBaseBySub[subIdx] + adjustedFaceK; keep the legacy triangle branch unchanged.
Comment on lines
+2941
to
2979
| // Build the CutPoint list, translating OnVertex clicks into edge | ||
| // clicks: pick any incident edge to the vertex and use t=0 or t=1 | ||
| // depending on which endpoint the vertex sits at. `cutPath` only | ||
| // accepts edge inputs (its `splitEdge` primitive is edge-keyed), | ||
| // and `splitEdge` clamps t away from 0/1 by 1e-4 to avoid sliver | ||
| // faces — so the resulting vertex sits a hair off the user's click | ||
| // but topology stays clean. OnFace clicks are still rejected: the | ||
| // commit can't represent them with the current edge-walk algorithm. | ||
| std::vector<HalfEdgeMesh::CutPoint> cpts; | ||
| cpts.reserve(m_knifeSession.points.size()); | ||
| for (const auto& p : m_knifeSession.points) { | ||
| if (p.kind != KnifePoint::OnEdge) { | ||
| if (p.kind == KnifePoint::OnEdge) { | ||
| cpts.push_back({p.edgeIndex, p.edgeT}); | ||
| continue; | ||
| } | ||
| if (p.kind != KnifePoint::OnVertex) { | ||
| SentryReporter::addBreadcrumb("edit_mode", | ||
| "Knife: commit rejected (non-edge point — only edge cuts supported)"); | ||
| "Knife: commit rejected (OnFace point — only edge / vertex supported)"); | ||
| cancelKnife(); | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| std::vector<HalfEdgeMesh::CutPoint> cpts; | ||
| cpts.reserve(m_knifeSession.points.size()); | ||
| for (const auto& p : m_knifeSession.points) { | ||
| cpts.push_back({p.edgeIndex, p.edgeT}); | ||
| // OnVertex → find any incident edge in the triangle-mode HE | ||
| // and pick the t that puts the new split-vertex closest to the | ||
| // clicked vertex. | ||
| int incidentEdge = -1; | ||
| float incidentT = 0.0f; | ||
| for (size_t e = 0; e < hm.edgeCount(); ++e) { | ||
| const auto [ev0, ev1] = hm.edgeVertices(static_cast<int>(e)); | ||
| if (ev0 == p.vertexIndex) { incidentEdge = static_cast<int>(e); incidentT = 0.0f; break; } | ||
| if (ev1 == p.vertexIndex) { incidentEdge = static_cast<int>(e); incidentT = 1.0f; break; } | ||
| } | ||
| if (incidentEdge < 0) { | ||
| SentryReporter::addBreadcrumb("edit_mode", | ||
| "Knife: commit rejected (OnVertex point — no incident edge)"); | ||
| cancelKnife(); | ||
| return false; | ||
| } | ||
| cpts.push_back({incidentEdge, incidentT}); | ||
| } |
There was a problem hiding this comment.
Vertex knife clicks still commit as an arbitrary near-edge split.
An OnVertex click is converted to "first incident edge + t=0/1", and splitEdge() then clamps that away from the endpoint. The preview shows a true vertex hit, but commit creates a near-duplicate point on an arbitrary edge, with iteration-order-dependent results around high-valence corners.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/EditModeController.cpp` around lines 2941 - 2979, The OnVertex handling
picks the first incident edge and exact t=0/1 which splitEdge then clamps,
producing nondeterministic near-edge splits; instead, gather all incident edges
for p.vertexIndex using hm.edgeCount()/edgeVertices, pick a deterministic one
(e.g. smallest edge id) and push a t nudged slightly off the endpoint (use a
small epsilon like 1e-6 or 1-1e-6 depending on whether ev0 or ev1 matched) into
the HalfEdgeMesh::CutPoint so the commit is deterministic and consistently
places the split adjacent to the clicked vertex (update the loop that sets
incidentEdge/incidentT for KnifePoint::OnVertex to implement this deterministic
selection and epsilon nudging).
Comment on lines
+3666
to
+3695
| // Split target faces by arity (chunk 4b): triangles take the | ||
| // 1-to-4 split via `subdivideFaces`, n-gons take the 1-to-N quad | ||
| // split via `subdivideFacesToQuads`. Without this dispatch, the | ||
| // existing `subdivideFaces` would silently skip non-triangles | ||
| // (its triangle-only MVP), making clicks on quads no-op. | ||
| std::vector<int> triFaces, ngonFaces; | ||
| for (int f : targetFaces) { | ||
| if (f < 0 || f >= static_cast<int>(hm.faceCount())) continue; | ||
| const auto verts = hm.faceVertices(f); | ||
| if (verts.size() == 3) triFaces.push_back(f); | ||
| else if (verts.size() > 3) ngonFaces.push_back(f); | ||
| } | ||
|
|
||
| // Capture target positions of the new midpoints BEFORE the mesh is | ||
| // re-packed by toEditableMesh — the indices change after the round | ||
| // trip, so we re-find the survivors by position (matches the merge | ||
| // pipeline pattern). HE-vertex positions are stable across the call. | ||
| const auto newVertHE = hm.subdivideFaces(targetFaces); | ||
| std::vector<int> newVertHE; | ||
| if (!triFaces.empty()) { | ||
| auto v = hm.subdivideFaces(triFaces); | ||
| newVertHE.insert(newVertHE.end(), v.begin(), v.end()); | ||
| } | ||
| if (!ngonFaces.empty()) { | ||
| // subdivideFacesToQuads must run AFTER subdivideFaces because | ||
| // both rebuild the edge tables; running tris first means n-gon | ||
| // edge lookups happen against the post-tri-split topology. | ||
| // Fortunately tri-split doesn't touch n-gon faces. | ||
| auto v = hm.subdivideFacesToQuads(ngonFaces); | ||
| newVertHE.insert(newVertHE.end(), v.begin(), v.end()); | ||
| } |
There was a problem hiding this comment.
This wires a known T-junction-producing path into the public subdivide action.
subdivideFacesToQuads() explicitly leaves adjacent unselected faces untouched, but subdivideSelection() now routes partial n-gon selections into it. That breaks the existing subdivide invariant and will leave T-junctions/cracks along the selection boundary.
Based on learnings: "Subdivide performs 1-to-4 triangle split with adjacent face retriangulation to avoid T-junctions."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/EditModeController.cpp` around lines 3666 - 3695, The current flow calls
hm.subdivideFacesToQuads(...) on a partial n-gon selection which leaves adjacent
unselected faces untouched and produces T-junctions; to fix, detect selection
boundary neighbors and retriangulate adjacent faces (same strategy as
subdivideSelection for triangles) so all edges along the selection perimeter are
split consistently: (1) compute boundary edges/neighbor face IDs from
targetFaces, (2) include those neighbor faces (or their affected triangles) in
the retriangulation set or invoke the existing retriangulation routine after
hm.subdivideFacesToQuads, and (3) preserve the current order (run tri splits
first via hm.subdivideFaces, then run the n-gon quad-split, then run the
adjacency retriangulation step) so subdivideFaces, subdivideFacesToQuads and the
new adjacency-retriangulate pass produce a crack-free result while still
collecting newVertHE.
Comment on lines
+3850
to
+3909
| if (totalMerges == 0) { | ||
| // Promotion alone counts as a meaningful change (downstream | ||
| // n-gon-aware features start working) — but if every submesh | ||
| // already had .faces and no merges happened, this is a true | ||
| // no-op. Detect by comparing face counts pre/post. | ||
| bool topologyChanged = false; | ||
| const auto& cur = m_editableMesh->subMeshes(); | ||
| if (cur.size() != originalSubMeshes.size()) { | ||
| topologyChanged = true; | ||
| } else { | ||
| for (size_t i = 0; i < cur.size(); ++i) { | ||
| if (cur[i].faces.size() != originalSubMeshes[i].faces.size()) { | ||
| topologyChanged = true; | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| globalBase += static_cast<int>(sub.vertices.size()); | ||
| if (!topologyChanged) { | ||
| // Restore — promote was a no-op too. | ||
| m_editableMesh->subMeshes() = std::move(originalSubMeshes); | ||
| return 0; | ||
| } | ||
| } | ||
|
|
||
| if (m_normalsMode == 0) m_editableMesh->recalculateNormals(); | ||
| else m_editableMesh->recalculateNormalsFlat(); | ||
|
|
||
| m_editableMesh->resizeEntityBuffers(m_editEntity); | ||
| rewriteEntityAfterTopologyChange(m_editEntity); | ||
|
|
||
| // Vertex IDs are unchanged by the merge (we only restructured face | ||
| // groupings), so per-vertex selection survives. Edge/face IDs are | ||
| // not stable across the rebuild — clear them. | ||
| m_selectedEdges.clear(); | ||
| m_selectedFaces.clear(); | ||
|
|
||
| auto* cmd = new EditMeshTopologyCommand( | ||
| std::move(originalSubMeshes), | ||
| m_editableMesh->subMeshes(), | ||
| preSelectedVerts, preSelectedEdges, preSelectedFaces, | ||
| m_selectedVertices, m_selectedEdges, m_selectedFaces, | ||
| QStringLiteral("Subdivide")); | ||
| QStringLiteral("Convert to Quads")); | ||
| UndoManager::getSingleton()->push(cmd); | ||
|
|
||
| validateMesh(); | ||
| SentryReporter::addBreadcrumb("edit_mode", | ||
| QString("Subdivide (faces=%1, midpoints=%2)") | ||
| .arg(targetFaces.size()).arg(newVertHE.size())); | ||
| QString("Convert to Quads (merges=%1)").arg(totalMerges)); | ||
|
|
||
| // Wireframe path may have flipped from PM_WIREFRAME to the n-gon | ||
| // boundary overlay (or back, if undo is invoked) — re-apply. | ||
| if (m_wireframeEnabled) { | ||
| removeWireframeMaterials(); | ||
| applyWireframeMaterials(); | ||
| } | ||
|
|
||
| updateSelectionOverlay(); | ||
| refreshNormalVisualizer(); | ||
| emit editSelectionChanged(); | ||
| emit meshDataChanged(); | ||
| return static_cast<int>(targetFaces.size()); | ||
| return totalMerges; |
There was a problem hiding this comment.
Return a success value when promotion changed the representation.
When promoteTrianglesToFaces() is the only effective change, this method still mutates the mesh, pushes undo, and enables the n-gon path, but it returns 0. Any caller treating 0 as no-op/failure will misreport a successful conversion.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/EditModeController.cpp` around lines 3850 - 3909, The method currently
returns totalMerges, which yields 0 when only promoteTrianglesToFaces() changed
representation (no merges) even though the mesh was mutated and undo was pushed;
change the final return to return a non-zero success code when topologyChanged
occurred but totalMerges==0 (e.g. compute a return value like: return
(totalMerges == 0 ? 1 : totalMerges)). Locate the tail of the routine after
UndoManager::getSingleton()->push(cmd) / validateMesh() where the code currently
does "return totalMerges" and replace it with the conditional non-zero return so
callers see a successful conversion when promotion-only changes happened.
Comment on lines
+1056
to
+1058
| // Convert to Quads: whole-mesh; disable once the mesh already | ||
| // has n-gon canonical faces (no work to do). | ||
| convertToQuadsButton->setEnabled(!c->isMeshQuadBased()); |
There was a problem hiding this comment.
Don't disable “Convert to Quads” on mixed meshes.
isMeshQuadBased() flips true as soon as any submesh has canonical faces, so a mixed tri+quad mesh will lose this action even though triangle-only regions can still be merged. A more precise canConvertToQuads() check would be ideal; until then, leaving the action enabled is safer than hiding valid work.
Suggested minimal fix
- convertToQuadsButton->setEnabled(!c->isMeshQuadBased());
+ // `isMeshQuadBased()` is also true for mixed tri+quad meshes.
+ // Prefer a dedicated canConvertToQuads() predicate when available.
+ convertToQuadsButton->setEnabled(true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/mainwindow.cpp` around lines 1056 - 1058, The Convert to Quads button is
being disabled using c->isMeshQuadBased(), which returns true for any submesh
with canonical faces and thus hides the action on mixed tri/quad meshes; change
the logic that sets convertToQuadsButton->setEnabled(...) so the action remains
available for mixed meshes (e.g., always enable the button for now by removing
the isMeshQuadBased() check) and add a TODO to switch to a proper
canConvertToQuads() query when available; update references around
convertToQuadsButton and isMeshQuadBased() accordingly.
CodeRabbit (Major): Ctrl+R fell through to Scale when loopCutSelection short-circuited (e.g. on triangle meshes). Once the user is in edit-mode + edge-mode + has an edge selected, Ctrl+R always means "loop cut" — the no-op case must still consume the key, not silently switch tools. CodeRabbit (Major): editHintMessage routed through statusBar()->showMessage, which frameEnded() rewrites every frame — the 5s hint lasted ~16ms in practice. Use a dedicated permanent QLabel (m_editHintLabel) with a QTimer::singleShot(5000) auto-hide. Italic amber styling distinguishes it from the normal selection-summary slot. Eight other review items are real bugs but pre-existing in feat/quads, not regressions introduced by this merge. Tracked as master follow-ups. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
6 tasks
Owner
Author
|
Pushed two merge-blocker fixes in
The other 8 review items are real bugs but pre-existing in feat/quads (not regressions introduced by this merge). Tracked as follow-up PR work in #326: #326 (comment) Plan: ship this PR once CI is green, then focused follow-ups for chunk 6 (exporter quad preservation) and the bug-fix sweep. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (6)
src/EditModeController.cpp (5)
3863-3922:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReturn success when promotion changed the representation.
This method can promote triangle-only submeshes into
faces, resize buffers, push undo, and refresh wireframe, then still return0becausetotalMergesstayed zero. Any caller treating0as no-op/failure will misreport a successful conversion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/EditModeController.cpp` around lines 3863 - 3922, The code treats totalMerges==0 as a no-op even when a promotion changed the mesh representation; detect when promotion changed (the existing topologyChanged check comparing originalSubMeshes vs m_editableMesh->subMeshes()), carry that information past the early-return branch (e.g. into a bool promotionChanged), and at the final return replace returning totalMerges with something that signals success when promotionChanged is true (for example: return promotionChanged ? 1 : totalMerges). Update references to totalMerges, topologyChanged, originalSubMeshes, and m_editableMesh->subMeshes() accordingly.
751-782:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDeselecting a face still leaves mirrored edge/vertex state behind.
selectFace()now expands intom_selectedEdgesandm_selectedVertices, butdeselectFace()only removes the face entries. Ctrl-deselect will leave stale overlays/state unless you also recompute the mirrored perimeter edges/vertices for the remaining selected faces.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/EditModeController.cpp` around lines 751 - 782, deselectFace currently removes only face IDs from m_selectedFaces, leaving mirrored edge/vertex state (m_selectedEdges, m_selectedVertices) stale; modify deselectFace to, after updating m_selectedFaces (the same triangles loop using faceIndexForTriangle/localTriToGlobal), recompute the perimeter edges and vertices for the remaining selected faces and update m_selectedEdges and m_selectedVertices accordingly (mirror the logic used by selectFace that expands to edges/vertices), then call updateSelectionOverlay() and emit editSelectionChanged() only if any selection actually changed; ensure you reuse any existing helper(s) that compute face perimeter edges/vertices or encapsulate that logic to avoid duplication.
2954-2992:⚠️ Potential issue | 🟠 Major | ⚡ Quick winVertex knife commits are still arbitrary near-edge splits.
OnVertexclicks are converted to the first incident edge witht=0/1, andsplitEdge()then clamps that away from the endpoint. The preview shows a true vertex hit, but commit lands on an iteration-order-dependent adjacent edge instead. Pick a deterministic incident edge and nudgetoff the endpoint before building theCutPoint.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/EditModeController.cpp` around lines 2954 - 2992, The commit currently picks the first incident edge by iteration order for KnifePoint::OnVertex and uses t=0/1 which splitEdge then clamps, causing nondeterministic adjacent-edge splits; instead, gather incident edges via hm.edgeVertices for the clicked vertex (from m_knifeSession.points), deterministically choose one (e.g. the smallest edge index or other stable criterion), and set incidentT to a small nudge off the endpoint (use the same epsilon used by splitEdge, e.g. 1e-4) before pushing a HalfEdgeMesh::CutPoint; update the search that currently loops over hm.edgeCount()/hm.edgeVertices to select the deterministic edge and t, then push cpts.push_back({incidentEdge, incidentT}) and keep the existing error handling (SentryReporter/cancelKnife) unchanged.
843-894:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCompact invalid faces before converting to HalfEdge face indices.
heBaseBySubskips invalidEditableFaces, but the selected face is still inserted as rawfaceK. If an invalid face exists earlier in the samesub.faces, every later HE-face index shifts, so face-mode extrude/delete/dissolve/subdivide can target the wrong face or no-op.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/EditModeController.cpp` around lines 843 - 894, In selectedFacesAsHEFaceIndices(), the code uses faceIndexForTriangle's raw index (faceK) directly even though heBaseBySub was computed by counting only valid EditableFace::isValid() entries; fix by compacting faceK into the index among valid faces before adding the per-submesh base: when faceK >= 0, iterate the submesh's subs[subIdx].faces from start up to the raw faceK and count only those f.isValid() to produce compactFaceIdx, then insert heBaseBySub[subIdx] + compactFaceIdx; keep the legacy-triangle branch (faceK < 0) unchanged. This ensures indices align with how heBaseBySub was computed and prevents off-by-ones when invalid faces exist.
3679-3708:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftPartial n-gon subdivide still leaves T-junctions on the selection boundary.
subdivideFacesToQuads(ngonFaces)only touches the selected n-gons here. Neighboring unselected faces are left unsplit, so partial selections can still create cracks/T-junctions along shared edges unless you add the same adjacency retriangulation/boundary-split step the triangle subdivide path relies on. Based on learnings: "Subdivide performs 1-to-4 triangle split with adjacent face retriangulation to avoid T-junctions."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/EditModeController.cpp` around lines 3679 - 3708, The n-gon path (subdivideFacesToQuads) only splits selected n-gons and leaves adjacent unselected faces intact, causing T-junctions; update the flow to perform the same adjacency retriangulation/boundary-split step used by the triangle path before calling subdivideFacesToQuads: identify boundary edges of ngonFaces, invoke the mesh helper that retriangulates/splits neighboring faces (the same operation triggered by subdivideFaces for triFaces), then run hm.subdivideFacesToQuads(ngonFaces) and collect newVertHE as before so shared edges are consistently split and T-junctions are prevented (refer to functions/variables: subdivideFaces, subdivideFacesToQuads, triFaces, ngonFaces, newVertHE, and hm).src/mainwindow.cpp (1)
1066-1068:⚠️ Potential issue | 🟠 Major | ⚡ Quick winConvert-to-Quads enablement is still too strict on mixed topology
Line 1068 disables the action using
isMeshQuadBased(), which can hide valid tri→quad conversion work on mixed meshes.Suggested minimal fix
- convertToQuadsButton->setEnabled(!c->isMeshQuadBased()); + // TODO: replace with a dedicated canConvertToQuads() predicate. + convertToQuadsButton->setEnabled(true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mainwindow.cpp` around lines 1066 - 1068, The button is incorrectly disabled for mixed-topology meshes because it uses c->isMeshQuadBased(); change the enablement to test for any non-quad faces instead (i.e., enable when there exist triangles or n-gons). Replace the predicate "!c->isMeshQuadBased()" with a check like "c->hasNonQuadFaces()" (or implement inline: iterate faces and return true if face.vertexCount() != 4) so convertToQuadsButton is enabled whenever there are faces that can be converted; update or add the helper method (hasNonQuadFaces / equivalent) near the Mesh/face utilities if it doesn't already exist.
🤖 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/EditModeController.cpp`:
- Around line 2216-2232: The current check uses !sub.faces.empty() to decide
between bevelEdges and bevelEdgesNgon which incorrectly treats triangle-only
Assimp imports as n-gon meshes; instead iterate m_editableMesh->subMeshes() and
inspect each face's vertexCount() (or equivalent face.vertexCount()) and set
meshHasNGons = true only if you find a face with vertexCount() > 3; update all
similar sites (the other occurrences noted) so bevelEdges is chosen for
purely-triangle meshes and bevelEdgesNgon only when an actual >3-vertex face
exists.
In `@src/mainwindow.cpp`:
- Around line 184-189: The current use of QTimer::singleShot in the
editHintMessage lambda can let an earlier timer hide a newer hint; update the
handler for EditModeController::editHintMessage (the lambda that sets
m_editHintLabel) to capture the current msg and schedule hide logic that only
hides when the label still shows that exact msg (or alternatively use a member
QTimer m_editHintTimer: call m_editHintTimer->stop() then start(5000) to restart
the countdown instead of firing multiple singleShot timers). Keep references to
EditModeController::instance(), editHintMessage, m_editHintLabel and the new
m_editHintTimer (if used) so the hide action is tied to the current message.
---
Duplicate comments:
In `@src/EditModeController.cpp`:
- Around line 3863-3922: The code treats totalMerges==0 as a no-op even when a
promotion changed the mesh representation; detect when promotion changed (the
existing topologyChanged check comparing originalSubMeshes vs
m_editableMesh->subMeshes()), carry that information past the early-return
branch (e.g. into a bool promotionChanged), and at the final return replace
returning totalMerges with something that signals success when promotionChanged
is true (for example: return promotionChanged ? 1 : totalMerges). Update
references to totalMerges, topologyChanged, originalSubMeshes, and
m_editableMesh->subMeshes() accordingly.
- Around line 751-782: deselectFace currently removes only face IDs from
m_selectedFaces, leaving mirrored edge/vertex state (m_selectedEdges,
m_selectedVertices) stale; modify deselectFace to, after updating
m_selectedFaces (the same triangles loop using
faceIndexForTriangle/localTriToGlobal), recompute the perimeter edges and
vertices for the remaining selected faces and update m_selectedEdges and
m_selectedVertices accordingly (mirror the logic used by selectFace that expands
to edges/vertices), then call updateSelectionOverlay() and emit
editSelectionChanged() only if any selection actually changed; ensure you reuse
any existing helper(s) that compute face perimeter edges/vertices or encapsulate
that logic to avoid duplication.
- Around line 2954-2992: The commit currently picks the first incident edge by
iteration order for KnifePoint::OnVertex and uses t=0/1 which splitEdge then
clamps, causing nondeterministic adjacent-edge splits; instead, gather incident
edges via hm.edgeVertices for the clicked vertex (from m_knifeSession.points),
deterministically choose one (e.g. the smallest edge index or other stable
criterion), and set incidentT to a small nudge off the endpoint (use the same
epsilon used by splitEdge, e.g. 1e-4) before pushing a HalfEdgeMesh::CutPoint;
update the search that currently loops over hm.edgeCount()/hm.edgeVertices to
select the deterministic edge and t, then push cpts.push_back({incidentEdge,
incidentT}) and keep the existing error handling (SentryReporter/cancelKnife)
unchanged.
- Around line 843-894: In selectedFacesAsHEFaceIndices(), the code uses
faceIndexForTriangle's raw index (faceK) directly even though heBaseBySub was
computed by counting only valid EditableFace::isValid() entries; fix by
compacting faceK into the index among valid faces before adding the per-submesh
base: when faceK >= 0, iterate the submesh's subs[subIdx].faces from start up to
the raw faceK and count only those f.isValid() to produce compactFaceIdx, then
insert heBaseBySub[subIdx] + compactFaceIdx; keep the legacy-triangle branch
(faceK < 0) unchanged. This ensures indices align with how heBaseBySub was
computed and prevents off-by-ones when invalid faces exist.
- Around line 3679-3708: The n-gon path (subdivideFacesToQuads) only splits
selected n-gons and leaves adjacent unselected faces intact, causing
T-junctions; update the flow to perform the same adjacency
retriangulation/boundary-split step used by the triangle path before calling
subdivideFacesToQuads: identify boundary edges of ngonFaces, invoke the mesh
helper that retriangulates/splits neighboring faces (the same operation
triggered by subdivideFaces for triFaces), then run
hm.subdivideFacesToQuads(ngonFaces) and collect newVertHE as before so shared
edges are consistently split and T-junctions are prevented (refer to
functions/variables: subdivideFaces, subdivideFacesToQuads, triFaces, ngonFaces,
newVertHE, and hm).
In `@src/mainwindow.cpp`:
- Around line 1066-1068: The button is incorrectly disabled for mixed-topology
meshes because it uses c->isMeshQuadBased(); change the enablement to test for
any non-quad faces instead (i.e., enable when there exist triangles or n-gons).
Replace the predicate "!c->isMeshQuadBased()" with a check like
"c->hasNonQuadFaces()" (or implement inline: iterate faces and return true if
face.vertexCount() != 4) so convertToQuadsButton is enabled whenever there are
faces that can be converted; update or add the helper method (hasNonQuadFaces /
equivalent) near the Mesh/face utilities if it doesn't already exist.
🪄 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: cbef08a5-c40d-4b62-93af-ae302017b993
📒 Files selected for processing (3)
src/EditModeController.cppsrc/mainwindow.cppsrc/mainwindow.h
Comment on lines
+2216
to
+2232
| // Bevel has two implementations: | ||
| // - `bevelEdges` (triangle-only): the original, with crease / | ||
| // coplanar-sibling detection and segment / profile support. | ||
| // - `bevelEdgesNgon` (n-gon-aware MVP): handles arbitrary face | ||
| // arity but only single-segment flat chamfers. | ||
| // | ||
| // Pick the right one based on whether the editable mesh actually | ||
| // carries n-gon canonicalised faces. Triangle-only meshes | ||
| // (procedural primitives, post-edit re-entries, .scene.glb sub- | ||
| // entities) keep using `bevelEdges` so we don't lose its quality | ||
| // features. Quad-imported meshes (FBX, glTF) get `bevelEdgesNgon`, | ||
| // which produces visible chamfers without the "all-triangulated" | ||
| // workaround that previously triangulated entire submeshes. | ||
| bool meshHasNGons = false; | ||
| for (const auto& sub : m_editableMesh->subMeshes()) { | ||
| if (!sub.faces.empty()) { meshHasNGons = true; break; } | ||
| } |
There was a problem hiding this comment.
!sub.faces.empty() is not a reliable “has n-gons” check.
On the Assimp path, faces becomes the canonical storage even for triangle-only meshes. With this predicate, pure triangle imports are routed through the n-gon bevel/wireframe path, which your own comments describe as lower-featured than the legacy triangle implementation. Gate these branches on an actual vertexCount() > 3 face instead.
Also applies to: 2334-2342, 5032-5038
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/EditModeController.cpp` around lines 2216 - 2232, The current check uses
!sub.faces.empty() to decide between bevelEdges and bevelEdgesNgon which
incorrectly treats triangle-only Assimp imports as n-gon meshes; instead iterate
m_editableMesh->subMeshes() and inspect each face's vertexCount() (or equivalent
face.vertexCount()) and set meshHasNGons = true only if you find a face with
vertexCount() > 3; update all similar sites (the other occurrences noted) so
bevelEdges is chosen for purely-triangle meshes and bevelEdgesNgon only when an
actual >3-vertex face exists.
Comment on lines
+184
to
+189
| connect(EditModeController::instance(), &EditModeController::editHintMessage, | ||
| this, [this](const QString& msg) { | ||
| m_editHintLabel->setText(msg); | ||
| m_editHintLabel->setVisible(true); | ||
| QTimer::singleShot(5000, m_editHintLabel, | ||
| [this]() { m_editHintLabel->setVisible(false); }); |
There was a problem hiding this comment.
Hint auto-hide can clear newer messages too early
Line 188 uses a fresh QTimer::singleShot per message. If hints arrive close together, an older timer can hide a newer hint before its full 5s display window.
Suggested fix
+ m_editHintHideTimer = new QTimer(this);
+ m_editHintHideTimer->setSingleShot(true);
+ connect(m_editHintHideTimer, &QTimer::timeout, this, [this]() {
+ if (m_editHintLabel) m_editHintLabel->setVisible(false);
+ });
+
connect(EditModeController::instance(), &EditModeController::editHintMessage,
this, [this](const QString& msg) {
m_editHintLabel->setText(msg);
m_editHintLabel->setVisible(true);
- QTimer::singleShot(5000, m_editHintLabel,
- [this]() { m_editHintLabel->setVisible(false); });
+ m_editHintHideTimer->start(5000); // restart window for latest hint
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/mainwindow.cpp` around lines 184 - 189, The current use of
QTimer::singleShot in the editHintMessage lambda can let an earlier timer hide a
newer hint; update the handler for EditModeController::editHintMessage (the
lambda that sets m_editHintLabel) to capture the current msg and schedule hide
logic that only hides when the label still shows that exact msg (or
alternatively use a member QTimer m_editHintTimer: call m_editHintTimer->stop()
then start(5000) to restart the countdown instead of firing multiple singleShot
timers). Keep references to EditModeController::instance(), editHintMessage,
m_editHintLabel and the new m_editHintTimer (if used) so the hide action is tied
to the current message.
|
4 tasks
11 tasks
fernandotonon
added a commit
that referenced
this pull request
Apr 30, 2026
Address the deferred items from PR #347 review (#326 follow-up). Eight discrete fixes; the remaining two (vertex-knife near-duplicate on OnVertex hits, subdivideFacesToQuads T-junctions) are heavier lifts deferred to a separate PR. 1. HalfEdgeMesh::loopCut rejects mixed quad/tri adjacency upfront. Previously walked f1 and f2 independently, so a quad+tri start edge produced a one-sided cut on the quad side and silently mutated topology. (Codex P1.) +1 unit test. 2. applyWireframeMaterials handles mixed meshes per submesh. Tri-only submeshes inside a mesh that had ANY n-gon submesh were losing wireframe entirely (boundary overlay only emits n-gon submeshes). Now PM_WIREFRAME on tri-only submeshes coexists with the boundary overlay on n-gon submeshes. (Codex P2.) 3. buildSubMeshBuffers explicitly clears the GPU vertex/index buffers when the submesh has no vertices or no triangles to draw. Prior early-return left stale buffers attached, so deleting the last face in a submesh kept the old geometry rendering forever. 4. selectedFacesAsHEFaceIndices compacts invalid faces in the per-face mapping, not just the per-submesh base offset. Without this, a selected face whose raw index is shifted by an earlier invalid face mapped to the wrong HE face — silently breaking face-mode extrude/delete/dissolve/subdivide. 5. canConvertToQuads predicate replaces !isMeshQuadBased on the toolbar gate. Mixed meshes (some submeshes quad, some tri) still have tri-only submeshes worth merging — the previous check wrongly disabled the action. 6. convertToQuads correctly returns non-zero on promote-only runs. Tracks promotion count separately from merge count, returns their sum. Previous return-of-totalMerges falsely reported 0 when only the n-gon promotion happened. 7. deselectFace mirrors selectFace's vertex/edge dilation. Without this, ctrl-deselect erased the face triangles but left perimeter vertex/edge entries — looked like a stuck partial selection. 8. Test in EditModeControllerBevelE2E uses a real (0.001f) translate instead of Vector3::ZERO so the commit-path assertion stays meaningful even if the controller ever short-circuits zero deltas. Two items remain for a follow-up PR: - Vertex knife OnVertex clicks producing near-duplicate points (the click is converted to first-incident-edge + t=0/1 then splitEdge clamps it; need a different splitFace path for true vertex hits). - subdivideFacesToQuads T-junctions on partial n-gon selections (needs adjacent-face retriangulation pass like the existing tri subdivide does). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
fernandotonon
added a commit
that referenced
this pull request
Apr 30, 2026
Address the deferred items from PR #347 review (#326 follow-up). Eight discrete fixes; the remaining two (vertex-knife near-duplicate on OnVertex hits, subdivideFacesToQuads T-junctions) are heavier lifts deferred to a separate PR. 1. HalfEdgeMesh::loopCut rejects mixed quad/tri adjacency upfront. Previously walked f1 and f2 independently, so a quad+tri start edge produced a one-sided cut on the quad side and silently mutated topology. (Codex P1.) +1 unit test. 2. applyWireframeMaterials handles mixed meshes per submesh. Tri-only submeshes inside a mesh that had ANY n-gon submesh were losing wireframe entirely (boundary overlay only emits n-gon submeshes). Now PM_WIREFRAME on tri-only submeshes coexists with the boundary overlay on n-gon submeshes. (Codex P2.) 3. buildSubMeshBuffers explicitly clears the GPU vertex/index buffers when the submesh has no vertices or no triangles to draw. Prior early-return left stale buffers attached, so deleting the last face in a submesh kept the old geometry rendering forever. 4. selectedFacesAsHEFaceIndices compacts invalid faces in the per-face mapping, not just the per-submesh base offset. Without this, a selected face whose raw index is shifted by an earlier invalid face mapped to the wrong HE face — silently breaking face-mode extrude/delete/dissolve/subdivide. 5. canConvertToQuads predicate replaces !isMeshQuadBased on the toolbar gate. Mixed meshes (some submeshes quad, some tri) still have tri-only submeshes worth merging — the previous check wrongly disabled the action. 6. convertToQuads correctly returns non-zero on promote-only runs. Tracks promotion count separately from merge count, returns their sum. Previous return-of-totalMerges falsely reported 0 when only the n-gon promotion happened. 7. deselectFace mirrors selectFace's vertex/edge dilation. Without this, ctrl-deselect erased the face triangles but left perimeter vertex/edge entries — looked like a stuck partial selection. 8. Test in EditModeControllerBevelE2E uses a real (0.001f) translate instead of Vector3::ZERO so the commit-path assertion stays meaningful even if the controller ever short-circuits zero deltas. Two items remain for a follow-up PR: - Vertex knife OnVertex clicks producing near-duplicate points (the click is converted to first-incident-edge + t=0/1 then splitEdge clamps it; need a different splitFace path for true vertex hits). - subdivideFacesToQuads T-junctions on partial n-gon selections (needs adjacent-face retriangulation pass like the existing tri subdivide does). Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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 quad-mesh epic foundation (#326 chunks 1-5 + 7). Migrates the editor from a triangle-only internal representation to an n-gon-aware one, with quads as the primary case but the data model supporting any polygon ≥ 3 vertices. Loop cut, n-gon-aware bevel, Catmull-Clark subdivision, and a tris→quads converter all ship together.
Deferred to follow-up PRs (on master):
What's in this PR
Foundation (chunks 1–3, all merged into feat/quads)
EditableFacedata model —EditableSubMesh::facesn-gon storage that coexists withtriangles. Round-trip tested through HalfEdgeMesh.EditableMesh::loadFromAssimpFilere-imports the source file withaiProcess_Triangulatedisabled so source quads survive. Source path cached on the Ogre mesh.Edit Mode wiring (chunks 4 + 5)
enterEditModeuses the n-gon import path when available (gated by a single-bit "modified" flag on the cached source path so re-imports never blow away prior edits).splitEdge,dissolve(edges + verts), merge cleanup, fill, knife — every topology op now traverses n-gons natively.bevelEdgesNgon) and vertex bevel (bevelVerticesNgon), with profile-bulge controls and chamfer winding fixes.Loop cut (chunk 7)
Ctrl+Rand toolbar ‖ button. Walks the perpendicular ring of quads via opposite-edge correspondence; closes on closed manifolds. Quad-only by design (matches Blender) — surfaces a status hint when triggered on a triangle mesh.Convert to Quads + quad-aware wireframe
mergeCoplanarTrianglesToQuads). Auto-disables once the mesh is already quad-based. The wireframe overlay now draws lines along n-gon face boundaries only — no fan-triangulation diagonals — when the mesh has.faces. Pure-tri meshes keep PM_WIREFRAME so behavior is unchanged.Acceptance criteria
Ctrl+RTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
UI Enhancements
Bug Fixes
Tests