From 59fabc16ac324d655faa70c5b79155e7bfe772b5 Mon Sep 17 00:00:00 2001 From: selune <35337584+0xlillith@users.noreply.github.com> Date: Mon, 12 Oct 2026 17:05:57 +0200 Subject: [PATCH] Reject ambiguous origin and construction plane names (#247) * Reject ambiguous origin and construction plane names * Reserve origin plane names for construction planes; suggest the other alias A construction plane can no longer be created with or renamed to XY, XZ, YZ, Top, Front or Right, so a plane argument can't become ambiguous in new designs. For older designs that already hold such a plane, the ambiguity error now also names the origin plane's other alias (XY for Top...). The existing tests build those legacy names directly. Co-Authored-By: Claude Opus 5.5 --------- Co-authored-by: bherbruck Co-authored-by: Claude Opus 5.5 --- ROADMAP.md | 4 + crates/engine/src/cmd/edit.rs | 3 + crates/engine/src/cmd/features.rs | 47 ++++++++++- crates/engine/src/cmd/section.rs | 2 + crates/engine/src/construct_tests.rs | 113 +++++++++++++++++++++++++++ docs/book/src/guide/sketching.md | 8 ++ 6 files changed, 176 insertions(+), 1 deletion(-) diff --git a/ROADMAP.md b/ROADMAP.md index 79231e73..e505cc5a 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -64,6 +64,10 @@ headline counts in-scope tabs only and lists the deferred ones separately. ## Dimensioning and error reporting +- Construction planes cannot take an origin-plane name (XY, XZ, YZ, Top, Front, Right). Plane + arguments reject such a name when an older design's construction plane already has it. Use + the other alias, the construction feature's numeric ID or an explicit plane; existing + stored references keep their saved meaning. Section Analysis uses the same check. - Sketch text can be edited from a selected outline's context menu or the Edit Text command. Content, baseline X/Y, height and rotation update the original text source in one undo step; cancelling or rejecting an invalid value preserves the sketch. Position and height fields diff --git a/crates/engine/src/cmd/edit.rs b/crates/engine/src/cmd/edit.rs index 74dc3379..df9bc62c 100644 --- a/crates/engine/src/cmd/edit.rs +++ b/crates/engine/src/cmd/edit.rs @@ -145,6 +145,9 @@ fn rename(s: &mut Session, p: &Value) -> Result { if s.doc.features.iter().any(|f| f.name == name && f.id != id) { return Err(bad(cmd, format!("another feature is called `{name}`"))); } + if s.doc.feature(id).is_some_and(|f| matches!(f.kind, FeatureKind::ConstructionPlane { .. })) { + super::features::check_plane_name(name, cmd)?; + } if let Some(f) = s.doc_mut().feature_mut(id) { f.name = name.to_string(); } diff --git a/crates/engine/src/cmd/features.rs b/crates/engine/src/cmd/features.rs index cec986e0..b5117f69 100644 --- a/crates/engine/src/cmd/features.rs +++ b/crates/engine/src/cmd/features.rs @@ -417,6 +417,11 @@ fn add_feature_in(s: &mut Session, p: &Value, kind: FeatureKind, world: bool) -> if participants.len() > 10_000 || participants.iter().any(|b| b != "*" && s.model.state().body(b).is_none()) { return Err(EngineError::Other("`participants` must list existing bodies".into())); } + if matches!(kind, FeatureKind::ConstructionPlane { .. }) + && let Some(n) = name + { + check_plane_name(n, "construct.plane")?; + } let id = s.doc_mut().add_feature(kind, name)?; if let Some(f) = s.doc_mut().feature_mut(id) { f.participants = participants; @@ -977,6 +982,31 @@ fn mirror(s: &mut Session, p: &Value) -> Result { add_feature(s, p, FeatureKind::Mirror { features, plane, bodies, combine }) } +/// The other name of an origin plane (`Top` for `XY`, `XY` for `Top`...), if `name` is one. +pub(super) fn origin_plane_alias(name: &str) -> Option<&'static str> { + match name.trim().to_ascii_uppercase().as_str() { + "XY" => Some("Top"), + "TOP" => Some("XY"), + "XZ" => Some("Front"), + "FRONT" => Some("XZ"), + "YZ" => Some("Right"), + "RIGHT" => Some("YZ"), + _ => None, + } +} + +/// A construction plane may not take an origin plane's name (`XY`, `Top`...): a plane +/// parameter could then mean either. +pub(super) fn check_plane_name(name: &str, cmd: &str) -> Result<()> { + if solvecraft_geom::Plane::named(name.trim()).is_some() { + return Err(bad( + cmd, + format!("`{}` is reserved for an origin plane (XY, XZ, YZ, Top, Front, Right); give the construction plane another name", name.trim()), + )); + } + Ok(()) +} + /// 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 @@ -990,7 +1020,22 @@ pub(super) fn plane_param(s: &Session, v: Option<&Value>, cmd: &str) -> Result Err(bad(cmd, format!("no plane `{r}` (a plane is {WANT})"))), }; match v { - 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)) if solvecraft_geom::Plane::named(n.trim()).is_some() => { + if let Some(f) = s.doc.find_feature(n.trim()) + && matches!(f.kind, FeatureKind::ConstructionPlane { .. }) + { + return Err(bad( + cmd, + format!( + "plane `{}` is ambiguous: an origin-plane alias and a construction plane; use `{}` for the origin plane, construction plane feature id {}, or an explicit {{origin, normal}} plane", + n.trim(), + origin_plane_alias(n).unwrap_or("XY"), + f.id + ), + )); + } + 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()), diff --git a/crates/engine/src/cmd/section.rs b/crates/engine/src/cmd/section.rs index 48457ab1..a65f7ee4 100644 --- a/crates/engine/src/cmd/section.rs +++ b/crates/engine/src/cmd/section.rs @@ -25,6 +25,8 @@ 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)) if Plane::named(n.trim()).is_some() => { + // Apply the same ambiguity check as modelling commands before treating a name as an origin alias. + super::features::plane_param(s, p.get("plane"), cmd)?; 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); diff --git a/crates/engine/src/construct_tests.rs b/crates/engine/src/construct_tests.rs index 3c1d72e0..5e15bc28 100644 --- a/crates/engine/src/construct_tests.rs +++ b/crates/engine/src/construct_tests.rs @@ -272,3 +272,116 @@ fn plane_perpendicular_to_a_plane_through_a_line() { // A line square to the plane has no single answer. assert!(s.execute("construct.plane.perpendicular", &json!({"plane": "XY", "line": {"axis": "Z"}})).is_err()); } + +#[test] +fn ambiguous_plane_aliases_refuse_without_document_or_history_changes() { + for alias in ["Top", "Front", "Right", "XY", "XZ", "YZ"] { + let mut s = Session::default(); + run(&mut s, "parameters.add", json!({"name":"height","expression":"19.05 mm"})); + run(&mut s, "sketch.create", json!({"plane":alias})); + run(&mut s, "sketch.finish", json!({})); + let made = run(&mut s, "construct.plane.offset", json!({"base":{"origin":[0,0,0],"normal":[0,0,1]},"offset":"height","name":"Lid"})); + let id = made["feature"].as_u64().unwrap(); + legacy_plane_name(&mut s, id, alias); + let before = s.doc.to_json(); + let history = s.undo.len(); + for command in ["sketch.create", "inspect.section"] { + let error = s.execute(command, &json!({"plane":format!(" {} ",alias.to_lowercase())})).unwrap_err(); + assert!(error.to_string().contains("ambiguous"), "{error}"); + assert!(error.to_string().contains(&format!("feature id {id}")), "{error}"); + let other = match alias { + "Top" => "XY", + "XY" => "Top", + "Front" => "XZ", + "XZ" => "Front", + "Right" => "YZ", + _ => "Right", + }; + assert!(error.to_string().contains(&format!("use `{other}` for the origin plane")), "{error}"); + assert_eq!(s.doc.to_json(), before); + assert_eq!(s.undo.len(), history); + assert!(s.section.is_none()); + } + run(&mut s, "sketch.create", json!({"plane":id})); + let inspected = run(&mut s, "sketch.inspect", json!({})); + assert_eq!(inspected["plane"]["origin"], json!([0.0, 0.0, 19.05])); + let cut = run(&mut s, "inspect.section", json!({"plane":id})); + assert_eq!(cut["section"]["origin"], json!([0.0, 0.0, 19.05])); + run(&mut s, "parameters.change", json!({"name":"height","expression":"25.4 mm"})); + let mut reopened = Session::new(solvecraft_doc::Document::from_json(&s.doc.to_json()).unwrap()); + let old = run(&mut reopened, "sketch.inspect", json!({"sketch":"Sketch1"})); + assert_eq!(old["plane"]["origin"], json!([0.0, 0.0, 0.0]), "saved origin reference is not reinterpreted"); + let linked = run(&mut reopened, "sketch.inspect", json!({"sketch":"Sketch2"})); + assert_eq!(linked["plane"]["origin"], json!([0.0, 0.0, 25.4]), "disambiguated reference follows edits and reopen"); + } +} + +#[test] +fn disambiguated_top_plane_pocket_survives_undo_and_cold_reopen() { + let mut s = Session::default(); + run(&mut s, "sketch.create", json!({"plane":"XY"})); + run(&mut s, "sketch.rectangle.two_point", json!({"p0":[0,0],"p1":[100,200]})); + run(&mut s, "sketch.finish", json!({})); + run(&mut s, "solid.extrude", json!({"distance":19.05,"name":"Plate"})); + let top = run(&mut s, "construct.plane.offset", json!({"base":"XY","offset":19.05,"name":"Lid"})); + legacy_plane_name(&mut s, top["feature"].as_u64().unwrap(), "Top"); + assert!(s.execute("sketch.create", &json!({"plane":"Top"})).unwrap_err().to_string().contains("ambiguous")); + run(&mut s, "sketch.create", json!({"plane":top["feature"]})); + run(&mut s, "sketch.rectangle.two_point", json!({"p0":[3,3],"p1":[97,30]})); + run(&mut s, "sketch.finish", json!({})); + let before = s.doc.to_json(); + run(&mut s, "solid.extrude", json!({"distance":12.7,"direction":"negative","operation":"cut","name":"Pocket"})); + let saved = s.doc.to_json(); + let expected = 100.0 * 200.0 * 19.05 - 94.0 * 27.0 * 12.7; + let measured = run(&mut s, "inspect.measure", json!({})); + assert!((measured["total"]["volume_mm3"].as_f64().unwrap() - expected).abs() < 1e-6, "{measured}"); + run(&mut s, "edit.undo", json!({})); + assert_eq!(s.doc.to_json(), before); + run(&mut s, "edit.redo", json!({})); + assert_eq!(s.doc.to_json(), saved); + let mut reopened = Session::new(solvecraft_doc::Document::from_json(&saved).unwrap()); + let measured = run(&mut reopened, "inspect.measure", json!({})); + assert!((measured["total"]["volume_mm3"].as_f64().unwrap() - expected).abs() < 1e-6, "{measured}"); +} + +/// A construction plane named like an origin plane, as an older saved design can hold. +fn legacy_plane_name(s: &mut Session, id: u64, name: &str) { + s.doc_mut().feature_mut(id).unwrap().name = name.to_string(); +} + +#[test] +fn construction_planes_cannot_take_origin_plane_names() { + let mut s = Session::default(); + let made = run(&mut s, "construct.plane.offset", json!({"base":"XY","offset":5,"name":"Lid"})); + let id = made["feature"].as_u64().unwrap(); + for alias in ["XY", "XZ", "YZ", "Top", "Front", "Right", " top ", "xz"] { + let before = s.doc.to_json(); + let history = s.undo.len(); + let created = s.execute("construct.plane.offset", &json!({"base":"XY","offset":10,"name":alias})).unwrap_err(); + assert!(created.to_string().contains("reserved for an origin plane"), "{created}"); + let renamed = s.execute("timeline.rename", &json!({"feature":id,"name":alias})).unwrap_err(); + assert!(renamed.to_string().contains("reserved for an origin plane"), "{renamed}"); + assert_eq!(s.doc.to_json(), before); + assert_eq!(s.undo.len(), history); + } + // Other features may still use the names; only a plane parameter could confuse them. + run(&mut s, "sketch.create", json!({"plane":"XY"})); + run(&mut s, "sketch.finish", json!({})); + run(&mut s, "timeline.rename", json!({"feature":"Sketch1","name":"Top"})); + run(&mut s, "timeline.rename", json!({"feature":id,"name":"Topside"})); +} + +#[test] +fn ambiguity_error_names_the_other_origin_alias() { + for (name, other) in [("Top", "XY"), ("XY", "Top"), ("Front", "XZ"), ("XZ", "Front"), ("Right", "YZ"), ("YZ", "Right")] { + let mut s = Session::default(); + let made = run(&mut s, "construct.plane.offset", json!({"base":"XY","offset":5,"name":"Lid"})); + legacy_plane_name(&mut s, made["feature"].as_u64().unwrap(), name); + let error = s.execute("sketch.create", &json!({"plane":name})).unwrap_err().to_string(); + assert!(error.contains(&format!("use `{other}` for the origin plane")), "{error}"); + // The suggested alias is unambiguous and reaches the origin plane. + run(&mut s, "sketch.create", json!({"plane":other})); + let inspected = run(&mut s, "sketch.inspect", json!({})); + assert_eq!(inspected["plane"]["origin"], json!([0.0, 0.0, 0.0])); + } +} diff --git a/docs/book/src/guide/sketching.md b/docs/book/src/guide/sketching.md index 068b22f7..e215b17a 100644 --- a/docs/book/src/guide/sketching.md +++ b/docs/book/src/guide/sketching.md @@ -4,6 +4,14 @@ A sketch lies on a plane: an origin plane, a construction plane or a planar face **Create Sketch** (`sketch.create`) and click a plane, or pick a face first. **Finish Sketch** returns to the model. +In scripts, `XY`, `XZ`, `YZ` and their `Top`, `Front`, `Right` aliases name origin planes. +These names are reserved: a construction plane cannot be created with or renamed to one +(case-insensitive). If an older design already has a construction plane with such a name, a +string argument naming it is ambiguous and is rejected; use the other alias (`XY` for `Top`) +for the origin plane, the construction feature's numeric ID to select it, or an explicit +`{origin: [x,y,z], normal: [x,y,z]}` plane. Existing saved references retain their meaning; +opening a design does not reinterpret old sketches. + Curves adapt to the current zoom and display scale. Picking and highlighting use the same visible boundary, including small circles at Fit and sketches in rotated components. Profile shading follows those curves while preserving holes. Zooming changes the display