Merge pull request #387 from itrycodinganditsbad/fix-250

Keep later meter changes on their bars when an earlier meter is edited

crates/engine/src/cmd/event.rs

@@ -402,6 +402,22 @@ mod tests {
         assert!((s.tempo.tempo_at_tick(tick) - 133.5).abs() < 1e-9);
     }
 
+    /// Changing the initial meter keeps a later meter change at its bar (issue #250).
+    #[test]
+    fn earlier_meter_change_keeps_later_meter_on_its_bar() {
+        use soundcraft_time::BarBeat;
+        let mut e = crate::Engine::default();
+        assert_eq!(e.session().sample_rate.hz(), 48_000);
+        e.execute("event.meter", &json!({"numerator": 3, "denominator": 4, "at": 192_000})).unwrap();
+        e.execute("event.meter", &json!({"numerator": 6, "denominator": 4, "at": 0})).unwrap();
+        let map = &e.session().tempo;
+        let later = map.meters().get(1).copied().unwrap();
+        assert_eq!((later.numerator, later.denominator), (3, 4));
+        assert_eq!(map.bar_beat_at_tick(later.tick), BarBeat { bar: 3, beat: 1, tick: 0 });
+        let bar2_beat3 = BarBeat { bar: 2, beat: 3, tick: 0 };
+        assert_eq!(map.bar_beat_at_tick(map.tick_at_bar_beat(bar2_beat3)), bar2_beat3);
+    }
+
     fn marker_name(e: &crate::Engine, id: u64) -> String {
         e.session().markers.iter().find(|m| m.id.0 == id).map(|m| m.name.clone()).unwrap_or_default()
     }

crates/time/src/tempo.rs

@@ -96,13 +96,31 @@ impl TempoMap {
     }
 
     /// Insert or replace the meter at `tick` (snapped to the bar line at or before it).
