From 5bb3b6717f4b3f498eabbe30dd6580c29d9e66eb Mon Sep 17 00:00:00 2001 From: HiroKenEz <59478073+HiroKenEz@users.noreply.github.com> Date: Sat, 31 Oct 2026 10:54:06 +0000 Subject: [PATCH] Equation curves: the variable can carry its range's unit (#9333) (#9549) * claim: #9333 * Equation curves: the variable can carry its range's unit (#9333) A curve can set variable_unit "range" (or tick the new box in its window): the variable is then a length, so a ^ 2 / (4 * t) is a length when a is one. Start and end must both be lengths, or one a length and the other a plain number in the design's unit; anything else is refused. Saved only when set, as one pending format change; without it curves behave as before. Co-Authored-By: Claude Opus 5.5 --------- Co-authored-by: Claude Opus 5.5 --- .../pending/equation-variable-unit.json | 34 ++++ .../format/pending/equation-variable-unit.rs | 5 + crates/doc/src/sketch_equation.rs | 155 ++++++++++++++++-- crates/engine/src/cmd/sketch_equation.rs | 16 +- .../engine/src/cmd/sketch_equation_tests.rs | 149 +++++++++++++++++ crates/engine/src/schema/sketch_equation.rs | 33 ++++ .../src/schema/sketch_equation_tests.rs | 17 +- crates/sketch/src/equation.rs | 34 +++- crates/sketch/src/lib.rs | 2 +- crates/ui-egui/src/dialogs_sketch_tests.rs | 45 +++++ crates/ui-egui/src/sketch_modify_ui.rs | 15 +- docs/book/src/guide/equation-curves.md | 18 ++ docs/book/src/reference/glossary.md | 3 +- 13 files changed, 496 insertions(+), 30 deletions(-) create mode 100644 assets/locales/pending/equation-variable-unit.json create mode 100644 crates/doc/src/format/pending/equation-variable-unit.rs diff --git a/assets/locales/pending/equation-variable-unit.json b/assets/locales/pending/equation-variable-unit.json new file mode 100644 index 0000000000..def58055b1 --- /dev/null +++ b/assets/locales/pending/equation-variable-unit.json @@ -0,0 +1,34 @@ +{ + "en": { + "ui.equation_note_range": "The variable is a length, as its range is: mix it with length parameters, and divide it by a length for sin(). A result without a unit is in the design's unit.", + "ui.equation_variable_unit": "Variable has the range's unit" + }, + "ar": { + "ui.equation_note_range": "المتغير طول مثل نطاقه: استخدمه مع معاملات الطول، واقسمه على طول قبل sin(). النتيجة بلا وحدة تكون بوحدة التصميم.", + "ui.equation_variable_unit": "المتغير بوحدة النطاق" + }, + "de": { + "ui.equation_note_range": "Die Variable ist eine Länge, wie ihr Bereich: Sie lässt sich mit Längenparametern kombinieren; für sin() durch eine Länge teilen. Ein Ergebnis ohne Einheit gilt in der Einheit der Konstruktion.", + "ui.equation_variable_unit": "Variable hat die Einheit des Bereichs" + }, + "he": { + "ui.equation_note_range": "המשתנה הוא אורך, כמו הטווח שלו: אפשר לשלב אותו עם פרמטרי אורך, ולחלק אותו באורך עבור sin(). תוצאה ללא יחידה היא ביחידת התכן.", + "ui.equation_variable_unit": "המשתנה ביחידת הטווח" + }, + "ja": { + "ui.equation_note_range": "変数は範囲と同じく長さです。長さのパラメータと組み合わせられます。sin() には長さで割って渡します。単位のない結果はデザインの単位になります。", + "ui.equation_variable_unit": "変数に範囲の単位を持たせる" + }, + "ko": { + "ui.equation_note_range": "변수는 범위와 같이 길이입니다. 길이 매개변수와 함께 쓸 수 있으며, sin()에는 길이로 나누어 넣습니다. 단위가 없는 결과는 디자인 단위로 읽습니다.", + "ui.equation_variable_unit": "변수에 범위의 단위 사용" + }, + "vi": { + "ui.equation_note_range": "Biến là một độ dài, giống như khoảng của nó: có thể kết hợp với tham số độ dài, và chia cho một độ dài khi dùng sin(). Kết quả không có đơn vị sẽ theo đơn vị của thiết kế.", + "ui.equation_variable_unit": "Biến có đơn vị của khoảng" + }, + "zh": { + "ui.equation_note_range": "变量与其范围一样是长度:可与长度参数混用,用于 sin() 时需除以一个长度。没有单位的结果采用设计的单位。", + "ui.equation_variable_unit": "变量使用范围的单位" + } +} diff --git a/crates/doc/src/format/pending/equation-variable-unit.rs b/crates/doc/src/format/pending/equation-variable-unit.rs new file mode 100644 index 0000000000..d94a59a949 --- /dev/null +++ b/crates/doc/src/format/pending/equation-variable-unit.rs @@ -0,0 +1,5 @@ +//! An equation curve can save `variable_unit: "range"`: its variable is a length, as its range is, +//! instead of a plain number (#9333). An older design has none, and its variable stays a plain +//! number, as before; a curve with a plain variable saves nothing new. The last release refuses a +//! design with this change and asks for an update, instead of opening it and losing the key on the +//! next save. diff --git a/crates/doc/src/sketch_equation.rs b/crates/doc/src/sketch_equation.rs index dcfdbcbbc3..ce5fe0e2d3 100644 --- a/crates/doc/src/sketch_equation.rs +++ b/crates/doc/src/sketch_equation.rs @@ -8,7 +8,7 @@ use std::collections::BTreeMap; use solidcraft_geom::Vec2; -use solidcraft_sketch::{EquationCurve, MAX_COORD, MAX_EQUATION_TEXT, MAX_SPLINE_POINTS, Sketch, fit_beziers_mode}; +use solidcraft_sketch::{EquationCurve, MAX_COORD, MAX_EQUATION_TEXT, MAX_SPLINE_POINTS, Sketch, VariableUnit, fit_beziers_mode}; use crate::expr::{self, Defaults, Kind, Value}; use crate::{DocError, Result}; @@ -36,8 +36,13 @@ pub struct EquationFit { pub deviation: f64, } -/// How bare numbers read in the formulas: angles in radians (the variable is a plain number, -/// `sin(t)` its sine), lengths in the design's unit. +/// Why a range cannot give the variable its unit (#9333): a length and an angle have no unit in +/// common, and two plain numbers have none to give. +const RANGE_UNIT_REFUSED: &str = + "with the variable in its range's unit, start and end must both be lengths, or one a length and the other a plain number"; + +/// How bare numbers read in the formulas: angles in radians (a plain variable's `sin(t)` is its +/// sine), lengths in the design's unit. fn defaults(vals: &BTreeMap) -> Defaults { let len = vals.get(crate::eval::DESIGN_LEN).map(|v| v.v).filter(|v| v.is_finite() && *v > 0.0).unwrap_or(1.0); Defaults { len, ang: 1.0 } @@ -54,31 +59,45 @@ fn checked_text<'a>(what: &str, e: &'a str) -> Result<&'a str> { Ok(e) } +/// The variable as the formulas see it, where it is `at` (a number, or the design's unit). +fn variable_value(eq: &EquationCurve, at: f64, d: Defaults) -> Value { + match eq.variable_unit { + VariableUnit::Number => Value::num(at), + VariableUnit::Range => Value::length(at * d.len), + } +} + /// A formula's value (mm) where the variable is `at`. The variable hides a parameter of its name. -fn formula(vals: &BTreeMap, e: &str, variable: &str, at: f64, d: Defaults) -> Result { +fn formula(vals: &BTreeMap, e: &str, variable: &str, at: Value, d: Defaults) -> Result { let lookup = |n: &str| { - if n == variable { Ok(Value::num(at)) } else { vals.get(n).copied().ok_or_else(|| DocError::Expr(format!("unknown parameter `{n}`"))) } + if n == variable { Ok(at) } else { vals.get(n).copied().ok_or_else(|| DocError::Expr(format!("unknown parameter `{n}`"))) } }; let v = expr::eval_with_defaults(e, &lookup, d)?.to_kind_in(Kind::Length, d)?; if v.is_finite() && v.abs() < MAX_COORD { Ok(v) } else { Err(DocError::Expr("the curve goes out of range".into())) } } -/// Where the variable starts or ends, as the plain number the formulas see: a number, an angle -/// (in radians) or a length (in the design's unit). -fn range_end(vals: &BTreeMap, e: &str, d: Defaults) -> Result { +/// Where the variable starts or ends, as a number: a number, an angle (in radians) or a length +/// (in the design's unit); and which of them it is. +fn range_end(vals: &BTreeMap, e: &str, d: Defaults) -> Result<(f64, Kind)> { let v = expr::eval_with_defaults(e, &|n| vals.get(n).copied().ok_or_else(|| DocError::Expr(format!("unknown parameter `{n}`"))), d)?; - let out = match v.kind() { - Some(Kind::Unitless | Kind::Angle) => v.v, - Some(Kind::Length) => v.v / d.len, + let (out, kind) = match v.kind() { + Some(kind @ (Kind::Unitless | Kind::Angle)) => (v.v, kind), + Some(Kind::Length) => (v.v / d.len, Kind::Length), None => return Err(DocError::Expr(format!("expected a number, an angle or a length, got {}", v.describe()))), }; - if out.is_finite() { Ok(out) } else { Err(DocError::Expr("not a finite number".into())) } + if out.is_finite() { Ok((out, kind)) } else { Err(DocError::Expr("not a finite number".into())) } } /// The variable's range, checked. fn range(vals: &BTreeMap, eq: &EquationCurve, d: Defaults) -> Result<(f64, f64)> { - let a = range_end(vals, checked_text("the start", &eq.start)?, d).map_err(|e| DocError::Expr(format!("start: {e}")))?; - let b = range_end(vals, checked_text("the end", &eq.end)?, d).map_err(|e| DocError::Expr(format!("end: {e}")))?; + let (a, start) = range_end(vals, checked_text("the start", &eq.start)?, d).map_err(|e| DocError::Expr(format!("start: {e}")))?; + let (b, end) = range_end(vals, checked_text("the end", &eq.end)?, d).map_err(|e| DocError::Expr(format!("end: {e}")))?; + // A plain number beside a length is in the design's unit, as `range_end` already reads it. + if eq.variable_unit == VariableUnit::Range + && !matches!((start, end), (Kind::Length, Kind::Length | Kind::Unitless) | (Kind::Unitless, Kind::Length)) + { + return Err(DocError::Invalid(RANGE_UNIT_REFUSED.into())); + } if (b - a).abs() <= 1e-12 * a.abs().max(b.abs()).max(1.0) { return Err(DocError::Invalid("the start and the end of the range must differ".into())); } @@ -91,8 +110,9 @@ fn range(vals: &BTreeMap, eq: &EquationCurve, d: Defaults) -> Res /// The curve's point where the variable is `at`. fn point_at(vals: &BTreeMap, eq: &EquationCurve, at: f64, d: Defaults) -> Result { let variable = eq.variable(); + let value = variable_value(eq, at, d); let x = match &eq.x { - Some(e) => formula(vals, e, variable, at, d).map_err(|e| DocError::Expr(format!("x: {e}")))?, + Some(e) => formula(vals, e, variable, value, d).map_err(|e| DocError::Expr(format!("x: {e}")))?, // The explicit form: x is the variable, in the design's unit. None => { let x = at * d.len; @@ -102,7 +122,7 @@ fn point_at(vals: &BTreeMap, eq: &EquationCurve, at: f64, d: Defa x } }; - let y = formula(vals, &eq.y, variable, at, d).map_err(|e| DocError::Expr(format!("y: {e}")))?; + let y = formula(vals, &eq.y, variable, value, d).map_err(|e| DocError::Expr(format!("y: {e}")))?; Ok(Vec2::new(x, y)) } @@ -286,9 +306,24 @@ mod tests { start: start.into(), end: end.into(), tolerance: DEFAULT_TOLERANCE, + variable_unit: VariableUnit::Number, } } + /// The same curve with its variable in its range's unit. + fn with_range_unit(eq: EquationCurve) -> EquationCurve { + EquationCurve { variable_unit: VariableUnit::Range, ..eq } + } + + fn with_a(mm: f64, design_len: Option) -> BTreeMap { + let mut vals = BTreeMap::new(); + vals.insert("a".to_string(), Value::length(mm)); + if let Some(len) = design_len { + vals.insert(crate::eval::DESIGN_LEN.to_string(), Value::num(len)); + } + vals + } + #[test] fn a_circle_closes_and_stays_within_the_tolerance() { let vals = BTreeMap::new(); @@ -360,4 +395,92 @@ mod tests { let long = "t + ".repeat(400) + "t"; assert!(fit(&vals, &eq(Some(&long), "t", "0", "1")).is_err()); } + + /// The issue's parabola (#9333): y = a^2 / (4 t) over t from a to 3a, where t is a length. + /// Every point is exact, and the curve is the same in millimetres whatever the design's unit. + #[test] + fn a_variable_in_its_ranges_unit_gives_the_exact_parabola_in_any_design_unit() { + for design_len in [None, Some(25.4)] { + let vals = with_a(10.0, design_len); + let e = with_range_unit(eq(Some("t"), "a ^ 2 / (4 * t)", "a", "3 * a")); + let fit = fit(&vals, &e).unwrap(); + let (first, last) = (fit.points[0], *fit.points.last().unwrap()); + assert!((first - Vec2::new(10.0, 2.5)).len() < 1e-9, "{design_len:?} {first:?}"); + assert!((last - Vec2::new(30.0, 100.0 / 120.0)).len() < 1e-9, "{design_len:?} {last:?}"); + for p in &fit.points { + assert!((p.y - 100.0 / (4.0 * p.x)).abs() < 1e-9 && (10.0 - 1e-9..=30.0 + 1e-9).contains(&p.x), "{design_len:?} {p:?}"); + } + assert!(fit.deviation <= DEFAULT_TOLERANCE); + } + } + + /// The explicit form names its variable x; bare numbers beside it are in the design's unit. + #[test] + fn the_explicit_form_and_bare_numbers_follow_the_design_unit() { + let vals = with_a(25.4, Some(25.4)); + let fit = fit(&vals, &with_range_unit(eq(None, "x + 1", "0", "a"))).unwrap(); + let (first, last) = (fit.points[0], *fit.points.last().unwrap()); + assert!((first - Vec2::new(0.0, 25.4)).len() < 1e-9, "{first:?}"); + assert!((last - Vec2::new(25.4, 50.8)).len() < 1e-9, "{last:?}"); + } + + /// The rule the issue settled for the range's two ends (#9333). + #[test] + fn a_range_gives_its_unit_only_from_lengths_and_plain_numbers_beside_them() { + // A plain number beside a length is in the design's unit, on either side. + for (design_len, unit) in [(None, 1.0), (Some(25.4), 25.4)] { + let vals = with_a(100.0, design_len); + let up = fit(&vals, &with_range_unit(eq(Some("t"), "t / 2", "1", "a"))).unwrap(); + assert!((up.points[0] - Vec2::new(unit, unit / 2.0)).len() < 1e-9, "{:?}", up.points[0]); + assert!((*up.points.last().unwrap() - Vec2::new(100.0, 50.0)).len() < 1e-9); + let down = fit(&vals, &with_range_unit(eq(Some("t"), "t / 2", "a", "1"))).unwrap(); + assert!((down.points[0] - Vec2::new(100.0, 50.0)).len() < 1e-9); + assert!((*down.points.last().unwrap() - Vec2::new(unit, unit / 2.0)).len() < 1e-9); + } + // Nothing else: a length and an angle, and ends with no length to give. + let vals = with_a(10.0, None); + for (a, b) in [("a", "90 deg"), ("90 deg", "a"), ("0 deg", "90 deg"), ("0", "90 deg"), ("90 deg", "0"), ("0", "1")] { + let why = fit(&vals, &with_range_unit(eq(Some("t"), "t", a, b))).unwrap_err().to_string(); + assert!( + why.contains( + "with the variable in its range's unit, start and end must both be lengths, or one a length and the other a plain number" + ), + "{a}..{b}: {why}" + ); + } + // Off, the same ranges build, with the variable a plain number. + assert!(fit(&vals, &eq(Some("t"), "t", "a", "90 deg")).is_ok()); + } + + /// Off (#9333): the variable is a plain number over a length range, as saved curves expect. + #[test] + fn without_the_flag_the_variable_stays_a_plain_number() { + let vals = with_a(10.0, None); + let plain = fit(&vals, &eq(Some("t"), "t * t / 20", "0 mm", "40 mm")).unwrap(); + let last = *plain.points.last().unwrap(); + assert!((last - Vec2::new(40.0, 80.0)).len() < 1e-9, "{last:?}"); + assert!(fit(&vals, &eq(Some("t"), "a ^ 2 / (4 * t)", "a", "3 * a")).unwrap_err().to_string().contains("length^2")); + // On, that plain-number formula is a length squared, and is refused rather than misread. + assert!(fit(&vals, &with_range_unit(eq(Some("t"), "t * t / 20", "0 mm", "40 mm"))).is_err()); + } + + #[test] + fn hostile_formulas_with_the_variable_in_its_ranges_unit_are_refused() { + let vals = with_a(10.0, Some(25.4)); + for (x, y, a, b) in [ + ("t", "a ^ 2 / (4 * t)", "0", "a"), + ("t", "sqrt(t)", "a", "3 * a"), + ("t", "t ^ 1000", "a", "3 * a"), + ("t", "t", "a", "a"), + ("t", "t", "0", "1e300 mm"), + ("t", "t", "a", "1e308 in"), + ("t", "t", "a", "nan"), + ("t", "t", "a * a", "3 * a"), + ("1e300 * t", "t", "a", "3 * a"), + ("sin(t)", "t", "a", "3 * a"), + ("t", "unknown_name * t", "a", "3 * a"), + ] { + assert!(fit(&vals, &with_range_unit(eq(Some(x), y, a, b))).is_err(), "{x} {y} {a}..{b}"); + } + } } diff --git a/crates/engine/src/cmd/sketch_equation.rs b/crates/engine/src/cmd/sketch_equation.rs index 5aac5da3eb..1ab4bf5b27 100644 --- a/crates/engine/src/cmd/sketch_equation.rs +++ b/crates/engine/src/cmd/sketch_equation.rs @@ -5,7 +5,7 @@ use serde_json::{Value, json}; use solidcraft_doc::sketch_equation::{self, DEFAULT_TOLERANCE, EquationFit}; -use solidcraft_sketch::{EquationCurve, Sketch}; +use solidcraft_sketch::{EquationCurve, Sketch, VariableUnit}; use super::sketch::{edit, mark_construction}; use super::{CommandSpec, in_sketch}; @@ -31,8 +31,10 @@ pub static COMMANDS: &[CommandSpec] = &[ .params(concat!( "x: x(t), y: y(t): formulas of t in the design's expression language and parameters (\"r * (cos(t) + t * sin(t))\"); or explicit: true and y: f(x) alone (no x); \ start, end: the range of t (or of x): a number or an expression (\"2 * pi\", \"360 deg\" is 2·pi, a length is in the design's unit); \ - tolerance?: mm (default 0.01): how far the curve may stray from the formulas (0.0001 to 10); construction?: bool. \ + tolerance?: mm (default 0.01): how far the curve may stray from the formulas (0.0001 to 10); construction?: bool; \ + variable_unit?: \"number\" (default) or \"range\". \ The variable is a plain number: sin(t) reads it in radians; a result without a unit is in the design's length unit. \ + With variable_unit \"range\" it is a length, so a ^ 2 / (4 * t) is a length when a is one: start and end must both be lengths, or one a length and the other a plain number, which is then in the design's unit. \ Makes a fit point spline through fixed points, as many as the tolerance needs (up to 500), closed smoothly when it ends where it starts; \ the points are placed again from the formulas whenever the parameters change. Returns curve, points (how many), closed, deviation (mm)", created_doc!() @@ -43,7 +45,7 @@ pub static COMMANDS: &[CommandSpec] = &[ .contract(&EDIT_CONTRACT) .output(output_schema_for::) .params( - "curve: an equation curve's id; x?, y?, explicit?, start?, end?, tolerance?: as in sketch.equation_curve, each kept when left out (explicit: true drops x; false needs x). \ + "curve: an equation curve's id; x?, y?, explicit?, start?, end?, tolerance?, variable_unit?: as in sketch.equation_curve, each kept when left out (explicit: true drops x; false needs x). \ The points are chosen again for the new formulas; the curve's start point (and the end point of an open curve) keeps its id, so geometry joined there stays joined. Returns curve, points, closed, deviation", ), ]; @@ -81,6 +83,13 @@ fn explicit(p: &Value, cmd: &str) -> Result> { } } +fn variable_unit(p: &Value, cmd: &str) -> Result> { + match p.get("variable_unit") { + None | Some(Value::Null) => Ok(None), + Some(v) => v.as_str().and_then(VariableUnit::parse).map(Some).ok_or_else(|| bad(cmd, "`variable_unit` must be \"number\" or \"range\"")), + } +} + /// The equation a command's parameters give, over `old` (each left out kept). fn equation(p: &Value, cmd: &str, old: Option<&EquationCurve>) -> Result { let x = text(p, cmd, "x")?; @@ -111,6 +120,7 @@ fn equation(p: &Value, cmd: &str, old: Option<&EquationCurve>) -> Result String { + run(s, "parameters.add", json!({"name": "a", "expression": "10 mm"})); + run(s, "sketch.create", json!({"plane": "XY", "name": "S"})); + let r = run(s, "sketch.equation_curve", json!({"x": "t", "y": "a ^ 2 / (4 * t)", "start": "a", "end": "3 * a", "variable_unit": "range"})); + assert_eq!(r["closed"], false, "{r}"); + let id = r["curve"].as_str().unwrap().to_string(); + run(s, "sketch.finish", json!({})); + no_trouble(s); + id +} + +#[test] +fn a_variable_in_its_ranges_unit_follows_the_parameter_and_the_design_unit() { + let mut s = Session::default(); + // Without the flag this very formula is refused, as it always was. + run(&mut s, "parameters.add", json!({"name": "b", "expression": "10 mm"})); + run(&mut s, "sketch.create", json!({"plane": "XY", "name": "Off"})); + let why = s.execute("sketch.equation_curve", &json!({"x": "t", "y": "b ^ 2 / (4 * t)", "start": "b", "end": "3 * b"})).unwrap_err().to_string(); + assert!(why.contains("length^2"), "{why}"); + run(&mut s, "sketch.finish", json!({})); + + let id = range_unit_curve(&mut s); + assert_parabola(&s, &id, 10.0); + run(&mut s, "parameters.change", json!({"name": "a", "expression": "20 mm"})); + no_trouble(&s); + assert_parabola(&s, &id, 20.0); + // In millimetres the parameter's length does not depend on the design's unit, nor the curve. + run(&mut s, "document.units", json!({"units": "in"})); + no_trouble(&s); + assert_parabola(&s, &id, 20.0); + // A parameter given in inches, in an inch design. + run(&mut s, "parameters.change", json!({"name": "a", "expression": "2 in"})); + no_trouble(&s); + assert_parabola(&s, &id, 50.8); +} + +/// A plain end is in the design's unit, so it moves when the unit changes (#9333). +#[test] +fn a_plain_end_of_the_range_takes_the_design_unit() { + let mut s = Session::default(); + run(&mut s, "parameters.add", json!({"name": "a", "expression": "100 mm"})); + run(&mut s, "sketch.create", json!({"plane": "XY", "name": "S"})); + let r = run(&mut s, "sketch.equation_curve", json!({"x": "t", "y": "t / 2", "start": 1, "end": "a", "variable_unit": "range"})); + let id = r["curve"].as_str().unwrap().to_string(); + run(&mut s, "sketch.finish", json!({})); + assert!((curve_points(&s, &id)[0] - Vec2::new(1.0, 0.5)).len() < 1e-9); + run(&mut s, "document.units", json!({"units": "in"})); + no_trouble(&s); + let pts = curve_points(&s, &id); + assert!((pts[0] - Vec2::new(25.4, 12.7)).len() < 1e-9, "{:?}", pts[0]); + assert!((*pts.last().unwrap() - Vec2::new(100.0, 50.0)).len() < 1e-9); +} + +#[test] +fn the_refused_range_says_why_and_changes_nothing() { + let mut s = Session::default(); + run(&mut s, "parameters.add", json!({"name": "a", "expression": "10 mm"})); + run(&mut s, "sketch.create", json!({"plane": "XY", "name": "S"})); + let before = s.doc.to_json(); + for (start, end) in [("a", "90 deg"), ("90 deg", "a"), ("0", "1")] { + let p = json!({"x": "t", "y": "t", "start": start, "end": end, "variable_unit": "range"}); + let why = s.execute("sketch.equation_curve", &p).unwrap_err().to_string(); + assert!( + why.contains("with the variable in its range's unit, start and end must both be lengths, or one a length and the other a plain number"), + "{p}: {why}" + ); + assert_eq!(s.doc.to_json(), before, "{p} changed the design"); + } + for bad in [json!("mm"), json!(1), json!(true), json!({}), json!("Range")] { + assert!(s.execute("sketch.equation_curve", &json!({"x": "t", "y": "t", "start": 0, "end": "a", "variable_unit": bad})).is_err(), "{bad}"); + } +} + +/// A design file is hostile (#9333): a range the rule refuses is reported on the sketch, and a +/// value that means nothing refuses the file. Neither crashes. +#[test] +fn a_hostile_saved_flag_or_range_is_an_error_not_a_crash() { + let mut s = Session::default(); + range_unit_curve(&mut s); + let saved = s.doc.to_json(); + assert!(saved.contains("3 * a") && saved.contains("\"range\""), "{saved}"); + let angle = saved.replace("3 * a", "90 deg"); + let r = Session::new(solidcraft_doc::Document::from_json(&angle).unwrap()); + let res = r.model.results.iter().find(|r| r.name == "S").unwrap(); + assert!(res.error.as_deref().is_some_and(|e| e.contains("equation curve") && e.contains("must both be lengths")), "{:?}", res.error); + assert!(solidcraft_doc::Document::from_json(&saved.replace("\"range\"", "\"furlong\"")).is_err()); +} + +#[test] +fn an_edit_keeps_the_flag_until_told_and_a_saved_design_keeps_it() { + use solidcraft_sketch::VariableUnit; + let mut s = Session::default(); + let id = range_unit_curve(&mut s); + let f = s.doc.find_feature("S").unwrap().id; + // The file says it, and reopening follows it. + let saved = s.doc.to_json(); + assert!(saved.contains("variable_unit"), "not saved"); + let mut r = Session::new(solidcraft_doc::Document::from_json(&saved).unwrap()); + no_trouble(&r); + assert_eq!(r.doc.sketch(f).unwrap().equation_of(&id).unwrap().variable_unit, VariableUnit::Range); + assert_parabola(&r, &id, 10.0); + run(&mut r, "parameters.change", json!({"name": "a", "expression": "20 mm"})); + assert_parabola(&r, &id, 20.0); + // An edit that leaves it out keeps it. + run(&mut s, "sketch.edit", json!({"sketch": "S"})); + run(&mut s, "sketch.equation_curve.edit", json!({"curve": id, "tolerance": 0.005})); + assert_eq!(s.doc.sketch(f).unwrap().equation_of(&id).unwrap().variable_unit, VariableUnit::Range); + // Off again, that formula is refused and the curve stays as it was. + assert!(s.execute("sketch.equation_curve.edit", &json!({"curve": id, "variable_unit": "number"})).is_err()); + assert_eq!(s.doc.sketch(f).unwrap().equation_of(&id).unwrap().variable_unit, VariableUnit::Range); + run(&mut s, "sketch.equation_curve.edit", json!({"curve": id, "y": "t / 2", "start": 0, "end": 10, "variable_unit": "number"})); + assert_eq!(s.doc.sketch(f).unwrap().equation_of(&id).unwrap().variable_unit, VariableUnit::Number); + assert!(!s.doc.to_json().contains("variable_unit")); +} + +/// A design written without the flag (#9333): nothing in the file changes, and its curve is what +/// it was, with the variable a plain number over a range in length. +#[test] +fn a_design_saved_without_the_flag_reopens_the_same() { + let mut s = Session::default(); + run(&mut s, "sketch.create", json!({"plane": "XY", "name": "S"})); + let r = run(&mut s, "sketch.equation_curve", json!({"x": "t", "y": "t * t / 20", "start": "0 mm", "end": "40 mm"})); + let id = r["curve"].as_str().unwrap().to_string(); + run(&mut s, "sketch.finish", json!({})); + let saved = s.doc.to_json(); + assert!(!saved.contains("variable_unit"), "a curve without the flag writes no new key"); + let want = curve_points(&s, &id); + assert!((*want.last().unwrap() - Vec2::new(40.0, 80.0)).len() < 1e-9); + // The format before this change, as the last release wrote it. + let mut old: Value = serde_json::from_str(&saved).unwrap(); + old["format"] = json!(solidcraft_doc::format::format_string(solidcraft_doc::format::FORMAT - 1)); + if let Some(o) = old.as_object_mut() { + o.remove("format_pending"); + } + let r = Session::new(solidcraft_doc::Document::from_json(&old.to_string()).unwrap()); + no_trouble(&r); + assert_eq!(curve_points(&r, &id), want); +} diff --git a/crates/engine/src/schema/sketch_equation.rs b/crates/engine/src/schema/sketch_equation.rs index 66b1b18dd0..ee3b04f720 100644 --- a/crates/engine/src/schema/sketch_equation.rs +++ b/crates/engine/src/schema/sketch_equation.rs @@ -68,6 +68,33 @@ impl JsonSchema for Tolerance { } } +/// What the curve's variable is in its formulas. +#[derive(Clone, Copy, Debug, PartialEq)] +pub struct VariableUnitParam(pub solidcraft_sketch::VariableUnit); + +impl<'de> Deserialize<'de> for VariableUnitParam { + fn deserialize>(d: D) -> Result { + Value::deserialize(d)? + .as_str() + .and_then(solidcraft_sketch::VariableUnit::parse) + .map(VariableUnitParam) + .ok_or_else(|| D::Error::custom("must be \"number\" or \"range\"")) + } +} + +impl JsonSchema for VariableUnitParam { + fn schema_name() -> Cow<'static, str> { + "EquationVariableUnit".into() + } + fn json_schema(_: &mut SchemaGenerator) -> Schema { + json_schema!({ + "description": "What the variable is in the formulas. \"number\": a plain number. \"range\": a length, as the range is; start and end must then both be lengths, or one a length and the other a plain number, which is in the design's unit. Default \"number\".", + "type": "string", + "enum": ["number", "range"] + }) + } +} + /// An equation curve's id. #[derive(Clone, Debug, PartialEq)] pub struct CurveId(pub String); @@ -113,6 +140,9 @@ pub struct EquationCurveParams { /// How far (mm) the curve may stray from its formulas. Default 0.01. #[serde(default)] pub tolerance: Option, + /// What the variable is in the formulas. Default "number". + #[serde(default)] + pub variable_unit: Option, /// Make it construction geometry. Default false. #[serde(default)] pub construction: Option, @@ -147,6 +177,9 @@ pub struct EquationEditParams { /// How far (mm) the curve may stray from its formulas. Kept when left out. #[serde(default)] pub tolerance: Option, + /// What the variable is in the formulas. Kept when left out. + #[serde(default)] + pub variable_unit: Option, /// The sketch (default: the one being edited). #[serde(default, deserialize_with = "present", skip_serializing_if = "Option::is_none")] #[schemars(with = "SketchRef")] diff --git a/crates/engine/src/schema/sketch_equation_tests.rs b/crates/engine/src/schema/sketch_equation_tests.rs index ca3ff39689..3d76691252 100644 --- a/crates/engine/src/schema/sketch_equation_tests.rs +++ b/crates/engine/src/schema/sketch_equation_tests.rs @@ -7,8 +7,11 @@ use crate::schema::{ParamCheck, input_schema_for, output_schema_for}; #[test] fn schemas_and_the_key_check_agree() { - assert_keys(input_schema_for::(), &["construction", "end", "explicit", "sketch", "start", "tolerance", "x", "y"]); - assert_keys(input_schema_for::(), &["curve", "end", "explicit", "sketch", "start", "tolerance", "x", "y"]); + assert_keys( + input_schema_for::(), + &["construction", "end", "explicit", "sketch", "start", "tolerance", "variable_unit", "x", "y"], + ); + assert_keys(input_schema_for::(), &["curve", "end", "explicit", "sketch", "start", "tolerance", "variable_unit", "x", "y"]); } #[test] @@ -21,6 +24,9 @@ fn strict_keeps_todays_errors() { ("sketch.equation_curve", json!({"x": "t", "y": "t", "start": 0, "end": 1, "explicit": 1})), ("sketch.equation_curve", json!({"x": "t", "y": "t", "start": 0, "end": 1e12})), ("sketch.equation_curve", json!({"y": "t", "start": 0, "end": 1})), + ("sketch.equation_curve", json!({"x": "t", "y": "t", "start": 0, "end": 1, "variable_unit": "mm"})), + ("sketch.equation_curve", json!({"x": "t", "y": "t", "start": 0, "end": 1, "variable_unit": true})), + ("sketch.equation_curve", json!({"x": "t", "y": "t", "start": 0, "end": 1, "variable_unit": "range"})), ("sketch.equation_curve.edit", json!({"curve": 7})), ("sketch.equation_curve.edit", json!({"curve": "nope", "y": "t"})), ]); @@ -43,4 +49,11 @@ fn replies_match_their_schemas() { let edited = edited.as_value(); let r = run(&mut s, "sketch.equation_curve.edit", json!({"curve": id, "end": "6 * pi"})); assert!(conforms(edited, edited, &r), "{r} doesn't match {edited}"); + // The variable in its range's unit, then back: both accepted in strict, nothing warned. + s.param_check = ParamCheck::Strict; + let r = run(&mut s, "sketch.equation_curve", json!({"x": "t", "y": "t / 2", "start": 0, "end": "10 mm", "variable_unit": "range"})); + assert!(conforms(created, created, &r), "{r} doesn't match {created}"); + let id = r["curve"].as_str().unwrap_or_else(|| panic!("{r}")).to_string(); + let r = run(&mut s, "sketch.equation_curve.edit", json!({"curve": id, "variable_unit": "number"})); + assert!(conforms(edited, edited, &r), "{r} doesn't match {edited}"); } diff --git a/crates/sketch/src/equation.rs b/crates/sketch/src/equation.rs index 90949072a3..ec78b9b05b 100644 --- a/crates/sketch/src/equation.rs +++ b/crates/sketch/src/equation.rs @@ -12,10 +12,37 @@ use crate::model::{CurveKind, Result, Sketch, SketchError}; /// Longest formula or range expression an equation curve takes (characters). pub const MAX_EQUATION_TEXT: usize = 1024; +/// What the variable of an equation curve is in its formulas (#9333). +#[derive(Clone, Copy, Debug, Default, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "lowercase")] +pub enum VariableUnit { + /// A plain number: `sin(t)` reads it in radians, and a range in a unit gives up that unit. + #[default] + Number, + /// A length, as the range is: `a ^ 2 / (4 * t)` is a length when `a` is one. + Range, +} + +impl VariableUnit { + /// The word a command or a design spells it with. + pub fn parse(word: &str) -> Option { + match word { + "number" => Some(VariableUnit::Number), + "range" => Some(VariableUnit::Range), + _ => None, + } + } + + fn is_number(&self) -> bool { + *self == VariableUnit::Number + } +} + /// A curve drawn from formulas: x(t) and y(t) over t from `start` to `end`, or (with `x` left /// out) the explicit y = f(x) over x from `start` to `end`. The formulas use the design's -/// expression language and parameters; the variable is a plain number, so `sin(t)` reads it in -/// radians, and a result without a unit is in the design's length unit. +/// expression language and parameters; the variable is a plain number unless `variable_unit` +/// says otherwise, so `sin(t)` reads it in radians, and a result without a unit is in the +/// design's length unit. #[derive(Clone, Debug, PartialEq, Serialize, Deserialize)] pub struct EquationCurve { /// The fit point spline it places (a curve id of the sketch). @@ -31,6 +58,9 @@ pub struct EquationCurve { /// How far (mm) the spline may stray from the formulas: the number of points was chosen for /// it when the curve was made or last edited. pub tolerance: f64, + /// What the variable is in the formulas. Left out of the file for a plain number. + #[serde(default, skip_serializing_if = "VariableUnit::is_number")] + pub variable_unit: VariableUnit, } impl EquationCurve { diff --git a/crates/sketch/src/lib.rs b/crates/sketch/src/lib.rs index 6621a6263f..068f85c860 100644 --- a/crates/sketch/src/lib.rs +++ b/crates/sketch/src/lib.rs @@ -52,7 +52,7 @@ pub use dims::{ DimFrame, DimLayout, chain_centres, default_text, default_text_offset, dim_frame, dim_layout, dim_layout_reaching, dim_reach, encode_text, }; pub use ends::{END_TOL, EndReport, EndTouch, UntiedEnd, loose_ends}; -pub use equation::{EquationCurve, MAX_EQUATION_TEXT}; +pub use equation::{EquationCurve, MAX_EQUATION_TEXT, VariableUnit}; pub use fonts::{DEFAULT_FONT_FAMILY, FontData, families_in, find_style_in, font_data, font_data_styled, installed_families, set_font_file_check}; pub use freeform_offset::{FreeformOffset, FreeformOffsetOutput, OFFSET_TOLERANCE}; pub use link::{Link, LinkGeom, LinkKind, LinkSource, MAX_LINK_ENTITIES, SKETCH_AXIS_HALF, TextExprs, sketch_axis_geom}; diff --git a/crates/ui-egui/src/dialogs_sketch_tests.rs b/crates/ui-egui/src/dialogs_sketch_tests.rs index 84d82b3023..fb9ad5766f 100644 --- a/crates/ui-egui/src/dialogs_sketch_tests.rs +++ b/crates/ui-egui/src/dialogs_sketch_tests.rs @@ -127,6 +127,51 @@ fn equation_curve_window_draws_and_reopens_on_its_formulas() { assert_eq!(sk.equation_of(&eq.curve).map(|e| e.end.as_str()), Some("20")); } +/// The window reaches the variable's unit (#9333): its checkbox draws the issue's curve, the +/// window reopens with it ticked, and unticking it reaches the curve too. +#[test] +fn equation_curve_window_gives_the_variable_its_ranges_unit() { + use solidcraft_engine::sketch::VariableUnit; + let mut h = fixture(); + h.app.session.execute("parameters.add", &json!({"name": "a", "expression": "10 mm"})).unwrap(); + h.app.start("sketch.equation_curve"); + h.settle(); + let Some(Kind::SketchModify(Mod::Equation { x, y, start, end, range_unit, .. })) = h.app.dialog.as_mut().map(|d| &mut d.kind) else { + panic!("no equation window") + }; + assert!(!*range_unit, "off to begin with"); + *x = "t".into(); + *y = "a ^ 2 / (4 * t)".into(); + *start = "a".into(); + *end = "3 * a".into(); + h.settle(); + let handle = format!("dialog:{}", crate::i18n::tr("ui.equation_variable_unit")); + h.step(&json!({"click": {"handle": handle}})).unwrap(); + h.settle(); + let Some(Kind::SketchModify(Mod::Equation { range_unit, .. })) = h.app.dialog.as_ref().map(|d| &d.kind) else { panic!("no equation window") }; + assert!(*range_unit, "the click ticks it"); + h.step(&json!({"click":{"handle":"dialog:ok"}})).unwrap(); + h.settle(); + assert!(h.app.dialog.is_none(), "the curve is made"); + let sid = h.app.session.active_sketch.unwrap(); + let eq = h.app.session.doc.sketch(sid).unwrap().equations.first().cloned().expect("an equation curve"); + assert_eq!(eq.variable_unit, VariableUnit::Range); + assert!(crate::sketch_modify_ui::edit_equation(&mut h.app, &eq.curve)); + h.settle(); + let Some(Kind::SketchModify(Mod::Equation { y, start, end, range_unit, .. })) = h.app.dialog.as_mut().map(|d| &mut d.kind) else { + panic!("no equation window") + }; + assert!(*range_unit, "reopens ticked"); + *y = "t / 2".into(); + *start = "0".into(); + *end = "10".into(); + *range_unit = false; + h.settle(); + h.step(&json!({"click":{"handle":"dialog:ok"}})).unwrap(); + h.settle(); + assert_eq!(h.app.session.doc.sketch(sid).unwrap().equation_of(&eq.curve).map(|e| e.variable_unit), Some(VariableUnit::Number)); +} + #[test] fn create_sketch_uses_card_and_still_starts_immediately_on_plane_pick() { let mut h = Harness::new(); diff --git a/crates/ui-egui/src/sketch_modify_ui.rs b/crates/ui-egui/src/sketch_modify_ui.rs index aa0c0a93fa..c773dc64ee 100644 --- a/crates/ui-egui/src/sketch_modify_ui.rs +++ b/crates/ui-egui/src/sketch_modify_ui.rs @@ -32,7 +32,7 @@ use solidcraft_engine::Sel; use solidcraft_engine::doc::Document; use solidcraft_engine::doc::expr::Kind as ValueKind; use solidcraft_engine::geom::{Vec2, Vec3}; -use solidcraft_engine::sketch::PatternAlignment; +use solidcraft_engine::sketch::{PatternAlignment, VariableUnit}; use crate::SolveApp; use crate::dialogs::{CHAMFER_TYPES, Dialog, Kind}; @@ -142,7 +142,7 @@ pub enum Mod { /// `sketch.equation_curve` (or `.edit` on the curve the window's `curve` extra names): the /// form (an index into [`EQUATION_FORMS`]), x(t) and y(t) (or y(x)), the variable's range and /// how far the curve may stray from the formulas. - Equation { form: usize, x: String, y: String, start: String, end: String, tolerance: String }, + Equation { form: usize, x: String, y: String, start: String, end: String, tolerance: String, range_unit: bool }, } impl Mod { @@ -271,6 +271,7 @@ pub fn start(app: &SolveApp, id: &str) -> Option<(Kind, Vec)> { start: "0".into(), end: "2 * pi".into(), tolerance: "0.01 mm".into(), + range_unit: false, }, _ => return None, }; @@ -291,6 +292,7 @@ pub fn edit_equation(app: &mut SolveApp, id: &str) -> bool { start: eq.start, end: eq.end, tolerance: format!("{} mm", eq.tolerance), + range_unit: eq.variable_unit == VariableUnit::Range, }; let mut dialog = Dialog::new(Kind::SketchModify(kind), Vec::new()); dialog.extra.insert("curve".into(), json!(id)); @@ -584,7 +586,7 @@ pub fn rows(ui: &mut egui::Ui, k: &mut Mod, doc: &Document) -> bool { alignment(ui, "Alignment", a, reversed); pattern_hint(ui); } - Mod::Equation { form, x, y, start, end, tolerance } => { + Mod::Equation { form, x, y, start, end, tolerance, range_unit } => { ui_kit::choice_row(ui, "Form", "equation_form", form, &EQUATION_FORMS); if *form == 0 { enter |= number(ui, "x(t)", x); @@ -595,7 +597,9 @@ pub fn rows(ui: &mut egui::Ui, k: &mut Mod, doc: &Document) -> bool { enter |= number(ui, "Start", start); enter |= number(ui, "End", end); enter |= number(ui, "Tolerance", tolerance); - ui_kit::text(ui, crate::i18n::tr("ui.equation_note"), ui_kit::Text::Note("equation")); + ui_kit::checkbox_grid(ui, &mut [(crate::i18n::tr("ui.equation_variable_unit"), range_unit)], 1); + let note = if *range_unit { "ui.equation_note_range" } else { "ui.equation_note" }; + ui_kit::text(ui, crate::i18n::tr(note), ui_kit::Text::Note("equation")); } Mod::Corner { chamfer, ctype, size, distance2, angle, flip } => { if *chamfer { @@ -1066,7 +1070,7 @@ pub fn commands(app: &SolveApp, k: &Mod, inputs: &[SelInput], extra: &Map ("sketch.chamfer.equal_distance", p), } } - Mod::Equation { form, x, y, start, end, tolerance } => { + Mod::Equation { form, x, y, start, end, tolerance, range_unit } => { let filled = |label: &str, text: &str| -> Result { let text = text.trim(); if text.is_empty() { Err(format!("{label} is empty")) } else { Ok(text.to_string()) } @@ -1081,6 +1085,7 @@ pub fn commands(app: &SolveApp, k: &Mod, inputs: &[SelInput], extra: &Map