From 2ec0fee84c1abf5599a3c4bd0ad10381aa45ea84 Mon Sep 17 00:00:00 2001 From: iris <2+iris@noreply.localhost> Date: Mon, 7 Sep 2026 20:36:41 -0400 Subject: [PATCH] iris: the input clock anchors on the first event's oldest sample, not its own time docs/REVIEW-2026-09-07.md's D4. `on_touch_event` took its one anchor as `(Instant::now(), event.event_time_nanos())` from the first MotionEvent the view ever sees, and dated every later sample as `anchor_at + (sample - anchor).max(0)`. An event's historical samples are by definition *older* than its own event_time, so if that first event is a Move -- the Down went to another view, or the view was attached mid-gesture -- its whole batch clamps onto one instant: three samples at the same time make the Lsq2 fit degenerate and the flick reads 0 px/s. In a debug build the ordering debug_assert fired first, and it was comparing against `anchor_nanos`, a value from a different event, so it was also the wrong comparison for the first sample of every later event. The arithmetic moves into `sense::PointerClock`, which anchors at `now - (event_time - oldest_sample)` and carries the last sample seen across events, so `sample()`'s ordering assert compares against the previous event's last sample. It lives in `sense` rather than in the android backend because `iris::android` is cfg'd out everywhere but the device, and this is exactly the arithmetic that wanted a test off one: `the_first_events_batched_samples_are_dated_apart` reports [0ns, 0ns, 0ns] against the old anchoring. The assert stays a `debug_assert!` and now says why in a comment: it runs once per touch sample, hundreds a second on a batching 120Hz screen, and a mis-ordered sample degrades a velocity rather than drawing something wrong. Also drops the stale reference to `VelocityTracker::add_sample` in the comment above it (the review's rule finding); the method is `add_position`. Verified: `cargo test --lib -p iris` and `cargo ndk -t x86_64 -P 29 check -p iris` clean, fmt and clippy clean. Co-Authored-By: Claude Fable 5.1 --- iris/src/android/view.rs | 75 ++++++++++++++++++---------------------- iris/src/sense.rs | 72 ++++++++++++++++++++++++++++++++++++++ iris/src/sense_tests.rs | 49 +++++++++++++++++++++++++- 3 files changed, 154 insertions(+), 42 deletions(-) diff --git a/iris/src/android/view.rs b/iris/src/android/view.rs index ede551d..9ac6397 100644 --- a/iris/src/android/view.rs +++ b/iris/src/android/view.rs @@ -7,7 +7,7 @@ use android_view::{ jni::{ JNIEnv, JavaVM, objects::{GlobalRef, JValue}, - sys::{jint, jlong}, + sys::jint, }, ndk::event::{Axis, Keycode, MotionAction}, }; @@ -20,7 +20,7 @@ use std::{ marker::{PhantomData, Sized}, rc::Rc, sync::Arc, - time::{Duration, Instant}, + time::Instant, }; use super::{ @@ -312,12 +312,11 @@ pub struct IrisViewPeer { pub(super) render: UiRenderState, pub(super) state: State, task_recv: TaskMsgReceiver>, - /// `(an Instant, the input-event nanosecond stamp it was taken at)`, - /// captured from the first `MotionEvent` this view receives and never - /// changed after -- how `on_touch_event` dates every touch sample. Its - /// path out is the peer's own drop: it holds nothing but two numbers - /// and is meaningless to any other view. - input_clock: Option<(Instant, jlong)>, + /// Anchored on the first `MotionEvent` this view receives and never + /// re-anchored after -- how `on_touch_event` dates every touch sample. + /// Its path out is the peer's own drop: it holds nothing but three + /// numbers and is meaningless to any other view. + input_clock: Option, } impl>> std::ops::Index for AndroidRsc { @@ -639,17 +638,22 @@ impl ViewPeer for IrisViewPeer { // `CLOCK_MONOTONIC` an `Instant` reads, so a single // `(Instant, nanos)` pair converts every later sample exactly. // Anchoring **once** rather than per event is what keeps the times - // ordered: a fresh `Instant::now()` per event, minus each sample's - // age inside it, can date a later event's first historical sample - // before the previous event's last one whenever delivery jitters by - // more than the batch spans -- and `VelocityTracker::add_sample`'s - // debug assert would rightly fire on that. See `CursorState::time`. + // ordered, and anchoring on the first event's *oldest* sample + // rather than on its own time is what keeps that event's batch + // from collapsing onto one instant -- `sense::PointerClock`'s doc + // has both, and owns the arithmetic so it can be unit-tested off a + // device (`sense_tests.rs`). See `CursorState::time`. let event_time = event.event_time_nanos(&mut ctx.env); - let (anchor_at, anchor_nanos) = - *self.input_clock.get_or_insert((Instant::now(), event_time)); - let at = |sample_time: jlong| { - anchor_at + Duration::from_nanos(sample_time.saturating_sub(anchor_nanos).max(0) as u64) - }; + if self.input_clock.is_none() { + let history = event.history_size(&mut ctx.env); + let oldest = if history > 0 { + event.historical_event_time_nanos(&mut ctx.env, 0) + } else { + event_time + }; + self.input_clock = Some(PointerClock::anchored(Instant::now(), event_time, oldest)); + } + let mut clock = self.input_clock.expect("anchored just above"); // `iris::input`'s own doc (`sense::log_input_event`): collected // only when tracing is on, since this is otherwise a `Vec` per // `MotionEvent` for a line nobody is reading -- the JNI reads @@ -658,7 +662,6 @@ impl ViewPeer for IrisViewPeer { // function does regardless of tracing. let trace_input = crate::diagnostics::trace_enabled(); let mut historical_ms: Vec<(u64, f32, f32)> = Vec::new(); - let ms_since_anchor = |t: jlong| (t.saturating_sub(anchor_nanos).max(0) as u64) / 1_000_000; // **Historical samples first.** A flick on a 120Hz screen is // delivered as one or two `MotionEvent`s with the intermediate @@ -678,34 +681,30 @@ impl ViewPeer for IrisViewPeer { // the event's own sample as the newest of the batch; everything // downstream (`VelocityTracker`, `DragArbiter`'s long-press // clock) assumes it, so say so here rather than at each reader. - let mut previous = anchor_nanos; + // `PointerClock::sample` is what asserts it, and it carries the + // last sample seen *across* events, so the first sample of + // every event is checked against the previous event's last one + // rather than against the anchor. for pos in 0..history { let hx = event.historical_axis(&mut ctx.env, Axis::X, 0, pos); let hy = event.historical_axis(&mut ctx.env, Axis::Y, 0, pos); let ht = event.historical_event_time_nanos(&mut ctx.env, pos); - debug_assert!( - ht >= previous, - "historical sample {pos} of {history} is dated {ht}ns, before the {previous}ns \ - sample ahead of it -- the input clock is not what this assumes" - ); - previous = ht; + let sample_at = clock.sample(ht); if trace_input { - historical_ms.push((ms_since_anchor(ht), hx, hy)); + historical_ms.push((clock.ms_since_anchor(ht), hx, hy)); } let ui_state = self.state.android_state_mut(); ui_state.cursor.pos = vec2(hx, hy); - ui_state.cursor.time = at(ht); + ui_state.cursor.time = sample_at; self.run_input_frame(ctx); } - debug_assert!( - event_time >= previous, - "the event's own sample is dated {event_time}ns, before its last historical \ - sample at {previous}ns" - ); } + let event_at = clock.sample(event_time); + let event_ms = clock.ms_since_anchor(event_time); + self.input_clock = Some(clock); let ui_state = self.state.android_state_mut(); - ui_state.cursor.time = at(event_time); + ui_state.cursor.time = event_at; match action { MotionAction::Down => { ui_state.cursor.pos = vec2(x, y); @@ -736,13 +735,7 @@ impl ViewPeer for IrisViewPeer { MotionAction::Cancel => "cancel", _ => "other", }; - crate::sense::log_input_event( - action_word, - x, - y, - ms_since_anchor(event_time), - &historical_ms, - ); + crate::sense::log_input_event(action_word, x, y, event_ms, &historical_ms); } self.after_input(ctx); true diff --git a/iris/src/sense.rs b/iris/src/sense.rs index 8fa572f..78df9e5 100644 --- a/iris/src/sense.rs +++ b/iris/src/sense.rs @@ -505,6 +505,78 @@ pub fn log_input_event(action: &str, x: f32, y: f32, t_ms: u64, historical: &[(u ); } +/// Converts a platform's own monotonic input timestamps into [`Instant`]s +/// through **one** anchor taken at the first event, so that every sample +/// this process ever sees is dated on a single ruler. +/// +/// A fresh `Instant::now()` per event, minus each sample's age inside it, +/// can date a later event's first sample before the previous event's last +/// one whenever delivery jitters by more than the batch spans -- which +/// [`VelocityTracker`] would rightly reject. +/// +/// The anchor is taken from the **earliest sample of the first event**, +/// not from that event's own time: an event batches samples that are by +/// definition older than itself, and anchoring on the event's own time +/// leaves them before the anchor, where they clamp onto one instant. Three +/// samples sharing a timestamp make the Lsq2 fit degenerate, so the flick +/// that produced them reads 0 px/s -- reachable whenever the first event a +/// view sees is a `Move` (the `Down` went to another view, or the view was +/// attached mid-gesture). Found by review, 2026-09-07 (docs/REVIEW-2026-09-07.md's D4). +#[derive(Clone, Copy)] +pub struct PointerClock { + anchor_at: Instant, + anchor_nanos: i64, + last_nanos: i64, +} + +impl PointerClock { + /// `now` is when the first event arrived, `event_time` its own + /// timestamp, and `oldest` the timestamp of the earliest sample it + /// carries -- equal to `event_time` when it batches none. + pub fn anchored(now: Instant, event_time: i64, oldest: i64) -> Self { + let batch_span = Duration::from_nanos(event_time.saturating_sub(oldest).max(0) as u64); + Self { + // `checked_sub` rather than `-`: an `Instant` taken very early + // in a process's life has nothing to subtract from, and the + // saturating answer (everything in the first batch at `now`) + // is the old behaviour rather than a panic. + anchor_at: now.checked_sub(batch_span).unwrap_or(now), + anchor_nanos: oldest, + last_nanos: oldest, + } + } + + /// Dates one sample, in the order the platform delivers them. + /// + /// The `debug_assert!` is deliberately not an `assert!`: this runs once + /// per touch sample, which on a 120Hz screen with batching is hundreds + /// a second, and a mis-ordered sample degrades a velocity rather than + /// drawing something wrong (CODE_RULES' hot-loop exception). + pub fn sample(&mut self, nanos: i64) -> Instant { + debug_assert!( + nanos >= self.last_nanos, + "input sample is dated {nanos}ns, before the {}ns sample ahead of it -- the input \ + clock is not what this assumes", + self.last_nanos, + ); + self.last_nanos = self.last_nanos.max(nanos); + self.at(nanos) + } + + /// Dates a sample without treating it as the newest one seen -- for + /// reads that are out of band, such as looking at a batch before + /// replaying it. + pub fn at(&self, nanos: i64) -> Instant { + self.anchor_at + Duration::from_nanos(nanos.saturating_sub(self.anchor_nanos).max(0) as u64) + } + + /// Milliseconds since the anchor, which is the column an `iris::input` + /// line and a `.touch` file both carry (see [`log_input_event`]). + pub fn ms_since_anchor(&self, nanos: i64) -> u64 { + (nanos.saturating_sub(self.anchor_nanos).max(0) as u64) / 1_000_000 + } +} + /// How long a stationary press has to be held before it is treated as a /// long-press rather than the start of a pan. pub const LONG_PRESS: Duration = Duration::from_millis(500); diff --git a/iris/src/sense_tests.rs b/iris/src/sense_tests.rs index 49239f0..172ac28 100644 --- a/iris/src/sense_tests.rs +++ b/iris/src/sense_tests.rs @@ -7,7 +7,7 @@ //! impl need no GPU or window. use crate::prelude::*; -use std::{cell::Cell, rc::Rc}; +use std::{cell::Cell, rc::Rc, time::Instant}; struct SenseRsc { ui: UiData, @@ -317,3 +317,50 @@ fn a_finger_drag_over_a_scroll_area_pans_it() { // widget even once the finger leaves its box. assert_eq!(render.captured_pointer(), Some(scroll.id())); } + +/// docs/REVIEW-2026-09-07.md's D4. The first `MotionEvent` a view sees can +/// be a `Move` -- the `Down` went to another view, or the view was attached +/// mid-gesture -- and its batched samples are older than its own +/// timestamp. Anchoring on that timestamp clamped every one of them onto +/// the anchor, so the tracker saw three samples at one instant, the Lsq2 +/// fit went degenerate, and the flick read 0 px/s. +#[test] +fn the_first_events_batched_samples_are_dated_apart() { + const MS: i64 = 1_000_000; + let now = Instant::now(); + // A 120Hz batch: three historical samples at 0/4/8ms and the event's + // own at 12ms. + let clock = PointerClock::anchored(now, 12 * MS, 0); + + assert_eq!( + clock.at(12 * MS), + now, + "the event's own sample is the one that arrived now" + ); + let batch = [clock.at(0), clock.at(4 * MS), clock.at(8 * MS)]; + assert!( + batch[0] < batch[1] && batch[1] < batch[2] && batch[2] < now, + "the batch must keep the 4ms between its samples, got {:?}", + batch + .iter() + .map(|t| now.duration_since(*t)) + .collect::>() + ); + assert_eq!(clock.ms_since_anchor(8 * MS), 8); +} + +/// The same clock has to keep ordering *across* events: the sample it +/// compares a new event's first sample against is the previous event's +/// last one, never the anchor. +#[test] +fn the_clock_orders_samples_across_events() { + const MS: i64 = 1_000_000; + let mut clock = PointerClock::anchored(Instant::now(), 12 * MS, 0); + let first = clock.sample(12 * MS); + let second = clock.sample(28 * MS); + assert!(second > first); + assert_eq!( + second.duration_since(first), + std::time::Duration::from_millis(16) + ); +}