chore: move build output under build/ and scripts into scripts/
Layout: - scripts/ holds the four build scripts plus the new clean.sh - cmake/ holds mingw64-toolchain.cmake - the scripts default to build/linux, build/macos, build/windows, build/harness - docs/ holds the audit documents, with the round 1 task list archived Cleanup: - scripts/clean.sh removes generated output, with --dry-run, --staging, --legacy and --all, and refuses tracked, protected or outside paths - the Windows VST3 manifest now comes from cmake/moduleinfo.json.in, so its version, vendor and class IDs follow cmake/Plugin.cmake instead of a hand-written heredoc that could drift - deleted the unused Source/GUI/Slider, which no code constructed - fixed the hidden double processBlock overload: three warning lines to zero - unslopped the build scripts, the two .cjs helpers and the CMake comments - .gitignore covers the upstream JUCE Projucer and DemoRunner binaries Verified: five scripts pass bash -n; the harness passes CTest with zero warning lines; the Linux VST3 and Standalone link; the Windows cross-build passes its x86-64, runtime-import and entry-point checks; each configure exposes only its own targets; and a full build leaves Source/ and third_party/JUCE unchanged. The generated manifest is identical to the previous hand-written one.
This commit is contained in:
biggy committed
1 parent
e4cea300db
commit
facd5c5b5e
25 files changed
+438
-313
No files matched your search
@@ -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` (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 AGENTS.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).
|
||||
@@ -0,0 +1,412 @@
|
||||
# SerumAlt UI and UX audit
|
||||
|
||||
Written from the working tree on branch `main`. The user report was "much of
|
||||
features arent correctly described and UI is bearly functional". That bundles
|
||||
two problems, and this audit keeps them apart:
|
||||
|
||||
1. Description. Controls lack names, units and plain descriptions.
|
||||
2. Function. Some controls do not reach the DSP they claim to drive.
|
||||
|
||||
The single largest finding explains the "barely functional" complaint on its
|
||||
own. No knob ever painted its name. `Knob` is a `juce::Slider` with
|
||||
`NoTextBox`, and JUCE does not draw a slider's component name, so every one of
|
||||
the roughly fifty knobs was an unlabelled arc whose only identification was a
|
||||
tooltip hover. The label strings passed to `makeKnob` were correct but
|
||||
invisible, and the runtime FX rename table in `updateVisuals` never reached a
|
||||
user's eyes.
|
||||
|
||||
---
|
||||
|
||||
## How this was produced
|
||||
|
||||
Reconnaissance read `Source/PluginEditor.{h,cpp}`, `Source/Params.{h,cpp}`,
|
||||
`Source/Resources.{h,cpp}`, `Source/PluginProcessor.cpp`, `Source/Engine.cpp`,
|
||||
the nine effects in `Source/EffectUnits/`, and every file in `Source/GUI/`.
|
||||
Four specialist passes then covered naming and descriptions, dead controls,
|
||||
interaction state, and layout plus accessibility. Findings were checked against
|
||||
`AUDIT_REPORT.md` (a codebase structure audit, no UI text coverage) and
|
||||
`JANITOR_TASKS.md` (fourteen tasks, all reported done; JT-01 removed the unused
|
||||
look-and-feel, JT-06 built the FX type list from `FXProcessor::fxTypeName`).
|
||||
Nothing in either file duplicates the findings below.
|
||||
|
||||
The two prior documents also correct the task brief in one respect.
|
||||
`Source/GUI/SerumLookAndFeel.h` no longer exists; JT-01 deleted it because it was
|
||||
never installed. The GUI has no custom look-and-feel, so "draw all controls
|
||||
through `SerumLookAndFeel`" resolves to "use `theme::` colours", which this work
|
||||
does.
|
||||
|
||||
Verification used `BUILD_DIR=/tmp/... ./build_harness.sh --run`.
|
||||
`UI_UX_AUDIT.md` is the deliverable that survives; the fixes listed as applied
|
||||
are already in the tree.
|
||||
|
||||
---
|
||||
|
||||
## Status vocabulary
|
||||
|
||||
- Functioning. The parameter is read by the DSP and changes what you hear.
|
||||
- Dead. Nothing reads it, or the thing that reads it throws the value away.
|
||||
- Conditional. Read and audible only in some modes or for some effect types.
|
||||
- Design decision. Fixing it needs a choice about behaviour or layout, so it was
|
||||
left alone and recorded for the user.
|
||||
|
||||
---
|
||||
|
||||
## Control inventory
|
||||
|
||||
Every user-visible control, by tab. "Readout" is what the knob shows while you
|
||||
drag it.
|
||||
|
||||
### Top bar
|
||||
|
||||
| Control | Parameter | Current label | Current tooltip | What it does and its unit | Recommended label | Recommended tooltip | Status |
|
||||
|---|---|---|---|---|---|---|---|
|
||||
| Preset combo | n/a | (preset name) | Select a factory preset | Loads one of the factory presets and replaces the whole patch | Preset | Loads a factory preset and replaces the current patch | Functioning |
|
||||
| RAVE toggle | `rave` | RAVE | RAVE one-click boost (unison, width, drive, OTT, reverb) | Render-time override: unison to 8, spread and detune to full, both filter drives to 60 %, Hyper intensity to 100 %, reverb mix +0.4 | Rave | Adds unison, width, drive, OTT and reverb to the current patch | Functioning, description was accurate, label case fixed |
|
||||
| Scale combo | `uiScale` | 75%..200% | Interface scale (75% - 200%) | Window scale, 75 % to 200 %. Index maps to a five-entry scale table | Scale | Interface scale, 75 % to 200 % | Functioning |
|
||||
| Master knob | `master` | (invisible) | Master output volume (0-100%) | Master output gain, 0 to 100 % of full scale | Master | Master output level, 0 % to 100 % | Functioning. Percent is not the engineering unit for a gain, see deferred D-8 |
|
||||
| Master readout | `master` | MASTER | none | Painted numeric readout of the same parameter | Master | none needed | Functioning |
|
||||
| Tabs | n/a | OSC, FILTER, MOD, FX, MACRO | Oscillators (Tab cycles sections) etc. | Switches the visible section. Plain Tab also cycles sections | Osc, Filter, Mod, FX, Macro | Oscillators. Tab cycles sections. | Functioning, label case fixed |
|
||||
|
||||
### Oscillator tab
|
||||
|
||||
Oscillator A and Oscillator B have identical control sets. Rows 1 and 2 are
|
||||
listed once.
|
||||
|
||||
| Control | Parameter | Current label | Current tooltip | What it does and its unit | Recommended label | Recommended tooltip | Status |
|
||||
|---|---|---|---|---|---|---|---|
|
||||
| On | `oscAOn` / `oscBOn` | On | Enable oscillator A | Gates the oscillator in `Oscillator::render` | On | Turns oscillator A on or off | Functioning |
|
||||
| Wavetable | `oscAWave` / `oscBWave` | (combo, no visible label) | Oscillator A wavetable | Selects one of 10 factory wavetables | Table | Selects the wavetable, 10 factory tables | Functioning, combo shows its own item text |
|
||||
| Warp mode | `oscAWarp` / `oscBWarp` | (combo) | Oscillator A warp mode | Selects one of 8 warp modes | Warp | Selects the wavetable warp mode | Functioning |
|
||||
| Wavetable position | `oscAWtPos` / `oscBWtPos` | Wavetable Position | Wavetable position (0-100%) | Frame position in the table, 0 to 100 % | Wt position | Wavetable frame position, 0 % to 100 % | Functioning |
|
||||
| Warp amount | `oscAWarpAmt` / `oscBWarpAmt` | Warp Amount | Warp amount (0-100%) | Warp depth, 0 to 100 % | Warp amount | Warp depth, 0 % to 100 % | Functioning |
|
||||
| Coarse | `oscACoarse` / `oscBCoarse` | Coarse | Pitch coarse (-24 to +24 semitones) | Coarse pitch, -24 to +24 semitones | Coarse | Coarse pitch, -24 to +24 semitones | Functioning |
|
||||
| Fine | `oscAFine` / `oscBFine` | Fine | Pitch fine (-100 to +100 cents) | Fine pitch, -100 to +100 cents | Fine | Fine pitch, -100 to +100 cents | Functioning |
|
||||
| Level | `oscALevel` / `oscBLevel` | Level | Oscillator A level (0-100%) | Linear amplitude gain, 0 to 1 | Level | Oscillator A level, 0 % to 100 % | Functioning |
|
||||
| Pan | `oscAPan` / `oscBPan` | Pan | Oscillator A pan (L100-R100) | Stereo position, hard left to hard right | Pan | Oscillator A pan, hard left to hard right | Functioning |
|
||||
| Unison | `oscAUnison` / `oscBUnison` | Unison | Unison voices (1-16) | Voice count, 1 to 16. Now detented so the dial rests on whole voices | Unison | Unison voices, 1 to 16 | Functioning |
|
||||
| Detune | `oscADetune` / `oscBDetune` | Detune | Unison detune (0-100%) | Per-voice detune spread, -50 to +50 cents | Detune | Unison detune spread, -50 to +50 cents | Functioning. Readout used to say "50 %" for 25 cents |
|
||||
| Spread | `oscASpread` / `oscBSpread` | Spread | Unison spread (0-100%) | Stereo spread of the unison voices, 0 to 100 % | Spread | Unison stereo spread, 0 % to 100 % | Functioning |
|
||||
| Phase | `oscAPhase` / `oscBPhase` | Phase | Phase (0-100%) | Start phase at note on, 0 to 360 degrees | Phase | Start phase at note on, 0 to 360 degrees | Functioning. Readout used to say a percentage |
|
||||
| Random phase | `oscARandPh` / `oscBRandPh` | Random Phase | Random phase (0-100%) | Random start phase per voice, up to 360 degrees | Rand phase | Random start phase per voice, 0 to 360 degrees | Functioning. Readout used to say a percentage |
|
||||
| Sub On | `subOn` | On | Enable sub oscillator | Gates the sub oscillator | On | Turns the sub oscillator on or off | Functioning |
|
||||
| Sub shape | `subShape` | (combo) | Sub oscillator waveform | Sine or triangle | Shape | Sub oscillator waveform, sine or triangle | Functioning |
|
||||
| Sub octave | `subOct` | (combo) | Sub oscillator octave | -2, -1 or 0 octaves, engine reads `value - 2` | Octave | Sub oscillator octave, 2 below to unison | Functioning |
|
||||
| Sub level | `subLevel` | Level | Sub oscillator level (0-100%) | Linear amplitude gain | Level | Sub oscillator level, 0 % to 100 % | Functioning |
|
||||
| Noise On | `noiseOn` | On | Enable noise | Gates the noise source | On | Turns noise on or off | Functioning |
|
||||
| Noise type | `noiseType` | (combo) | Noise type | White or pink | Type | Noise colour, white or pink | Functioning |
|
||||
| Noise level | `noiseLevel` | Level | Noise level (0-100%) | Linear amplitude gain | Level | Noise level, 0 % to 100 % | Functioning |
|
||||
|
||||
### Filter tab
|
||||
|
||||
| Control | Parameter | Current label | Current tooltip | What it does and its unit | Recommended label | Recommended tooltip | Status |
|
||||
|---|---|---|---|---|---|---|---|
|
||||
| On | `f1On` / `f2On` | On | Enable filter 1 | Gates the filter in `FilterBank` | On | Turns filter 1 on or off | Functioning |
|
||||
| Type | `f1Type` / `f2Type` | (combo) | Filter 1 type | One of 7 models: Ladder LP, Ladder HP, Ladder BP, Diode, Comb, Formant, Screamer | Type | Selects the filter 1 model | Functioning |
|
||||
| Slope | `f1Slope` / `f2Slope` | (combo) 6 dB, 12 dB, 24 dB | Filter 1 slope | One, two or four one-pole stages on the ladder and diode models. Comb, Formant and Screamer ignore it | Slope | Slope, 6, 12 or 24 dB per octave on the ladder and diode models | Conditional, see D-2 |
|
||||
| Cutoff | `f1Cutoff` / `f2Cutoff` | Cutoff | Filter 1 cutoff (20 Hz - 20 kHz) | Cutoff, 20 Hz to 20 kHz before key tracking, clamped to 18 kHz after it | Cutoff | Filter 1 cutoff, 20 Hz to 18 kHz | Functioning. Tooltip overstated the reachable top |
|
||||
| Resonance | `f1Res` / `f2Res` | Resonance | Filter 1 resonance (0-100%) | Resonance feedback, 0 to 98 % | Resonance | Filter 1 resonance, 0 % to 100 % | Functioning |
|
||||
| Drive | `f1Drive` / `f2Drive` | Drive | Filter 1 drive (0-100%) | Per-sample drive. Damps the comb model. Formant ignores it | Drive | Filter 1 drive, 0 % to 100 %. Damps the comb model. | Conditional, see D-3 |
|
||||
| Keytrack | `f1Key` / `f2Key` | Keytrack | Filter 1 keytrack (0-100%) | Cutoff follows note pitch, 100 % is full tracking | Keytrack | Filter 1 key tracking, 0 % to 100 % | Functioning |
|
||||
| Routing | `fRoute` | (combo) | Filter routing (serial / parallel / split) | Serial, parallel or split | Routing | Filter routing, serial, parallel or split | Functioning |
|
||||
| Mix | `fMix` | Mix | Filter mix (0-100%) | Crossfades filter 1 into filter 2. Read only in parallel routing | Mix | Blends filter 2 over filter 1 in parallel routing, 0 % to 100 % | Conditional, see D-1 |
|
||||
| Output | `fOut` | Output | Filter output (0-100%) | Output gain, engine multiplies by 1.5, so the 0.667 default is unity | Output | Filter output gain, 0 % to 150 % | Functioning. The old readout understated the range: 67 % is unity |
|
||||
|
||||
### Modulation tab
|
||||
|
||||
| Control | Parameter | Current label | Current tooltip | What it does and its unit | Recommended label | Recommended tooltip | Status |
|
||||
|---|---|---|---|---|---|---|---|
|
||||
| Attack | `env1A`..`env4A` | Attack | Attack time (0.5 ms - 12 s) | Attack time, 0.5 ms to 12 s, cubic taper | Attack | Attack time, 0.5 ms to 12 s | Functioning |
|
||||
| Decay | `env1D`..`env4D` | Decay | Decay time (0.5 ms - 12 s) | Decay time, 0.5 ms to 12 s | Decay | Decay time, 0.5 ms to 12 s | Functioning |
|
||||
| Sustain | `env1S`..`env4S` | Sustain | Sustain level (0-100%) | Sustain level, 0 to 100 % | Sustain | Sustain level, 0 % to 100 % | Functioning |
|
||||
| Release | `env1R`..`env4R` | Release | Release time (0.5 ms - 12 s) | Release time, 0.5 ms to 12 s | Release | Release time, 0.5 ms to 12 s | Functioning |
|
||||
| Curve | `env1Curve`..`env4Curve` | Curve | Envelope curve (0-100%) | Shape exponent, mapped to 0.3 to 3.0 | Curve | Curve shape. Low is a fast attack and slow decay, high is the reverse. | Functioning. The readout is still a percentage of an exponent, see D-9 |
|
||||
| Envelope graph | n/a | n/a | none | ADSR curve preview, drawn from the same shape functions as the DSP | n/a | Add a tooltip naming the envelope | Functioning |
|
||||
| Shape | `lfo1Shape`..`lfo4Shape` | (combo) | LFO shape | Sine, Triangle, Saw, Square, S&H, Step, Freehand | Shape | Selects the LFO waveform | Functioning |
|
||||
| Sync | `lfo1Sync`..`lfo4Sync` | Sync | Tempo-sync LFO | Locks the rate to host tempo and uses Beat instead of Rate | Sync | Locks the LFO to host tempo and uses the Beat knob | Functioning |
|
||||
| Rate | `lfo1Rate`..`lfo4Rate` | Rate | LFO rate (0.02 Hz - 30 Hz) | Rate, 0.02 Hz to 30 Hz, log taper | Rate | LFO rate, 0.02 Hz to 30 Hz | Functioning. Readout used to say "50 %" for about 0.77 Hz |
|
||||
| Beat | `lfo1Beat`..`lfo4Beat` | Beat | Beat division (1/32 - 4 bars) | Period of one cycle, one of 16 divisions from 1/32 beat to 16 beats | Beat | Cycle length, 1/32 beat to 16 beats | Functioning. Readout used to say a percentage, and the old tooltip was wrong at the fast end |
|
||||
| Phase | `lfo1Phase`..`lfo4Phase` | Phase | Phase (0-100%) | Start phase offset, 0 to 360 degrees | Phase | Start phase, 0 to 360 degrees | Functioning. Readout used to say a percentage |
|
||||
| Fade | `lfo1Fade`..`lfo4Fade` | Fade | Fade in (0-100%) | Fade in time, 0 to 8 seconds | Fade | Fade in time, 0 s to 8 s | Functioning. Readout and tooltip both said percent for seconds |
|
||||
| Delay | `lfo1Delay`..`lfo4Delay` | Delay | Start delay (0-100%) | Start delay, 0 to 4 seconds | Delay | Start delay, 0 s to 4 s | Functioning. Readout and tooltip both said percent for seconds |
|
||||
| LFO graph | n/a | n/a | none | Shape preview. Step and Freehand are drag-editable | n/a | Describe the drag gesture | Functioning, discoverability is D-11 |
|
||||
| Source | n/a | (combo) | Modulation source | One of 17 sources | Source | Selects the modulation source | Functioning |
|
||||
| Target | n/a | (combo) | Modulation destination | One of 35 destinations | Target | Selects the modulation destination | Functioning |
|
||||
| Depth | n/a | (invisible) | Modulation depth (-1 to +1) | Amount applied to the next Add, -100 % to +100 % of the target range. Negative inverts | Depth | Modulation amount for the next Add, -100 % to +100 % of the target range | Functioning. Readout and tooltip rewritten |
|
||||
| Bipolar | n/a | Bipolar | Bipolar modulation depth | Recentres a one-sided source for the next Add | Bipolar | Recentres a one-sided source so the modulation swings both ways, for the next Add | Functioning |
|
||||
| Add | n/a | Add | Add modulation connection | Appends a connection, capped at 32 | Add | Adds the selected source and destination to the matrix | Functioning |
|
||||
| Remove last | n/a | Remove Last | Remove last modulation connection | Removes the newest row only | Remove last | Removes the newest matrix row, older rows stay | Functioning |
|
||||
| Clear | n/a | Clear | Clear all modulation connections | Removes every connection | Clear all | Removes every modulation connection | Functioning |
|
||||
| Connection list | n/a | (read-only text) | none | Source, target, depth and polarity per row | n/a | Every modulation connection, newest last | Functioning |
|
||||
|
||||
### Effects tab
|
||||
|
||||
Each of the eight slots has the same shape: a type combo, up and down arrows,
|
||||
and five knobs. The knob names, tooltips and readout units follow the selected
|
||||
effect, and that table was rebuilt from the DSP each effect actually runs.
|
||||
|
||||
| Control | Parameter | Current label | What it does and its unit | Readout unit | Status |
|
||||
|---|---|---|---|---|---|
|
||||
| Type | `fx1Type`..`fx8Type` | (combo) | Off, Hyper, Chorus, Flanger, Phaser, Distortion, EQ, Compressor, Delay, Reverb | n/a | Functioning |
|
||||
| Up / Down | n/a | arrow glyphs | Swaps this slot's six parameters with the neighbour | n/a | Functioning. The end arrows are now disabled instead of doing nothing |
|
||||
| Mix | `fx1Mix`..`fx8Mix` | Mix | Dry and wet mix, 0 to 100 % | percent | Functioning |
|
||||
| Off P1..P4 | `fx*P1`..`fx*P4` | Unused | The rack skips an Off slot, so all four do nothing | n/a | Dead by design, now labelled and greyed |
|
||||
| Hyper | | Intensity, Low Amount, High Amount, Output | 0-100 %, 40-100 %, 40-100 %, 0.5x-1.5x | percent, percent, percent, x | Functioning |
|
||||
| Chorus | | Rate, Depth, Width, Unused | 0.1-5 Hz, 0.5-6 ms, 0-100 %, nothing | Hz, ms, percent | Rate and Depth used to read as percent; P4 is genuinely dead |
|
||||
| Flanger | | Rate, Depth, Feedback, Unused | 0.05-2.05 Hz, 0.3-5 ms, 0-90 %, nothing | Hz, ms, percent | Rate and Depth used to read as percent; P4 is genuinely dead |
|
||||
| Phaser | | Rate, Depth, Feedback, Stages | 0.05-2.05 Hz, 30-90 %, 0-70 %, 2-6 | Hz, percent, percent, count | Rate used to read as percent; Stages used to read "50 %" |
|
||||
| Distortion | | Drive, Shape, Tone, Output | 1x-25x, 0-100 %, 800 Hz-16 kHz, 0.4x-1.6x | x, percent, Hz, x | All four used to read as percent |
|
||||
| EQ | | Low Gain, Mid Gain, High Gain, Mid Freq | +/-15 dB, +/-12 dB, +/-15 dB, 200 Hz-5 kHz | dB, dB, dB, Hz | All four used to read as percent |
|
||||
| Compressor | | Threshold, Ratio, Attack, Release | -40-0 dB, 2:1-20:1, 2-100 ms, 30-500 ms | dB, ratio, ms, ms | All four used to read as percent, and two of them share a name with a different unit on the Modulation tab |
|
||||
| Delay | | Time, Feedback, Damping, Ping-Pong | 1-2000 ms, 0-92 %, 0-95 %, on or off | ms, percent, percent, on/off | Time used to read as percent; Ping-Pong used to read "50 %" |
|
||||
| Reverb | | Size, Damping, Width, Predelay | 20-99 %, 0-100 %, 0-100 %, 0-150 ms | percent, percent, percent, ms | Predelay used to read as percent |
|
||||
|
||||
The old tooltips were worse than the readouts. They were only rewritten when a
|
||||
knob's name changed, so two effects sharing a name (Rate, Damping, Feedback,
|
||||
Output) left the previous effect's tooltip on screen, and every one of them
|
||||
claimed "normalised 0-100%".
|
||||
|
||||
### Macro tab
|
||||
|
||||
| Control | Parameter | Current label | Current tooltip | What it does and its unit | Recommended label | Recommended tooltip | Status |
|
||||
|---|---|---|---|---|---|---|---|
|
||||
| Macro knobs | `macro1`..`macro4` | Energy, Width, Drive, Atmosphere | (name) macro (0-100%) | Macro amount, 0 to 100 %, drives its assigned destinations | Energy / Width / Drive / Atmosphere | Energy macro, 0 % to 100 %, drives its assigned destinations | Functioning. Names come from `MacroControls::macroName`; the macro panels have no title of their own, so the knob label is the only identifier |
|
||||
| Macro | n/a | (combo) | Macro to assign | Selects which of the four macros to edit | Macro | Selects the macro to edit | Functioning |
|
||||
| Target | n/a | (combo) | Destination parameter | One of 35 destinations | Target | Selects the destination parameter | Functioning |
|
||||
| Depth | n/a | (invisible) | Macro assignment depth (0 to 1) | Amount applied to the next Assign, 0 to 100 % of the target range | Depth | Macro amount for the next Assign, 0 % to 100 % of the target range | Functioning. Started at zero, so a default Assign was silent; now starts at half |
|
||||
| Assign | n/a | Assign | Assign destination to the selected macro | Appends an assignment, capped at 8 per macro | Assign | Adds this destination to the selected macro | Functioning |
|
||||
| Clear all | n/a | Clear All | Clear all macro assignments | Empties all four macros | Clear all | Removes every macro assignment from all four macros | Functioning |
|
||||
| Assignment list | n/a | (read-only text) | none | Target and depth per macro | n/a | Every macro assignment, grouped by macro | Functioning |
|
||||
|
||||
---
|
||||
|
||||
## Applied mechanical fixes
|
||||
|
||||
Every item below was applied, compiles, and passes the harness.
|
||||
|
||||
### Description fixes
|
||||
|
||||
- A1. Knobs now paint their name. New `Source/GUI/KnobLookAndFeel.{h,cpp}` draws
|
||||
the dial from `theme::` colours and reserves a 16 px strip at the foot of the
|
||||
knob for the label, which `Knob::paint` fills. This is the change that makes
|
||||
every other label in the editor visible for the first time.
|
||||
- A2. All eight FX parameter knobs are named, tooltipped and unit-formatted per
|
||||
effect type from one table (`kFxParamDisplay` in `PluginEditor.cpp`), replacing
|
||||
the two tables that disagreed with each other.
|
||||
- A3. Unit formatters added: LFO rate in Hz, LFO beat as a cycle length, LFO fade
|
||||
and delay in seconds, oscillator detune and both phases in cents and degrees,
|
||||
filter output as a percentage of its real 0-150 % range, modulation depth as
|
||||
-100 % to +100 %, macro depth as 0-100 %, compressor threshold and ratio,
|
||||
filter and EQ gains in dB, chorus and flanger rate in Hz and depth in ms,
|
||||
distortion drive and output as multiples, phaser stage count, ping-pong as on
|
||||
or off, reverb predelay in ms.
|
||||
- A4. Tooltips rewritten for every control that had a vague one: all eight
|
||||
effect types, both filters (cutoff now says 18 kHz, which is the real ceiling),
|
||||
all four envelopes, all four LFOs, the modulation matrix buttons, the macro
|
||||
buttons, the filters' drive and slope, both lists, the preset and scale combos,
|
||||
and the RAVE toggle.
|
||||
- A5. Labels corrected where the name was wrong or abbreviated: tab labels are
|
||||
now Osc/Filter/Mod/FX/Macro, "Random Phase" is "Rand phase" to fit its 76 px
|
||||
knob, and the RAVE toggle reads "Rave".
|
||||
|
||||
### Function fixes that were one line each
|
||||
|
||||
- B1. Double click restored a hardcoded 0.5 for every knob. Forty-nine knob
|
||||
parameters have a different default, so the gesture silently wrote the wrong
|
||||
value. `makeKnob` now reads the parameter's own default through
|
||||
`AudioProcessorParameter::convertFrom0to1`. The modulation depth knob double
|
||||
clicks to its neutral zero, and the macro depth knob to its new half.
|
||||
- B2. The macro depth knob started at zero, so an Assign made without touching
|
||||
it created an inaudible assignment. It now starts at half.
|
||||
- B3. Unison had no detent, so the dial rested between voices. Both unison knobs
|
||||
now step in whole voices.
|
||||
- B4. The first FX slot's up arrow and the last slot's down arrow were enabled
|
||||
but did nothing. They are now disabled.
|
||||
- B5. An FX parameter the selected effect ignores stayed live and editable. Those
|
||||
knobs are now disabled and labelled Unused.
|
||||
- B6. The knob value popup was never clamped to the window, so it was cut off for
|
||||
knobs near an edge. It is now clamped on both axes.
|
||||
- B7. The popup is parented to the editor while the interface scale is applied to
|
||||
a child, so it ignored the 75-200 % scale. It now measures the scale from its
|
||||
own transform and sizes itself to match.
|
||||
- B8. Switching section mid-drag could strand the popup on screen, because it is
|
||||
not a child of the hidden view. `Knob::visibilityChanged` now takes it down.
|
||||
|
||||
### Interaction and accessibility fixes
|
||||
|
||||
- C1. Toggle buttons repainted on mouse activity but never read the hover state,
|
||||
so every On, Sync and Bipolar LED looked identical under the cursor. `paint`
|
||||
now brightens the LED and the name on hover and dims both when disabled.
|
||||
- C2. `ToggleButton::paint` ignored `isEnabled()`. It no longer does.
|
||||
- C3. Knobs now take keyboard focus. `juce::Slider` disables focus in its own
|
||||
constructor, which left every knob unreachable without a mouse and cut off the
|
||||
accessible value interface.
|
||||
|
||||
### Layout constant fixes
|
||||
|
||||
`Source/GUI/AGENTS.md` requires every position to be derived from the `layout`
|
||||
namespace. Each tab declared its own private `colW`, `colGap` and panel heights,
|
||||
and the tab-bar height was the bare literal `60` in six places. Those metrics now
|
||||
live in `layout`, and the per-tab locals alias them, so the value has one home:
|
||||
|
||||
- L1. Named the tab-bar height as `layout::tabBarHeight` and used it in all five
|
||||
tab views plus the matrix and macro panel height calculations.
|
||||
- L2. Hoisted the panel grids: `twoColW`, `threeColW`, `fourColW`, `fourColGap`,
|
||||
and the panel heights `oscPanelH`, `smallPanelH`, `filterPanelH`, `envPanelH`,
|
||||
`lfoPanelH`, `macroPanelH`.
|
||||
- L3. Named `depthKnobHeight` for the matrix and macro depth knob, which was the
|
||||
literal `60` in two `setBounds` calls and again inside both list tops.
|
||||
- L4. Named `toggleWide` for the Bipolar toggle width, which was the literal `60`
|
||||
in its `setBounds` and again in the `addX` offset that had to stay in step
|
||||
with it.
|
||||
- L5. Named `headerCentreY` for the header row centre line, which was
|
||||
`titleHeight + 30` in two places with the `30` unexplained.
|
||||
|
||||
The first attempt at L5 derived the centre from `headerHeight`, which is the
|
||||
36 px toggle, and produced 40 instead of 52. That would have shifted the whole
|
||||
matrix and macro header row by 12 px. It is derived from `depthKnobHeight` now,
|
||||
and a separate check compiles the namespace and asserts all fourteen new
|
||||
constants equal the literals they replaced.
|
||||
|
||||
Two layout reports were left in place rather than acted on. The top bar still
|
||||
positions its controls with literals (`presetCombo` at 200, 16, 220, 24;
|
||||
`masterKnob` at 906, 0, 64, 60; the tab pitch of 92, and so on), because naming
|
||||
those means deciding what the top bar's own grid is, which is a design choice
|
||||
rather than a rename. The `layout::labelHeight` constant was deleted: it was
|
||||
declared and never read.
|
||||
|
||||
---
|
||||
|
||||
## Deferred items needing a design decision
|
||||
|
||||
None of these were applied. Each changes behaviour, layout or sound, and each
|
||||
needs the user's call.
|
||||
|
||||
- D-1. Filter Mix does nothing in serial and split routing. `FilterBank` reads
|
||||
`p.mix` only in the parallel branch. Either define a crossfade for the other
|
||||
two routes or disable the knob in them. Real DSP work.
|
||||
- D-2. Filter Slope does nothing for Comb, Formant and Screamer, and on
|
||||
Ladder BP the 6 dB and 12 dB settings produce the same two-stage filter.
|
||||
Making the label honest needs DSP work; the tooltip now at least names the
|
||||
models it affects.
|
||||
- D-3. Filter Drive does nothing for Formant. `Filter::formant` is called
|
||||
without the drive argument.
|
||||
- D-4. The Modulation and Macro depth knobs and the Bipolar toggle only affect
|
||||
the next Add or Assign. They look like live controls for an existing
|
||||
connection. Relabel them as defaults, or make them edit the selected row.
|
||||
- D-5. The first FX slot's down arrow moves an effect earlier in the chain while
|
||||
pointing down. In the two-column grid the reading order wraps right to left
|
||||
between rows, so slot 2 sits to the right of slot 3. Number the slots, or
|
||||
relabel the arrows for a two-column grid.
|
||||
- D-6. The Filter tab leaves about 370 px of empty space below three 300 px
|
||||
panels. Nothing breaks, but the tab looks unfinished.
|
||||
- D-7. The OSC tab puts Sub and Noise in a second row beneath both oscillators.
|
||||
The reading order is fine; the geometry suggests Sub belongs to Osc A and
|
||||
Noise to Osc B without saying so.
|
||||
- D-8. Master and the oscillator, sub and noise Level knobs show a percentage
|
||||
for a linear gain. dB is the engineering unit. Changing it changes what every
|
||||
readout says, so it was left as percent.
|
||||
- D-9. Envelope Curve shows a percentage of a shape exponent that runs 0.3 to
|
||||
3.0. "Amount" or a named curve would be better than a percentage.
|
||||
- D-10. No control marks itself as changed from its default. Nothing reads
|
||||
`getDefaultValue()`.
|
||||
- D-11. Step and Freehand LFO shapes are drag-editable, but nothing says so: no
|
||||
tooltip on an editable graph, no cursor change, no hover state, no undo, and a
|
||||
preset load silently discards the drawn shape. `LFODisplay` is not a
|
||||
`SettableTooltipClient` today.
|
||||
- D-12. Plain Tab switches section, which means JUCE's focus traversal never
|
||||
runs and a keyboard-only user cannot reach any control except a combo box or a
|
||||
text editor. This is a convention choice: keep Tab for sections and move it to
|
||||
Ctrl+Tab, or keep Tab for controls and use function keys for sections.
|
||||
- D-13. `ToggleButton` derives from `juce::Component`, so it has no accessible
|
||||
role, name or state, and cannot be activated from the keyboard. Deriving from
|
||||
`juce::Button` would fix all of that at once but changes the class contract.
|
||||
- D-14. The waveform, filter, envelope and LFO graphs have no text or numeric
|
||||
equivalent, so a screen reader announces nothing for any of them. Each needs a
|
||||
description string.
|
||||
- D-15. Knobs have no focus outline. `hasKeyboardFocus` is read nowhere.
|
||||
- D-16. No group is marked, so a group that the selected shape ignores keeps its
|
||||
old values.
|
||||
|
||||
---
|
||||
|
||||
## Verified as functioning, with evidence
|
||||
|
||||
The following were suspected dead and are not. Recorded so the check is not
|
||||
repeated.
|
||||
|
||||
- Modulation depth and polarity reach `SynthVoice` and `Engine`, and the bipolar
|
||||
flag changes the modulation math.
|
||||
- Macro depth reaches both the global and per-voice modulation sums.
|
||||
- Remove Last removes the newest matrix row, matching its label, with no hidden
|
||||
ordering rule.
|
||||
- The master readout and the master knob drive the same parameter the output
|
||||
stage uses.
|
||||
- The filter type and LFO shape combo indices agree between the combo, the
|
||||
stored value, the DSP and the display, for all 7 and all 7 values.
|
||||
- The UI scale combo's five items map exactly onto the five-entry scale table.
|
||||
- All six On toggles gate their DSP. All eight warps, all four sub and noise
|
||||
controls, all four envelopes, all four LFOs, and the matrix and macro editors
|
||||
are live.
|
||||
- RAVE's tooltip matched its implementation exactly.
|
||||
- No knob can write outside its parameter range, and the Unison readout and the
|
||||
DSP round identically.
|
||||
|
||||
---
|
||||
|
||||
## Janitor cleanups made beyond UI text
|
||||
|
||||
Kept separate from the fixes above.
|
||||
|
||||
- J-1. Deleted the now-dead formatter `fmtDepth`, which the modulation and macro
|
||||
depth knobs were the only users of.
|
||||
- J-2. Deleted `layout::labelHeight`, which was declared and never read. Toggle
|
||||
and readout label sizes are hardcoded, so the constant described nothing.
|
||||
- J-3. Replaced the duplicated per-effect label table with the single
|
||||
`kFxParamDisplay` table, which removes the latent stale-tooltip bug rather than
|
||||
only fixing its symptoms.
|
||||
- J-4. `Source/GUI/Slider.h` and `Slider.cpp` define a horizontal slider that no
|
||||
code constructs. Reported only: deleting it means editing `CMakeLists.txt` and
|
||||
the GUI module's owned-file list, which is wider than this change. Recommend
|
||||
it as a follow-up.
|
||||
- J-5. `MacroControls::removeAssignment` is defined and never called. The Macro
|
||||
tab has no control that would call it, so a single macro assignment cannot be
|
||||
deleted. Reported only: adding the control is a new feature.
|
||||
|
||||
---
|
||||
|
||||
## What could not be verified
|
||||
|
||||
There is no headless way to check a visual change in this repository. The
|
||||
harness drives `SerumAltAudioProcessor` and never constructs the editor, so it
|
||||
proves the code compiles and the audio path still renders every preset, and it
|
||||
proves nothing about the pixels. The knob label, the toggle hover state, the
|
||||
disabled FX arrows and the popup clamping were reasoned from the JUCE 7.0.12
|
||||
sources in `third_party/JUCE` and checked against them line by line, but they
|
||||
have not been seen on screen. Do not treat a passing harness run as visual
|
||||
proof.
|
||||
|
||||
One consequence worth stating plainly: the new knob look-and-feel replaces
|
||||
`LookAndFeel_V4::drawRotarySlider` for knobs only. Every other control still uses
|
||||
the shared default look-and-feel, exactly as before.
|
||||
|
||||
---
|
||||
|
||||
## Verification
|
||||
|
||||
- `BUILD_DIR=/tmp/sa-verify ./build_harness.sh --run` reaches `100% tests passed`
|
||||
with zero first-party warnings.
|
||||
- The parameter surface is unchanged, checked at runtime rather than by reading
|
||||
the source. A temporary build printed every parameter of the live processor as
|
||||
`id | name | default`. It reported 155 parameters (the 96 declared in
|
||||
`createParameterLayout` plus the 59 generated entries), and the sorted list had
|
||||
fingerprint `62b4556078a261a5b1aaa8ba34081f44` both before and after the UI
|
||||
edits. No ID, name, range or default moved. The only parameter interaction the
|
||||
fix added is a read of `getDefaultValue()` and `convertFrom0to1()`.
|
||||
- Nothing under `Source/Params.h`, `Source/Params.cpp` or
|
||||
`createParameterLayout` in `Source/PluginProcessor.cpp` was edited.
|
||||
- `third_party/JUCE` is untouched. The untracked `third_party/JUCE/DemoRunner`
|
||||
and `third_party/JUCE/Projucer` files are upstream binaries dated 2024-04-15,
|
||||
not build products of this work.
|
||||
- No checked-in `build_*` directory was written to. Every build used a directory
|
||||
under `/tmp`.
|
||||
- The pre-existing uncommitted work in the tree was preserved.
|
||||
@@ -0,0 +1,699 @@
|
||||
# Prepared janitor tasks
|
||||
|
||||
Prepared from the current working tree on branch `main`, using the `graft` context
|
||||
graph and a read-only verification pass. All 14 tasks were then dispatched to
|
||||
`janitors-coder` agents on the user's confirmation and completed. See the
|
||||
Execution status section at the end of this file for what each agent changed and
|
||||
the one remaining limitation.
|
||||
|
||||
## How to read this file
|
||||
|
||||
Each task is a handoff for one `janitors-coder` agent. The shape follows the
|
||||
janitor skill: observed problem, desired behavior, file locations, acceptance
|
||||
check, and conflicts.
|
||||
|
||||
Task types are `Defect fix` when current behavior is wrong for the user, and
|
||||
`Cleanup` when the change should preserve behavior. Fixes that change what the
|
||||
user sees or hears are marked `Risk`.
|
||||
|
||||
Baseline at preparation time. A clean harness build and CTest pass:
|
||||
`BUILD_DIR=/tmp/serumalt-check ./build_harness.sh --run` reaches `100% tests
|
||||
passed`. The build also reports first-party compiler warnings (task JT-05).
|
||||
|
||||
## Ownership before dispatch
|
||||
|
||||
The worktree is dirty. These files have uncommitted edits and should be
|
||||
committed or frozen before any task touches them:
|
||||
|
||||
```text
|
||||
Source/FXProcessor.cpp Source/PluginEditor.cpp
|
||||
Source/SynthVoice.cpp Source/SynthVoice.h
|
||||
README.md AGENTS.md
|
||||
AUDIT_REPORT.md opencode.json
|
||||
Source/AGENT.md -> AGENTS.md (staged rename)
|
||||
Source/*/AGENT.md -> AGENTS.md (staged renames)
|
||||
```
|
||||
|
||||
The `Source/FXProcessor.cpp` edit repairs code that does not compile at `HEAD`
|
||||
(`HEAD` declares a two-dimensional `units` array in `FXProcessor.h` but indexes
|
||||
it as one-dimensional in `FXProcessor.cpp`). Commit it first. Do not let two
|
||||
agents edit the same file at once.
|
||||
|
||||
## Task index
|
||||
|
||||
| Id | Title | Type | Priority | Primary files |
|
||||
| --- | --- | --- | --- | --- |
|
||||
| JT-01 | Remove the unused look-and-feel | Cleanup | P1 | `Source/PluginEditor.h`, `Source/GUI/` |
|
||||
| JT-02 | Make the LFO preview use the real LFO shape | Defect fix | P1 | `Source/LFO.{h,cpp}`, `Source/GUI/LFODisplay.{h,cpp}` |
|
||||
| JT-03 | Give macros one set of names | Defect fix | P1 | `Source/MacroControls.*`, `Source/PluginProcessor.cpp`, `Source/PluginEditor.cpp` |
|
||||
| JT-04 | Report rejected mod and macro inserts | Defect fix | P1 | `Source/PluginEditor.cpp` |
|
||||
| JT-05 | Drive first-party compiler warnings to zero | Cleanup | P2 | several `Source/` files |
|
||||
| JT-06 | Use `FXProcessor::fxTypeName` for the FX type list | Cleanup | P2 | `Source/PluginEditor.cpp` |
|
||||
| JT-07 | Remove unused members and dead tracking | Cleanup | P2 | `Source/PluginEditor.h`, `Source/Filter.*`, `Source/Engine.*` |
|
||||
| JT-08 | Remove dead parameters and discarded computations | Cleanup | P2 | `Source/SubOscillator.*`, `Source/Oscillator.*`, `Source/Wavetable.cpp`, `Source/Filter.*`, `Source/FilterBank.*` |
|
||||
| JT-09 | De-duplicate the test render loop | Cleanup | P2 | `Source/Tests/TestMain.cpp` |
|
||||
| JT-10 | Unify pi and two-pi constants | Cleanup | P2 | `Source/EffectUnits/*`, `Source/Wavetable.cpp`, `Source/SubOscillator.h`, `Source/GUI/LFODisplay.cpp`, `Source/EffectUnits/Biquad.h` |
|
||||
| JT-11 | Share the envelope curve and mark the filter preview | Cleanup | P2 | `Source/Envelope.*`, `Source/GUI/EnvelopeDisplay.cpp`, `Source/GUI/FilterDisplay.cpp` |
|
||||
| JT-12 | Reuse `paramIds::` in `PluginProcessor` | Cleanup | P3 | `Source/PluginProcessor.cpp` |
|
||||
| JT-13 | Guard the mono-buffer and capacity assumptions | Hardening | P3 | `Source/FXProcessor.cpp`, `Source/SynthVoice.cpp` |
|
||||
| JT-14 | Fix stale guidance and remaining prose slop | Cleanup | P3 | `AGENTS.md` files, `.claude/`, `build_windows.sh`, `AUDIT_REPORT.md` |
|
||||
|
||||
---
|
||||
|
||||
## JT-01 Remove the unused look-and-feel
|
||||
|
||||
Type. Cleanup.
|
||||
Priority. P1.
|
||||
Risk. None visible. The class was never installed, so deleting it changes nothing
|
||||
on screen. This outcome was chosen by the user on 2026-05-10: keep JUCE defaults.
|
||||
|
||||
Observed problem. `Source/PluginEditor.h:39` declares `SerumLookAndFeel laf`, and
|
||||
no source file installs it. A repo-wide search finds no `setLookAndFeel` or
|
||||
`setDefaultLookAndFeel` call, and `git log -S setLookAndFeel` shows none ever
|
||||
existed. Every control draws with JUCE's default look-and-feel, and the custom
|
||||
`drawRotarySlider` and `drawLinearSlider` in `Source/GUI/SerumLookAndFeel.h` have
|
||||
never run. The class and its member are dead, and `Source/GUI/AGENTS.md:19` still
|
||||
requires controls to be drawn through it. `Source/GUI/Knob.h:11` repeats the claim.
|
||||
|
||||
Desired behavior. JUCE's default look-and-feel stays in use, the dead class and
|
||||
member are gone, and the guidance matches the code.
|
||||
|
||||
Work. Delete `Source/GUI/SerumLookAndFeel.h`. Remove the include at
|
||||
`Source/PluginEditor.h:5` and the `laf` member at `Source/PluginEditor.h:39`.
|
||||
Rewrite the comment at `Source/GUI/Knob.h:11` so it no longer says the knob is
|
||||
styled by `SerumLookAndFeel`. In `Source/GUI/AGENTS.md`, drop `SerumLookAndFeel.h`
|
||||
from Owned files and remove the "Draw all controls through `SerumLookAndFeel`"
|
||||
rule. The header is header-only and absent from `SERUMALT_SOURCES`, so CMake needs
|
||||
no change.
|
||||
|
||||
Files in scope. `Source/PluginEditor.h`, `Source/GUI/SerumLookAndFeel.h`
|
||||
(delete), `Source/GUI/Knob.h`, `Source/GUI/AGENTS.md`.
|
||||
|
||||
Acceptance. `BUILD_DIR=/tmp/serumalt-check ./build_harness.sh --run` passes.
|
||||
`grep -rn "SerumLookAndFeel" Source/` returns nothing. The GUI is unchanged,
|
||||
because the class was never installed.
|
||||
|
||||
---
|
||||
|
||||
## JT-02 Make the LFO preview use the real LFO shape
|
||||
|
||||
Type. Defect fix.
|
||||
Priority. P1.
|
||||
Risk. Changes the LFO display. The audio path should not change.
|
||||
|
||||
Observed problem. `Source/GUI/LFODisplay.cpp:23-52` re-implements every LFO
|
||||
shape. The audio path implements the same shapes in `Source/LFO.cpp:65-96`. Two
|
||||
problems follow.
|
||||
|
||||
1. Sample and hold disagrees. The display computes a fixed multiplicative hash
|
||||
at `Source/GUI/LFODisplay.cpp:31-32`
|
||||
(`((step * 2654435761u) & 0xffffu) / 32768.0f - 1.0f`), while the audio
|
||||
returns `holdValue`, which comes from `juce::Random`
|
||||
(`Source/LFO.cpp:13,79,131-133`). The preview shows a sequence the synth
|
||||
never plays.
|
||||
2. The remaining shapes can drift. Triangle, saw, square, step and freehand are
|
||||
copied at `LFODisplay.cpp:26-28,34-52` and `LFO.cpp:73,75,77,80-92`.
|
||||
|
||||
The display also hardcodes `64` at `Source/GUI/LFODisplay.cpp:8,9,10,17`, while
|
||||
`LFO.h:16` already exposes `static constexpr int kShapePoints = 64;`.
|
||||
|
||||
Desired behavior. One function decides the value of a shape, and both the audio
|
||||
render and the display call it. The preview then cannot disagree with the sound.
|
||||
|
||||
Work. Extract the shape switch from `LFO::shapeValue` (`Source/LFO.cpp:65-96`)
|
||||
into a shared, pure function that takes phase, shape, step count and the shape
|
||||
buffer, for example `float lfoShapeValue (float phase, LfoShape shape, int steps,
|
||||
const float* data, int dataSize)`. Call it from both `LFO::shapeValue` and
|
||||
`LFODisplay::valueAt`. Replace the hardcoded `64` in `LFODisplay` with
|
||||
`LFO::kShapePoints`. For sample and hold, the preview should not invent values;
|
||||
either have the display read the live hold value through `Engine::getLfos()`
|
||||
(`Source/Engine.h:42`), or draw the S&H shape as a clearly labelled schematic and
|
||||
stop pretending it is the played sequence.
|
||||
|
||||
Files in scope. `Source/LFO.h`, `Source/LFO.cpp`, `Source/GUI/LFODisplay.h`,
|
||||
`Source/GUI/LFODisplay.cpp`.
|
||||
|
||||
Acceptance. `BUILD_DIR=/tmp/serumalt-check ./build_harness.sh --run` passes. A
|
||||
focused check confirms the display and `LFO::shapeValue` return the same value for
|
||||
the same phase and shape, including sample and hold. State which S&H approach you
|
||||
took.
|
||||
|
||||
---
|
||||
|
||||
## JT-03 Give macros one set of names
|
||||
|
||||
Type. Defect fix.
|
||||
Priority. P1.
|
||||
Risk. The macro parameter display names change in the host. Parameter IDs do not
|
||||
change, so saved projects still load.
|
||||
|
||||
Observed problem. The same four macros have two names. `Source/MacroControls.cpp:30-40`
|
||||
returns "Energy", "Width", "Drive" and "Atmosphere". The APVTS parameters are
|
||||
named "Macro 1" through "Macro 4" at `Source/PluginProcessor.cpp:168-171`, and
|
||||
the assignment combo repeats "Macro 1" through "Macro 4" at
|
||||
`Source/PluginEditor.cpp:686`. The macro panels and the macro list use
|
||||
`MacroControls::macroName` (`Source/PluginEditor.cpp:668-669,948`). A user sees
|
||||
"Energy" on the panel and "Macro 1" in the host automation lane and in the assign
|
||||
combo.
|
||||
|
||||
Desired behavior. One source of truth for the four macro names.
|
||||
|
||||
Work. Make `MacroControls::macroName` the source of truth. Use it to build the
|
||||
APVTS display names in `PluginProcessor.cpp` and the `macroAssignIndex` items in
|
||||
`PluginEditor.cpp`. Leave the parameter IDs (`ids::macro1` and friends) and the
|
||||
normalized values unchanged.
|
||||
|
||||
Files in scope. `Source/MacroControls.h`, `Source/MacroControls.cpp`,
|
||||
`Source/PluginProcessor.cpp`, `Source/PluginEditor.cpp`.
|
||||
|
||||
Acceptance. `BUILD_DIR=/tmp/serumalt-check ./build_harness.sh --run` passes. The
|
||||
host automation names, the assign combo and the panel labels show the same four
|
||||
strings. `grep -rn '"Macro 1"' Source/` returns only
|
||||
`Source/Params.cpp:22`, which is the stable `ModSource` serialisation token used by
|
||||
saved state and must not change.
|
||||
|
||||
---
|
||||
|
||||
## JT-04 Report rejected mod and macro inserts
|
||||
|
||||
Type. Defect fix.
|
||||
Priority. P1.
|
||||
Risk. Small UI change, no audio change.
|
||||
|
||||
Observed problem. `ModulationMatrix::addConnection` and
|
||||
`MacroControls::addAssignment` return `bool` and reject inserts past
|
||||
`kMaxConnections` or `kMaxAssignments` (`Source/ModulationMatrix.cpp:6-12`,
|
||||
`Source/MacroControls.cpp:6-13`). The editor ignores both results at
|
||||
`Source/PluginEditor.cpp:550` and `Source/PluginEditor.cpp:716`, so a click that
|
||||
fails looks identical to a click that worked. The list does not change and no
|
||||
message appears.
|
||||
|
||||
Desired behavior. A rejected insert is visible to the user.
|
||||
|
||||
Work. Check the returned value at both call sites. The smallest useful fix is to
|
||||
disable or grey the Add buttons when the relevant count reaches its cap, and to
|
||||
keep the list text and selection unchanged on rejection. A status message is also
|
||||
acceptable. Do not change the caps.
|
||||
|
||||
Files in scope. `Source/PluginEditor.cpp`. Read the caps from
|
||||
`ModulationMatrix::kMaxConnections` and `MacroControls::kMaxAssignments`; do not
|
||||
hardcode the numbers.
|
||||
|
||||
Acceptance. `BUILD_DIR=/tmp/serumalt-check ./build_harness.sh --run` passes. With
|
||||
the connection or assignment count at its cap, the Add control is visibly
|
||||
unavailable and clicking it changes nothing.
|
||||
|
||||
---
|
||||
|
||||
## JT-05 Drive first-party compiler warnings to zero
|
||||
|
||||
Type. Cleanup.
|
||||
Priority. P2.
|
||||
|
||||
Observed problem. The harness build compiles with `-Wall -Wextra` plus JUCE's
|
||||
recommended warning flags and reports more than fifty warning lines from files
|
||||
under `Source/`. Representative sites, all current:
|
||||
|
||||
- `Source/SynthVoice.cpp:131,138` sign conversion from `int` to `size_t`.
|
||||
- `Source/GUI/LFODisplay.cpp:11,12,16,32` sign conversion and a float equality.
|
||||
- `Source/PluginEditor.cpp:88` `fmtSlope` is defined and never used.
|
||||
- `Source/Oscillator.cpp:15` unused parameter `freqHz`.
|
||||
- `Source/GUI/Slider.cpp:6` a local named `formatter` shadows a member.
|
||||
- `Source/GUI/Knob.cpp:77` a local named `cursor` shadows a member.
|
||||
- `Source/Params.cpp:8,33,58,97`, `Source/Oscillator.cpp:85,108`,
|
||||
`Source/LFO.cpp:68`, `Source/Filter.cpp:57`, `Source/FXProcessor.cpp:53`,
|
||||
`Source/GUI/FilterDisplay.cpp:13`, `Source/GUI/LFODisplay.cpp:23`
|
||||
`-Wswitch-enum` cases.
|
||||
- `-Wfloat-equal` comparisons in `Source/GUI/EnvelopeDisplay.cpp:8`,
|
||||
`Source/GUI/FilterDisplay.cpp:40`, `Source/GUI/WaveformDisplay.h:18`,
|
||||
`Source/Filter.cpp:99`, `Source/LFO.cpp:43`, `Source/Oscillator.cpp:28,29`,
|
||||
`Source/MacroControls.cpp:82`, `Source/PluginProcessor.cpp:409`.
|
||||
|
||||
Desired behavior. The harness build reports no warnings from `Source/`. Warnings
|
||||
from `third_party/JUCE` are out of scope.
|
||||
|
||||
Work. List the warnings with:
|
||||
|
||||
```bash
|
||||
BUILD_DIR=/tmp/serumalt-check ./build_harness.sh 2>&1 | grep 'warning:' | grep -v third_party
|
||||
```
|
||||
|
||||
Fix the unused function, the unused parameter and the two shadowed locals by
|
||||
deletion or rename. Fix the sign-conversion sites with explicit `(size_t)` or
|
||||
`(juce::uint32)` casts, matching the surrounding style. For `-Wswitch-enum`, name
|
||||
every enumerator explicitly. For `-Wfloat-equal`, check the meaning of each
|
||||
comparison first. Some are deliberate cache keys that compare a stored parameter
|
||||
against the current one. If the exact comparison is intended, keep the behavior
|
||||
and make the intent explicit with a small named helper or a narrow, commented
|
||||
suppression. Do not silently weaken a change-detection check.
|
||||
|
||||
Files in scope. The `Source/` files named above and any other first-party file
|
||||
that the command reports.
|
||||
|
||||
Acceptance. The command above prints nothing from `Source/`.
|
||||
`BUILD_DIR=/tmp/serumalt-check ./build_harness.sh --run` passes.
|
||||
|
||||
---
|
||||
|
||||
## JT-06 Use `FXProcessor::fxTypeName` for the FX type list
|
||||
|
||||
Type. Cleanup.
|
||||
Priority. P2.
|
||||
|
||||
Observed problem. `FXProcessor::fxTypeName` is declared at
|
||||
`Source/FXProcessor.h:47` and defined at `Source/FXProcessor.cpp:51-67` with no
|
||||
callers. The same ten labels are written again as a literal `juce::StringArray`
|
||||
at `Source/PluginEditor.cpp:604-605`. The two lists must stay in the same order as
|
||||
the `FxType` enum, and nothing enforces that.
|
||||
|
||||
Desired behavior. The FX type combo is built from the one label function.
|
||||
|
||||
Work. In `PluginEditor::buildFxTab`, replace the literal array with a loop over
|
||||
`(int) FxType::Count` that adds `FXProcessor::fxTypeName (t)`. Confirm the combo
|
||||
indices still line up with `FxType` values, since the combo index is used
|
||||
directly as the parameter value (`Source/PluginEditor.cpp:246` area and the FX
|
||||
type attachment).
|
||||
|
||||
Files in scope. `Source/PluginEditor.cpp`, `Source/FXProcessor.h`.
|
||||
|
||||
Acceptance. `BUILD_DIR=/tmp/serumalt-check ./build_harness.sh --run` passes. The
|
||||
combo shows Off through Reverb in enum order and selecting a type still sets the
|
||||
matching slot type. `FXProcessor::fxTypeName` now has a caller.
|
||||
|
||||
Conflict. `Source/FXProcessor.cpp` and `Source/PluginEditor.cpp` both have
|
||||
uncommitted edits. Commit or freeze them first.
|
||||
|
||||
---
|
||||
|
||||
## JT-07 Remove unused members and dead tracking
|
||||
|
||||
Type. Cleanup.
|
||||
Priority. P2.
|
||||
|
||||
Observed problem. Four unused declarations remain:
|
||||
|
||||
1. `Source/PluginEditor.h:66` declares
|
||||
`std::vector<juce::Component*> oscAComponents, oscBComponents;`. Neither name
|
||||
appears anywhere else in `Source/`.
|
||||
2. `Filter::svfLow` and `Filter::svfHigh` are declared at
|
||||
`Source/Filter.h:53,55` and defined at `Source/Filter.cpp:108,130`. Each name
|
||||
occurs twice, at the declaration and the definition. The sibling `svfBand` is
|
||||
used at `Source/Filter.cpp:222`.
|
||||
3. `Engine::getActiveVoiceCount` (`Source/Engine.h:46`) has no callers, so
|
||||
`activeVoiceCount` (`Source/Engine.h:70`) is written at
|
||||
`Source/Engine.cpp:59` and `Source/Engine.cpp:363` and never read. The per-block
|
||||
tally at `Source/Engine.cpp:360-362` exists only to feed it.
|
||||
4. `PluginEditor.cpp:88` `fmtSlope` is unused, which the compiler also reports
|
||||
(task JT-05 overlaps here; one agent should own both).
|
||||
|
||||
Desired behavior. No unused storage and no work that feeds nothing.
|
||||
|
||||
Work. Delete the two `PluginEditor` vectors. Delete `svfLow` and `svfHigh` from
|
||||
the header and the implementation. Delete the getter, the atomic field, the
|
||||
`reset()` store and the per-block tally, unless you can point to an intended
|
||||
caller and wire it up instead. Delete `fmtSlope` unless a knob needs it; the
|
||||
slope readout currently uses a different formatter.
|
||||
|
||||
Files in scope. `Source/PluginEditor.h`, `Source/Filter.h`, `Source/Filter.cpp`,
|
||||
`Source/Engine.h`, `Source/Engine.cpp`. Optionally `Source/PluginEditor.cpp` for
|
||||
`fmtSlope`.
|
||||
|
||||
Acceptance. `BUILD_DIR=/tmp/serumalt-check ./build_harness.sh --run` passes and
|
||||
`grep -rn "oscAComponents\|oscBComponents\|svfLow\|svfHigh\|getActiveVoiceCount\|activeVoiceCount" Source/`
|
||||
returns nothing.
|
||||
|
||||
---
|
||||
|
||||
## JT-08 Remove dead parameters and discarded computations
|
||||
|
||||
Type. Cleanup.
|
||||
Priority. P2.
|
||||
|
||||
Observed problem. Four sites compute or accept values that nothing uses:
|
||||
|
||||
1. `Source/SubOscillator.cpp:9-11` computes `mult` from `octaveShift`, then
|
||||
discards it and `freqHz` with `(void)` casts. Only `phase = 0.0;` has an
|
||||
effect. The octave shift is already baked into `freqHz` by the voice
|
||||
(`Source/SynthVoice.cpp:200,212`). The only caller passes `-1` for the shift
|
||||
(`Source/SynthVoice.cpp:58`).
|
||||
2. `Source/Oscillator.cpp:15` accepts `freqHz` only to feed
|
||||
`jassert (freqHz > 0.0)`. In a release build the assert compiles out and the
|
||||
parameter is unused.
|
||||
3. `Source/Wavetable.cpp:238` and `:244` compute `const float u = ...` and
|
||||
discard it with `(void) u` at `:240` and `:246`, while `:248` immediately
|
||||
recomputes the same expression as `tt`, which is the value actually used.
|
||||
4. `Source/Filter::prepare` accepts `maxBlockSize` and discards it at
|
||||
`Source/Filter.cpp:28`. `FilterBank::prepare` forwards it to four `Filter`
|
||||
objects (`Source/FilterBank.cpp:8-11`), so the parameter is dead end to end.
|
||||
|
||||
Desired behavior. Signatures and locals describe what the code uses.
|
||||
|
||||
Work. Reduce `SubOscillator::noteOn` to the work it does, which is resetting
|
||||
phase, and update the one caller. Drop `freqHz` from `Oscillator::noteOn` and
|
||||
update the two callers at `Source/SynthVoice.cpp:170-171`. Delete the two `u`
|
||||
locals and both `(void) u` casts in `Wavetable.cpp`, keeping `tt`. Drop
|
||||
`maxBlockSize` from `Filter::prepare`, `FilterBank::prepare` and their headers,
|
||||
and update the `FilterBank` call at `Source/SynthVoice.cpp:20`. The `FXUnit`
|
||||
interface keeps its `prepare (double, int)` shape because all units share it, and
|
||||
the units that ignore the block size already leave the parameter unnamed, which is
|
||||
the existing convention. See `Source/EffectUnits/AGENTS.md` and
|
||||
`Source/synth/AGENTS.md`.
|
||||
|
||||
Files in scope. `Source/SubOscillator.h`, `Source/SubOscillator.cpp`,
|
||||
`Source/Oscillator.h`, `Source/Oscillator.cpp`, `Source/Wavetable.cpp`,
|
||||
`Source/Filter.h`, `Source/Filter.cpp`, `Source/FilterBank.h`,
|
||||
`Source/FilterBank.cpp`, `Source/SynthVoice.cpp`.
|
||||
|
||||
Acceptance. `BUILD_DIR=/tmp/serumalt-check ./build_harness.sh --run` passes.
|
||||
`grep -rn "(void)" Source/SubOscillator.cpp Source/Wavetable.cpp Source/Filter.cpp`
|
||||
returns nothing. The sub oscillator still tracks the voice frequency, because
|
||||
that frequency is applied in `SynthVoice::render`.
|
||||
|
||||
Conflict. `Source/SynthVoice.cpp` has uncommitted edits.
|
||||
|
||||
---
|
||||
|
||||
## JT-09 De-duplicate the test render loop
|
||||
|
||||
Type. Cleanup.
|
||||
Priority. P2.
|
||||
|
||||
Observed problem. `Source/Tests/TestMain.cpp` repeats the same render loop twice.
|
||||
The init-preset block at lines 49-74 and the per-preset block at lines 76-105 do
|
||||
the same work: build three note-ons, render 240 blocks of 512 samples, release at
|
||||
block 180, accumulate a NaN flag and a peak, compare against `0.001f`, print, and
|
||||
count failures. The RAVE block at lines 107-130 is a smaller variant.
|
||||
|
||||
Desired behavior. One helper runs a preset and returns whether it passed.
|
||||
|
||||
Work. Extract a function, for example
|
||||
`bool runPreset (serum::SerumAltAudioProcessor&, const juce::String& label, double maxPeakFloor)`,
|
||||
and call it for the init case and for each factory preset. Keep the output format
|
||||
and the exit code behavior. Keep the rule from `Source/Tests/AGENTS.md`: check
|
||||
non-silence across the whole render, not per block, because a release can produce
|
||||
a legitimately silent block.
|
||||
|
||||
Files in scope. `Source/Tests/TestMain.cpp`.
|
||||
|
||||
Acceptance. `BUILD_DIR=/tmp/serumalt-check ./build_harness.sh --run` passes and
|
||||
still prints one PASS line per preset.
|
||||
|
||||
---
|
||||
|
||||
## JT-10 Unify pi and two-pi constants
|
||||
|
||||
Type. Cleanup.
|
||||
Priority. P2.
|
||||
|
||||
Observed problem. `Source/synth/AGENTS.md` and
|
||||
`Source/EffectUnits/AGENTS.md` require `juce::MathConstants`. Raw literals and
|
||||
local constants remain:
|
||||
|
||||
- `Source/SubOscillator.h:26` `kTwoPi = 6.28318530717958647692`.
|
||||
- `Source/Wavetable.cpp:21` `twoPi = 6.28318530717958647692`.
|
||||
- `Source/Wavetable.cpp:156` `kPi = 3.14159265358979323846f`.
|
||||
- `Source/EffectUnits/Chorus.cpp:40,41` two-pi literals, `:46` a pi literal.
|
||||
- `Source/EffectUnits/Flanger.cpp:38,39` two-pi literals.
|
||||
- `Source/EffectUnits/Phaser.cpp:30,31` two-pi literals.
|
||||
- `Source/GUI/LFODisplay.cpp:25` `6.2831853f`.
|
||||
- `Source/EffectUnits/Biquad.h:31,45,60,75,90` write
|
||||
`2.0 * juce::MathConstants<double>::pi`, which should be `twoPi`.
|
||||
|
||||
Already converted and correct: `Source/Oscillator.h:72`, `Source/Filter.cpp:8`,
|
||||
`Source/LFO.cpp:71`, `Source/Oscillator.cpp:63` (half pi, leave it).
|
||||
|
||||
Desired behavior. One spelling for each constant, taken from
|
||||
`juce::MathConstants`.
|
||||
|
||||
Work. Replace each literal with `juce::MathConstants<double>::twoPi` or
|
||||
`juce::MathConstants<float>::twoPi` as the surrounding precision requires, and
|
||||
`pi` where only pi is meant. Remove the now-unused local constants. Keep the
|
||||
per-sample DSP value types unchanged, so an `f`-suffixed literal becomes
|
||||
`juce::MathConstants<float>::twoPi` and a `double` literal becomes the `double`
|
||||
form.
|
||||
|
||||
Files in scope. `Source/SubOscillator.h`, `Source/SubOscillator.cpp`,
|
||||
`Source/Wavetable.cpp`, `Source/EffectUnits/Chorus.cpp`,
|
||||
`Source/EffectUnits/Flanger.cpp`, `Source/EffectUnits/Phaser.cpp`,
|
||||
`Source/EffectUnits/Biquad.h`, `Source/GUI/LFODisplay.cpp`.
|
||||
|
||||
Acceptance. `BUILD_DIR=/tmp/serumalt-check ./build_harness.sh --run` passes.
|
||||
`grep -rn "6.28318530717958647692\|3.14159265358979323846\|6.2831853f" Source/`
|
||||
returns nothing. The harness reports no NaN and preset peaks match the values
|
||||
from the unmodified build.
|
||||
|
||||
---
|
||||
|
||||
## JT-11 Share the envelope curve and mark the filter preview
|
||||
|
||||
Type. Cleanup.
|
||||
Priority. P2.
|
||||
|
||||
Observed problem. Two smaller duplications remain between DSP and display.
|
||||
|
||||
1. Envelope curve. `Source/Envelope.cpp:44-45` computes
|
||||
`attackShape = 0.3 + curve * 2.7` and `decayShape = 3.0 - curve * 2.7`.
|
||||
`Source/GUI/EnvelopeDisplay.cpp:22-23` repeats both formulas with `f` suffixes.
|
||||
2. Filter preview. `Source/GUI/FilterDisplay.cpp:6-36` re-implements a magnitude
|
||||
response that does not match the DSP in `Source/Filter.cpp:87-88`. The display
|
||||
uses `q = 0.6f + res * 14.0f`, while the DSP uses
|
||||
`k = clamp (2.0 * (1.0 - res), 0.05, 2.0)`. The header calls the view
|
||||
approximate (`Source/GUI/FilterDisplay.h:11,17`), but the re-implementation
|
||||
site has no comment at all.
|
||||
|
||||
Desired behavior. The envelope preview cannot drift from the envelope DSP, and
|
||||
the filter preview states at the code that it is schematic.
|
||||
|
||||
Work. Add one shared helper for the curve mapping, for example
|
||||
`attackShapeFor (float curve)` and `decayShapeFor (float curve)` in
|
||||
`Source/Envelope.h`, and call it from both `Envelope::setParams` and
|
||||
`EnvelopeDisplay::paint`. Add a short comment at the top of
|
||||
`FilterDisplay::magnitude` that says the response is a schematic preview and does
|
||||
not model the DSP, the drive, the slope or modulation, and point to
|
||||
`Source/Filter.cpp`. Do not try to make the preview numerically match the
|
||||
nonlinear DSP.
|
||||
|
||||
Files in scope. `Source/Envelope.h`, `Source/Envelope.cpp`,
|
||||
`Source/GUI/EnvelopeDisplay.cpp`, `Source/GUI/FilterDisplay.cpp`.
|
||||
|
||||
Acceptance. `BUILD_DIR=/tmp/serumalt-check ./build_harness.sh --run` passes.
|
||||
`grep -n "2.7" Source/Envelope.h Source/Envelope.cpp Source/GUI/EnvelopeDisplay.cpp`
|
||||
returns the two uses in the shared helper only, and nothing in the two callers.
|
||||
`FilterDisplay.cpp` carries the approximate comment.
|
||||
|
||||
---
|
||||
|
||||
## JT-12 Reuse `paramIds::` in `PluginProcessor`
|
||||
|
||||
Type. Cleanup.
|
||||
Priority. P3.
|
||||
|
||||
Observed problem. `Source/Params.h:229-269` defines the `paramIds::` indexed
|
||||
groups. `Source/Engine.cpp` and `Source/PluginEditor.cpp` now iterate those
|
||||
groups. `Source/PluginProcessor.cpp` still builds its own arrays that mirror the
|
||||
same groups:
|
||||
|
||||
- `:102-106` five envelope arrays.
|
||||
- `:121-127` seven LFO arrays.
|
||||
- `:142-155` FX type, mix and parameter arrays.
|
||||
|
||||
The arrays reference `ids::`, so no string literal is duplicated. The structure
|
||||
is.
|
||||
|
||||
Desired behavior. The parameter layout walks the shared groups instead of
|
||||
re-declaring parallel arrays.
|
||||
|
||||
Work. Replace the local arrays with `paramIds::envAttack` and the other groups, or
|
||||
iterate them directly. Keep the parameter names and defaults exactly as they are,
|
||||
because they define the host-visible layout and the saved-state order. Do not
|
||||
change parameter IDs.
|
||||
|
||||
Files in scope. `Source/PluginProcessor.cpp`.
|
||||
|
||||
Acceptance. `BUILD_DIR=/tmp/serumalt-check ./build_harness.sh --run` passes.
|
||||
The parameter count and order are unchanged. A saved state from before the change
|
||||
still loads.
|
||||
|
||||
---
|
||||
|
||||
## JT-13 Guard the mono-buffer and capacity assumptions
|
||||
|
||||
Type. Hardening.
|
||||
Priority. P3.
|
||||
Risk. No behavior change for the current 2-channel, 64-sample path.
|
||||
|
||||
Observed problem. `FXProcessor::process` reads and writes channel 1
|
||||
unconditionally (`Source/FXProcessor.cpp:92,98`). It is safe today only because
|
||||
`Engine` always hands it a 2-channel view (`Source/Engine.cpp:307`). A future
|
||||
one-channel caller reads a null channel. Similarly, `FXProcessor::process` and
|
||||
`SynthVoice::render` trust the sample count to fit the buffers sized in
|
||||
`prepare` (`Source/FXProcessor.cpp:38`, `Source/SynthVoice.cpp:23`) with no
|
||||
assert. The engine's 64-sample segmentation (`Source/Engine.cpp:270`) is the only
|
||||
thing keeping that true.
|
||||
|
||||
Desired behavior. The boundaries are explicit, and a debug build catches a
|
||||
violation instead of corrupting memory.
|
||||
|
||||
Work. In `FXProcessor::process`, handle `buffer.getNumChannels() < 2` by
|
||||
processing only the channels present, or by returning early with a `jassert`.
|
||||
Add `jassert (n <= wet.getNumSamples())` and
|
||||
`jassert (numSamples <= scratch.getNumSamples())` at the top of the two render
|
||||
functions. Do not add allocation and do not change the audio-thread behavior for
|
||||
valid input.
|
||||
|
||||
Files in scope. `Source/FXProcessor.cpp`, `Source/SynthVoice.cpp`.
|
||||
|
||||
Acceptance. `BUILD_DIR=/tmp/serumalt-check ./build_harness.sh --run` passes. A
|
||||
debug build asserts when `SynthVoice::render` is called with
|
||||
`numSamples > scratch.getNumSamples()`.
|
||||
|
||||
Conflict. Both files have uncommitted edits.
|
||||
|
||||
---
|
||||
|
||||
## JT-14 Fix stale guidance and remaining prose slop
|
||||
|
||||
Type. Cleanup.
|
||||
Priority. P3.
|
||||
|
||||
Observed problem. Two kinds of documentation drift.
|
||||
|
||||
Guidance that contradicts the code:
|
||||
|
||||
- `Source/params/AGENTS.md` says to use the `ids::` namespace for every
|
||||
parameter ID and never to re-declare one outside `Params.h`. It does not mention
|
||||
the `paramIds::` indexed groups at `Source/Params.h:229`, which the engine and
|
||||
editor now use.
|
||||
- `Source/synth/AGENTS.md` says "Wavetables are prebuilt during Engine
|
||||
construction" but its example still ends with `// in prepareToPlay`. The real
|
||||
call is `Source/Engine.cpp:22` in the constructor.
|
||||
- `Source/engine/AGENTS.md` shows a "GOOD" example that calls
|
||||
`mixBuffer.clear()` in `processBlock`. The code instead builds a non-owning view
|
||||
per segment (`Source/Engine.cpp:307`). Update the example to match.
|
||||
|
||||
Prose that still violates the unslop rules:
|
||||
|
||||
- `.claude/skills/graft/SKILL.md` carries em dashes, an emoji at line 129,
|
||||
boldface across whole sentences, and colon connectors. This is the largest
|
||||
remaining prose problem.
|
||||
- `build_windows.sh:3` has an em dash, and `:58-59` uses a colon as a connector.
|
||||
- `AUDIT_REPORT.md:123,141,247,320,322` use colons as mid-sentence connectors, and
|
||||
`:286` reads "very sparingly".
|
||||
- `.claude/helpers/graft-hooks.cjs:67` and `.claude/helpers/graft-statusline.cjs:67`
|
||||
have an em dash in a trailing comment.
|
||||
- Root `AGENTS.md` has nine em dashes inside the generated
|
||||
`<!-- graft:start -->` block. That block is produced by `graft init` from a
|
||||
Graft template, so editing it in place will be overwritten on the next
|
||||
`graft init`. Fix the template source, or leave the block and note it as
|
||||
generated.
|
||||
|
||||
Desired behavior. Guidance matches the code, and prose reads plainly.
|
||||
|
||||
Work. Apply the fixes above. For each unslop edit, replace em dashes with a
|
||||
comma or a full stop, split colon connectors into two sentences, remove the
|
||||
emoji, and unbold body text. Keep literal technical terms such as "harness",
|
||||
"vector" and "surface" when they name a real thing. Do not edit
|
||||
`third_party/JUCE`.
|
||||
|
||||
Files in scope. `Source/params/AGENTS.md`, `Source/synth/AGENTS.md`,
|
||||
`Source/engine/AGENTS.md`, `.claude/skills/graft/SKILL.md`,
|
||||
`.claude/helpers/graft-hooks.cjs`, `.claude/helpers/graft-statusline.cjs`,
|
||||
`build_windows.sh`, `AUDIT_REPORT.md`, and root `AGENTS.md`.
|
||||
|
||||
Acceptance. `bash -n` passes for `build_*.sh`.
|
||||
`grep -rn "—" AGENTS.md Source/*/AGENTS.md build_windows.sh .claude/skills/graft/SKILL.md`
|
||||
returns nothing outside the generated graft block. A build and harness run still
|
||||
pass, since only comments and prose changed.
|
||||
|
||||
Conflict. `AGENTS.md`, `AUDIT_REPORT.md` and the `Source/**/AGENTS.md` files have
|
||||
uncommitted edits. Commit or freeze them first.
|
||||
|
||||
---
|
||||
|
||||
## Considered and rejected
|
||||
|
||||
These came up during verification and are deliberately not tasks.
|
||||
|
||||
- Rename `Source/RAVEButton.{h,cpp}` to match `class RaveController`. Both
|
||||
`Source/modulation/AGENTS.md:19` and root `AGENTS.md` say to keep the filenames
|
||||
unless a rename is in scope.
|
||||
- Rewrite `AUDIT_REPORT.md`. Its own header and root `AGENTS.md` mark it as a
|
||||
historical snapshot, so it is not a pending instruction. Only its prose is
|
||||
touched, in JT-14.
|
||||
- Move `PluginEditor`, `FXProcessor`, `Resources` and `RAVEButton` into
|
||||
subdirectories. The proposed moves in `AUDIT_REPORT.md` are suggestions, and
|
||||
root `AGENTS.md` says the current layout is intentional until a move is part of
|
||||
a requested task.
|
||||
- Make `ModulationMatrix::connections` and `MacroControls::assignments` private
|
||||
and force all mutation through locked members. Every current caller already
|
||||
holds `Engine::getControlLock()`, and the change is a wide interface refactor,
|
||||
not cleanup. Revisit only if a caller appears outside the lock.
|
||||
- Make the lazy `WavetableLibrary::getTable` build branch unreachable. The
|
||||
constructor prebuilds every index (`Source/Engine.cpp:22`), so the branch is
|
||||
dead today. JT-13 covers the defensive asserts; deleting the lazy path is a
|
||||
separate design decision.
|
||||
- Add tests. `AGENTS.md` says to add test infrastructure only when requested.
|
||||
|
||||
## Proposed dispatch order
|
||||
|
||||
Dispatch one task at a time per file cluster. The conflicts column above names
|
||||
the files with uncommitted edits.
|
||||
|
||||
- Batch 1, independent files: JT-02, JT-09, JT-11, JT-13.
|
||||
- Batch 2, editor cluster, serial: JT-01, then JT-03, then JT-04, then JT-06.
|
||||
- Batch 3, dead-code cluster, serial across `SynthVoice` and `Filter`: JT-08,
|
||||
then JT-07.
|
||||
- Batch 4, wide edits, serial: JT-05, then JT-10, then JT-12, then JT-14.
|
||||
|
||||
Each agent must run the harness before handing back:
|
||||
|
||||
```bash
|
||||
BUILD_DIR=/tmp/serumalt-check ./build_harness.sh --run
|
||||
```
|
||||
|
||||
## Execution status
|
||||
|
||||
All 14 tasks were dispatched and completed. Final consolidated verification on the
|
||||
frozen tree:
|
||||
|
||||
```bash
|
||||
BUILD_DIR=/tmp/jt-final CMAKE_BUILD_PARALLEL_LEVEL=8 ./build_harness.sh --run
|
||||
```
|
||||
|
||||
Result: exit 0, `1/1 Test #1: SerumAltTest ... Passed`, `100% tests passed`, and
|
||||
zero first-party compiler warnings in `Source/` (down from 55 warning lines).
|
||||
|
||||
| Id | Result | Note |
|
||||
| --- | --- | --- |
|
||||
| JT-01 | Done | `Source/GUI/SerumLookAndFeel.h` deleted. The class was never installed, so the GUI is unchanged. |
|
||||
| JT-02 | Done | One `lfoShapeValue` function now serves `LFO` and `LFODisplay`. The sample-and-hold preview is a labelled schematic and no longer invents a sequence. |
|
||||
| JT-03 | Done | Macro names come from `MacroControls::macroName` in the APVTS, the assign combo, the panels and the list. Only the `Params.cpp` serialisation tokens keep the old strings. |
|
||||
| JT-04 | Done | Add and Assign grey out at `kMaxConnections` and `kMaxAssignments`, and rejected inserts no longer look like success. |
|
||||
| JT-05 | Done | 55 first-party warning lines to 0. New `Source/ExactCompare.h` names the deliberate exact float comparisons instead of weakening them to an epsilon. |
|
||||
| JT-06 | Done | The FX type combo is built from `FXProcessor::fxTypeName`; the duplicate literal list is gone. |
|
||||
| JT-07 | Done | Removed the unused `oscAComponents`/`oscBComponents`, `Filter::svfLow`/`svfHigh`, `Engine::getActiveVoiceCount` and the per-block tally, and the unused `fmtSlope`. |
|
||||
| JT-08 | Done | Removed the dead `SubOscillator` octave math, the assert-only `Oscillator::noteOn` frequency, the discarded `Wavetable` locals, and the unused `maxBlockSize` chain through `Filter` and `FilterBank`. |
|
||||
| JT-09 | Done | The duplicated test loops are one `runPreset` helper. Output format and peak values are unchanged. |
|
||||
| JT-10 | Done | Raw pi and two-pi literals replaced with `juce::MathConstants`. The only remaining match is the intentional "BAD" example in `Source/synth/AGENTS.md`. |
|
||||
| JT-11 | Done | The envelope curve mapping lives in `Envelope.h` and both callers use it. `FilterDisplay::magnitude` now states that it is a schematic preview. |
|
||||
| JT-12 | Done | `PluginProcessor::createParameterLayout` uses the `paramIds::` groups. A layout comparison confirmed all 96 `(id, name, default)` entries are identical and in the same order. |
|
||||
| JT-13 | Done | `FXProcessor::process` returns early on a mono buffer, and capacity asserts guard `FXProcessor::process` and `SynthVoice::render`. |
|
||||
| JT-14 | Done, one limitation | Guidance and prose fixed across the `AGENTS.md` files, `.claude/skills/graft/SKILL.md`, `build_windows.sh`, `AUDIT_REPORT.md` and the two helper scripts. Nine em dashes remain inside the generated `<!-- graft:start -->` block in root `AGENTS.md`. |
|
||||
|
||||
The JT-14 limitation. The graft block in root `AGENTS.md` is generated by the
|
||||
installed `@nanonets/graft` package, whose template carries the em dashes.
|
||||
`graft init` would reproduce them, so the block was left as generated. Removing
|
||||
them needs a change to the package template at
|
||||
`/home/biggy/Documents/Coding/_skills/Graft/src/hosts/instructions.ts` and a
|
||||
reinstall, which is outside this repository.
|
||||
|
||||
Other notes.
|
||||
|
||||
- The pre-existing uncommitted work in the tree was preserved throughout. It
|
||||
includes the `FXProcessor` per-slot repair, the `SynthVoice::getModulationValue`
|
||||
addition, the editor control-lock line, and the `AGENT.md` to `AGENTS.md`
|
||||
renames. `HEAD` does not compile without the `FXProcessor` repair, so commit the
|
||||
current tree before treating any single task as an isolated diff.
|
||||
- Every task ran its own harness build in `/tmp`, so no `build_*` directory in the
|
||||
repository was modified and `third_party/JUCE` was not touched.
|
||||
- No task changed APVTS parameter IDs, so saved projects stay compatible.
|
||||
Reference in new issue
Block a user