Fix perpendicular fillets after connected rounds (#1581)

Thanks @cfaulkingham.

ROADMAP.md

@@ -307,7 +307,9 @@ annotations and exact projected curves remain deferred. See [view DXF](docs/view
   is an equivalent circular setback. Curved faces/edges, sloped ends and corner junctions remain unsupported.
 - Fillets and chamfers: constant, variable-radius and chord-length fillets; a radius per edge,
   rounds next to earlier rounds of another radius, and smooth corner patches where three rounds
-  meet (a sphere for equal radii); straight edges (convex or concave) between planar faces with perpendicular
+  meet (a sphere for equal radii); perpendicular corner fillets after a closed ring of earlier
+  rounds preserve the ring's radii and the enclosed planar face (#1557);
+  straight edges (convex or concave) between planar faces with perpendicular
   planar end faces (equal-distance chamfers also meet sloped planar end faces, including inner
   basin rims within the rim width); convex fillets also meet sloped planar ends and reflex
   corners around pocket rims, with unequal wall slopes and extensions confined to the face;

crates/engine/src/cmd/features.rs

@@ -1888,3 +1888,6 @@ mod thin_extrude_tests;
 #[cfg(test)]
 #[path = "extrude_convention_tests.rs"]
 mod extrude_convention_tests;
+#[cfg(test)]
+#[path = "fillet_connected_rounds_tests.rs"]
+mod fillet_connected_rounds_tests;

crates/engine/src/cmd/fillet_connected_rounds_tests.rs

@@ -0,0 +1,64 @@
+use serde_json::{Value, json};
+
+use crate::Session;
+
+fn run(s: &mut Session, command: &str, params: Value) -> Value {
+    s.execute(command, &params).unwrap_or_else(|e| panic!("{command}: {e}"))
+}
+
+fn bottom_rounds() -> Session {
+    let mut s = Session::default();
+    run(&mut s, "parameters.change", json!({"name":"corner_radius", "expression":"2 mm"}));
+    run(&mut s, "sketch.create", json!({"plane":"XY", "name":"Rectangle"}));
+    run(&mut s, "sketch.rectangle.two_point", json!({"p0":[0,0], "p1":[25,25]}));
+    run(&mut s, "sketch.finish", json!({}));
+    run(&mut s, "solid.extrude", json!({"distance":"10 mm", "body_name":"Block"}));
+    run(&mut s, "solid.fillet", json!({"edges":[[12.5,0,0],[25,12.5,0],[12.5,25,0],[0,12.5,0]], "radius":"2 mm"}));
+    s
+}
+
+fn volume(s: &mut Session) -> f64 {
+    let value = run(s, "inspect.measure", json!({}));
+    assert_eq!(value["body_count"], 1);
+    let state = s.model.state();
+    let body = &state.body("Block").unwrap().body;
+    assert!(body.validity().is_empty(), "{:?}", body.validity());
+    value["total"]["volume_mm3"].as_f64().unwrap()
+}
+
+#[test]
+fn issue1557_fillet_recomputes_and_survives_undo_and_reload() {
+    for edges in [json!([[25, 0, 6]]), json!([[25, 0, 6], [24.287266336668306, 0.7127336633316916, 0.48266373325510686]])] {
+        let mut s = bottom_rounds();
+        let before = volume(&mut s);
+        let out = run(&mut s, "solid.fillet", json!({"edges":edges, "radius":"corner_radius"}));
+        let id = out["feature"].as_u64().unwrap();
+        let after = volume(&mut s);
+        assert!(after < before - 1.0);
+        assert_eq!(s.doc.features.len(), 4);
+        run(&mut s, "edit.undo", json!({}));
+        assert!((volume(&mut s) - before).abs() < 1e-6);
+        run(&mut s, "edit.redo", json!({}));
+        assert!((volume(&mut s) - after).abs() < 1e-6);
+        run(&mut s, "parameters.change", json!({"name":"corner_radius", "expression":"1 mm"}));
+        let smaller = volume(&mut s);
+        assert!(smaller > after && smaller < before);
+        assert!(run(&mut s, "timeline.inspect", json!({"feature":id}))["error"].is_null());
+        let bytes = crate::io::write_design_container(&s.doc).unwrap();
+        let mut reopened = Session::new(crate::io::read_design(&bytes).unwrap());
+        assert!((volume(&mut reopened) - smaller).abs() < 1e-6);
+        assert_eq!(reopened.doc.features.len(), 4);
+    }
+}
+
+#[test]
+fn issue1557_failed_radius_keeps_the_existing_rounds() {
+    let mut s = bottom_rounds();
+    let before = s.doc.to_json();
+    let v = volume(&mut s);
+    let undo = s.undo.len();
+    assert!(s.execute("solid.fillet", &json!({"edges":[[25,0,6]], "radius":"1000 mm"})).is_err());
+    assert_eq!(s.doc.to_json(), before);
+    assert_eq!(s.undo.len(), undo);
+    assert!((volume(&mut s) - v).abs() < 1e-6);
+}

crates/kernel/src/blend.rs

@@ -1611,7 +1611,7 @@ fn tangent_faces(solid: &Solid, edge: &mt::Edge) -> bool {
 
 /// The chain of edges continuing `edge` smoothly at its ends (its ids): closed, or open where
 /// the next edge turns sharply.
-fn smooth_chain(solid: &Solid, edge: &mt::Edge) -> Option<Vec<mt::EdgeID>> {
+pub(crate) fn smooth_chain(solid: &Solid, edge: &mt::Edge) -> Option<Vec<mt::EdgeID>> {
     smooth_chain_within(solid, edge, &|_| true)
 }
 

crates/kernel/src/blend_corner.rs

@@ -44,12 +44,13 @@ pub fn fillet_radii(body: &Body, edges: &[(Vec3, f64)]) -> Result<Body> {
             Ok(b) => return Ok(crate::record::after(b, &cur, &counts)),
             Err(e) => e,
         };
-        let rounds = rounds_at(&cur, &sel);
+        let rounds = guard("recognise earlier rounds", || rounds_at(&cur, &sel))?;
         if rounds.is_empty() {
             return Err(first.unwrap_or(err));
         }
         first.get_or_insert(err);
         let idx: Vec<usize> = rounds.iter().map(|r| r.0).collect();
+        sel = guard("map picks across earlier rounds", || surviving_picks(&cur, &sel, &idx))?;
         cur = crate::record::after(crate::delete_faces(&cur, &idx)?, &cur, &counts);
         sel.extend(rounds.iter().map(|r| (r.1, r.2)));
     }
@@ -793,9 +794,35 @@ fn crease(a: &Sel, b: &Sel, q: Vec3, end: Vec3, size: f64) -> Result<mt::Curve>
     crate::splitface::fit(&pts, size)
 }
 
-/// Faces at the filleted edges' ends that are rounds of a straight edge between planar faces:
+/// Keep selected sharp edges while consuming a selected mitre that is on the same smooth
+/// chain as one of them. After removing the rounds, that mitre no longer exists; using its
+/// old point would silently choose a different nearby edge, possibly with a different radius.
+fn surviving_picks(cur: &Body, sel: &[(Vec3, f64)], removed: &[usize]) -> Result<Vec<(Vec3, f64)>> {
+    let solid = &*cur.solid;
+    let edges = Body::unique_edges(solid);
+    let faces: Vec<_> = solid.face_iter().collect();
+    let mut selected = Vec::new();
+    for (p, r) in sel {
+        let (idx, _) = cur.nearest_edge(*p, (cur.size() * 2e-3).max(1e-3))?.ok_or_else(|| unsupported("no edge at a fillet pick"))?;
+        let e = edges.get(idx).ok_or_else(|| unsupported("no edge at a fillet pick"))?;
+        let count = removed.iter().filter_map(|i| faces.get(*i)).filter(|f| f.edge_iter().any(|x| x.id() == e.id())).count();
+        selected.push((e, *p, *r, count));
+    }
+    for (e, _, r, count) in &selected {
+        if *count == 0 {
+            continue;
+        }
+        let chain = crate::blend::smooth_chain(solid, e).unwrap_or_default();
+        if *count != 2 || !selected.iter().any(|(other, _, rr, n)| *n == 0 && (r - rr).abs() <= r * 1e-9 && chain.contains(&other.id())) {
+            return Err(unsupported("a selected earlier-round boundary cannot be mapped to a sharp edge"));
+        }
+    }
+    Ok(selected.into_iter().filter(|(_, _, _, n)| *n == 0).map(|(_, p, r, _)| (p, r)).collect())
+}
+
+/// Connected faces at the filleted edges' ends that are rounds of straight edges between planar faces:
 /// (face index, a point on the sharp edge it replaced, its radius).
-fn rounds_at(cur: &Body, sel: &[(Vec3, f64)]) -> Vec<(usize, Vec3, f64)> {
+fn rounds_at(cur: &Body, sel: &[(Vec3, f64)]) -> Result<Vec<(usize, Vec3, f64)>> {
     let size = cur.size();
     let tol = (size * 2e-3).max(1e-3);
     let solid = &*cur.solid;
@@ -810,16 +837,43 @@ fn rounds_at(cur: &Body, sel: &[(Vec3, f64)]) -> Vec<(usize, Vec3, f64)> {
         }
     }
     let faces: Vec<&mt::Face> = solid.face_iter().collect();
-    let mut out = Vec::new();
+    let mut neighbours: HashMap<mt::EdgeID, Vec<usize>> = HashMap::new();
     for (fi, f) in faces.iter().enumerate() {
-        if plane_normal(f).is_some() || !f.vertex_iter().any(|v| ends.iter().any(|p| p.dist(vtx(&v)) <= size * 1e-9)) {
+        for e in f.edge_iter() {
+            neighbours.entry(e.id()).or_default().push(fi);
+        }
+    }
+    let mut pending: Vec<usize> = faces
+        .iter()
+        .enumerate()
+        .filter(|(_, f)| f.vertex_iter().any(|v| ends.iter().any(|p| p.dist(vtx(&v)) <= size * 1e-9)))
+        .map(|(fi, _)| fi)
+        .collect();
+    let mut visited = HashSet::new();
+    let mut out = Vec::new();
+    while let Some(fi) = pending.pop() {
+        if !visited.insert(fi) {
+            continue;
+        }
+        let Some(f) = faces.get(fi) else { continue };
+        if plane_normal(f).is_some() {
             continue;
         }
         if let Some((p, r)) = round_of(&faces, f, size) {
+            if out.len() + sel.len() >= 1000 {
+                return Err(unsupported("too many connected earlier rounds to rebuild"));
+            }
             out.push((fi, p, r));
+            // Healing requires planar neighbours. Include the other recognised rounds
+            // sharing this one's mitres, rather than leaving a curved neighbour across
+            // the gap. A closed ring must be restored and reblended as a whole (#1557).
+            for e in f.edge_iter() {
+                pending.extend(neighbours.get(&e.id()).into_iter().flatten().copied().filter(|i| !visited.contains(i)));
+            }
         }
     }
-    out
+    out.sort_by_key(|r| r.0);
+    Ok(out)
 }
 
 /// A round between two planes: two parallel straight boundary edges on planar faces, the
@@ -1274,6 +1328,76 @@ mod tests {
         assert!((vol(&e) - strips(2.0, 2.0, 0.0)).abs() < 0.05);
     }
 
+    #[test]
+    fn issue1557_vertical_after_bottom_rounds() {
+        let b = box_solid(Vec3::ZERO, Vec3::new(25.0, 25.0, 10.0)).unwrap();
+        let bottom = [Vec3::new(12.5, 0.0, 0.0), Vec3::new(25.0, 12.5, 0.0), Vec3::new(12.5, 25.0, 0.0), Vec3::new(0.0, 12.5, 0.0)];
+        let rounded = fillet(&b, &bottom, 2.0).unwrap();
+        let vertical = Vec3::new(25.0, 0.0, 6.0);
+        let seam = Vec3::new(24.287266336668306, 0.7127336633316916, 0.48266373325510686);
+        let mut together = bottom.to_vec();
+        together.push(vertical);
+        let reference = fillet(&b, &together, 2.0).unwrap();
+        for picks in [vec![vertical], vec![vertical, seam], vec![seam, vertical]] {
+            let result = fillet(&rounded, &picks, 2.0).unwrap();
+            assert_eq!(result.face_count(), reference.face_count());
+            assert_eq!(result.unmeshed_faces(), 0);
+            assert!(result.validity().is_empty(), "{:?}", result.validity());
+            assert!((vol(&result) - vol(&reference)).abs() < 0.05);
+            assert!(!result.contains(Vec3::new(24.9, 0.1, 5.0)));
+            assert!(result.contains(Vec3::new(24.0, 1.0, 5.0)));
+        }
+    }
+
+    #[test]
+    fn issue1557_connected_rounds_keep_their_radii() {
+        let b = box_solid(Vec3::ZERO, Vec3::new(25.0, 25.0, 10.0)).unwrap();
+        let bottom = [Vec3::new(12.5, 0.0, 0.0), Vec3::new(25.0, 12.5, 0.0), Vec3::new(12.5, 25.0, 0.0), Vec3::new(0.0, 12.5, 0.0)];
+        let rounded = fillet(&b, &bottom, 2.0).unwrap();
+        let vertical = Vec3::new(25.0, 0.0, 6.0);
+        let seam = Vec3::new(24.287266336668306, 0.7127336633316916, 0.48266373325510686);
+        for radius in [0.5, 1.0, 3.0] {
+            let mut together: Vec<_> = bottom.iter().map(|p| (*p, 2.0)).collect();
+            together.push((vertical, radius));
+            let reference = fillet_radii(&b, &together).unwrap();
+            let result = fillet(&rounded, &[seam, vertical], radius).unwrap();
+            assert!(result.validity().is_empty(), "{:?}", result.validity());
+            assert!((vol(&result) - vol(&reference)).abs() < 0.05, "radius {radius}");
+            // The old bottom radius is still 2, away from the newly rounded corner.
+            assert!(!result.contains(Vec3::new(12.5, 0.5, 0.5)));
+            assert!(result.contains(Vec3::new(12.5, 1.0, 1.0)));
+        }
+        // A tangent boundary of an earlier round is not a sharp edge, and cannot be
+        // reinterpreted as the sharp edge exposed when that round is taken off.
+        assert!(fillet(&rounded, &[Vec3::new(12.5, 0.0, 2.0)], 1.0).is_err());
+        // Consuming a mitre must not silently discard its independently requested radius,
+        // or turn a mitre-only selection into a different sharp edge.
+        assert!(fillet_radii(&rounded, &[(vertical, 1.0), (seam, 2.0)]).is_err());
+        assert!(fillet_radii(&rounded, &[(seam, 2.0)]).is_err());
+    }
+
+    #[test]
+    fn issue1557_all_vertical_corners_match_analytic_volume() {
+        let origin = Vec3::new(10.0, -5.0, 7.0);
+        let b = box_solid(origin, origin + Vec3::new(25.0, 25.0, 10.0)).unwrap();
+        let bottom =
+            [Vec3::new(12.5, 0.0, 0.0), Vec3::new(25.0, 12.5, 0.0), Vec3::new(12.5, 25.0, 0.0), Vec3::new(0.0, 12.5, 0.0)].map(|p| p + origin);
+        let vertical =
+            [Vec3::new(0.0, 0.0, 6.0), Vec3::new(25.0, 0.0, 6.0), Vec3::new(25.0, 25.0, 6.0), Vec3::new(0.0, 25.0, 6.0)].map(|p| p + origin);
+        let rounded = fillet(&b, &bottom, 2.0).unwrap();
+        let result = fillet(&rounded, &vertical, 2.0).unwrap();
+        assert!(result.validity().is_empty(), "{:?}", result.validity());
+        assert!(kink(&result) < 0.05);
+        // Integrate rounded-square horizontal sections. Above z=r the area is
+        // L²-(4-π)r²; below, a square of side L-2r plus a disk offset of radius
+        // sqrt(r²-(z-r)²). This includes the four spherical bottom corners.
+        let (l, h, r) = (25.0, 10.0, 2.0);
+        let pi = std::f64::consts::PI;
+        let a = l - 2.0 * r;
+        let expected = (h - r) * (l * l - (4.0 - pi) * r * r) + a * a * r + pi * a * r * r + 2.0 * pi * r * r * r / 3.0;
+        assert!((vol(&result) - expected).abs() < 0.1, "{} vs {expected}", vol(&result));
+    }
+
     #[test]
     fn three_rounds_at_a_corner() {
         let (bx, a, b, c) = setup();

crates/kernel/src/delete_face.rs

@@ -3,7 +3,8 @@
 //!
 //! The body is rebuilt from its own topology. Faces away from the gap are kept as they are.
 //! Each neighbouring face (planar) keeps its edges that don't touch the gap; a loop that ran
-//! entirely along deleted faces (a hole's rim, a boss's footprint) is dropped; a corner on the
+//! entirely along deleted faces is dropped for a hole's rim or a boss's footprint, or restored
+//! at the moved corners for a face enclosed by rounds. A corner on the
 //! gap moves to where its planes meet the neighbours across the gap (a fillet's tangent points
 //! go to the sharp corner), and the stretch of loop that bordered deleted faces closes with a
 //! straight edge between the moved corners. Anything else (curved neighbours, a gap the
@@ -216,6 +217,28 @@ pub fn delete_faces(body: &Body, faces: &[usize]) -> Result<Body> {
             for w in f.boundaries() {
                 let edges: Vec<mt::Edge> = w.edge_iter().cloned().collect();
                 if edges.iter().all(gap) {
+                    // A hole or boss footprint disappears when its walls are removed. A
+                    // face surrounded by rounds does not: its whole boundary moves back to
+                    // the intersections of the remaining planes (#1557). Keep that loop,
+                    // sharing its restored edges with the adjacent rebuilt faces.
+                    let moving = edges.iter().any(|e| moved.contains_key(&e.front().id()) || moved.contains_key(&e.back().id()));
+                    if moving {
+                        if !edges.iter().all(|e| moved.contains_key(&e.front().id()) && moved.contains_key(&e.back().id())) {
+                            return Err(fail(format!("face {fi} has a closed gap boundary that cannot be restored completely")));
+                        }
+                        let mut boundary = Vec::new();
+                        for e in &edges {
+                            let (a, b) = (pos(e.front()), pos(e.back()));
+                            if a.dist(b) > q * 10.0 {
+                                let restored = line(a, b, &mut verts);
+                                rec.piece(&restored, e);
+                                boundary.push(restored);
+                            }
+                        }
+                        if !boundary.is_empty() {
+                            wires.push(boundary.into());
+                        }
+                    }
                     continue;
                 }
                 // Start after a gap stretch so stretches don't wrap around.
@@ -321,6 +344,21 @@ mod tests {
             .collect()
     }
 
+    #[test]
+    fn issue1557_delete_closed_fillet_ring_keeps_the_floor() {
+        let b = box_solid(Vec3::ZERO, Vec3::new(25.0, 25.0, 10.0)).unwrap();
+        let picks = [Vec3::new(12.5, 0.0, 0.0), Vec3::new(25.0, 12.5, 0.0), Vec3::new(12.5, 25.0, 0.0), Vec3::new(0.0, 12.5, 0.0)];
+        let rounded = fillet(&b, &picks, 2.0).unwrap();
+        let rounds: Vec<usize> =
+            rounded.solid.face_iter().enumerate().filter(|(_, f)| crate::blend::plane_normal(f).is_none()).map(|(i, _)| i).collect();
+        assert_eq!(rounds.len(), 4);
+        let restored = delete_faces(&rounded, &rounds).unwrap();
+        assert_eq!(restored.face_count(), 6);
+        assert_eq!(restored.unmeshed_faces(), 0);
+        assert!((volume(&restored) - 6250.0).abs() < 1e-6);
+        assert!(restored.contains(Vec3::new(0.1, 0.1, 0.1)));
+    }
+
     #[test]
     fn delete_fillet_chamfer_hole_and_boss_faces() {
         let sharp = 24000.0;