feat(bevel): add segments and profile (convex fillet) parameters - #298
Merged
Conversation
HalfEdgeMesh::bevelEdges grows two new optional parameters:
- segments (int, default 1): subdivides the chamfer strip into N steps,
inserting segments-1 intermediate vertices on each endpoint between
v1a/v1b and v2a/v2b. The strip is emitted as 2*N triangles instead
of the original 2 — winding is preserved when N==1 so existing
fixtures pass unchanged.
- profile (float in [0, 1], default 0.5): profile-curve shape.
- 0.5 = flat (linear interpolation, identical to single-segment).
- >0.5 = convex (intermediates bulge toward where the original edge
was — fillet-like rounded chamfer).
- <0.5 = concave: currently CLAMPED TO FLAT. Inward bulge trips a
Phase-7 winding edge case in the corner-cap emission that produces
inverted triangles even for tiny inward shifts. The flat/convex
half of the parameter range covers Blender's typical use; concave
is gated behind that bug fix in a follow-up.
Bulge magnitude is capped at 0.5*width at the chord midpoint, applied
along the (chord, v) plane perpendicular to the chord.
Wires through EditModeController:
- BevelSession gains segments + profile fields.
- updateBevelSegments(int) and updateBevelProfile(float) re-apply the
bevel on the snapshot mesh, mirroring updateBevelWidth.
QML inspector grows a Bevel section that's only visible while a bevel
session is active: a SpinBox for segments (1..16) and a Slider for
profile (0..1, snap 0.05) with a help line explaining the value
range.
6 new unit tests covering segments=1/2/3/4 (flat), segments=4 convex,
and a profile-shifts-position assertion. 88 HalfEdge/EditMode tests
pass overall.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds multi-segment, profile-driven beveling: new QML controls (segments + draggable profile graph) expose session parameters; EditModeController stores/applies segments and per-point profile values and exposes QML invokables/signals; HalfEdgeMesh emits multi-segment chamfer geometry; tests and resources updated. (34 words) Changes
Sequence DiagramsequenceDiagram
participant User as User
participant QML as QML UI
participant Controller as EditModeController
participant Mesh as HalfEdgeMesh
User->>QML: change segments / drag profile handle
QML->>Controller: updateBevelSegments(segments)\nor updateBevelProfilePoint(index, value)
Controller->>Controller: clamp/validate & resample profile (if needed)
Controller->>Controller: restore pre-bevel snapshots
Controller->>Mesh: bevelEdges(edges, width, segments, profilePoints)
Mesh->>Mesh: clamp inputs, build per-segment vertex chains\nand emit subdivided quads/triangles
Mesh-->>Controller: return indices / mesh modifications
Controller->>Controller: update session state, emit bevelProfilePointsChanged()
Controller-->>QML: property change notifications
QML-->>User: UI refresh
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: 712324ca4d
ℹ️ 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".
| // Lets the user tweak segment count and profile shape while the | ||
| // gizmo is up. | ||
| Column { | ||
| visible: EditModeController.bevelSessionActive |
There was a problem hiding this comment.
bevelSessionActive, bevelSegments, and bevelProfile are exposed as Q_INVOKABLE methods, but this binding uses method references (EditModeController.bevelSessionActive) instead of invoking them. In QML this binds a function object rather than the current value, so the bevel controls can render with incorrect visibility/value state (and may not track session changes correctly). This makes the new bevel UI unreliable during normal edit-mode interaction.
Useful? React with 👍 / 👎.
…segments >=8 Two bugs fixed together because they share a root: the hole-filler post-pass was dropping legitimate fill opportunities in two scenarios a user would hit daily. 1) segments >= 8 lost its corner-cap fill triangles. The post-pass had a hard `if (loop.size() > 8) continue;` cap meant to skip bizarrely large (probably non-simple) loops. But a multi- segment bevel's corner-cap loop is legitimately 2*(segments+1) verts: one per chain step on each side. segments=8 lands at 18, immediately over the cap. The fix raises the cap to 64 — generous for any reasonable bevel, still a finite guard against runaway walker output. Symptom: a visible HOLE on the front + back of a cube edge beveled with 8+ segments. 2) concave profile produced inverted fill triangles. The fill-winding decision was geometry-first: compute the face normal from loop[0..2], compare to the neighbor-averaged refNormal, flip if they oppose. That's fine when the fill surface is roughly flat, but breaks when the loop vertices sit on a curved profile — concave profiles pull the midpoint inward enough that the computed loop-triangle normal actually points the OPPOSITE direction from the surrounding surface. Result: same topology, reversed winding → non-manifold. Swap to topology-first: use the `flipScore vs noFlipScore` direction that partners the most existing directed boundary edges. Only fall back to the geometric refNormal check when those scores are exactly tied (ambiguous topology, only geometry can decide). This is robust against any profile curvature and is also the right default on submesh-seam cracks where the fill's face normal isn't well defined. With both fixes, profile now runs full [0, 1] range at any segment count. The safeProfile clamp + concave-disabled comments are removed from HalfEdgeMesh.h/cpp, EditModeController.h, and the QML help string. New tests: - BevelSegments8/12/16 flat (cover the loop-size cap). - BevelSegments4/8 concave (cover the winding bug). - BevelProfileShiftsIntermediateVertexPosition now asserts BOTH convex (closer to corner) AND concave (farther from corner). 93 HalfEdge + EditMode tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/EditModeController.h (1)
244-268:⚠️ Potential issue | 🟠 MajorExpose bevel session state as Q_PROPERTY with NOTIFY signals.
QML in
PropertiesPanel.qmlbinds these with property syntax (visible:,value:), but they're exposed only asQ_INVOKABLEmethods withoutQ_PROPERTYdeclarations or NOTIFY signals. WhenupdateBevelSegments()orupdateBevelProfile()modify the state, QML receives no change notification and displays stale values.Suggested direction
class EditModeController : public QObject { Q_OBJECT QML_ELEMENT QML_SINGLETON + + Q_PROPERTY(bool bevelSessionActive READ bevelSessionActive NOTIFY bevelSessionChanged) + Q_PROPERTY(int bevelSegments READ bevelSegments NOTIFY bevelSessionChanged) + Q_PROPERTY(float bevelProfile READ bevelProfile NOTIFY bevelSessionChanged)signals: void validationChanged(); + void bevelSessionChanged();Emit
bevelSessionChanged()fromupdateBevelSegments()andupdateBevelProfile()after modifying the state, and from begin/commit/cancel paths that changem_bevelSession.active.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/EditModeController.h` around lines 244 - 268, The properties exposing bevel state are currently only Q_INVOKABLE methods so QML won't get change notifications; declare Q_PROPERTY entries for bevelSessionActive (bool), bevelGizmoOrigin (Ogre::Vector3), bevelGizmoAxis (Ogre::Vector3), bevelGizmoWidth (float), bevelSegments (int) and bevelProfile (float) with a NOTIFY signal (e.g. bevelSessionChanged()), add the signal declaration (void bevelSessionChanged()), and ensure you emit bevelSessionChanged() from updateBevelSegments(int) and updateBevelProfile(float) after changing m_bevelSession, and also emit it in the begin/commit/cancel code paths that toggle m_bevelSession.active so QML bindings update.
🧹 Nitpick comments (2)
src/HalfEdgeMesh.cpp (1)
2504-2569: Dead variables after topology-first refactor.
noFlipConflicts/flipConflicts(lines 2504-2505) are only consumed by the(void)discards at 2568-2569 — the new winding-selection logic operates onnoFlipScore/flipScoredirectly. Remove both the computations and the(void)casts to drop the clutter and avoid future readers wondering which score the decision uses.🧹 Suggested cleanup
int noFlipScore = scoreWinding(false); int flipScore = scoreWinding(true); - int noFlipConflicts = -noFlipScore; - int flipConflicts = -flipScore; @@ } else { flipWinding = false; } - (void)noFlipConflicts; - (void)flipConflicts; for (size_t i = 1; i + 1 < loop.size(); ++i) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/HalfEdgeMesh.cpp` around lines 2504 - 2569, Remove the dead temporary variables noFlipConflicts and flipConflicts and their unused casts; the winding decision uses noFlipScore and flipScore directly. Locate the declarations/initializations of noFlipConflicts and flipConflicts and delete those two lines, then remove the trailing (void)noFlipConflicts; and (void)flipConflicts; statements so only the active variables (noFlipScore, flipScore, refNormal, flipWinding, etc.) remain.src/EditModeController.cpp (1)
1731-1745: Consider an upper-bound clamp onsegments.The QML
SpinBoxis limited to1..16, but thisQ_INVOKABLEis callable from any QML/C++ caller. Very large values cause each beveled edge to spawnO(segments)extra vertices and can push the post-pass hole-filler loops past the 64-vertex cap inHalfEdgeMesh::bevelEdges, silently skipping the fill. A cheap safety clamp keeps behavior aligned with the UI contract.🔧 Suggested clamp
void EditModeController::updateBevelSegments(int segments) { if (!m_bevelSession.active) return; if (segments < 1) segments = 1; + if (segments > 64) segments = 64; // matches HalfEdgeMesh hole-filler cap if (segments == m_bevelSession.segments) return;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/EditModeController.cpp` around lines 1731 - 1745, Clamp the incoming segments value to the safe UI bounds before using it in the bevel session: in EditModeController::updateBevelSegments, after the existing min clamp (segments < 1) add an upper-bound clamp (e.g. if (segments > 16) segments = 16) so callers cannot pass very large segment counts that will blow up HalfEdgeMesh::bevelEdges hole-filler loops; keep using m_bevelSession.* and applyBevelTopology as-is and only assign m_bevelSession.segments when applyBevelTopology succeeds.
🤖 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/HalfEdgeMesh.cpp`:
- Around line 1942-1956: The Phase 6 banner's bulge formula is inconsistent with
the implementation: the banner claims bulge = (profile - 0.5) * 2 * width *
sin(πt) but the code computes bulgeScale = (profile - 0.5f) * w (used with
sin(πt)), producing half the amplitude; fix by either (A) updating the banner
text to bulge = (profile - 0.5) * width * sin(πt) to match the code, or (B)
changing the calculation of bulgeScale in the corner-building code (the variable
named bulgeScale where profile and w are used) to multiply by 2 so bulgeScale =
(profile - 0.5f) * 2.0f * w to match the banner; ensure the inline comment and
any other mentions of the formula (Phase 6 banner and the nearby comments around
bulgeScale/profile/w usage) are kept consistent.
---
Outside diff comments:
In `@src/EditModeController.h`:
- Around line 244-268: The properties exposing bevel state are currently only
Q_INVOKABLE methods so QML won't get change notifications; declare Q_PROPERTY
entries for bevelSessionActive (bool), bevelGizmoOrigin (Ogre::Vector3),
bevelGizmoAxis (Ogre::Vector3), bevelGizmoWidth (float), bevelSegments (int) and
bevelProfile (float) with a NOTIFY signal (e.g. bevelSessionChanged()), add the
signal declaration (void bevelSessionChanged()), and ensure you emit
bevelSessionChanged() from updateBevelSegments(int) and
updateBevelProfile(float) after changing m_bevelSession, and also emit it in the
begin/commit/cancel code paths that toggle m_bevelSession.active so QML bindings
update.
---
Nitpick comments:
In `@src/EditModeController.cpp`:
- Around line 1731-1745: Clamp the incoming segments value to the safe UI bounds
before using it in the bevel session: in
EditModeController::updateBevelSegments, after the existing min clamp (segments
< 1) add an upper-bound clamp (e.g. if (segments > 16) segments = 16) so callers
cannot pass very large segment counts that will blow up HalfEdgeMesh::bevelEdges
hole-filler loops; keep using m_bevelSession.* and applyBevelTopology as-is and
only assign m_bevelSession.segments when applyBevelTopology succeeds.
In `@src/HalfEdgeMesh.cpp`:
- Around line 2504-2569: Remove the dead temporary variables noFlipConflicts and
flipConflicts and their unused casts; the winding decision uses noFlipScore and
flipScore directly. Locate the declarations/initializations of noFlipConflicts
and flipConflicts and delete those two lines, then remove the trailing
(void)noFlipConflicts; and (void)flipConflicts; statements so only the active
variables (noFlipScore, flipScore, refNormal, flipWinding, etc.) remain.
🪄 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: 36c73e2b-5edc-443d-b7ab-df1945b45b6b
📒 Files selected for processing (6)
qml/PropertiesPanel.qmlsrc/EditModeController.cppsrc/EditModeController.hsrc/HalfEdgeMesh.cppsrc/HalfEdgeMesh.hsrc/HalfEdgeMesh_test.cpp
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/HalfEdgeMesh_test.cpp (3)
2561-2585: Bulge-bound check assumes z-axis captures the full bulge magnitude — verify for the chosen edge.The test uses
minZNearV1as a proxy for bulge magnitude and asserts> 1 - w - 1e-3. This holds for edge 5↔3 because the neighbor-face inward direction at v1=5 (the front face,-Z) is axis-aligned, so the entire bulge lives inz. If the bulge direction is ever changed (e.g., to use an interpolated ring normal, as the earlier PR notes describe, or to a non-axis-aligned neighbor), the projectedzcomponent would underestimate the true magnitude and this bound could spuriously pass even with an over-shoot along the other axes. Consider measuring the 3D distance from the chord midpoint instead, so the bound stays meaningful regardless of bulge direction.♻️ Suggested stronger bound
- float minZNearV1 = 1.0f; - for (const auto& v : sub.vertices) { - if (v.position.x > 0.9f && v.position.y > 0.9f && v.position.z > 0.0f) - minZNearV1 = std::min(minZNearV1, v.position.z); - } - EXPECT_LT(minZNearV1, 1.0f) - << "no concave dip detected at v1 side"; - // Peak magnitude ≈ w * sin(π/2) = w. Add 1e-3 slack for floats. - EXPECT_GT(minZNearV1, 1.0f - w - 1e-3f) - << "concave bulge exceeded width magnitude"; + // Measure distance from the chamfer chord midpoint (roughly the flat- + // profile intermediate position) so the bound stays valid regardless of + // which axis the neighbor-face inward direction happens to align with. + float maxBulge = 0.0f; + bool anyDip = false; + for (const auto& v : sub.vertices) { + if (v.position.x > 0.9f && v.position.y > 0.9f && v.position.z > 0.0f + && v.position.z < 1.0f - 1e-4f) { + anyDip = true; + maxBulge = std::max(maxBulge, 1.0f - v.position.z); + } + } + EXPECT_TRUE(anyDip) << "no concave dip detected at v1 side"; + EXPECT_LT(maxBulge, w + 1e-3f) << "concave bulge exceeded width magnitude";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/HalfEdgeMesh_test.cpp` around lines 2561 - 2585, The test BevelConcaveWidthVsBulgeIsWellBounded assumes the bulge is entirely along Z; instead compute the 3D distance from each candidate vertex to the chord midpoint for the beveled edge and use that scalar to bound the bulge. Locate the test and replace the minZNearV1 logic that inspects v.position.z with: find the chord midpoint for edge e (use the two endpoint vertex positions from HalfEdgeMesh/he or from the original EditableMesh back.subMeshes()[0] as appropriate), then compute Euclidean distance from each vertex near v1 to that midpoint and take the minimum distance; assert that minDistance < 1.0f and minDistance > 1.0f - w - 1e-3f. Reference symbols: HalfEdgeMesh::bevelEdges, test BevelConcaveWidthVsBulgeIsWellBounded, findEdge(he, 5, 3), and back.subMeshes()[0].vertices.
2405-2425: Equivalence check only compares counts — consider a geometric comparison.
BevelSegments1FlatMatchesBaselineverifies that the default-arg call and the explicit(1, 0.5f)call produce the same number of vertices and triangles, but a future change that (e.g.) re-orders vertices, offsets positions slightly, or rewrites triangulation while preserving counts would pass this test despite diverging geometry. Since the whole point of this test is to pin the defaults to the pre-existing behavior, a position-level comparison would make it a much stronger regression sentinel.♻️ Suggested strengthening
EXPECT_EQ(outA.subMeshes()[0].vertices.size(), outB.subMeshes()[0].vertices.size()); EXPECT_EQ(outA.subMeshes()[0].triangles.size(), outB.subMeshes()[0].triangles.size()); + // Position-set equivalence (order-independent): every vertex in A has + // a matching vertex in B within float tolerance. + const auto& va = outA.subMeshes()[0].vertices; + const auto& vb = outB.subMeshes()[0].vertices; + for (const auto& pa : va) { + bool matched = false; + for (const auto& pb : vb) { + if (pa.position.squaredDistance(pb.position) < 1e-10f) { + matched = true; break; + } + } + EXPECT_TRUE(matched) + << "default-args vertex (" << pa.position.x << "," << pa.position.y + << "," << pa.position.z << ") has no match in (1, 0.5f) output"; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/HalfEdgeMesh_test.cpp` around lines 2405 - 2425, The test BevelSegments1FlatMatchesBaseline currently only compares vertex/triangle counts; change it to perform a geometric equivalence check by converting both HalfEdgeMesh instances to EditableMesh (use heA.toEditableMesh(outA) and heB.toEditableMesh(outB)) and then compare per-vertex positions and per-triangle indices for the first submesh: verify outA.subMeshes()[0].vertices.size()==outB... and then check that every corresponding vertex position (e.g., compare Vector3/pos fields) is equal within a small tolerance (e.g., epsilon) and every triangle index triplet matches exactly (or matches up to consistent reordering if indices may be permuted), so that bevelEdges({eA},0.05f) and bevelEdges({eB},0.05f,1,0.5f) are validated geometrically rather than just by counts.
2593-2624:runOncelambda uses EXPECT_ instead of ASSERT_ — failures cascade silently.**If
buildFromEditableMeshfails,findEdgereturns -1, orbevelEdgesreturns empty, the lambda continues and returns aminDistcomputed against potentially untouched geometry. The downstreamEXPECT_LT(dConvex, dFlat)/EXPECT_GT(dConcave, dFlat)would then fire misleading diagnostics that point at "profile doesn't shift the vertex" when the actual root cause is upstream. SinceASSERT_*can't be used in a non-void lambda, either early-return a sentinel on failure or factor the body into avoidhelper and have callers useASSERT_NO_FATAL_FAILURE.♻️ Suggested early-return guard
- auto runOnce = [](float profile) -> float { + auto runOnce = [](float profile) -> float { auto em = makeCubeMesh(); HalfEdgeMesh he; - EXPECT_TRUE(he.buildFromEditableMesh(em)); + if (!he.buildFromEditableMesh(em)) { ADD_FAILURE(); return -1.0f; } int edgeIdx = findEdge(he, 5, 3); - EXPECT_GE(edgeIdx, 0); + if (edgeIdx < 0) { ADD_FAILURE(); return -1.0f; } auto newVerts = he.bevelEdges({edgeIdx}, 0.05f, 2, profile); - EXPECT_FALSE(newVerts.empty()); + if (newVerts.empty()) { ADD_FAILURE(); return -1.0f; } EditableMesh back; - EXPECT_TRUE(he.toEditableMesh(back)); + if (!he.toEditableMesh(back)) { ADD_FAILURE(); return -1.0f; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/HalfEdgeMesh_test.cpp` around lines 2593 - 2624, The runOnce lambda uses EXPECT_* checks and can return a meaningless minDist on upstream failures; change it to return a success flag (e.g., bool runOnce(float profile, float& outMinDist)) or factor its body into a void helper (e.g., computeMinDistanceForProfile) that performs ASSERT_* (ASSERT_TRUE(buildFromEditableMesh(...)), ASSERT_GE(edgeIdx,0), ASSERT_FALSE(newVerts.empty())) before computing minDist; then in the test call the helper and ASSERT_TRUE on the returned success (or wrap helper calls with ASSERT_NO_FATAL_FAILURE) before using dFlat/dConvex/dConcave so failures in buildFromEditableMesh, findEdge, or bevelEdges are fatal and stop the test rather than producing misleading downstream EXPECTs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/HalfEdgeMesh_test.cpp`:
- Around line 2561-2585: The test BevelConcaveWidthVsBulgeIsWellBounded assumes
the bulge is entirely along Z; instead compute the 3D distance from each
candidate vertex to the chord midpoint for the beveled edge and use that scalar
to bound the bulge. Locate the test and replace the minZNearV1 logic that
inspects v.position.z with: find the chord midpoint for edge e (use the two
endpoint vertex positions from HalfEdgeMesh/he or from the original EditableMesh
back.subMeshes()[0] as appropriate), then compute Euclidean distance from each
vertex near v1 to that midpoint and take the minimum distance; assert that
minDistance < 1.0f and minDistance > 1.0f - w - 1e-3f. Reference symbols:
HalfEdgeMesh::bevelEdges, test BevelConcaveWidthVsBulgeIsWellBounded,
findEdge(he, 5, 3), and back.subMeshes()[0].vertices.
- Around line 2405-2425: The test BevelSegments1FlatMatchesBaseline currently
only compares vertex/triangle counts; change it to perform a geometric
equivalence check by converting both HalfEdgeMesh instances to EditableMesh (use
heA.toEditableMesh(outA) and heB.toEditableMesh(outB)) and then compare
per-vertex positions and per-triangle indices for the first submesh: verify
outA.subMeshes()[0].vertices.size()==outB... and then check that every
corresponding vertex position (e.g., compare Vector3/pos fields) is equal within
a small tolerance (e.g., epsilon) and every triangle index triplet matches
exactly (or matches up to consistent reordering if indices may be permuted), so
that bevelEdges({eA},0.05f) and bevelEdges({eB},0.05f,1,0.5f) are validated
geometrically rather than just by counts.
- Around line 2593-2624: The runOnce lambda uses EXPECT_* checks and can return
a meaningless minDist on upstream failures; change it to return a success flag
(e.g., bool runOnce(float profile, float& outMinDist)) or factor its body into a
void helper (e.g., computeMinDistanceForProfile) that performs ASSERT_*
(ASSERT_TRUE(buildFromEditableMesh(...)), ASSERT_GE(edgeIdx,0),
ASSERT_FALSE(newVerts.empty())) before computing minDist; then in the test call
the helper and ASSERT_TRUE on the returned success (or wrap helper calls with
ASSERT_NO_FATAL_FAILURE) before using dFlat/dConvex/dConcave so failures in
buildFromEditableMesh, findEdge, or bevelEdges are fatal and stop the test
rather than producing misleading downstream EXPECTs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cfc7e056-8694-471e-8caa-a2e177057220
📒 Files selected for processing (2)
src/HalfEdgeMesh.cppsrc/HalfEdgeMesh_test.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/HalfEdgeMesh.cpp
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/HalfEdgeMesh_test.cpp`:
- Line 2613: The test uses std::numeric_limits<float>::max() but the file
doesn't include the header; add an explicit `#include` <limits> to the test
translation unit (e.g., near other standard includes in
src/HalfEdgeMesh_test.cpp) so std::numeric_limits and
std::numeric_limits<float>::max() are defined across toolchains.
In `@src/HalfEdgeMesh.cpp`:
- Around line 2059-2071: The comment shows concave intermediates (handled where
bulgeDir and magnitude are set) are created in Phase 6 but neighbor-face
retriangulation (Phase 5) already ran, so the code either needs to ensure
concave vertex chains are wired into neighbor retriangulation before Phase 5
runs or else prevent concave bulging by clamping profile values so profile <
0.5f becomes 0.5f (i.e., force magnitude non-negative/zero for concave case).
Fix by either moving the creation/wiring of concave intermediates (the chains
used by chamfer strip triangles and referenced when building neighbor faces) to
occur before neighbor retriangulation (so bulgeDir/magnitude adjustments are
valid) or by adding a clamp in the code path that computes profile/magnitude
(used alongside bulgeDir) to enforce profile >= 0.5f and skip flipping magnitude
for concave; update the logic around bulgeDir and magnitude to reflect the
chosen approach and ensure downstream chamfer strip triangle construction still
finds consistent vertices.
- Around line 944-946: The code only enforces a lower bound for the public
parameter segments; clamp segments to the same UI upper bound (e.g. 16) to
prevent excessive allocations and work in chain.reserve(segments + 1) and the
strip emission loops and to keep it consistent with the hole-filler limit
(loop.size() > 64). Update the clamping logic that currently checks segments < 1
to also enforce segments = min(segments, 16) (or equivalent) so all downstream
uses of segments are safe.
🪄 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: 137845f6-cecf-49a1-a2ae-488bb353754d
📒 Files selected for processing (2)
src/HalfEdgeMesh.cppsrc/HalfEdgeMesh_test.cpp
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/HalfEdgeMesh_test.cpp`:
- Around line 2664-2682: The lambda runOnce uses EXPECT_* (newVerts.empty() and
he.toEditableMesh(back)) which won’t abort on failure and can lead to a crash
when accessing back.subMeshes()[0]; fix by guarding the dereference: check the
results of he.bevelEdges and he.toEditableMesh (e.g., store bool ok =
he.toEditableMesh(back); if (newVerts.empty() || !ok) return
std::numeric_limits<float>::max(); ) before accessing back.subMeshes(), or
alternatively refactor the lambda into a free helper (e.g., runOnceHelper(float
profile, float& outMinDist) or returning std::optional<float>) so the test body
can use ASSERT_* on the helper’s success and avoid dereferencing an empty
vector.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Adds ProfileGraph.qml — a draggable-dot graph that replaces the profile Slider in the bevel session controls. MVP version: single midpoint handle fixed at t=0.5, vertical drag maps to profile in [0, 1]. Click anywhere on the widget to jump the value; double-click resets to flat (0.5). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replaces the single-scalar profile with per-segment control points. For N bevel segments the graph now exposes N-1 interior handles, each with a value in [0, 1] (0.5 = flat, 1 = max outward bulge, 0 = max inward). Press-to-grab-nearest drag, double-click resets to flat, resizing the segment count resamples the curve so the shape is preserved. - HalfEdgeMesh::bevelEdges gets a vector overload that takes per-segment values directly; the scalar overload builds a sin-envelope vector and delegates. Behavior is unchanged for the scalar path — all existing tests pass. - EditModeController exposes bevelSessionActiveValue, bevelSegmentsValue and bevelProfilePointsList as Q_PROPERTY so QML bindings receive real values (not function references). Updates re-apply the bevel and emit bevelProfilePointsChanged. - The Profile graph is only shown when segments > 1; the whole bevel session panel was already gated on bevelSessionActive. - Version bump 2.28.0 → 2.28.1. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
fernandotonon
force-pushed
the
feat/bevel-segments-profile
branch
from
April 22, 2026 02:56
dcf473d to
2978d79
Compare
- Clamp bevelEdges segments to [1, 16] — the UI cap — so the public API can't overflow the hole-filler's 64-vertex loop budget. - Reconcile the Phase 6 banner comment with the implemented bulge formula (per-point linear, not sin * 2 * width). - Add <limits> and <cmath> includes to HalfEdgeMesh_test.cpp. - Lift the runOnce lambdas into named namespace-scope helpers so ASSERT_FALSE(std::isnan(...)) can abort on bevel failure instead of EXPECT_* letting downstream code crash on empty buffers. - Drop the redundant trailing return type on buildSegmentVerts. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
src/HalfEdgeMesh_test.cpp (2)
2486-2486:⚠️ Potential issue | 🟡 MinorAdd direct standard-library includes for the new test utilities.
This file now uses
std::numeric_limits,std::min_element, andstd::max_element; include<limits>and<algorithm>directly instead of relying on transitive project headers.#!/bin/bash # Description: Verify direct include coverage for standard-library utilities used by HalfEdgeMesh_test.cpp. # Expectation: The file should include <limits> and <algorithm>. rg -n '(^#include <(limits|algorithm)>|std::(numeric_limits|min_element|max_element))' --iglob 'HalfEdgeMesh_test.cpp'🧩 Proposed include fix
`#include` <gtest/gtest.h> +#include <algorithm> +#include <limits> `#include` "HalfEdgeMesh.h"Also applies to: 2579-2580
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/HalfEdgeMesh_test.cpp` at line 2486, The test uses std::numeric_limits, std::min_element and std::max_element but relies on transitive includes; add direct standard headers by including <limits> for std::numeric_limits and <algorithm> for std::min_element/std::max_element at the top of HalfEdgeMesh_test.cpp so the symbols resolve even if other headers change.
2474-2484:⚠️ Potential issue | 🟡 MinorGuard the lambda before dereferencing
back.subMeshes()[0].
EXPECT_*does not abort inside this lambda, so a failed bevel/conversion can still reachback.subMeshes()[0]and crash the test process instead of reporting the real failure.🛡️ Proposed guard
auto runOnce = [](float profile) -> float { auto em = makeCubeMesh(); HalfEdgeMesh he; - EXPECT_TRUE(he.buildFromEditableMesh(em)); + if (!he.buildFromEditableMesh(em)) return std::numeric_limits<float>::max(); int edgeIdx = findEdge(he, 5, 3); - EXPECT_GE(edgeIdx, 0); + if (edgeIdx < 0) return std::numeric_limits<float>::max(); auto newVerts = he.bevelEdges({edgeIdx}, 0.05f, 2, profile); - EXPECT_FALSE(newVerts.empty()); + if (newVerts.empty()) return std::numeric_limits<float>::max(); EditableMesh back; - EXPECT_TRUE(he.toEditableMesh(back)); + if (!he.toEditableMesh(back) || back.subMeshes().empty()) + return std::numeric_limits<float>::max(); const auto& sub = back.subMeshes()[0];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/HalfEdgeMesh_test.cpp` around lines 2474 - 2484, The lambda runOnce dereferences back.subMeshes()[0] without ensuring subMeshes() is non-empty, which can crash if he.bevelEdges or he.toEditableMesh failed; after EXPECT_TRUE(he.toEditableMesh(back)) add a guard that checks back.subMeshes().empty() (e.g., EXPECT_FALSE(back.subMeshes().empty()) and if empty return an appropriate float or early-fail) before accessing back.subMeshes()[0]; reference the runOnce lambda, he.bevelEdges, he.toEditableMesh, and back.subMeshes() when making the change.src/HalfEdgeMesh.cpp (2)
941-941:⚠️ Potential issue | 🟠 MajorClamp
segmentsto the supported maximum in both overloads.Line 941 and Line 970 only enforce the lower bound. Since this is a public API and the hole-filler cap at Line 2501 assumes realistic segment counts, clamp to the same supported max exposed by the UI/tests, e.g.
1..16.🛡️ Proposed fix
- if (segments < 1) segments = 1; + segments = std::clamp(segments, 1, 16);- if (segments < 1) segments = 1; + segments = std::clamp(segments, 1, 16);Also applies to: 970-970, 2494-2501
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/HalfEdgeMesh.cpp` at line 941, The code only enforces a lower bound on the local variable `segments` in both overloads and in the hole-filler path; add an upper bound clamp so `segments` is constrained to the supported range (1..16). Introduce a single constant (e.g. MAX_SEGMENTS = 16) and replace the existing `if (segments < 1) segments = 1;` sites (the two overloads where `segments` is validated and the hole-filler cap around the lines that build caps) with a single clamp operation (or equivalent min/max) that ensures segments = std::clamp(segments, 1, MAX_SEGMENTS) so all paths use the same limit.
942-956:⚠️ Potential issue | 🟠 MajorClamp concave profile values to flat until concave bevels are supported.
The PR states concave profiles are currently clamped to flat, but Line 956 can generate values below
0.5f, and Lines 980-983 also accept profile points below0.5f. That enables the known concave path instead of flattening it. The Phase 6 comment should also match the actual offset formula: the current implementation applies(pt - 0.5f) * w, not2 * width.🛠️ Proposed fix
- if (profile < 0.0f) profile = 0.0f; + if (profile < 0.5f) profile = 0.5f; // Concave profiles are clamped to flat for now. if (profile > 1.0f) profile = 1.0f;- if (p < 0.0f) p = 0.0f; + if (p < 0.5f) p = 0.5f; // Concave profiles are clamped to flat for now. if (p > 1.0f) p = 1.0f;- // chamfer-plane normal: bulge = (profile - 0.5) * 2 * width * sin(πt). + // chamfer-plane normal: scalar profile becomes + // (profile - 0.5) * width * sin(πt). ... - // - profile < 0.5 → concave (cut inward, groove-like). + // - profile < 0.5 → currently clamped to flat.Also applies to: 980-983, 1983-1987, 2019-2028
🤖 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 1771-1781: Before overwriting the mesh with
m_bevelSession.originalSubMeshes, capture the current preview state (e.g., copy
of m_editableMesh->subMeshes(), selected verts/edges/faces) so if
applyBevelTopology(m_bevelSession.targetEdges, m_bevelSession.width, segments,
newPoints) returns false you can restore that captured preview instead of
leaving the pre-bevel snapshot; on success continue to update
m_bevelSession.segments and m_bevelSession.profilePoints = std::move(newPoints)
and emit bevelProfilePointsChanged() as before. Apply the same pattern to the
other similar blocks that restore originalSubMeshes and then call
applyBevelTopology (the ones that update m_bevelSession.* and emit
bevelProfilePointsChanged()).
- Around line 1745-1750: Clamp the incoming segments value to the UI-supported
range (1..16) before using it to resize profile points or assign to
m_bevelSession.segments; e.g., apply a clamp to the local segments variable
(rather than relying on HalfEdgeMesh) so the newPoints vector is constructed
with the bounded count and m_bevelSession.segments is updated only with the
clamped value. This keeps the Q_INVOKABLE path, m_bevelSession.segments and the
profile-point list within the 1..16 range.
---
Duplicate comments:
In `@src/HalfEdgeMesh_test.cpp`:
- Line 2486: The test uses std::numeric_limits, std::min_element and
std::max_element but relies on transitive includes; add direct standard headers
by including <limits> for std::numeric_limits and <algorithm> for
std::min_element/std::max_element at the top of HalfEdgeMesh_test.cpp so the
symbols resolve even if other headers change.
- Around line 2474-2484: The lambda runOnce dereferences back.subMeshes()[0]
without ensuring subMeshes() is non-empty, which can crash if he.bevelEdges or
he.toEditableMesh failed; after EXPECT_TRUE(he.toEditableMesh(back)) add a guard
that checks back.subMeshes().empty() (e.g.,
EXPECT_FALSE(back.subMeshes().empty()) and if empty return an appropriate float
or early-fail) before accessing back.subMeshes()[0]; reference the runOnce
lambda, he.bevelEdges, he.toEditableMesh, and back.subMeshes() when making the
change.
In `@src/HalfEdgeMesh.cpp`:
- Line 941: The code only enforces a lower bound on the local variable
`segments` in both overloads and in the hole-filler path; add an upper bound
clamp so `segments` is constrained to the supported range (1..16). Introduce a
single constant (e.g. MAX_SEGMENTS = 16) and replace the existing `if (segments
< 1) segments = 1;` sites (the two overloads where `segments` is validated and
the hole-filler cap around the lines that build caps) with a single clamp
operation (or equivalent min/max) that ensures segments = std::clamp(segments,
1, MAX_SEGMENTS) so all paths use the same limit.
🪄 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: 34b339c4-6c67-4453-90ec-9a630e00bbae
📒 Files selected for processing (10)
CMakeLists.txtqml/ProfileGraph.qmlqml/PropertiesPanel.qmlsrc/EditModeController.cppsrc/EditModeController.hsrc/EditModeController_test.cppsrc/HalfEdgeMesh.cppsrc/HalfEdgeMesh.hsrc/HalfEdgeMesh_test.cppsrc/qml_resources.qrc
✅ Files skipped from review due to trivial changes (2)
- CMakeLists.txt
- src/qml_resources.qrc
🚧 Files skipped from review as they are similar to previous changes (1)
- src/HalfEdgeMesh.h
Comment on lines
+1745
to
+1750
| if (segments < 1) segments = 1; | ||
| if (segments == m_bevelSession.segments) return; | ||
|
|
||
| // Resample existing profile points onto the new segment count so the | ||
| // user's curve shape is preserved when the spinner moves up/down. | ||
| std::vector<float> newPoints(segments > 1 ? segments - 1 : 0, 0.5f); |
There was a problem hiding this comment.
Clamp bevel session segments to the UI-supported maximum.
Line 1745 allows arbitrary values through the Q_INVOKABLE path. Even if HalfEdgeMesh clamps internally, m_bevelSession.segments and the profile-point list can still grow without bound. Keep the controller state aligned with the exposed 1..16 range.
🛡️ Proposed fix
- if (segments < 1) segments = 1;
+ segments = std::clamp(segments, 1, 16);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/EditModeController.cpp` around lines 1745 - 1750, Clamp the incoming
segments value to the UI-supported range (1..16) before using it to resize
profile points or assign to m_bevelSession.segments; e.g., apply a clamp to the
local segments variable (rather than relying on HalfEdgeMesh) so the newPoints
vector is constructed with the bounded count and m_bevelSession.segments is
updated only with the clamped value. This keeps the Q_INVOKABLE path,
m_bevelSession.segments and the profile-point list within the 1..16 range.
Comment on lines
+1771
to
+1781
| m_editableMesh->subMeshes() = m_bevelSession.originalSubMeshes; | ||
| m_selectedVertices = m_bevelSession.origSelectedVertices; | ||
| m_selectedEdges = m_bevelSession.origSelectedEdges; | ||
| m_selectedFaces = m_bevelSession.origSelectedFaces; | ||
|
|
||
| if (applyBevelTopology(m_bevelSession.targetEdges, m_bevelSession.width, | ||
| segments, newPoints)) { | ||
| m_bevelSession.segments = segments; | ||
| m_bevelSession.profilePoints = std::move(newPoints); | ||
| emit bevelProfilePointsChanged(); | ||
| } |
There was a problem hiding this comment.
Preserve the current bevel preview when reapply fails.
These paths restore originalSubMeshes before calling applyBevelTopology(). If reapply fails, the session remains active but the mesh is left at the pre-bevel snapshot, so the previous valid preview is lost. Snapshot the current preview before restoring, and roll back to it on failure.
🧯 Proposed pattern
+ auto previewSubMeshes = m_editableMesh->subMeshes();
+ auto previewSelectedVertices = m_selectedVertices;
+ auto previewSelectedEdges = m_selectedEdges;
+ auto previewSelectedFaces = m_selectedFaces;
+
m_editableMesh->subMeshes() = m_bevelSession.originalSubMeshes;
m_selectedVertices = m_bevelSession.origSelectedVertices;
m_selectedEdges = m_bevelSession.origSelectedEdges;
m_selectedFaces = m_bevelSession.origSelectedFaces;
if (applyBevelTopology(m_bevelSession.targetEdges, m_bevelSession.width,
segments, newPoints)) {
m_bevelSession.segments = segments;
m_bevelSession.profilePoints = std::move(newPoints);
emit bevelProfilePointsChanged();
+ } else {
+ m_editableMesh->subMeshes() = std::move(previewSubMeshes);
+ m_selectedVertices = std::move(previewSelectedVertices);
+ m_selectedEdges = std::move(previewSelectedEdges);
+ m_selectedFaces = std::move(previewSelectedFaces);
+ updateSelectionOverlay();
}Also applies to: 1797-1806, 1817-1826
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/EditModeController.cpp` around lines 1771 - 1781, Before overwriting the
mesh with m_bevelSession.originalSubMeshes, capture the current preview state
(e.g., copy of m_editableMesh->subMeshes(), selected verts/edges/faces) so if
applyBevelTopology(m_bevelSession.targetEdges, m_bevelSession.width, segments,
newPoints) returns false you can restore that captured preview instead of
leaving the pre-bevel snapshot; on success continue to update
m_bevelSession.segments and m_bevelSession.profilePoints = std::move(newPoints)
and emit bevelProfilePointsChanged() as before. Apply the same pattern to the
other similar blocks that restore originalSubMeshes and then call
applyBevelTopology (the ones that update m_bevelSession.* and emit
bevelProfilePointsChanged()).
CI caught a test-fixture bug: EditModeControllerBevelE2ETest reused the mesh name "BevelE2E_cube" across every test, so the first test registered it with Ogre and every subsequent SetUp threw ItemIdentityException. The first test passed; 11 new ones failed at SetUp. Unique per-test names + MeshManager::remove in TearDown so the fixture is safe to re-enter. Sonar quality-gate cleanup on new code: - Split buildSegmentVerts into computeOutward + per-step helper (S1188). - std::clamp the profilePoints values + init-statement chordLen2 (S134, S6004). - Use `auto` for obvious local types to cut redundant Ogre::Vector3 noise (S5827). - Brace the single-line `if (m_bevelGizmo)` in commitBevel so Sonar stops reading the subsequent emit as unconditional (S2681). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Further splits the buildSegmentVerts lambda to drop it below Sonar's 20-line cap. Also uses auto for the obvious cast return type. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sonar flagged the vector-overload of bevelEdges (the one we added for per-segment profile points) as newly-introduced high-complexity code even though the Phase 1-7 body is identical to the scalar overload. Lifting that body into a private bevelEdgesImpl method leaves both public overloads as thin input-sanitizers, so the complexity is no longer attributed to new code. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Instead of a second overload and a private helper, add an optional profilePoints parameter to the pre-existing scalar bevelEdges. When empty the function synthesizes the per-segment values from `profile` via a sin envelope (old behavior); when supplied, it uses those values directly. Keeps the Phase 1-7 body on its original function signature so Sonar stops re-attributing the existing complexity as new code. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The Phase 1-7 bevel topology body shipped with complexity >1000 before this PR; refactoring it into phase-sized helpers is meaningful work outside the scope of adding per-segment profile support. Use NOSONAR on the signature with a justification comment so the PR quality gate can pass on everything else. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
This was referenced Apr 24, 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 Blender-style bevel controls:
Plumbed through:
HalfEdgeMesh::bevelEdges(edges, width, segments=1, profile=0.5)— backwards-compatible defaults so all existing callers compile unchanged.EditModeController::BevelSession+ newQ_INVOKABLE updateBevelSegments(int)/updateBevelProfile(float)(mirrorsupdateBevelWidth).qml/PropertiesPanel.qml— Bevel section appears only while a bevel session is active, with a SpinBox (1..16) and a Slider.Known limitation
Concave (profile < 0.5) currently clamps to flat. Even a tiny inward bulge (profile=0.45 with segments=2) flips Phase 7's corner-cap winding for a subset of triangles — the emitted face indices match the flat case in count but get reversed argument order in
appendTriangle. Topology is identical between flat and concave (chains,f1WalksAB, all index lookups) — the divergence is somewhere downstream that reads positions from the in-progress mesh state. Tracked as a follow-up; convex covers the most common UX (rounded fillet) so the feature is still useful while the bug is open.Tests
BevelSegments1FlatMatchesBaseline— explicit defaults match implicitBevelSegments2/3/4FlatStillManifoldBevelSegments4ConvexStillManifoldBevelProfileConvexShiftsIntermediateVertexPosition— convex bulge moves the chamfer midpoint closer to the cut-off cornerManual test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Tests
Chores