From f88844177df58c80032e7690fb314dc016d36c0f Mon Sep 17 00:00:00 2001 From: Raphael Morrissy <54593695+raphaelmorrissy@users.noreply.github.com> Date: Mon, 31 Aug 2026 01:20:06 -0300 Subject: [PATCH 1/2] fix: preserve Guitar Pro pick direction Signed-off-by: Raphael Morrissy <54593695+raphaelmorrissy@users.noreply.github.com> --- CHANGELOG.md | 7 ++++-- lib/gp2rs.py | 31 +++++++++++++++++++++++ lib/gp2rs_gpx.py | 15 +++++++++++ tests/test_gp2rs.py | 55 +++++++++++++++++++++++++++++++++++++++++ tests/test_gp2rs_gpx.py | 36 +++++++++++++++++++++++++++ 5 files changed, 142 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2c262781..98b6064a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -632,8 +632,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Security - **Path traversal in archive extractors and library path resolution.** `lib/sloppak.py::_unpack_zip` and `server.py::_resolve_dlc_path` previously concatenated attacker-controlled entry names or filenames directly onto the extraction or library directory, so a crafted sloppak zip member or library filename with `..` segments, an absolute path, or backslash separators could write or read outside the intended directory. Any code path that unpacks a user-supplied archive (library upload, click-to-play, retune) or resolves a library path was reachable. Both locations now delegate to a new `lib/safepath.py::safe_join` helper that resolves each destination once and rejects entries that don't fall under the target directory; rejected entries are logged and skipped, the rest of the archive still extracts. The stem-split paths in `lib/sloppak_convert.py::split_stems` and `scripts/split_stems.py` previously called `ZipFile.extractall()` directly on user-supplied sloppaks; both now delegate to the same hardened `lib/sloppak.py::_unpack_zip` so every sloppak-unzip site in the codebase shares one containment guarantee. Tests in `tests/test_archive_traversal.py` and `tests/test_safepath.py` pin the contract for `../`, deep traversal, absolute paths, mixed `subdir/../../` forms, Windows-style separators, NUL bytes, names that resolve to the unpack root, and symlinked roots. -### Fixed -- E Standard retune now stays metadata-consistent across a chart's arrangement files (feedBack-plugin-notedetect#50). Previously the retune path could shift the audio and update manifests while leaving some arrangement metadata untouched, so `load_song()` later exposed the *original* tuning at runtime. `lib/retune.py` now updates every arrangement's tuning metadata consistently before applying the E Standard tuning, and raises on a partial update instead of silently packing split tuning metadata. EStd files generated before this fix should be re-converted so their metadata is consistent. +### Fixed +- Guitar Pro imports now preserve explicitly authored up/down pick strokes for + both GP3/4/5 and GP6/7/8 files instead of leaving every note's + `pickDirection` unset (#1054). +- E Standard retune now stays metadata-consistent across a chart's arrangement files (feedBack-plugin-notedetect#50). Previously the retune path could shift the audio and update manifests while leaving some arrangement metadata untouched, so `load_song()` later exposed the *original* tuning at runtime. `lib/retune.py` now updates every arrangement's tuning metadata consistently before applying the E Standard tuning, and raises on a partial update instead of silently packing split tuning metadata. EStd files generated before this fix should be re-converted so their metadata is consistent. - Keyboard shortcut help now opens from the Player/3D Highway context when Linux/Electron reports Shift+Slash as `key="/"`, including while player controls such as the visualization picker are focused (#598). - 3D Highway left-handed mode now has regression coverage for fret-axis mirroring, board rebuilds on runtime lefty changes, and mirrored camera state including the lookahead target and shoulder offset; the maintainer guide no longer claims the renderer ignores `bundle.lefty` (#321). - Chord-level `fretHandMute` is now parsed into each note's `fret_hand_mute` (wire `fhm`) instead of being folded into `mute` (`mt`), matching `_parse_note` and preserving wire-format fidelity for both the template-expanded (synthetic-note) and explicit-`chordNote` paths. The 3D highway renders the fret-hand-mute X for `mt` *or* `fhm` notes, so the muted-chord overlay still shows. Also fixes the per-note fret-connector label vanishing exactly at the hit line (the fade now holds full opacity through `dt = 0`). diff --git a/lib/gp2rs.py b/lib/gp2rs.py index 48a8ee72..bf1bb88a 100644 --- a/lib/gp2rs.py +++ b/lib/gp2rs.py @@ -72,6 +72,9 @@ class RsNote: tremolo: bool = False tap: bool = False link_next: bool = False + # Teaching mark (§6.2.2): 0 down-stroke, 1 up-stroke, -1 unset. + # Display only — never used for grading. + pick_direction: int = -1 # Teaching mark (§6.2.2): fret-hand finger (-1 unset, 0 thumb..4 pinky). # Display only — never used for grading. fret_finger: int = -1 @@ -268,6 +271,30 @@ def _finger_xml_attrs(n: "RsNote") -> dict: return {} +def _pick_direction_xml_attrs(n: "RsNote") -> dict: + """Optional ``pickDirection`` XML attribute for a converted note.""" + if getattr(n, "pick_direction", -1) in (0, 1): + return {"pickDirection": str(int(n.pick_direction))} + return {} + + +def _gp_beat_pick_direction(beat_effect) -> int: + """Map a PyGuitarPro beat's authored stroke to the feedBack convention. + + PyGuitarPro represents a pick stroke directly as ``pickStroke`` and older + brush/strum effects as ``stroke.direction``. Both use ``up=1, down=2``; + feedBack/RS XML uses ``down=0, up=1``. Prefer the explicit pick stroke and + fall back to the legacy beat stroke. Missing/unknown values stay unset. + """ + if beat_effect is None: + return -1 + direction = getattr(beat_effect, "pickStroke", None) + if getattr(direction, "name", "none").lower() == "none": + direction = getattr(getattr(beat_effect, "stroke", None), "direction", None) + name = getattr(direction, "name", "").lower() + return {"down": 0, "up": 1}.get(name, -1) + + def _tempo_at_tick(tick: int, tempo_map: list[TempoEvent]) -> float: """Get the tempo at a given tick.""" result = tempo_map[0].tempo @@ -796,6 +823,7 @@ def convert_track( + entry.output_start_secs + audio_offset tempo = _tempo_at_tick(beat.start, tempo_map) dur = _duration_to_seconds(beat.duration, tempo) + pick_direction = _gp_beat_pick_direction(beat.effect) beat_notes = [] for note in beat.notes: @@ -820,6 +848,7 @@ def convert_track( fret=fret, sustain=dur if dur > 0.2 else 0.0, mute=note.type == guitarpro.NoteType.dead, + pick_direction=pick_direction, ) # Techniques @@ -1213,6 +1242,7 @@ 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 @@ -1246,6 +1276,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..59050c5a 100644 --- a/lib/gp2rs_gpx.py +++ b/lib/gp2rs_gpx.py @@ -903,6 +903,17 @@ def _beat_has_tremolo(beat_el: ET.Element) -> bool: return beat_el.find('Tremolo') is not None +def _gpif_pick_direction(beat_el: ET.Element) -> int: + """Map GPIF ``Stroke/Direction`` to feedBack's pick-direction values. + + GPIF writes the direction as a case-insensitive ``Up`` or ``Down`` token. + feedBack/RS XML uses ``0`` for down and ``1`` for up; absent or unknown + tokens remain ``-1`` (unset) rather than fabricating a direction. + """ + raw = (beat_el.findtext('Stroke/Direction') or '').strip().lower() + return {'down': 0, 'up': 1}.get(raw, -1) + + # --------------------------------------------------------------------------- # list_tracks — mirrors gp2rs.list_tracks interface # --------------------------------------------------------------------------- @@ -1771,6 +1782,7 @@ def _auto_guitar_hint(track_idx: int): dur = _beat_dur_secs(beat_el, rhythms_dict, _cur_tempo) t = voice_time + audio_offset + pick_direction = _gpif_pick_direction(beat_el) notes_text = beat_el.findtext('Notes', '').strip() if notes_text: @@ -1856,6 +1868,9 @@ def _auto_guitar_hint(track_idx: int): string=rs_str, fret=rs_fret, sustain=sustain, + pick_direction=( + pick_direction if not (is_drum or is_keys) else -1 + ), ) # Techniques — GPIF stores these as diff --git a/tests/test_gp2rs.py b/tests/test_gp2rs.py index bbaf6114..4fa2b6d2 100644 --- a/tests/test_gp2rs.py +++ b/tests/test_gp2rs.py @@ -25,6 +25,7 @@ _compute_tuning, _extract_year, _gp_bend_shape, + _gp_beat_pick_direction, _gp_string_to_rs, _is_bass_track, _standard_tuning_for, @@ -869,6 +870,60 @@ def test_tied_note_without_predecessor_is_silently_dropped(): assert len(notes) == 0 +@pytest.mark.parametrize("direction, expected", [ + (guitarpro.models.BeatStrokeDirection.down, 0), + (guitarpro.models.BeatStrokeDirection.up, 1), + (guitarpro.models.BeatStrokeDirection.none, -1), +]) +def test_gp_pick_stroke_direction_mapping(direction, expected): + effect = SimpleNamespace( + pickStroke=direction, + stroke=SimpleNamespace( + direction=guitarpro.models.BeatStrokeDirection.none), + ) + assert _gp_beat_pick_direction(effect) == expected + + +def test_gp_legacy_beat_stroke_is_fallback(): + effect = SimpleNamespace( + pickStroke=guitarpro.models.BeatStrokeDirection.none, + stroke=SimpleNamespace( + direction=guitarpro.models.BeatStrokeDirection.up), + ) + assert _gp_beat_pick_direction(effect) == 1 + + +@pytest.mark.parametrize("direction, expected", [ + (guitarpro.models.BeatStrokeDirection.down, "0"), + (guitarpro.models.BeatStrokeDirection.up, "1"), +]) +def test_convert_track_preserves_pick_direction(direction, expected): + beat = _ct_beat( + tick=0, + dur_value=4, + notes=[_ct_note(guitarpro.NoteType.normal, gp_string=1, fret=5)], + ) + beat.effect.pickStroke = direction + root = ET.fromstring(convert_track(_ct_song([beat]), track_index=0)) # noqa: S314 + assert root.find(".//notes/note").get("pickDirection") == expected + + +def test_convert_track_preserves_pick_direction_on_every_chord_note(): + beat = _ct_beat( + tick=0, + dur_value=4, + notes=[ + _ct_note(guitarpro.NoteType.normal, gp_string=1, fret=3), + _ct_note(guitarpro.NoteType.normal, gp_string=2, fret=2), + ], + ) + beat.effect.pickStroke = guitarpro.models.BeatStrokeDirection.down + root = ET.fromstring(convert_track(_ct_song([beat]), track_index=0)) # noqa: S314 + chord_notes = root.findall(".//chords/chord/chordNote") + assert len(chord_notes) == 2 + assert {note.get("pickDirection") for note in chord_notes} == {"0"} + + # ── convert_track: bend shape (bn / bt / bnv, §6.2.1) ──────────────────────── def _ct_bend(points): diff --git a/tests/test_gp2rs_gpx.py b/tests/test_gp2rs_gpx.py index 4b91106a..62bfaca1 100644 --- a/tests/test_gp2rs_gpx.py +++ b/tests/test_gp2rs_gpx.py @@ -22,6 +22,7 @@ _note_is_tie, _note_has_vibrato, _beat_has_tremolo, + _gpif_pick_direction, _note_midi, _gpx_percussion_midis, _gpx_tuning, @@ -681,6 +682,25 @@ def test_convert_file_gp8_tremolo_beat_flags_note(tmp_path, monkeypatch): assert tremolo_by_string[5] == "0" +def test_convert_file_gp8_preserves_stroke_direction(tmp_path, monkeypatch): + gpif = _GPIF_GUITAR_TREMOLO.replace( + '1/8', + 'Up', + ) + monkeypatch.setattr(gp2rs_gpx, "_load_gpif", lambda _p: ET.fromstring(gpif)) + out_files = convert_file( + "dummy.gp", str(tmp_path), + track_indices=[0], arrangement_names={0: "Lead"}, + ) + root = ET.parse(out_files[0]).getroot() + direction_by_string = { + int(note.get("string")): note.get("pickDirection") + for note in root.iter() if note.tag == "note" + } + assert direction_by_string[0] == "1" + assert direction_by_string[5] is None + + def test_vocal_pitch_sidecar_sorts_multi_voice_by_time(): # Two voices in one bar. Voice 0 (traversed first) emits its lyric note at # t=0.5 (a no-lyric quarter precedes it); voice 1 (traversed second) emits @@ -889,6 +909,22 @@ def test_beat_tremolo_independent_of_whammy_trembar(): assert _beat_has_tremolo(b) is False +@pytest.mark.parametrize("token, expected", [ + ("Down", 0), + ("Up", 1), + ("down", 0), + ("Sideways", -1), +]) +def test_gpif_pick_direction_mapping(token, expected): + beat = ET.fromstring( + f'{token}') + assert _gpif_pick_direction(beat) == expected + + +def test_gpif_pick_direction_absent_is_unset(): + assert _gpif_pick_direction(ET.fromstring('')) == -1 + + # ── _gpif_left_fingering (GP7/GP8 per-note fret-hand finger -> fg) ─────────── # GPIF stores a single note's fret-hand finger as a direct # child of (NOT a ), with classical p-i-m-a-c letter codes — From 88f5479cbec8881ec6244d2ec304bd6739f89d38 Mon Sep 17 00:00:00 2001 From: Raphael Morrissy <54593695+raphaelmorrissy@users.noreply.github.com> Date: Mon, 31 Aug 2026 01:57:00 -0300 Subject: [PATCH 2/2] chore: address review feedback Signed-off-by: Raphael Morrissy <54593695+raphaelmorrissy@users.noreply.github.com> --- CHANGELOG.md | 4 ++-- tests/test_gp2rs.py | 4 ++++ tests/test_gp2rs_gpx.py | 3 +++ 3 files changed, 9 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 98b6064a..a29b0b78 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -632,11 +632,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Security - **Path traversal in archive extractors and library path resolution.** `lib/sloppak.py::_unpack_zip` and `server.py::_resolve_dlc_path` previously concatenated attacker-controlled entry names or filenames directly onto the extraction or library directory, so a crafted sloppak zip member or library filename with `..` segments, an absolute path, or backslash separators could write or read outside the intended directory. Any code path that unpacks a user-supplied archive (library upload, click-to-play, retune) or resolves a library path was reachable. Both locations now delegate to a new `lib/safepath.py::safe_join` helper that resolves each destination once and rejects entries that don't fall under the target directory; rejected entries are logged and skipped, the rest of the archive still extracts. The stem-split paths in `lib/sloppak_convert.py::split_stems` and `scripts/split_stems.py` previously called `ZipFile.extractall()` directly on user-supplied sloppaks; both now delegate to the same hardened `lib/sloppak.py::_unpack_zip` so every sloppak-unzip site in the codebase shares one containment guarantee. Tests in `tests/test_archive_traversal.py` and `tests/test_safepath.py` pin the contract for `../`, deep traversal, absolute paths, mixed `subdir/../../` forms, Windows-style separators, NUL bytes, names that resolve to the unpack root, and symlinked roots. -### Fixed +### Fixed - Guitar Pro imports now preserve explicitly authored up/down pick strokes for both GP3/4/5 and GP6/7/8 files instead of leaving every note's `pickDirection` unset (#1054). -- E Standard retune now stays metadata-consistent across a chart's arrangement files (feedBack-plugin-notedetect#50). Previously the retune path could shift the audio and update manifests while leaving some arrangement metadata untouched, so `load_song()` later exposed the *original* tuning at runtime. `lib/retune.py` now updates every arrangement's tuning metadata consistently before applying the E Standard tuning, and raises on a partial update instead of silently packing split tuning metadata. EStd files generated before this fix should be re-converted so their metadata is consistent. +- E Standard retune now stays metadata-consistent across a chart's arrangement files (feedBack-plugin-notedetect#50). Previously the retune path could shift the audio and update manifests while leaving some arrangement metadata untouched, so `load_song()` later exposed the *original* tuning at runtime. `lib/retune.py` now updates every arrangement's tuning metadata consistently before applying the E Standard tuning, and raises on a partial update instead of silently packing split tuning metadata. EStd files generated before this fix should be re-converted so their metadata is consistent. - Keyboard shortcut help now opens from the Player/3D Highway context when Linux/Electron reports Shift+Slash as `key="/"`, including while player controls such as the visualization picker are focused (#598). - 3D Highway left-handed mode now has regression coverage for fret-axis mirroring, board rebuilds on runtime lefty changes, and mirrored camera state including the lookahead target and shoulder offset; the maintainer guide no longer claims the renderer ignores `bundle.lefty` (#321). - Chord-level `fretHandMute` is now parsed into each note's `fret_hand_mute` (wire `fhm`) instead of being folded into `mute` (`mt`), matching `_parse_note` and preserving wire-format fidelity for both the template-expanded (synthetic-note) and explicit-`chordNote` paths. The 3D highway renders the fret-hand-mute X for `mt` *or* `fhm` notes, so the muted-chord overlay still shows. Also fixes the per-note fret-connector label vanishing exactly at the hit line (the fade now holds full opacity through `dt = 0`). diff --git a/tests/test_gp2rs.py b/tests/test_gp2rs.py index 4fa2b6d2..a1624ce5 100644 --- a/tests/test_gp2rs.py +++ b/tests/test_gp2rs.py @@ -876,6 +876,7 @@ def test_tied_note_without_predecessor_is_silently_dropped(): (guitarpro.models.BeatStrokeDirection.none, -1), ]) def test_gp_pick_stroke_direction_mapping(direction, expected): + """Map each PyGuitarPro pick-stroke enum to the feedBack wire value.""" effect = SimpleNamespace( pickStroke=direction, stroke=SimpleNamespace( @@ -885,6 +886,7 @@ def test_gp_pick_stroke_direction_mapping(direction, expected): def test_gp_legacy_beat_stroke_is_fallback(): + """Use the legacy beat stroke when no explicit pick stroke is authored.""" effect = SimpleNamespace( pickStroke=guitarpro.models.BeatStrokeDirection.none, stroke=SimpleNamespace( @@ -898,6 +900,7 @@ def test_gp_legacy_beat_stroke_is_fallback(): (guitarpro.models.BeatStrokeDirection.up, "1"), ]) def test_convert_track_preserves_pick_direction(direction, expected): + """Serialize an authored pick direction on a converted standalone note.""" beat = _ct_beat( tick=0, dur_value=4, @@ -909,6 +912,7 @@ def test_convert_track_preserves_pick_direction(direction, expected): def test_convert_track_preserves_pick_direction_on_every_chord_note(): + """Serialize the beat's pick direction on every converted chord note.""" beat = _ct_beat( tick=0, dur_value=4, diff --git a/tests/test_gp2rs_gpx.py b/tests/test_gp2rs_gpx.py index 62bfaca1..6a81b219 100644 --- a/tests/test_gp2rs_gpx.py +++ b/tests/test_gp2rs_gpx.py @@ -683,6 +683,7 @@ def test_convert_file_gp8_tremolo_beat_flags_note(tmp_path, monkeypatch): def test_convert_file_gp8_preserves_stroke_direction(tmp_path, monkeypatch): + """Preserve GPIF stroke direction without assigning it to other beats.""" gpif = _GPIF_GUITAR_TREMOLO.replace( '1/8', 'Up', @@ -916,12 +917,14 @@ def test_beat_tremolo_independent_of_whammy_trembar(): ("Sideways", -1), ]) def test_gpif_pick_direction_mapping(token, expected): + """Map supported GPIF direction tokens and reject unknown directions.""" beat = ET.fromstring( f'{token}') assert _gpif_pick_direction(beat) == expected def test_gpif_pick_direction_absent_is_unset(): + """Leave pick direction unset when a GPIF beat has no stroke element.""" assert _gpif_pick_direction(ET.fromstring('')) == -1