Fix notes ending too soon in GUS path - #280
Conversation
…age 3; fixed-point division bound on tail
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change revises MIDI envelope release and tail timing, adds exponential envelope amplitude mapping, preserves release allowance during looping, links static builds with the configured math library, corrects reverb validation, updates playback progress display, and refreshes documentation. ChangesMIDI release and rendering
Runtime configuration and display
Documentation updates
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/internal_midi.c`:
- Around line 1043-1046: Update _WM_do_control_channel_notes_off() to preserve
the env == 0 fallback before calling _WM_do_note_off_extra(note_data), matching
the guard used by _WM_do_note_off(). Ensure notes with a zero envelope wait for
the initial envelope step instead of entering release stage 3 directly.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: f759bde4-d104-4bea-bc45-4c4cd2d3277d
📒 Files selected for processing (2)
src/internal_midi.csrc/wildmidi_lib.c
|
I'm not proficient enough in midi/gus/etc to comment on code changes, but this midi, for e.g., now sounds weirder than it's used to: hex1.mid.zip (excessively sustained notes???) |
How does it sound like on timidiy (normal?). I would like also @afritz1 time chime in about his XMI to check if it's excessive or not as well with his xmi files. |
|
…ing notes ~12 dB too loud mid-decay.
|
@sezero try again with latest commit? I tried to follow timidity's code a bit here |
|
I'm not a MIDI connoisseur but I agree some notes are excessively sustained now. Before this PR the rhythm in OVERSNOW.XMI was like staccato. Now it's the opposite, very drawn out. The strings at 53s are also more sustained. Could we get a middle ground maybe? I exported these two .WAVs with Saphyr 2000: oversnow.zip |
Thanks! Please try with latest commit applied as well? |
|
Yeah your latest commit feels like how the song should be IMO, it's not excessive anymore. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/wildmidi_lib.c`:
- Around line 1807-1813: Update the build definition for libwildmidi-static to
publicly propagate the math library dependency introduced by pow(), so consumers
linking only this target automatically receive M_LIBRARY. Preserve the existing
static target linkage and apply the dependency through the target’s
exported/public link interface.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 12328c44-1c32-46ba-90ca-d54b62e812ff
📒 Files selected for processing (2)
src/internal_midi.csrc/wildmidi_lib.c
🚧 Files skipped from review as they are similar to previous changes (1)
- src/internal_midi.c
|
I noticed the song duration on the command prompt is not exact now. in ARABCITY.XMI it thinks 100% is when the last note starts but then it goes back to ~55% and then is fading out the last note until 100% again. |
Definitely better. |
|
How does this affect #230 ? |
I'm not sure, I'd need to read the thread but I'll leave that for tomorrow. I'm a bit dead right now. 💀 Is this PR good for merge? want 0.5.0 to be a great big bang release :) |
OK
Heh :)
Sounds fair to me, but @afritz1 reported duration issues above?
Will be that way, as i seems ;) |
…00; cmake: propagate libm on libwildmidi-static INTERFACE
There was a problem hiding this comment.
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)
1211-1214: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMatch the release estimate to the envelope direction.
_WM_Release_Allowanceforcesenv_incnegative here, but the mixer and_WM_do_note_off_extrachoose the sign by comparingenv_leveltoenv_target[note->env]. If stage 3 needs to rise before falling,longest_releasewill be too small and the tail can be cut off early. Mirror the same sign check here.🤖 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 1211 - 1214, Update the release estimate near _WM_Release_Allowance so note->env_inc uses the same direction as the mixer and _WM_do_note_off_extra: choose its sign by comparing note->env_level with note->sample->env_target[note->env], rather than always negating env_rate. Keep the release calculation based on note->env_level / -note->env_inc after selecting the correct direction.
🤖 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.
Outside diff comments:
In `@src/internal_midi.c`:
- Around line 1211-1214: Update the release estimate near _WM_Release_Allowance
so note->env_inc uses the same direction as the mixer and _WM_do_note_off_extra:
choose its sign by comparing note->env_level with
note->sample->env_target[note->env], rather than always negating env_rate. Keep
the release calculation based on note->env_level / -note->env_inc after
selecting the correct direction.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 33fddeac-efbe-459c-80f7-d62f70dbb282
📒 Files selected for processing (3)
src/CMakeLists.txtsrc/internal_midi.csrc/player/wildmidi.c
… reverb_listener_posy by room length, not width
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@docs/man/man1/wildmidi.1`:
- Line 12: Update the wildmidi SYNOPSIS option cluster to include the
no-argument x option alongside the existing flags, matching the Convert to MIDI
option documented in the OPTIONS section.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7175e8fd-4303-47d3-9cb3-647b5d7ce3d8
📒 Files selected for processing (4)
docs/man/man1/wildmidi.1docs/man/man3/WildMidi_Init.3docs/man/man5/wildmidi.cfg.5src/wildmidi_lib.c
It's better, now it stays at 100% from the beginning of the last note until it's faded out completely. I think this only matters for really short songs like these city entrance jingles. |
|
Indeed, that's the case. |
3 participants
Closes #229
Closes #230
Summary by CodeRabbit
Summary by CodeRabbit