Skip to content

Vocal phrase refactor - #90

Merged
Geomitron merged 2 commits into
Geomitron:masterfrom
elicwhite:vocal-phrase-refactor
Apr 19, 2026
Merged

Vocal phrase refactor#90
Geomitron merged 2 commits into
Geomitron:masterfrom
elicwhite:vocal-phrase-refactor

Conversation

@elicwhite

@elicwhite elicwhite commented Apr 18, 2026

Copy link
Copy Markdown
Contributor

Stacked on #84. Merge #84 first.

What changed

Type structure — single source of truth for vocal data

Removed top-level notes[] and lyrics[] from NormalizedVocalPart. All vocal content is now accessed through phrase grouping:

  • NormalizedVocalPart.notePhrases[].notes[] — vocal notes (was: NormalizedVocalPart.notes + redundant copy under each phrase)
  • NormalizedVocalPart.notePhrases[].lyrics[] — lyrics belonging to each phrase
  • NormalizedVocalPart.staticLyricPhrases[] — note 106 display-only phrases (HARM2/3)

Previously the same notes/lyrics existed in two places (top-level array + per-phrase). Consumers had to know which to use, and a malformed file could cause them to disagree. The new structure has one location per piece of data.

New fields on NormalizedVocalPart

  • rangeShifts[] — vocal range shift markers (MIDI note 0). Per-part because PART VOCALS and HARM1 often have distinct sets that must be preserved separately for round-trip.
  • lyricShifts[] — vocal lyric shift markers (MIDI note 1). Same per-part rationale.
  • textEvents[] — raw text events on the vocal track (stance markers, Band_PlayFacialAnim, etc.). YARG considers a VocalsPart non-empty iff it has phrases OR text events; without storing these, vocal tracks with only stance markers (no notes/lyrics/phrases) round-trip to empty and the track disappears.

New field on NormalizedVocalPhrase

  • player?: 1 | 2 — versus player tag (PART VOCALS only). Note 105 → player 1, note 106 → player 2. Required for versus-mode rendering. Previously, note 106 phrases were stored separately in staticLyricPhrases on PART VOCALS, which lost the distinction between "this is a player 2 scoring phrase" and "this is a HARM2/3 static lyric phrase".

PART VOCALS phrase merging

PART VOCALS now merges note 105 + note 106 phrase boundaries into a single notePhrases list (with player tags), matching YARG's MoonSongLoader.Vocals.cs behavior. Both notes create scoring phrases in YARG; they only differ in versus-player attribution. Previously we treated 105 and 106 as separate phrase lists, which caused notes near 106 boundaries to appear "orphaned" (~10K charts in the corpus had this artifact).

Harmonies keep 105/106 separate

HARM1/2/3 keep distinct 105 (scoring) and 106 (static lyric) phrase lists for lossless round-trip. The writer needs to emit each on its original MIDI note number, and CopyDown relies on HARM1's vocalPhrases (note 105 only) to know which phrases to clone to HARM2/3.

HARM3 CopyDownPhrases

HARM3 now clones staticLyricPhrases from HARM2 (in addition to scoring phrases from HARM1), matching YARG CopyDownPhrases exactly. Previously HARM3 had no static lyric phrases unless authored directly on the HARM3 track.

Lyric text preservation

