solid.mirror: a plane's {origin} takes length expressions and follows them (#1492) (#1501)

Thanks @erkexzcx.

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);

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);

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<PlaneRef>, 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)?))

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.

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;

crates/doc/src/walk.rs

@@ -264,8 +264,9 @@ impl<F: Fn(usize, usize) -> &'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,
             }
         }

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<solidcraft_doc::PlaneRef> {
     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<s
             None => 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}"))),
     }

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();

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.

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.