From 749af31cc3f9fa93bcacd3952744473711ec84f0 Mon Sep 17 00:00:00 2001 From: OmikronApex <45161725+OmikronApex@users.noreply.github.com> Date: Thu, 2 Jul 2026 08:40:20 +0200 Subject: [PATCH] fix: correctly import and notate multi-staff (piano/keys) tracks from GP8 (#692) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: correctly import and notate multi-staff (piano/keys) tracks from GP8 Fixes bass stave being dropped on import (bar-column enumeration bug) and wrong hand-split heuristic in notation_lift for chords straddling middle C. Co-Authored-By: Claude Sonnet 5 Signed-off-by: byrongamatos * fix(gp-import): fold all grand-staff staves, per-stave tuning, playable hand-splits Addresses review on #692 (topkoa): - split_hands: only use the middle-C boundary when both resulting hands are within HAND_SPLIT_SPAN_SEMITONES, else fall back to the largest-gap heuristic — a hard middle-C split otherwise put a 19-semitone (unplayable) span in one hand for bass-under-treble voicings (e.g. E2+B3 under an Em7 shape). - Treat any multi-stave (grand-staff) track as keys end-to-end, so the stave-0 and folded stave-1+ notes share one encoding and note_count (which sums every stave column) matches what actually imports — closing the phantom-count case for grand-staff instruments the name/program heuristics miss (harp, celesta, marimba). - Fold *every* extra stave (stave_columns[1:]), not just stave 1. - Per-staff tuning fall-back to the track-level Tuning property so an untuned staff never yields an empty pitch list (silent note loss); via a shared _parse_tuning helper. - Extract _collect_column_notes / _merge_lh_notes so the GPX LH/RH pair merge and the GP8 grand-staff fold share one implementation and can't drift in tie/timing/dedup handling. - Rebuild filtered_to_raw from the already-computed stave_columns (one source of truth for the counting rule) and drop the dead num_raw_tracks/raw_tracks. Tests: grand-staff fold + bar-column offset (test_gp2notation.py); both middle-C split cases (test_notation_lift.py). CHANGELOG updated. Co-Authored-By: Claude Opus 4.8 (1M context) Signed-off-by: byrongamatos --------- Signed-off-by: byrongamatos Co-authored-by: Claude Sonnet 5 Co-authored-by: byrongamatos --- CHANGELOG.md | 1 + lib/gp2rs_gpx.py | 361 ++++++++++++++++++++++-------------- lib/notation_lift.py | 26 ++- tests/test_gp2notation.py | 80 ++++++++ tests/test_notation_lift.py | 20 ++ 5 files changed, 343 insertions(+), 145 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 31c465b..e1cc427 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -58,6 +58,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **v3 library: exact artist/album filters + scroll/page-depth restore** (feedBack#857). The v3 Songs toolbar gains Artist and Album dropdowns (Album populates from the selected artist and stays disabled until one is chosen), backed by new exact, case-insensitive (`COLLATE NOCASE`) `artist` / `album` query params threaded through `MetadataDB._build_where` → `query_page` / `query_artists` / `query_stats` and the `/api/library`, `/api/library/artists`, `/api/library/stats` endpoints (the free-text `q` search stays fuzzy and composes with the exact filters). The artist/album catalog is fetched independently of the active artist/album selection so the dropdowns always list the full set for the current provider/search. The toolbar is now sticky so filter controls stay reachable when browsing deep libraries, and returning from the player restores the previous scroll position **and** the loaded infinite-scroll page depth via a `sessionStorage` snapshot keyed by a filter/sort/view state hash (invalidated whenever those change, so a filter change still resets to the top). Tests: `tests/test_library_filters.py` (backend artist/album filters), `tests/js/v3_songs_scroll.test.js` (state-hash + snapshot helpers). ### Fixed +- **GP8 multi-staff (piano/keys) tracks now import both hands — the bass stave was being silently dropped, and hand-splits landed on the wrong hand.** A GP8 grand-staff keyboard part is one `` with two `` entries, and `MasterBar/Bars` lists one bar id per **stave**, not per track (`lib/gp2rs_gpx.py`). Two bugs fell out of assuming one stave per track: (1) the bar-column lookup used a raw `enumerate(Tracks)` index, so every track *after* a multi-stave track read the wrong column; (2) the string-tuning parse scanned all `.//Property` descendants and let the last stave's `` overwrite the first, so a treble note indexed against the 5-entry bass tuning fell out of range in `_note_midi` and was **dropped without a trace**. The importer now advances a bar-column counter by each track's stave count, reads tuning **per stave** (with a per-staff fall-back to the track-level property so an untuned staff never yields empty pitches), and folds **every** extra stave's notes into the arrangement (not just stave 1), keeping the `note_count` import-preview honest. A grand-staff track is now classified as keys end-to-end so the stave-0 and folded stave-1+ notes share one encoding. Separately, `notation_lift.split_hands` no longer forces a hard middle-C split when doing so produces a physically unplayable hand (e.g. a bass note under an Em7-shape voicing dipping below C4 would put a 19-semitone span in one hand) — it uses the middle-C boundary only when both resulting hands are within `HAND_SPLIT_SPAN_SEMITONES`, else falls back to the largest-gap heuristic. The GPX LH/RH pair merge and the GP8 stave fold now share one `_collect_column_notes` / `_merge_lh_notes` pair so the two formats can't drift in tie/timing/dedup handling. Companion editor change: got-feedback/feedBack-plugin-editor#38. Tests: `tests/test_gp2notation.py` (grand-staff fold + bar-column offset), `tests/test_notation_lift.py` (both middle-C split cases). Follow-up: `lib/gp_autosync.py` still carries the pre-fix bar-column + tuning logic (CLI/tests only, no production caller). - **Tuner auto-open is now opt-in and persists instead of flashing open-then-shut.** When you entered a song (or switched arrangement) whose tuning differed from the last, the tuner auto-opened and — for some testers — vanished ~1s later (reported macOS+Windows, 0.3.0). Root cause: the tuner closes itself on `song:play` (`plugins/tuner/utils/ui.js` — you don't tune while playing), so a **song switch** fired autoplay → `song:play` → the just-auto-opened tuner closed; an **arrangement switch** (which never arms autoplay) had no `song:play`, so it stayed open — exactly why two testers saw opposite behaviour (it wasn't the mic). Now: (1) the feature is a **new opt-in setting** ("Auto-open on tuning change", in the tuner's Settings panel, persisted as `autoOpenOnTuningChange`, **default OFF**); (2) an **auto**-opened tuner *persists* — it ignores the autoplay `song:play`, stray outside-clicks, and same-screen re-emits, closing only via the new in-panel **`×`** / **"Skip"** buttons or when you leave the song. A *manually* opened tuner keeps its classic click-away / play-to-close behaviour. The panel previously had no in-box close at all; this adds one (`×` + contextual Skip). All in the tuner plugin (`routes.py` config, `screen.js` gate + persist, `utils/ui.js` buttons + `song:play` guard, `settings.html` toggle) — **no core `app.js` changes**. Tests: `tests/js/tuner_auto_open.test.js` (opt-in gate, `{ auto: true }` persist mode, play/click-proofing). **Default (opt-in vs opt-out) is teed up for Byron to decide — flip one boolean.** - **Tuner auto-open is now tuning-coverage-aware — extended-range players aren't nagged for songs their instrument already covers.** With the opt-in auto-open on, it now prompts only when your **current physical tuning** (from your instrument selection in Settings) doesn't already cover the song. FeedBack is tune-to-song — the highway draws tab in the song's tuning — so the check aligns the song's open-string tuning string-for-string against your instrument: an **8-string F♯-standard** player gets **no** prompt for a 6- or 7-string standard song (its top strings already match those tunings), while a song needing an open string you don't have (e.g. a **Drop-A 7-string**, whose low A isn't an open string on an F♯ 8-string) **still** prompts. A whole-instrument reference difference also prompts — A440 vs A432, or an octave-down `centOffset` (which the auto-open now accounts for; it was previously ignored). The player's instrument is read from core **`/api/settings`** (the v3 instrument selector — a stable physical reference, not the tuner's song-tracking selection); when nothing's declared or the lookup is unavailable it falls back to a conservative prompt, so a real retune is never silently skipped. **v3-only** (the instrument selector is v3). All in the tuner plugin (`plugins/tuner/screen.js`) — **no core changes**. Tests: `tests/js/tuner_auto_open.test.js` (covered vs uncovered, the Drop-A case, reference-pitch mismatch, contiguous alignment). _Follow-up (E1.6): a passive "different tuning" badge cue that names the string(s) to retune, plus the splitscreen / no-usable-input guards._ - **The tuner badge now passively flags when a song needs a different tuning — and names the retune.** Building on the coverage check: when you enter a song your current instrument doesn't cover, the topbar tuner badge gets an amber ring and a tooltip that **names the change** — e.g. *"retune B→A"* for a Drop-A song on an F♯ 8-string, or *"the reference pitch"* for an A440-vs-A432 mismatch. It's purely **advisory** (it never auto-opens the panel — tap the badge to tune), recomputed on `song:ready` and cleared when a new song loads or you leave the player. The retune diff comes from the tuner plugin's coverage report (`window._tunerAutoOpen.coverageReport` → `{ covered, retune: [{ from, to }], reference, cantCover }`); the cue is CSS-free (an inline ring + native tooltip — no Tailwind rebuild) and no-ops when the tuner plugin isn't installed. **v3-only.** Touches `static/v3/badges.js` (the cue) + `plugins/tuner/screen.js` (the report). Tests: `tests/js/tuner_auto_open.test.js` (the report names the strings; reference mismatch; the badge wiring). _(The splitscreen-suppress and no-usable-input guards move to the playback-gate stage, where they matter for its no-trap rule.)_ diff --git a/lib/gp2rs_gpx.py b/lib/gp2rs_gpx.py index 36adbc2..2b4dba6 100644 --- a/lib/gp2rs_gpx.py +++ b/lib/gp2rs_gpx.py @@ -229,12 +229,29 @@ def _build_tempo_map(root: ET.Element) -> list[tuple[int, float]]: return events +def _parse_tuning(el: ET.Element) -> list[int]: + """Return the string-tuning MIDI pitches from the first ``Tuning`` Property + at or below ``el`` (a Track or a single Staff), high string first. ``[]`` if + there is no Tuning property or its Pitches text is unparseable.""" + for prop in el.findall('.//Property'): + if prop.get('name') == 'Tuning': + pe = prop.find('Pitches') + if pe is not None and pe.text: + try: + return [int(p) for p in pe.text.split()] + except ValueError: + return [] + break + return [] + + def _gpif_tracks(root: ET.Element) -> list[dict]: """Return a list of raw track dicts from the GPIF Tracks element.""" # Lookups for per-track note counting. MasterBar/Bars lists one bar id per - # track in raw Tracks order, so the enumerate index below (which counts - # skipped pseudo-tracks) is the correct bar-lookup index — same mapping - # convert_file uses via filtered_to_raw. + # *stave* (not per Track element) in document order. A multi-stave track + # (e.g. GP8 piano with treble + bass) occupies N consecutive columns; the + # bar_column counter below advances by num_staves per track so every track + # gets the correct column regardless of neighbour stave counts. _masterbars = list(root.find('MasterBars') or []) _bars_by_id = {b.get('id'): b for b in (root.find('Bars') or [])} _voices_by_id = {v.get('id'): v for v in (root.find('Voices') or [])} @@ -279,10 +296,17 @@ def _gpif_tracks(root: ET.Element) -> list[dict]: return n result = [] - for raw_idx, t in enumerate(root.find('Tracks') or []): + bar_column = 0 + for t in (root.find('Tracks') or []): + # Count staves: each Staff occupies one column in MasterBar/Bars. + # Default to 1 for tracks with no explicit (GP3/4/5, old GPX). + num_staves = max(1, len(list(t.findall('Staves/Staff')))) + stave_columns = list(range(bar_column, bar_column + num_staves)) + name = (t.findtext('Name') or '').strip() if name.startswith('@$') and name.endswith('$@'): - continue # GP internal pseudo-tracks (raw_idx still advances) + bar_column += num_staves + continue # GP internal pseudo-tracks (bar_column still advances) gm = t.find('GeneralMidi') midi_program = 0 @@ -319,27 +343,37 @@ def _gpif_tracks(root: ET.Element) -> list[dict]: except (ValueError, TypeError): pass - # String tuning - string_pitches: list[int] = [] - for prop in t.findall('.//Property'): - if prop.get('name') == 'Tuning': - pe = prop.find('Pitches') - if pe is not None and pe.text: - try: - string_pitches = [int(p) for p in pe.text.split()] - except ValueError: - pass + # String tuning — one list per stave, in stave order. Reading all + # `.//Property` descendants across every stave meant the last stave's + # tuning overwrote the first; for a GP8 piano (treble 6-string + + # bass 5-string) that caused stave-0 notes with String=5 to be + # out-of-range against the 5-entry bass tuning and silently dropped. + # A staff with no Tuning of its own falls back to the track-level + # property (never to []) — an empty list silently drops every fretted + # note on that stave in `_note_midi`. The list stays parallel to + # `stave_columns` so a per-stave column always has a matching tuning. + _track_tuning = _parse_tuning(t) + _staff_els = list(t.findall('Staves/Staff')) + if _staff_els: + stave_pitches = [(_parse_tuning(s) or _track_tuning) for s in _staff_els] + else: + # No (GP3/4/5 or old GPX): single track-level tuning. + stave_pitches = [_track_tuning] result.append({ '_el': t, 'id': t.get('id', ''), 'name': name, - 'string_pitches': string_pitches, + 'string_pitches': stave_pitches[0], # primary stave (existing key) + 'num_staves': num_staves, + 'stave_columns': stave_columns, + 'stave_pitches': stave_pitches, 'is_drums': is_drums, 'midi_program': midi_program, 'midi_channel': midi_channel, - 'note_count': _note_count_for_raw(raw_idx), + 'note_count': sum(_note_count_for_raw(c) for c in stave_columns), }) + bar_column += num_staves return result @@ -367,6 +401,121 @@ def _beat_dur_secs(beat_el: ET.Element, rhythms_dict: dict, tempo_bpm: float) -> return dur_qn * (60.0 / tempo_bpm) +def _collect_column_notes( + col: int, + string_pitches: list[int], + *, + masterbars: list, + bars_by_id: dict, + voices_dict: dict, + beats_dict: dict, + notes_dict: dict, + rhythms_dict: dict, + tempo_map: list, + tempo_bpm: float, + audio_offset: float, +) -> list['RsNote']: + """Walk one ``MasterBar/Bars`` column (a single stave / hand) and return its + notes as keys-encoded ``RsNote`` (``string = midi // 24``, ``fret = midi % + 24``). Tie destinations extend the matching prior note's sustain (keyed by + pitch, so polyphonic parts are handled) rather than emitting a new note — + mirroring the main ``convert_file`` builder, including its full-precision + timing and the 0.2s sustain threshold. + + Shared by the GPX LH/RH pair merge and the GP8 multi-stave (grand-staff) + fold so the two code paths can never drift in tie / timing / dedup handling. + """ + from gp2rs import RsNote # lazy: gp2rs<->gpx circular import (see convert_file) + + notes: list[RsNote] = [] + last_per_key: dict[int, RsNote] = {} + tempo_iter = iter(tempo_map) + next_bar, next_bpm = next(tempo_iter, (999999, tempo_bpm)) + cur_tempo = tempo_bpm + t_cursor = 0.0 + + for mb_idx, mb in enumerate(masterbars): + while mb_idx >= next_bar: + cur_tempo = next_bpm + next_bar, next_bpm = next(tempo_iter, (999999, cur_tempo)) + ts = mb.findtext('Time', '4/4') + try: + nb, db = [int(x) for x in ts.split('/')] + except ValueError: + nb, db = 4, 4 + bar_dur = nb * (4.0 / db) * (60.0 / cur_tempo) + bar_ids = mb.findtext('Bars', '').split() + bid = bar_ids[col] if col < len(bar_ids) else '-1' + if bid != '-1' and bid: + bar = bars_by_id.get(bid) + if bar is not None: + for vid in bar.findtext('Voices', '').split(): + if vid == '-1': + continue + voice = voices_dict.get(vid) + if voice is None: + continue + vt = t_cursor + for beat_id in voice.findtext('Beats', '').split(): + beat = beats_dict.get(beat_id) + if beat is None: + continue + dur = _beat_dur_secs(beat, rhythms_dict, cur_tempo) + for nid in beat.findtext('Notes', '').strip().split(): + note_el = notes_dict.get(nid) + if note_el is None: + continue + if _note_is_tie(note_el): + tie_midi = _note_midi(note_el, string_pitches) + if tie_midi is not None: + prev = last_per_key.get(tie_midi) + tie_t = vt + audio_offset + if prev is not None and prev.time < tie_t: + prev.sustain = max( + prev.sustain, (tie_t + dur) - prev.time) + continue + midi = _note_midi(note_el, string_pitches) + if midi is None: + continue + rn = RsNote( + time=vt + audio_offset, + string=midi // 24, + fret=midi % 24, + sustain=dur if dur > 0.2 else 0.0, + ) + notes.append(rn) + last_per_key[midi] = rn + vt += dur + t_cursor += bar_dur + return notes + + +def _merge_lh_notes(rs_notes: list, rs_chords: list, lh_notes: list) -> None: + """Fold ``lh_notes`` (a second stave / left hand) into ``rs_notes`` in + place, de-duplicating simultaneous same-pitch notes and keeping the LONGER + sustain when both hands strike the same key at the same instant. Seeds the + dedup set from chord notes too (polyphonic RH beats live in + ``rs_chords[*].notes``). No-op for an empty ``lh_notes``.""" + if not lh_notes: + return + seen: dict[tuple, RsNote] = {} + for n in rs_notes: + seen.setdefault((round(n.time, 3), n.string, n.fret), n) + for c in rs_chords: + for cn in c.notes: + seen.setdefault((round(cn.time, 3), cn.string, cn.fret), cn) + for rn in lh_notes: + k = (round(rn.time, 3), rn.string, rn.fret) + existing = seen.get(k) + if existing is None: + rs_notes.append(rn) + seen[k] = rn + elif rn.sustain > existing.sustain: + # Mutating the RsNote also updates it in place inside any RH chord. + existing.sustain = rn.sustain + rs_notes.sort(key=lambda n: (n.time, n.string)) + + # --------------------------------------------------------------------------- # Drum encoding tables — ported from alphaTab PercussionMapper (MIT licensed) # --------------------------------------------------------------------------- @@ -1366,17 +1515,16 @@ def convert_file( rhythms_dict = {r.get('id'): r for r in (root.find('Rhythms') or [])} _bend_divisor = _gpx_bend_scale(root) # GPIF bend value -> semitones - # Map filtered track index -> raw track index (needed for bar lookup) - raw_tracks = list(root.find('Tracks') or []) - filtered_to_raw: dict[int, int] = {} - filtered_pos = 0 - for raw_idx, t_el in enumerate(raw_tracks): - name = (t_el.findtext('Name') or '').strip() - if name.startswith('@$') and name.endswith('$@'): - continue - filtered_to_raw[filtered_pos] = raw_idx - filtered_pos += 1 - + # Map filtered track index -> bar column (MasterBar/Bars position for + # stave 0 of that track). `_gpif_tracks` already computed the per-stave + # column layout (advancing by num_staves per track, pseudo-tracks skipped), + # so reuse its `stave_columns[0]` rather than re-deriving the counting rule + # here — divergence in stave counting *is* the bug class this fix closes. + # NB: despite the historical name, the value is a bar *column*, not a raw + # Track index — do not index `root.find('Tracks')` with it. + filtered_to_raw: dict[int, int] = { + i: t['stave_columns'][0] for i, t in enumerate(tracks) + } # Detect and merge Piano LH+RH pairs into single full-keyboard arrangements track_indices, _piano_merge_map = _find_piano_pairs(track_indices, tracks, names) @@ -1468,7 +1616,16 @@ def convert_file( is_keys = ( not is_drum and not is_vocal and ( - any(kw in track['name'].lower() for kw in ('piano', 'keys', 'keyboard', 'organ')) + # A multi-stave track is a grand staff (treble + bass) — i.e. a + # keyboard-family part. Treating it as keys end-to-end keeps the + # stave-0 encoding and the folded stave-1+ encoding consistent + # (both midi//24, midi%24) and makes the `note_count` preview + # (which sums every stave column) match what actually imports, + # even for instruments the name/program heuristics miss (harp, + # celesta, marimba). GPIF writes guitars as a single Staff, so + # this does not sweep in ordinary fretted tracks. + track.get('num_staves', 1) > 1 + or any(kw in track['name'].lower() for kw in ('piano', 'keys', 'keyboard', 'organ')) or arr_name.lower().startswith('keys') or ( not track['string_pitches'] @@ -1534,7 +1691,6 @@ def convert_file( pending_slides: list = [] # (RsNote, rs_string, gp_slide_flags) — resolved post-loop current_time = 0.0 - num_raw_tracks = len(raw_tracks) # Resolve current tempo per bar from the tempo map _tempo_iter = iter(tempo_map) @@ -1866,112 +2022,20 @@ def convert_file( tuning = _gpx_tuning(track) # Merge Piano LH notes into this (RH) arrangement if a pair was detected + _walk_kwargs = dict( + masterbars=masterbars, bars_by_id=bars_by_id, + voices_dict=voices_dict, beats_dict=beats_dict, + notes_dict=notes_dict, rhythms_dict=rhythms_dict, + tempo_map=tempo_map, tempo_bpm=tempo_bpm, audio_offset=audio_offset, + ) if is_keys and track_idx in _piano_merge_map: + # GPX LH/RH pair: the left hand is a *separate* Track element. Walk + # its column and fold it into this (right-hand) arrangement. lh_idx = _piano_merge_map[track_idx] lh_track = tracks[lh_idx] lh_raw_idx = filtered_to_raw.get(lh_idx, lh_idx) - - _lh_notes: list[RsNote] = [] - _lh_last_per_key: dict[int, RsNote] = {} - _lh_tempo_iter = iter(tempo_map) - _lh_next_bar, _lh_next_bpm = next(_lh_tempo_iter, (999999, tempo_bpm)) - _lh_cur_tempo = tempo_bpm - _lh_time = 0.0 - - for _lh_mb_idx, _lh_mb in enumerate(masterbars): - while _lh_mb_idx >= _lh_next_bar: - _lh_cur_tempo = _lh_next_bpm - _lh_next_bar, _lh_next_bpm = next(_lh_tempo_iter, (999999, _lh_cur_tempo)) - _lh_ts = _lh_mb.findtext('Time', '4/4') - try: - _lh_nb, _lh_db = [int(x) for x in _lh_ts.split('/')] - except ValueError: - _lh_nb, _lh_db = 4, 4 - _lh_bar_dur = _lh_nb * (4.0 / _lh_db) * (60.0 / _lh_cur_tempo) - _lh_bar_ids = _lh_mb.findtext('Bars', '').split() - _lh_bid = _lh_bar_ids[lh_raw_idx] if lh_raw_idx < len(_lh_bar_ids) else '-1' - if _lh_bid != '-1' and _lh_bid: - _lh_bar = bars_by_id.get(_lh_bid) - if _lh_bar is not None: - for _lh_vid in _lh_bar.findtext('Voices', '').split(): - if _lh_vid == '-1': - continue - _lh_voice = voices_dict.get(_lh_vid) - if _lh_voice is None: - continue - _lh_vt = _lh_time - for _lh_beat_id in _lh_voice.findtext('Beats', '').split(): - _lh_beat = beats_dict.get(_lh_beat_id) - if _lh_beat is None: - continue - _lh_dur = _beat_dur_secs(_lh_beat, rhythms_dict, _lh_cur_tempo) - for _lh_nid in _lh_beat.findtext('Notes', '').strip().split(): - _lh_note_el = notes_dict.get(_lh_nid) - if _lh_note_el is None: - continue - if _note_is_tie(_lh_note_el): - # Extend the matching prior note (same - # pitch), mirroring the main builder's - # last_note_per_key handling — blindly - # extending the last-emitted note - # mishandles polyphonic (chord) LH parts. - _tie_midi = _note_midi(_lh_note_el, lh_track['string_pitches']) - if _tie_midi is not None: - _prev = _lh_last_per_key.get(_tie_midi) - # Full-precision comparison (matching - # the main builder); rounding only - # happens at XML serialization. Rounding - # here could make a short note appear to - # start at the tie time and skip the - # sustain extension. - _tie_t = _lh_vt + audio_offset - if _prev is not None and _prev.time < _tie_t: - _prev.sustain = max( - _prev.sustain, - (_tie_t + _lh_dur) - _prev.time, - ) - continue - _lh_midi = _note_midi(_lh_note_el, lh_track['string_pitches']) - if _lh_midi is None: - continue - # Keep full-precision time (like the main - # convert_file() builder — rounding happens at - # serialization); same 0.2s sustain threshold. - _lh_rn = RsNote( - time=_lh_vt + audio_offset, - string=_lh_midi // 24, - fret=_lh_midi % 24, - sustain=_lh_dur if _lh_dur > 0.2 else 0.0, - ) - _lh_notes.append(_lh_rn) - _lh_last_per_key[_lh_midi] = _lh_rn - _lh_vt += _lh_dur - _lh_time += _lh_bar_dur - - # Merge: combine and deduplicate simultaneous same-pitch notes, then - # sort by time. Map each (time, string, fret) to its existing RsNote - # so that when both hands hit the same key at the same instant we - # keep the LONGER sustain instead of arbitrarily discarding the LH - # one. Seed from both single notes and chord notes — polyphonic RH - # beats live in rs_chords[*].notes, so seeding from rs_notes alone - # would let an identical LH note slip in as a duplicate. - _seen: dict[tuple, RsNote] = {} - for _n in rs_notes: - _seen.setdefault((round(_n.time, 3), _n.string, _n.fret), _n) - for _c in rs_chords: - for _cn in _c.notes: - _seen.setdefault((round(_cn.time, 3), _cn.string, _cn.fret), _cn) - for _lh_rn in _lh_notes: - _k = (round(_lh_rn.time, 3), _lh_rn.string, _lh_rn.fret) - _existing = _seen.get(_k) - if _existing is None: - rs_notes.append(_lh_rn) - _seen[_k] = _lh_rn - elif _lh_rn.sustain > _existing.sustain: - # Same key both hands — preserve the longer sustain (mutating - # the RsNote also updates it in place inside any RH chord). - _existing.sustain = _lh_rn.sustain - rs_notes.sort(key=lambda n: (n.time, n.string)) + _merge_lh_notes(rs_notes, rs_chords, _collect_column_notes( + lh_raw_idx, lh_track['string_pitches'], **_walk_kwargs)) # Collapse "Keys 2" -> "Keys": the merged LH+RH is a single # keyboard arrangement. Keep the standard "Keys" name (not "Piano") @@ -1979,6 +2043,18 @@ def convert_file( # auto-select (which keys on arr_name.startswith("keys")) still work. arr_name = re.sub(r'\s*\d+$', '', arr_name).strip() or 'Keys' + elif track.get('num_staves', 1) > 1: + # GP8 grand-staff keyboard: staves 1+ (bass clef, and any further + # staves) are extra MasterBar/Bars columns for the SAME Track + # element. Fold each one in, exactly like the GPX LH merge above. + # (num_staves > 1 implies is_keys, set above.) Iterating every + # extra column — not just stave_columns[1] — keeps the arrangement + # consistent with note_count, which sums all columns. + for _col, _sp in zip(track['stave_columns'][1:], + track['stave_pitches'][1:]): + _merge_lh_notes(rs_notes, rs_chords, _collect_column_notes( + _col, _sp, **_walk_kwargs)) + # Resolve pending slides now that every note on each string is known. # GPIF slide flags: 1=shift, 2=legato (both slide to the NEXT note on the # string); 4=slide out downwards, 8=slide out upwards (unpitched). @@ -2032,19 +2108,24 @@ def convert_file( try: import gp2notation as _gp2notation _lh_idx = _piano_merge_map.get(track_idx) + if _lh_idx is not None: + # GPX LH/RH pair (two separate Track elements) + _nt_lh_raw = filtered_to_raw.get(_lh_idx, _lh_idx) + _nt_lh_sp = tracks[_lh_idx]['string_pitches'] + elif track.get('num_staves', 1) > 1: + # GP8 two-stave piano (one Track with multiple ) + _nt_lh_raw = track['stave_columns'][1] + _nt_lh_sp = (track['stave_pitches'][1] + if len(track.get('stave_pitches', [])) > 1 else []) + else: + _nt_lh_raw, _nt_lh_sp = None, [] _payload = _gp2notation.convert_track_to_notation( root, raw_idx, track['string_pitches'], instrument='piano', audio_offset=audio_offset, track_name=track['name'], - lh_raw_idx=( - filtered_to_raw.get(_lh_idx, _lh_idx) - if _lh_idx is not None else None - ), - lh_string_pitches=( - tracks[_lh_idx]['string_pitches'] - if _lh_idx is not None else None - ), + lh_raw_idx=_nt_lh_raw, + lh_string_pitches=_nt_lh_sp or None, ) _gp2notation.write_notation_sidecar(filepath, _payload) except Exception: diff --git a/lib/notation_lift.py b/lib/notation_lift.py index 7fc0ba8..7a40fb7 100644 --- a/lib/notation_lift.py +++ b/lib/notation_lift.py @@ -114,11 +114,27 @@ def split_hands(notes: list[dict]) -> dict[str, list[dict]]: pitches = sorted(n["midi"] for n in group) span = pitches[-1] - pitches[0] if len(pitches) > 1 and span > HAND_SPLIT_SPAN_SEMITONES: - # Largest internal gap; ties resolve to the lowest such gap so the - # left hand keeps the tight low cluster. - gaps = [pitches[i + 1] - pitches[i] for i in range(len(pitches) - 1)] - split_after = gaps.index(max(gaps)) - threshold = pitches[split_after] # lh: midi <= threshold + # Prefer middle C as the split boundary when notes straddle it — + # this correctly handles bass+treble chords from piano imports where + # the largest-gap heuristic picks the wrong split point (e.g. + # [G2, E3, C4]: largest gap is G2→E3 but the real split is E3|C4). + # BUT only when both resulting hands are themselves playable: a bass + # note under a treble voicing that merely dips below C4 (e.g. + # [E2, B3, D4, G4]) would otherwise land E2+B3 in one hand — a + # 19-semitone span that re-violates HAND_SPLIT_SPAN_SEMITONES. When + # the middle-C split produces an unplayable hand, fall back to the + # largest internal gap (which correctly isolates E2 there). + threshold = None + if pitches[0] < MIDDLE_C <= pitches[-1]: + _lh = [p for p in pitches if p < MIDDLE_C] + _rh = [p for p in pitches if p >= MIDDLE_C] + if (_lh[-1] - _lh[0] <= HAND_SPLIT_SPAN_SEMITONES + and _rh[-1] - _rh[0] <= HAND_SPLIT_SPAN_SEMITONES): + threshold = MIDDLE_C - 1 # lh: midi < MIDDLE_C + if threshold is None: + gaps = [pitches[i + 1] - pitches[i] for i in range(len(pitches) - 1)] + split_after = gaps.index(max(gaps)) + threshold = pitches[split_after] for n in group: hands["lh" if n["midi"] <= threshold else "rh"].append(n) else: diff --git a/tests/test_gp2notation.py b/tests/test_gp2notation.py index d00bde1..972038a 100644 --- a/tests/test_gp2notation.py +++ b/tests/test_gp2notation.py @@ -454,6 +454,86 @@ def test_convert_file_writes_notation_sidecar_for_keys_only(tmp_path, monkeypatc assert beats[0]["notes"] == [{"midi": 60}] # 48 + fret 12 +# A GP8 grand-staff piano (one Track, two entries → two MasterBar/Bars +# columns) followed by a guitar track. The treble stave carries a String=5 note +# that only decodes against the 6-entry treble tuning — under the old +# last-stave-tuning-wins bug it indexed out of range against the 5-entry bass +# tuning and was silently dropped. The guitar's single note sits at column 2, +# so it also exercises the bar-column offset after a multi-stave predecessor. +_GPIF_GRAND_STAFF_PIANO = """ + + TA + + Piano + + + 76 71 67 62 57 52 + + 50 45 40 35 30 + + Lead Guitar + 64 59 55 50 45 40 + + + 0 1 2 + + + 0 + 1 + 2 + + + b0 + b1 + b2 + + + n0 + n1 + n2 + + + + 5 + 0 + + 0 + 0 + + 0 + 3 + + Quarter + +""" + + +def test_convert_file_folds_grand_staff_and_offsets_bar_column(tmp_path, monkeypatch): + monkeypatch.setattr(gp2rs_gpx, "_load_gpif", + lambda _p: ET.fromstring(_GPIF_GRAND_STAFF_PIANO)) + out_files = gp2rs_gpx.convert_file( + "dummy.gpx", str(tmp_path), track_indices=[0, 1], + arrangement_names={0: "Keys", 1: "Lead"}, + ) + keys_xml = next(p for p in out_files if "Keys" in p) + guitar_xml = next(p for p in out_files if "Lead" in p) + + def _notes(xml_path): + root = ET.parse(xml_path).getroot() + return [(int(n.get("string")), int(n.get("fret"))) for n in root.iter("note")] + + # Both staves are folded into the one keys arrangement (keys encoding = + # midi//24, midi%24): treble E3 (52, the String=5 note the old bug dropped) + # AND bass D3 (50, the stave-1 column). + keys_midi = sorted(s * 24 + f for s, f in _notes(keys_xml)) + assert keys_midi == [50, 52] + # The guitar note lives at bar column 2 (after the 2-column piano). Its own + # bar carries fret 3; the bass column it would wrongly read has fret 0, so a + # single fret-3 note proves the multi-stave column offset landed correctly. + guitar = _notes(guitar_xml) + assert len(guitar) == 1 and guitar[0][1] == 3 + + def test_convert_file_sidecar_failure_does_not_break_conversion(tmp_path, monkeypatch): monkeypatch.setattr(gp2rs_gpx, "_load_gpif", lambda _p: ET.fromstring(_GPIF_KEYS_AND_GUITAR)) diff --git a/tests/test_notation_lift.py b/tests/test_notation_lift.py index 9843e92..7a8c8f8 100644 --- a/tests/test_notation_lift.py +++ b/tests/test_notation_lift.py @@ -85,6 +85,26 @@ def test_split_hands_narrow_group_goes_by_mean_vs_middle_c(): assert "rh" in high and "lh" not in high # mean ~65.5 ≥ 60 +def test_split_hands_straddling_middle_c_splits_at_middle_c(): + # [G2, E3, C4] = [43, 52, 60]: largest gap is G2→E3 (9) but the musically + # correct split is E3|C4. Middle-C boundary → lh=[G2,E3], rh=[C4]. + notes = [{"t": 0.0, "midi": m, "sus": 0} for m in (43, 52, 60)] + hands = nl.split_hands(notes) + assert sorted(n["midi"] for n in hands["lh"]) == [43, 52] + assert [n["midi"] for n in hands["rh"]] == [60] + + +def test_split_hands_middle_c_split_falls_back_when_it_makes_unplayable_hand(): + # Em7-shape RH voicing over a low bass note: [E2, B3, D4, G4] = [40,59,62,67]. + # A hard middle-C split would put E2+B3 in the LH — a 19-semitone span that + # re-violates the 12-semitone threshold. Must fall back to the largest gap, + # isolating E2 in the LH and keeping the treble voicing in the RH. + notes = [{"t": 0.0, "midi": m, "sus": 0} for m in (40, 59, 62, 67)] + hands = nl.split_hands(notes) + assert [n["midi"] for n in hands["lh"]] == [40] + assert sorted(n["midi"] for n in hands["rh"]) == [59, 62, 67] + + # ── Timing ─────────────────────────────────────────────────────────────────── def test_downbeat_times_filters_non_downbeats_and_sorts():