feat(scroll): Drive scroll motion from a uniform frame clock - #23987
Open
MartinZikmund wants to merge 8 commits into
Open
feat(scroll): Drive scroll motion from a uniform frame clock#23987MartinZikmund wants to merge 8 commits into
MartinZikmund wants to merge 8 commits into
Conversation
Damage was accumulated by unioning SKPaths, so a frame that moves a large subtree paid a path union per visual. A scroll moves everything in the viewport at once, which is the worst case for that shape. Rects are now appended as contours to one builder under the nonzero fill rule, and collapse to their bounding rect past a small count -- past which the clip mask costs more than the pixels it saves. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011w88LWryCBS83AzcTezBUM
Manipulation deltas were held back until they crossed a significance threshold, so a slow drag advanced the content in 2px steps instead of following the finger, and wheel deltas were truncated to whole detents by an int division. Scrolling asks for every delta as it arrives; the threshold exists to suppress accidental micro-manipulations, which is not what a scroll presenter wants. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011w88LWryCBS83AzcTezBUM
…vers Frames present one per vsync, but ticks are not scheduled on one, so a raw clock read wobbles by milliseconds around a cadence that is otherwise exact. A driver whose position is a function of time turns that wobble into v*dt of position error -- at scroll speeds, a visible fraction of a frame step, and worse as refresh rate rises, since the error is v*dt regardless of rate while the step itself halves. Drivers now evaluate against a phase-locked grid recovered from the median tick interval: whole-frame steps for dropped frames and idle gaps, a gentle pull otherwise so no single frame carries a visible correction, and monotone by construction -- a backward step would make a fling's elapsed time negative, which its curve reads as "not started". It lives on the CompositionTarget rather than the process-wide Compositor: several record loops sharing one clock push near-zero intervals into its window and can collapse the estimated period, silently degrading it to the raw clock it replaces. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011w88LWryCBS83AzcTezBUM
…cord A driver writing from inside the record produces a frame request the render state machine cannot tell apart from "content changed since the last record", so it arms a present for a picture that tick will not produce and the previous one is shown again. Raising the hook from the tick, before UpdateLayout, makes a driver's write an ordinary pre-frame invalidation -- the same shape a pointer event produces -- and lets the same tick clean up the layout it dirties. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011w88LWryCBS83AzcTezBUM
A Draw acquiring the frame gate between the publish and the count read the fresh picture against the stale generation and recorded a dropped frame that never happened. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011w88LWryCBS83AzcTezBUM
Both wheel and touch motion restarted an ease-out towards a fixed target on every input. An ease front-loads its motion, so each restart re-injects a large first step, and since input arrives asynchronously to the frame clock that step lands on an arbitrary frame -- the presented displacement then alternates between very large and very small. The wheel now uses exponential decay, which is memoryless, so a detent composes with whatever motion is in flight: velocity stays continuous and only its slope changes. Touch inertia uses the platform's own curve -- Android's spline distance, Apple's drag constant -- launched from a least-squares fit over the recent gesture rather than a two-sample difference, whose divisor is whatever interval those two samples happened to have. Both are analytic in absolute time and evaluated once per frame against the frame clock, so a late or early frame produces the correct position instead of accumulating error. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011w88LWryCBS83AzcTezBUM
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011w88LWryCBS83AzcTezBUM
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011w88LWryCBS83AzcTezBUM
github-actions
Bot
added
area/skia ✏️
Contributor
|
Claude finished @MartinZikmund's task in 10m 50s —— View job Code ReviewPR #23987 — feat(scroll): Drive scroll motion from a uniform frame clock
Overall verdictfix-first — 1 medium issue with a targeted fix, 1 low-severity efficiency issue, and 2 pre-existing items worth cleaning up while the file is open. Issues found: 2 (inline) + 2 pre-existing (not in diff)Medium —
Low — In a Pre-existing issues (not in this diff, worth noting)
What's working well
|
Contributor
|
Tick the box to add this pull request to the merge queue (same as
|
Contributor
There was a problem hiding this comment.
Pull request overview
Implements a Skia-specific “single frame timestamp” discipline for scroll-related motion, routing wheel/touch inertia through a uniform frame clock so scroll position is evaluated once per frame (pre-layout / pre-record) instead of being driven by event arrival timing.
Changes:
- Add a Skia
FrameClockand aCompositionTarget.FrameStartingpre-layout hook to provide a single per-frame timestamp to motion drivers. - Rework ScrollViewer wheel + touch inertia to be time-parameterized (decay/fling simulations, unquantized deltas, fractional wheel detents).
- Add runtime tests plus a manual benchmark sample and a design spec documenting the rationale and constraints.
Reviewed changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/Uno.UI/UI/Xaml/Media/CompositionTarget.Rendering.skia.cs | Moves frame-record accounting under the publish gate to avoid false dropped-frame reporting. |
| src/Uno.UI/UI/Xaml/Media/CompositionTarget.FrameClock.skia.cs | Introduces FrameStarting event + per-target frame clock integration for Skia. |
| src/Uno.UI/UI/Xaml/Internal/InputManager.Pointers.Managed.cs | Sends fractional wheel “detents” (double) to trackers to avoid precision touchpad dead-zone. |
| src/Uno.UI/UI/Xaml/Internal/DirectManipulation.cs | Enables unquantized deltas for direct manipulation (scrolling) gesture recognizer. |
| src/Uno.UI/UI/Xaml/Internal/CoreServices.cs | Raises FrameStarting pre-layout (Skia) to tick motion drivers before recording. |
| src/Uno.UI/UI/Xaml/Controls/ScrollContentPresenter/ScrollFlingSimulation.cs | Adds closed-form fling simulation (Android/iOS parity) for touch inertia. |
| src/Uno.UI/UI/Xaml/Controls/ScrollContentPresenter/ScrollDecaySimulation.cs | Adds exponential decay simulation for wheel motion with impulse accumulation. |
| src/Uno.UI/UI/Xaml/Controls/ScrollContentPresenter/ScrollContentPresenter.Managed.cs | Wires wheel decay + touch fling to FrameStarting, adds velocity fitting, and updates animation frame handling. |
| src/Uno.UI/UI/Xaml/Controls/ScrollContentPresenter/ScrollContentPresenter.cs | Switches wheel path to feed the accumulating decay via AddWheelImpulse. |
| src/Uno.UI/UI/Input/WinRT/ScrollVelocityTracker.cs | Adds least-squares velocity estimation over recent samples for stable inertia launch velocity. |
| src/Uno.UI/UI/Input/WinRT/GestureRecognizer.Manipulation.InertiaProcessor.cs | Uses FrameStarting (Skia) to tick inertia pre-record instead of CompositionTarget.Rendering. |
| src/Uno.UI/UI/Input/WinRT/GestureRecognizer.Manipulation.cs | Ensures manipulation deltas can be committed even when below thresholds for scrolling. |
| src/Uno.UI/UI/Input/WinRT/GestureRecognizer.cs | Adds ReportsUnquantizedDeltas init flag to control delta threshold behavior for scroll. |
| src/Uno.UI.RuntimeTests/Tests/Windows_UI_Xaml_Controls/Given_ScrollViewer.cs | Updates/adds scroll behavior tests for wheel monotonicity, fling proportionality, and unquantized slow-drag. |
| src/Uno.UI.RuntimeTests/Tests/Windows_UI_Composition/Given_Compositor.cs | Adds frame clock behavior tests (jitter smoothing, drift, drops, reanchor, monotonicity). |
| src/Uno.UI.Composition/Composition/InteractionTracker/InteractionTrackerState.cs | Updates wheel delta API to accept double ticks. |
| src/Uno.UI.Composition/Composition/InteractionTracker/InteractionTrackerInteractingState.cs | Signature update for wheel delta (double). |
| src/Uno.UI.Composition/Composition/InteractionTracker/InteractionTrackerInertiaState.cs | Uses double wheel delta and casts to float vectors for inertia handling. |
| src/Uno.UI.Composition/Composition/InteractionTracker/InteractionTrackerIdleState.cs | Uses double wheel delta and adjusts velocity computation accordingly. |
| src/Uno.UI.Composition/Composition/InteractionTracker/InteractionTrackerCustomAnimationState.cs | Signature update for wheel delta (double). |
| src/Uno.UI.Composition/Composition/InteractionTracker/InteractionTracker.cs | Documents and implements fractional wheel detents end-to-end (double). |
| src/Uno.UI.Composition/Composition/FrameClock.skia.cs | New frame clock implementation based on median tick intervals and monotone correction. |
| src/Uno.UI.Composition/Composition/Compositor.skia.cs | Counts “frame drivers” as animating work so WaitForIdle(...waitForCompositionAnimations:true) covers them. |
| src/SamplesApp/SamplesApp.Samples/Windows_UI_Xaml_Controls/ScrollViewerTests/ScrollSmoothnessBenchmark.xaml.cs | Adds a manual benchmark to report per-frame offset-delta jitter statistics while scrolling. |
| src/SamplesApp/SamplesApp.Samples/Windows_UI_Xaml_Controls/ScrollViewerTests/ScrollSmoothnessBenchmark.xaml | UI for the scroll smoothness benchmark sample. |
| specs/scroll-smoothness/spec.md | Adds design spec capturing goals, invariants, research references, and staged plan. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+631
to
+640
| var compositor = Visual.Compositor; | ||
| var now = compositor.TimestampInTicks; | ||
|
|
||
| if (!_isWheelDecayRunning) | ||
| { | ||
| _wheelDecayH.Start(HorizontalOffset, now); | ||
| _wheelDecayV.Start(VerticalOffset, now); | ||
| _isWheelDecayRunning = true; | ||
| FrameDriverTarget!.FrameStarting += OnWheelDecayFrame; | ||
| } |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| if (_handler is not null) | ||
| { | ||
| CompositionTarget.Rendering -= _handler; | ||
| if (Microsoft.UI.Xaml.Media.CompositionTarget.MainFrameDriverTarget is { } target) |
Contributor
There was a problem hiding this comment.
Stop() re-fetches MainFrameDriverTarget instead of using the same target instance captured in Start(). In a multi-window scenario, or if the window closes between Start and Stop, MainFrameDriverTarget could return null or a different CompositionTarget than the one the handler was subscribed to — leaving the subscription alive and _handler set to null so it can never be unsubscribed again. The finalizer (~CompositionInertiaProcessorTimer()) inherits the same flaw.
The fix is to store the target captured during Start as a field and use it in Stop:
private Microsoft.UI.Xaml.Media.CompositionTarget? _target;
public void Start()
{
Stop();
if (Microsoft.UI.Xaml.Media.CompositionTarget.MainFrameDriverTarget is not { } target)
return;
_target = target;
_startTimestamp = 0;
_handler = (_, timestamp) => { … };
target.FrameStarting += _handler;
}
public void Stop()
{
if (_handler is not null)
{
_target?.FrameStarting -= _handler; // same instance that was subscribed
_target = null;
_handler = null;
}
}
|
|
||
| // Done once both curves are spent, or the content has run into an edge in the direction of travel. | ||
| var running = elapsed < Math.Max(_flingH.Duration, _flingV.Duration) | ||
| && (h > 0 && h < maxH || v > 0 && v < maxV); |
Contributor
There was a problem hiding this comment.
The running condition can keep the fling active for extra no-op frames in a bidirectional-scrollable ScrollViewer. If the fling is purely vertical but HorizontalOffset is mid-range (0 < h < maxH), then h > 0 && h < maxH remains true even after the vertical motion settles at an edge. The fling loop continues issuing Set calls that change nothing, until elapsed >= _flingV.Duration.
The frame-clock requests and resulting wakeups waste CPU while the content is stationary. The simplest fix is to also stop when neither axis changed this frame:
var previousH = HorizontalOffset;
var previousV = VerticalOffset;
// … Set call …
var running = elapsed < Math.Max(_flingH.Duration, _flingV.Duration)
&& (HorizontalOffset != previousH || VerticalOffset != previousV
|| (h > 0 && h < maxH) || (v > 0 && v < maxV));Or conservatively: check that at least one axis is still between its limits and hasn't been pinned there for two consecutive frames.
Xiaoy312
approved these changes
Aug 10, 2026
MartinZikmund
force-pushed
the
dev/mazi/scroll-damage
branch
2 times, most recently
from
August 14, 2026 18:24
80cfa2b to
68659d3
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
GitHub Issue: closes #23985
Important
This is PR 2 of 2 in a stack. It targets
dev/mazi/scroll-damage(PR 1), notfeature/breakingchanges— review that one first. The diff shown here is only this layer's changes. Once PR 1 merges, this retargets automatically.PR Type:
✨ Feature
What changed? 🚀
Current behavior. Scroll motion is produced by six independent sources, none of which agree on a clock, and two input paths quantize motion before it reaches the visual:
v·dtof position error — at scroll speeds a visible fraction of a frame step, and worse as refresh rate rises, since the error staysv·dtwhile the step halves.ManipulationDeltaevents, but on the scroll path it acts as a motion quantizer: content does not move until 2 px accumulate, then jumps by the whole amount, so a slow drag advances every other frame. Inertia already bypasses it for the same reason.This PR establishes two invariants:
Render()entry; every animation and simulation in that frame is evaluated at it. A newFrameClockrecovers a phase-locked grid from the median tick interval: whole-frame steps for dropped frames and idle gaps, a gentle pull otherwise so no single frame carries a visible correction, and monotone by construction — a backward step would make a fling's elapsed time negative, which its curve reads as "not started". It lives on theCompositionTargetrather than the process-wideCompositor, because several record loops sharing one clock push near-zero intervals into its window and collapse the estimated period.ScrollFlingSimulation,ScrollDecaySimulation,ScrollVelocityTracker). Impulses accumulate into the running simulation rather than restarting it, removing the per-detent hitch.Also included: frame drivers tick before layout rather than inside the record; a recorded frame is counted under the gate that publishes it (a
Drawacquiring the gate in between read a fresh picture against a stale generation and logged a dropped frame that never happened).Explicitly not adopted (each would be a silent parity deviation or a regression): pointer resampling as the headline fix — Flutter ships it off and it costs ~38 ms of latency; pointer prediction/extrapolation — absent from dxaml, causes overshoot; a
CubicBezieron default programmatic scroll — WinUI supplies no easing function.Design rationale and the full research trail are in
specs/scroll-smoothness/spec.md, added here.PR Checklist ✅
Given_Compositor(frame clock) andGiven_ScrollViewer(scroll motion), plus aScrollSmoothnessBenchmarksample for manual frame-pacing comparison.specs/scroll-smoothness/spec.mdcarries the design. No public API surface changed.Screenshots Compare Test Runresults.Validation
Uno.UI.Skia,Uno.UI.RuntimeTests.Skia,SamplesApp.Skia.Generic(net10.0) — all 0 warnings, 0 errors.ScrollSmoothnessBenchmarksample is the intended reviewer-facing check — it reports frame pacing directly, which a pass/fail test cannot capture.