From b2ded208881f0fed4aa8a96cfe20ec7f9da7a7c6 Mon Sep 17 00:00:00 2001 From: SBaksa Date: Thu, 5 Nov 2026 12:36:59 +1100 Subject: [PATCH] OLE2FRAME: the axis-aligned check scales its tolerance with the matrix (review on #2587) ALIGN builds its matrix from two angles that agree only to rounding, so a large scale leaves a rotation part of about scale * 1e-16 that an absolute 1e-12 bound refused as a turn. The bound now grows with the scale; a real turn is still refused. Test: ole_frame_accepts_a_large_scale_with_rounding_noise_in_its_rotation. --- crates/doc/src/extents.rs | 19 +++++++++++++- crates/engine/tests/ole_frame_transform.rs | 29 ++++++++++++++++++++++ 2 files changed, 47 insertions(+), 1 deletion(-) diff --git a/crates/doc/src/extents.rs b/crates/doc/src/extents.rs index 7713395a4..55138649d 100644 --- a/crates/doc/src/extents.rs +++ b/crates/doc/src/extents.rs @@ -161,7 +161,10 @@ pub fn ole_frame_transform(tags: &mut [crate::RawTag], m: &cadcraft_geom::Mat3) /// upper-left. pub fn ole_frame_accepts(m: &cadcraft_geom::Mat3) -> bool { const T: f64 = 1e-12; - m.b.abs() < T && m.c.abs() < T && m.a > 0.0 && (m.a - m.d).abs() <= T * m.a.abs().max(1.0) + // The tolerance grows with the scale: a matrix built from two angles (ALIGN) leaves rounding noise + // in b and c of about `scale * 1e-16`. + let t = T * m.a.abs().max(1.0); + m.b.abs() < t && m.c.abs() < t && m.a > 0.0 && (m.a - m.d).abs() <= t } /// Bounds of a single-line TEXT / ATTDEF / ATTRIB of `len` characters, following its justification @@ -767,6 +770,20 @@ mod tests { assert!(!ole_frame_accepts(&Mat3::rotate_about(p, 0.5))); assert!(!ole_frame_accepts(&Mat3::mirror(Vec2::new(0.0, 0.0), Vec2::new(1.0, 0.0)))); } + /// ALIGN with a scale builds its matrix from two angles that agree only to rounding, so a large + /// scale leaves a rotation part of about `scale * 1e-16`. That is a scale, not a turn: the + /// tolerance for it grows with the scale, while a real turn is still refused (review on #2587). + #[test] + fn ole_frame_accepts_a_large_scale_with_rounding_noise_in_its_rotation() { + use cadcraft_geom::Mat3; + let p = Vec2::new(3.0, -4.0); + let noisy = Mat3::scale_about(p, 1e6).then_before(Mat3::rotate_about(p, 2e-16)); + assert!(noisy.b.abs() > 1e-12, "the setup must exceed an absolute tolerance: {}", noisy.b); + assert!(ole_frame_accepts(&noisy)); + let turned = Mat3::scale_about(p, 1e6).then_before(Mat3::rotate_about(p, 1e-6)); + assert!(!ole_frame_accepts(&turned)); + } + /// The corners (x, y) in the OLE data header of a frame built by [`big_frame`]-style tags. fn header_corners(tags: &[crate::RawTag]) -> Vec<(f64, f64)> { let hex: String = tags.iter().filter(|t| t.code == 310).map(|t| t.value.as_str()).collect(); diff --git a/crates/engine/tests/ole_frame_transform.rs b/crates/engine/tests/ole_frame_transform.rs index 3569d39e5..afe138ff9 100644 --- a/crates/engine/tests/ole_frame_transform.rs +++ b/crates/engine/tests/ole_frame_transform.rs @@ -236,3 +236,32 @@ fn arraypath_refuses_before_adding_any_copy() { assert_eq!(s.doc().unwrap().model.len(), count, "no copy was added"); assert_eq!(corners(&s), before); } + +/// A refused command leaves the drawing and its undo history alone: UNDO afterwards takes back the +/// earlier MOVE, not a phantom step (review on #2587). +#[test] +fn a_refused_rotation_adds_no_undo_step() { + let (mut s, h) = session(); + let original = corners(&s); + s.execute("move", &json!({ "handles": [h.clone()], "delta": [10, 5] })).unwrap(); + let undo = s.state().unwrap().undo.len(); + s.execute("rotate", &json!({ "handles": [h], "base": [0, 0], "angle": 30 })).expect_err("rotate"); + assert_eq!(s.state().unwrap().undo.len(), undo); + s.execute("undo", &json!({})).unwrap(); + assert_eq!(corners(&s), original); +} + +/// The classic -ARRAY makes separate copies: a polar array that turns its items refuses an OLE +/// frame before the first copy, and one that keeps the items upright carries it. +#[test] +fn classic_polar_array_refuses_a_turned_frame_but_not_an_upright_one() { + let (mut s, h) = session(); + let before = corners(&s); + let turned = json!({ "handles": [h.clone()], "type": "polar", "center": [0, 0], "count": 4, "rotate": true }); + let e = s.execute("-array", &turned).expect_err("turned items"); + assert!(e.to_string().contains("OLE object"), "{e}"); + assert_eq!((s.doc().unwrap().model.len(), corners(&s)), (1, before)); + let upright = json!({ "handles": [h], "type": "polar", "center": [0, 0], "count": 4, "rotate": false }); + s.execute("-array", &upright).unwrap(); + assert_eq!(s.doc().unwrap().model.len(), 4); +}