From d019f2d5bfbe740b1d66222498d061d8602a04bd Mon Sep 17 00:00:00 2001 From: bherbruck Date: Wed, 14 Oct 2026 10:51:07 -0400 Subject: [PATCH] Straight edges stay one edge after cuts (#1049) (#1101) * Straight edges stay one edge after cuts (#1049) Co-Authored-By: Claude Opus 5.5 * Robustness goldens: cuts join straight edges along untouched walls (#1049) Co-Authored-By: Claude Opus 5.5 * fmt Co-Authored-By: Claude Opus 5.5 * Robustness goldens note: what each changed seed shows when replayed on both builds Co-Authored-By: Claude Opus 5.5 --------- Co-authored-by: Claude Opus 5.5 --- .../tests/straight_edges_after_cuts.rs | 88 ++++++++++++++++++ crates/kernel/src/heal.rs | 21 +++-- crates/kernel/src/untouched_faces_tests.rs | 63 +++++++++++++ crates/sketch/src/link.rs | 41 +++++++- crates/sketch/src/tests.rs | 38 ++++++++ .../designs/crazy-geom-cuts.solidcraft | Bin 0 -> 3539 bytes tests/robustness/goldens.json | 61 ++++++------ 7 files changed, 271 insertions(+), 41 deletions(-) create mode 100644 apps/solidcraft-cli/tests/straight_edges_after_cuts.rs create mode 100644 tests/action_matrix/designs/crazy-geom-cuts.solidcraft diff --git a/apps/solidcraft-cli/tests/straight_edges_after_cuts.rs b/apps/solidcraft-cli/tests/straight_edges_after_cuts.rs new file mode 100644 index 00000000..a52a24a1 --- /dev/null +++ b/apps/solidcraft-cli/tests/straight_edges_after_cuts.rs @@ -0,0 +1,88 @@ +//! Straight edges stay one edge after many cuts (#1049). The owner's design +//! (`tests/action_matrix/designs/crazy-geom-cuts.solidcraft`, saved by 0.10.0): five extrudes, +//! joins and cuts, over four sketches, three of them on faces of the body with the faces' +//! edges projected. Cuts used to leave the straight edges of the walls they sliced split at +//! vertices nothing else used, so an edge that is one straight line was drawn (and lit on hover) +//! as many pieces, and a sketch on such a face projected every piece. + +use std::collections::HashMap; +use std::path::{Path, PathBuf}; + +use serde_json::json; +use solidcraft_engine::Session; +use solidcraft_engine::doc::kernel::{FaceSurface, face_surface}; +use solidcraft_engine::geom::Vec3; + +fn design() -> PathBuf { + Path::new(env!("CARGO_MANIFEST_DIR")).join("../../tests/action_matrix/designs/crazy-geom-cuts.solidcraft") +} + +fn open(path: &Path) -> Session { + let mut s = Session::default(); + s.execute("doc.open", &json!({"path": path.to_string_lossy()})).unwrap_or_else(|e| panic!("{}: {e}", path.display())); + s +} + +/// The design built from scratch: saved without cached geometry and opened again. +fn rebuilt() -> Session { + let s = open(&design()); + let dir = std::env::temp_dir().join(format!("solidcraft-straight-edges-{}", std::process::id())); + std::fs::create_dir_all(&dir).unwrap(); + let path = dir.join("crazy-geom-cuts.json"); + let mut t = s.scratch(); + t.execute("file.save", &json!({"path": path.to_string_lossy(), "format": "json"})).unwrap(); + let out = open(&path); + let _ = std::fs::remove_dir_all(&dir); + out +} + +fn errors(s: &Session) -> Vec<(u64, String)> { + s.model.results.iter().filter_map(|r| r.error.clone().map(|e| (r.id, e))).collect() +} + +fn key(v: Vec3) -> [i64; 3] { + [v.x, v.y, v.z].map(|c| (c * 1e5).round() as i64) +} + +#[test] +fn straight_edges_are_one_edge_after_many_cuts() { + for (what, s) in [("as saved", open(&design())), ("built from scratch", rebuilt())] { + assert_eq!(errors(&s), vec![], "{what}: every feature evaluates"); + let st = s.world_state(); + let bodies: Vec<_> = st.bodies.iter().filter(|b| !b.body.is_mesh()).collect(); + assert_eq!(bodies.len(), 1, "{what}: one body"); + let body = &bodies[0].body; + let mesh = body.tessellate(0.01).unwrap(); + // Every edge is drawn as one polyline; a straight one as one segment. + let mut at: HashMap<[i64; 3], Vec> = HashMap::new(); + for (i, e) in mesh.edges.iter().enumerate() { + let (Some(a), Some(z)) = (e.first(), e.last()) else { panic!("{what}: edge {i} is empty") }; + at.entry(key(*a)).or_default().push(i); + at.entry(key(*z)).or_default().push(i); + } + let straight = |i: usize| mesh.edges.get(i).filter(|e| e.len() == 2).and_then(|e| (e[1] - e[0]).normalized()); + let split: Vec<_> = at + .iter() + .filter(|(_, es)| match es.as_slice() { + [a, b] => straight(*a).zip(straight(*b)).is_some_and(|(u, w)| u.dot(w).abs() > 1. - 1e-9), + _ => false, + }) + .map(|(k, _)| k.map(|c| c as f64 / 1e5)) + .collect(); + assert_eq!(split, Vec::<[f64; 3]>::new(), "{what}: straight edges split where nothing else meets them"); + // No two faces side by side in one plane. + let plane = |f: usize| match face_surface(body, f) { + Some(FaceSurface::Plane { normal, point }) => Some((normal, normal.dot(point))), + _ => None, + }; + for (i, faces) in mesh.edge_faces.iter().enumerate() { + if let [a, b] = faces.as_slice() + && a != b + && let (Some((na, da)), Some((nb, db))) = (plane(*a as usize), plane(*b as usize)) + { + assert!(na.dot(nb) < 1. - 1e-9 || (da - db).abs() > 1e-6, "{what}: faces {a} and {b} meet at edge {i} in one plane"); + } + } + assert_eq!((body.face_count(), body.edge_count(), body.vertex_count()), (35, 107, 71), "{what}: faces, edges and vertices"); + } +} diff --git a/crates/kernel/src/heal.rs b/crates/kernel/src/heal.rs index 20a93e58..ee1b4259 100644 --- a/crates/kernel/src/heal.rs +++ b/crates/kernel/src/heal.rs @@ -318,12 +318,13 @@ fn merge_collinear(shell: &mt::Shell, trace: &mut Trace) -> Option { es.push(e.clone()); continue; }; - // The run's pieces become its joined edge, in this wire's direction. - if done.insert(r) - && let Some((line, pos)) = joined.get(r) - { + // The run's pieces become its joined edge, in this wire's direction: once each + // way, as a seam runs along its wire both ways. + if let Some((line, pos)) = joined.get(r) { let forward = pos.get(&e.front().id()) < pos.get(&e.back().id()); - es.push(if forward { line.clone() } else { line.inverse() }); + if done.insert((r, forward)) { + es.push(if forward { line.clone() } else { line.inverse() }); + } } } wires.push(mt::Wire::from(es)); @@ -547,9 +548,9 @@ pub(crate) fn heal_traced(solid: Solid, size: f64, keep: &[Vec]) -> (Solid healed(solid, size, keep, true, false) } -/// [`heal_traced`] that only merges coplanar neighbouring faces, leaving every edge, curve and -/// surface as it is: for results whose geometry is already exact (a prism boolean's), where -/// joining edges would change more than the faces split in pieces. +/// [`heal_traced`] that only merges coplanar neighbouring faces and joins the straight edges +/// they leave split in line (#1049), leaving every curve and surface as it is: for results +/// whose geometry is already exact (a prism boolean's). pub(crate) fn merge_faces_traced(solid: Solid, size: f64, keep: &[Vec]) -> (Solid, Option) { healed(solid, size, keep, true, true) } @@ -632,7 +633,9 @@ fn heal_shells(solid: &Solid, size: f64, keep: &[Vec], traced: bool, faces from = groups.iter().map(|g| g.iter().filter_map(|i| from.get(*i)).flatten().copied().collect()).collect(); changed = true; } - if !faces_only && let Some(m) = merge_collinear(&cur, &mut trace) { + // Straight edges the merged pieces leave split at vertices nothing else uses join + // into one, after any boolean (#1049): later features meet one edge, not the pieces. + if let Some(m) = merge_collinear(&cur, &mut trace) { cur = m; changed = true; } diff --git a/crates/kernel/src/untouched_faces_tests.rs b/crates/kernel/src/untouched_faces_tests.rs index 39737565..a1e06bd0 100644 --- a/crates/kernel/src/untouched_faces_tests.rs +++ b/crates/kernel/src/untouched_faces_tests.rs @@ -148,3 +148,66 @@ fn random_cuts_and_joins_never_split_other_faces() { } } } + +/// The edges of `b` lying along the line through `a` and `z`. +fn edges_along(b: &Body, a: Vec3, z: Vec3) -> usize { + let on = |p: Vec3| p.dist_to_segment(a, z) < 1e-6; + let edges: std::collections::HashSet<_> = b + .solid + .edge_iter() + .filter(|e| on(crate::body::from_p3(e.front().point())) && on(crate::body::from_p3(e.back().point()))) + .map(|e| e.id()) + .collect(); + edges.len() +} + +/// Vertices of `b` where just two straight edges meet in line (a straight edge split in two). +fn stations(b: &Body) -> usize { + use std::collections::HashMap; + let mut at: HashMap> = HashMap::new(); + let mut seen = std::collections::HashSet::new(); + for e in b.solid.edge_iter() { + if seen.insert(e.id()) { + for v in [e.front(), e.back()] { + at.entry(v.id()).or_default().push(e.clone()); + } + } + } + at.values() + .filter(|es| match es.as_slice() { + [a, b] => { + let dir = |e: &truck_modeling::Edge| (crate::body::from_p3(e.back().point()) - crate::body::from_p3(e.front().point())).normalized(); + matches!((a.curve(), b.curve()), (truck_modeling::Curve::Line(_), truck_modeling::Curve::Line(_))) + && dir(a).zip(dir(b)).is_some_and(|(u, w)| u.dot(w).abs() > 1. - 1e-9) + } + _ => false, + }) + .count() +} + +/// Cuts at several heights through a block's walls leave no vertices along its straight edges +/// (#1049): the edges they never touch stay one edge each, and so does an edge a later join +/// lies beside, instead of being drawn (and met by later features) as many pieces. +#[test] +fn cuts_leave_straight_edges_whole() { + let mut b = box_solid(Vec3::new(0., 0., 0.), Vec3::new(40., 20., 10.)).unwrap().with_box_certificate(7).unwrap(); + // Notches through the front wall at different heights, a pocket in the top and a slot + // through the end: each cut is sliced at its own heights. + b = cut(&b, Vec3::new(5., -1., 2.), Vec3::new(10., 3., 4.)); + b = cut(&b, Vec3::new(15., -1., 5.), Vec3::new(20., 3., 8.)); + b = cut(&b, Vec3::new(25., 8., 6.), Vec3::new(30., 12., 11.)); + b = cut(&b, Vec3::new(37., 5., 3.), Vec3::new(41., 15., 7.)); + assert!(b.history().is_some(), "the cuts report their history"); + assert_eq!(stations(&b), 0, "a straight edge is split at a vertex nothing else uses"); + // The block's four upright edges and the top's far edges are one edge each. + for (x, y) in [(0., 0.), (40., 0.), (0., 20.), (40., 20.)] { + assert_eq!(edges_along(&b, Vec3::new(x, y, 0.), Vec3::new(x, y, 10.)), 1, "upright edge at ({x}, {y})"); + } + assert_eq!(edges_along(&b, Vec3::new(0., 20., 10.), Vec3::new(40., 20., 10.)), 1, "the top's back edge"); + assert_eq!(edges_along(&b, Vec3::new(0., 0., 0.), Vec3::new(0., 20., 0.)), 1, "the bottom's left edge"); + // A boss joined on the top beside its back edge, then another cut: still one edge. + b = boolean(&b, &box_solid(Vec3::new(2., 15., 10.), Vec3::new(8., 20., 14.)).unwrap(), BoolOp::Union).unwrap().unwrap(); + b = cut(&b, Vec3::new(12., 4., 1.), Vec3::new(14., 21., 9.)); + assert_eq!(stations(&b), 0, "after a join and another cut"); + assert_eq!(edges_along(&b, Vec3::new(0., 0., 0.), Vec3::new(0., 0., 10.)), 1, "upright edge after a join and a cut"); +} diff --git a/crates/sketch/src/link.rs b/crates/sketch/src/link.rs index 89bfb1cf..005578d4 100644 --- a/crates/sketch/src/link.rs +++ b/crates/sketch/src/link.rs @@ -356,6 +356,44 @@ fn same_geometry(a: &[(LCurve, Vec)], b: &[(LCurve, Vec)]) -> bool { } /// The point indices a layout curve uses, in order. +/// Is `fresh` the boundary `old` was, with only the stations along its straight lines gone +/// (#1049: straight edges that booleans left split at vertices nothing else used are now one +/// edge)? Every old line lies along exactly one fresh line and the old pieces cover it end to +/// end; every other curve is unchanged. A link saved with the stations keeps its pieces then: +/// the region they bound is the same, and the features that name them by id still find them. +fn only_stations_gone(old: &[(LCurve, Vec)], fresh: &[(LCurve, Vec)]) -> bool { + const TOL: f64 = 1e-6; + let is_line = |c: &&(LCurve, Vec)| matches!(c.0, LCurve::Line(..)); + let ends = |c: &(LCurve, Vec)| match c.1.as_slice() { + [a, b] => Some((*a, *b)), + _ => None, + }; + let (old_lines, old_rest): (Vec<_>, Vec<_>) = old.iter().partition(is_line); + let (fresh_lines, fresh_rest): (Vec<_>, Vec<_>) = fresh.iter().partition(is_line); + if fresh_lines.len() >= old_lines.len() || old_lines.len() > 512 { + return false; + } + let rest = |xs: Vec<&(LCurve, Vec)>| xs.into_iter().cloned().collect::>(); + if !same_geometry(&rest(old_rest), &rest(fresh_rest)) { + return false; + } + let Some(fresh_segs) = fresh_lines.iter().map(|c| ends(c)).collect::>>() else { return false }; + let mut covered = vec![0.0; fresh_segs.len()]; + for c in &old_lines { + let Some((p, q)) = ends(c) else { return false }; + let on = |x: Vec2, (a, b): (Vec2, Vec2)| { + let ab = b - a; + let t = ((x - a).dot(ab) / ab.dot(ab).max(f64::MIN_POSITIVE)).clamp(0.0, 1.0); + x.dist(a + ab * t) <= TOL * (1.0 + a.len().max(b.len())) + }; + let Some(k) = fresh_segs.iter().position(|seg| on(p, *seg) && on(q, *seg)) else { return false }; + if let Some(slot) = covered.get_mut(k) { + *slot += p.dist(q); + } + } + fresh_segs.iter().zip(&covered).all(|((a, b), len)| (a.dist(*b) - len).abs() <= TOL * (1.0 + a.len().max(b.len()))) +} + fn curve_points(c: &LCurve) -> Vec { match *c { LCurve::Line(a, b) => vec![a, b], @@ -677,9 +715,8 @@ impl Sketch { let lone_old: Vec = points.iter().filter_map(|pi| pos(*pi)).collect(); if let (Some(old), Some(fresh)) = (old, fresh) && lay.lone.is_empty() - && points.len() == lay.pts.len() && lone_old.len() == points.len() - && same_geometry(&old, &fresh) + && (points.len() == lay.pts.len() && same_geometry(&old, &fresh) || only_stations_gone(&old, &fresh)) { return Ok(false); } diff --git a/crates/sketch/src/tests.rs b/crates/sketch/src/tests.rs index 0435ec8e..9278b4fd 100644 --- a/crates/sketch/src/tests.rs +++ b/crates/sketch/src/tests.rs @@ -883,3 +883,41 @@ fn exact_profiles_for_high_degree_and_cut_free_form_curves() { let uncut: f64 = find_profiles(&sk).iter().map(|p| p.area).sum(); assert!((whole - uncut).abs() < 1e-6 * uncut, "{whole} vs {uncut}"); } + +/// A face boundary saved with stations along its straight edges (#1049: booleans used to leave +/// vertices nothing else used) keeps its saved pieces when the face comes back with each +/// straight edge whole: the same region, and features that name the pieces still find them. +#[test] +fn projected_boundary_keeps_its_pieces_when_only_stations_go() { + use crate::{LinkGeom, LinkKind, LinkSource}; + let mut sk = Sketch::new(); + let v = Vec2::new; + let split = vec![ + LinkGeom::Line(v(0.0, 0.0), v(5.0, 0.0)), + LinkGeom::Line(v(5.0, 0.0), v(10.0, 0.0)), + LinkGeom::Line(v(10.0, 0.0), v(10.0, 4.0)), + LinkGeom::Line(v(10.0, 4.0), v(10.0, 10.0)), + LinkGeom::Line(v(10.0, 10.0), v(0.0, 10.0)), + LinkGeom::Line(v(0.0, 10.0), v(0.0, 0.0)), + ]; + let j = sk.add_link(LinkKind::Project, LinkSource::Origin, &split).unwrap(); + let ids: Vec = sk.link_curves(&j).iter().map(|c| sk.curves[*c].id.clone()).collect(); + let whole = vec![ + LinkGeom::Line(v(0.0, 0.0), v(10.0, 0.0)), + LinkGeom::Line(v(10.0, 0.0), v(10.0, 10.0)), + LinkGeom::Line(v(10.0, 10.0), v(0.0, 10.0)), + LinkGeom::Line(v(0.0, 10.0), v(0.0, 0.0)), + ]; + assert!(!sk.update_link(&j, &whole).unwrap(), "only the stations went: the pieces stay"); + let kept: Vec = sk.link_curves(&j).iter().map(|c| sk.curves[*c].id.clone()).collect(); + assert_eq!(ids, kept); + // A boundary that really changed is rebuilt. + let smaller = vec![ + LinkGeom::Line(v(0.0, 0.0), v(8.0, 0.0)), + LinkGeom::Line(v(8.0, 0.0), v(8.0, 10.0)), + LinkGeom::Line(v(8.0, 10.0), v(0.0, 10.0)), + LinkGeom::Line(v(0.0, 10.0), v(0.0, 0.0)), + ]; + assert!(sk.update_link(&j, &smaller).unwrap()); + assert_eq!(sk.link_curves(&j).len(), 4); +} diff --git a/tests/action_matrix/designs/crazy-geom-cuts.solidcraft b/tests/action_matrix/designs/crazy-geom-cuts.solidcraft new file mode 100644 index 0000000000000000000000000000000000000000..f8dd8cda078cb834cf1d7c7147ecb420512a6f98 GIT binary patch literal 3539 zcmZ`+c~}!h7SA2Xfq?`B1WgFR07(sJ5k(WinGg*}B610YGgYn-rL<}eiQJ7CP!KU- zA~!063$9y1p+XD@6i^Y{pnw&Riq%zF+uhjxzJA~Sv2VWjX1wvM)?|{me0Yyy&&ozpnCJ7zH%*cqnS)GK$XC!WT%sCna zQx2s<%xoNcGZ%nX}XrpfYg8MUI$_~!9>*d{?*xZ9$JWJF)t+lksjfL4iMq3QEB^MaD*v5)f15mq4^Mjn2+wIOaSQRL$&mgd=h zPrr5Id-@#x;FG%?LkxWQRkYDpzAd;zre1OIf@=B4t~S%#=oOp6eqejBpFEly32L+6 z+Pq$pSIF4m=E86TgqsTM3|=_gdAJ7489yHWL!%^;$$0fo5jg!u<xdP_ZAKLnap79`0?@ zw0M#%ak`Jt*1!@aBEoq~@Ae(eJGB+o?Kv zZXckkl&=GR+zzk*PBcKM7i4H~?buEg7;(dnyK)DpZMRd}OtK7Z>oozv&Ty414~14G z3l=IuGk9hQR7UnFH@MY0y-(x+EvW0pWq4SEu+Tv&aSK{!%?*{<(e{3<{W=;njM}Z1 z+_ZXhN=*Zn$@O=E)r751eg35`1W>6M+xHazh^Kt?`js#_Aus5Nc}r4tbWGKWeYkzf z=&HsPOLUfa$!hVbG}5;3hp{cJQ9YXi96sVnDU#34(%z9u0-v>#@aG3YM_8ku54`v= zg?rnB>aq1QWS%)bu4jjLN9c@7oG`f8^~A$zUlQ9N>s3*%T-ijpO!a1U;-@eBQej<| zs*a|?K#=28Nho)`wRm^O2t*sqED)6q2U^ohP+{fb{IRzv4mB)HPod~JdMc_M zCC)cKKxht%%AryyVd(OT3N$q=d0|1ZLMyn>h%*s?2Xv=_mNGDvGvRH;yGJPz-5|J) z{(vv5I;2TG;0f82c*$JeGnA4uK==3uuh&AeHtd!l$+#-`HI2UZkK&(@e4b&tc;0V# z$`bfAJ!dy0#)U8~!J?XK8=zT5>GFrS)H^^evFlb=Pd?(`9o66A8_x?Okz1D3ILP?> z*G@bCsDY6eur$~d=JwAzFdTQ}9=(q9A$Gsv(oi-^Bdxsg4EDu0s~fj?gKxlBQ&y60Fs7-sAI>#Jf+J$rlNH5N>cvl`S&FrYOh{;5V zT}EtJy}fEyZTD68sT^!yYk55a|I#bRrtCqK|7>wXuhL7ly=-3SKYP646n>5F`}*PW zhOhB!15oFfTgV6AZ?DTHih{9)q1A^O+1&0BVa3}p?tXXLU?$S%0kb#R7juT8jiB_4<<~0|@QD9ZEHnWX0Gp6pV_DnI0J`A$-0^R<2&i#!BC;%ljRSMl32e zYTV>EI1sb2PX`=aygKnO13#frZxN8PW_%Z6dp!%kAJ1(65D#vPH?d|GIo;#hrjgF~ z!(A@Ymv-iM`sw-hnq2ToaZ?XqjGp=gpZQ8^>oZaTUp$EGn`i3wO{76WkbOTQg%Emh z@`>KYr&-0`+-mu((tq|?!zuGR`s_j;;Z@DXKL0^2+OOT80^SQLaf@3UIDje#0H1@+ z`+6Fm20A{U@P}xnCalB8hd8a!$@f}NVcLkdx+PG!T#JoDuEa7?u-4(PC}dN*O7;lA zXJ3Ii?Ef@n_tU)Wsu`n1p7VSHwEH<`aoq@wM2=!~8vhY?ueY0JyfDjVPnN4{nxON~ zlr;_`ORufgL8*aEzetftL}VhFU@VphCJLTJ#S56+i5?i~yc zjTl9sy%((rn!~KzTNYEQ^LLk;%^4mX;Lr>lMAh4UG51-2$@Y+OxGki&E5?Kj6*zw#CKLb3nc`)tS zpa;okPo<>U#kAkFs)2!h|6v|7{s}S<2`$4nFL;>7`D19NRd$T$s@Jsv0oPz(BYT_x zM`2Pu$sz~q7%oGEewzi-W>K7NHRwgNPnovYr_pD!LJ*K@<%btAQYz&d!g+jPv^3U9lT%g&n`+Hz9WMJ$%1nu@rbv+(78%zuUkG1 z_Em13tO3|~K@z)&KVK|D7B0){K<@P5dzb@;`_|2Z3DL$^W&2eTEfXSf~y=9;auZfko)aw8UTSq=jXU0bXQyvM(gXWb)NOOEA6x_)Zy;V-jQ! za)T~fbab-oSLNYFbQEaYaZRzJW18+FAix-r4bqLQwZO9>^j%vFTES@s%1z-3Pue|% zIXTfx&RK?(w}FRa3p{Yb+cqi%p-}9W&ZM8sn)mQA2*FMF!tSEH@Ep?&s5ZJjrlREB z&%iyKF!D?No6dx)D{VPW1&O~OVi#<-o;wO3_F4Gj@1%x5wmBa1&s*(>9z0+haLc|Z82mT)HeB9e6Q>oS|LjdhB1d$$knM<=}xK3DK$~D8fa?D z1&wA|YIQnQw%&Yrh0G{=bMhp+tTf`cs>xx(w)BBJC)6y$Z{>+%byDTy@=tHB7dQ^y zpNj!-jj#t{1UR3G0C+L<4nZ`wKC_vT1=(5oNts!xd4J-bZyeIH^A05CJNPVHtxXH^9F|J(9kMF2n}eQyM2f9?9G VWn4BC_IVs+EQgGB&}YmB_zzr