Skip to content

feat(gp): import dead notes and bends (GP import fidelity) - #51

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

feat(gp): import dead notes and bends (GP import fidelity)#51
PhysShell merged 2 commits into
mainfrom
claude/modest-maxwell-0zecwn

Conversation

@PhysShell

@PhysShell PhysShell commented Jun 15, 2026

Copy link
Copy Markdown
Owner

What

GP import fidelity (S3, ADR-0018): two techniques the importer used to discard to LossReport now map onto the canonical model.

  • NoteType::Dead (a muted "X") → a positioned note carrying NoteMark::DeadNote (keeps its string/fret instead of being skipped).
  • effect.bend → a Bend TechniqueSpan (was never mapped at all).

Why

S3 made Guitar Pro a first-class input, but dead notes and bends — both representable in the canonical model — were dropped, losing real musical content from imported tabs.

TDD

Honest red → green in history:

  • test(gp)dead_note_imports_as_dead_marked_note, gp_bend_emits_explicit_span (red, failing for the right reason)
  • feat(gp) — implementation + the module loss note updated (dead notes are no longer a loss)

Checks

  • cargo test -p griff-core green (incl. the two new gp tests; 26 in the module)
  • cargo clippy -p griff-core --lib -- -D warnings clean; gp.rs is rustfmt-clean

https://claude.ai/code/session_01TTUbGjzD8ysnVnCJnZJE95


Generated by Claude Code

Summary by CodeRabbit

  • New Features

    • Bend effects are now properly represented in imported Guitar Pro files.
  • Bug Fixes

    • Dead notes now import correctly with their string and fret positions preserved.
  • Documentation

    • Updated documentation to reflect improved handling of dead notes, percussion tracks, and tied notes.
  • Tests

    • Added test coverage for bend effect handling and dead note importing.

claude added 2 commits June 15, 2026 05:42
Two fidelity gaps in the GP importer, each representable in the canonical
model but currently dropped:
- NoteType::Dead notes carry a real (string, fret) but are skipped to
  loss; they should import as a positioned note bearing NoteMark::DeadNote.
- effect.bend is never mapped; it should surface as a Bend TechniqueSpan
  (ADR-0018, Guitar Pro is source-of-truth).
Two representable GP techniques were discarded to loss; both now map onto
the canonical model (ADR-0018):
- NoteType::Dead -> a positioned note carrying NoteMark::DeadNote (keeps the
  muted hit's string/fret instead of skipping it).
- effect.bend -> a Bend TechniqueSpan.

The module loss note is updated: dead notes are no longer a loss.
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

@coderabbitai

coderabbitai Bot commented Jun 15, 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: 1899cc7e-85cb-4378-bb48-9e362b71a9bb

📥 Commits

Reviewing files that changed from the base of the PR and between ab552e7 and b4895fc.

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

📝 Walkthrough

Walkthrough

In core/src/gp.rs, NoteType::Dead is merged into the same import code path as NoteType::Normal, inserting NoteMark::DeadNote while preserving the note's string/fret position. map_gp_note_marks now emits a TechniqueSpan::Bend when a bend effect is present. Module docs and unit tests are updated to match.

Changes

GP Importer: dead note and bend span handling

Layer / File(s) Summary
Dead note and bend span implementation
core/src/gp.rs
NoteType::Dead is routed through the same branch as NoteType::Normal; NoteMark::DeadNote is inserted for dead notes while the string/fret position is retained. map_gp_note_marks gains a TechniqueSpan::Bend emission for effect.bend. Module-level doc comment updated to describe the per-technique TechniqueSpan / per-note NoteMark model.
Unit tests for bend and dead note import
core/src/gp.rs
Clippy allow-list extended; BendEffect imported. New tests verify a bend effect produces SpanTechnique::Bend and that NoteType::Dead imports as a positioned note with NoteMark::DeadNote.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐇 A dead note was lost, just skipped with a shrug,
Now it lands on its string with a proper dead thug.
The bend gets its span, no longer ignored,
Each technique accounted for, perfectly stored.
Hop hop, little fret — you're on the score! 🎸

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly and accurately summarizes the main changes: importing dead notes and bends to improve Guitar Pro import fidelity, matching the primary objectives.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
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.

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