Skip to content

live: cap animation-canvas DPR at 1.5 and redraw at ~60fps#1737

Open
ArcanConsulting wants to merge 1 commit into
Kpa-clawbot:masterfrom
ArcanConsulting:perf/anim-dpr-fps-cap
Open

live: cap animation-canvas DPR at 1.5 and redraw at ~60fps#1737
ArcanConsulting wants to merge 1 commit into
Kpa-clawbot:masterfrom
ArcanConsulting:perf/anim-dpr-fps-cap

Conversation

@ArcanConsulting

Copy link
Copy Markdown

The live-map animation overlay re-clears and re-draws its full backing store
every animation frame. Two unbounded multipliers make that expensive:

  1. devicePixelRatio is uncapped in updateAnimCanvas(). The canvas is already
    ~1.4x the screen area (20% pad per side), so at DPR 2-3 it allocates and
    fills 5-12x the screen's pixels per frame. Cap at 1.5 — lines stay crisp,
    per-frame fill cost drops up to ~4x on hi-DPI displays.

  2. renderAnimations() reschedules via rAF with no rate limit, so on 120/144Hz
    displays it does 2-2.4x the work for no visible gain. Add a ~60fps guard.
    Progress is time-based (tickDt, itself capped at 32ms), so skipping frames
    preserves motion exactly. Paused frames fall through to the existing sleep.

No behavior change on a standard 60Hz / 1x-DPI display. Existing animation
tests (test-live-dt-cap-1524, test-live-anims) unaffected.

The live-map animation overlay re-clears and re-draws its full backing store
every animation frame. Two unbounded multipliers make that expensive:

1. devicePixelRatio is uncapped in updateAnimCanvas(). The canvas is already
   ~1.4x the screen area (20% pad per side), so at DPR 2-3 it allocates and
   fills 5-12x the screen's pixels per frame. Cap at 1.5 — lines stay crisp,
   per-frame fill cost drops up to ~4x on hi-DPI displays.

2. renderAnimations() reschedules via rAF with no rate limit, so on 120/144Hz
   displays it does 2-2.4x the work for no visible gain. Add a ~60fps guard.
   Progress is time-based (tickDt, itself capped at 32ms), so skipping frames
   preserves motion exactly. Paused frames fall through to the existing sleep.

No behavior change on a standard 60Hz / 1x-DPI display. Existing animation
tests (test-live-dt-cap-1524, test-live-anims) unaffected.
@Kpa-clawbot

Copy link
Copy Markdown
Owner

Polish review — focused (tufte + carmack lens on a 20-line perf tweak)

3-axis status: mergeable=MERGEABLE · mergeStateStatus=BLOCKED · CI=action_required (first-time contributor; the CI/CD Pipeline workflow is waiting on operator approval before any check runs). No tests touched.

Verdict: LGTM with one nit + one defensive ask

The reasoning in the PR body matches the code. Capping DPR at 1.5 and throttling redraws to ~60fps are both low-risk wins on hi-DPI / high-refresh displays, and the comments inline (public/live.js:1298-1302, 3866-3873) explain why clearly enough that future readers won't undo them by accident. The isPaused carve-out is correct — paused frames still need to paint state changes (selection, pulses already in flight), and they're cheap because tickDt short-circuits motion.

MINOR (non-blocking)

  1. _lastAnimFrame is updated only on the rendered branch (live.js:3873). The early-return branches at :3850-3854 (no active anims) and :3868 (throttled) intentionally don't update it. The empty-list early-return is fine. The throttled-skip path is also fine because the next rAF will compare against the same baseline and either fire or skip again — that's the intended hysteresis. No change needed, just calling it out so a future reader doesn't "fix" it.

  2. High-refresh skip math. ANIM_MIN_FRAME_MS = 15 targets ~66fps, not exactly 60fps. On a 120Hz display (~8.33ms/frame), this means: frame 1 fires, frame 2 (~16ms) clears the gate, frame 3 (~25ms) skips, frame 4 (~33ms) fires — so you get ~60fps with a jitter of one rAF tick. That's fine and matches the comment ("don't redraw faster than ~60fps"). On a 144Hz display (~6.94ms/frame), you'll get a slightly less even cadence (~57-60fps). Acceptable — requestAnimationFrame was always going to alias against refresh.

NIT

  • Comment placement, live.js:1298-1302: the dpr cap comment is indented one level deeper than the assignment it documents (4 vs 5 spaces in the surrounding block). Cosmetic; matches existing inconsistent indent in updateAnimCanvas() anyway.

Test coverage

No tests in this PR. Acceptable for a perf-only tweak that doesn't change rendered output (DPR<1.5 displays see no change; DPR>1.5 see lower resolution which is a deliberate trade). A CDP-based "canvas backing-store width matches Math.min(dpr, 1.5)*w" assertion would be the only useful test, and that's overkill for 20 lines of guard-rail config. Per project AGENTS.md TDD exemption: pure-perf changes that don't alter behavior assertions are exempt.

CDP verification

Skipping live-browser verification — the PR is a measurable-by-instrumentation change (frame count, backing-store dimensions), and CI hasn't approved-to-run yet so there's no staging artifact to point a browser at. Once operator approves the workflow run and staging deploys, a 30-second chrome://gpu / DevTools Performance comparison would close the loop.

Operator action required

This PR is safe to merge once CI is approved-and-green, but auto-merge is blocked by the first-time contributor gate (action_required on the workflow run). Operator needs to click Approve and run workflows on the PR page before any auto-merge gate can evaluate.

— mc-bot watcher v2 · 2026-06-16 20:48Z

@Kpa-clawbot

Copy link
Copy Markdown
Owner

🤖 Hourly PR-watch review

Verdict: APPROVE-WITH-CONCERNS — CI blocked by #1747, perf change unverified

Two perf caps on the live animation canvas: DPR capped at 1.5 + per-frame redraw throttle at ~60fps via _lastAnimFrame. Reasoning in the code comments is sound — backing-store fill cost on hi-DPI screens scales quadratically with DPR, and on 120/144Hz panels rAF will otherwise issue 2-2.4x the necessary frames for time-based motion.

Diff review

  • Math.min(window.devicePixelRatio || 1, 1.5): correct guard, won't downscale below the device ratio when DPR < 1.5.
  • Frame skip gate gracefully falls through for isPaused — preserves the existing sleep path. ✅
  • tickDt is already 32ms-capped per-object, so a 15ms render skip cannot drop motion. ✅

Concerns

  • MINOR: _lastAnimFrame initialized to 0, so the first real frame after load passes the gate trivially (correct).
  • MINOR: No before/after measurement attached. A claim like "cuts per-frame fill cost by ~4x" deserves a Chrome DevTools Performance trace screenshot (DPR before/after, frame time histogram). Without it the operator has to trust the math.
  • MINOR: No test. Justifiable: this is a perf knob with no behavioral surface to assert against. AGENTS.md exempts pure perf changes when no behavioral assertion can be written.

CI status

Playwright failure is the cross-PR fixture-aging issue tracked in #1747, not this change.

Findings: 0 BLOCKER / 0 MAJOR / 3 MINOR — community author, leaving as comment-only per watcher policy.

Reviewed by: carmack (perf surface) + tufte (visual fidelity).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants