address review: grid fallback stays on snap positions and bars; thumbnail positions in f64

crates/ui-egui/src/panels/midi_editor.rs

@@ -282,16 +282,23 @@ fn draw_roll_background(painter: &egui::Painter, g: &Roll<'_>) {
 const MIN_GRID_PX: f32 = 4.0;
 
 /// Ticks between grid lines: the grid when its lines are more than [`MIN_GRID_PX`] apart, else
-/// bars, else as many bars as keep that spacing. The lines drawn are then bounded by the roll's
-/// width, however long the clip, instead of by its length.
+/// as few common multiples of the grid and a bar as keep that spacing (so every line is a snap
+/// position and falls on a bar). The lines drawn are then bounded by the roll's width, however long
+/// the clip, instead of by its length.
 fn grid_stride(grid_ticks: i64, px_per_tick: f32) -> i64 {
     let bar_ticks = TICKS_PER_QUARTER * 4;
     if grid_ticks as f32 * px_per_tick > MIN_GRID_PX {
         return grid_ticks.max(1);
     }
-    // `as` saturates a huge or NaN quotient; a zero `px_per_tick` gives one bar.
-    let bars = (MIN_GRID_PX / (bar_ticks as f32 * px_per_tick)).ceil().max(1.0) as i64;
-    bar_ticks.saturating_mul(bars)
+    let grid = grid_ticks.max(1);
+    let (mut a, mut b) = (grid, bar_ticks);
+    while b != 0 {
+        (a, b) = (b, a % b);
+    }
+    let common = (grid / a).saturating_mul(bar_ticks);
+    // `as` saturates a huge or NaN quotient; a zero `px_per_tick` gives one common multiple.
+    let n = (MIN_GRID_PX / (common as f32 * px_per_tick)).ceil().max(1.0) as i64;
+    common.saturating_mul(n)
 }
 
 /// The notes, with a drag in progress applied.
@@ -503,6 +510,11 @@ mod tests {
         let step = grid_stride(sixteenth, px_per_tick);
         assert_eq!(step % (TICKS_PER_QUARTER * 4), 0, "lines fall on bars");
         assert!((len / step) <= 251, "{} lines", len / step);
+        // a dotted quarter grid (1440 ticks): every line is also a grid position
+        let dotted = TICKS_PER_QUARTER * 3 / 2;
+        let step = grid_stride(dotted, px_per_tick);
+        assert_eq!((step % dotted, step % (TICKS_PER_QUARTER * 4)), (0, 0), "lines are snap positions on bars: {step}");
+        assert!((len / step) <= 251, "{} lines", len / step);
         assert!(grid_stride(0, 0.0) >= TICKS_PER_QUARTER * 4, "a zero grid or width still advances");
     }
 

crates/ui-egui/src/panels/video_window.rs

@@ -119,6 +119,16 @@ fn first_movie_aspect(s: &soundcraft_model::Session) -> Option<f32> {
 /// widest strip holds.
 const MAX_UNIVERSE_THUMBS: usize = 1 << 12;
 
+/// Left edges of the thumbnails of a clip starting at `clip_x` (each `tw` wide) that lie in
+/// `shown_min..shown_max`. The index and position are worked out in `f64`: an `f32` this far from
+/// the origin (a clip starting a billion points left) cannot step by a thumbnail's width.
+fn thumb_xs(clip_x: f32, shown_min: f32, shown_max: f32, tw: f32) -> impl Iterator<Item = f32> {
+    let (origin, tw) = (f64::from(clip_x), f64::from(tw));
+    let first = ((f64::from(shown_min) - origin) / tw).floor().max(0.0);
+    let count = ((f64::from(shown_max) - origin) / tw - first).ceil().max(0.0) as usize;
+    (0..count.min(MAX_UNIVERSE_THUMBS)).map(move |i| (origin + (first + i as f64) * tw) as f32)
+}
+
 fn universe(app: &mut SoundApp, ctx: &egui::Context) {
     app.video.poll(ctx);
     let s = app.engine.session_arc();
@@ -152,10 +162,7 @@ fn universe(app: &mut SoundApp, ctx: &egui::Context) {
             // Only the thumbnails inside the strip, counted rather than stepped: a clip starting
             // far left of it has an edge so large that adding a thumbnail's width wouldn't move it.
             let shown = cr.intersect(r);
-            let first = ((shown.min.x - cr.min.x) / tw).floor().max(0.0);
-            let count = ((shown.max.x - cr.min.x) / tw - first).ceil().max(0.0) as usize;
-            for i in 0..count.min(MAX_UNIVERSE_THUMBS) {
-                let x = cr.min.x + (first + i as f32) * tw;
+            for x in thumb_xs(cr.min.x, shown.min.x, shown.max.x, tw) {
                 let secs = movie_secs(&s, c.start, offset, strip_position(x, r.min.x, r.width(), end, c.start)).max(0.0);
                 if let Some(tex) = app.video.thumb(ctx, key, &v.path, secs, hpx) {
                     let tr = Rect::from_min_size(pos2(x, cr.min.y), vec2(tw, cr.height()));
@@ -190,7 +197,18 @@ fn universe(app: &mut SoundApp, ctx: &egui::Context) {
 
 #[cfg(test)]
 mod tests {
-    use super::strip_position;
+    use super::{strip_position, thumb_xs};
+
+    #[test]
+    fn thumbnails_of_a_clip_far_left_of_the_strip_are_evenly_spaced() {
+        // 2^30 points left of the strip with 8 pt thumbnails: f32 steps of `first + i` here are 16 apart
+        let xs: Vec<f32> = thumb_xs(-1_073_741_824.0, 0.0, 80.0, 8.0).collect();
+        assert_eq!(xs.len(), 10);
+        assert!(xs.windows(2).all(|w| w[1] - w[0] == 8.0), "{xs:?}");
+        assert_eq!(xs[0], 0.0);
+        // a clip inside the strip starts at its own edge
+        assert_eq!(thumb_xs(10.0, 10.0, 34.0, 8.0).collect::<Vec<_>>(), vec![10.0, 18.0, 26.0]);
+    }
 
     // A 1024 px strip over 1_024_000 samples: 1000 samples per point, exact in f32.
     const LEFT: f32 = 100.0;