Report ActionClip conflicts instead of panicking and fix field span offset

crates/motiongfx/Cargo.toml

@@ -16,6 +16,7 @@ hashbrown = { workspace = true }
 nonempty = { workspace = true }
 typarena = { workspace = true }
 libm = { workspace = true }
+tracing = { workspace = true, optional = true }
 
 [dev-dependencies]
 criterion = { version = "0.5", features = ["html_reports"] }
@@ -28,5 +29,6 @@ harness = false
 workspace = true
 
 [features]
-default = ["std"]
+default = ["std", "tracing"]
+tracing = ["dep:tracing"]
 std = []

crates/motiongfx/src/sequence.rs

@@ -49,42 +49,86 @@ impl Sequence {
 }
 
 impl Sequence {
+    /// Reports every clip from `first_new` on that starts before the
+    /// clip listed ahead of it, and every earlier clip it overlaps.
+    #[cfg(feature = "tracing")]
+    fn report_conflicts_from(&self, first_new: usize) {
+        // The latest end time among all clips before the new clips.
+        let mut max_end = self
+            .clips
+            .iter()
+            .take(first_new)
+            .map(ActionClip::end)
+            .max()
+            .unwrap_or(Duration::ZERO);
+
+        for (index, clip) in
+            self.clips.iter().enumerate().skip(first_new)
+        {
+            if let Some(prev) = index
+                .checked_sub(1)
+                .and_then(|prev_index| self.clips.get(prev_index))
+                .filter(|prev| clip.start < prev.start)
+            {
+                tracing::error!(
+                    "`ActionClip` {} starts at {:?}, before clip {} at {:?} on the same field",
+                    index,
+                    clip.start,
+                    index - 1,
+                    prev.start,
+                );
+            }
+
+            // No earlier clip reaches this clip, so nothing can
+            // overlap it and the scan is skipped.
+            if clip.start < max_end {
+                for (before_index, before) in
+                    self.clips.iter().enumerate().take(index)
+                {
+                    if clip.start < before.end()
+                        && before.start < clip.end()
+                    {
+                        tracing::error!(
+                            "`ActionClip` {} ({:?}..{:?}) overlaps clip {} ({:?}..{:?}) on the same field",
+                            index,
+                            clip.start,
+                            clip.end(),
+                            before_index,
+                            before.start,
+                            before.end(),
+                        );
+                    }
+                }
+            }
+
+            max_end = max_end.max(clip.end());
+        }
+    }
+
+    /// Appends a clip, reporting any conflict it introduces.
     #[inline]
     pub fn push(&mut self, span: ActionClip) {
-        debug_assert!(
-            span.start >= self.end(),
-            "({:?} >= {:?}) `ActionClip`s shouldn't overlap!",
-            span.start,
-            self.end(),
-        );
-
         self.clips.push(span);
+
+        #[cfg(feature = "tracing")]
+        self.report_conflicts_from(self.clips.len() - 1);
     }
 }
 
 impl Extend<ActionClip> for Sequence {
+    /// Appends clips, reporting any conflict they introduce.
     #[inline]
     fn extend<T: IntoIterator<Item = ActionClip>>(
         &mut self,
         iter: T,
     ) {
-        #[cfg(debug_assertions)]
-        let mut end = self.end();
-        #[cfg(debug_assertions)]
-        let iter = {
-            iter.into_iter().inspect(|clip| {
-                debug_assert!(
-                    clip.start >= end,
-                    "({:?} >= {:?}) `ActionClip`s shouldn't overlap!",
-                    clip.start,
-                    end,
-                );
-
-                end = clip.end();
-            })
-        };
+        #[cfg(feature = "tracing")]
+        let first_new = self.clips.len();
 
         self.clips.extend(iter);
+
+        #[cfg(feature = "tracing")]
+        self.report_conflicts_from(first_new);
     }
 }
 

crates/motiongfx/src/track.rs

@@ -253,7 +253,7 @@ impl TrackFragment {
                 ));
 
                 field = key.field();
-                field_offset = field_len;
+                field_offset += field_len;
                 field_len = 0;
             }
             field_len += 1;
@@ -582,4 +582,61 @@ mod tests {
         assert_eq!(track.duration(), Duration::ZERO);
         assert!(track.sequences_spans().is_empty());
     }
+
+    #[test]
+    fn field_spans_cover_every_lane() {
+        const DUMMY: Sequence = Sequence::new(clip(0));
+
+        let fa = UntypedField::placeholder_with_path("a");
+        let fb = UntypedField::placeholder_with_path("b");
+        let fc = UntypedField::placeholder_with_path("c");
+
+        let mut ids = IdRegistry::new();
+        let s1 = ids.register_instance(DummyId(1));
+        let s2 = ids.register_instance(DummyId(2));
+
+        let k = |sid, field| {
+            ActionKey::new(
+                UntypedSubjectId::new::<DummyId>(sid),
+                field,
+            )
+        };
+
+        let track = TrackFragment::new()
+            .upsert_sequence(k(s1, fa), DUMMY.clone())
+            .upsert_sequence(k(s2, fa), DUMMY.clone())
+            .upsert_sequence(k(s1, fb), DUMMY.clone())
+            .upsert_sequence(k(s1, fc), DUMMY.clone())
+            .compile();
+
+        // Fields in the order `compile` sorts them into.
+        let mut covered = Vec::new();
+
+        for (field, expected) in [(fa, 2), (fb, 1), (fc, 1)] {
+            let spans = track
+                .lookup_field_spans(field)
+                .expect("field was compiled in");
+
+            assert_eq!(spans.len(), expected, "{field:?}");
+            assert!(
+                spans.iter().all(|(key, _)| *key.field() == field),
+                "{field:?} span points at the wrong lanes",
+            );
+
+            covered.extend(spans.iter().map(|(key, _)| *key));
+        }
+
+        // Every lane, once: a drifted offset repeats one and skips
+        // another.
+        let lanes = track
+            .sequences_spans()
+            .iter()
+            .map(|(key, _)| *key)
+            .collect::<Vec<_>>();
+
+        assert_eq!(
+            covered, lanes,
+            "field spans must cover every lane"
+        );
+    }
 }