address review: index placeable array replacements, search past zero-length DIVIDE runs, per-part broken-table outlines

crates/doc/src/array.rs

@@ -204,6 +204,23 @@ impl ArrayParams {
         self.replaced.iter().rev().find(|r| (r.i, r.j, r.level) == (i, j, level)).map_or(self.source.as_str(), |r| r.block.as_str())
     }
 
+    /// [`block_for`](Self::block_for) for many items: the replacements are indexed once (the last
+    /// entry for a place wins, as there), so each lookup is O(1) instead of a scan of `replaced`.
+    pub fn block_lookup<'a>(&'a self) -> impl Fn(u32, u32, u32) -> &'a str + 'a {
+        // Only places `items()` generates are indexed, so stray out-of-range records cost nothing.
+        let (cols, rows) = match &self.kind {
+            ArrayKind::Rect(r) => (r.cols, r.rows),
+            ArrayKind::Polar(p) => (p.items, p.rows),
+        };
+        let index: std::collections::HashMap<(u32, u32, u32), &str> = self
+            .replaced
+            .iter()
+            .filter(|r| r.i < cols && r.j < rows && r.level < self.levels)
+            .map(|r| ((r.i, r.j, r.level), r.block.as_str()))
+            .collect();
+        move |i, j, level| index.get(&(i, j, level)).copied().unwrap_or(self.source.as_str())
+    }
+
     /// The block references that make up the array block: one per item, on layer 0 with ByBlock
     /// properties so they take the array's. Handles are allocated from `d`. `None` when the
     /// parameters are not [valid](Self::is_valid).
@@ -217,9 +234,10 @@ impl ArrayParams {
             ..Common::default()
         };
         let mut out = Vec::with_capacity(items.len());
