Fallback built-in synthesizer - #274
Conversation
📝 WalkthroughWalkthroughThis PR adds an emergency live-synthesis fallback for WildMIDI, with a new synth module, runtime integration for sample loading and initialization, CLI triggering, build wiring, platform config updates, and a design document. ChangesEmergency Soundbank Synthesis
Estimated code review effort: 4 (Complex) | ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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
🧹 Nitpick comments (2)
docs/synth_plan.md (2)
90-91: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale filenames in the task list don't match the actual implementation.
The task list (lines 90, 109) references
src/emerg_synth.candinclude/emerg_synth.h, but the actual files added in this PR aresrc/synth.candinclude/synth.h. Update the doc to match.Also applies to: 109-109
🤖 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 `@docs/synth_plan.md` around lines 90 - 91, The task list contains stale implementation filenames and should be updated to match the actual added symbols/files. In the plan entries that mention the emerg_synth implementation, replace references to src/emerg_synth.c and include/emerg_synth.h with src/synth.c and include/synth.h so the documentation aligns with the current codebase.
24-25: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClarify whether the fallback is automatic or opt-in.
Line 24 says "or automatic fallback when no cfg is found," but line 116–117 says "opt-in via option, not silent, so debugging stays honest." These are contradictory — one implies silent auto-enablement, the other explicit opt-in. Reconcile the wording to match the intended behavior.
Also applies to: 116-117
🤖 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 `@docs/synth_plan.md` around lines 24 - 25, The plan currently contradicts itself about fallback behavior: one section says `--emergency` can trigger an automatic fallback when no cfg is found, while the later note for the fallback flow says it must be opt-in and not silent. Update the wording in the relevant plan sections so the behavior is described consistently, and make sure the references to the trigger and fallback policy match the intended implementation in the same terms throughout.
🤖 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/synth.c`:
- Around line 467-485: Rollback the partial initialization in
_WM_emergency_init_patches: if alloc_patch fails after some entries of
_WM_patch[] have already been set, the function should clean up every patch
allocated so far before returning -1. Update the loop logic around alloc_patch,
_WM_patch[id], and the drum-chain append so any failure triggers teardown of
previously installed patches and leaves the global array in a consistent
uninitialized state.
---
Nitpick comments:
In `@docs/synth_plan.md`:
- Around line 90-91: The task list contains stale implementation filenames and
should be updated to match the actual added symbols/files. In the plan entries
that mention the emerg_synth implementation, replace references to
src/emerg_synth.c and include/emerg_synth.h with src/synth.c and include/synth.h
so the documentation aligns with the current codebase.
- Around line 24-25: The plan currently contradicts itself about fallback
behavior: one section says `--emergency` can trigger an automatic fallback when
no cfg is found, while the later note for the fallback flow says it must be
opt-in and not silent. Update the wording in the relevant plan sections so the
behavior is described consistently, and make sure the references to the trigger
and fallback policy match the intended implementation in the same terms
throughout.
🪄 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: fa771ffd-1a3c-4d91-8a4d-c16d35f5ba9c
📒 Files selected for processing (7)
docs/synth_plan.mdinclude/synth.hsrc/CMakeLists.txtsrc/player/wildmidi.csrc/sample.csrc/synth.csrc/wildmidi_lib.c
|
Pushed build fixes to this branch |
Cool, I had a fix for those ready but you beat me to it... you're very eager :P Thank you though. You get a chance to play with it? I've had it baking for awhile... It uses less CPU/Memory than with sf2 and patches which was surprising. Just sounds like a gameboy though 😆 |
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/synth_plan.md`:
- Around line 19-22: The fenced example in the markdown doc is missing a
language identifier, which triggers the MD040 lint rule. Update the fenced block
containing the _WM_load_gus_pat and _WM_synth_patch example so it uses the c
language tag on the opening fence, keeping the existing example content
unchanged.
🪄 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: 40a8dcbd-3fbe-4d40-b67b-3cceb8907bc6
📒 Files selected for processing (25)
CMakeLists.txtamiga/Makefileamiga/Makefile.vbccamiga/config.handroid/jni/Android.mkandroid/jni/config.hdjgpp/Makefiledjgpp/config.hdocs/synth_plan.mdinclude/config.h.cmakeinclude/synth.hmacosx/Makefilemacosx/config.hmingw/Makefilemingw/config.hmsvc/common.makmsvc/config.hos2/config.hos2/makefileos2/makefile.emxsrc/CMakeLists.txtsrc/player/wildmidi.csrc/sample.csrc/synth.csrc/wildmidi_lib.c
✅ Files skipped from review due to trivial changes (10)
- include/config.h.cmake
- msvc/common.mak
- djgpp/config.h
- CMakeLists.txt
- android/jni/config.h
- mingw/config.h
- macosx/config.h
- amiga/config.h
- src/CMakeLists.txt
- mingw/Makefile
🚧 Files skipped from review as they are similar to previous changes (12)
- os2/makefile.emx
- os2/makefile
- amiga/Makefile
- android/jni/Android.mk
- include/synth.h
- djgpp/Makefile
- amiga/Makefile.vbcc
- src/wildmidi_lib.c
- src/sample.c
- os2/config.h
- msvc/config.h
- src/synth.c
| ``` | ||
| if (guspat = _WM_load_gus_pat(...)) { /* existing */ } | ||
| else if (emergency_bank_enabled) { guspat = _WM_synth_patch(patchid); } | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language tag to this fenced example.
The docs lint already flags this block (MD040). Mark it as c so the markdown check passes.
Fix
-```
+```c
if (guspat = _WM_load_gus_pat(...)) { /* existing */ }
else if (emergency_bank_enabled) { guspat = _WM_synth_patch(patchid); }
-```
+```📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ``` | |
| if (guspat = _WM_load_gus_pat(...)) { /* existing */ } | |
| else if (emergency_bank_enabled) { guspat = _WM_synth_patch(patchid); } | |
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 19-19: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@docs/synth_plan.md` around lines 19 - 22, The fenced example in the markdown
doc is missing a language identifier, which triggers the MD040 lint rule. Update
the fenced block containing the _WM_load_gus_pat and _WM_synth_patch example so
it uses the c language tag on the opening fence, keeping the existing example
content unchanged.
Source: Linters/SAST tools
|
Rebased to current master.
Abominable. So silent, I practically can't hear anything, even if I max-out my speakers' volume. |
Really? I can hear it just fine here? I mean, sure... it sounds like a gameboy, but it's not silent or do you mean something else? If you feel like this is hot garbage, I can close and try something else. |
|
Well, all I hear are "bop bop bop" sounds -- this is with the current rebased branch: out.mp3 (I did |
|
I'm going to try something else... going to put it in another branch/PR |
It sounds ... fair. At least I have proper sound with it. |
2 participants
Fallback built-in synthesizer
There are 128 tonal (additive, 8 partials, 16 GM family profiles) + full GM percussion map with kick/snare/hat/cymbal/bell/click voice types;
Reversed engineered from OPL-3 sf2 to match the tone/pitch/timbre of the in C; can still be fine-tuned and expanded.
Extensions/future:
Closes #121
Summary by CodeRabbit
Summary by CodeRabbit
New Features
.pat/.sf2resources are available.--emergency/-Eto activate the fallback, including synthesized tonal and GM drum/percussion sounds.Bug Fixes
Tests
Documentation