+    /// Later meter changes keep their bar numbers, so they stay on bar lines.
     pub fn set_meter(&mut self, tick: i64, numerator: u32, denominator: u32) -> Result<(), TimeError> {
         if !(1..=64).contains(&numerator) || ![1, 2, 4, 8, 16, 32, 64].contains(&denominator) {
             return Err(TimeError::Meter(numerator, denominator));
         }
         let tick = if tick <= 0 { 0 } else { self.bar_start_tick(self.bar_beat_at_tick(tick).bar) };
-        let ev = MeterEvent { tick, numerator, denominator };
-        match self.meters.binary_search_by(|e| e.tick.cmp(&tick)) {
+        let later = self.take_meters_after(tick);
+        self.put_meter(MeterEvent { tick, numerator, denominator });
+        self.restore_meters_at_bars(later);
+        Ok(())
+    }
+
+    /// Remove a meter event (the one at tick 0 is kept). Later meter changes keep their bar numbers.
+    pub fn remove_meter(&mut self, tick: i64) -> bool {
+        if tick == 0 || !self.meters.iter().any(|e| e.tick == tick) {
+            return false;
+        }
+        let later = self.take_meters_after(tick);
+        self.meters.retain(|e| e.tick != tick);
+        self.restore_meters_at_bars(later);
+        true
+    }
+
+    fn put_meter(&mut self, ev: MeterEvent) {
+        match self.meters.binary_search_by(|e| e.tick.cmp(&ev.tick)) {
             Ok(i) => {
                 if let Some(e) = self.meters.get_mut(i) {
                     *e = ev;
@@ -110,16 +128,21 @@ impl TempoMap {
             }
             Err(i) => self.meters.insert(i, ev),
         }
-        Ok(())
     }
 
-    pub fn remove_meter(&mut self, tick: i64) -> bool {
-        if tick == 0 {
-            return false;
+    /// Remove the meters after `tick`, returning each with the bar it starts.
+    fn take_meters_after(&mut self, tick: i64) -> Vec<(i64, MeterEvent)> {
+        let later: Vec<_> = self.meters.iter().filter(|e| e.tick > tick).map(|e| (self.bar_beat_at_tick(e.tick).bar, *e)).collect();
+        self.meters.retain(|e| e.tick <= tick);
+        later
+    }
+
+    /// Put meters back at the start of their bars under the current earlier meters, in bar order.
+    fn restore_meters_at_bars(&mut self, later: Vec<(i64, MeterEvent)>) {
+        for (bar, ev) in later {
+            let tick = self.bar_start_tick(bar);
+            self.put_meter(MeterEvent { tick, ..ev });
         }
-        let before = self.meters.len();
-        self.meters.retain(|e| e.tick != tick);
-        before != self.meters.len()
     }
 
     /// Tempo in effect at `tick`.

crates/time/tests/meter_grid_contract.rs

@@ -0,0 +1,72 @@
+//! Meter changes stay on bar lines when an earlier meter is edited (issue #250).
+
+use soundcraft_time::{BarBeat, MeterEvent, SampleRate, TempoMap};
+
+fn assert_positions_round_trip(map: &TempoMap) {
+    for bar in 1..=5 {
+        let meter = map.meter_at_tick(map.bar_start_tick(bar));
+        for beat in 1..=i64::from(meter.numerator) {
+            let position = BarBeat { bar, beat, tick: 0 };
+            assert_eq!(map.bar_beat_at_tick(map.tick_at_bar_beat(position)), position);
+        }
+    }
+}
+
+fn meter_bars(map: &TempoMap) -> Vec<(i64, u32, u32)> {
+    map.meters()
+        .iter()
+        .map(|m| {
+            let bb = map.bar_beat_at_tick(m.tick);
+            assert_eq!((bb.beat, bb.tick), (1, 0), "meter at tick {} is off the bar line", m.tick);
+            (bb.bar, m.numerator, m.denominator)
+        })
+        .collect()
+}
+
+#[test]
+fn earlier_meter_edit_preserves_bar_positions() {
+    let mut map = TempoMap::default();
+    let at = map.samples_to_ticks(192_000, SampleRate::HZ_48000);
+    assert_eq!(at, 7680);
+    map.set_meter(at, 3, 4).unwrap();
+    assert_positions_round_trip(&map);
+    map.set_meter(0, 6, 4).unwrap();
+    assert_positions_round_trip(&map);
+    // The 3/4 change stays at bar 3, now two 6/4 bars in.
+    assert_eq!(meter_bars(&map), vec![(1, 6, 4), (3, 3, 4)]);
+    assert_eq!(map.meters().get(1).map(|m| m.tick), Some(2 * 5760));
+}
+
+#[test]
+fn inserting_and_removing_meters_keeps_later_bars() {
+    let mut map = TempoMap::default();
+    map.set_meter(map.bar_start_tick(5), 5, 4).unwrap();
+    map.set_meter(map.bar_start_tick(3), 3, 4).unwrap();
+    assert_eq!(meter_bars(&map), vec![(1, 4, 4), (3, 3, 4), (5, 5, 4)]);
+    // Inserting 7/8 at bar 2 keeps 3/4 at bar 3 and 5/4 at bar 5.
+    map.set_meter(map.bar_start_tick(2), 7, 8).unwrap();
+    assert_eq!(meter_bars(&map), vec![(1, 4, 4), (2, 7, 8), (3, 3, 4), (5, 5, 4)]);
+    assert_positions_round_trip(&map);
+    // Removing 3/4 leaves 5/4 at bar 5.
+    let three_four = map.meters().iter().find(|m| m.numerator == 3).map(|m| m.tick).unwrap();
+    assert!(map.remove_meter(three_four));
+    assert_eq!(meter_bars(&map), vec![(1, 4, 4), (2, 7, 8), (5, 5, 4)]);
+    assert_positions_round_trip(&map);
+}
+
+#[test]
+fn initial_meter_first_and_restoring_controls() {
+    // Setting the initial meter first, then the later change at the same raw tick, snaps to a bar line.
+    let mut map = TempoMap::default();
+    map.set_meter(0, 6, 4).unwrap();
+    map.set_meter(7680, 3, 4).unwrap();
+    assert_eq!(map.meters().get(1), Some(&MeterEvent { tick: 5760, numerator: 3, denominator: 4 }));
+    assert_positions_round_trip(&map);
+    // Restoring 4/4 after the edit puts the 3/4 change back where it started.
+    let mut map = TempoMap::default();
+    map.set_meter(7680, 3, 4).unwrap();
+    map.set_meter(0, 6, 4).unwrap();
+    map.set_meter(0, 4, 4).unwrap();
+    assert_eq!(map.meters().get(1), Some(&MeterEvent { tick: 7680, numerator: 3, denominator: 4 }));
+    assert_positions_round_trip(&map);
+}