feat(knife): multi-point cut tool with live preview - #308
Merged
Conversation
…fe tool Adds the topology primitives the knife tool needs. All three are pure data-structure operations with no rendering coupling, covered by unit tests so the knife UI layer above can rely on them. - appendFace(vector<int>, subMeshIndex): n-gon generalization of the existing appendTriangle helper. Single-polygon face append; the standard rebuildEdgesAndTwins / buildBoundaryHalfEdges / fixVertex cleanup still runs afterwards. appendTriangle is now a thin wrapper. - splitEdge(edgeIdx, t): insert a new vertex on an edge at parametric position t ∈ (0,1). Replaces each adjacent triangle with two smaller triangles meeting at the new midpoint. Interpolates position, normal, UV, color, tangent, and bone weights (bone list is unioned by index). Works on boundary edges too (only one face splits). MVP limit: only splits faces that are already triangles. - splitFace(faceIdx, vA, vB): insert a diagonal edge between two boundary vertices of an n-gon face, producing two new faces. Rejects adjacent boundary-vertex pairs (would duplicate an existing edge), same-vertex arguments, and out-of-range inputs. Triangle faces always fail (any pair of tri verts is adjacent) — which is fine for the knife, since two splitEdges on one triangle already produce the cut edge, no follow-up splitFace needed. Tests (9 new, all pass; full HE suite 106/106): - SplitEdgeMidpointOfInteriorEdgeDoublesTriangles - SplitEdgeInterpolatesNormalAndUV - SplitEdgeBoundaryEdgeProducesOneExtraTriangle - SplitEdgeClampsExtremeT - SplitEdgeInvalidIndexReturnsMinusOne - SplitEdgePreservesSubmeshCount - SplitFaceRejectsAdjacentBoundaryVertices - SplitFaceRejectsInvalidVertexIndices - TwoSplitEdgesOnOneTriangleProduceMidpointEdge (knife commit invariant) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds the knife tool that was the original goal of this branch. Layered on top of splitEdge/splitFace/appendFace (landed in the previous commit) so the controller side stays small and test-friendly. User-facing flow: - K in edit mode (or click the scissors button on the edit-mode toolbar) opens a knife session. - Left-click places a cut point. The hit-test priority is vertex snap → edge snap → face ray-cast (10px for each snap radius). - Cursor-hover draws a dim ghost segment from the last confirmed point to the current snap target, so the user previews the pending cut. - Enter / Return commits: the confirmed point list is walked, each OnEdge point triggers a splitEdge, the final mesh is pushed through EditMeshTopologyCommand so undo/redo rewinds in one step. - Esc cancels without mutating the mesh. - Edit-mode exit, or starting a bevel, cancels any active session. Wiring: - EditModeController: KnifePoint/KnifeSession types, beginKnife / addKnifePoint / updateKnifeHover / commitKnife / cancelKnife plus knifeSessionActiveValue and knifePointCountValue Q_PROPERTYs for QML. knifeHitTest uses the existing hitTestVertex/Edge/Face plumbing. Preview overlay is a ManualObject drawn via the EditMode/EdgeSelection material (same pipeline as the bevel selection overlay). - TransformOperator: mousePressEvent / mouseMoveEvent get a knife priority branch that short-circuits selection/bevel/transform while the session is active. - mainwindow.cpp: K keyboard shortcut (edit mode only, no modifier; follows the Blender convention). Esc and Enter routed to cancelKnife / commitKnife when the session is active. Toolbar gets a scissors button next to Extrude/Bevel, enabled whenever edit mode is on regardless of selection (knife hit-tests against geometry, not selection). - Sentry breadcrumbs: edit_mode for begin / point-added / commit / cancel, ui.action for the toolbar click, ui.shortcut for K / Esc / Enter. Tests: - 9 splitEdge/splitFace unit tests (prior commit) — all 106 HE tests still pass. - 6 new EditModeControllerBevelE2ETest knife scenarios covering the session lifecycle (begin→cancel, commit-without-points, mutual exclusion with bevel in both directions, exitEditMode cleanup, beginKnife outside edit mode). They skip on macOS where Ogre isn't available headlessly (same pattern as all E2E tests) and run on Linux CI. Known MVP limits documented in splitEdge/splitFace contracts: - splitEdge requires adjacent faces to already be triangles. - splitFace is only useful on n-gons; two splitEdges on a single tri already leave the M1↔M2 cut edge in place so splitFace isn't called in the common case. - On-face cut points are captured in the preview but skipped by the commit pipeline for now; the visible cut is only real on edges and vertices. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 52 minutes and 23 seconds. ⌛ 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 (2)
📝 WalkthroughWalkthroughAdds an interactive multi-point "Knife" editing tool: session lifecycle and UI hooks in EditModeController, topology primitives (edge/face split and cutPath) in HalfEdgeMesh, input/preview integration in TransformOperator/mainwindow, and tests covering session behavior and topology correctness. Changes
Sequence DiagramsequenceDiagram
participant User as User
participant TransformOp as TransformOperator
participant EditCtrl as EditModeController
participant HEM as HalfEdgeMesh
participant Preview as PreviewOverlay
User->>TransformOp: Mouse Move
TransformOp->>EditCtrl: updateKnifeHover(widget, x,y)
EditCtrl->>EditCtrl: knifeHitTest(screenPos)
EditCtrl->>Preview: updateKnifePreviewOverlay()
Preview-->>User: render hover
User->>TransformOp: Left Click
TransformOp->>EditCtrl: addKnifePoint(widget, x,y)
EditCtrl->>EditCtrl: validate & append confirmed KnifePoint
EditCtrl-->>TransformOp: consume click (early return)
User->>EditCtrl: Press Enter (Commit)
EditCtrl->>HEM: cutPath(resolvedCutPoints)
HEM->>HEM: splitEdge / splitFace (interpolate & replace faces)
HEM-->>EditCtrl: return new vertex indices
EditCtrl->>EditCtrl: toEditableMesh + rebuild Ogre entity + push undo command
EditCtrl->>Preview: destroyKnifePreviewOverlay()
EditCtrl-->>User: knifeSessionChanged (inactive)
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 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: 058a274baf
ℹ️ 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
+2297
to
+2300
| for (const auto& p : m_knifeSession.points) { | ||
| if (p.kind == KnifePoint::OnEdge && p.edgeIndex >= 0) { | ||
| if (hm.splitEdge(p.edgeIndex, p.edgeT) >= 0) ++inserted; | ||
| } |
There was a problem hiding this comment.
The commit loop replays edge indices captured at click time via hm.splitEdge(p.edgeIndex, p.edgeT), but splitEdge rebuilds the half-edge/edge tables after each successful split, which can renumber edges mid-commit. In multi-point cuts (for example two points on different edges of the same original triangle), later points can therefore apply to the wrong edge or fail, producing incorrect topology for the committed cut.
Useful? React with 👍 / 👎.
Comment on lines
+2404
to
+2405
| const Ogre::Vector3 w = localOrigin - p0; | ||
| const float t = std::clamp(d.dotProduct(w) / dd, 0.0f, 1.0f); |
There was a problem hiding this comment.
The edge snap t is derived from localOrigin only (dot(d, localOrigin - p0)), while the ray direction is ignored. Under perspective projection, the ray origin is the camera position for every screen pixel, so this makes t effectively independent of cursor position along that edge; users will get nearly fixed split points instead of cuts following where they clicked.
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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/HalfEdgeMesh_test.cpp (1)
2242-2242:⚠️ Potential issue | 🔴 CriticalRename this test; the current name collides with an existing GTest.
TEST(HalfEdgeMeshStandalone, SmoothSurfaceBevelProducesManifold)is already defined earlier in this file at Line 1824. Reusing the same suite/name pair causes a redefinition in the generated GTest symbols, so this file will not compile.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/HalfEdgeMesh_test.cpp` at line 2242, The TEST macro invocation duplicates an existing GTest symbol; change the test name used in TEST(HalfEdgeMeshStandalone, SmoothCharacterBevelProducesManifold) to a unique identifier (e.g. SmoothCharacterBevelProducesManifold_Unique or another distinct name) so it no longer collides with the previously defined TEST(HalfEdgeMeshStandalone, SmoothSurfaceBevelProducesManifold); update only the second parameter of the TEST macro to a new unique test name and ensure no other TEST in the file uses that same suite/name pair.
🧹 Nitpick comments (2)
src/EditModeController_test.cpp (1)
1207-1276: Consider assertingenterEditMode()preconditions.Each test calls
ctrl->enterEditMode()without a matchingASSERT_TRUE, unlike the bevel tests above in this same fixture (e.g., Line 913 callsctrl->enterEditMode()the same way, butbevelSelection()is thenASSERT_TRUE'd). For the knife tests, the very first assertion —EXPECT_FALSE(ctrl->knifeSessionActive())— doesn't distinguish between "edit mode failed to start" and "knife correctly reports inactive". Promoting these toASSERT_TRUE(ctrl->enterEditMode())(except inKnifeBeginOutsideEditModeFails) would make failures easier to diagnose if fixture setup drifts.🧪 Example hardening for one test
TEST_F(EditModeControllerBevelE2ETest, KnifeBeginThenCancelRestoresIdleState) { auto* ctrl = EditModeController::instance(); - ctrl->enterEditMode(); + ASSERT_TRUE(ctrl->enterEditMode()); EXPECT_FALSE(ctrl->knifeSessionActive());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/EditModeController_test.cpp` around lines 1207 - 1276, These tests call ctrl->enterEditMode() without asserting it succeeded, which can mask setup failures; update the five knife-related tests (KnifeBeginThenCancelRestoresIdleState, KnifeCommitWithoutPointsRejectsAndCleansUp, KnifeBeginCancelsActiveBevelFirst, BeginBevelCancelsActiveKnifeFirst, ExitEditModeCancelsKnifeSession) to use ASSERT_TRUE(ctrl->enterEditMode()) immediately after obtaining the controller so the fixture setup is validated (leave KnifeBeginOutsideEditModeFails unchanged since it intentionally expects edit mode off).src/HalfEdgeMesh.cpp (1)
3768-3793: Validate both sides before retiring the old face.
sideAB.size() / sideBA.size() < 3is checked at line 3793 after the face's HEs andm_faces[faceIdx].halfEdgehave already been cleared (lines 3768-3778). The earlier gap rejection makes this check dead for the currentn ∈ {3,4}range, but the ordering is fragile — any future tweak that loosens the gap check would turn this path into silent corruption (face gone, no replacements, hole left in the mesh).Prefer to compute the sides first and retire only once both sides are known to be well-formed.
♻️ Reorder validation before retirement
- const int subMeshIndex = m_faces[faceIdx].subMeshIndex; - - // Retire the old face. - { - const int startHE = m_faces[faceIdx].halfEdge; - int he = startHE; - do { - const int next = m_halfEdges[he].next; - m_halfEdges[he].face = -1; - he = next; - } while (he != startHE); - m_faces[faceIdx].halfEdge = -1; - } - - // Walk the old loop from vA to vB to collect one side, then from vB - // to vA for the other. appendFace needs at least 3 vertices, so a - // degenerate side (empty) bails the whole operation. std::vector<int> sideAB; for (int i = posA; ; i = (i + 1) % n) { sideAB.push_back(verts[i]); if (i == posB) break; } std::vector<int> sideBA; for (int i = posB; ; i = (i + 1) % n) { sideBA.push_back(verts[i]); if (i == posA) break; } if (sideAB.size() < 3 || sideBA.size() < 3) return false; + const int subMeshIndex = m_faces[faceIdx].subMeshIndex; + + // Retire the old face only once both sides are known to be valid. + { + const int startHE = m_faces[faceIdx].halfEdge; + int he = startHE; + do { + const int next = m_halfEdges[he].next; + m_halfEdges[he].face = -1; + he = next; + } while (he != startHE); + m_faces[faceIdx].halfEdge = -1; + } + appendFace(sideAB, subMeshIndex); appendFace(sideBA, subMeshIndex);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/HalfEdgeMesh.cpp` around lines 3768 - 3793, Compute and validate the two vertex loops (sideAB and sideBA) before modifying or retiring the existing face data to avoid leaving the mesh in a corrupted state; specifically, build sideAB and sideBA using verts, posA, posB, and n, check that both sizes are >= 3, and only if validation passes then proceed to clear m_halfEdges[*].face and set m_faces[faceIdx].halfEdge = -1 and continue with appendFace; ensure references to m_faces[faceIdx], m_halfEdges, sideAB, sideBA, appendFace remain unchanged so the rest of the flow uses the validated sides.
🤖 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 2296-2300: The loop is using stale KnifePoint.edgeIndex values
(captured from a temporary mesh) before calling hm.splitEdge, so later splits
can target the wrong/invalid edge; for each point in m_knifeSession.points,
re-resolve the target edge against the current hm just prior to splitting (e.g.,
use the stored point position/uv from KnifePoint to look up the current edge and
param t with the existing hit-test/find method) and only call hm.splitEdge with
the freshly-resolved edgeIndex/t; update or skip the split if the lookup fails.
Ensure you reference m_knifeSession.points, KnifePoint.edgeIndex/edgeT and
hm.splitEdge() when making the change.
- Around line 2310-2334: The knife commit updates m_editableMesh->subMeshes()
but does not sync those changes to the live Ogre entity; replicate the
post-topology-update steps used in extrudeSelection()/applyBevelTopology():
after setting m_editableMesh->subMeshes(), call the same sequence that resizes
entity buffers, reinitializes subentities and invalidates RTSS/material state
(e.g. invoke resizeEntityBuffers(...), reinitSubEntities(...) and
invalidateRTSSMaterials() or their local equivalents), then push the
EditMeshTopologyCommand and emit the same signals so the cut becomes immediately
visible; locate these helper calls in extrudeSelection/applyBevelTopology and
apply them here around the EditMeshTopologyCommand usage.
- Around line 2386-2405: The current code computes t using only localOrigin and
the edge (p0,p1), ignoring the ray direction (localDir); replace that with a
proper closest-point solve between the ray (localOrigin + s*localDir) and the
segment (p0 + t*d): compute r0 = localOrigin - p0, a = localDir.dot(localDir), b
= localDir.dot(d), c = d.dot(d) (dd), e = localDir.dot(r0), f = d.dot(r0), then
solve the 2x2 normal equations for t = (a*f - b*e)/(a*c - b*b) and s = (b*t -
e)/a; clamp t into [0,1]; if the denominator (a*c - b*b) is near zero (parallel
case) fall back to projecting localOrigin onto the segment with t = clamp(f /
c); update the code that currently sets t from d.dotProduct(w) / dd to use this
computation (use the existing symbols localDir, localOrigin, p0, p1, d, dd, t)
and ensure vector dot products use float/Ogre::Real as appropriate.
- Around line 2471-2487: updateSelectionOverlay() creates m_overlayNode under
the entity node and feeds it local-space geometry, but this code reuses
m_overlayNode for the knife and then applies the entity's derived transform
again, causing double-transforms; instead create a distinct scene node for the
knife (e.g., m_overlayKnifeNode or m_knifeNode) and attach m_overlayKnife to
that node rather than m_overlayNode, then set the derived
position/orientation/scale on the new knife node (or attach it under the same
parent as m_overlayNode if you want same-space behavior) so you no longer
overwrite or double-transform m_overlayNode; update the code in the block that
checks/creates m_overlayNode/m_overlayKnife to create and use m_overlayKnifeNode
and attach m_overlayKnife to it.
In `@src/HalfEdgeMesh.cpp`:
- Around line 3747-3748: The guard in splitFace that rejects faces with
verts.size() > 4 contradicts the documented "n-gon" support; update splitFace to
accept any face with verts.size() >= 3 by removing the upper-bound check (locate
the check using faceVertices(faceIdx) and the conditional that returns false on
size() > 4) so the function can handle arbitrary n-gons (appendFace already
supports n ≥ 3); ensure any subsequent logic that iterates face vertices still
works for variable-length faces.
- Around line 3687-3729: The splitEdge implementation lets the split proceed
when only one adjacent face is a triangle which corrupts topology; in splitEdge,
replace the permissive early-out that returns only if both describeFace calls
fail with a guard that requires both adjacent faces to be valid triangles (i.e.
if (!hasA || !hasB) return -1) so the routine aborts when either side is a
non-triangle; reference describeFace, splitEdge, the retireFace lambda,
appendFace, rebuildEdgesAndTwins and buildBoundaryHalfEdges when making this
change to ensure no mesh mutation occurs unless both faces are triangles.
In `@src/mainwindow.cpp`:
- Around line 1060-1075: During an active knife session
(editCtrl->knifeSessionActive()) other edit-mode shortcuts are still handled
later by the isEditModeActive() switch, causing mode flips; update the
knife-session branch to also consume (event->accept(); return;) or otherwise
block the edit-mode keys so they don't fall through. Specifically, intercept
digit keys (1/2/3), unmodified 'K', and modifier combos like Ctrl+A/Alt+A,
Ctrl+E, Ctrl+B (and any other edit-mode shortcuts handled by isEditModeActive())
while knifeSessionActive() is true, or add a clear comment in the knife-session
block explaining that letting those keys through is intentional.
---
Outside diff comments:
In `@src/HalfEdgeMesh_test.cpp`:
- Line 2242: The TEST macro invocation duplicates an existing GTest symbol;
change the test name used in TEST(HalfEdgeMeshStandalone,
SmoothCharacterBevelProducesManifold) to a unique identifier (e.g.
SmoothCharacterBevelProducesManifold_Unique or another distinct name) so it no
longer collides with the previously defined TEST(HalfEdgeMeshStandalone,
SmoothSurfaceBevelProducesManifold); update only the second parameter of the
TEST macro to a new unique test name and ensure no other TEST in the file uses
that same suite/name pair.
---
Nitpick comments:
In `@src/EditModeController_test.cpp`:
- Around line 1207-1276: These tests call ctrl->enterEditMode() without
asserting it succeeded, which can mask setup failures; update the five
knife-related tests (KnifeBeginThenCancelRestoresIdleState,
KnifeCommitWithoutPointsRejectsAndCleansUp, KnifeBeginCancelsActiveBevelFirst,
BeginBevelCancelsActiveKnifeFirst, ExitEditModeCancelsKnifeSession) to use
ASSERT_TRUE(ctrl->enterEditMode()) immediately after obtaining the controller so
the fixture setup is validated (leave KnifeBeginOutsideEditModeFails unchanged
since it intentionally expects edit mode off).
In `@src/HalfEdgeMesh.cpp`:
- Around line 3768-3793: Compute and validate the two vertex loops (sideAB and
sideBA) before modifying or retiring the existing face data to avoid leaving the
mesh in a corrupted state; specifically, build sideAB and sideBA using verts,
posA, posB, and n, check that both sizes are >= 3, and only if validation passes
then proceed to clear m_halfEdges[*].face and set m_faces[faceIdx].halfEdge = -1
and continue with appendFace; ensure references to m_faces[faceIdx],
m_halfEdges, sideAB, sideBA, appendFace remain unchanged so the rest of the flow
uses the validated sides.
🪄 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: 941270d1-2ce1-4ef6-8f49-64640d1b9514
📒 Files selected for processing (8)
src/EditModeController.cppsrc/EditModeController.hsrc/EditModeController_test.cppsrc/HalfEdgeMesh.cppsrc/HalfEdgeMesh.hsrc/HalfEdgeMesh_test.cppsrc/TransformOperator.cppsrc/mainwindow.cpp
Comment on lines
+3747
to
+3748
| const auto verts = faceVertices(faceIdx); | ||
| if (verts.size() < 3 || verts.size() > 4) return false; |
There was a problem hiding this comment.
Doc vs. code: n-gon claim contradicts the size() > 4 reject.
The PR description (and the knife-tool companion docs) describe splitFace as cutting a diagonal across "an n-gon", but the guard here refuses anything larger than a quad. appendFace already supports arbitrary n ≥ 3, so the cap looks like an MVP choice rather than an algorithmic constraint.
Either drop the upper bound (the existing gap check and side-size walk handle arbitrary n) or add a doxygen comment stating the v1 limit explicitly so callers know the n-gon phrasing is aspirational.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/HalfEdgeMesh.cpp` around lines 3747 - 3748, The guard in splitFace that
rejects faces with verts.size() > 4 contradicts the documented "n-gon" support;
update splitFace to accept any face with verts.size() >= 3 by removing the
upper-bound check (locate the check using faceVertices(faceIdx) and the
conditional that returns false on size() > 4) so the function can handle
arbitrary n-gons (appendFace already supports n ≥ 3); ensure any subsequent
logic that iterates face vertices still works for variable-length faces.
…edges The MVP knife dropped isolated vertices on each clicked edge but left the segments between them as pure preview, which made the tool useless beyond "insert vertex on edge." This changes the commit pipeline to actually chase the cut line across the mesh. New HalfEdgeMesh::cutPath(vector<CutPoint>): - Re-resolves each click's edge by its endpoint vertex pair after every internal splitEdge (edge indices aren't stable across rebuildEdgesAndTwins; vertex indices are append-only). - Splits each click endpoint, then for every consecutive pair walks the tris between the resulting vertices. For each step the algorithm picks the face adjacent to the current anchor vertex and intersects the 3D segment to the next anchor with every non-entry edge of that face. The nearest forward crossing that sits strictly inside an edge wins; splitEdge runs there, producing the interior mid-cut vertex. The shared-face short-circuit stops the walk once the two anchors sit on the same triangle — the tri's two existing splitEdges already left the connecting segment as a real edge. - Handles degenerate inputs (fewer than 2 points, invalid edge index) by returning an empty vector without mutating the mesh. closestOnSegments() helper: 3D segment-segment minimization used to find where the cut line exits a triangle. Returns the parameter pair plus squared separation so callers can filter out "close but not coincident" pairs. EditModeController::commitKnife now calls cutPath once per commit with the filtered on-edge click list. The breadcrumb count reflects the total new vertex count, which includes interior crossings. Tests (4 new, all pass; full HE suite 110/110): - CutPathCrossesInteriorDiagonalAndLinksEndpoints: bottom-edge to top-edge cut across a two-tri quad produces three new verts on the x=0.5 line and they're linked by real edges. - CutPathSingleTriangleStillProducesEndpointEdge: two edges of one tri, no interior crossing expected. - CutPathBailsOnFewerThanTwoPoints: empty / single-point input. - CutPathFailsOnInvalidEdgeIndex: negative / out-of-range edges. Not yet pushed — waiting for in-app validation first.
…mp to 2.29.0 Completes the knife tool wrap-up started in the walk-and-cut commit: - addKnifePointOnEdge(heEdgeIndex, t): programmatic cut-point entry that skips the widget-based hit-test. Scripted/automated knife flows can use it; it also unblocks the headless controller-level tests that can't run QWidget + camera projection. - Three new EditModeControllerBevelE2ETest cases covering the commit pipeline end to end: vertex count grows manifoldly after a cut, undo rewinds to the original count, invalid edge indices are refused without mutating the session. All three skip on macOS (Ogre headless) like every other BevelE2E test and run on Linux CI. - CMakeLists.txt VERSION bump 2.28.3 → 2.29.0 for the knife feature. Tests: full HalfEdgeMeshStandalone suite still 110/110. Controller suite builds clean.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/EditModeController.cpp (1)
2563-2579:⚠️ Potential issue | 🟠 MajorUse a dedicated scene node for the knife overlay.
m_overlayNodeis shared with the selection overlays, which normally render local-space geometry under the entity node. Reusing that node here and then writing the entity’s derived world transform onto it will double-transform whichever overlay created it first. Putm_overlayKnifeon its own node, or keep it in the same local-space parent without mutatingm_overlayNode.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/EditModeController.cpp` around lines 2563 - 2579, The overlay knife is being attached to the shared m_overlayNode and then that node is overwritten with the entity's derived world transform, causing double-transforms for other overlays; fix by creating and using a dedicated scene node (e.g., m_overlayKnifeNode) for m_overlayKnife instead of m_overlayNode: allocate m_overlayKnifeNode from sceneMgr->getRootSceneNode()->createChildSceneNode(), attach m_overlayKnife to that node, and apply the entNode->_getDerivedPosition()/Orientation()/Scale() to the new m_overlayKnifeNode (leaving m_overlayNode untouched) so selection overlays continue to render in local space.
🧹 Nitpick comments (1)
src/HalfEdgeMesh.cpp (1)
3935-3962: Exit-edge proximity tolerance is a fixed absolute in world-space.The
hit.squaredSeparation > 1e-4fcheck on line 3949 (and thekNearZero = 1e-5fendpoint guard on 3954) are absolute world-space values. Meshes imported at small scales (e.g., typical character rigs in meters where ~0.01 local units is already a meaningful feature) can see1e-2-class separations on valid crossings, while giant props may produce sub-threshold noise on clearly-missed edges. Consider scaling the tolerance against the cut segment length|pT - pA|or a precomputed bounding-box diagonal so the walk behaves consistently across import scales.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/HalfEdgeMesh.cpp` around lines 3935 - 3962, The absolute proximity checks (hit.squaredSeparation > 1e-4f and hit.sB < kNearZero / > 1.0f - kNearZero) make edge-exit detection scale dependent; replace those fixed world-space constants with scale-aware tolerances computed from the current cut segment length or a mesh scale metric. Specifically, compute a local scale like float segLen = (pT - pA).length() (or use the mesh bbox diagonal) and derive squaredTol = (segLen * relEps) * (segLen * relEps) and nearZero = relNear * segLen (choose relEps ~ 1e-3..1e-2, relNear ~ 1e-5..1e-3), then compare hit.squaredSeparation against squaredTol and hit.sB against nearZero/ (1.0f - nearZero) instead of the fixed 1e-4f and kNearZero; update references to hit.squaredSeparation, hit.sB, kNearZero, and any uses of kMinStep if it should also be relative.
🤖 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 2368-2389: This commit path diverges from applyBevelTopology():
after calling m_editEntity->_deinitialise() / _initialise(true) you must restore
SubEntity material overrides and refresh/clear topology selections before
creating/pushing the EditMeshTopologyCommand so we don't keep stale IDs or lose
wireframe/material overrides. Locate the equivalent post-topology sequence in
applyBevelTopology() and copy its steps here: re-apply per-SubEntity material
overrides to m_editableMesh->subMeshes(), call the same selection refresh/clear
logic that updates m_selectedVertices/m_selectedEdges/m_selectedFaces (or clears
them if the topology no longer contains those IDs), then construct and push the
EditMeshTopologyCommand, followed by the existing Sentry breadcrumb,
m_knifeSession reset, destroyKnifePreviewOverlay() and emits.
- Around line 2336-2348: The commit currently filters m_knifeSession.points to
build cpts and silently drops non-OnEdge points, producing an incorrect cut for
previews containing OnFace/OnVertex; modify the commit guard in
EditModeController.cpp to first scan m_knifeSession.points for any KnifePoint
whose kind is not KnifePoint::OnEdge (e.g. KnifePoint::OnFace or
KnifePoint::OnVertex) and if any are found, log the same breadcrumb ("Knife:
commit rejected (unsupported point types)") and call cancelKnife() and return
false instead of proceeding to build cpts, so only fully OnEdge sequences are
ever committed; reference m_knifeSession, KnifePoint::OnEdge, cancelKnife(), and
cpts when locating where to add this check.
In `@src/HalfEdgeMesh.cpp`:
- Around line 3852-3894: cutPath currently mutates the mesh incrementally so a
mid-sequence failure (e.g. two CutPoints resolving to the same edge/endpoints)
leaves partial splits in m_edges and newVertices; detect and reject such
conflicting inputs up front by validating the resolved endpoint pairs in
clickEdgeVerts before performing any splitEdge calls. Specifically, in
HalfEdgeMesh::cutPath, after populating clickEdgeVerts and clickT but before the
loop that calls splitEdge, scan clickEdgeVerts for duplicates or pairs that
would collide (identical unordered vertex pairs) and return an empty newVertices
if any are found; reference the existing symbols clickEdgeVerts,
findEdgeByVerts, splitEdge, m_edges, and newVertices to locate and implement
this pre-check. Ensure the check treats (va,vb) and (vb,va) as equal so no split
occurs when two clicks target the same logical edge.
---
Duplicate comments:
In `@src/EditModeController.cpp`:
- Around line 2563-2579: The overlay knife is being attached to the shared
m_overlayNode and then that node is overwritten with the entity's derived world
transform, causing double-transforms for other overlays; fix by creating and
using a dedicated scene node (e.g., m_overlayKnifeNode) for m_overlayKnife
instead of m_overlayNode: allocate m_overlayKnifeNode from
sceneMgr->getRootSceneNode()->createChildSceneNode(), attach m_overlayKnife to
that node, and apply the entNode->_getDerivedPosition()/Orientation()/Scale() to
the new m_overlayKnifeNode (leaving m_overlayNode untouched) so selection
overlays continue to render in local space.
---
Nitpick comments:
In `@src/HalfEdgeMesh.cpp`:
- Around line 3935-3962: The absolute proximity checks (hit.squaredSeparation >
1e-4f and hit.sB < kNearZero / > 1.0f - kNearZero) make edge-exit detection
scale dependent; replace those fixed world-space constants with scale-aware
tolerances computed from the current cut segment length or a mesh scale metric.
Specifically, compute a local scale like float segLen = (pT - pA).length() (or
use the mesh bbox diagonal) and derive squaredTol = (segLen * relEps) * (segLen
* relEps) and nearZero = relNear * segLen (choose relEps ~ 1e-3..1e-2, relNear ~
1e-5..1e-3), then compare hit.squaredSeparation against squaredTol and hit.sB
against nearZero/ (1.0f - nearZero) instead of the fixed 1e-4f and kNearZero;
update references to hit.squaredSeparation, hit.sB, kNearZero, and any uses of
kMinStep if it should also be relative.
🪄 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: 5ebd1d2f-4a7c-4900-8353-95f0baf3ffda
📒 Files selected for processing (7)
CMakeLists.txtsrc/EditModeController.cppsrc/EditModeController.hsrc/EditModeController_test.cppsrc/HalfEdgeMesh.cppsrc/HalfEdgeMesh.hsrc/HalfEdgeMesh_test.cpp
✅ Files skipped from review due to trivial changes (1)
- CMakeLists.txt
🚧 Files skipped from review as they are similar to previous changes (2)
- src/EditModeController_test.cpp
- src/HalfEdgeMesh.h
# Conflicts: # src/EditModeController_test.cpp
Seven fixes for the walk-and-cut knife, none behaviour changes in the happy path but all correctness / UX for edge cases. - splitEdge atomic on mixed tri/non-tri adjacency: if either adjacent face exists and isn't a triangle, bail with -1 before any mutation. Previously the half-split path silently desynced the untouched side's half-edge pointers against a face that was still an n-gon. - cutPath atomic on mid-sequence failure: snapshot m_halfEdges / m_vertices / m_faces / m_edges at entry and restore on any early return. Duplicate-edge CutPoints (two clicks resolving to the same edge) used to leave a half-applied cut visible to the caller; now the mesh is byte-identical to its pre-call state when cutPath fails. New test: CutPathRollsBackWhenSecondEdgeDuplicatesFirst. - commitKnife refuses non-edge clicks: if any confirmed point is OnFace or OnVertex, bail with a Sentry breadcrumb instead of silently dropping it. A visible preview edge→face→edge would have committed as edge→edge, diverging from what the user drew. - Knife session swallows every keyboard input except Esc/Enter: plain 1/2/3, Ctrl+A, Ctrl+E, Ctrl+B, K, etc. used to fall through and mutate state mid-cut. Session keyPressEvent now event->accept()s everything else and returns. - Knife preview owns its own scene node (m_overlayKnifeNode) rather than reusing m_overlayNode with the selection overlays. The knife mirrors the entity's world transform; the selection overlays want origin-parked local-space parenting. They can't share. - commitKnife mirrors applyBevelTopology's SubEntity-material restore + RTSS invalidate cycle. Without it, wireframe / MaterialEditor overrides got wiped whenever _deinitialise/_initialise ran. Selection sets are also cleared since the cut can retire the topology IDs they reference. - splitFace doc clarified: explicitly triangle-rejecting (every pair of tri verts is adjacent), quad-splitting, higher n-gons refused as a scope choice not a contract limit. Tests: 111/111 HalfEdgeMeshStandalone. Controller E2E suite builds clean; knife scenarios skip on macOS (Ogre headless) and run on Linux CI. VERSION bump 2.29.0 → 2.29.1 — correctness patches on top of the knife feature shipped in the previous commits of this PR.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/EditModeController.h (1)
294-349: Clarify thatcommitKnife()is edge-only in v1.The public knife docs describe vertex/edge/face snapping as normal input, but the current commit path only succeeds for
OnEdgepoints and rejectsOnVertex/OnFace. Calling that out here would keep future callers from assuming any snapped point is committable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/EditModeController.h` around lines 294 - 349, The commitKnife() documentation should explicitly state that, in v1, only cut points resolved as OnEdge are applied (points snapped to vertices or faces are ignored/rejected); update the comment on the commitKnife() declaration to note this limitation and reference the knife point types and m_knifeSession handling so callers know to use addKnifePointOnEdge() or ensure points are OnEdge before committing; keep addKnifePoint(), addKnifePointOnEdge(), knifeSessionActive(), and knifePointCount() behavior unchanged.
🤖 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_test.cpp`:
- Around line 1451-1457: The test currently hard-codes half-edge indices (calls
to addKnifePointOnEdge(0, ...) and addKnifePointOnEdge(5, ...)) which rely on
internal HalfEdgeMesh ordering; instead, construct a fresh HalfEdgeMesh for the
cube face you intend to click, locate the edge indices by querying the mesh for
known vertex pairs or vertex positions on that face, then pass those resolved
edge indices into ctrl->addKnifePointOnEdge(...). Update the two places
mentioned (the calls at the shown location and the similar calls around
1493-1496) to compute edgeIndex = halfEdgeMesh.findEdgeIndexByVertexPair(vA, vB)
or by position lookup before invoking addKnifePointOnEdge, ensuring the two
resolved edges are surface-connected as required by the test.
In `@src/HalfEdgeMesh_test.cpp`:
- Around line 3281-3287: The test uses makeQuadMesh() which produces two
triangles so splitFace's triangle-rejection masks invalid-vertex checks; instead
build an editable mesh containing a single face with >=4 vertices (or otherwise
ensure the face under test is not a triangle) before calling
he.buildFromEditableMesh(em), then call he.splitFace(...) with out-of-range and
identical vertex indices to exercise the invalid-vertex-path; locate the setup
that creates em (makeQuadMesh or the EditableMesh construction) and replace it
with a single-quad or n-gon creation so splitFace(0, -1, 1), splitFace(0, 0,
999), and splitFace(0, 2, 2) actually reach the invalid-vertex validation.
- Around line 3154-3184: The test SplitEdgeInterpolatesNormalAndUV currently
omits any check of the interpolated UVs; update the test to assert that the new
vertex's UV (accessed via he.vertex(vMid).uv or v.uv) is present and equals the
expected interpolation between he.vertex(1).uv and he.vertex(2).uv at t=0.25
(use EXPECT_NEAR on each UV component with a small epsilon, mirroring how
position/normal are checked), so that splitEdge's UV interpolation is actually
verified.
---
Nitpick comments:
In `@src/EditModeController.h`:
- Around line 294-349: The commitKnife() documentation should explicitly state
that, in v1, only cut points resolved as OnEdge are applied (points snapped to
vertices or faces are ignored/rejected); update the comment on the commitKnife()
declaration to note this limitation and reference the knife point types and
m_knifeSession handling so callers know to use addKnifePointOnEdge() or ensure
points are OnEdge before committing; keep addKnifePoint(),
addKnifePointOnEdge(), knifeSessionActive(), and knifePointCount() behavior
unchanged.
🪄 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: 6fd72ee4-d7e4-4e0c-9061-1a6953b76aeb
📒 Files selected for processing (8)
CMakeLists.txtsrc/EditModeController.cppsrc/EditModeController.hsrc/EditModeController_test.cppsrc/HalfEdgeMesh.cppsrc/HalfEdgeMesh.hsrc/HalfEdgeMesh_test.cppsrc/mainwindow.cpp
✅ Files skipped from review due to trivial changes (2)
- src/EditModeController.cpp
- src/HalfEdgeMesh.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- CMakeLists.txt
- src/mainwindow.cpp
Comment on lines
+3281
to
+3287
| TEST(HalfEdgeMeshStandalone, SplitFaceRejectsInvalidVertexIndices) { | ||
| auto em = makeQuadMesh(); | ||
| HalfEdgeMesh he; | ||
| ASSERT_TRUE(he.buildFromEditableMesh(em)); | ||
| EXPECT_FALSE(he.splitFace(0, -1, 1)); | ||
| EXPECT_FALSE(he.splitFace(0, 0, 999)); | ||
| EXPECT_FALSE(he.splitFace(0, 2, 2)); // same vertex |
There was a problem hiding this comment.
This setup doesn't actually hit the invalid-vertex path.
makeQuadMesh() still builds two triangles, and splitFace is documented to reject triangles outright. These assertions can pass even if the invalid-vertex validation is broken, because triangle rejection short-circuits first.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/HalfEdgeMesh_test.cpp` around lines 3281 - 3287, The test uses
makeQuadMesh() which produces two triangles so splitFace's triangle-rejection
masks invalid-vertex checks; instead build an editable mesh containing a single
face with >=4 vertices (or otherwise ensure the face under test is not a
triangle) before calling he.buildFromEditableMesh(em), then call
he.splitFace(...) with out-of-range and identical vertex indices to exercise the
invalid-vertex-path; locate the setup that creates em (makeQuadMesh or the
EditableMesh construction) and replace it with a single-quad or n-gon creation
so splitFace(0, -1, 1), splitFace(0, 0, 999), and splitFace(0, 2, 2) actually
reach the invalid-vertex validation.
Three targeted test improvements from the second CodeRabbit review
pass. No product-code changes; just stricter / more portable tests.
- Resolve cube edges by vertex pair in KnifeCommit* tests instead of
hard-coding HE edge indices (0 / 5). The edge numbering is an
internal rebuild artefact and the hard-coded values also didn't
guarantee the two clicks landed on a surface-connected path, so
these tests could have flaked on a harmless topology-order change.
New resolveEdgeByVerts() helper plus (2, 3) / (4, 5) top-face
edges keeps the cut on a real coplanar region.
- SplitEdgeInterpolatesNormalAndUV now actually asserts UV. The
existing test claimed to cover attribute lerp but only checked
position and normal — a broken v.uv implementation would have
passed silently. Adds the x+y=1 invariant (always true for
v1=(1,0) / v2=(0,1) linear blends) and the distance-along-UV-
segment ratio so both a zeroed UV and a wrong-parameter UV fail.
- SplitFaceRejectsInvalidVertexIndices clarified: documents that
the invalid-vertex guards run BEFORE the triangle-rejection
check (implementation order matters here), and adds a case for
"valid vertex not on this face" — triangle 0 of makeQuadMesh
contains verts {0,1,2}, so splitFace(0, 0, 3) hits the boundary-
loop check specifically.
Tests: 111/111 HalfEdgeMeshStandalone still green. Controller E2E
suite compiles cleanly; the knife scenarios still skip on macOS
(Ogre headless) and run on Linux CI.
|
This was referenced Apr 26, 2026
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
Adds the knife topology tool that was scoped when we decided against starting with loop-cut: unlike loop-cut, the knife doesn't care about manifold ring walks, so it works cleanly on both primitives and arbitrary imported meshes.
HalfEdgeMesh):splitEdge(edgeIdx, t)inserts a vertex at parametricton an edge and splits each adjacent triangle into two;splitFace(faceIdx, vA, vB)adds a diagonal between two boundary vertices of an n-gon;appendFace(vector<int>, subMeshIndex)is the n-gon generalization ofappendTriangle.splitEdgeinterpolates position, normal, UV, color, tangent, and bone weights between the endpoints. Boundary edges handled (one face splits instead of two).EditModeController):beginKnife/addKnifePoint/updateKnifeHover/commitKnife/cancelKnife. AKnifeSessionholds the confirmed point list + the live hover snap. Points are typed —OnVertex,OnEdge(edgeIdx, t), orOnFace(triIdx, localPos).tis computed via closest-point-on-line-segment against the click ray in local mesh space, so the placement tracks the cursor even on oblique edges.ManualObjectdraws the confirmed polyline in solid yellow and the pending segment (last-confirmed → cursor snap) in a dimmer ghost yellow, using the existingEditMode/EdgeSelectionmaterial.splitEdgefor eachOnEdgepoint, pushes oneEditMeshTopologyCommandso undo/redo rewinds the whole cut in one step. TheM1↔M2edge across a single triangle falls out of twosplitEdgecalls automatically — nosplitFaceneeded in the common case.Kenters knife mode,Esccancels,Enter/Returncommits, scissors button on the edit-mode toolbar next to Extrude/Bevel. Mutual exclusion with bevel in both directions. Knife always tears down on edit-mode exit (committing a stale mid-cut would be surprising).edit_modefor session lifecycle with point counts (Knife: point added (n=3),Knife: commit (points=3, cuts=2)),ui.actionfor toolbar click,ui.shortcutfor K / Esc / Enter.Scope limits (deliberate)
splitEdgerequires adjacent faces to already be triangles — our entire pipeline feeds triangles through Assimp, so this matches reality.splitFaceis a correct primitive but not used by the commit pipeline yet; it'll become relevant if we add a loop-cut or knife-project tool.splitFace+ a new vertex on the face interior, which is a real project but not v1).Tests
splitEdge/splitFace(pure topology, no Ogre): midpoint split of an interior edge, attribute interpolation (UV/normal), boundary-edge split, t-clamping, invalid-index rejection, submesh preservation, bad-hint fallback, adjacent-vertex rejection,M1↔M2edge invariant after twosplitEdges. FullHalfEdgeMeshStandalonesuite: 106/106.EditModeControllerBevelE2ETest): begin→cancel lifecycle, commit-without-points rejection, mutual exclusion with bevel (both directions),exitEditModetears down the session,beginKnifeoutside edit mode refuses. Skip on macOS (Ogre headless limitation) and run on Linux CI.Test plan
./build_local/bin/UnitTests --gtest_filter="HalfEdgeMeshStandalone.*"→ 106/106 pass./build_local/bin/UnitTests --gtest_filter="*Standalone*:*Geometry*"→ 181 pass, 3 pre-existing skips🤖 Generated with Claude Code
Summary by CodeRabbit