From e575bded0ea1789c9ec7f62276672e5b24bb0c48 Mon Sep 17 00:00:00 2001 From: CypherNaugh_0x <9931495+CypherNaught-0x@users.noreply.github.com> Date: Sun, 11 Oct 2026 01:20:51 +0200 Subject: [PATCH] Fix construction-plane references in mirror, section, and sketches (#99) Accept construction planes by name or feature ID, preserve parametric mirror references, and reject invalid plane inputs. Retain both mirror-plane and primitive-point regression test modules after resolving the conflict with main. Validation: cargo xtask ci passed, including 282 engine tests, 138 UI tests, 62 UI scenarios, and WebAssembly. --- crates/engine/src/cmd/features.rs | 44 ++++++++-------- crates/engine/src/cmd/section.rs | 21 ++++++-- crates/engine/src/cmd/sketch.rs | 15 ++---- crates/engine/src/tests.rs | 1 + crates/engine/src/tests/mirror_planes.rs | 65 ++++++++++++++++++++++++ 5 files changed, 109 insertions(+), 37 deletions(-) create mode 100644 crates/engine/src/tests/mirror_planes.rs diff --git a/crates/engine/src/cmd/features.rs b/crates/engine/src/cmd/features.rs index 985d5db2..263e16b1 100644 --- a/crates/engine/src/cmd/features.rs +++ b/crates/engine/src/cmd/features.rs @@ -46,7 +46,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 | {origin, x_dir, y_dir}"), + 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.hole", "Hole", hole) .at("SOLID", "CREATE") .icon("hole") @@ -869,35 +869,35 @@ fn mirror(s: &mut Session, p: &Value) -> Result { let cmd = "solid.mirror"; let bodies = string_list(p, "bodies"); let features = if bodies.is_empty() { source_features(s, p, cmd)? } else { Vec::new() }; - let plane = match p.get("plane") { - Some(Value::String(n)) if solvecraft_geom::Plane::named(n).is_some() => solvecraft_doc::PlaneRef::Origin { name: n.to_ascii_uppercase() }, - Some(o @ Value::Object(_)) => { - let origin = o.get("origin").and_then(vec3).unwrap_or(Vec3::ZERO); - 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), _) => solvecraft_geom::Plane::new(origin, x, y), - (_, _, Some(n)) => solvecraft_geom::Plane::from_normal(origin, n), - _ => None, - }; - solvecraft_doc::PlaneRef::Custom { plane: pl.ok_or_else(|| bad(cmd, "plane needs x_dir and y_dir, or normal"))? } - } - _ => return Err(bad(cmd, "`plane` must be XY, XZ, YZ or {origin, normal}")), - }; + let plane = plane_param(s, p.get("plane"), cmd)?; let combine = bool_(p, "combine").unwrap_or(false); add_feature(s, p, FeatureKind::Mirror { features, plane, bodies, combine }) } -/// A plane reference from a parameter: an origin plane, a construction plane name, or an -/// explicit plane `{origin, x_dir, y_dir}` / `{origin, normal}`. +/// A plane reference from a parameter: an origin plane, a construction plane (by name or +/// feature id; kept by name, so the feature follows the plane when it changes), or an +/// explicit plane `{origin, x_dir, y_dir}` / `{origin, normal}`. Every command that takes a +/// plane reads it here. pub(super) fn plane_param(s: &Session, v: Option<&Value>, cmd: &str) -> Result { use solvecraft_doc::PlaneRef; + const WANT: &str = "XY, XZ, YZ, a construction plane name or id, or {origin, normal}"; + 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))), + None => Err(bad(cmd, format!("no plane `{r}` (a plane is {WANT})"))), + }; match v { - Some(Value::String(n)) if solvecraft_geom::Plane::named(n).is_some() => Ok(PlaneRef::Origin { name: n.to_ascii_uppercase() }), - Some(Value::String(n)) => match s.doc.find_feature(n).map(|f| &f.kind) { - Some(FeatureKind::ConstructionPlane { .. }) => Ok(PlaneRef::Construction { name: n.clone() }), - _ => Err(bad(cmd, format!("no plane `{n}`"))), + Some(Value::String(n)) if solvecraft_geom::Plane::named(n.trim()).is_some() => Ok(PlaneRef::Origin { name: n.trim().to_ascii_uppercase() }), + Some(Value::String(n)) => construction(n.trim()), + Some(Value::Number(id)) => match id.as_u64() { + Some(id) => construction(&id.to_string()), + None => Err(bad(cmd, format!("`plane` must be {WANT}"))), }, Some(o @ Value::Object(_)) => { - let origin = o.get("origin").and_then(vec3).unwrap_or(Vec3::ZERO); + 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]"))?, + }; 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), _) => solvecraft_geom::Plane::new(origin, x, y), (_, _, Some(n)) => solvecraft_geom::Plane::from_normal(origin, n), @@ -905,7 +905,7 @@ pub(super) fn plane_param(s: &Session, v: Option<&Value>, cmd: &str) -> Result Err(bad(cmd, "`plane` must be XY, XZ, YZ, a construction plane name or {origin, normal}")), + _ => Err(bad(cmd, format!("`plane` must be {WANT}"))), } } diff --git a/crates/engine/src/cmd/section.rs b/crates/engine/src/cmd/section.rs index 4486817a..48457ab1 100644 --- a/crates/engine/src/cmd/section.rs +++ b/crates/engine/src/cmd/section.rs @@ -10,7 +10,7 @@ use crate::{Result, Session}; pub static COMMANDS: &[CommandSpec] = &[CommandSpec::new("inspect.section", "Section Analysis", section).at("SOLID", "INSPECT").icon("section").noundo().params( - "plane: XY|XZ|YZ | {origin, normal}; offset?: expr along the plane's normal (XY: +Z, XZ: +Y, YZ: +X); \ + "plane: XY|XZ|YZ | construction plane name or id | {origin, normal}; offset?: expr along the plane's normal (XY: +Z, XZ: +Y, YZ: +X); \ at?: expr, the cut's world coordinate on the named plane's axis (XY: Z, XZ: Y, YZ: X), instead of offset; \ flip?: bool; or clear: true. Returns the cut's origin and normal, and for a named plane its axis and at", )]; @@ -24,18 +24,31 @@ fn section(s: &mut Session, p: &Value) -> Result { } // A named plane's world axis: which component of a point lies along its normal. let (origin, normal, axis) = match p.get("plane") { - Some(Value::String(n)) => { - let pl = Plane::named(n).ok_or_else(|| bad(cmd, format!("unknown plane `{n}` (XY, XZ, YZ)")))?; + Some(Value::String(n)) if Plane::named(n.trim()).is_some() => { + let pl = Plane::named(n.trim()).ok_or_else(|| bad(cmd, format!("unknown plane `{n}`")))?; let normal = pl.normal(); let axis = [normal.x, normal.y, normal.z].iter().position(|c| c.abs() > 0.5); (pl.origin, normal, axis) } + // A construction plane (name or id): where it is now. + Some(v @ (Value::String(_) | Value::Number(_))) => { + let r = super::features::plane_param(s, Some(v), cmd)?; + let solvecraft_doc::PlaneRef::Construction { name } = &r else { return Err(bad(cmd, "`plane` must be a plane")) }; + let st = s.model.state(); + let pl = st + .construct + .iter() + .find(|c| &c.name == name) + .and_then(|c| if let solvecraft_doc::construct::ConstructGeom::Plane(pl) = &c.geom { Some(*pl) } else { None }) + .ok_or_else(|| bad(cmd, format!("construction plane `{name}` is not built (rolled back, suppressed or failed)")))?; + (pl.origin, pl.normal(), None) + } Some(o @ Value::Object(_)) => { let origin = o.get("origin").and_then(vec3).unwrap_or(Vec3::ZERO); let n = o.get("normal").and_then(vec3).and_then(|n| n.normalized()).ok_or_else(|| bad(cmd, "the plane needs a non-zero `normal`"))?; (origin, n, None) } - _ => return Err(bad(cmd, "`plane` must be XY, XZ, YZ or {origin, normal}")), + _ => return Err(bad(cmd, "`plane` must be XY, XZ, YZ, a construction plane name or id, or {origin, normal}")), }; let length = |key: &str| -> Result> { match expr(p, key) { diff --git a/crates/engine/src/cmd/sketch.rs b/crates/engine/src/cmd/sketch.rs index 6b34a4fd..3a390268 100644 --- a/crates/engine/src/cmd/sketch.rs +++ b/crates/engine/src/cmd/sketch.rs @@ -20,7 +20,7 @@ pub static COMMANDS: &[CommandSpec] = &[ CommandSpec::new("sketch.create", "Create Sketch", create_sketch) .at("SOLID", "CREATE") .icon("sketch") - .params("plane: XY|XZ|YZ | {origin, x_dir, y_dir} | {face: [x,y,z]} (planar face at point), offset?: expr, name?"), + .params("plane: XY|XZ|YZ | construction plane name or id | {origin, x_dir, y_dir} | {face: [x,y,z]} (planar face at point), offset?: expr, name?"), CommandSpec::new("sketch.edit", "Edit Sketch", edit_sketch).icon("sketch").params("sketch: id or name"), CommandSpec::new("sketch.finish", "Finish Sketch", finish_sketch).at("SKETCH", "FINISH SKETCH").icon("finish").enabled(in_sketch).noundo(), CommandSpec::new("sketch.line", "Line", draw_line) @@ -348,15 +348,8 @@ pub(super) fn result(sk_out: (Vec, Vec), info: Value) -> Value { pub(super) fn plane_ref(s: &Session, p: &Value, cmd: &str) -> Result { let base = match p.get("plane") { None => PlaneRef::Origin { name: "XY".into() }, - Some(Value::String(n)) => { - if Plane::named(n).is_some() { - PlaneRef::Origin { name: n.to_ascii_uppercase() } - } else if matches!(s.doc.find_feature(n).map(|f| &f.kind), Some(FeatureKind::ConstructionPlane { .. })) { - PlaneRef::Construction { name: n.clone() } - } else { - return Err(bad(cmd, format!("unknown plane `{n}` (XY, XZ, YZ or a construction plane)"))); - } - } + // An origin plane, or a construction plane by name or id. + Some(v @ (Value::String(_) | Value::Number(_))) => super::features::plane_param(s, Some(v), cmd)?, Some(v @ Value::Object(o)) => { if let Some(fp) = o.get("face").and_then(vec3) { // A planar face of a body at this point, as the active component sees the model @@ -388,7 +381,7 @@ pub(super) fn plane_ref(s: &Session, p: &Value, cmd: &str) -> Result { PlaneRef::Custom { plane: Plane::new(origin, x, y).ok_or_else(|| bad(cmd, "degenerate plane"))? } } } - _ => return Err(bad(cmd, "`plane` must be XY, XZ, YZ or an object")), + _ => return Err(bad(cmd, "`plane` must be XY, XZ, YZ, a construction plane name or id, or an object")), }; Ok(match expr(p, "offset") { Some(d) => PlaneRef::Offset { base: Box::new(base), distance: d }, diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 944d6fbf..37c18963 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -36,6 +36,7 @@ fn join_extrude_from_a_chamfered_top_face() { } mod component_frames; +mod mirror_planes; mod primitive_points; mod robustness; diff --git a/crates/engine/src/tests/mirror_planes.rs b/crates/engine/src/tests/mirror_planes.rs new file mode 100644 index 00000000..a1b4ca98 --- /dev/null +++ b/crates/engine/src/tests/mirror_planes.rs @@ -0,0 +1,65 @@ +//! Commands that take a plane accept construction planes by name or id, like the origin planes +//! (user report: `solid.mirror` refused a construction plane although its docs list plane +//! names). + +use serde_json::json; + +use super::run; +use crate::*; + +fn min_x(s: &Session, body: &str) -> f64 { + let b = s.model.state().body(body).map(|b| b.body.clone()).unwrap(); + solvecraft_kernel::measure(&b).unwrap().bbox.min.x +} + +#[test] +fn mirror_across_a_construction_plane_follows_it() { + let mut s = Session::default(); + run(&mut s, "parameters.change", json!({"name": "w", "expression": "100 mm"})); + let pl = run(&mut s, "construct.plane.offset", json!({"base": "YZ", "offset": "w / 2", "name": "Mid"})); + run(&mut s, "solid.box", json!({"length": 10, "width": 10, "height": 10, "body_name": "Ledger"})); + let before = s.model.state().bodies.len(); + // By name: the copy lands at x = 90…100. + run(&mut s, "solid.mirror", json!({"bodies": ["Ledger"], "plane": "Mid"})); + let st = s.model.state(); + assert_eq!(st.bodies.len(), before + 1); + let copy = st.bodies.last().map(|b| b.name.clone()).unwrap(); + assert!((min_x(&s, ©) - 90.0).abs() < 1e-6, "{}", min_x(&s, ©)); + // Parametric: moving the plane moves the copy. + run(&mut s, "parameters.change", json!({"name": "w", "expression": "60 mm"})); + assert!((min_x(&s, ©) - 50.0).abs() < 1e-6, "{}", min_x(&s, ©)); + // By id, and case-insensitively by name. + let id = pl["feature"].as_u64().unwrap(); + run(&mut s, "solid.mirror", json!({"bodies": ["Ledger"], "plane": id.to_string()})); + run(&mut s, "solid.mirror", json!({"bodies": ["Ledger"], "plane": "mid"})); + let st = s.model.state(); + assert_eq!(st.bodies.len(), before + 3); + for b in st.bodies.iter().skip(before) { + assert!((min_x(&s, &b.name) - 50.0).abs() < 1e-6, "{}", b.name); + } +} + +#[test] +fn unknown_mirror_planes_are_refused_with_the_choices() { + let mut s = Session::default(); + run(&mut s, "solid.box", json!({"length": 10, "width": 10, "height": 10, "body_name": "B"})); + let e = s.execute("solid.mirror", &json!({"bodies": ["B"], "plane": "Nope"})).unwrap_err().to_string(); + assert!(e.contains("Nope"), "{e}"); + let e = s.execute("solid.mirror", &json!({"bodies": ["B"], "plane": 5})).unwrap_err().to_string(); + assert!(e.contains("construction plane"), "{e}"); + // A plane object with an unreadable origin is refused, not put at the world origin. + assert!(s.execute("solid.mirror", &json!({"bodies": ["B"], "plane": {"origin": ["x", 0, 0], "normal": [1, 0, 0]}})).is_err()); + // A feature that is not a plane is no plane. + assert!(s.execute("solid.mirror", &json!({"bodies": ["B"], "plane": "Box1"})).is_err()); + assert_eq!(s.model.state().bodies.len(), 1); +} + +/// Section Analysis cuts along a construction plane too. +#[test] +fn section_by_a_construction_plane() { + let mut s = Session::default(); + run(&mut s, "construct.plane.offset", json!({"base": "XY", "offset": 7, "name": "Cut"})); + let r = run(&mut s, "inspect.section", json!({"plane": "Cut"})); + assert_eq!(r["section"]["origin"][2].as_f64(), Some(7.0), "{r}"); + assert!(s.execute("inspect.section", &json!({"plane": "Nope"})).is_err()); +}