Skip to content

Timing engine: per-note tempo lookup for ms->PPQ conversion#3

Merged
wretcher207 merged 1 commit into
mainfrom
fix/timing-engine-per-note-tempo
Jun 17, 2026
Merged

Timing engine: per-note tempo lookup for ms->PPQ conversion#3
wretcher207 merged 1 commit into
mainfrom
fix/timing-engine-per-note-tempo

Conversation

@wretcher207

Copy link
Copy Markdown
Owner

Summary

C4 from internal review. apply_timing sampled Master_GetTempo() once at function entry, derived a single ppq_per_ms ratio, and applied it to every note. Any project with tempo automation produced wrong PPQ offsets for notes after the first tempo change — by exactly the ratio of (new BPM / sampled BPM).

Independent of #1 and #2; all three branch from main and can land in any order.

The fix

Round-trip each note's ms offset through project time so Reaper applies the correct local tempo:

local function ms_offset_to_ppq(take, ppq_pos, offset_ms)
  local t_sec      = reaper.MIDI_GetProjTimeFromPPQPos(take, ppq_pos)
  local target_ppq = reaper.MIDI_GetPPQPosFromProjTime(take, t_sec + offset_ms * 0.001)
  return target_ppq - ppq_pos
end

Cost

2 API calls per note instead of 1 multiplication. APPLY is user-triggered, not per-frame. For a 500-note item that's 1000 API roundtrips at APPLY-time — sub-millisecond total in practice. No effect on render-loop performance.

Preserved behavior

  • Note duration is still preserved in PPQ via n.new_endppq = new_start + (n.endppq - n.ppq). Inside a tempo change that crosses a note, this keeps musical-grid duration stable; only the note start gets the per-note ms-to-PPQ adjustment. This matches the conservative choice — a humanizer should not silently retime note ends as a side effect of nudging the starts.
  • get_tempo_info() is unchanged. It powers the timing-slider tooltip preview ("Lean center: X ms (Y PPQ)"), which is a rough display estimate — the user isn't pointing at a specific note when looking at the slider tooltip.

Test plan

  • Static tempo (e.g. 120 BPM throughout): APPLY TIMING with lean=+10ms. Verify offsets match pre-fix behavior numerically — should be the same since no tempo change is in play.
  • Tempo ramp (120 → 180 over the item): APPLY TIMING with lean=+10ms. Notes near the end should land roughly 10ms ahead in time, not 10ms ahead at the 120-BPM PPQ ratio. Listen on playback; the rush should feel uniform across the ramp instead of compressing toward the start.
  • Tempo jump mid-item (sudden 120 → 240): APPLY TIMING on notes spanning the boundary. Pre-fix would push post-jump notes too far in PPQ; post-fix should be musically consistent on both sides.
  • Tooltip display still reads sensible values — it's still BPM-sampled-at-entry, that's intentional.
  • Undo round-trip: APPLY, Ctrl+Z, verify notes restore exactly to original PPQ.
  • luac -p already verified on both file copies.

🤖 Generated with Claude Code

apply_timing used to sample Master_GetTempo() once at function entry, derive
a single ppq_per_ms ratio, then apply that same ratio to every note. Any
project with tempo automation produced wrong PPQ offsets for notes after
the first tempo change — by exactly the ratio of (new BPM / sampled BPM).

Fix: round-trip each note's ms offset through project time.

  local function ms_offset_to_ppq(take, ppq_pos, offset_ms)
    local t_sec      = reaper.MIDI_GetProjTimeFromPPQPos(take, ppq_pos)
    local target_ppq = reaper.MIDI_GetPPQPosFromProjTime(take, t_sec + offset_ms * 0.001)
    return target_ppq - ppq_pos
  end

Costs 2 API calls per note instead of 1 multiplication, but APPLY is user-
initiated, not per-frame — sub-millisecond total cost for typical drum
content (~500 notes per item).

Note duration is preserved in PPQ (n.new_endppq = new_start + (n.endppq -
n.ppq)), keeping musical-grid duration stable across a tempo change inside
the note. Only the note start gets the per-note ms->PPQ adjustment.

get_tempo_info() is unchanged; it powers the timing-slider tooltip preview,
which is a rough display estimate by design.

Mirrored to Scripts/dehumanizer-pro.lua. Syntax verified with luac.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jun 17, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@wretcher207, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 9 minutes and 45 seconds. Learn how PR review limits work.

Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file).

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 09a2c35d-1c9a-4391-a1cd-c8a8a0516f45

📥 Commits

Reviewing files that changed from the base of the PR and between 0f8aa62 and 3a6cc67.

📒 Files selected for processing (2)
  • Scripts/dehumanizer-pro.lua
  • dehumanizer-pro.lua
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/timing-engine-per-note-tempo

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 and usage tips.

wretcher207 pushed a commit that referenced this pull request Jun 17, 2026
- MEMORY.md: repo conventions (dual-path ReaPack mirroring) + 2026-06-16
  session summary covering the three open fix PRs (#1 velocity parity,
  #2 lifecycle persistence, #3 per-note tempo).
- HANDOFF.md: next-chat resume point with verification checklist for the
  three PRs and gotchas (script mirrored at /dehumanizer-pro.lua and
  /Scripts/dehumanizer-pro.lua per ReaPack packaging).

No code changes.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@wretcher207 wretcher207 merged commit d76e6eb into main Jun 17, 2026
2 checks 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