Add CC1 modulation wheel (vibrato) support - #289
Conversation
CC1 was parsed then discarded, and the GUS engine had no LFO at all. Adds a 5Hz triangle vibrato LFO (TiMidity++ style) for GUS patches, wires CC1 to vibLfoToPitch for SF2 (50 cents at full wheel, per SF2.01 8.4.4 as FluidSynth does), and adds an LFO to the MA-FM synth. RPN 5 rescales depth; CC121 and the resets clear it. Also fixes -o MIDI export, which would otherwise drop the new event type. Files without CC1 render bit-for-bit identical to before. Fixes #143
|
Warning Review limit reached
Next review available in: 20 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds CC1 modulation-wheel support and RPN 5 vibrato-depth handling. Vibrato state and block-based LFO updates are integrated into MIDI playback, GUS mixing, SF2 voices, and the MA FM synthesizer. ChangesModulation wheel vibrato
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant MIDI as MIDI input
participant WildMIDI as WildMIDI channel state
participant Synth as GUS, SF2, or MA FM voice
participant Audio as Audio renderer
MIDI->>WildMIDI: Send CC1 modulation or RPN 5 depth
WildMIDI->>Synth: Apply vibrato depth to active voices
Audio->>Synth: Advance LFO during rendering
Synth-->>Audio: Render pitch-modulated audio
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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. Comment |
|
@coderabbitai summary |
✅ Action performedSummary regeneration triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/internal_midi.c (1)
1283-1317: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winGM reset doesn't clear vibrato on already-sounding notes.
_WM_do_sysex_gm_resetresetschannel[i].modulation/mod_depth_rangeto defaults but never callsset_channel_vibrato(mdi, i), unlike_WM_do_control_channel_controllers_off(Line 1161) which does. Any note already vibrato-ing when an in-song GM reset sysex arrives keeps its stalevib_depth/vib_phase/vib_inc(those live onstruct _note, untouched by this loop), continuing to vibrato after a reset that's supposed to clear it.🐛 Proposed fix
mdi->channel[i].modulation = 0; mdi->channel[i].mod_depth_range = VIB_DEPTH_DEFAULT; + set_channel_vibrato(mdi, i); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/internal_midi.c` around lines 1283 - 1317, Update _WM_do_sysex_gm_reset to call set_channel_vibrato(mdi, i) for each channel after resetting modulation and mod_depth_range, matching _WM_do_control_channel_controllers_off. Ensure this updates already-sounding notes so their vibrato state is cleared while preserving the existing GM reset initialization.
🧹 Nitpick comments (1)
src/wildmidi_lib.c (1)
1030-1043: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate vibrato-tick logic across the two mixers. The block counter +
VIB_BLOCKgate + active-note walk calling_WM_update_note_vibratois copy-pasted verbatim between the Linear and Gauss output paths; the shared root cause is the lack of a single helper for this logic.
src/wildmidi_lib.c#L1030-L1043: extract this block into a shared helper (e.g._WM_tick_vibrato(struct _mdi *mdi)in internal_midi.c) and call it here.src/wildmidi_lib.c#L1383-L1396: replace this identical block with a call to the same shared helper.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/wildmidi_lib.c` around lines 1030 - 1043, Extract the duplicated vibrato tick logic into a shared _WM_tick_vibrato(struct _mdi *mdi) helper in internal_midi.c, preserving the VIB_BLOCK counter gate, reset, active-note traversal, and _WM_update_note_vibrato calls. Replace the inline blocks at src/wildmidi_lib.c lines 1030-1043 and 1383-1396 with calls to this helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/mafm.c`:
- Around line 591-605: Update _WM_MAFM_Event and _WM_MAFM_Reset so
controller-off and applicable reset events clear s->chan_modulation for every
channel, matching the existing CC modulation-zero behavior. Ensure reset
processing also clears the channel modulation state rather than leaving vibrato
values active, or explicitly preserve the state only if that is the intended
contract.
In `@src/tsf/tsf.h`:
- Around line 2114-2118: Cap the RPN 5 calculation in the midiRPN == 5 branch
before assigning c->modDepthRange, limiting it to the existing VIB_DEPTH_MAX
ceiling (600 cents). Preserve the current modulation-depth conversion and
subsequent tsf_channel_apply_modulation call.
- Around line 1286-1288: Update vibrato LFO setup in tsf_voice_lfo_setup() to
use the SF2 default frequency of 240 when the region does not provide
freqVibLFO, ensuring v->viblfo.delta is nonzero before updateVibLFO checks CC1
modulation. Preserve explicitly configured region frequencies and the existing
modulation behavior.
---
Outside diff comments:
In `@src/internal_midi.c`:
- Around line 1283-1317: Update _WM_do_sysex_gm_reset to call
set_channel_vibrato(mdi, i) for each channel after resetting modulation and
mod_depth_range, matching _WM_do_control_channel_controllers_off. Ensure this
updates already-sounding notes so their vibrato state is cleared while
preserving the existing GM reset initialization.
---
Nitpick comments:
In `@src/wildmidi_lib.c`:
- Around line 1030-1043: Extract the duplicated vibrato tick logic into a shared
_WM_tick_vibrato(struct _mdi *mdi) helper in internal_midi.c, preserving the
VIB_BLOCK counter gate, reset, active-note traversal, and
_WM_update_note_vibrato calls. Replace the inline blocks at src/wildmidi_lib.c
lines 1030-1043 and 1383-1396 with calls to this helper.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9d8f9e9d-9c95-42e7-8ca6-74cec9cc37d7
📒 Files selected for processing (8)
README.mdinclude/internal_midi.hsrc/f_midi.csrc/internal_midi.csrc/mafm.csrc/sf2.csrc/tsf/tsf.hsrc/wildmidi_lib.c
sezero
left a comment
sezero
left a comment
There was a problem hiding this comment.
OK by me.
I'll add tsf.h changes to que I submitted to mainstream (with you as the author)
Thank you 🙏🏼 I was wondering about that. |
2 participants
CC1 was parsed then discarded, and the GUS engine had no LFO at all. Adds a 5Hz triangle vibrato LFO (TiMidity++ style) for GUS patches, wires CC1 to vibLfoToPitch for SF2 (50 cents at full wheel, per SF2.01 8.4.4 as FluidSynth does), and adds an LFO to the MA-FM synth. RPN 5 rescales depth; CC121 and the resets clear it. Also fixes -o MIDI export, which would otherwise drop the new event type.
Files without CC1 render bit-for-bit identical to before.
Closes #143
Test files
modulation_testing.zip