feat(edit): merge vertices (Phase 4 topology op) - #312
Merged
Conversation
Implements the four standard variants from Blender:
• Merge At Center — selection collapses to centroid
• Merge At First — collapses to lowest-index selected vertex
• Merge At Last — collapses to highest-index selected vertex
• Merge By Distance — auto-fuse vertex pairs within threshold (1e-4
world units default), each cluster collapses
to its centroid
Core HE primitive `HalfEdgeMesh::mergeVertices(verts, targetPos)`:
- Picks `verts[0]` as survivor, retires the rest
- Re-points half-edges that referenced doomed verts
- Retires triangles that become degenerate after the rewrite
- Retires triangles that become *duplicates* of an earlier survivor
(same {submesh, sorted-vert-set} key) — common after pair-merging
- Refuses cross-submesh merges to preserve UV seams + material groups
- Standard rebuild trio: edges/twins → compact boundaries → fix vertex
half-edge pointers (mirrors splitFace / extrude pattern)
`mergeVerticesByDistance` is union-find by spatial proximity (O(N²) on
the candidate set, fine for typical UI selection sizes).
Controller surface (`EditModeController`):
- `mergeAtCenter / mergeAtFirst / mergeAtLast / mergeByDistance`
- All four push one `EditMeshTopologyCommand` so undo/redo is one step
- All four refuse outside vertex mode or with <2 verts selected
- Shared `rewriteEntityAfterTopologyChange` helper (extracted from the
existing knife commit path) handles Ogre buffer resize + material
preservation + RTSS shader invalidation
UI:
- Toolbar button (⨀ glyph) with a four-item dropdown menu, placed next
to the existing Knife scissor button
- Visibility tied to edit mode; enabled only when ≥2 verts selected in
vertex mode
- M key shortcut runs Merge At Center directly (the headline path
from the issue spec); other targets via the dropdown
Tests (HalfEdgeMesh_test.cpp):
- Less-than-two inputs is a no-op
- Merging both ends of a shared edge retires both adjacent triangles
- Non-adjacent merge on a tri-strip leaves all faces alive
- By-distance fuses near-coincident vertex pairs, collapses dup tris
- Cross-submesh merge is refused
- Tight by-distance threshold ignores well-separated verts
All 117 active HalfEdgeMesh tests pass on macOS; full suite has the
same 6 pre-existing macOS-Ogre-init failures as master baseline.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (28)
📝 WalkthroughWalkthroughAdds four vertex-merge operations (at center, first, last, by-distance), implements half-edge merge algorithms and clustering, integrates controller undo/mesh-rebuild flows including Ogre entity/material rewrite, exposes UI toolbar/menu/shortcut, and adds unit tests for merge correctness. Changes
Sequence DiagramsequenceDiagram
participant User
participant UI as MainWindow
participant Controller as EditModeController
participant Topology as HalfEdgeMesh
participant Renderer as Ogre Entity
User->>UI: Trigger merge (menu / M key)
UI->>Controller: mergeAtCenter() / mergeByDistance(threshold)
Controller->>Controller: Validate edit mode, VertexMode, selection
Controller->>Topology: Build from EditableMesh
Controller->>Topology: mergeVertices(...) or mergeVerticesByDistance(...)
Topology->>Topology: Rewire half-edges, remove degenerate faces, rebuild topology
Topology-->>Controller: Return retired vertex count
Controller->>Controller: Convert to EditableMesh, clear selections
Controller->>Renderer: Resize/rebuild buffers, rewrite entity, preserve subentity materials, invalidate RTShader materials
Controller->>Controller: Push EditMeshTopologyCommand (undo), emit mesh/selection signals
Controller->>UI: Notify mesh/selection changed
UI-->>User: View updated mesh
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: a13a2487c1
ℹ️ 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
+2580
to
+2583
| const Ogre::Vector3 target = hm.vertex(verts.back()).position; | ||
|
|
||
| auto originalSubMeshes = m_editableMesh->subMeshes(); | ||
| const int retired = hm.mergeVertices(verts, target); |
There was a problem hiding this comment.
mergeAtLast() computes the target position from verts.back(), but it still passes the original sorted verts vector into mergeVertices(), which uses vertexIndices[0] as the survivor. This means the lowest-index vertex survives instead of the highest-index one, so "At Last" does not actually preserve the last vertex’s attributes/identity (UVs, normals, skin weights), despite the function contract and inline comment indicating it should.
Useful? React with 👍 / 👎.
Comment on lines
+2510
to
+2514
| auto* cmd = new EditMeshTopologyCommand( | ||
| std::move(originalSubMeshes), | ||
| m_editableMesh->subMeshes(), | ||
| m_selectedVertices, m_selectedEdges, m_selectedFaces, | ||
| m_selectedVertices, m_selectedEdges, m_selectedFaces, |
There was a problem hiding this comment.
Each merge path clears m_selectedVertices/m_selectedEdges/m_selectedFaces and then constructs EditMeshTopologyCommand using those cleared sets for both old and new selection snapshots. As a result, undoing a merge restores mesh topology but not the pre-merge selection, which breaks the command’s selection state restoration behavior (and differs from other topology commands that preserve old selection).
Useful? React with 👍 / 👎.
Comment on lines
+4175
to
+4177
| if (pa.squaredDistance(pb) <= t2) | ||
| unite(static_cast<int>(i), static_cast<int>(j)); | ||
| } |
There was a problem hiding this comment.
mergeVerticesByDistance() unions vertices purely by distance, then calls mergeVertices() per cluster. If a cluster contains vertices from multiple submeshes, mergeVertices() rejects the whole cluster (mergeSubs.size() > 1), which also drops valid same-submesh merges inside that cluster. This contradicts the API comment that cross-submesh pairs are skipped; currently they can suppress unrelated in-submesh merges.
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: 4
🧹 Nitpick comments (6)
src/HalfEdgeMesh.cpp (4)
4067-4070: Comment is self-contradictory."sub-mesh agnostic — same trio in the same submesh is a dup" reads as if submesh is ignored, but
canonicalKey(line 4086-4091) includessubIdxin the tuple. You likely meant winding-agnostic (the vertex triple is sorted before keying) and same-submesh-only. Suggest:- // up with identical vertex sets. We compare unordered vertex sets - // (sub-mesh agnostic — same trio in the same submesh is a dup). + // up with identical vertex sets. Keys are (submesh, sorted-verts), so + // we ignore winding but require submesh agreement before declaring a + // duplicate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/HalfEdgeMesh.cpp` around lines 4067 - 4070, The comment is contradictory about submesh handling; either update the code or the comment: if the intent is to treat triangles as identical only within the same submesh (as implemented by the tuple that includes subIdx in canonicalKey), change the comment to say "winding-agnostic and same-submesh-only" (i.e., the vertex triple is sorted but submesh is respected); alternatively, if you truly meant sub-mesh agnostic, remove subIdx from the tuple construction around canonicalKey so duplicates are detected across submeshes. Locate the tuple construction named canonicalKey and adjust the comment text or the tuple contents accordingly.
4094-4116: Duplicate-face dedup scans the entirem_facesarray — pre-existing duplicates unrelated to the merge get silently retired.The dedup loop walks every face, not just faces incident to the merge set. If the input mesh already contains two same-submesh triangles with identical sorted vertex triples (rare, but possible after prior topology ops or imports of malformed meshes), the second one will be retired as a side effect of any
mergeVerticescall — even when the merge itself didn't touch those faces.Limiting the scan to faces that actually contained a doomed vertex pre-rewrite would make the operation strictly local. You can collect the candidate face set in step 3 while iterating half-edges (push
m_halfEdges[i].faceinto a smallstd::unordered_set<int>wheneverdoomed.count(he.vertex)), then run the degenerate/duplicate sweep only over that set.Sketch
- for (auto& he : m_halfEdges) { - if (he.face < 0) continue; // skip retired / boundary HEs - if (doomed.count(he.vertex)) - he.vertex = survivor; - } + std::unordered_set<int> touchedFaces; + for (auto& he : m_halfEdges) { + if (he.face < 0) continue; + if (doomed.count(he.vertex)) { + he.vertex = survivor; + touchedFaces.insert(he.face); + } + } @@ - for (int f = 0; f < static_cast<int>(m_faces.size()); ++f) { + for (int f : touchedFaces) { int startHE = m_faces[f].halfEdge; if (startHE < 0) continue;Low priority since a well-formed mesh will never trip this, but it makes the operation's blast radius easier to reason about.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/HalfEdgeMesh.cpp` around lines 4094 - 4116, The dedup loop currently scans all m_faces and can retire unrelated pre-existing duplicate triangles; instead restrict the sweep to faces actually affected by the vertex merge: during the half-edge iteration step where you check doomed vertices (the same place you examine m_halfEdges[i].vertex and the existing doomed set used by mergeVertices), collect m_halfEdges[i].face into a small std::unordered_set<int> candidateFaces; then replace the for-loop over all faces (the block using faceVertices, canonicalKey, retireFace, degenerate checks on m_faces[f]) with a loop over candidateFaces so only faces incident to doomed vertices are tested and potentially retired. Ensure you still skip invalid faces (startHE < 0) and use the same canonicalKey/retireFace/faceVertices helpers.
4054-4062: Survivor's vertex attributes aren't blended towardtargetPos— Merge-At-Center may show shading discontinuity.For Merge At Center,
targetPosis the centroid of the cluster, but onlypositionis overwritten.m_vertices[survivor]'snormal,uv,tangent,color, andboneAssignmentsremain those of the first selected vertex. After collapse, the merged vertex sits at the centroid but its normal points in the original direction, which can produce visible shading kinks (and skinning artifacts on rigged meshes) when the cluster spans a curved/seam region.Two reasonable options:
- Have
mergeVerticesaccept the full target attributes (or a per-attribute blend mode), pushing the choice up to the caller (controller knows whether it's center / first / last).- Compute a centroid-blend of attributes inside
mergeVerticeswhentargetPos != m_vertices[survivor].position(i.e., infer "center-style" merge).The "first/last" flavors (where targetPos coincides with one input vertex) keep the current behavior naturally; only the center variant is affected.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/HalfEdgeMesh.cpp` around lines 4054 - 4062, The merge step only updates m_vertices[survivor].position to targetPos but leaves other vertex attributes (normal, uv, tangent, color, boneAssignments) unchanged, causing shading/skin artifacts for Merge-At-Center; update mergeVertices to either accept full target attributes from the caller or compute centroid blends when targetPos differs from the survivor position: gather attribute-weighted centroids from doomed+survivor, set m_vertices[survivor].normal (and re-normalize), uv, tangent, color, and blend boneAssignments accordingly before the loop that re-points half-edges (the symbols to modify are mergeVertices, targetPos, m_vertices[survivor], and the doomed set/loop over m_halfEdges).
4159-4168:std::functionis unnecessary here — a plain lambda will avoid the type-erasure allocation.
findis iterative (no recursive self-call), so it doesn't needstd::functionfor self-reference. Replacing withautoremoves a heap allocation per call site and lets the compiler inline.- std::function<int(int)> find = [&](int i) { - while (parent[i] != i) { parent[i] = parent[parent[i]]; i = parent[i]; } - return i; - }; + auto find = [&](int i) { + while (parent[i] != i) { parent[i] = parent[parent[i]]; i = parent[i]; } + return i; + };(With this change, the
<functional>include is still needed elsewhere in the file, so no other adjustments required.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/HalfEdgeMesh.cpp` around lines 4159 - 4168, Replace the heap-allocated std::function wrapper for the finder with a plain lambda: change the declaration of find from "std::function<int(int)> find = [&](int i) { ... };" to "auto find = [&](int i) -> int { ... };" so unite can still call find and parent path compression remains unchanged; no other logic changes needed.src/EditModeController.cpp (2)
2442-2444: Two stale/misleading comments.
- Line 2442:
applyMergeOp factors that boilerplate.— there is noapplyMergeOpin this file; the boilerplate is open-coded in each of the four functions. Either drop the reference or land the helper (see the de-dup comment).- Lines 2577–2579 in
mergeAtLast: "move the desired anchor to the front and re-target manually" — the code does not reorderverts; it only passesverts.back()'s position as the target. The survivor is stillverts[0](lowest index) per the documentedmergeVerticescontract, just placed at the highest-index vertex's coordinates. Worth tightening so future readers don't expect a reordering that isn't happening.Also applies to: 2577-2580
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/EditModeController.cpp` around lines 2442 - 2444, The comments are stale/misleading: remove or update the reference to a non-existent helper "applyMergeOp" in the namespace comment and either implement that helper to factor the duplicated boilerplate used across the four merge functions or drop the mention; and in mergeAtLast update the comment about reordering so it accurately states that mergeVertices retains verts[0] as the survivor and only uses verts.back()’s position as the target (no reordering of the verts array is performed). Locate symbols applyMergeOp (comment mention), mergeAtLast, mergeVertices, and verts to apply these changes.
2475-2651: De-duplicate the four merge entry points.
mergeAtCenter,mergeAtFirst,mergeAtLast, andmergeByDistanceare ~95% identical — only the choice of HE call (mergeVerticesvsmergeVerticesByDistance) and the target-position computation differ. The block comment at line 2442 even references anapplyMergeOphelper that doesn't actually exist in this diff. Extracting it would make the undo/normals/refresh fix above land in one place instead of four (and avoids the four near-identicalEditMeshTopologyCommandconstructions silently drifting apart later).♻️ Sketch — single helper, four thin wrappers
// In the anonymous namespace alongside rewriteEntityAfterTopologyChange: struct MergeResult { int retired = 0; }; int EditModeController::applyMergeOp( const char* label, const std::function<int(HalfEdgeMesh&, const std::vector<int>&)>& runMerge) { if (!m_editModeActive || !m_editableMesh || !m_editEntity) return 0; if (m_selectionMode != VertexMode) return 0; if (m_selectedVertices.size() < 2) return 0; HalfEdgeMesh hm; if (!hm.buildFromEditableMesh(*m_editableMesh)) return 0; std::vector<int> verts(m_selectedVertices.begin(), m_selectedVertices.end()); auto oldSelV = m_selectedVertices; auto oldSelE = m_selectedEdges; auto oldSelF = m_selectedFaces; auto originalSubMeshes = m_editableMesh->subMeshes(); const int retired = runMerge(hm, verts); if (retired == 0) return 0; EditableMesh updated; if (!hm.toEditableMesh(updated)) return 0; m_editableMesh->subMeshes() = std::move(updated.subMeshes()); if (m_normalsMode == 0) m_editableMesh->recalculateNormals(); else m_editableMesh->recalculateNormalsFlat(); m_editableMesh->resizeEntityBuffers(m_editEntity); rewriteEntityAfterTopologyChange(m_editEntity); m_selectedVertices.clear(); m_selectedEdges.clear(); m_selectedFaces.clear(); UndoManager::getSingleton()->push(new EditMeshTopologyCommand( std::move(originalSubMeshes), m_editableMesh->subMeshes(), oldSelV, oldSelE, oldSelF, m_selectedVertices, m_selectedEdges, m_selectedFaces, label)); SentryReporter::addBreadcrumb("edit_mode", QString("Merge: %1 (removed=%2)").arg(label).arg(retired)); refreshNormalVisualizer(); updateSelectionOverlay(); validateMesh(); emit editSelectionChanged(); emit meshDataChanged(); return retired; } int EditModeController::mergeAtCenter() { return applyMergeOp("Merge At Center", [](HalfEdgeMesh& hm, const std::vector<int>& verts) { Ogre::Vector3 c = Ogre::Vector3::ZERO; for (int v : verts) c += hm.vertex(v).position; c /= static_cast<float>(verts.size()); return hm.mergeVertices(verts, c); }); } // ...mergeAtFirst / mergeAtLast / mergeByDistance similarly thin.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/EditModeController.cpp` around lines 2475 - 2651, The four nearly-identical methods mergeAtCenter, mergeAtFirst, mergeAtLast and mergeByDistance should delegate their common pre/post-work to a new helper (e.g. applyMergeOp) that builds the HalfEdgeMesh, snapshots originalSubMeshes and old selections, runs a provided merge lambda (taking HalfEdgeMesh& and verts vector), converts back to EditableMesh, recalculates normals, resizes buffers, calls rewriteEntityAfterTopologyChange, clears selections, constructs a single consistent EditMeshTopologyCommand, pushes it via UndoManager, emits SentryReporter breadcrumb and mesh/selection signals, and returns retired; then make each merge* method a thin wrapper that computes the target (center/first/last/threshold) and calls applyMergeOp with the appropriate lambda (or calls mergeVerticesByDistance). Reference symbols: applyMergeOp, mergeAtCenter, mergeAtFirst, mergeAtLast, mergeByDistance, HalfEdgeMesh, m_editableMesh, m_selectedVertices, originalSubMeshes, EditMeshTopologyCommand, rewriteEntityAfterTopologyChange, UndoManager, SentryReporter.
🤖 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 2510-2516: The merge commands construct EditMeshTopologyCommand
after clearing the controller's selection members so the command's old*
selection tuples receive empty sets; to fix, capture copies of the pre-merge
selections (e.g., create local variables like origSelectedVertices =
m_selectedVertices, origSelectedEdges = m_selectedEdges, origSelectedFaces =
m_selectedFaces) and pass those copies as the old* arguments to
EditMeshTopologyCommand (the constructor call shown) before you clear
m_selectedVertices/m_selectedEdges/m_selectedFaces, and apply the same change to
the other merge helpers (mergeAtFirst, mergeAtLast, mergeByDistance) so undo
restores the original selections.
- Around line 2496-2522: After writing back the new submeshes in each merge
operation (mergeAtCenter, mergeAtFirst, mergeAtLast, mergeByDistance) you must
recompute normals and refresh post-op visual/validation state the same way other
topology ops do: after rewriteEntityAfterTopologyChange(m_editEntity) call the
appropriate normal recompute (recalculateNormals() or recalculateNormalsFlat()
as used by bevel ops for the survivor and its 1‑ring), then call
refreshNormalVisualizer(), updateSelectionOverlay(), and validateMesh() before
creating/pushing the EditMeshTopologyCommand and emitting
editSelectionChanged()/meshDataChanged(); do this in the same location in each
merge function so normals, normal visualizer, selection overlay and
m_degenerateTriangleCount are up-to-date.
In `@src/HalfEdgeMesh.cpp`:
- Around line 4170-4178: Clamp the incoming threshold to a safe range before
squaring to avoid overflow: validate the variable named threshold (used to
compute t2) and cap it to a reasonable maximum (and non-negative minimum) prior
to computing t2 = threshold * threshold; then use t2 in the existing comparison
with m_vertices[*].position.squaredDistance(...) and call unite(...) as before.
Update the code around the loop that iterates over alive, referencing the
variables threshold, t2, pa/pb, squaredDistance, and unite to ensure the clamp
is applied once before the nested loops.
In `@src/HalfEdgeMesh.h`:
- Around line 484-501: The header declares mergeVerticesByDistance(world-space
threshold) but the implementation compares against local vertex positions;
update the call site in EditModeController::mergeByDistance to convert the
world-space threshold to local space before calling
HalfEdgeMesh::mergeVerticesByDistance by dividing the provided threshold by the
entity's scale (e.g., compute localThreshold = worldThreshold / entityScale) so
the distance comparison uses consistent units; alternatively, if you prefer
changing the contract, update the HalfEdgeMesh::mergeVerticesByDistance
documentation to state the threshold is in local space and keep callers
unchanged—pick one consistent approach and adjust callers or docs accordingly.
---
Nitpick comments:
In `@src/EditModeController.cpp`:
- Around line 2442-2444: The comments are stale/misleading: remove or update the
reference to a non-existent helper "applyMergeOp" in the namespace comment and
either implement that helper to factor the duplicated boilerplate used across
the four merge functions or drop the mention; and in mergeAtLast update the
comment about reordering so it accurately states that mergeVertices retains
verts[0] as the survivor and only uses verts.back()’s position as the target (no
reordering of the verts array is performed). Locate symbols applyMergeOp
(comment mention), mergeAtLast, mergeVertices, and verts to apply these changes.
- Around line 2475-2651: The four nearly-identical methods mergeAtCenter,
mergeAtFirst, mergeAtLast and mergeByDistance should delegate their common
pre/post-work to a new helper (e.g. applyMergeOp) that builds the HalfEdgeMesh,
snapshots originalSubMeshes and old selections, runs a provided merge lambda
(taking HalfEdgeMesh& and verts vector), converts back to EditableMesh,
recalculates normals, resizes buffers, calls rewriteEntityAfterTopologyChange,
clears selections, constructs a single consistent EditMeshTopologyCommand,
pushes it via UndoManager, emits SentryReporter breadcrumb and mesh/selection
signals, and returns retired; then make each merge* method a thin wrapper that
computes the target (center/first/last/threshold) and calls applyMergeOp with
the appropriate lambda (or calls mergeVerticesByDistance). Reference symbols:
applyMergeOp, mergeAtCenter, mergeAtFirst, mergeAtLast, mergeByDistance,
HalfEdgeMesh, m_editableMesh, m_selectedVertices, originalSubMeshes,
EditMeshTopologyCommand, rewriteEntityAfterTopologyChange, UndoManager,
SentryReporter.
In `@src/HalfEdgeMesh.cpp`:
- Around line 4067-4070: The comment is contradictory about submesh handling;
either update the code or the comment: if the intent is to treat triangles as
identical only within the same submesh (as implemented by the tuple that
includes subIdx in canonicalKey), change the comment to say "winding-agnostic
and same-submesh-only" (i.e., the vertex triple is sorted but submesh is
respected); alternatively, if you truly meant sub-mesh agnostic, remove subIdx
from the tuple construction around canonicalKey so duplicates are detected
across submeshes. Locate the tuple construction named canonicalKey and adjust
the comment text or the tuple contents accordingly.
- Around line 4094-4116: The dedup loop currently scans all m_faces and can
retire unrelated pre-existing duplicate triangles; instead restrict the sweep to
faces actually affected by the vertex merge: during the half-edge iteration step
where you check doomed vertices (the same place you examine
m_halfEdges[i].vertex and the existing doomed set used by mergeVertices),
collect m_halfEdges[i].face into a small std::unordered_set<int> candidateFaces;
then replace the for-loop over all faces (the block using faceVertices,
canonicalKey, retireFace, degenerate checks on m_faces[f]) with a loop over
candidateFaces so only faces incident to doomed vertices are tested and
potentially retired. Ensure you still skip invalid faces (startHE < 0) and use
the same canonicalKey/retireFace/faceVertices helpers.
- Around line 4054-4062: The merge step only updates
m_vertices[survivor].position to targetPos but leaves other vertex attributes
(normal, uv, tangent, color, boneAssignments) unchanged, causing shading/skin
artifacts for Merge-At-Center; update mergeVertices to either accept full target
attributes from the caller or compute centroid blends when targetPos differs
from the survivor position: gather attribute-weighted centroids from
doomed+survivor, set m_vertices[survivor].normal (and re-normalize), uv,
tangent, color, and blend boneAssignments accordingly before the loop that
re-points half-edges (the symbols to modify are mergeVertices, targetPos,
m_vertices[survivor], and the doomed set/loop over m_halfEdges).
- Around line 4159-4168: Replace the heap-allocated std::function wrapper for
the finder with a plain lambda: change the declaration of find from
"std::function<int(int)> find = [&](int i) { ... };" to "auto find = [&](int i)
-> int { ... };" so unite can still call find and parent path compression
remains unchanged; no other logic changes needed.
🪄 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: 3610e8e7-aab0-460e-919f-6ae9520b458d
📒 Files selected for processing (6)
src/EditModeController.cppsrc/EditModeController.hsrc/HalfEdgeMesh.cppsrc/HalfEdgeMesh.hsrc/HalfEdgeMesh_test.cppsrc/mainwindow.cpp
Comment on lines
+484
to
+501
| /** | ||
| * @brief Find pairs of vertices within `threshold` of each other and | ||
| * merge each cluster to its centroid. Operates only on the | ||
| * provided candidate set (so the caller controls scope — | ||
| * usually the current selection). Cross-submesh pairs are | ||
| * skipped, same as `mergeVertices`. | ||
| * | ||
| * Implementation: union-find by spatial proximity. A vertex landing | ||
| * in two clusters joins both; the merged cluster collapses to the | ||
| * combined centroid. | ||
| * | ||
| * @param vertexIndices Candidate vertices. | ||
| * @param threshold World-space distance under which a pair fuses. | ||
| * Defaults to 1e-4 (≈0.1 mm at meter scale). | ||
| * @return Number of vertices retired across all clusters. | ||
| */ | ||
| int mergeVerticesByDistance(const std::vector<int>& vertexIndices, | ||
| float threshold = 1e-4f); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
EDIT_CPP="$(fd '^EditModeController\.cpp$' src | head -n1)"
HE_CPP="$(fd '^HalfEdgeMesh\.cpp$' src | head -n1)"
echo "EditModeController.cpp: ${EDIT_CPP:-NOT FOUND}"
echo "HalfEdgeMesh.cpp: ${HE_CPP:-NOT FOUND}"
if [[ -n "${EDIT_CPP}" ]]; then
echo
echo "--- EditModeController::mergeByDistance ---"
rg -n -C4 '\bEditModeController::mergeByDistance\s*\(' "$EDIT_CPP"
echo
echo "--- Possible world/local conversion nearby ---"
rg -n -C3 '_getDerived|convert.*World|world.*position|scale|local.*threshold|threshold.*local' "$EDIT_CPP"
fi
if [[ -n "${HE_CPP}" ]]; then
echo
echo "--- HalfEdgeMesh::mergeVerticesByDistance ---"
rg -n -C6 '\bHalfEdgeMesh::mergeVerticesByDistance\s*\(' "$HE_CPP"
fiRepository: fernandotonon/QtMeshEditor
Length of output: 6815
🏁 Script executed:
#!/bin/bash
set -euo pipefail
EDIT_CPP="src/EditModeController.cpp"
HE_CPP="src/HalfEdgeMesh.cpp"
echo "=== EditModeController::mergeByDistance (full function) ==="
# Get line 2611 and the next 30 lines to see the full implementation
sed -n '2611,2650p' "$EDIT_CPP"
echo
echo "=== HalfEdgeMesh::mergeVerticesByDistance (full function start) ==="
# Get line 4136 and the next 40 lines to see the full implementation
sed -n '4136,4180p' "$HE_CPP"
echo
echo "=== Check for entity scale access in mergeByDistance context ==="
# Look for any scale-related code around mergeByDistance
sed -n '2611,2650p' "$EDIT_CPP" | grep -i 'scale\|derive\|world\|entity'Repository: fernandotonon/QtMeshEditor
Length of output: 3839
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get the header documentation around mergeVerticesByDistance
sed -n '475,510p' src/HalfEdgeMesh.hRepository: fernandotonon/QtMeshEditor
Length of output: 1643
Fix the world-space to local-space threshold conversion for scaled entities.
The header documents threshold as world-space, but HalfEdgeMesh::mergeVerticesByDistance() compares it directly against local vertex positions without scaling. When the entity is scaled (e.g., 2.0×), the actual merge distance will be incorrect. Either change the contract to explicitly document the threshold as local-space, or convert the threshold in EditModeController::mergeByDistance() by dividing it by the entity's scale before passing it to HalfEdgeMesh.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/HalfEdgeMesh.h` around lines 484 - 501, The header declares
mergeVerticesByDistance(world-space threshold) but the implementation compares
against local vertex positions; update the call site in
EditModeController::mergeByDistance to convert the world-space threshold to
local space before calling HalfEdgeMesh::mergeVerticesByDistance by dividing the
provided threshold by the entity's scale (e.g., compute localThreshold =
worldThreshold / entityScale) so the distance comparison uses consistent units;
alternatively, if you prefer changing the contract, update the
HalfEdgeMesh::mergeVerticesByDistance documentation to state the threshold is in
local space and keep callers unchanged—pick one consistent approach and adjust
callers or docs accordingly.
After a successful merge, the controller cleared m_selected{Vertices,Edges,Faces}
but didn't redraw the overlay. The previously-highlighted markers stayed on
screen, pointing at vertex slots that the merge had retired or shifted
(toEditableMesh re-packs per submesh).
All four entry points (mergeAtCenter / At First / At Last / By Distance)
now call updateSelectionOverlay() before emitting editSelectionChanged so
the highlights disappear together with the underlying selection state.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/EditModeController.cpp (2)
2506-2516:⚠️ Potential issue | 🔴 CriticalPre-merge selection still lost on undo (all four merge ops).
The
EditMeshTopologyCommandis still constructed after the selection sets are cleared (same pattern at 2506–2516, 2553–2563, 2598–2608, 2640–2650). Both theold*andnew*tuples receive the post-clear (empty) sets, so perEditMeshTopologyCommand::applyMeshStateundo restores empty selections instead of the user's pre-merge selection — the "single-step undo/redo" guarantee in the PR description doesn't actually round-trip selection.extrudeSelectionat 1305–1307 already shows the right pattern (snapshot before mutation).🐛 Proposed fix — apply identically in mergeAtFirst / mergeAtLast / mergeByDistance
if (m_selectedVertices.size() < 2) return 0; + // Snapshot pre-merge selection for undo before any mutation. + auto oldSelectedVertices = m_selectedVertices; + auto oldSelectedEdges = m_selectedEdges; + auto oldSelectedFaces = m_selectedFaces; + HalfEdgeMesh hm; if (!hm.buildFromEditableMesh(*m_editableMesh)) return 0; @@ auto* cmd = new EditMeshTopologyCommand( std::move(originalSubMeshes), m_editableMesh->subMeshes(), - m_selectedVertices, m_selectedEdges, m_selectedFaces, + oldSelectedVertices, oldSelectedEdges, oldSelectedFaces, m_selectedVertices, m_selectedEdges, m_selectedFaces, "Merge At Center");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/EditModeController.cpp` around lines 2506 - 2516, The selection sets are being cleared before constructing EditMeshTopologyCommand so both the old and new tuples receive empty selections; capture the pre-merge selections (m_selectedVertices, m_selectedEdges, m_selectedFaces) into local temporaries (e.g., originalSelectedVertices/Edges/Faces) before calling clear(), then construct EditMeshTopologyCommand using those temporaries as the "old" selection arguments and the post-clear/current selections as the "new" selection arguments (same pattern used by extrudeSelection), updating the constructor call sites for EditMeshTopologyCommand in mergeAtCenter/mergeAtFirst/mergeAtLast/mergeByDistance to use the saved originals.
2500-2527:⚠️ Potential issue | 🟠 MajorStill missing normals recompute and post-op refresh after merge (all four ops).
After
rewriteEntityAfterTopologyChangethe merge ops only callupdateSelectionOverlay()and emit signals. Compared with sibling topology ops in this same file:
applyBevelTopology(1607–1610) andapplyBevelVertexTopology(1732–1735) recompute normals (recalculateNormals/recalculateNormalsFlat) — without this the survivor (now at the centroid/anchor) and its 1-ring keep stale normals, so shading around the merged region is wrong.applyBevelTopology(1666–1668) andextrudeSelection(1546–1548) also callrefreshNormalVisualizer()andvalidateMesh(). WithoutvalidateMesh()m_degenerateTriangleCountis not refreshed, and per the HEmergeVerticesimplementation merges do both retire degenerate triangles and rebuild edges. WithoutrefreshNormalVisualizer()the normal-arrow gizmos stay parked on retired vertices.Apply the same
recalculateNormals*()/refreshNormalVisualizer()/validateMesh()block in each of mergeAtCenter / mergeAtFirst / mergeAtLast / mergeByDistance, just before pushing the undo command.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/EditModeController.cpp` around lines 2500 - 2527, After rewriteEntityAfterTopologyChange in each merge operation (mergeAtCenter, mergeAtFirst, mergeAtLast, mergeByDistance) add the same post-topology refresh block used by bevel/extrude: call recalculateNormals() / recalculateNormalsFlat() as appropriate, then call refreshNormalVisualizer() and validateMesh() before creating/pushing the EditMeshTopologyCommand (the new command push should remain after these calls) so normals, the normal gizmos, and m_degenerateTriangleCount are updated prior to the undo snapshot.
🧹 Nitpick comments (1)
src/EditModeController.cpp (1)
2444-2473: Nice extraction — consider also routing the existing call sites through it.
rewriteEntityAfterTopologyChangecleanly captures the deinit/init + per-subentity material restore + RTSS invalidate sequence. The same block is currently inlined inextrudeSelection(1477–1491, missing the material-restore step),applyBevelTopology(1619–1642),applyBevelVertexTopology(1740–1762),cancelBevel(2177–2200), andcommitKnife(2385–2407). Routing them through this helper would remove ~100 lines of near-duplicate code and, in theextrudeSelectioncase, also pick up the material-restore that's currently absent there (a separate latent issue: extrude into a wireframe/material-edited mesh loses the SubEntity overrides).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/EditModeController.cpp` around lines 2444 - 2473, Replace the duplicated deinit/_initialise/RTSS invalidation blocks in extrudeSelection, applyBevelTopology, applyBevelVertexTopology, cancelBevel, and commitKnife with a call to the new helper rewriteEntityAfterTopologyChange(ent); ensure you remove the inlined loops that reapply materials/ call ShaderGenerator::invalidateMaterial and instead rely on rewriteEntityAfterTopologyChange to restore per-subentity material names (fixing extrudeSelection's missing material-restore), and keep the semantics identical (capture and use the same Ogre::Entity* variable passed into the original blocks).
🤖 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 2582-2589: The code only copies the last vertex position into the
survivor but does not make the last-selected vertex the survivor slot, so
mergeVertices still preserves verts[0]'s attributes; fix by reordering the verts
vector so the desired anchor (m_selectedVertices' last element) is placed at
verts[0] before calling HalfEdgeMesh::mergeVertices: build verts such that
verts.front() == original last selected index (or std::rotate the vector), keep
computing target from that survivor if needed, then call hm.mergeVertices(verts,
target) so the correct vertex slot (and its UVs/normals/colors/etc.) survives;
reference m_selectedVertices, verts, target, and hm.mergeVertices when making
the change.
---
Duplicate comments:
In `@src/EditModeController.cpp`:
- Around line 2506-2516: The selection sets are being cleared before
constructing EditMeshTopologyCommand so both the old and new tuples receive
empty selections; capture the pre-merge selections (m_selectedVertices,
m_selectedEdges, m_selectedFaces) into local temporaries (e.g.,
originalSelectedVertices/Edges/Faces) before calling clear(), then construct
EditMeshTopologyCommand using those temporaries as the "old" selection arguments
and the post-clear/current selections as the "new" selection arguments (same
pattern used by extrudeSelection), updating the constructor call sites for
EditMeshTopologyCommand in
mergeAtCenter/mergeAtFirst/mergeAtLast/mergeByDistance to use the saved
originals.
- Around line 2500-2527: After rewriteEntityAfterTopologyChange in each merge
operation (mergeAtCenter, mergeAtFirst, mergeAtLast, mergeByDistance) add the
same post-topology refresh block used by bevel/extrude: call
recalculateNormals() / recalculateNormalsFlat() as appropriate, then call
refreshNormalVisualizer() and validateMesh() before creating/pushing the
EditMeshTopologyCommand (the new command push should remain after these calls)
so normals, the normal gizmos, and m_degenerateTriangleCount are updated prior
to the undo snapshot.
---
Nitpick comments:
In `@src/EditModeController.cpp`:
- Around line 2444-2473: Replace the duplicated deinit/_initialise/RTSS
invalidation blocks in extrudeSelection, applyBevelTopology,
applyBevelVertexTopology, cancelBevel, and commitKnife with a call to the new
helper rewriteEntityAfterTopologyChange(ent); ensure you remove the inlined
loops that reapply materials/ call ShaderGenerator::invalidateMaterial and
instead rely on rewriteEntityAfterTopologyChange to restore per-subentity
material names (fixing extrudeSelection's missing material-restore), and keep
the semantics identical (capture and use the same Ogre::Entity* variable passed
into the original blocks).
🪄 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: 6dd24927-6ff0-4fca-8421-f7220037f866
📒 Files selected for processing (1)
src/EditModeController.cpp
The Homebrew-installed bundle was failing to load several panels with 'plugin "qtquickcontrols2plugin" not found' for QML files that imported QtQuick.Controls 2.15. The plugin was bundled and frameworks were present — the version pin was the problem. Qt 6's bundled qmldir for QtQuick.Controls no longer carries the 2.15 compat shim, so plugin discovery only resolves the unversioned form. Local dev runs worked because the system Qt SDK still ships the legacy mappings; deployed bundles (where macdeployqt copies only the unversioned qmldir) didn't. Drops the legacy version numbers from every QML import so Qt6's plugin loader resolves them via the modern unversioned path. Project-internal modules (PropertiesPanel, AssetBrowser, etc.) keep their 1.0 versions since those are our own qmlRegisterSingletonType registrations. Also updates the macOS deploy step to write `QmlImports = PlugIns/qml` into qt.conf alongside the deprecated `Qml2Imports` key (Qt 6.5+ uses the new name). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds 7 more HE merge tests covering survivor target-position write, survivor bone-weight preservation, three-vert cluster collapse, filtering of invalid/duplicate/retired input indices, by-distance with empty input and negative threshold, and merge on boundary edges (single- triangle mesh). 13 merge tests now; full HalfEdgeMesh suite 124/124 green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Critical:
- Pre-merge selection was lost on undo. Each merge op cleared
m_selected{Vertices,Edges,Faces} BEFORE constructing
EditMeshTopologyCommand and then passed the cleared sets for both old
and new state. After undo, the selection sat empty even though the
mesh topology was restored. Now we snapshot the selection before the
mutation and pass the pre-merge sets as the command's old state.
- mergeAtLast didn't actually anchor on the last vertex. The HE
primitive uses verts[0] as the survivor, so mergeAtLast was running
"Merge At First, with last's position assigned to the first vert".
Now we swap the highest-index vert to the front before the call.
Major:
- Missing post-op refresh. Bevel/extrude run recalculateNormals() and
refreshNormalVisualizer() after topology changes; merge didn't, so
shading on the affected faces wasn't recomputed and the normals
overlay went stale. All four merge ops now do both.
- mergeVerticesByDistance unioned across submeshes, then
mergeVertices() rejected the whole cluster. UV-seam-adjacent verts
the user wanted to fuse were silently dropped. The HE primitive now
pre-partitions the union step by submesh, and the controller does
the same when computing survivor positions.
Minor:
- Defensive threshold clamp in mergeVerticesByDistance: large values
(or unit-mismatched callers) overflowed t² to +∞ and collapsed the
whole selection into one cluster. Capped at 1e18 (sqrt(FLT_MAX/2)).
Refactor:
- Extracted applyMergeAndRefresh to share the snapshot/recompute/
re-select boilerplate across the four entry points. Survivor(s) are
re-selected by hunting the target position(s) in the re-packed mesh
— same pattern bevel uses for its newly-created vertices.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
This was referenced Apr 27, 2026
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Phase 4 (#259) — merge vertices.
Summary
Implements the four standard merge variants from Blender:
Core primitive
`HalfEdgeMesh::mergeVertices(verts, targetPos)`:
`mergeVerticesByDistance` is union-find by spatial proximity, O(N²) on the candidate set — fine for UI-selection sizes.
Controller surface
`EditModeController::mergeAtCenter / mergeAtFirst / mergeAtLast / mergeByDistance`. All four push a single `EditMeshTopologyCommand` so undo/redo is one step, and all refuse outside vertex mode or with <2 verts selected.
A shared `rewriteEntityAfterTopologyChange` helper was extracted from the existing knife commit path — handles Ogre buffer resize, material preservation, and RTSS shader invalidation.
UI
Tests
All 117 active HalfEdgeMesh tests pass on macOS. Full suite has the same 6 pre-existing macOS-Ogre-init failures as master baseline (no new regressions).
Test plan
Closes part of #259 (Merge Vertices section).
🤖 Generated with Claude Code
Summary by CodeRabbit