From acf4669f73a3aa58c575d4b125f5ec547ae92a18 Mon Sep 17 00:00:00 2001 From: bherbruck Date: Sat, 10 Oct 2026 17:13:36 -0400 Subject: [PATCH 1/4] fix: select complete circular rims and curved surfaces in 3D Group kernel pieces across circular rims and hidden surface seams for hovering, selection, dialogs, and measurement. Preserve physical references and cover full-item toggling and picking with regression tests. Fixes #88 --- crates/engine/src/cmd/measure_sel.rs | 59 +++++- crates/geom/src/mesh.rs | 65 +++++++ crates/geom/src/tests.rs | 40 ++++ crates/ui-egui/src/dialogs.rs | 15 +- crates/ui-egui/src/selection.rs | 15 +- crates/ui-egui/src/viewport.rs | 264 +++++++++++++++++++++++++-- 6 files changed, 424 insertions(+), 34 deletions(-) diff --git a/crates/engine/src/cmd/measure_sel.rs b/crates/engine/src/cmd/measure_sel.rs index 95dc2dc9..9c978dfc 100644 --- a/crates/engine/src/cmd/measure_sel.rs +++ b/crates/engine/src/cmd/measure_sel.rs @@ -55,13 +55,13 @@ fn densify(pts: &[Vec3]) -> Vec { /// The mesh edge a selection means: by index if it still passes through the point, otherwise /// the nearest. -fn edge_points(m: &Mesh, index: usize, point: Vec3) -> Option> { +fn edge_index(m: &Mesh, index: usize, point: Vec3) -> Option { let near = |e: &Vec| e.windows(2).map(|w| point.dist_to_segment(w[0], w[1])).fold(f64::INFINITY, f64::min); let tol = m.bounds().diagonal().max(1.0) * 1e-6; - if let Some(e) = m.edges.get(index).filter(|e| near(e) < tol) { - return Some(e.clone()); + if m.edges.get(index).is_some_and(|e| near(e) < tol) { + return Some(index); } - m.edges.iter().min_by(|a, b| near(a).total_cmp(&near(b))).cloned() + m.edges.iter().enumerate().min_by(|(_, a), (_, b)| near(a).total_cmp(&near(b))).map(|(i, _)| i) } fn face_tris(m: &Mesh, f: usize) -> Vec<[Vec3; 3]> { @@ -75,15 +75,23 @@ fn describe(s: &Session, sel: &Sel) -> Option<(Geo, Option, Value)> { Sel::Vertex { point, .. } => (Geo::Points(vec![*point]), None, json!({"type": "vertex", "position": point})), Sel::Edge { body, index, point } => { let m = st.body(body)?.mesh(); - let pts = edge_points(&m, *index, *point)?; + let index = edge_index(&m, *index, *point)?; + let pts = m.edges.get(index)?; + let rim = m.circular_rim(index); + if rim.len() > 1 { + let pieces: Vec<&Vec> = rim.iter().filter_map(|&i| m.edges.get(i)).collect(); + let len: f64 = pieces.iter().map(|p| polyline_len(p)).sum(); + let points = pieces.iter().flat_map(|p| densify(p)).collect(); + return Some((Geo::Points(points), None, json!({"type": "edge", "length_mm": len}))); + } let (a, b) = (*pts.first()?, *pts.last()?); let straight = (b - a).normalized().filter(|d| pts.iter().all(|p| (*p - a).cross(*d).len() < 1e-6 * (1.0 + a.dist(b)))); - let len = polyline_len(&pts); - (Geo::Points(densify(&pts)), straight, json!({"type": "edge", "length_mm": len})) + let len = polyline_len(pts); + (Geo::Points(densify(pts)), straight, json!({"type": "edge", "length_mm": len})) } Sel::Face { body, index, .. } => { let m = st.body(body)?.mesh(); - let tris = face_tris(&m, *index); + let tris = m.surface_patch(*index).into_iter().flat_map(|f| face_tris(&m, f)).collect::>(); let area: f64 = tris.iter().map(|[a, b, c]| (*b - *a).cross(*c - *a).len() * 0.5).sum(); let normals: Vec = tris.iter().filter_map(|[a, b, c]| (*b - *a).cross(*c - *a).normalized()).collect(); let planar = normals.first().copied().filter(|n0| normals.iter().all(|n| n.dot(*n0) > 1.0 - 1e-6)); @@ -150,10 +158,41 @@ fn closest(a: &Geo, b: &Geo) -> Option<(Vec3, Vec3)> { pub(crate) fn measure_items(s: &Session, items: &Value) -> Result { let cmd = "inspect.measure"; let list = items.as_array().ok_or_else(|| bad(cmd, "`items` must be a list of selections"))?; - if list.is_empty() || list.len() > 2 { + if list.is_empty() || list.len() > 1000 { return Err(bad(cmd, "measure one or two items")); } - let sels: Vec = list.iter().map(|v| serde_json::from_value::(v.clone()).map_err(|e| bad(cmd, e.to_string()))).collect::>()?; + let st = s.world_state(); + let mut sels = Vec::new(); + for value in list { + let mut sel = serde_json::from_value::(value.clone()).map_err(|e| bad(cmd, e.to_string()))?; + // Two kernel halves of one visible item are one measurement target. + match &mut sel { + Sel::Edge { body, index, point } => { + if let Some(b) = st.body(body) { + let m = b.mesh(); + let resolved = edge_index(&m, *index, *point).unwrap_or(*index); + *index = m.circular_rim(resolved).first().copied().unwrap_or(resolved); + } + } + Sel::Face { body, index, .. } => { + if let Some(b) = st.body(body) { + *index = b.mesh().surface_patch(*index).first().copied().unwrap_or(*index); + } + } + _ => {} + } + let duplicate = sels.iter().any(|x| match (x, &sel) { + (Sel::Edge { body: a, index: i, .. }, Sel::Edge { body: b, index: j, .. }) + | (Sel::Face { body: a, index: i, .. }, Sel::Face { body: b, index: j, .. }) => a == b && i == j, + _ => false, + }); + if !duplicate { + sels.push(sel); + } + if sels.len() > 2 { + return Err(bad(cmd, "measure one or two items")); + } + } let mut described = Vec::new(); for x in &sels { described.push(describe(s, x).ok_or_else(|| bad(cmd, "an item can't be measured (pick vertices, edges, faces or bodies)"))?); diff --git a/crates/geom/src/mesh.rs b/crates/geom/src/mesh.rs index 32c3373a..296f4808 100644 --- a/crates/geom/src/mesh.rs +++ b/crates/geom/src/mesh.rs @@ -82,6 +82,71 @@ impl Mesh { pub fn face_edges(&self, f: u32) -> Vec { self.edge_faces.iter().enumerate().filter(|(_, fs)| fs.contains(&f)).map(|(i, _)| i).collect() } + /// Pieces of one surface joined across hidden seams, including `face` itself. + pub fn surface_patch(&self, face: usize) -> Vec { + let mut patch = vec![face]; + let mut todo = vec![face]; + while let Some(f) = todo.pop() { + for (ei, fs) in self.edge_faces.iter().enumerate() { + if !self.seams.get(ei).copied().unwrap_or(false) || !fs.iter().any(|x| *x as usize == f) { + continue; + } + for &next in fs { + let next = next as usize; + if !patch.contains(&next) { + patch.push(next); + todo.push(next); + } + } + } + } + patch.sort_unstable(); + patch + } + + /// A complete circular rim may be split into several kernel edges. Keep its pieces + /// together for picking, without joining other tangent curves or changing edge indices. + pub fn circular_rim(&self, edge: usize) -> Vec { + let single = vec![edge]; + let Some(p) = self.edges.get(edge).filter(|p| p.len() >= 3) else { return single }; + let (Some(&a), Some(&b), Some(&c)) = (p.first(), p.get(p.len() / 2), p.last()) else { return single }; + let (u, v) = (b - a, c - a); + let n = u.cross(v); + if !(n.len2() > u.len2() * v.len2() * 1e-12) { + return single; + } + let center = a + (v.cross(n) * u.len2() + n.cross(u) * v.len2()) / (2.0 * n.len2()); + let radius = center.dist(a); + let normal = n.normalized().unwrap_or(Vec3::ZERO); + let tol = (radius * 1e-6).max(1e-9); + let mut rim = self.tangent_chain(edge, 2f64.to_radians()); + if rim.len() < 2 || !radius.is_finite() { + return single; + } + // All samples must lie on this circle, and every end must join exactly one other + // piece. Open arcs, ellipses, and branching tangent chains stay separate. + for &i in &rim { + let Some(points) = self.edges.get(i) else { return single }; + if points.iter().any(|q| !q.is_finite() || (q.dist(center) - radius).abs() > tol || (*q - center).dot(normal).abs() > tol) { + return single; + } + for end in [points.first(), points.last()].into_iter().flatten() { + let joins = rim + .iter() + .filter(|&&j| j != i) + .filter_map(|&j| self.edges.get(j)) + .flat_map(|q| [q.first(), q.last()]) + .flatten() + .filter(|q| q.dist(*end) < tol) + .count(); + if joins != 1 { + return single; + } + } + } + rim.sort_unstable(); + rim + } /// The edges reached from `e` through vertices where the next edge continues smoothly /// (angle between the end tangents below `max_angle`), including `e` itself. pub fn tangent_chain(&self, e: usize, max_angle: f64) -> Vec { diff --git a/crates/geom/src/tests.rs b/crates/geom/src/tests.rs index af1b6f68..3e996d14 100644 --- a/crates/geom/src/tests.rs +++ b/crates/geom/src/tests.rs @@ -84,6 +84,46 @@ fn tangent_chains_follow_smooth_edges() { assert_eq!(m.face_edges(1), vec![1, 2]); } +#[test] +fn circular_rims_group_only_closed_cocircular_edges() { + let arc = |start: f64, sweep: f64, yscale: f64| -> Vec { + (0..=32) + .map(|i| { + let a = start + sweep * i as f64 / 32.0; + Vec3::new(10.0 * a.cos(), yscale * 10.0 * a.sin(), 5.0) + }) + .collect() + }; + let pi = std::f64::consts::PI; + let mut m = Mesh { + positions: vec![Vec3::new(-10.0, -10.0, 5.0), Vec3::new(10.0, 10.0, 5.0)], + edges: vec![arc(0.0, pi, 1.0), arc(2.0 * pi, -pi, 1.0)], + ..Default::default() + }; + assert_eq!(m.circular_rim(0), vec![0, 1]); + assert_eq!(m.circular_rim(1), vec![0, 1]); + // A hidden seam cannot form a selectable rim. + m.seams = vec![false, true]; + assert_eq!(m.circular_rim(0), vec![0]); + m.seams.clear(); + m.edges[1] = arc(pi, pi / 2.0, 1.0); + assert_eq!(m.circular_rim(0), vec![0]); + // A closed, smooth ellipse is not a circle. + m.edges = vec![arc(0.0, pi, 0.7), arc(pi, pi, 0.7)]; + assert_eq!(m.circular_rim(0), vec![0]); + assert_eq!(m.circular_rim(99), vec![99]); +} + +#[test] +fn surface_patches_cross_only_hidden_seams() { + let mut m = Mesh { edge_faces: vec![vec![0, 1], vec![1, 2], vec![2, 3]], seams: vec![true, true, false], ..Default::default() }; + assert_eq!(m.surface_patch(0), vec![0, 1, 2]); + assert_eq!(m.surface_patch(2), vec![0, 1, 2]); + assert_eq!(m.surface_patch(3), vec![3]); + m.seams.clear(); + assert_eq!(m.surface_patch(0), vec![0]); +} + #[test] fn cubic_and_conic_segments_measure_exactly() { use crate::{Loop2, Seg2, Vec2}; diff --git a/crates/ui-egui/src/dialogs.rs b/crates/ui-egui/src/dialogs.rs index d1382537..7f3f83ef 100644 --- a/crates/ui-egui/src/dialogs.rs +++ b/crates/ui-egui/src/dialogs.rs @@ -480,10 +480,12 @@ impl Dialog { .collect::>() }) .unwrap_or_else(|| vec![sel.clone()]), + (Kind::Measure { .. }, _) => vec![crate::viewport::measurement_selection(s, sel)], + _ if inp.multi => crate::viewport::grouped_selection(s, sel), _ => vec![sel.clone()], }; for x in picked { - if !inp.items.contains(&x) { + if !inp.items.iter().any(|y| selection::same_item(y, &x)) { inp.items.push(x); } } @@ -631,8 +633,11 @@ impl Dialog { /// A pick in the viewport (already accepted by the active input). pub fn pick(&mut self, s: &Session, sel: Sel) { + let measure = matches!(self.kind, Kind::Measure { .. }); + let sel = if measure { crate::viewport::measurement_selection(s, &sel) } else { sel }; let chain = matches!(self.kind, Kind::Fillet { chain: true, .. }); - let mut picked = vec![sel.clone()]; + let mut picked = + if !measure && self.active_input().is_some_and(|i| i.multi) { crate::viewport::grouped_selection(s, &sel) } else { vec![sel.clone()] }; // Tangent chain: an edge brings the edges that continue it smoothly. if chain && let Sel::Edge { body, index, .. } = &sel @@ -667,13 +672,15 @@ impl Dialog { } /// Box selection result: replace (or add to) the active input. - pub fn take_box(&mut self, sels: Vec, add: bool) { + pub fn take_box(&mut self, s: &Session, sels: Vec, add: bool) { + let measure = matches!(self.kind, Kind::Measure { .. }); let Some(inp) = self.inputs.get_mut(self.active) else { return }; if !add { inp.items.clear(); } for x in sels { - if fits(inp.accept, &x) && !inp.items.contains(&x) { + let x = if measure { crate::viewport::measurement_selection(s, &x) } else { x }; + if fits(inp.accept, &x) && !inp.items.iter().any(|y| selection::same_item(y, &x)) { inp.items.push(x); } } diff --git a/crates/ui-egui/src/selection.rs b/crates/ui-egui/src/selection.rs index 413f660d..cfbfe9fe 100644 --- a/crates/ui-egui/src/selection.rs +++ b/crates/ui-egui/src/selection.rs @@ -8,6 +8,15 @@ use solvecraft_engine::geom::Vec3; use crate::viewport::Hit; +/// Model picks retain an on-surface point, but their identity is the body and topology index. +pub(crate) fn same_item(a: &Sel, b: &Sel) -> bool { + match (a, b) { + (Sel::Edge { body: a, index: i, .. }, Sel::Edge { body: b, index: j, .. }) + | (Sel::Face { body: a, index: i, .. }, Sel::Face { body: b, index: j, .. }) => a == b && i == j, + _ => a == b, + } +} + /// What a selection input accepts (bit set). pub type Accept = u16; pub const PROFILES: Accept = 1; @@ -73,11 +82,11 @@ impl SelInput { return; } // A pick of something already in (or a chain fully in) takes it out. - if picked.iter().all(|p| self.items.contains(p)) { - self.items.retain(|x| !picked.contains(x)); + if picked.iter().all(|p| self.items.iter().any(|x| same_item(x, p))) { + self.items.retain(|x| !picked.iter().any(|p| same_item(x, p))); } else { for p in picked { - if !self.items.contains(&p) { + if !self.items.iter().any(|x| same_item(x, &p)) { self.items.push(p); } } diff --git a/crates/ui-egui/src/viewport.rs b/crates/ui-egui/src/viewport.rs index 45c1d11f..d436dd90 100644 --- a/crates/ui-egui/src/viewport.rs +++ b/crates/ui-egui/src/viewport.rs @@ -1091,7 +1091,14 @@ fn build_highlight(app: &SolveApp) -> GpuScene { let t = Tokens::get(); let mut sc = GpuScene::default(); let st = app.session.world_state(); - let sel = app.highlighted(); + let mut sel = Vec::new(); + for item in app.highlighted() { + for piece in grouped_selection(&app.session, &item) { + if !sel.iter().any(|x| crate::selection::same_item(x, &piece)) { + sel.push(piece); + } + } + } let hover = app.viewport.hover.as_ref(); let size = origin_size(app.cam.half_height()); // Origin planes and construction planes. @@ -1242,9 +1249,12 @@ fn build_highlight(app: &SolveApp) -> GpuScene { if let Some(b) = st.body(body) { let k = t.hover_face_lift; let c = body_rgb(app, body); - face_tris(&mut sc, &b.mesh(), *index, [c.0.saturating_add(k), c.1.saturating_add(k), c.2.saturating_add(k), 255], true); let m = b.mesh(); - for e in m.face_edges(u32::try_from(*index).unwrap_or(u32::MAX)) { + let patch = m.surface_patch(*index); + for &f in &patch { + face_tris(&mut sc, &m, f, [c.0.saturating_add(k), c.1.saturating_add(k), c.2.saturating_add(k), 255], true); + } + for (e, _) in m.edge_faces.iter().enumerate().filter(|(_, fs)| fs.iter().any(|f| patch.contains(&(*f as usize)))) { if let Some(p) = m.edges.get(e) && !m.seams.get(e).copied().unwrap_or(false) { @@ -1254,12 +1264,13 @@ fn build_highlight(app: &SolveApp) -> GpuScene { } } Some(Hit::Edge { body, index, mid }) => { - if let Some(b) = st.body(body) - && let Some(e) = edge_of(&b.mesh(), *index, *mid) - { - let selected = sel.iter().any(|x| matches!(x, Sel::Edge { point, .. } if point.dist(*mid) < 1e-9)); - if !selected { - edge_lines(&mut sc, e, t.hover_edge, t.hover_edge_halo, 2.0); + if let Some(b) = st.body(body) { + let m = b.mesh(); + for i in m.circular_rim(*index) { + let selected = sel.iter().any(|x| matches!(x, Sel::Edge { body: b, index: j, .. } if b == body && *j == i)); + if !selected && let Some(e) = if i == *index { edge_of(&m, i, *mid) } else { m.edges.get(i) } { + edge_lines(&mut sc, e, t.hover_edge, t.hover_edge_halo, 2.0); + } } } } @@ -1286,7 +1297,7 @@ fn body_rgb(app: &SolveApp, name: &str) -> (u8, u8, u8) { (c.r(), c.g(), c.b()) } -fn polyline_mid(pts: &[Vec3]) -> Vec3 { +pub(crate) fn polyline_mid(pts: &[Vec3]) -> Vec3 { let len: f64 = pts.windows(2).map(|w| w[0].dist(w[1])).sum(); let mut acc = 0.0; for w in pts.windows(2) { @@ -1730,12 +1741,13 @@ pub fn show(app: &mut SolveApp, ui: &mut egui::Ui) { fn select(app: &mut SolveApp, sel: Option, add: bool) { match sel { Some(x) => { - let item = serde_json::to_value(&x).unwrap_or_default(); - if add && app.session.selection.contains(&x) { - let rest: Vec = app.session.selection.iter().filter(|y| **y != x).cloned().collect(); + let items = grouped_selection(&app.session, &x); + if add && items.iter().all(|x| app.session.selection.iter().any(|y| crate::selection::same_item(x, y))) { + let rest: Vec = + app.session.selection.iter().filter(|y| !items.iter().any(|x| crate::selection::same_item(x, y))).cloned().collect(); let _ = app.run("select.set", json!({ "items": rest })); } else { - let _ = app.run("select.set", json!({"items": [item], "add": add})); + let _ = app.run("select.set", json!({"items": items, "add": add})); } } None if !add => { @@ -1745,6 +1757,75 @@ fn select(app: &mut SolveApp, sel: Option, add: bool) { } } +/// Expand a visible rim or surface into the kernel pieces a command needs. The original +/// face pick keeps its point (used by tools such as Hole and construction geometry). +pub(crate) fn grouped_selection(s: &solvecraft_engine::Session, sel: &Sel) -> Vec { + let st = s.world_state(); + match sel { + Sel::Edge { body, index, .. } => { + if let Some(b) = st.body(body) { + let m = b.mesh(); + return m + .circular_rim(*index) + .into_iter() + .filter_map(|i| m.edges.get(i).map(|e| Sel::Edge { body: body.clone(), index: i, point: polyline_mid(e) })) + .collect(); + } + } + Sel::Face { body, index, .. } => { + if let Some(b) = st.body(body) { + let m = b.mesh(); + let mut patch = m.surface_patch(*index); + patch.sort_by_key(|i| usize::from(i != index)); + return patch + .into_iter() + .filter_map(|i| { + if i == *index { + return Some(sel.clone()); + } + m.triangles.iter().zip(&m.tri_face).find_map(|(t, f)| { + if *f as usize != i { + return None; + } + let [a, b, c] = m.tri(t)?; + Some(Sel::Face { body: body.clone(), index: i, point: (a + b + c) / 3.0 }) + }) + }) + .collect(); + } + } + _ => {} + } + vec![sel.clone()] +} + +/// Measure keeps one entry per visible item, rather than using its two slots for kernel halves. +pub(crate) fn measurement_selection(s: &solvecraft_engine::Session, sel: &Sel) -> Sel { + let pieces = grouped_selection(s, sel); + let mut representative = sel.clone(); + match &mut representative { + Sel::Edge { index, .. } => { + if let Some(Sel::Edge { index: i, .. }) = pieces.first() { + *index = *i; + } + } + Sel::Face { index, .. } => { + if let Some(i) = pieces + .iter() + .filter_map(|p| match p { + Sel::Face { index, .. } => Some(*index), + _ => None, + }) + .min() + { + *index = i; + } + } + _ => {} + } + representative +} + /// What a box selects: sketch entities while sketching, otherwise what the open dialog's /// active input takes (edges, faces or bodies), otherwise bodies. fn box_select(app: &mut SolveApp, proj: &Proj, bx: BoxSel, add: bool) { @@ -1794,7 +1875,9 @@ fn box_select(app: &mut SolveApp, proj: &Proj, bx: BoxSel, add: bool) { let m = b.mesh(); if accept & EDGES != 0 { for (ei, e) in m.edges.iter().enumerate() { - if !m.seams.get(ei).copied().unwrap_or(false) && bx.takes(&to2(e)) { + let rim = m.circular_rim(ei); + let takes = if bx.crossing { bx.takes(&to2(e)) } else { rim.iter().all(|&i| m.edges.get(i).is_some_and(|p| bx.takes(&to2(p)))) }; + if !m.seams.get(ei).copied().unwrap_or(false) && takes { out.push(Sel::Edge { body: b.name.clone(), index: ei, point: polyline_mid(e) }); } } @@ -1811,8 +1894,15 @@ fn box_select(app: &mut SolveApp, proj: &Proj, bx: BoxSel, add: bool) { .find(|(_, tf)| **tf as usize == f) .and_then(|(t, _)| m.tri(t)) .map(|[a, bb, c]| (a + bb + c) / 3.0); + let takes = bx.takes(&to2(&pts)) + && (bx.crossing + || m.surface_patch(f).iter().all(|&face| { + m.face_edges(u32::try_from(face).unwrap_or(u32::MAX)) + .iter() + .all(|&e| m.edges.get(e).is_some_and(|p| bx.takes(&to2(p)))) + })); if let Some(point) = point - && bx.takes(&to2(&pts)) + && takes { out.push(Sel::Face { body: b.name.clone(), index: f, point }); } @@ -1837,8 +1927,17 @@ fn box_select(app: &mut SolveApp, proj: &Proj, bx: BoxSel, add: bool) { } } } + let mut grouped = Vec::new(); + for item in out { + for piece in grouped_selection(&app.session, &item) { + if !grouped.iter().any(|x| crate::selection::same_item(x, &piece)) { + grouped.push(piece); + } + } + } + let out = grouped; if let Some(mut d) = app.dialog.take() { - d.take_box(out, add); + d.take_box(&app.session, out, add); app.dialog = Some(d); return; } @@ -2304,6 +2403,137 @@ fn view_menu(app: &mut SolveApp, ui: &mut egui::Ui, button: Rect) -> bool { mod tests { use super::*; + fn cylinder_app() -> SolveApp { + let mut s = solvecraft_engine::Session::default(); + s.execute("solid.cylinder", &json!({"radius": 10, "height": 20})).unwrap(); + let mut app = SolveApp::new(s, crate::Services::default()); + app.ui.show_origin = false; + app + } + + #[test] + fn circular_items_measure_as_whole_rims_and_surfaces() { + let mut app = cylinder_app(); + let st = app.session.world_state(); + let b = &st.bodies[0]; + let m = b.mesh(); + let edges: Vec = m + .edges + .iter() + .enumerate() + .filter(|(_, p)| p.len() > 3 && p.iter().all(|q| (q.z - 20.0).abs() < 1e-6)) + .map(|(index, p)| Sel::Edge { body: b.name.clone(), index, point: polyline_mid(p) }) + .collect(); + let result = app.session.measure_items(&edges); + assert_eq!(result["items"].as_array().unwrap().len(), 1); + assert!((result["items"][0]["length_mm"].as_f64().unwrap() - 20.0 * std::f64::consts::PI).abs() < 0.2, "{result}"); + app.session.selection = edges; + let d = crate::dialogs::Dialog::for_command(&app, "inspect.measure").unwrap(); + assert_eq!(d.items().len(), 1); + + let faces = m.edge_faces.iter().zip(&m.seams).find(|(_, seam)| **seam).unwrap().0; + let wall = Sel::Face { body: b.name.clone(), index: faces[0] as usize, point: Vec3::new(10.0, 0.0, 10.0) }; + let wall_pieces = grouped_selection(&app.session, &wall); + let result = app.session.measure_items(&wall_pieces); + assert_eq!(result["items"].as_array().unwrap().len(), 1); + assert!((result["items"][0]["area_mm2"].as_f64().unwrap() - 400.0 * std::f64::consts::PI).abs() < 10.0, "{result}"); + let mut d = crate::dialogs::Dialog::for_command(&app, "inspect.measure").unwrap(); + d.pick(&app.session, wall.clone()); + assert_eq!(d.items().len(), 2, "a rim and a wall are two measurement targets"); + d.pick(&app.session, Sel::Face { body: b.name.clone(), index: faces[1] as usize, point: Vec3::new(-10.0, 0.0, 10.0) }); + assert_eq!(d.items().len(), 1, "the other half toggles the same wall"); + } + + #[test] + fn circular_rim_hover_click_and_dialog_pick_include_both_halves() { + let mut app = cylinder_app(); + let st = app.session.world_state(); + let b = &st.bodies[0]; + let m = b.mesh(); + let top = m.edges.iter().position(|e| e.len() > 3 && e.iter().all(|p| (p.z - 20.0).abs() < 1e-6)).unwrap(); + let rim = m.circular_rim(top); + assert_eq!(rim.len(), 2); + let line_count: usize = rim.iter().map(|&i| m.edges[i].len() - 1).sum(); + for &i in &rim { + let mid = polyline_mid(&m.edges[i]); + app.viewport.hover = Some(Hit::Edge { body: b.name.clone(), index: i, mid }); + assert_eq!(build_highlight(&app).lines.len(), 2 * line_count * crate::gpu::LINE_SIZE); + let item = hit_sel(app.viewport.hover.as_ref().unwrap()).unwrap(); + select(&mut app, Some(item.clone()), false); + assert_eq!(app.session.selection.len(), 2); + // Selecting both pieces must not draw each highlight twice. + assert_eq!(build_highlight(&app).lines.len(), 2 * line_count * crate::gpu::LINE_SIZE); + // Ctrl/Shift clicking either half deselects the entire rim. + select(&mut app, Some(item.clone()), true); + assert!(app.session.selection.is_empty()); + let mut d = crate::dialogs::Dialog::for_command(&app, "solid.chamfer").unwrap(); + if let crate::dialogs::Kind::Fillet { chain, .. } = &mut d.kind { + *chain = false; + } + d.pick(&app.session, item.clone()); + assert_eq!(d.items().len(), 2); + d.pick(&app.session, item); + assert!(d.items().is_empty()); + } + // Screen-space picking works from either side of the rim as the model turns. + for &i in &rim { + let mid = polyline_mid(&m.edges[i]); + app.cam = app.cam.looking_from(Vec3::new(mid.x, mid.y, 20.0)); + app.cam.target = Vec3::new(0.0, 0.0, 10.0); + for perspective in [false, true] { + app.ui.perspective = perspective; + let proj = projection(&app, Rect::from_min_size(pos2(0.0, 0.0), vec2(800.0, 600.0))); + let hits = pick(&app, &proj, proj.to_screen(mid).unwrap()); + let (_, sel) = candidate(&app, &hits).unwrap(); + assert!(matches!(sel, Sel::Edge { .. }), "{hits:?}"); + assert_eq!(grouped_selection(&app.session, &sel).len(), 2); + } + } + } + + #[test] + fn curved_face_hover_selection_and_dialogs_cross_the_seam() { + let mut app = cylinder_app(); + let st = app.session.world_state(); + let b = &st.bodies[0]; + let m = b.mesh(); + let faces = m.edge_faces.iter().zip(&m.seams).find(|(_, seam)| **seam).unwrap().0; + assert_eq!(faces.len(), 2); + let triangle_count = m.tri_face.iter().filter(|f| faces.contains(f)).count(); + for &f in faces { + let index = f as usize; + let t = m.triangles.iter().zip(&m.tri_face).find(|(_, tf)| **tf == f).unwrap().0; + let [a, c, d] = m.tri(t).unwrap(); + let point = (a + c + d) / 3.0; + app.viewport.hover = Some(Hit::Face { body: b.name.clone(), index, point }); + assert_eq!(build_highlight(&app).tris.len(), triangle_count * 3 * crate::gpu::TRI_SIZE); + let item = hit_sel(app.viewport.hover.as_ref().unwrap()).unwrap(); + select(&mut app, Some(item.clone()), false); + assert_eq!(app.session.selection.len(), 2); + assert_eq!(build_highlight(&app).tris.len(), triangle_count * 3 * crate::gpu::TRI_SIZE); + assert_eq!(app.session.selection.first(), Some(&item), "keep the original tool placement point first"); + let mut dialog = crate::dialogs::Dialog::for_command(&app, "solid.chamfer").unwrap(); + assert_eq!(dialog.items().len(), 2); + dialog.pick(&app.session, item.clone()); + assert!(dialog.items().is_empty()); + dialog.pick(&app.session, item); + assert_eq!(dialog.items().len(), 2); + // A point elsewhere on the opposite half still toggles the whole surface off. + let other = faces.iter().find(|&&x| x != f).unwrap(); + select(&mut app, Some(Sel::Face { body: b.name.clone(), index: *other as usize, point: m.positions[0] }), true); + assert!(app.session.selection.is_empty()); + } + // Planar caps and box corners stay separate. + let cap = (0..b.body.face_count()).find(|&i| m.surface_patch(i).len() == 1).unwrap(); + assert_eq!(m.surface_patch(cap), vec![cap]); + app.session.execute("solid.box", &json!({"length": 10, "width": 10, "height": 10})).unwrap(); + let st = app.session.world_state(); + let box_body = st.bodies.last().unwrap(); + for i in 0..box_body.body.face_count() { + assert_eq!(box_body.mesh().surface_patch(i), vec![i]); + } + } + #[test] fn section_outlines_follow_the_cut() { let mut s = solvecraft_engine::Session::default(); From 54a86dc6f573955b1f5bc566d5c0a28874a4354a Mon Sep 17 00:00:00 2001 From: CypherNaugh_0x <9931495+CypherNaught-0x@users.noreply.github.com> Date: Sat, 10 Oct 2026 20:21:47 +0200 Subject: [PATCH 2/4] Deleting a sketch deletes every feature built from it: a pipe whose path sketch was deleted stayed in the timeline as a broken feature ("unknown sketch"), and so did holes drilled at sketch points and patches filling sketch regions. Which sketches a feature uses now comes from one place, FeatureKind::input_sketches, a match over every kind with no wildcard arm, so a new feature kind does not compile until it says which sketches it uses; a test reads the FeatureKind definition and fails when a kind with a sketch field is listed as using none (it catches a sketch field added to an existing kind). timeline.dependents reports the same features --- crates/doc/src/document.rs | 73 +++++++++++++++++++++++++++++++------- crates/doc/src/tests.rs | 50 ++++++++++++++++++++++++++ crates/engine/src/tests.rs | 16 +++++++++ 3 files changed, 127 insertions(+), 12 deletions(-) diff --git a/crates/doc/src/document.rs b/crates/doc/src/document.rs index c7f70e3a..7b94485c 100644 --- a/crates/doc/src/document.rs +++ b/crates/doc/src/document.rs @@ -939,6 +939,66 @@ fn z_axis() -> Vec3 { } impl FeatureKind { + /// Sketches the feature is built from (profiles, paths, points): deleting one of them + /// deletes the feature. Every kind is listed, so a new one has to say what it uses. + pub fn input_sketches(&self) -> Vec { + use FeatureKind as K; + match self { + K::Extrude { sketch, .. } + | K::Revolve { sketch, .. } + | K::Emboss { sketch, .. } + | K::Rib { sketch, .. } + | K::SheetBase { sketch, .. } + | K::SheetContour { sketch, .. } => vec![*sketch], + K::Sweep { sketch, path_sketch, .. } => vec![*sketch, *path_sketch], + K::Pipe { path_sketch, .. } | K::Pattern { pattern: PatternKind::Path { path_sketch, .. }, .. } => vec![*path_sketch], + K::Loft { sections, .. } => sections.iter().map(|s| s.sketch).collect(), + K::SheetFold { sketch, .. } | K::SplitFace { sketch, .. } | K::Patch { sketch, .. } => sketch.iter().copied().collect(), + K::Hole { points, .. } => points.iter().map(|p| p.sketch).collect(), + K::Sketch { .. } + | K::Pattern { .. } + | K::Fillet { .. } + | K::Chamfer { .. } + | K::Box { .. } + | K::Cylinder { .. } + | K::Sphere { .. } + | K::Torus { .. } + | K::Combine { .. } + | K::Mirror { .. } + | K::Thread { .. } + | K::Shell { .. } + | K::Draft { .. } + | K::ConstructionPlane { .. } + | K::Split { .. } + | K::ConstructionAxis { .. } + | K::ConstructionPoint { .. } + | K::Scale { .. } + | K::OffsetFace { .. } + | K::DeleteFace { .. } + | K::BoundingSolid { .. } + | K::ReplaceFace { .. } + | K::Align { .. } + | K::Coil { .. } + | K::SheetFlange { .. } + | K::SheetHem { .. } + | K::SheetUnfold { .. } + | K::SheetConvert { .. } + | K::Boss { .. } + | K::Lip { .. } + | K::SnapFit { .. } + | K::Rest { .. } + | K::BoundaryFill { .. } + | K::Remove { .. } + | K::Stitch { .. } + | K::Thicken { .. } + | K::SurfaceExtend { .. } + | K::SurfaceTrim { .. } + | K::Move { .. } + | K::Import { .. } + | K::MeshImport { .. } => Vec::new(), + } + } + /// A sheet metal feature (its result depends on the sheet metal rules). pub fn is_plastic(&self) -> bool { matches!(self, FeatureKind::Boss { .. } | FeatureKind::Lip { .. } | FeatureKind::SnapFit { .. } | FeatureKind::Rest { .. }) @@ -1620,18 +1680,7 @@ impl Document { pub fn delete_feature(&mut self, id: u64) -> Result> { let idx = self.feature_index(id).ok_or_else(|| DocError::Unknown(format!("feature {id}")))?; let mut gone = vec![id]; - for f in &self.features { - match &f.kind { - FeatureKind::Extrude { sketch, .. } | FeatureKind::Revolve { sketch, .. } if *sketch == id => gone.push(f.id), - FeatureKind::Sweep { sketch, path_sketch, .. } if *sketch == id || *path_sketch == id => gone.push(f.id), - FeatureKind::Loft { sections, .. } if sections.iter().any(|s| s.sketch == id) => gone.push(f.id), - FeatureKind::Pattern { pattern: PatternKind::Path { path_sketch, .. }, .. } if *path_sketch == id => gone.push(f.id), - FeatureKind::Emboss { sketch, .. } | FeatureKind::Rib { sketch, .. } if *sketch == id => gone.push(f.id), - FeatureKind::SheetBase { sketch, .. } | FeatureKind::SheetContour { sketch, .. } if *sketch == id => gone.push(f.id), - FeatureKind::SheetFold { sketch: Some(s), .. } | FeatureKind::SplitFace { sketch: Some(s), .. } if *s == id => gone.push(f.id), - _ => {} - } - } + gone.extend(self.features.iter().filter(|f| f.kind.input_sketches().contains(&id)).map(|f| f.id)); self.features.retain(|f| !gone.contains(&f.id)); // Configuration columns of deleted features go with them. let keep: Vec = self.configs.columns.iter().map(|c| !matches!(c, crate::config::Column::Suppress(id) if gone.contains(id))).collect(); diff --git a/crates/doc/src/tests.rs b/crates/doc/src/tests.rs index b74f6da1..5a4b9016 100644 --- a/crates/doc/src/tests.rs +++ b/crates/doc/src/tests.rs @@ -295,3 +295,53 @@ fn parameter_cycles_are_found() { assert!(errs["a"].contains("circular reference: a → b → c → a"), "{}", errs["a"]); assert!(vals.contains_key("d") && errs.contains_key("a") && errs.contains_key("e")); } + +/// Features that use a sketch's points or regions go with it (not only profile and path users). +#[test] +fn input_sketches_cover_points_and_patches() { + let hole: FeatureKind = serde_json::from_value(serde_json::json!({ + "type": "hole", "position": [0, 0, 0], "direction": [0, 0, -1], "diameter": "3", + "points": {"sketch": 7, "ids": ["p1"]}, + })) + .unwrap(); + assert_eq!(hole.input_sketches(), vec![7]); + let patch: FeatureKind = serde_json::from_value(serde_json::json!({"type": "patch", "sketch": 9})).unwrap(); + assert_eq!(patch.input_sketches(), vec![9]); + let pipe: FeatureKind = serde_json::from_value(serde_json::json!({"type": "pipe", "path_sketch": 72, "path": ["l1"], "diameter": "4"})).unwrap(); + assert_eq!(pipe.input_sketches(), vec![72]); +} + +/// Guard for `FeatureKind::input_sketches`: a variant with a field that refers to a sketch +/// (a field named `*sketch*` holding an id, sketch points or loft sections) must not sit in the +/// arm that returns no sketches. New variants are caught by the compiler (the match has no +/// wildcard); this catches a sketch field added to an existing variant. +#[test] +fn input_sketches_lists_every_variant_with_a_sketch_field() { + let src = include_str!("document.rs"); + let body = src.split("pub enum FeatureKind {").nth(1).and_then(|s| s.split("\n}\n").next()).unwrap(); + let mut with_sketch = Vec::new(); + let mut variant = ""; + for line in body.lines() { + let t = line.trim(); + if line.starts_with(" ") && !line.starts_with(" ") && t.chars().next().is_some_and(|c| c.is_ascii_uppercase()) { + variant = t.split(|c: char| !c.is_alphanumeric()).next().unwrap_or(""); + continue; + } + let Some((name, ty)) = t.strip_prefix("pub ").unwrap_or(t).split_once(':') else { continue }; + let refers = (name.contains("sketch") && (ty.contains("u64") || ty.contains("SketchPoints"))) + || ty.contains("SketchPoints") + || ty.contains("LoftSection"); + if refers && !line.trim_start().starts_with("//") && !with_sketch.contains(&variant) { + with_sketch.push(variant); + } + } + assert!(with_sketch.len() >= 10, "the FeatureKind parse found too few sketch users: {with_sketch:?}"); + let f = src.split("pub fn input_sketches(&self)").nth(1).unwrap(); + let empty_arm = f.split("=> Vec::new()").next().and_then(|s| s.rsplit("=> ").next()).unwrap(); + for v in with_sketch { + assert!( + !empty_arm.contains(&format!("K::{v} {{")), + "FeatureKind::{v} has a sketch field but input_sketches says it uses no sketch: deleting the sketch would leave it broken" + ); + } +} diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 3c4a6714..3a06c2e6 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -839,6 +839,22 @@ fn pipe_scale_offset_bounding_and_materials() { assert!(s.execute("material.assign", &json!({"bodies": ["Cube"], "material": "unobtainium"})).is_err()); } +/// Regression: deleting a pipe's path sketch left the pipe in the timeline ("unknown sketch"). +#[test] +fn deleting_a_path_sketch_deletes_its_pipe() { + let mut s = Session::default(); + run(&mut s, "sketch.create", json!({"plane": "XY", "name": "Path"})); + run(&mut s, "sketch.line", json!({"points": [[0, 0], [30, 0]], "ids": ["l1"]})); + run(&mut s, "sketch.finish", json!({})); + let pipe = run(&mut s, "solid.pipe", json!({"path_sketch": "Path", "path": ["l1"], "diameter": 4})); + let deps = run(&mut s, "timeline.dependents", json!({"feature": "Path"})); + assert_eq!(deps["deleted_with_it"].as_array().map(Vec::len), Some(1), "{deps} (pipe {pipe})"); + let r = run(&mut s, "timeline.delete", json!({"features": ["Path"]})); + assert_eq!(r["deleted"].as_array().map(Vec::len), Some(2), "{r}"); + assert!(s.doc.features.is_empty(), "{:?}", s.doc.features.iter().map(|f| &f.name).collect::>()); + assert!(s.model.results.iter().all(|r| r.error.is_none())); +} + #[test] fn components_occurrences_and_world_placement() { let mut s = Session::default(); From ab23fc99061254e480bbb7840ff024bd32c908ef Mon Sep 17 00:00:00 2001 From: CypherNaugh_0x <9931495+CypherNaught-0x@users.noreply.github.com> Date: Sat, 10 Oct 2026 20:21:47 +0200 Subject: [PATCH 3/4] A failed MCP batch goes back to where it started however long the undo history is: rollback undid as many steps as the undo history had grown, but the history is capped at 200 steps, so once it was full a failed batch reported undone: 0 and left its sketches behind (an empty Sketch1, two sketches named TapHole). New commands edit.checkpoint and edit.restore_checkpoint (forget: true drops one) remember and restore the design, the active sketch and the undo history; batch takes a checkpoint before its first step, restores it on a failure and forgets it on success. Against an app without checkpoints batch falls back to counting undo steps --- crates/engine/src/cmd/edit.rs | 21 ++++++++++++ crates/engine/src/lib.rs | 58 +++++++++++++++++++++++++++++++ crates/engine/src/tests.rs | 29 ++++++++++++++++ crates/mcp/src/tests.rs | 29 ++++++++++++++++ crates/mcp/src/tools.rs | 64 +++++++++++++++++++++++++++++------ docs/mcp.md | 7 ++-- 6 files changed, 195 insertions(+), 13 deletions(-) diff --git a/crates/engine/src/cmd/edit.rs b/crates/engine/src/cmd/edit.rs index 08eb8695..cdfcd4cc 100644 --- a/crates/engine/src/cmd/edit.rs +++ b/crates/engine/src/cmd/edit.rs @@ -10,6 +10,12 @@ use crate::{EngineError, Result, Sel, Session, Snapshot}; pub static COMMANDS: &[CommandSpec] = &[ CommandSpec::new("edit.undo", "Undo", undo).icon("undo").key("Ctrl+Z").noundo(), CommandSpec::new("edit.redo", "Redo", redo).icon("redo").key("Ctrl+Y").noundo(), + CommandSpec::new("edit.checkpoint", "Checkpoint", checkpoint) + .noundo() + .params("→ {checkpoint: id}: remember the design, active sketch and undo history (restore with edit.restore_checkpoint)"), + CommandSpec::new("edit.restore_checkpoint", "Restore Checkpoint", restore_checkpoint) + .noundo() + .params("checkpoint: id; forget?: bool (true = drop it without going back). Goes back to it, undo history included"), CommandSpec::new("timeline.compute_all", "Compute All", compute_all).at("SOLID", "MODIFY").icon("compute").noundo(), CommandSpec::new("timeline.delete", "Delete", delete) .at("SOLID", "MODIFY") @@ -60,6 +66,21 @@ fn redo(s: &mut Session, _p: &Value) -> Result { Ok(json!({"redone": label})) } +fn checkpoint(s: &mut Session, _p: &Value) -> Result { + Ok(json!({"checkpoint": s.checkpoint()})) +} + +fn restore_checkpoint(s: &mut Session, p: &Value) -> Result { + let cmd = "edit.restore_checkpoint"; + let id = p.get("checkpoint").and_then(Value::as_u64).ok_or_else(|| bad(cmd, "`checkpoint` must be an id from edit.checkpoint"))?; + if bool_(p, "forget") == Some(true) { + s.drop_checkpoint(id); + return Ok(json!({"checkpoint": id, "restored": false})); + } + s.restore_checkpoint(id)?; + Ok(json!({"checkpoint": id, "restored": true})) +} + fn compute_all(s: &mut Session, _p: &Value) -> Result { s.model = solvecraft_doc::Model::new(); s.refresh(); diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index 2ce89495..22a5106f 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -170,6 +170,20 @@ pub struct Session { pub analysis: Option, /// What is hidden or shown in the view (`browser.visibility`). pub visibility: Visibility, + /// States to go back to (`edit.checkpoint`, `edit.restore_checkpoint`): how batches of + /// commands are undone however long the undo history is. + pub(crate) checkpoints: Vec, + next_checkpoint: u64, +} + +/// The design, active sketch and undo history at an `edit.checkpoint`. +#[derive(Clone, Debug)] +pub(crate) struct Checkpoint { + pub id: u64, + pub doc: Arc, + pub active_sketch: Option, + pub undo: Vec, + pub redo: Vec, } /// Items hidden or shown one by one (view state, not part of the design and not an undo step). @@ -204,6 +218,8 @@ pub enum SurfaceAnalysis { } const MAX_UNDO: usize = 200; +/// Checkpoints kept (the oldest go first). +const MAX_CHECKPOINTS: usize = 16; const MAX_LOG: usize = 2000; impl Default for Session { @@ -235,6 +251,8 @@ impl Session { clipboard: Vec::new(), section: None, analysis: None, + checkpoints: Vec::new(), + next_checkpoint: 1, visibility: Visibility::default(), } } @@ -356,6 +374,44 @@ impl Session { } } + /// Remember the design, active sketch and undo history; returns the checkpoint's id. + pub fn checkpoint(&mut self) -> u64 { + let id = self.next_checkpoint; + self.next_checkpoint += 1; + self.checkpoints.push(Checkpoint { + id, + doc: self.doc.clone(), + active_sketch: self.active_sketch, + undo: self.undo.clone(), + redo: self.redo.clone(), + }); + if self.checkpoints.len() > MAX_CHECKPOINTS { + self.checkpoints.remove(0); + } + id + } + + /// Go back to checkpoint `id` as if nothing since had happened (the undo history too). It + /// and later checkpoints are used up. + pub fn restore_checkpoint(&mut self, id: u64) -> Result<()> { + let i = self.checkpoints.iter().position(|c| c.id == id).ok_or_else(|| EngineError::Other(format!("no checkpoint {id}")))?; + let Some(c) = self.checkpoints.drain(i..).next() else { return Err(EngineError::Other(format!("no checkpoint {id}"))) }; + self.doc = c.doc; + self.active_sketch = c.active_sketch; + self.undo = c.undo; + self.redo = c.redo; + self.selection.clear(); + self.refresh(); + Ok(()) + } + + /// Forget checkpoint `id` (and later ones) without going back. + pub fn drop_checkpoint(&mut self, id: u64) { + if let Some(i) = self.checkpoints.iter().position(|c| c.id == id) { + self.checkpoints.truncate(i); + } + } + /// A scratch copy of the session (same design and model, no history or log) to try /// commands on without touching this one. pub fn scratch(&self) -> Session { @@ -377,6 +433,8 @@ impl Session { clipboard: self.clipboard.clone(), section: None, analysis: None, + checkpoints: Vec::new(), + next_checkpoint: 1, visibility: Visibility::default(), } } diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 3a06c2e6..2495c83c 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -855,6 +855,35 @@ fn deleting_a_path_sketch_deletes_its_pipe() { assert!(s.model.results.iter().all(|r| r.error.is_none())); } +/// Regression: batches were rolled back by undoing as many steps as the undo history grew, +/// which is none once the history is full, leaving their sketches behind. +#[test] +fn checkpoint_restores_with_a_full_undo_history() { + let mut s = Session::default(); + for i in 0..MAX_UNDO + 5 { + run(&mut s, "parameters.add", json!({"name": format!("p{i}"), "expression": "1 mm"})); + } + assert_eq!(s.undo.len(), MAX_UNDO); + let labels: Vec = s.undo.iter().map(|u| u.label.clone()).collect(); + let doc = s.doc.clone(); + let id = run(&mut s, "edit.checkpoint", json!({}))["checkpoint"].as_u64().unwrap(); + run(&mut s, "sketch.create", json!({"plane": "XY", "name": "TapHole"})); + run(&mut s, "sketch.circle.center", json!({"center": [0, 0], "radius": 2})); + assert!(s.active_sketch.is_some()); + run(&mut s, "edit.restore_checkpoint", json!({"checkpoint": id})); + assert!(Arc::ptr_eq(&s.doc, &doc) && s.active_sketch.is_none()); + assert!(s.doc.find_feature("TapHole").is_none()); + assert_eq!(s.undo.iter().map(|u| u.label.clone()).collect::>(), labels); + // Used up: a second restore is an error, not a jump somewhere else. + assert!(s.execute("edit.restore_checkpoint", &json!({"checkpoint": id})).is_err()); + // Forgetting one leaves the design alone. + let id = run(&mut s, "edit.checkpoint", json!({}))["checkpoint"].as_u64().unwrap(); + run(&mut s, "sketch.create", json!({"plane": "XY", "name": "Kept"})); + run(&mut s, "edit.restore_checkpoint", json!({"checkpoint": id, "forget": true})); + assert!(s.doc.find_feature("Kept").is_some()); + assert!(s.execute("edit.restore_checkpoint", &json!({"checkpoint": id})).is_err()); +} + #[test] fn components_occurrences_and_world_placement() { let mut s = Session::default(); diff --git a/crates/mcp/src/tests.rs b/crates/mcp/src/tests.rs index f050cd49..64aa12f8 100644 --- a/crates/mcp/src/tests.rs +++ b/crates/mcp/src/tests.rs @@ -213,6 +213,35 @@ fn batch_stops_at_first_error_and_rolls_back() { assert_eq!(tool(&mut s, "measure", json!({}))["body_count"], 2); } +/// Regression: with a full undo history a failed batch reported `undone: 0` and left its +/// sketches behind (an empty "Sketch1", two sketches named "TapHole"). +#[test] +fn batch_rolls_back_with_a_full_undo_history() { + let mut s = server(); + let fill: Vec = + (0..230).map(|i| json!({"command": "parameters.add", "params": {"name": format!("p{i}"), "expression": "1 mm"}})).collect(); + tool(&mut s, "batch", json!({"commands": fill})); + let undo = tool(&mut s, "inspect_design", json!({"measure": false}))["undo"].clone(); + for _ in 0..2 { + let r = raw_tool( + &mut s, + "batch", + json!({"commands": [ + {"command": "sketch.create", "params": {"plane": "XY", "name": "TapHole"}}, + {"command": "sketch.circle.center", "params": {"center": [0, 0], "radius": 2}}, + {"command": "sketch.dimension", "params": {"entities": ["nope"], "value": 3}}, + ]}), + ); + let body: Value = serde_json::from_str(r["result"]["content"][0]["text"].as_str().unwrap()).unwrap(); + assert_eq!(body["failed"]["index"], 2, "{body}"); + assert_eq!(body["undone"], 2, "{body}"); + } + let d = tool(&mut s, "inspect_design", json!({"measure": false, "sketches": false})); + assert_eq!(d["sketches"].as_array().map(Vec::len), Some(0), "{d}"); + assert!(d["active_sketch"].is_null(), "{d}"); + assert_eq!(d["undo"], undo); +} + #[test] fn helper_sketches_can_be_hidden() { let mut s = server(); diff --git a/crates/mcp/src/tools.rs b/crates/mcp/src/tools.rs index 0d1e993a..938e0de7 100644 --- a/crates/mcp/src/tools.rs +++ b/crates/mcp/src/tools.rs @@ -361,7 +361,56 @@ fn undo_depth(b: &mut dyn Backend) -> Option { b.call("document.inspect", json!({})).ok().and_then(|d| d["undo"].as_u64()) } -/// `batch`: run steps in order, stop at the first failure and (by default) undo what ran. +/// How a failed batch goes back to where it started. +enum Undo { + /// An engine checkpoint: the design, active sketch and undo history exactly as they were. + Checkpoint(u64), + /// An app without checkpoints: undo as many steps as the batch added (wrong once the undo + /// history is full, as its depth no longer grows). + Depth(u64), + None, +} + +impl Undo { + fn begin(b: &mut dyn Backend) -> Undo { + match exec(b, "edit.checkpoint", json!({})).ok().and_then(|v| v["checkpoint"].as_u64()) { + Some(id) => Undo::Checkpoint(id), + None => undo_depth(b).map_or(Undo::None, Undo::Depth), + } + } + + /// Go back; the number of steps undone. + fn rollback(self, b: &mut dyn Backend, steps: usize) -> u64 { + match self { + Undo::Checkpoint(id) => match exec(b, "edit.restore_checkpoint", json!({"checkpoint": id})) { + Ok(_) => steps as u64, + Err(_) => 0, + }, + Undo::Depth(before) => { + let after = undo_depth(b).unwrap_or(before); + let mut undone = 0u64; + for _ in before..after { + if exec(b, "edit.undo", json!({})).is_err() { + break; + } + undone += 1; + } + undone + } + Undo::None => 0, + } + } + + /// The batch went through: forget the checkpoint. + fn finish(self, b: &mut dyn Backend) { + if let Undo::Checkpoint(id) = self { + let _ = exec(b, "edit.restore_checkpoint", json!({"checkpoint": id, "forget": true})); + } + } +} + +/// `batch`: run steps in order, stop at the first failure and (by default) go back to the +/// design as it was before the batch. fn batch(b: &mut dyn Backend, a: &Map) -> Value { let Some(steps) = a.get("commands").and_then(Value::as_array) else { return text_result(&json!("`commands` must be an array of {command, params}"), true); @@ -370,7 +419,7 @@ fn batch(b: &mut dyn Backend, a: &Map) -> Value { return text_result(&json!(format!("batch too long ({} steps, at most {MAX_BATCH})", steps.len())), true); } let rollback = a.get("rollback").and_then(Value::as_bool).unwrap_or(true); - let before = if rollback { undo_depth(b) } else { None }; + let start = if rollback { Undo::begin(b) } else { Undo::None }; let mut results = Vec::with_capacity(steps.len()); for (i, step) in steps.iter().enumerate() { let command = step.get("command").or_else(|| step.get("id")).and_then(Value::as_str); @@ -381,15 +430,7 @@ fn batch(b: &mut dyn Backend, a: &Map) -> Value { match r { Ok(v) => results.push(v), Err(e) => { - let mut undone = 0u64; - if let (Some(before), Some(after)) = (before, rollback.then(|| undo_depth(b)).flatten()) { - for _ in before..after { - if exec(b, "edit.undo", json!({})).is_err() { - break; - } - undone += 1; - } - } + let undone = start.rollback(b, i); return text_result( &json!({ "ok": false, @@ -404,6 +445,7 @@ fn batch(b: &mut dyn Backend, a: &Map) -> Value { } } } + start.finish(b); text_result(&json!({"ok": true, "completed": results.len(), "results": results}), false) } diff --git a/docs/mcp.md b/docs/mcp.md index 3aec985e..63370d71 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -69,8 +69,11 @@ Errors: an unknown tool or arguments that don't match the tool's schema are JSON `-32602` errors; an engine failure (bad command parameters, a feature that fails) is a tool result with `isError: true` and the engine's message, which the model can read and correct. -`batch` rollback undoes as many steps as the batch added to the undo history; batches that -themselves call `edit.undo`, `file.new` or `doc.open` are not rolled back exactly. +`batch` rollback goes back to an engine checkpoint (`edit.checkpoint`) taken before the first +step: the design, the active sketch and the undo history are restored exactly as they were, +however long the history is. Batches that call `file.new` or `doc.open` get the design back, +not its file path. (An older app without checkpoints falls back to undoing as many steps as +the batch added to the undo history.) ## Demo: an enclosure built over MCP From 6f16b317710733d352255a7b40ed506d137ca253 Mon Sep 17 00:00:00 2001 From: bherbruck Date: Sat, 10 Oct 2026 17:28:12 -0400 Subject: [PATCH 4/4] Fix viewport input leaking through dialogs Use egui pointer ownership for viewport navigation and picking, view-cube interaction, and sketch-dimension hover. Cover scrolling, model selection, and cube interaction under floating dialogs, including after they close. Shield constraint glyph and move-gizmo highlights from overlapping UI too. Clear strict Clippy warnings and use the same saved path in the recent-file scenario on Windows and Unix. Fixes #87 --- crates/doc/src/project.rs | 2 +- crates/doc/src/sheet.rs | 2 +- crates/engine/src/cmd/sketch_modify.rs | 2 +- crates/sketch/src/model.rs | 12 +-- crates/sketch/src/text.rs | 2 +- crates/ui-egui/src/dialogs.rs | 2 +- crates/ui-egui/src/dim_view.rs | 2 +- crates/ui-egui/src/gizmo.rs | 2 +- crates/ui-egui/src/sketch_tools.rs | 2 +- crates/ui-egui/src/viewport.rs | 123 ++++++++++++++++++++++++- crates/ui-egui/tests/scenarios.rs | 4 +- xtask/src/layers.rs | 7 +- 12 files changed, 138 insertions(+), 24 deletions(-) diff --git a/crates/doc/src/project.rs b/crates/doc/src/project.rs index 6e42aa42..5378b1b8 100644 --- a/crates/doc/src/project.rs +++ b/crates/doc/src/project.rs @@ -485,7 +485,7 @@ fn tilted_circle(plane: &Plane, pts: &[Vec3]) -> Option> { let (c, n, r, closed) = circle3(pts)?; let sn = plane.normal(); let cosang = n.dot(sn).abs(); - if cosang > 1.0 - 1e-12 || cosang < 1e-9 { + if !(1e-9..=1.0 - 1e-12).contains(&cosang) { return None; } if closed { diff --git a/crates/doc/src/sheet.rs b/crates/doc/src/sheet.rs index cdbd6a91..6bee00c8 100644 --- a/crates/doc/src/sheet.rs +++ b/crates/doc/src/sheet.rs @@ -899,7 +899,7 @@ impl SheetBody { return Err(DocError::Invalid("the hem is shorter than its bend".into())); } // A hem folds back over the face it starts from (toward the bottom unless flipped). - let up = !top == flip; + let up = top != flip; let up = !up; self.flanges.push(SheetFlange { parent: panel, diff --git a/crates/engine/src/cmd/sketch_modify.rs b/crates/engine/src/cmd/sketch_modify.rs index 14d4b739..f79c3697 100644 --- a/crates/engine/src/cmd/sketch_modify.rs +++ b/crates/engine/src/cmd/sketch_modify.rs @@ -520,7 +520,7 @@ fn fillet_corner(sk: &mut Sketch, p: &Value, r: f64, cmd: &str) -> Result<(usize let u = (pa - pc).normalized().ok_or_else(|| bad(cmd, "degenerate line"))?; let v = (pb - pc).normalized().ok_or_else(|| bad(cmd, "degenerate line"))?; let half = 0.5 * u.cross(v).atan2(u.dot(v)).abs(); - if half < 1e-6 || half > std::f64::consts::FRAC_PI_2 - 1e-9 { + if !(1e-6..=std::f64::consts::FRAC_PI_2 - 1e-9).contains(&half) { return Err(bad(cmd, "the lines are parallel")); } let d = r / half.tan(); diff --git a/crates/sketch/src/model.rs b/crates/sketch/src/model.rs index d86535fb..2b5a0079 100644 --- a/crates/sketch/src/model.rs +++ b/crates/sketch/src/model.rs @@ -698,10 +698,8 @@ impl Sketch { | Symmetric { l, .. } | PointLineDistance { l, .. } | LinearDiameter { l, .. } => need_line(l)?, - ArcLength { c, .. } => { - if !matches!(self.curves.get(c).map(|c| &c.kind), Some(CurveKind::Arc { .. })) { - return Err(SketchError::WrongKind(name(c), "arc length needs an arc".into())); - } + ArcLength { c, .. } if !matches!(self.curves.get(c).map(|c| &c.kind), Some(CurveKind::Arc { .. })) => { + return Err(SketchError::WrongKind(name(c), "arc length needs an arc".into())); } Smooth { a, b } => { let ends = |i: usize| self.curves.get(i).and_then(|c| c.kind.ends()); @@ -730,10 +728,8 @@ impl Sketch { return Err(SketchError::WrongKind(name(a), "tangent needs a circle or arc".into())); } } - Equal { a, b } => { - if is_line(a) != is_line(b) { - return Err(SketchError::WrongKind(name(b), "equal needs two lines or two circles/arcs".into())); - } + Equal { a, b } if is_line(a) != is_line(b) => { + return Err(SketchError::WrongKind(name(b), "equal needs two lines or two circles/arcs".into())); } Coincident { p, q } if p == q => return Err(SketchError::Invalid("a point is always coincident with itself".into())), _ => {} diff --git a/crates/sketch/src/text.rs b/crates/sketch/src/text.rs index f0f378b0..f1c9d439 100644 --- a/crates/sketch/src/text.rs +++ b/crates/sketch/src/text.rs @@ -74,7 +74,7 @@ pub fn text_geometry(text: &str, at: Vec2, height: f64, angle: f64) -> Result MAX_TEXT_CHARS { return Err(SketchError::TooLarge); } - if !(height.is_finite() && height > 1e-6 && height < 1e6) || !angle.is_finite() || !at.is_finite() { + if !(height.is_finite() && height > 1e-6 && height < 1e6 && angle.is_finite() && at.is_finite()) { return Err(SketchError::Invalid("text height must be positive".into())); } let face = ttf_parser::Face::parse(FONT, 0).map_err(|e| SketchError::Invalid(format!("font: {e}")))?; diff --git a/crates/ui-egui/src/dialogs.rs b/crates/ui-egui/src/dialogs.rs index 7f3f83ef..27912c30 100644 --- a/crates/ui-egui/src/dialogs.rs +++ b/crates/ui-egui/src/dialogs.rs @@ -1855,7 +1855,7 @@ pub fn show(app: &mut SolveApp, ctx: &egui::Context) { Err(e) => d.error = Some(e), } } - if !(keep && !cancel) { + if !keep || cancel { crate::dialogs_motion::closed(app, &mut d, applied); } if keep && !cancel { diff --git a/crates/ui-egui/src/dim_view.rs b/crates/ui-egui/src/dim_view.rs index 66382a4c..105529eb 100644 --- a/crates/ui-egui/src/dim_view.rs +++ b/crates/ui-egui/src/dim_view.rs @@ -187,7 +187,7 @@ pub fn show(app: &mut SolveApp, ui: &egui::Ui, painter: &egui::Painter, proj: &P } let tk = Tokens::get(); let is_sel = |id: &str| app.session.selection.iter().any(|s| matches!(s, Sel::SketchConstraint { id: x } if x == id)); - let hover = ui.input(|i| i.pointer.hover_pos()).filter(|p| proj.rect.contains(*p)).and_then(hit); + let hover = ui.input(|i| i.pointer.hover_pos()).filter(|_| ui.rect_contains_pointer(proj.rect)).and_then(hit); let to = |p: Vec2| proj.to_screen(ss.plane.to_world(p)); let font = FontId::proportional(12.0); let centres = chain_centres(sk); diff --git a/crates/ui-egui/src/gizmo.rs b/crates/ui-egui/src/gizmo.rs index 5f1b1eb1..8c211e6a 100644 --- a/crates/ui-egui/src/gizmo.rs +++ b/crates/ui-egui/src/gizmo.rs @@ -255,7 +255,7 @@ pub fn show(ui: &mut egui::Ui, painter: &egui::Painter, proj: &Proj, id: egui::I } let state: Option = ui.data(|d| d.get_temp(id)); let pointer = ui.input(|i| i.pointer.hover_pos()); - let hovered = if state.is_none() { pointer.filter(|p| proj.rect.contains(*p)).and_then(|p| hit(&sh, p)) } else { None }; + let hovered = if state.is_none() { pointer.filter(|_| ui.rect_contains_pointer(proj.rect)).and_then(|p| hit(&sh, p)) } else { None }; let active = state.map(|s| s.handle).or(hovered); let hot = |h: Handle| active == Some(h); let col = |h: Handle, i: usize| if hot(h) { tk.accent } else { axis_color(i) }; diff --git a/crates/ui-egui/src/sketch_tools.rs b/crates/ui-egui/src/sketch_tools.rs index 2f87a9fc..39f02e8f 100644 --- a/crates/ui-egui/src/sketch_tools.rs +++ b/crates/ui-egui/src/sketch_tools.rs @@ -1090,7 +1090,7 @@ fn snap_hint(app: &SolveApp, painter: &egui::Painter, proj: &Proj) { /// Per frame, after the viewport is drawn: constraint glyphs and the text-entry box. pub fn show(app: &mut SolveApp, ui: &egui::Ui, painter: &egui::Painter, proj: &Proj) { SHIFT.with(|c| c.set(ui.input(|i| i.modifiers.shift))); - glyphs(app, painter, proj, ui.input(|i| i.pointer.hover_pos())); + glyphs(app, painter, proj, ui.input(|i| i.pointer.hover_pos()).filter(|_| ui.rect_contains_pointer(proj.rect))); snap_hint(app, painter, proj); combs(app, painter, proj); if DEFERRED.with(|d| d.replace(false)) { diff --git a/crates/ui-egui/src/viewport.rs b/crates/ui-egui/src/viewport.rs index d436dd90..0808c9dd 100644 --- a/crates/ui-egui/src/viewport.rs +++ b/crates/ui-egui/src/viewport.rs @@ -1371,7 +1371,8 @@ pub fn show(app: &mut SolveApp, ui: &mut egui::Ui) { let secondary_down = secondary_down && app.viewport.context_menu.is_none(); // The view cube handles its own clicks; the model underneath must not see them. let cube = Rect::from_center_size(pos2(rect.right() - 80.0, rect.top() + 80.0), vec2(150.0, 150.0)); - let inside = hover.is_some_and(|p| rect.contains(p) && !cube.contains(p)); + // Floating windows and menus own input over the model, including wheel navigation. + let inside = resp.contains_pointer() && hover.is_some_and(|p| !cube.contains(p)); app.viewport.mouse = hover.filter(|_| inside); // Ctrl or Alt held: drawing and dragging don't snap. crate::drag_snap::set_suppressed(mods.ctrl || mods.alt || mods.command); @@ -2086,7 +2087,7 @@ fn view_cube(app: &mut SolveApp, ui: &mut egui::Ui, rect: Rect) { let (r, u, b) = cube_basis(&app.cam); let (wr, wu, _) = app.cam.basis(); let painter = ui.painter_at(rect); - let hover = ui.input(|i| i.pointer.hover_pos()); + let hover = ui.input(|i| i.pointer.hover_pos()).filter(|_| ui.rect_contains_pointer(rect)); let targets = cube_targets(); let shapes = cube_projected(&targets, (r, u, b), c); let hovered = hover.and_then(|h| cube_hit(&shapes, h)); @@ -2534,6 +2535,124 @@ mod tests { } } + #[test] + fn shortcuts_window_wheel_does_not_zoom_the_viewport() { + let ctx = egui::Context::default(); + let mut app = SolveApp::new(solvecraft_engine::Session::default(), crate::Services::default()); + app.viewport.no_pixels = true; + crate::help::run(&mut app, &ctx, "shortcuts"); + let mut time = 0.0; + let mut frame = |app: &mut SolveApp, pointer: Option, wheel: f32| { + time += 1.0 / 60.0; + let mut events = Vec::new(); + if let Some(p) = pointer { + events.push(egui::Event::PointerMoved(p)); + } + if wheel != 0.0 { + events.push(egui::Event::MouseWheel { + unit: egui::MouseWheelUnit::Point, + delta: vec2(0.0, wheel), + modifiers: egui::Modifiers::NONE, + phase: egui::TouchPhase::Move, + }); + } + let _ = ctx.run_ui( + egui::RawInput { + screen_rect: Some(Rect::from_min_size(Pos2::ZERO, vec2(1100.0, 800.0))), + time: Some(time), + events, + ..Default::default() + }, + |ui| { + egui::CentralPanel::default().show(ui, |ui| show(app, ui)); + crate::prefs::show(app, ui.ctx()); + }, + ); + }; + // Let egui establish the floating window's size and hit-test widgets. + for _ in 0..4 { + frame(&mut app, None, 0.0); + } + let window = ctx.memory(|m| m.area_rect(egui::Id::new("sc_prefs"))).unwrap(); + let list = window.center() + vec2(140.0, 0.0); + let height = app.cam.half_height(); + let target = app.cam.target; + frame(&mut app, Some(list), 0.0); + for n in 0..12 { + frame(&mut app, Some(list), if n == 0 { -120.0 } else { 0.0 }); + assert_eq!(app.cam.half_height(), height, "wheel input over shortcuts must stay in the popup"); + assert_eq!(app.cam.target, target); + } + // The window's non-scrolling sidebar must also shield the model. + frame(&mut app, Some(window.left_center() + vec2(30.0, 0.0)), -120.0); + assert_eq!(app.cam.half_height(), height); + // An exposed part of the viewport still accepts wheel navigation with the popup open. + let exposed = pos2(40.0, 700.0); + assert!(!window.contains(exposed)); + frame(&mut app, Some(exposed), 0.0); + frame(&mut app, Some(exposed), -120.0); + assert_ne!(app.cam.half_height(), height); + // Closing the popup returns its former area to the viewport. + app.prefs_window.open = false; + for _ in 0..12 { + frame(&mut app, Some(list), 0.0); + } + let height = app.cam.half_height(); + frame(&mut app, Some(list), -120.0); + assert_ne!(app.cam.half_height(), height); + } + + #[test] + fn floating_dialog_blocks_viewport_hover_and_clicks() { + let mut h = crate::scenario::Harness::new(); + h.call("engine.execute", json!({"command": "solid.box", "params": {"length": 40, "width": 30, "height": 20}})); + h.call("ui.view", json!({"view": "fit"})); + h.app.ui.pick_bodies = true; + let p = h.app.viewport.rect.unwrap().center(); + h.call("ui.move", json!({"x": p.x, "y": p.y})); + assert!(h.app.viewport.hover.is_some(), "the test point must hover over the model"); + h.call("ui.click", json!({"x": p.x, "y": p.y})); + assert!(!h.app.session.selection.is_empty(), "the uncovered model must be selectable"); + h.app.session.selection.clear(); + + h.call("ui.prefs", json!({"open": "Navigation"})); + let window = h.ctx.memory(|m| m.area_rect(egui::Id::new("sc_prefs"))).unwrap(); + assert!(window.contains(p), "the dialog must cover the model at the test point"); + h.call("ui.move", json!({"x": p.x, "y": p.y})); + assert!(h.app.viewport.hover.is_none(), "the dialog must block model highlights"); + assert!(h.app.viewport.mouse.is_none()); + h.call("ui.click", json!({"x": p.x, "y": p.y})); + assert!(h.app.session.selection.is_empty(), "clicks in the dialog must not select the model"); + + h.call("ui.prefs", json!({"button": "cancel"})); + h.call("ui.move", json!({"x": p.x, "y": p.y})); + assert!(h.app.viewport.hover.is_some(), "closing the dialog must restore model highlights"); + h.call("ui.click", json!({"x": p.x, "y": p.y})); + assert!(!h.app.session.selection.is_empty()); + } + + #[test] + fn floating_dialog_blocks_view_cube_highlights_and_clicks() { + let mut h = crate::scenario::Harness::new(); + h.call("ui.resize", json!({"width": 800, "height": 700})); + let p = crate::scenario::handle_at("cube:top").unwrap(); + h.call("ui.move", json!({"x": p.x, "y": p.y})); + assert_eq!(crate::scenario::count_of("cube_highlights"), Some(1.0)); + h.call("ui.prefs", json!({"open": "Navigation"})); + let window = h.ctx.memory(|m| m.area_rect(egui::Id::new("sc_prefs"))).unwrap(); + assert!(window.contains(p), "the floating dialog must cover the cube target"); + let camera = serde_json::to_value(h.app.cam).unwrap(); + h.call("ui.move", json!({"x": p.x, "y": p.y})); + assert_eq!(crate::scenario::count_of("cube_highlights"), Some(0.0)); + h.call("ui.click", json!({"x": p.x, "y": p.y})); + assert_eq!(serde_json::to_value(h.app.cam).unwrap(), camera, "dialog clicks must not turn the view cube"); + h.call("ui.prefs", json!({"button": "cancel"})); + h.call("ui.move", json!({"x": p.x, "y": p.y})); + assert_eq!(crate::scenario::count_of("cube_highlights"), Some(1.0)); + h.call("ui.click", json!({"x": p.x, "y": p.y})); + assert_ne!(serde_json::to_value(h.app.cam).unwrap(), camera); + } + #[test] fn section_outlines_follow_the_cut() { let mut s = solvecraft_engine::Session::default(); diff --git a/crates/ui-egui/tests/scenarios.rs b/crates/ui-egui/tests/scenarios.rs index f0ef2c06..0574bb20 100644 --- a/crates/ui-egui/tests/scenarios.rs +++ b/crates/ui-egui/tests/scenarios.rs @@ -70,9 +70,10 @@ fn angle_dimension_both_interactive_workflows() { fn home_tab_and_file_recent_open_designs() { use serde_json::json; let mut h = Harness::new(); + let path = h.dir.join("recent.solvecraft").to_string_lossy().into_owned(); h.run(&json!([ {"call":"engine.execute","params":{"command":"solid.box","params":{"length":10,"width":10,"height":10}}}, - {"call":"engine.execute","params":{"command":"file.save_as","params":{"path":"$DIR/recent.solvecraft"}}}, + {"call":"engine.execute","params":{"command":"file.save_as","params":{"path":path}}}, {"click":{"handle":"tab:home"}}, {"expect":{"home":true,"documents":1}}, {"click":{"handle":"tab:design:0"}}, @@ -84,7 +85,6 @@ fn home_tab_and_file_recent_open_designs() { {"widget":"Open Recent"} ])) .unwrap(); - let path = h.dir.join("recent.solvecraft").to_string_lossy().into_owned(); h.run(&json!([ {"widget":path}, {"expect":{"home":false,"bodies":1,"documents":3}} diff --git a/xtask/src/layers.rs b/xtask/src/layers.rs index 0a65c1e8..29abb446 100644 --- a/xtask/src/layers.rs +++ b/xtask/src/layers.rs @@ -163,11 +163,10 @@ pub fn check(crates: &[Crate]) -> Vec { match classify(&d.name) { // Unregistered deps are reported on their own entry. None => {} - Some(Class::Testkit) => { - if d.kind != DepKind::Dev { - out.push(Violation::TestkitAsNormalDep { krate: c.name.clone() }); - } + Some(Class::Testkit) if d.kind != DepKind::Dev => { + out.push(Violation::TestkitAsNormalDep { krate: c.name.clone() }); } + Some(Class::Testkit) => {} Some(dc) => { let to = dc.layer().unwrap_or(u8::MAX); if to >= layer && !(to == layer && intra_layer_allowed(&c.name, &d.name)) {