From deb96318a1709a7c0d357ce56bdf6e1f93f4515b Mon Sep 17 00:00:00 2001 From: Irakli Kourtis Date: Mon, 12 Oct 2026 18:57:09 -0400 Subject: [PATCH] fix: fillet fit review follow-ups (#404) * fix: fillet fit review follow-ups From the review of #388 (merged before these landed): - The Fillet dialog's Applied Radius showed nothing when editing a fillet that was already reduced: the preview found the radius by diffing the states before and after. The engine's preview now keeps the command replies, and the dialog reads the radius of the feature they name. - Pattern copies of a reduced fillet try the radius the original was reduced to first, instead of each searching again (up to 16 fillets). - fit: reduce with a chord or variable radius is refused instead of being silently ignored. - One helper appends a note to a feature's warning; too_large is imported like unsupported in the loop and curve blends. Refs #293 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01TWc1smHYwTSVrVc8SNvS44 * Use the solidcraft_doc crate name after the rename Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01HWLPLy7ug7URSK5WybjRNa --------- Co-authored-by: Claude Opus 5.5 Co-authored-by: bherbruck --- crates/doc/src/eval.rs | 30 +++++++++++++-------------- crates/engine/src/cmd/features.rs | 3 +++ crates/engine/src/lib.rs | 14 ++++++++----- crates/engine/src/tests.rs | 17 ++++++++++++++- crates/kernel/src/curveblend.rs | 3 ++- crates/kernel/src/loopblend.rs | 15 ++++++-------- crates/ui-egui/src/preselect_tests.rs | 4 ++++ crates/ui-egui/src/preview.rs | 4 +++- 8 files changed, 58 insertions(+), 32 deletions(-) diff --git a/crates/doc/src/eval.rs b/crates/doc/src/eval.rs index 5f361a8f..9728e54d 100644 --- a/crates/doc/src/eval.rs +++ b/crates/doc/src/eval.rs @@ -274,6 +274,14 @@ fn fillet_fitted( } } +/// Add `note` to a feature's warning. +fn append_warning(warning: &mut Option, note: String) { + *warning = Some(match warning.take() { + Some(w) => format!("{w}; {note}"), + None => note, + }); +} + /// A length in mm, without trailing zeros. fn show_mm(v: f64) -> String { let s = format!("{v:.6}"); @@ -285,11 +293,6 @@ impl ModelState { pub fn fitted_radius(&self, feature: u64) -> Option { self.fitted.get(&feature).copied() } - - /// Every reduced fillet radius (by feature id). - pub fn fitted_radii(&self) -> impl Iterator + '_ { - self.fitted.iter().map(|(k, v)| (*k, *v)) - } } /// Model after some prefix of the timeline. @@ -948,10 +951,7 @@ impl Model { }); if r.is_ok() { if let Some(note) = boolean_diagnostics.warning() { - warning = Some(match warning { - Some(existing) => format!("{existing}; {note}"), - None => note, - }); + append_warning(&mut warning, note); } } else { boolean_diagnostics = Default::default(); @@ -2842,7 +2842,11 @@ fn replay( let Some(mb) = st.bodies.get(ti).cloned() else { continue }; let nb = match &src.kind { FeatureKind::Chamfer { distance2, angle, flip, .. } => chamfer_with(vals, &mb.body, &moved, r, distance2, angle, *flip)?, - FeatureKind::Fillet { fit, .. } => fillet_fitted(&mb.body, &moved, r, *fit)?.0, + // A copy is congruent to the original: the radius it was reduced to first. + FeatureKind::Fillet { fit, .. } => match st.fitted.get(&src.id).copied() { + Some(used) => kernel::fillet(&mb.body, &moved, used).or_else(|_| fillet_fitted(&mb.body, &moved, r, *fit).map(|x| x.0))?, + None => fillet_fitted(&mb.body, &moved, r, *fit)?.0, + }, _ => kernel::fillet(&mb.body, &moved, r)?, }; let nb = Traced::step(nb, &[&mb.traced()]); @@ -3595,11 +3599,7 @@ fn eval_feature(doc: &Document, vals: &BTreeMap, f: &Feature, st: let (b, used) = fillet_fitted(&mb.body, &edges, r, *fit)?; if let Some(used) = used { st.fitted.insert(f.id, used); - let note = format!("the radius is reduced to {} mm, the largest that fits these edges", show_mm(used)); - *warning = Some(match warning.take() { - Some(w) => format!("{w}; {note}"), - None => note, - }); + append_warning(warning, format!("the radius is reduced to {} mm, the largest that fits these edges", show_mm(used))); } (b, input) } diff --git a/crates/engine/src/cmd/features.rs b/crates/engine/src/cmd/features.rs index df8c67da..3adfe75f 100644 --- a/crates/engine/src/cmd/features.rs +++ b/crates/engine/src/cmd/features.rs @@ -790,6 +790,9 @@ fn fillet(s: &mut Session, p: &Value) -> Result { Some("reduce") => solidcraft_doc::FilletFit::Reduce, Some(o) => return Err(bad(cmd, format!("unknown fit `{o}` (exact or reduce)"))), }; + if fit == solidcraft_doc::FilletFit::Reduce && !style.is_constant() { + return Err(bad(cmd, "`fit: reduce` takes a constant radius")); + } let mut out = add_feature(s, p, FeatureKind::Fillet { edges, radius, body: str_(p, "body").map(str::to_string), style, fit })?; // The radius the fillet used, when `fit: reduce` reduced it. if let Some(id) = out.get("feature").and_then(Value::as_u64) diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index 1a38ba67..f4f78e97 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -157,6 +157,8 @@ pub struct Preview { pub before: Arc, /// The model after the commands. pub after: Arc, + /// What each command replied (as `execute` returns it). + pub replies: Vec, } /// Prepared world geometry depends on design/model identity and placement context, not @@ -693,12 +695,12 @@ impl Session { /// A `timeline.redefine` is shown even while the timeline is rolled back to the feature /// being edited. pub fn preview(&self, commands: &[(String, Value)]) -> Result { - self.scratch().preview_in_place(commands).map(|after| Preview { before: self.model.state(), after: after.model.state() }) + self.scratch().preview_in_place(commands).map(|(after, replies)| Preview { before: self.model.state(), after: after.model.state(), replies }) } /// The same evaluated preview in occurrence coordinates, including uncaptured moves. pub fn preview_world(&self, commands: &[(String, Value)]) -> Result { - self.scratch().preview_in_place(commands).map(|after| Preview { before: self.world_state(), after: after.world_state() }) + self.scratch().preview_in_place(commands).map(|(after, replies)| Preview { before: self.world_state(), after: after.world_state(), replies }) } /// World preview with each session's visibility applied, including new body placements. @@ -709,15 +711,17 @@ impl Session { shown.bodies.retain(|body| session.body_visible(&body.name)); Arc::new(shown) }; - self.scratch().preview_in_place(commands).map(|after| Preview { before: visible(self), after: visible(&after) }) + self.scratch().preview_in_place(commands).map(|(after, replies)| Preview { before: visible(self), after: visible(&after), replies }) } - fn preview_in_place(mut self, commands: &[(String, Value)]) -> Result { + fn preview_in_place(mut self, commands: &[(String, Value)]) -> Result<(Self, Vec)> { if commands.is_empty() { return Err(EngineError::Other("nothing to preview".into())); } + let mut replies = Vec::with_capacity(commands.len()); for (id, params) in commands { let r = self.execute(id, params)?; + replies.push(r.clone()); if id == "timeline.redefine" && let Some(fid) = r.get("feature").and_then(Value::as_u64) && let Some(idx) = self.doc.feature_index(fid) @@ -730,7 +734,7 @@ impl Session { } } } - Ok(self) + Ok((self, replies)) } /// Measure one or two selections (see `inspect.measure` with `items`); errors come back as diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 4442b187..b09c2af9 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -2815,6 +2815,21 @@ fn fillet_fit_reduce_uses_the_largest_radius_that_fits() { // Saved only when set. let json = serde_json::to_string(&s.doc.features).unwrap(); assert_eq!(json.matches("\"fit\":\"reduce\"").count(), 1, "{json}"); - // An unknown mode is refused. + // An unknown mode is refused, and so is reduce with a radius that is not constant. assert!(s.execute("solid.fillet", &json!({"edges": mids, "radius": 1, "fit": "taper"})).is_err()); + assert!(s.execute("solid.fillet", &json!({"edges": mids, "radius": 1, "fit": "reduce", "type": "chord"})).is_err()); +} + +#[test] +fn a_pattern_of_a_reduced_fillet_rounds_each_copy_alike() { + // #293: a 6 mm wide post on a plate, its four vertical edges at 3.5 reduced to 3, patterned + // three times: every copy is a full round. + let mut s = Session::default(); + run(&mut s, "solid.box", json!({"length": 60, "width": 20, "height": 2})); + run(&mut s, "solid.box", json!({"length": 14, "width": 6, "height": 10, "corner": [2, 7, 2], "operation": "join", "name": "Post"})); + let mids = json!([[2, 7, 7], [16, 7, 7], [2, 13, 7], [16, 13, 7]]); + run(&mut s, "solid.fillet", json!({"edges": mids, "radius": 3.5, "fit": "reduce", "name": "Round"})); + run(&mut s, "solid.pattern.rectangular", json!({"features": ["Post", "Round"], "dir1": [1, 0, 0], "count1": 3, "spacing1": 20})); + let expect = 60.0 * 20.0 * 2.0 + 3.0 * (840.0 - (4.0 - PI) * 9.0 * 10.0); + assert!(rel(volume(&mut s), expect) < 2e-4, "{} vs {expect}", volume(&mut s)); } diff --git a/crates/kernel/src/curveblend.rs b/crates/kernel/src/curveblend.rs index 758852b4..a53f5c97 100644 --- a/crates/kernel/src/curveblend.rs +++ b/crates/kernel/src/curveblend.rs @@ -11,6 +11,7 @@ use std::collections::HashMap; use solidcraft_geom::Vec3; use truck_modeling::{self as mt, builder}; +use crate::blend::too_large; use crate::body::{Solid, from_p3, p3}; use crate::record::Rebuild; use crate::{KernelError, Result}; @@ -691,7 +692,7 @@ fn curve_blend_with_limit(solid: &Solid, ids: &[mt::EdgeID], size: f64, r: f64, let (ta, oa) = a.as_ref().map(at).unwrap_or((0.0, 0.0)); let (tb, ob) = b.as_ref().map(at).unwrap_or((1.0, 0.0)); if oa > size * 1e-5 || ob > size * 1e-5 || ta < -1e-9 || tb > 1.0 + 1e-9 || tb - ta < 1e-6 { - return Err(crate::blend::too_large("the blend is larger than the neighbouring faces")); + return Err(too_large("the blend is larger than the neighbouring faces")); } let a = a.unwrap_or_else(|| seam.front().clone()); let b = b.unwrap_or_else(|| seam.back().clone()); diff --git a/crates/kernel/src/loopblend.rs b/crates/kernel/src/loopblend.rs index 4b9f5b18..7538747e 100644 --- a/crates/kernel/src/loopblend.rs +++ b/crates/kernel/src/loopblend.rs @@ -10,6 +10,7 @@ use std::collections::HashMap; use solidcraft_geom::Vec3; use truck_modeling::{self as mt, builder}; +use crate::blend::too_large; use crate::body::{Solid, from_p3, p3, v3}; use crate::record::Rebuild; use crate::{KernelError, Result}; @@ -356,7 +357,7 @@ fn build( } let p = joint_point(i).ok_or_else(|| unsupported("degenerate loop joint"))?; if !crate::blend::extension_fits(f, p, *q, size * 1e-7) { - return Err(crate::blend::too_large("the blend is larger than the neighbouring faces")); + return Err(too_large("the blend is larger than the neighbouring faces")); } } // The offset arcs must not turn inside out; an arc of exactly the blend's radius shrinks @@ -371,12 +372,12 @@ fn build( *x = true; } } else if toward && a.dist(c) <= r + tol { - return Err(crate::blend::too_large("the blend is larger than a corner radius")); + return Err(too_large("the blend is larger than a corner radius")); } } } if collapsed.iter().all(|x| *x) { - return Err(crate::blend::too_large("the blend is larger than a corner radius")); + return Err(too_large("the blend is larger than a corner radius")); } let mut va: Vec = pa.iter().map(|p| builder::vertex(p3(*p))).collect(); // A collapsed piece starts and ends at one vertex. @@ -546,11 +547,7 @@ fn build( let along = (from_p3(other.point()) - from_p3(v.point())).dot(target - from_p3(v.point())); let reach = (target - from_p3(v.point())).len2(); if along <= reach + tol { - return Err(crate::blend::too_large(if face_side { - "the blend is longer than the next edge" - } else { - "the blend is taller than the walls" - })); + return Err(too_large(if face_side { "the blend is longer than the next edge" } else { "the blend is taller than the walls" })); } let nb = if face_side { va.get(i) } else { vb.get(i) }.ok_or_else(|| KernelError::Failed("joint".into()))?; let slot = seam_ends.entry(e.id()).or_insert_with(|| (e.absolute_clone(), None, None)); @@ -566,7 +563,7 @@ fn build( let a = a.unwrap_or_else(|| e.absolute_front().clone()); let b = b.unwrap_or_else(|| e.absolute_back().clone()); if from_p3(a.point()).dist(from_p3(b.point())) < tol * 10.0 { - return Err(crate::blend::too_large("the blend is longer than the next edge")); + return Err(too_large("the blend is longer than the next edge")); } seam_subst.insert(id, builder::line(&a, &b)); } diff --git a/crates/ui-egui/src/preselect_tests.rs b/crates/ui-egui/src/preselect_tests.rs index 77a82c6e..241ff237 100644 --- a/crates/ui-egui/src/preselect_tests.rs +++ b/crates/ui-egui/src/preselect_tests.rs @@ -543,6 +543,10 @@ fn fillet_radius_fit_reduces_and_shows_the_applied_radius() { let fillet = app.session.doc.features.last().unwrap().id; let e = crate::dialogs::for_feature(&app, fillet, None).unwrap(); assert_eq!(e.extra.get("fit"), Some(&json!("reduce"))); + // Its edit preview shows the radius it uses too. + let cmds = apply_commands(&app, &e).unwrap(); + let job = crate::preview::compute(&app.session, &cmds, crate::preview::Colors::from_tokens(&crate::theme::Tokens::dark())).unwrap(); + assert_eq!(job.applied, Some(3.0)); } /// Apply a dialog's commands to the session; the new feature's id. diff --git a/crates/ui-egui/src/preview.rs b/crates/ui-egui/src/preview.rs index aa3351aa..fd851234 100644 --- a/crates/ui-egui/src/preview.rs +++ b/crates/ui-egui/src/preview.rs @@ -229,7 +229,9 @@ pub fn compute(s: &Session, cmds: &[(String, Value)], colors: Colors) -> Job { let p = real.map_err(|e| e.to_string())?; let mut b = guard(&|| build(&p.before, &p.after, colors))?; b.bounds = state_bounds(&p.after); - b.applied = p.after.fitted_radii().find(|(id, _)| p.before.fitted_radius(*id).is_none()).map(|(_, r)| r); + // The radius a previewed fillet used when it reduced the one asked for (a new + // fillet or an edited one: the feature the replies name). + b.applied = p.replies.iter().filter_map(|r| r.get("feature").and_then(Value::as_u64)).find_map(|id| p.after.fitted_radius(id)); Ok(b) } }