Preserve Guitar Pro strum direction - #2
Merged
Conversation
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 <Beat><Stroke><Direction> 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#1054. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
…ickStroke 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 <noreply@anthropic.com>
…troke Same class of bug as the GP4/5-side fix on this branch: the original _gpif_pick_direction guessed a <Beat><Stroke><Direction> shape that doesn't exist in any real GPIF export. Verified against real GP8 files: the actual chord-strum marking is <Properties><Property name="Brush"> <Direction>Down|Up</Direction></Property></Properties> — the direct GPIF equivalent of GP4/5's beat.effect.stroke (see gp2rs.py's analogous fix). <Arpeggio>Down|Up</Arpeggio> (a direct <Beat> 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 <noreply@anthropic.com>
carochacs
marked this pull request as ready for review
July 31, 2026 20:00
This was referenced Jul 31, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Why
Authored up/down strumming direction was detected inconsistently and then lost during Guitar Pro conversion, reducing chart fidelity.
User impact
Imported guitar chords retain their authored stroke direction while charts without an explicit direction remain unset.
Validation
Related feedpakr issue: get-flashbacks/feedBack-plugin-feedpakr#18