From 30741555461eb66f204234f639d6cf4d15423714 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 13 Sep 2026 03:26:29 +0000 Subject: [PATCH 1/4] fix: propagate GP arpeggio flag through gp2rs.py chord templates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GP3/4/5 imports never read the arpeggio "brush" marker off pyguitarpro's BeatEffect, so ChordTemplate.arp was always false for GP-sourced charts and every arpeggiated passage rendered as an ordinary strummed chord. The wire format and downstream consumers (feedpakr's derive_handshapes, highway_3d's arpeggio frame/bracket visualization) already handle arp correctly for loose-folder/XML imports — this was purely a gp2rs.py gap. Adds ChordTemplate.arp and _gp_beat_arpeggio(), which reads the same effect.stroke field _gp_pick_direction() already uses for strum direction, and emits arp="1" on for song.py's existing _chord_template_arpeggio_flag() to read back. Closes #57 Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_018jAfdcSJGQw9cMnVpjbfQe --- CHANGELOG.md | 1 + lib/gp2rs.py | 27 +++++++++++ tests/test_gp2rs.py | 106 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 134 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2666b1c7..fec6848e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **`lib/gp2rs.py` now propagates the Guitar Pro arpeggio/handshape flag on import** (#57). GP3/4/5 chord beats never set `arp` on their `ChordTemplate`, so every GP-sourced arpeggiated passage imported as an ordinary strummed chord and the downstream `.feedpak` handshape data (`handshapes[].arp`) was always `false` — even when the source GP file explicitly marked the chord as an arpeggio. The already-implemented downstream consumers (feedpakr's `derive_handshapes()`, the `highway_3d` plugin's arpeggio frame/bracket visualization) were simply never fed the flag for GP imports. Added a `ChordTemplate.arp` field and a `_gp_beat_arpeggio()` detector that reads the same `effect.stroke` "brush" field `_gp_pick_direction()` already uses for strum direction, treating a present, non-`none` stroke as the arpeggio marker (a bare `.pickStroke` stays a plain strum, not an arpeggio). Threaded through to the `` XML attribute, which `lib/song.py`'s existing `_chord_template_arpeggio_flag()` already reads back — no downstream changes needed. - **keys_highway_3d now respects the mastery/difficulty slider** (#67). The plugin fetches `notation_measures` over its own private `/ws/highway` connection and never consulted the host's difficulty-filtered note stream, so lowering the mastery percentage had no visible effect — the highway always showed the full chart. Notation carries no per-note difficulty tag of its own (it's a structurally separate representation from the tab/gameplay note stream — see feedback-plugin-difficulty-ladder#90), so this correlates each flattened notation event's onset against the chart's phrase windows (now also streamed to this plugin, chunked like `notation_measures`) and the render bundle's ALREADY mastery-filtered tab content (`bundle.notes`/`bundle.chords`): a phrase is "playable" when the tab has any filtered note/chord in that phrase's `[start_time, end_time)` window, and a notation event is kept when its onset falls inside a playable phrase. Recomputes (and rebuilds note meshes + resets scoring) whenever `bundle.mastery` changes; a fixed-difficulty chart with no phrase data is unaffected. ### Changed diff --git a/lib/gp2rs.py b/lib/gp2rs.py index 316dbf69..f05d4b2d 100644 --- a/lib/gp2rs.py +++ b/lib/gp2rs.py @@ -112,6 +112,7 @@ class ChordTemplate: name: str frets: list[int] # per string, -1 = unused fingers: list[int] # per string, -1 = unused + arp: bool = False # arpeggio/handshape marker — see _gp_beat_arpeggio @dataclass @@ -302,6 +303,22 @@ def _gp_pick_direction(effect) -> int: return -1 +def _gp_beat_arpeggio(effect) -> bool: + """Read a chord beat's arpeggio marker off a pyguitarpro BeatEffect. + + `effect.stroke` (BeatStroke(direction, value)) is the "brush" GP's own UI + writes when you drag across a chord to arpeggiate it — see + `_gp_pick_direction` above for the same field used for strum direction. + Unlike pick direction, arpeggio does NOT fall back to `.pickStroke`: a + bare pick-stroke mark is a plain up/down strum, not an arpeggio brush. + """ + if effect is None: + return False + stroke = getattr(effect, "stroke", None) + direction = getattr(stroke, "direction", None) if stroke is not None else None + return direction is not None and direction != guitarpro.BeatStrokeDirection.none + + 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 @@ -1033,6 +1050,14 @@ def convert_track( else: idx = chord_template_map[fret_key] + # Arpeggio is a per-beat articulation, not diagram data, so + # it attaches regardless of whether this beat also carries + # a chord diagram. Templates dedupe by fret_key, so one + # arpeggiated beat marks every strum sharing that voicing + # — same template-level model as the name/fingers back-fill. + if _gp_beat_arpeggio(beat.effect): + chord_templates[idx].arp = True + # Enrich the template from the GP chord diagram attached to # this beat — but ONLY when the diagram describes the voicing # actually played (same width-normalized fret pattern). A @@ -1248,6 +1273,8 @@ def _build_xml( for i in range(width): attrs[f"fret{i}"] = str(ct.frets[i] if i < len(ct.frets) else -1) attrs[f"finger{i}"] = str(ct.fingers[i] if i < len(ct.fingers) else -1) + if ct.arp: + attrs["arp"] = "1" ET.SubElement(ct_el, "chordTemplate", **attrs) # Single difficulty level with all notes diff --git a/tests/test_gp2rs.py b/tests/test_gp2rs.py index ea22e1dc..91286a65 100644 --- a/tests/test_gp2rs.py +++ b/tests/test_gp2rs.py @@ -24,6 +24,7 @@ _build_playback_schedule, _compute_tuning, _extract_year, + _gp_beat_arpeggio, _gp_bend_shape, _gp_string_to_rs, _is_bass_track, @@ -1356,6 +1357,111 @@ def test_note_no_stroke_omits_pick_direction(): assert "pkd" not in note_to_wire(_parse_note(xn)) +# ── convert_track: arpeggio flag on chord templates (issue #57) ───────────── +# GP-authored chord arpeggios are marked via `beat.effect.stroke` — the same +# "brush" field _gp_pick_direction reads for strum direction — but as a +# *presence* signal (any real direction), not translated into up/down. This +# must attach to the ChordTemplate as `arp`, distinct from the per-note +# pickDirection derived from the same field. + +def test_gp_beat_arpeggio_detector(): + assert _gp_beat_arpeggio(None) is False + assert _gp_beat_arpeggio(SimpleNamespace(stroke=None, pickStroke=None)) is False + assert _gp_beat_arpeggio(SimpleNamespace( + stroke=SimpleNamespace(direction=guitarpro.BeatStrokeDirection.none, value=0), + )) is False + assert _gp_beat_arpeggio(SimpleNamespace( + stroke=SimpleNamespace(direction=guitarpro.BeatStrokeDirection.down, value=64), + )) is True + assert _gp_beat_arpeggio(SimpleNamespace( + stroke=SimpleNamespace(direction=guitarpro.BeatStrokeDirection.up, value=64), + )) is True + # A bare pickStroke (no .stroke) is a plain strum, not an arpeggio. + assert _gp_beat_arpeggio(SimpleNamespace( + stroke=None, pickStroke=guitarpro.BeatStrokeDirection.up, + )) is False + + +def test_chord_beat_with_stroke_marks_template_arpeggio(): + """A chord beat carrying `.stroke` emits `arp="1"` on its , + and the chord notes still carry their own pickDirection — both signals + coexist independently.""" + 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) + + xml_str = convert_track(_ct_song([beat]), track_index=0) + root = ET.fromstring(xml_str) # noqa: S314 + ct = root.find(".//chordTemplates/chordTemplate") + assert ct.get("arp") == "1" + for cn in root.findall(".//chords/chord/chordNote"): + assert cn.get("pickDirection") == "0" + + +def test_chord_beat_without_stroke_omits_arpeggio(): + """A plain chord strum (no .stroke) leaves `arp` unset — no fabricated flag.""" + 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 + ct = root.find(".//chordTemplates/chordTemplate") + assert ct.get("arp") is None + + +def test_chord_beat_with_only_pick_stroke_omits_arpeggio(): + """A bare `.pickStroke` (no `.stroke`) is a plain up/down strum, not an + arpeggio — `arp` stays unset even though pickDirection is still derived.""" + 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 + ct = root.find(".//chordTemplates/chordTemplate") + assert ct.get("arp") is None + cn = root.findall(".//chords/chord/chordNote")[0] + assert cn.get("pickDirection") == "1" + + +def test_chord_beat_with_none_direction_stroke_omits_arpeggio(): + """A `.stroke` object present but carrying `BeatStrokeDirection.none` + (attrs' default_factory always creates one) must not be mistaken for an + authored arpeggio marker.""" + 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.none, value=0) + + xml_str = convert_track(_ct_song([beat]), track_index=0) + root = ET.fromstring(xml_str) # noqa: S314 + ct = root.find(".//chordTemplates/chordTemplate") + assert ct.get("arp") is None + + +def test_arpeggio_flag_survives_xml_to_wire_round_trip(tmp_path): + """The `arp` attribute survives XML -> Arrangement -> wire, matching the + already-implemented downstream consumer (feedpakr's derive_handshapes).""" + from song import chord_template_to_wire, parse_arrangement + 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) + + xml_str = convert_track(_ct_song([beat]), track_index=0) + xml_path = tmp_path / "arr.xml" + xml_path.write_text(xml_str, encoding="utf-8") + arr = parse_arrangement(str(xml_path)) + assert len(arr.chord_templates) == 1 + assert chord_template_to_wire(arr.chord_templates[0])["arp"] is True + + 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 From af58bd9c6b1608e2c2dadf29b4c18639eea19036 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 13 Sep 2026 03:30:30 +0000 Subject: [PATCH 2/4] test: dedupe repeated chord-beat setup in arpeggio tests Five arpeggio regression tests each rebuilt the same two-note chord beat inline. Extracted _arp_chord_beat() so CodeFactor's duplicate- code detector (4 issues found on PR #78) has nothing left to flag. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_018jAfdcSJGQw9cMnVpjbfQe --- tests/test_gp2rs.py | 29 ++++++++++++++--------------- 1 file changed, 14 insertions(+), 15 deletions(-) diff --git a/tests/test_gp2rs.py b/tests/test_gp2rs.py index 91286a65..2ef9a929 100644 --- a/tests/test_gp2rs.py +++ b/tests/test_gp2rs.py @@ -1364,6 +1364,15 @@ def test_note_no_stroke_omits_pick_direction(): # must attach to the ChordTemplate as `arp`, distinct from the per-note # pickDirection derived from the same field. +def _arp_chord_beat(): + """A fresh two-note chord beat (high e fret 3 + B fret 2) with no + stroke/pickStroke set — the shared starting point for the arpeggio tests + below, each of which sets `.stroke`/`.pickStroke` differently.""" + note_e = _ct_note(guitarpro.NoteType.normal, gp_string=1, fret=3) + note_b = _ct_note(guitarpro.NoteType.normal, gp_string=2, fret=2) + return _ct_beat(tick=0, dur_value=4, notes=[note_e, note_b]) + + def test_gp_beat_arpeggio_detector(): assert _gp_beat_arpeggio(None) is False assert _gp_beat_arpeggio(SimpleNamespace(stroke=None, pickStroke=None)) is False @@ -1386,9 +1395,7 @@ def test_chord_beat_with_stroke_marks_template_arpeggio(): """A chord beat carrying `.stroke` emits `arp="1"` on its , and the chord notes still carry their own pickDirection — both signals coexist independently.""" - 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 = _arp_chord_beat() beat.effect.stroke = SimpleNamespace( direction=guitarpro.BeatStrokeDirection.down, value=64) @@ -1402,9 +1409,7 @@ def test_chord_beat_with_stroke_marks_template_arpeggio(): def test_chord_beat_without_stroke_omits_arpeggio(): """A plain chord strum (no .stroke) leaves `arp` unset — no fabricated flag.""" - 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 = _arp_chord_beat() xml_str = convert_track(_ct_song([beat]), track_index=0) root = ET.fromstring(xml_str) # noqa: S314 @@ -1415,9 +1420,7 @@ def test_chord_beat_without_stroke_omits_arpeggio(): def test_chord_beat_with_only_pick_stroke_omits_arpeggio(): """A bare `.pickStroke` (no `.stroke`) is a plain up/down strum, not an arpeggio — `arp` stays unset even though pickDirection is still derived.""" - 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 = _arp_chord_beat() beat.effect.pickStroke = guitarpro.BeatStrokeDirection.up xml_str = convert_track(_ct_song([beat]), track_index=0) @@ -1432,9 +1435,7 @@ def test_chord_beat_with_none_direction_stroke_omits_arpeggio(): """A `.stroke` object present but carrying `BeatStrokeDirection.none` (attrs' default_factory always creates one) must not be mistaken for an authored arpeggio marker.""" - 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 = _arp_chord_beat() beat.effect.stroke = SimpleNamespace( direction=guitarpro.BeatStrokeDirection.none, value=0) @@ -1448,9 +1449,7 @@ def test_arpeggio_flag_survives_xml_to_wire_round_trip(tmp_path): """The `arp` attribute survives XML -> Arrangement -> wire, matching the already-implemented downstream consumer (feedpakr's derive_handshapes).""" from song import chord_template_to_wire, parse_arrangement - 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 = _arp_chord_beat() beat.effect.stroke = SimpleNamespace( direction=guitarpro.BeatStrokeDirection.down, value=64) From c3a0998b7415c9e812a0e0ed1f1910c7894e6714 Mon Sep 17 00:00:00 2001 From: "pullfrog[bot]" <226033991+pullfrog[bot]@users.noreply.github.com> Date: Sun, 13 Sep 2026 03:41:43 +0000 Subject: [PATCH 3/4] fix: parse gp2rs test XML via safe_xml.safe_fromstring to clear CodeFactor B314 --- tests/test_gp2rs.py | 68 ++++++++++++++++++++++----------------------- 1 file changed, 34 insertions(+), 34 deletions(-) diff --git a/tests/test_gp2rs.py b/tests/test_gp2rs.py index 2ef9a929..d8e18b86 100644 --- a/tests/test_gp2rs.py +++ b/tests/test_gp2rs.py @@ -10,12 +10,12 @@ walker. """ -import xml.etree.ElementTree as ET from types import SimpleNamespace from unittest import mock import guitarpro import pytest +import safe_xml from gp2rs import ( GP_TICKS_PER_QUARTER, @@ -113,7 +113,7 @@ def _converter_ebeats(converter, numerator, denominator, tempo_changes=None): denominator=denominator, tempo_changes=tempo_changes, ) - root = ET.fromstring(converter(song, 0)) + root = safe_xml.safe_fromstring(converter(song, 0)) ebeats = root.find("ebeats") assert ebeats is not None, "Converter output missing node" return ebeats @@ -856,7 +856,7 @@ def test_tied_note_extends_sustain_not_duplicate(): beat2 = _ct_beat(tick=GP_TICKS_PER_QUARTER, dur_value=4, notes=[note2]) xml_str = convert_track(_ct_song([beat1, beat2]), track_index=0) - root = ET.fromstring(xml_str) # noqa: S314 + root = safe_xml.safe_fromstring(xml_str) notes = root.findall(".//notes/note") assert len(notes) == 1, f"tie must not emit a second note; got {len(notes)}" @@ -871,7 +871,7 @@ def test_tied_note_without_predecessor_is_silently_dropped(): beat = _ct_beat(tick=0, dur_value=4, notes=[note]) xml_str = convert_track(_ct_song([beat]), track_index=0) - root = ET.fromstring(xml_str) # noqa: S314 + root = safe_xml.safe_fromstring(xml_str) notes = root.findall(".//notes/note") assert len(notes) == 0 @@ -919,7 +919,7 @@ def test_bent_note_imports_with_curve_through_wire(): note.effect.bend = _ct_bend([(0, 0), (6, 4), (12, 0)]) beat = _ct_beat(tick=0, dur_value=4, notes=[note]) - root = ET.fromstring(convert_track(_ct_song([beat]), track_index=0)) # noqa: S314 + root = safe_xml.safe_fromstring(convert_track(_ct_song([beat]), track_index=0)) xn = root.findall(".//notes/note")[0] assert xn.get("bend") == "2.0" assert xn.get("bendIntent") == "4" # round-trip @@ -937,7 +937,7 @@ def test_bent_note_imports_with_curve_through_wire(): def test_non_bent_note_has_no_curve(): note = _ct_note(guitarpro.NoteType.normal, gp_string=1, fret=5) # bend=None beat = _ct_beat(tick=0, dur_value=4, notes=[note]) - root = ET.fromstring(convert_track(_ct_song([beat]), track_index=0)) # noqa: S314 + root = safe_xml.safe_fromstring(convert_track(_ct_song([beat]), track_index=0)) xn = root.findall(".//notes/note")[0] assert xn.get("bend") == "0" assert xn.get("bendIntent") is None @@ -997,7 +997,7 @@ def test_tie_does_not_attach_to_overwritten_earlier_voice_note(): xml_str = convert_track(_ct_multivoice_song([[v0_beat2], [v1_beat0, v1_tie]]), track_index=0) - root = ET.fromstring(xml_str) # noqa: S314 + root = safe_xml.safe_fromstring(xml_str) notes = root.findall(".//notes/note") sustains = {n.get("fret"): float(n.get("sustain")) for n in notes} @@ -1016,7 +1016,7 @@ def test_two_normal_notes_on_same_string_are_both_emitted(): beat2 = _ct_beat(tick=GP_TICKS_PER_QUARTER, dur_value=4, notes=[note2]) xml_str = convert_track(_ct_song([beat1, beat2]), track_index=0) - root = ET.fromstring(xml_str) # noqa: S314 + root = safe_xml.safe_fromstring(xml_str) notes = root.findall(".//notes/note") assert len(notes) == 2 @@ -1071,7 +1071,7 @@ def test_piano_tied_chord_both_notes_extended(): beat2 = _ct_beat(tick=GP_TICKS_PER_QUARTER, dur_value=4, notes=[t_c3, t_d3]) xml_str = convert_piano_track(_piano_song([beat1, beat2]), track_index=0) - root = ET.fromstring(xml_str) # noqa: S314 + root = safe_xml.safe_fromstring(xml_str) chords = root.findall(".//chords/chord") assert len(chords) == 1, "tie beat must not emit a second chord" @@ -1173,7 +1173,7 @@ def test_tie_not_extended_across_repeat_boundary(): notes=[_ct_note(guitarpro.NoteType.normal, gp_string=1, fret=5)]) xml_str = convert_track(_ct_song_repeat([tie_beat], [normal_beat]), track_index=0) - root = ET.fromstring(xml_str) # noqa: S314 + root = safe_xml.safe_fromstring(xml_str) notes = root.findall(".//notes/note") # Two passes through measure 1 → two normal notes; the ties are both dropped. @@ -1220,7 +1220,7 @@ def test_chord_diagram_fingers_extracted(): ) xml_str = convert_track(_ct_song([beat]), track_index=0) - root = ET.fromstring(xml_str) # noqa: S314 + root = safe_xml.safe_fromstring(xml_str) ct = root.find(".//chordTemplates/chordTemplate") assert ct is not None assert ct.get("chordName") == "Gtest" @@ -1238,7 +1238,7 @@ def test_single_note_left_hand_finger_imports_as_fg(): note.effect.leftHandFinger = guitarpro.Fingering.middle # -> 2 beat = _ct_beat(tick=0, dur_value=4, notes=[note]) - root = ET.fromstring(convert_track(_ct_song([beat]), track_index=0)) # noqa: S314 + root = safe_xml.safe_fromstring(convert_track(_ct_song([beat]), track_index=0)) xn = root.findall(".//notes/note")[0] assert xn.get("fretFinger") == "2" assert note_to_wire(_parse_note(xn))["fg"] == 2 @@ -1251,7 +1251,7 @@ def test_single_note_open_finger_omits_fg(): note.effect.leftHandFinger = guitarpro.Fingering.open # -1 -> unset beat = _ct_beat(tick=0, dur_value=4, notes=[note]) - root = ET.fromstring(convert_track(_ct_song([beat]), track_index=0)) # noqa: S314 + root = safe_xml.safe_fromstring(convert_track(_ct_song([beat]), track_index=0)) xn = root.findall(".//notes/note")[0] assert xn.get("fretFinger") is None assert "fg" not in note_to_wire(_parse_note(xn)) @@ -1276,7 +1276,7 @@ def test_chord_beat_stroke_field_imports_as_pkd(): # 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 + root = safe_xml.safe_fromstring(xml_str) chord_notes = root.findall(".//chords/chord/chordNote") assert len(chord_notes) == 2 for cn in chord_notes: @@ -1295,7 +1295,7 @@ def test_stroke_field_preferred_over_pick_stroke_when_both_set(): 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 + root = safe_xml.safe_fromstring(convert_track(_ct_song([beat]), track_index=0)) xn = root.findall(".//notes/note")[0] assert xn.get("pickDirection") == "0" # stroke (down), not pickStroke (up) @@ -1311,7 +1311,7 @@ def test_stroke_field_none_direction_falls_back_to_pick_stroke(): 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 + root = safe_xml.safe_fromstring(convert_track(_ct_song([beat]), track_index=0)) xn = root.findall(".//notes/note")[0] assert xn.get("pickDirection") == "1" @@ -1325,7 +1325,7 @@ def test_single_note_down_stroke_imports_as_pkd_0(): 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 + root = safe_xml.safe_fromstring(convert_track(_ct_song([beat]), track_index=0)) xn = root.findall(".//notes/note")[0] assert xn.get("pickDirection") == "0" assert note_to_wire(_parse_note(xn))["pkd"] == 0 @@ -1337,7 +1337,7 @@ def test_single_note_up_stroke_imports_as_pkd_1(): 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 + root = safe_xml.safe_fromstring(convert_track(_ct_song([beat]), track_index=0)) xn = root.findall(".//notes/note")[0] assert xn.get("pickDirection") == "1" assert note_to_wire(_parse_note(xn))["pkd"] == 1 @@ -1351,7 +1351,7 @@ def test_note_no_stroke_omits_pick_direction(): 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 + root = safe_xml.safe_fromstring(convert_track(_ct_song([beat]), track_index=0)) xn = root.findall(".//notes/note")[0] assert xn.get("pickDirection") is None assert "pkd" not in note_to_wire(_parse_note(xn)) @@ -1400,7 +1400,7 @@ def test_chord_beat_with_stroke_marks_template_arpeggio(): direction=guitarpro.BeatStrokeDirection.down, value=64) xml_str = convert_track(_ct_song([beat]), track_index=0) - root = ET.fromstring(xml_str) # noqa: S314 + root = safe_xml.safe_fromstring(xml_str) ct = root.find(".//chordTemplates/chordTemplate") assert ct.get("arp") == "1" for cn in root.findall(".//chords/chord/chordNote"): @@ -1412,7 +1412,7 @@ def test_chord_beat_without_stroke_omits_arpeggio(): beat = _arp_chord_beat() xml_str = convert_track(_ct_song([beat]), track_index=0) - root = ET.fromstring(xml_str) # noqa: S314 + root = safe_xml.safe_fromstring(xml_str) ct = root.find(".//chordTemplates/chordTemplate") assert ct.get("arp") is None @@ -1424,7 +1424,7 @@ def test_chord_beat_with_only_pick_stroke_omits_arpeggio(): beat.effect.pickStroke = guitarpro.BeatStrokeDirection.up xml_str = convert_track(_ct_song([beat]), track_index=0) - root = ET.fromstring(xml_str) # noqa: S314 + root = safe_xml.safe_fromstring(xml_str) ct = root.find(".//chordTemplates/chordTemplate") assert ct.get("arp") is None cn = root.findall(".//chords/chord/chordNote")[0] @@ -1440,7 +1440,7 @@ def test_chord_beat_with_none_direction_stroke_omits_arpeggio(): direction=guitarpro.BeatStrokeDirection.none, value=0) xml_str = convert_track(_ct_song([beat]), track_index=0) - root = ET.fromstring(xml_str) # noqa: S314 + root = safe_xml.safe_fromstring(xml_str) ct = root.find(".//chordTemplates/chordTemplate") assert ct.get("arp") is None @@ -1468,7 +1468,7 @@ def test_note_missing_pick_stroke_attr_omits_pick_direction(): 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 + root = safe_xml.safe_fromstring(convert_track(_ct_song([beat]), track_index=0)) xn = root.findall(".//notes/note")[0] assert xn.get("pickDirection") is None @@ -1482,7 +1482,7 @@ def test_chord_notes_all_share_the_beats_single_stroke_direction(): beat.effect.pickStroke = guitarpro.BeatStrokeDirection.up xml_str = convert_track(_ct_song([beat]), track_index=0) - root = ET.fromstring(xml_str) # noqa: S314 + root = safe_xml.safe_fromstring(xml_str) chord_notes = root.findall(".//chords/chord/chordNote") assert len(chord_notes) == 2 assert all(cn.get("pickDirection") == "1" for cn in chord_notes) @@ -1497,7 +1497,7 @@ def test_track_capo_imports_into_xml_and_arrangement(tmp_path): 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 + root = safe_xml.safe_fromstring(xml_str) assert root.find("capo").text == "2" xml_path = tmp_path / "arr.xml" @@ -1513,7 +1513,7 @@ def test_track_no_offset_attr_imports_capo_zero(): 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 + root = safe_xml.safe_fromstring(xml_str) assert root.find("capo").text == "0" @@ -1526,7 +1526,7 @@ def test_chord_element_has_no_dead_strum_attribute(): 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 + root = safe_xml.safe_fromstring(xml_str) chord_el = root.find(".//chords/chord") assert chord_el is not None assert chord_el.get("strum") is None @@ -1540,7 +1540,7 @@ def test_chord_without_diagram_has_blank_fingers(): beat = _ct_beat(tick=0, dur_value=4, notes=[note_e, note_b]) # chord=None xml_str = convert_track(_ct_song([beat]), track_index=0) - root = ET.fromstring(xml_str) # noqa: S314 + root = safe_xml.safe_fromstring(xml_str) ct = root.find(".//chordTemplates/chordTemplate") assert ct is not None assert ct.get("chordName") == "" @@ -1566,7 +1566,7 @@ def test_chord_diagram_backfills_template_first_strummed_unannotated(): ) xml_str = convert_track(_ct_song([plain, annotated]), track_index=0) - root = ET.fromstring(xml_str) # noqa: S314 + root = safe_xml.safe_fromstring(xml_str) cts = root.findall(".//chordTemplates/chordTemplate") assert len(cts) == 1, "same voicing must dedup to one template" assert cts[0].get("chordName") == "Gtest" @@ -1587,7 +1587,7 @@ def test_chord_diagram_mismatch_not_applied(): ) xml_str = convert_track(_ct_song([beat]), track_index=0) - root = ET.fromstring(xml_str) # noqa: S314 + root = safe_xml.safe_fromstring(xml_str) ct = root.find(".//chordTemplates/chordTemplate") assert ct is not None assert ct.get("chordName") == "" @@ -1615,7 +1615,7 @@ def _beat(tick, name, fingerings): [guitarpro.Fingering.middle, guitarpro.Fingering.index]) xml_str = convert_track(_ct_song([first, second]), track_index=0) - root = ET.fromstring(xml_str) # noqa: S314 + root = safe_xml.safe_fromstring(xml_str) cts = root.findall(".//chordTemplates/chordTemplate") assert len(cts) == 1 assert cts[0].get("chordName") == "Gtest" # from the first (name-only) beat @@ -1638,7 +1638,7 @@ def test_chord_diagram_barre_higher_position_matches(): beat.effect.chord = ch xml_str = convert_track(_ct_song([beat]), track_index=0) - root = ET.fromstring(xml_str) # noqa: S314 + root = safe_xml.safe_fromstring(xml_str) ct = root.find(".//chordTemplates/chordTemplate") assert ct is not None assert ct.get("chordName") == "A" @@ -1664,7 +1664,7 @@ def test_chord_diagram_extended_string_outside_played_width_not_applied(): ) xml_str = convert_track(_ct_song([beat], string_values=seven), track_index=0) - root = ET.fromstring(xml_str) # noqa: S314 + root = safe_xml.safe_fromstring(xml_str) ct = root.find(".//chordTemplates/chordTemplate") assert ct is not None assert ct.get("chordName") == "" From 3238e65db9d07ab9b54f9be02cd6fbff22756f2f Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 14 Sep 2026 02:18:27 +0000 Subject: [PATCH 4/4] fix: stop arpeggio flag leaking across chord templates sharing a voicing Chord templates deduped by fret voicing alone, so a chord shape played both arpeggiated in one passage and plainly strummed elsewhere in the same song shared a single ChordTemplate. Marking that template arp=True for the arpeggiated beat then incorrectly marked every other strum of the same voicing as an arpeggio too. Fold arp status into the dedup key (fret_key) so the two articulations of the same voicing get separate templates, and pass arp on construction instead of mutating a shared template after the fact. Found by Codacy's automated review on PR #78. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_018jAfdcSJGQw9cMnVpjbfQe --- CHANGELOG.md | 2 +- lib/gp2rs.py | 19 ++++++++++--------- tests/test_gp2rs.py | 27 +++++++++++++++++++++++++++ 3 files changed, 38 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fec6848e..f88552df 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed -- **`lib/gp2rs.py` now propagates the Guitar Pro arpeggio/handshape flag on import** (#57). GP3/4/5 chord beats never set `arp` on their `ChordTemplate`, so every GP-sourced arpeggiated passage imported as an ordinary strummed chord and the downstream `.feedpak` handshape data (`handshapes[].arp`) was always `false` — even when the source GP file explicitly marked the chord as an arpeggio. The already-implemented downstream consumers (feedpakr's `derive_handshapes()`, the `highway_3d` plugin's arpeggio frame/bracket visualization) were simply never fed the flag for GP imports. Added a `ChordTemplate.arp` field and a `_gp_beat_arpeggio()` detector that reads the same `effect.stroke` "brush" field `_gp_pick_direction()` already uses for strum direction, treating a present, non-`none` stroke as the arpeggio marker (a bare `.pickStroke` stays a plain strum, not an arpeggio). Threaded through to the `` XML attribute, which `lib/song.py`'s existing `_chord_template_arpeggio_flag()` already reads back — no downstream changes needed. +- **`lib/gp2rs.py` now propagates the Guitar Pro arpeggio/handshape flag on import** (#57). GP3/4/5 chord beats never set `arp` on their `ChordTemplate`, so every GP-sourced arpeggiated passage imported as an ordinary strummed chord and the downstream `.feedpak` handshape data (`handshapes[].arp`) was always `false` — even when the source GP file explicitly marked the chord as an arpeggio. The already-implemented downstream consumers (feedpakr's `derive_handshapes()`, the `highway_3d` plugin's arpeggio frame/bracket visualization) were simply never fed the flag for GP imports. Added a `ChordTemplate.arp` field and a `_gp_beat_arpeggio()` detector that reads the same `effect.stroke` "brush" field `_gp_pick_direction()` already uses for strum direction, treating a present, non-`none` stroke as the arpeggio marker (a bare `.pickStroke` stays a plain strum, not an arpeggio). Threaded through to the `` XML attribute, which `lib/song.py`'s existing `_chord_template_arpeggio_flag()` already reads back — no downstream changes needed. Chord templates dedupe by fret voicing **and** arp status together, so a voicing played both arpeggiated in one passage and plainly strummed in another gets two separate templates instead of one shared template "leaking" the arpeggio flag onto the unrelated strum (caught in review on PR #78). - **keys_highway_3d now respects the mastery/difficulty slider** (#67). The plugin fetches `notation_measures` over its own private `/ws/highway` connection and never consulted the host's difficulty-filtered note stream, so lowering the mastery percentage had no visible effect — the highway always showed the full chart. Notation carries no per-note difficulty tag of its own (it's a structurally separate representation from the tab/gameplay note stream — see feedback-plugin-difficulty-ladder#90), so this correlates each flattened notation event's onset against the chart's phrase windows (now also streamed to this plugin, chunked like `notation_measures`) and the render bundle's ALREADY mastery-filtered tab content (`bundle.notes`/`bundle.chords`): a phrase is "playable" when the tab has any filtered note/chord in that phrase's `[start_time, end_time)` window, and a notation event is kept when its onset falls inside a playable phrase. Recomputes (and rebuilds note meshes + resets scoring) whenever `bundle.mastery` changes; a fixed-difficulty chart with no phrase data is unaffected. ### Changed diff --git a/lib/gp2rs.py b/lib/gp2rs.py index f05d4b2d..7ec33cb5 100644 --- a/lib/gp2rs.py +++ b/lib/gp2rs.py @@ -1037,7 +1037,15 @@ def convert_track( for n in beat_notes: if 0 <= n.string < width: frets[n.string] = n.fret - fret_key = tuple(frets) + # Arpeggio is part of the template's identity, not a + # backfilled attribute: the same voicing can be strummed + # in one passage and arpeggiated in another, and each + # must render correctly on its own. Folding is_arp into + # the dedup key (rather than mutating a shared template's + # `arp` after the fact) keeps those two uses from + # "leaking" into each other via one shared ChordTemplate. + is_arp = _gp_beat_arpeggio(beat.effect) + fret_key = (tuple(frets), is_arp) if fret_key not in chord_template_map: idx = len(chord_templates) @@ -1045,19 +1053,12 @@ def convert_track( name="", frets=list(frets), fingers=[-1] * width, + arp=is_arp, )) chord_template_map[fret_key] = idx else: idx = chord_template_map[fret_key] - # Arpeggio is a per-beat articulation, not diagram data, so - # it attaches regardless of whether this beat also carries - # a chord diagram. Templates dedupe by fret_key, so one - # arpeggiated beat marks every strum sharing that voicing - # — same template-level model as the name/fingers back-fill. - if _gp_beat_arpeggio(beat.effect): - chord_templates[idx].arp = True - # Enrich the template from the GP chord diagram attached to # this beat — but ONLY when the diagram describes the voicing # actually played (same width-normalized fret pattern). A diff --git a/tests/test_gp2rs.py b/tests/test_gp2rs.py index d8e18b86..c50ad5a3 100644 --- a/tests/test_gp2rs.py +++ b/tests/test_gp2rs.py @@ -1461,6 +1461,33 @@ def test_arpeggio_flag_survives_xml_to_wire_round_trip(tmp_path): assert chord_template_to_wire(arr.chord_templates[0])["arp"] is True +def test_arpeggio_does_not_leak_to_other_strums_of_same_voicing(): + """A voicing played BOTH arpeggiated (beat 1) and plainly strummed (beat 2) + elsewhere in the song must not share one ChordTemplate — otherwise marking + the first beat's template `arp=True` would incorrectly mark the second, + unrelated strum as an arpeggio too (Codacy review on PR #78).""" + arp_beat = _arp_chord_beat() + arp_beat.effect.stroke = SimpleNamespace( + direction=guitarpro.BeatStrokeDirection.down, value=64) + strum_beat = _arp_chord_beat() # identical voicing, no stroke + strum_beat.start = GP_TICKS_PER_QUARTER + + xml_str = convert_track( + _ct_song([arp_beat, strum_beat]), track_index=0) + root = safe_xml.safe_fromstring(xml_str) + templates = root.findall(".//chordTemplates/chordTemplate") + assert len(templates) == 2, \ + "same voicing with different arp status must get separate templates" + + chords = root.findall(".//chords/chord") + assert len(chords) == 2 + arp_ct_id = int(chords[0].get("chordId")) + strum_ct_id = int(chords[1].get("chordId")) + assert arp_ct_id != strum_ct_id + assert templates[arp_ct_id].get("arp") == "1" + assert templates[strum_ct_id].get("arp") is None + + 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