From d1cf06cbea7d6bd9f33385af3cbe5777581671a4 Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Sun, 25 Oct 2026 21:50:25 -0400 Subject: [PATCH] refactor: split complex fillet, model, dimstyle and rectangle code into helpers (#2188) Pure extraction, no behaviour change: same order of side effects, same early returns and error values. Clears cognitive-complexity findings on tangent_circles, Model::build_budgeted, DimStyle::apply_fields and run_rectang2 (S3776), and replaces a redundant closure with a method reference in menus.rs (S1612). --- crates/constraints/src/model.rs | 137 +++++++++++++++++--------------- crates/doc/src/tables.rs | 78 ++++++++++-------- crates/engine/src/cmd/draw2.rs | 46 ++++++----- crates/geom/src/fillet.rs | 103 +++++++++++++++--------- crates/ui-egui/src/menus.rs | 2 +- 5 files changed, 209 insertions(+), 157 deletions(-) diff --git a/crates/constraints/src/model.rs b/crates/constraints/src/model.rs index 165afac8d..d18646c32 100644 --- a/crates/constraints/src/model.rs +++ b/crates/constraints/src/model.rs @@ -191,6 +191,77 @@ impl Model { Self::build_budgeted(d, handles, &mut members_left) } + /// Add the parameters of one supported entity to `m`, and return its shape; `None` when it is + /// left out (too large, degenerate or unsupported). + fn add_params(m: &mut Model, d: &Drawing, h: Handle, kind: &EntityKind, off: usize, members_left: &mut usize) -> Option { + let shape = match kind { + EntityKind::Line(l) => { + m.x0.extend([l.a.x, l.a.y, l.b.x, l.b.y]); + Shape::Line + } + EntityKind::Circle(c) => { + m.x0.extend([c.center.x, c.center.y, c.radius]); + Shape::Circle + } + EntityKind::Arc(a) => { + m.x0.extend([a.center.x, a.center.y, a.radius, a.start, a.end]); + Shape::Arc + } + EntityKind::LwPolyline(p) => { + if p.vertices.len() < 2 || off + p.vertices.len() * 2 > MAX_PARAMS { + return None; + } + for v in &p.vertices { + m.x0.extend([v.p.x, v.p.y]); + } + Shape::Poly { n: p.vertices.len(), closed: p.closed } + } + EntityKind::Point(p) => { + m.x0.extend([p.p.x, p.p.y]); + Shape::Point + } + EntityKind::Ellipse(el) => { + let ell = cadcraft_geom::Ellipse { center: el.center.xy(), major: el.major.xy(), ratio: el.ratio, start: el.start, end: el.end }; + let arc = !ell.is_full(); + m.x0.extend([el.center.x, el.center.y, el.major.x, el.major.y, el.ratio]); + if arc { + m.x0.extend([el.start, el.end]); + } + Shape::Ellipse { arc } + } + EntityKind::Spline(sp) => { + let fit = fit_mode(sp); + let pts = if fit { &sp.fit } else { &sp.control }; + let n = pts.len(); + if !(2..=MAX_SPLINE_POINTS).contains(&n) || off + n * 2 > MAX_PARAMS || (!fit && !sp.is_valid()) { + return None; + } + for p in pts { + m.x0.extend([p.x, p.y]); + } + // Fit points interpolate exactly, so the curve's ends are the first and last + // fit point; a control polygon's ends are its first and last points when the + // knots are clamped. + let ends = !sp.closed && (fit || (clamped(sp, false) && clamped(sp, true))); + // Missing weights default to 1 in eval; only excess weights are unused. + let mut kept = sp.clone(); + kept.weights.truncate(kept.control.len()); + m.splines.insert(h, kept); + Shape::Spline { n, fit, ends } + } + EntityKind::Insert(ins) => { + let base = Vec2::new(ins.insert.x, ins.insert.y); + let pts: Vec = rigid_points_budgeted(d, ins, members_left).into_iter().map(|p| (p - base).rotate(-ins.rotation)).collect(); + m.x0.extend([ins.insert.x, ins.insert.y, ins.rotation]); + let n = pts.len(); + m.rigid.insert(h, pts); + Shape::Insert { n } + } + _ => return None, + }; + Some(shape) + } + /// Build solver parameters for supported entities while sharing rigid-member sampling work with other models. pub(crate) fn build_budgeted(d: &Drawing, handles: &[Handle], members_left: &mut usize) -> Model { let mut m = Model::default(); @@ -200,71 +271,7 @@ impl Model { } let Some(e) = d.entity(h) else { continue }; let off = m.x0.len(); - let shape = match &e.kind { - EntityKind::Line(l) => { - m.x0.extend([l.a.x, l.a.y, l.b.x, l.b.y]); - Shape::Line - } - EntityKind::Circle(c) => { - m.x0.extend([c.center.x, c.center.y, c.radius]); - Shape::Circle - } - EntityKind::Arc(a) => { - m.x0.extend([a.center.x, a.center.y, a.radius, a.start, a.end]); - Shape::Arc - } - EntityKind::LwPolyline(p) => { - if p.vertices.len() < 2 || off + p.vertices.len() * 2 > MAX_PARAMS { - continue; - } - for v in &p.vertices { - m.x0.extend([v.p.x, v.p.y]); - } - Shape::Poly { n: p.vertices.len(), closed: p.closed } - } - EntityKind::Point(p) => { - m.x0.extend([p.p.x, p.p.y]); - Shape::Point - } - EntityKind::Ellipse(el) => { - let ell = cadcraft_geom::Ellipse { center: el.center.xy(), major: el.major.xy(), ratio: el.ratio, start: el.start, end: el.end }; - let arc = !ell.is_full(); - m.x0.extend([el.center.x, el.center.y, el.major.x, el.major.y, el.ratio]); - if arc { - m.x0.extend([el.start, el.end]); - } - Shape::Ellipse { arc } - } - EntityKind::Spline(sp) => { - let fit = fit_mode(sp); - let pts = if fit { &sp.fit } else { &sp.control }; - let n = pts.len(); - if !(2..=MAX_SPLINE_POINTS).contains(&n) || off + n * 2 > MAX_PARAMS || (!fit && !sp.is_valid()) { - continue; - } - for p in pts { - m.x0.extend([p.x, p.y]); - } - // Fit points interpolate exactly, so the curve's ends are the first and last - // fit point; a control polygon's ends are its first and last points when the - // knots are clamped. - let ends = !sp.closed && (fit || (clamped(sp, false) && clamped(sp, true))); - // Missing weights default to 1 in eval; only excess weights are unused. - let mut kept = sp.clone(); - kept.weights.truncate(kept.control.len()); - m.splines.insert(h, kept); - Shape::Spline { n, fit, ends } - } - EntityKind::Insert(ins) => { - let base = Vec2::new(ins.insert.x, ins.insert.y); - let pts: Vec = rigid_points_budgeted(d, ins, members_left).into_iter().map(|p| (p - base).rotate(-ins.rotation)).collect(); - m.x0.extend([ins.insert.x, ins.insert.y, ins.rotation]); - let n = pts.len(); - m.rigid.insert(h, pts); - Shape::Insert { n } - } - _ => continue, - }; + let Some(shape) = Self::add_params(&mut m, d, h, &e.kind, off, members_left) else { continue }; if m.x0.len() > MAX_PARAMS || m.x0.get(off..).unwrap_or(&[]).iter().any(|v| !v.is_finite()) { m.x0.truncate(off); continue; diff --git a/crates/doc/src/tables.rs b/crates/doc/src/tables.rs index 328634831..a2c63ca6a 100644 --- a/crates/doc/src/tables.rs +++ b/crates/doc/src/tables.rs @@ -441,6 +441,49 @@ impl DimStyle { .or_else(|| ["name", "annotative"].into_iter().find(|f| f.eq_ignore_ascii_case(name))) } + /// The value `v` takes for a field whose current value is `old`: coerced to the field's type where + /// the text or number can be read as it. + fn coerce_value(old: &serde_json::Value, v: &serde_json::Value, field: &str) -> serde_json::Value { + use serde_json::Value; + match (old, v) { + (Value::Bool(_), Value::Number(n)) => Value::Bool(n.as_f64().is_some_and(|x| x != 0.0)), + (Value::Bool(_), Value::String(t)) => Value::Bool(matches!(t.to_ascii_lowercase().as_str(), "1" | "on" | "true" | "yes")), + (Value::Number(_), Value::String(t)) => match t.trim().parse::() { + Ok(x) => serde_json::json!(x), + // Lineweight names. + Err(_) if matches!(field, "dimLineweight" | "extLineweight") => match t.trim().to_ascii_lowercase().as_str() { + "bylayer" => serde_json::json!(-1), + "byblock" => serde_json::json!(-2), + "default" => serde_json::json!(-3), + _ => v.clone(), + }, + Err(_) => v.clone(), + }, + (Value::Number(_), Value::Bool(b)) => serde_json::json!(u8::from(*b)), + (Value::String(_), Value::Number(n)) => Value::String(n.to_string()), + (Value::Object(_), Value::Number(n)) => { + let c = n.as_i64().and_then(|i| i16::try_from(i).ok()).map(Color::from_aci); + c.and_then(|c| serde_json::to_value(c).ok()).unwrap_or_else(|| v.clone()) + } + (Value::Object(_), Value::String(t)) => Color::parse(t).and_then(|c| serde_json::to_value(c).ok()).unwrap_or_else(|| v.clone()), + _ => v.clone(), + } + } + + /// Whether `coerced` is outside the range its `field` can take. + fn out_of_range(field: &str, coerced: &serde_json::Value) -> bool { + let x = coerced.as_f64(); + match field { + "textHeight" | "tolScale" => x.is_some_and(|x| x <= 0.0), + "scale" | "arrowSize" | "tickSize" | "fixedExtLength" => x.is_some_and(|x| x < 0.0), + "fit" | "angularZeroSuppression" => x.is_some_and(|x| x > 3.0), + "textMove" | "tolJust" | "arcSymbol" | "textFill" => x.is_some_and(|x| x > 2.0), + "dimLineweight" | "extLineweight" => x.is_some_and(|x| !(-3.0..=211.0).contains(&x)), + "jogAngle" => x.is_some_and(|x| !(5.0..=90.0).contains(&x)), + _ => false, + } + } + /// Set fields from a JSON object of field or DIM* variable names (numbers, booleans as /// 0/1, colours as ACI numbers or names). Returns the names that were not recognised or /// had unusable values. @@ -458,29 +501,7 @@ impl DimStyle { rejected.push(k.clone()); continue; }; - let coerced = match (&old, v) { - (Value::Bool(_), Value::Number(n)) => Value::Bool(n.as_f64().is_some_and(|x| x != 0.0)), - (Value::Bool(_), Value::String(t)) => Value::Bool(matches!(t.to_ascii_lowercase().as_str(), "1" | "on" | "true" | "yes")), - (Value::Number(_), Value::String(t)) => match t.trim().parse::() { - Ok(x) => serde_json::json!(x), - // Lineweight names. - Err(_) if matches!(field, "dimLineweight" | "extLineweight") => match t.trim().to_ascii_lowercase().as_str() { - "bylayer" => serde_json::json!(-1), - "byblock" => serde_json::json!(-2), - "default" => serde_json::json!(-3), - _ => v.clone(), - }, - Err(_) => v.clone(), - }, - (Value::Number(_), Value::Bool(b)) => serde_json::json!(u8::from(*b)), - (Value::String(_), Value::Number(n)) => Value::String(n.to_string()), - (Value::Object(_), Value::Number(n)) => { - let c = n.as_i64().and_then(|i| i16::try_from(i).ok()).map(Color::from_aci); - c.and_then(|c| serde_json::to_value(c).ok()).unwrap_or_else(|| v.clone()) - } - (Value::Object(_), Value::String(t)) => Color::parse(t).and_then(|c| serde_json::to_value(c).ok()).unwrap_or_else(|| v.clone()), - _ => v.clone(), - }; + let coerced = Self::coerce_value(&old, v, field); // Integers stored in u8 fields: round floats. let coerced = match (&old, &coerced) { // Lineweights (i16): whole hundredths of a mm or -1/-2/-3. @@ -502,16 +523,7 @@ impl DimStyle { _ => coerced, }; // Sizes that can't be negative (text height and tolerance scale: not zero either). - let x = coerced.as_f64(); - let out_of_range = match field { - "textHeight" | "tolScale" => x.is_some_and(|x| x <= 0.0), - "scale" | "arrowSize" | "tickSize" | "fixedExtLength" => x.is_some_and(|x| x < 0.0), - "fit" | "angularZeroSuppression" => x.is_some_and(|x| x > 3.0), - "textMove" | "tolJust" | "arcSymbol" | "textFill" => x.is_some_and(|x| x > 2.0), - "dimLineweight" | "extLineweight" => x.is_some_and(|x| !(-3.0..=211.0).contains(&x)), - "jogAngle" => x.is_some_and(|x| !(5.0..=90.0).contains(&x)), - _ => false, - }; + let out_of_range = Self::out_of_range(field, &coerced); if out_of_range { rejected.push(k.clone()); continue; diff --git a/crates/engine/src/cmd/draw2.rs b/crates/engine/src/cmd/draw2.rs index c174994c1..4f52bbef5 100644 --- a/crates/engine/src/cmd/draw2.rs +++ b/crates/engine/src/cmd/draw2.rs @@ -1585,25 +1585,10 @@ fn rect_kind(first: Vec2, (w, h): (f64, f64), rot: f64, fillet: f64, chamfer: (f k } -/// Draws a rectangle from the JSON form: `p1` is a corner, or the centre when `center` is -/// true, and the size comes from `p2`, `dimensions` or `area` plus `length`/`breadth`. -pub(crate) fn run_rectang2(s: &mut Session, p: &Value) -> Result { - let a = point_req("rectang", p, "p1")?; - // Centre mode: `p1` is the rectangle's centre, so the rectangle spans it evenly. - let centered = bool_or(p, "center", false); - let rot = f64_or(p, "rotation", 0.0).to_radians(); - let ch = p - .get("chamfer") - .and_then(Value::as_array) - .map(|c| (c.first().and_then(Value::as_f64).unwrap_or(0.0), c.get(1).and_then(Value::as_f64).unwrap_or(0.0))) - .map(|(x, y)| (if x.is_finite() { x } else { 0.0 }, if y.is_finite() { y } else { 0.0 })) - .unwrap_or((0.0, 0.0)); - if ch.0 < 0.0 || ch.1 < 0.0 { - return Err(bad("rectang", "`chamfer` distances must be zero or positive")); - } - let fillet = size_param("rectang", p, "fillet", true)?.unwrap_or(0.0); - let width = size_param("rectang", p, "width", true)?.unwrap_or(0.0); - let (w, h) = if let Some(d) = p.get("dimensions").and_then(Value::as_array) { +/// The width and height of a rectangle from its `dimensions`, its `p2` corner, its `area` with +/// `length` or `breadth`, or its second corner `p2`, rotated by `rot` about `a`. +fn rect_size(p: &Value, a: Vec2, rot: f64, centered: bool) -> Result<(f64, f64)> { + Ok(if let Some(d) = p.get("dimensions").and_then(Value::as_array) { let l = d.first().and_then(Value::as_f64).filter(|v| v.is_finite()).ok_or_else(|| bad("rectang", "dimensions: [length, width]"))?; let wd = d.get(1).and_then(Value::as_f64).filter(|v| v.is_finite()).ok_or_else(|| bad("rectang", "dimensions: [length, width]"))?; let (sx, sy) = match point_param(p, "p2") { @@ -1628,7 +1613,28 @@ pub(crate) fn run_rectang2(s: &mut Session, p: &Value) -> Result { let loc = (b - a).rotate(-rot); // In centre mode the picked point is one corner, mirrored across the centre. if centered { (2.0 * loc.x, 2.0 * loc.y) } else { (loc.x, loc.y) } - }; + }) +} + +/// Draws a rectangle from the JSON form: `p1` is a corner, or the centre when `center` is +/// true, and the size comes from `p2`, `dimensions` or `area` plus `length`/`breadth`. +pub(crate) fn run_rectang2(s: &mut Session, p: &Value) -> Result { + let a = point_req("rectang", p, "p1")?; + // Centre mode: `p1` is the rectangle's centre, so the rectangle spans it evenly. + let centered = bool_or(p, "center", false); + let rot = f64_or(p, "rotation", 0.0).to_radians(); + let ch = p + .get("chamfer") + .and_then(Value::as_array) + .map(|c| (c.first().and_then(Value::as_f64).unwrap_or(0.0), c.get(1).and_then(Value::as_f64).unwrap_or(0.0))) + .map(|(x, y)| (if x.is_finite() { x } else { 0.0 }, if y.is_finite() { y } else { 0.0 })) + .unwrap_or((0.0, 0.0)); + if ch.0 < 0.0 || ch.1 < 0.0 { + return Err(bad("rectang", "`chamfer` distances must be zero or positive")); + } + let fillet = size_param("rectang", p, "fillet", true)?.unwrap_or(0.0); + let width = size_param("rectang", p, "width", true)?.unwrap_or(0.0); + let (w, h) = rect_size(p, a, rot, centered)?; if w.abs() < 1e-12 || h.abs() < 1e-12 || !w.is_finite() || !h.is_finite() { return Err(bad("rectang", "rectangle has no area")); } diff --git a/crates/geom/src/fillet.rs b/crates/geom/src/fillet.rs index d31757f79..78e1fc93b 100644 --- a/crates/geom/src/fillet.rs +++ b/crates/geom/src/fillet.rs @@ -236,6 +236,46 @@ pub fn tangent_circles(c1: &TanCurve, c2: &TanCurve, r: f64, near: &[Vec2]) -> V if !(r.is_finite() && r >= 0.0) { return Vec::new(); } + let Some(win) = search_window(c1, c2, near) else { return Vec::new() }; + let size = win.width().max(win.height()).max(r).max(1e-9); + let win = win.expand(2.0 * r + 0.25 * size); + let tol = 1e-9 * size.max(1.0); + let (Some(r1), Some(r2)) = (c1.range(&win), c2.range(&win)) else { return Vec::new() }; + let signs: &[(f64, f64)] = if r > 0.0 { &[(1.0, 1.0), (1.0, -1.0), (-1.0, 1.0), (-1.0, -1.0)] } else { &[(0.0, 0.0)] }; + let mut out: Vec = Vec::new(); + let mut work = 16_000_000usize; + for (s1, s2) in signs { + let (d1, d2) = (s1 * r, s2 * r); + let (o1, o2) = (sample(c1, &r1, d1), sample(c2, &r2, d2)); + let search = TanSearch { c1, r1: &r1, d1, c2, r2: &r2, d2, tol }; + // Refine each pair before visiting the next: raw duplicate seeds in an early run must + // not consume all candidate slots. A separate request-wide budget bounds actual work. + for a in &o1 { + for b in &o2 { + if !refine_pair(&search, a, b, &mut work, &mut out) { + return out; + } + } + } + } + out +} + +/// The two curves of one tangent search for one sign choice: their ranges and offsets, and the +/// tolerance. +struct TanSearch<'a> { + c1: &'a TanCurve, + r1: &'a Range, + d1: f64, + c2: &'a TanCurve, + r2: &'a Range, + d2: f64, + tol: f64, +} + +/// The window the tangent search looks in: the pick points and the extents of the curves that +/// are not lines. `None` when it is empty or not finite. +fn search_window(c1: &TanCurve, c2: &TanCurve, near: &[Vec2]) -> Option { let mut win = Bounds2::from_points(near.iter().copied().filter(|p| p.is_finite())); for c in [c1, c2] { match c { @@ -251,47 +291,34 @@ pub fn tangent_circles(c1: &TanCurve, c2: &TanCurve, r: f64, near: &[Vec2]) -> V } } if win.is_empty() || !(win.width().is_finite() && win.height().is_finite()) { - return Vec::new(); + return None; } - let size = win.width().max(win.height()).max(r).max(1e-9); - let win = win.expand(2.0 * r + 0.25 * size); - let tol = 1e-9 * size.max(1.0); - let (Some(r1), Some(r2)) = (c1.range(&win), c2.range(&win)) else { return Vec::new() }; - let signs: &[(f64, f64)] = if r > 0.0 { &[(1.0, 1.0), (1.0, -1.0), (-1.0, 1.0), (-1.0, -1.0)] } else { &[(0.0, 0.0)] }; - let mut out: Vec = Vec::new(); - let mut work = 16_000_000usize; - for (s1, s2) in signs { - let (d1, d2) = (s1 * r, s2 * r); - let (o1, o2) = (sample(c1, &r1, d1), sample(c2, &r2, d2)); - // Refine each pair before visiting the next: raw duplicate seeds in an early run must - // not consume all candidate slots. A separate request-wide budget bounds actual work. - for a in &o1 { - for b in &o2 { - let mut seeds = Vec::new(); - crossings(a, b, &mut seeds, &mut work); - seeds.sort_by(|x, y| x.0.total_cmp(&y.0).then(x.1.total_cmp(&y.1))); - seeds.dedup_by(|x, y| (x.0 - y.0).abs() < 1e-9 && (x.1 - y.1).abs() < 1e-9); - for seed in seeds { - // Newton refinement has a fixed iteration cap and several curve evaluations. - let Some(left) = work.checked_sub(128) else { return out }; - work = left; - let Some((t1, t2)) = refine(c1, &r1, d1, c2, &r2, d2, seed, tol) else { continue }; - let (Some(center), p1, p2) = (c1.offset(t1, d1), c1.point(t1), c2.point(t2)) else { continue }; - let near_dup = |f: &CornerFit| f.center.near(center, tol * 1e3) && f.p1.near(p1, tol * 1e3) && f.p2.near(p2, tol * 1e3); - if !out.iter().any(near_dup) { - out.push(CornerFit { t1, t2, p1, p2, center }); - } - if out.len() >= MAX_SOLUTIONS { - return out; - } - } - if work == 0 { - return out; - } - } + Some(win) +} + +/// Refine the crossings of the samples `a` and `b` into circles, adding the distinct ones to +/// `out`. `false` when the work budget or the solution cap ends the search. +fn refine_pair(search: &TanSearch, a: &[(f64, Vec2)], b: &[(f64, Vec2)], work: &mut usize, out: &mut Vec) -> bool { + let mut seeds = Vec::new(); + crossings(a, b, &mut seeds, work); + seeds.sort_by(|x, y| x.0.total_cmp(&y.0).then(x.1.total_cmp(&y.1))); + seeds.dedup_by(|x, y| (x.0 - y.0).abs() < 1e-9 && (x.1 - y.1).abs() < 1e-9); + for seed in seeds { + // Newton refinement has a fixed iteration cap and several curve evaluations. + let Some(left) = work.checked_sub(128) else { return false }; + *work = left; + let TanSearch { c1, r1, d1, c2, r2, d2, tol } = *search; + let Some((t1, t2)) = refine(c1, r1, d1, c2, r2, d2, seed, tol) else { continue }; + let (Some(center), p1, p2) = (c1.offset(t1, d1), c1.point(t1), c2.point(t2)) else { continue }; + let near_dup = |f: &CornerFit| f.center.near(center, tol * 1e3) && f.p1.near(p1, tol * 1e3) && f.p2.near(p2, tol * 1e3); + if !out.iter().any(near_dup) { + out.push(CornerFit { t1, t2, p1, p2, center }); + } + if out.len() >= MAX_SOLUTIONS { + return false; } } - out + *work != 0 } #[cfg(test)] diff --git a/crates/ui-egui/src/menus.rs b/crates/ui-egui/src/menus.rs index 480d5563d..40115388b 100644 --- a/crates/ui-egui/src/menus.rs +++ b/crates/ui-egui/src/menus.rs @@ -172,7 +172,7 @@ fn run_ui_command_inner(app: &mut CadApp, id: &str, params: &Value) -> Option