744 lines
51 KiB
Markdown
744 lines
51 KiB
Markdown
# Code organization & structural quality — Phase Q framing
|
||
|
||
The **why** behind a dedicated code-reorganization / structural-refactor pillar. This is
|
||
the framing and settled-decision record for **Phase Q (Quality)** — bringing the ReaSampler
|
||
`src/` tree into a healthier structure (more encapsulation, granular namespaces, `src/`
|
||
subdirectories) **without sacrificing runtime performance**, against a stated quality bar:
|
||
|
||
> "**mtytel Vital is my code reference for quality.**" — Daniel
|
||
>
|
||
> Bring the codebase "**into the realm of something I can stand to look at.**"
|
||
|
||
Its build roadmap lives in **PLAN.md §Phase Q** and its authoritative spec in
|
||
**CONTEXT.md §Phase Q**. This doc holds the *why* — the quality bar, the evidence base
|
||
(a grep-verified SOLID audit), the target directory/namespace shape grounded in the Vital
|
||
reference, and the numbered fork decisions.
|
||
|
||
**Status:** framed by product-designer (2026-07-26); gate reconciled + naming dimension added
|
||
(2026-07-27); **pre-restructure functional + DSP audit wave (Q-W0) added 2026-07-27** (Daniel's ask —
|
||
§2c, forks Q-10/Q-11). Forks Q-1 … Q-11 below are the decision record; **Q-1 (namespace letter),
|
||
Q-10 (audit-report home), and Q-11 (pitch-remediation depth) are SETTLED (Daniel, 2026-07-27)**; the
|
||
remaining forks (Q-2…Q-9) carry a leading recommendation and are Daniel's to call.
|
||
Three audits ground the phase: the two structural, grep-verified ones — a **SOLID audit** (§2) and a
|
||
**naming/symbol-consistency audit** (§2b) — plus the **functional + DSP audit** defined as Q-W0 (§2c),
|
||
which runs *first* and is *executed* in-phase (this doc scopes it, staff-engineer performs it).
|
||
|
||
---
|
||
|
||
## 0. TL;DR
|
||
|
||
- **Phase Q opens with a pre-restructure audit (Q-W0), then does the structural reorg.** Before a
|
||
single file moves, a **functional + DSP quality audit** (§2c) runs first — a close-eyed pass over
|
||
the DSP paths (**pitch especially**), duplicate code / reinvented wheels, and env-coupled-constant
|
||
domain-modeling smells — producing a triaged findings report. **Q-W1 (the first structural wave)
|
||
is gated on that triage being complete and Daniel signing off on each finding's disposition.**
|
||
Fixes classified fix-now are eliminated in Q-W0 (or folded into the wave that opens the file), not
|
||
carried untouched into the reorg. Everything below §2c is the structural reorg that follows.
|
||
- **This is not a feature phase — it is a quality phase.** The code works. The pure-core /
|
||
shell split is real and healthy (30 pure static libs, each with its own CTest executable,
|
||
the discipline CMake-enforces). What Phase Q fixes is that the *shape* of the code doesn't
|
||
yet read the way the architecture actually is: `src/` is one flat 45-file directory, all
|
||
37 headers sit in one flat `reasampler` namespace, four modules have grown into
|
||
god-modules, one utility (JSON parsing) is copy-pasted across four models, and a set of
|
||
symbols are named inconsistently or collision-prone (§2b). Phase Q addresses four dimensions:
|
||
**structure** (directories), **encapsulation** (namespaces + JSON dedupe), **factoring**
|
||
(god-module splits), and **naming** (a consistent component-naming scheme, §2b + Q-7…Q-9).
|
||
- **The quality bar is Vital** (§1). Vital groups its ~1,000-file synth by *subsystem*
|
||
(`common/` `synthesis/` `interface/` `plugin/`) with nested functional sub-dirs. That is
|
||
the aspirational shape: the directory tree *is* the architecture diagram. ReaSampler's
|
||
subsystem groupings already exist — latent in the CMake link graph — but are invisible in
|
||
the code's shape. Phase Q makes them visible.
|
||
- **Performance is a hard constraint, not a nicety.** The reorg is directory + namespace +
|
||
file-split — it must cost **zero runtime**. The two hot paths (`peaks` envelope compute;
|
||
audition/preview; the realtime-capture tick) must keep their exact call/inline shape: **no
|
||
added virtual dispatch, no header→TU indirection on a hot path.** This is an explicit
|
||
acceptance criterion on every point, not a footnote (§3).
|
||
- **This phase is GATED on the tree being otherwise quiescent** (§4). A structural reorg that
|
||
lands while Phase S (on the phase-s worktree) is mid-flight would create catastrophic merge
|
||
conflicts — the reorg touches nearly every file, and every in-flight branch is diffed against
|
||
the *old* layout. As of 2026-07-27 the outstanding work is **Phase S** (merged to dev) and
|
||
**Phase L L3** (the VST restyle, itself gated on Phase S) — Daniel's plain target: *"when
|
||
Phase S and L3 are finished."* (L1/L2/L4–L7 have already landed; D2 is functionally complete;
|
||
M9 is abandoned.) Stated prominently because getting the gate wrong is the one way this phase
|
||
does real damage.
|
||
- **Beyond SOLID, Phase Q also fixes naming.** The reorg gives every symbol a *directory +
|
||
namespace home* (Q-3/Q-4); §2b's grep-verified naming audit adds the orthogonal dimension of
|
||
giving poorly/inconsistently-named symbols a *consistent name*, measured against the same
|
||
Vital "something I can stand to look at" bar. Renames ride the waves that already relocate the
|
||
file (a rename is nearly free when a file is already moving); the scheme + new forks (Q-7…Q-9)
|
||
are in §6.
|
||
- **Every point is independently landable and CTest-green at every step** (§5). The CMake
|
||
targets already draw the module seams; a file move + namespace change keeps
|
||
`ctest --test-dir build` green at each point. Green-CTest-at-every-point is an acceptance
|
||
criterion, not an aspiration — it is *how* a reorg this broad stays safe.
|
||
|
||
---
|
||
|
||
## 1. The quality bar — Vital, read from its actual structure
|
||
|
||
Daniel named **Vital** (`github.com/mtytel/vital`, the spectral-warping wavetable synth) as
|
||
his code-quality reference. Read from the actual repository, not from reputation, here is
|
||
what makes Vital's structure the aspirational shape — and what maps cleanly onto ReaSampler.
|
||
|
||
### 1.1 Vital groups by subsystem; the directory tree *is* the architecture
|
||
|
||
Vital's `src/` is organized top-level by **subsystem concern**:
|
||
|
||
| Vital `src/` dir | Contains | ReaSampler analogue |
|
||
|---|---|---|
|
||
| `common/` | shared utilities, data types used everywhere | the pure data/util core (`bank_model`, `app_version`, a new `json`) |
|
||
| `synthesis/` | the audio engine — nested further by concern | the capture/audio core (`capture`, `peaks`, `render_settings`, `wav_trim`) |
|
||
| `interface/` | all UI / editor code | the UI core + shells (`theme`, `component_geometry`, `bank_panel`, `draw_kit`) |
|
||
| `plugin/` | the plugin-wrapper / host-boundary layer | the REAPER-facing shells + `main.cpp` |
|
||
| `headless/` `standalone/` | build-shape entry points | (n/a in one extension binary; the Phase S VST3 is the second artifact) |
|
||
|
||
And critically, `synthesis/` is **itself** subdivided by function —
|
||
`synth_engine/ modulators/ filters/ effects/ producers/ framework/ lookups/ utilities/`.
|
||
The grouping principle is **layered functional architecture**: core generation → processing
|
||
→ infrastructure → support, each a directory. You can read the subsystem map off the folder
|
||
tree without opening a file. *That* is "code you can stand to look at": the structure
|
||
teaches you the architecture.
|
||
|
||
### 1.2 What ReaSampler has vs. what Vital has
|
||
|
||
ReaSampler's architecture is **already** subsystem-layered — it is just invisible:
|
||
|
||
- **The pure/shell split is real and CMake-enforced** — 30 pure static libraries, each with
|
||
its own test executable, none linking a REAPER SDK. This is the *load-bearing* discipline
|
||
and it is genuinely healthy (the SOLID audit confirms: "NO genuine core→REAPER leaks
|
||
found — the split is intact and CMake-enforced").
|
||
- **But the *shape* hides it.** All 45 files sit in **one flat `src/` directory**; all 37
|
||
headers sit in **one flat `reasampler` namespace**. The subsystem groupings —
|
||
model / view / capture / audio / ui / reclaim / version — exist only in the link graph
|
||
(`target_link_libraries` edges), never in the code you *read*.
|
||
- **Vital's lesson, applied:** ReaSampler doesn't need to *invent* an architecture — it needs
|
||
to make its existing one **legible**. The reorg is a change of *shape*, not of *substance*.
|
||
That is exactly why it can be zero-runtime-cost and CTest-green throughout: the seams
|
||
already exist; Phase Q draws them where a reader can see them.
|
||
|
||
*(Note on the reference: Daniel wrote "mtytell"; the canonical repo is `mtytel/vital`. The
|
||
structural facts above are read from that repository's `src/` tree. Vital is GPLv3 — we
|
||
borrow its **structural pattern**, not its code.)*
|
||
|
||
---
|
||
|
||
## 2. The evidence base — grep-verified SOLID audit (Single-Responsibility lens)
|
||
|
||
The reorg is not driven by taste alone; it is grounded in a **grep-verified** staff-engineer
|
||
SOLID analysis of the actual `src/` tree (45 files / ~19,800 LOC). Reproduced here as the
|
||
decision-grade evidence; the PLAN points and CONTEXT spec cite back to it.
|
||
|
||
### 2.1 The three structural problems that dominate
|
||
|
||
1. **Four god-modules** — each carrying 4–8 distinct responsibilities:
|
||
- `bank_panel.cpp` (**2,424 LOC**) — 8+ responsibilities: rendering, thumbnail
|
||
compute+cache, audio audition engine, bank CRUD (*duplicates `actions.cpp`'s verbs*),
|
||
context menus, input handling, new-content detection, OS drag-out + drop-target, window
|
||
lifecycle + a ~20-function public API.
|
||
- `main.cpp` (**1,762 LOC**) — owns-the-API-pointers is only ~120 lines of it. Also
|
||
carries: full capture orchestration (`RunCapture` / `captureAndIndexOne` /
|
||
`renderOffline` / batch/recapture/realtime `Run*`), `FxBypassGuard` (100+ LOC,
|
||
precision-invariant-critical, buried), scope/source resolution, provenance assembly,
|
||
the realtime-capture lifecycle state machine + globals, two RAII selection guards, and
|
||
~350 lines of hand-written registration boilerplate.
|
||
- `actions.cpp` (**981 LOC**) — two unrelated command-id families in one TU (Design View
|
||
family + multi-bank/prune family, the latter holding `doBankPruneFolder`, **the only
|
||
file-deletion authority**), plus its own `promptText`/`mintBankId` *duplicating*
|
||
`bank_panel`.
|
||
- `persist.cpp` (**766 LOC**) — 5 responsibilities: session lifecycle+poll, ext-state
|
||
serialization bridge, folder relocation, GUID minting, and prune scanning + filesystem
|
||
deletion (`deleteOrphanFile` via `SHFileOperationW`).
|
||
|
||
2. **JSON serialization duplicated across four model modules.** `bank_model`, `bank_book`,
|
||
`view_mode_model`, and `owned_manifest` **each hand-roll their own `Parser`**
|
||
(parseString / parseInt / parseKey / skipValue + escape). This is the single largest
|
||
DRY + SRP violation and the **highest-leverage single fix** in the phase.
|
||
|
||
3. **Zero namespace granularity.** All 37 headers live in one flat `reasampler` namespace;
|
||
`src/` is one flat directory. Subsystem groupings are latent in the CMake link graph but
|
||
invisible in code shape — the exact gap §1 identifies against Vital.
|
||
|
||
### 2.2 Per-module Single-Responsibility verdicts (the audit's own words)
|
||
|
||
- **Pure core is mostly clean.** `peaks`, `provenance`, `prune_reconcile`, and every geometry
|
||
mirror (`mode_switch` / `bank_grid` / `tab_strip` / `action_buttons` /
|
||
`component_geometry` / `prune_button`) are **single-responsibility exemplars** — leave them
|
||
alone, just relocate + namespace them.
|
||
- **Mixed (model + JSON):** `bank_model`, `bank_book`, `view_mode_model` (1,040 LOC — 4
|
||
indices + planner + JSON), `owned_manifest`. The JSON extraction (Q-W1) resolves the mixed
|
||
half of each.
|
||
- **Shells split cleanly:** `capture` / `capture_realtime` (two backends behind one header);
|
||
`view` (derive-visibility + apply-to-REAPER).
|
||
|
||
### 2.3 Other SOLID letters (secondary, in-scope where cheap)
|
||
|
||
- **O (Open/Closed):** the capture-action *table* is good OCP; the ~350-line hand-written
|
||
**non-table** registration blocks in `main.cpp` are the opposite — every new action edits
|
||
4 parallel places. A registration-table closes this (a candidate Q point, Q-6 dependent).
|
||
- **I (Interface Segregation):** `capture.h` / `persist.h` / `bank_panel.h` are **fat
|
||
headers** — split them alongside their TU splits.
|
||
- **L / D:** `ICaptureBackend`/`Offline`/`Realtime` is a clean Liskov story (no violation);
|
||
`main`/`bank_panel` depending on concrete backends is a low-priority D concern, explicitly
|
||
**out of scope** for Phase Q (it is a design change, not a reorg).
|
||
|
||
### 2.4 Encapsulation gaps (beyond the four god-modules)
|
||
|
||
- **JSON parser duplicated 4× → extract a pure `json` core** (the highest-leverage change).
|
||
- **Bank-CRUD verbs duplicated** in `bank_panel.cpp` *and* `actions.cpp` — dedupe to one
|
||
owner.
|
||
- `peaks` forces whole-file `std::vector<float>` materialization on the thumbnail path (clean
|
||
API, but a data-ownership boundary forces a copy) — **noted, not scoped**: touching it
|
||
risks the hot path (§3), so it is a deliberate non-goal for Phase Q.
|
||
|
||
---
|
||
|
||
## 2b. The naming audit — grep-verified symbol/module-naming inconsistencies
|
||
|
||
The SOLID audit (§2) grounds *where responsibilities live*; this section grounds *what things are
|
||
called*. Same rigor: **every claim below cites a symbol or file verified by grep/read of the
|
||
actual `src/` tree (2026-07-27), not taste asserted in the abstract.** The reorg is the moment to
|
||
fix naming because a rename is nearly free when the file is already being relocated or split
|
||
(Q-W1–Q-W6). Measured against the same bar: *"something I can stand to look at"* means a newcomer
|
||
can predict a symbol's name from its role and never meets two unrelated things sharing one name.
|
||
|
||
### 2b.1 The good news — what is already consistent (leave alone)
|
||
|
||
Two families are already named on a legible principle; the audit's job there is only to *protect*
|
||
them through the reorg, not to change them:
|
||
|
||
- **The geometry-mirror *verb* vocabulary is consistent.** Every pure layout/hit-test module uses
|
||
the same two verbs: `compute<Thing>Rects` / `compute<Thing>` for layout and `hitTest<Thing>` for
|
||
hit-testing — verified across `bank_grid` (`computeCellRects`/`hitTestCell`), `mode_switch`
|
||
(`computeSegmentRects`/`hitTestSegment`), `action_buttons` (`computeButtonRects`/`hitTestButton`),
|
||
`action_bar` (`computeBarSlots`/`hitTestActionBar`), `tab_strip`
|
||
(`computeTabRects`/`hitTestTabStrip`), `prune_button` (`computePruneButton`/`hitTestPruneButton`),
|
||
`overflow_menu` (`computeMenuButton`/`hitTestMenuButton`), `footer_bar`
|
||
(`computeFooterBar`/`hitTestFooterBar`), `card_drag` (`computeSlotRects`/`hitTestSlot`),
|
||
`component_geometry` (`computeButtonBox`/`hitTestBox`). This is a real, followed convention —
|
||
preserve it verbatim.
|
||
- **The `_tests` suffix is uniform.** Every pure module's CTest executable is `<module>_tests`
|
||
(30 targets in CLAUDE.md's table, no exceptions). No action.
|
||
|
||
### 2b.2 Ambiguous / collision-prone symbols (the highest-priority fixes)
|
||
|
||
These are the naming equivalent of the JSON-`Parser` DRY violation — concrete hazards, not taste:
|
||
|
||
1. **Four hand-rolled `Parser` classes, one name.** `class Parser` is defined **four times** —
|
||
`bank_model.cpp:306`, `bank_book.cpp:663`, `owned_manifest.cpp:107`, `view_mode_model.cpp:654`.
|
||
Q-W1 already deletes three of them by extracting `core/json`; the naming rule is that the
|
||
survivor is **`json::Parser`** (or a more specific `json::Reader`/`json::Writer` pair — see
|
||
Q-8), never a bare `Parser` in flat scope.
|
||
2. **`FooterRect` and `ButtonRect` are shared across pure UI modules — and the codebase already
|
||
*knows* it.** `struct FooterRect` and `struct ButtonRect` are defined in `prune_button.h`
|
||
(lines 32, 46) and **reused** by `footer_bar.h`, which carries an explicit in-file "NAME NOTE"
|
||
(`footer_bar.h:27–34`) documenting that `ButtonRect / FooterRect / SegmentRect / ActionBarRect /
|
||
KitBox / KitButtonBox` are "already owned in this namespace" and that new types must carry a
|
||
`FooterBar*` prefix to avoid collision. That comment is a smell made visible: the flat
|
||
`reasampler::` namespace forces every pure-UI author to hand-check for name collisions before
|
||
minting a type. This is the single strongest in-codebase argument for the Q-4 sub-namespaces —
|
||
under `reasampler::ui` these shared rect types get one clear owner and the hand-checking stops.
|
||
3. **`Sample` (`bank_model.h:69`, the bank metadata struct) vs `AudioSample` (the `peaks` float
|
||
alias).** Already flagged in §2.4/Q-4; verified — `Sample` is the model record, `AudioSample`
|
||
is a raw PCM float. Under `model::Sample` vs `audio::AudioSample` the collision risk is gone,
|
||
but the *names* still read oddly side by side (a `Sample` that is metadata, an `AudioSample`
|
||
that is one float). Noted; the namespace split is the required fix, a rename is optional (Q-8).
|
||
4. **`Selection` (`bank_grid.h:112`) and `CellRect` (`bank_grid.h:23`) are generic names in a
|
||
flat namespace.** `Selection` in particular is the kind of name a newcomer cannot place without
|
||
opening the file. `ui::Selection` / `ui::CellRect` resolve it structurally; no rename needed
|
||
beyond the namespace.
|
||
|
||
### 2b.3 Inconsistent module/type *naming families* (the taste-but-grounded tier)
|
||
|
||
Here the names are legal and non-colliding but do not read on one principle — the "stand to look
|
||
at" gap:
|
||
|
||
1. **The model-family suffixes disagree: `_model` vs `_book` vs `Index`.** Verified: the pure model
|
||
modules are `bank_model.{h,cpp}` (owning `class BankIndex`, `bank_model.h:132`), `bank_book.{h,cpp}`
|
||
(owning `class BankBook`, `bank_book.h:208`), `view_mode_model.{h,cpp}` (owning `class ViewModeModel`,
|
||
`view_mode_model.h:376`), `owned_manifest.{h,cpp}` (owning `class OwnedFileManifest`,
|
||
`owned_manifest.h:52`). Four modules, four different file↔class naming relationships:
|
||
`bank_model`→`BankIndex` (file says "model," class says "index"), `bank_book`→`BankBook`
|
||
(file = class), `view_mode_model`→`ViewModeModel` (file = class), `owned_manifest`→`OwnedFileManifest`
|
||
(file ≈ class, but the class adds "File"). The `bank_model`/`BankIndex` mismatch is the worst:
|
||
the file name and its primary class name share no word. This is a genuine legibility wart — the
|
||
fix is a *rename decision* (Q-8), not something the directory move alone resolves.
|
||
2. **The `bank_book` "wraps `bank_model`" relationship is invisible in the names.** `BankBook`
|
||
(`bank_book.h:208`) is a registry of `Bank` (`bank_book.h:147`), each wrapping a `BankIndex`
|
||
(`bank_model.h:132`). The names `Book` → `Bank` → `Index` do not read as a containment hierarchy;
|
||
a reader has to learn it. (Not necessarily worth a rename — "book of banks" is evocative — but
|
||
it is the kind of call Q-8 should make deliberately, not by accident.)
|
||
3. **`realtime_record.h` (pure) vs `capture_realtime.cpp` (shell) — the word order flips.** Verified:
|
||
the pure realtime module is `realtime_record.{h}` (owning `RecordModePlan`/`RecordPhase`/
|
||
`RecordTickInputs`, `realtime_record.h:57–173`) while its shell is `capture_realtime.cpp`. So the
|
||
pure core is `realtime_record` but the shell is `capture_realtime` — the two halves of one feature
|
||
are named on inverted word order (`realtime_record` vs `capture_realtime`). Compare the *clean*
|
||
shell-pair convention elsewhere: `drag_out` (pure) ↔ `drag_out_win` (shell) — same stem, suffix
|
||
marks the platform shell. The realtime pair breaks that pattern. This is the clearest shell↔core
|
||
naming-drift instance in the tree (Q-9).
|
||
4. **`capture.{h,cpp}` is the *offline* backend shell, but the name claims all of capture.**
|
||
Verified: `capture.h` declares `ICaptureBackend`, `OfflineRenderBackend`, **and**
|
||
`RealtimeRecordBackend` (`capture.h:112,124,201`), while the realtime *implementation* lives in
|
||
`capture_realtime.cpp` and its pure planner in `realtime_record.h`. So `capture` is really
|
||
"capture interface + offline backend," a fat header (the §2.3 Interface-Segregation concern) whose
|
||
name oversells its scope. Its Q-W3 hoist (`capture_orchestrator`/`scope_resolve`) is the moment
|
||
to right-size the name.
|
||
|
||
### 2b.4 Abbreviations / opacity (low-severity, opportunistic)
|
||
|
||
Swept for names a newcomer couldn't decode; the tree is mostly clean here (a credit to it). Two
|
||
minor notes:
|
||
|
||
- **`guid_diff` / `GuidBaseline` (`guid_diff.h:40`)** — "GUID diff" is decodable in context (it
|
||
diffs the live track/item GUID set between polls) but `GuidBaseline` reads more clearly as "the
|
||
previous-poll snapshot" than the module name suggests. Low priority; leave unless its `core/view`
|
||
relocation invites it.
|
||
- **`MinMax` (`peaks.h:30`), `KitBox` (`component_geometry.h:28`)** — terse but correct and local;
|
||
no change. Named here only to record they were swept and cleared.
|
||
|
||
### 2b.5 What the naming audit does NOT touch (hard boundary)
|
||
|
||
The FOREVER-STABLE on-the-wire/on-disk contracts are **not** C++ symbol names and are **out of
|
||
scope for every rename**: `command_id` strings (`CEREBELLUM_REASAMPLER_*` / `_BETA_`), action
|
||
display names (`"ReaSampler: …"`), ext-state namespace (`"reasampler"` / `"reasampler_beta"`) and
|
||
its keys (`"banks"`, `"view_state"`, `"tail_setting"`, `"owned_files"`, `"version"`), the
|
||
`reasampler:` lane-name prefix, and the Phase S VST3 class UID. Renaming a C++ class is orthogonal
|
||
to these strings; the audit's renames touch symbols only, never a shipped contract literal. This
|
||
is the same guardrail §7 states for the reorg, restated for the naming dimension because a careless
|
||
"tidy the names" pass is exactly how a shipped id gets broken.
|
||
|
||
---
|
||
|
||
## 2c. The pre-restructure functional + DSP quality audit (Q-W0 — runs FIRST)
|
||
|
||
The §2 SOLID audit and §2b naming audit are **structural** analyses — they ground *where
|
||
responsibilities live* and *what things are called*. Daniel asked (2026-07-27) for a **third audit
|
||
that runs before the reorg begins**, on a different axis: *does the code do the right thing, and does
|
||
it do it well.* That is **Q-W0** — a thorough functional + DSP static analysis producing a written,
|
||
triaged findings report, complementary to (not a repeat of) §2/§2b. Its rationale: the structural
|
||
reorg is the wrong moment to discover a bad algorithm. A reinvented wheel or a numerically-fragile
|
||
DSP path should be eliminated — or consciously documented — **before** it is relocated,
|
||
re-namespaced, and split, not carried forward untouched into a tidier tree. "Something I can stand to
|
||
look at" is partly shape (§2/§2b) and partly *functional soundness* (§2c).
|
||
|
||
Q-W0 is **not executed by product-designer** — this doc *defines the wave and its scope*; the audit
|
||
itself is staff-engineer/DSP work. The authoritative wave definition lives in **PLAN.md §Q-W0** and
|
||
its spec framing in **CONTEXT.md §"The pre-restructure audit wave (Q-W0)"**; this section is the
|
||
evidence-doc framing (the *why* and the smell taxonomy), matching how §2/§2b frame the structural
|
||
audits.
|
||
|
||
### 2c.1 Audit surfaces (the named targets)
|
||
|
||
1. **DSP / audio — a close eye on pitch** (Daniel's verbatim emphasis). Assess *algorithm quality* —
|
||
correctness, artifacts, numerical robustness, interpolation quality, reinvented-wheel vs.
|
||
established DSP technique — across:
|
||
- **`src/vst/pitch_shift`** — the **hand-rolled OLA pitch-preserve** engine. Highest priority.
|
||
Look at window function + overlap factor choice, phase coherence between frames, transient
|
||
smearing and formant behavior, and buffer-edge handling. A hand-rolled OLA shifter is exactly
|
||
the class of "reinvented wheel / poor-quality algorithm" Daniel wants scrutinized: is the
|
||
technique sound, are the artifacts acceptable, is a better-established method (e.g. a
|
||
phase-vocoder or WSOLA variant) warranted — and if the current approach is kept, is that a
|
||
*documented* decision?
|
||
- **`sampler_core`** — repitch ratio math, interpolation order/quality (linear vs. higher-order),
|
||
loop-point-aware sustain crossfade behavior, and voice-stealing correctness (clicks or
|
||
discontinuities on steal).
|
||
- **`peaks`** — envelope min/max binning correctness.
|
||
- **`wav_trim`** — the realtime-tail decay-scan threshold + truncate plan (numerical edge cases).
|
||
- **capture / tail paths** — DSP-adjacent arithmetic in range/tail handling.
|
||
2. **Architecture smells (functional lens).** Duplicate code, reinvented wheels, poor abstractions,
|
||
and leaky pure/shell boundaries — overlapping §2/§2b's territory but reporting the *functional*
|
||
smell those audits did not target (a duplicated *algorithm*, not merely a duplicated
|
||
*responsibility*; a `core/` module reaching a host type; algorithm math stranded untestable in a
|
||
shell).
|
||
3. **Env-coupled-constant domain-modeling smells — a first-class category.** ANY value stored in a
|
||
**frame / rate / DPI / tick-coupled** domain that should instead be stored **rate-free and
|
||
resolved at the point of use** is a domain-modeling smell. The correct framing is *store rate-free,
|
||
resolve at use* — **not** "rescale by the rate at the boundary." This is grounded in the
|
||
load-bearing invariant that wall-clock times are stored as **rate-free SECONDS** resolved against
|
||
the live project rate (`sample_map`), with **NO hardcoded sample rates in `src/`** (Daniel's
|
||
standing ruling). **There was a prior incident on exactly this** — envelope times stored in the
|
||
frame domain — which is why it is called out as its own smell category and not folded into
|
||
"architecture smells." Sweep at least: envelope times, loop points, fade lengths, tail lengths,
|
||
and any UI geometry constant that silently bakes in a DPI or a rate.
|
||
|
||
### 2c.2 Deliverable, triage, and the sub-gate
|
||
|
||
- **Deliverable:** a **written findings report** over the surfaces above — a committed doc,
|
||
`docs/product/code-quality-audit.md` (Q-10, settled §2c.3), beside this framing.
|
||
- **Triage:** every finding is classified **eliminate-before-restructure** (fix-now) or
|
||
**document-and-defer** (with a one-line rationale, so a deferral is a recorded decision, never a
|
||
silent omission).
|
||
- **Where fixes land:** fix-now findings are remediated **in Q-W0**, or folded into the downstream
|
||
wave that already opens the file (recorded per finding). They are **not** silently deferred into the
|
||
structural waves — eliminations happen here or as folded-in remediation, so Q-W1+ inherits a
|
||
functionally-sound tree.
|
||
- **Reshaping downstream:** Q-W0's findings may **add or reshape** Q-W1..Q-W6 points (an algorithm
|
||
rewrite that changes a module's shape; a domain-modeling fix that changes a payload). Those reshapes
|
||
are folded in before Q-W1 begins.
|
||
- **The sub-gate:** Q-W1 does not begin until Q-W0's **triage is complete and Daniel has signed off**
|
||
on every finding's disposition. This is the entry gate to the whole structural reorg.
|
||
|
||
### 2c.3 Q-10 — where the findings report lives. SETTLED (Daniel, 2026-07-27): a committed doc.
|
||
|
||
The Q-W0 findings report is a **committed doc under `docs/product/`** — `code-quality-audit.md`,
|
||
beside the SOLID/naming audit this doc already holds. It travels with the tree, each finding's
|
||
disposition is reviewable in one place, and it matches the precedent set by §2/§2b living as
|
||
committed framing. The Q-W0 "Triage + report" step writes *this* file — the report's home is
|
||
explicit, not a placeholder.
|
||
|
||
- **Considered and set aside:** a tracked **issue list** (one issue per finding). Would fit if each
|
||
finding needed independent lifecycle state (assignee, open/closed) driving work item by item. Set
|
||
aside because the audit is a one-shot pre-reorg sweep, not an ongoing backlog, and a single
|
||
reviewable doc reads better against the "stand to look at" bar.
|
||
|
||
### 2c.4 Q-11 — how deep pitch remediation goes. SETTLED (Daniel, 2026-07-27): defer to findings.
|
||
|
||
The remediation depth is **not pre-committed** — it is decided by what the Q-W0 audit actually
|
||
finds in the OLA pitch-preserve engine (`src/vst/pitch_shift`). The settled rule:
|
||
|
||
- **Default: document-and-defer.** Absent findings that matter, record the artifacts/limits as a
|
||
known, documented trade-off and change nothing now — this keeps Phase Q a reorg.
|
||
- **Escalation, in order, only if the audit surfaces artifacts that matter for the tool's use:**
|
||
weigh a **bounded fix** (tune window/overlap/edge handling within the existing OLA approach — no
|
||
new dependency, no new module shape) **before** a **technique replacement**.
|
||
- **Technique replacement is a Daniel decision at triage time, not an automatic Q-W0 action.**
|
||
Swapping in a stronger method (phase-vocoder / WSOLA) is a real DSP change that reshapes
|
||
`pitch_shift` and would **spill a downstream Q-wave point** — so if the audit's findings point that
|
||
way, Q-W0 surfaces the recommendation and Daniel makes the call; Q-W0 does not perform a technique
|
||
replacement on its own authority.
|
||
|
||
---
|
||
|
||
## 3. Performance is a hard constraint (the guardrail, carried verbatim-in-spirit)
|
||
|
||
Daniel's stated non-negotiable: reorganize **without sacrificing actual performance.** The
|
||
reorg is directory / namespace / file-split — it is **zero runtime cost IF** the following
|
||
hold. These are acceptance criteria on every relevant point, not advice:
|
||
|
||
1. **Hot path — `peaks`** (`computeEnvelope` / `lastFrameAboveThreshold` over full PCM):
|
||
**NO virtual dispatch, NO `peaks` interface, NO added header→TU indirection.** Keep
|
||
`computeEnvelope` a **free function on `const std::vector<float>&`** so it inlines exactly
|
||
as today. Relocating the file + namespacing it is fine; wrapping it in an abstraction is
|
||
forbidden.
|
||
2. **Hot path — audition / preview:** splitting `panel_audition` into its own TU is fine —
|
||
but the call must stay a **direct call-through, not virtual**.
|
||
3. **Hot path — realtime-capture tick:** keep the idle fast-path a **single pointer test.**
|
||
The realtime lifecycle may move to its own TU (`realtime_lifecycle`) but the tick's
|
||
branch shape must not change.
|
||
4. **JSON extraction is OFF all hot paths** — serialization runs at save/load, never per
|
||
frame. Safe to abstract freely (this is *why* Q-W1 is the safe opener).
|
||
5. **`FxBypassGuard` runs per-capture, not per-frame** — keep it a **stack RAII** object when
|
||
it moves out of `main.cpp`; do not heap-allocate or virtualize it.
|
||
|
||
**Net (the audit's own conclusion):** every recommended split falls on a **cold path** or
|
||
**preserves call/inline shape** on the two hot ones. The acceptance bar for the phase:
|
||
*if a split would add an indirection on a path in the list above, it is out of scope —
|
||
rework the split to avoid it, or drop it.*
|
||
|
||
---
|
||
|
||
## 4. The GATE — Phase Q starts only when the tree is quiescent (load-bearing)
|
||
|
||
**Phase Q is gated on the completion of ALL other scheduled and in-flight work.** State this
|
||
prominently; it is the single most important sequencing fact in the phase.
|
||
|
||
**Why the gate exists — merge-conflict blast radius.** Phase Q's whole nature is that it
|
||
touches **nearly every file in `src/`** (relocating into subdirectories, changing the
|
||
namespace of every header, splitting the four largest TUs). Meanwhile:
|
||
|
||
- **Phase S** lives on the **phase-s worktree**, is **not on dev**, and is a large body of
|
||
work (a whole second VST3 build artifact + pure sampler core). Its branch is diffed against
|
||
the *current* flat layout.
|
||
- **Phase L** has **L3** (VST editor + embed-strip restyle, gated on Phase S landing on dev)
|
||
still to land — it touches the Phase S draw shells (`reasampler_editor` / `reasampler_embed`),
|
||
which arrive on dev with Phase S. (L1/L2/L4/L5/L6/L7 have **already landed** — see
|
||
`COMPLETED.md`; the once-listed "L2 pending" is stale and has been corrected here.)
|
||
- **D2** is **functionally complete** (D2-W1..W3-B landed; the only open item — a per-track
|
||
lane-split panel indicator — is *explicitly deferred*, not a blocking residual). **M9** (slots)
|
||
is **abandoned** (Daniel, 2026-07-27) — will not be built. D2 is named in the gate only so a
|
||
future reactivation of its deferred panel indicator re-arms the "tree must be quiescent"
|
||
condition; M9 is out and cannot re-arm it.
|
||
|
||
A structural reorg landing while any of these is mid-flight would force every in-flight
|
||
branch through a **rename-and-relocate-everything** merge — the worst possible conflict
|
||
class (every hunk moved, every namespace-qualified reference changed). The cost is not linear;
|
||
it is a combinatorial re-resolution of every open branch against a moved tree.
|
||
|
||
**The gate, stated as a rule (reconciled to reality, product-designer 2026-07-27):** Phase Q
|
||
does not begin until the tree is **quiescent**, with no large branch outstanding. As of
|
||
2026-07-27 the outstanding work is precisely:
|
||
|
||
1. **Phase S** — merged to dev (currently on the phase-s worktree; the large second-artifact
|
||
branch, the dominant gate item).
|
||
2. **Phase L L3** — merged to dev (the VST restyle; itself gated on Phase S, so it lands after
|
||
Phase S reaches dev). **L2 is already landed** — the earlier "L2 + L3" wording was stale.
|
||
3. **D2** — confirmed complete or its deferred indicator explicitly re-deferred. It is
|
||
functionally complete today; this line stays only so that if the deferred panel indicator is
|
||
picked up as active work, it re-arms the quiescence condition.
|
||
4. **M9** — **abandoned** (Daniel, 2026-07-27). This gate condition is permanently satisfied;
|
||
M9 will not be reactivated.
|
||
|
||
Restated as the plain readiness target Daniel named: **"when Phase S and L3 are finished."**
|
||
Phase Q is the *last* structural pillar precisely because it reshapes the ground every other
|
||
pillar stands on. Landing it early would tax every subsequent phase; landing it last taxes
|
||
nothing.
|
||
|
||
*(Sequencing corollary: because the gate is "everything else first," Phase Q's own internal
|
||
sequencing —§5— is about risk-ordering the reorg, not about racing other phases.)*
|
||
|
||
---
|
||
|
||
## 5. Sequencing shape — big-bang vs. incremental (feeds the waves)
|
||
|
||
**Settled shape: incremental, risk-ordered waves, each independently landable and
|
||
CTest-green.** A big-bang "rename everything in one commit" is rejected — it defeats the one
|
||
safety property the CMake seams give us (green CTest at every step) and produces an
|
||
un-reviewable diff. Instead, the reorg is decomposed so each wave is a safe, reviewable,
|
||
individually-revertible step:
|
||
|
||
- **W1 — the safe, high-leverage opener (zero-god-module-risk):** extract the pure `json`
|
||
core and delete the four duplicate `Parser`s, *plus* impose the `core/ shell/ app/`
|
||
directory layout and sub-namespaces on the modules that **don't** need splitting (the 30
|
||
clean pure libs + the clean shells). This is pure relocation + one genuine
|
||
encapsulation win, no god-module surgery. Lowest risk, highest legibility payoff, done
|
||
first.
|
||
- **W2–W5 — the four god-module splits, one per wave**, ordered by risk (the audit's own
|
||
S-leverage ranking): `bank_panel` (W2, biggest), `main.cpp` (W3), `actions.cpp` (W4,
|
||
includes deduping bank verbs against `bank_panel`), `persist.cpp` (W5, isolates the
|
||
single file-deletion authority). Each god-module split is its own wave because each is a
|
||
large, independently-reviewable change with its own verification surface.
|
||
- **W6 — the OCP registration-table** (close the ~350-line hand-written registration blocks)
|
||
and any fat-header (I) splits not already resolved. Sequenced last because it depends on
|
||
the `main.cpp` split (W3) having already isolated the registration code.
|
||
|
||
**Where the naming work (§2b) rides — renames follow relocations, no dedicated wave (Q-7).**
|
||
A rename is cheapest when the file is already moving or splitting, so naming does **not** get its
|
||
own wave; each fix rides the wave that already touches its file:
|
||
|
||
- **W1 absorbs** the collision fixes (§2b.2): the survivor `Parser` becomes `json::Parser` (or the
|
||
Q-8 `Reader`/`Writer` pair) as the four copies collapse; and every clean pure-UI type
|
||
(`FooterRect`/`ButtonRect`/`Selection`/`CellRect`) gets its `ui::` (etc.) home as the modules
|
||
relocate — retiring the `footer_bar.h` hand-collision "NAME NOTE." W1 already re-namespaces the
|
||
clean modules, so the sub-namespace half of every §2b fix lands here for free.
|
||
- **W1 also carries** any *pure-model* class rename Q-8 settles (e.g. `BankIndex`→a name matching
|
||
`bank_model`), because those modules relocate in W1 and a class rename is a mechanical
|
||
find-replace verified by the module's own test executable.
|
||
- **W2 absorbs** the `bank_panel`-side names as the god-module splits into `panel_*`.
|
||
- **W3 absorbs** the `capture`/`realtime` shell↔core word-order fix (Q-9) — the realtime lifecycle
|
||
is *already* being hoisted in W3, so aligning `capture_realtime`/`realtime_record` naming is a
|
||
rider on a move that is happening regardless.
|
||
|
||
The rule (Q-7): **no rename lands on a file that is not otherwise being touched by its wave.** A
|
||
rename that would force a file to move *only* to be renamed is deferred — the churn/legibility
|
||
trade isn't worth a standalone edit. This keeps the naming dimension inside the same
|
||
"green-CTest-at-every-point, minimal-diff-per-wave" discipline as the rest of Phase Q.
|
||
|
||
**Why incremental beats big-bang here, concretely:** the CMake per-module static-lib + per-
|
||
module test-executable structure means a file move + namespace change is *mechanically*
|
||
verifiable — `ctest` is green or it isn't, at every point. That property only pays off if the
|
||
reorg is *in points*. One giant commit throws the property away. Incremental is not just
|
||
safer; it is the only shape that uses the seams the architecture already gives us.
|
||
|
||
---
|
||
|
||
## 6. Forks — the decision record
|
||
|
||
**Fork Q-1 — namespace letter. SETTLED (this doc): `Q` (Quality).**
|
||
|
||
- **Settled:** the phase is namespaced **`Q` (Quality)**. M / D / B / R / V / S / L are all
|
||
taken (Milestone / Design / Bank / Reclaim / Version / Sampler / Look-and-feel).
|
||
- **Considered and set aside:** `O` (Organization) — rejected on two grounds: (1) the glyph
|
||
`O` reads ambiguously against zero in point ids (`O1`, `O10`), a real legibility cost in a
|
||
phase *about* legibility; (2) "Organization" undersells the charter — this phase is
|
||
measured against a **quality bar** Daniel set (Vital), and the reorg is the *means*, not
|
||
the end. `Q` names the end.
|
||
- **Reasoning:** `Q` is unambiguous, unused, and reads sensibly ("Phase Q — Quality"). The
|
||
point-id family is `Q1 … Qn` with wave prefixes `Q-W1 … Q-W6` matching the house
|
||
wave-naming (cf. `D2-W1`, `ps-w6`, `pL-w2`).
|
||
|
||
**Fork Q-2 — JSON extraction in scope? RECOMMEND: yes, and it is the W1 opener.**
|
||
|
||
- **Recommendation:** **in scope, and first.** The 4× duplicated `Parser` is the single
|
||
largest DRY+SRP violation (§2.1), it is entirely **off the hot paths** (§3.4, safe to
|
||
abstract freely), and it is the highest-leverage single change in the audit. Extracting a
|
||
pure `core/json` module and deleting the four copies is the ideal low-risk opener — it
|
||
proves the wave discipline (relocate + encapsulate, CTest-green) before any god-module
|
||
surgery.
|
||
- **Alternative considered:** defer JSON to a later, separate cleanup. Rejected — it is the
|
||
cheapest, safest, highest-payoff move; deferring it wastes the opener slot on pure
|
||
relocation with no encapsulation win.
|
||
- **Watch (the audit flags it):** guard the `Parser` name-unification against cross-lib
|
||
collisions when the four copies merge into one; and mind `Sample` vs `AudioSample` when
|
||
sub-namespacing (§Q-5 collision note).
|
||
|
||
**Fork Q-3 — directory naming: `core/ shell/ app/` vs a Vital-style subsystem-first tree.
|
||
RECOMMEND: `core/ shell/ app/` as the top split, subsystem dirs beneath `core/`.**
|
||
|
||
- **Recommendation:** top-level by the **load-bearing discipline** (`core/` = pure/testable,
|
||
`shell/` = REAPER-facing, `app/` = the entry TU), then **subsystem dirs beneath** — the
|
||
audit's proposed map: `core/model/ core/view/ core/capture/ core/audio/ core/ui/
|
||
core/reclaim/ core/version/ core/json/`, and `shell/capture/ shell/panel/ shell/view/
|
||
shell/persist/ shell/actions/`, with `app/main.cpp`.
|
||
- **Why this over pure-Vital (subsystem-first, e.g. `model/ capture/ ui/` at top):** the
|
||
**pure/shell split is ReaSampler's most load-bearing invariant** — it is CMake-enforced and
|
||
it is the thing that keeps the core unit-testable outside the DAW. Making it the *top* level
|
||
of the directory tree makes the invariant **structurally visible and hard to violate** (a
|
||
file's directory tells you instantly whether it may touch a REAPER type). Vital has no
|
||
equivalent pure/host split to protect, so its subsystem-first tree is right *for Vital*;
|
||
`core/ shell/` is the ReaSampler-native reading of the same "directory = architecture"
|
||
lesson. Subsystem grouping still happens — one level down — so we get both readings.
|
||
- **Alternative:** flat subsystem-first (`model/ view/ capture/ audio/ ui/ …`) with the
|
||
pure/shell distinction living only in namespaces. Rejected: it demotes the most important
|
||
invariant from *structure* to *convention*, which is exactly the drift Phase Q exists to
|
||
reverse.
|
||
|
||
**Fork Q-4 — sub-namespace, or sub-directory only? RECOMMEND: both — sub-namespace to match
|
||
the sub-directory.**
|
||
|
||
- **Recommendation:** granular sub-namespaces mirroring the directories:
|
||
`reasampler::model`, `reasampler::view`, `reasampler::capture`, `reasampler::audio`,
|
||
`reasampler::ui`, `reasampler::reclaim`, `reasampler::version`, `reasampler::json`. Directory
|
||
and namespace agree, so a symbol's home is unambiguous from either.
|
||
- **Why:** namespaces are the *code-visible* half of the "make the architecture legible" goal
|
||
(directories are the filesystem half). Sub-dirs without sub-namespaces leaves every symbol
|
||
still in one flat `reasampler::` soup — half the win. Vital's own headers group by concern;
|
||
matching namespace-to-directory is the standard C++ reading of that.
|
||
- **Cost (name it):** sub-namespacing is the change with the **widest edit surface** — every
|
||
qualified reference across TUs updates. This is precisely why the GATE (§4) matters and why
|
||
W1 does the namespace move on the *clean* modules first (mechanical, no logic change), with
|
||
the god-module splits (W2–W5) namespacing their own new TUs as they land.
|
||
- **Collision watch (audit):** `Sample` (bank_model) vs `AudioSample` (peaks alias) and the
|
||
unified `Parser` must not collide once sub-namespaced — resolve by their new
|
||
`model::` / `audio::` / `json::` homes.
|
||
|
||
**Fork Q-5 — how aggressive the god-module split? RECOMMEND: split to the audit's named seams,
|
||
no finer.**
|
||
|
||
- **Recommendation:** split each god-module along the **exact seams the audit names** and
|
||
stop there:
|
||
- `bank_panel` → `panel_render` / `panel_thumbnails` / `panel_audition` / `panel_input` /
|
||
`panel_bank_ops` / `panel_window`.
|
||
- `main.cpp` → hoist `capture_orchestrator` / `scope_resolve` / `realtime_lifecycle`,
|
||
leaving `main` = API pointers + entry + dispatch.
|
||
- `actions.cpp` → `design_view_actions` / `bank_actions` / `prune_action`, deduping the
|
||
bank verbs against `bank_panel`'s `panel_bank_ops`.
|
||
- `persist.cpp` → `session` / `ext_state_io` / `prune_fs` (**isolating the single
|
||
file-deletion authority** into `prune_fs`).
|
||
- **Why stop there:** the seams are already validated by the audit and correspond to real
|
||
responsibilities. Splitting *finer* (one file per function) would trade a god-module for a
|
||
fragmentation problem — the opposite failure. "Something Daniel can stand to look at" is
|
||
well-factored, not atomized.
|
||
- **Alternative (lighter):** split only the two worst (`bank_panel`, `main.cpp`), leave
|
||
`actions`/`persist` as-is. Rejected: `persist`'s buried file-deletion authority and
|
||
`actions`'s duplicated bank verbs are real hazards worth resolving while the tree is open;
|
||
doing them now (behind the gate) is cheaper than a second reorg later.
|
||
|
||
**Fork Q-6 — the OCP registration-table (W6): in scope or deferred? RECOMMEND: in scope, last.**
|
||
|
||
- **Recommendation:** **in scope as the final wave.** The ~350-line hand-written registration
|
||
blocks are a genuine OCP wart (every new action edits 4 parallel places); a registration
|
||
table closes it. Sequenced **last** because it depends on the `main.cpp` split (W3) having
|
||
isolated the registration code — you can't table-ify code you haven't first extracted.
|
||
- **Alternative:** drop it — it's OCP, not the SRP focus of the phase. A reasonable trim if
|
||
Daniel wants Phase Q strictly scoped to the reorg. Kept in with a leading recommendation
|
||
because, once W3 has hoisted the registration code, tabling it is a small, high-legibility
|
||
finish — but it is the most droppable point if the phase needs narrowing.
|
||
|
||
**Fork Q-7 — is naming its own wave, or does it ride the relocation waves? RECOMMEND: rides the
|
||
waves; no dedicated naming wave.** SETTLED-by-structure once Q-3/Q-4 are settled.
|
||
|
||
- **Recommendation:** naming fixes ride the wave that already relocates or splits the file (§5),
|
||
under the rule *no rename lands on a file the wave isn't otherwise touching.* A rename is nearly
|
||
free during a relocation (the file is open, the diff is already large, the module's own test
|
||
executable verifies it) and near-pure-churn as a standalone edit. Because Q-3 (directories) and
|
||
Q-4 (sub-namespaces) already move and re-namespace every file, the *collision* half of the
|
||
naming audit (§2b.2) is resolved by the namespace split with zero extra renames — the sub-
|
||
namespace *is* the fix. Only the genuine *class/module renames* (Q-8/Q-9) add symbol churn, and
|
||
those are scoped to files already in motion.
|
||
- **Alternative considered:** a dedicated final "naming pass" wave (Q-W7). Rejected: it would
|
||
re-open files W1–W6 just closed, producing exactly the churn-without-relocation the rule forbids,
|
||
and a diff that touches everything again defeats the per-wave reviewability property.
|
||
- **This makes Q-7 not really a judgment call once Q-3/Q-4 are settled** — it is the forced
|
||
consequence of "renames are cheapest during relocation." Recorded as a fork only because Daniel
|
||
might still want naming called out as a first-class deliverable rather than folded silently into
|
||
the reorg waves; if so, the plan *names* the riders per wave (it does, §5) without adding a wave.
|
||
|
||
**Fork Q-8 — how far to push *class/module* renames (beyond the free namespace fix)? RECOMMEND:
|
||
fix the two that actively mislead; leave the merely-quirky.** Daniel's to call.
|
||
|
||
- **Recommendation (leading):** rename only where a name *actively misleads* a reader, and stop:
|
||
- **`BankIndex` → a name matching `bank_model`** (§2b.3.1) — the file/class word-mismatch is the
|
||
worst offender (`bank_model.h` owns `class BankIndex`; the two share no word). Two shapes:
|
||
(a) rename the class to `BankModel` (file = class, matches `ViewModeModel`/`BankBook`); or
|
||
(b) rename the *file* to `bank_index.{h,cpp}` (class stays `BankIndex`). **Prefer (a)** — it
|
||
makes the model family read on one principle (`BankModel`/`BankBook`/`ViewModeModel`, all
|
||
`<noun>Model`/`<noun>Book`), and it is a class rename W1 verifies via `bank_model_tests`.
|
||
- **The unified JSON `Parser`** (§2b.2.1) — make it a `json::Reader` + `json::Writer` pair (or
|
||
keep `json::Parser` if extraction stays parse-only). This is a *new* module's naming, decided
|
||
at W1 mint time, so it costs nothing to get right.
|
||
- **Leave quirky-but-harmless:** `Book`→`Bank`→`Index` containment (§2b.3.2 — evocative, learnable),
|
||
`Sample`/`AudioSample` (§2b.2.3 — the namespace split already de-collides them; renaming
|
||
`AudioSample`→`Pcm`/`PcmSample` is optional polish), `guid_diff`/`GuidBaseline`, `MinMax`,
|
||
`KitBox`. Renaming these is pure taste with no misleading-a-reader payoff — the "stand to look
|
||
at" bar is met by the namespace homes alone.
|
||
- **Alternative (more aggressive):** normalize the *entire* model family to one suffix
|
||
(`BankModel`/`BankBookModel`/`ViewModeModel`/`OwnedManifestModel`) and rename `AudioSample`→
|
||
`PcmSample`. Rejected as the lead because it renames things that already read fine, adding symbol
|
||
churn (every call site, every test) for marginal legibility — but it is a coherent option if
|
||
Daniel wants the model family *rigidly* uniform. Kept on the table as Daniel's call.
|
||
- **Alternative (minimal):** do zero class renames; let the sub-namespaces (Q-4) carry the whole
|
||
naming win. Defensible — it is the lowest-churn, lowest-risk reading, and the namespace split
|
||
genuinely resolves every *collision*. Rejected as the lead only because `bank_model`/`BankIndex`
|
||
is a standing "what is this file" cost the reorg is uniquely cheap to fix.
|
||
|
||
**Fork Q-9 — align the `capture_realtime` / `realtime_record` shell↔core word order? RECOMMEND:
|
||
yes, during W3.** Daniel's to call.
|
||
|
||
- **Recommendation:** align the pair to the house shell↔core convention (`drag_out` ↔
|
||
`drag_out_win`: shared stem, suffix marks the shell). The pure planner `realtime_record` and its
|
||
shell `capture_realtime` invert word order for one feature — the tree's clearest shell/core drift
|
||
(§2b.3.3). W3 is *already* hoisting the realtime lifecycle, so aligning the names is a rider on a
|
||
move that happens anyway. Two shapes: (a) core `capture_realtime` / shell `capture_realtime_shell`
|
||
(stem = `capture_realtime`, matches the `capture` offline pair); (b) core `realtime_record` /
|
||
shell `realtime_record_shell`. **Prefer (a)** — it nests the realtime naming under `capture_*`
|
||
alongside the offline path, so the whole capture subsystem reads on one stem.
|
||
- **Alternative:** leave it — the inversion is cosmetic and both names are individually clear.
|
||
Reasonable if Daniel wants W3 kept strictly to the god-module split with no adjacent renames. The
|
||
fix is cheap enough (W3 touches these files regardless) that the lead is to take it, but it is
|
||
the most droppable of the three naming forks.
|
||
|
||
---
|
||
|
||
## 7. What Phase Q does NOT change (guardrails)
|
||
|
||
- **The pure-core / shell split is strengthened, never dissolved.** The whole point is to
|
||
make it *more* legible (top-level `core/` vs `shell/`). No file moves across the boundary;
|
||
no core file gains a REAPER type; the CMake per-module test executables stay green.
|
||
- **Every precision invariant holds.** Null-test, bit-identical repeats, non-destructive
|
||
capture, exact bounds, relative-paths-only — none is code Phase Q rewrites. `FxBypassGuard`
|
||
(the precision-critical guard) *moves* out of `main.cpp` but stays a stack RAII object with
|
||
identical behavior (§3.5).
|
||
- **Capture ≠ placement.** No `Run*` / capture-orchestrator path may gain an `InsertMedia`
|
||
call during the split. The load-bearing rule is invariant under reorganization.
|
||
- **The single file-deletion authority stays one obvious module.** `persist`'s `prune_fs`
|
||
split *concentrates* the deletion authority (`SHFileOperationW`) into one named module —
|
||
it must not spread it. This is a safety property the reorg improves, never dilutes.
|
||
- **Relative-paths-only in the persisted index** — untouched (a data invariant, not a
|
||
structural one).
|
||
- **Performance** — §3 is a hard acceptance bar: no hot-path indirection, ever.
|
||
- **No behavior change.** Phase Q is a pure structural refactor. If a point changes observable
|
||
behavior, it has exceeded its charter and must be reworked. The test suite passing
|
||
unchanged **is** the proof of correctness (green CTest at every point).
|
||
|
||
---
|
||
|
||
## 8. Sources
|
||
|
||
- Vital source structure — `github.com/mtytel/vital`, `src/` subsystem tree
|
||
(`common/ synthesis/ interface/ plugin/ headless/ standalone/`) and the nested
|
||
`synthesis/` functional sub-dirs (`synth_engine/ modulators/ filters/ effects/ producers/
|
||
framework/ lookups/ utilities/`). Read from the repository tree, 2026-07-26. Vital is
|
||
GPLv3 — the borrowed artifact is the **structural pattern**, not code.
|
||
- The SOLID-compliance audit (§2) — grep-verified staff-engineer analysis of the actual
|
||
ReaSampler `src/` tree (45 files / ~19,800 LOC), 2026-07-26. The evidence base for every
|
||
reorg move and the performance guardrails (§3).
|