Restore lenient tick-0 trackName matching in MIDI parser - #94
Merged
Geomitron merged 1 commit intoApr 19, 2026
Merged
Conversation
Revert the vocal-phrase-refactor change that made getTracks() take only
the FIRST tick-0 trackName event (matching YARG.Core's
MidiExtensions.GetTrackName). That change dropped tracks from real
charts that have a bogus or descriptive name first, followed by the
real instrument name — also at tick 0:
[ENHANCED_OPENS] → PART BASS (Culture Killer - Blindfolded Death)
[ENHANCED_OPENS] → PART GUITAR (Culture Killer - Blindfolded Death)
TEMPO TRACK → PART DRUMS (Periphery - Ji)
school food punishment - close, down, back to → PART DRUMS (school food punishment)
YARG drops these tracks; we don't. Walk every tick-0 trackName event
and accept the first one that matches a recognized instrument. Capture
the first tick-0 trackName for unrecognized-track round-trip purposes
regardless of whether anything matched (preserves the
unrecognizedMidiTracks behavior added later in the stack).
Add unit tests covering the happy path (recognized name after a bogus
leading name), the all-recognized case (first match wins), the
unrecognized-track fallback (first tick-0 name captured), and the
"trackName past tick 0 doesn't count" edge case.
Validation:
- vitest: 283/283 (5 new) pass
- 78,046-chart hash baseline: zero diffs vs scan-chart@8.0.1
(the 3 previously-diffing charts are now matched)
- 78,452-chart chart-edit roundtrip corpus: 78,452/78,453 deeply
equal (one known by-design failure unchanged)
elicwhite
marked this pull request as ready for review
April 19, 2026 04:33
This was referenced Apr 19, 2026
elicwhite
added a commit
to elicwhite/scan-chart
that referenced
this pull request
Apr 19, 2026
## Summary Split the monolithic `scanChartFolder(files, config?)` into two functions so consumers that only need the parsed shape can skip the expensive validation/hashing/asset-scanning step. ```ts parseChartAndIni(files): ParseChartAndIniResult // file discovery + parseChartFile + scanIni // returns ParsedChart (with chartBytes/format/iniChartModifiers attached // for downstream hashing) plus the ini scan results. // No hashing, no asset I/O. scanChart(files, parseResult, config?): ScannedChart // What scanChartFolder used to do, minus the parsing. // Hashing + notesData + difficulty / playable / metadata-flatten / audio / // image / video logic. Returns the same ScannedChart shape as before. // Same ScanChartFolderConfig knobs. ``` \`scanChartFolder\` is preserved as a deprecated 3-line shim: \`\`\`ts /** @deprecated ... back-compat shim */ export function scanChartFolder(files, config?) { return scanChart(files, parseChartAndIni(files), config) } \`\`\` So this is **not a breaking change** — existing callers keep working unchanged (just see a deprecation warning), and new callers can opt into the two-step API to skip hashing when they don't need it. \`ScanChartFolderConfig\` and \`ScannedChart\` interfaces are unchanged. All other helpers — \`findChartIssues\`, \`getChartHash\`, \`legacyGetChartHash\`, the asset scanners — stay where they were. ## Review tour A single commit, 6 files (\`interfaces.ts\` unchanged): - **\`src/chart/parse-chart-and-ini.ts\`** (new) — \`parseChartAndIni()\` and the \`findChartData\` helper relocated from \`chart-scanner.ts\`. The new \`ParsedChart\` type extends \`ReturnType<typeof parseChartFile>\` with \`chartBytes\`, \`format\`, and \`iniChartModifiers\` so downstream hashing can run without re-parsing. - **\`src/chart/chart-scanner.ts\`** — the existing module-level \`scanChart(files, ini, btrack)\` is renamed to \`scanParsedChart(parsedChart, includeBTrack?)\` (now a private helper, not exported from the package) and now takes a \`ParsedChart\`. Body change is structural: drop the inline \`findChartData\` + \`parseChartFile\` + try/catch + \`null\` return path (parsing now happens in \`parseChartAndIni\`), and use \`result.X\` for byte-equivalent body refs (via \`const result = parsedChart\`). The chart-hash call uses \`result.chartBytes\`. \`findChartIssues\`, \`getChartHash\`, \`legacyGetChartHash\`, etc. unchanged. **Most of the apparent diff is indentation; the body code is byte-identical to master.** - **\`src/index.ts\`** — adds the new exports + \`scanChart()\`. The validation logic — \`checkMissingDifficulty\`, \`checkExtraDifficulty\`, \`playable\`, \`chart_offset\`, metadata flattening, audio/image/video scans — is unchanged; only the renames \`iniData.metadata\` → \`parseResult.iniMetadata\`, \`chartData.metadata\` → \`parseResult.parsedChart?.metadata\` differ. \`scanChartFolder\` is now a 3-line \`@deprecated\` shim at the bottom. - **\`src/chart/index.ts\`** — one-line addition to re-export from the new module. - **\`src/test.ts\`** — CLI updated to call \`scanChart(files, parseChartAndIni(files), …)\`. - **\`readme.md\`** — documents the new functions; marks \`scanChartFolder\` as deprecated. The reviewer ignoring whitespace will see a much smaller diff in \`chart-scanner.ts\` than the raw line count suggests. ### New API usage \`\`\`ts const parsed = parseChartAndIni(files); const result = scanChart(files, parsed, { includeBTrack, includeMd5 }); \`\`\` For tooling that doesn't need hashes or chart-issue detection, just stop at \`parseChartAndIni(files).parsedChart\`. ### Why \`chartBytes\` on \`ParsedChart\`? scan-chart's current \`chartHash\` is \`blake3(chartBytes ++ ini-modifier name/value pairs)\` — it hashes the file contents directly plus the few ini knobs that affect parsing. That format predates the SongHash spec and is what Clone Hero uses today to decide whether an in-game score should reset. Because it consumes the raw bytes, the hashing helper needs them on the \`ParsedChart\`. The newer SongHash spec computes the chart-folder hash from metadata strings + duration + per-track BTrack hashes — no raw bytes required. scan-chart does not implement SongHash today; once it does, \`chartBytes\` can be dropped from \`ParsedChart\`. ## Validation - vitest: **278 / 278 pass** - 78,452-chart chart-edit roundtrip corpus: **78,452 / 78,453 deeply equal** (matches baseline; one known by-design failure: Old Man's Child BEAT track with literal negative MIDI delta values) - 78,046-chart hash baseline (against \`scan-chart@8.0.1\`): same 3 pre-existing trackname-discovery diffs vs baseline. Those are independently fixed in Geomitron#94. ## Related - Geomitron#94: independent fix for 3-chart trackname-discovery regression introduced by Geomitron#90 (vocal phrase refactor)
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
2 participants
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.
Summary
PR #90 (Vocal phrase refactor) changed
getTracks()insrc/chart/midi-parser.tsto take only the FIRST tick-0trackNameevent, matching YARG.Core'sMidiExtensions.GetTrackName. That's stricter than the prior behavior, which walked all tick-0 trackNames and accepted the first one matching a known instrument. The stricter behavior breaks real charts in the wild that emit a bogus or descriptive trackName at tick 0 immediately followed by the real instrument trackName, also at tick 0:[ENHANCED_OPENS]PART BASS[ENHANCED_OPENS]PART GUITARTEMPO TRACKPART DRUMSPART DRUMSYARG drops these 16 tracks across the 3 charts. Pre-#90, scan-chart kept them.
This PR restores the lenient behavior: walk every tick-0 trackName event and accept the first one that matches a recognized instrument. The first tick-0 trackName is still captured for
unrecognizedMidiTracksround-trip purposes when nothing matches (PR #91 behavior preserved).Tests
New file
src/__tests__/tick-zero-trackname.test.ts(5 tests) — synthetic MIDI fixtures that mirror the real-chart patterns above, plus the all-recognized-names case (first match wins), the unrecognized-track fallback (first tick-0 name captured), and the "trackName past tick 0 doesn't count" edge case.Validation
scan-chart@8.0.1): zero diffs — the 3 affected charts above had previously produced 16 dropped track hashes vs the npm baseline and now match again.Background
Found while bisecting a 3-chart hash regression in elicwhite#21 — see elicwhite#22 for the fork-side write-up of the bisect and the same fix.