From c8e0ad3f75f06c47419d751265abb1167bd3359b Mon Sep 17 00:00:00 2001 From: Byron Gamatos Date: Fri, 26 Jun 2026 18:58:35 +0200 Subject: [PATCH] fix(gp-import): correct bass string count, lead/rhythm roles, preview note count (#601) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four tester-reported GP-import issues, all in the converter/parse layer: * String count (bugs 2 & 4): is padded to 6 slots, so a 4-string bass, 5-string bass and 6-string guitar were byte-identical and the real count was lost — a 5-string bass played on 4 strings and a 4-string bass showed a phantom B in the editor. Record the authoritative count in a new attribute (gp2rs._build_xml) and trim the padded tail back to it on read (song.parse_arrangement). All consumers already trust a non-6 tuning length (arrangement_string_count, the editor's _stringCountFor and build-time _normalize_tuning_to_count), so this fixes the create-mode preview AND the built sloppak with no consumer changes. * Lead/Rhythm reversed (bug 3): guitar arrangements were named by appearance order (first guitar -> Lead), swapping roles for files that list Rhythm before Lead. Honor 'lead'/'rhythm' in the GP track name; unhinted tracks keep positional fallback. Applied to both convert_file's fallback (the editor's track_indices-without-names path) and _auto_select_gpx, with cross-role dedup so name-based and positional labels can't collide. * Preview note count (bug 1): the importer's per-track count included tie-continuation notes, which are folded into the previous note's sustain and never become separate RS notes (260 shown vs 241 imported). Exclude tie destinations so the preview matches the imported result. Adds regression tests for all three. Bug 5 (no stems from synced audio) is environment-dependent (best-effort demucs backend) and not addressed here. Co-authored-by: Claude Opus 4.8 (1M context) --- lib/gp2rs.py | 10 ++ lib/gp2rs_gpx.py | 163 ++++++++++++++++++++++------ lib/song.py | 14 +++ tests/test_gp2rs_gpx.py | 234 ++++++++++++++++++++++++++++++++++++++++ 4 files changed, 391 insertions(+), 30 deletions(-) diff --git a/lib/gp2rs.py b/lib/gp2rs.py index 3bc653b..da0233e 100644 --- a/lib/gp2rs.py +++ b/lib/gp2rs.py @@ -1124,7 +1124,17 @@ def _build_xml( # for compatibility, and emit additional string6+ attributes (up to # `len(tuning)-1`) for 7+ string arrangements. FeedBack parses # them; the format ignores them. + # + # `stringCount` records the AUTHORITATIVE string count (== len(tuning)), + # because the 6-slot padding above erases the 4-vs-5-vs-6-string + # distinction for standard tunings (a 4-string bass, 5-string bass and + # 6-string guitar are otherwise byte-identical, all string0..5 = 0). + # parse_arrangement trims `tuning` back to this on read so downstream + # string-count derivation (song.arrangement_string_count, the editor's + # _stringCountFor) sees the real width instead of guessing. RS2014 and + # any other consumer simply ignore the unknown attribute. tuning_el = ET.SubElement(root, "tuning") + tuning_el.set("stringCount", str(len(tuning))) for i in range(max(6, len(tuning))): tuning_el.set(f"string{i}", str(tuning[i] if i < len(tuning) else 0)) ET.SubElement(root, "capo").text = "0" diff --git a/lib/gp2rs_gpx.py b/lib/gp2rs_gpx.py index 3bd8fd1..36adbc2 100644 --- a/lib/gp2rs_gpx.py +++ b/lib/gp2rs_gpx.py @@ -239,12 +239,20 @@ def _gpif_tracks(root: ET.Element) -> list[dict]: _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 [])} _beats_by_id = {b.get('id'): b for b in (root.find('Beats') or [])} + _notes_by_id = {n.get('id'): n for n in (root.find('Notes') or [])} def _note_count_for_raw(raw_idx: int) -> int: - # Total note count for the track (sum of notes across all its beats). - # This is the single source of truth: list_tracks surfaces it as the - # 'notes' field, and _auto_select_gpx uses (count == 0) to skip empty - # tracks — so the graph is walked once here, not again in list_tracks. + # Count of notes that ACTUALLY become RS notes for the track. This is + # the single source of truth: list_tracks surfaces it as the 'notes' + # field (the importer's per-track preview count) and _auto_select_gpx + # uses (count == 0) to skip empty tracks — so the graph is walked once + # here, not again in list_tracks. + # + # Tie-DESTINATION notes are excluded: a tied note is folded into the + # previous note as extended sustain (see the `_note_is_tie` skips in + # convert_file), so it never becomes a separate RS note. Counting them + # made the preview overstate the result (e.g. 260 shown, 241 imported); + # excluding them makes the preview match what the user actually gets. n = 0 for mb in _masterbars: bar_ids = mb.findtext('Bars', '').split() @@ -263,9 +271,11 @@ def _gpif_tracks(root: ET.Element) -> list[dict]: beat = _beats_by_id.get(bid) if beat is None: continue - notes_text = beat.findtext('Notes', '').strip() - if notes_text: - n += len(notes_text.split()) + for nid in beat.findtext('Notes', '').split(): + note_el = _notes_by_id.get(nid) + if note_el is not None and _note_is_tie(note_el): + continue + n += 1 return n result = [] @@ -1372,9 +1382,69 @@ def convert_file( track_indices, _piano_merge_map = _find_piano_pairs(track_indices, tracks, names) output_files = [] - # Counts of auto-named guitar/bass arrangements so far, so multiple guitars - # get distinct RS roles (Lead, Rhythm, Combo, …) instead of all "Lead". - _role_counts: dict[str, int] = {} + # All auto-assigned arrangement names handed out so far, so multiple + # arrangements get distinct labels (Lead, Rhythm, Combo, Bass, Bass 2, …) + # and the name-aware and positional guitar paths never collide. + _used_arr_names: set[str] = set() + + def _unique_arr_name(base: str) -> str: + """Return `base`, or `base 2`/`base 3`/… if it's already been used.""" + if base not in _used_arr_names: + _used_arr_names.add(base) + return base + k = 2 + while f"{base} {k}" in _used_arr_names: + k += 1 + name = f"{base} {k}" + _used_arr_names.add(name) + return name + + _KEYS_PROGS = set(range(0, 8)) | set(range(16, 24)) | {80, 81, 82, 83} + + def _auto_guitar_hint(track_idx: int): + """For a track that auto-resolves to a guitar arrangement, return its + role hint: 'lead', 'rhythm', or '' (unhinted). None when the track is + NOT an auto-named guitar (explicitly named, bass, drum, vocal, keys). + Mirrors the per-track classification in the conversion loop below.""" + if track_idx >= len(tracks) or names.get(track_idx): + return None + t = tracks[track_idx] + if t['is_drums'] or _is_vocal_track(t): + return None + low = t['name'].lower() + sp = t['string_pitches'] + prog = t['midi_program'] + if (isinstance(prog, int) and 32 <= prog <= 39) or (bool(sp) and max(sp) <= 48) or 'bass' in low: + return None # bass + if (not sp and prog in _KEYS_PROGS) or any(kw in low for kw in ('piano', 'keys', 'keyboard', 'organ')): + return None # keys + if 'lead' in low and 'rhythm' not in low: + return 'lead' + if 'rhythm' in low and 'lead' not in low: + return 'rhythm' + return '' # guitar, no role hint + + # Two-pass guitar role naming, resolved up front so the per-track loop just + # looks names up. Reserve every name-hinted Lead/Rhythm first, THEN fill + # unhinted guitars into the remaining canonical roles. A single pass would + # let an unhinted guitar that appears BEFORE a hinted one steal its role, + # pushing the real Lead/Rhythm to a non-canonical "Rhythm 2" that the + # downstream name-based path classification doesn't recognise. + _guitar_name_by_idx: dict[int, str] = {} + _unhinted_guitars: list[int] = [] + for _ti in track_indices: + hint = _auto_guitar_hint(_ti) + if hint is None: + continue + if hint == 'lead': + _guitar_name_by_idx[_ti] = _unique_arr_name('Lead') + elif hint == 'rhythm': + _guitar_name_by_idx[_ti] = _unique_arr_name('Rhythm') + else: + _unhinted_guitars.append(_ti) + for _ti in _unhinted_guitars: + base = next((r for r in ('Lead', 'Rhythm', 'Combo') if r not in _used_arr_names), 'Combo') + _guitar_name_by_idx[_ti] = _unique_arr_name(base) for track_idx in track_indices: if track_idx >= len(tracks): @@ -1422,16 +1492,12 @@ def convert_file( ) low = track['name'].lower() if is_bass or 'bass' in low: - _bc = _role_counts.get('bass', 0) - _role_counts['bass'] = _bc + 1 - arr_name = 'Bass' if _bc == 0 else f'Bass {_bc + 1}' + arr_name = _unique_arr_name('Bass') else: - # Distinct guitar roles by appearance order so two guitars - # don't both become "Lead": Lead, Rhythm, Combo, then Combo N. - _gc = _role_counts.get('guitar', 0) - _role_counts['guitar'] = _gc + 1 - _roles = ('Lead', 'Rhythm', 'Combo') - arr_name = _roles[_gc] if _gc < len(_roles) else f'Combo {_gc - 1}' + # Guitar role was resolved up front (two-pass, honoring + # "lead"/"rhythm" in the GP track name so a Rhythm-before-Lead + # file isn't swapped by positional assignment). + arr_name = _guitar_name_by_idx.get(track_idx) or _unique_arr_name('Lead') # Vocal tracks get their own converter — outputs vocals XML, not notes XML if is_vocal: @@ -2317,7 +2383,15 @@ def _auto_select_gpx(tracks: list[dict]) -> tuple[list[int], dict[int, str]]: if is_bass: selected.append((i, 'bass')) elif is_guitar: - selected.append((i, 'guitar')) + # Honor "lead"/"rhythm" in the GP track name so two guitars keep + # the author's roles instead of being labelled by appearance order + # (which swaps a Rhythm-before-Lead file). Unhinted → positional. + if 'lead' in name_l and 'rhythm' not in name_l: + selected.append((i, 'guitar_lead')) + elif 'rhythm' in name_l and 'lead' not in name_l: + selected.append((i, 'guitar_rhythm')) + else: + selected.append((i, 'guitar')) elif is_keys: selected.append((i, 'keys')) @@ -2326,19 +2400,48 @@ def _auto_select_gpx(tracks: list[dict]) -> tuple[list[int], dict[int, str]]: if not t['is_drums'] and t.get('note_count', 1) > 0: selected.append((i, 'guitar')) - indices = [] name_map = {} counts: dict[str, int] = {} - RS_NAMES = {'guitar': ('Lead', 'Rhythm', 'Combo'), 'bass': ('Bass',), 'keys': ('Keys',), 'drums': ('Drums',), 'vocal': ('Vocals',)} + RS_NAMES = {'bass': ('Bass',), 'keys': ('Keys',), + 'drums': ('Drums',), 'vocal': ('Vocals',)} + used: set[str] = set() + + def _unique(base: str) -> str: + if base not in used: + used.add(base) + return base + k = 2 + while f"{base} {k}" in used: + k += 1 + used.add(f"{base} {k}") + return f"{base} {k}" + + # Two passes so name-hinted Lead/Rhythm guitars reserve their canonical role + # BEFORE unhinted guitars are filled in — otherwise an unhinted guitar that + # appears before a hinted one steals its role (real Rhythm → "Rhythm 2"). + # Non-guitar roles are handled in pass 1. `name_map` keys by track index so + # this does not affect arrangement (selection) order, computed separately. + for idx, role in selected: + if role == 'guitar': + continue + if role == 'guitar_lead': + base = 'Lead' + elif role == 'guitar_rhythm': + base = 'Rhythm' + else: + counts[role] = counts.get(role, 0) + 1 + c = counts[role] + names_for_role = RS_NAMES.get(role, (role.title(),)) + base = names_for_role[min(c - 1, len(names_for_role) - 1)] + if c > len(names_for_role): + base = f"{names_for_role[-1]} {c}" + name_map[idx] = _unique(base) for idx, role in selected: - counts[role] = counts.get(role, 0) + 1 - c = counts[role] - names_for_role = RS_NAMES.get(role, (role.title(),)) - arr_name = names_for_role[min(c - 1, len(names_for_role) - 1)] - if c > len(names_for_role): - arr_name = f"{names_for_role[-1]} {c}" - indices.append(idx) - name_map[idx] = arr_name + if role != 'guitar': + continue + base = next((r for r in ('Lead', 'Rhythm', 'Combo') if r not in used), 'Combo') + name_map[idx] = _unique(base) + indices = [idx for idx, _role in selected] return indices, name_map diff --git a/lib/song.py b/lib/song.py index 1602239..c993c71 100644 --- a/lib/song.py +++ b/lib/song.py @@ -1090,6 +1090,20 @@ def parse_arrangement(xml_path: str) -> Arrangement: while el.get(f"string{i}") is not None: tuning.append(_int(el, f"string{i}")) i += 1 + # Authoritative string count, written by the GP/RS serializer + # (gp2rs._build_xml). The schema pads `` to 6 slots, which + # erases the 4-vs-5-vs-6-string distinction for standard tunings; + # when the real count was recorded, trim the padded tail so + # arrangement_string_count / the editor see 4 or 5 instead of 6. + # Absent (archive / legacy sources) → leave the 6-slot tuning as-is. + sc = el.get("stringCount") + if sc is not None: + try: + n = int(sc) + except (TypeError, ValueError): + n = 0 + if 1 <= n <= len(tuning): + tuning = tuning[:n] # Capo capo = 0 diff --git a/tests/test_gp2rs_gpx.py b/tests/test_gp2rs_gpx.py index 9c0a4ea..d677caf 100644 --- a/tests/test_gp2rs_gpx.py +++ b/tests/test_gp2rs_gpx.py @@ -925,3 +925,237 @@ def test_convert_file_gp8_no_diagram_leaves_template_blank(tmp_path, monkeypatch assert [ct.get(f"finger{i}") for i in range(6)] == ["-1"] * 6 # Fret pattern itself is unchanged (the join key still works). assert ct.get("fret0") == "3" and ct.get("fret1") == "2" + + +# ── GP import correctness fixes (tester-reported) ─────────────────────────── + +# Two guitar tracks, Rhythm listed BEFORE Lead. The importer used to name +# arrangements purely by appearance order (first guitar -> "Lead"), which +# swapped the roles for any file that lists Rhythm first. +_GPIF_RHYTHM_BEFORE_LEAD = """ + + TA + + Rhythm Guitar + 40 45 50 55 59 64 + Lead Guitar + 40 45 50 55 59 64 + + 0 1 + + 0 + 1 + + + 0 + 1 + + + 0 + 1 + + + 00 + 00 + + Quarter + +""" + + +def test_convert_file_guitar_roles_follow_gp_name_not_order(tmp_path, monkeypatch): + # Editor path: track_indices but no arrangement_names → convert_file's + # fallback naming. The Rhythm track (listed first) must stay "Rhythm" and + # the Lead track "Lead" — not be swapped by appearance order. + monkeypatch.setattr(gp2rs_gpx, "_load_gpif", + lambda _p: ET.fromstring(_GPIF_RHYTHM_BEFORE_LEAD)) + out_files = convert_file("dummy.gp", str(tmp_path), track_indices=[0, 1]) + names = [ET.parse(f).getroot().findtext("arrangement") for f in out_files] + assert names == ["Rhythm", "Lead"] + + +def _gpif_bass(pitches: str) -> str: + return f""" + + TA + + Bass + {pitches} + + 0 + 0 + 0 + 0 + + 0 + 0 + + Quarter + +""" + + +@pytest.mark.parametrize("pitches,expected", [ + ("28 33 38 43", 4), # 4-string E-A-D-G + ("23 28 33 38 43", 5), # 5-string low-B +]) +def test_convert_file_bass_string_count_round_trips(tmp_path, monkeypatch, pitches, expected): + # The element pads to 6 slots (RS2014 schema), which erased the + # 4-vs-5-string distinction. The serializer now records the real count in + # `stringCount`; parse_arrangement trims to it so a 5-string bass reports 5 + # (was 4 → it rendered/played on 4 strings). + from song import parse_arrangement, arrangement_string_count + monkeypatch.setattr(gp2rs_gpx, "_load_gpif", + lambda _p: ET.fromstring(_gpif_bass(pitches))) + out_files = convert_file("dummy.gp", str(tmp_path), track_indices=[0]) + root = ET.parse(out_files[0]).getroot() + assert root.find("tuning").get("stringCount") == str(expected) + arr = parse_arrangement(out_files[0]) + assert len(arr.tuning) == expected + assert arrangement_string_count(arr) == expected + + +# A track with one normal note and one tie-DESTINATION note. The tie is folded +# into the previous note's sustain, so it is not a separate RS note; the +# importer's preview count must exclude it (matches the imported result). +_GPIF_WITH_TIE = """ + + TA + + Lead + 40 45 50 55 59 64 + + 0 + 0 + 0 1 + + 0 + 1 + + + 03 + 03 + + Quarter + +""" + + +def test_list_tracks_note_count_excludes_tie_continuations(monkeypatch): + monkeypatch.setattr(gp2rs_gpx, "_load_gpif", + lambda _p: ET.fromstring(_GPIF_WITH_TIE)) + tracks = gp2rs_gpx.list_tracks("dummy.gp") + # 2 raw notes, 1 of them a tie destination → 1 importable note. + assert tracks[0]["notes"] == 1 + + +# A hinted "Lead Guitar" followed by an UNHINTED guitar: the unhinted one must +# advance to the next canonical role ("Rhythm"), not collide into "Lead 2". +_GPIF_LEAD_THEN_UNHINTED = """ + + TA + + Lead Guitar + 40 45 50 55 59 64 + Guitar 2 + 40 45 50 55 59 64 + + 0 1 + + 0 + 1 + + + 0 + 1 + + + 0 + 1 + + + 00 + 00 + + Quarter + +""" + + +def test_convert_file_unhinted_guitar_takes_next_canonical_role(tmp_path, monkeypatch): + # Mix of hinted + unhinted guitars spreads across Lead → Rhythm, not Lead/Lead 2. + monkeypatch.setattr(gp2rs_gpx, "_load_gpif", + lambda _p: ET.fromstring(_GPIF_LEAD_THEN_UNHINTED)) + out_files = convert_file("dummy.gp", str(tmp_path), track_indices=[0, 1]) + names = [ET.parse(f).getroot().findtext("arrangement") for f in out_files] + assert names == ["Lead", "Rhythm"] + + +def test_auto_select_gpx_unhinted_guitar_takes_next_canonical_role(): + # _auto_select_gpx path (auto-select-all): same spread rule. + root = ET.fromstring(_GPIF_LEAD_THEN_UNHINTED) + tracks = gp2rs_gpx._gpif_tracks(root) + _indices, names = gp2rs_gpx._auto_select_gpx(tracks) + assert sorted(names.values()) == ["Lead", "Rhythm"] + + +# Codex scenario: unhinted guitar BEFORE a later hinted "Rhythm Guitar". The +# real rhythm track must still get the canonical "Rhythm" (two-pass reserves +# hinted roles first); the unhinted one takes the leftover canonical role. +_GPIF_UNHINTED_BEFORE_RHYTHM = """ + + TA + + Lead Guitar + 40 45 50 55 59 64 + Guitar 2 + 40 45 50 55 59 64 + Rhythm Guitar + 40 45 50 55 59 64 + + 0 1 2 + + 0 + 1 + 2 + + + 0 + 1 + 2 + + + 0 + 1 + 2 + + + 00 + 00 + 00 + + Quarter + +""" + + +def test_convert_file_unhinted_does_not_steal_later_rhythm(tmp_path, monkeypatch): + monkeypatch.setattr(gp2rs_gpx, "_load_gpif", + lambda _p: ET.fromstring(_GPIF_UNHINTED_BEFORE_RHYTHM)) + out_files = convert_file("dummy.gp", str(tmp_path), track_indices=[0, 1, 2]) + names = [ET.parse(f).getroot().findtext("arrangement") for f in out_files] + # Hinted roles reserved first → real Rhythm keeps canonical "Rhythm"; + # the unhinted middle track takes the leftover canonical role. + assert names[0] == "Lead" + assert names[2] == "Rhythm" + assert names[1] == "Combo" + assert "Rhythm 2" not in names and "Lead 2" not in names + + +def test_auto_select_gpx_unhinted_does_not_steal_later_rhythm(): + root = ET.fromstring(_GPIF_UNHINTED_BEFORE_RHYTHM) + tracks = gp2rs_gpx._gpif_tracks(root) + indices, names = gp2rs_gpx._auto_select_gpx(tracks) + assert names[indices[0]] == "Lead" + assert names[indices[2]] == "Rhythm" + assert names[indices[1]] == "Combo"