diff --git a/crates/io/src/dxf_read/tables.rs b/crates/io/src/dxf_read/tables.rs index 001944b86..c7a600b1f 100644 --- a/crates/io/src/dxf_read/tables.rs +++ b/crates/io/src/dxf_read/tables.rs @@ -324,18 +324,22 @@ fn vport_record(d: &mut Drawing, t: &T) { let centred = t.f(12).is_some() && t.f(22).is_some(); let height = t.fd(40, 0.0); let unit = |v: f64| v.is_finite() && (-1e-9..=1.0 + 1e-9).contains(&v); - let ok = [min.x, min.y, max.x, max.y].into_iter().all(unit) - && max.x > min.x + 1e-6 - && max.y > min.y + 1e-6 - && center.is_finite() - && height.is_finite() - && height > 0.0; - if name.is_empty() || !ok { + // One record carries two separate things: where the tile sits on the screen (10/11) and the + // view it shows (12/22, 40). They are judged apart, because a record can hold a usable view + // with a tile rectangle that is not usable -- a first `*ACTIVE` like that used to return + // before the view was read, so a file whose header has no view opened on the extent fit + // instead of where it was saved (noted in review). + let view_ok = center.is_finite() && height.is_finite() && height > 0.0; + let tile_ok = [min.x, min.y, max.x, max.y].into_iter().all(unit) && max.x > min.x + 1e-6 && max.y > min.y + 1e-6; + if name.is_empty() { return; } - if first_active && centred { + if first_active && centred && view_ok { d.header.set_saved_view(center, height); } + if !view_ok || !tile_ok { + return; + } let aspect = t.f(41).filter(|a| a.is_finite() && *a > 0.0).unwrap_or(0.0); let clamp = |p: Vec2| Vec2::new(p.x.clamp(0.0, 1.0), p.y.clamp(0.0, 1.0)); let tile = VportTile { min: clamp(min), max: clamp(max), center, height, aspect }; diff --git a/crates/io/src/dxf_write/layout.rs b/crates/io/src/dxf_write/layout.rs index 7384fb766..7a6ed9d83 100644 --- a/crates/io/src/dxf_write/layout.rs +++ b/crates/io/src/dxf_write/layout.rs @@ -75,9 +75,17 @@ pub(super) fn vport_table(w: &mut W, d: &Drawing, ext: Bounds2) { }; let single = VportTile { min: Vec2::ZERO, max: Vec2::new(1.0, 1.0), center, height, aspect: ASPECT }; let tiled = d.vports.iter().find(|c| c.is_active() && c.tiles.len() > 1); - let mut records: Vec<(&str, &VportTile)> = match tiled { - Some(c) => c.tiles.iter().map(|t| (VportConfig::ACTIVE, t)).collect(), - None => vec![(VportConfig::ACTIVE, &single)], + // The first `*ACTIVE` record is the one a reader takes the view from, so it carries the saved + // view while keeping its own place on the screen and the tiles after it unchanged. Writing + // the stored tiles as they stand put the new view in HEADER and the old one here, and the + // read takes this one, so updating the view of a tiled drawing did not survive a round trip + // (noted in review). + let first = tiled.and_then(|c| c.tiles.first()).map(|t| VportTile { center, height, ..*t }); + let mut records: Vec<(&str, &VportTile)> = match (tiled, first.as_ref()) { + (Some(c), Some(first)) => { + std::iter::once((VportConfig::ACTIVE, first)).chain(c.tiles.iter().skip(1).map(|t| (VportConfig::ACTIVE, t))).collect() + } + _ => vec![(VportConfig::ACTIVE, &single)], }; records.extend(d.vports.iter().filter(|c| !c.is_active() && !c.name.is_empty()).flat_map(|c| c.tiles.iter().map(|t| (c.name.as_str(), t)))); let th = table_head(w, "VPORT", records.len()); diff --git a/crates/io/src/tests.rs b/crates/io/src/tests.rs index 230c202f7..edb48fa20 100644 --- a/crates/io/src/tests.rs +++ b/crates/io/src/tests.rs @@ -587,6 +587,58 @@ fn a_viewport_record_without_a_centre_keeps_the_header_view() { assert_eq!(d.header.saved_view(), Some((Vec2::new(50.0, 50.0), 20.0)), "the header pair is kept"); } +/// Updating the saved view of a drawing that has several model-space tiles survives a round +/// trip. The write put the new view in HEADER but emitted the stored tiles unchanged, and the +/// read takes its view from the first `*ACTIVE` record, so the update was read back as the old +/// view (noted in review of #1602). +#[test] +fn a_saved_view_set_on_a_tiled_drawing_survives_a_round_trip() { + let mut d = sample(); + // Two tiles side by side, the first the current one, each with its own old view. + let tile = |min: Vec2, max: Vec2, center: Vec2| VportTile { min, max, center, height: 10.0, aspect: 1.0 }; + d.vports.push(VportConfig { + name: VportConfig::ACTIVE.to_string(), + tiles: vec![tile(Vec2::ZERO, Vec2::new(0.5, 1.0), Vec2::new(1.0, 1.0)), tile(Vec2::new(0.5, 0.0), Vec2::new(1.0, 1.0), Vec2::new(2.0, 2.0))], + }); + d.header.set_saved_view(Vec2::new(70.0, 80.0), 30.0); + + let back = read_dxf(write_dxf(&d).as_bytes()).unwrap(); + assert_eq!(back.header.saved_view(), Some((Vec2::new(70.0, 80.0), 30.0)), "the view that was set is the one read back"); + // The tiled configuration is still there, and the first tile keeps its own half of the screen. + let active = back.vports.iter().find(|c| c.is_active()).expect("the tiled configuration is kept"); + assert_eq!(active.tiles.len(), 2, "both tiles are written: {:?}", active.tiles); + assert_eq!(active.tiles[0].max, Vec2::new(0.5, 1.0), "the first tile keeps its rectangle"); + assert_eq!(active.tiles[1].center, Vec2::new(2.0, 2.0), "and the second tile keeps its own view"); +} + +/// A first `*ACTIVE` record can hold a usable view (12/22 centre, 40 height) with a tile +/// rectangle that is not usable. The record used to be dropped whole before the view was read, +/// so a file whose header carries no view opened on the extent fit rather than where it was +/// saved (noted in review of #1602). +#[test] +fn a_viewport_record_with_a_bad_tile_still_gives_its_view() { + // No `$VIEWCTR`/`$VIEWSIZE`, and a tile whose corners are equal (10/11 both 0,0), so the + // rectangle is unusable while the view is not: centre 50,50 and height 20. + const BAD_TILE: &str = "\ +0\nSECTION\n2\nHEADER\n0\nENDSEC\n\ +0\nSECTION\n2\nTABLES\n0\nTABLE\n2\nVPORT\n0\nVPORT\n2\n*ACTIVE\n70\n0\n10\n0.0\n20\n0.0\n11\n0.0\n21\n0.0\n\ +12\n50.0\n22\n50.0\n40\n20.0\n0\nENDTAB\n0\nENDSEC\n\ +0\nSECTION\n2\nENTITIES\n0\nLINE\n8\n0\n10\n0.0\n20\n0.0\n11\n9.0\n21\n9.0\n0\nENDSEC\n0\nEOF\n"; + let d = read_dxf(BAD_TILE.as_bytes()).unwrap(); + assert_eq!(d.header.saved_view(), Some((Vec2::new(50.0, 50.0), 20.0)), "the view the record holds is read"); + // The unusable rectangle is still refused, so the split did not make the tile permissive. + assert!(d.vports.iter().all(|c| c.tiles.is_empty()), "the tile itself is not kept: {:?}", d.vports); + + // And a record with no usable view either leaves the header alone, as before. + const NEITHER: &str = "\ +0\nSECTION\n2\nHEADER\n0\nENDSEC\n\ +0\nSECTION\n2\nTABLES\n0\nTABLE\n2\nVPORT\n0\nVPORT\n2\n*ACTIVE\n70\n0\n10\n0.0\n20\n0.0\n11\n0.0\n21\n0.0\n\ +12\n50.0\n22\n50.0\n40\n0.0\n0\nENDTAB\n0\nENDSEC\n\ +0\nSECTION\n2\nENTITIES\n0\nLINE\n8\n0\n10\n0.0\n20\n0.0\n11\n9.0\n21\n9.0\n0\nENDSEC\n0\nEOF\n"; + let neither = read_dxf(NEITHER.as_bytes()).unwrap(); + assert_eq!(neither.header.saved_view(), None, "a zero height is not a view, so the header keeps none"); +} + #[test] fn dxf_roundtrips_page_setup() { let mut d = sample();