From dd5278a4021c8e518175c6c0ae8aef5ac6ffeb2d Mon Sep 17 00:00:00 2001 From: DuBearte <130190089+DuBearte@users.noreply.github.com> Date: Fri, 16 Oct 2026 08:26:23 -0400 Subject: [PATCH] solid.hole: position takes length expressions and follows them (#1678) (#1683) Thanks @DuBearte. --- crates/derive/src/lib.rs | 6 ++-- crates/doc/src/document.rs | 22 ++++++++++++ crates/doc/src/eval.rs | 31 +++++++++++----- .../pending/hole-position-expressions.rs | 5 +++ crates/engine/src/cmd/features.rs | 33 +++++++++++++++-- crates/engine/src/tests.rs | 35 +++++++++++++++++++ crates/ui-egui/src/dialogs.rs | 11 ++++-- skills/solidcraft-modeling/reference.md | 2 +- 8 files changed, 128 insertions(+), 17 deletions(-) create mode 100644 crates/doc/src/format/pending/hole-position-expressions.rs diff --git a/crates/derive/src/lib.rs b/crates/derive/src/lib.rs index 74d4ff4e..4fb8e1a7 100644 --- a/crates/derive/src/lib.rs +++ b/crates/derive/src/lib.rs @@ -21,7 +21,8 @@ //! `|i: usize, n: usize| -> &'static str`; it may read the other fields; //! - `pinned`: a face pick that stays where it is when the feature moves; //! `pick_if = cond`: a face pick when `cond` holds, else a position; -//! - `placement`: a primitive's placement point (its X, Y and Z expressions are not inputs); +//! - `placement`: a primitive's placement point, or an optional one (its X, Y and Z expressions +//! are not inputs); //! - `profiles_of = sketch_field`, optionally with `when = cond`: a profile selection; //! - `after = field`: visit this field right after `field` instead of in declaration order (where //! the parameter names already given need it); @@ -162,7 +163,8 @@ fn walk_field(b: &Ident, r: &Roles, which: Which, mutable: bool) -> Tokens { #w::Each::<#w::Vec3>::#each(#b, &mut |p| f(if #pick { #w::Geo::Pick(p) } else { #w::Geo::Pos(p) })); } } - Some(Geo::Placement) => quote! { f(#w::Geo::Placement(#b)); }, + // A point or an optional one (a hole's `at`, #1678). + Some(Geo::Placement) => quote! { #w::Each::::#each(#b, &mut |p| f(#w::Geo::Placement(p))); }, Some(Geo::Profiles { .. }) => { let (id, when) = (pre(b, "sketch"), pre(b, "when")); quote! { diff --git a/crates/doc/src/document.rs b/crates/doc/src/document.rs index 97780eb4..00523f8e 100644 --- a/crates/doc/src/document.rs +++ b/crates/doc/src/document.rs @@ -662,6 +662,12 @@ pub enum FeatureKind { Hole { #[walk(pick_if = points.is_none())] position: FacePick, + /// The position as length expressions, when it was given that way (#1678): the hole is + /// drilled where they evaluate, and follows the parameters. `position` keeps the point + /// they gave when the hole was made. + #[serde(default, skip_serializing_if = "Option::is_none", with = "opt_point_expr")] + #[walk(placement)] + at: Option, /// Drilling direction (into the material). direction: Dir, #[walk(length = "Diameter")] @@ -1935,6 +1941,22 @@ mod point_expr { } } +/// [`point_expr`] for an optional point. +mod opt_point_expr { + use serde::{Deserialize, Deserializer, Serialize, Serializer}; + + #[derive(Serialize, Deserialize)] + struct Wrap(#[serde(with = "super::point_expr")] super::PointExpr); + + pub fn serialize(p: &Option, s: S) -> Result { + p.as_ref().map(|p| Wrap(p.clone())).serialize(s) + } + + pub fn deserialize<'de, D: Deserializer<'de>>(d: D) -> Result, D::Error> { + Ok(Option::::deserialize(d)?.map(|w| w.0)) + } +} + impl FeatureKind { /// A sheet metal feature (its result depends on the sheet metal rules). pub fn is_plastic(&self) -> bool { diff --git a/crates/doc/src/eval.rs b/crates/doc/src/eval.rs index 40cde9bd..60ef47f8 100644 --- a/crates/doc/src/eval.rs +++ b/crates/doc/src/eval.rs @@ -2300,8 +2300,8 @@ fn sketch_hole_dir(st: &ModelState, spots: &[Vec3], n: Vec3) -> Vec3 { /// Where a hole feature drills (each on the surface) and in which direction: at its sketch /// points perpendicular to the sketch (into the material), else at its position. -fn hole_spots(st: &ModelState, f: &Feature) -> Result<(Vec, Vec3)> { - let FeatureKind::Hole { position, direction, points, .. } = &f.kind else { return Err(DocError::Invalid("not a hole".into())) }; +fn hole_spots(vals: &BTreeMap, st: &ModelState, f: &Feature) -> Result<(Vec, Vec3)> { + let FeatureKind::Hole { position, at, direction, points, .. } = &f.kind else { return Err(DocError::Invalid("not a hole".into())) }; let (spots, dir) = match points { Some(sp) => { let ss = @@ -2317,7 +2317,7 @@ fn hole_spots(st: &ModelState, f: &Feature) -> Result<(Vec, Vec3)> { } None => { let dir = direction.normalized().ok_or_else(|| DocError::Invalid("hole direction".into()))?; - let at = hole_on_surface(st, position.0, dir); + let at = hole_on_surface(st, hole_position(vals, position, at)?, dir); let dir = hole_dir_into_face(st, at, dir); return Ok((vec![hole_on_surface(st, at, dir)], dir)); } @@ -2351,8 +2351,8 @@ fn hole_depth_to(st: &ModelState, at: &Vec3, dir: Vec3, to: &Option, depth } /// A hole's spots as given (not yet moved onto the surface) and its direction. -fn hole_spots_raw(st: &ModelState, f: &Feature) -> Result<(Vec, Vec3)> { - let FeatureKind::Hole { position, direction, points, .. } = &f.kind else { return Err(DocError::Invalid("not a hole".into())) }; +fn hole_spots_raw(vals: &BTreeMap, st: &ModelState, f: &Feature) -> Result<(Vec, Vec3)> { + let FeatureKind::Hole { position, at, direction, points, .. } = &f.kind else { return Err(DocError::Invalid("not a hole".into())) }; match points { Some(sp) => { let ss = st.sketch(sp.sketch.0).ok_or_else(|| DocError::Unknown(format!("sketch {}", sp.sketch)))?; @@ -2364,16 +2364,29 @@ fn hole_spots_raw(st: &ModelState, f: &Feature) -> Result<(Vec, Vec3)> { } None => { let dir = direction.normalized().ok_or_else(|| DocError::Invalid("hole direction".into()))?; - Ok((vec![position.0], hole_dir_into_face(st, hole_on_surface(st, position.0, dir), dir))) + let p = hole_position(vals, position, at)?; + Ok((vec![p], hole_dir_into_face(st, hole_on_surface(st, p, dir), dir))) } } } +/// Where a hole placed by position is drilled: its `at` expressions evaluated, so it follows the +/// parameters (#1678), else the point it was given. +fn hole_position(vals: &BTreeMap, position: &crate::FacePick, at: &Option) -> Result { + let Some([x, y, z]) = at else { return Ok(position.0) }; + let p = + Vec3::new(Document::eval_in(vals, x, Kind::Length)?, Document::eval_in(vals, y, Kind::Length)?, Document::eval_in(vals, z, Kind::Length)?); + if ![p.x, p.y, p.z].iter().all(|c| c.is_finite() && c.abs() < 1e9) { + return Err(DocError::Invalid("the hole's position is out of range".into())); + } + Ok(p) +} + /// The cutting tools of a hole feature at every spot, each with its exact geometry when it is a /// straight cylinder (see [`hole_tools_and_prisms`]). fn hole_feature_tools(vals: &BTreeMap, st: &ModelState, f: &Feature) -> Result)>> { let FeatureKind::Hole { diameter, depth, hole, to, .. } = &f.kind else { return Err(DocError::Invalid("not a hole".into())) }; - let (spots, dir) = hole_spots(st, f)?; + let (spots, dir) = hole_spots(vals, st, f)?; if spots.is_empty() || spots.len() > 10_000 { return Err(DocError::Invalid("a hole needs 1…10000 points".into())); } @@ -3755,7 +3768,7 @@ fn replay( // A hole is drilled again at each copy of its spot (found on the surface there), so // a blind hole's depth counts from the face at the copy, not from the bottom of the // original hole under the original spot. - let (spots, dir) = hole_spots_raw(st, src)?; + let (spots, dir) = hole_spots_raw(vals, st, src)?; for (k, m) in mats.iter().enumerate() { let d = crate::apply_vector(m, dir).normalized().ok_or_else(|| DocError::Invalid("hole direction".into()))?; for p in &spots { @@ -4208,7 +4221,7 @@ fn eval_feature(doc: &Document, vals: &BTreeMap, f: &Feature, st: apply_traced(st, f, tools, *operation, &crate::plain(targets)) } FeatureKind::Hole { thread, diameter, .. } => { - let (spots, dir) = hole_spots(st, f)?; + let (spots, dir) = hole_spots(vals, st, f)?; let tools = hole_feature_tools(vals, st, f)?; let handled = sheet_hole_cut(st, f, &tools, warning)?; let tools: Vec = tools.into_iter().map(|(tool, _)| tool).collect(); diff --git a/crates/doc/src/format/pending/hole-position-expressions.rs b/crates/doc/src/format/pending/hole-position-expressions.rs new file mode 100644 index 00000000..8c5257ab --- /dev/null +++ b/crates/doc/src/format/pending/hole-position-expressions.rs @@ -0,0 +1,5 @@ +//! A Hole placed by position can keep that position as length expressions (`at`: `[x, y, z]`, +//! numbers as numbers, expressions as strings), and is drilled where they evaluate, so it follows +//! the design's parameters (#1678). `position` still holds the point it was made at. Older designs +//! need no transformation: a hole without `at` is drilled at `position` as before; 0.13.x asks for +//! an update instead of drilling a moved hole at its old point. diff --git a/crates/engine/src/cmd/features.rs b/crates/engine/src/cmd/features.rs index fa07dd15..bbd35cae 100644 --- a/crates/engine/src/cmd/features.rs +++ b/crates/engine/src/cmd/features.rs @@ -55,7 +55,7 @@ pub static COMMANDS: &[CommandSpec] = &[ .at("SOLID", "CREATE") .icon("hole") .key("H") - .params("position: [x,y,z] on a face | sketch + points: [sketch point ids] (one hole each, perpendicular to the sketch, into the material on whichever side of the sketch plane it is) | sketch alone (every sketch point: standalone points and circle centres); flip?: bool (sketch points: drill to the other side); direction?: [x,y,z] (default: into the face, against its outward normal; one that leaves the face is mirrored into it; ignored with sketch points); diameter | thread (a tapped hole: the tap drill, major − pitch) | clearance: \"M6\" with fit?: close|normal|loose (ISO 273); depth? (default through all) | to?: [x,y,z] (To Object: drill to that face or point); type?: simple|drilled|counterbore|countersink; tip_angle?; cb_diameter?, cb_depth?; cs_diameter?, cs_angle?; thread?: \"M6\" (cosmetic)"), + .params("position: [x,y,z] on a face (numbers, or length expressions the hole follows) | sketch + points: [sketch point ids] (one hole each, perpendicular to the sketch, into the material on whichever side of the sketch plane it is) | sketch alone (every sketch point: standalone points and circle centres); flip?: bool (sketch points: drill to the other side); direction?: [x,y,z] (default: into the face, against its outward normal; one that leaves the face is mirrored into it; ignored with sketch points); diameter | thread (a tapped hole: the tap drill, major − pitch) | clearance: \"M6\" with fit?: close|normal|loose (ISO 273); depth? (default through all) | to?: [x,y,z] (To Object: drill to that face or point); type?: simple|drilled|counterbore|countersink; tip_angle?; cb_diameter?, cb_depth?; cs_diameter?, cs_angle?; thread?: \"M6\" (cosmetic)"), CommandSpec::new("solid.pipe", "Pipe", pipe) .at("SOLID", "CREATE") .icon("pipe") @@ -1633,10 +1633,36 @@ fn hole(s: &mut Session, p: &Value) -> Result { { return Err(bad(cmd, format!("unknown thread `{t}` (ISO metric, like M6 or M6x0.75)"))); } - let position = match (&points, p.get("position").and_then(vec3)) { + // A position given as length expressions (#1678), say `["leg_h*0.4", "width/2", "t"]`: kept, + // so the hole follows the parameters; drilled where they evaluate now. + let at: Option = match p.get("position").and_then(Value::as_array) { + Some(a) if points.is_none() && a.len() == 3 && a.iter().any(Value::is_string) => { + let mut coords = Vec::with_capacity(3); + for v in a { + let e = match v { + Value::String(x) => x.trim().to_string(), + Value::Number(n) => n.to_string(), + _ => return Err(bad(cmd, "`position` must be [x, y, z]: numbers or length expressions")), + }; + check_expr(s, &e, Kind::Length, cmd, "position")?; + coords.push(solidcraft_doc::Expr::new(e)); + } + let [x, y, z]: [solidcraft_doc::Expr; 3] = coords.try_into().map_err(|_| bad(cmd, "`position` must be [x, y, z]"))?; + Some([x, y, z]) + } + _ => None, + }; + let at_value = match &at { + Some([x, y, z]) => { + let c = |e: &solidcraft_doc::Expr| s.doc.eval(e, Kind::Length).map_err(|e| bad(cmd, format!("`position`: {e}"))); + Some(Vec3::new(c(x)?, c(y)?, c(z)?)).filter(|v| v.is_finite()) + } + None => None, + }; + let position = match (&points, at_value.or_else(|| p.get("position").and_then(vec3))) { (Some((_, first, _)), None) => *first, (_, Some(x)) => x, - (None, None) => return Err(bad(cmd, "`position` must be [x, y, z] (or give `sketch` and `points`)")), + (None, None) => return Err(bad(cmd, "`position` must be [x, y, z], numbers or length expressions (or give `sketch` and `points`)")), }; let direction = match (p.get("direction").and_then(vec3), &points) { (None, Some((_, _, d))) => *d, @@ -1706,6 +1732,7 @@ fn hole(s: &mut Session, p: &Value) -> Result { &p, FeatureKind::Hole { position: FacePick(position), + at, direction: Dir(direction), diameter, depth, diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index c5c419f8..23e33e46 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -1217,6 +1217,41 @@ fn extrude_auto_operation_follows_the_to_object() { assert_eq!(extrude_reach(&s, &json!({"face": [20, 15, 20], "distance": 5, "direction": "negative"})), Some(-5.0)); } +/// #1678: a hole's `position` takes length expressions, keeps them and follows the parameters: a +/// hole on a plate's middle line stays there when the plate gets wider. A position of plain +/// numbers is stored as before, with no `at`. +#[test] +fn a_hole_position_of_expressions_follows_the_parameters() { + let mut s = Session::default(); + run(&mut s, "parameters.add", json!({"name": "width", "expression": "30 mm"})); + run(&mut s, "parameters.add", json!({"name": "t", "expression": "4 mm"})); + run(&mut s, "solid.box", json!({"length": 100, "width": "width", "height": "t", "body_name": "Plate"})); + run(&mut s, "solid.hole", json!({"position": ["40", "width/2", "t"], "clearance": "M5", "name": "Middle"})); + run(&mut s, "solid.hole", json!({"position": [80, 10, 4], "clearance": "M5", "name": "Fixed"})); + let centre_y = |s: &Session| { + let st = s.model.state(); + solidcraft_kernel::measure(&st.body("Plate").unwrap().body).unwrap().centroid.y + }; + let hole = |s: &Session, name: &str| -> Value { + let saved: Value = serde_json::from_str(&s.doc.to_json()).unwrap(); + saved["features"].as_array().and_then(|f| f.iter().find(|f| f["name"] == name)).cloned().unwrap() + }; + assert_eq!(hole(&s, "Middle")["at"], json!([40.0, "width/2", "t"]), "the expressions are kept"); + assert!(hole(&s, "Fixed").get("at").is_none(), "plain numbers are stored as before"); + // The fixed hole sits off the middle line, so it alone moves the centre; widen the plate. + let before = centre_y(&s); + run(&mut s, "parameters.change", json!({"name": "width", "expression": "40 mm"})); + let after = centre_y(&s); + // The middle hole is at y = 20 now (the plate's middle); the fixed one stays at y = 10. With + // the middle hole left at y = 15 the centre would sit lower still. + let plate = 100.0 * 40.0 * 4.0; + let hole_v = std::f64::consts::PI * (5.5f64 / 2.0).powi(2) * 4.0; + let want = (plate * 20.0 - hole_v * 20.0 - hole_v * 10.0) / (plate - 2.0 * hole_v); + assert!((after - want).abs() < 1e-3, "centre y {after}, want {want} (was {before})"); + // A bad expression is refused. + assert!(s.execute("solid.hole", &json!({"position": ["40", "nope/2", "t"], "diameter": 3})).is_err()); +} + /// Additive extrusions default to joining even when only a profile side touches material. #[test] fn auto_extrude_joins_side_contact_elbow_and_keeps_new_body_explicit() { diff --git a/crates/ui-egui/src/dialogs.rs b/crates/ui-egui/src/dialogs.rs index 10219146..d1385ca2 100644 --- a/crates/ui-egui/src/dialogs.rs +++ b/crates/ui-egui/src/dialogs.rs @@ -4109,7 +4109,8 @@ fn dialog_commands(app: &SolveApp, d: &Dialog) -> Result, S continue; } taken.extend(crate::viewport::grouped_selection(s, x)); - places.extend(face_point(x).map(|p| json!({ "position": p }))); + let kept = d.extra.get("position_exprs").filter(|k| face_point(x).is_some_and(|p| p == k["point"])).map(|k| k["at"].clone()); + places.extend(face_point(x).map(|p| json!({ "position": kept.unwrap_or(p) }))); } } let mut out = Vec::new(); @@ -4697,7 +4698,7 @@ pub fn for_feature(app: &SolveApp, id: u64, marker: Option) -> Option { + FeatureKind::Hole { position, at, direction, diameter, depth, hole, points, thread, to } => { let mut d = start("solid.hole")?; let mut k = hole_defaults(); if let Kind::Hole { diameter: dia, depth: dep, kind, cb_diameter, cb_depth, cs_diameter, cs_angle, opts } = &mut k { @@ -4745,6 +4746,12 @@ pub fn for_feature(app: &SolveApp, id: u64, marker: Option) -> Option = at.iter().map(|e| e.as_str()).collect(); + d.extra.insert("position_exprs".into(), json!({"at": exprs, "point": pt3(position.0)})); + } } } if to.is_some() { diff --git a/skills/solidcraft-modeling/reference.md b/skills/solidcraft-modeling/reference.md index 7cab578e..27624cdf 100644 --- a/skills/solidcraft-modeling/reference.md +++ b/skills/solidcraft-modeling/reference.md @@ -75,7 +75,7 @@ Distances are unsigned. Draw the geometry on the correct side first, and the dim | `sketch.slot.overall` / `sketch.slot.center_to_center` | `p0`, `p1` (overall ends / arc centres), `width` | Ids and dof are in the "Ids" section above. | | `solid.fillet` / `solid.chamfer` | `edges:[[x,y,z]]`, `radius` / `distance` | The pick snaps to the **nearest** edge, with no distance limit and no error, so check that the face count changed. One pick takes the whole tangent chain: a rounded rim needs 1 point, not 8. | | `solid.shell` | `faces:[[x,y,z]]`, `thickness` | A point on each face to remove, or its name from `body_topology`. Fillet first, except an edge of a removed face (see Order in `SKILL.md`). | -| `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.hole` | `position:[x,y,z]` on a face (numbers or length expressions such as `["40", "width/2", "t"]`, which the hole then follows) + `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}` whose origin may use length expressions) | Follows parameters. Mirror and pattern features cannot be mirrored or patterned again. |