From 63accec16e287c6b20fe06716ad3835d49752b5d Mon Sep 17 00:00:00 2001 From: "H. Conrad" <78785932+Ian-cmd-ops@users.noreply.github.com> Date: Thu, 15 Oct 2026 19:19:10 -0700 Subject: [PATCH] STEP export: shelled revolved bodies export instead of panicking (#1496) A scale across a revolution's axis keeps the face a surface of revolution and is written as one; any other refusal from the writer is an export error, not a panic. --- .../engine/tests/step_export_regressions.rs | 68 +++++++++++++++++++ crates/kernel/src/step.rs | 17 ++++- crates/kernel/src/step_in/tests.rs | 39 +++++++++++ vendor/README.md | 6 ++ vendor/truck-stepio/src/out/geometry.rs | 44 ++++++++---- 5 files changed, 161 insertions(+), 13 deletions(-) create mode 100644 crates/engine/tests/step_export_regressions.rs diff --git a/crates/engine/tests/step_export_regressions.rs b/crates/engine/tests/step_export_regressions.rs new file mode 100644 index 00000000..45c92c50 --- /dev/null +++ b/crates/engine/tests/step_export_regressions.rs @@ -0,0 +1,68 @@ +//! STEP export regressions found by round-tripping the robustness designs: a body the app built +//! must export, and read 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 19, shrunk: a three-quarter turn of a rectangle, a sphere cut, a 1 mm shell. +/// The shell moves revolved faces by a scale across their axis; export refused them and the +/// refusal surfaced as a panic ("a Display implementation returned an error"). +#[test] +fn shelled_revolve_with_a_sphere_cut_exports() { + let mut s = Session::default(); + run( + &mut s, + json!([ + {"command":"sketch.create","params":{"plane":"XZ"}}, + {"command":"sketch.rectangle.two_point","params":{"p0":[5.0,0],"p1":[25.0,5.0]}}, + {"command":"sketch.finish","params":{}}, + {"command":"solid.revolve","params":{"angle":"270 deg","axis":"Z"}}, + {"command":"solid.sphere","params":{"center":[-2.5,5.0,-2.5],"operation":"cut","radius":10.0}}, + {"command":"solid.shell","params":{"faces":[[-2.7554552980815448e-15,-15.0,-2.5000000000000004]],"thickness":1.0}} + ]), + ); + round_trips(&s); +} + +/// Robustness seed 24, shrunk: a three-quarter turn of a rectangle, a chamfer of two edges, a +/// 1.5 mm shell. The same refusal as seed 19's, reached through a chamfer. +#[test] +fn shelled_chamfered_revolve_exports() { + 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":[27.5,22.5]}}, + {"command":"sketch.finish","params":{}}, + {"command":"solid.revolve","params":{"angle":"270 deg","axis":"Z"}}, + {"command":"solid.chamfer","params":{"distance":2.0,"edges":[[7.0710678118654755,7.0710678118654755,0.0],[-19.445436482630054,19.44543648263006,0.0]]}}, + {"command":"solid.shell","params":{"faces":[[18.75,0.0,-11.358616634594824]],"thickness":1.5}} + ]), + ); + round_trips(&s); +} diff --git a/crates/kernel/src/step.rs b/crates/kernel/src/step.rs index b6656ec7..1f6d343e 100644 --- a/crates/kernel/src/step.rs +++ b/crates/kernel/src/step.rs @@ -74,10 +74,25 @@ pub(crate) fn truck_step(bodies: &[&Body], system: &str) -> Result { let cs: Vec<_> = bodies.iter().map(|b| plain_curves(b).map(|solid| solid.compress())).collect::>()?; let models: out::StepModels<_, _, _> = cs.iter().collect(); let header = out::StepHeaderDescriptor { organization_system: system.to_owned(), ..Default::default() }; - Ok(out::CompleteStepDisplay::new(models, header).to_string()) + written(&out::CompleteStepDisplay::new(models, header)) }) } +/// The text of truck's STEP writer, or an error. The writer refuses some geometry by returning +/// `fmt::Error` after a line saying why; `to_string` would turn that into a panic (caught by +/// `guard` on the desktop, but the browser build aborts on any panic). +fn written(display: &impl std::fmt::Display) -> Result { + use std::fmt::Write; + let mut text = String::new(); + match write!(text, "{display}") { + Ok(()) => Ok(text), + Err(_) => { + let why = text.lines().last().map(str::trim).filter(|l| !l.is_empty() && !l.starts_with('#')).unwrap_or("unsupported geometry"); + Err(KernelError::Failed(format!("not supported yet: STEP export of this body ({why})"))) + } + } +} + /// STEP (ISO 10303-21, AP242) text for the given bodies as one part (`Body1`…, with the /// bodies' colours). pub fn step_export(bodies: &[&Body], system: &str) -> Result { diff --git a/crates/kernel/src/step_in/tests.rs b/crates/kernel/src/step_in/tests.rs index 6012461c..5b7d7680 100644 --- a/crates/kernel/src/step_in/tests.rs +++ b/crates/kernel/src/step_in/tests.rs @@ -1202,3 +1202,42 @@ fn large_imports_are_freed_with_their_last_user() { drop((first, again)); assert!(weak.upgrade().is_none(), "the cache must not keep a large import alive after its design is gone"); } + +/// A three-quarter turn of a rectangle about Z: planar end caps, revolved cylinders and annuli. +fn three_quarter_ring() -> Body { + let rect = + Region2 { outer: Loop2::polygon(&[Vec2::new(5.0, 0.0), Vec2::new(25.0, 0.0), Vec2::new(25.0, 5.0), Vec2::new(5.0, 5.0)]), holes: vec![] }; + revolve(&Plane::XZ, &[rect], Vec2::ZERO, Vec2::Y, 1.5 * std::f64::consts::PI).unwrap().pop().unwrap() +} + +fn scaled(b: &Body, x: f64, y: f64, z: f64) -> Body { + crate::transform_matrix(b, [[x, 0.0, 0.0, 0.0], [0.0, y, 0.0, 0.0], [0.0, 0.0, z, 0.0], [0.0, 0.0, 0.0, 1.0]]).unwrap() +} + +/// A scale across the revolution axis that differs from the scale along it (how a shell or an +/// offset moves a revolved face) keeps the faces surfaces of revolution. The writer refused it +/// ("non-uniform scale"), and the refusal surfaced as a panic in export (robustness seeds 19, 24). +#[test] +fn revolved_faces_scaled_across_the_axis_round_trip() { + let ring = three_quarter_ring(); + let v0 = measure(&ring).unwrap().volume; + let b = scaled(&ring, 1.5, 1.5, 0.5); + assert!(rel(measure(&b).unwrap().volume, v0 * 1.5 * 1.5 * 0.5) < 1e-3); + round_trip(&b); +} + +/// A transform that does not keep the faces surfaces of revolution: export may refuse the body, +/// but with an error, never a panic (the browser build aborts on any panic), and anything it does +/// write must read back as the same solid. +#[test] +fn revolved_faces_skewed_never_panic_on_export() { + let b = scaled(&three_quarter_ring(), 1.5, 1.0, 1.0); + let r = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| step_export(&[&b], "t"))); + let Ok(r) = r else { panic!("STEP export panicked") }; + match r { + Err(e) => assert!(e.to_string().contains("not supported yet"), "{e}"), + Ok(_) => { + round_trip(&b); + } + } +} diff --git a/vendor/README.md b/vendor/README.md index 1f38c883..eff274ac 100644 --- a/vendor/README.md +++ b/vendor/README.md @@ -53,6 +53,12 @@ Changes: mirrored revolutions (ISO 10303-42 parameterises them by angle then profile, so their normal is the opposite of truck's; a mirroring transform reverses it again), and the modeling-surface wrapper passes the revolution's sense through instead of always `true`. +- `out/geometry.rs`: a surface of revolution under a transform that commutes with turning about + its axis (a scale across the axis that differs from the scale along it, as a shell or offset + makes of a revolved face) is written as the transformed profile revolved about the + transformed axis. Only uniform scales were taken. Other transforms are still refused: the + writer stops with a formatting error, which SolidCraft's `step.rs` now reports as an export + error (`to_string` panicked on it). - `lib.rs`: compiler warnings no longer fail the build. Example and test targets are dropped (their sources are not vendored). diff --git a/vendor/truck-stepio/src/out/geometry.rs b/vendor/truck-stepio/src/out/geometry.rs index 293cdfa5..91abc3ee 100644 --- a/vendor/truck-stepio/src/out/geometry.rs +++ b/vendor/truck-stepio/src/out/geometry.rs @@ -748,22 +748,42 @@ impl StepSurface for RevolutedCurve { impl DisplayByStep for Processor, Matrix4> where C: StepLength + Transformed + DisplayByStep { + // SolidCraft: a transform that commutes with turning about the axis keeps the surface one of + // revolution: the transformed profile revolved about the transformed axis. That holds for + // uniform scales (the only case taken before) and also for a scale across the axis that + // differs from the scale along it, which is how a shell or an offset moves a revolved + // cylinder or cone face. Any other transform is still refused. fn fmt(&self, idx: usize, f: &mut Formatter<'_>) -> Result { let surface = self.entity(); - let transform = self.transform(); - let (k, a, _) = match transform.iwasawa_decomposition() { - Some(x) => x, - None => { - f.write_str("Transform is not regular")?; - return ERR; - } - }; - if !a[0][0].near(&a[1][1]) || !a[1][1].near(&a[2][2]) { - f.write_str("Transform contains non-uniform scale.")?; + let transform = *self.transform(); + if transform.determinant().so_small() { + f.write_str("Transform is not regular")?; return ERR; } - let curve = surface.entity_curve().transformed(*transform); - let axis = k.transform_vector(surface.axis()); + let a = surface.axis().normalize(); + let la = transform.transform_vector(a); + if la.so_small() { + f.write_str("Transform is not regular")?; + return ERR; + } + let axis = la.normalize(); + // Two directions across the axis, and their images. + let seed = if a.x.abs() < 0.9 { Vector3::unit_x() } else { Vector3::unit_y() }; + let u = (seed - a * a.dot(seed)).normalize(); + let v = a.cross(u); + let (lu, lv) = (transform.transform_vector(u), transform.transform_vector(v)); + let s = lu.magnitude(); + // Across the axis: a similarity onto the plane across the image axis. + let commutes = !s.so_small() + && (lu.dot(axis) / s).so_small() + && (lv.dot(axis) / s).so_small() + && (lu.dot(lv) / (s * s)).so_small() + && (lv.magnitude() / s).near(&1.0); + if !commutes { + f.write_str("Transform does not keep the surface one of revolution.")?; + return ERR; + } + let curve = surface.entity_curve().transformed(transform); let origin = transform.transform_point(surface.origin()); let surface = RevolutedCurve::by_revolution(curve, origin, axis); DisplayByStep::fmt(&surface, idx, f)