Require a vocal note for hasVocals - #126
Open
elicwhite wants to merge 1 commit into
Open
Conversation
`notesData.hasVocals` was derived from `notePhrases.length > 0`. A phrase is
only a range marker, though: lyrics live inside phrases, and a phrase need not
contain any vocal note. Every chart that ships lyrics therefore reported
`hasVocals: true`, which raised a spurious
Metadata is missing a "diff_vocals" value.
on charts that have no vocals to declare a difficulty for. The `.chart` format
makes this unconditional — its parser hardcodes an empty vocal note list, so
`.chart` can never express a vocal note, yet every lyric-bearing `.chart`
claimed to have vocals.
Derive `hasVocals` from the presence of a vocal note inside a phrase instead,
keeping the flag state-derived.
This cuts both ways, and the tests pin both directions. A lyrics-only chart no
longer asks for a `diff_vocals` value; a lyrics-only chart that *sets*
`diff_vocals` now reports it as an extra value, where previously it reported
nothing.
`hasLyrics` is unchanged. The `noNotes` check keeps its own broader test — any
vocal phrase, lyrics-only included, is still enough content to keep a chart out
of `noNotes` — so it is renamed `hasVocalContent` to mark the difference as
deliberate.
elicwhite
force-pushed
the
fix/has-vocals-requires-vocal-notes
branch
from
August 21, 2026 14:50
f57f184 to
072fce3
Compare
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.
The bug
notesData.hasVocalsis derived fromnotePhrases.length > 0inscan-parsed-chart.ts:A
notePhraseis only a range marker (MIDI note 105/106). Lyrics live inside it asphrase.lyrics, and a phrase need not contain any vocal note. So any chart that ships lyrics reportshasVocals: true, which trips the check inscan-chart.ts:on a chart that has no vocals and no vocals difficulty to declare.
For the
.chartformat this is unconditional.chart-file-parser.tshardcodesnotes: []for the vocals part, so a.chartcan never express a vocal note — yet every lyric-bearing.chartclaims to have vocals.Why it regressed
7be766d moved the flag from
ParsedCharttoScannedChartso it would stay accurate when consumers mutate chart data post-parse. That goal is sound and is preserved here — but the replacement predicate counts phrase markers as vocals.For reference, the two earlier implementations both keyed on notes rather than phrases: bef6a54 tested for a
PART VOCALSevent that was not a 105/106 phrase marker, and the version immediately before 7be766d tested for a pitched note in 36–84. This PR is not a restoration of either — it counts vocal percussion, and it considers harmony parts — but it returns to the principle that a phrase marker alone is not vocals.The fix
Derive
hasVocalsfrom the presence of a vocal note inside a phrase, keeping the flag state-derived.hasLyricsis unchanged.The
noNotescheck has its own, deliberately broader test: any vocal phrase — lyrics-only included — is still enough content to keep a chart out ofnoNotes. Behavior there is unchanged; the local is renamedhasVocalContentso the difference fromnotesData.hasVocalsreads as intentional rather than an oversight.The trade this makes
Worth being explicit, since it is user-visible in both directions:
diff_vocalsin iniThe second row is a new issue on charts that previously reported nothing. I believe it is the correct signal — there are no vocals for that value to describe — but since the old behavior effectively encouraged setting
diff_vocalson lyrics-only charts, some existing charts will move from one issue to the other rather than to clean. Both rows are pinned by tests. Happy to suppress theextraValueside too if you would rather this be a pure reduction in noise.Known limitation
hasVocalsreadsphrase.notes, which is the playable projection rather than every note in the source:groupIntoPhrasesdrops hidden percussion (MIDI 97), drops a percussion note sitting at its phrase's own start tick when it is the first note, and requirestick >= phrase.tickso notes ahead of their phrase marker are excluded. A vocals chart built only from such notes will reporthasVocals: false. These are pre-existing parser behaviors, not introduced here, and I left them alone to keep this PR narrow — but they are the reason the code comment says "a vocal note inside a phrase" rather than "a vocal note". Say the word if you would prefer the flag read from an unfiltered source instead.Tests
test/unit/derived-flags.test.tscontained an assertion that locked in the buggy behavior (hasVocals = truefor a lyrics-only.chart); it is flipped tofalsewith a comment explaining why. Added:.chartraises nodiff_vocalsissue.chartthat setsdiff_vocalsreports it as an extra valuePART VOCALSMIDI with a pitched note inside a phrase still reportshasVocals = truesong.inilackingdiff_vocals, still raises the missing-value issue — the positive direction, which needs an ini present or the whole metadata block is skippedFull suite passes: 319 tests across 11 files.
Note:
npm run lintreports pre-existingBigInt/setBigInt64errors insrc/chart/track-hasher.ts. I confirmed these are identical on a pristine checkout ofmasterand unrelated to this change.