From a50efd9d752ce4f7d092de379b813191c16d7720 Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Tue, 10 Nov 2026 23:57:38 +0000 Subject: [PATCH] address review: chart raster and label budget fixes --- crates/chart/src/raster.rs | 16 ++++++++--- crates/chart/src/render/lines.rs | 2 +- crates/chart/src/resolve.rs | 48 +++++++++++++++++++++++++++----- 3 files changed, 54 insertions(+), 12 deletions(-) diff --git a/crates/chart/src/raster.rs b/crates/chart/src/raster.rs index e2d8a7b9c..39616ddcd 100644 --- a/crates/chart/src/raster.rs +++ b/crates/chart/src/raster.rs @@ -13,6 +13,9 @@ const SUB: usize = 4; /// image; once the allowance is spent the remaining fills are skipped. 2^28 is about a second, /// far beyond any chart. const MAX_SCAN_WORK: u64 = 1 << 28; +/// Most points one filled path may have: more is skipped before any edge is built. Chart +/// renderers cap and decimate their paths far below this; it only bounds direct callers. +const MAX_FILL_POINTS: usize = 1 << 22; type Path = Vec>; @@ -49,6 +52,10 @@ impl Canvas { /// Fills a multi-contour path with the even-odd rule. fn fill(&mut self, path: &Path, color: Rgba) { + // Out of work, or more geometry than the allowance could ever scan: no edge list is built. + if self.work == 0 || path.iter().map(Vec::len).fold(0usize, usize::saturating_add) > MAX_FILL_POINTS { + return; + } // Each edge as [top y, bottom y, x0, y0, x1, y1]. let mut edges: Vec<[f32; 6]> = Vec::new(); let (mut minx, mut miny, mut maxx, mut maxy) = (f32::INFINITY, f32::INFINITY, f32::NEG_INFINITY, f32::NEG_INFINITY); @@ -95,12 +102,14 @@ impl Canvas { if active.is_empty() { continue; } - let tests = (active.len() * SUB) as u64; - if tests > self.work { + // The row's edge tests and its `bw` accumulator clears and blends, charged before any of + // them, so rows with no coverage still count. + let cost = (active.len() * SUB) as u64 + bw as u64; + if cost > self.work { self.work = 0; return; } - self.work -= tests; + self.work -= cost; for v in self.acc.iter_mut().take(bw) { *v = 0.0; } @@ -136,7 +145,6 @@ impl Canvas { if !any { continue; } - self.work = self.work.saturating_sub(bw as u64); for px in x0..x1 { let cov = self.acc.get(px - x0).copied().unwrap_or(0.0); if cov > 0.0 { diff --git a/crates/chart/src/render/lines.rs b/crates/chart/src/render/lines.rs index 1a978ec69..678181326 100644 --- a/crates/chart/src/render/lines.rs +++ b/crates/chart/src/render/lines.rs @@ -137,7 +137,7 @@ pub(super) fn decimate_area(top: Vec<[f32; 2]>, base: Vec<[f32; 2]>, max: usize) /// Draws a polyline split at gaps (`None`), decimated when long. With `smooth` (the plot box), /// each run is interpolated with a Catmull-Rom spline so the line curves through the data points. /// -/// The points interpolation adds are budgeted across all the runs of the line, not per run: +/// The points of the smoothed runs (data points included) are budgeted across all the runs of the line, not per run: /// short runs between gaps escape decimation, and 100 sub-segments each would turn a /// 100,000-point series of three-point runs into millions of points. Past the budget the /// remaining runs are drawn through their data points only. diff --git a/crates/chart/src/resolve.rs b/crates/chart/src/resolve.rs index e99ca3b1c..a588c728c 100644 --- a/crates/chart/src/resolve.rs +++ b/crates/chart/src/resolve.rs @@ -156,6 +156,9 @@ fn clip_label(s: &str, room: usize) -> String { /// The label of category `i` (value `v`) in at most `room` bytes: formatted with its source /// cell's number format when it is a number. fn category_label(wb: &Workbook, src: Option<(&gridcraft_model::Sheet, RangeRef)>, i: usize, v: &Value, room: usize) -> String { + if room == 0 { + return String::new(); // nothing fits: skip the number formatting + } if let (Some((s, r)), Value::Number(_)) = (src, v) { let w = r.width().max(1) as usize; let at = r.start.offset((i / w) as i64, (i % w) as i64); @@ -167,6 +170,10 @@ fn category_label(wb: &Workbook, src: Option<(&gridcraft_model::Sheet, RangeRef) } } // Text is clipped before it is copied: the source may be one long shared string. + let mut v = v; + while let Value::Array(a) = v { + v = a.get(0, 0).unwrap_or(&Value::Empty); + } match v { Value::Text(t) => clip_label(t, room), _ => clip_label(&value_text(v), room), @@ -181,23 +188,30 @@ fn category_table(wb: &Workbook, sheet: usize, formula: &str, values: &str) -> ( let vals = eval_list(wb, sheet, formula); let cells: Option<(usize, RangeRef)> = parse_ref(wb, sheet, formula); let src = cells.and_then(|(sh, r)| wb.sheet(sh).map(|s| (s, r))); + let value_range = src.and_then(|_| parse_ref(wb, sheet, values).map(|(_, r)| r)); + // The retained levels are known before any label is charged: a cell on a discarded level + // gets no label and spends none of the budget. `None`: a flat list (every cell is a label). + let layout = src.and_then(|(_, r)| { + let levels = category_levels(&r, value_range.as_ref())?; + let (w, h) = (r.width().max(1) as usize, r.height().max(1) as usize); + (vals.len() == w.saturating_mul(h)).then_some((levels, w, h)) // else truncated or not a plain block + }); + let kept = + |i: usize| layout.is_none_or(|(l, w, h)| if l.by_columns { i % w + 1 + l.outer as usize >= w } else { i / w + 1 + l.outer as usize >= h }); let mut room = MAX_CATEGORY_BYTES; let texts: Vec = vals .iter() .enumerate() .map(|(i, v)| { + if !kept(i) { + return String::new(); + } let label = category_label(wb, src, i, v, room); room = room.saturating_sub(label.len()); label }) .collect(); - let Some((_, r)) = src else { return (texts, vec![]) }; - let value_range = parse_ref(wb, sheet, values).map(|(_, r)| r); - let Some(levels) = category_levels(&r, value_range.as_ref()) else { return (texts, vec![]) }; - let (w, h) = (r.width().max(1) as usize, r.height().max(1) as usize); - if texts.len() != w.saturating_mul(h) { - return (texts, vec![]); // truncated or not a plain block: a flat list - } + let Some((levels, w, h)) = layout else { return (texts, vec![]) }; let at = |row: usize, col: usize| texts.get(row * w + col).cloned().unwrap_or_default(); let column = |c: usize| (0..h).map(|row| at(row, c)).collect::>(); let row = |rr: usize| (0..w).map(|c| at(rr, c)).collect::>(); @@ -328,4 +342,24 @@ mod label_budget_tests { assert_eq!(super::clip_label("aé", 2), "a"); assert_eq!(super::clip_label("aé", 3), "aé"); } + + /// Cells on discarded outer levels (only the innermost labels and three outer levels are + /// kept) spend none of the byte budget. + #[test] + fn discarded_levels_do_not_spend_the_label_budget() { + let mut wb = Workbook::new(); + let rows = 8000u32; + if let Some(sh) = wb.sheet_mut(0) { + for r in 0..rows { + for c in 0..5 { + sh.set_value(gridcraft_core::CellRef::new(r, c), gridcraft_core::Value::text("x".repeat(255))); + } + } + } + // Five columns: column A is a fourth outer level and is discarded. + let (inner, outer) = super::category_table(&wb, 0, "=A1:E8000", ""); + assert_eq!(outer.len(), 3); + assert_eq!(inner.last().map(String::len), Some(super::MAX_LABEL_CHARS)); + assert_eq!(outer.first().and_then(|l| l.last()).map(String::len), Some(super::MAX_LABEL_CHARS)); + } }