Add chart_style toggle (singable default) for singable vs score-optimized charts - #128
Conversation
|
@coderabbitai review |
|
Warning Review limit reached
Next review available in: 19 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR introduces a Estimated code review effort: 3 (Moderate) | ~25 minutes ChangesChart Style Resolution
Sequence Diagram(s)sequenceDiagram
participant CLI
participant init_settings
participant Settings
CLI->>init_settings: pass --chart_style / --ptakf_refit / --disable_ptakf_refit
init_settings->>Settings: reset ptakf_refit_explicit
init_settings->>Settings: set chart_style
init_settings->>Settings: mark ptakf_refit_explicit if override flag present
init_settings->>init_settings: validate chart_style (default to singable if invalid)
init_settings->>Settings: set ptakf_refit = (chart_style == score) unless explicit override
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/gui/config.py (1)
119-136: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMigrate legacy
ptakf_refitintochart_styleinsrc/gui/config.py::load_config(). Old GUI configs can still carryptakf_refit, but this path never maps it tochart_style, so they fall back to"singable"and change chart generation on upgrade.🤖 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 `@src/gui/config.py` around lines 119 - 136, Legacy GUI configs still expose ptakf_refit, but load_config() does not translate it into chart_style, so upgraded configs silently keep the default singable behavior. Update src/gui/config.py in load_config() to read the legacy ptakf_refit setting and map it to the new chart_style value during config migration, preserving the intended refit behavior for existing users while keeping the current chart_style-based config structure.README.md (1)
756-764: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winStale "Enabled by default" claim now contradicts the new
chart_styledefault.This section still states
--ptakf_refitis "Enabled by default (disable with--disable_ptakf_refit)" (Line 760), but per this PRptakf_refitis now only on by default when--chart_style scoreis used, andsingable(the new default) leaves it off. This directly conflicts with the new "Chart style" section added right below it (Lines 804-812), which correctly explains the new default issingable(refit off).📝 Proposed fix
-This maximizes the score an exact-match singer (or the extracted vocal track itself) can achieve. Benchmark over 10 songs: Medium score 72.8% → 90.0%, Easy 81.1% → 94.2%. The trade-off is a higher note count (roughly +40%, many short `~` notes). Enabled by default (disable with `--disable_ptakf_refit`). +This maximizes the score an exact-match singer (or the extracted vocal track itself) can achieve. Benchmark over 10 songs: Medium score 72.8% → 90.0%, Easy 81.1% → 94.2%. The trade-off is a higher note count (roughly +40%, many short `~` notes). Enabled automatically when `--chart_style score` is used (see below), or force it on directly with `--ptakf_refit`.🤖 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 `@README.md` around lines 756 - 764, Update the README section for ptAKF Chart Refit so the default behavior matches the new chart_style rules. In the ptAKF Chart Refit paragraph, remove or qualify the blanket “Enabled by default” claim and refer readers to the chart_style default instead; make it clear that ptAKF refit is on by default only for the score style and off for singable. Use the nearby chart_style documentation and the --ptakf_refit / --disable_ptakf_refit wording to keep the description consistent.
🧹 Nitpick comments (2)
pytest/test_UltraSinger.py (1)
84-114: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGood coverage, but missing a regression test for the
chart_stylesingleton-leak scenario.Consider adding a test that calls
init_settingswith--chart_style scorefirst, then calls it again without--chart_styleand assertssettings.chart_style == "singable"andsettings.ptakf_refit is False. This would catch the leak flagged insrc/UltraSinger.py(missingsettings.chart_stylereset alongsidesettings.ptakf_refit_explicit).🤖 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 `@pytest/test_UltraSinger.py` around lines 84 - 114, The test suite is missing a regression case for the Settings singleton leak in init_settings. Add a test in TestChartStyleResolution that first initializes with --chart_style score and then calls init_settings again without chart_style, asserting the second Settings instance resets chart_style to singable and ptakf_refit to False. Use init_settings and Settings to verify the singleton state is cleared properly, including the missing chart_style reset alongside ptakf_refit_explicit.src/Settings.py (1)
62-66: 📐 Maintainability & Code Quality | 🔵 Trivial
ptakf_refitclass default (True) contradicts the newchart_style="singable"default.
chart_styledefaults to"singable", which per the resolution logic ininit_settingsshould yieldptakf_refit=False. But the raw class attribute here still defaults toTrue. Any code path that readssettings.ptakf_refit(or instantiates a freshSettings()) beforeinit_settings()resolves it will see the old, now-incorrect default. Consider aligning the literal default with the new intended behavior to avoid confusion for future consumers.♻️ Proposed fix
- ptakf_refit = True # Resolved from chart_style unless --ptakf_refit/--disable_ptakf_refit is passed + ptakf_refit = False # Resolved from chart_style unless --ptakf_refit/--disable_ptakf_refit is passed🤖 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 `@src/Settings.py` around lines 62 - 66, Align the default for ptakf_refit in Settings with the new chart_style="singable" behavior so a fresh Settings() instance does not expose the old value. Update the class-level default alongside chart_style and keep init_settings as the source of any explicit override resolution; check the Settings attributes ptakf_refit and ptakf_refit_explicit when making the change.
🤖 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.
Inline comments:
In `@src/UltraSinger.py`:
- Around line 798-806: The “Singable chart style” message is gated only by
settings.ptakf_refit, so it can print for score-style runs when refit was
explicitly disabled. Update the conditional around the print in UltraSinger.py
to check the actual chart_style selection as well, using the chart_style logic
that distinguishes "singable" from "score", so the note only appears when the
run is truly in singable mode. Keep the fix local to the block near the existing
settings.ptakf_refit check and preserve the current message text for genuine
singable runs.
- Around line 1936-1937: init_settings() is reusing the settings singleton, and
chart_style is not being reset with the other per-run state, which can leak a
previous run’s value into the next one and affect the ptakf_refit default.
Update the reset block in init_settings() near ptakf_refit_explicit to also
restore settings.chart_style to "singable" so each run starts from the same
baseline.
---
Outside diff comments:
In `@README.md`:
- Around line 756-764: Update the README section for ptAKF Chart Refit so the
default behavior matches the new chart_style rules. In the ptAKF Chart Refit
paragraph, remove or qualify the blanket “Enabled by default” claim and refer
readers to the chart_style default instead; make it clear that ptAKF refit is on
by default only for the score style and off for singable. Use the nearby
chart_style documentation and the --ptakf_refit / --disable_ptakf_refit wording
to keep the description consistent.
In `@src/gui/config.py`:
- Around line 119-136: Legacy GUI configs still expose ptakf_refit, but
load_config() does not translate it into chart_style, so upgraded configs
silently keep the default singable behavior. Update src/gui/config.py in
load_config() to read the legacy ptakf_refit setting and map it to the new
chart_style value during config migration, preserving the intended refit
behavior for existing users while keeping the current chart_style-based config
structure.
---
Nitpick comments:
In `@pytest/test_UltraSinger.py`:
- Around line 84-114: The test suite is missing a regression case for the
Settings singleton leak in init_settings. Add a test in TestChartStyleResolution
that first initializes with --chart_style score and then calls init_settings
again without chart_style, asserting the second Settings instance resets
chart_style to singable and ptakf_refit to False. Use init_settings and Settings
to verify the singleton state is cleared properly, including the missing
chart_style reset alongside ptakf_refit_explicit.
In `@src/Settings.py`:
- Around line 62-66: Align the default for ptakf_refit in Settings with the new
chart_style="singable" behavior so a fresh Settings() instance does not expose
the old value. Update the class-level default alongside chart_style and keep
init_settings as the source of any explicit override resolution; check the
Settings attributes ptakf_refit and ptakf_refit_explicit when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 389a1b72-c8c2-416e-a965-e95823145b06
📒 Files selected for processing (8)
README.mdpytest/test_UltraSinger.pysrc/Settings.pysrc/UltraSinger.pysrc/gui/config.pysrc/gui/settings_tab.pysrc/gui/ultrasinger_runner.pysrc/modules/common_print.py
…align class default, add leak tests
|
All three addressed:
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
Problem
Generated charts scored very high on the in-game ptAKF metric (Medium ~92-98%) but were hard to actually sing: far too many short notes, vibrato traced note-for-note, and noise inserting spurious notes. An 8-song experiment against professional reference charts (SingStar) quantified and root-caused this.
Finding (8 reference songs, measured vs the original .txt)
The ptAKF refit is the cause. It's a score-maximiser that charts the vocal's exact per-beat tone, so every vibrato wiggle becomes its own note. Consistent across all 8 songs: default = 2.0-3.2x too many notes, 60-75% micro-notes (<120ms).
Scoring the professional reference charts themselves is the key context:
The hand-made professional charts only score ~77% Medium against isolated vocals — that is the natural ceiling for a singable chart, because the scorer rewards tracing the vocal exactly. Refit-off matches it almost exactly (77.2 vs 76.8) and reproduces the professional note profile (1.03x note count). The default's 94% reflects over-fitting to the recording, not a better karaoke chart.
Change
Adds a user-facing
chart_stylesetting (defaultsingable):singable: natural, held notes like a professional chart (refit off) - best to sing.score: rebuild onto the game's exact beat tones (refit on) for a higher score number.Surfaces (kept in sync):
--chart_style singable|score; resolves the internal ptAKF refit. The advanced--ptakf_refit/--disable_ptakf_refitflags still override it. Unknown values fall back tosingablewith a notice.scorestyle).Default change
The default output is now the singable chart. This lowers the reported game score for the default run (to professional level) - intended, and explained inline and in the docs. The
scorestyle reproduces the previous behaviour exactly.Tests
6 new resolution tests (default→singable/refit-off, score→refit-on, explicit-flag overrides, unknown fallback) + fixed a singleton-state leak in
init_settings. Full suite 857 passed / 4 skipped; GUI suite green; end-to-end verified (default 253 notes vsscore612 on the same song).Summary by CodeRabbit
New Features
Bug Fixes