docs: add agent guidance and historical audit report
Add root AGENT.md with project conventions, build verification steps, source layout, style rules, and real-time/concurrency requirements. Add per-module AGENT.md files for each existing and proposed source subdirectory. Add AUDIT_REPORT.md as a historical Phase 1 snapshot documenting memory management, error handling, concurrency model, naming conventions, and anti-pattern catalog.
This commit is contained in:
+500
@@ -0,0 +1,500 @@
|
||||
# SerumAlt codebase audit (Phase 1)
|
||||
|
||||
Historical snapshot, taken before the platform/harness build separation. Build
|
||||
commands, defaults, target locations and line numbers below describe that earlier
|
||||
state. Use README.md's Building section and the current CMake files for builds.
|
||||
The proposed source moves are suggestions, not an approved implementation plan.
|
||||
|
||||
Scope: the SerumAlt C++/JUCE VST3 + Standalone synthesiser. This report records the
|
||||
implicit standards, anti-patterns, memory choices, error handling, concurrency model and
|
||||
module boundaries found by reading the real repository files. No source, build or
|
||||
documentation files were modified. All findings are cited with `path:line` references read
|
||||
during this task.
|
||||
|
||||
Repository note: at the start of this audit the working tree already had uncommitted
|
||||
changes to `CMakeLists.txt`, several `Source/` files and several `third_party/JUCE/` files,
|
||||
plus untracked `build/`, `build_windows/` and `SerumAlt_Windows/` directories. Those
|
||||
changes predate this audit and are not analysed here except where the current on-disk
|
||||
content is the source of truth.
|
||||
|
||||
---
|
||||
|
||||
## 1. Build & test setup
|
||||
|
||||
### Project metadata and targets
|
||||
|
||||
- Project name and version: `project(SerumAlt VERSION 1.0.0 LANGUAGES CXX)` at `CMakeLists.txt:3`.
|
||||
- C++ standard: C++17, required, extensions off at `CMakeLists.txt:8-10`
|
||||
(`CMAKE_CXX_STANDARD 17`, `CMAKE_CXX_STANDARD_REQUIRED ON`, `CMAKE_CXX_EXTENSIONS OFF`).
|
||||
- The plugin target is `SerumAlt`, declared with `juce_add_plugin(...)` at
|
||||
`CMakeLists.txt:33-46`. Formats are `VST3 AU Standalone` at `CMakeLists.txt:44`. JUCE
|
||||
generates the concrete per-format targets (`SerumAlt_VST3`, `SerumAlt_Standalone`, and
|
||||
`SerumAlt_AU` on macOS). There is no separate `PluginProcessor`/`PluginEditor` target;
|
||||
those are ordinary classes compiled into `SerumAlt`.
|
||||
- A console test target `SerumAltTest` is added at `CMakeLists.txt:126-140` via
|
||||
`juce_add_console_app`. It compiles the entire `${SERUMALT_SOURCES}` list plus
|
||||
`Source/Tests/TestMain.cpp` (`CMakeLists.txt:128`).
|
||||
|
||||
### Source list
|
||||
|
||||
Sources are an explicit list, not a glob. `set(SERUMALT_SOURCES ...)` spans
|
||||
`CMakeLists.txt:50-87` and is consumed by `target_sources(SerumAlt PRIVATE
|
||||
${SERUMALT_SOURCES})` at `CMakeLists.txt:89`. The list enumerates every `.cpp` used by the
|
||||
plugin, including the subdirectory files `Source/EffectUnits/*.cpp`, `Source/GUI/*.cpp`
|
||||
and `Source/Presets/FactoryPresets.cpp`.
|
||||
|
||||
Two header-only files are intentionally absent from the list because they have no
|
||||
translation unit: `Source/EffectUnits/Biquad.h` and `Source/GUI/SerumLookAndFeel.h`.
|
||||
|
||||
Consequence for later phases: any file move or rename must update this list, because there
|
||||
is no glob to pick files up automatically.
|
||||
|
||||
### Custom flags and linkage
|
||||
|
||||
- Compile definitions at `CMakeLists.txt:91-95`: `JUCE_WEB_BROWSER=0`, `JUCE_USE_CURL=0`,
|
||||
`JUCE_VST3_CAN_REPLACE_VST2=0`.
|
||||
- Link libraries at `CMakeLists.txt:97-103`: `juce::juce_audio_utils` and `juce::juce_dsp`
|
||||
(private), `juce::juce_recommended_config_flags` and `juce::juce_recommended_warning_flags`
|
||||
(public).
|
||||
- Warning flags at `CMakeLists.txt:142-146`: `/W4` under MSVC, `-Wall -Wextra` otherwise.
|
||||
- MinGW cross-build static runtime at `CMakeLists.txt:113-118`: when
|
||||
`CMAKE_CROSSCOMPILING`, `SerumAlt_VST3` gets `-static-libgcc -static-libstdc++ -static`
|
||||
so the DLL is self-contained.
|
||||
- VST3 manifest handling at `CMakeLists.txt:23-31`: `SERUMALT_VST3_AUTO_MANIFEST` is `FALSE`
|
||||
when cross-compiling (because JUCE 7.0.12 builds the manifest helper with the same
|
||||
toolchain, producing a Windows `.exe` that cannot run on the Linux host). The manifest is
|
||||
instead injected by `build_windows.sh`.
|
||||
|
||||
### Windows cross-build (build_windows.sh)
|
||||
|
||||
`build_windows.sh` is the canonical Windows build path. Exact steps, in order:
|
||||
|
||||
1. Verify `x86_64-w64-mingw32-g++` exists, else exit with an error (`build_windows.sh:16-21`).
|
||||
2. Regenerate `mingw64-toolchain.cmake` from a heredoc inside the script
|
||||
(`build_windows.sh:26-42`). This is identical in content to the checked-in
|
||||
`mingw64-toolchain.cmake` (see below), so the toolchain file is defined in two places.
|
||||
3. Configure: `cmake -S . -B build_windows -G Ninja -DCMAKE_BUILD_TYPE=Release
|
||||
-DCMAKE_TOOLCHAIN_FILE=mingw64-toolchain.cmake -DSERUMALT_BUILD_TESTS=OFF`
|
||||
(`build_windows.sh:47-50`). Note the test harness is explicitly disabled here.
|
||||
4. Build: `cmake --build build_windows --target SerumAlt_VST3` (`build_windows.sh:53`).
|
||||
5. Inject `Contents/Resources/moduleinfo.json` into the `.vst3` bundle by writing the file
|
||||
directly, because the automatic manifest step was disabled (`build_windows.sh:63-126`).
|
||||
6. Copy the bundle to `SerumAlt_Windows/SerumAlt.vst3` (`build_windows.sh:129-131`).
|
||||
|
||||
The toolchain (`mingw64-toolchain.cmake:1-15`) sets `CMAKE_SYSTEM_NAME Windows`,
|
||||
`CMAKE_SYSTEM_PROCESSOR x86_64`, the `x86_64-w64-mingw32-*` compilers/tools, and
|
||||
`CMAKE_FIND_ROOT_PATH /usr/x86_64-w64-mingw32` with `PROGRAM NEVER` and the other
|
||||
`..._MODE_*` values `ONLY`.
|
||||
|
||||
### Native Linux build (README only)
|
||||
|
||||
`README.md:76-78` documents a native build:
|
||||
|
||||
```
|
||||
cmake -S . -B build -G Ninja -DCMAKE_BUILD_TYPE=Release
|
||||
cmake --build build
|
||||
```
|
||||
|
||||
Outputs listed at `README.md:80-84` are
|
||||
`build/SerumAlt_artefacts/Release/VST3/SerumAlt.vst3` and the standalone app.
|
||||
|
||||
### Tests
|
||||
|
||||
Tests exist. `Source/Tests/TestMain.cpp` is a console harness that instantiates the
|
||||
processor, plays notes through every factory preset, checks for NaN/inf and silence, and
|
||||
toggles RAVE (`TestMain.cpp:37-135`). It returns a non-zero exit code on failure
|
||||
(`TestMain.cpp:134`).
|
||||
|
||||
Tests are run through the native build, documented at `README.md:96-98`:
|
||||
|
||||
```
|
||||
cmake --build build --target SerumAltTest
|
||||
./build/SerumAltTest_artefacts/Release/SerumAltTest
|
||||
```
|
||||
|
||||
The Windows cross-build does not build or run tests: `build_windows.sh:50` passes
|
||||
`-DSERUMALT_BUILD_TESTS=OFF`.
|
||||
|
||||
### Clean build and test command
|
||||
|
||||
There is no clean step in `build_windows.sh`. The script configures into `build_windows/`
|
||||
and builds without removing or cleaning it first. A clean rebuild is not scripted; it would
|
||||
require manually removing `build_windows/` (or invoking Ninja's clean target, which the
|
||||
script does not do). A combined "clean build + run tests" command does not exist: the
|
||||
Windows script disables tests, and the README test instructions assume an already-built
|
||||
native `build/` tree.
|
||||
|
||||
---
|
||||
|
||||
## 2. Proposed module boundaries
|
||||
|
||||
The codebase already has three real subdirectory boundaries (`EffectUnits/`, `GUI/`,
|
||||
`Presets/`) plus `Resources/` (assets only) and `Tests/`. The flat files at `Source/` root
|
||||
mix four distinct concerns, and three root files clearly belong inside existing
|
||||
subdirectories. The proposal below is based on the include graph actually present, not on
|
||||
generic synth architecture.
|
||||
|
||||
### Dependency facts that drive the split
|
||||
|
||||
- `Params.h`/`Params.cpp` depend only on `<JuceHeader.h>` (`Params.h:3`). It declares the
|
||||
`ids`, `maps` namespaces, all enums, `ModSource`/`ModTarget`, and the wavetable name
|
||||
table. It is the shared foundation: nearly every other file includes it.
|
||||
- `Wavetable`, `Oscillator`, `SubOscillator`, `NoiseOscillator`, `Envelope`, `LFO`,
|
||||
`Filter`, `FilterBank`, `SynthVoice` depend only on `Params`, `JuceHeader`, and each
|
||||
other (`Oscillator.h:4-5`, `FilterBank.h:4-5`, `SynthVoice.h:4-12`).
|
||||
- `ModulationMatrix` and `MacroControls` depend only on `Params` (`ModulationMatrix.h:4`,
|
||||
`MacroControls.h:4`).
|
||||
- `RAVEButton.h`/`RaveController` depends only on `Params` (`RAVEButton.h:4`) but manipulates
|
||||
the APVTS, so it sits between modulation and plugin plumbing.
|
||||
- `FXProcessor.h` depends only on `Params` (`FXProcessor.h:4`); every file in
|
||||
`EffectUnits/` includes `../FXProcessor.h` (`Hyper.h:3`, `Chorus.h:3`, etc.), so
|
||||
`FXProcessor` is the base the effect units build on.
|
||||
- `Engine` aggregates the DSP: `Engine.h:4-10` includes `Wavetable`, `SynthVoice`, `LFO`,
|
||||
`FXProcessor`, `ModulationMatrix`, `MacroControls`.
|
||||
- `PluginProcessor.h` includes `Params`, `Engine`, `RAVEButton` (`PluginProcessor.h:4-6`);
|
||||
it is the JUCE `AudioProcessor` boundary plus state/preset/RAVE glue.
|
||||
- `PluginEditor.h` includes `PluginProcessor` and every `GUI/` header
|
||||
(`PluginEditor.h:4-14`); it is pure GUI.
|
||||
- `Resources.h` depends on `JuceHeader` plus `Params` for the format helpers
|
||||
(`Resources.cpp:2`); it holds the theme and SVG strings. The SVG asset files live in
|
||||
`Source/Resources/`, but the `Resources.{h,cpp}` code lives at root, splitting one
|
||||
concept across two locations.
|
||||
- `Presets/FactoryPresets` depends on `Params` and `ModulationMatrix`
|
||||
(`FactoryPresets.h:4-5`).
|
||||
- `Tests/TestMain.cpp` depends only on `PluginProcessor` (`TestMain.cpp:4`).
|
||||
|
||||
### Proposed modules
|
||||
|
||||
| Module | Files (current location) | Depends on |
|
||||
| --- | --- | --- |
|
||||
| params (foundation) | `Params.{h,cpp}` | JuceHeader only |
|
||||
| synth (DSP building blocks + voice) | `Wavetable.{h,cpp}`, `Oscillator.{h,cpp}`, `SubOscillator.{h,cpp}`, `NoiseOscillator.{h,cpp}`, `Envelope.{h,cpp}`, `LFO.{h,cpp}`, `Filter.{h,cpp}`, `FilterBank.{h,cpp}`, `SynthVoice.{h,cpp}` | params |
|
||||
| modulation | `ModulationMatrix.{h,cpp}`, `MacroControls.{h,cpp}`, `RAVEButton.{h,cpp}` | params |
|
||||
| effects | `FXProcessor.{h,cpp}` plus all of `EffectUnits/` (including `Biquad.h`) | params |
|
||||
| engine | `Engine.{h,cpp}` | synth, effects, modulation |
|
||||
| gui | `PluginEditor.{h,cpp}` plus all of `GUI/` (including `SerumLookAndFeel.h`) | params, synth (for wavetable display), plugin |
|
||||
| resources | `Resources.{h,cpp}` plus `Resources/*.svg` | params |
|
||||
| presets | `Presets/FactoryPresets.{h,cpp}` | params, modulation |
|
||||
| plugin (JUCE plumbing) | `PluginProcessor.{h,cpp}` | engine, modulation, presets, params |
|
||||
| tests | `Tests/TestMain.cpp` | plugin |
|
||||
|
||||
### Where the current layout violates the boundaries
|
||||
|
||||
- `FXProcessor.{h,cpp}` sits at `Source/` root but is the base class and owner of the
|
||||
`EffectUnits/` classes; `EffectUnits/*.h` include it as `../FXProcessor.h`. It belongs in
|
||||
`EffectUnits/` (or in a renamed `FX/` directory with the units).
|
||||
- `PluginEditor.{h,cpp}` sits at root but is pure GUI and includes every `GUI/` header. It
|
||||
belongs in `GUI/`.
|
||||
- `Resources.{h,cpp}` holds the theme and SVG strings, while the `.svg` assets live in
|
||||
`Source/Resources/`. The code and assets should live together under one `resources`
|
||||
module.
|
||||
- `RAVEButton.{h,cpp}` is orphaned at root under a filename that does not match its class
|
||||
(`RaveController`). It is a macro-like boost over the APVTS, so it fits the `modulation`
|
||||
module, though its APVTS manipulation also makes `plugin` a defensible home.
|
||||
- The remaining root files (`Params`, `Wavetable`, `Oscillator`, `SubOscillator`,
|
||||
`NoiseOscillator`, `Envelope`, `LFO`, `Filter`, `FilterBank`, `SynthVoice`,
|
||||
`ModulationMatrix`, `MacroControls`, `Engine`, `PluginProcessor`) are the synthesis core
|
||||
and JUCE boundary. Grouping them into `synth/` and `modulation/` subdirectories would
|
||||
match the dependencies above, but the flat layout is at least internally consistent:
|
||||
they are the only files that legitimately belong at root today.
|
||||
|
||||
---
|
||||
|
||||
## 3. Memory management standards
|
||||
|
||||
### Dominant idiom: value semantics plus `std::unique_ptr` for ownership
|
||||
|
||||
- Owned sub-objects are held by value, not by pointer: `juce::AudioProcessorValueTreeState
|
||||
parameters; Engine engine; RaveController rave;` at `PluginProcessor.h:56-58`; the
|
||||
`Engine` holds `std::array<SynthVoice, kNumVoices> voices`, `std::array<LFO, kNumLfos>
|
||||
lfos`, and `WavetableLibrary`, `FXProcessor`, `ModulationMatrix`, `MacroControls` by value
|
||||
at `Engine.h:45-50`.
|
||||
- Owned polymorphic objects use `std::unique_ptr`: `std::array<std::unique_ptr<FXUnit>, 9>
|
||||
units` at `FXProcessor.h:50`, populated with `std::make_unique` at `FXProcessor.cpp:15-26`.
|
||||
- GUI attachments are owned by `std::unique_ptr`: `std::vector<std::unique_ptr<...
|
||||
SliderAttachment>>` and `...ComboBoxAttachment` at `PluginEditor.h:103-104`, filled at
|
||||
`PluginEditor.cpp:169-170` and `183-184`.
|
||||
- Transient GUI objects are `std::unique_ptr`: `std::unique_ptr<juce::Label> valuePopup`
|
||||
at `Knob.h:38`, created at `Knob.cpp:61` and released with `valuePopup.reset()` at
|
||||
`Knob.cpp:83`.
|
||||
- JUCE-managed objects are returned as raw `new` pointers because JUCE takes ownership
|
||||
through `addAndMakeVisible` or the plugin API: `createEditor()` returns
|
||||
`new PluginEditor (*this)` at `PluginProcessor.cpp:195`; the plugin entry point returns
|
||||
`new serum::SerumAltAudioProcessor()` at `PluginProcessor.cpp:380`; GUI children are
|
||||
created with `new` at `PluginEditor.cpp:166,178,193,602,606`. These raw pointers are never
|
||||
deleted by the code, which is correct only because JUCE's component tree and plugin host
|
||||
own them.
|
||||
- Non-owning cross-object references are raw pointers or references: `juce::RangedAudioParameter*
|
||||
param` at `ToggleButton.h:38`, `const WavetableLibrary* wtLib` at `WaveformDisplay.h:24`,
|
||||
and the `RenderContext` raw `const` pointers to the library/matrix/macros at
|
||||
`SynthVoice.h:24,32-33`.
|
||||
|
||||
### Containers and buffers
|
||||
|
||||
- `std::vector<float>` is the standard delay line / table buffer: `Wavetable.h:24`
|
||||
(`frames`), `Filter.h:39` (`combLine`), `Chorus.h:21`, `Delay.h:21`, `Reverb.h:22-29`.
|
||||
- `std::array` is used for fixed-size DSP state: `Oscillator.h:62`
|
||||
(`std::array<SubVoice, kMaxUnison>`), `Filter.h:36,44`, `Reverb.h:22-27`.
|
||||
- `juce::AudioBuffer<float>` is used for block buffers: `Engine.h:59` (`mixBuffer`),
|
||||
`SynthVoice.h:95` (`scratch`), `FXProcessor.h:51` (`dry`, `wet`). These are sized once in
|
||||
`prepare()` (for example `Engine.cpp:68`, `SynthVoice.cpp:23`, `FXProcessor.cpp:34-35`)
|
||||
and cleared per block.
|
||||
|
||||
### Realtime vs non-realtime allocation
|
||||
|
||||
The code intends to allocate only at prepare time, but it does not fully achieve this:
|
||||
|
||||
- Prepare-time allocation is the norm: delay lines and buffers are sized in `prepare()`
|
||||
(`Filter.cpp:24`, `Chorus.cpp:9-11`, `Reverb.cpp:17-33`, `Engine.cpp:68`).
|
||||
- Violation 1: `Engine::processBlock` calls `mixBuffer.setSize (2, n, false, false, true)`
|
||||
on every block at `Engine.cpp:211`. `AudioBuffer::setSize` is a no-op when the size is
|
||||
unchanged, but it is still a per-callback reconfiguration call on the audio thread.
|
||||
- Violation 2: wavetables are built lazily. `WavetableLibrary::getTable` builds the table
|
||||
on first access (`Wavetable.cpp:126-132`) and `getTable` is reached from the audio path at
|
||||
`SynthVoice.cpp:177-178`. The `prebuild()` method exists at `Wavetable.cpp:120-124` but is
|
||||
never called (it is not referenced anywhere else, and `Engine::prepare` at
|
||||
`Engine.cpp:58-70` does not call it). The first rendered block for a wavetable therefore
|
||||
allocates `256 x 2048` floats plus harmonic work buffers on the audio thread.
|
||||
|
||||
---
|
||||
|
||||
## 4. Error handling
|
||||
|
||||
There is no exception handling and no user-facing alerting. Failures are handled with a
|
||||
mix of asserts, null guards, and return codes.
|
||||
|
||||
- `jassert` is used exactly once in the entire codebase: `jassert (freqHz > 0.0)` at
|
||||
`Oscillator.cpp:20`. There are no `throw`, `try` or `catch` statements (verified by grep).
|
||||
- Null guards with silent fallback are the dominant pattern. The APVTS helper `v()` returns
|
||||
`0.0f` when a parameter is missing: `Engine.cpp:8-13`. State restore silently returns on
|
||||
malformed input: `PluginProcessor.cpp:313-315` returns when the XML is null and
|
||||
`PluginProcessor.cpp:318-319` returns when the `ValueTree` is invalid. Preset loads skip
|
||||
missing parameters with `if (auto* param = ...)` at `PluginProcessor.cpp:248-250`.
|
||||
- Return codes carry failure for the two bounded collections:
|
||||
`ModulationMatrix::addConnection` returns `false` when the target is `None` or the
|
||||
connection limit is reached (`ModulationMatrix.cpp:6-12`), and
|
||||
`MacroControls::addAssignment` returns `false` similarly (`MacroControls.cpp:6-13`).
|
||||
These return values are ignored by their callers (`PluginEditor.cpp:536`, `PluginEditor.cpp:687`,
|
||||
`PluginProcessor.cpp:254`, `PluginProcessor.cpp:258`).
|
||||
- Out-of-range lookups degrade to empty/default values: `getProgramName` returns an empty
|
||||
string for a bad index (`PluginProcessor.cpp:218-224`); the enum-string converters fall
|
||||
through to `return {}` (`Params.cpp:27,44,91`).
|
||||
- There are no `juce::AlertWindow`, no `MessageManager` calls, and no logging in the plugin
|
||||
itself. The only reporting is the test harness printing to `std::cout` and returning an
|
||||
exit code (`TestMain.cpp:73,103,129,132-134`).
|
||||
|
||||
Net style: programmer errors use `jassert` (very sparingly), runtime/state errors use null
|
||||
guards or default values, and bounded-insertion failures use `bool` return codes that
|
||||
callers currently ignore.
|
||||
|
||||
---
|
||||
|
||||
## 5. Concurrency
|
||||
|
||||
The codebase is single-threaded and has no explicit synchronisation. A grep for threads,
|
||||
mutexes, locks, atomics, `CriticalSection`, `SpinLock` and `ScopedLock` returns nothing
|
||||
outside JUCE's own headers.
|
||||
|
||||
- The only non-audio execution context is the JUCE message thread, driven by a `juce::Timer`:
|
||||
`PluginEditor` inherits `juce::Timer` (`PluginEditor.h:24`), starts it at 30 Hz
|
||||
(`PluginEditor.cpp:135`), and refreshes visuals in `timerCallback` (`PluginEditor.cpp:875-880`).
|
||||
- The audio thread is `SerumAltAudioProcessor::processBlock` (`PluginProcessor.cpp:187-191`)
|
||||
delegating to `Engine::processBlock` (`Engine.cpp:176-318`). It reads the playhead tempo at
|
||||
`Engine.cpp:184-187`.
|
||||
|
||||
The significant issue is unsynchronised sharing between the message thread and the audio
|
||||
thread:
|
||||
|
||||
- The GUI mutates `ModulationMatrix::connections` via `addConnection`/`removeConnection`/
|
||||
`clear` (`PluginEditor.cpp:536,549,559`) while the audio thread iterates the same vector in
|
||||
`SynthVoice::render` at `SynthVoice.cpp:114`. `connections` is a public `std::vector`
|
||||
(`ModulationMatrix.h:30`).
|
||||
- The GUI mutates `MacroControls::assignments` (`PluginEditor.cpp:687,699`) while the audio
|
||||
thread iterates it at `SynthVoice.cpp:125`. `assignments` is a public `std::array` of
|
||||
vectors (`MacroControls.h:28`).
|
||||
- The GUI writes LFO shape data through `engine.setLfoShapeData` (`PluginEditor.cpp:483-486`)
|
||||
which calls `LFO::setShapeData` (`Engine.cpp:84-88`, `LFO.cpp:51-59`), while the audio
|
||||
thread reads the same `shapeBuffer` in `LFO::shapeValue` (`LFO.cpp:82-91`).
|
||||
|
||||
None of these shared collections are locked or double-buffered. This is a real data race
|
||||
today: resizing `connections` while the audio thread iterates it is undefined behaviour.
|
||||
|
||||
The processor header acknowledges GUI-thread access but does not address the race: the
|
||||
comment "Public DSP state (read/write from the GUI thread)" at `PluginProcessor.h:55` only
|
||||
describes intent.
|
||||
|
||||
---
|
||||
|
||||
## 6. Naming & style conventions
|
||||
|
||||
- Namespace: every file is inside `namespace serum` (`PluginProcessor.h:8`, `Engine.h:12`,
|
||||
etc.). File-local helpers use an anonymous namespace (`Engine.cpp:6`, `SynthVoice.cpp:6`,
|
||||
`PluginEditor.cpp:6`, `FactoryPresets.cpp:6`).
|
||||
- Include guard: `#pragma once` in every header (for example `PluginProcessor.h:1`,
|
||||
`Params.h:1`, `Biquad.h:1`). No `#ifndef` guards exist.
|
||||
- Include order: `<JuceHeader.h>` first, then local headers (`PluginProcessor.h:3-6`).
|
||||
Subdirectory files include root files with a `../` relative path (`Hyper.h:3`,
|
||||
`Knob.h:5`, `FactoryPresets.h:4`).
|
||||
- File/header pairs: one primary class per `.h`/`.cpp` pair, with the pair named after the
|
||||
class (`Oscillator.h`/`Oscillator.cpp`, `FilterBank.h`/`FilterBank.cpp`). Header-only
|
||||
helpers exist where they have no state logic worth a TU (`Biquad.h`,
|
||||
`SerumLookAndFeel.h`).
|
||||
- Class and struct names: PascalCase, no prefix (`SynthVoice`, `RenderContext`,
|
||||
`FilterBankParams`, `ModConnection`).
|
||||
- Members: no `m_` prefix and no trailing underscore. Plain camelCase fields such as
|
||||
`note`, `velocity`, `baseFreq`, `active`, `released`, `noteId` at `SynthVoice.h:83-88`,
|
||||
and `sr`, `blockSize`, `bpm`, `pitchBend` at `Engine.h:52-57`.
|
||||
- Constants: `k` prefix for compile-time constants (`kNumVoices`, `kNumLfos` at
|
||||
`Params.h:221-227`; `kFrames`, `kTableSize` at `Wavetable.h:18-19`; `kMaxConnections` at
|
||||
`ModulationMatrix.h:28`; `kMaxAssignments` at `MacroControls.h:26`). Local constant arrays
|
||||
in anonymous namespaces also use `k` (`kEnvAttack` at `Engine.cpp:30`, `kFxType` at
|
||||
`PluginEditor.cpp:97`).
|
||||
- Enums: `enum class` (scoped) with PascalCase enumerators (`WarpMode::BendPlus`,
|
||||
`ModSource::Lfo1`, `ModTarget::Filter1Cutoff`) at `Params.h:213-219` and `Params.h:232-256`.
|
||||
- Namespaces for related constants: `ids::` for parameter ID strings (`Params.h:18`),
|
||||
`theme::` for colours (`Resources.h:12`), `maps::` for unit mappings (`Params.h:309`).
|
||||
- Method naming: camelCase with a leading verb (`noteOn`, `processAdd`, `getValue`).
|
||||
JUCE overrides keep JUCE's spelling (`processBlock`, `prepareToPlay`, `resized`,
|
||||
`timerCallback`).
|
||||
- `noexcept` is applied consistently to the per-sample DSP hot paths (`Oscillator.cpp:93`,
|
||||
`Filter.cpp:181,236`, `Wavetable.cpp:81`, `Envelope.cpp:54`, `SynthVoice.cpp:72`,
|
||||
`LFO.cpp:98`) and to cheap getters (`SynthVoice.h:71-74`).
|
||||
- Formatting: two-space indentation; a space before the opening parenthesis in function
|
||||
calls and definitions (`f (x)` at `PluginProcessor.cpp:12`, `juce::jlimit (...)`;
|
||||
`if (...)`, `for (...)`). Aligned member initialiser lists (`PluginEditor.cpp:113-115`).
|
||||
- Section dividers: `// ===...` banners at class/file tops and `// ---...` dividers within
|
||||
files (`PluginProcessor.cpp:17,198,261`, `PluginEditor.cpp:161`).
|
||||
- Casting idiom: `(size_t)` casts on int loop indices when indexing `std::vector`/`std::array`
|
||||
(`Engine.cpp:220`, `SynthVoice.cpp:79`, `FactoryPresets.cpp` throughout), and `(int)`
|
||||
casts on `size()` results.
|
||||
|
||||
---
|
||||
|
||||
## 7. Anti-pattern catalog
|
||||
|
||||
Each entry lists the location, the problem, and the standard that a later AGENT.md phase
|
||||
should encode.
|
||||
|
||||
1. Parameter ID tables duplicated three times.
|
||||
- `Params.h:18-208` (canonical `ids::`), `Engine.cpp:30-55` (`kEnv*`, `kLfo*`, `kFx*`),
|
||||
`PluginEditor.cpp:97-108,433-437,458-464,820-825,833` (the same arrays again), plus the
|
||||
display names and defaults in `PluginProcessor.cpp:31-171`.
|
||||
- Standard: one source of truth in `Params.h` (or a single generated table); engine and
|
||||
editor must reference it rather than re-declaring ID arrays.
|
||||
|
||||
2. Wavetable library allocates on the audio thread.
|
||||
- `WavetableLibrary::getTable` builds lazily (`Wavetable.cpp:126-132`), reached from
|
||||
`SynthVoice::render` (`SynthVoice.cpp:177-178`); `prebuild()` is defined
|
||||
(`Wavetable.cpp:120-124`) but never called.
|
||||
- Standard: call `prebuild()` (or build all tables) in `prepareToPlay`, never in the
|
||||
render path.
|
||||
|
||||
3. Per-block buffer resize in the audio callback.
|
||||
- `Engine::processBlock` calls `mixBuffer.setSize(...)` every block at `Engine.cpp:211`.
|
||||
- Standard: size the buffer once in `prepare()` and only `clear()` per block.
|
||||
|
||||
4. Unsynchronised GUI/audio sharing of container state.
|
||||
- `ModulationMatrix::connections` is public (`ModulationMatrix.h:30`), written by the GUI
|
||||
(`PluginEditor.cpp:536,549,559`) and read on the audio thread (`SynthVoice.cpp:114`).
|
||||
`MacroControls::assignments` is public (`MacroControls.h:28`), written
|
||||
(`PluginEditor.cpp:687,699`) and read (`SynthVoice.cpp:125`). LFO `shapeBuffer` is
|
||||
written via `setShapeData` (`LFO.cpp:51-59`) and read (`LFO.cpp:82-91`).
|
||||
- Standard: either protect these collections with a lock, or use atomic/double-buffered
|
||||
exchange between the message and audio threads.
|
||||
|
||||
5. Class name does not match its file.
|
||||
- `Source/RAVEButton.h` declares `RaveController` (`RAVEButton.h:14`), and
|
||||
`RAVEButton.cpp` implements it.
|
||||
- Standard: file pair name matches the class (`RaveController.h/.cpp`), or the class is
|
||||
renamed to match the file.
|
||||
|
||||
6. Dead code in `SubOscillator::noteOn`.
|
||||
- `SubOscillator.cpp:9-12` computes `mult` and then discards it with `(void) freqHz;
|
||||
(void) mult;` because the octave shift is already baked into the frequency by the voice.
|
||||
- Standard: remove the unused parameters or make the signature reflect what is actually
|
||||
used (the octave shift is applied at `SynthVoice.cpp:179`).
|
||||
|
||||
7. No-op override.
|
||||
- `changeProgramName` is an empty override at `PluginProcessor.cpp:226-228`.
|
||||
- Standard: implement it or do not override it.
|
||||
|
||||
8. Ignored prepare-time parameter.
|
||||
- `Filter::prepare` accepts `maxBlockSize` and discards it with `(void) maxBlockSize;`
|
||||
(`Filter.cpp:21-29`); effect units name the parameter `int` but never use it
|
||||
(`Hyper.cpp:6`, `Chorus.cpp:6`, `Flanger.cpp:6`, etc.).
|
||||
- Standard: keep the `FXUnit` interface uniform but do not add unused parameters to
|
||||
concrete `prepare()` implementations.
|
||||
|
||||
9. Duplicated envelope curve constants.
|
||||
- The attack/decay shape mapping `0.3 + curve * 2.7` / `3.0 - curve * 2.7` appears in
|
||||
`Envelope.cpp:44-45` and again in the display `EnvelopeDisplay.cpp:19-20`.
|
||||
- Standard: expose one shared helper so the preview cannot drift from the DSP.
|
||||
|
||||
10. Filter response reimplemented for display.
|
||||
- `FilterDisplay::magnitude` (`FilterDisplay.cpp:6-36`) hardcodes a parallel set of
|
||||
transfer-function approximations that duplicate the real DSP in `Filter.cpp`
|
||||
(`Filter.cpp:87-234`).
|
||||
- Standard: keep the display response derived from one documented source, or mark it as
|
||||
an approximation with a comment that the two are not the same code.
|
||||
|
||||
11. Inconsistent pi constants.
|
||||
- `kTwoPi` is defined at `Oscillator.h:67` and `SubOscillator.h:26`; `twoPi` at
|
||||
`Wavetable.cpp:21`; `kPi` at `Wavetable.cpp:156` and `Filter.cpp:8`; raw literals
|
||||
`6.28318530717958647692` appear at `Chorus.cpp:40-41`, `Flanger.cpp:38-39`,
|
||||
`Phaser.cpp:30-31`, and `6.2831853f` at `LFO.cpp:67` and `LFODisplay.cpp:19`.
|
||||
- Standard: use `juce::MathConstants<T>::pi` / `twoPi` everywhere.
|
||||
|
||||
12. Public mutable state exposes internals.
|
||||
- `PluginProcessor.h:56-58` exposes `parameters`, `engine` and `rave` as public fields;
|
||||
`ModulationMatrix.h:30` and `MacroControls.h:28` expose their collections;
|
||||
`Engine.h:36-38` returns non-const references from `getMatrix()`, `getMacros()` and
|
||||
`getWavetables()`.
|
||||
- Standard: return `const&` from accessors and route mutation through member functions,
|
||||
except where a public field is an explicit, documented design choice.
|
||||
|
||||
13. Unchecked raw parameter dereference.
|
||||
- `PluginEditor.cpp:794` dereferences `getRawParameterValue(id)->load()` without a null
|
||||
check, while the equivalent helper in `Engine.cpp:8-13` guards the pointer.
|
||||
- Standard: always guard `getRawParameterValue`/`getParameter` results before use.
|
||||
|
||||
14. Mixed ownership for GUI children.
|
||||
- `makeKnob`/`makeCombo`/`makeToggle` return raw pointers to `new`-allocated children
|
||||
that JUCE owns via `addAndMakeVisible` (`PluginEditor.cpp:162-199`), and `fxUp`/
|
||||
`fxDown` use raw `new` at `PluginEditor.cpp:602,606`. This is correct only because of
|
||||
JUCE's component-tree ownership, but it is easy to misread as a leak.
|
||||
- Standard: document that JUCE-owned children are raw pointers and never deleted, and
|
||||
keep `std::unique_ptr` for everything the plugin owns directly.
|
||||
|
||||
---
|
||||
|
||||
## 8. Hard constraints worth codifying
|
||||
|
||||
Only rules that the current code and build actually support are listed.
|
||||
|
||||
1. Do not allocate or resize containers in the audio callback. Evidence that this is the
|
||||
intent: all DSP state is sized in `prepare()` (`Engine.cpp:68`, `SynthVoice.cpp:23`,
|
||||
`FXProcessor.cpp:34-35`) and the code uses pre-sized `juce::AudioBuffer` and
|
||||
`std::array`. The current violations (`Engine.cpp:211` and the lazy wavetable build) are
|
||||
exceptions to fix, not the rule.
|
||||
2. All audio parameters are normalised to 0..1 in the APVTS, and physical units are mapped
|
||||
only through `maps::` (`Params.h:309-342`). DSP must consume the normalised values, not
|
||||
physical units.
|
||||
3. Parameter IDs live in the `ids` namespace in `Params.h` (`Params.h:18-208`). Engine,
|
||||
editor and preset code must reference `ids::`, not re-declare string literals. (This is
|
||||
the stated intent at `Params.h:5-9`; the current duplication in `Engine.cpp` and
|
||||
`PluginEditor.cpp` is a violation to eliminate.)
|
||||
4. The CMake source list is explicit (`CMakeLists.txt:50-87`), not a glob. Every file move
|
||||
or rename must update `SERUMALT_SOURCES`.
|
||||
5. All code is inside `namespace serum`, and every header uses `#pragma once` (verified
|
||||
across all headers).
|
||||
6. No exceptions and no user-facing alert dialogs. Use `jassert` for programmer errors,
|
||||
null guards for runtime lookups, and `bool` returns for bounded inserts (see
|
||||
`Oscillator.cpp:20`, `Engine.cpp:8-13`, `ModulationMatrix.cpp:6-12`).
|
||||
7. When cross-compiling for Windows, keep the VST3 manifest step disabled and let
|
||||
`build_windows.sh` inject `moduleinfo.json`, because JUCE's manifest helper must not be
|
||||
built as a Windows executable on a Linux host (`CMakeLists.txt:23-31`,
|
||||
`build_windows.sh:55-126`). Keep the injected metadata in sync with
|
||||
`PLUGIN_CODE`/`PLUGIN_MANUFACTURER_CODE`/`VERSION` (`build_windows.sh:60-62`).
|
||||
8. The project is single-threaded by design. Any future thread, mutex or atomic must come
|
||||
with an explicit plan for the GUI-to-audio boundary, because the current matrix/macro/LFO
|
||||
state is shared without synchronisation (section 5).
|
||||
Reference in New Issue
Block a user