Skip to content

feat: import Guitar Pro tapping as a tapped note (#75) - #90

Merged
PhysShell merged 2 commits into
mainfrom
claude/modest-maxwell-0zecwn
Jun 20, 2026
Merged

feat: import Guitar Pro tapping as a tapped note (#75)#90
PhysShell merged 2 commits into
mainfrom
claude/modest-maxwell-0zecwn

Conversation

@PhysShell

@PhysShell PhysShell commented Jun 20, 2026

Copy link
Copy Markdown
Owner

Imports Guitar Pro's two-hand tapping as NoteMark::Tap, finishing #75's missing-technique-parsing asks (alongside let-ring; pull-off was re-scoped — it needs pitch-direction inference, not a field map).

What

  • Tapping in GP is a beat-level effect (BeatEffects::slap_effect == Tapping), not a note effect — so append_beat resolves it once per beat and marks every note struck in the beat with NoteMark::Tap, exactly mirroring the existing dead-note flag (map_gp_note_marks only sees per-note effects, so it stays out of it).
  • Slapping and popping (bass techniques) have no griff mark and are deliberately left unmapped (documented inline).
  • Tapping flows into curation's free-form techniques list as "tap"names-only, like accent/ghost. The passage-level TappingPassage tag is dominance-based and is deliberately not derived per-occurrence (technique.rs), so there's no new SwancoreTag and no SCHEMA_VERSION change.

NoteMark::Tap already existed in the model but no importer set it — this wires it up.

TDD

red (52191a6) → green (495031e).

Verification

  • New tapping_beat_marks_every_note_as_tapped test (builds a tapped Beat, asserts the imported note bears NoteMark::Tap) + full workspace suite green
  • cargo clippy --workspace --all-targets -- -D warnings clean
  • cargo fmt --all --check clean

🤖 Generated with Claude Code

https://claude.ai/code/session_01TTUbGjzD8ysnVnCJnZJE95


Generated by Claude Code

Summary by CodeRabbit

  • New Features

    • Added support for importing Guitar Pro beat-level tapping effects during file processing.
  • Tests

    • Added unit test to verify tapping effects are correctly applied to imported notes.

claude added 2 commits June 20, 2026 01:36
Failing test for mapping Guitar Pro's beat-level tapping (BeatEffects
slap_effect == Tapping) onto NoteMark::Tap for every note struck in the
beat. NoteMark::Tap already exists; append_beat does not set it yet, so the
assertion fails until the green step.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TTUbGjzD8ysnVnCJnZJE95
`append_beat` now reads the beat-level `BeatEffects::slap_effect` and, when
it is `Tapping`, marks every note struck in the beat with `NoteMark::Tap`
— resolved at the beat level like the dead-note flag, since tapping is not
a per-note effect. Slapping and popping (bass techniques) have no griff
mark and are left unmapped.

Tapping now flows into `derive_techniques`' free-form `techniques` list as
"tap" (names-only — like accent/ghost; the passage-level `TappingPassage`
tag is dominance-based and is deliberately not derived per-occurrence).
Finishes #75's missing-technique-parsing asks alongside let-ring.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TTUbGjzD8ysnVnCJnZJE95
@coderabbitai

coderabbitai Bot commented Jun 20, 2026

Copy link
Copy Markdown

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: c0607071-3b31-4ba8-8e1d-956b8395e906

📥 Commits

Reviewing files that changed from the base of the PR and between 8561a9e and 495031e.

📒 Files selected for processing (1)
  • core/src/gp.rs

📝 Walkthrough

Walkthrough

In append_beat, a boolean is derived from beat.effect.slap_effect == SlapEffect::Tapping and used to insert NoteMark::Tap into each normal/dead note's marks set. A unit test constructs a tapping beat and asserts the resulting note carries NoteMark::Tap.

Changes

GP Beat Tapping → NoteMark::Tap

Layer / File(s) Summary
Tapping detection, mark insertion, and test
core/src/gp.rs
append_beat computes a is_tapping boolean from the beat's slap_effect field and conditionally inserts NoteMark::Tap into each normal/dead note's marks set. A new test constructs a beat with slap_effect = Tapping and asserts the imported note includes NoteMark::Tap.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~5 minutes

Possibly related PRs

  • PhysShell/griff#53: Modifies the same append_beat function in core/src/gp.rs for tie handling, directly adjacent to where the tapping flag is now computed.
  • PhysShell/griff#69: derive_techniques in core/src/technique.rs reads NoteMark values to produce technique tags, so NoteMark::Tap emitted by this PR will now feed into that derivation pipeline.

Poem

🐇 Tap tap tap upon the string,
A beat effect, a new mark ring,
SlapEffect::Tapping caught with care,
NoteMark::Tap placed right in there,
The test confirms it, hip hooray—
The rabbit taps its feet today! 🎸

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: importing Guitar Pro tapping technique as a tapped note mark.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/modest-maxwell-0zecwn

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

@PhysShell

Copy link
Copy Markdown
Owner Author

@codex brooo

@chatgpt-codex-connector

Copy link
Copy Markdown

Summary

Testing

  • cargo fmt --all --check
  • cargo clippy --workspace --all-targets -- -D warnings
  • cargo test --workspace

View task →

@PhysShell
PhysShell merged commit be51d2f into main Jun 20, 2026
1 check passed
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.

2 participants