Skip to content

fix: preserve Guitar Pro pick direction - #1068

Open
raphaelmorrissy wants to merge 2 commits into
got-feedBack:mainfrom
raphaelmorrissy:fix/1054-preserve-pick-direction
Open

fix: preserve Guitar Pro pick direction#1068
raphaelmorrissy wants to merge 2 commits into
got-feedBack:mainfrom
raphaelmorrissy:fix/1054-preserve-pick-direction

Conversation

@raphaelmorrissy

@raphaelmorrissy raphaelmorrissy commented Aug 31, 2026

Copy link
Copy Markdown

What

Preserve explicitly authored Guitar Pro pick/strum direction during import instead of leaving every converted note's pickDirection unset.

  • maps PyGuitarPro pickStroke and legacy stroke.direction values to feedBack's down = 0 / up = 1 convention
  • reads GP6/7/8 GPIF <Stroke><Direction> values
  • writes pickDirection for standalone notes and every note in a chord
  • leaves missing or unknown directions unset (-1) and does not fabricate strum_group values
  • documents the behavior in the changelog and regression tests

Closes #1054

feedpak surface

  • This PR does not change how the app reads/writes feedpaks (manifest keys, pack files, folder layout)
  • …or it does, and the spec change landed first via the FEP process — FEP / spec PR: got-feedback/feedpak-spec#___

Checklist

  • CHANGELOG.md [Unreleased] updated (user-visible changes)
  • Tests added/updated for new behaviour
  • Commits are DCO signed off (git commit -s)

Testing

  • 391 passed across the GP converters, XML encoding, and song model suites
  • Full Python suite: 2769 passed, 32 skipped; 12 unrelated Windows/environment failures (symlink privileges, path/EOL, and packaging checks)
  • py_compile and git diff --check pass

Summary by CodeRabbit

  • Bug Fixes

    • Guitar Pro imports now preserve authored up/down picking directions across GP3–GP8 files.
    • Picking directions are retained for both individual notes and chord notes.
    • E Standard retuning now keeps tuning metadata consistent across all arrangement files.
    • Retuning fails clearly if metadata cannot be updated everywhere, preventing inconsistent results.
  • Tests

    • Added coverage for picking-direction imports, legacy direction data, chord handling, and GP8 files.

Signed-off-by: Raphael Morrissy <54593695+raphaelmorrissy@users.noreply.github.com>
@coderabbitai

coderabbitai Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: c0bf728b-33c9-449c-9fa9-15ebad98a924

📥 Commits

Reviewing files that changed from the base of the PR and between f888441 and 88f5479.

📒 Files selected for processing (3)
  • CHANGELOG.md
  • tests/test_gp2rs.py
  • tests/test_gp2rs_gpx.py
🚧 Files skipped from review as they are similar to previous changes (1)
  • tests/test_gp2rs.py

Included review availability: Your plan provides up to 2 included reviews per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

Guitar Pro imports now preserve authored up/down pick directions for GP3/4/5 and GP6/7/8 files. The direction is stored on imported notes and serialized as pickDirection for single notes and chord notes. Tests cover mappings, fallbacks, track handling, and XML output.

Changes

Pick-direction import

Layer / File(s) Summary
GP3/4/5 pick-direction propagation
lib/gp2rs.py, tests/test_gp2rs.py, CHANGELOG.md
The importer maps pickStroke and legacy stroke values to RS directions. It stores the value on each beat note and emits pickDirection for notes and chord notes. Tests cover mappings, fallback behavior, and XML output.
GP6/7/8 pick-direction propagation
lib/gp2rs_gpx.py, tests/test_gp2rs_gpx.py
The GPIF importer maps case-insensitive Stroke/Direction tokens for guitar and bass tracks. Drum and keys notes remain unset. Tests cover token mapping, absent strokes, and GP8 conversion output.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to 88f54

This PR preserves authored Guitar Pro pick directions during import while leaving unknown values unset; no actionable merge-blocking risk remains after normal checks and review.

Sequence Diagram(s)

sequenceDiagram
  participant GuitarProFile
  participant GPImporter
  participant RsNote
  participant RSXML
  GuitarProFile->>GPImporter: provide beat stroke direction
  GPImporter->>RsNote: map and assign pick_direction
  RsNote->>RSXML: provide note metadata
  RSXML->>RSXML: emit pickDirection when set
Loading
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: preserving Guitar Pro pick direction during import.
Description check ✅ Passed The description includes the required What, feedpak surface, and Checklist sections. It documents the behavior, linked issue, tests, changelog update, and DCO status.
Linked Issues check ✅ Passed The changes satisfy issue #1054. Both Guitar Pro import paths read source pick-direction values, map them to feedBack values, preserve directions for standalone and chord notes, and leave missing or u…
Out of Scope Changes check ✅ Passed The changes remain within scope. They update the two Guitar Pro importers, add regression tests, and update the changelog. No unrelated code or feedpak format changes are present.
Docstring Coverage ✅ Passed Docstring coverage is 80.95% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 21 functions across 4 files. (1 skipped: 1 …
Full details: Linked Issues check

Explanation

The changes satisfy issue #1054. Both Guitar Pro import paths read source pick-direction values, map them to feedBack values, preserve directions for standalone and chord notes, and leave missing or unknown values unset. The PR does not fabricate strum_group values.

Full details: Docstring Coverage

Explanation

Docstring coverage is 80.95% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 21 functions across 4 files. (1 skipped: 1 unsupported.)

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@raphaelmorrissy

Copy link
Copy Markdown
Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor
⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@raphaelmorrissy

Copy link
Copy Markdown
Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Signed-off-by: Raphael Morrissy <54593695+raphaelmorrissy@users.noreply.github.com>
@raphaelmorrissy

Copy link
Copy Markdown
Author

@ChrisBeWithYou, when you have a moment, could you please review this GP importer fix? CodeRabbit reports no actionable comments, all 5 pre-merge checks pass, and the related converter tests are green. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

GP import discards strumming direction (pick_direction stays unset for every imported note)

1 participant