fix(fillet): variable fillet removes material instead of inflating volume#739
Conversation
…lume Variable-radius fillet on a convex edge was bulging the blend surface outward and trimming adjacent faces with a setback that did not match the blend boundary, producing a non-watertight shell with volume above the base solid (~1.3x on a unit cube). Compute a single shared inward contact point per (vertex, edge, face) and use it both to trim the planar faces and to anchor the blend boundary, so the trimmed-face vertices and blend boundary coincide. Place the arc middle control point at the tangent-cone apex (the edge point) so the rational quadratic bulges concavely into the solid. Adopt the validated rolling-ball inward sign convention and reverse blend faces whose normal points inward. Split the corner on side faces sharing a filleted-edge endpoint.
WASM Binary Size
|
There was a problem hiding this comment.
Pull request overview
This PR fixes variable-radius fillet construction so convex-edge fillets reliably remove material (no volume inflation) and produce a watertight, manifold result by making the planar-face trims and blend boundaries coincide and by correcting the rolling-ball cross-section control-point/sign conventions.
Changes:
- Adds a shared
(vertex, edge, face) -> contact_pointmap and uses it for both planar trimming and fillet-surface boundary anchoring to eliminate trim/blend gaps. - Updates variable fillet canal surface cross-sections to use a consistent inward rolling-ball sign convention and sets the rational-arc middle control point at the tangent-cone apex (edge point).
- Adds a regression test asserting variable fillets on a convex edge reduce volume and still yield a manifold, genus-0 shell.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| crates/operations/src/fillet/mod.rs | Reworks variable-radius fillet trimming and NURBS canal construction; introduces shared contact map and new face-reversal handling logic. |
| crates/operations/src/fillet/geometry.rs | Adds CrossSection + cross_section_dirs helper to centralize rolling-ball cross-section direction/sign computation. |
| crates/operations/src/fillet/tests.rs | Adds a new regression test verifying variable fillet removes material and maintains manifold/genus-0 topology. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| (true, true, _) => { | ||
| let ei_after = poly.wire_edge_ids[i].index(); | ||
| if let Some(&pt) = fillet_contact_map.get(&(vi, ei_after, fi)) { | ||
| new_verts.push(pt); | ||
| } else { | ||
| let dir_prev = (prev_pos - pos).normalize()?; | ||
| new_verts.push(pos + dir_prev * edge_law_map[&ei_after].evaluate(0.0)); | ||
| } | ||
| let ei_before = poly.wire_edge_ids[prev_i].index(); | ||
| if let Some(&pt) = fillet_contact_map.get(&(vi, ei_before, fi)) { | ||
| new_verts.push(pt); | ||
| } else { | ||
| let dir_next = (next_pos - pos).normalize()?; | ||
| new_verts.push(pos + dir_next * edge_law_map[&ei_before].evaluate(1.0)); | ||
| } |
| let solid_id = crate::boolean::assemble_solid_mixed(topo, &all_specs, tol)?; | ||
|
|
||
| if !fillet_face_indices.is_empty() { | ||
| let solid_data = topo.solid(solid_id)?; | ||
| let shell = topo.shell(solid_data.outer_shell())?; |
| let mut unique_contacts: Vec<Point3> = Vec::new(); | ||
| for (&(vi_k, _, _), &pt) in &fillet_contact_map { | ||
| if vi_k == vi | ||
| && !unique_contacts | ||
| .iter() | ||
| .any(|uc| (*uc - pt).length() < tol.linear) | ||
| { | ||
| unique_contacts.push(pt); |
Greptile SummaryThis PR fixes a variable-radius fillet that was inflating solid volume by correcting three geometric errors: the cross-section direction convention (blend was bulging outward), the middle NURBS control point placement (
Confidence Score: 3/5Safe to merge for the single-edge fillet case, but the side-face corner-split logic and the post-assembly face reversal both have correctness gaps that would surface as silent wrong geometry on multi-edge fillets. The single-edge test passes and the geometric fix (mid-CP at edge point, shared contact map) is sound. However, the crates/operations/src/fillet/mod.rs — the contact-collection loop (line 619) and the post-assembly reversal block (lines 887–898) Important Files Changed
Prompt To Fix All With AIFix the following 4 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 4
crates/operations/src/fillet/mod.rs:619-645
**Contact collection ignores adjacency — wrong corners for multi-edge fillets**
The `(false, false, true)` arm collects every contact where `vi_k == vi`, across all edges and faces in `fillet_contact_map`. For a single filleted edge (the current test case) this yields exactly the 2 correct contacts. But if two filleted edges share this vertex — which `vertex_fillet_endpoints` would mark as `at_fillet_endpoint = true` — the map contains 4 contacts at `vi` (2 per edge × 2 faces). The code then takes `unique_contacts[0]` and `unique_contacts[1]` based on non-deterministic `HashMap` iteration order, and the proximity sort only decides which of those two comes first. The two selected contacts may belong to different edges, producing a side face that doesn't connect correctly to either blend boundary and leaving the shell non-manifold with no error raised.
### Issue 2 of 4
crates/operations/src/fillet/mod.rs:887-898
**Face reversal depends on undocumented index correspondence between `all_specs` and assembled `shell.faces()`**
`fillet_face_indices` stores raw indices into `all_specs` (e.g., the value of `all_specs.len() - 1` at the time each fillet face is pushed). After `assemble_solid_mixed` those indices are then used as indices into `shell.faces().to_vec()`. This works today because: (a) no `CylindricalFace` specs are present (so the cylindrical-first reordering in the assembler does not apply), and (b) no face spec has fewer than 3 vertices (which would cause the assembler's `continue` to skip a face, shifting all subsequent indices). Neither constraint is documented, checked at the call-site, or enforced. A future caller that adds even one `CylindricalFace` spec or receives a degenerate planar face would silently reverse the wrong face without a compile-time or runtime warning.
### Issue 3 of 4
crates/operations/src/fillet/mod.rs:669-679
Direct HashMap indexing `edge_law_map[&ei]` panics if the key is absent. The previous code used `map_or(0.0, |law| ...)` as a safe fallback. While the invariant (`after_filleted == true` implies `ei ∈ edge_law_map`) currently holds, replacing `.map_or` with direct indexing removes the defensive pattern the codebase was using everywhere else. The same pattern appears at the fallback for all three filleted-arm branches.
```suggestion
} else {
let dir_prev = (prev_pos - pos).normalize()?;
let r = edge_law_map.get(&ei_after).map_or(0.0, |l| l.evaluate(0.0));
new_verts.push(pos + dir_prev * r);
}
let ei_before = poly.wire_edge_ids[prev_i].index();
if let Some(&pt) = fillet_contact_map.get(&(vi, ei_before, fi)) {
new_verts.push(pt);
} else {
let dir_next = (next_pos - pos).normalize()?;
let r = edge_law_map.get(&ei_before).map_or(0.0, |l| l.evaluate(1.0));
new_verts.push(pos + dir_next * r);
}
```
### Issue 4 of 4
crates/operations/src/fillet/geometry.rs:129-130
The variable `cos_half` is named as if it holds `cos(half_angle)`, but it actually holds `cos(full_angle)` — `ld1.dot(ld2)` is the cosine of the full arc sweep. The half-angle is then computed as `acos(cos_full) / 2`. This inverted naming could cause confusion when reading the formula, e.g. in the weight expression `cs.half_angle.cos()` downstream.
```suggestion
let cos_full = ld1.dot(ld2).clamp(-1.0, 1.0);
let half_angle = cos_full.acos() / 2.0;
```
Reviews (1): Last reviewed commit: "fix(fillet): variable fillet removes mat..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| (false, false, true) => { | ||
| let mut unique_contacts: Vec<Point3> = Vec::new(); | ||
| for (&(vi_k, _, _), &pt) in &fillet_contact_map { | ||
| if vi_k == vi | ||
| && !unique_contacts | ||
| .iter() | ||
| .any(|uc| (*uc - pt).length() < tol.linear) | ||
| { | ||
| unique_contacts.push(pt); | ||
| } | ||
| } | ||
| if unique_contacts.len() >= 2 { | ||
| let approx_prev = (prev_pos - pos) | ||
| .normalize() | ||
| .map_or(pos, |d| pos + d * tol.linear); | ||
| let d0 = (unique_contacts[0] - approx_prev).length(); | ||
| let d1 = (unique_contacts[1] - approx_prev).length(); | ||
| if d0 <= d1 { | ||
| new_verts.push(unique_contacts[0]); | ||
| new_verts.push(unique_contacts[1]); | ||
| } else { | ||
| new_verts.push(unique_contacts[1]); | ||
| new_verts.push(unique_contacts[0]); | ||
| } | ||
| } else { | ||
| new_verts.push(pos); | ||
| } |
There was a problem hiding this comment.
The (false, false, true) arm collects every contact where vi_k == vi, across all edges and faces in fillet_contact_map. For a single filleted edge (the current test case) this yields exactly the 2 correct contacts. But if two filleted edges share this vertex — which vertex_fillet_endpoints would mark as at_fillet_endpoint = true — the map contains 4 contacts at vi (2 per edge × 2 faces). The code then takes unique_contacts[0] and unique_contacts[1] based on non-deterministic HashMap iteration order, and the proximity sort only decides which of those two comes first. The two selected contacts may belong to different edges, producing a side face that doesn't connect correctly to either blend boundary and leaving the shell non-manifold with no error raised.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/operations/src/fillet/mod.rs
Line: 619-645
Comment:
**Contact collection ignores adjacency — wrong corners for multi-edge fillets**
The `(false, false, true)` arm collects every contact where `vi_k == vi`, across all edges and faces in `fillet_contact_map`. For a single filleted edge (the current test case) this yields exactly the 2 correct contacts. But if two filleted edges share this vertex — which `vertex_fillet_endpoints` would mark as `at_fillet_endpoint = true` — the map contains 4 contacts at `vi` (2 per edge × 2 faces). The code then takes `unique_contacts[0]` and `unique_contacts[1]` based on non-deterministic `HashMap` iteration order, and the proximity sort only decides which of those two comes first. The two selected contacts may belong to different edges, producing a side face that doesn't connect correctly to either blend boundary and leaving the shell non-manifold with no error raised.
How can I resolve this? If you propose a fix, please make it concise.
| let solid_id = crate::boolean::assemble_solid_mixed(topo, &all_specs, tol)?; | ||
|
|
||
| if !fillet_face_indices.is_empty() { | ||
| let solid_data = topo.solid(solid_id)?; | ||
| let shell = topo.shell(solid_data.outer_shell())?; | ||
| let face_ids: Vec<_> = shell.faces().to_vec(); | ||
| for &fi in &fillet_face_indices { | ||
| if fi < face_ids.len() { | ||
| topo.face_mut(face_ids[fi])?.set_reversed(true); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
all_specs and assembled shell.faces()
fillet_face_indices stores raw indices into all_specs (e.g., the value of all_specs.len() - 1 at the time each fillet face is pushed). After assemble_solid_mixed those indices are then used as indices into shell.faces().to_vec(). This works today because: (a) no CylindricalFace specs are present (so the cylindrical-first reordering in the assembler does not apply), and (b) no face spec has fewer than 3 vertices (which would cause the assembler's continue to skip a face, shifting all subsequent indices). Neither constraint is documented, checked at the call-site, or enforced. A future caller that adds even one CylindricalFace spec or receives a degenerate planar face would silently reverse the wrong face without a compile-time or runtime warning.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/operations/src/fillet/mod.rs
Line: 887-898
Comment:
**Face reversal depends on undocumented index correspondence between `all_specs` and assembled `shell.faces()`**
`fillet_face_indices` stores raw indices into `all_specs` (e.g., the value of `all_specs.len() - 1` at the time each fillet face is pushed). After `assemble_solid_mixed` those indices are then used as indices into `shell.faces().to_vec()`. This works today because: (a) no `CylindricalFace` specs are present (so the cylindrical-first reordering in the assembler does not apply), and (b) no face spec has fewer than 3 vertices (which would cause the assembler's `continue` to skip a face, shifting all subsequent indices). Neither constraint is documented, checked at the call-site, or enforced. A future caller that adds even one `CylindricalFace` spec or receives a degenerate planar face would silently reverse the wrong face without a compile-time or runtime warning.
How can I resolve this? If you propose a fix, please make it concise.| } else { | ||
| let dir_prev = (prev_pos - pos).normalize()?; | ||
| new_verts.push(pos + dir_prev * edge_law_map[&ei_after].evaluate(0.0)); | ||
| } | ||
| let ei_before = poly.wire_edge_ids[prev_i].index(); | ||
| if let Some(&pt) = fillet_contact_map.get(&(vi, ei_before, fi)) { | ||
| new_verts.push(pt); | ||
| } else { | ||
| let dir_next = (next_pos - pos).normalize()?; | ||
| new_verts.push(pos + dir_next * edge_law_map[&ei_before].evaluate(1.0)); | ||
| } |
There was a problem hiding this comment.
edge_law_map[&ei] panics if the key is absent. The previous code used map_or(0.0, |law| ...) as a safe fallback. While the invariant (after_filleted == true implies ei ∈ edge_law_map) currently holds, replacing .map_or with direct indexing removes the defensive pattern the codebase was using everywhere else. The same pattern appears at the fallback for all three filleted-arm branches.
| } else { | |
| let dir_prev = (prev_pos - pos).normalize()?; | |
| new_verts.push(pos + dir_prev * edge_law_map[&ei_after].evaluate(0.0)); | |
| } | |
| let ei_before = poly.wire_edge_ids[prev_i].index(); | |
| if let Some(&pt) = fillet_contact_map.get(&(vi, ei_before, fi)) { | |
| new_verts.push(pt); | |
| } else { | |
| let dir_next = (next_pos - pos).normalize()?; | |
| new_verts.push(pos + dir_next * edge_law_map[&ei_before].evaluate(1.0)); | |
| } | |
| } else { | |
| let dir_prev = (prev_pos - pos).normalize()?; | |
| let r = edge_law_map.get(&ei_after).map_or(0.0, |l| l.evaluate(0.0)); | |
| new_verts.push(pos + dir_prev * r); | |
| } | |
| let ei_before = poly.wire_edge_ids[prev_i].index(); | |
| if let Some(&pt) = fillet_contact_map.get(&(vi, ei_before, fi)) { | |
| new_verts.push(pt); | |
| } else { | |
| let dir_next = (next_pos - pos).normalize()?; | |
| let r = edge_law_map.get(&ei_before).map_or(0.0, |l| l.evaluate(1.0)); | |
| new_verts.push(pos + dir_next * r); | |
| } |
Rule Used: New code must not introduce unwrap(), expect(), or... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/operations/src/fillet/mod.rs
Line: 669-679
Comment:
Direct HashMap indexing `edge_law_map[&ei]` panics if the key is absent. The previous code used `map_or(0.0, |law| ...)` as a safe fallback. While the invariant (`after_filleted == true` implies `ei ∈ edge_law_map`) currently holds, replacing `.map_or` with direct indexing removes the defensive pattern the codebase was using everywhere else. The same pattern appears at the fallback for all three filleted-arm branches.
```suggestion
} else {
let dir_prev = (prev_pos - pos).normalize()?;
let r = edge_law_map.get(&ei_after).map_or(0.0, |l| l.evaluate(0.0));
new_verts.push(pos + dir_prev * r);
}
let ei_before = poly.wire_edge_ids[prev_i].index();
if let Some(&pt) = fillet_contact_map.get(&(vi, ei_before, fi)) {
new_verts.push(pt);
} else {
let dir_next = (next_pos - pos).normalize()?;
let r = edge_law_map.get(&ei_before).map_or(0.0, |l| l.evaluate(1.0));
new_verts.push(pos + dir_next * r);
}
```
**Rule Used:** New code must not introduce unwrap(), expect(), or... ([source](https://app.greptile.com/review/custom-context?memory=no-new-unwrap))
How can I resolve this? If you propose a fix, please make it concise.| let cos_half = ld1.dot(ld2).clamp(-1.0, 1.0); | ||
| let half_angle = cos_half.acos() / 2.0; |
There was a problem hiding this comment.
cos_half is named as if it holds cos(half_angle), but it actually holds cos(full_angle) — ld1.dot(ld2) is the cosine of the full arc sweep. The half-angle is then computed as acos(cos_full) / 2. This inverted naming could cause confusion when reading the formula, e.g. in the weight expression cs.half_angle.cos() downstream.
| let cos_half = ld1.dot(ld2).clamp(-1.0, 1.0); | |
| let half_angle = cos_half.acos() / 2.0; | |
| let cos_full = ld1.dot(ld2).clamp(-1.0, 1.0); | |
| let half_angle = cos_full.acos() / 2.0; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/operations/src/fillet/geometry.rs
Line: 129-130
Comment:
The variable `cos_half` is named as if it holds `cos(half_angle)`, but it actually holds `cos(full_angle)` — `ld1.dot(ld2)` is the cosine of the full arc sweep. The half-angle is then computed as `acos(cos_full) / 2`. This inverted naming could cause confusion when reading the formula, e.g. in the weight expression `cs.half_angle.cos()` downstream.
```suggestion
let cos_full = ld1.dot(ld2).clamp(-1.0, 1.0);
let half_angle = cos_full.acos() / 2.0;
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
🤖 I have created a release *beep* *boop* --- ## [2.101.1](v2.101.0...v2.101.1) (2026-05-29) ### Bug Fixes * **fillet:** variable fillet removes material instead of inflating volume ([#739](#739)) ([7398d23](7398d23)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: brepkit[bot] <265643962+brepkit[bot]@users.noreply.github.com>
## Summary brepkit previously inflated solid volume on a variable-radius fillet (a fillet on a convex edge can only ever remove material), so `kernelOps.variableFilletRadius` was registered as a `skip` divergence. The upstream kernel fix resolves the volume-inflation bug. This PR: - Removes the `kernelOps.variableFilletRadius` divergence entry from `tests/helpers/kernelDivergences.ts`. - Un-skips the `fillet variable radius` test in `tests/kernel-ops.test.ts` so it runs on every kernel, asserting the filleted volume stays strictly below the base box (8000) and above 7000 — a regression guard against the old inflation bug. - Drops the now-unused `skipIfDiverges` import. ## Verification Built the kernel from the upstream fix branch, swapped the wasm into this worktree, and confirmed the un-skipped test passes on all three kernels (brepkit, occt-wasm, occt) with full cross-kernel agreement. ##⚠️ Blocked **Do not merge yet.** This is blocked on the brepkit-wasm release that contains the upstream fix: - Upstream fix: andymai/brepkit#739 Once that release ships and `brepkit-wasm` is bumped here, this PR can be marked ready and merged. Until then, CI will fail against the brepkit kernel because the published wasm still inflates the volume. ## Note on remaining divergences `modifierFns.variableFilletRadius` and `modifierFns.variableFilletCallback` remain `it.skip(...)` at the test-runner level because the reference kernel itself crashes on variable-radius fillet (memory access out of bounds) — those are not brepkit-only divergences and are intentionally left untouched.
Summary
A variable-radius fillet on a convex edge was bulging the blend surface outward and trimming the adjacent planar faces with a setback that did not match the blend boundary. The result was a non-watertight shell whose volume exceeded the base solid (~1.3x on a unit cube) — geometrically impossible for a fillet, which can only remove material on a convex edge.
Fix
(vertex, edge, face)and use it both to trim the planar faces and to anchor the blend boundary, so the trimmed-face vertices and the blend boundary coincide (closes the watertightness gap).Testing
fillet_variable_removes_material_linear_lawasserts the filleted volume is strictly less than the base solid.brepkit-operationsfillet suite green (51 passed, 1 ignored).cargo fmt --check,cargo clippy -p brepkit-operations --all-targets -- -D warnings, andscripts/check-boundaries.shall clean. Full pre-push test suite + cargo-deny passed.