NormalizedLyricEvent.text now stores the original unstripped text including markup symbols (#, ^, +, =, $, _, §, etc.). Consumers should use the flags bitmask for semantic interpretation rather than parsing the text directly. Previously we stripped flag symbols and replaced = with -, which made round-trip writing incorrect (writers couldn't reconstruct the original text).

Pitch preservation for nonPitched notes

NormalizedVocalNote.pitch keeps the original MIDI pitch for nonPitched notes (lyric flags #/^/*). Previously we set pitch to -1 for both percussion and nonPitched, losing the distinction. Consumers check the associated lyric's nonPitched flag for semantic meaning.

Pre-collect lyrics per phrase

The phrase-grouping algorithm now advances the lyric index to the phrase end regardless of whether the phrase has notes. Previously, pitch-slide-only phrases (no notes) would leave their lyrics stranded between phrases, causing divergence on round-trip.

Pitch-slide skip refinement

Pitch-slide notes are only skipped when their associated lyric survives the emptiness filter. Prevents losing slides whose lyrics would otherwise be dropped.

extractMidiVocalTextEvents

New helper extracts bracketed control-event text events on vocal tracks (stance markers, Band_PlayFacialAnim, etc.) into VocalTrackData.textEvents. Filters out lyrics (handled separately) and events scan-chart consumes internally (ENHANCED_OPENS, [mix N drumsM], [range_shift ...]).

Generic chart-parser helpers (used by both .chart and .mid paths)

These aren't strictly vocal-related but are bundled because the vocal work needed them and they're load-bearing elsewhere:

  • resolveChartTrackName(sectionName) — matches YARG ChartReader logic for resolving a .chart section name like ExpertDoubleDrums into an {instrument, difficulty} pair. Handles non-standard names (e.g. "ExpertDoubleDrums" in Megadeth - Bite the Hand) which YARG accepts via prefix+suffix matching.
  • parseChartSectionEventText(text) — matches YARG's TextEvents.NormalizeTextEvent → TryParseSectionEvent pipeline for section event detection. Handles typo cases like sections Pre-Chorus that YARG's StartsWith("section") accepts.

Why

The previous vocal data structure (PR #85 / vocal-normalization) correctly produced phrase-grouped output but left several issues unresolved:

  1. Ambiguity: notes/lyrics existed in two places (top-level + per-phrase). Future code changes risked them drifting apart.
  2. Round-trip incorrectness: lyric symbols were stripped, pitches were lost, range/lyric shifts and text events weren't exposed, so the writer couldn't reconstruct the original chart.
  3. Versus mode unsupported: player attribution was lost when 105/106 phrases were merged on PART VOCALS, blocking versus-mode rendering.
  4. HARM2/3 round-trip: needed the distinction between 105 (scoring, copied from HARM1) and 106 (static lyric, kept on the harmony track) for the writer to re-emit them on the correct MIDI note.

This refactor doesn't change YARG-comparable output — both before and after produce 0 vocal phrase diffs across 78,452 charts. The value is enabling round-trip writing, exposing data consumers need (player tags, shifts, text events), and removing the dual-storage ambiguity.

Validation

  • 268 unit tests pass
  • 0 hash regressions vs upstream master baseline (15,524 charts)
  • 0 vocal phrase diffs vs YARG dumps across the full corpus (78,452 charts) — phrase boundaries, note ticks/pitches, lyric counts/text/flags all match YARG exactly

@elicwhite
elicwhite force-pushed the vocal-phrase-refactor branch 12 times, most recently from a43c92d to 8dbb09a Compare April 18, 2026 17:38
Eliminate top-level notes[]/lyrics[] from NormalizedVocalPart. All vocal
content is accessed through phrase grouping.

- NormalizedVocalPart: removed notes[] and lyrics[]
- NormalizedVocalPhrase: added player?: 1 | 2 for versus mode
- NormalizedLyricEvent.text: stores original unstripped markup symbols
- NormalizedVocalNote.pitch: nonPitched keeps original MIDI pitch
- PART VOCALS merges note 105+106 phrases with player tags
- Harmonies keep separate 105/106 for lossless round-trip
- Pre-collect lyrics per phrase (lyricIdx advances to phraseEnd
  regardless of notes, preventing divergence on pitch slide round-trip)
- Pitch slide notes only skipped when lyric survives emptiness filter
- HARM3 CopyDown: clone staticLyricPhrases from HARM2
- Writer emits union of notes/lyrics from both phrase sets
- extractMidiVocalTextEvents for stance/facial anim events on vocal tracks

Generic chart-parser helpers (used elsewhere too):
- resolveChartTrackName: matches YARG's ChartReader logic for instrument+
  difficulty resolution from .chart section names ("ExpertDoubleDrums" etc.)
- parseChartSectionEventText: matches YARG's TextEvents.NormalizeTextEvent
  pipeline for section event detection
@elicwhite
elicwhite force-pushed the vocal-phrase-refactor branch from 8dbb09a to 6ec8508 Compare April 18, 2026 17:42
Verify that a lyric at tick T (where phrase 1 ends at T and phrase 2 starts at T)
ends up in the new phrase, including when the lyric event appears before the
phrase noteOn events in MIDI file order.
@Geomitron
Geomitron merged commit 5c462c6 into Geomitron:master 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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants