From 4caf26ee60346262fedfe269b62e56f21754edd5 Mon Sep 17 00:00:00 2001 From: SBaksa Date: Sat, 31 Oct 2026 14:27:48 +1100 Subject: [PATCH 01/29] OLE2FRAME: draw its frame outline; a DWG save no longer lists it as left out (#2422) The frame is the rectangle from the entity's two corner points (groups 10/20 and 11/21). A DWG write and read-back keeps the OLE data (ole_frame.rs), so the DWG note leaves the type out of notSaved. --- crates/engine/src/cmd/file.rs | 8 ++++- crates/engine/tests/save_reports_not_saved.rs | 13 +++++++ crates/io/tests/it/main.rs | 1 + crates/io/tests/it/ole_frame.rs | 36 +++++++++++++++++++ crates/render/src/draw.rs | 14 ++++++++ crates/render/src/tests.rs | 14 ++++++++ 6 files changed, 85 insertions(+), 1 deletion(-) create mode 100644 crates/io/tests/it/ole_frame.rs diff --git a/crates/engine/src/cmd/file.rs b/crates/engine/src/cmd/file.rs index 9aaff4631..bf59d51f1 100644 --- a/crates/engine/src/cmd/file.rs +++ b/crates/engine/src/cmd/file.rs @@ -720,6 +720,10 @@ pub fn not_saved(d: &Drawing) -> std::collections::BTreeMap { /// store section, which is written back with the drawing (`Foreign::data_store`). const MODELER_TYPES: [&str; 3] = ["3DSOLID", "REGION", "BODY"]; +/// Types the DWG bridge writes back whole although CADCraft does not model them: a DWG save and +/// read-back keeps their data (`ole_frame.rs`, #2422), so they are not "left out". +const DWG_KEPT_TYPES: [&str; 1] = ["OLE2FRAME"]; + /// [`not_saved`] for a file saved as `version`. From 2013 on, a 3D solid, region or body the data /// store has a record for keeps its geometry and is not counted. pub fn not_saved_as(d: &Drawing, version: Version) -> std::collections::BTreeMap { @@ -728,7 +732,9 @@ pub fn not_saved_as(d: &Drawing, version: Version) -> std::collections::BTreeMap let stores = std::iter::once(&d.model).chain(d.layouts.iter().map(|l| &l.entities)).chain(d.blocks.values().map(|b| &b.entities)); for e in stores.flat_map(|s| s.iter()) { if let cadcraft_doc::EntityKind::Unknown(u) = &e.kind { - if MODELER_TYPES.iter().any(|t| u.dxf_type.eq_ignore_ascii_case(t)) && stored.contains(&e.handle) { + if DWG_KEPT_TYPES.iter().any(|t| u.dxf_type.eq_ignore_ascii_case(t)) + || (MODELER_TYPES.iter().any(|t| u.dxf_type.eq_ignore_ascii_case(t)) && stored.contains(&e.handle)) + { continue; } let n = out.entry(u.dxf_type.clone()).or_insert(0); diff --git a/crates/engine/tests/save_reports_not_saved.rs b/crates/engine/tests/save_reports_not_saved.rs index 615028a21..62a4cd41c 100644 --- a/crates/engine/tests/save_reports_not_saved.rs +++ b/crates/engine/tests/save_reports_not_saved.rs @@ -82,3 +82,16 @@ fn solids_whose_geometry_the_data_store_keeps_are_not_reported_from_2013() { // Before 2013 a file has no data store: every solid loses its geometry. assert_eq!(not_saved_as(&d, Version::R2010), [("3DSOLID".to_string(), 2), ("REGION".to_string(), 1)].into_iter().collect()); } + +/// An OLE2FRAME is written back whole by a DWG save, so the DWG note doesn't list it; other +/// unmodelled types are still listed (#2422). +#[test] +fn a_dwg_save_does_not_list_an_ole_frame() { + use cadcraft_engine::cmd::file::{Version, not_saved_as}; + let mut d = Drawing::new_imperial(); + d.add(&Space::Model, Common::default(), unknown("OLE2FRAME")).unwrap(); + d.add(&Space::Model, Common::default(), unknown("MLINE")).unwrap(); + for v in [Version::R2000, Version::R2018] { + assert_eq!(not_saved_as(&d, v), [("MLINE".to_string(), 1)].into_iter().collect(), "{v:?}"); + } +} diff --git a/crates/io/tests/it/main.rs b/crates/io/tests/it/main.rs index 7f6ad8f20..ea482d42a 100644 --- a/crates/io/tests/it/main.rs +++ b/crates/io/tests/it/main.rs @@ -44,6 +44,7 @@ mod mtext_columns_roundtrip; mod mtext_embedded_object; mod mtext_line_spacing; mod multiline_attributes; +mod ole_frame; mod paperview_marker; mod passthrough_image_roundtrip; mod pdf_followups; diff --git a/crates/io/tests/it/ole_frame.rs b/crates/io/tests/it/ole_frame.rs new file mode 100644 index 000000000..aa328a895 --- /dev/null +++ b/crates/io/tests/it/ole_frame.rs @@ -0,0 +1,36 @@ +//! An OLE2FRAME is kept as an unknown entity; a DWG save carries it with its data (#2422). + +use cadcraft_doc::EntityKind; +use cadcraft_io::{SaveOptions, Version, read, read_dxf, write_with}; + +/// A 2000 DXF with one OLE2FRAME: corners, the binary data and the end marker (DXF Reference). +fn dxf() -> String { + String::from( + "0\nSECTION\n2\nHEADER\n9\n$ACADVER\n1\nAC1015\n0\nENDSEC\n0\nSECTION\n2\nENTITIES\n\ + 0\nOLE2FRAME\n5\n2B\n100\nAcDbEntity\n8\n0\n100\nAcDbOle2Frame\n70\n2\n3\nOLE Object\n\ + 10\n30.14\n20\n-18.99\n30\n0\n11\n35.27\n21\n-22.39\n31\n0\n71\n2\n72\n0\n90\n8\n310\n0102030405060708\n1\nOLE\n\ + 0\nENDSEC\n0\nEOF\n", + ) +} + +fn ole(d: &cadcraft_doc::Drawing) -> Option { + d.model.iter().find_map(|e| match &e.kind { + EntityKind::Unknown(u) if u.dxf_type == "OLE2FRAME" => Some(u.clone()), + _ => None, + }) +} + +#[test] +fn a_dwg_keeps_an_ole_frame_with_its_data() { + let d = read_dxf(dxf().as_bytes()).unwrap(); + assert!(ole(&d).is_some()); + for v in [Version::R2000, Version::R2010, Version::R2018] { + let dwg = write_with(&d, "x.dwg", &SaveOptions { version: Some(v), binary: false }).unwrap(); + let back = read(&dwg, "x.dwg").unwrap(); + let u = ole(&back).unwrap_or_else(|| panic!("{v:?}: the OLE2FRAME is gone")); + let data: String = u.tags.iter().filter(|t| t.code == 310).map(|t| t.value.to_ascii_uppercase()).collect(); + assert_eq!(data, "0102030405060708", "{v:?}: data"); + // Only the data is compared: with this stand-in blob the DWG round trip returns default + // corners (1,1)/(0,0), which a real object might not do; not verified without a real file. + } +} diff --git a/crates/render/src/draw.rs b/crates/render/src/draw.rs index 7e00b4c8c..e51a1812c 100644 --- a/crates/render/src/draw.rs +++ b/crates/render/src/draw.rs @@ -234,6 +234,12 @@ fn entity_parts(b: &mut Builder, ctx: &Ctx, e: &Entity) { b.polyline(ctx, rgb, lw, l); } } + // An OLE object's content is not drawn; its frame, from its two corner points, is. + EntityKind::Unknown(u) if u.dxf_type.eq_ignore_ascii_case("OLE2FRAME") => { + if let Some(q) = ole_frame(&u.tags) { + b.polyline(ctx, rgb, lw, &q); + } + } kind => { for prim in kind.prims() { match prim { @@ -379,3 +385,11 @@ fn leader_scale(d: &Drawing, st: Option<&cadcraft_doc::DimStyle>) -> f64 { let dimscale = d.header.f64("DIMSCALE", 1.0); st.map(|s| s.effective_scale(dimscale)).unwrap_or_else(|| cadcraft_doc::DimStyle::default().effective_scale(dimscale)) } + +/// The closed outline of an OLE2FRAME from its upper-left (groups 10, 20) and lower-right +/// (groups 11, 21) corners (DXF Reference, OLE2FRAME); `None` when either is missing or not finite. +fn ole_frame(tags: &[cadcraft_doc::RawTag]) -> Option<[Vec2; 5]> { + let get = |code: i32| tags.iter().find(|t| t.code == code).and_then(|t| t.value.trim().parse::().ok()).filter(|v| v.is_finite()); + let (a, b) = (Vec2::new(get(10)?, get(20)?), Vec2::new(get(11)?, get(21)?)); + Some([a, Vec2::new(b.x, a.y), b, Vec2::new(a.x, b.y), a]) +} diff --git a/crates/render/src/tests.rs b/crates/render/src/tests.rs index ae35f27f9..475488d4a 100644 --- a/crates/render/src/tests.rs +++ b/crates/render/src/tests.rs @@ -1122,3 +1122,17 @@ fn raster_hatch_pattern_matches_one_by_one() { assert!(render(&l, &view, &o).unwrap().data() == render_one_by_one(&l, &view, &o).data()); } } + +/// An OLE2FRAME draws its frame, the rectangle from its two corner points (#2422), and nothing +/// when a corner is missing. +#[test] +fn ole_frame_draws_its_outline() { + let tag = |code: i32, value: &str| cadcraft_doc::RawTag { code, value: value.into() }; + let frame = |tags: Vec| EntityKind::Unknown(cadcraft_doc::Unknown { dxf_type: "OLE2FRAME".into(), tags }); + let mut d = Drawing::new_imperial(); + d.add(&Space::Model, Common::default(), frame(vec![tag(10, "30"), tag(20, "-19"), tag(11, "35"), tag(21, "-22")])).unwrap(); + d.add(&Space::Model, Common::default(), frame(vec![tag(10, "1"), tag(20, "2")])).unwrap(); + let l = build(&d, &Space::Model, &Options { tolerance: 0.01, ..Default::default() }); + assert_eq!(l.prims.len(), 1, "only the frame with both corners is drawn"); + assert_eq!(l.prims[0].len, 5, "a closed rectangle: five points"); +} From ada99e613d7535e6ac41752b2a084d04ca13ca55 Mon Sep 17 00:00:00 2001 From: SBaksa Date: Sat, 31 Oct 2026 14:31:00 +1100 Subject: [PATCH 02/29] Parity checklist: regenerated after merging main --- docs/parity-checklist.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/parity-checklist.md b/docs/parity-checklist.md index 53ffffc96..f842c51a9 100644 --- a/docs/parity-checklist.md +++ b/docs/parity-checklist.md @@ -1,10 +1,10 @@ # Parity checklist -> **Generated:** 2026-10-30 by `cargo xtask parity`. Do not edit by hand. +> **Generated:** 2026-10-31 by `cargo xtask parity`. Do not edit by hand. Menu-breadth parity: a reference menu item counts as covered when a CADCraft command is registered under the same menu label. It measures breadth, not depth; see [target-app-parity.md](target-app-parity.md) for the weighted assessment. -**Menu breadth: 298 / 491 items (61%)** · 444 registered commands. +**Menu breadth: 298 / 491 items (61%)** · 446 registered commands. | Menu | Items | Covered | % | |---|---|---|---| From 74ddfb377fb2f2b1c093b58d9589b820e32d66be Mon Sep 17 00:00:00 2001 From: SBaksa Date: Sat, 31 Oct 2026 14:49:13 +1100 Subject: [PATCH 03/29] Test: a DWG keeps an OLE2FRAME's data and frame corners (#2422) The DWG object has no corner fields; the position travels in the start of the OLE data, which the bridge keeps. The test builds that header with known corners and checks them at 2000, 2010 and 2018. --- crates/io/tests/it/ole_frame.rs | 40 +++++++++++++++++++++++++++------ 1 file changed, 33 insertions(+), 7 deletions(-) diff --git a/crates/io/tests/it/ole_frame.rs b/crates/io/tests/it/ole_frame.rs index aa328a895..c8c8c73a0 100644 --- a/crates/io/tests/it/ole_frame.rs +++ b/crates/io/tests/it/ole_frame.rs @@ -1,15 +1,40 @@ -//! An OLE2FRAME is kept as an unknown entity; a DWG save carries it with its data (#2422). +//! An OLE2FRAME is kept as an unknown entity; a DWG save carries it with its data and frame +//! position (#2422). A DWG's OLE2FRAME object has no corner fields: the position travels in the +//! start of the OLE data (a 2-byte marker, then the frame's four corners as three little-endian +//! doubles each: upper left, upper right, lower right, lower left), which is how the DXF's +//! groups 10 and 11 come back. That layout is read from the acadrust dependency's decoder. use cadcraft_doc::EntityKind; use cadcraft_io::{SaveOptions, Version, read, read_dxf, write_with}; +const UL: (f64, f64) = (30.14, -18.99); +const LR: (f64, f64) = (35.27, -22.39); + +/// The OLE data: the envelope with the frame's corners. +fn blob() -> Vec { + let mut b = vec![1u8, 0]; + for (x, y) in [UL, (LR.0, UL.1), LR, (UL.0, LR.1)] { + for v in [x, y, 0.0] { + b.extend_from_slice(&v.to_le_bytes()); + } + } + b +} + /// A 2000 DXF with one OLE2FRAME: corners, the binary data and the end marker (DXF Reference). fn dxf() -> String { - String::from( + let data = blob(); + let hex: String = data.iter().map(|b| format!("{b:02X}")).collect(); + format!( "0\nSECTION\n2\nHEADER\n9\n$ACADVER\n1\nAC1015\n0\nENDSEC\n0\nSECTION\n2\nENTITIES\n\ 0\nOLE2FRAME\n5\n2B\n100\nAcDbEntity\n8\n0\n100\nAcDbOle2Frame\n70\n2\n3\nOLE Object\n\ - 10\n30.14\n20\n-18.99\n30\n0\n11\n35.27\n21\n-22.39\n31\n0\n71\n2\n72\n0\n90\n8\n310\n0102030405060708\n1\nOLE\n\ + 10\n{}\n20\n{}\n30\n0\n11\n{}\n21\n{}\n31\n0\n71\n2\n72\n0\n90\n{}\n310\n{hex}\n1\nOLE\n\ 0\nENDSEC\n0\nEOF\n", + UL.0, + UL.1, + LR.0, + LR.1, + data.len() ) } @@ -21,16 +46,17 @@ fn ole(d: &cadcraft_doc::Drawing) -> Option { } #[test] -fn a_dwg_keeps_an_ole_frame_with_its_data() { +fn a_dwg_keeps_an_ole_frame_with_its_data_and_position() { let d = read_dxf(dxf().as_bytes()).unwrap(); assert!(ole(&d).is_some()); + let want: String = blob().iter().map(|b| format!("{b:02X}")).collect(); for v in [Version::R2000, Version::R2010, Version::R2018] { let dwg = write_with(&d, "x.dwg", &SaveOptions { version: Some(v), binary: false }).unwrap(); let back = read(&dwg, "x.dwg").unwrap(); let u = ole(&back).unwrap_or_else(|| panic!("{v:?}: the OLE2FRAME is gone")); let data: String = u.tags.iter().filter(|t| t.code == 310).map(|t| t.value.to_ascii_uppercase()).collect(); - assert_eq!(data, "0102030405060708", "{v:?}: data"); - // Only the data is compared: with this stand-in blob the DWG round trip returns default - // corners (1,1)/(0,0), which a real object might not do; not verified without a real file. + let corner = |code: i32| u.tags.iter().find(|t| t.code == code).and_then(|t| t.value.trim().parse::().ok()); + assert_eq!(data, want, "{v:?}: data"); + assert_eq!((corner(10), corner(20), corner(11), corner(21)), (Some(UL.0), Some(UL.1), Some(LR.0), Some(LR.1)), "{v:?}: frame corners"); } } From a8d05d689443cedd2b8212d762e4c744b8f36df2 Mon Sep 17 00:00:00 2001 From: SBaksa Date: Sat, 31 Oct 2026 15:14:32 +1100 Subject: [PATCH 04/29] CLI: an OLE2FRAME is not reported as undrawn by an image export (#2422) Found end to end: convert to PNG drew the frame outline but still said '1 entity not drawn (OLE2FRAME: 1)'. --- apps/cadcraft-cli/src/output.rs | 5 ++++- apps/cadcraft-cli/tests/it/undrawn_entities.rs | 14 ++++++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/apps/cadcraft-cli/src/output.rs b/apps/cadcraft-cli/src/output.rs index d783267dc..d7471e7b1 100644 --- a/apps/cadcraft-cli/src/output.rs +++ b/apps/cadcraft-cli/src/output.rs @@ -151,7 +151,10 @@ pub(crate) fn export_framed(s: &Session, out: &Path, window: Option, io pub(crate) fn undrawn_message(d: &cadcraft_engine::doc::Drawing, path: &str) -> Option { let mut types: std::collections::BTreeMap<&str, usize> = std::collections::BTreeMap::new(); for e in d.model.iter() { - if let cadcraft_engine::doc::EntityKind::Unknown(u) = &e.kind { + // An OLE2FRAME's frame outline is drawn (the OLE content is not), so it isn't reported. + if let cadcraft_engine::doc::EntityKind::Unknown(u) = &e.kind + && !u.dxf_type.eq_ignore_ascii_case("OLE2FRAME") + { let n = types.entry(u.dxf_type.as_str()).or_insert(0); *n = n.saturating_add(1); } diff --git a/apps/cadcraft-cli/tests/it/undrawn_entities.rs b/apps/cadcraft-cli/tests/it/undrawn_entities.rs index 7e7189379..bf626a7d5 100644 --- a/apps/cadcraft-cli/tests/it/undrawn_entities.rs +++ b/apps/cadcraft-cli/tests/it/undrawn_entities.rs @@ -51,3 +51,17 @@ fn convert_to_an_image_warns_about_entities_it_cannot_draw() { assert!(!String::from_utf8_lossy(&out.stderr).contains("not drawn"), "{}", String::from_utf8_lossy(&out.stderr)); let _ = std::fs::remove_dir_all(&dir); } + +/// An OLE2FRAME's frame outline is drawn, so an image export doesn't call it undrawn (#2422). +#[test] +fn an_ole_frame_is_not_reported_as_undrawn() { + let dir = std::env::temp_dir().join(format!("cc-cli-ole-{}", std::process::id())); + std::fs::create_dir_all(&dir).unwrap(); + let input = dir.join("ole.dxf"); + let ole = "0\nSECTION\n2\nENTITIES\n0\nOLE2FRAME\n8\n0\n100\nAcDbOle2Frame\n10\n0\n20\n0\n30\n0\n11\n5\n21\n-3\n31\n0\n0\nENDSEC\n0\nEOF\n"; + std::fs::write(&input, ole).unwrap(); + let out = cli(&["convert", input.to_str().unwrap(), dir.join("out.png").to_str().unwrap()]); + assert!(out.status.success()); + assert!(!String::from_utf8_lossy(&out.stderr).contains("not drawn"), "{}", String::from_utf8_lossy(&out.stderr)); + let _ = std::fs::remove_dir_all(&dir); +} From ad609fab0235dca742108e336925f0a116001472 Mon Sep 17 00:00:00 2001 From: SBaksa Date: Sun, 1 Nov 2026 15:38:48 +1100 Subject: [PATCH 05/29] OLE2FRAME: the frame counts in the drawing extents; CLI test uses a self-cleaning dir (review on #2587) bounds_in now takes the frame from its corner tags (shared ole_frame in the doc crate), so zoom extents shows a frame outside other geometry. The undrawn_entities tests clean up on drop. --- .../cadcraft-cli/tests/it/undrawn_entities.rs | 29 +++++++++++++++---- crates/doc/src/extents.rs | 25 ++++++++++++++++ crates/doc/src/lib.rs | 2 +- crates/render/src/draw.rs | 10 +------ 4 files changed, 50 insertions(+), 16 deletions(-) diff --git a/apps/cadcraft-cli/tests/it/undrawn_entities.rs b/apps/cadcraft-cli/tests/it/undrawn_entities.rs index bf626a7d5..ca5338c2f 100644 --- a/apps/cadcraft-cli/tests/it/undrawn_entities.rs +++ b/apps/cadcraft-cli/tests/it/undrawn_entities.rs @@ -7,6 +7,25 @@ fn cli(args: &[&str]) -> Output { Command::new(env!("CARGO_BIN_EXE_cadcraft-cli")).args(args).output().unwrap() } +/// A fresh scratch directory that is removed on drop, also when an assertion fails. +struct Scratch(std::path::PathBuf); + +impl Scratch { + fn new(tag: &str) -> Self { + let dir = std::env::temp_dir().join(format!("cc-cli-{tag}-{}", std::process::id())); + // A directory left behind by an earlier run with the same process id must not leak in. + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir(&dir).expect("create scratch dir"); + Scratch(dir) + } +} + +impl Drop for Scratch { + fn drop(&mut self) { + let _ = std::fs::remove_dir_all(&self.0); + } +} + /// A line, two foreign REGIONs and a PDF underlay: the last three are kept as unknown entities. const PROXIES: &str = "0\nSECTION\n2\nENTITIES\n\ 0\nLINE\n8\n0\n10\n0\n20\n0\n11\n5\n21\n5\n\ @@ -17,8 +36,8 @@ const PROXIES: &str = "0\nSECTION\n2\nENTITIES\n\ #[test] fn convert_to_an_image_warns_about_entities_it_cannot_draw() { - let dir = std::env::temp_dir().join(format!("cc-cli-undrawn-{}", std::process::id())); - std::fs::create_dir_all(&dir).unwrap(); + let scratch = Scratch::new("undrawn"); + let dir = scratch.0.clone(); let input = dir.join("proxies.dxf"); std::fs::write(&input, PROXIES).unwrap(); let input = input.to_str().unwrap(); @@ -49,19 +68,17 @@ fn convert_to_an_image_warns_about_entities_it_cannot_draw() { let out = cli(&["convert", sample.to_str().unwrap(), dir.join("sample.svg").to_str().unwrap()]); assert!(out.status.success()); assert!(!String::from_utf8_lossy(&out.stderr).contains("not drawn"), "{}", String::from_utf8_lossy(&out.stderr)); - let _ = std::fs::remove_dir_all(&dir); } /// An OLE2FRAME's frame outline is drawn, so an image export doesn't call it undrawn (#2422). #[test] fn an_ole_frame_is_not_reported_as_undrawn() { - let dir = std::env::temp_dir().join(format!("cc-cli-ole-{}", std::process::id())); - std::fs::create_dir_all(&dir).unwrap(); + let scratch = Scratch::new("ole"); + let dir = scratch.0.clone(); let input = dir.join("ole.dxf"); let ole = "0\nSECTION\n2\nENTITIES\n0\nOLE2FRAME\n8\n0\n100\nAcDbOle2Frame\n10\n0\n20\n0\n30\n0\n11\n5\n21\n-3\n31\n0\n0\nENDSEC\n0\nEOF\n"; std::fs::write(&input, ole).unwrap(); let out = cli(&["convert", input.to_str().unwrap(), dir.join("out.png").to_str().unwrap()]); assert!(out.status.success()); assert!(!String::from_utf8_lossy(&out.stderr).contains("not drawn"), "{}", String::from_utf8_lossy(&out.stderr)); - let _ = std::fs::remove_dir_all(&dir); } diff --git a/crates/doc/src/extents.rs b/crates/doc/src/extents.rs index 353a42b53..55e3e5d24 100644 --- a/crates/doc/src/extents.rs +++ b/crates/doc/src/extents.rs @@ -9,6 +9,14 @@ use crate::{Drawing, Entity, EntityKind, LwPolyline, Prim}; /// Nested block references deeper than this are ignored (cyclic or hostile files). pub const MAX_BLOCK_DEPTH: usize = 16; +/// The closed outline of an OLE2FRAME from its upper-left (groups 10, 20) and lower-right +/// (groups 11, 21) corners (DXF Reference, OLE2FRAME); `None` when either is missing or not finite. +pub fn ole_frame(tags: &[crate::RawTag]) -> Option<[Vec2; 5]> { + let get = |code: i32| tags.iter().find(|t| t.code == code).and_then(|t| t.value.trim().parse::().ok()).filter(|v| v.is_finite()); + let (a, b) = (Vec2::new(get(10)?, get(20)?), Vec2::new(get(11)?, get(21)?)); + Some([a, Vec2::new(b.x, a.y), b, Vec2::new(a.x, b.y), a]) +} + /// Bounds of a single-line TEXT / ATTDEF / ATTRIB of `len` characters, following its justification /// the same way the renderer places it (the stroke font is about 0.88 of the height per character). fn text_box(t: &crate::Text, len: usize) -> Bounds2 { @@ -244,6 +252,10 @@ fn bounds_in<'a>(d: &'a Drawing, e: &Entity, depth: usize, blocks: &mut HashMap< Bounds2::from_points([o, o + u, o + v, o + u + v]) } EntityKind::LwPolyline(p) => lwpolyline_bounds(p), + // An OLE object's content is not drawn, but its frame is, so the frame counts (#2422). + EntityKind::Unknown(u) if u.dxf_type.eq_ignore_ascii_case("OLE2FRAME") => { + ole_frame(&u.tags).map_or(Bounds2::EMPTY, Bounds2::from_points) + } kind => { let mut b = Bounds2::EMPTY; for p in kind.prims() { @@ -438,4 +450,17 @@ mod tests { empty.text = Some(crate::MText { contents: " ".into(), ..mtext }); assert!(bounds(EntityKind::MLeader(empty)).max.x < 26.0); } + + /// An OLE2FRAME counts in the extents through its corner tags (#2422); a missing or + /// non-finite corner counts as nothing. + #[test] + fn ole_frame_counts_in_extents() { + let tag = |code: i32, value: &str| crate::RawTag { code, value: value.into() }; + let frame = |tags: Vec| EntityKind::Unknown(crate::Unknown { dxf_type: "OLE2FRAME".into(), tags }); + let b = bounds(frame(vec![tag(10, "30"), tag(20, "-19"), tag(11, "35"), tag(21, "-22")])); + assert_eq!((b.min, b.max), (Vec2::new(30.0, -22.0), Vec2::new(35.0, -19.0))); + assert!(bounds(frame(vec![tag(10, "1"), tag(20, "2")])).is_empty(), "one corner is not a frame"); + assert!(bounds(frame(vec![tag(10, "NaN"), tag(20, "0"), tag(11, "1"), tag(21, "inf")])).is_empty()); + assert!(bounds(EntityKind::Unknown(crate::Unknown { dxf_type: "OTHER".into(), tags: vec![tag(10, "5"), tag(20, "5")] })).is_empty()); + } } diff --git a/crates/doc/src/lib.rs b/crates/doc/src/lib.rs index e07a08609..55169219f 100644 --- a/crates/doc/src/lib.rs +++ b/crates/doc/src/lib.rs @@ -27,7 +27,7 @@ pub use cadcraft_color as color; pub use cadcraft_geom as geom; pub use constraint::*; pub use entity::*; -pub use extents::{MAX_BLOCK_DEPTH, entity_bounds, extent_bounds}; +pub use extents::{MAX_BLOCK_DEPTH, entity_bounds, extent_bounds, ole_frame}; pub use format::Format; pub use gdt::*; pub use header::{HVal, Header}; diff --git a/crates/render/src/draw.rs b/crates/render/src/draw.rs index e51a1812c..0ff9515ed 100644 --- a/crates/render/src/draw.rs +++ b/crates/render/src/draw.rs @@ -236,7 +236,7 @@ fn entity_parts(b: &mut Builder, ctx: &Ctx, e: &Entity) { } // An OLE object's content is not drawn; its frame, from its two corner points, is. EntityKind::Unknown(u) if u.dxf_type.eq_ignore_ascii_case("OLE2FRAME") => { - if let Some(q) = ole_frame(&u.tags) { + if let Some(q) = cadcraft_doc::ole_frame(&u.tags) { b.polyline(ctx, rgb, lw, &q); } } @@ -385,11 +385,3 @@ fn leader_scale(d: &Drawing, st: Option<&cadcraft_doc::DimStyle>) -> f64 { let dimscale = d.header.f64("DIMSCALE", 1.0); st.map(|s| s.effective_scale(dimscale)).unwrap_or_else(|| cadcraft_doc::DimStyle::default().effective_scale(dimscale)) } - -/// The closed outline of an OLE2FRAME from its upper-left (groups 10, 20) and lower-right -/// (groups 11, 21) corners (DXF Reference, OLE2FRAME); `None` when either is missing or not finite. -fn ole_frame(tags: &[cadcraft_doc::RawTag]) -> Option<[Vec2; 5]> { - let get = |code: i32| tags.iter().find(|t| t.code == code).and_then(|t| t.value.trim().parse::().ok()).filter(|v| v.is_finite()); - let (a, b) = (Vec2::new(get(10)?, get(20)?), Vec2::new(get(11)?, get(21)?)); - Some([a, Vec2::new(b.x, a.y), b, Vec2::new(a.x, b.y), a]) -} From b51318003be6e74e276bddccfc693168b5cb5664 Mon Sep 17 00:00:00 2001 From: SBaksa Date: Sun, 1 Nov 2026 16:21:24 +1100 Subject: [PATCH 06/29] Parity checklist: regenerated after merging main --- docs/parity-checklist.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/parity-checklist.md b/docs/parity-checklist.md index f842c51a9..5fb179f5a 100644 --- a/docs/parity-checklist.md +++ b/docs/parity-checklist.md @@ -1,6 +1,6 @@ # Parity checklist -> **Generated:** 2026-10-31 by `cargo xtask parity`. Do not edit by hand. +> **Generated:** 2026-11-01 by `cargo xtask parity`. Do not edit by hand. Menu-breadth parity: a reference menu item counts as covered when a CADCraft command is registered under the same menu label. It measures breadth, not depth; see [target-app-parity.md](target-app-parity.md) for the weighted assessment. From 8dc927e01c8b8c860b09c1fd6d7d82f2b2daf4cc Mon Sep 17 00:00:00 2001 From: SBaksa Date: Sun, 1 Nov 2026 16:57:17 +1100 Subject: [PATCH 07/29] Format: rustfmt after the clippy fix (review on #2587) --- crates/doc/src/extents.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/crates/doc/src/extents.rs b/crates/doc/src/extents.rs index 55e3e5d24..fbbb4824a 100644 --- a/crates/doc/src/extents.rs +++ b/crates/doc/src/extents.rs @@ -253,9 +253,7 @@ fn bounds_in<'a>(d: &'a Drawing, e: &Entity, depth: usize, blocks: &mut HashMap< } EntityKind::LwPolyline(p) => lwpolyline_bounds(p), // An OLE object's content is not drawn, but its frame is, so the frame counts (#2422). - EntityKind::Unknown(u) if u.dxf_type.eq_ignore_ascii_case("OLE2FRAME") => { - ole_frame(&u.tags).map_or(Bounds2::EMPTY, Bounds2::from_points) - } + EntityKind::Unknown(u) if u.dxf_type.eq_ignore_ascii_case("OLE2FRAME") => ole_frame(&u.tags).map_or(Bounds2::EMPTY, Bounds2::from_points), kind => { let mut b = Bounds2::EMPTY; for p in kind.prims() { From 87f3c087b2d9412f1b9b282ecf782b4b2262da35 Mon Sep 17 00:00:00 2001 From: SBaksa Date: Sun, 1 Nov 2026 18:12:57 +1100 Subject: [PATCH 08/29] OLE2FRAME: corners skip application groups; MOVE/COPY carry the frame, rotate/scale/mirror refuse it (review on #2587) ole_frame read 10/20 and 11/21 from inside 102 groups; it now skips them as the DXF reader does. A window-selected OLE2FRAME was left in place by MOVE/COPY. A DWG save writes the position from the corner groups (checked by write + read-back), so MOVE/COPY now shift them; transforms that would not keep it axis aligned return an error before anything changes. --- crates/doc/src/entity.rs | 1 + crates/doc/src/extents.rs | 75 +++++++++++++++++++++- crates/doc/src/lib.rs | 2 +- crates/engine/src/cmd/modify/transform.rs | 11 ++++ crates/engine/tests/ole_frame_transform.rs | 57 ++++++++++++++++ crates/io/tests/it/ole_frame.rs | 15 +++++ 6 files changed, 158 insertions(+), 3 deletions(-) create mode 100644 crates/engine/tests/ole_frame_transform.rs diff --git a/crates/doc/src/entity.rs b/crates/doc/src/entity.rs index d13b82722..a32fb820a 100644 --- a/crates/doc/src/entity.rs +++ b/crates/doc/src/entity.rs @@ -461,6 +461,7 @@ impl EntityKind { let a = if mirror { t.rotation() } else { t.rotation() + rot }; t.dir = Vec2::from_angle(cadcraft_geom::norm_angle(a)).to3(0.0); } + EntityKind::Unknown(u) if u.dxf_type.eq_ignore_ascii_case("OLE2FRAME") => crate::ole_frame_transform(&mut u.tags, m), EntityKind::Unknown(_) => {} } } diff --git a/crates/doc/src/extents.rs b/crates/doc/src/extents.rs index fbbb4824a..4bbdf3cf9 100644 --- a/crates/doc/src/extents.rs +++ b/crates/doc/src/extents.rs @@ -9,14 +9,63 @@ use crate::{Drawing, Entity, EntityKind, LwPolyline, Prim}; /// Nested block references deeper than this are ignored (cyclic or hostile files). pub const MAX_BLOCK_DEPTH: usize = 16; +/// Positions in `tags` of the groups outside application-defined `102 {` .. `102 }` groups, whose +/// codes are the application's own and not the entity's. +fn outside_app(tags: &[crate::RawTag]) -> Vec { + let mut in_app = false; + let mut out = Vec::new(); + for (i, t) in tags.iter().enumerate() { + if t.code == 102 { + in_app = t.value.starts_with('{'); + } else if !in_app { + out.push(i); + } + } + out +} + +/// The positions of the frame's corner groups 10, 20, 11, 21 (in that order) and their values. +fn ole_corner_groups(tags: &[crate::RawTag]) -> Option<[(usize, f64); 4]> { + let own = outside_app(tags); + let find = |code: i32| { + let i = own.iter().copied().find(|&i| tags.get(i).is_some_and(|t| t.code == code))?; + let v = tags.get(i)?.value.trim().parse::().ok().filter(|v| v.is_finite())?; + Some((i, v)) + }; + Some([find(10)?, find(20)?, find(11)?, find(21)?]) +} + /// The closed outline of an OLE2FRAME from its upper-left (groups 10, 20) and lower-right /// (groups 11, 21) corners (DXF Reference, OLE2FRAME); `None` when either is missing or not finite. pub fn ole_frame(tags: &[crate::RawTag]) -> Option<[Vec2; 5]> { - let get = |code: i32| tags.iter().find(|t| t.code == code).and_then(|t| t.value.trim().parse::().ok()).filter(|v| v.is_finite()); - let (a, b) = (Vec2::new(get(10)?, get(20)?), Vec2::new(get(11)?, get(21)?)); + let [(_, x1), (_, y1), (_, x2), (_, y2)] = ole_corner_groups(tags)?; + let (a, b) = (Vec2::new(x1, y1), Vec2::new(x2, y2)); Some([a, Vec2::new(b.x, a.y), b, Vec2::new(a.x, b.y), a]) } +/// Move an OLE2FRAME by `m`, a translation: its two corners are mapped through `m` (a DWG save +/// writes the frame's position from these groups). Other transforms do not keep a frame axis +/// aligned, so callers refuse them (see [`ole_frame_accepts`]). A frame with no usable corners is +/// left alone. +pub fn ole_frame_transform(tags: &mut [crate::RawTag], m: &cadcraft_geom::Mat3) { + let Some([(ix1, x1), (iy1, y1), (ix2, x2), (iy2, y2)]) = ole_corner_groups(tags) else { return }; + let (a, b) = (m.apply(Vec2::new(x1, y1)), m.apply(Vec2::new(x2, y2))); + if !(a.x.is_finite() && a.y.is_finite() && b.x.is_finite() && b.y.is_finite()) { + return; + } + for (i, v) in [(ix1, a.x), (iy1, a.y), (ix2, b.x), (iy2, b.y)] { + if let Some(t) = tags.get_mut(i) { + t.value = v.to_string(); + } + } +} + +/// Whether `m` can move an OLE2FRAME: only a translation (move, copy, stretch of the whole frame). +pub fn ole_frame_accepts(m: &cadcraft_geom::Mat3) -> bool { + const T: f64 = 1e-12; + (m.a - 1.0).abs() < T && m.b.abs() < T && m.c.abs() < T && (m.d - 1.0).abs() < T +} + /// Bounds of a single-line TEXT / ATTDEF / ATTRIB of `len` characters, following its justification /// the same way the renderer places it (the stroke font is about 0.88 of the height per character). fn text_box(t: &crate::Text, len: usize) -> Bounds2 { @@ -461,4 +510,26 @@ mod tests { assert!(bounds(frame(vec![tag(10, "NaN"), tag(20, "0"), tag(11, "1"), tag(21, "inf")])).is_empty()); assert!(bounds(EntityKind::Unknown(crate::Unknown { dxf_type: "OTHER".into(), tags: vec![tag(10, "5"), tag(20, "5")] })).is_empty()); } + + /// Groups 10/20 inside an application-defined `102 {...}` group belong to the application, not + /// to the frame: the real corners come after it (review on #2587). + #[test] + fn ole_frame_skips_application_groups() { + let tag = |code: i32, value: &str| crate::RawTag { code, value: value.into() }; + let tags = vec![ + tag(102, "{ACAD_REACTORS"), + tag(10, "900"), + tag(20, "900"), + tag(11, "901"), + tag(21, "901"), + tag(102, "}"), + tag(10, "30"), + tag(20, "-19"), + tag(11, "35"), + tag(21, "-22"), + ]; + let q = ole_frame(&tags).expect("frame"); + assert_eq!((q[0], q[2]), (Vec2::new(30.0, -19.0), Vec2::new(35.0, -22.0))); + assert!(ole_frame(&tags[..6]).is_none(), "corners only inside the group are not a frame"); + } } diff --git a/crates/doc/src/lib.rs b/crates/doc/src/lib.rs index 55169219f..748f2c9e8 100644 --- a/crates/doc/src/lib.rs +++ b/crates/doc/src/lib.rs @@ -27,7 +27,7 @@ pub use cadcraft_color as color; pub use cadcraft_geom as geom; pub use constraint::*; pub use entity::*; -pub use extents::{MAX_BLOCK_DEPTH, entity_bounds, extent_bounds, ole_frame}; +pub use extents::{MAX_BLOCK_DEPTH, entity_bounds, extent_bounds, ole_frame, ole_frame_accepts, ole_frame_transform}; pub use format::Format; pub use gdt::*; pub use header::{HVal, Header}; diff --git a/crates/engine/src/cmd/modify/transform.rs b/crates/engine/src/cmd/modify/transform.rs index 5a86d9e07..c4a507262 100644 --- a/crates/engine/src/cmd/modify/transform.rs +++ b/crates/engine/src/cmd/modify/transform.rs @@ -4,6 +4,17 @@ use super::{Entity, Handle, Mat3, Result, Session, Value, Vec2, bad, bool_or, f6 pub(crate) fn transform_entities(s: &mut Session, hs: &[Handle], m: &Mat3, copy: bool) -> Result> { let d = s.doc_mut()?; + // An OLE2FRAME only translates (its corners stay axis aligned); refuse the rest before anything + // changes, instead of reporting success and leaving the frame where it was. + if !cadcraft_doc::ole_frame_accepts(m) + && hs.iter().any(|h| { + d.entity(*h).is_some_and(|e| matches!(&e.kind, cadcraft_doc::EntityKind::Unknown(u) if u.dxf_type.eq_ignore_ascii_case("OLE2FRAME"))) + }) + { + return Err(crate::EngineError::Other( + "An OLE object can only be moved or copied; rotating, scaling and mirroring it are not supported.".into(), + )); + } let mut out = Vec::new(); for h in hs { let Some(sp) = d.space_of(*h) else { continue }; diff --git a/crates/engine/tests/ole_frame_transform.rs b/crates/engine/tests/ole_frame_transform.rs new file mode 100644 index 000000000..0c0fade22 --- /dev/null +++ b/crates/engine/tests/ole_frame_transform.rs @@ -0,0 +1,57 @@ +//! An OLE2FRAME is window-selectable (it counts in the extents, #2422): MOVE and COPY carry it by +//! the offset, and the transforms that would not keep it axis aligned are refused before anything +//! reports success (review on #2587). + +use cadcraft_engine::Session; +use cadcraft_engine::doc::{Common, Drawing, EntityKind, RawTag, Space, Unknown}; +use serde_json::json; + +fn session() -> (Session, String) { + let tag = |code: i32, v: &str| RawTag { code, value: v.into() }; + let mut d = Drawing::new_imperial(); + let kind = + EntityKind::Unknown(Unknown { dxf_type: "OLE2FRAME".into(), tags: vec![tag(10, "30"), tag(20, "-19"), tag(11, "35"), tag(21, "-22")] }); + let h = d.add(&Space::Model, Common::default(), kind).unwrap(); + let mut s = Session::empty(); + s.open_drawing(d, "ole.dxf", None); + (s, h.hex()) +} + +fn corners(s: &Session) -> Vec<(i32, String)> { + let d = s.doc().unwrap(); + d.model + .iter() + .filter_map(|e| { + if let EntityKind::Unknown(u) = &e.kind { Some(u.tags.iter().map(|t| (t.code, t.value.clone())).collect::>()) } else { None } + }) + .flatten() + .collect() +} + +#[test] +fn move_and_copy_carry_an_ole_frame() { + let (mut s, h) = session(); + s.execute("move", &json!({ "handles": [h], "delta": [10, 5] })).unwrap(); + let want: Vec<(i32, String)> = [(10, "40"), (20, "-14"), (11, "45"), (21, "-17")].iter().map(|(c, v)| (*c, v.to_string())).collect(); + assert_eq!(corners(&s), want); + s.execute("copy", &json!({ "handles": [h], "delta": [10, 5] })).unwrap(); + let all = corners(&s); + assert_eq!(all.len(), 8); + assert_eq!(all[4..].iter().map(|(_, v)| v.as_str()).collect::>(), ["50", "-9", "55", "-12"]); +} + +#[test] +fn rotate_scale_and_mirror_refuse_an_ole_frame() { + let (mut s, h) = session(); + let before = corners(&s); + for (cmd, p) in [ + ("rotate", json!({ "handles": [h], "base": [0, 0], "angle": 30 })), + ("scale", json!({ "handles": [h], "base": [0, 0], "factor": 2 })), + ("mirror", json!({ "handles": [h], "p1": [0, 0], "p2": [1, 0] })), + ] { + let e = s.execute(cmd, &p).expect_err(cmd); + assert!(e.to_string().contains("OLE object"), "{cmd}: {e}"); + } + assert_eq!(corners(&s), before); + assert_eq!(s.doc().unwrap().model.len(), 1, "nothing was copied"); +} diff --git a/crates/io/tests/it/ole_frame.rs b/crates/io/tests/it/ole_frame.rs index c8c8c73a0..0ca0bc23e 100644 --- a/crates/io/tests/it/ole_frame.rs +++ b/crates/io/tests/it/ole_frame.rs @@ -60,3 +60,18 @@ fn a_dwg_keeps_an_ole_frame_with_its_data_and_position() { assert_eq!((corner(10), corner(20), corner(11), corner(21)), (Some(UL.0), Some(UL.1), Some(LR.0), Some(LR.1)), "{v:?}: frame corners"); } } + +/// A frame moved through its corner tags (what MOVE does) keeps the new position in a DWG save +/// and read-back (review on #2587). +#[test] +fn a_moved_ole_frame_keeps_its_position_in_a_dwg() { + let mut d = read_dxf(dxf().as_bytes()).unwrap(); + for h in d.model.handles() { + d.model.modify(h, |e| e.kind.transform(&cadcraft_geom::Mat3::translate(cadcraft_geom::Vec2::new(100.0, -50.0)))); + } + let dwg = write_with(&d, "x.dwg", &SaveOptions { version: Some(Version::R2018), binary: false }).unwrap(); + let u = ole(&read(&dwg, "x.dwg").unwrap()).unwrap(); + let c = |code: i32| u.tags.iter().find(|t| t.code == code).and_then(|t| t.value.trim().parse::().ok()).unwrap(); + let near = |a: f64, b: f64| (a - b).abs() < 1e-9; + assert!(near(c(10), UL.0 + 100.0) && near(c(20), UL.1 - 50.0) && near(c(11), LR.0 + 100.0) && near(c(21), LR.1 - 50.0), "{:?}", u.tags); +} From c1b88a6cbcb76744f97d58ca71795c39b61d96b3 Mon Sep 17 00:00:00 2001 From: SBaksa Date: Sun, 1 Nov 2026 19:21:22 +1100 Subject: [PATCH 09/29] OLE2FRAME: an image export notes that the OLE content is not drawn (review on #2587) --- apps/cadcraft-cli/src/output.rs | 27 ++++++++++++------- .../cadcraft-cli/tests/it/undrawn_entities.rs | 10 ++++--- 2 files changed, 25 insertions(+), 12 deletions(-) diff --git a/apps/cadcraft-cli/src/output.rs b/apps/cadcraft-cli/src/output.rs index d7471e7b1..8a8f7d97c 100644 --- a/apps/cadcraft-cli/src/output.rs +++ b/apps/cadcraft-cli/src/output.rs @@ -150,22 +150,31 @@ pub(crate) fn export_framed(s: &Session, out: &Path, window: Option, io /// as unknown objects, `Proxy` in `info`), or `None` when there are none. pub(crate) fn undrawn_message(d: &cadcraft_engine::doc::Drawing, path: &str) -> Option { let mut types: std::collections::BTreeMap<&str, usize> = std::collections::BTreeMap::new(); + let mut ole = 0usize; for e in d.model.iter() { - // An OLE2FRAME's frame outline is drawn (the OLE content is not), so it isn't reported. - if let cadcraft_engine::doc::EntityKind::Unknown(u) = &e.kind - && !u.dxf_type.eq_ignore_ascii_case("OLE2FRAME") - { + // An OLE2FRAME's frame outline is drawn, so it isn't an undrawn entity; its content is + // not, which gets a note of its own. + if let cadcraft_engine::doc::EntityKind::Unknown(u) = &e.kind { + if u.dxf_type.eq_ignore_ascii_case("OLE2FRAME") { + ole = ole.saturating_add(1); + continue; + } let n = types.entry(u.dxf_type.as_str()).or_insert(0); *n = n.saturating_add(1); } } let n = types.values().fold(0usize, |a, n| a.saturating_add(*n)); - if n == 0 { - return None; + let mut lines = Vec::new(); + if n > 0 { + let list: Vec = types.iter().map(|(t, n)| format!("{t}: {n}")).collect(); + let entities = if n == 1 { "entity" } else { "entities" }; + lines.push(format!("{path}: {n} {entities} not drawn ({})", list.join(", "))); } - let list: Vec = types.iter().map(|(t, n)| format!("{t}: {n}")).collect(); - let entities = if n == 1 { "entity" } else { "entities" }; - Some(format!("{path}: {n} {entities} not drawn ({})", list.join(", "))) + if ole > 0 { + let objects = if ole == 1 { "object" } else { "objects" }; + lines.push(format!("{path}: {ole} OLE {objects} shown as the frame only (the OLE content is not drawn)")); + } + if lines.is_empty() { None } else { Some(lines.join("\n")) } } /// The warning for the records of `path` the reader could not place, or `None` when there are none. diff --git a/apps/cadcraft-cli/tests/it/undrawn_entities.rs b/apps/cadcraft-cli/tests/it/undrawn_entities.rs index ca5338c2f..8b176cbbc 100644 --- a/apps/cadcraft-cli/tests/it/undrawn_entities.rs +++ b/apps/cadcraft-cli/tests/it/undrawn_entities.rs @@ -70,9 +70,10 @@ fn convert_to_an_image_warns_about_entities_it_cannot_draw() { assert!(!String::from_utf8_lossy(&out.stderr).contains("not drawn"), "{}", String::from_utf8_lossy(&out.stderr)); } -/// An OLE2FRAME's frame outline is drawn, so an image export doesn't call it undrawn (#2422). +/// An OLE2FRAME's frame outline is drawn, so an image export doesn't call it an undrawn entity (#2422); it +/// says once that the OLE content inside the frame is not drawn (review on #2587). #[test] -fn an_ole_frame_is_not_reported_as_undrawn() { +fn an_ole_frame_is_noted_as_frame_only() { let scratch = Scratch::new("ole"); let dir = scratch.0.clone(); let input = dir.join("ole.dxf"); @@ -80,5 +81,8 @@ fn an_ole_frame_is_not_reported_as_undrawn() { std::fs::write(&input, ole).unwrap(); let out = cli(&["convert", input.to_str().unwrap(), dir.join("out.png").to_str().unwrap()]); assert!(out.status.success()); - assert!(!String::from_utf8_lossy(&out.stderr).contains("not drawn"), "{}", String::from_utf8_lossy(&out.stderr)); + let err = String::from_utf8_lossy(&out.stderr); + assert!(!err.contains("entity not drawn") && !err.contains("entities not drawn"), "{err}"); + let notes: Vec<&str> = err.lines().filter(|l| l.contains("OLE")).collect(); + assert_eq!(notes, [format!("{}: 1 OLE object shown as the frame only (the OLE content is not drawn)", input.display())], "{err}"); } From afb24ee6e5da70bec7bb85f2814c33d7a01844f9 Mon Sep 17 00:00:00 2001 From: SBaksa Date: Sun, 1 Nov 2026 21:08:47 +1100 Subject: [PATCH 10/29] OLE2FRAME: MOVE also moves the corners inside the OLE data (review on #2587) The OLE data (310 groups) starts with the frame's four corners. MOVE shifted groups 10/20/11/21 only, so a DXF save wrote new corner groups beside a data header with the old position. The transform now rewrites those doubles (x, y; z kept) in the decoded hex, keeping each chunk's length; malformed or short data is left untouched. --- crates/doc/src/extents.rs | 57 +++++++++++++++++- crates/engine/tests/ole_frame_transform.rs | 69 ++++++++++++++++++++++ crates/io/tests/it/ole_frame.rs | 19 ++++++ 3 files changed, 144 insertions(+), 1 deletion(-) diff --git a/crates/doc/src/extents.rs b/crates/doc/src/extents.rs index 4bbdf3cf9..4aff09d8b 100644 --- a/crates/doc/src/extents.rs +++ b/crates/doc/src/extents.rs @@ -43,8 +43,62 @@ pub fn ole_frame(tags: &[crate::RawTag]) -> Option<[Vec2; 5]> { Some([a, Vec2::new(b.x, a.y), b, Vec2::new(a.x, b.y), a]) } +/// The bytes of the OLE data before its corners: a 2-byte marker. +const OLE_HEADER: usize = 2; +/// The corners in the OLE data: four of x, y, z as little-endian doubles (upper left, upper right, +/// lower right, lower left). +const OLE_CORNERS: usize = 12; + +/// Move the four corners at the start of the OLE data (the 310 groups' hex, in order) by `m`, +/// keeping z and every tag's length. Data that is not valid hex or is too short is left alone. +fn ole_data_transform(tags: &mut [crate::RawTag], m: &cadcraft_geom::Mat3) { + let idx: Vec = outside_app(tags).into_iter().filter(|&i| tags.get(i).is_some_and(|t| t.code == 310)).collect(); + let mut bytes: Vec = Vec::new(); + let mut lens: Vec = Vec::new(); + for &i in &idx { + let Some(t) = tags.get(i) else { return }; + let s = t.value.trim(); + if s.len() % 2 != 0 || !s.is_ascii() { + return; + } + for k in (0..s.len()).step_by(2) { + let Some(b) = s.get(k..k + 2).and_then(|h| u8::from_str_radix(h, 16).ok()) else { return }; + bytes.push(b); + } + lens.push(s.len() / 2); + } + let end = OLE_HEADER + OLE_CORNERS * 8; + if bytes.len() < end { + return; + } + let mut out = bytes.clone(); + for c in 0..4 { + let at = OLE_HEADER + c * 24; + let rd = |o: usize| out.get(o..o + 8).and_then(|s| <[u8; 8]>::try_from(s).ok()).map(f64::from_le_bytes).filter(|v| v.is_finite()); + let (Some(x), Some(y)) = (rd(at), rd(at + 8)) else { return }; + let p = m.apply(Vec2::new(x, y)); + if !(p.x.is_finite() && p.y.is_finite()) { + return; + } + if let Some(s) = out.get_mut(at..at + 8) { + s.copy_from_slice(&p.x.to_le_bytes()); + } + if let Some(s) = out.get_mut(at + 8..at + 16) { + s.copy_from_slice(&p.y.to_le_bytes()); + } + } + let mut pos = 0; + for (&i, &n) in idx.iter().zip(&lens) { + let (Some(t), Some(chunk)) = (tags.get_mut(i), out.get(pos..pos + n)) else { return }; + let lower = t.value.bytes().any(|b| b.is_ascii_lowercase()); + t.value = chunk.iter().map(|b| if lower { format!("{b:02x}") } else { format!("{b:02X}") }).collect(); + pos += n; + } +} + /// Move an OLE2FRAME by `m`, a translation: its two corners are mapped through `m` (a DWG save -/// writes the frame's position from these groups). Other transforms do not keep a frame axis +/// writes the frame's position from these groups), and so are the corners at the start of its OLE +/// data (310 groups), which a reader of the saved file may trust instead. Other transforms do not keep a frame axis /// aligned, so callers refuse them (see [`ole_frame_accepts`]). A frame with no usable corners is /// left alone. pub fn ole_frame_transform(tags: &mut [crate::RawTag], m: &cadcraft_geom::Mat3) { @@ -58,6 +112,7 @@ pub fn ole_frame_transform(tags: &mut [crate::RawTag], m: &cadcraft_geom::Mat3) t.value = v.to_string(); } } + ole_data_transform(tags, m); } /// Whether `m` can move an OLE2FRAME: only a translation (move, copy, stretch of the whole frame). diff --git a/crates/engine/tests/ole_frame_transform.rs b/crates/engine/tests/ole_frame_transform.rs index 0c0fade22..50ecc1f2f 100644 --- a/crates/engine/tests/ole_frame_transform.rs +++ b/crates/engine/tests/ole_frame_transform.rs @@ -55,3 +55,72 @@ fn rotate_scale_and_mirror_refuse_an_ole_frame() { assert_eq!(corners(&s), before); assert_eq!(s.doc().unwrap().model.len(), 1, "nothing was copied"); } + +/// The OLE data's marker and four corners (x, y, z doubles each), as hex split into chunks. +fn data_session(chunks: &[usize], hex_override: Option<&str>) -> (Session, String) { + let tag = |code: i32, v: &str| RawTag { code, value: v.into() }; + let mut blob = vec![1u8, 0]; + for (x, y) in [(30.0f64, -19.0f64), (35.0, -19.0), (35.0, -22.0), (30.0, -22.0)] { + for v in [x, y, 7.0] { + blob.extend_from_slice(&v.to_le_bytes()); + } + } + let hex: String = hex_override.map(str::to_string).unwrap_or_else(|| blob.iter().map(|b| format!("{b:02X}")).collect()); + let mut tags = vec![tag(10, "30"), tag(20, "-19"), tag(11, "35"), tag(21, "-22")]; + let mut at = 0; + for &n in chunks { + let end = (at + n).min(hex.len()); + tags.push(tag(310, &hex[at..end])); + at = end; + } + if at < hex.len() { + tags.push(tag(310, &hex[at..])); + } + let mut d = Drawing::new_imperial(); + let h = d.add(&Space::Model, Common::default(), EntityKind::Unknown(Unknown { dxf_type: "OLE2FRAME".into(), tags })).unwrap(); + let mut s = Session::empty(); + s.open_drawing(d, "ole.dxf", None); + (s, h.hex()) +} + +fn data_tags(s: &Session) -> Vec { + let d = s.doc().unwrap(); + d.model + .iter() + .filter_map(|e| { + if let EntityKind::Unknown(u) = &e.kind { + Some(u.tags.iter().filter(|t| t.code == 310).map(|t| t.value.clone()).collect::>()) + } else { + None + } + }) + .flatten() + .collect() +} + +#[test] +fn move_also_moves_the_corners_in_the_ole_data() { + let (mut s, h) = data_session(&[10, 100, 40], None); + let before = data_tags(&s); + s.execute("move", &json!({ "handles": [h], "delta": [10, 5] })).unwrap(); + let after = data_tags(&s); + assert_eq!(after.iter().map(String::len).collect::>(), before.iter().map(String::len).collect::>(), "chunk lengths are kept"); + let hex: String = after.concat(); + let bytes: Vec = (0..hex.len() / 2).map(|i| u8::from_str_radix(&hex[2 * i..2 * i + 2], 16).unwrap()).collect(); + let f = |o: usize| f64::from_le_bytes(bytes[o..o + 8].try_into().unwrap()); + let want = [(40.0, -14.0), (45.0, -14.0), (45.0, -17.0), (40.0, -17.0)]; + for (c, (x, y)) in want.iter().enumerate() { + assert_eq!((f(2 + c * 24), f(10 + c * 24), f(18 + c * 24)), (*x, *y, 7.0), "corner {c}"); + } +} + +#[test] +fn move_leaves_malformed_or_short_ole_data_alone() { + for (chunks, hex) in [(vec![4], "0100"), (vec![3, 3], "ABCDE"), (vec![10], "ZZ0100ZZ0100"), (vec![2], "é")] { + let (mut s, h) = data_session(&chunks, Some(hex)); + let before = data_tags(&s); + s.execute("move", &json!({ "handles": [h], "delta": [10, 5] })).unwrap(); + assert_eq!(data_tags(&s), before, "{hex}"); + assert_eq!(corners(&s)[0].1, "40", "the corner groups still move"); + } +} diff --git a/crates/io/tests/it/ole_frame.rs b/crates/io/tests/it/ole_frame.rs index 0ca0bc23e..188987db5 100644 --- a/crates/io/tests/it/ole_frame.rs +++ b/crates/io/tests/it/ole_frame.rs @@ -75,3 +75,22 @@ fn a_moved_ole_frame_keeps_its_position_in_a_dwg() { let near = |a: f64, b: f64| (a - b).abs() < 1e-9; assert!(near(c(10), UL.0 + 100.0) && near(c(20), UL.1 - 50.0) && near(c(11), LR.0 + 100.0) && near(c(21), LR.1 - 50.0), "{:?}", u.tags); } + +/// After a move, a DXF save and read-back carries the moved corners both in groups 10/11 and in +/// the OLE data header (review on #2587). +#[test] +fn a_moved_ole_frame_keeps_its_data_header_in_a_dxf() { + let mut d = read_dxf(dxf().as_bytes()).unwrap(); + for h in d.model.handles() { + d.model.modify(h, |e| e.kind.transform(&cadcraft_geom::Mat3::translate(cadcraft_geom::Vec2::new(100.0, -50.0)))); + } + let bytes = write_with(&d, "x.dxf", &SaveOptions { version: None, binary: false }).unwrap(); + let u = ole(&read_dxf(&bytes).unwrap()).unwrap(); + let hex: String = u.tags.iter().filter(|t| t.code == 310).map(|t| t.value.clone()).collect(); + let b: Vec = (0..hex.len() / 2).map(|i| u8::from_str_radix(&hex[2 * i..2 * i + 2], 16).unwrap()).collect(); + let f = |o: usize| f64::from_le_bytes(b[o..o + 8].try_into().unwrap()); + let near = |a: f64, c: f64| (a - c).abs() < 1e-9; + assert!(near(f(2), UL.0 + 100.0) && near(f(10), UL.1 - 50.0) && near(f(2 + 48), LR.0 + 100.0) && near(f(10 + 48), LR.1 - 50.0)); + let g = |code: i32| u.tags.iter().find(|t| t.code == code).and_then(|t| t.value.trim().parse::().ok()).unwrap(); + assert!(near(g(10), UL.0 + 100.0) && near(g(21), LR.1 - 50.0)); +} From 63bc5381134ad73ead4263e87ce1d3e261100639 Mon Sep 17 00:00:00 2001 From: SBaksa Date: Sun, 1 Nov 2026 22:09:49 +1100 Subject: [PATCH 11/29] Parity checklist: regenerated after merging main --- docs/parity-checklist.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/parity-checklist.md b/docs/parity-checklist.md index 5fb179f5a..a91ee2f25 100644 --- a/docs/parity-checklist.md +++ b/docs/parity-checklist.md @@ -4,7 +4,7 @@ Menu-breadth parity: a reference menu item counts as covered when a CADCraft command is registered under the same menu label. It measures breadth, not depth; see [target-app-parity.md](target-app-parity.md) for the weighted assessment. -**Menu breadth: 298 / 491 items (61%)** · 446 registered commands. +**Menu breadth: 298 / 491 items (61%)** · 449 registered commands. | Menu | Items | Covered | % | |---|---|---|---| From 0c486e337a880dee0f218d4b0692978637f9892f Mon Sep 17 00:00:00 2001 From: SBaksa Date: Sun, 1 Nov 2026 22:25:36 +1100 Subject: [PATCH 12/29] Engine: refuse rotating, scaling or mirroring an OLE frame on main's transform paths (#2587) Main moved the edit loop into edit_entities and gave SCALE its own scale_entities, so the merged check no longer compiled (CI: m not in scope). The refusal is now one helper that transform_entities and scale_entities call before anything changes. --- crates/engine/src/cmd/modify/transform.rs | 31 +++++++++++++++-------- 1 file changed, 20 insertions(+), 11 deletions(-) diff --git a/crates/engine/src/cmd/modify/transform.rs b/crates/engine/src/cmd/modify/transform.rs index e3765713c..b3b897847 100644 --- a/crates/engine/src/cmd/modify/transform.rs +++ b/crates/engine/src/cmd/modify/transform.rs @@ -5,6 +5,7 @@ use super::{Entity, Handle, Mat3, Result, Session, Value, Vec2, bad, bool_or, f6 // ---------------- core operations ---------------- pub(crate) fn transform_entities(s: &mut Session, hs: &[Handle], m: &Mat3, copy: bool) -> Result> { + refuse_ole_frames(s, hs, m)?; edit_entities(s, hs, copy, |e| e.kind.transform(m)) } @@ -12,6 +13,7 @@ pub(crate) fn transform_entities(s: &mut Session, hs: &[Handle], m: &Mat3, copy: /// and every height of the objects about its Z, their thickness too. pub(crate) fn scale_entities(s: &mut Session, hs: &[Handle], base: Vec3, f: f64, copy: bool) -> Result> { let m = Mat3::scale_about(base.xy(), f); + refuse_ole_frames(s, hs, &m)?; edit_entities(s, hs, copy, |e| { e.kind.transform(&m); e.kind.scale_heights(base.z, f); @@ -19,6 +21,24 @@ pub(crate) fn scale_entities(s: &mut Session, hs: &[Handle], base: Vec3, f: f64, }) } +/// An OLE2FRAME only translates (its corners stay axis aligned): refuse any other `m` before anything +/// changes, instead of reporting success and leaving the frame where it was. +fn refuse_ole_frames(s: &Session, hs: &[Handle], m: &Mat3) -> Result<()> { + if cadcraft_doc::ole_frame_accepts(m) { + return Ok(()); + } + let d = s.doc()?; + let ole = |h: &Handle| { + d.entity(*h).is_some_and(|e| matches!(&e.kind, cadcraft_doc::EntityKind::Unknown(u) if u.dxf_type.eq_ignore_ascii_case("OLE2FRAME"))) + }; + if hs.iter().any(ole) { + return Err(crate::EngineError::Other( + "An OLE object can only be moved or copied; rotating, scaling and mirroring it are not supported.".into(), + )); + } + Ok(()) +} + /// `p` at the current elevation (ELEVATION), the Z AutoCAD gives a point entered without one. pub(crate) fn at_elevation(s: &Session, p: Vec2) -> Vec3 { let z = s.doc().map(|d| d.header.f64("ELEVATION", 0.0)).ok().filter(|z| z.is_finite()); @@ -29,17 +49,6 @@ pub(crate) fn at_elevation(s: &Session, p: Vec2) -> Vec3 { /// on a locked layer is left alone unless it is copied. The handles edited or made. fn edit_entities(s: &mut Session, hs: &[Handle], copy: bool, edit: impl Fn(&mut Entity)) -> Result> { let d = s.doc_mut()?; - // An OLE2FRAME only translates (its corners stay axis aligned); refuse the rest before anything - // changes, instead of reporting success and leaving the frame where it was. - if !cadcraft_doc::ole_frame_accepts(m) - && hs.iter().any(|h| { - d.entity(*h).is_some_and(|e| matches!(&e.kind, cadcraft_doc::EntityKind::Unknown(u) if u.dxf_type.eq_ignore_ascii_case("OLE2FRAME"))) - }) - { - return Err(crate::EngineError::Other( - "An OLE object can only be moved or copied; rotating, scaling and mirroring it are not supported.".into(), - )); - } let mut out = Vec::new(); for h in hs { let Some(sp) = d.space_of(*h) else { continue }; From 7fdda542d359dd7d47163fba7e177dace53c3de0 Mon Sep 17 00:00:00 2001 From: SBaksa Date: Sun, 1 Nov 2026 23:32:58 +1100 Subject: [PATCH 13/29] OLE2FRAME: MOVE rewrites the corners in the OLE data only when they are the frame's corners (review on #2587) The data layout is only known when its corners match the frame's groups; any other header is left byte-identical while the tags still move. Test: move_leaves_ole_data_that_is_not_the_frames_corners_alone. --- crates/doc/src/extents.rs | 18 ++++++++++++++++-- crates/engine/tests/ole_frame_transform.rs | 18 ++++++++++++++++++ 2 files changed, 34 insertions(+), 2 deletions(-) diff --git a/crates/doc/src/extents.rs b/crates/doc/src/extents.rs index 4aff09d8b..651ac4b05 100644 --- a/crates/doc/src/extents.rs +++ b/crates/doc/src/extents.rs @@ -51,7 +51,7 @@ const OLE_CORNERS: usize = 12; /// Move the four corners at the start of the OLE data (the 310 groups' hex, in order) by `m`, /// keeping z and every tag's length. Data that is not valid hex or is too short is left alone. -fn ole_data_transform(tags: &mut [crate::RawTag], m: &cadcraft_geom::Mat3) { +fn ole_data_transform(tags: &mut [crate::RawTag], m: &cadcraft_geom::Mat3, frame: [f64; 4]) { let idx: Vec = outside_app(tags).into_iter().filter(|&i| tags.get(i).is_some_and(|t| t.code == 310)).collect(); let mut bytes: Vec = Vec::new(); let mut lens: Vec = Vec::new(); @@ -71,6 +71,19 @@ fn ole_data_transform(tags: &mut [crate::RawTag], m: &cadcraft_geom::Mat3) { if bytes.len() < end { return; } + // Only data whose corners are this frame's corners (UL, UR, LR, LL) has the layout above; any + // other header is not ours to rewrite. + let [x1, y1, x2, y2] = frame; + let want = [(x1, y1), (x2, y1), (x2, y2), (x1, y2)]; + let near = |a: f64, b: f64| (a - b).abs() <= 1e-9 * a.abs().max(b.abs()).max(1.0); + for (c, (wx, wy)) in want.iter().enumerate() { + let at = OLE_HEADER + c * 24; + let rd = |o: usize| bytes.get(o..o + 8).and_then(|s| <[u8; 8]>::try_from(s).ok()).map(f64::from_le_bytes); + let (Some(x), Some(y)) = (rd(at), rd(at + 8)) else { return }; + if !(near(x, *wx) && near(y, *wy)) { + return; + } + } let mut out = bytes.clone(); for c in 0..4 { let at = OLE_HEADER + c * 24; @@ -103,6 +116,7 @@ fn ole_data_transform(tags: &mut [crate::RawTag], m: &cadcraft_geom::Mat3) { /// left alone. pub fn ole_frame_transform(tags: &mut [crate::RawTag], m: &cadcraft_geom::Mat3) { let Some([(ix1, x1), (iy1, y1), (ix2, x2), (iy2, y2)]) = ole_corner_groups(tags) else { return }; + let frame = [x1, y1, x2, y2]; let (a, b) = (m.apply(Vec2::new(x1, y1)), m.apply(Vec2::new(x2, y2))); if !(a.x.is_finite() && a.y.is_finite() && b.x.is_finite() && b.y.is_finite()) { return; @@ -112,7 +126,7 @@ pub fn ole_frame_transform(tags: &mut [crate::RawTag], m: &cadcraft_geom::Mat3) t.value = v.to_string(); } } - ole_data_transform(tags, m); + ole_data_transform(tags, m, frame); } /// Whether `m` can move an OLE2FRAME: only a translation (move, copy, stretch of the whole frame). diff --git a/crates/engine/tests/ole_frame_transform.rs b/crates/engine/tests/ole_frame_transform.rs index 50ecc1f2f..22d88075a 100644 --- a/crates/engine/tests/ole_frame_transform.rs +++ b/crates/engine/tests/ole_frame_transform.rs @@ -124,3 +124,21 @@ fn move_leaves_malformed_or_short_ole_data_alone() { assert_eq!(corners(&s)[0].1, "40", "the corner groups still move"); } } + +/// OLE data whose bytes are not this frame's corners (an unknown header, so the layout does not +/// apply) is not rewritten by MOVE; the frame's own corner groups still move (review on #2587). +#[test] +fn move_leaves_ole_data_that_is_not_the_frames_corners_alone() { + let mut blob = vec![9u8, 9]; + for (x, y) in [(1.0f64, 2.0f64), (3.0, 2.0), (3.0, 4.0), (1.0, 4.0)] { + for v in [x, y, 7.0] { + blob.extend_from_slice(&v.to_le_bytes()); + } + } + let hex: String = blob.iter().map(|b| format!("{b:02X}")).collect(); + let (mut s, h) = data_session(&[40, 100], Some(&hex)); + let before = data_tags(&s); + s.execute("move", &json!({ "handles": [h], "delta": [10, 5] })).unwrap(); + assert_eq!(data_tags(&s), before); + assert_eq!(corners(&s)[0].1, "40", "the corner groups still move"); +} From c9ffc3e3943b75499b5ca6e063860bd0d32e9187 Mon Sep 17 00:00:00 2001 From: SBaksa Date: Sun, 1 Nov 2026 23:32:58 +1100 Subject: [PATCH 14/29] OLE2FRAME: a frame without usable corners counts as not drawn in an image export (review on #2587) The renderer draws nothing for it, so it is no longer reported as shown as the frame only. Test: an_ole_frame_without_corners_is_counted_as_not_drawn. --- apps/cadcraft-cli/src/output.rs | 2 +- apps/cadcraft-cli/tests/it/undrawn_entities.rs | 16 ++++++++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/apps/cadcraft-cli/src/output.rs b/apps/cadcraft-cli/src/output.rs index 8a8f7d97c..c810ede20 100644 --- a/apps/cadcraft-cli/src/output.rs +++ b/apps/cadcraft-cli/src/output.rs @@ -155,7 +155,7 @@ pub(crate) fn undrawn_message(d: &cadcraft_engine::doc::Drawing, path: &str) -> // An OLE2FRAME's frame outline is drawn, so it isn't an undrawn entity; its content is // not, which gets a note of its own. if let cadcraft_engine::doc::EntityKind::Unknown(u) = &e.kind { - if u.dxf_type.eq_ignore_ascii_case("OLE2FRAME") { + if u.dxf_type.eq_ignore_ascii_case("OLE2FRAME") && cadcraft_engine::doc::ole_frame(&u.tags).is_some() { ole = ole.saturating_add(1); continue; } diff --git a/apps/cadcraft-cli/tests/it/undrawn_entities.rs b/apps/cadcraft-cli/tests/it/undrawn_entities.rs index 8b176cbbc..59762d283 100644 --- a/apps/cadcraft-cli/tests/it/undrawn_entities.rs +++ b/apps/cadcraft-cli/tests/it/undrawn_entities.rs @@ -86,3 +86,19 @@ fn an_ole_frame_is_noted_as_frame_only() { let notes: Vec<&str> = err.lines().filter(|l| l.contains("OLE")).collect(); assert_eq!(notes, [format!("{}: 1 OLE object shown as the frame only (the OLE content is not drawn)", input.display())], "{err}"); } + +/// An OLE2FRAME without usable corners (here group 11/21 missing) has no outline to draw, so it is +/// an undrawn entity, not "shown as the frame only" (review on #2587). +#[test] +fn an_ole_frame_without_corners_is_counted_as_not_drawn() { + let scratch = Scratch::new("ole-nocorner"); + let dir = scratch.0.clone(); + let input = dir.join("ole.dxf"); + let ole = "0\nSECTION\n2\nENTITIES\n0\nOLE2FRAME\n8\n0\n100\nAcDbOle2Frame\n10\n0\n20\n0\n30\n0\n0\nENDSEC\n0\nEOF\n"; + std::fs::write(&input, ole).unwrap(); + let out = cli(&["convert", input.to_str().unwrap(), dir.join("out.png").to_str().unwrap()]); + assert!(out.status.success()); + let err = String::from_utf8_lossy(&out.stderr); + assert!(err.contains(&format!("{}: 1 entity not drawn (OLE2FRAME: 1)", input.display())), "{err}"); + assert!(!err.contains("frame only"), "{err}"); +} From b54adc597625ad647dac55b587d8a48af1298f7c Mon Sep 17 00:00:00 2001 From: SBaksa Date: Sun, 1 Nov 2026 23:32:59 +1100 Subject: [PATCH 15/29] OLE2FRAME: MOCORO checks before it changes anything; a locked frame doesn't block in-place edits (review on #2587) MOCORO refuses a rotate or scale step on an OLE frame up front, before an earlier move or copy is applied. refuse_ole_frames ignores a frame on a locked layer for in-place edits, as edit_entities does. Tests: mocoro_refuses_before_it_changes_anything, a_locked_ole_frame_does_not_block_an_in_place_rotation. --- crates/engine/src/cmd/modify.rs | 2 +- crates/engine/src/cmd/modify/transform.rs | 12 ++++--- crates/engine/src/cmd/modify2/mocoro.rs | 11 ++++++- crates/engine/tests/ole_frame_transform.rs | 38 ++++++++++++++++++++++ 4 files changed, 57 insertions(+), 6 deletions(-) diff --git a/crates/engine/src/cmd/modify.rs b/crates/engine/src/cmd/modify.rs index 22126f900..ad138348d 100644 --- a/crates/engine/src/cmd/modify.rs +++ b/crates/engine/src/cmd/modify.rs @@ -32,7 +32,7 @@ use fillet::{FilletM, run_chamfer, run_fillet}; use offset::{OffsetM, run_offset}; use select_then::{DisplaceM, Op, SelectThen}; use stretch::{StretchM, run_stretch}; -pub(crate) use transform::transform_entities; +pub(crate) use transform::{refuse_ole_frames, transform_entities}; use transform::{run_copy, run_erase, run_mirror, run_move, run_rotate, run_scale}; pub(crate) use trim::{Edges, extend, trim}; diff --git a/crates/engine/src/cmd/modify/transform.rs b/crates/engine/src/cmd/modify/transform.rs index b3b897847..9737afb8f 100644 --- a/crates/engine/src/cmd/modify/transform.rs +++ b/crates/engine/src/cmd/modify/transform.rs @@ -5,7 +5,7 @@ use super::{Entity, Handle, Mat3, Result, Session, Value, Vec2, bad, bool_or, f6 // ---------------- core operations ---------------- pub(crate) fn transform_entities(s: &mut Session, hs: &[Handle], m: &Mat3, copy: bool) -> Result> { - refuse_ole_frames(s, hs, m)?; + refuse_ole_frames(s, hs, m, copy)?; edit_entities(s, hs, copy, |e| e.kind.transform(m)) } @@ -13,7 +13,7 @@ pub(crate) fn transform_entities(s: &mut Session, hs: &[Handle], m: &Mat3, copy: /// and every height of the objects about its Z, their thickness too. pub(crate) fn scale_entities(s: &mut Session, hs: &[Handle], base: Vec3, f: f64, copy: bool) -> Result> { let m = Mat3::scale_about(base.xy(), f); - refuse_ole_frames(s, hs, &m)?; + refuse_ole_frames(s, hs, &m, copy)?; edit_entities(s, hs, copy, |e| { e.kind.transform(&m); e.kind.scale_heights(base.z, f); @@ -23,13 +23,17 @@ pub(crate) fn scale_entities(s: &mut Session, hs: &[Handle], base: Vec3, f: f64, /// An OLE2FRAME only translates (its corners stay axis aligned): refuse any other `m` before anything /// changes, instead of reporting success and leaving the frame where it was. -fn refuse_ole_frames(s: &Session, hs: &[Handle], m: &Mat3) -> Result<()> { +pub(crate) fn refuse_ole_frames(s: &Session, hs: &[Handle], m: &Mat3, copy: bool) -> Result<()> { if cadcraft_doc::ole_frame_accepts(m) { return Ok(()); } let d = s.doc()?; let ole = |h: &Handle| { - d.entity(*h).is_some_and(|e| matches!(&e.kind, cadcraft_doc::EntityKind::Unknown(u) if u.dxf_type.eq_ignore_ascii_case("OLE2FRAME"))) + d.entity(*h).is_some_and(|e| { + // An in-place edit skips an object on a locked layer (see edit_entities), so it cannot be refused. + let locked = !copy && d.layer(&e.common.layer).is_some_and(|l| l.locked); + !locked && matches!(&e.kind, cadcraft_doc::EntityKind::Unknown(u) if u.dxf_type.eq_ignore_ascii_case("OLE2FRAME")) + }) }; if hs.iter().any(ole) { return Err(crate::EngineError::Other( diff --git a/crates/engine/src/cmd/modify2/mocoro.rs b/crates/engine/src/cmd/modify2/mocoro.rs index c701e24ae..93732a7cf 100644 --- a/crates/engine/src/cmd/modify2/mocoro.rs +++ b/crates/engine/src/cmd/modify2/mocoro.rs @@ -16,7 +16,7 @@ use super::other; use crate::cmd::curves::KW; use crate::cmd::helpers::line; use crate::cmd::machines::{SelOutcome, SelectPhase, number}; -use crate::cmd::modify::transform_entities; +use crate::cmd::modify::{refuse_ole_frames, transform_entities}; use crate::cmd::{bad, point_req, point_value, targets}; use crate::prompt::Drag; use crate::{Accept, Input, Interactive, Prompt, Result, Session, Step}; @@ -108,6 +108,15 @@ pub(super) fn run_mocoro(s: &mut Session, p: &Value) -> Result { } // Every option is checked before the drawing changes. let ops = list.iter().enumerate().map(|(i, v)| parse_op(i, v)).collect::>>()?; + // An OLE frame only translates: a rotate or scale step that would refuse it must do so before an + // earlier move or copy changes the drawing (a refused command is not rolled back). + for op in &ops { + match *op { + Op::Rotate(a) => refuse_ole_frames(s, &objs, &Mat3::rotate_about(base, a), false)?, + Op::Scale(f) => refuse_ole_frames(s, &objs, &Mat3::scale_about(base, f), false)?, + _ => {} + } + } let mut copies = Vec::new(); for op in ops { copies.extend(apply(s, &objs, &mut base, op)?); diff --git a/crates/engine/tests/ole_frame_transform.rs b/crates/engine/tests/ole_frame_transform.rs index 22d88075a..653d9f017 100644 --- a/crates/engine/tests/ole_frame_transform.rs +++ b/crates/engine/tests/ole_frame_transform.rs @@ -142,3 +142,41 @@ fn move_leaves_ole_data_that_is_not_the_frames_corners_alone() { assert_eq!(data_tags(&s), before); assert_eq!(corners(&s)[0].1, "40", "the corner groups still move"); } + +#[test] +fn mocoro_refuses_before_it_changes_anything() { + let (mut s, h) = session(); + let before = corners(&s); + let undo = s.state().unwrap().undo.len(); + let ops = json!([{ "move": [10, 5] }, { "copy": [20, 0] }, { "rotate": 30 }]); + let e = s.execute("mocoro", &json!({ "handles": [h], "base": [0, 0], "ops": ops })).expect_err("rotate"); + assert!(e.to_string().contains("OLE object"), "{e}"); + assert_eq!(corners(&s), before, "the earlier move and copy were not applied"); + assert_eq!(s.doc().unwrap().model.len(), 1); + assert_eq!(s.state().unwrap().undo.len(), undo); +} + +#[test] +fn a_locked_ole_frame_does_not_block_an_in_place_rotation() { + let tag = |code: i32, v: &str| RawTag { code, value: v.into() }; + let mut d = Drawing::new_imperial(); + d.ensure_layer("L"); + let common = Common { layer: "L".into(), ..Common::default() }; + let kind = + EntityKind::Unknown(Unknown { dxf_type: "OLE2FRAME".into(), tags: vec![tag(10, "30"), tag(20, "-19"), tag(11, "35"), tag(21, "-22")] }); + let ole = d.add(&Space::Model, common, kind).unwrap(); + if let Some(l) = d.layer_mut("L") { + l.locked = true; + } + let mut s = Session::empty(); + s.open_drawing(d, "ole.dxf", None); + let ln = s.execute("line", &json!({ "points": [[0, 0], [10, 0]] })).unwrap(); + let ln = ln["handle"].as_str().or_else(|| ln["handles"][0].as_str()).unwrap().to_string(); + let before = corners(&s); + s.execute("rotate", &json!({ "handles": [ln.clone(), ole.hex()], "base": [0, 0], "angle": 90 })).unwrap(); + assert_eq!(corners(&s), before, "the locked frame is left alone"); + let d = s.doc().unwrap(); + let e = d.entity(cadcraft_engine::doc::Handle::parse_hex(&ln).unwrap()).unwrap(); + let cadcraft_engine::doc::EntityKind::Line(l) = &e.kind else { panic!("line") }; + assert!((l.b.y - 10.0).abs() < 1e-9 && l.b.x.abs() < 1e-9, "{l:?}"); +} From 9bd5c479e39be920139c223a4516d0e14d5bcb19 Mon Sep 17 00:00:00 2001 From: SBaksa Date: Tue, 3 Nov 2026 15:45:05 +1100 Subject: [PATCH 16/29] OLE2FRAME: SCALE carries a frame with a uniform positive factor (review on #2587) ole_frame_accepts took only a translation, so SCALE on a selection with a frame failed as a whole. A uniform positive scale keeps the frame axis-aligned and upper-left, so the corner rewrite handles it; rotation and mirroring are still refused. Tests: scale_carries_an_ole_frame_with_the_rest_of_the_selection, mocoro_scales_an_ole_frame, ole_frame_accepts_translations_and_uniform_positive_scales_only. --- crates/doc/src/extents.rs | 24 ++++++++-- crates/engine/src/cmd/modify/transform.rs | 4 +- crates/engine/src/cmd/modify2/mocoro.rs | 2 +- crates/engine/tests/ole_frame_transform.rs | 52 +++++++++++++++++++--- 4 files changed, 70 insertions(+), 12 deletions(-) diff --git a/crates/doc/src/extents.rs b/crates/doc/src/extents.rs index 568cb7023..596a194a5 100644 --- a/crates/doc/src/extents.rs +++ b/crates/doc/src/extents.rs @@ -109,7 +109,7 @@ fn ole_data_transform(tags: &mut [crate::RawTag], m: &cadcraft_geom::Mat3, frame } } -/// Move an OLE2FRAME by `m`, a translation: its two corners are mapped through `m` (a DWG save +/// Move an OLE2FRAME by `m`, a translation or a uniform positive scale: its two corners are mapped through `m` (a DWG save /// writes the frame's position from these groups), and so are the corners at the start of its OLE /// data (310 groups), which a reader of the saved file may trust instead. Other transforms do not keep a frame axis /// aligned, so callers refuse them (see [`ole_frame_accepts`]). A frame with no usable corners is @@ -129,10 +129,12 @@ pub fn ole_frame_transform(tags: &mut [crate::RawTag], m: &cadcraft_geom::Mat3) ole_data_transform(tags, m, frame); } -/// Whether `m` can move an OLE2FRAME: only a translation (move, copy, stretch of the whole frame). +/// Whether `m` can move an OLE2FRAME: a translation (move, copy, stretch of the whole frame) or a +/// uniform scale with a positive factor, which keep the frame axis aligned and its upper-left corner +/// upper-left. pub fn ole_frame_accepts(m: &cadcraft_geom::Mat3) -> bool { const T: f64 = 1e-12; - (m.a - 1.0).abs() < T && m.b.abs() < T && m.c.abs() < T && (m.d - 1.0).abs() < T + m.b.abs() < T && m.c.abs() < T && m.a > 0.0 && (m.a - m.d).abs() <= T * m.a.abs().max(1.0) } /// Bounds of a single-line TEXT / ATTDEF / ATTRIB of `len` characters, following its justification @@ -677,4 +679,20 @@ mod tests { assert!(extents.width() > drawn.width()); } } + + /// A translation, and a uniform positive scale about any point, keep a frame axis aligned and + /// upper-left; a rotation, a mirror, a non-uniform or non-positive scale do not. + #[test] + fn ole_frame_accepts_translations_and_uniform_positive_scales_only() { + use cadcraft_geom::Mat3; + let p = Vec2::new(3.0, -4.0); + assert!(ole_frame_accepts(&Mat3::translate(p))); + assert!(ole_frame_accepts(&Mat3::scale_about(p, 2.5))); + assert!(ole_frame_accepts(&Mat3::scale_about(p, 1e-3))); + assert!(!ole_frame_accepts(&Mat3::scale(2.0, 3.0))); + assert!(!ole_frame_accepts(&Mat3::scale_about(p, -1.0))); + assert!(!ole_frame_accepts(&Mat3::scale_about(p, 0.0))); + 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)))); + } } diff --git a/crates/engine/src/cmd/modify/transform.rs b/crates/engine/src/cmd/modify/transform.rs index 589fc4b2d..d1e6a463e 100644 --- a/crates/engine/src/cmd/modify/transform.rs +++ b/crates/engine/src/cmd/modify/transform.rs @@ -21,7 +21,7 @@ pub(crate) fn scale_entities(s: &mut Session, hs: &[Handle], base: Vec3, f: f64, }) } -/// An OLE2FRAME only translates (its corners stay axis aligned): refuse any other `m` before anything +/// An OLE2FRAME only translates or scales uniformly (its corners stay axis aligned): refuse any other `m` before anything /// changes, instead of reporting success and leaving the frame where it was. pub(crate) fn refuse_ole_frames(s: &Session, hs: &[Handle], m: &Mat3, copy: bool) -> Result<()> { if cadcraft_doc::ole_frame_accepts(m) { @@ -37,7 +37,7 @@ pub(crate) fn refuse_ole_frames(s: &Session, hs: &[Handle], m: &Mat3, copy: bool }; if hs.iter().any(ole) { return Err(crate::EngineError::Other( - "An OLE object can only be moved or copied; rotating, scaling and mirroring it are not supported.".into(), + "An OLE object can only be moved, copied or scaled uniformly; rotating and mirroring it are not supported.".into(), )); } Ok(()) diff --git a/crates/engine/src/cmd/modify2/mocoro.rs b/crates/engine/src/cmd/modify2/mocoro.rs index 93732a7cf..47d6833fb 100644 --- a/crates/engine/src/cmd/modify2/mocoro.rs +++ b/crates/engine/src/cmd/modify2/mocoro.rs @@ -108,7 +108,7 @@ pub(super) fn run_mocoro(s: &mut Session, p: &Value) -> Result { } // Every option is checked before the drawing changes. let ops = list.iter().enumerate().map(|(i, v)| parse_op(i, v)).collect::>>()?; - // An OLE frame only translates: a rotate or scale step that would refuse it must do so before an + // An OLE frame only translates or scales uniformly: a rotate step (or any step) that would refuse it must do so before an // earlier move or copy changes the drawing (a refused command is not rolled back). for op in &ops { match *op { diff --git a/crates/engine/tests/ole_frame_transform.rs b/crates/engine/tests/ole_frame_transform.rs index 653d9f017..2578c4e18 100644 --- a/crates/engine/tests/ole_frame_transform.rs +++ b/crates/engine/tests/ole_frame_transform.rs @@ -41,14 +41,12 @@ fn move_and_copy_carry_an_ole_frame() { } #[test] -fn rotate_scale_and_mirror_refuse_an_ole_frame() { +fn rotate_and_mirror_refuse_an_ole_frame() { let (mut s, h) = session(); let before = corners(&s); - for (cmd, p) in [ - ("rotate", json!({ "handles": [h], "base": [0, 0], "angle": 30 })), - ("scale", json!({ "handles": [h], "base": [0, 0], "factor": 2 })), - ("mirror", json!({ "handles": [h], "p1": [0, 0], "p2": [1, 0] })), - ] { + for (cmd, p) in + [("rotate", json!({ "handles": [h], "base": [0, 0], "angle": 30 })), ("mirror", json!({ "handles": [h], "p1": [0, 0], "p2": [1, 0] }))] + { let e = s.execute(cmd, &p).expect_err(cmd); assert!(e.to_string().contains("OLE object"), "{cmd}: {e}"); } @@ -180,3 +178,45 @@ fn a_locked_ole_frame_does_not_block_an_in_place_rotation() { let cadcraft_engine::doc::EntityKind::Line(l) = &e.kind else { panic!("line") }; assert!((l.b.y - 10.0).abs() < 1e-9 && l.b.x.abs() < 1e-9, "{l:?}"); } + +/// The four corners (x, y) in the OLE data header, read back from the 310 groups. +fn data_corners(s: &Session) -> Vec<(f64, f64)> { + let hex: String = data_tags(s).concat(); + let bytes: Vec = (0..hex.len() / 2).map(|i| u8::from_str_radix(&hex[2 * i..2 * i + 2], 16).unwrap()).collect(); + let f = |o: usize| f64::from_le_bytes(bytes[o..o + 8].try_into().unwrap()); + (0..4).map(|c| (f(2 + c * 24), f(10 + c * 24))).collect() +} + +fn line_in(s: &mut Session) -> String { + let ln = s.execute("line", &json!({ "points": [[1, 2], [4, 6]] })).unwrap(); + ln["handle"].as_str().or_else(|| ln["handles"][0].as_str()).unwrap().to_string() +} + +fn line_ends(s: &Session, h: &str) -> ((f64, f64), (f64, f64)) { + let e = s.doc().unwrap().entity(cadcraft_engine::doc::Handle::parse_hex(h).unwrap()).unwrap(); + let EntityKind::Line(l) = &e.kind else { panic!("line") }; + ((l.a.x, l.a.y), (l.b.x, l.b.y)) +} + +/// SCALE with a uniform positive factor carries an OLE frame with the rest of the selection: the +/// corner groups and the corners in the OLE data are both mapped (review on #2587). +#[test] +fn scale_carries_an_ole_frame_with_the_rest_of_the_selection() { + let (mut s, h) = data_session(&[10, 100, 40], None); + let ln = line_in(&mut s); + s.execute("scale", &json!({ "handles": [ln.clone(), h], "base": [0, 0], "factor": 2 })).unwrap(); + assert_eq!(line_ends(&s, &ln), ((2.0, 4.0), (8.0, 12.0))); + let want: Vec<(i32, String)> = [(10, "60"), (20, "-38"), (11, "70"), (21, "-44")].iter().map(|(c, v)| (*c, v.to_string())).collect(); + let tags: Vec<(i32, String)> = corners(&s).into_iter().filter(|(c, _)| *c != 310).collect(); + assert_eq!(tags, want); + assert_eq!(data_corners(&s), vec![(60.0, -38.0), (70.0, -38.0), (70.0, -44.0), (60.0, -44.0)]); +} + +/// MOCORO with a scale step carries the frame too, like SCALE. +#[test] +fn mocoro_scales_an_ole_frame() { + let (mut s, h) = data_session(&[10, 100, 40], None); + s.execute("mocoro", &json!({ "handles": [h], "base": [0, 0], "ops": [{ "scale": 3 }] })).unwrap(); + assert_eq!(data_corners(&s)[0], (90.0, -57.0)); + assert_eq!(corners(&s)[0].1, "90"); +} From 8cc2c4e8478b14a2bbdef6dd2be7cff274227e4b Mon Sep 17 00:00:00 2001 From: SBaksa Date: Tue, 3 Nov 2026 15:45:20 +1100 Subject: [PATCH 17/29] OLE2FRAME: corner match has a fixed ceiling at large coordinates (review on #2587) The relative tolerance grew with the coordinate: at 1e9 a payload 0.5 off the frame still matched and was rewritten. It is now capped at CORNER_EPS (1e-6), still above an ulp at 1e9. Tests: a_half_unit_offset_at_large_coordinates_is_not_the_frame, exact_corners_at_large_coordinates_still_move. --- crates/doc/src/extents.rs | 53 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 52 insertions(+), 1 deletion(-) diff --git a/crates/doc/src/extents.rs b/crates/doc/src/extents.rs index 596a194a5..4eaa0b754 100644 --- a/crates/doc/src/extents.rs +++ b/crates/doc/src/extents.rs @@ -75,7 +75,7 @@ fn ole_data_transform(tags: &mut [crate::RawTag], m: &cadcraft_geom::Mat3, frame // other header is not ours to rewrite. let [x1, y1, x2, y2] = frame; let want = [(x1, y1), (x2, y1), (x2, y2), (x1, y2)]; - let near = |a: f64, b: f64| (a - b).abs() <= 1e-9 * a.abs().max(b.abs()).max(1.0); + let near = |a: f64, b: f64| (a - b).abs() <= (1e-9 * a.abs().max(b.abs()).max(1.0)).min(CORNER_EPS); for (c, (wx, wy)) in want.iter().enumerate() { let at = OLE_HEADER + c * 24; let rd = |o: usize| bytes.get(o..o + 8).and_then(|s| <[u8; 8]>::try_from(s).ok()).map(f64::from_le_bytes); @@ -109,6 +109,12 @@ fn ole_data_transform(tags: &mut [crate::RawTag], m: &cadcraft_geom::Mat3, frame } } +/// Ceiling for the tolerance when matching the corners in the OLE data to the frame's. The header +/// holds the same f64 the frame groups print, so they agree to a few ulps; a fixed ceiling keeps +/// distinct geometry from matching at large coordinates, where a relative tolerance would grow to +/// whole units. +const CORNER_EPS: f64 = 1e-6; + /// Move an OLE2FRAME by `m`, a translation or a uniform positive scale: its two corners are mapped through `m` (a DWG save /// writes the frame's position from these groups), and so are the corners at the start of its OLE /// data (310 groups), which a reader of the saved file may trust instead. Other transforms do not keep a frame axis @@ -679,6 +685,51 @@ mod tests { assert!(extents.width() > drawn.width()); } } + /// An OLE2FRAME at large coordinates, with its OLE data header holding the frame's corners + /// shifted by `dx` in x. + fn big_frame(dx: f64) -> Vec { + let (x1, y1, x2, y2) = (1e9, 1e9, 1e9 + 10.0, 1e9 + 5.0); + let mut blob = vec![1u8, 0]; + for (x, y) in [(x1, y1), (x2, y1), (x2, y2), (x1, y2)] { + for v in [x + dx, y, 7.0] { + blob.extend_from_slice(&v.to_le_bytes()); + } + } + let tag = |code: i32, value: String| crate::RawTag { code, value }; + vec![ + tag(10, x1.to_string()), + tag(20, y1.to_string()), + tag(11, x2.to_string()), + tag(21, y2.to_string()), + tag(310, blob.iter().map(|b| format!("{b:02X}")).collect()), + ] + } + + /// The x of the first corner in the OLE data of a [`big_frame`]. + fn data_x(tags: &[crate::RawTag]) -> f64 { + let hex = tags.last().map(|t| t.value.as_str()).unwrap_or_default(); + let bytes: Vec = (0..hex.len() / 2).filter_map(|i| u8::from_str_radix(hex.get(2 * i..2 * i + 2)?, 16).ok()).collect(); + bytes.get(2..10).and_then(|s| <[u8; 8]>::try_from(s).ok()).map(f64::from_le_bytes).unwrap_or(f64::NAN) + } + + /// The match between the frame and the corners in its OLE data does not loosen with the + /// coordinate: half a unit off at 1e9 is not the frame, so the bytes stay as they are (review + /// on #2587). + #[test] + fn a_half_unit_offset_at_large_coordinates_is_not_the_frame() { + let mut tags = big_frame(0.5); + let before = tags.last().cloned(); + ole_frame_transform(&mut tags, &cadcraft_geom::Mat3::translate(Vec2::new(1.0, 1.0))); + assert_eq!(tags.first().map(|t| t.value.as_str()), Some("1000000001"), "the frame tags move"); + assert_eq!(tags.last().map(|t| &t.value), before.as_ref().map(|t| &t.value), "the 310 bytes stay identical"); + } + + #[test] + fn exact_corners_at_large_coordinates_still_move() { + let mut tags = big_frame(0.0); + ole_frame_transform(&mut tags, &cadcraft_geom::Mat3::translate(Vec2::new(1.0, 1.0))); + assert_eq!(data_x(&tags), 1e9 + 1.0); + } /// A translation, and a uniform positive scale about any point, keep a frame axis aligned and /// upper-left; a rotation, a mirror, a non-uniform or non-positive scale do not. From 48165e6cd93f3191794efbd1e5e4d5f82cdf03ea Mon Sep 17 00:00:00 2001 From: SBaksa Date: Tue, 3 Nov 2026 16:15:10 +1100 Subject: [PATCH 18/29] io tests: order the ole_frame module as rustfmt wants after the merge --- crates/io/tests/it/main.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/io/tests/it/main.rs b/crates/io/tests/it/main.rs index 29e695875..2ed30c966 100644 --- a/crates/io/tests/it/main.rs +++ b/crates/io/tests/it/main.rs @@ -47,9 +47,9 @@ mod mtext_columns_roundtrip; mod mtext_embedded_object; mod mtext_line_spacing; mod multiline_attributes; -mod ole_frame; #[cfg(not(target_arch = "wasm32"))] mod ole2frame_roundtrip; +mod ole_frame; mod paperview_marker; mod passthrough_image_roundtrip; mod pdf_followups; From 0a0a820759852c7017f38e59b3e26a3394bbb595 Mon Sep 17 00:00:00 2001 From: SBaksa Date: Tue, 3 Nov 2026 16:36:05 +1100 Subject: [PATCH 19/29] OLE2FRAME: any transform leaves the frame axis-aligned and its data consistent (review on #2587) EXPLODE, REFEDIT, paste and array placement call EntityKind::transform with rotations and mirrors, which left groups 10/20 not upper-left and the data header not matching. ole_frame_transform now takes the box of the two mapped corners (upper-left, lower-right) and writes the header as UL, UR, LR, LL of that box. Test: a_rotated_frame_stays_axis_aligned_and_consistent (rotate 30 and mirror). --- crates/doc/src/extents.rs | 72 ++++++++++++++++++++++++++++++++------- 1 file changed, 60 insertions(+), 12 deletions(-) diff --git a/crates/doc/src/extents.rs b/crates/doc/src/extents.rs index 4eaa0b754..5e52f5a1f 100644 --- a/crates/doc/src/extents.rs +++ b/crates/doc/src/extents.rs @@ -49,9 +49,10 @@ const OLE_HEADER: usize = 2; /// lower right, lower left). const OLE_CORNERS: usize = 12; -/// Move the four corners at the start of the OLE data (the 310 groups' hex, in order) by `m`, -/// keeping z and every tag's length. Data that is not valid hex or is too short is left alone. -fn ole_data_transform(tags: &mut [crate::RawTag], m: &cadcraft_geom::Mat3, frame: [f64; 4]) { +/// Replace the four corners at the start of the OLE data (the 310 groups' hex, in order) by `new` +/// (upper left, upper right, lower right, lower left), keeping z and every tag's length. Data that +/// is not valid hex, too short, or whose corners are not the old `frame`'s is left alone. +fn ole_data_transform(tags: &mut [crate::RawTag], new: [Vec2; 4], frame: [f64; 4]) { let idx: Vec = outside_app(tags).into_iter().filter(|&i| tags.get(i).is_some_and(|t| t.code == 310)).collect(); let mut bytes: Vec = Vec::new(); let mut lens: Vec = Vec::new(); @@ -88,11 +89,10 @@ fn ole_data_transform(tags: &mut [crate::RawTag], m: &cadcraft_geom::Mat3, frame for c in 0..4 { let at = OLE_HEADER + c * 24; let rd = |o: usize| out.get(o..o + 8).and_then(|s| <[u8; 8]>::try_from(s).ok()).map(f64::from_le_bytes).filter(|v| v.is_finite()); - let (Some(x), Some(y)) = (rd(at), rd(at + 8)) else { return }; - let p = m.apply(Vec2::new(x, y)); - if !(p.x.is_finite() && p.y.is_finite()) { + if rd(at).is_none() || rd(at + 8).is_none() { return; } + let Some(p) = new.get(c).copied() else { return }; if let Some(s) = out.get_mut(at..at + 8) { s.copy_from_slice(&p.x.to_le_bytes()); } @@ -115,10 +115,12 @@ fn ole_data_transform(tags: &mut [crate::RawTag], m: &cadcraft_geom::Mat3, frame /// whole units. const CORNER_EPS: f64 = 1e-6; -/// Move an OLE2FRAME by `m`, a translation or a uniform positive scale: its two corners are mapped through `m` (a DWG save -/// writes the frame's position from these groups), and so are the corners at the start of its OLE -/// data (310 groups), which a reader of the saved file may trust instead. Other transforms do not keep a frame axis -/// aligned, so callers refuse them (see [`ole_frame_accepts`]). A frame with no usable corners is +/// Transform an OLE2FRAME by `m`: its two corners are mapped through `m` (a DWG save writes the +/// frame's position from these groups), and so are the corners at the start of its OLE data (310 +/// groups), which a reader of the saved file may trust instead. The frame stays axis aligned with +/// its upper-left corner first: the result is the box of the two mapped corners. Commands refuse a +/// rotation or mirror (see [`ole_frame_accepts`]), but internal callers such as EXPLODE, REFEDIT and +/// paste still apply them, and must leave a consistent frame. A frame with no usable corners is /// left alone. pub fn ole_frame_transform(tags: &mut [crate::RawTag], m: &cadcraft_geom::Mat3) { let Some([(ix1, x1), (iy1, y1), (ix2, x2), (iy2, y2)]) = ole_corner_groups(tags) else { return }; @@ -127,12 +129,14 @@ pub fn ole_frame_transform(tags: &mut [crate::RawTag], m: &cadcraft_geom::Mat3) if !(a.x.is_finite() && a.y.is_finite() && b.x.is_finite() && b.y.is_finite()) { return; } - for (i, v) in [(ix1, a.x), (iy1, a.y), (ix2, b.x), (iy2, b.y)] { + let (lo, hi) = (Vec2::new(a.x.min(b.x), a.y.min(b.y)), Vec2::new(a.x.max(b.x), a.y.max(b.y))); + let (ul, lr) = (Vec2::new(lo.x, hi.y), Vec2::new(hi.x, lo.y)); + for (i, v) in [(ix1, ul.x), (iy1, ul.y), (ix2, lr.x), (iy2, lr.y)] { if let Some(t) = tags.get_mut(i) { t.value = v.to_string(); } } - ole_data_transform(tags, m, frame); + ole_data_transform(tags, [ul, Vec2::new(lr.x, ul.y), lr, Vec2::new(ul.x, lr.y)], frame); } /// Whether `m` can move an OLE2FRAME: a translation (move, copy, stretch of the whole frame) or a @@ -746,4 +750,48 @@ 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)))); } + /// 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(); + let bytes: Vec = (0..hex.len() / 2).filter_map(|i| u8::from_str_radix(hex.get(2 * i..2 * i + 2)?, 16).ok()).collect(); + let f = |o: usize| bytes.get(o..o + 8).and_then(|s| <[u8; 8]>::try_from(s).ok()).map(f64::from_le_bytes).unwrap_or(f64::NAN); + (0..4).map(|c| (f(2 + c * 24), f(10 + c * 24))).collect() + } + + fn small_frame() -> Vec { + let mut blob = vec![1u8, 0]; + for (x, y) in [(30.0f64, -19.0f64), (35.0, -19.0), (35.0, -22.0), (30.0, -22.0)] { + for v in [x, y, 7.0] { + blob.extend_from_slice(&v.to_le_bytes()); + } + } + let tag = |code: i32, value: &str| crate::RawTag { code, value: value.into() }; + vec![ + tag(10, "30"), + tag(20, "-19"), + tag(11, "35"), + tag(21, "-22"), + crate::RawTag { code: 310, value: blob.iter().map(|b| format!("{b:02X}")).collect() }, + ] + } + + /// Internal callers (EXPLODE, REFEDIT, paste, array placement) transform a frame by any matrix. + /// It stays axis aligned: groups 10/20 are the upper-left of the box of the mapped corners, + /// 11/21 the lower-right, and the OLE data holds the same box (review on #2587). + #[test] + fn a_rotated_frame_stays_axis_aligned_and_consistent() { + use cadcraft_geom::Mat3; + let rotate = Mat3::rotate_about(Vec2::new(1.0, 2.0), 30f64.to_radians()); + let mirror = Mat3::mirror(Vec2::new(0.0, 0.0), Vec2::new(1.0, 0.0)); + for m in [rotate, mirror] { + let mut tags = small_frame(); + let mapped = [m.apply(Vec2::new(30.0, -19.0)), m.apply(Vec2::new(35.0, -22.0))]; + let (minx, maxx) = (mapped[0].x.min(mapped[1].x), mapped[0].x.max(mapped[1].x)); + let (miny, maxy) = (mapped[0].y.min(mapped[1].y), mapped[0].y.max(mapped[1].y)); + ole_frame_transform(&mut tags, &m); + let g = |code: i32| tags.iter().find(|t| t.code == code).and_then(|t| t.value.parse::().ok()).unwrap_or(f64::NAN); + assert_eq!((g(10), g(20), g(11), g(21)), (minx, maxy, maxx, miny)); + assert_eq!(header_corners(&tags), vec![(minx, maxy), (maxx, maxy), (maxx, miny), (minx, miny)]); + } + } } From be413af02691c5ce7b3d5ab9474ab1e33021c03c Mon Sep 17 00:00:00 2001 From: SBaksa Date: Tue, 3 Nov 2026 16:37:51 +1100 Subject: [PATCH 20/29] OLE2FRAME: arrays refuse before adding copies; clearer refusal (review on #2587) ARRAYPATH with align and polar arrays that rotate items refused part way, leaving the earlier copies in the drawing. Every copy's matrix is now checked before the first copy is made. The refusal text names the other unsupported transforms. Test: arraypath_refuses_before_adding_any_copy. --- crates/engine/src/cmd/array.rs | 10 ++++++++-- crates/engine/src/cmd/modify/transform.rs | 2 +- crates/engine/src/cmd/modify2/patharray.rs | 13 +++++++++++-- crates/engine/tests/ole_frame_transform.rs | 16 ++++++++++++++++ 4 files changed, 36 insertions(+), 5 deletions(-) diff --git a/crates/engine/src/cmd/array.rs b/crates/engine/src/cmd/array.rs index da96ccfed..5fc16b310 100644 --- a/crates/engine/src/cmd/array.rs +++ b/crates/engine/src/cmd/array.rs @@ -247,9 +247,15 @@ fn max_count(s: &Session, others: u64, objs: usize) -> u64 { } fn copy_all(s: &mut Session, hs: &[Handle], mats: impl Iterator) -> Result { + // Every copy is checked before the first is made: a refusal part way would leave the copies + // already added in the drawing (a failed command is not rolled back). + let mats: Vec = mats.collect(); + for m in &mats { + super::modify::refuse_ole_frames(s, hs, m, true)?; + } let mut n = 0; - for m in mats { - n += transform_entities(s, hs, &m, true)?.len(); + for m in &mats { + n += transform_entities(s, hs, m, true)?.len(); } Ok(n) } diff --git a/crates/engine/src/cmd/modify/transform.rs b/crates/engine/src/cmd/modify/transform.rs index d1e6a463e..2fad5ee7b 100644 --- a/crates/engine/src/cmd/modify/transform.rs +++ b/crates/engine/src/cmd/modify/transform.rs @@ -37,7 +37,7 @@ pub(crate) fn refuse_ole_frames(s: &Session, hs: &[Handle], m: &Mat3, copy: bool }; if hs.iter().any(ole) { return Err(crate::EngineError::Other( - "An OLE object can only be moved, copied or scaled uniformly; rotating and mirroring it are not supported.".into(), + "An OLE object can only be moved, copied or scaled uniformly; other transforms (rotation, mirroring, non-uniform scale, alignment) are not supported.".into(), )); } Ok(()) diff --git a/crates/engine/src/cmd/modify2/patharray.rs b/crates/engine/src/cmd/modify2/patharray.rs index 3f9445249..aff058ab5 100644 --- a/crates/engine/src/cmd/modify2/patharray.rs +++ b/crates/engine/src/cmd/modify2/patharray.rs @@ -35,14 +35,23 @@ pub(crate) fn arraypath( }; crate::cmd::array::check_size(s, n as u64, objs.len())?; let Some((p0, t0)) = c.at_length(0.0) else { return Ok(Vec::new()) }; - let mut out = Vec::new(); + let mut mats = Vec::new(); for i in 1..n { let Some((p, t)) = c.at_length(step * i as f64) else { continue }; let mut m = Mat3::translate(p - p0); if align { m = Mat3::rotate_about(p, t.angle() - t0.angle()).then_before(m); } - out.extend(crate::cmd::modify::transform_entities(s, &objs, &m, true)?); + mats.push(m); + } + // Every copy is checked before the first is made: a refusal part way would leave the copies + // already added in the drawing (a failed command is not rolled back). + for m in &mats { + crate::cmd::modify::refuse_ole_frames(s, &objs, m, true)?; + } + let mut out = Vec::new(); + for m in &mats { + out.extend(crate::cmd::modify::transform_entities(s, &objs, m, true)?); } Ok(out) } diff --git a/crates/engine/tests/ole_frame_transform.rs b/crates/engine/tests/ole_frame_transform.rs index 2578c4e18..3569d39e5 100644 --- a/crates/engine/tests/ole_frame_transform.rs +++ b/crates/engine/tests/ole_frame_transform.rs @@ -220,3 +220,19 @@ fn mocoro_scales_an_ole_frame() { assert_eq!(data_corners(&s)[0], (90.0, -57.0)); assert_eq!(corners(&s)[0].1, "90"); } + +/// ARRAYPATH with align rotates each copy, which an OLE frame refuses. The refusal comes before +/// the first copy is added: an error does not roll the drawing back (review on #2587). +#[test] +fn arraypath_refuses_before_adding_any_copy() { + let (mut s, h) = session(); + // Straight for 30 units, then a right-angle bend: the first copies only move, a later one turns. + let path = s.execute("pline", &json!({ "points": [[0, 0], [30, 0], [30, 30]] })).unwrap(); + let path = path["handle"].as_str().or_else(|| path["handles"][0].as_str()).unwrap().to_string(); + let count = s.doc().unwrap().model.len(); + let before = corners(&s); + let e = s.execute("arraypath", &json!({ "handles": [h], "path": path, "count": 7, "align": true })).expect_err("align"); + assert!(e.to_string().contains("OLE object"), "{e}"); + assert_eq!(s.doc().unwrap().model.len(), count, "no copy was added"); + assert_eq!(corners(&s), before); +} From 457627df33ffcfa1bf94219b304c4e912b45b516 Mon Sep 17 00:00:00 2001 From: SBaksa Date: Tue, 3 Nov 2026 16:44:30 +1100 Subject: [PATCH 21/29] OLE2FRAME: validate hex strictly, rewrite only the header groups, cheaper corner lookup (review on #2587) A sign in a hex pair is no longer accepted; only the 310 groups holding the 98-byte header are decoded and rewritten, in place and without cloning the payload; the corner lookup is one allocation-free pass; the io ole_frame test module is not built for wasm like its DWG siblings; Unknown::is_ole_frame replaces five copies of the type check. Tests: a_plus_sign_in_ole_data_is_not_hex, a_large_ole_payload_only_rewrites_its_leading_groups. --- apps/cadcraft-cli/src/output.rs | 2 +- crates/doc/src/entity.rs | 9 +- crates/doc/src/extents.rs | 137 ++++++++++++++++------ crates/engine/src/cmd/modify/transform.rs | 2 +- crates/io/tests/it/main.rs | 1 + crates/render/src/draw.rs | 2 +- 6 files changed, 110 insertions(+), 43 deletions(-) diff --git a/apps/cadcraft-cli/src/output.rs b/apps/cadcraft-cli/src/output.rs index f1c742ed3..46283753e 100644 --- a/apps/cadcraft-cli/src/output.rs +++ b/apps/cadcraft-cli/src/output.rs @@ -167,7 +167,7 @@ pub(crate) fn undrawn_message(d: &cadcraft_engine::doc::Drawing, path: &str) -> // An OLE2FRAME's frame outline is drawn, so it isn't an undrawn entity; its content is // not, which gets a note of its own. if let cadcraft_engine::doc::EntityKind::Unknown(u) = &e.kind { - if u.dxf_type.eq_ignore_ascii_case("OLE2FRAME") && cadcraft_engine::doc::ole_frame(&u.tags).is_some() { + if u.is_ole_frame() && cadcraft_engine::doc::ole_frame(&u.tags).is_some() { ole = ole.saturating_add(1); continue; } diff --git a/crates/doc/src/entity.rs b/crates/doc/src/entity.rs index 33111e832..1237463f1 100644 --- a/crates/doc/src/entity.rs +++ b/crates/doc/src/entity.rs @@ -60,6 +60,13 @@ pub struct Unknown { pub tags: Vec, } +impl Unknown { + /// Whether this is an OLE2FRAME (the type name is matched without regard to case). + pub fn is_ole_frame(&self) -> bool { + self.dxf_type.eq_ignore_ascii_case("OLE2FRAME") + } +} + #[derive(Clone, Debug, PartialEq, Serialize, Deserialize)] #[serde(tag = "type", rename_all = "camelCase")] pub enum EntityKind { @@ -465,7 +472,7 @@ impl EntityKind { let a = if mirror { t.rotation() } else { t.rotation() + rot }; t.dir = Vec2::from_angle(cadcraft_geom::norm_angle(a)).to3(0.0); } - EntityKind::Unknown(u) if u.dxf_type.eq_ignore_ascii_case("OLE2FRAME") => crate::ole_frame_transform(&mut u.tags, m), + EntityKind::Unknown(u) if u.is_ole_frame() => crate::ole_frame_transform(&mut u.tags, m), EntityKind::Unknown(_) => {} } } diff --git a/crates/doc/src/extents.rs b/crates/doc/src/extents.rs index 5e52f5a1f..7713395a4 100644 --- a/crates/doc/src/extents.rs +++ b/crates/doc/src/extents.rs @@ -11,28 +11,40 @@ pub const MAX_BLOCK_DEPTH: usize = 16; /// Positions in `tags` of the groups outside application-defined `102 {` .. `102 }` groups, whose /// codes are the application's own and not the entity's. -fn outside_app(tags: &[crate::RawTag]) -> Vec { +fn outside_app(tags: &[crate::RawTag]) -> impl Iterator + '_ { let mut in_app = false; - let mut out = Vec::new(); - for (i, t) in tags.iter().enumerate() { + tags.iter().enumerate().filter_map(move |(i, t)| { if t.code == 102 { in_app = t.value.starts_with('{'); - } else if !in_app { - out.push(i); + None + } else { + (!in_app).then_some(i) } - } - out + }) } -/// The positions of the frame's corner groups 10, 20, 11, 21 (in that order) and their values. +/// The positions of the frame's corner groups 10, 20, 11, 21 (in that order) and their values: +/// the first of each outside application groups, found in one pass that stops at the last. fn ole_corner_groups(tags: &[crate::RawTag]) -> Option<[(usize, f64); 4]> { - let own = outside_app(tags); - let find = |code: i32| { - let i = own.iter().copied().find(|&i| tags.get(i).is_some_and(|t| t.code == code))?; - let v = tags.get(i)?.value.trim().parse::().ok().filter(|v| v.is_finite())?; - Some((i, v)) - }; - Some([find(10)?, find(20)?, find(11)?, find(21)?]) + let mut found: [Option<(usize, f64)>; 4] = [None; 4]; + for i in outside_app(tags) { + let Some(t) = tags.get(i) else { continue }; + let slot = match t.code { + 10 => 0, + 20 => 1, + 11 => 2, + 21 => 3, + _ => continue, + }; + if let Some(f) = found.get_mut(slot).filter(|f| f.is_none()) { + *f = Some((i, t.value.trim().parse::().ok().filter(|v| v.is_finite())?)); + if found.iter().all(Option::is_some) { + break; + } + } + } + let [Some(a), Some(b), Some(c), Some(d)] = found else { return None }; + Some([a, b, c, d]) } /// The closed outline of an OLE2FRAME from its upper-left (groups 10, 20) and lower-right @@ -50,26 +62,34 @@ const OLE_HEADER: usize = 2; const OLE_CORNERS: usize = 12; /// Replace the four corners at the start of the OLE data (the 310 groups' hex, in order) by `new` -/// (upper left, upper right, lower right, lower left), keeping z and every tag's length. Data that -/// is not valid hex, too short, or whose corners are not the old `frame`'s is left alone. +/// (upper left, upper right, lower right, lower left), keeping z and every group's length and +/// letter case. Only the groups that cover the header are decoded and rewritten; the rest of the +/// payload is not touched. Data that is not valid hex, too short, or whose corners are not the old +/// `frame`'s is left alone. fn ole_data_transform(tags: &mut [crate::RawTag], new: [Vec2; 4], frame: [f64; 4]) { - let idx: Vec = outside_app(tags).into_iter().filter(|&i| tags.get(i).is_some_and(|t| t.code == 310)).collect(); - let mut bytes: Vec = Vec::new(); - let mut lens: Vec = Vec::new(); - for &i in &idx { + const END: usize = OLE_HEADER + OLE_CORNERS * 8; + let mut bytes: Vec = Vec::with_capacity(END); + // The groups read, and how many leading bytes of each belong to the header. + let mut spans: Vec<(usize, usize)> = Vec::new(); + for i in outside_app(tags).filter(|&i| tags.get(i).is_some_and(|t| t.code == 310)) { + if bytes.len() >= END { + break; + } let Some(t) = tags.get(i) else { return }; let s = t.value.trim(); if s.len() % 2 != 0 || !s.is_ascii() { return; } - for k in (0..s.len()).step_by(2) { - let Some(b) = s.get(k..k + 2).and_then(|h| u8::from_str_radix(h, 16).ok()) else { return }; + let take = (s.len() / 2).min(END - bytes.len()); + for k in 0..take { + // `from_str_radix` also takes a sign, so each digit is checked first. + let Some(h) = s.get(2 * k..2 * k + 2).filter(|h| h.bytes().all(|b| b.is_ascii_hexdigit())) else { return }; + let Ok(b) = u8::from_str_radix(h, 16) else { return }; bytes.push(b); } - lens.push(s.len() / 2); + spans.push((i, take)); } - let end = OLE_HEADER + OLE_CORNERS * 8; - if bytes.len() < end { + if bytes.len() < END { return; } // Only data whose corners are this frame's corners (UL, UR, LR, LL) has the layout above; any @@ -85,26 +105,23 @@ fn ole_data_transform(tags: &mut [crate::RawTag], new: [Vec2; 4], frame: [f64; 4 return; } } - let mut out = bytes.clone(); - for c in 0..4 { + for (c, p) in new.iter().enumerate() { let at = OLE_HEADER + c * 24; - let rd = |o: usize| out.get(o..o + 8).and_then(|s| <[u8; 8]>::try_from(s).ok()).map(f64::from_le_bytes).filter(|v| v.is_finite()); - if rd(at).is_none() || rd(at + 8).is_none() { - return; - } - let Some(p) = new.get(c).copied() else { return }; - if let Some(s) = out.get_mut(at..at + 8) { + if let Some(s) = bytes.get_mut(at..at + 8) { s.copy_from_slice(&p.x.to_le_bytes()); } - if let Some(s) = out.get_mut(at + 8..at + 16) { + if let Some(s) = bytes.get_mut(at + 8..at + 16) { s.copy_from_slice(&p.y.to_le_bytes()); } } let mut pos = 0; - for (&i, &n) in idx.iter().zip(&lens) { - let (Some(t), Some(chunk)) = (tags.get_mut(i), out.get(pos..pos + n)) else { return }; + for (i, n) in spans { + let (Some(t), Some(chunk)) = (tags.get_mut(i), bytes.get(pos..pos + n)) else { return }; let lower = t.value.bytes().any(|b| b.is_ascii_lowercase()); - t.value = chunk.iter().map(|b| if lower { format!("{b:02x}") } else { format!("{b:02X}") }).collect(); + let hex: String = chunk.iter().map(|b| if lower { format!("{b:02x}") } else { format!("{b:02X}") }).collect(); + // Leave any padding around the digits, and the digits after the header, as they were. + let start = t.value.len() - t.value.trim_start().len(); + t.value.replace_range(start..start + 2 * n, &hex); pos += n; } } @@ -410,7 +427,7 @@ fn bounds_in<'a>(d: &'a Drawing, e: &Entity, depth: usize, blocks: &mut HashMap< } EntityKind::LwPolyline(p) => lwpolyline_bounds(p), // An OLE object's content is not drawn, but its frame is, so the frame counts (#2422). - EntityKind::Unknown(u) if u.dxf_type.eq_ignore_ascii_case("OLE2FRAME") => ole_frame(&u.tags).map_or(Bounds2::EMPTY, Bounds2::from_points), + EntityKind::Unknown(u) if u.is_ole_frame() => ole_frame(&u.tags).map_or(Bounds2::EMPTY, Bounds2::from_points), kind => { let mut b = Bounds2::EMPTY; for p in kind.prims() { @@ -794,4 +811,46 @@ mod tests { assert_eq!(header_corners(&tags), vec![(minx, maxy), (maxx, maxy), (maxx, miny), (minx, miny)]); } } + /// `from_str_radix` accepts a leading sign, so "+A" must not count as a hex byte (review on + /// #2587): data with one is not valid and MOVE leaves its bytes alone. + #[test] + fn a_plus_sign_in_ole_data_is_not_hex() { + let mut tags = small_frame(); + let hex = tags.last().map(|t| t.value.clone()).unwrap_or_default(); + // The marker's first byte is 01: "+1" would decode to the same byte. + let signed = format!("+1{}", hex.get(2..).unwrap_or_default()); + if let Some(t) = tags.last_mut() { + t.value = signed.clone(); + } + ole_frame_transform(&mut tags, &cadcraft_geom::Mat3::translate(Vec2::new(10.0, 5.0))); + assert_eq!(tags.first().map(|t| t.value.as_str()), Some("40"), "the frame tags still move"); + assert_eq!(tags.last().map(|t| t.value.as_str()), Some(signed.as_str())); + } + + /// A payload of 1,000 groups of 127 bytes: only the group(s) holding the 98-byte header are + /// rewritten. Later groups stay as they were, even ones that are not valid hex or have padding + /// (the old code re-encoded the whole payload and gave up on any bad group). + #[test] + fn a_large_ole_payload_only_rewrites_its_leading_groups() { + let mut tags = small_frame(); + let first = tags.pop().map(|t| t.value).unwrap_or_default(); + let mut head = first.clone(); + // Fill the first group up to 127 bytes. + head.push_str(&"ab".repeat(127 - first.len() / 2)); + tags.push(crate::RawTag { code: 310, value: head }); + for g in 1..1000 { + let value = match g { + 2 => format!(" {} ", "Ab".repeat(127)), + 500 => "ZZ".repeat(127), + _ => "ab".repeat(127), + }; + tags.push(crate::RawTag { code: 310, value }); + } + let rest: Vec = tags.iter().skip(5).map(|t| t.value.clone()).collect(); + ole_frame_transform(&mut tags, &cadcraft_geom::Mat3::translate(Vec2::new(10.0, 5.0))); + assert_eq!(header_corners(&tags[..5]), vec![(40.0, -14.0), (45.0, -14.0), (45.0, -17.0), (40.0, -17.0)]); + let after: Vec = tags.iter().skip(5).map(|t| t.value.clone()).collect(); + assert_eq!(after, rest, "every group after the first is string-identical"); + assert_eq!(tags.get(4).map(|t| t.value.len()), Some(254), "the first group keeps its length"); + } } diff --git a/crates/engine/src/cmd/modify/transform.rs b/crates/engine/src/cmd/modify/transform.rs index 2fad5ee7b..219468f93 100644 --- a/crates/engine/src/cmd/modify/transform.rs +++ b/crates/engine/src/cmd/modify/transform.rs @@ -32,7 +32,7 @@ pub(crate) fn refuse_ole_frames(s: &Session, hs: &[Handle], m: &Mat3, copy: bool d.entity(*h).is_some_and(|e| { // An in-place edit skips an object on a locked layer (see edit_entities), so it cannot be refused. let locked = !copy && d.layer(&e.common.layer).is_some_and(|l| l.locked); - !locked && matches!(&e.kind, cadcraft_doc::EntityKind::Unknown(u) if u.dxf_type.eq_ignore_ascii_case("OLE2FRAME")) + !locked && matches!(&e.kind, cadcraft_doc::EntityKind::Unknown(u) if u.is_ole_frame()) }) }; if hs.iter().any(ole) { diff --git a/crates/io/tests/it/main.rs b/crates/io/tests/it/main.rs index 2ed30c966..55b96bdc8 100644 --- a/crates/io/tests/it/main.rs +++ b/crates/io/tests/it/main.rs @@ -49,6 +49,7 @@ mod mtext_line_spacing; mod multiline_attributes; #[cfg(not(target_arch = "wasm32"))] mod ole2frame_roundtrip; +#[cfg(not(target_arch = "wasm32"))] mod ole_frame; mod paperview_marker; mod passthrough_image_roundtrip; diff --git a/crates/render/src/draw.rs b/crates/render/src/draw.rs index abfe536e8..3b1b6bbbb 100644 --- a/crates/render/src/draw.rs +++ b/crates/render/src/draw.rs @@ -235,7 +235,7 @@ fn entity_parts(b: &mut Builder, ctx: &Ctx, e: &Entity) { } } // An OLE object's content is not drawn; its frame, from its two corner points, is. - EntityKind::Unknown(u) if u.dxf_type.eq_ignore_ascii_case("OLE2FRAME") => { + EntityKind::Unknown(u) if u.is_ole_frame() => { if let Some(q) = cadcraft_doc::ole_frame(&u.tags) { b.polyline(ctx, rgb, lw, &q); } From b2ded208881f0fed4aa8a96cfe20ec7f9da7a7c6 Mon Sep 17 00:00:00 2001 From: SBaksa Date: Thu, 5 Nov 2026 12:36:59 +1100 Subject: [PATCH 22/29] 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); +} From c397ab1e9e1f212887ae243084794c59cf78971e Mon Sep 17 00:00:00 2001 From: SBaksa Date: Thu, 5 Nov 2026 13:55:27 +1100 Subject: [PATCH 23/29] OLE2FRAME: the frame can be picked by its edges (review on #2587) An OLE2FRAME is drawn but prims() gave nothing for it, so hit testing never saw it and a click on its outline picked nothing. prims() now returns the frame's four edges from the same corner helper the extents use, so the extents arm is gone. The renderer keeps its closed-polyline arm (one joined outline, not four segments). Test: clicking_an_ole_frame_edge_selects_it. --- crates/doc/src/entity.rs | 4 ++++ crates/doc/src/extents.rs | 2 -- crates/engine/src/select.rs | 15 +++++++++++++++ 3 files changed, 19 insertions(+), 2 deletions(-) diff --git a/crates/doc/src/entity.rs b/crates/doc/src/entity.rs index 1237463f1..d24f7f49b 100644 --- a/crates/doc/src/entity.rs +++ b/crates/doc/src/entity.rs @@ -264,6 +264,10 @@ impl EntityKind { EntityKind::Image(i) => vec![Prim::Point(i.insert.xy())], EntityKind::Table(t) => vec![Prim::Point(t.insert.xy())], EntityKind::Tolerance(t) => vec![Prim::Point(t.insert.xy())], + // An OLE object's content is not drawn, but its frame is: the edges between its corners. + EntityKind::Unknown(u) if u.is_ole_frame() => crate::ole_frame(&u.tags) + .map(|q| q.windows(2).filter_map(|w| Some(Prim::Seg(Segment::Line(GLine::new(*w.first()?, *w.get(1)?))))).collect()) + .unwrap_or_default(), EntityKind::Unknown(_) => Vec::new(), } } diff --git a/crates/doc/src/extents.rs b/crates/doc/src/extents.rs index 55138649d..6584980ac 100644 --- a/crates/doc/src/extents.rs +++ b/crates/doc/src/extents.rs @@ -429,8 +429,6 @@ fn bounds_in<'a>(d: &'a Drawing, e: &Entity, depth: usize, blocks: &mut HashMap< Bounds2::from_points([o, o + u, o + v, o + u + v]) } EntityKind::LwPolyline(p) => lwpolyline_bounds(p), - // An OLE object's content is not drawn, but its frame is, so the frame counts (#2422). - EntityKind::Unknown(u) if u.is_ole_frame() => ole_frame(&u.tags).map_or(Bounds2::EMPTY, Bounds2::from_points), kind => { let mut b = Bounds2::EMPTY; for p in kind.prims() { diff --git a/crates/engine/src/select.rs b/crates/engine/src/select.rs index 6721cdad8..a3d361e84 100644 --- a/crates/engine/src/select.rs +++ b/crates/engine/src/select.rs @@ -507,6 +507,21 @@ mod tests { assert_eq!(pick(&d, &Space::Model, Vec2::new(41.3, 50.0), 1.0), Some(line)); } + /// An OLE2FRAME draws as its frame, so a click on an edge picks it (review on #2587); a click in + /// the middle of the frame, or on a frame without both corners, does not. + #[test] + fn clicking_an_ole_frame_edge_selects_it() { + let tag = |code: i32, value: &str| cadcraft_doc::RawTag { code, value: value.into() }; + let frame = |tags: Vec| EntityKind::Unknown(cadcraft_doc::Unknown { dxf_type: "OLE2FRAME".into(), tags }); + let mut d = Drawing::new_metric(); + let h = d.add(&Space::Model, Common::default(), frame(vec![tag(10, "30"), tag(20, "-19"), tag(11, "35"), tag(21, "-22")])).unwrap(); + d.add(&Space::Model, Common::default(), frame(vec![tag(10, "80"), tag(20, "80")])).unwrap(); + assert_eq!(pick(&d, &Space::Model, Vec2::new(32.5, -19.0), 0.2), Some(h), "top edge"); + assert_eq!(pick(&d, &Space::Model, Vec2::new(35.0, -20.5), 0.2), Some(h), "right edge"); + assert_eq!(pick(&d, &Space::Model, Vec2::new(32.5, -20.5), 0.2), None, "inside the frame"); + assert_eq!(pick(&d, &Space::Model, Vec2::new(80.0, 80.0), 0.2), None, "a frame without a second corner"); + } + /// A block holding a diagonal dashed line, inserted at `scale`. fn dashed_block(length: f64, scale: f64) -> (Drawing, Handle) { let mut d = Drawing::new_imperial(); From a779dadad597b0821f7d6ba1f3ba665dba9cb711 Mon Sep 17 00:00:00 2001 From: SBaksa Date: Thu, 5 Nov 2026 13:57:06 +1100 Subject: [PATCH 24/29] OLE2FRAME: corner match and rotation bound scale with the coordinates; hex keeps its case (review on #2587) The corner match was capped at 1e-6, under one ulp above about 8.6e9, so a frame there never matched its header; a few ulps of the larger value now also match. The rotation bound had a floor of 1 on the scale, so a small real turn at a tiny scale passed; it is now purely relative. Hex digits are re-encoded in the case of the digit at the same place, so untouched bytes keep their text. Tests: ole_frame_moves_at_2e10_with_a_one_ulp_text_corner, ole_frame_refuses_a_small_turn_at_a_tiny_scale, moving_an_ole_frame_keeps_each_hex_digits_case. --- crates/doc/src/extents.rs | 102 ++++++++++++++++++++++++++++++++++++-- 1 file changed, 98 insertions(+), 4 deletions(-) diff --git a/crates/doc/src/extents.rs b/crates/doc/src/extents.rs index 6584980ac..2e3f16a48 100644 --- a/crates/doc/src/extents.rs +++ b/crates/doc/src/extents.rs @@ -96,7 +96,12 @@ fn ole_data_transform(tags: &mut [crate::RawTag], new: [Vec2; 4], frame: [f64; 4 // other header is not ours to rewrite. let [x1, y1, x2, y2] = frame; let want = [(x1, y1), (x2, y1), (x2, y2), (x1, y2)]; - let near = |a: f64, b: f64| (a - b).abs() <= (1e-9 * a.abs().max(b.abs()).max(1.0)).min(CORNER_EPS); + // The floor is the fixed-ceiling match; above about 8.6e9 one ulp exceeds it, so a few ulps of the + // larger value also match. + let near = |a: f64, b: f64| { + let m = a.abs().max(b.abs()); + (a - b).abs() <= (1e-9 * m.max(1.0)).min(CORNER_EPS).max(4.0 * f64::EPSILON * m) + }; for (c, (wx, wy)) in want.iter().enumerate() { let at = OLE_HEADER + c * 24; let rd = |o: usize| bytes.get(o..o + 8).and_then(|s| <[u8; 8]>::try_from(s).ok()).map(f64::from_le_bytes); @@ -117,15 +122,39 @@ fn ole_data_transform(tags: &mut [crate::RawTag], new: [Vec2; 4], frame: [f64; 4 let mut pos = 0; for (i, n) in spans { let (Some(t), Some(chunk)) = (tags.get_mut(i), bytes.get(pos..pos + n)) else { return }; - let lower = t.value.bytes().any(|b| b.is_ascii_lowercase()); - let hex: String = chunk.iter().map(|b| if lower { format!("{b:02x}") } else { format!("{b:02X}") }).collect(); // Leave any padding around the digits, and the digits after the header, as they were. let start = t.value.len() - t.value.trim_start().len(); + let hex = recase(chunk, t.value.get(start..).unwrap_or_default()); t.value.replace_range(start..start + 2 * n, &hex); pos += n; } } +/// The hex of `chunk`, each digit in the case of the digit at the same place in `old`. A digit that +/// was a number and becomes a letter takes the case of the nearest letter in `old`, upper when there +/// is none, so bytes that did not change keep their exact text. +fn recase(chunk: &[u8], old: &str) -> String { + let old = old.as_bytes(); + let lower_at = |i: usize| -> bool { + if let Some(c) = old.get(i).filter(|c| c.is_ascii_alphabetic()) { + return c.is_ascii_lowercase(); + } + let nearest = (1..=old.len()).find_map(|d| { + let before = i.checked_sub(d).and_then(|j| old.get(j)).filter(|c| c.is_ascii_alphabetic()); + before.or_else(|| old.get(i + d).filter(|c| c.is_ascii_alphabetic())) + }); + nearest.is_some_and(|c| c.is_ascii_lowercase()) + }; + let mut out = String::with_capacity(chunk.len() * 2); + for (k, b) in chunk.iter().enumerate() { + for (j, d) in [b >> 4, b & 15].into_iter().enumerate() { + let digit = char::from_digit(d.into(), 16).unwrap_or('0'); + out.push(if lower_at(2 * k + j) { digit } else { digit.to_ascii_uppercase() }); + } + } + out +} + /// Ceiling for the tolerance when matching the corners in the OLE data to the frame's. The header /// holds the same f64 the frame groups print, so they agree to a few ulps; a fixed ceiling keeps /// distinct geometry from matching at large coordinates, where a relative tolerance would grow to @@ -163,7 +192,7 @@ pub fn ole_frame_accepts(m: &cadcraft_geom::Mat3) -> bool { const T: f64 = 1e-12; // 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); + let t = T * m.a.abs(); m.b.abs() < t && m.c.abs() < t && m.a > 0.0 && (m.a - m.d).abs() <= t } @@ -868,4 +897,69 @@ mod tests { assert_eq!(after, rest, "every group after the first is string-identical"); assert_eq!(tags.get(4).map(|t| t.value.len()), Some(254), "the first group keeps its length"); } + + /// Above about 8.6e9 one ulp of an f64 is more than the fixed ceiling on the corner match, so a + /// text corner one ulp from the header double must still count as the same corner (review on + /// #2587). + #[test] + fn ole_frame_moves_at_2e10_with_a_one_ulp_text_corner() { + let mut tags = big_frame(0.0); + // Rebuild the frame at 2e10: the header keeps the exact corners, the text is one ulp off. + let (x1, y1, x2, y2) = (2e10f64, 2e10f64, 2e10 + 10.0, 2e10 + 5.0); + let mut blob = vec![1u8, 0]; + for (x, y) in [(x1, y1), (x2, y1), (x2, y2), (x1, y2)] { + for v in [x, y, 7.0] { + blob.extend_from_slice(&v.to_le_bytes()); + } + } + let text_x1 = f64::from_bits(x1.to_bits() + 1); + tags[0].value = text_x1.to_string(); + tags[1].value = y1.to_string(); + tags[2].value = x2.to_string(); + tags[3].value = y2.to_string(); + if let Some(t) = tags.last_mut() { + t.value = blob.iter().map(|b| format!("{b:02X}")).collect(); + } + ole_frame_transform(&mut tags, &cadcraft_geom::Mat3::translate(Vec2::new(1e4, 1e4))); + assert_eq!(header_corners(&tags)[0].0, text_x1 + 1e4, "the header moved with the frame"); + } + + /// Below scale 1 the rotation bound is still relative to the scale: a small real turn at a tiny + /// scale is refused, and so is a non-uniform scale whose two factors differ by more than a + /// relative 1e-12 (review on #2587). + #[test] + fn ole_frame_refuses_a_small_turn_at_a_tiny_scale() { + use cadcraft_geom::Mat3; + let p = Vec2::new(3.0, -4.0); + let turned = Mat3::scale_about(p, 1e-9).then_before(Mat3::rotate_about(p, 0.05f64.to_radians())); + assert!(!ole_frame_accepts(&turned)); + assert!(ole_frame_accepts(&Mat3::scale_about(p, 1e-9))); + assert!(!ole_frame_accepts(&Mat3::scale(1e-6, 1e-6 + 5e-13))); + } + + /// Hex digits keep the case they had: bytes the transform does not change come back as the + /// identical text, also in a group that mixes upper and lower case (review on #2587). + #[test] + fn moving_an_ole_frame_keeps_each_hex_digits_case() { + let mut tags = small_frame(); + let old = tags.last().map(|t| t.value.clone()).unwrap_or_default(); + let mixed: String = old.chars().enumerate().map(|(i, c)| if i < 24 { c.to_ascii_lowercase() } else { c }).collect(); + assert_ne!(mixed, old); + if let Some(t) = tags.last_mut() { + t.value = mixed.clone(); + } + ole_frame_transform(&mut tags, &cadcraft_geom::Mat3::translate(Vec2::new(10.0, 5.0))); + let new = tags.last().map(|t| t.value.clone()).unwrap_or_default(); + assert_eq!(new.len(), mixed.len()); + let byte = |s: &str, i: usize| s.get(2 * i..2 * i + 2).and_then(|h| u8::from_str_radix(h, 16).ok()); + let mut same = 0; + for i in 0..mixed.len() / 2 { + if byte(&mixed, i) == byte(&new, i) { + same += 1; + assert_eq!(mixed.get(2 * i..2 * i + 2), new.get(2 * i..2 * i + 2), "byte {i}"); + } + } + assert!(same > 40, "most of the header is unchanged: {same}"); + assert_eq!(header_corners(&tags)[0], (40.0, -14.0)); + } } From 485c74da6b7bb798805c743b636ebd2e9b0d6411 Mon Sep 17 00:00:00 2001 From: SBaksa Date: Thu, 5 Nov 2026 13:58:15 +1100 Subject: [PATCH 25/29] ARRAY: check OLE frames once (review on #2587) copy_all scanned the selection for an OLE frame once per matrix before copying. It now looks at the objects once and then only tests each matrix. Covered by the existing refusal tests (classic_polar_array_refuses_a_turned_frame_but_not_an_upright_one). --- crates/engine/src/cmd/array.rs | 5 +++-- crates/engine/src/cmd/modify.rs | 2 +- crates/engine/src/cmd/modify/transform.rs | 26 ++++++++++++++--------- 3 files changed, 20 insertions(+), 13 deletions(-) diff --git a/crates/engine/src/cmd/array.rs b/crates/engine/src/cmd/array.rs index 5fc16b310..c4948c9e0 100644 --- a/crates/engine/src/cmd/array.rs +++ b/crates/engine/src/cmd/array.rs @@ -250,8 +250,9 @@ fn copy_all(s: &mut Session, hs: &[Handle], mats: impl Iterator) -> // Every copy is checked before the first is made: a refusal part way would leave the copies // already added in the drawing (a failed command is not rolled back). let mats: Vec = mats.collect(); - for m in &mats { - super::modify::refuse_ole_frames(s, hs, m, true)?; + // The objects are looked at once; then a matrix only needs its own check. + if super::modify::holds_ole_frame(s, hs, true)? && mats.iter().any(|m| !cadcraft_doc::ole_frame_accepts(m)) { + return Err(super::modify::ole_refusal()); } let mut n = 0; for m in &mats { diff --git a/crates/engine/src/cmd/modify.rs b/crates/engine/src/cmd/modify.rs index 825add837..f9fae40e2 100644 --- a/crates/engine/src/cmd/modify.rs +++ b/crates/engine/src/cmd/modify.rs @@ -35,7 +35,7 @@ use oops::run_oops; pub(crate) use oops::{Erased, erase_kept}; use select_then::{DisplaceM, Op, SelectThen}; use stretch::{StretchM, run_stretch}; -pub(crate) use transform::{refuse_ole_frames, transform_entities}; +pub(crate) use transform::{holds_ole_frame, ole_refusal, refuse_ole_frames, transform_entities}; use transform::{run_copy, run_erase, run_mirror, run_move, run_rotate, run_scale}; pub(crate) use trim::{Edges, extend, trim}; diff --git a/crates/engine/src/cmd/modify/transform.rs b/crates/engine/src/cmd/modify/transform.rs index 219468f93..df43f782c 100644 --- a/crates/engine/src/cmd/modify/transform.rs +++ b/crates/engine/src/cmd/modify/transform.rs @@ -24,23 +24,29 @@ pub(crate) fn scale_entities(s: &mut Session, hs: &[Handle], base: Vec3, f: f64, /// An OLE2FRAME only translates or scales uniformly (its corners stay axis aligned): refuse any other `m` before anything /// changes, instead of reporting success and leaving the frame where it was. pub(crate) fn refuse_ole_frames(s: &Session, hs: &[Handle], m: &Mat3, copy: bool) -> Result<()> { - if cadcraft_doc::ole_frame_accepts(m) { + if cadcraft_doc::ole_frame_accepts(m) || !holds_ole_frame(s, hs, copy)? { return Ok(()); } + Err(ole_refusal()) +} + +/// The refusal for a transform an OLE2FRAME cannot take. +pub(crate) fn ole_refusal() -> crate::EngineError { + crate::EngineError::Other( + "An OLE object can only be moved, copied or scaled uniformly; other transforms (rotation, mirroring, non-uniform scale, alignment) are not supported.".into(), + ) +} + +/// Whether `hs` holds an OLE2FRAME a transform would touch. An in-place edit (`copy` false) skips +/// an object on a locked layer (see edit_entities), so such a frame cannot be refused. +pub(crate) fn holds_ole_frame(s: &Session, hs: &[Handle], copy: bool) -> Result { let d = s.doc()?; - let ole = |h: &Handle| { + Ok(hs.iter().any(|h| { d.entity(*h).is_some_and(|e| { - // An in-place edit skips an object on a locked layer (see edit_entities), so it cannot be refused. let locked = !copy && d.layer(&e.common.layer).is_some_and(|l| l.locked); !locked && matches!(&e.kind, cadcraft_doc::EntityKind::Unknown(u) if u.is_ole_frame()) }) - }; - if hs.iter().any(ole) { - return Err(crate::EngineError::Other( - "An OLE object can only be moved, copied or scaled uniformly; other transforms (rotation, mirroring, non-uniform scale, alignment) are not supported.".into(), - )); - } - Ok(()) + })) } /// `p` at the current elevation (ELEVATION), the Z AutoCAD gives a point entered without one. From 95396c032d761369fb3275496ac131446a422200 Mon Sep 17 00:00:00 2001 From: SBaksa Date: Thu, 5 Nov 2026 15:34:08 +1100 Subject: [PATCH 26/29] OLE2FRAME: only picking sees the frame edges (review on #2587) Putting the frame in prims() also made it a TRIM/EXTEND edge, an object snap target and dimension geometry. prims() is back to nothing for it, the extents arm is restored, and select::hit_polylines adds the frame's outline for picking (none for a frame with no width or height or a bad corner). Tests: clicking_an_ole_frame_edge_selects_it (now also a frame with no width), ole_frame_is_not_a_trim_edge_or_snap_target. --- crates/doc/src/entity.rs | 4 ---- crates/doc/src/extents.rs | 2 ++ crates/engine/src/select.rs | 9 +++++++++ crates/engine/tests/ole_frame_transform.rs | 18 ++++++++++++++++++ 4 files changed, 29 insertions(+), 4 deletions(-) diff --git a/crates/doc/src/entity.rs b/crates/doc/src/entity.rs index d24f7f49b..1237463f1 100644 --- a/crates/doc/src/entity.rs +++ b/crates/doc/src/entity.rs @@ -264,10 +264,6 @@ impl EntityKind { EntityKind::Image(i) => vec![Prim::Point(i.insert.xy())], EntityKind::Table(t) => vec![Prim::Point(t.insert.xy())], EntityKind::Tolerance(t) => vec![Prim::Point(t.insert.xy())], - // An OLE object's content is not drawn, but its frame is: the edges between its corners. - EntityKind::Unknown(u) if u.is_ole_frame() => crate::ole_frame(&u.tags) - .map(|q| q.windows(2).filter_map(|w| Some(Prim::Seg(Segment::Line(GLine::new(*w.first()?, *w.get(1)?))))).collect()) - .unwrap_or_default(), EntityKind::Unknown(_) => Vec::new(), } } diff --git a/crates/doc/src/extents.rs b/crates/doc/src/extents.rs index 2e3f16a48..3630f8e66 100644 --- a/crates/doc/src/extents.rs +++ b/crates/doc/src/extents.rs @@ -458,6 +458,8 @@ fn bounds_in<'a>(d: &'a Drawing, e: &Entity, depth: usize, blocks: &mut HashMap< Bounds2::from_points([o, o + u, o + v, o + u + v]) } EntityKind::LwPolyline(p) => lwpolyline_bounds(p), + // An OLE object's content is not drawn, but its frame is, so the frame counts (#2422). + EntityKind::Unknown(u) if u.is_ole_frame() => ole_frame(&u.tags).map_or(Bounds2::EMPTY, Bounds2::from_points), kind => { let mut b = Bounds2::EMPTY; for p in kind.prims() { diff --git a/crates/engine/src/select.rs b/crates/engine/src/select.rs index a3d361e84..c8a689194 100644 --- a/crates/engine/src/select.rs +++ b/crates/engine/src/select.rs @@ -36,6 +36,13 @@ pub fn hit_polylines(d: &Drawing, e: &Entity, tol: f64) -> Vec> { } v } + // An OLE object's frame is drawn, so it is picked on its outline (the corners the extents and + // the renderer use); a frame with no area has nothing to hit. It stays out of prims(), which + // TRIM, object snap and the dimension commands read. + EntityKind::Unknown(u) if u.is_ole_frame() => match cadcraft_doc::ole_frame(&u.tags) { + Some(q) if q.first().zip(q.get(2)).is_some_and(|(a, c)| a.x != c.x && a.y != c.y) => vec![q.to_vec()], + _ => Vec::new(), + }, kind => { let mut out = Vec::new(); for p in kind.prims() { @@ -516,10 +523,12 @@ mod tests { let mut d = Drawing::new_metric(); let h = d.add(&Space::Model, Common::default(), frame(vec![tag(10, "30"), tag(20, "-19"), tag(11, "35"), tag(21, "-22")])).unwrap(); d.add(&Space::Model, Common::default(), frame(vec![tag(10, "80"), tag(20, "80")])).unwrap(); + d.add(&Space::Model, Common::default(), frame(vec![tag(10, "60"), tag(20, "-19"), tag(11, "60"), tag(21, "-22")])).unwrap(); assert_eq!(pick(&d, &Space::Model, Vec2::new(32.5, -19.0), 0.2), Some(h), "top edge"); assert_eq!(pick(&d, &Space::Model, Vec2::new(35.0, -20.5), 0.2), Some(h), "right edge"); assert_eq!(pick(&d, &Space::Model, Vec2::new(32.5, -20.5), 0.2), None, "inside the frame"); assert_eq!(pick(&d, &Space::Model, Vec2::new(80.0, 80.0), 0.2), None, "a frame without a second corner"); + assert_eq!(pick(&d, &Space::Model, Vec2::new(60.0, -20.0), 0.2), None, "a frame with no width"); } /// A block holding a diagonal dashed line, inserted at `scale`. diff --git a/crates/engine/tests/ole_frame_transform.rs b/crates/engine/tests/ole_frame_transform.rs index afe138ff9..171af556c 100644 --- a/crates/engine/tests/ole_frame_transform.rs +++ b/crates/engine/tests/ole_frame_transform.rs @@ -265,3 +265,21 @@ fn classic_polar_array_refuses_a_turned_frame_but_not_an_upright_one() { s.execute("-array", &upright).unwrap(); assert_eq!(s.doc().unwrap().model.len(), 4); } + +/// The frame is picked by its outline but is not geometry for other commands: a line across it is +/// not cut at its edges by TRIM, and object snap near its corner offers no frame point (review on +/// #2587). +#[test] +fn ole_frame_is_not_a_trim_edge_or_snap_target() { + use cadcraft_engine::snap::{mode, osnap}; + let (mut s, _frame) = session(); + let ln = s.execute("line", &json!({ "points": [[28, -20.5], [37, -20.5]] })).unwrap(); + let ln = ln["handle"].as_str().or_else(|| ln["handles"][0].as_str()).unwrap().to_string(); + // No cutting edge is named, so every object but the line itself is one. + let _ = s.execute("trim", &json!({ "handle": ln.clone(), "pick": [32, -20.5] })); + assert_eq!(line_ends(&s, &ln), ((28.0, -20.5), (37.0, -20.5)), "the line is not cut at the frame"); + assert_eq!(s.doc().unwrap().model.len(), 2); + let all = mode::END | mode::MID | mode::INT | mode::PER | mode::CEN | mode::QUA | mode::NOD; + let hit = osnap(s.doc().unwrap(), &Space::Model, cadcraft_engine::geom::Vec2::new(30.0, -19.0), 0.5, all, None, false); + assert!(hit.is_none(), "no snap point on the frame: {hit:?}"); +} From 63f53f6ed603abaefd9e80260cb88ed31fa80863 Mon Sep 17 00:00:00 2001 From: SBaksa Date: Thu, 5 Nov 2026 15:34:23 +1100 Subject: [PATCH 27/29] ARRAY, ARRAYPATH: one OLE frame check, after the matrices (review on #2587) A matrix every frame accepts (a rectangular array, plain translations) now needs no look at the objects, and an empty matrix list never touches the drawing. ARRAYPATH scans the selection once instead of once per item. Covered by the existing refusal tests; copy_all is not reachable without a drawing through any command, so there is no no-drawing test. --- crates/engine/src/cmd/array.rs | 4 ++-- crates/engine/src/cmd/modify2/patharray.rs | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/engine/src/cmd/array.rs b/crates/engine/src/cmd/array.rs index c4948c9e0..89c6013d2 100644 --- a/crates/engine/src/cmd/array.rs +++ b/crates/engine/src/cmd/array.rs @@ -250,8 +250,8 @@ fn copy_all(s: &mut Session, hs: &[Handle], mats: impl Iterator) -> // Every copy is checked before the first is made: a refusal part way would leave the copies // already added in the drawing (a failed command is not rolled back). let mats: Vec = mats.collect(); - // The objects are looked at once; then a matrix only needs its own check. - if super::modify::holds_ole_frame(s, hs, true)? && mats.iter().any(|m| !cadcraft_doc::ole_frame_accepts(m)) { + // A matrix every frame accepts needs no look at the objects; otherwise they are looked at once. + if mats.iter().any(|m| !cadcraft_doc::ole_frame_accepts(m)) && super::modify::holds_ole_frame(s, hs, true)? { return Err(super::modify::ole_refusal()); } let mut n = 0; diff --git a/crates/engine/src/cmd/modify2/patharray.rs b/crates/engine/src/cmd/modify2/patharray.rs index aff058ab5..dd9597177 100644 --- a/crates/engine/src/cmd/modify2/patharray.rs +++ b/crates/engine/src/cmd/modify2/patharray.rs @@ -46,8 +46,8 @@ pub(crate) fn arraypath( } // Every copy is checked before the first is made: a refusal part way would leave the copies // already added in the drawing (a failed command is not rolled back). - for m in &mats { - crate::cmd::modify::refuse_ole_frames(s, &objs, m, true)?; + if mats.iter().any(|m| !cadcraft_doc::ole_frame_accepts(m)) && crate::cmd::modify::holds_ole_frame(s, &objs, true)? { + return Err(crate::cmd::modify::ole_refusal()); } let mut out = Vec::new(); for m in &mats { From 35e85f8694f408643bcff47c265a2807244ba0ed Mon Sep 17 00:00:00 2001 From: SBaksa Date: Thu, 5 Nov 2026 15:34:23 +1100 Subject: [PATCH 28/29] OLE2FRAME: comments match the tolerance; case lookup in one pass (review on #2587) The ulp term takes over from CORNER_EPS from about 1.1e9, and the doc now says so. The case of the nearest letter for each hex digit is found in two linear passes instead of a search per digit; same result, covered by moving_an_ole_frame_keeps_each_hex_digits_case. --- crates/doc/src/extents.rs | 57 ++++++++++++++++++++++++++++----------- 1 file changed, 41 insertions(+), 16 deletions(-) diff --git a/crates/doc/src/extents.rs b/crates/doc/src/extents.rs index 3630f8e66..4cffb359f 100644 --- a/crates/doc/src/extents.rs +++ b/crates/doc/src/extents.rs @@ -96,8 +96,8 @@ fn ole_data_transform(tags: &mut [crate::RawTag], new: [Vec2; 4], frame: [f64; 4 // other header is not ours to rewrite. let [x1, y1, x2, y2] = frame; let want = [(x1, y1), (x2, y1), (x2, y2), (x1, y2)]; - // The floor is the fixed-ceiling match; above about 8.6e9 one ulp exceeds it, so a few ulps of the - // larger value also match. + // Two corners match within a fixed tolerance for ordinary coordinates (CORNER_EPS at most), or + // within a few ulps of the coordinate where that is larger, which is from about 1.1e9 up. let near = |a: f64, b: f64| { let m = a.abs().max(b.abs()); (a - b).abs() <= (1e-9 * m.max(1.0)).min(CORNER_EPS).max(4.0 * f64::EPSILON * m) @@ -135,30 +135,55 @@ fn ole_data_transform(tags: &mut [crate::RawTag], new: [Vec2; 4], frame: [f64; 4 /// is none, so bytes that did not change keep their exact text. fn recase(chunk: &[u8], old: &str) -> String { let old = old.as_bytes(); - let lower_at = |i: usize| -> bool { - if let Some(c) = old.get(i).filter(|c| c.is_ascii_alphabetic()) { - return c.is_ascii_lowercase(); + // For each place, whether the letter nearest to it (the one at it, else the closest before or + // after; the one before on a tie) is lower case: one pass for the next letter at or after each + // place, one for the previous, then the nearer of the two. + let mut next: Vec> = vec![None; old.len()]; + let mut seen: Option<(usize, bool)> = None; + for (i, c) in old.iter().enumerate().rev() { + if c.is_ascii_alphabetic() { + seen = Some((i, c.is_ascii_lowercase())); } - let nearest = (1..=old.len()).find_map(|d| { - let before = i.checked_sub(d).and_then(|j| old.get(j)).filter(|c| c.is_ascii_alphabetic()); - before.or_else(|| old.get(i + d).filter(|c| c.is_ascii_alphabetic())) - }); - nearest.is_some_and(|c| c.is_ascii_lowercase()) - }; + if let Some(slot) = next.get_mut(i) { + *slot = seen; + } + } + let mut lower_at = vec![false; old.len()]; + let mut prev: Option<(usize, bool)> = None; + for (i, c) in old.iter().enumerate() { + if c.is_ascii_alphabetic() { + prev = Some((i, c.is_ascii_lowercase())); + } + let after = next.get(i).copied().flatten(); + let lower = match (prev, after) { + (Some((p, lp)), Some((q, lq))) => { + if i - p <= q - i { + lp + } else { + lq + } + } + (Some((_, l)), None) | (None, Some((_, l))) => l, + (None, None) => false, + }; + if let Some(slot) = lower_at.get_mut(i) { + *slot = lower; + } + } let mut out = String::with_capacity(chunk.len() * 2); for (k, b) in chunk.iter().enumerate() { for (j, d) in [b >> 4, b & 15].into_iter().enumerate() { let digit = char::from_digit(d.into(), 16).unwrap_or('0'); - out.push(if lower_at(2 * k + j) { digit } else { digit.to_ascii_uppercase() }); + out.push(if lower_at.get(2 * k + j).copied().unwrap_or(false) { digit } else { digit.to_ascii_uppercase() }); } } out } -/// Ceiling for the tolerance when matching the corners in the OLE data to the frame's. The header -/// holds the same f64 the frame groups print, so they agree to a few ulps; a fixed ceiling keeps -/// distinct geometry from matching at large coordinates, where a relative tolerance would grow to -/// whole units. +/// The fixed part of the tolerance when matching the corners in the OLE data to the frame's. The +/// header holds the same f64 the frame groups print, so they agree to a few ulps. Up to about 1e9 a +/// fixed tolerance of at most this decides, so that distinct geometry does not match; from about +/// 1.1e9 on, where one ulp is no longer below it, a few ulps of the coordinate decide instead. const CORNER_EPS: f64 = 1e-6; /// Transform an OLE2FRAME by `m`: its two corners are mapped through `m` (a DWG save writes the From 62d1c806f7fcfd4b9e68b53bc0254b2a325f73d9 Mon Sep 17 00:00:00 2001 From: SBaksa Date: Thu, 5 Nov 2026 16:06:13 +1100 Subject: [PATCH 29/29] OLE2FRAME: a frame with no width or height is picked as the line it is drawn as (review) The renderer draws a zero-width frame as a line, so leaving it out of the hit geometry made a visible object unpickable. Also corrects where the corner tolerance switches to ulps (4 ulps pass 1e-6 near 1.1e9). --- crates/doc/src/extents.rs | 6 +++--- crates/engine/src/select.rs | 12 ++++++------ 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/crates/doc/src/extents.rs b/crates/doc/src/extents.rs index 4cffb359f..101c69ff0 100644 --- a/crates/doc/src/extents.rs +++ b/crates/doc/src/extents.rs @@ -181,9 +181,9 @@ fn recase(chunk: &[u8], old: &str) -> String { } /// The fixed part of the tolerance when matching the corners in the OLE data to the frame's. The -/// header holds the same f64 the frame groups print, so they agree to a few ulps. Up to about 1e9 a -/// fixed tolerance of at most this decides, so that distinct geometry does not match; from about -/// 1.1e9 on, where one ulp is no longer below it, a few ulps of the coordinate decide instead. +/// header holds the same f64 the frame groups print, so they agree to a few ulps. Up to about 1.1e9 +/// a fixed tolerance of at most this decides, so that distinct geometry does not match; above that, +/// four ulps of the coordinate (`4 * EPSILON * |x|`) exceed it and decide instead. const CORNER_EPS: f64 = 1e-6; /// Transform an OLE2FRAME by `m`: its two corners are mapped through `m` (a DWG save writes the diff --git a/crates/engine/src/select.rs b/crates/engine/src/select.rs index c8a689194..1ce7ecc52 100644 --- a/crates/engine/src/select.rs +++ b/crates/engine/src/select.rs @@ -37,11 +37,11 @@ pub fn hit_polylines(d: &Drawing, e: &Entity, tol: f64) -> Vec> { v } // An OLE object's frame is drawn, so it is picked on its outline (the corners the extents and - // the renderer use); a frame with no area has nothing to hit. It stays out of prims(), which - // TRIM, object snap and the dimension commands read. + // the renderer use); a frame with no width or height is drawn as a line, so it is picked as + // one. It stays out of prims(), which TRIM, object snap and the dimension commands read. EntityKind::Unknown(u) if u.is_ole_frame() => match cadcraft_doc::ole_frame(&u.tags) { - Some(q) if q.first().zip(q.get(2)).is_some_and(|(a, c)| a.x != c.x && a.y != c.y) => vec![q.to_vec()], - _ => Vec::new(), + Some(q) => vec![q.to_vec()], + None => Vec::new(), }, kind => { let mut out = Vec::new(); @@ -523,12 +523,12 @@ mod tests { let mut d = Drawing::new_metric(); let h = d.add(&Space::Model, Common::default(), frame(vec![tag(10, "30"), tag(20, "-19"), tag(11, "35"), tag(21, "-22")])).unwrap(); d.add(&Space::Model, Common::default(), frame(vec![tag(10, "80"), tag(20, "80")])).unwrap(); - d.add(&Space::Model, Common::default(), frame(vec![tag(10, "60"), tag(20, "-19"), tag(11, "60"), tag(21, "-22")])).unwrap(); + let line = d.add(&Space::Model, Common::default(), frame(vec![tag(10, "60"), tag(20, "-19"), tag(11, "60"), tag(21, "-22")])).unwrap(); assert_eq!(pick(&d, &Space::Model, Vec2::new(32.5, -19.0), 0.2), Some(h), "top edge"); assert_eq!(pick(&d, &Space::Model, Vec2::new(35.0, -20.5), 0.2), Some(h), "right edge"); assert_eq!(pick(&d, &Space::Model, Vec2::new(32.5, -20.5), 0.2), None, "inside the frame"); assert_eq!(pick(&d, &Space::Model, Vec2::new(80.0, 80.0), 0.2), None, "a frame without a second corner"); - assert_eq!(pick(&d, &Space::Model, Vec2::new(60.0, -20.0), 0.2), None, "a frame with no width"); + assert_eq!(pick(&d, &Space::Model, Vec2::new(60.0, -20.0), 0.2), Some(line), "a frame with no width is drawn as a line and picked as one"); } /// A block holding a diagonal dashed line, inserted at `scale`.