chore: land janitor round 1 and graft integration
Round 1 findings and fixes, all previously uncommitted: - FXProcessor per-slot DSP history repair (HEAD does not compile without it) - SynthVoice::getModulationValue, editor control-lock guard - ExactCompare.h naming the deliberate exact float comparisons - KnobLookAndFeel replacing the never-installed SerumLookAndFeel - 55 first-party compiler warnings to 0 - AGENT.md renamed to AGENTS.md across Source/ - graft repository context graph and its hooks - JANITOR_TASKS.md and UI_UX_AUDIT.md records
This commit is contained in:
biggy committed
1 parent
5c942cad0c
commit
e4cea300db
63 files changed
+2360
-805
No files matched your search
@@ -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