fix(bevel+gizmo): reach edge midpoints, freeze shaft, screen-correct X, stable size - #306
Conversation
…orrect X arrow, stable size Addresses several issues users hit on the translate/scale/bevel gizmos: - Multi-vertex bevel now actually reaches the shared edge midpoint. The pre-budget clamp was 0.49*edgeLen (so two adjacent bevels on a length-2 edge stopped at ~0.49 each, leaving half the edge untouched); bumped to 0.999*edgeLen and added an internal skip-flag so the single-vertex path honors the pre-budgeted width instead of re-clamping against the now-mutated mesh. - Interactive bevel shaft now stops growing when the mesh bevel caps. BevelSession now precomputes maxWidth from the pre-bevel mesh; the drag handler clamps both the applied width and the gizmo handle offset against it, so the visible shaft freezes the instant the bevel stops changing. - Translate/Scale X arrow geometry flipped to point toward screen-right to match the ViewCube indicator. The viewport camera is set up looking from -Z toward +Z, which visually flips world +X across the screen; picking/drag math already use screen-consistent coords, so only the geometry plus the X bounding-box extents needed to flip. Explicit bbox is also re-asserted on each hover rebuild so the pickable region stays aligned with the flipped geometry. - Transform gizmos now appear at the correct distance-scaled size on the first frame. OgreWidget's frameStarted gated the per-frame scale tick on TransformOperator::getActiveWidget()==this, which was null until the user first clicked a viewport; relaxed the gate so any viewport ticks when no active widget is registered yet. - Scene Tree (Inspector) now hides BevelGizmo scaffold nodes. Manager's isForbiddenNodeName drops any node ending in Gizmo_Node/Gizmo_Shaft/ Gizmo_Handle so future gizmos following the same naming convention stay out of the tree automatically. Tests: 97/97 HalfEdgeMeshStandalone pass; the symmetric-budget test now asserts the new ~0.999 offset instead of the old 0.49. 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 50 minutes and 44 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 (1)
📝 WalkthroughWalkthroughBevel sessions now compute and store a per-session maxWidth from a reconstructed HalfEdgeMesh; drag updates clamp requested bevel widths to that max and adjust gizmo visuals. X-axis geometry for scale/translate gizmos is flipped to -X, gizmo ticking gets an initial-viewport fallback, and three transient bevel nodes are excluded from the scene tree. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User (drag)
participant Edit as EditModeController
participant Mesh as HalfEdgeMesh
participant Gizmo as BevelGizmo
participant View as OgreWidget
User->>Edit: beginBevel(selection)
Edit->>Mesh: reconstruct & evaluate budgets
Mesh-->>Edit: per-session maxWidth
Edit->>Gizmo: init with session maxWidth
User->>Edit: drag(newRequestedWidth)
Edit->>Mesh: (optional) compute effective bevel result
Edit->>Edit: clamp newWidth = min(requested, maxWidth)
Edit->>Gizmo: update handle/shaft using clamped width
View->>Gizmo: tick (initial viewport fallback if no active widget)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/HalfEdgeMesh_test.cpp (1)
2955-2979:⚠️ Potential issue | 🟡 MinorAssert the offsets stay on opposite sides of the midpoint.
These checks still pass if both new points collapse exactly to
x == 0:dist4 == dist5 == 1.0fis within the current0.999f ± 0.002ftolerance. That misses the regression this test comment is trying to prevent. Please also assert that the two shared-edge offsets straddle zero and leave the expected nonzero gap.Possible tightening
EXPECT_NEAR(dist4, dist5, 1e-3f) << "offsets at each end of shared edge should be symmetric"; EXPECT_NEAR(dist4, 0.999f, 2e-3f) << "each side should reach to the midpoint of the 2-unit shared edge"; + EXPECT_LT(onSharedEdge[0].x * onSharedEdge[1].x, 0.0f) + << "offsets should remain on opposite sides of the midpoint"; + EXPECT_NEAR(std::abs(onSharedEdge[0].x - onSharedEdge[1].x), 0.002f, 2e-3f) + << "offsets should leave the expected sliver instead of collapsing to the midpoint";As per coding guidelines, "src/**/*_test.cpp: Add Google Test unit tests for new functionality."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/HalfEdgeMesh_test.cpp` around lines 2955 - 2979, The test currently only checks distances but misses the case where both offsets collapse to x==0; update the assertions after collecting onSharedEdge (from he.bevelVertices and he.vertex(...).position) to also verify the two shared-edge offset points straddle the midpoint: assert one point has x < 0 and the other x > 0 (i.e., opposite signs), and assert their absolute separation along x (or full distance) is nonzero and close to the expected sliver width (~0.002) — e.g., check abs(x0 - x1) > a small epsilon and is within a tolerance around 0.002 — to ensure offsets do not both collapse to zero.src/HalfEdgeMesh.cpp (1)
2931-2952:⚠️ Potential issue | 🔴 CriticalKeep the old 0.499 safety cap on unshared edges.
With
ScopedSkipClampenabled,share == 1.0fnow lets a multi-vertex bevel use0.999 * edgeLenon unshared crease edges. That bypasses the old0.499 * minEdgeLenguard entirely, so a selection that includes any non-shared corner can drive its offset almost onto the neighboring vertex and produce degenerate/inverted retriangulation. The0.999 * 0.5relaxation should only apply to edges whose far endpoint is also selected.Suggested fix
- float share = selected.count(n) ? 0.5f : 1.0f; - float budget = edgeLen * 0.999f * share; + const float budget = edgeLen + * (selected.count(n) ? (0.999f * 0.5f) : 0.499f);Also applies to: 3131-3133
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/HalfEdgeMesh.cpp` around lines 2931 - 2952, The current logic lets unshared edges use budget = edgeLen * 0.999f (share == 1.0f) which bypasses the old 0.499 safety cap and can produce degenerate geometry; change the budget computation so unshared (far endpoint not selected) edges retain the conservative cap (e.g., enforce budget = min(budget, edgeLen * 0.499f) when selected.count(n) is false). Update the block that computes share/budget (variables: share, budget, edgeLen, selected, minBudget, perVertexWidth) and replicate the same safe-cap change at the other occurrence referenced in the comment (around lines 3131-3133), leaving ScopedSkipClamp and the rest 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.cpp`:
- Around line 1782-1872: The session maxWidth calculation must match
HalfEdgeMesh's own budget logic: for vertex bevels, compute per-incident-edge
budget as 0.499f × shortestIncident × share where share = 0.5 if the opposite
vertex is also selected and 1.0 otherwise, then take the min across all incident
edges (mirror HalfEdgeMesh::bevelVertices); for edge bevels, for each selected
edge (pair gV1,gV2) find both incident faces (check both half-edge orientations
or locate both half-edges for v1→v2 and v2→v1), compute the shortest of the edge
length and the two opposite-vertex legs, then cap that edge at 0.4f ×
shortestAdjacent and take the min across selected edges (mirror HalfEdgeMesh's
edge cap logic); replace the current ad-hoc 0.999f factor and the one-sided face
walk with these exact formulas and assign s.maxWidth to the resulting cap only
if finite and positive.
In `@src/Manager.cpp`:
- Around line 791-797: The global suffix checks added in isForbiddenNodeName()
are unsafe; instead mark gizmo scaffolding explicitly and check that marker
everywhere those suffixes were relied on. Remove the
endsWith("Gizmo_Node"/"Gizmo_Shaft"/"Gizmo_Handle") checks from
isForbiddenNodeName(), add an explicit boolean flag or metadata on nodes (e.g.,
node->setGizmoScaffold(true) or node->meta["gizmo_scaffold"]=true) when
BevelGizmo (and any other gizmo factory) creates the "_Node"/"_Shaft"/"_Handle"
children, and update destroySceneNode(), the recursive scene/entity collectors,
and any Manager code that called isForbiddenNodeName() to consult that explicit
flag instead of name suffixes so user/imported nodes with those suffixes are not
hidden or protected.
In `@src/OgreWidget.cpp`:
- Around line 199-212: When transform->getActiveWidget() is nullptr the code
runs in every OgreWidget, causing multiple cameras to race when calling
EditModeController::instance()->tickBevelGizmo(...) and
transform->tickTransformGizmoScale(...); change the branch so the nullptr
fallback is handled by exactly one deterministic widget only (not all
viewports). Concretely: keep the existing active==this path, but replace the
active==nullptr branch with a deterministic check (for example add/use a
TransformOperator method like getWidgetForCamera(Camera*) or getPrimaryWidget()
and only run the gizmo tick when active==nullptr &&
transform->getWidgetForCamera(mCamera->getCamera())==this). Update the code that
calls EditModeController::instance()->tickBevelGizmo(camera) and
transform->tickTransformGizmoScale(camera) accordingly so only that single
chosen widget performs the ticks.
---
Outside diff comments:
In `@src/HalfEdgeMesh_test.cpp`:
- Around line 2955-2979: The test currently only checks distances but misses the
case where both offsets collapse to x==0; update the assertions after collecting
onSharedEdge (from he.bevelVertices and he.vertex(...).position) to also verify
the two shared-edge offset points straddle the midpoint: assert one point has x
< 0 and the other x > 0 (i.e., opposite signs), and assert their absolute
separation along x (or full distance) is nonzero and close to the expected
sliver width (~0.002) — e.g., check abs(x0 - x1) > a small epsilon and is within
a tolerance around 0.002 — to ensure offsets do not both collapse to zero.
In `@src/HalfEdgeMesh.cpp`:
- Around line 2931-2952: The current logic lets unshared edges use budget =
edgeLen * 0.999f (share == 1.0f) which bypasses the old 0.499 safety cap and can
produce degenerate geometry; change the budget computation so unshared (far
endpoint not selected) edges retain the conservative cap (e.g., enforce budget =
min(budget, edgeLen * 0.499f) when selected.count(n) is false). Update the block
that computes share/budget (variables: share, budget, edgeLen, selected,
minBudget, perVertexWidth) and replicate the same safe-cap change at the other
occurrence referenced in the comment (around lines 3131-3133), leaving
ScopedSkipClamp and the rest 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: bdd80b32-dc1f-4a0b-a0c8-abcb2826dccf
📒 Files selected for processing (8)
src/EditModeController.cppsrc/EditModeController.hsrc/HalfEdgeMesh.cppsrc/HalfEdgeMesh_test.cppsrc/Manager.cppsrc/OgreWidget.cppsrc/ScaleGizmo.cppsrc/TranslationGizmo.cpp
| ||_name.startsWith("Unnamed_") //This is the cameras's nodes | ||
| // Gizmo scaffolding nodes (BevelGizmo creates "<name>_Node" with | ||
| // "<name>_Shaft" / "<name>_Handle" children). Any gizmo following | ||
| // the same *_Node/*_Shaft/*_Handle convention is hidden here. | ||
| ||_name.endsWith("Gizmo_Node") | ||
| ||_name.endsWith("Gizmo_Shaft") | ||
| ||_name.endsWith("Gizmo_Handle")); |
There was a problem hiding this comment.
Don't use a global suffix blacklist for transient gizmos.
isForbiddenNodeName() is not just a Scene Tree filter; destroySceneNode() and the recursive scene/entity collectors also rely on it. With these new suffixes, any real imported or user-created node whose name ends with Gizmo_Node, Gizmo_Shaft, or Gizmo_Handle will disappear from the tree and become undeletable through Manager. Please mark gizmo scaffolding explicitly instead of hiding arbitrary names by suffix.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Manager.cpp` around lines 791 - 797, The global suffix checks added in
isForbiddenNodeName() are unsafe; instead mark gizmo scaffolding explicitly and
check that marker everywhere those suffixes were relied on. Remove the
endsWith("Gizmo_Node"/"Gizmo_Shaft"/"Gizmo_Handle") checks from
isForbiddenNodeName(), add an explicit boolean flag or metadata on nodes (e.g.,
node->setGizmoScaffold(true) or node->meta["gizmo_scaffold"]=true) when
BevelGizmo (and any other gizmo factory) creates the "_Node"/"_Shaft"/"_Handle"
children, and update destroySceneNode(), the recursive scene/entity collectors,
and any Manager code that called isForbiddenNodeName() to consult that explicit
flag instead of name suffixes so user/imported nodes with those suffixes are not
hidden or protected.
![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: 034aae76e8
ℹ️ 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".
| ScopedSkipClamp skip(true); | ||
| for (size_t i = 0; i < vertexIndices.size(); ++i) { | ||
| auto added = bevelVertices({vertexIndices[i]}, perVertexWidth[i], | ||
| segments, profile, profilePointsIn); |
There was a problem hiding this comment.
This unconditionally enables ScopedSkipClamp for every multi-vertex bevel, but perVertexWidth uses share = 1.0 on unshared edges, so disconnected selections can drive offsets up to 0.999 * edgeLen with no fallback to the single-vertex 0.499 * minEdgeLen guard. A practical case is selecting two far-apart cube corners and dragging wide: each corner can bevel almost to its neighbors, producing near-degenerate sliver faces and behavior that is much more aggressive than single-vertex beveling.
Useful? React with 👍 / 👎.
| if (hm.halfEdge(he).vertex == v2) { | ||
| // This face contains the v1→v2 edge. Find the |
There was a problem hiding this comment.
The cap precomputation for edge bevel only checks outgoing half-edges from v1 and matches hm.halfEdge(he).vertex == v2, which in a manifold mesh identifies only the v1→v2 side of the edge. That means shortestAdj is derived from at most one adjacent face, so on asymmetric edges m_bevelSession.maxWidth can be larger than the real bevel clamp; the mesh stops changing first while the gizmo handle keeps sliding, which is exactly the freeze mismatch this change is trying to prevent.
Useful? React with 👍 / 👎.
- SceneTreeModel: gizmo filter moved out of Manager::isForbiddenNodeName (which also gates destroySceneNode and the entity collector, and a suffix match there could hide/block user meshes named "MyGizmo_Node" etc). The filter is now an explicit exact-name check in SceneTreeModel::buildChildren where only the Scene Tree view is affected. Manager.cpp reverted to its original whitelist. - HalfEdgeMesh::bevelVertices pre-budget: unshared-edge budget dropped from 0.999 × edgeLen to 0.499 × edgeLen so disconnected multi-vertex selections don't bypass the safety clamp when ScopedSkipClamp is active. Shared-edge budget keeps its 0.999 × 0.5 symmetric behavior. EditModeController's mirror precompute updated to match. - EditModeController edge-bevel maxWidth: previously walked only v1's outgoing HEs and matched he.vertex == v2, which saw only the v1→v2 adjacent face (f1) and missed f2's opposite vertex. Split the walk into a reusable clampAgainstFace helper invoked from both ends so the shortest-adjacent computation mirrors HalfEdgeMesh::bevelEdges's effectiveWidth(). - OgreWidget::frameStarted: the nullptr-active fallback restricted to the first viewport (getIndex() == 0) so multi-viewport layouts no longer have N frame listeners racing on the shared gizmo singleton before any viewport gets focus. Tests: 97/97 HalfEdgeMeshStandalone still pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/EditModeController.cpp (1)
1782-1888:⚠️ Potential issue | 🟠 MajorSingle-vertex
maxWidthstill doesn't mirrorHalfEdgeMesh::bevelVertices().This budgets against every outgoing half-edge, but the runtime single-vertex path only clamps against
creaseTargetsafter collapsing coplanar runs (src/HalfEdgeMesh.cpp:3049-3135). On triangulated planar faces,s.maxWidthcan therefore end up smaller than the width the mesh would actually accept, so the gizmo freezes early. Please mirror the single-vertex crease-based clamp here as well, and clamps.widthto the computed cap before the firstapplyBevel*()call so already-capped sessions don't start from an unreachable width and snap backward on first drag.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/EditModeController.cpp` around lines 1782 - 1888, The current vertex-cap computation budgets every outgoing half-edge but doesn't reproduce HalfEdgeMesh::bevelVertices's crease-based clamp (collapse coplanar runs into creaseTargets), causing s.maxWidth to be more restrictive than the mesh's actual limit; update the Vertices branch to mirror bevelVertices by computing the same creaseTargets (collapse coplanar runs and mark crease edges) and only budget against those creaseTargets per-vertex (using the same 0.999×0.5 and 0.499 factors), then assign s.maxWidth = cap and immediately clamp s.width = std::min(s.width, s.maxWidth) before calling applyBevelVertexTopology/applyBevelTopology so sessions don't start at an unreachable width.
🤖 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/SceneTreeModel.cpp`:
- Around line 111-120: The name-filter for
"BevelGizmo_Node"/"BevelGizmo_Shaft"/"BevelGizmo_Handle" should only apply to
nodes that are direct children of the scene root; update the condition in
SceneTreeModel.cpp (the block containing the if (name == "...") continue; check)
to also verify the node's parent is the root (e.g., compare node->parent() or
parentId to the root node/ID) before skipping, so nested user nodes with those
exact names are not hidden.
---
Duplicate comments:
In `@src/EditModeController.cpp`:
- Around line 1782-1888: The current vertex-cap computation budgets every
outgoing half-edge but doesn't reproduce HalfEdgeMesh::bevelVertices's
crease-based clamp (collapse coplanar runs into creaseTargets), causing
s.maxWidth to be more restrictive than the mesh's actual limit; update the
Vertices branch to mirror bevelVertices by computing the same creaseTargets
(collapse coplanar runs and mark crease edges) and only budget against those
creaseTargets per-vertex (using the same 0.999×0.5 and 0.499 factors), then
assign s.maxWidth = cap and immediately clamp s.width = std::min(s.width,
s.maxWidth) before calling applyBevelVertexTopology/applyBevelTopology so
sessions don't start at an unreachable width.
🪄 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: bff89eef-967d-41d3-93d9-1b2a8b75bca1
📒 Files selected for processing (4)
src/EditModeController.cppsrc/HalfEdgeMesh.cppsrc/OgreWidget.cppsrc/SceneTreeModel.cpp
| // Hide transient gizmo scaffolding that editor code creates as | ||
| // children of the root scene node (currently: BevelGizmo's handle | ||
| // rig). Matched on the exact names the gizmo creates — see | ||
| // BevelGizmo.cpp — so a user mesh sharing the suffix doesn't | ||
| // vanish from the tree. A future gizmo with different names | ||
| // needs to add its own entries here. | ||
| if (name == "BevelGizmo_Node" | ||
| || name == "BevelGizmo_Shaft" | ||
| || name == "BevelGizmo_Handle") | ||
| continue; |
There was a problem hiding this comment.
Scope the BevelGizmo-name filter to root children only.
These names are only used for root-attached bevel gizmo scaffolding, but this check runs for every subtree. A real user node nested elsewhere with one of these exact names will still disappear from the Scene Tree.
Suggested fix
- if (name == "BevelGizmo_Node"
- || name == "BevelGizmo_Shaft"
- || name == "BevelGizmo_Handle")
+ const bool parentIsRoot =
+ sceneNode == Manager::getSingleton()->getSceneMgr()->getRootSceneNode();
+ if (parentIsRoot && (name == "BevelGizmo_Node"
+ || name == "BevelGizmo_Shaft"
+ || name == "BevelGizmo_Handle"))
continue;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Hide transient gizmo scaffolding that editor code creates as | |
| // children of the root scene node (currently: BevelGizmo's handle | |
| // rig). Matched on the exact names the gizmo creates — see | |
| // BevelGizmo.cpp — so a user mesh sharing the suffix doesn't | |
| // vanish from the tree. A future gizmo with different names | |
| // needs to add its own entries here. | |
| if (name == "BevelGizmo_Node" | |
| || name == "BevelGizmo_Shaft" | |
| || name == "BevelGizmo_Handle") | |
| continue; | |
| // Hide transient gizmo scaffolding that editor code creates as | |
| // children of the root scene node (currently: BevelGizmo's handle | |
| // rig). Matched on the exact names the gizmo creates — see | |
| // BevelGizmo.cpp — so a user mesh sharing the suffix doesn't | |
| // vanish from the tree. A future gizmo with different names | |
| // needs to add its own entries here. | |
| const bool parentIsRoot = | |
| sceneNode == Manager::getSingleton()->getSceneMgr()->getRootSceneNode(); | |
| if (parentIsRoot && (name == "BevelGizmo_Node" | |
| || name == "BevelGizmo_Shaft" | |
| || name == "BevelGizmo_Handle")) | |
| continue; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/SceneTreeModel.cpp` around lines 111 - 120, The name-filter for
"BevelGizmo_Node"/"BevelGizmo_Shaft"/"BevelGizmo_Handle" should only apply to
nodes that are direct children of the scene root; update the condition in
SceneTreeModel.cpp (the block containing the if (name == "...") continue; check)
to also verify the node's parent is the root (e.g., compare node->parent() or
parentId to the root node/ID) before skipping, so nested user nodes with those
exact names are not hidden.
SonarCloud flagged B Maintainability on the PR's new code. The worst
offender was beginBevel's inline maxWidth precompute: 80+ lines of
nested fan walks inside an anonymous brace block, a 28-line lambda,
and triple-nested break/return sentinels — all of which Sonar also
flagged individually.
- EditModeController.cpp: extract the precompute into three named
free helpers in an anonymous namespace at the top of the file:
- nextAroundVertex() — one step of the prev→twin fan
rotation; returns -1 as a single
terminate-iteration sentinel, so
each call site is a flat for-loop.
- computeVertexBevelCap() — mirror of HalfEdgeMesh::
bevelVertices's pre-budget.
- computeEdgeBevelCap() + — mirror of HalfEdgeMesh::
shrinkEdgeBevelAdjacency() bevelEdges::effectiveWidth.
beginBevel now contains a two-line call that picks the right helper.
No more than 2 nesting levels, no nested break triples, no 28-line
lambdas.
- HalfEdgeMesh: replace the thread_local ScopedSkipClamp RAII struct
(Sonar flagged it for const-global + rule-of-five) with a private
member flag `m_skipVertexBevelClamp`. The multi-vertex path sets it
around the recursive loop and clears it afterwards. Same behavior,
no thread-local state, no class that needs rule-of-five.
- TranslationGizmo::createAxis bbox lambda: collapse the duplicate
"negative range" branch (X axis, and Z under left-handed cs) into a
single `flipAxis` bool. Sonar-flagged as duplicate branch.
- OgreWidget::frameStarted: `active` typed as pointer-to-const (Sonar
minor). No behavior change.
Tests: 97/97 HalfEdgeMeshStandalone still pass. App builds clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The existing edit_mode breadcrumbs for bevel begin/commit/cancel and extrudeSelection were plain strings, so a Sentry crash trace couldn't tell you the scale of the operation in flight. Enrich them so post- mortem diagnostics carry the useful numbers: - "Extrude selection" → "Extrude selection (faces=N)". - "Bevel: begin session" → "Bevel: begin session (edges|vertices=N)". - "Bevel: commit" → "Bevel: commit (width=X, segments=N)". - "Bevel: cancel" → "Bevel: cancel (width=X, segments=N)". No behavior change; ui.action / ui.shortcut / ui.transform breadcrumbs emitted by the toolbar/shortcut callers are untouched. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/HalfEdgeMesh.h (1)
431-438: Consider an RAII guard for this flag to keep exception safety.
bevelVerticesdoes many allocations (vectors, unordered_maps, repeatedm_vertices.push_back, polygon/chain growth, ear-clipping scratch buffers). Any of those can throwstd::bad_alloc, and the manualm_skipVertexBevelClamp = true; ... = false;pair in the.cppisn't exception-safe — a throw between set and reset leaves the flag latched on, and the next unrelated top-levelbevelVerticescall on this mesh would silently skip the single-vertex safety clamp and be able to over-reach against the pristine-mesh safety bound.A per-instance scope guard keeps the stated intent of dropping
thread_localwhile closing the window:🛡️ Proposed fix
private: ... bool m_skipVertexBevelClamp = false; + + // Scoped toggler for m_skipVertexBevelClamp used by the multi-vertex + // bevel path. Ensures the flag is reset even if the recursive single- + // vertex call throws (std::bad_alloc from the many internal vectors). + struct SkipVertexBevelClampGuard { + HalfEdgeMesh& mesh; + bool prev; + explicit SkipVertexBevelClampGuard(HalfEdgeMesh& m) + : mesh(m), prev(m.m_skipVertexBevelClamp) + { mesh.m_skipVertexBevelClamp = true; } + ~SkipVertexBevelClampGuard() { mesh.m_skipVertexBevelClamp = prev; } + SkipVertexBevelClampGuard(const SkipVertexBevelClampGuard&) = delete; + SkipVertexBevelClampGuard& operator=(const SkipVertexBevelClampGuard&) = delete; + };And at the use site in
HalfEdgeMesh.cpparound the multi-vertex branch:- m_skipVertexBevelClamp = true; - for (size_t i = 0; i < vertexIndices.size(); ++i) { - auto added = bevelVertices({vertexIndices[i]}, perVertexWidth[i], - segments, profile, profilePointsIn); - newVertices.insert(newVertices.end(), added.begin(), added.end()); - } - m_skipVertexBevelClamp = false; + { + SkipVertexBevelClampGuard guard(*this); + for (size_t i = 0; i < vertexIndices.size(); ++i) { + auto added = bevelVertices({vertexIndices[i]}, perVertexWidth[i], + segments, profile, profilePointsIn); + newVertices.insert(newVertices.end(), added.begin(), added.end()); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/HalfEdgeMesh.h` around lines 431 - 438, The field m_skipVertexBevelClamp is toggled manually in bevelVertices and can be left set if an exception (e.g., std::bad_alloc) is thrown; create a small RAII guard (e.g., SkipVertexBevelClampGuard) that takes a HalfEdgeMesh& in its constructor, sets mesh.m_skipVertexBevelClamp = true, and restores the previous value in its destructor, then replace the manual m_skipVertexBevelClamp = true/false pair in the multi-vertex pre-budgeted branch of bevelVertices with a scoped instance of this guard so the flag is reset on all exit paths.src/EditModeController.cpp (1)
1905-1913: Optional: avoid buildingHalfEdgeMeshtwice duringbeginBevel.
applyBevelTopology/applyBevelVertexTopologyeach callheMesh.buildFromEditableMesh(*m_editableMesh)internally, so on everybeginBevelthe half-edge structure is built twice — once here for the cap, and once again a few lines down to do the actual topology op. For typical interactive meshes the cost is fine and only paid on session start, but if this ever shows up in profiles on dense meshes, consider factoring theHalfEdgeMeshconstruction so both the cap computation and the apply path can share it (e.g., a variant ofapplyBevel*that accepts an already-builtHalfEdgeMesh&). No functional problem; flagging for awareness.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/EditModeController.cpp` around lines 1905 - 1913, beginBevel currently constructs a local HalfEdgeMesh via heMesh.buildFromEditableMesh(*m_editableMesh) to compute the cap and then applyBevelTopology/applyBevelVertexTopology rebuilds it again; to avoid the duplicate build, factor the HalfEdgeMesh creation out of beginBevel and the apply functions by creating the HalfEdgeMesh once and passing it into the apply path—either add overloads applyBevelTopology(HalfEdgeMesh&, const BevelSession&) / applyBevelVertexTopology(HalfEdgeMesh&, const BevelSession&) or modify existing applyBevel* to accept a prebuilt HalfEdgeMesh&; call heMesh.buildFromEditableMesh(*m_editableMesh) once in beginBevel, use computeVertexBevelCap/computeEdgeBevelCap on that instance, then pass the same HalfEdgeMesh& into the applyBevel* call so the mesh is not rebuilt.
🤖 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 1896-1917: The initial s.width (set to 0.05f) must be clamped to
the computed cap so the session's startWidth never exceeds s.maxWidth; after
computing s.maxWidth via computeVertexBevelCap/computeEdgeBevelCap (the block
that constructs HalfEdgeMesh hm and calls buildFromEditableMesh), set s.width =
std::min(s.width, s.maxWidth) (or equivalent) before calling applyBevelTopology
/ applyBevelVertexTopology so the recorded session width matches the actual
applied/clamped width.
---
Nitpick comments:
In `@src/EditModeController.cpp`:
- Around line 1905-1913: beginBevel currently constructs a local HalfEdgeMesh
via heMesh.buildFromEditableMesh(*m_editableMesh) to compute the cap and then
applyBevelTopology/applyBevelVertexTopology rebuilds it again; to avoid the
duplicate build, factor the HalfEdgeMesh creation out of beginBevel and the
apply functions by creating the HalfEdgeMesh once and passing it into the apply
path—either add overloads applyBevelTopology(HalfEdgeMesh&, const BevelSession&)
/ applyBevelVertexTopology(HalfEdgeMesh&, const BevelSession&) or modify
existing applyBevel* to accept a prebuilt HalfEdgeMesh&; call
heMesh.buildFromEditableMesh(*m_editableMesh) once in beginBevel, use
computeVertexBevelCap/computeEdgeBevelCap on that instance, then pass the same
HalfEdgeMesh& into the applyBevel* call so the mesh is not rebuilt.
In `@src/HalfEdgeMesh.h`:
- Around line 431-438: The field m_skipVertexBevelClamp is toggled manually in
bevelVertices and can be left set if an exception (e.g., std::bad_alloc) is
thrown; create a small RAII guard (e.g., SkipVertexBevelClampGuard) that takes a
HalfEdgeMesh& in its constructor, sets mesh.m_skipVertexBevelClamp = true, and
restores the previous value in its destructor, then replace the manual
m_skipVertexBevelClamp = true/false pair in the multi-vertex pre-budgeted branch
of bevelVertices with a scoped instance of this guard so the flag is reset on
all exit paths.
🪄 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: e5d6e4a4-0474-40b3-97d0-e5da88cdadd7
📒 Files selected for processing (5)
src/EditModeController.cppsrc/HalfEdgeMesh.cppsrc/HalfEdgeMesh.hsrc/OgreWidget.cppsrc/TranslationGizmo.cpp
| s.width = 0.05f; // 2.5% of a 2-unit cube — visible initial chamfer | ||
|
|
||
| // Compute the largest width the bevel algorithm will actually apply | ||
| // before its internal per-vertex/per-edge clamp takes over, so the | ||
| // drag handler can freeze the gizmo at the same scalar the topology | ||
| // op would freeze at. See the named helpers at the top of this file | ||
| // (computeVertexBevelCap / computeEdgeBevelCap) for the exact | ||
| // formulas, which mirror HalfEdgeMesh::bevelVertices and | ||
| // HalfEdgeMesh::bevelEdges respectively. | ||
| { | ||
| HalfEdgeMesh hm; | ||
| if (hm.buildFromEditableMesh(*m_editableMesh)) { | ||
| const float cap = (s.kind == BevelSession::Vertices) | ||
| ? computeVertexBevelCap(hm, s.targetVertices) | ||
| : computeEdgeBevelCap(hm, s.targetEdges); | ||
| if (cap > 0.0f && std::isfinite(cap)) s.maxWidth = cap; | ||
| } | ||
| } | ||
|
|
||
| const bool applied = (s.kind == BevelSession::Edges) | ||
| ? applyBevelTopology(s.targetEdges, s.width) | ||
| : applyBevelVertexTopology(s.targetVertices, s.width); |
There was a problem hiding this comment.
Clamp initial s.width to the computed s.maxWidth.
s.width is hard-coded to 0.05f on line 1896 and then passed to applyBevel* on lines 1915–1917 without being reconciled against the cap just computed above. On a small mesh where s.maxWidth happens to be below 0.05f, the bevel algorithm internally clamps the applied width (so the mesh looks right) but the session still records width = 0.05f. The first drag then starts with startWidth > s.maxWidth, so updateBevelFromDrag enters its capped branch immediately and the user has to drag through a dead zone (roughly startWidth - maxWidth worth of motion) before the bevel visibly responds to shrinking, and the handle jumps off its default tip position on the very first frame.
🛠️ Proposed fix — reconcile initial width with the cap
{
HalfEdgeMesh hm;
if (hm.buildFromEditableMesh(*m_editableMesh)) {
const float cap = (s.kind == BevelSession::Vertices)
? computeVertexBevelCap(hm, s.targetVertices)
: computeEdgeBevelCap(hm, s.targetEdges);
- if (cap > 0.0f && std::isfinite(cap)) s.maxWidth = cap;
+ if (cap > 0.0f && std::isfinite(cap)) {
+ s.maxWidth = cap;
+ if (s.width > s.maxWidth) s.width = s.maxWidth;
+ }
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| s.width = 0.05f; // 2.5% of a 2-unit cube — visible initial chamfer | |
| // Compute the largest width the bevel algorithm will actually apply | |
| // before its internal per-vertex/per-edge clamp takes over, so the | |
| // drag handler can freeze the gizmo at the same scalar the topology | |
| // op would freeze at. See the named helpers at the top of this file | |
| // (computeVertexBevelCap / computeEdgeBevelCap) for the exact | |
| // formulas, which mirror HalfEdgeMesh::bevelVertices and | |
| // HalfEdgeMesh::bevelEdges respectively. | |
| { | |
| HalfEdgeMesh hm; | |
| if (hm.buildFromEditableMesh(*m_editableMesh)) { | |
| const float cap = (s.kind == BevelSession::Vertices) | |
| ? computeVertexBevelCap(hm, s.targetVertices) | |
| : computeEdgeBevelCap(hm, s.targetEdges); | |
| if (cap > 0.0f && std::isfinite(cap)) s.maxWidth = cap; | |
| } | |
| } | |
| const bool applied = (s.kind == BevelSession::Edges) | |
| ? applyBevelTopology(s.targetEdges, s.width) | |
| : applyBevelVertexTopology(s.targetVertices, s.width); | |
| s.width = 0.05f; // 2.5% of a 2-unit cube — visible initial chamfer | |
| // Compute the largest width the bevel algorithm will actually apply | |
| // before its internal per-vertex/per-edge clamp takes over, so the | |
| // drag handler can freeze the gizmo at the same scalar the topology | |
| // op would freeze at. See the named helpers at the top of this file | |
| // (computeVertexBevelCap / computeEdgeBevelCap) for the exact | |
| // formulas, which mirror HalfEdgeMesh::bevelVertices and | |
| // HalfEdgeMesh::bevelEdges respectively. | |
| { | |
| HalfEdgeMesh hm; | |
| if (hm.buildFromEditableMesh(*m_editableMesh)) { | |
| const float cap = (s.kind == BevelSession::Vertices) | |
| ? computeVertexBevelCap(hm, s.targetVertices) | |
| : computeEdgeBevelCap(hm, s.targetEdges); | |
| if (cap > 0.0f && std::isfinite(cap)) { | |
| s.maxWidth = cap; | |
| if (s.width > s.maxWidth) s.width = s.maxWidth; | |
| } | |
| } | |
| } | |
| const bool applied = (s.kind == BevelSession::Edges) | |
| ? applyBevelTopology(s.targetEdges, s.width) | |
| : applyBevelVertexTopology(s.targetVertices, s.width); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/EditModeController.cpp` around lines 1896 - 1917, The initial s.width
(set to 0.05f) must be clamped to the computed cap so the session's startWidth
never exceeds s.maxWidth; after computing s.maxWidth via
computeVertexBevelCap/computeEdgeBevelCap (the block that constructs
HalfEdgeMesh hm and calls buildFromEditableMesh), set s.width =
std::min(s.width, s.maxWidth) (or equivalent) before calling applyBevelTopology
/ applyBevelVertexTopology so the recorded session width matches the actual
applied/clamped width.
|
1 participant
Summary
BevelSessionnow precomputesmaxWidthand the drag handler clamps both the applied width and the handle offset against it.OgreWidget::frameStartedpreviously gated the per-frame scale tick on the "active widget" being registered, which doesn't happen until first viewport focus; the gate now also accepts the nullptr case.BevelGizmo_Node/BevelGizmo_Shaft/BevelGizmo_Handleso the transient gizmo scaffolding no longer shows up alongside real scene nodes. Any future gizmo following the same suffix convention inherits this filter.Test plan
./build_local/bin/UnitTests --gtest_filter="HalfEdgeMeshStandalone.*"→ 97/97 pass, including the updatedBevelVertexSymmetricBudgetOnSharedEdgenow asserting the 0.999 reach.BevelGizmo_*nodes appear in the Scene Tree.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests