fix(fillet): variable-radius fillet removes material on convex edges#743
fix(fillet): variable-radius fillet removes material on convex edges#743andymai wants to merge 1 commit into
Conversation
The variable-radius canal-surface path chose the in-plane contact direction by comparing against the neighbouring face's outward normal with the wrong sign, and placed the arc middle control point away from the edge. On a convex edge this inverted the blend so it bulged outward, producing a malformed shell whose volume exceeded the original solid. Use the inward section-plane direction (opposite each face's own outward normal), put the arc middle control point at the sharp-edge point so the arc bulges toward the material, derive the trimmed neighbour-face boundary vertices from the same contact rails for a watertight seam, and set the blend strip's reversed flag from the bisector.
Greptile SummaryFixes three root causes of the variable-radius fillet incorrectly inflating solid volume on convex edges: the in-plane contact direction sign, the arc middle control point placement, and non-watertight seam vertices. The new
Confidence Score: 4/5Safe to merge for all practical geometries; the two flagged paths are narrow degenerate cases that don't affect boxes or standard convex shapes. The core geometric fix is sound and well-exercised by three new tests. The two open issues sit behind degenerate-geometry guards that normal solids never reach. The fallback paths in mod.rs lines 695-698 and the strip_reversed computation at line 837 are worth a second look. Important Files Changed
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
crates/operations/src/fillet/mod.rs:695-698
The fallback when `inward_section_dir` returns `None` returns the raw (unnormalized) cross product `edge_dir.cross(n1_start)`. The old code explicitly normalized: `d1_ref.normalize().unwrap_or(d1_ref)`. If this degenerate path is hit, `d1_ref` is near-zero and is later used as the fallback in `ld1 = geometry::inward_section_dir(...).unwrap_or(d1_ref)`, making `contact1 = p + ld1 * r ≈ p` (zero-radius contact). Adding `.normalize().unwrap_or(edge_dir)` keeps parity with the old behavior and makes the fallback unit-length when non-zero.
```suggestion
let d1_ref = geometry::inward_section_dir(edge_dir, n1_start, n2_start)
.unwrap_or_else(|| edge_dir.cross(n1_start).normalize().unwrap_or(edge_dir));
let d2_ref = geometry::inward_section_dir(edge_dir, n2_start, n1_start)
.unwrap_or_else(|| edge_dir.cross(n2_start).normalize().unwrap_or(edge_dir));
```
### Issue 2 of 2
crates/operations/src/fillet/mod.rs:837
**Degenerate bisector used as `bisector_ref`**
`bisector = (ld1 + ld2).normalize().unwrap_or(d1_ref)` falls back to `d1_ref` (face-1 inward direction) when `ld1 + ld2` is near-zero — which happens for nearly-flat convex edges (~180°) where the two inward directions are nearly antiparallel. In that case `bisector_ref` ends up being the inward direction for face 1 alone, and `strip_normal.dot(bisector_ref) > 0.0` can give the wrong `strip_reversed` flag, producing an inside-out blend face. A dedicated zero-check or using a hard-coded fall-through orientation would be more robust for this edge case.
Reviews (1): Last reviewed commit: "fix(fillet): variable-radius fillet remo..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| let d1_ref = geometry::inward_section_dir(edge_dir, n1_start, n2_start) | ||
| .unwrap_or_else(|| edge_dir.cross(n1_start)); | ||
| let d2_ref = geometry::inward_section_dir(edge_dir, n2_start, n1_start) | ||
| .unwrap_or_else(|| edge_dir.cross(n2_start)); |
There was a problem hiding this comment.
inward_section_dir returns None returns the raw (unnormalized) cross product edge_dir.cross(n1_start). The old code explicitly normalized: d1_ref.normalize().unwrap_or(d1_ref). If this degenerate path is hit, d1_ref is near-zero and is later used as the fallback in ld1 = geometry::inward_section_dir(...).unwrap_or(d1_ref), making contact1 = p + ld1 * r ≈ p (zero-radius contact). Adding .normalize().unwrap_or(edge_dir) keeps parity with the old behavior and makes the fallback unit-length when non-zero.
| let d1_ref = geometry::inward_section_dir(edge_dir, n1_start, n2_start) | |
| .unwrap_or_else(|| edge_dir.cross(n1_start)); | |
| let d2_ref = geometry::inward_section_dir(edge_dir, n2_start, n1_start) | |
| .unwrap_or_else(|| edge_dir.cross(n2_start)); | |
| let d1_ref = geometry::inward_section_dir(edge_dir, n1_start, n2_start) | |
| .unwrap_or_else(|| edge_dir.cross(n1_start).normalize().unwrap_or(edge_dir)); | |
| let d2_ref = geometry::inward_section_dir(edge_dir, n2_start, n1_start) | |
| .unwrap_or_else(|| edge_dir.cross(n2_start).normalize().unwrap_or(edge_dir)); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/operations/src/fillet/mod.rs
Line: 695-698
Comment:
The fallback when `inward_section_dir` returns `None` returns the raw (unnormalized) cross product `edge_dir.cross(n1_start)`. The old code explicitly normalized: `d1_ref.normalize().unwrap_or(d1_ref)`. If this degenerate path is hit, `d1_ref` is near-zero and is later used as the fallback in `ld1 = geometry::inward_section_dir(...).unwrap_or(d1_ref)`, making `contact1 = p + ld1 * r ≈ p` (zero-radius contact). Adding `.normalize().unwrap_or(edge_dir)` keeps parity with the old behavior and makes the fallback unit-length when non-zero.
```suggestion
let d1_ref = geometry::inward_section_dir(edge_dir, n1_start, n2_start)
.unwrap_or_else(|| edge_dir.cross(n1_start).normalize().unwrap_or(edge_dir));
let d2_ref = geometry::inward_section_dir(edge_dir, n2_start, n1_start)
.unwrap_or_else(|| edge_dir.cross(n2_start).normalize().unwrap_or(edge_dir));
```
How can I resolve this? If you propose a fix, please make it concise.
| // `bisector_ref` points from the edge toward the material interior, so | ||
| // the strip's outward normal must oppose it. Reverse when they agree. | ||
| let strip_normal = surface.normal(0.5, 0.5).unwrap_or(bisector_ref); | ||
| let strip_reversed = strip_normal.dot(bisector_ref) > 0.0; |
There was a problem hiding this comment.
bisector_ref
bisector = (ld1 + ld2).normalize().unwrap_or(d1_ref) falls back to d1_ref (face-1 inward direction) when ld1 + ld2 is near-zero — which happens for nearly-flat convex edges (~180°) where the two inward directions are nearly antiparallel. In that case bisector_ref ends up being the inward direction for face 1 alone, and strip_normal.dot(bisector_ref) > 0.0 can give the wrong strip_reversed flag, producing an inside-out blend face. A dedicated zero-check or using a hard-coded fall-through orientation would be more robust for this edge case.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/operations/src/fillet/mod.rs
Line: 837
Comment:
**Degenerate bisector used as `bisector_ref`**
`bisector = (ld1 + ld2).normalize().unwrap_or(d1_ref)` falls back to `d1_ref` (face-1 inward direction) when `ld1 + ld2` is near-zero — which happens for nearly-flat convex edges (~180°) where the two inward directions are nearly antiparallel. In that case `bisector_ref` ends up being the inward direction for face 1 alone, and `strip_normal.dot(bisector_ref) > 0.0` can give the wrong `strip_reversed` flag, producing an inside-out blend face. A dedicated zero-check or using a hard-coded fall-through orientation would be more robust for this edge case.
How can I resolve this? If you propose a fix, please make it concise.
Problem
The variable-radius (canal-surface) fillet path inflated solid volume on convex
edges: filleting an edge of a unit cube produced a solid with volume greater than
the original. A fillet on a convex edge must remove material, so this was
geometrically impossible and produced a malformed shell.
Root causes
each face's rolling-ball contact was selected by comparing against the
neighbouring face's outward normal with the wrong sign, so the blend bulged
outward (away from the material) instead of inward.
arc's middle control point was offset along the bisector away from the sharp
edge, so the arc bulged outward rather than toward the material.
derived independently from the blend rails, producing a non-watertight seam.
Fix
geometry::inward_section_dir(tangent, n_self, n_other)— the in-planedirection pointing into the material wedge (opposite the face's own outward
normal), with the sign fixed unambiguously by the neighbour normal. Used for
both the per-station canal build and the t=0 fallback.
rational arc bulges toward the material (convex fillet that removes material).
(vertex, edge, face); trimming and the blend rails reuse these exact positions
for a watertight seam.
reversedflag from the bisector so the strip's outwardnormal points out of the solid.
This also removes the previous workaround note about ~30% volume inflation on the
all-edges path — the corrected direction convention resolves it.
Tests
fillet_variable_linear_lawnow asserts the filleted unit cube has volume < 1.fillet_variable_linear_law_removes_material— box(20,20,20), single-edgelinear law: result is manifold, has exactly one added blend face, and
7000 < V < 8000 (material removed).
fillet_variable_constant_law_matches_rolling_ball— a constant-law variablefillet matches the rolling-ball fillet volume within 1%.
All
brepkit-operationsfillet tests pass; fmt, clippy (-D warnings),crate-boundary checks, and the full pre-push suite are green.