Repository navigation
fix: score against the chart's effective tuning and capo - #86
JohanStolk wants to merge 1 commit into
Conversation
_syncChartStateFromHw() took the tuning and capo from getSongInfo(), which the host keeps as the ORIGINAL metadata. A chart-transform provider (core #952) can re-fret the chart and change its tuning to match, e.g. a Drop D part shown as D-standard shapes; scoring those frets against the original tuning expected the wrong pitch on every re-fretted string. Prefer the highway's effective getTuning() and getCapo() (what the highway shows), falling back to songInfo on older hosts or when they return nothing usable. Without a transform the two are identical, so nothing changes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Johan Stolk <johan.stolk@tutamail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Description Check |  |
Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | |
The title clearly and concisely describes the main change: scoring now uses the chart's effective tuning and capo. |
| Docstring Coverage | |
No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1… |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
Full details: Docstring Coverage
-
Explanation
No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1 files. (1 skipped: 1 too large.)
✨ Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving 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 @coderabbitai help to get the list of available commands.
|
@coderabbitai review |
✅ Action performedReview finished.
|
_syncChartStateFromHw()took the tuning and capo fromgetSongInfo(). The host keepsgetSongInfo()as the original song metadata (feedBack#952: "getSongInfo()retains original metadata; effective metadata is available through …getTuning(),getCapo()"), whilegetNotes()/getChords()return the chart after any chart-transform provider ran.So when a transform re-frets the chart and changes the tuning to match, note detection combined transformed frets with the original tuning and expected the wrong pitch. Example: a Drop D part re-fretted as D-standard shapes (low string unchanged, the other five strings +2 frets, tuning reported as
[-2,-2,-2,-2,-2,-2]). Scored against the original[-2,0,0,0,0,0], every note on strings 2–6 is expected 2 semitones high, far outside the pitch tolerance, so correctly played notes would be scored as misses.Change: prefer the highway's effective
getTuning()/getCapo(), falling back tosongInfoon older hosts (no getters), or when the getter returns nothing usable (empty array, throws). The string-count fallbacks use the same tuning. Without a transform the effective values equal the original ones, so nothing changes for normal play.Tests (
test/chart_state_sync.test.js, 3 new):npm test: 356 pass, 0 fail. Also verified end to end in the FeedBack web app with a chart-transform provider that re-frets a Drop D chart: note detection's chart state reports the transformed[-2,-2,-2,-2,-2,-2].🤖 Generated with Claude Code
Summary by CodeRabbit