Skip to content

feat(gp): continue tied notes (extend the held note's duration) - #53

Merged
PhysShell merged 3 commits into
mainfrom
claude/gp-tied-notes
Jun 15, 2026
Merged

feat(gp): continue tied notes (extend the held note's duration)#53
PhysShell merged 3 commits into
mainfrom
claude/gp-tied-notes

Conversation

@PhysShell

@PhysShell PhysShell commented Jun 15, 2026

Copy link
Copy Markdown
Owner

What

GP import fidelity, slice 2 (S3, ADR-0018): tied notes (NoteType::Tie) now continue the preceding note on their string instead of being dropped.

Before, a tie was skipped to LossReport, which truncated the held note's duration — a half-note written as two tied quarters imported as a single quarter plus a dropped note. Now the tie extends the most recent note on its string by the tie beat's duration, and an all-tie beat emits no event of its own (the held note carries the sound).

How (TDD: refactor → red → green)

  • refactor — replace build_event_group (one beat → one group) with append_beat, which pushes onto the voice's group list and threads a VoiceAccum (groups + a per-string held-note map + loss). Behaviour unchanged; the dead-note characterization test moves to the new seam.
  • test (red) — a struck note then a tie on the same string must yield one note of the summed duration; fails while ties are skipped (stray rest, original duration).
  • feat (green)extend_tie lengthens the held note; orphan ties (no preceding note on the string) remain a loss. Module loss note updated.

Notes

  • Assumes a tie continues the immediately-preceding sound on its string (well-formed GP); a malformed orphan tie is reported, not silently mis-extended.

Checks

  • cargo test --workspace green (502 tests; new tied_note_extends_previous_note_duration)
  • cargo clippy --all-targets -- -D warnings clean; gp.rs rustfmt-clean

https://claude.ai/code/session_01TTUbGjzD8ysnVnCJnZJE95


Generated by Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • Fixed handling of tied notes in Guitar Pro files to properly extend note durations.
    • Improved error reporting when encountering invalid ties without preceding notes.

claude added 3 commits June 15, 2026 09:03
Replaces build_event_group (one beat -> one EventGroup) with append_beat,
which pushes onto the voice's growing group list and threads a VoiceAccum
(groups + a per-string held-note map + loss). Behaviour is unchanged —
ties are still skipped to loss — but the held map is now populated, ready
for the next slice to consume. The dead-note characterization test moves
to the new seam.
A NoteType::Tie continues the note on its string, so it must lengthen that
note rather than be dropped. The importer still skips ties, so this fails:
the tie beat emits a stray rest and the held note keeps its original
duration.
NoteType::Tie previously dropped the tied beat to loss, truncating the
held note. Now a tie extends the most recent note on its string by the tie
beat's duration (extend_tie); an all-tie beat emits no event of its own.
Orphan ties (no preceding note on the string) remain a loss. The module
loss note is updated.
@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: 18d9216b-f3f8-4fa3-8887-f443b528004f

📥 Commits

Reviewing files that changed from the base of the PR and between dc5509c and 5607ceb.

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

📝 Walkthrough

Walkthrough

Refactors GP beat processing in core/src/gp.rs to support tied notes. Introduces a VoiceAccum struct that threads per-voice mutable state (held-note map, event groups, loss report) through a new append_beat helper, replacing the previous build_event_group call. Ties extend the held note's duration instead of emitting a new event group; orphan ties record a loss.

Changes

GP Tied-Note Beat Processing

Layer / File(s) Summary
VoiceAccum struct and build_gp_voice wiring
core/src/gp.rs
Adds HashMap import, defines VoiceAccum bundling event_groups, held map, and loss, replaces the per-beat build_event_group call with append_beat, and adjusts the module-level doc comment on LossReport.
append_beat implementation with tie and held-note logic
core/src/gp.rs
Implements append_beat: rest/empty beats push a rest group and return; normal/dead note beats iterate notes, convert pitches, collect TechniqueSpans, record atom_index in the held map, and record out-of-range losses. Unsupported notes record losses through the accumulator. All-tie beats emit no new EventGroup; otherwise atoms are wrapped into a single-note or chord group. Adds extend_tie which extends a held note's duration or records an orphan-tie loss.
Updated dead-note test and new tied-note test
core/src/gp.rs
Adds HashMap import to the test module, updates the dead-note test to use append_beat/VoiceAccum, and adds a new test asserting that NoteType::Tie extends the prior note's duration without creating a new event group.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

  • PhysShell/griff#51: Both PRs modify core/src/gp.rs beat/event-group handling — the retrieved PR changes dead-note mapping to NoteMark::DeadNote through map_gp_note_marks, the same technique-span path that this PR threads through append_beat.

Poem

🐰 A note rings out, its string still warm,
A tie arrives to stretch its form.
No new event — just longer sound!
In held the last-struck pitch is found.
The rabbit maps each beat with care,
Orphan ties? A loss declared.
hop 🎸

🚥 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 clearly and specifically describes the main feature: support for tied notes by extending held note duration, which is the primary change in the refactored beat processing logic.
Docstring Coverage ✅ Passed Docstring coverage is 85.71% 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/gp-tied-notes

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