From 42d4ebeb16f765401ad659f211a7a083edd3641e Mon Sep 17 00:00:00 2001 From: "H. Conrad" <78785932+Ian-cmd-ops@users.noreply.github.com> Date: Thu, 15 Oct 2026 21:27:48 -0700 Subject: [PATCH] STEP export: a merged partial cylinder keeps its cylinder (silent volume loss) (#1497) Thanks @Ian-cmd-ops. --- crates/engine/tests/step_merge_regressions.rs | 80 +++++++++++++++++++ crates/kernel/src/step_in/tests.rs | 14 ++++ crates/kernel/src/step_out.rs | 57 +++++++++++-- 3 files changed, 146 insertions(+), 5 deletions(-) create mode 100644 crates/engine/tests/step_merge_regressions.rs diff --git a/crates/engine/tests/step_merge_regressions.rs b/crates/engine/tests/step_merge_regressions.rs new file mode 100644 index 00000000..940e051b --- /dev/null +++ b/crates/engine/tests/step_merge_regressions.rs @@ -0,0 +1,80 @@ +//! STEP export regressions found by round-tripping the robustness designs: faces the writer +//! merges must keep their whole surface, so the body reads back as the same solid. + +use serde_json::{Value, json}; +use solidcraft_engine::Session; +use solidcraft_kernel::{measure, step_export, step_import}; + +fn run(s: &mut Session, script: Value) { + for c in script.as_array().into_iter().flatten() { + let (id, p) = (c["command"].as_str().unwrap_or_default(), &c["params"]); + if let Err(e) = s.execute(id, p) { + panic!("{id} {p}: {e}"); + } + } +} + +/// The design's one body exports, and reads back closed with the same volume and area. +fn round_trips(s: &Session) { + let st = s.model.state(); + assert_eq!(st.bodies.len(), 1); + let body = &st.bodies[0].body; + let text = step_export(&[body], "test").unwrap_or_else(|e| panic!("export: {e}")); + let imp = step_import(&text).unwrap_or_else(|e| panic!("import: {e}")); + assert_eq!(imp.bodies.len(), 1, "{:?}", imp.warnings); + assert!(imp.bodies[0].closed && imp.warnings.is_empty(), "{:?}", imp.warnings); + let (a, b) = (measure(body).unwrap(), measure(&imp.bodies[0].body).unwrap()); + assert!((a.volume - b.volume).abs() < 1e-3 * a.volume, "volume {} -> {}", a.volume, b.volume); + assert!((a.area - b.area).abs() < 1e-3 * a.area, "area {} -> {}", a.area, b.area); +} + +/// Robustness seed 17, shrunk: a thin three-quarter tube shelled 1 mm with its bottom open. The +/// export merged each wall's three quarter pieces into one face but kept the first piece's +/// quarter sheet as its surface; the body read back closed, with no warning, 7% too small. +#[test] +fn shelled_thin_three_quarter_tube_keeps_its_volume() { + let mut s = Session::default(); + run( + &mut s, + json!([ + {"command":"sketch.create","params":{"plane":"XZ"}}, + {"command":"sketch.rectangle.two_point","params":{"p0":[10.0,0],"p1":[12.5,27.5]}}, + {"command":"sketch.finish","params":{}}, + {"command":"solid.revolve","params":{"angle":"270 deg","axis":"Z"}}, + {"command":"solid.shell","params":{"faces":[[-7.182422893157272,7.182422893245109,0.0]],"thickness":1.0}} + ]), + ); + round_trips(&s); +} + +/// The same tube turned further: the merged wall leaves a gap of 60°, 30° or 10°. Its two straight +/// end edges lie either side of the gap, so the mean direction of the face's points no longer +/// points into the face, and a seam put opposite it falls inside the face. The seam belongs in the +/// middle of the gap. +#[test] +fn shelled_thin_wide_tube_keeps_its_volume() { + for turn in [300.0_f64, 330.0, 350.0] { + let mid = (turn / 2.0).to_radians(); + let mut s = Session::default(); + run( + &mut s, + json!([ + {"command":"sketch.create","params":{"plane":"XZ"}}, + {"command":"sketch.rectangle.two_point","params":{"p0":[10.0,0],"p1":[12.5,27.5]}}, + {"command":"sketch.finish","params":{}}, + {"command":"solid.revolve","params":{"angle":format!("{turn} deg"),"axis":"Z"}}, + {"command":"solid.shell","params":{"faces":[[11.0 * mid.cos(), 11.0 * mid.sin(), 0.0]],"thickness":1.0}} + ]), + ); + let st = s.model.state(); + assert_eq!(st.bodies.len(), 1, "{turn} deg"); + let body = &st.bodies[0].body; + let text = step_export(&[body], "test").unwrap_or_else(|e| panic!("{turn} deg export: {e}")); + let imp = step_import(&text).unwrap_or_else(|e| panic!("{turn} deg import: {e}")); + assert_eq!(imp.bodies.len(), 1, "{turn} deg {:?}", imp.warnings); + assert!(imp.bodies[0].closed && imp.warnings.is_empty(), "{turn} deg {:?}", imp.warnings); + let (a, b) = (measure(body).unwrap(), measure(&imp.bodies[0].body).unwrap()); + assert!((a.volume - b.volume).abs() < 1e-3 * a.volume, "{turn} deg volume {} -> {}", a.volume, b.volume); + assert!((a.area - b.area).abs() < 1e-3 * a.area, "{turn} deg area {} -> {}", a.area, b.area); + } +} diff --git a/crates/kernel/src/step_in/tests.rs b/crates/kernel/src/step_in/tests.rs index 5b7d7680..6dd7f4ea 100644 --- a/crates/kernel/src/step_in/tests.rs +++ b/crates/kernel/src/step_in/tests.rs @@ -238,6 +238,20 @@ fn round_trip_features() { round_trip(&donut[0]); } +/// Robustness seed 17: a thin three-quarter tube shelled with its bottom open. truck writes each +/// wall as three quarter-turn B-spline pieces; the export merges them into one 270° face and must +/// write that face's cylinder, not the first piece's quarter sheet. The seam direction came from +/// the mean direction of the face's vertices, which is zero at 0°, 90°, 180° and 270°, so the +/// merged face kept the quarter sheet and read back about 7% short with no warning. +#[test] +fn shelled_three_quarter_tube_round_trips() { + let rect = + Region2 { outer: Loop2::polygon(&[Vec2::new(10.0, 0.0), Vec2::new(12.5, 0.0), Vec2::new(12.5, 27.5), Vec2::new(10.0, 27.5)]), holes: vec![] }; + let tube = revolve(&Plane::XZ, &[rect], Vec2::ZERO, Vec2::Y, 1.5 * std::f64::consts::PI).unwrap().pop().unwrap(); + let b = crate::shell(&tube, &[Vec3::new(-7.182422893157272, 7.182422893245109, 0.0)], 1.0).unwrap(); + round_trip(&b); +} + #[test] fn several_bodies_and_names() { let a = box_solid(Vec3::ZERO, Vec3::new(10.0, 10.0, 10.0)).unwrap(); diff --git a/crates/kernel/src/step_out.rs b/crates/kernel/src/step_out.rs index c45e46a6..3cb1c59b 100644 --- a/crates/kernel/src/step_out.rs +++ b/crates/kernel/src/step_out.rs @@ -1252,7 +1252,8 @@ fn merge_split_faces(ex: &mut p21::Exchange, paint: &mut HashMap, /// B-spline cylinder and cone sheets written as CYLINDRICAL_SURFACE and CONICAL_SURFACE (the /// sheet covers only its own piece, and a fillet's may reach far past it), with the direction of -/// the parameter seam: on the face's seam edge for a full turn, else opposite the face's middle. +/// the parameter seam: on the face's seam edge for a full turn, else in the middle of the gap the +/// face leaves (`gap_middle`), else opposite the face's middle. /// Sheets under faces merged from pieces always; a lone piece when its edges are circles square /// to the axis and rulings. (Faces replaced by merges stay in the exchange unreferenced: only the /// shells' faces count.) @@ -1292,12 +1293,13 @@ fn analytic_sheets(ex: &p21::Exchange, merged_surfaces: &std::collections::HashS let p = ex.get(edge)?.params(); [p.get(1), p.get(2)].into_iter().filter_map(|v| vertex_point(ex, v?.as_ref_id()?)).find_map(|q| unit(radial(q))) }); - let x = match seam_dir { + let x = match seam_dir.or_else(|| gap_middle(ex, &loops, z, &radial)) { Some(x) => Some(x), None => { - // Points along the edges, not just their ends: the corners of a half turn (a - // slot's round end) point both ways and cancel out (#1198). The sheet's own - // middle where even those do. + // No single gap to find (a half turn has two corners, a tube cut in pieces + // several). Points along the edges, not just their ends: the corners of a half + // turn (a slot's round end) point both ways and cancel out (#1198). The sheet's + // own middle where even those do. let mean = loops .iter() .flatten() @@ -1316,6 +1318,51 @@ fn analytic_sheets(ex: &p21::Exchange, merged_surfaces: &std::collections::HashS out } +/// The middle of the one angular gap that a face on a cylinder or cone leaves around its axis +/// `z`. The face's boundary vertices are sorted by angle; each two neighbours are joined by one +/// of its edges except across the gap (a 270° face split at 0°, 90°, 180° and 270° leaves +/// 270°–360°). None unless there are three angles or more and exactly one gap: two vertices can +/// be joined either way round. +fn gap_middle(ex: &p21::Exchange, loops: &[Vec<(u64, bool)>], z: V3, radial: &impl Fn(V3) -> V3) -> Option { + use std::f64::consts::TAU; + let mut edges: Vec<(V3, V3)> = Vec::new(); + for (e, _) in loops.iter().flatten() { + let p = ex.get(*e)?.params(); + let (a, b) = (vertex_point(ex, p.get(1)?.as_ref_id()?)?, vertex_point(ex, p.get(2)?.as_ref_id()?)?); + edges.push((unit(radial(a))?, unit(radial(b))?)); + } + let e1 = edges.first()?.0; + let e2 = unit(cross(z, e1))?; + let angle = |d: V3| dot(d, e2).atan2(dot(d, e1)).rem_euclid(TAU); + let close = |a: f64, b: f64| { + let d = (a - b).rem_euclid(TAU); + d < 1e-6 || TAU - d < 1e-6 + }; + let mut angles: Vec = Vec::new(); + for (a, b) in &edges { + for t in [angle(*a), angle(*b)] { + if !angles.iter().any(|s| close(*s, t)) { + angles.push(t); + } + } + } + if angles.len() < 3 { + return None; + } + angles.sort_by(f64::total_cmp); + let joined = |s: f64, t: f64| { + edges.iter().any(|(a, b)| { + let (x, y) = (angle(*a), angle(*b)); + (close(x, s) && close(y, t)) || (close(x, t) && close(y, s)) + }) + }; + let n = angles.len(); + let gaps: Vec<(f64, f64)> = (0..n).filter_map(|i| Some((*angles.get(i)?, *angles.get((i + 1) % n)?))).filter(|(s, t)| !joined(*s, *t)).collect(); + let [(s, t)] = gaps[..] else { return None }; + let m = s + (t - s).rem_euclid(TAU) / 2.0; + unit(add(mul(e1, m.cos()), mul(e2, m.sin()))) +} + /// Margin left round a face when its sheet is cut down to it, as a share of the face's own /// parameter range on each side. const SHEET_MARGIN: f64 = 0.02;