refactor(#299): スペクトラム平滑化をリフレッシュレート非依存化#304
Open
GeneralD wants to merge 4 commits into
Open
Conversation
cava の平滑化定数は 66fps 基準 (framerate_mod = 66/fps) だが、移植時に 66/60 固定だったため 120Hz ProMotion / 可変リフレッシュでは CADisplayLink の tick が倍の頻度で発火し、バーの落下が2倍速にズレていた。#298 レビュー 指摘の HW 依存 2 件目 (課題2)。 - DisplayLinkDriver が CADisplayLink の実フレーム間隔 (targetTimestamp - timestamp) を onFrame ファンアウト経由で SpectrumPresenter.tick(frameInterval:) まで伝搬。 - 純粋関数 spectrumFramerateConstants(frameInterval:) が framerateMod / integralMod / gravityScale をクランプ済み (24…240fps) レートから毎フレーム導出 → 落下速度・積分減衰・autosens が実時間で どのリフレッシュレートでも同じ速さに収束。 - tick() は frameInterval を 1/60 デフォルトにし、60Hz 挙動は完全不変、 タイミング非依存の呼び出し元/テストも無改修。 - ベースの「詰め」定数 (fallIncrement・66 基準・autosens ステップ) は 据え置き。物理を正すと 120Hz で従来補正していた見た目が変わるため、 実機 120Hz での再チューニングは別途 (フレームワークのみ本 PR)。 導出の純粋関数 (60fps一致・120fps半減・クランプ) と、リフレッシュレート 非依存の落下 (120Hz は 60Hz より多フレームで消える) をユニットテスト検証。 Refs #299
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughThis PR propagates real per-frame timing intervals from DisplayLinkDriver through AppRouter into SpectrumPresenter.tick, replacing fixed 60fps smoothing constants with interval-derived, clamped framerate compensation constants. Tests, documentation, and version file are updated accordingly. ChangesRefresh-rate-independent spectrum smoothing
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant DisplayLinkDriver
participant AppRouter
participant SpectrumPresenter
DisplayLinkDriver->>DisplayLinkDriver: tick(_:) computes targetTimestamp - timestamp
DisplayLinkDriver->>AppRouter: onFrame(interval)
AppRouter->>SpectrumPresenter: tick(frameInterval: interval)
SpectrumPresenter->>SpectrumPresenter: spectrumFramerateConstants(frameInterval:)
SpectrumPresenter->>SpectrumPresenter: stepped(...) using gravityScale/integralMod
SpectrumPresenter->>SpectrumPresenter: adjustSens(framerateMod:)
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors the Spectrum smoothing pipeline to be refresh-rate independent by propagating the real CADisplayLink frame interval through the frame fan-out and deriving cava-style smoothing constants per frame. This aligns Spectrum fall/decay/autosens behavior to wall-clock time across 60 Hz, 120 Hz ProMotion, and variable refresh displays while keeping the default 60 Hz behavior unchanged for timing-agnostic callers/tests.
Changes:
- Propagate
frameInterval(targetTimestamp - timestamp) fromDisplayLinkDriver→AppRouterframe fan-out →SpectrumPresenter.tick(frameInterval:). - Introduce
spectrumFramerateConstants(frameInterval:)and use its derived constants to scale gravity/integral/autosens per frame (with 24…240 fps clamp). - Add/adjust tests and docs; bump version to
2.20.2.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| Tests/PresentersTests/SpectrumPresenterTests.swift | Adds unit tests for framerate-derived constants and refresh-rate-independent fall behavior. |
| Tests/AppRouterTests/AppLaunchEnvironmentTests.swift | Updates frame scheduler test scaffolding for the new onFrame(Double) signature. |
| Sources/Views/Shared/DisplayLinkDriver.swift | Emits real per-frame interval to consumers via onFrame(frameInterval:). |
| Sources/Presenters/Spectrum/SpectrumPresenter.swift | Adds tick(frameInterval:) and per-frame framerate compensation constants + pure derivation function. |
| Sources/AppRouter/AppRouter.swift | Updates frame fan-out plumbing to pass frameInterval to enabled handlers (Spectrum). |
| Sources/VersionHandler/Resources/version.txt | Version bump to 2.20.2. |
| .claude/CLAUDE.md | Documents the refresh-rate-independent smoothing design and intent (#299). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+206
to
+211
| func spectrumFramerateConstants(frameInterval: Double) -> SpectrumFramerateConstants { | ||
| let fps = min(max(1 / max(frameInterval, .leastNormalMagnitude), 24), 240) | ||
| let mod = Float(66 / fps) | ||
| return SpectrumFramerateConstants( | ||
| framerateMod: mod, integralMod: pow(mod, 0.1), gravityScale: pow(mod, 2.5) * 2) | ||
| } |
Owner
Author
There was a problem hiding this comment.
isFinite ガードを追加し、NaN / ±∞ のフレーム間隔は 60fps 基準にフォールバックするようにしました（min/max が NaN を素通りして framerateMod を汚染する問題を）。ゼロ/負の有限値は従来どおり 24…240fps クランプで処理します。非有限入力の回帰テストも追加しました。4558984
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
NaN / ±∞ のフレームタイムスタンプは min/max を素通りして framerateMod を NaN 汚染し、以降のスムージング定数を壊す。isFinite で弾いて 60fps 基準に フォールバックさせる。ゼロ/負の有限値は従来どおり 24…240fps クランプで処理。 Copilot 指摘への対応。非有限入力の回帰テストを追加。
frameHandlers内のspectrumPresenter.tick(frameInterval:)呼び出しが 未カバーだったため、既存のripple版テストに倣いFixtureSpectrumInteractor を追加してカバレッジの穴を埋める。
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Tests/AppRouterTests/AppLaunchEnvironmentTests.swift (1)
78-86: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winNew spectrum-tick test doesn't exercise
tick(frameInterval:).
FixtureSpectrumInteractor.isCapturingreturns a permanently-empty publisher (Line 81), socapturingonSpectrumPresenternever becomestrue. Sincemotionstarts empty,tick's guard (capturing || !motion.isEmpty) short-circuits before any of the new frame-interval logic runs. The test's only assertions —isEnabled == trueandstartCallCount == 1(Lines 513, 517) — are already true beforedriver.fire(frameInterval:)is called, so this test doesn't actually validate that the propagated interval reachesSpectrumPresenter.tick(frameInterval:)or produces any effect, despite the PR's stated goal of covering theAppRouterspectrum tick path.♻️ Suggested fix: make capturing controllable and assert on tick effects
private struct FixtureSpectrumInteractor: SpectrumInteractor { let spectrumStyle: SpectrumStyle + let capturingSubject = CurrentValueSubject<Bool, Never>(false) - var isCapturing: AnyPublisher<Bool, Never> { Empty().eraseToAnyPublisher() } + var isCapturing: AnyPublisher<Bool, Never> { capturingSubject.eraseToAnyPublisher() } func start() {} func stop() {} func magnitudes(barCount: Int) -> [Float] { Array(repeating: 0.5, count: barCount) } }+ let spectrumInteractor = FixtureSpectrumInteractor(spectrumStyle: SpectrumStyle(enabled: true)) - dependencies.spectrumInteractor = FixtureSpectrumInteractor(spectrumStyle: SpectrumStyle(enabled: true)) + dependencies.spectrumInteractor = spectrumInteractor ... let spectrumPresenter: SpectrumPresenter? = value(named: "spectrumPresenter", from: router) `#expect`(spectrumPresenter?.isEnabled == true) + spectrumInteractor.capturingSubject.send(true) driver.fire(frameInterval: 1.0 / 60.0) + `#expect`(spectrumPresenter?.isAnimating == true) `#expect`(driver.startCallCount == 1)Also applies to: 486-519
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Tests/AppRouterTests/AppLaunchEnvironmentTests.swift` around lines 78 - 86, The new spectrum-tick test currently never drives SpectrumPresenter.tick(frameInterval:) because FixtureSpectrumInteractor.isCapturing always stays empty, so capturing never becomes true and the tick guard short-circuits. Make FixtureSpectrumInteractor controllable (or otherwise emit a capturing value) so the test can actually exercise the frameInterval path through AppRouter and SpectrumPresenter. Then assert on a tick-specific effect from driver.fire(frameInterval:) rather than only on pre-existing state like isEnabled and startCallCount, using the SpectrumPresenter and driver.fire hooks to prove the interval is propagated.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@Tests/AppRouterTests/AppLaunchEnvironmentTests.swift`:
- Around line 78-86: The new spectrum-tick test currently never drives
SpectrumPresenter.tick(frameInterval:) because
FixtureSpectrumInteractor.isCapturing always stays empty, so capturing never
becomes true and the tick guard short-circuits. Make FixtureSpectrumInteractor
controllable (or otherwise emit a capturing value) so the test can actually
exercise the frameInterval path through AppRouter and SpectrumPresenter. Then
assert on a tick-specific effect from driver.fire(frameInterval:) rather than
only on pre-existing state like isEnabled and startCallCount, using the
SpectrumPresenter and driver.fire hooks to prove the interval is propagated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 47326dbb-52ad-4324-ad19-5faed5006def
📒 Files selected for processing (7)
.claude/CLAUDE.mdSources/AppRouter/AppRouter.swiftSources/Presenters/Spectrum/SpectrumPresenter.swiftSources/VersionHandler/Resources/version.txtSources/Views/Shared/DisplayLinkDriver.swiftTests/AppRouterTests/AppLaunchEnvironmentTests.swiftTests/PresentersTests/SpectrumPresenterTests.swift
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.
概要
#299 のうち 課題2（平滑化定数が 60fps 前提） に対応する。cava の平滑化定数は 66fps 基準（
framerate_mod = 66 / fps）だが、移植時に66 / 60固定だったため、CADisplayLink駆動のtick()が 120Hz ProMotion / 可変リフレッシュでは倍の頻度で発火し、バーの落下が 2倍速 にズレていた。実フレーム間隔を平滑化まで伝搬させて非依存化する。変更内容
DisplayLinkDriver:CADisplayLinkの実フレーム間隔（targetTimestamp - timestamp）をonFrameファンアウト経由でSpectrumPresenter.tick(frameInterval:)まで伝搬。spectrumFramerateConstants(frameInterval:):framerateMod/integralMod/gravityScaleをクランプ済み（24…240fps）レートから毎フレーム導出 → 落下速度・積分減衰・autosens が実時間でどのリフレッシュレートでも同じ速さに収束。tick(frameInterval: Double = 1.0/60.0): デフォルト 1/60 で 60Hz 挙動は完全不変、タイミング非依存の呼び出し元/テストも無改修。背景・動機
#298 のレビュー（CodeRabbit / Copilot）で挙がった HW 依存 2 件のうち 2 件目。1 件目（サンプルレート伝搬）は #303。
現在の見た目は実機 120Hz で
66/60固定のまま詰めたものです。本 PR で物理的に正すと、120Hz での落下が正しく（従来比で遅く）なるため、体感が変わります。ベースの「詰め」定数（fallIncrement・66 基準・autosens ステップ）は据え置きにしてあり、実機 120Hz での最終チューニングは別途お願いします（このフレームワーク導入後に定数だけ調整可能）。テスト計画
spectrumFramerateConstants純粋関数: 60fps で従来値一致 / 120fps で半減 / 絶対値・負値・ゼロのクランプ / 導出フィールドの関係SpectrumPresenter: リフレッシュレート非依存の落下（120Hz は 60Hz より多フレームで消える）AppRouter/DisplayLinkDriverのonFrameシグネチャ変更に追従make lintクリーン備考
2.20.2に更新。Refs #299, #298
Summary by CodeRabbit
New Features
Bug Fixes
Chores