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 <noreply@anthropic.com>
This commit is contained in:
1 parent
992c472975
commit
2ec0fee84c
3 files changed
+153
-41
No files matched your search
+33
-40
@@ -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<State: AndroidAppState> {
|
||||
pub(super) render: UiRenderState,
|
||||
pub(super) state: State,
|
||||
task_recv: TaskMsgReceiver<AndroidRsc<State>>,
|
||||
/// `(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<PointerClock>,
|
||||
}
|
||||
|
||||
impl<State: 'static, I: RscIdx<AndroidRsc<State>>> std::ops::Index<I> for AndroidRsc<State> {
|
||||
@@ -639,17 +638,22 @@ impl<State: AndroidAppState> ViewPeer for IrisViewPeer<State> {
|
||||
// `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<State: AndroidAppState> ViewPeer for IrisViewPeer<State> {
|
||||
// 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<State: AndroidAppState> ViewPeer for IrisViewPeer<State> {
|
||||
// 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<State: AndroidAppState> ViewPeer for IrisViewPeer<State> {
|
||||
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
|
||||
|
||||
@@ -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);
|
||||
|
||||
+48
-1
@@ -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::<Vec<_>>()
|
||||
);
|
||||
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)
|
||||
);
|
||||
}
|
||||
Reference in new issue
Block a user