diff --git a/.changeset/lucky-pandas-shave.md b/.changeset/lucky-pandas-shave.md new file mode 100644 index 0000000..882b11b --- /dev/null +++ b/.changeset/lucky-pandas-shave.md @@ -0,0 +1,18 @@ +--- +'scan-chart': patch +--- + +Fix `hasVocals` treating a lyrics-only chart as a vocals chart + +`notesData.hasVocals` was derived from `notePhrases.length > 0`, but a phrase is +only a range marker — lyrics live inside phrases that need not hold any vocal +note. Every chart that ships lyrics therefore reported `hasVocals: true` and +raised a spurious `Metadata is missing a "diff_vocals" value.` issue. This was +guaranteed for the `.chart` format, which cannot encode a vocal note at all. + +`hasVocals` now requires a vocal note inside a phrase. `hasLyrics` is unchanged, +and the `noNotes` check still accepts a lyrics-only vocal phrase as content. + +Note the other side of this: a lyrics-only chart that *sets* `diff_vocals` now +reports `Metadata contains "diff_vocals", but vocals are not charted.` where it +previously reported nothing. diff --git a/src/chart/scan-parsed-chart.ts b/src/chart/scan-parsed-chart.ts index 84f0714..a81181b 100644 --- a/src/chart/scan-parsed-chart.ts +++ b/src/chart/scan-parsed-chart.ts @@ -83,14 +83,25 @@ export function scanParsedChart(parsedChart: ParsedChart, includeBTrack = false) // `hasLyrics` / `hasVocals` are derived from the normalized vocal tracks // rather than snapshotted at parse time — keeps them state-accurate if // downstream code adds or removes vocal data. + // + // `hasVocals` means "there is a singable vocals part", so it requires a + // vocal note inside a phrase. A phrase on its own is only a range marker: a + // lyrics-only chart carries its lyrics inside phrases that hold no notes, + // and that is a chart with lyrics, not a chart with vocals. The .chart + // format cannot encode a vocal note at all, so `hasVocals` is always + // false there. + // + // `phrase.notes` is the playable projection, not every note in the source: + // the parser drops hidden percussion and notes that fall outside their + // phrase range. A vocals chart built only from such notes reports false. let hasLyrics = false let hasVocals = false - for (const part of Object.values(result.vocalTracks.parts)) { - if (part.notePhrases.length > 0) hasVocals = true + vocals: for (const part of Object.values(result.vocalTracks.parts)) { for (const phrase of part.notePhrases) { - if (phrase.lyrics.length > 0) { hasLyrics = true; break } + if (phrase.notes.length > 0) hasVocals = true + if (phrase.lyrics.length > 0) hasLyrics = true + if (hasLyrics && hasVocals) break vocals } - if (hasLyrics && hasVocals) break } return { @@ -238,8 +249,11 @@ function findChartIssues( // noNotes { - const hasVocals = Object.values(chartData.vocalTracks.parts).some(p => p.notePhrases.length > 0) - if (chartData.trackData.every(track => track.noteEventGroups.length === 0) && !hasVocals) { + // Deliberately broader than `notesData.hasVocals`: any vocal phrase is + // enough content to keep a chart out of `noNotes`, even a lyrics-only + // phrase that carries no vocal note. + const hasVocalContent = Object.values(chartData.vocalTracks.parts).some(p => p.notePhrases.length > 0) + if (chartData.trackData.every(track => track.noteEventGroups.length === 0) && !hasVocalContent) { addIssue(null, null, 'noNotes') } } diff --git a/test/unit/derived-flags.test.ts b/test/unit/derived-flags.test.ts index 81c77ea..4782fa7 100644 --- a/test/unit/derived-flags.test.ts +++ b/test/unit/derived-flags.test.ts @@ -12,6 +12,7 @@ */ import { describe, expect, it } from 'vitest' +import { writeMidi, MidiData } from '@geomitron/midi-file' import { parseChartFile } from 'src/chart/parse-chart-file' import { defaultMetadata } from 'src/ini/metadata' @@ -23,6 +24,39 @@ function buildChart(body: string): File[] { return [{ fileName: 'notes.chart', data: new TextEncoder().encode(body) }] } +/** + * A minimal `notes.mid` with a PART VOCALS track that has a real vocal note: + * a note 105 phrase marker, a sung pitch inside it, and a matching lyric. + * This is the case `.chart` cannot express, and the case `hasVocals` is for. + */ +function buildVocalMidi(iniLines?: string[]): File[] { + const tempoTrack: MidiData['tracks'][number] = [ + { deltaTime: 0, type: 'trackName', text: '' }, + { deltaTime: 0, type: 'setTempo', microsecondsPerBeat: 500000 }, + { deltaTime: 0, type: 'timeSignature', numerator: 4, denominator: 4, metronome: 24, thirtyseconds: 8 }, + { deltaTime: 0, type: 'endOfTrack' }, + ] + const vocalsTrack: MidiData['tracks'][number] = [ + { deltaTime: 0, type: 'trackName', text: 'PART VOCALS' }, + // Phrase marker (note 105) opens at tick 0. + { deltaTime: 0, type: 'noteOn', channel: 0, noteNumber: 105, velocity: 100 }, + // A sung pitch inside the phrase, with its lyric. + { deltaTime: 0, type: 'noteOn', channel: 0, noteNumber: 60, velocity: 100 }, + { deltaTime: 0, type: 'lyrics', text: 'Hello' }, + { deltaTime: 240, type: 'noteOff', channel: 0, noteNumber: 60, velocity: 0 }, + { deltaTime: 240, type: 'noteOff', channel: 0, noteNumber: 105, velocity: 0 }, + { deltaTime: 0, type: 'endOfTrack' }, + ] + const data = new Uint8Array( + writeMidi({ header: { format: 1, numTracks: 2, ticksPerBeat: 480 }, tracks: [tempoTrack, vocalsTrack] }), + ) + const files: File[] = [{ fileName: 'notes.mid', data }] + if (iniLines) { + files.push({ fileName: 'song.ini', data: new TextEncoder().encode(['[Song]', ...iniLines].join('\n')) }) + } + return files +} + describe('ParsedChart shape: derived flags no longer at top level', () => { it('parseChartFile output does not expose hasLyrics/hasVocals/hasForcedNotes', () => { const body = [ @@ -58,7 +92,10 @@ describe('scanChart: hasLyrics / hasVocals state-derived in notesData', () => { expect(scanned.notesData!.hasLyrics).toBe(false) }) - it('hasVocals = true / hasLyrics = true when [Events] has phrase + lyric events', () => { + it('hasLyrics = true but hasVocals = false when [Events] has phrase + lyric events', () => { + // A phrase is only a range marker. The .chart format has no way to encode + // a vocal note, so a .chart with lyrics is a lyrics-only chart: it has + // lyrics to display and nothing to sing. const body = [ '[Song]', '{', ' Resolution = 480', '}', '[SyncTrack]', '{', ' 0 = B 120000', '}', @@ -72,9 +109,82 @@ describe('scanChart: hasLyrics / hasVocals state-derived in notesData', () => { const files = buildChart(body) const parseResult = parseChartAndIni(files) const scanned = scanChart(files, parseResult, { includeMd5: false }) + expect(scanned.notesData!.hasVocals).toBe(false) + expect(scanned.notesData!.hasLyrics).toBe(true) + }) + + it('does not ask for a diff_vocals value on a lyrics-only chart', () => { + // `hasVocals` gates the "Metadata is missing a diff_vocals value" issue. + // A lyrics-only chart has no vocals difficulty to declare, so asking for + // one is noise on every .chart that ships lyrics. + const body = [ + '[Song]', '{', ' Resolution = 480', '}', + '[SyncTrack]', '{', ' 0 = B 120000', '}', + '[Events]', '{', + ' 0 = E "phrase_start"', + ' 120 = E "lyric Hel"', + ' 240 = E "lyric lo"', + ' 480 = E "phrase_end"', + '}', + '[ExpertSingle]', '{', + ' 0 = N 0 0', + '}', + ].join('\r\n') + const files = buildChart(body) + const scanned = scanChart(files, parseChartAndIni(files), { includeMd5: false }) + const diffVocalsIssues = scanned.metadataIssues.filter(i => i.description.includes('diff_vocals')) + expect(diffVocalsIssues).toEqual([]) + }) + + it('hasVocals = true when a vocal phrase contains a pitched note', () => { + const files = buildVocalMidi() + const scanned = scanChart(files, parseChartAndIni(files), { includeMd5: false }) expect(scanned.notesData!.hasVocals).toBe(true) expect(scanned.notesData!.hasLyrics).toBe(true) }) + + it('still asks for a diff_vocals value when the chart really has vocals', () => { + // The positive direction of the fix. Needs a song.ini: without one, + // scanChart skips the whole metadata-issue block. + const files = buildVocalMidi(['name = T', 'artist = A', 'charter = C']) + const scanned = scanChart(files, parseChartAndIni(files), { includeMd5: false }) + const missing = scanned.metadataIssues.filter( + i => i.metadataIssue === 'missingValue' && i.description.includes('diff_vocals'), + ) + expect(missing).toHaveLength(1) + }) + + it('reports diff_vocals as an extra value on a lyrics-only chart that sets it', () => { + // The other side of the trade this fix makes. Before, a lyrics-only chart + // counted as having vocals, so setting diff_vocals looked correct and + // omitting it was flagged. Now it is the reverse: the value is extra, + // because there are no vocals for it to describe. + const body = [ + '[Song]', '{', ' Resolution = 480', '}', + '[SyncTrack]', '{', ' 0 = B 120000', '}', + '[Events]', '{', + ' 0 = E "phrase_start"', + ' 120 = E "lyric Hel"', + ' 480 = E "phrase_end"', + '}', + '[ExpertSingle]', '{', ' 0 = N 0 0', '}', + ].join('\r\n') + const files: File[] = [ + { fileName: 'notes.chart', data: new TextEncoder().encode(body) }, + { + fileName: 'song.ini', + data: new TextEncoder().encode( + ['[Song]', 'name = T', 'artist = A', 'charter = C', 'diff_guitar = 3', 'diff_vocals = 4'].join('\n'), + ), + }, + ] + const scanned = scanChart(files, parseChartAndIni(files), { includeMd5: false }) + expect(scanned.notesData!.hasVocals).toBe(false) + const extra = scanned.metadataIssues.filter(i => i.description.includes('diff_vocals')) + expect(extra).toEqual([ + { metadataIssue: 'extraValue', description: 'Metadata contains "diff_vocals", but vocals are not charted.' }, + ]) + }) }) describe('scanChart: hasForcedNotes state-derived (flag disagrees with natural state)', () => {