Fix lost LRCLIB linebreaks and cap the visible line length - #135
Conversation
Converted songs ended up with almost no linebreaks: entire verses were rendered as one screen line, compressing every note to an unreadable sliver and pushing the lyrics off-screen. Root cause: _trim_word_to_voiced() rebuilt the word dict from scratch and silently dropped the line_end (and backing) flags. CTC alignment stretches precisely the LAST word of each LRC line into the following pause, so almost every line-end word went through trimming - which destroyed nearly all LRCLIB linebreaks (one affected song kept 10 of ~48; another kept none). Trimming now preserves all extra keys. Safety net: the writer now enforces a maximum visible line length (45 characters). Overlong lines - LRCLIB lines that are genuinely too long, or silence-fallback songs without enough distinct gaps - are split at the largest pause on a word boundary (never inside a melisma run); when no pause stands out, the split closest to the middle wins. The linebreak decision was refactored out of the streaming writer loop into _compute_linebreak_indices() to make this possible; the previous LRCLIB-flag and silence-percentile behavior is ported unchanged. Verified on a previously affected song: 10 -> 69 breaks across 355 notes, longest screen line down from 368 to 45 visible characters, with 48 LRC linebreaks now surviving the pipeline (12 new unit tests).
|
Warning Review limit reached
Next review available in: 51 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 (2)
📝 WalkthroughWalkthroughThe changes preserve metadata when trimming reference lyric words and refactor UltraStar linebreak handling into precomputed helpers with silence, LRCLIB, and maximum-length rules. New tests cover metadata retention, split selection, melisma handling, nested splits, and existing break indices. ChangesReference lyrics metadata
UltraStar linebreak calculation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant create_ultrastar_txt
participant _compute_linebreak_indices
participant _enforce_max_line_length
participant UltraStarOutput
create_ultrastar_txt->>_compute_linebreak_indices: Pass MIDI segments and silence threshold
_compute_linebreak_indices->>_enforce_max_line_length: Enforce maximum lyric line length
_enforce_max_line_length-->>_compute_linebreak_indices: Return additional break indices
_compute_linebreak_indices-->>create_ultrastar_txt: Return complete break set
create_ultrastar_txt->>UltraStarOutput: Emit notes and LINEBREAK tags
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 review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/modules/Ultrastar/ultrastar_writer.py`:
- Around line 203-272: Update the visible character calculations in
_enforce_max_line_length, including visible_chars and the candidate-scan
left_chars accumulation, to count the rendered concatenated text rather than
adding a space for every segment; preserve spaces only where
midi_segments[k].word actually includes its trailing boundary space, so melisma
continuations are not overcounted. Add a regression test covering exact rendered
lengths for lines containing continuation segments.
🪄 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: 5a4095cf-680a-44aa-8102-24184ebad6e2
📒 Files selected for processing (4)
pytest/modules/Speech_Recognition/test_reference_lyrics_aligner.pypytest/modules/UltraStar/test_ultrastar_writer.pysrc/modules/Speech_Recognition/reference_lyrics_aligner.pysrc/modules/Ultrastar/ultrastar_writer.py
Syllable and continuation notes carry no trailing space and attach directly to the previous note in the rendered line - counting a separator per note overcounted such lines and could force-split lines whose true rendered length was within the limit.
1 participant
Problem
Converted songs ended up with almost no linebreaks: entire verses rendered as ONE screen line in the game. That compresses every note to an unreadable sliver (note width shrinks with the line's total duration) and makes the lyric line overflow the screen. A recently converted song had 10 breaks across 321 notes, with the longest screen line at 368 visible characters; the standard test song had zero LRC linebreaks surviving.
Root cause
_trim_word_to_voiced()(reference-lyrics pipeline, step 2b) rebuilt the word dict from scratch:This silently dropped the
line_end(andbacking) flags. CTC alignment stretches precisely the last word of each LRC line into the following pause - so almost every line-end word went through trimming, destroying nearly all LRCLIB linebreaks before they reached the writer. Trimming now preserves all extra keys ({**word, ...}), with a regression test.Safety net: maximum visible line length
Even with correct flags, overlong lines can still happen (genuinely long LRC lines, or silence-fallback songs whose gaps are too uniform). The writer now enforces a cap of 45 visible characters per line:
To make this possible, the linebreak decision moved out of the streaming writer loop into
_compute_linebreak_indices(). The previous behavior (LRCLIB flags preferred, silence-percentile fallback including the separated-word workaround) is ported unchanged and covered by tests.Verification
Re-converted a previously affected song end-to-end with the fix:
test_ultrastar_writer.py/test_reference_lyrics_aligner.py(librosa lazy-loader environment issue) are unrelated and unchanged.Summary by CodeRabbit
Bug Fixes
Improvements