Skip to content

Commit fa08b69

Browse files
committed
fix(dataviz): honour start_at, the layout box, and degenerate scales
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.
1 parent fa0eb84 commit fa08b69

11 files changed

Lines changed: 1143 additions & 96 deletions

File tree

crates/rustmotion-components/src/chart/bar.rs

Lines changed: 373 additions & 23 deletions
Large diffs are not rendered by default.

crates/rustmotion-components/src/chart/line.rs

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,11 @@ impl Chart {
4343
let (min_val, max_val, norm) = series_scale(self.data.iter().map(|d| d.value));
4444

4545
let n = self.data.len();
46-
let x_labels: Vec<String> = self.data.iter().filter_map(|d| d.label.clone()).collect();
46+
let x_labels: Vec<String> = self
47+
.data
48+
.iter()
49+
.map(|d| d.label.clone().unwrap_or_default())
50+
.collect();
4751
self.draw_axes(
4852
canvas, ml, mt, chart_w, chart_h, min_val, max_val, &x_labels, false,
4953
);
@@ -131,7 +135,11 @@ impl Chart {
131135
let (min_val, max_val, norm) = series_scale(self.data.iter().map(|d| d.value));
132136

133137
let n = self.data.len();
134-
let x_labels: Vec<String> = self.data.iter().filter_map(|d| d.label.clone()).collect();
138+
let x_labels: Vec<String> = self
139+
.data
140+
.iter()
141+
.map(|d| d.label.clone().unwrap_or_default())
142+
.collect();
135143
self.draw_axes(
136144
canvas, ml, mt, chart_w, chart_h, min_val, max_val, &x_labels, false,
137145
);

crates/rustmotion-components/src/chart/mod.rs

Lines changed: 91 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -210,7 +210,13 @@ impl Chart {
210210
if !self.animated {
211211
return 1.0;
212212
}
213-
let p = (time / self.animation_duration).clamp(0.0, 1.0) as f32;
213+
// Ramp measured from `start_at`, not from scene time zero — matches
214+
// `Counter::ramp_progress`. A chart delayed with `start_at` used to
215+
// read raw scene time, so it was already fully drawn on the very
216+
// first frame it became visible.
217+
let start = self.timing.start_at.unwrap_or(0.0);
218+
let elapsed = (time - start).max(0.0);
219+
let p = (elapsed / self.animation_duration).clamp(0.0, 1.0) as f32;
214220
// ease_out_cubic
215221
1.0 - (1.0 - p).powi(3)
216222
}
@@ -318,3 +324,87 @@ impl Painter for Chart {
318324
let _ = self.paint(canvas, layout.width, layout.height, ctx.time);
319325
}
320326
}
327+
328+
#[cfg(test)]
329+
mod tests {
330+
use super::*;
331+
use rustmotion_core::traits::TimingConfig;
332+
333+
fn base_chart() -> Chart {
334+
Chart {
335+
chart_type: ChartType::Bar,
336+
data: Vec::new(),
337+
animated: true,
338+
animation_duration: 1.5,
339+
colors: None,
340+
inner_radius: 0.6,
341+
fill_opacity: 0.3,
342+
smooth: false,
343+
categories: Vec::new(),
344+
series: Vec::new(),
345+
axes: Vec::new(),
346+
radar_data: Vec::new(),
347+
points: Vec::new(),
348+
direction: None,
349+
show_grid: false,
350+
show_x_labels: false,
351+
show_y_labels: false,
352+
grid_color: default_grid_color(),
353+
label_color: default_label_color(),
354+
label_font_size: default_label_font_size(),
355+
show_labels: false,
356+
timing: TimingConfig::default(),
357+
style: rustmotion_core::css::CssStyle::default(),
358+
timeline: Vec::new(),
359+
stagger: None,
360+
}
361+
}
362+
363+
#[test]
364+
fn progress_ramp_starts_at_start_at_not_at_scene_time_zero() {
365+
// #3's exact repro: a chart delayed with `start_at: 2.0` and
366+
// `animation_duration: 1.5` was already fully drawn (progress 1.0)
367+
// on the very first frame it became visible, because the ramp read
368+
// raw scene time instead of time-since-`start_at` — the same defect
369+
// `Counter::ramp_progress` was fixed for.
370+
let mut chart = base_chart();
371+
chart.animation_duration = 1.5;
372+
chart.timing = TimingConfig {
373+
start_at: Some(2.0),
374+
end_at: None,
375+
};
376+
377+
assert_eq!(
378+
chart.progress_at(2.0),
379+
0.0,
380+
"no time has elapsed since start_at yet"
381+
);
382+
assert!(
383+
chart.progress_at(2.75) < 1.0,
384+
"still mid-ramp half a second after start_at"
385+
);
386+
assert_eq!(
387+
chart.progress_at(3.5),
388+
1.0,
389+
"animation_duration has fully elapsed since start_at"
390+
);
391+
}
392+
393+
#[test]
394+
fn progress_ramp_with_no_start_at_behaves_like_before() {
395+
let chart = base_chart();
396+
assert_eq!(chart.progress_at(0.0), 0.0);
397+
assert_eq!(chart.progress_at(1.5), 1.0);
398+
}
399+
400+
#[test]
401+
fn progress_ramp_when_not_animated_is_always_complete() {
402+
let mut chart = base_chart();
403+
chart.animated = false;
404+
chart.timing = TimingConfig {
405+
start_at: Some(2.0),
406+
end_at: None,
407+
};
408+
assert_eq!(chart.progress_at(0.0), 1.0);
409+
}
410+
}

crates/rustmotion-components/src/chart/waterfall.rs

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,11 @@ impl Chart {
3535
let max_val = all_vals.iter().fold(f64::MIN, |a, &b| a.max(b));
3636
let range = (max_val - min_val).max(0.001);
3737

38-
let x_labels: Vec<String> = self.data.iter().filter_map(|d| d.label.clone()).collect();
38+
let x_labels: Vec<String> = self
39+
.data
40+
.iter()
41+
.map(|d| d.label.clone().unwrap_or_default())
42+
.collect();
3943
self.draw_axes(
4044
canvas, ml, mt, chart_w, chart_h, min_val, max_val, &x_labels, true,
4145
);

crates/rustmotion-components/src/dot_map.rs

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -133,7 +133,11 @@ impl DotMap {
133133
if !self.animated {
134134
return 1.0;
135135
}
136-
let p = (time / self.animation_duration).clamp(0.0, 1.0) as f32;
136+
// Measure from `start_at`, like every other animated component: driving
137+
// the ramp off raw scene time makes a delayed map arrive already drawn.
138+
let start = self.timing.start_at.unwrap_or(0.0);
139+
let elapsed = (time - start).max(0.0);
140+
let p = (elapsed / self.animation_duration).clamp(0.0, 1.0) as f32;
137141
1.0 - (1.0 - p).powi(3)
138142
}
139143

crates/rustmotion-components/src/gauge.rs

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -95,7 +95,11 @@ impl Gauge {
9595
if !self.animated {
9696
return 1.0;
9797
}
98-
let p = (time / self.animation_duration).clamp(0.0, 1.0) as f32;
98+
// Measure from `start_at`, like every other animated component: driving
99+
// the ramp off raw scene time makes a delayed gauge arrive already full.
100+
let start = self.timing.start_at.unwrap_or(0.0);
101+
let elapsed = (time - start).max(0.0);
102+
let p = (elapsed / self.animation_duration).clamp(0.0, 1.0) as f32;
99103
1.0 - (1.0 - p).powi(3)
100104
}
101105

crates/rustmotion-components/src/heatmap.rs

Lines changed: 137 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -90,11 +90,16 @@ fn interpolate_color(scale: &[String], t: f32) -> (u8, u8, u8) {
9090
return (r, g, b);
9191
}
9292
let n = scale.len() - 1;
93-
let segment = (t * n as f32).floor() as usize;
94-
let local_t = t * n as f32 - segment as f32;
95-
let i = segment.min(n - 1);
96-
let (r1, g1, b1, _) = parse_hex_color(&scale[i]);
97-
let (r2, g2, b2, _) = parse_hex_color(&scale[i + 1]);
93+
let scaled = t * n as f32;
94+
// Clamp the segment index (t=1.0 lands exactly on `n`, one past the
95+
// last valid segment), but re-derive `local_t` from the *clamped*
96+
// segment rather than reusing the unclamped one — otherwise t=1.0
97+
// computed local_t=0.0 against the clamped (second-to-last) segment and
98+
// resolved to the second-to-last color instead of the last one.
99+
let segment = (scaled.floor() as usize).min(n - 1);
100+
let local_t = (scaled - segment as f32).clamp(0.0, 1.0);
101+
let (r1, g1, b1, _) = parse_hex_color(&scale[segment]);
102+
let (r2, g2, b2, _) = parse_hex_color(&scale[segment + 1]);
98103
(
99104
lerp_u8(r1, r2, local_t),
100105
lerp_u8(g1, g2, local_t),
@@ -107,7 +112,13 @@ impl Heatmap {
107112
if !self.animated {
108113
return 1.0;
109114
}
110-
let p = (time / self.animation_duration).clamp(0.0, 1.0) as f32;
115+
// Ramp measured from `start_at`, not from scene time zero — matches
116+
// `Counter::ramp_progress`. A heatmap delayed with `start_at` used
117+
// to read raw scene time, so it was already fully revealed on the
118+
// very first frame it became visible.
119+
let start = self.timing.start_at.unwrap_or(0.0);
120+
let elapsed = (time - start).max(0.0);
121+
let p = (elapsed / self.animation_duration).clamp(0.0, 1.0) as f32;
111122
1.0 - (1.0 - p).powi(3)
112123
}
113124

@@ -121,17 +132,6 @@ impl Heatmap {
121132

122133
let progress = self.progress_at(time);
123134

124-
// Find min/max across all cells
125-
let mut min_val = f64::MAX;
126-
let mut max_val = f64::MIN;
127-
for row in &self.data {
128-
for &val in row {
129-
min_val = min_val.min(val);
130-
max_val = max_val.max(val);
131-
}
132-
}
133-
let range = (max_val - min_val).max(0.001);
134-
135135
// Animation: clip rect expanding from left to right
136136
let clip_w = w * progress;
137137
canvas.save();
@@ -145,7 +145,15 @@ impl Heatmap {
145145

146146
for (row_idx, row) in self.data.iter().enumerate() {
147147
for (col_idx, &val) in row.iter().enumerate() {
148-
let normalized = ((val - min_val) / range) as f32;
148+
// `color_scale` documents an absolute 0.0-1.0 semantic
149+
// (SKILL.md: "2D array of f64, values 0.0-1.0"), not a
150+
// per-render min-max scale. Renormalizing meant a grid of
151+
// constant values (or any subrange, e.g. [0.8, 0.9, 1.0])
152+
// painted identically to a grid of zeros — a flat or
153+
// uniformly-high grid is not the same fact as "nothing
154+
// happened". Clamp into the documented range instead of
155+
// rescaling to whatever the data happens to span.
156+
let normalized = (val as f32).clamp(0.0, 1.0);
149157
let (r, g, b) = interpolate_color(&self.color_scale, normalized);
150158

151159
let x = col_idx as f32 * step;
@@ -179,3 +187,114 @@ impl Painter for Heatmap {
179187
self.paint(canvas, layout.width, layout.height, ctx.time);
180188
}
181189
}
190+
191+
#[cfg(test)]
192+
mod tests {
193+
use super::*;
194+
use rustmotion_core::traits::TimingConfig;
195+
196+
fn base_heatmap(data: Vec<Vec<f64>>) -> Heatmap {
197+
Heatmap {
198+
data,
199+
color_scale: default_color_scale(),
200+
cell_size: default_cell_size(),
201+
cell_gap: default_cell_gap(),
202+
cell_radius: default_cell_radius(),
203+
animated: true,
204+
animation_duration: 1.5,
205+
timing: TimingConfig::default(),
206+
style: CssStyle::default(),
207+
timeline: Vec::new(),
208+
stagger: None,
209+
}
210+
}
211+
212+
fn cell_color(heatmap: &Heatmap, w: i32, h: i32, time: f64) -> (u8, u8, u8) {
213+
let mut surface = skia_safe::surfaces::raster_n32_premul((w, h)).expect("raster surface");
214+
{
215+
let canvas = surface.canvas();
216+
heatmap.paint(canvas, w as f32, h as f32, time);
217+
}
218+
let snapshot = surface.image_snapshot();
219+
let info = skia_safe::ImageInfo::new(
220+
(1, 1),
221+
skia_safe::ColorType::RGBA8888,
222+
skia_safe::AlphaType::Premul,
223+
None,
224+
);
225+
let mut buf = [0u8; 4];
226+
// Sample the middle of the top-left cell.
227+
let x = (heatmap.cell_size / 2.0) as i32;
228+
let y = (heatmap.cell_size / 2.0) as i32;
229+
snapshot.read_pixels(
230+
&info,
231+
&mut buf,
232+
4,
233+
skia_safe::IPoint::new(x, y),
234+
skia_safe::image::CachingHint::Disallow,
235+
);
236+
(buf[0], buf[1], buf[2])
237+
}
238+
239+
#[test]
240+
fn a_uniformly_low_grid_is_not_identical_to_an_all_zero_grid() {
241+
// #6's exact repro: `color_scale` is documented (SKILL.md) as an
242+
// *absolute* 0.0-1.0 scale, but the painter renormalized min→max —
243+
// so a grid of constant 5.0s (or any other constant) rendered
244+
// pixel-for-pixel identical to a grid of constant 0.0s, both
245+
// collapsing to the scale's first (lowest) color.
246+
let uniform = base_heatmap(vec![vec![5.0, 5.0, 5.0], vec![5.0, 5.0, 5.0]]);
247+
let zero = base_heatmap(vec![vec![0.0, 0.0, 0.0], vec![0.0, 0.0, 0.0]]);
248+
let uniform_color = cell_color(&uniform, 200, 100, 10.0);
249+
let zero_color = cell_color(&zero, 200, 100, 10.0);
250+
assert_ne!(
251+
uniform_color, zero_color,
252+
"a grid of 5.0s must not render identically to a grid of 0.0s"
253+
);
254+
}
255+
256+
#[test]
257+
fn absolute_values_are_not_renormalized_to_the_data_subrange() {
258+
// A grid whose values happen to span [0.8, 1.0] must not stretch
259+
// that subrange to fill the whole color scale — 0.8 reads as
260+
// "mostly full", not as "the bottom of whatever this grid contains".
261+
let high = base_heatmap(vec![vec![0.8, 0.9, 1.0]]);
262+
let low = base_heatmap(vec![vec![0.0, 0.1, 0.2]]);
263+
let high_first_cell = cell_color(&high, 200, 100, 10.0);
264+
let low_first_cell = cell_color(&low, 200, 100, 10.0);
265+
assert_ne!(
266+
high_first_cell, low_first_cell,
267+
"0.8 and 0.0 must not render as the same color"
268+
);
269+
}
270+
271+
#[test]
272+
fn interpolate_color_at_the_top_of_the_scale_returns_the_last_color() {
273+
// Surfaced while chasing #6: the clamped segment index was reused
274+
// for `local_t` too, so t=1.0 exactly computed `local_t = 0.0` for
275+
// the *clamped* (second-to-last) segment instead of `local_t = 1.0`
276+
// — landing on the second-to-last color rather than the last
277+
// (brightest) one.
278+
let scale = default_color_scale();
279+
let (r, g, b) = interpolate_color(&scale, 1.0);
280+
let (er, eg, eb, _) = parse_hex_color(scale.last().unwrap());
281+
assert_eq!(
282+
(r, g, b),
283+
(er, eg, eb),
284+
"t=1.0 must resolve to the last color in the scale"
285+
);
286+
}
287+
288+
#[test]
289+
fn progress_ramp_starts_at_start_at_not_at_scene_time_zero() {
290+
let mut heatmap = base_heatmap(vec![vec![1.0]]);
291+
heatmap.animation_duration = 1.5;
292+
heatmap.timing = TimingConfig {
293+
start_at: Some(2.0),
294+
end_at: None,
295+
};
296+
assert_eq!(heatmap.progress_at(2.0), 0.0);
297+
assert!(heatmap.progress_at(2.75) < 1.0);
298+
assert_eq!(heatmap.progress_at(3.5), 1.0);
299+
}
300+
}

0 commit comments

Comments
 (0)