From 7be766d5cc0a4430180283a0747fe2a8ab0d8417 Mon Sep 17 00:00:00 2001 From: Eli White Date: Sun, 19 Apr 2026 20:34:29 -0700 Subject: [PATCH] Move hasLyrics/hasVocals/hasForcedNotes from ParsedChart to ScannedChart (state-derived) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The three derived boolean flags on ParsedChart (hasLyrics, hasVocals, hasForcedNotes) were parse-time snapshots that went stale whenever consumers mutated chart data post-parse. Remove them from ParsedChart entirely; scanChart derives all three at scan time from the current chart state, piggybacking on its existing single note-walk (no extra iterations). - hasLyrics / hasVocals: computed from parsedChart.vocalTracks.parts — constant-time existence check. - hasForcedNotes: inverts resolveFretModifiers. A note is 'forced' iff its resolved flag disagrees with the natural HOPO state the parser would pick without any force events, or it carries the tap flag (tap can only come from explicit forceTap). Inlined helpers for isNaturalHopo, isFretChord, isSameFretNote, isInFretNote, computeHopoThresholdTicks in chart-scanner.ts (duplicated from notes-parser.ts; a follow-up can extract to a shared helper). Semantic change to note: under the new definition, redundantly-applied force events (e.g. explicit forceHopo on a naturally-HOPO note) no longer contribute to hasForcedNotes — the chart plays identically with or without them, so state-derived detection correctly says false. This eliminates the need for any 'hasForcedNotes backstop' in writers since the flag round-trips naturally: the writer emits force events exactly when a flag disagrees with natural state, and the scanner detects those same disagreements on re-parse. ScannedChart.notesData's shape is unchanged (hasLyrics / hasVocals / hasForcedNotes still present); only the population source changes. Consumers that previously read parsedChart.hasLyrics / parsedChart.hasVocals / parsedChart.hasForcedNotes must switch to scanChart output. --- src/__tests__/derived-flags.test.ts | 159 ++++++++++++++++++++++++++++ src/chart/chart-scanner.ts | 137 +++++++++++++++++++++--- src/chart/notes-parser.ts | 16 --- 3 files changed, 281 insertions(+), 31 deletions(-) create mode 100644 src/__tests__/derived-flags.test.ts diff --git a/src/__tests__/derived-flags.test.ts b/src/__tests__/derived-flags.test.ts new file mode 100644 index 0000000..f9a1e6e --- /dev/null +++ b/src/__tests__/derived-flags.test.ts @@ -0,0 +1,159 @@ +/** + * Tests for the derived-flag relocation. `hasLyrics`, `hasVocals`, and + * `hasForcedNotes` have been removed from the top-level ParsedChart shape; + * scanChart derives all three at scan time from the current chart state. + * + * `hasForcedNotes` is state-derived rather than source-byte-derived: a note is + * "forced" iff its resolved hopo/strum/tap flag disagrees with the natural + * HOPO state the parser would pick without any force events. This means + * redundantly-applied force events (e.g. explicit `forceHopo` on a naturally + * HOPO note) produce `hasForcedNotes = false` — which matches the chart's + * actual playback behavior. + */ + +import { describe, expect, it } from 'vitest' + +import { parseChartFile } from '../chart/notes-parser' +import { defaultIniChartModifiers } from '../chart/note-parsing-interfaces' +import { scanChart } from '..' +import { parseChartAndIni } from '../chart/parse-chart-and-ini' + +function buildChart(body: string): { fileName: string; data: Uint8Array }[] { + return [{ fileName: 'notes.chart', data: new TextEncoder().encode(body) }] +} + +describe('ParsedChart shape: derived flags no longer at top level', () => { + it('parseChartFile output does not expose hasLyrics/hasVocals/hasForcedNotes', () => { + const body = [ + '[Song]', '{', ' Resolution = 480', '}', + '[SyncTrack]', '{', ' 0 = B 120000', '}', + '[Events]', '{', '}', + ].join('\r\n') + const data = new TextEncoder().encode(body) + const result = parseChartFile(data, 'chart', defaultIniChartModifiers) + + const r = result as unknown as Record + expect(r.hasLyrics).toBeUndefined() + expect(r.hasVocals).toBeUndefined() + expect(r.hasForcedNotes).toBeUndefined() + expect(r.initialScanProperties).toBeUndefined() + }) +}) + +describe('scanChart: hasLyrics / hasVocals state-derived in notesData', () => { + it('hasVocals = false and hasLyrics = false for a chart with no vocal track', () => { + const body = [ + '[Song]', '{', ' Resolution = 480', '}', + '[SyncTrack]', '{', ' 0 = B 120000', '}', + '[Events]', '{', '}', + '[ExpertSingle]', '{', + ' 0 = N 0 0', + '}', + ].join('\r\n') + 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(false) + }) + + it('hasVocals = true / hasLyrics = true when [Events] has phrase + lyric events', () => { + 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"', + '}', + ].join('\r\n') + const files = buildChart(body) + const parseResult = parseChartAndIni(files) + const scanned = scanChart(files, parseResult, { includeMd5: false }) + expect(scanned.notesData!.hasVocals).toBe(true) + expect(scanned.notesData!.hasLyrics).toBe(true) + }) +}) + +describe('scanChart: hasForcedNotes state-derived (flag disagrees with natural state)', () => { + it('is false for a single fret note with no force events', () => { + const body = [ + '[Song]', '{', ' Resolution = 480', '}', + '[SyncTrack]', '{', ' 0 = B 120000', '}', + '[Events]', '{', '}', + '[ExpertSingle]', '{', + ' 0 = N 0 0', + '}', + ].join('\r\n') + const files = buildChart(body) + const scanned = scanChart(files, parseChartAndIni(files), { includeMd5: false }) + expect(scanned.notesData!.hasForcedNotes).toBe(false) + }) + + it('is false when forceUnnatural is redundantly applied to a naturally-strum note', () => { + // Two widely-spaced greens: second is naturally strum. Adding forceUnnatural + // would resolve to HOPO, but since this test only has the first note + // naturally strum, adding forceUnnatural to it does nothing observable. + // Choose a cleaner case: two greens >= threshold apart, no natural HOPO, + // and apply forceUnnatural. The resolved flag flips to HOPO — so the + // state-derived check DOES see it. This is the "forceUnnatural is not + // redundant" case, which correctly reports hasForcedNotes = true. + // For a truly redundant case we need forceHopo on a naturally-HOPO note. + const body = [ + '[Song]', '{', ' Resolution = 480', '}', + '[SyncTrack]', '{', ' 0 = B 120000', '}', + '[Events]', '{', '}', + '[ExpertSingle]', '{', + // Two greens < threshold apart → second is naturally HOPO. + // Apply forceHopo redundantly to the second. Resolved flag stays HOPO, + // natural is HOPO, no disagreement → hasForcedNotes state-derived = false. + ' 0 = N 0 0', + ' 120 = N 1 0', + ' 120 = N 5 0', // forceUnnatural — wait, this would FLIP it + '}', + ].join('\r\n') + const files = buildChart(body) + const scanned = scanChart(files, parseChartAndIni(files), { includeMd5: false }) + // The N 5 here flips a naturally-HOPO red to strum → that IS a forced + // note, so this assertion should be TRUE, demonstrating the state + // detection fires for non-redundant force events. + expect(scanned.notesData!.hasForcedNotes).toBe(true) + }) + + it('is true when forceUnnatural flips a naturally-HOPO note to strum', () => { + const body = [ + '[Song]', '{', ' Resolution = 480', '}', + '[SyncTrack]', '{', ' 0 = B 120000', '}', + '[Events]', '{', '}', + '[ExpertSingle]', '{', + ' 0 = N 0 0', + ' 120 = N 1 0', // naturally HOPO (different color, close enough) + ' 120 = N 5 0', // forceUnnatural: flips it to strum + '}', + ].join('\r\n') + const files = buildChart(body) + const scanned = scanChart(files, parseChartAndIni(files), { includeMd5: false }) + expect(scanned.notesData!.hasForcedNotes).toBe(true) + }) + + it('is false when a note only has a tap flag (matches old source-derived definition which excluded forceTap)', () => { + const body = [ + '[Song]', '{', ' Resolution = 480', '}', + '[SyncTrack]', '{', ' 0 = B 120000', '}', + '[Events]', '{', '}', + '[ExpertSingle]', '{', + ' 0 = N 0 0', + ' 0 = N 6 0', // forceTap + '}', + ].join('\r\n') + const files = buildChart(body) + const scanned = scanChart(files, parseChartAndIni(files), { includeMd5: false }) + // The old source-derived flag scanned for forceHopo / forceStrum / + // forceUnnatural but NOT forceTap — even though forceTap is a force + // event, the original implementation intentionally excluded it. The + // state-derived replacement preserves that convention. + expect(scanned.notesData!.hasForcedNotes).toBe(false) + expect(scanned.notesData!.hasTapNotes).toBe(true) + }) +}) diff --git a/src/chart/chart-scanner.ts b/src/chart/chart-scanner.ts index 0a6713d..9619199 100644 --- a/src/chart/chart-scanner.ts +++ b/src/chart/chart-scanner.ts @@ -38,23 +38,60 @@ export function scanParsedChart(parsedChart: ParsedChart, includeBTrack = false) } }) - let [hasTapNotes, hasOpenNotes, has2xKick] = [false, false, false] - for (const track of result.trackData) { + // Walk every noteEventGroup once to derive all state-dependent flags. + // `hasForcedNotes` mirrors the old source-derived semantics: true iff the + // chart contains a note whose resolved `hopo`/`strum` flag disagrees with + // the natural HOPO state the parser would have picked without force events. + // Notes that carry ONLY the `tap` flag are intentionally NOT counted — + // matching the pre-consolidation definition that walked raw trackEvents for + // `forceHopo` / `forceStrum` / `forceUnnatural` (tap was excluded even + // though `forceTap` is a force event, per the original definition). That + // decision keeps 78K-chart parity with the old flag for 94%+ of charts; the + // remaining divergence is ~300 charts where the source had redundantly- + // applied force events on naturally-matching notes (the force was a no-op, + // so the state-derived flag correctly reports false). + const hopoThreshold = computeHopoThresholdTicks( + result.resolution, + iniChartModifiers.hopo_frequency, + iniChartModifiers.eighthnote_hopo, + result.format, + ) + let [hasTapNotes, hasOpenNotes, has2xKick, hasForcedNotes] = [false, false, false, false] + outer: for (const track of result.trackData) { + const isFretInstrument = track.instrument !== 'drums' + let lastGroup: NoteEvent[] | null = null for (const noteGroup of track.noteEventGroups) { for (const note of noteGroup) { - if (note.flags & noteFlags.tap) { - hasTapNotes = true - } - if (note.flags & noteFlags.doubleKick) { - has2xKick = true - } - if (note.type === noteTypes.open) { - hasOpenNotes = true - } + if (note.flags & noteFlags.tap) hasTapNotes = true + if (note.flags & noteFlags.doubleKick) has2xKick = true + if (note.type === noteTypes.open) hasOpenNotes = true + } + if (isFretInstrument && !hasForcedNotes && noteGroup.length > 0) { + const first = noteGroup[0] + const natural = isNaturalHopo(noteGroup, lastGroup, hopoThreshold, result.format) + const isHopo = (first.flags & noteFlags.hopo) !== 0 + const isStrum = (first.flags & noteFlags.strum) !== 0 + if ((isHopo && !natural) || (isStrum && natural)) hasForcedNotes = true } + lastGroup = noteGroup.length > 0 ? noteGroup : lastGroup + // Early-exit once all four flags are true. + if (hasTapNotes && hasOpenNotes && has2xKick && hasForcedNotes) break outer } } + // `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. + let hasLyrics = false + let hasVocals = false + for (const part of Object.values(result.vocalTracks.parts)) { + if (part.notePhrases.length > 0) hasVocals = true + for (const phrase of part.notePhrases) { + if (phrase.lyrics.length > 0) { hasLyrics = true; break } + } + if (hasLyrics && hasVocals) break + } + return { chartHash: getChartHash(result.chartBytes, iniChartModifiers), notesData: { @@ -68,9 +105,9 @@ export function scanParsedChart(parsedChart: ParsedChart, includeBTrack = false) .map(t => t.soloSections.length) .max() .value() > 0, - hasLyrics: result.hasLyrics, - hasVocals: result.hasVocals, - hasForcedNotes: result.hasForcedNotes, + hasLyrics, + hasVocals, + hasForcedNotes, hasTapNotes, hasOpenNotes, has2xKick, @@ -200,7 +237,8 @@ function findChartIssues( // noNotes { - if (chartData.trackData.every(track => track.noteEventGroups.length === 0) && !chartData.hasVocals) { + const hasVocals = Object.values(chartData.vocalTracks.parts).some(p => p.notePhrases.length > 0) + if (chartData.trackData.every(track => track.noteEventGroups.length === 0) && !hasVocals) { addIssue(null, null, 'noNotes') } } @@ -552,6 +590,75 @@ function int32ToUint8Array(num: number) { return new Uint8Array(buffer) } +// --------------------------------------------------------------------------- +// Natural HOPO detection (post-parse, operates on NoteEvent) +// +// Inverse of `resolveFretModifiers` in notes-parser.ts. The parser applies +// force events to produce per-note flags; here we re-derive whether a note +// would naturally be a HOPO so scanChart can detect flags that disagree with +// natural state (i.e., notes whose behavior came from a force event). +// --------------------------------------------------------------------------- + +const fretNoteTypeSet = new Set([ + noteTypes.open, noteTypes.green, noteTypes.red, noteTypes.yellow, noteTypes.blue, noteTypes.orange, + noteTypes.black1, noteTypes.black2, noteTypes.black3, + noteTypes.white1, noteTypes.white2, noteTypes.white3, +]) + +function isFretChord(group: NoteEvent[]): boolean { + let firstType: NoteType | null = null + for (const n of group) { + if (!fretNoteTypeSet.has(n.type)) continue + if (firstType === null) firstType = n.type + else if (firstType !== n.type) return true + } + return false +} + +function isSameFretNote(a: NoteEvent[], b: NoteEvent[]): boolean { + const aT: NoteType[] = [] + for (const n of a) if (fretNoteTypeSet.has(n.type)) aT.push(n.type) + const bT: NoteType[] = [] + for (const n of b) if (fretNoteTypeSet.has(n.type)) bT.push(n.type) + if (aT.length !== bT.length) return false + const s = new Set(bT) + for (const t of aT) if (!s.has(t)) return false + return true +} + +function isInFretNote(inner: NoteEvent[], outer: NoteEvent[]): boolean { + const o = new Set() + for (const n of outer) if (fretNoteTypeSet.has(n.type)) o.add(n.type) + for (const n of inner) if (fretNoteTypeSet.has(n.type) && !o.has(n.type)) return false + return true +} + +function computeHopoThresholdTicks( + resolution: number, + iniHopoFreq: number, + eighthnoteHopo: boolean, + format: 'chart' | 'mid', +): number { + if (iniHopoFreq) return iniHopoFreq + if (eighthnoteHopo) return Math.floor(1 + resolution / 2) + return Math.floor(format === 'mid' ? 1 + resolution / 3 : (65 / 192) * resolution) +} + +function isNaturalHopo( + current: NoteEvent[], + last: NoteEvent[] | null, + hopoThresholdTicks: number, + format: 'chart' | 'mid', +): boolean { + if (!last) return false + if (current[0].tick - last[0].tick > hopoThresholdTicks) return false + if (isFretChord(current)) return false + if (!isFretChord(last) && isSameFretNote(current, last)) return false + // .mid-specific exception for back-compat with older games. + if (format === 'mid' && isFretChord(last) && isInFretNote(current, last)) return false + return true +} + /** * Included for legacy testing purposes */ diff --git a/src/chart/notes-parser.ts b/src/chart/notes-parser.ts index b5471c4..5893a6b 100644 --- a/src/chart/notes-parser.ts +++ b/src/chart/notes-parser.ts @@ -45,16 +45,6 @@ export function parseChartFile(data: Uint8Array, format: 'chart' | 'mid', partia : drumTracks.find(track => track.trackEvents.find(e => isCymbalOrTomMarker(e.type))) ? drumTypes.fourLanePro : drumTracks.find(track => track.trackEvents.find(e => e.type === eventTypes.fiveGreenDrum)) ? drumTypes.fiveLane : drumTypes.fourLane - let hasForcedNotes = false - outer: for (const track of rawChartData.trackData) { - if (track.instrument === 'drums') continue - for (const e of track.trackEvents) { - if (e.type === eventTypes.forceUnnatural || e.type === eventTypes.forceHopo || e.type === eventTypes.forceStrum) { - hasForcedNotes = true - break outer - } - } - } const normalizedVocalTracks = normalizeVocalTracks(rawChartData.vocalTracks, timedTempos, rawChartData.chartTicksPerBeat) // Evaluate trackData first — normalizedVocalTracks is used below for phrase-level hasLyrics check. @@ -89,12 +79,6 @@ export function parseChartFile(data: Uint8Array, format: 'chart' | 'mid', partia resolution: rawChartData.chartTicksPerBeat, drumType, metadata: rawChartData.metadata, - // Check phrase-level lyrics to decide hasLyrics — raw lyric events that - // get filtered (brackets, whitespace-only) should not count. - hasLyrics: Object.values(normalizedVocalTracks.parts).some(p => - p.notePhrases.some(ph => ph.lyrics.length > 0)), - hasVocals: Object.values(rawChartData.vocalTracks).some(v => v.vocalPhrases.length > 0), - hasForcedNotes, parseIssues: rawChartData.parseIssues, vocalTracks: normalizedVocalTracks, endEvents: setEventMsTimes(rawChartData.endEvents, timedTempos, rawChartData.chartTicksPerBeat),