From 0f31065bfd2e39e6219090dc1742771a2843be9e Mon Sep 17 00:00:00 2001 From: Baptiste Parmantier Date: Sat, 8 Aug 2026 14:54:10 +0200 Subject: [PATCH] fix(dataviz): honour start_at, the layout box, and degenerate scales MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Eight confirmed findings across the data components, plus the same defects found by the audit in files the workstream did not own. Five components drove their reveal off raw scene time, so a chart with `start_at: 2.0` was already fully drawn when it appeared. They now measure elapsed time from `start_at`, matching `Counter::ramp_progress`. The same bug in `gauge` and `dot_map` is fixed here rather than left for a later pass — it is one defect in seven copies. `progress` painted at its declared `width`/`height` instead of the box taffy computed, so a bar inside a sized container ignored its own layout. It now paints at `layout.width`/`layout.height`. `stacked_bar` had no signed extent: negative totals rendered outside the box. Stacks now grow either side of an anchored zero. `heatmap` renormalised its data min→max, so a uniform grid of 5.0 painted identically to a grid of 0.0 and `color_scale` did not mean what the docs say. The scale is now the documented absolute 0..1. A neighbouring bug in `interpolate_color` went with it: `t = 1.0` resolved to the second-to-last colour because the local fraction was recomputed from the clamped segment. A flat sparkline series divided by a floored range, normalising every point to 0 and gluing the line to the bottom edge — it read as "collapsed to zero" rather than "unchanged". Flat series now centre. Fixed in `sparkline` and in the `stat` card that reimplements the same maths. Axis labels were collected with `filter_map`, so one datum without a label shifted every subsequent label onto the wrong bar. Labels now keep one slot per datum. Fixed in `bar`, and in `line` and `waterfall` which carry the identical bug. `treemap` drew its label and value on a single baseline, so the value overprinted the label. Fragments now stack. --- crates/rustmotion-components/src/chart/bar.rs | 396 +++++++++++++++++- .../rustmotion-components/src/chart/line.rs | 12 +- crates/rustmotion-components/src/chart/mod.rs | 92 +++- .../src/chart/waterfall.rs | 6 +- crates/rustmotion-components/src/dot_map.rs | 6 +- crates/rustmotion-components/src/gauge.rs | 6 +- crates/rustmotion-components/src/heatmap.rs | 155 ++++++- crates/rustmotion-components/src/progress.rs | 167 +++++++- crates/rustmotion-components/src/sparkline.rs | 139 +++++- crates/rustmotion-components/src/stat.rs | 10 +- crates/rustmotion-components/src/treemap.rs | 250 +++++++++-- 11 files changed, 1143 insertions(+), 96 deletions(-) diff --git a/crates/rustmotion-components/src/chart/bar.rs b/crates/rustmotion-components/src/chart/bar.rs index f492341..05c59f9 100644 --- a/crates/rustmotion-components/src/chart/bar.rs +++ b/crates/rustmotion-components/src/chart/bar.rs @@ -23,7 +23,16 @@ impl Chart { // reduces to the previous `value / max_val`. let (min_val, max_val, range) = self.value_extent(); - let x_labels: Vec = self.data.iter().filter_map(|d| d.label.clone()).collect(); + // One slot per data point, unlabeled points included as `""`. + // `draw_axes` positions label `i` at slot `i` of `n = x_labels.len()` + // — a `filter_map` that dropped unlabeled points compacted the list, + // so `n` no longer matched `self.data.len()` and every surviving + // label slid onto the wrong bar as soon as one point had no label. + let x_labels: Vec = self + .data + .iter() + .map(|d| d.label.clone().unwrap_or_default()) + .collect(); self.draw_axes( canvas, ml, mt, chart_w, chart_h, min_val, max_val, &x_labels, true, @@ -269,33 +278,73 @@ impl Chart { let chart_h = h - mt - mb; let n_cats = self.categories.len(); - // Find max stacked total - let max_val = (0..n_cats) + // A stacked total can go negative — either every series is negative + // (e.g. an all-cost breakdown) or the series mix signs within one + // category (revenue vs. cost). Scale from the true signed extent — + // positive segments stack above zero, negative segments stack below + // — the same zero-anchored contract `value_extent` gives + // `render_bar`. Scaling by the largest *positive* total alone + // (previously `max_val = ... .max(0.001)`, floored to 0.001 when + // every total was negative) sent every segment's height through a + // near-zero divisor and painted it thousands of chart-heights + // outside the box; even a bounded mixed-sign case (revenue stacked + // as if `max_val` were the net total) pushed the positive segment + // taller than the whole chart. + let totals: Vec<(f64, f64)> = (0..n_cats) .map(|ci| { self.series .iter() - .map(|s| s.data.get(ci).copied().unwrap_or(0.0)) - .sum::() + .fold((0.0_f64, 0.0_f64), |(pos, neg), s| { + let v = s.data.get(ci).copied().unwrap_or(0.0); + if v >= 0.0 { + (pos + v, neg) + } else { + (pos, neg + v) + } + }) }) - .fold(0.0_f64, f64::max) - .max(0.001); + .collect(); + let min_val = totals.iter().map(|(_, neg)| *neg).fold(0.0_f64, f64::min); + let max_val = totals.iter().map(|(pos, _)| *pos).fold(0.0_f64, f64::max); + let range = (max_val - min_val).max(0.001); let x_labels: Vec = self.categories.clone(); self.draw_axes( - canvas, ml, mt, chart_w, chart_h, 0.0, max_val, &x_labels, true, + canvas, ml, mt, chart_w, chart_h, min_val, max_val, &x_labels, true, ); let gap = 8.0; let bar_w = (chart_w - gap * (n_cats + 1) as f32) / n_cats as f32; + let zero_y = mt + chart_h - ((0.0 - min_val) / range) as f32 * chart_h; for ci in 0..n_cats { let x = ml + gap + ci as f32 * (bar_w + gap); - let mut cumulative_h = 0.0_f32; + // Positive segments stack upward from `zero_y`; negative + // segments stack downward from it, independently — each + // direction has its own running edge. + let mut pos_top = zero_y; + let mut neg_bottom = zero_y; + let last_pos_si = + self.series.iter().enumerate().rev().find_map(|(si, s)| { + (s.data.get(ci).copied().unwrap_or(0.0) > 0.0).then_some(si) + }); + let last_neg_si = + self.series.iter().enumerate().rev().find_map(|(si, s)| { + (s.data.get(ci).copied().unwrap_or(0.0) < 0.0).then_some(si) + }); for (si, series) in self.series.iter().enumerate() { let val = series.data.get(ci).copied().unwrap_or(0.0); - let seg_h = (val / max_val) as f32 * chart_h * progress; - let y = mt + chart_h - cumulative_h - seg_h; + if val == 0.0 { + continue; + } + let seg_h = (val.abs() / range) as f32 * chart_h * progress; + let negative = val < 0.0; + let y = if negative { + neg_bottom + } else { + pos_top - seg_h + }; let color = series .color @@ -306,27 +355,328 @@ impl Chart { paint.set_anti_alias(true); let rect = Rect::from_xywh(x, y, bar_w, seg_h); - // Rounded top on the topmost segment only - if si == self.series.len() - 1 { + // Round the outer edge of each stack: the top of the + // topmost positive segment, the bottom of the bottommost + // negative segment. + let is_outer_edge = if negative { + Some(si) == last_neg_si + } else { + Some(si) == last_pos_si + }; + if is_outer_edge { let radius = (bar_w * 0.15).min(8.0); - let rrect = skia_safe::RRect::new_rect_radii( - rect, - &[ - (radius, radius).into(), - (radius, radius).into(), - (0.0, 0.0).into(), - (0.0, 0.0).into(), - ], - ); + let round = (radius, radius).into(); + let square = (0.0, 0.0).into(); + let radii = if negative { + [square, square, round, round] + } else { + [round, round, square, square] + }; + let rrect = skia_safe::RRect::new_rect_radii(rect, &radii); canvas.draw_rrect(rrect, &paint); } else { canvas.draw_rect(rect, &paint); } - cumulative_h += seg_h; + if negative { + neg_bottom += seg_h; + } else { + pos_top -= seg_h; + } } } Ok(()) } } + +#[cfg(test)] +mod tests { + use super::*; + use crate::chart::{ChartDataPoint, ChartSeries, ChartType}; + use rustmotion_core::css::CssStyle; + use rustmotion_core::traits::TimingConfig; + + fn base_chart(chart_type: ChartType) -> Chart { + Chart { + chart_type, + data: Vec::new(), + animated: true, + animation_duration: 1.5, + colors: None, + inner_radius: 0.6, + fill_opacity: 0.3, + smooth: false, + categories: Vec::new(), + series: Vec::new(), + axes: Vec::new(), + radar_data: Vec::new(), + points: Vec::new(), + direction: None, + show_grid: false, + show_x_labels: false, + show_y_labels: false, + grid_color: "#FFFFFF15".to_string(), + label_color: "#888888".to_string(), + label_font_size: 18.0, + show_labels: false, + timing: TimingConfig::default(), + style: CssStyle::default(), + timeline: Vec::new(), + stagger: None, + } + } + + /// Bounding box (min_x, max_x, min_y, max_y) of every non-transparent + /// pixel on the surface, or `None` if nothing was painted. Mirrors the + /// helper `stat.rs`/`caption.rs` already use for the same kind of proof. + fn ink_bounds( + surface: &mut skia_safe::Surface, + w: i32, + h: i32, + ) -> Option<(i32, i32, i32, i32)> { + let snapshot = surface.image_snapshot(); + let info = skia_safe::ImageInfo::new( + (w, h), + skia_safe::ColorType::RGBA8888, + skia_safe::AlphaType::Premul, + None, + ); + let mut buf = vec![0u8; (w * h * 4) as usize]; + let ok = snapshot.read_pixels( + &info, + &mut buf, + (w * 4) as usize, + skia_safe::IPoint::new(0, 0), + skia_safe::image::CachingHint::Disallow, + ); + assert!(ok, "pixel read should succeed"); + let (mut minx, mut maxx, mut miny, mut maxy) = (i32::MAX, i32::MIN, i32::MAX, i32::MIN); + for y in 0..h { + for x in 0..w { + if buf[((y * w + x) * 4 + 3) as usize] > 0 { + minx = minx.min(x); + maxx = maxx.max(x); + miny = miny.min(y); + maxy = maxy.max(y); + } + } + } + (minx <= maxx).then_some((minx, maxx, miny, maxy)) + } + + /// Column-wise ink centers: for each x with any ink, returns the + /// vertical midpoint of the ink found in that column. Used to find the + /// horizontal center of a run of colored pixels (a bar or a label). + fn colored_columns( + surface: &mut skia_safe::Surface, + w: i32, + h: i32, + matches_color: impl Fn(u8, u8, u8) -> bool, + ) -> Vec { + let snapshot = surface.image_snapshot(); + let info = skia_safe::ImageInfo::new( + (w, h), + skia_safe::ColorType::RGBA8888, + skia_safe::AlphaType::Premul, + None, + ); + let mut buf = vec![0u8; (w * h * 4) as usize]; + snapshot.read_pixels( + &info, + &mut buf, + (w * 4) as usize, + skia_safe::IPoint::new(0, 0), + skia_safe::image::CachingHint::Disallow, + ); + let mut cols = vec![]; + for x in 0..w { + let mut any = false; + for y in 0..h { + let idx = ((y * w + x) * 4) as usize; + let (r, g, b, a) = (buf[idx], buf[idx + 1], buf[idx + 2], buf[idx + 3]); + if a > 0 && matches_color(r, g, b) { + any = true; + break; + } + } + if any { + cols.push(x); + } + } + cols + } + + /// Paint a stacked-bar chart for a `box_w`x`box_h` box, but into a + /// canvas with `MARGIN` px of headroom above and below it, and return + /// the ink's vertical extent in *box-local* coordinates (0 = box top). + /// + /// A same-size canvas hides the bug this exists to catch: an + /// overflowing segment's `Rect::from_xywh` gets a huge *negative* + /// height, which skia normalizes when drawing, so a coincidental sliver + /// of the inverted rect can still land inside a same-size canvas even + /// though the segment as a whole is nowhere near the box. Margin on + /// both sides catches overflow in either direction; local coordinates + /// let the assertion read directly as "outside the box" without redoing + /// the offset math at every call site. + fn stacked_bar_ink_local_range(chart: &Chart, box_w: i32, box_h: i32, time: f64) -> (i32, i32) { + const MARGIN: i32 = 300; + let surface_h = box_h + MARGIN * 2; + let mut surface = + skia_safe::surfaces::raster_n32_premul((box_w, surface_h)).expect("raster surface"); + { + let canvas = surface.canvas(); + canvas.translate((0.0, MARGIN as f32)); + chart + .paint(canvas, box_w as f32, box_h as f32, time) + .expect("paint must not error"); + } + let (_minx, _maxx, miny, maxy) = ink_bounds(&mut surface, box_w, surface_h) + .expect("stacked bar chart must paint visible bars"); + (miny - MARGIN, maxy - MARGIN) + } + + #[test] + fn stacked_bar_with_all_negative_totals_stays_inside_the_box() { + // #2's exact repro: every category totals negative, so the old + // `max_val = ... .max(0.001)` floor made `seg_h` divide by a + // near-zero value and blew the segment thousands of chart-heights + // past the bottom of the box. + let mut chart = base_chart(ChartType::StackedBar); + chart.categories = vec!["Q1".to_string(), "Q2".to_string()]; + chart.series = vec![ChartSeries { + name: "net".to_string(), + data: vec![-5.0, -3.0], + color: None, + }]; + + let (local_min, local_max) = stacked_bar_ink_local_range(&chart, 400, 300, 10.0); + assert!( + local_min >= 0 && local_max < 300, + "ink escaped the 400x300 box vertically: local y=[{local_min}..{local_max}]" + ); + } + + #[test] + fn stacked_bar_with_mixed_sign_series_stays_inside_the_box() { + // Revenue/cost style stack: mixed signs within the same category. + // `max_val` alone (the pre-fix scale) ignored the negative side + // entirely, so the cost segment was scaled as if it were tiny and + // painted far below the box. + let mut chart = base_chart(ChartType::StackedBar); + chart.categories = vec!["Q1".to_string(), "Q2".to_string()]; + chart.series = vec![ + ChartSeries { + name: "revenue".to_string(), + data: vec![10.0, 8.0], + color: None, + }, + ChartSeries { + name: "cost".to_string(), + data: vec![-5.0, -3.0], + color: None, + }, + ]; + + let (local_min, local_max) = stacked_bar_ink_local_range(&chart, 400, 300, 10.0); + assert!( + local_min >= 0 && local_max < 300, + "ink escaped the 400x300 box vertically: local y=[{local_min}..{local_max}]" + ); + } + + #[test] + fn bar_x_labels_align_with_their_own_bar_when_some_points_are_unlabeled() { + // #5's exact repro: with `filter_map` compacting the label list, the + // 2 surviving labels ("AAA", "DDD") were spread across only 2 of the + // 4 slots `draw_axes` computes from `x_labels.len()`, sliding every + // label onto the wrong bar. + let mut chart = base_chart(ChartType::Bar); + chart.show_x_labels = true; + chart.data = vec![ + ChartDataPoint { + value: 50.0, + label: Some("AAA".to_string()), + color: Some("#0000FF".to_string()), + }, + ChartDataPoint { + value: 50.0, + label: None, + color: Some("#0000FF".to_string()), + }, + ChartDataPoint { + value: 50.0, + label: None, + color: Some("#0000FF".to_string()), + }, + ChartDataPoint { + value: 50.0, + label: Some("DDD".to_string()), + color: Some("#0000FF".to_string()), + }, + ]; + + const W: i32 = 400; + const H: i32 = 300; + let mut surface = skia_safe::surfaces::raster_n32_premul((W, H)).expect("raster surface"); + let bar_cols = { + let canvas = surface.canvas(); + chart + .paint(canvas, W as f32, H as f32, 10.0) + .expect("paint must not error"); + colored_columns(&mut surface, W, H, |r, g, b| r < 40 && g < 40 && b > 200) + }; + // The bar (blue) columns split into 4 contiguous runs (gaps between + // them). The label (red) columns should center within a small + // distance of the *first* and *last* runs' centers, not drift onto + // neighboring slots. + assert!(!bar_cols.is_empty(), "no bars painted"); + let mut runs: Vec<(i32, i32)> = vec![]; + for x in bar_cols { + match runs.last_mut() { + Some((_, end)) if x <= *end + 1 => *end = x, + _ => runs.push((x, x)), + } + } + assert_eq!(runs.len(), 4, "expected 4 bar slots, got {runs:?}"); + let bar0_center = (runs[0].0 + runs[0].1) as f32 / 2.0; + let bar3_center = (runs[3].0 + runs[3].1) as f32 / 2.0; + + let mut label_surface = + skia_safe::surfaces::raster_n32_premul((W, H)).expect("raster surface"); + let label_cols = { + let canvas = label_surface.canvas(); + chart + .paint(canvas, W as f32, H as f32, 10.0) + .expect("paint must not error"); + colored_columns(&mut label_surface, W, H, |r, g, b| { + // label_color default #888888 + (120..160).contains(&r) && (120..160).contains(&g) && (120..160).contains(&b) + }) + }; + assert!(!label_cols.is_empty(), "no labels painted"); + let mut label_runs: Vec<(i32, i32)> = vec![]; + for x in label_cols { + match label_runs.last_mut() { + Some((_, end)) if x <= *end + 4 => *end = x, + _ => label_runs.push((x, x)), + } + } + assert_eq!( + label_runs.len(), + 2, + "expected 2 label runs (AAA, DDD), got {label_runs:?}" + ); + let label_aaa_center = (label_runs[0].0 + label_runs[0].1) as f32 / 2.0; + let label_ddd_center = (label_runs[1].0 + label_runs[1].1) as f32 / 2.0; + + assert!( + (label_aaa_center - bar0_center).abs() < 15.0, + "AAA label (center {label_aaa_center}) should sit under bar 0 (center {bar0_center})" + ); + assert!( + (label_ddd_center - bar3_center).abs() < 15.0, + "DDD label (center {label_ddd_center}) should sit under bar 3 (center {bar3_center})" + ); + } +} diff --git a/crates/rustmotion-components/src/chart/line.rs b/crates/rustmotion-components/src/chart/line.rs index 3576fc0..16951d6 100644 --- a/crates/rustmotion-components/src/chart/line.rs +++ b/crates/rustmotion-components/src/chart/line.rs @@ -43,7 +43,11 @@ impl Chart { let (min_val, max_val, norm) = series_scale(self.data.iter().map(|d| d.value)); let n = self.data.len(); - let x_labels: Vec = self.data.iter().filter_map(|d| d.label.clone()).collect(); + let x_labels: Vec = self + .data + .iter() + .map(|d| d.label.clone().unwrap_or_default()) + .collect(); self.draw_axes( canvas, ml, mt, chart_w, chart_h, min_val, max_val, &x_labels, false, ); @@ -131,7 +135,11 @@ impl Chart { let (min_val, max_val, norm) = series_scale(self.data.iter().map(|d| d.value)); let n = self.data.len(); - let x_labels: Vec = self.data.iter().filter_map(|d| d.label.clone()).collect(); + let x_labels: Vec = self + .data + .iter() + .map(|d| d.label.clone().unwrap_or_default()) + .collect(); self.draw_axes( canvas, ml, mt, chart_w, chart_h, min_val, max_val, &x_labels, false, ); diff --git a/crates/rustmotion-components/src/chart/mod.rs b/crates/rustmotion-components/src/chart/mod.rs index 3409e12..72e1b5c 100644 --- a/crates/rustmotion-components/src/chart/mod.rs +++ b/crates/rustmotion-components/src/chart/mod.rs @@ -210,7 +210,13 @@ impl Chart { if !self.animated { return 1.0; } - let p = (time / self.animation_duration).clamp(0.0, 1.0) as f32; + // Ramp measured from `start_at`, not from scene time zero — matches + // `Counter::ramp_progress`. A chart delayed with `start_at` used to + // read raw scene time, so it was already fully drawn on the very + // first frame it became visible. + let start = self.timing.start_at.unwrap_or(0.0); + let elapsed = (time - start).max(0.0); + let p = (elapsed / self.animation_duration).clamp(0.0, 1.0) as f32; // ease_out_cubic 1.0 - (1.0 - p).powi(3) } @@ -318,3 +324,87 @@ impl Painter for Chart { let _ = self.paint(canvas, layout.width, layout.height, ctx.time); } } + +#[cfg(test)] +mod tests { + use super::*; + use rustmotion_core::traits::TimingConfig; + + fn base_chart() -> Chart { + Chart { + chart_type: ChartType::Bar, + data: Vec::new(), + animated: true, + animation_duration: 1.5, + colors: None, + inner_radius: 0.6, + fill_opacity: 0.3, + smooth: false, + categories: Vec::new(), + series: Vec::new(), + axes: Vec::new(), + radar_data: Vec::new(), + points: Vec::new(), + direction: None, + show_grid: false, + show_x_labels: false, + show_y_labels: false, + grid_color: default_grid_color(), + label_color: default_label_color(), + label_font_size: default_label_font_size(), + show_labels: false, + timing: TimingConfig::default(), + style: rustmotion_core::css::CssStyle::default(), + timeline: Vec::new(), + stagger: None, + } + } + + #[test] + fn progress_ramp_starts_at_start_at_not_at_scene_time_zero() { + // #3's exact repro: a chart delayed with `start_at: 2.0` and + // `animation_duration: 1.5` was already fully drawn (progress 1.0) + // on the very first frame it became visible, because the ramp read + // raw scene time instead of time-since-`start_at` — the same defect + // `Counter::ramp_progress` was fixed for. + let mut chart = base_chart(); + chart.animation_duration = 1.5; + chart.timing = TimingConfig { + start_at: Some(2.0), + end_at: None, + }; + + assert_eq!( + chart.progress_at(2.0), + 0.0, + "no time has elapsed since start_at yet" + ); + assert!( + chart.progress_at(2.75) < 1.0, + "still mid-ramp half a second after start_at" + ); + assert_eq!( + chart.progress_at(3.5), + 1.0, + "animation_duration has fully elapsed since start_at" + ); + } + + #[test] + fn progress_ramp_with_no_start_at_behaves_like_before() { + let chart = base_chart(); + assert_eq!(chart.progress_at(0.0), 0.0); + assert_eq!(chart.progress_at(1.5), 1.0); + } + + #[test] + fn progress_ramp_when_not_animated_is_always_complete() { + let mut chart = base_chart(); + chart.animated = false; + chart.timing = TimingConfig { + start_at: Some(2.0), + end_at: None, + }; + assert_eq!(chart.progress_at(0.0), 1.0); + } +} diff --git a/crates/rustmotion-components/src/chart/waterfall.rs b/crates/rustmotion-components/src/chart/waterfall.rs index 8b7c269..bda5090 100644 --- a/crates/rustmotion-components/src/chart/waterfall.rs +++ b/crates/rustmotion-components/src/chart/waterfall.rs @@ -35,7 +35,11 @@ impl Chart { let max_val = all_vals.iter().fold(f64::MIN, |a, &b| a.max(b)); let range = (max_val - min_val).max(0.001); - let x_labels: Vec = self.data.iter().filter_map(|d| d.label.clone()).collect(); + let x_labels: Vec = self + .data + .iter() + .map(|d| d.label.clone().unwrap_or_default()) + .collect(); self.draw_axes( canvas, ml, mt, chart_w, chart_h, min_val, max_val, &x_labels, true, ); diff --git a/crates/rustmotion-components/src/dot_map.rs b/crates/rustmotion-components/src/dot_map.rs index 8fa9438..5e5680a 100644 --- a/crates/rustmotion-components/src/dot_map.rs +++ b/crates/rustmotion-components/src/dot_map.rs @@ -133,7 +133,11 @@ impl DotMap { if !self.animated { return 1.0; } - let p = (time / self.animation_duration).clamp(0.0, 1.0) as f32; + // Measure from `start_at`, like every other animated component: driving + // the ramp off raw scene time makes a delayed map arrive already drawn. + let start = self.timing.start_at.unwrap_or(0.0); + let elapsed = (time - start).max(0.0); + let p = (elapsed / self.animation_duration).clamp(0.0, 1.0) as f32; 1.0 - (1.0 - p).powi(3) } diff --git a/crates/rustmotion-components/src/gauge.rs b/crates/rustmotion-components/src/gauge.rs index c62449d..bb939c9 100644 --- a/crates/rustmotion-components/src/gauge.rs +++ b/crates/rustmotion-components/src/gauge.rs @@ -95,7 +95,11 @@ impl Gauge { if !self.animated { return 1.0; } - let p = (time / self.animation_duration).clamp(0.0, 1.0) as f32; + // Measure from `start_at`, like every other animated component: driving + // the ramp off raw scene time makes a delayed gauge arrive already full. + let start = self.timing.start_at.unwrap_or(0.0); + let elapsed = (time - start).max(0.0); + let p = (elapsed / self.animation_duration).clamp(0.0, 1.0) as f32; 1.0 - (1.0 - p).powi(3) } diff --git a/crates/rustmotion-components/src/heatmap.rs b/crates/rustmotion-components/src/heatmap.rs index 246ae1f..bfb20e4 100644 --- a/crates/rustmotion-components/src/heatmap.rs +++ b/crates/rustmotion-components/src/heatmap.rs @@ -90,11 +90,16 @@ fn interpolate_color(scale: &[String], t: f32) -> (u8, u8, u8) { return (r, g, b); } let n = scale.len() - 1; - let segment = (t * n as f32).floor() as usize; - let local_t = t * n as f32 - segment as f32; - let i = segment.min(n - 1); - let (r1, g1, b1, _) = parse_hex_color(&scale[i]); - let (r2, g2, b2, _) = parse_hex_color(&scale[i + 1]); + let scaled = t * n as f32; + // Clamp the segment index (t=1.0 lands exactly on `n`, one past the + // last valid segment), but re-derive `local_t` from the *clamped* + // segment rather than reusing the unclamped one — otherwise t=1.0 + // computed local_t=0.0 against the clamped (second-to-last) segment and + // resolved to the second-to-last color instead of the last one. + let segment = (scaled.floor() as usize).min(n - 1); + let local_t = (scaled - segment as f32).clamp(0.0, 1.0); + let (r1, g1, b1, _) = parse_hex_color(&scale[segment]); + let (r2, g2, b2, _) = parse_hex_color(&scale[segment + 1]); ( lerp_u8(r1, r2, local_t), lerp_u8(g1, g2, local_t), @@ -107,7 +112,13 @@ impl Heatmap { if !self.animated { return 1.0; } - let p = (time / self.animation_duration).clamp(0.0, 1.0) as f32; + // Ramp measured from `start_at`, not from scene time zero — matches + // `Counter::ramp_progress`. A heatmap delayed with `start_at` used + // to read raw scene time, so it was already fully revealed on the + // very first frame it became visible. + let start = self.timing.start_at.unwrap_or(0.0); + let elapsed = (time - start).max(0.0); + let p = (elapsed / self.animation_duration).clamp(0.0, 1.0) as f32; 1.0 - (1.0 - p).powi(3) } @@ -121,17 +132,6 @@ impl Heatmap { let progress = self.progress_at(time); - // Find min/max across all cells - let mut min_val = f64::MAX; - let mut max_val = f64::MIN; - for row in &self.data { - for &val in row { - min_val = min_val.min(val); - max_val = max_val.max(val); - } - } - let range = (max_val - min_val).max(0.001); - // Animation: clip rect expanding from left to right let clip_w = w * progress; canvas.save(); @@ -145,7 +145,15 @@ impl Heatmap { for (row_idx, row) in self.data.iter().enumerate() { for (col_idx, &val) in row.iter().enumerate() { - let normalized = ((val - min_val) / range) as f32; + // `color_scale` documents an absolute 0.0-1.0 semantic + // (SKILL.md: "2D array of f64, values 0.0-1.0"), not a + // per-render min-max scale. Renormalizing meant a grid of + // constant values (or any subrange, e.g. [0.8, 0.9, 1.0]) + // painted identically to a grid of zeros — a flat or + // uniformly-high grid is not the same fact as "nothing + // happened". Clamp into the documented range instead of + // rescaling to whatever the data happens to span. + let normalized = (val as f32).clamp(0.0, 1.0); let (r, g, b) = interpolate_color(&self.color_scale, normalized); let x = col_idx as f32 * step; @@ -179,3 +187,114 @@ impl Painter for Heatmap { self.paint(canvas, layout.width, layout.height, ctx.time); } } + +#[cfg(test)] +mod tests { + use super::*; + use rustmotion_core::traits::TimingConfig; + + fn base_heatmap(data: Vec>) -> Heatmap { + Heatmap { + data, + color_scale: default_color_scale(), + cell_size: default_cell_size(), + cell_gap: default_cell_gap(), + cell_radius: default_cell_radius(), + animated: true, + animation_duration: 1.5, + timing: TimingConfig::default(), + style: CssStyle::default(), + timeline: Vec::new(), + stagger: None, + } + } + + fn cell_color(heatmap: &Heatmap, w: i32, h: i32, time: f64) -> (u8, u8, u8) { + let mut surface = skia_safe::surfaces::raster_n32_premul((w, h)).expect("raster surface"); + { + let canvas = surface.canvas(); + heatmap.paint(canvas, w as f32, h as f32, time); + } + let snapshot = surface.image_snapshot(); + let info = skia_safe::ImageInfo::new( + (1, 1), + skia_safe::ColorType::RGBA8888, + skia_safe::AlphaType::Premul, + None, + ); + let mut buf = [0u8; 4]; + // Sample the middle of the top-left cell. + let x = (heatmap.cell_size / 2.0) as i32; + let y = (heatmap.cell_size / 2.0) as i32; + snapshot.read_pixels( + &info, + &mut buf, + 4, + skia_safe::IPoint::new(x, y), + skia_safe::image::CachingHint::Disallow, + ); + (buf[0], buf[1], buf[2]) + } + + #[test] + fn a_uniformly_low_grid_is_not_identical_to_an_all_zero_grid() { + // #6's exact repro: `color_scale` is documented (SKILL.md) as an + // *absolute* 0.0-1.0 scale, but the painter renormalized min→max — + // so a grid of constant 5.0s (or any other constant) rendered + // pixel-for-pixel identical to a grid of constant 0.0s, both + // collapsing to the scale's first (lowest) color. + let uniform = base_heatmap(vec![vec![5.0, 5.0, 5.0], vec![5.0, 5.0, 5.0]]); + let zero = base_heatmap(vec![vec![0.0, 0.0, 0.0], vec![0.0, 0.0, 0.0]]); + let uniform_color = cell_color(&uniform, 200, 100, 10.0); + let zero_color = cell_color(&zero, 200, 100, 10.0); + assert_ne!( + uniform_color, zero_color, + "a grid of 5.0s must not render identically to a grid of 0.0s" + ); + } + + #[test] + fn absolute_values_are_not_renormalized_to_the_data_subrange() { + // A grid whose values happen to span [0.8, 1.0] must not stretch + // that subrange to fill the whole color scale — 0.8 reads as + // "mostly full", not as "the bottom of whatever this grid contains". + let high = base_heatmap(vec![vec![0.8, 0.9, 1.0]]); + let low = base_heatmap(vec![vec![0.0, 0.1, 0.2]]); + let high_first_cell = cell_color(&high, 200, 100, 10.0); + let low_first_cell = cell_color(&low, 200, 100, 10.0); + assert_ne!( + high_first_cell, low_first_cell, + "0.8 and 0.0 must not render as the same color" + ); + } + + #[test] + fn interpolate_color_at_the_top_of_the_scale_returns_the_last_color() { + // Surfaced while chasing #6: the clamped segment index was reused + // for `local_t` too, so t=1.0 exactly computed `local_t = 0.0` for + // the *clamped* (second-to-last) segment instead of `local_t = 1.0` + // — landing on the second-to-last color rather than the last + // (brightest) one. + let scale = default_color_scale(); + let (r, g, b) = interpolate_color(&scale, 1.0); + let (er, eg, eb, _) = parse_hex_color(scale.last().unwrap()); + assert_eq!( + (r, g, b), + (er, eg, eb), + "t=1.0 must resolve to the last color in the scale" + ); + } + + #[test] + fn progress_ramp_starts_at_start_at_not_at_scene_time_zero() { + let mut heatmap = base_heatmap(vec![vec![1.0]]); + heatmap.animation_duration = 1.5; + heatmap.timing = TimingConfig { + start_at: Some(2.0), + end_at: None, + }; + assert_eq!(heatmap.progress_at(2.0), 0.0); + assert!(heatmap.progress_at(2.75) < 1.0); + assert_eq!(heatmap.progress_at(3.5), 1.0); + } +} diff --git a/crates/rustmotion-components/src/progress.rs b/crates/rustmotion-components/src/progress.rs index 3a0f0d5..0c0b358 100644 --- a/crates/rustmotion-components/src/progress.rs +++ b/crates/rustmotion-components/src/progress.rs @@ -75,10 +75,10 @@ rustmotion_core::impl_traits!(Progress { }); impl Progress { - fn paint(&self, canvas: &Canvas) -> Result<()> { + fn paint(&self, canvas: &Canvas, w: f32, h: f32) -> Result<()> { match self.variant { - ProgressVariant::Linear => self.render_linear(canvas), - ProgressVariant::Circular => self.render_circular(canvas), + ProgressVariant::Linear => self.render_linear(canvas, w, h), + ProgressVariant::Circular => self.render_circular(canvas, w, h), } } } @@ -87,18 +87,24 @@ impl Painter for Progress { fn paint_content( &self, canvas: &Canvas, - _layout: &BoxLayout, + layout: &BoxLayout, _props: &AnimatedProperties, _ctx: &PaintCtx, ) { - let _ = self.paint(canvas); + // `self.width`/`self.height` only seed the *intrinsic* size in + // `box_builder` (promoted to CSS when `style.width`/`style.height` + // are absent) — the box taffy actually assigns can differ whenever + // an author sets `style.width`/`style.height` or a flex-grow + // idiom directly, which `html-css-mental-model.md` recommends. + // Painting at `self.width`/`self.height` regardless left the fill + // sized to whichever one happened to be smaller, filling only part + // of its own box (or overflowing it) instead of the box. + let _ = self.paint(canvas, layout.width, layout.height); } } impl Progress { - fn render_linear(&self, canvas: &Canvas) -> Result<()> { - let w = self.width; - let h = self.height; + fn render_linear(&self, canvas: &Canvas, w: f32, h: f32) -> Result<()> { let radius = self.border_radius; let progress = self.progress.clamp(0.0, 1.0) as f32; @@ -129,14 +135,12 @@ impl Progress { Ok(()) } - fn render_circular(&self, canvas: &Canvas) -> Result<()> { - let w = self.width; - let h = self.height; + fn render_circular(&self, canvas: &Canvas, w: f32, h: f32) -> Result<()> { let progress = self.progress.clamp(0.0, 1.0) as f32; let cx = w / 2.0; let cy = h / 2.0; - let radius = cx.min(cy) - self.track_width / 2.0 - 2.0; + let radius = (cx.min(cy) - self.track_width / 2.0 - 2.0).max(0.0); let oval = Rect::from_xywh(cx - radius, cy - radius, radius * 2.0, radius * 2.0); // Track @@ -192,3 +196,142 @@ impl Progress { Ok(()) } } + +#[cfg(test)] +mod tests { + use super::*; + + fn base_progress(variant: ProgressVariant) -> Progress { + Progress { + progress: 0.5, + variant, + width: default_progress_width(), + height: default_progress_height(), + background_color: default_progress_bg(), + fill_color: default_progress_fill(), + border_radius: 0.0, + track_width: default_track_width(), + show_value: false, + timing: TimingConfig::default(), + style: CssStyle::default(), + timeline: Vec::new(), + stagger: None, + } + } + + fn base_ctx() -> PaintCtx { + PaintCtx { + time: 0.0, + scene_duration: 2.0, + frame_index: 0, + fps: 30, + video_width: 900, + video_height: 200, + stagger_offset: 0.0, + } + } + + fn ink_bounds( + surface: &mut skia_safe::Surface, + w: i32, + h: i32, + ) -> Option<(i32, i32, i32, i32)> { + let snapshot = surface.image_snapshot(); + let info = skia_safe::ImageInfo::new( + (w, h), + skia_safe::ColorType::RGBA8888, + skia_safe::AlphaType::Premul, + None, + ); + let mut buf = vec![0u8; (w * h * 4) as usize]; + snapshot.read_pixels( + &info, + &mut buf, + (w * 4) as usize, + skia_safe::IPoint::new(0, 0), + skia_safe::image::CachingHint::Disallow, + ); + let (mut minx, mut maxx, mut miny, mut maxy) = (i32::MAX, i32::MIN, i32::MAX, i32::MIN); + for y in 0..h { + for x in 0..w { + if buf[((y * w + x) * 4 + 3) as usize] > 0 { + minx = minx.min(x); + maxx = maxx.max(x); + miny = miny.min(y); + maxy = maxy.max(y); + } + } + } + (minx <= maxx).then_some((minx, maxx, miny, maxy)) + } + + #[test] + fn linear_progress_fills_the_layout_box_not_its_own_width_height() { + // #4's exact repro: `render_linear` always drew at `self.width` x + // `self.height` (defaults 300x20) regardless of the box taffy + // actually assigned it. `box_builder` only promotes `c.width`/ + // `c.height` to CSS when `style.width`/`style.height` are absent — + // so a `progress` sized via `style.width: 800` (the project's + // CSS-first idiom) painted a 300px-wide bar sitting inside an + // 800px-wide box, filling only 37% of it at `progress: 0.5`. + let progress = base_progress(ProgressVariant::Linear); + const BOX_W: f32 = 800.0; + const BOX_H: f32 = 24.0; + let layout = BoxLayout { + width: BOX_W, + height: BOX_H, + ..Default::default() + }; + let ctx = base_ctx(); + let props = AnimatedProperties::default(); + + let mut surface = + skia_safe::surfaces::raster_n32_premul((900, 200)).expect("raster surface"); + { + let canvas = surface.canvas(); + progress.paint_content(canvas, &layout, &props, &ctx); + } + let (_minx, maxx, _miny, _maxy) = + ink_bounds(&mut surface, 900, 200).expect("progress must paint something"); + // At progress 0.5 the fill should reach roughly the middle of the + // 800px box (~400px), not the middle of the component's own + // `width` field (300px -> 150px). + assert!( + maxx as f32 > BOX_W * 0.4, + "fill did not scale to the box's own width: max ink x = {maxx}, box width = {BOX_W}" + ); + } + + #[test] + fn circular_progress_fits_the_layout_box_not_its_own_width_height() { + let progress = base_progress(ProgressVariant::Circular); + const BOX_W: f32 = 60.0; + const BOX_H: f32 = 60.0; + let layout = BoxLayout { + width: BOX_W, + height: BOX_H, + ..Default::default() + }; + let ctx = base_ctx(); + let props = AnimatedProperties::default(); + + let mut surface = + skia_safe::surfaces::raster_n32_premul((300, 300)).expect("raster surface"); + { + let canvas = surface.canvas(); + progress.paint_content(canvas, &layout, &props, &ctx); + } + let (minx, maxx, miny, maxy) = + ink_bounds(&mut surface, 300, 300).expect("progress must paint something"); + // The ring must be centered on the 60x60 box's own center (30, 30), + // not on the component's own `width`/`height` fields' center + // (150, 10 for the 300x20 defaults) — the un-fixed painter puts the + // whole ring outside a small box entirely. + let center_x = (minx + maxx) as f32 / 2.0; + let center_y = (miny + maxy) as f32 / 2.0; + assert!( + (center_x - BOX_W / 2.0).abs() < 5.0 && (center_y - BOX_H / 2.0).abs() < 5.0, + "ring is not centered on the {BOX_W}x{BOX_H} box: center=({center_x}, {center_y})" + ); + } +} diff --git a/crates/rustmotion-components/src/sparkline.rs b/crates/rustmotion-components/src/sparkline.rs index a8946d4..e5fde0f 100644 --- a/crates/rustmotion-components/src/sparkline.rs +++ b/crates/rustmotion-components/src/sparkline.rs @@ -60,12 +60,45 @@ rustmotion_core::impl_traits!(Sparkline { Styled => style, }); +/// `(min, max, normalize)` for a min-max scaled series, matching +/// `chart::line::series_scale`'s flat-series handling: a constant series is +/// centred (`0.5`) instead of collapsing to the bottom edge. Dividing by a +/// `max(0.001)` floor mapped a constant series to 0, so a flat series read +/// as "all zero" instead of "constant at some value". Duplicated locally — +/// `chart::line::series_scale` is `pub(super)` to the `chart` module, not +/// reachable from here. +fn series_scale(values: impl Iterator + Clone) -> (f64, f64, impl Fn(f64) -> f32) { + let min_val = values.clone().fold(f64::INFINITY, f64::min); + let max_val = values.fold(f64::NEG_INFINITY, f64::max); + let (min_val, max_val) = if min_val.is_finite() && max_val.is_finite() { + (min_val, max_val) + } else { + (0.0, 0.0) + }; + let span = max_val - min_val; + let flat = span.abs() < f64::EPSILON; + let range = if flat { 1.0 } else { span }; + (min_val, max_val, move |v: f64| { + if flat { + 0.5 + } else { + ((v - min_val) / range) as f32 + } + }) +} + impl Sparkline { fn progress_at(&self, time: f64) -> f32 { if !self.animated { return 1.0; } - let p = (time / self.animation_duration).clamp(0.0, 1.0) as f32; + // Ramp measured from `start_at`, not from scene time zero — matches + // `Counter::ramp_progress`. A sparkline delayed with `start_at` used + // to read raw scene time, so it was already fully revealed on the + // very first frame it became visible. + let start = self.timing.start_at.unwrap_or(0.0); + let elapsed = (time - start).max(0.0); + let p = (elapsed / self.animation_duration).clamp(0.0, 1.0) as f32; 1.0 - (1.0 - p).powi(3) } @@ -79,9 +112,7 @@ impl Sparkline { let progress = self.progress_at(time); - let max_val = self.data.iter().fold(f64::MIN, |a, &b| a.max(b)); - let min_val = self.data.iter().fold(f64::MAX, |a, &b| a.min(b)); - let range = (max_val - min_val).max(0.001); + let (_, _, norm) = series_scale(self.data.iter().copied()); let pad = self.stroke_width; @@ -90,7 +121,7 @@ impl Sparkline { for (i, &val) in self.data.iter().enumerate() { let x = pad + (i as f32 / (n - 1) as f32) * (w - pad * 2.0); - let y = pad + (h - pad * 2.0) - ((val - min_val) / range) as f32 * (h - pad * 2.0); + let y = pad + (h - pad * 2.0) - norm(val) * (h - pad * 2.0); if i == 0 { line_path.move_to((x, y)); @@ -166,3 +197,101 @@ impl Painter for Sparkline { self.paint(canvas, layout.width, layout.height, ctx.time); } } + +#[cfg(test)] +mod tests { + use super::*; + use rustmotion_core::traits::TimingConfig; + + fn base_sparkline(data: Vec) -> Sparkline { + Sparkline { + data, + color: default_color(), + fill: false, + fill_opacity: default_fill_opacity(), + stroke_width: default_stroke_width(), + animated: true, + animation_duration: 1.0, + timing: TimingConfig::default(), + style: CssStyle::default(), + timeline: Vec::new(), + stagger: None, + } + } + + fn ink_bounds( + surface: &mut skia_safe::Surface, + w: i32, + h: i32, + ) -> Option<(i32, i32, i32, i32)> { + let snapshot = surface.image_snapshot(); + let info = skia_safe::ImageInfo::new( + (w, h), + skia_safe::ColorType::RGBA8888, + skia_safe::AlphaType::Premul, + None, + ); + let mut buf = vec![0u8; (w * h * 4) as usize]; + snapshot.read_pixels( + &info, + &mut buf, + (w * 4) as usize, + skia_safe::IPoint::new(0, 0), + skia_safe::image::CachingHint::Disallow, + ); + let (mut minx, mut maxx, mut miny, mut maxy) = (i32::MAX, i32::MIN, i32::MAX, i32::MIN); + for y in 0..h { + for x in 0..w { + if buf[((y * w + x) * 4 + 3) as usize] > 0 { + minx = minx.min(x); + maxx = maxx.max(x); + miny = miny.min(y); + maxy = maxy.max(y); + } + } + } + (minx <= maxx).then_some((minx, maxx, miny, maxy)) + } + + #[test] + fn a_flat_series_is_centered_not_pinned_to_the_bottom_edge() { + // #7's exact repro: with every value equal, `(val - min_val)` is + // 0 for every point, so the line was drawn on the bottom edge + // (`h - pad`) — reading as "a series of zeroes" instead of "a + // constant series at some value". `chart::line::series_scale` + // was fixed to center a flat series (0.5) for exactly this reason. + const H: i32 = 40; + let flat = base_sparkline(vec![7.0, 7.0, 7.0, 7.0, 7.0]); + let mut surface = skia_safe::surfaces::raster_n32_premul((120, H)).expect("raster surface"); + { + let canvas = surface.canvas(); + flat.paint(canvas, 120.0, H as f32, 10.0); + } + let (_minx, _maxx, miny, maxy) = + ink_bounds(&mut surface, 120, H).expect("flat sparkline must still paint a line"); + let mid = (miny + maxy) as f32 / 2.0; + let bottom_edge = H as f32 - flat.stroke_width; + assert!( + (mid - H as f32 / 2.0).abs() < 6.0, + "flat series line should sit near vertical center (y~{}), got y=[{miny}..{maxy}]", + H / 2 + ); + assert!( + (bottom_edge - maxy as f32).abs() > 6.0, + "flat series line must not be pinned to the bottom edge: y=[{miny}..{maxy}], bottom={bottom_edge}" + ); + } + + #[test] + fn progress_ramp_starts_at_start_at_not_at_scene_time_zero() { + let mut sparkline = base_sparkline(vec![1.0, 2.0, 3.0]); + sparkline.animation_duration = 1.5; + sparkline.timing = TimingConfig { + start_at: Some(2.0), + end_at: None, + }; + assert_eq!(sparkline.progress_at(2.0), 0.0); + assert!(sparkline.progress_at(2.75) < 1.0); + assert_eq!(sparkline.progress_at(3.5), 1.0); + } +} diff --git a/crates/rustmotion-components/src/stat.rs b/crates/rustmotion-components/src/stat.rs index e1b3b0d..740ed5d 100644 --- a/crates/rustmotion-components/src/stat.rs +++ b/crates/rustmotion-components/src/stat.rs @@ -323,7 +323,12 @@ impl Stat { let max_v = self.sparkline_data.iter().fold(f64::MIN, |a, &b| a.max(b)); let min_v = self.sparkline_data.iter().fold(f64::MAX, |a, &b| a.min(b)); - let range = (max_v - min_v).max(0.001); + // A flat series has no span. Flooring the divisor instead normalises + // every point to 0, which glues the line to the bottom edge and reads + // as "collapsed to zero" rather than "unchanged" — centre it instead, + // matching `chart::line`'s handling of the same case. + let span = max_v - min_v; + let flat = span.abs() < f64::EPSILON; let n = self.sparkline_data.len(); let spark_color = self.sparkline_color.as_deref().unwrap_or("#3B82F6"); @@ -333,7 +338,8 @@ impl Stat { for (i, &val) in self.sparkline_data.iter().enumerate() { let x = pad + (i as f32 / (n - 1) as f32) * spark_w; - let y = spark_y + spark_h - ((val - min_v) / range) as f32 * spark_h; + let norm = if flat { 0.5 } else { (val - min_v) / span }; + let y = spark_y + spark_h - norm as f32 * spark_h; if i == 0 { line_path.move_to((x, y)); diff --git a/crates/rustmotion-components/src/treemap.rs b/crates/rustmotion-components/src/treemap.rs index b29195d..fb7872b 100644 --- a/crates/rustmotion-components/src/treemap.rs +++ b/crates/rustmotion-components/src/treemap.rs @@ -113,7 +113,13 @@ impl Treemap { if !self.animated { return 1.0; } - let p = (time / self.animation_duration).clamp(0.0, 1.0) as f32; + // Ramp measured from `start_at`, not from scene time zero — matches + // `Counter::ramp_progress`. A treemap delayed with `start_at` used + // to read raw scene time, so it was already fully scaled in on the + // very first frame it became visible. + let start = self.timing.start_at.unwrap_or(0.0); + let elapsed = (time - start).max(0.0); + let p = (elapsed / self.animation_duration).clamp(0.0, 1.0) as f32; 1.0 - (1.0 - p).powi(3) } @@ -182,11 +188,35 @@ impl Treemap { // Labels if self.show_labels || self.show_values { - if scaled_rect.width() < 30.0 || scaled_rect.height() < 20.0 { + let mut text_parts: Vec = vec![]; + if self.show_labels { + if let Some(label) = &item.label { + text_parts.push(label.clone()); + } + } + if self.show_values { + text_parts.push(format!("{}", item.value)); + } + + if text_parts.is_empty() { continue; } let font_size = (scaled_rect.width() * 0.12).clamp(10.0, 24.0); + // `draw_text_with_fallback` builds a single-line `TextBlob` + // (renderer/text.rs) — joining label and value with "\n" + // never produced a line break, it fed the blob a literal + // control glyph. Each part now gets its own baseline, and + // the space each line needs (`20.0` per line, same floor + // the old single-line check used) is checked before + // drawing instead of after. + let line_height = font_size * 1.2; + if scaled_rect.width() < 30.0 + || scaled_rect.height() < 20.0 * text_parts.len() as f32 + { + continue; + } + let font = skia_safe::Font::from_typeface(&typeface, font_size); let emoji_font = emoji_typeface().map(|tf| skia_safe::Font::from_typeface(tf, font_size)); @@ -194,35 +224,29 @@ impl Treemap { let mut text_paint = paint_from_hex("#FFFFFF"); text_paint.set_anti_alias(true); - let mut text_parts: Vec = vec![]; - if self.show_labels { - if let Some(label) = &item.label { - text_parts.push(label.clone()); - } - } - if self.show_values { - text_parts.push(format!("{}", item.value)); - } - - let text = text_parts.join("\n"); - let text_w = measure_text_with_fallback(&text, &font, &emoji_font, 0.0); let (_, metrics) = font.metrics(); - - let text_x = scaled_rect.left + (scaled_rect.width() - text_w) / 2.0; - let text_y = scaled_rect.top - + scaled_rect.height() / 2.0 - + (-metrics.ascent - metrics.descent) / 2.0; - - draw_text_with_fallback( - canvas, - &text, - &font, - &emoji_font, - 0.0, - text_x, - text_y, - &text_paint, - ); + let ascent = -metrics.ascent; + let descent = metrics.descent; + let block_h = line_height * text_parts.len() as f32; + let block_top = scaled_rect.top + scaled_rect.height() / 2.0 - block_h / 2.0; + + for (li, part) in text_parts.iter().enumerate() { + let part_w = measure_text_with_fallback(part, &font, &emoji_font, 0.0); + let part_x = scaled_rect.left + (scaled_rect.width() - part_w) / 2.0; + let line_center_y = block_top + (li as f32 + 0.5) * line_height; + let part_y = line_center_y + (ascent - descent) / 2.0; + + draw_text_with_fallback( + canvas, + part, + &font, + &emoji_font, + 0.0, + part_x, + part_y, + &text_paint, + ); + } } } } @@ -239,3 +263,169 @@ impl Painter for Treemap { self.paint(canvas, layout.width, layout.height, ctx.time); } } + +#[cfg(test)] +mod tests { + use super::*; + use rustmotion_core::traits::TimingConfig; + + fn base_treemap(data: Vec) -> Treemap { + Treemap { + data, + gap: default_gap(), + border_radius: default_border_radius(), + show_labels: default_show_labels(), + show_values: false, + animated: true, + animation_duration: 1.0, + timing: TimingConfig::default(), + style: CssStyle::default(), + timeline: Vec::new(), + stagger: None, + } + } + + fn read_rgba(surface: &mut skia_safe::Surface, w: i32, h: i32) -> Vec { + let snapshot = surface.image_snapshot(); + let info = skia_safe::ImageInfo::new( + (w, h), + skia_safe::ColorType::RGBA8888, + skia_safe::AlphaType::Premul, + None, + ); + let mut buf = vec![0u8; (w * h * 4) as usize]; + snapshot.read_pixels( + &info, + &mut buf, + (w * 4) as usize, + skia_safe::IPoint::new(0, 0), + skia_safe::image::CachingHint::Disallow, + ); + buf + } + + /// A pixel is "text ink" if it's opaque-ish and near-white — the fixed + /// `#FFFFFF` label/value color, distinct from the cell's own colored + /// (never white) `DEFAULT_PALETTE` background fill that would otherwise + /// dominate a naive alpha-only scan. + fn is_text_ink(buf: &[u8], w: i32, x: i32, y: i32) -> bool { + let idx = ((y * w + x) * 4) as usize; + let (r, g, b, a) = (buf[idx], buf[idx + 1], buf[idx + 2], buf[idx + 3]); + a > 40 && r > 200 && g > 200 && b > 200 + } + + /// Contiguous vertical bands (start_y, end_y) of text ink, merging rows + /// separated by a 1px anti-aliasing gap but splitting on anything + /// wider — used to tell "two stacked text lines" apart from "one line + /// of text". + fn row_bands(buf: &[u8], w: i32, h: i32) -> Vec<(i32, i32)> { + let mut bands: Vec<(i32, i32)> = vec![]; + for y in 0..h { + let has_ink = (0..w).any(|x| is_text_ink(buf, w, x, y)); + if has_ink { + match bands.last_mut() { + Some((_, end)) if y <= *end + 1 => *end = y, + _ => bands.push((y, y)), + } + } + } + bands + } + + fn row_ink_x_range(buf: &[u8], w: i32, y0: i32, y1: i32) -> (i32, i32) { + let (mut minx, mut maxx) = (i32::MAX, i32::MIN); + for y in y0..=y1 { + for x in 0..w { + if is_text_ink(buf, w, x, y) { + minx = minx.min(x); + maxx = maxx.max(x); + } + } + } + (minx, maxx) + } + + #[test] + fn label_and_value_render_on_two_separate_centered_lines() { + // #8's exact repro: `text_parts.join("\n")` fed a single-line + // `TextBlob` a literal "\n" glyph — the label and value landed side + // by side on the same baseline instead of stacked, and the whole + // (wrongly wide) string was centered as one block, decentering the + // label itself. + const W: i32 = 300; + const H: i32 = 200; + let mut treemap = base_treemap(vec![TreemapItem { + label: Some("Alpha".to_string()), + value: 50.0, + color: None, + }]); + treemap.show_labels = true; + treemap.show_values = true; + treemap.animated = false; + + let mut surface = skia_safe::surfaces::raster_n32_premul((W, H)).expect("raster surface"); + { + let canvas = surface.canvas(); + treemap.paint(canvas, W as f32, H as f32, 0.0); + } + let buf = read_rgba(&mut surface, W, H); + let bands = row_bands(&buf, W, H); + assert_eq!( + bands.len(), + 2, + "label and value must render as two stacked lines, got bands={bands:?}" + ); + for (y0, y1) in bands { + let (minx, maxx) = row_ink_x_range(&buf, W, y0, y1); + let center = (minx + maxx) as f32 / 2.0; + assert!( + (center - W as f32 / 2.0).abs() < 12.0, + "line y=[{y0}..{y1}] is not centered on the box: ink x center = {center}" + ); + } + } + + #[test] + fn a_single_label_still_renders_as_one_centered_line() { + const W: i32 = 300; + const H: i32 = 200; + let mut treemap = base_treemap(vec![TreemapItem { + label: Some("Alpha".to_string()), + value: 50.0, + color: None, + }]); + treemap.show_labels = true; + treemap.show_values = false; + treemap.animated = false; + + let mut surface = skia_safe::surfaces::raster_n32_premul((W, H)).expect("raster surface"); + { + let canvas = surface.canvas(); + treemap.paint(canvas, W as f32, H as f32, 0.0); + } + let buf = read_rgba(&mut surface, W, H); + let bands = row_bands(&buf, W, H); + assert_eq!( + bands.len(), + 1, + "a single label is one line, got bands={bands:?}" + ); + } + + #[test] + fn progress_ramp_starts_at_start_at_not_at_scene_time_zero() { + let mut treemap = base_treemap(vec![TreemapItem { + label: None, + value: 1.0, + color: None, + }]); + treemap.animation_duration = 1.5; + treemap.timing = TimingConfig { + start_at: Some(2.0), + end_at: None, + }; + assert_eq!(treemap.progress_at(2.0), 0.0); + assert!(treemap.progress_at(2.75) < 1.0); + assert_eq!(treemap.progress_at(3.5), 1.0); + } +}