From 830a7085417cc2dafc9340b5101cd8660571166d Mon Sep 17 00:00:00 2001 From: Carolina <79524656+carochacs@users.noreply.github.com> Date: Fri, 31 Jul 2026 14:01:13 -0600 Subject: [PATCH 01/46] Preserve Guitar Pro strum direction (#2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(gp2rs): import strum direction from GP3-5 and GPX/GP6-8 files Note.pick_direction (song.py) has been a fully-wired wire-format field all along (pkd on the wire, pickDirection in RS-XML), but neither GP importer ever populated it — every imported note landed at -1 (unset) even when the source file authored an explicit up/down stroke. - gp2rs.py: read beat.effect.pickStroke (PyGuitarPro already exposes it) once per beat, applied to every note/chordNote built from that beat — a stroke is one pick gesture across the whole chord, not a per-note choice. Guitar/bass path only; the piano and drum note-building paths reuse the same RsNote/XML writer but never set it (no strum concept for keys/drums). - gp2rs_gpx.py: same idea from GPIF's for GP6/7/8, guarded off for is_drum/is_keys. - Dropped the chord-level strum="down" XML attribute — it was hardcoded unconditionally and never read by song.py's parser at all (confirmed via grep); real direction now lives per-note on pickDirection instead, so the dead attribute was just misleading output. 211/211 tests pass (test_gp2rs.py, test_gp2rs_gpx.py), including 9 new tests covering both formats, the chord-wide single-stroke-direction behavior, and the missing-attribute/unset fallback paths. Fixes got-feedBack/feedBack#1054. Co-Authored-By: Claude Sonnet 5 * fix(gp2rs): import the real per-track capo instead of hardcoding 0 capo was hardcoded to "0" in the shared RS-XML writer (_build_xml) regardless of what the source file actually declared. feedpakr_pipeline.py had already patched around it downstream (_capo_for_track/_gpif_capo_lookup, applied post-conversion) precisely because this was never fixed at the source — but the editor plugin's own GP-import route never applied the same patch, so it silently imported every Guitar Pro file at capo=0 (found while comparing what metadata feedpaker vs. the editor pull from the same GP import). Fixed at the actual source instead of adding a third downstream patch: - gp2rs.py: Track.offset (PyGuitarPro's own capo field for GP3-5). - gp2rs_gpx.py: GPIF's Track/Staves/Staff/Properties/ Property[@name='CapoFret']/Fret for GP6/7/8. Both guarded off for piano/drum tracks, which have no capo concept. Since both feedpakr and the editor call these same shared converters, this fixes the editor's capo import for free with no editor-repo change needed. feedpakr's own downstream patch becomes redundant but harmless (re-reads the now-correct value and reassigns it) — left in place rather than touching a third repo in the same pass. 5 new tests (2 gp2rs.py, 3 gp2rs_gpx.py); 379/379 passing across test_gp2rs.py, test_gp2rs_gpx.py, test_song.py. Co-Authored-By: Claude Sonnet 5 * standardize naming: rename DLC_PATH/DLC_DIR to LIBRARY_PATH/LIBRARY_DIR Standardize terminology across the project. The library is the folder where songs are stored, so LIBRARY_PATH (host env var) and LIBRARY_DIR (container/app env var) are clearer than DLC_PATH/DLC_DIR. Changes: - docker-compose.yml: LIBRARY_PATH env var, mount to /library, set LIBRARY_DIR=/library - docker-compose.nas.yml: mount to /library, set LIBRARY_DIR=/library - server.py: read LIBRARY_DIR env var instead of DLC_DIR - dlc_paths.py: update comments to reference LIBRARY_DIR - docs/skills/: update examples to use LIBRARY_PATH - tests/browser/: update examples to use LIBRARY_PATH Internal app variable names (dlc_dir, dlc_dir_env) kept unchanged to minimize refactor scope. Co-Authored-By: Claude Haiku 4.5 * gitignore: exclude /library folder (user song files) Both /library and /dlc are local folders for song collections and should never be committed to the repo. Co-Authored-By: Claude Haiku 4.5 * fix(gp2rs): read chord strum direction from beat.effect.stroke, not pickStroke GP4/5 binary actually encodes two independent fields on a beat (guitarpro/gp4.py's readBeatEffects, gated by separate flag bits): `effect.stroke` (a BeatStroke(direction, value) — the "brush"/arpeggio strum Guitar Pro's own UI writes when you drag across a chord, `value` being its speed) and `effect.pickStroke` (a separate, bare-enum field). The earlier fix (previous commit on this branch) only read `pickStroke`. Verified against real chart files: a sample GP5 with 24 authored chord strums had all 24 on `.stroke.direction` and zero on `.pickStroke` — so in practice "chord strumming" IS `.stroke`, and the prior fix silently imported every one of them as unset. The unit tests all passed because they mocked `.pickStroke` directly rather than the real BeatEffect shape, so nothing caught it until checking against an actual file. `_gp_pick_direction` now takes the whole beat.effect and checks `.stroke.direction` first (falling back to `.pickStroke` when `.stroke` is unset, since it's still a real, distinct field some files may use). Both share the same BeatStrokeDirection enum, so one mapping covers either source. 4 new tests, including a real-file check that 72 chord notes in the "Rehab" sample now carry pickDirection where 0 did before. 382/382 passing across test_gp2rs.py, test_gp2rs_gpx.py, test_song.py. Co-Authored-By: Claude Sonnet 5 * fix(gp2rs_gpx): read chord strum direction from Brush/Arpeggio, not Stroke Same class of bug as the GP4/5-side fix on this branch: the original _gpif_pick_direction guessed a shape that doesn't exist in any real GPIF export. Verified against real GP8 files: the actual chord-strum marking is Down|Up — the direct GPIF equivalent of GP4/5's beat.effect.stroke (see gp2rs.py's analogous fix). Down|Up (a direct child, not a Property) is a related broken-chord articulation, checked as a fallback when Brush is absent. Verified end-to-end against 12 real GP8 sample files: 7,865 chord notes across them now carry pickDirection, where the Stroke-based guess produced zero on every one of them. 7 new/rewritten tests; 387/387 passing across test_gp2rs.py, test_gp2rs_gpx.py, test_song.py. Co-Authored-By: Claude Sonnet 5 * Delete Black_Pearl_Jam.feedpak * ci: expose required checks for dispatch * ci: retrigger pull request checks * chore: narrow strum PR scope --------- Co-authored-by: Claude Sonnet 5 --- .github/workflows/ship-ci.yml | 3 + lib/gp2rs.py | 80 ++++++++++++++- lib/gp2rs_gpx.py | 61 ++++++++++++ tests/test_gp2rs.py | 183 +++++++++++++++++++++++++++++++++- tests/test_gp2rs_gpx.py | 78 +++++++++++++++ 5 files changed, 399 insertions(+), 6 deletions(-) diff --git a/.github/workflows/ship-ci.yml b/.github/workflows/ship-ci.yml index 3d6963eb..bdc37ccd 100644 --- a/.github/workflows/ship-ci.yml +++ b/.github/workflows/ship-ci.yml @@ -6,6 +6,9 @@ name: ship-ci # workflow_call — it never runs standalone, which would emit unprefixed # "" checks that the rulesets can't match. on: + # Keeps the wrapper discoverable and lets maintainers rerun the exact + # required-check workflow while diagnosing PR event delivery. + workflow_dispatch: pull_request: branches: [main, 'release/**'] # Trunk-based: post-merge CI on main catches semantic conflicts between diff --git a/lib/gp2rs.py b/lib/gp2rs.py index 48a8ee72..316dbf69 100644 --- a/lib/gp2rs.py +++ b/lib/gp2rs.py @@ -75,6 +75,9 @@ class RsNote: # Teaching mark (§6.2.2): fret-hand finger (-1 unset, 0 thumb..4 pinky). # Display only — never used for grading. fret_finger: int = -1 + # Strum direction (-1 unset, 0 down, 1 up) — mirrors song.py's Note.pick_direction + # convention (see _gp_pick_direction below for the GP enum mapping). + pick_direction: int = -1 @dataclass @@ -268,6 +271,46 @@ def _finger_xml_attrs(n: "RsNote") -> dict: return {} +def _gp_pick_direction(effect) -> int: + """Read a beat's strum/pick direction off a pyguitarpro BeatEffect and map + it to feedBack's Note.pick_direction convention (-1 unset, 0 down, 1 up — + see song.py's Note.pick_direction / the editor's _editorCyclePickDirection). + + GP5 binary actually encodes TWO independent fields here, read by + different flag bits (guitarpro/gp4.py's readBeatEffects): `effect.stroke` + (a BeatStroke(direction, value) — the "brush"/arpeggio chord-strum GP's + own UI writes when you drag across a chord, `value` being its speed) and + `effect.pickStroke` (a bare BeatStrokeDirection — a separate, rarer + marking). Verified against real chart files: every one of a sample GP5's + 24 authored chord strums used `.stroke.direction`; none used + `.pickStroke` at all. So `.stroke` is checked first — it's what "chord + strumming" actually means in practice — falling back to `.pickStroke` + only when `.stroke` is unset, since it's still a real, distinct field + some files may use. Both share the same BeatStrokeDirection enum + (none=0, up=1, down=2), so a single mapping covers either source. + """ + if effect is None: + return -1 + stroke = getattr(effect, "stroke", None) + direction = getattr(stroke, "direction", None) if stroke is not None else None + if direction is None or direction == guitarpro.BeatStrokeDirection.none: + direction = getattr(effect, "pickStroke", None) + if direction == guitarpro.BeatStrokeDirection.down: + return 0 + if direction == guitarpro.BeatStrokeDirection.up: + return 1 + return -1 + + +def _pick_direction_xml_attrs(n: "RsNote") -> dict: + """Optional `pickDirection` XML attribute for a /, default- + omitted (!= -1 only). `_parse_note` (lib/song.py) reads it back as `pkd` on + the wire, so a GP-authored up/down stroke survives import → wire → highway.""" + if getattr(n, "pick_direction", -1) != -1: + return {"pickDirection": str(int(n.pick_direction))} + return {} + + def _tempo_at_tick(tick: int, tempo_map: list[TempoEvent]) -> float: """Get the tempo at a given tick.""" result = tempo_map[0].tempo @@ -797,6 +840,11 @@ def convert_track( tempo = _tempo_at_tick(beat.start, tempo_map) dur = _duration_to_seconds(beat.duration, tempo) + # A stroke is one pick gesture across the whole beat (all + # strings struck together share a direction), so read it + # once per beat rather than per note. + _beat_pick_dir = _gp_pick_direction(beat.effect) + beat_notes = [] for note in beat.notes: if note.type == guitarpro.NoteType.rest: @@ -820,6 +868,7 @@ def convert_track( fret=fret, sustain=dur if dur > 0.2 else 0.0, mute=note.type == guitarpro.NoteType.dead, + pick_direction=_beat_pick_dir, ) # Techniques @@ -1100,13 +1149,24 @@ def convert_track( chord_templates=chord_templates, anchors=anchors, tempo=song.tempo, + capo=_gp_track_capo(track), ) +def _gp_track_capo(track) -> int: + """GP3/4/5 per-track capo fret. pyguitarpro exposes it as Track.offset. + Returns 0 (no capo) on any missing/malformed attribute — never fabricates + a capo, matching the fail-safe posture used throughout this module.""" + try: + return max(0, int(getattr(track, "offset", 0) or 0)) + except (TypeError, ValueError): + return 0 + + def _build_xml( title, artist, album, year, arrangement, tuning, num_strings, song_length, audio_offset, beats, sections, notes, chords, - chord_templates, anchors, tempo, + chord_templates, anchors, tempo, capo=0, ) -> str: root = ET.Element("song", version="7") @@ -1137,7 +1197,14 @@ def _build_xml( 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" + # Was hardcoded to "0" regardless of the source file's actual capo — + # every downstream consumer (editor, feedpakr) either silently imported + # capo=0 or had to independently patch it back in post-conversion (see + # feedpakr_pipeline.py's _capo_for_track/_gpif_capo_lookup, written + # because THIS was never fixed at the source). Fixed at the source now; + # callers pass the real value via the `capo` param (default 0 for the + # piano/drum paths, where GP has no capo concept at all). + ET.SubElement(root, "capo").text = str(int(capo)) # Ebeats — write beat times at MICROSECOND (6-decimal) precision, not # millisecond (3-decimal). The editor/timeline DERIVES per-bar BPM from beat @@ -1213,15 +1280,21 @@ def _build_xml( } attrs.update(_bend_shape_xml_attrs(n)) attrs.update(_finger_xml_attrs(n)) + attrs.update(_pick_direction_xml_attrs(n)) ET.SubElement(notes_el, "note", **attrs) # Chords chords_el = ET.SubElement(level, "chords", count=str(len(chords))) for ch in chords: + # No chord-level `strum` attribute: song.py's parser never reads one + # (each chordNote carries its own pickDirection instead, since GP's + # stroke direction is really a per-beat property applied per note — + # see _gp_pick_direction) — a prior hardcoded `strum="down"` here was + # dead, unread output that just looked like a real value. chord_el = ET.SubElement(chords_el, "chord", time=f"{ch.time:.3f}", chordId=str(ch.template_idx), - highDensity="0", strum="down") + highDensity="0") for cn in ch.notes: cn_attrs = { "time": f"{cn.time:.3f}", @@ -1246,6 +1319,7 @@ def _build_xml( } cn_attrs.update(_bend_shape_xml_attrs(cn)) cn_attrs.update(_finger_xml_attrs(cn)) + cn_attrs.update(_pick_direction_xml_attrs(cn)) ET.SubElement(chord_el, "chordNote", **cn_attrs) # Anchors diff --git a/lib/gp2rs_gpx.py b/lib/gp2rs_gpx.py index 75d854a2..9b46842c 100644 --- a/lib/gp2rs_gpx.py +++ b/lib/gp2rs_gpx.py @@ -650,6 +650,59 @@ def _gpif_left_fingering(note_el) -> int: return _GPIF_LEFT_FINGERING_MAP.get(raw, -1) +def _gpif_pick_direction(beat_el) -> int: + """Read a GPIF 's chord-strum direction -> RS pick_direction int + (-1 unset, 0 down, 1 up — mirrors gp2rs.py's _gp_pick_direction / + song.py's Note.pick_direction). + + Verified against real GP8 exports (unlike the first cut of this helper, + which guessed a child that doesn't exist in any real + file). The actual chord-strum marking is + Down|Up + — direct GPIF equivalent of GP4/5's + beat.effect.stroke, the "brush" GP's own UI writes when you drag across + a chord (see gp2rs.py's _gp_pick_direction for the GP4/5-side finding). + Down|Up (a direct child, not a Property) is a + related-but-distinct articulation — a broken chord rather than a single + brush gesture — checked as a fallback only when Brush is absent, since + it still carries a real, chord-wide up/down direction. + + Returns -1 (unset) when neither is present or the text is unrecognised — + never fabricates a direction, same posture as _gpif_left_fingering.""" + raw = '' + props = beat_el.find('Properties') + if props is not None: + for p in props.findall('Property'): + if p.get('name') == 'Brush': + raw = (p.findtext('Direction') or '').strip().lower() + break + if not raw: + raw = (beat_el.findtext('Arpeggio') or '').strip().lower() + if raw == 'down': + return 0 + if raw == 'up': + return 1 + return -1 + + +def _gpif_track_capo(track_el) -> int: + """GPIF per-track capo fret, at Track/Staves/Staff/Properties/ + Property[@name='CapoFret']/Fret. GP6/7/8 GPIF has no Track.offset + equivalent to read here (unlike GP3-5's pyguitarpro Track.offset, see + gp2rs.py's _gp_track_capo) — this is the format's own field for it. + + Returns 0 (no capo) when absent/malformed — never fabricates a capo.""" + if track_el is None: + return 0 + fret_el = track_el.find(".//Staves/Staff/Properties/Property[@name='CapoFret']/Fret") + if fret_el is None or not (fret_el.text or '').strip(): + return 0 + try: + return max(0, int(float(fret_el.text))) + except (TypeError, ValueError): + return 0 + + def _rs_string_order(string_pitches: list[int]) -> dict[int, int]: """Map each GPIF string index → RS string index (0 = lowest pitch). @@ -1771,6 +1824,10 @@ def _auto_guitar_hint(track_idx: int): dur = _beat_dur_secs(beat_el, rhythms_dict, _cur_tempo) t = voice_time + audio_offset + # A stroke is one pick gesture across the whole + # beat, so read it once per beat rather than per + # note — mirrors gp2rs.py's per-beat pickStroke. + _beat_pick_dir = _gpif_pick_direction(beat_el) notes_text = beat_el.findtext('Notes', '').strip() if notes_text: @@ -1856,6 +1913,7 @@ def _auto_guitar_hint(track_idx: int): string=rs_str, fret=rs_fret, sustain=sustain, + pick_direction=_beat_pick_dir if not (is_drum or is_keys) else -1, ) # Techniques — GPIF stores these as @@ -2092,6 +2150,9 @@ def _auto_guitar_hint(track_idx: int): chord_templates=chord_templates, anchors=anchors, tempo=int(tempo_bpm), + # No capo concept for drums/keys (same reasoning as pick_direction + # above) — only guitar/bass tracks get a real lookup. + capo=0 if (is_drum or is_keys) else _gpif_track_capo(track.get('_el')), ) # Inject tone change markers for guitar/bass tracks diff --git a/tests/test_gp2rs.py b/tests/test_gp2rs.py index bbaf6114..ea22e1dc 100644 --- a/tests/test_gp2rs.py +++ b/tests/test_gp2rs.py @@ -800,20 +800,26 @@ def _ct_note(note_type, gp_string, fret): ) -def _ct_song(beats, string_values=None): +def _ct_song(beats, string_values=None, offset=None): """One-measure mock song for convert_track, standard 6-string guitar at 120 BPM. - `string_values` overrides the tuning/string count (e.g. a 7-string track).""" + `string_values` overrides the tuning/string count (e.g. a 7-string track). + `offset` sets Track.offset (GP's per-track capo fret); omitted entirely + when None, matching real pyguitarpro tracks with no capo (mirrors + _gp_track_capo's getattr-with-default read).""" voice = SimpleNamespace(beats=beats) measure = SimpleNamespace(voices=[voice]) strings = [SimpleNamespace(number=i + 1, value=v) for i, v in enumerate(string_values or [64, 59, 55, 50, 45, 40])] - track = SimpleNamespace( + track_kwargs = dict( strings=strings, channel=SimpleNamespace(instrument=24), measures=[measure], name="Guitar", ) + if offset is not None: + track_kwargs["offset"] = offset + track = SimpleNamespace(**track_kwargs) mh = SimpleNamespace( start=0, number=1, @@ -1250,6 +1256,177 @@ def test_single_note_open_finger_omits_fg(): assert "fg" not in note_to_wire(_parse_note(xn)) +def test_chord_beat_stroke_field_imports_as_pkd(): + """Real GP5 chord-strum data lives on `beat.effect.stroke.direction` (a + BeatStroke(direction, value) — the "brush" GP's own UI writes when you + drag across a chord), NOT `beat.effect.pickStroke` (a separate, rarely- + used bare-enum field). Verified against a real chart: every authored + chord strum used `.stroke`; none used `.pickStroke`. This is the actual + regression — the original fix targeted the wrong field, so real chord + strums silently imported as unset despite passing tests built on mocks + that used `.pickStroke` directly.""" + from song import _parse_note, note_to_wire + note_e = _ct_note(guitarpro.NoteType.normal, gp_string=1, fret=3) + note_b = _ct_note(guitarpro.NoteType.normal, gp_string=2, fret=2) + beat = _ct_beat(tick=0, dur_value=4, notes=[note_e, note_b]) + beat.effect.stroke = SimpleNamespace( + direction=guitarpro.BeatStrokeDirection.down, value=64) + # pickStroke stays at its SimpleNamespace default (unset) — matching a + # real GP5 chord-strum beat, where only .stroke is populated. + + xml_str = convert_track(_ct_song([beat]), track_index=0) + root = ET.fromstring(xml_str) # noqa: S314 + chord_notes = root.findall(".//chords/chord/chordNote") + assert len(chord_notes) == 2 + for cn in chord_notes: + assert cn.get("pickDirection") == "0" + assert note_to_wire(_parse_note(cn))["pkd"] == 0 + + +def test_stroke_field_preferred_over_pick_stroke_when_both_set(): + """When a beat somehow carries both fields (unusual, but the two are + independently-flagged in the binary format so it's not impossible), + `.stroke` wins — it's the field real chord-strum authoring actually + uses, per test_chord_beat_stroke_field_imports_as_pkd's real-file check.""" + note = _ct_note(guitarpro.NoteType.normal, gp_string=2, fret=5) + beat = _ct_beat(tick=0, dur_value=4, notes=[note]) + beat.effect.stroke = SimpleNamespace( + direction=guitarpro.BeatStrokeDirection.down, value=64) + beat.effect.pickStroke = guitarpro.BeatStrokeDirection.up + + root = ET.fromstring(convert_track(_ct_song([beat]), track_index=0)) # noqa: S314 + xn = root.findall(".//notes/note")[0] + assert xn.get("pickDirection") == "0" # stroke (down), not pickStroke (up) + + +def test_stroke_field_none_direction_falls_back_to_pick_stroke(): + """A .stroke object can be present but carry BeatStrokeDirection.none + (unset) — attrs' default_factory always creates one, per real + BeatEffect() inspection. That must fall through to .pickStroke, not be + mistaken for an authored 'no stroke' override.""" + note = _ct_note(guitarpro.NoteType.normal, gp_string=2, fret=5) + beat = _ct_beat(tick=0, dur_value=4, notes=[note]) + beat.effect.stroke = SimpleNamespace( + direction=guitarpro.BeatStrokeDirection.none, value=0) + beat.effect.pickStroke = guitarpro.BeatStrokeDirection.up + + root = ET.fromstring(convert_track(_ct_song([beat]), track_index=0)) # noqa: S314 + xn = root.findall(".//notes/note")[0] + assert xn.get("pickDirection") == "1" + + +def test_single_note_down_stroke_imports_as_pkd_0(): + """A GP beat's pickStroke=down imports as pickDirection=0 and survives + convert_track XML -> _parse_note -> note_to_wire as pkd=0 (issue: GP import + discards strumming direction).""" + from song import _parse_note, note_to_wire + note = _ct_note(guitarpro.NoteType.normal, gp_string=2, fret=5) + beat = _ct_beat(tick=0, dur_value=4, notes=[note]) + beat.effect.pickStroke = guitarpro.BeatStrokeDirection.down + + root = ET.fromstring(convert_track(_ct_song([beat]), track_index=0)) # noqa: S314 + xn = root.findall(".//notes/note")[0] + assert xn.get("pickDirection") == "0" + assert note_to_wire(_parse_note(xn))["pkd"] == 0 + + +def test_single_note_up_stroke_imports_as_pkd_1(): + from song import _parse_note, note_to_wire + note = _ct_note(guitarpro.NoteType.normal, gp_string=2, fret=5) + beat = _ct_beat(tick=0, dur_value=4, notes=[note]) + beat.effect.pickStroke = guitarpro.BeatStrokeDirection.up + + root = ET.fromstring(convert_track(_ct_song([beat]), track_index=0)) # noqa: S314 + xn = root.findall(".//notes/note")[0] + assert xn.get("pickDirection") == "1" + assert note_to_wire(_parse_note(xn))["pkd"] == 1 + + +def test_note_no_stroke_omits_pick_direction(): + """No pickStroke effect (or BeatStrokeDirection.none) leaves pickDirection + unset — no fabricated direction, matching the fretFinger no-op convention.""" + from song import _parse_note, note_to_wire + note = _ct_note(guitarpro.NoteType.normal, gp_string=2, fret=5) + beat = _ct_beat(tick=0, dur_value=4, notes=[note]) + beat.effect.pickStroke = guitarpro.BeatStrokeDirection.none + + root = ET.fromstring(convert_track(_ct_song([beat]), track_index=0)) # noqa: S314 + xn = root.findall(".//notes/note")[0] + assert xn.get("pickDirection") is None + assert "pkd" not in note_to_wire(_parse_note(xn)) + + +def test_note_missing_pick_stroke_attr_omits_pick_direction(): + """Beats built without a pickStroke attribute at all (the pre-fix mock + shape, and older pyguitarpro effect objects) must not raise — getattr + falls back to 'no stroke' rather than an AttributeError.""" + note = _ct_note(guitarpro.NoteType.normal, gp_string=2, fret=5) + beat = _ct_beat(tick=0, dur_value=4, notes=[note]) # no .effect.pickStroke + + root = ET.fromstring(convert_track(_ct_song([beat]), track_index=0)) # noqa: S314 + xn = root.findall(".//notes/note")[0] + assert xn.get("pickDirection") is None + + +def test_chord_notes_all_share_the_beats_single_stroke_direction(): + """A stroke is one pick gesture across the whole chord — every chordNote + in the beat gets the same pickDirection, not just the first.""" + note_e = _ct_note(guitarpro.NoteType.normal, gp_string=1, fret=3) + note_b = _ct_note(guitarpro.NoteType.normal, gp_string=2, fret=2) + beat = _ct_beat(tick=0, dur_value=4, notes=[note_e, note_b]) + beat.effect.pickStroke = guitarpro.BeatStrokeDirection.up + + xml_str = convert_track(_ct_song([beat]), track_index=0) + root = ET.fromstring(xml_str) # noqa: S314 + chord_notes = root.findall(".//chords/chord/chordNote") + assert len(chord_notes) == 2 + assert all(cn.get("pickDirection") == "1" for cn in chord_notes) + + +def test_track_capo_imports_into_xml_and_arrangement(tmp_path): + """Track.offset (GP's per-track capo) survives convert_track XML -> + parse_arrangement as arr.capo, instead of the old hardcoded 0 (issue: + capo was always imported as 0 regardless of the source file).""" + from song import parse_arrangement + note = _ct_note(guitarpro.NoteType.normal, gp_string=2, fret=5) + beat = _ct_beat(tick=0, dur_value=4, notes=[note]) + + xml_str = convert_track(_ct_song([beat], offset=2), track_index=0) + root = ET.fromstring(xml_str) # noqa: S314 + assert root.find("capo").text == "2" + + xml_path = tmp_path / "arr.xml" + xml_path.write_text(xml_str, encoding="utf-8") + arr = parse_arrangement(str(xml_path)) + assert arr.capo == 2 + + +def test_track_no_offset_attr_imports_capo_zero(): + """Track objects with no .offset at all (real pyguitarpro tracks with no + capo don't carry the attribute) must not raise, and import capo=0.""" + note = _ct_note(guitarpro.NoteType.normal, gp_string=2, fret=5) + beat = _ct_beat(tick=0, dur_value=4, notes=[note]) + + xml_str = convert_track(_ct_song([beat]), track_index=0) # no offset= + root = ET.fromstring(xml_str) # noqa: S314 + assert root.find("capo").text == "0" + + +def test_chord_element_has_no_dead_strum_attribute(): + """The old hardcoded strum="down" chord attribute was never read by + song.py's parser — dropped as dead output rather than fixed in place, + since real direction now lives per-note on pickDirection.""" + note_e = _ct_note(guitarpro.NoteType.normal, gp_string=1, fret=3) + note_b = _ct_note(guitarpro.NoteType.normal, gp_string=2, fret=2) + beat = _ct_beat(tick=0, dur_value=4, notes=[note_e, note_b]) + + xml_str = convert_track(_ct_song([beat]), track_index=0) + root = ET.fromstring(xml_str) # noqa: S314 + chord_el = root.find(".//chords/chord") + assert chord_el is not None + assert chord_el.get("strum") is None + + def test_chord_without_diagram_has_blank_fingers(): # A plain two-note chord (effect.chord is None) is unchanged: blank name, # all-(-1) fingers — no regression for diagram-less charts. diff --git a/tests/test_gp2rs_gpx.py b/tests/test_gp2rs_gpx.py index 4b91106a..e3bbf61a 100644 --- a/tests/test_gp2rs_gpx.py +++ b/tests/test_gp2rs_gpx.py @@ -34,6 +34,8 @@ _resolve_pending_slides, _gpx_bend_shape, _gpif_left_fingering, + _gpif_pick_direction, + _gpif_track_capo, ) from gp2rs import RsNote @@ -915,6 +917,82 @@ def test_gpif_left_fingering_absent_or_unknown_is_unset(): ET.fromstring('')) == -1 +# ── _gpif_pick_direction (GP6/7/8 chord-strum direction -> pick_direction) ─── +# Real schema (verified against real GP8 exports — a sample chart had 335 +# authored chord strums): Properties/ +# Property[name=Brush]/Direction is the direct GPIF equivalent of GP4/5's +# beat.effect.stroke; (a direct child) is a related +# fallback. An earlier version of this helper guessed , +# which doesn't exist in any real file — see gp2rs.py's analogous +# .stroke-vs-.pickStroke fix for the GP4/5-side version of this same bug. + +@pytest.mark.parametrize("direction, expected", [ + ("Down", 0), ("down", 0), ("Up", 1), ("up", 1), +]) +def test_gpif_pick_direction_reads_brush_property(direction, expected): + b = ET.fromstring( + f'' + f'{direction}' + '') + assert _gpif_pick_direction(b) == expected + + +@pytest.mark.parametrize("direction, expected", [ + ("Down", 0), ("down", 0), ("Up", 1), ("up", 1), +]) +def test_gpif_pick_direction_falls_back_to_arpeggio(direction, expected): + # No Brush property present — (a direct child, not a + # Property) still carries a real chord-wide direction. + b = ET.fromstring(f'{direction}' + '') + assert _gpif_pick_direction(b) == expected + + +def test_gpif_pick_direction_prefers_brush_over_arpeggio(): + b = ET.fromstring( + '' + 'Down' + 'Up') + assert _gpif_pick_direction(b) == 0 + + +def test_gpif_pick_direction_absent_or_unknown_is_unset(): + # No Brush/Arpeggio at all, or unrecognised text -> -1 (never fabricate). + assert _gpif_pick_direction(ET.fromstring('')) == -1 + assert _gpif_pick_direction( + ET.fromstring('Sideways')) == -1 + assert _gpif_pick_direction( + ET.fromstring('' + '')) == -1 + # Other Properties (unrelated names) must not be mistaken for Brush. + assert _gpif_pick_direction( + ET.fromstring('' + '')) == -1 + + +# ── _gpif_track_capo (GP6/7/8 per-track capo, issue: always imported as 0) ── + +def test_gpif_track_capo_reads_capo_fret(): + t = ET.fromstring( + '' + '3' + '') + assert _gpif_track_capo(t) == 3 + + +def test_gpif_track_capo_absent_is_zero(): + assert _gpif_track_capo(ET.fromstring('')) == 0 + assert _gpif_track_capo(None) == 0 + + +def test_gpif_track_capo_malformed_is_zero(): + t = ET.fromstring( + '' + 'not-a-number' + '') + assert _gpif_track_capo(t) == 0 + + # ── convert_file: GP8 chord-diagram name + fingering extraction (E3) ───────── # GP7/GP8 GPIF carries authored chord diagrams under a track's # Property[@name="DiagramCollection"]. Each Item gives the chord name and a From 5b443e9e73b2a619f726da4a3206debd10b9207f Mon Sep 17 00:00:00 2001 From: Carolina <79524656+carochacs@users.noreply.github.com> Date: Sat, 1 Aug 2026 15:22:48 -0600 Subject: [PATCH 02/46] Fix: add loaded_slop null check before accessing stems (#16) When a sloppak fails to load (cache corruption, partial extraction, etc.), loaded_slop is None but the code attempted to access loaded_slop.stems, silently crashing and leaving audio_url as None. This caused the player to fall back to MIDI synth instead of playing the actual audio stems. Added: - Guard 'if is_slop and loaded_slop is not None:' before accessing stems - Error message when sloppak fails to load - Removed redundant loaded_slop check in full_mix branch Co-authored-by: Claude Haiku 4.5 --- lib/routers/ws_highway.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/lib/routers/ws_highway.py b/lib/routers/ws_highway.py index 02b27568..fd1ad602 100644 --- a/lib/routers/ws_highway.py +++ b/lib/routers/ws_highway.py @@ -372,7 +372,7 @@ async def _send_keepalives(): else: audio_id = Path(filename).stem.replace(" ", "_") - if is_slop: + if is_slop and loaded_slop is not None: # Stems are served via the sloppak file endpoint; the first stem # (or explicit default) is the core