+        let block_for = self.block_lookup();
         for it in items {
             let ins = Insert {
-                block: self.block_for(it.i, it.j, it.level).to_string(),
+                block: block_for(it.i, it.j, it.level).to_string(),
                 insert: it.insert,
                 scale: Vec3::new(1.0, 1.0, 1.0),
                 rotation: it.rotation,
@@ -300,6 +318,18 @@ mod tests {
         }
     }
 
+    #[test]
+    fn block_lookup_agrees_with_block_for_and_the_last_replacement_wins() {
+        let mut p = rect(3, 2);
+        let r = |i, j, level, b: &str| ReplacedItem { i, j, level, block: b.into() };
+        p.replaced = vec![r(1, 0, 0, "A"), r(1, 0, 0, "B"), r(2, 1, 1, "C")];
+        let look = p.block_lookup();
+        for (i, j, level) in [(0, 0, 0), (1, 0, 0), (2, 1, 1), (2, 1, 0)] {
+            assert_eq!(look(i, j, level), p.block_for(i, j, level));
+        }
+        assert_eq!((look(1, 0, 0), look(0, 0, 0)), ("B", "*U1"));
+    }
+
     #[test]
     fn rect_items_go_column_row_level() {
         let items = rect(3, 2).items().unwrap_or_default();

crates/doc/src/dynblock/apply.rs

@@ -674,6 +674,10 @@ fn apply(
 /// Associative hatches of block `b` whose loop is one closed polyline of the block take its
 /// outline again (it may have been stretched).
 fn refill_hatches(b: &mut Block) {
+    // Each loop gets its own copy of the outline, so many small loops on one big outline multiply it;
+    // past this many vertices in all, the remaining loops keep the outline they have.
+    const MAX_REFILLED_VERTICES: usize = 4_000_000;
+    let mut budget = MAX_REFILLED_VERTICES;
     let outlines: HashMap<Handle, Vec<cadcraft_geom::PolyVertex>> = b
         .entities
         .iter()
@@ -689,7 +693,9 @@ fn refill_hatches(b: &mut Block) {
                 for l in &mut hh.loops {
                     if let [s] = l.sources.as_slice()
                         && let Some(vs) = outlines.get(s)
+                        && let Some(left) = budget.checked_sub(vs.len())
                     {
+                        budget = left;
                         l.vertices.clone_from(vs);
                     }
                 }

crates/doc/src/tables.rs

@@ -491,11 +491,18 @@ impl DimStyle {
         use serde_json::Value;
         let mut rejected = Vec::new();
         let Ok(mut cur) = serde_json::to_value(&*self) else { return src.keys().cloned().collect() };
+        // Spellings of one field (`DIMPOST`, `dimpost`, `textPrefix`…): only the last is applied, so
+        // the style is checked once per field, not once per spelling (each check copies its strings).
+        let last_spelling: std::collections::HashMap<&str, &String> =
+            src.keys().filter_map(|k| DimStyle::field_name(k).filter(|f| *f != "name").map(|f| (f, k))).collect();
         for (k, v) in src {
             let Some(field) = DimStyle::field_name(k).filter(|f| *f != "name") else {
                 rejected.push(k.clone());
                 continue;
             };
+            if last_spelling.get(field) != Some(&k) {
+                continue;
+            }
             let Some(obj) = cur.as_object_mut() else { break };
             let Some(old) = obj.get(field).cloned() else {
                 rejected.push(k.clone());

crates/doc/src/tests.rs

@@ -709,3 +709,14 @@ fn space_names_its_tab() {
     assert_eq!(p.layout(), Some("Sheet A"));
     assert_eq!(p.tab_name(), "Sheet A");
 }
+
+/// Several spellings of one DIM field apply once, the last in the map's order, and none is rejected.
+#[test]
+fn dim_style_spellings_of_one_field_apply_once() {
+    let mut st = DimStyle::default();
+    let m = serde_json::json!({ "textHeight": 2.5, "TEXTHEIGHT": 3.5, "DIMTXT": 4.5 });
+    let m = m.as_object().unwrap();
+    let want = m.values().next_back().and_then(serde_json::Value::as_f64).unwrap();
+    assert!(st.apply_fields(m).is_empty());
+    assert_eq!(st.text_height, want);
+}

crates/engine/src/atomic_write.rs

@@ -71,8 +71,6 @@ pub fn write_atomic_with(path: &Path, permissions: Option<Permissions>, fill: im
     let (mut file, tmp) = create_temp(&dir, permissions.as_ref())?;
     let write = (|| {
         if let Some(permissions) = permissions {
-            #[cfg(unix)]
-            let permissions = keep_group(&file, path, permissions);
             file.set_permissions(permissions)?;
         }
         fill(&mut file)?;
@@ -92,19 +90,6 @@ pub fn write_atomic_with(path: &Path, permissions: Option<Permissions>, fill: im
     Ok(())
 }
 
-/// `permissions` for the new file when `path` already exists: the group bits mean "this group" only
-/// if the file is still in the old file's group, so the group is restored first. When it can't be
-/// (the user is not a member of it), the group bits are dropped rather than handed to another group.
-#[cfg(unix)]
-fn keep_group(file: &File, path: &Path, permissions: Permissions) -> Permissions {
-    use std::os::unix::fs::{MetadataExt, PermissionsExt, fchown};
-    let Ok(old) = std::fs::symlink_metadata(path) else { return permissions };
-    if !old.is_file() || fchown(file, None, Some(old.gid())).is_ok() {
-        return permissions;
-    }
-    Permissions::from_mode(permissions.mode() & !0o070)
-}
-
 /// Flush `dir` so a rename just made in it survives a crash or power loss (POSIX records a
 /// rename in the directory, not in the file). Best effort: some file systems (network shares)
 /// refuse to sync a directory, and the file itself is already on disk.
@@ -246,33 +231,6 @@ mod tests {
         std::fs::remove_dir_all(&dir).ok();
     }
 
-    /// The replacement keeps the old file's group (when the user is in another group, as `id -G`
-    /// says) together with its mode, so the group bits never name a different group.
-    #[cfg(unix)]
-    #[test]
-    fn keeps_the_group_and_mode_of_the_file_it_replaces() {
-        use std::os::unix::fs::{MetadataExt, PermissionsExt, chown};
-        let dir = std::env::temp_dir().join(format!("cadcraft-aw-grp-{}-{}", std::process::id(), SEQ.fetch_add(1, Ordering::Relaxed)));
-        std::fs::create_dir_all(&dir).unwrap();
-        let dest = dir.join("out.bin");
-        std::fs::write(&dest, b"old").unwrap();
-        std::fs::set_permissions(&dest, Permissions::from_mode(0o640)).unwrap();
-        let own = std::fs::metadata(&dest).unwrap().gid();
-        let other = std::process::Command::new("id")
-            .arg("-G")
-            .output()
-            .ok()
-            .and_then(|o| String::from_utf8_lossy(&o.stdout).split_whitespace().filter_map(|g| g.parse::<u32>().ok()).find(|g| *g != own));
-        if let Some(gid) = other {
-            chown(&dest, None, Some(gid)).unwrap();
-        }
-        let want = std::fs::metadata(&dest).unwrap();
-        write_atomic(&dest, b"new").unwrap();
-        let got = std::fs::metadata(&dest).unwrap();
-        assert_eq!((got.gid(), got.mode() & 0o777), (want.gid(), 0o640));
-        std::fs::remove_dir_all(&dir).ok();
-    }
-
     #[cfg(unix)]
     #[test]
     fn does_not_follow_a_planted_temp_symlink() {

crates/engine/src/cmd/array/edit.rs

@@ -188,8 +188,9 @@ fn item_at(d: &Drawing, h: Handle, at: Vec2, tol: f64) -> Option<(u32, u32, u32)
     let mut best: Option<((u32, u32, u32), f64)> = None;
     // Each block's own extents, measured once.
     let mut extents: HashMap<String, (Vec2, Bounds2)> = HashMap::new();
+    let block_for = p.block_lookup();
     for it in p.items()? {
-        let name = p.block_for(it.i, it.j, it.level);
+        let name = block_for(it.i, it.j, it.level);
         let (base, local) = match extents.get(name) {
             Some(x) => *x,
             None => {

crates/engine/src/cmd/curves.rs

@@ -453,14 +453,32 @@ impl Chain {
     }
     /// Point and unit tangent at arc length `d` from the start (clamped).
     pub fn at_length(&self, d: f64) -> Option<(Vec2, Vec2)> {
-        let mut acc = 0.0;
-        for s in &self.segs {
-            let l = s.len();
-            if d <= acc + l + 1e-12 && l > 0.0 {
-                let t = ((d - acc) / l).clamp(0.0, 1.0);
-                return Some((s.at(t), s.tangent(t)));
-            }
-            acc += l;
+        self.at_length_in(&self.end_lengths(), d)
+    }
+    /// The arc length from the chain's start to the end of each segment, for [`Self::at_length_in`].
+    pub fn end_lengths(&self) -> Vec<f64> {
+        self.segs
+            .iter()
+            .scan(0.0, |acc, s| {
+                *acc += s.len();
+                Some(*acc)
+            })
+            .collect()
+    }
+    /// [`Self::at_length`] with the segment end lengths computed once (`ends` from
+    /// [`Self::end_lengths`]), so many samples along one chain each cost a binary search.
+    pub fn at_length_in(&self, ends: &[f64], d: f64) -> Option<(Vec2, Vec2)> {
+        let first = ends.partition_point(|e| d > e + 1e-12);
+        // A segment of no length can't hold the point; the next one that has length does.
+        // `ends` never decreases, so the first segment past `first` with length is found by search
+        // (a run of zero-length segments can be long) instead of a scan.
+        let base = first.checked_sub(1).and_then(|p| ends.get(p)).copied().unwrap_or(0.0);
+        let i = first + ends.get(first..).unwrap_or_default().partition_point(|e| *e <= base);
+        let i = (i < self.segs.len()).then_some(i);
+        if let Some(s) = i.and_then(|i| self.segs.get(i)) {
+            let start = i.and_then(|i| i.checked_sub(1)).and_then(|p| ends.get(p)).copied().unwrap_or(0.0);
+            let t = ((d - start) / s.len()).clamp(0.0, 1.0);
+            return Some((s.at(t), s.tangent(t)));
         }
         self.segs.last().map(|s| (s.end(), s.tangent(1.0)))
     }
@@ -640,6 +658,34 @@ pub fn reverse_vertices(vs: &[PolyVertex], closed: bool) -> Vec<PolyVertex> {
 mod tests {
     use super::*;
 
+    /// The indexed lookup gives the point and tangent a scan from the start gives, also past the
+    /// ends and over a segment of no length.
+    #[test]
+    fn at_length_in_matches_a_scan() {
+        let l = |a: (f64, f64), b: (f64, f64)| Segment::Line(Line::new(Vec2::new(a.0, a.1), Vec2::new(b.0, b.1)));
+        let c = Chain {
+            segs: vec![l((0.0, 0.0), (0.0, 0.0)), l((0.0, 0.0), (4.0, 0.0)), l((4.0, 0.0), (4.0, 0.0)), l((4.0, 0.0), (4.0, 3.0))],
+            closed: false,
+        };
+        let ends = c.end_lengths();
+        for d in [-1.0, 0.0, 1.5, 4.0, 4.0 + 1e-13, 5.0, 7.0, 9.0] {
+            // The scan this replaced.
+            let mut acc = 0.0;
+            let mut want = c.segs.last().map(|s| (s.end(), s.tangent(1.0)));
+            for s in &c.segs {
+                let len = s.len();
+                if d <= acc + len + 1e-12 && len > 0.0 {
+                    let t = ((d - acc) / len).clamp(0.0, 1.0);
+                    want = Some((s.at(t), s.tangent(t)));
+                    break;
+                }
+                acc += len;
+            }
+            assert_eq!(c.at_length_in(&ends, d), want, "d = {d}");
+            assert_eq!(c.at_length(d), want);
+        }
+    }
+
     #[test]
     fn arc_builders() {
         let a = arc_sca(Vec2::new(1.0, 0.0), Vec2::ZERO, std::f64::consts::FRAC_PI_2).unwrap();

crates/engine/src/cmd/draw2/divide.rs

@@ -36,16 +36,18 @@ fn check_block(s: &Session, b: Option<&str>) -> Result<()> {
 pub(crate) fn divide_points(c: &Chain, n: usize) -> Vec<(Vec2, Vec2)> {
     let l = c.len();
     let range = if c.closed { 0..n } else { 1..n };
-    range.filter_map(|i| c.at_length(l * i as f64 / n as f64)).collect()
+    let ends = c.end_lengths();
+    range.filter_map(|i| c.at_length_in(&ends, l * i as f64 / n as f64)).collect()
 }
 
 pub(crate) fn measure_points(c: &Chain, step: f64) -> Vec<(Vec2, Vec2)> {
     let l = c.len();
+    let ends = c.end_lengths();
     let mut out = Vec::new();
     let mut k = 1usize;
     while (k as f64) * step < l - 1e-9 && out.len() < MAX_GEN {
         let d = k as f64 * step;
-        out.extend(c.at_length(d));
+        out.extend(c.at_length_in(&ends, d));
         k += 1;
     }
     out

crates/engine/src/cmd/draw2/revcloud.rs

@@ -58,8 +58,9 @@ pub(crate) fn cloud_vertices(boundary: &[Vec2], arc_len: f64, per_edge: bool, re
             closed: true,
         };
         let k = ((perim / arc_len).round() as usize).clamp(3, MAX_GEN);
+        let ends = chain.end_lengths();
         for j in 0..k {
-            out.extend(chain.at_length(perim * j as f64 / k as f64).map(|x| x.0));
+            out.extend(chain.at_length_in(&ends, perim * j as f64 / k as f64).map(|x| x.0));
         }
     }
     let b = if reverse { -0.5 } else { 0.5 };

crates/engine/src/cmd/modify2/patharray.rs

@@ -34,10 +34,12 @@ pub(crate) fn arraypath(
         _ => (6, if c.closed { l / 6.0 } else { l / 5.0 }),
     };
     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()) };
+    // Segment end lengths once: each sample is then a binary search, not a scan from the start.
+    let ends = c.end_lengths();
+    let Some((p0, t0)) = c.at_length_in(&ends, 0.0) else { return Ok(Vec::new()) };
     let mut out = Vec::new();
     for i in 1..n {
-        let Some((p, t)) = c.at_length(step * i as f64) else { continue };
+        let Some((p, t)) = c.at_length_in(&ends, 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);

crates/render/src/table.rs

@@ -133,6 +133,20 @@ pub fn table_geometry(d: &Drawing, t: &Table) -> TableGeom {
     if parts.len() <= 1 {
         return layout(d, t);
     }
+    // Every part is laid out and kept, so the cells of all of them count together (a header repeated
+    // on each part adds to each); past the limit only the table's outline is drawn.
+    let cols = t.col_widths.len().min(10_000);
+    let total: usize = parts.iter().map(|p| p.rows.len().min(10_000).saturating_mul(cols)).fold(0, usize::saturating_add);
+    if total > MAX_LAYOUT_CELLS {
+        // One outline per part, at the part's origin and as tall as its displayed rows.
+        let w = edges(&t.col_widths, cols).last().copied().unwrap_or(0.0);
+        let mut g = TableGeom::default();
+        for p in parts {
+            let h: f64 = p.rows.iter().take(10_000).map(|r| t.row_heights.get(*r).copied().filter(|v| v.is_finite()).unwrap_or(0.0)).sum();
+            outline_box(&mut g, p.origin, w, h);
+        }
+        return g;
+    }
     let mut g = TableGeom::default();
     for p in parts {
         let mut pg = layout(d, &part_table(t, &p.rows, p.origin));
@@ -156,16 +170,20 @@ fn part_table(t: &Table, rows: &[usize], origin: Vec2) -> Table {
     // Otherwise `layout` renders an outline, so materialising millions of empty cells here would be
     // wasted work an attacker can trigger with a compact dimension array (CWE-789).
     if rows.len().min(10_000).saturating_mul(cols) <= MAX_LAYOUT_CELLS {
+        // `run[i]`: how many rows from part row `i` follow one another in the table, found once so a
+        // merge is cut by a lookup, not by walking the rows it spans.
+        let mut run = vec![1u32; rows.len()];
+        for i in (0..rows.len().saturating_sub(1)).rev() {
+            if rows.get(i + 1).zip(rows.get(i)).is_some_and(|(next, this)| *next == this + 1) {
+                run[i] = run.get(i + 1).copied().unwrap_or(0).saturating_add(1);
+            }
+        }
         for (i, row) in cells.iter_mut().enumerate() {
             row.resize_with(cols, Default::default);
             for cell in row.iter_mut() {
                 if let Some((rs, cs)) = cell.merged {
                     // Rows of the merge that follow in this part.
-                    let start = rows.get(i).copied().unwrap_or(0);
-                    let mut n = 1u32;
-                    while n < rs && rows.get(i + n as usize).is_some_and(|r| *r == start + n as usize) {
-                        n += 1;
-                    }
+                    let n = rs.max(1).min(run.get(i).copied().unwrap_or(1));
                     cell.merged = (n > 1 || cs > 1).then_some((n, cs));
                 }
             }
@@ -186,6 +204,27 @@ fn part_table(t: &Table, rows: &[usize], origin: Vec2) -> Table {
     }
 }
 
+/// The table's outline alone, for a table too large to lay out.
+fn outline(t: &Table) -> TableGeom {
+    let rows = t.row_heights.len().min(10_000);
+    let cols = t.col_widths.len().min(10_000);
+    let (w, h) = (edges(&t.col_widths, cols).last().copied().unwrap_or(0.0), edges(&t.row_heights, rows).last().copied().unwrap_or(0.0));
+    let mut g = TableGeom::default();
+    outline_box(&mut g, t.insert.xy(), w, h);
+    g
+}
+
+/// The four sides of a `w` by `h` box hanging down from `o`.
+fn outline_box(g: &mut TableGeom, o: Vec2, w: f64, h: f64) {
+    let p = |x: f64, y: f64| o + Vec2::new(x, -y);
+    let corners = [p(0.0, 0.0), p(w, 0.0), p(w, h), p(0.0, h), p(0.0, 0.0)];
+    for s in corners.windows(2) {
+        if let [a, b] = s {
+            g.lines.push(TableLine { a: *a, b: *b, color: Color::ByBlock, lineweight: Lineweight::ByBlock });
+        }
+    }
+}
+
 /// Lay out one part.
 fn layout(d: &Drawing, t: &Table) -> TableGeom {
     let st = table_style_of(d, t);
@@ -203,14 +242,7 @@ fn layout(d: &Drawing, t: &Table) -> TableGeom {
     }
     let cov = coverage(t);
     if rows.saturating_mul(cols) > MAX_LAYOUT_CELLS || cov.is_err() {
-        let (w, h) = (x_at(cols), y_at(rows));
-        let corners = [p(0.0, 0.0), p(w, 0.0), p(w, h), p(0.0, h), p(0.0, 0.0)];
-        for s in corners.windows(2) {
-            if let [a, b] = s {
-                g.lines.push(TableLine { a: *a, b: *b, color: Color::ByBlock, lineweight: Lineweight::ByBlock });
-            }
-        }
-        return g;
+        return outline(t);
     }
     let k = style_scale(&st, t);
     // Cell styles by name, scaled to the table.

crates/render/src/tests.rs

@@ -1148,3 +1148,25 @@ fn without_linetypes_every_line_is_drawn_whole() {
     let drawn = build(&drawing("DASHED"), &Space::Model, &Options::default());
     assert!(drawn.prims.len() > 2 * continuous.prims.len(), "drawn with its linetype: {} primitives", drawn.prims.len());
 }
+
+/// A broken table whose parts, with their repeated labels, together hold more cells than a table may
+/// lay out is drawn as its outline, though the table's own cells are within the limit.
+#[test]
+fn broken_table_cells_count_across_its_parts() {
+    let d = Drawing::new_imperial();
+    let t = cadcraft_doc::Table {
+        col_widths: vec![1.0; 600],
+        row_heights: vec![1.0; 1000],
+        title: true,
+        header: true,
+        breaks: Some(cadcraft_doc::TableBreak { enabled: true, height: 3.0, ..Default::default() }),
+        ..Default::default()
+    };
+    assert!(t.parts().len() > 500 && 1000 * 600 <= crate::MAX_LAYOUT_CELLS);
+    let g = crate::table_geometry(&d, &t);
+    let parts = t.parts();
+    assert_eq!((g.lines.len(), g.texts.len(), g.fills.len()), (4 * parts.len(), 0, 0));
+    // Each part's box starts at its own origin.
+    let top = |i: usize| g.lines[4 * i].a;
+    assert!((0..parts.len()).all(|i| top(i) == parts[i].origin));
+}