From 99886594e7bffa4ac9c1934484e7ce0df846bfc8 Mon Sep 17 00:00:00 2001 From: bherbruck Date: Thu, 8 Oct 2026 10:33:25 +0000 Subject: [PATCH] Persistent naming: sketches on faces store the face's name and find it by name first (an upstream feature that adds a nearer parallel face no longer moves the sketch); a vanished face falls back to the old geometric search with a warning Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01AS5Tn3xieewUSg3uxrMVdL --- crates/doc/src/assembly.rs | 2 +- crates/doc/src/document.rs | 10 ++++++++-- crates/doc/src/eval.rs | 34 +++++++++++++++++++++++++++++++-- crates/engine/src/cmd/sketch.rs | 4 +++- crates/engine/src/tests.rs | 29 ++++++++++++++++++++++++++++ 5 files changed, 73 insertions(+), 6 deletions(-) diff --git a/crates/doc/src/assembly.rs b/crates/doc/src/assembly.rs index 9b8a77ff..878309d8 100644 --- a/crates/doc/src/assembly.rs +++ b/crates/doc/src/assembly.rs @@ -132,7 +132,7 @@ 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::Face { plane, at } => { + PlaneRef::Face { plane, at, .. } => { *plane = map_plane(m, plane); *at = apply_point(m, *at); } diff --git a/crates/doc/src/document.rs b/crates/doc/src/document.rs index ab198abe..4a42c706 100644 --- a/crates/doc/src/document.rs +++ b/crates/doc/src/document.rs @@ -45,8 +45,14 @@ pub enum PlaneRef { Construction { name: String }, /// A planar body face: found again on each evaluation (the face with this plane's normal /// nearest `at`), so sketches on it follow when earlier features change; `plane` is the - /// plane as picked (its frame, and the fallback when the face is gone). - Face { plane: Plane, at: Vec3 }, + /// plane as picked (its frame, and the fallback when the face is gone). `name` is the face's + /// persistent name (found first; see `naming`). + Face { + plane: Plane, + at: Vec3, + #[serde(default, skip_serializing_if = "Option::is_none")] + name: Option, + }, } /// Which closed profiles of a sketch a feature uses. diff --git a/crates/doc/src/eval.rs b/crates/doc/src/eval.rs index 43ad1433..2fa17333 100644 --- a/crates/doc/src/eval.rs +++ b/crates/doc/src/eval.rs @@ -1729,8 +1729,38 @@ mod more; /// The plane of the body face a sketch sits on, found again: among planar faces with the /// picked plane's normal, the one nearest `at`. The picked frame moves along its normal onto it. -fn face_plane(st: &ModelState, picked: &Plane, at: Vec3) -> (Plane, Option) { +fn face_plane(st: &ModelState, picked: &Plane, at: Vec3, name: Option<&str>) -> (Plane, Option) { let n = picked.normal(); + // By name first: the face (or a piece of it) with that name, facing the same way. + if let Some(name) = name { + let stem = crate::naming::strip_piece(name); + let mut found: Option<(f64, f64)> = None; + for b in st.bodies.iter().filter(|b| !b.body.is_mesh()) { + let names = crate::naming::face_names(b); + let tol = (b.body.size() * 1e-3).max(1e-3); + let Ok(faces) = b.body.faces(tol) else { continue }; + for f in faces { + let hit = names.get(f.index).is_some_and(|x| crate::naming::strip_piece(x) == stem); + if let (true, Some(fnrm)) = (hit, f.plane_normal) + && fnrm.dot(n) > 1.0 - 1e-6 + { + let d = f.centroid.dist(at); + if found.is_none_or(|x| d < x.0) { + found = Some((d, f.centroid.dot(n))); + } + } + } + } + if let Some((_, off)) = found { + return (picked.offset(off - picked.origin.dot(n)), None); + } + let (pl, w) = face_plane(st, picked, at, None); + let note = format!( + "the face `{name}` the sketch was on no longer exists; {}", + if w.is_some() { "the sketch stays where it was" } else { "using the face at the picked point" } + ); + return (pl, Some(note)); + } let mut best: Option<(f64, f64)> = None; // (distance from `at`, plane offset along n) for b in &st.bodies { let tol = (b.body.size() * 1e-3).max(1e-3); @@ -1781,7 +1811,7 @@ fn eval_feature(doc: &Document, vals: &BTreeMap, f: &Feature, st: | FeatureKind::Remove { .. } => more::eval(doc, vals, f, st), FeatureKind::Sketch { plane, sketch } => { let (plane, face_warning) = match plane { - PlaneRef::Face { plane: picked, at } => face_plane(st, picked, *at), + PlaneRef::Face { plane: picked, at, name } => face_plane(st, picked, *at, name.as_deref()), other => (doc.resolve_plane(vals, other, 0)?, None), }; let mut sk = sketch.clone(); diff --git a/crates/engine/src/cmd/sketch.rs b/crates/engine/src/cmd/sketch.rs index 02ca94ed..d3edf833 100644 --- a/crates/engine/src/cmd/sketch.rs +++ b/crates/engine/src/cmd/sketch.rs @@ -314,6 +314,7 @@ pub(super) fn plane_ref(s: &Session, p: &Value, cmd: &str) -> Result { // A planar face of a body at this point. let st = s.model.state(); let mut found = None; + let mut face_name = None; for b in &st.bodies { let tol = (b.body.size() * 1e-3).max(1e-3); for f in b.body.faces(tol).unwrap_or_default() { @@ -324,11 +325,12 @@ pub(super) fn plane_ref(s: &Session, p: &Value, cmd: &str) -> Result { // Sketch origin: the model origin projected onto the face plane. let d = (f.centroid + n * (fp - f.centroid).dot(n)).dot(n); found = Plane::from_normal(n * d, n); + face_name = solvecraft_doc::naming::face_names(b).get(f.index).cloned(); } } } let pl = found.ok_or_else(|| bad(cmd, "no planar face at that point"))?; - PlaneRef::Face { plane: pl, at: fp } + PlaneRef::Face { plane: pl, at: fp, name: face_name } } else { let origin = v.get("origin").and_then(vec3).unwrap_or(Vec3::ZERO); let x = v.get("x_dir").and_then(vec3).ok_or_else(|| bad(cmd, "plane needs x_dir"))?; diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index d86335fb..4748e9c9 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -1296,4 +1296,33 @@ mod naming { assert!(n.iter().any(|x| x.starts_with(want)), "{want} in {n:?}"); } } + + #[test] + fn a_sketch_on_a_face_stays_on_that_face() { + let mut s = Session::default(); + run(&mut s, "SketchCreate", json!({"plane": "XY", "name": "S"})); + run(&mut s, "ShapeRectangleTwoPoint", json!({"p0": [0, 0], "p1": [40, 30]})); + run(&mut s, "SketchStop", json!({})); + run(&mut s, "Extrude", json!({"distance": 10, "name": "Block"})); + run(&mut s, "SketchCreate", json!({"plane": {"face": [30, 15, 10]}, "name": "Top"})); + run(&mut s, "CircleCenterRadius", json!({"center": [30, 15], "radius": 3})); + run(&mut s, "SketchStop", json!({})); + run(&mut s, "Extrude", json!({"sketch": "Top", "distance": 5, "operation": "join", "name": "Peg"})); + let top = |s: &mut Session| run(s, "MeasureCommand", json!({}))["bodies"][0]["bbox"]["max"][2].as_f64().unwrap_or(0.0); + assert!((top(&mut s) - 15.0).abs() < 1e-6); + // Upstream, a tall boss over the picked point: the nearest upward face is now its top, + // but the sketch keeps to the block's top face by name. + run(&mut s, "timeline.rollTo", json!({"feature": "Block"})); + run(&mut s, "SketchCreate", json!({"plane": "XY", "name": "B"})); + run(&mut s, "ShapeRectangleTwoPoint", json!({"p0": [8, -5], "p1": [45, 35]})); + run(&mut s, "SketchStop", json!({})); + run(&mut s, "Extrude", json!({"sketch": "B", "distance": 20, "operation": "join", "name": "Boss"})); + run(&mut s, "timeline.rollTo", json!({})); + // (Its projected face outline changed, which it says; its plane did not.) + assert!(!warning(&s, "Top").unwrap_or_default().contains("no longer exists")); + // The peg starts on the block's top (z = 10), inside the boss: the part stays 20 tall. + assert!((top(&mut s) - 20.0).abs() < 1e-6, "{}", top(&mut s)); + let sk = s.model.state().sketch(s.doc.find_feature("Top").map(|f| f.id).unwrap_or(0)).map(|x| x.plane.origin.z); + assert_eq!(sk.map(|z| (z * 1e6).round() / 1e6), Some(10.0)); + } }