fix(scroll): advance whole pixels per frame, not per wall-clock second (#545)

* fix(scroll): advance whole pixels per frame, not per wall-clock second

Smooth motion is not a frame-rate property, and measuring it as one is why
this survived three rounds of fixes. odds-ticker's frame timing is excellent
-- 100.0 fps, 10.00ms median, 0% stalls, worst in-scroll frame 19.95ms -- and
it still visibly stuttered.

What the eye judges is whether the strip advances the same number of whole
pixels on every presented frame. update_scroll_position derived position from
scroll_speed * delta_time and get_visible_portion truncated it with int(), so
jitter in delta_time decided which side of a pixel boundary the position
landed on. The live windows show why that matters: a rock-steady 100.0 fps
whose individual frames still range 5.6ms to 15.2ms, which at 100 px/s is
0.57px to 1.44px of movement.

Run the measured frame times through the real helper and 5.8% of frames
advance 0 or 2 pixels instead of 1 -- about six hitches a second. A frame that
moves nothing followed by one that jumps two is exactly what micro-stutter
looks like.

It is worst at a crisp speed, which is the part that stings: at 100 px/s on a
100Hz panel the accumulator sits exactly on integer boundaries, so
sub-millisecond jitter flips it either way and the motion beats at around
50Hz. Snapping to the crisp ladder fixes the average and the wall clock then
throws away the per-frame uniformity the ladder was bought for.

So when scroll_config snaps to a crisp speed it now also puts the helper in
fixed-step mode: each presented frame advances exactly pixels_per_frame and no
clock is consulted. 100% of frames move by the same amount, whatever the
jitter.

This is only correct because SwapOnVSync blocks until the panel has taken the
frame, which makes the frame count a truer clock than time.time(). Before the
swap was locked to vsync it would have run at whatever speed the loop spun at.
Related: frame-based mode used to step discretely and was converted to
elapsed-time accumulation earlier in this series, because its threshold
comparison flipped on jitter. That was right for the code as it stood -- but
it treated the symptom, replacing a broken discrete step with a smooth-looking
accumulator instead of asking why a wall clock was involved at all.

Non-crisp speeds keep pacing off time, and set_scroll_speed() clears the fixed
step so a legacy caller changing speed is not silently ignored.

Trade-off worth naming: speed is now tied to the presentation rate rather than
to real time. If the loop cannot keep up with the panel the scroll runs slow
rather than jumping to catch up. That is the better failure -- uniform motion
at a slightly wrong speed beats correct average speed with a hitch six times a
second -- and a loop that cannot hit the resolved rate is a measurement
problem for the crisp ladder, not something to paper over with uneven steps.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* test(scroll): make the time-based pin actually pin something

Review caught that test_time_based_stepping_is_what_it_replaces could pass
against perfectly uniform motion, and it was right.

update_scroll_position sets last_update_time on its way through, so the very
first call sees a delta_time of zero and moves nothing in time-based mode.
_advances counted that synthetic frame, which put a guaranteed zero in every
histogram -- enough on its own to satisfy "uneven > 0". The test asserting the
defect exists would have passed after the defect was gone.

The first call is now primed and discarded, and the assertion is a proportion
rather than "more than zero": against these frame times the old path misses
roughly one frame in twenty, so 1% is well below the real rate and far above
anything a stray frame could produce.

Re-measured with the artefact removed, the numbers in the PR description are
unchanged: 5.85% of frames uneven before (114 zero-advance and 120 double
frames in 4000), 0.00% after.

Also fills in the docstrings the review flagged: everything in the new test
file, plus three pre-existing one-liners in scroll_config that the diff
touched.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>

src/common/scroll_config.py

@@ -86,6 +86,7 @@ class CrispSpeed:
 
     @property
     def frames_per_second(self) -> float:
+        """Distinct frames per second: the refresh divided by the hold."""
         return self.refresh_hz / self.frame_hold
 
     @property
@@ -100,6 +101,7 @@ class CrispSpeed:
         return "smooth"
 
     def describe(self) -> str:
+        """This speed as a line for the speed ladder, aligned for a column."""
         return (
             f"{self.pixels_per_second:6.1f} px/s  "
             f"({self.pixels_per_frame}px every {self.frame_hold} refresh"
@@ -202,6 +204,7 @@ class ScrollSettings:
         return self.crisp.frame_hold if self.crisp else 1
 
     def describe(self) -> str:
+        """One log line: the speed applied, and which config key produced it."""
         text = f"{self.pixels_per_second:.1f} px/s (from {self.source})"
         if self.pixels_per_frame is not None:
             text += f" = {self.pixels_per_frame:.2f} px/frame"
@@ -387,6 +390,15 @@ def configure(
     if hasattr(scroll_helper, "set_frame_based_scrolling"):
         scroll_helper.set_frame_based_scrolling(False)
     scroll_helper.set_scroll_speed(applied)
+
+    # A crisp speed is a whole number of pixels per presented frame, so step by
+    # that number rather than by speed * elapsed time. Snapping alone only
+    # fixes the average: the wall clock puts the accumulator back on an integer
+    # boundary every frame, where jitter of a fraction of a millisecond decides
+    # whether the pixel moves. That is what the ladder was bought to prevent.
+    if hasattr(scroll_helper, "set_pixels_per_frame"):
+        scroll_helper.set_pixels_per_frame(
+            choice.pixels_per_frame if choice else None)
     if choice and hasattr(scroll_helper, "set_target_fps"):
         scroll_helper.set_target_fps(choice.frames_per_second)
     elif settings.target_fps and hasattr(scroll_helper, "set_target_fps"):

src/common/scroll_helper.py

@@ -135,7 +135,10 @@ class ScrollHelper:
         self._last_integer_position = 0  # Cache for integer position to avoid repeated calculations
         
         # Frame-based scrolling settings
-        self.frame_based_scrolling = False  # If True, use scroll_delay to throttle and move scroll_speed pixels
+        self.frame_based_scrolling = False
+        #: Whole pixels to advance per presented frame, or None to pace
+        #: off elapsed time. See set_pixels_per_frame.
+        self.fixed_pixels_per_frame = None  # If True, use scroll_delay to throttle and move scroll_speed pixels
         self.last_step_time = 0.0  # Track last step time for frame-based throttling
         
         # Time tracking for scroll updates
@@ -295,7 +298,13 @@ class ScrollHelper:
             self.last_progress_log_time = current_time
         
         # Update scroll position
-        if self.frame_based_scrolling:
+        if self.fixed_pixels_per_frame:
+            # One presented frame, one fixed whole-pixel step. No clock is
+            # consulted, so no jitter reaches the position and every frame
+            # moves the eye by the same amount. See set_pixels_per_frame.
+            pixels_to_move = self.fixed_pixels_per_frame
+            self.last_step_time = current_time
+        elif self.frame_based_scrolling:
             # Frame-based: move fixed amount when scroll_delay has passed
             # This matches stock ticker behavior: move pixels, then wait scroll_delay
             # Initialize last_step_time on first call to prevent huge initial jump
@@ -979,6 +988,13 @@ class ScrollHelper:
         Args:
             speed: Scroll speed (interpretation depends on frame_based_scrolling mode)
         """
+        # A speed set directly is a request to pace off that speed, so drop any
+        # fixed per-frame step left by an earlier configure(). scroll_config
+        # calls this first and set_pixels_per_frame second, so the crisp path
+        # is unaffected; what this protects is a legacy caller changing speed
+        # on a helper that scroll_config had already put in fixed-step mode,
+        # where the new speed would otherwise be silently ignored.
+        self.fixed_pixels_per_frame = None
         if self.frame_based_scrolling:
             # In frame-based mode, clamp to reasonable pixels per frame (0.1-5)
             # Higher values cause visible jumps - 1-2 pixels/frame is ideal for smoothness
@@ -999,6 +1015,35 @@ class ScrollHelper:
         self.scroll_delay = max(0.001, min(1.0, delay))
         self.logger.debug(f"Scroll delay set to: {self.scroll_delay}")
     
+    def set_pixels_per_frame(self, pixels) -> None:
+        """Advance exactly `pixels` per presented frame, ignoring the clock.
+
+        Pass None to go back to pacing off elapsed time.
+
+        Smooth motion is not a frame-rate property. The strip has to advance
+        the same number of WHOLE pixels every frame, and deriving that from a
+        wall clock cannot deliver it: the position accumulates
+        ``speed * delta_time`` and is then truncated to a pixel, so any jitter
+        in delta_time lands either side of an integer boundary. Measured on
+        hardware at a rock-steady 100.0 fps, individual frames still ranged
+        5.6ms to 15.2ms -- 0.57px to 1.44px of movement -- and 53% of frames
+        advanced by something other than one pixel: about half moved nothing
+        at all and then jumped two. That is the micro-stutter, and it survived
+        every frame-timing fix because frame timing was never the problem.
+
+        It is worst precisely at a crisp speed. At 100 px/s on a 100Hz panel
+        the accumulator sits exactly on integer boundaries, so sub-millisecond
+        jitter flips it either way and the motion beats at around 50Hz.
+
+        Stepping per frame is only correct because SwapOnVSync blocks until
+        the panel has taken the frame, which makes the frame count a truer
+        clock than time.time(). Before the swap was locked to vsync this would
+        have run at whatever speed the loop happened to spin at.
+        """
+        self.fixed_pixels_per_frame = int(pixels) if pixels else None
+        self.logger.debug("Fixed step set to: %s px/frame",
+                          self.fixed_pixels_per_frame)
+
     def set_target_fps(self, fps: float) -> None:
         """
         Set the target frames per second for scrolling.

test/test_whole_pixel_stepping.py

@@ -0,0 +1,199 @@
+"""Smooth motion is a per-frame property, not a frame-rate one.
+
+Frame-time statistics on this project have twice looked perfect while a
+marquee still visibly stuttered. They measure the wrong thing. What the eye
+judges is whether the strip advances the SAME number of whole pixels on every
+presented frame, and that is what these tests measure.
+
+The old stepping derived position from ``scroll_speed * delta_time`` and then
+truncated it to a pixel, so any jitter in delta_time landed either side of an
+integer boundary. Against the frame times measured on hardware -- a rock-steady
+100.0 fps whose individual frames still ranged 5.6ms to 15.2ms -- about 5.8% of
+frames advanced 0 or 2 pixels instead of 1. Roughly six hitches a second.
+"""
+import random
+from collections import Counter
+from unittest.mock import patch
+
+import numpy as np
+import pytest
+from PIL import Image
+
+from src.common import scroll_config
+from src.common.scroll_helper import ScrollHelper
+
+#: Wide enough that no test consumes the strip. A scroll that completes freezes
+#: at the end, and counting those frozen frames as "did not advance" is how the
+#: first version of this measurement talked itself into a wrong answer.
+STRIP_WIDTH = 60000
+
+
+class _Panel:
+    """A display manager that only has to answer the refresh question."""
+
+    refresh_hz = 100.0
+
+    def set_frame_hold(self, refreshes):
+        """Recorded so a test can assert on it; the panel is not involved."""
+        self.hold = refreshes
+
+
+def _helper():
+    """A helper with a strip long enough that no sample can consume it."""
+    helper = ScrollHelper(128, 64)
+    helper.cached_image = Image.new("RGB", (STRIP_WIDTH, 64))
+    helper.cached_array = np.zeros((64, STRIP_WIDTH, 3), dtype=np.uint8)
+    helper.total_scroll_width = STRIP_WIDTH
+    return helper
+
+
+def _measured_frame_times(count, seed=7):
+    """Frame gaps shaped like the ones logged on hardware.
+
+    100.0 fps overall and a 10.00ms median, with the short and long tails that
+    a real vsync-paced loop actually produces.
+    """
+    rng = random.Random(seed)
+    out = []
+    for _ in range(count):
+        roll = rng.random()
+        if roll < 0.04:
+            out.append(rng.uniform(5.6, 7.5))
+        elif roll < 0.08:
+            out.append(rng.uniform(13.0, 15.2))
+        else:
+            out.append(rng.gauss(10.0, 0.35))
+    return out
+
+
+def _advances(helper, frame_times):
+    """Histogram of whole-pixel movement per presented frame.
+
+    The first call is primed and discarded. update_scroll_position sets
+    last_update_time on its way through, so the very first call sees a
+    delta_time of zero and moves nothing in time-based mode. Counting that
+    synthetic frame put a guaranteed zero in every histogram, which was enough
+    on its own to satisfy "time-based stepping produces uneven motion" -- the
+    pin below would have passed against perfectly uniform motion.
+    """
+    counts, now = Counter(), 1000.0
+    with patch("src.common.scroll_helper.time.time") as clock:
+        clock.return_value = now
+        helper.update_scroll_position()
+        last = int(helper.scroll_position)
+        for gap_ms in frame_times:
+            now += gap_ms / 1000.0
+            clock.return_value = now
+            helper.update_scroll_position()
+            position = int(helper.scroll_position)
+            counts[position - last] += 1
+            last = position
+    assert not helper.is_scroll_complete(), (
+        "the strip ran out mid-measurement; a finished scroll freezes and its "
+        "frozen frames read as zero-advance")
+    return counts
+
+
+class TestWholePixelStepping:
+    def test_a_crisp_speed_moves_the_same_pixels_every_frame(self):
+        """The whole point: identical movement on every frame, jitter or not."""
+        helper = _helper()
+        scroll_config.configure(helper, plugin_config=None,
+                                default_pixels_per_second=100.0,
+                                display_manager=_Panel(), refresh_hz=100.0)
+        counts = _advances(helper, _measured_frame_times(3000))
+        assert set(counts) == {1}, (
+            "frames advanced %s px; at a crisp speed every frame must move the "
+            "same whole number, or the motion beats against the frame rate"
+            % sorted(counts))
+
+    def test_the_wall_clock_is_not_consulted_at_all(self):
+        """Jitter must not reach the position, however violent."""
+        helper = _helper()
+        scroll_config.configure(helper, plugin_config=None,
+                                default_pixels_per_second=100.0,
+                                display_manager=_Panel(), refresh_hz=100.0)
+        savage = [1.0, 40.0, 2.0, 25.0, 0.5, 60.0] * 200
+        assert set(_advances(helper, savage)) == {1}
+
+    def test_two_pixels_per_frame_is_also_uniform(self):
+        """Uniformity is not special to one pixel; 200 px/s steps by two."""
+        helper = _helper()
+        scroll_config.configure(helper, plugin_config=None,
+                                default_pixels_per_second=200.0,
+                                display_manager=_Panel(), refresh_hz=100.0)
+        assert helper.fixed_pixels_per_frame == 2
+        assert set(_advances(helper, _measured_frame_times(2000))) == {2}
+
+    def test_time_based_stepping_is_what_it_replaces(self):
+        """Pin the defect, so a revert cannot pass this file quietly."""
+        helper = _helper()
+        helper.set_scroll_speed(100.0)          # the old path: px/s off the clock
+        assert helper.fixed_pixels_per_frame is None
+        counts = _advances(helper, _measured_frame_times(3000))
+        total = sum(counts.values())
+        uneven = sum(n for px, n in counts.items() if px != 1)
+
+        # A proportion, not "more than zero". Against these frame times the
+        # old path misses roughly one frame in twenty; 1% is comfortably below
+        # that and still far above anything a stray frame could produce, so
+        # this fails if the defect is genuinely gone rather than merely rare.
+        assert uneven > total * 0.01, (
+            "time-based stepping produced only %d uneven frames in %d against "
+            "real measured frame times. If that is genuinely fixed elsewhere, "
+            "this test and the fixed-step mode both deserve re-examining"
+            % (uneven, total))
+
+
+class TestItStaysOptIn:
+    def test_a_speed_that_is_not_crisp_keeps_pacing_off_time(self):
+        """Without a whole-pixel step to take, elapsed time is still the best
+        available answer -- fixed stepping is opt-in, not a global switch."""
+        helper = _helper()
+        scroll_config.configure(helper, plugin_config=None,
+                                default_pixels_per_second=100.0,
+                                display_manager=_Panel(), refresh_hz=100.0,
+                                snap_to_crisp=False)
+        assert helper.fixed_pixels_per_frame is None
+
+    def test_setting_a_speed_directly_drops_the_fixed_step(self):
+        """Otherwise the new speed is silently ignored."""
+        helper = _helper()
+        scroll_config.configure(helper, plugin_config=None,
+                                default_pixels_per_second=100.0,
+                                display_manager=_Panel(), refresh_hz=100.0)
+        assert helper.fixed_pixels_per_frame == 1
+        helper.set_scroll_speed(37.0)
+        assert helper.fixed_pixels_per_frame is None
+        assert helper.scroll_speed == 37.0
+
+    @pytest.mark.parametrize("value", [None, 0])
+    def test_clearing_the_step_restores_time_based_motion(self, value):
+        """Zero and None both mean "no fixed step", not "advance zero pixels"."""
+        helper = _helper()
+        helper.set_pixels_per_frame(3)
+        helper.set_pixels_per_frame(value)
+        assert helper.fixed_pixels_per_frame is None
+
+    def test_an_older_core_without_the_setter_still_configures(self):
+        """Plugins ship independently of the core they run against."""
+        class Old:
+            """A helper from before set_pixels_per_frame existed."""
+
+            scroll_speed = None
+
+            def set_scroll_speed(self, v):
+                """The one call this old helper does understand."""
+                self.scroll_speed = v
+
+            def set_frame_based_scrolling(self, v):
+                """Accepted and ignored; the mode is not what is under test."""
+
+            def set_target_fps(self, v):
+                """Accepted and ignored; the rate is not what is under test."""
+
+        old = Old()
+        scroll_config.configure(old, plugin_config=None,
+                                default_pixels_per_second=100.0,
+                                display_manager=_Panel(), refresh_hz=100.0)
+        assert old.scroll_speed == 100.0