diff --git a/crates/doc/src/assembly.rs b/crates/doc/src/assembly.rs index aeda6e26..20172b34 100644 --- a/crates/doc/src/assembly.rs +++ b/crates/doc/src/assembly.rs @@ -136,6 +136,11 @@ fn map_plane(m: &Mat, p: &Plane) -> Plane { fn map_plane_ref(m: &Mat, p: &mut PlaneRef) { match p { PlaneRef::Custom { plane } => *plane = map_plane(m, plane), + PlaneRef::Placed { origin, x_dir, y_dir } => { + map_point_expr(m, origin); + *x_dir = apply_vector(m, *x_dir); + *y_dir = apply_vector(m, *y_dir); + } PlaneRef::Face { plane, at, .. } => { *plane = map_plane(m, plane); *at = apply_point(m, *at); diff --git a/crates/doc/src/clipboard.rs b/crates/doc/src/clipboard.rs index cc449d70..13fa9829 100644 --- a/crates/doc/src/clipboard.rs +++ b/crates/doc/src/clipboard.rs @@ -223,7 +223,7 @@ impl crate::Feature { for (_, e, _) in self.kind.inputs_mut() { *e = clone_expression(e, parameters); } - for e in self.kind.point_expr_mut().into_iter().flatten() { + for e in self.kind.placement_exprs_mut() { *e = clone_expression(e, parameters); } self.kind.remap_bodies(bodies); diff --git a/crates/doc/src/document.rs b/crates/doc/src/document.rs index 2f13f4e3..233c0a2e 100644 --- a/crates/doc/src/document.rs +++ b/crates/doc/src/document.rs @@ -42,6 +42,14 @@ pub enum PlaneRef { Origin { name: String }, /// An explicit plane (e.g. picked from a planar face). Custom { plane: Plane }, + /// An explicit plane whose origin is given as length expressions + /// (`{origin: ["half", 0, 0], normal: [1, 0, 0]}`), so it follows parameter changes. + Placed { + #[serde(with = "point_expr")] + origin: PointExpr, + x_dir: Vec3, + y_dir: Vec3, + }, /// Another plane offset along its normal by an expression. Offset { base: Box, distance: String }, /// Another plane rotated about a line (in world coordinates) by an angle expression. @@ -2938,6 +2946,15 @@ impl Document { PlaneRef::Custom { plane } | PlaneRef::Face { plane, .. } => { Plane::new(plane.origin, plane.x, plane.y).ok_or_else(|| DocError::Invalid("degenerate plane".into())) } + PlaneRef::Placed { origin, x_dir, y_dir } => { + let [x, y, z] = origin; + let o = + Vec3::new(Self::eval_in(vals, x, Kind::Length)?, Self::eval_in(vals, y, Kind::Length)?, Self::eval_in(vals, z, Kind::Length)?); + if ![o.x, o.y, o.z].iter().all(|c| c.is_finite() && c.abs() < 1e9) { + return Err(DocError::Invalid("the plane's origin is out of range".into())); + } + Plane::new(o, *x_dir, *y_dir).ok_or_else(|| DocError::Invalid("degenerate plane".into())) + } PlaneRef::Offset { base, distance } => { let b = self.resolve_plane_in(vals, base, depth + 1, st)?; Ok(b.offset(Self::eval_in(vals, distance, Kind::Length)?)) diff --git a/crates/doc/src/format/pending/placed-plane-origins.rs b/crates/doc/src/format/pending/placed-plane-origins.rs new file mode 100644 index 00000000..91a18d62 --- /dev/null +++ b/crates/doc/src/format/pending/placed-plane-origins.rs @@ -0,0 +1,4 @@ +//! An explicit plane (a mirror's, or any plane reference) can be `placed`: an `origin` of three +//! length expressions with `x_dir` and `y_dir`, so it follows the design's parameters (#1492). A +//! plane whose origin is plain numbers is still saved as `custom`. Older designs need no +//! transformation; 0.13.x asks for an update instead of failing to read the plane. diff --git a/crates/doc/src/params.rs b/crates/doc/src/params.rs index c9753be4..f956bcf1 100644 --- a/crates/doc/src/params.rs +++ b/crates/doc/src/params.rs @@ -44,6 +44,30 @@ impl FeatureKind { v } + /// The coordinates of the points that place the feature: a primitive's position, an + /// explicit plane's origin. They are not inputs (no parameter names), but follow + /// parameters as inputs do. + pub fn placement_exprs_mut(&mut self) -> Vec<&mut String> { + let mut v = Vec::new(); + self.walk_exprs_mut(&mut |(_, e, _, role)| { + if role == ExprRole::Placement { + v.push(e); + } + }); + v + } + + /// Read-only view of [`FeatureKind::placement_exprs_mut`]. + pub fn placement_exprs(&self) -> Vec<&str> { + let mut v = Vec::new(); + self.walk_exprs(&mut |(_, e, _, role)| { + if role == ExprRole::Placement { + v.push(e.as_str()); + } + }); + v + } + /// Every expression the feature uses: its inputs, then a primitive's placement coordinates. pub fn expressions(&self) -> Vec<&str> { let (mut inputs, mut placement) = (Vec::new(), Vec::new()); @@ -575,7 +599,7 @@ impl Document { } } } - if f.kind.point_expr().is_some_and(|p| p.iter().any(|e| expr::references(e).iter().any(|r| r == name))) { + if f.kind.placement_exprs().iter().any(|e| expr::references(e).iter().any(|r| r == name)) { out.push(ParamUser::Feature { feature: f.id, name: f.name.clone(), input: "Position".into() }); } if let FeatureKind::Sketch { sketch, .. } = &f.kind { @@ -646,7 +670,7 @@ impl Document { *x = new.to_string(); } } - for e in f.kind.point_expr_mut().into_iter().flatten() { + for e in f.kind.placement_exprs_mut() { let r = expr::rename_reference(e, old, new); if r != *e { *e = r; diff --git a/crates/doc/src/walk.rs b/crates/doc/src/walk.rs index dc32c177..534af73c 100644 --- a/crates/doc/src/walk.rs +++ b/crates/doc/src/walk.rs @@ -264,8 +264,9 @@ impl &'static str> Label for F { } } -/// The offsets and angles of a plane, outermost first (nesting is capped like every walk over a -/// plane), the construction plane it is built on, and the plane itself. +/// The offsets and angles of a plane, outermost first, and an explicit origin's coordinates +/// (nesting is capped like every walk over a plane), the construction plane it is built on, and +/// the plane itself. impl Walk for PlaneRef { fn walk_exprs<'a>(&'a self, f: &mut dyn FnMut(ExprItem<'a>)) { let mut p = self; @@ -280,6 +281,12 @@ impl Walk for PlaneRef { p = base; } PlaneRef::AlongPath { t, .. } => return f(("Distance (0 to 1)", t, Kind::Unitless, ExprRole::Input)), + PlaneRef::Placed { origin, .. } => { + for (label, e) in ["X", "Y", "Z"].into_iter().zip(origin) { + f((label, e, Kind::Length, ExprRole::Placement)); + } + return; + } _ => return, } } @@ -297,6 +304,12 @@ impl Walk for PlaneRef { p = base; } PlaneRef::AlongPath { t, .. } => return f(("Distance (0 to 1)", t, Kind::Unitless, ExprRole::Input)), + PlaneRef::Placed { origin, .. } => { + for (label, e) in ["X", "Y", "Z"].into_iter().zip(origin) { + f((label, e, Kind::Length, ExprRole::Placement)); + } + return; + } _ => return, } } diff --git a/crates/engine/src/cmd/features.rs b/crates/engine/src/cmd/features.rs index ce17bea5..6adb6aeb 100644 --- a/crates/engine/src/cmd/features.rs +++ b/crates/engine/src/cmd/features.rs @@ -48,7 +48,7 @@ pub static COMMANDS: &[CommandSpec] = &[ .at("SOLID", "CREATE") .icon("pattern_rect") .params("features: [names] | bodies: [names]; path_sketch: sketch; path: [curve ids in order]; count; spacing (between instances) | distance (first to last); orientation?: identical|path; flip?: bool"), - CommandSpec::new("solid.mirror", "Mirror", mirror).at("SOLID", "CREATE").icon("mirror").params("features: [names] | bodies: [names] (combine?: join with the original); plane: XY|XZ|YZ|construction plane name or id|{origin, normal}|{origin, x_dir, y_dir} (a construction plane keeps the mirror following it)"), + CommandSpec::new("solid.mirror", "Mirror", mirror).at("SOLID", "CREATE").icon("mirror").params("features: [names] | bodies: [names] (combine?: join with the original); plane: XY|XZ|YZ|construction plane name or id|{origin, normal}|{origin, x_dir, y_dir} (origin coordinates may be length expressions; the mirror follows them, or a construction plane)"), CommandSpec::new("solid.hole", "Hole", hole) .at("SOLID", "CREATE") .icon("hole") @@ -1303,7 +1303,7 @@ pub(super) fn check_plane_name(name: &str, cmd: &str) -> Result<()> { /// plane reads it here. pub(super) fn plane_param(s: &Session, v: Option<&Value>, cmd: &str) -> Result { use solidcraft_doc::PlaneRef; - const WANT: &str = "XY, XZ, YZ, a construction plane name or id, or {origin, normal}"; + const WANT: &str = "XY, XZ, YZ, a construction plane name or id, or {origin, normal} (origin coordinates may be length expressions)"; let construction = |r: &str| match s.doc.find_feature(r) { Some(f) if matches!(f.kind, FeatureKind::ConstructionPlane { .. }) => Ok(PlaneRef::Construction { name: f.name.clone() }), Some(f) => Err(bad(cmd, format!("`{}` is not a construction plane (a plane is {WANT})", f.name))), @@ -1332,16 +1332,24 @@ pub(super) fn plane_param(s: &Session, v: Option<&Value>, cmd: &str) -> Result Err(bad(cmd, format!("`plane` must be {WANT}"))), }, Some(o @ Value::Object(_)) => { + // The origin's coordinates are numbers or length expressions; expressions are kept, + // so the plane follows its parameters. let origin = match o.get("origin") { - None => Vec3::ZERO, - Some(x) => vec3(x).ok_or_else(|| bad(cmd, "the plane's `origin` must be a point [x, y, z]"))?, + None => point_expr(Vec3::ZERO), + Some(_) => point_param(s, o, "origin", cmd)?.unwrap_or_else(|| point_expr(Vec3::ZERO)), }; + let at = |e: &String| s.doc.eval(e, Kind::Length).unwrap_or(0.0); + let here = Vec3::new(at(&origin[0]), at(&origin[1]), at(&origin[2])); let pl = match (o.get("x_dir").and_then(vec3), o.get("y_dir").and_then(vec3), o.get("normal").and_then(vec3)) { - (Some(x), Some(y), _) => solidcraft_geom::Plane::new(origin, x, y), - (_, _, Some(n)) => solidcraft_geom::Plane::from_normal(origin, n), + (Some(x), Some(y), _) => solidcraft_geom::Plane::new(here, x, y), + (_, _, Some(n)) => solidcraft_geom::Plane::from_normal(here, n), _ => None, }; - Ok(PlaneRef::Custom { plane: pl.ok_or_else(|| bad(cmd, "plane needs x_dir and y_dir, or normal"))? }) + let plane = pl.ok_or_else(|| bad(cmd, "plane needs x_dir and y_dir, or normal"))?; + if origin.iter().all(|e| solidcraft_doc::expr::is_literal(e)) { + return Ok(PlaneRef::Custom { plane }); + } + Ok(PlaneRef::Placed { origin, x_dir: plane.x, y_dir: plane.y }) } _ => Err(bad(cmd, format!("`plane` must be {WANT}"))), } diff --git a/crates/engine/src/tests/mirror_planes.rs b/crates/engine/src/tests/mirror_planes.rs index af766e59..03e6dc9c 100644 --- a/crates/engine/src/tests/mirror_planes.rs +++ b/crates/engine/src/tests/mirror_planes.rs @@ -39,6 +39,41 @@ fn mirror_across_a_construction_plane_follows_it() { } } +/// A plane `{origin, normal}` takes length expressions in its origin, as a primitive's corner +/// does, and the mirror follows them through parameter changes, renames and a save (#1492). +#[test] +fn mirror_plane_origin_follows_its_parameters() { + let mut s = Session::default(); + run(&mut s, "parameters.add", json!({"name": "half", "expression": "50 mm"})); + run(&mut s, "solid.box", json!({"length": 20, "width": 20, "height": 20})); + run(&mut s, "solid.mirror", json!({"features": ["Box1"], "plane": {"origin": ["half", 0, 0], "normal": [1, 0, 0]}, "name": "Twin"})); + let at = |s: &Session| min_x(s, &last_body(s)); + // The box spans x = 0…20; mirrored about x = 50 it spans 80…100. + assert!((at(&s) - 80.0).abs() < 1e-6, "{}", at(&s)); + run(&mut s, "parameters.change", json!({"name": "half", "expression": "30 mm"})); + assert!((at(&s) - 40.0).abs() < 1e-6, "{}", at(&s)); + run(&mut s, "parameters.rename", json!({"name": "half", "new_name": "mid"})); + run(&mut s, "parameters.change", json!({"name": "mid", "expression": "25 mm"})); + assert!((at(&s) - 30.0).abs() < 1e-6, "{}", at(&s)); + // Saved and opened again, the plane still names the parameter. + let mut s2 = Session::new(solidcraft_doc::Document::from_json(&s.doc.to_json()).unwrap()); + assert!(s2.doc.to_json().contains("\"mid\"")); + run(&mut s2, "parameters.change", json!({"name": "mid", "expression": "60 mm"})); + assert!((at(&s2) - 100.0).abs() < 1e-6, "{}", at(&s2)); + // A parameter the plane uses can't be deleted from under it. + assert!(s2.execute("parameters.delete", &json!({"name": "mid"})).is_err()); + // Plain numbers still make a fixed plane. + let r = run(&mut s2, "solid.mirror", json!({"features": ["Box1"], "plane": {"origin": [-10, 0, 0], "normal": [1, 0, 0]}})); + let id = r["feature"].as_u64().unwrap(); + let f = s2.doc.feature(id).unwrap(); + assert!(matches!(f.kind, solidcraft_doc::FeatureKind::Mirror { plane: solidcraft_doc::PlaneRef::Custom { .. }, .. }), "{:?}", f.kind); + assert!((at(&s2) + 40.0).abs() < 1e-6, "{}", at(&s2)); +} + +fn last_body(s: &Session) -> String { + s.model.state().bodies.last().map(|b| b.name.clone()).unwrap() +} + #[test] fn unknown_mirror_planes_are_refused_with_the_choices() { let mut s = Session::default(); diff --git a/skills/solidcraft-modeling/SKILL.md b/skills/solidcraft-modeling/SKILL.md index 6e2ca4ce..be74e91b 100644 --- a/skills/solidcraft-modeling/SKILL.md +++ b/skills/solidcraft-modeling/SKILL.md @@ -36,7 +36,7 @@ Work the part out as numbers first. Then build it in a few large `batch` calls, - expression fields such as `solid.cylinder base:["w/2-6","d/2-6","wall"]` A plain number never follows a parameter. -- **Repeats:** `solid.pattern.rectangular` takes expression spacing; `solid.mirror` takes `XY`/`XZ`/`YZ`, a construction plane or `{origin, normal}`. A mirror or pattern feature cannot be mirrored or patterned again. +- **Repeats:** `solid.pattern.rectangular` takes expression spacing; `solid.mirror` takes `XY`/`XZ`/`YZ`, a construction plane or `{origin, normal}` (origin coordinates may be length expressions such as `["half", 0, 0]`). A mirror or pattern feature cannot be mirrored or patterned again. - **Edge and face picks** snap to the nearest edge or face. There is no distance limit and no error, so compute the exact midpoint from your geometry (the vertical corner edge of a centred box is `[w/2, d/2, h/2]`). - Picks must be plain numbers. - A pick is resolved once and then follows its edge when parameters change. diff --git a/skills/solidcraft-modeling/reference.md b/skills/solidcraft-modeling/reference.md index 4216d56c..7cab578e 100644 --- a/skills/solidcraft-modeling/reference.md +++ b/skills/solidcraft-modeling/reference.md @@ -78,7 +78,7 @@ Distances are unsigned. Draw the geometry on the correct side first, and the dim | `solid.hole` | `position:[x,y,z]` on a face + `diameter`, or `sketch` (+ `points`) | `clearance:"M3"`. `depth` defaults to through all. `type` is one of simple/drilled/counterbore/countersink. A `countersink` needs `cs_diameter` and `cs_angle`; a `counterbore` needs `cb_diameter` and `cb_depth`. `position` is a fixed point. | | `solid.pattern.rectangular` | `features`, `dir1`, `count1`, `spacing1` (+ `dir2`, `count2`, `spacing2`) | Spacing takes expressions and follows parameters. | | `solid.pattern.circular` | `features`, `axis` (`"Z"` or `{origin, dir}`), `count` | `angle` defaults to 360 deg. A full pattern keeps the COM on the axis, so check its radius with a top-view screenshot. | -| `solid.mirror` | `features:[names]`, `plane` (`XY`/`XZ`/`YZ`, a construction plane, or `{origin, normal}`) | Follows parameters. Mirror and pattern features cannot be mirrored or patterned again. | +| `solid.mirror` | `features:[names]`, `plane` (`XY`/`XZ`/`YZ`, a construction plane, or `{origin, normal}` whose origin may use length expressions) | Follows parameters. Mirror and pattern features cannot be mirrored or patterned again. | - **Names:** name every feature (`"name":"Standoff"`) and keep names unique across sketches and features. `timeline.roll_to {"feature":"Plate"}` picks the first match, which can be the sketch. - **Counts as parameters:** `parameters.add {"name":"n","expression":"6","unit":""}` creates a unitless parameter, and `"count":"n"` uses it in a pattern.