Decouple the instrument reload from VST3 activation, and make the master meter's accumulate exact
This commit is contained in:
@@ -6,6 +6,24 @@ original Goal, Verify, and checklist points with boxes marked done.
|
||||
This file holds the current (1.x) cycle's landed milestones only. For all
|
||||
pre-1.0 (version-0) history, see `docs/ARCHIVE.md`.
|
||||
|
||||
### Decouple the instrument reload from VST3 activation (filed follow-up, discharged in Γ-W3)
|
||||
|
||||
`ReaSamplerProcessor::setActive` meant two things at once — "the audio thread may run" and "the
|
||||
decoded `SampleData` is (re)built" — so every host-driven activation cycle paid a bridge read
|
||||
and a full WAV decode that nothing about activation required. The two lifetimes are now
|
||||
separate: `setActive(false)` parks the decoded sample and destroys the voice state,
|
||||
`setActive(true)` rebuilds the voices around the parked sample through the drain-slot swap
|
||||
`rebuildVoiceEngine` already used for voice-count edits. A cycle costs no disk I/O and no
|
||||
decode; sounding voices are still destroyed across it (a surviving `live_` would be displaced
|
||||
into the drain slot and resurrect stale sustained voices as ghosts); an instance with nothing
|
||||
decoded still routes through the full reload, which is where the pre-v10 legacy lift lives; and
|
||||
`getLatencySamples()` still answers from the persisted enable, untouched by the cycle. The
|
||||
build shared by the reload, the voice-param rebuild and the reactivation was factored to one
|
||||
site so the three cannot drift on the generation stamp or the ring size. Daniel reversed the
|
||||
deferral (*"I thought we agreed to decouple the unnecessary functions from the reactivation
|
||||
path"*); Γ-F2 and Γ-F6 are untouched — dynamic latency ships, the deactivate/reactivate is
|
||||
still the accepted cost of the toggle, just a much cheaper one.
|
||||
|
||||
### Comment-reduction pass (tree-wide, twelve parallel tracks)
|
||||
|
||||
Cut source comment volume tree-wide: 209 files changed, net **−6,493** lines.
|
||||
|
||||
+22
-46
@@ -240,57 +240,33 @@ alpha and this entry is re-filed against the new value.
|
||||
|
||||
**Nothing here is actionable as a TODO.** Delete this entry when Γ-W1-T1 lands.
|
||||
|
||||
## Decouple the instrument reload from VST3 activation
|
||||
## The editor's drag state machine has no seam, and `reasampler_editor.h` is near the ceiling
|
||||
|
||||
**Context (Daniel, 2026-08-01 — Phase Γ fork Γ-F6, ruled closed).** Γ-W1-T2 ships the plugin's
|
||||
first latency reporting: `getLatencySamples()` returns 0 with the limiter off and the lookahead
|
||||
with it on, and the toggle calls `IComponentHandler::restartComponent(kLatencyChanged)`. The
|
||||
vendored SDK defines that flag as a host **deactivate/reactivate**
|
||||
(`pluginterfaces/vst/ivsteditcontroller.h:105-108`). **Dynamic latency reporting is routine for
|
||||
VST3 instruments and REAPER handles it as a matter of course** — the deactivate/reactivate is
|
||||
the normal contract, and for a typical plugin `setActive` only allocates and frees buffers.
|
||||
Γ-F6 was originally posed as "is this SDK cost acceptable?"; Daniel's answer relocated it:
|
||||
*"you have to have missed something, I used plenty of VST3s inside of REAPER that report PDC
|
||||
dynamically... Toggling the limiter killing the voices isn't a deal breaker though, the limiter
|
||||
will either be on or off on its instance, toggling during playback is not a use case."*
|
||||
**Context (Γ-W3, meter re-review).** `reasampler_editor.h` stands at **563 lines** against the
|
||||
~600-line ceiling — 37 lines of margin — and it keeps growing because every new surface on the
|
||||
Sample face adds its transient state there. The obvious seam is the drag state machine: `drag_`
|
||||
plus the per-gesture anchors it is read against.
|
||||
|
||||
**The wart — and it is ours, not the SDK's.** `ReaSamplerProcessor::setActive(true)` calls
|
||||
`reloadInstrument()` (`src/shell/instrument/reasampler_processor.cpp:89-97`) — a bridge read
|
||||
plus a **full WAV re-decode** plus a fresh engine. `setActive(false)` frees `live_`,
|
||||
`draining_` and the graveyard (`:98-107`). So every host-driven activation cycle — a
|
||||
latency-change restart, an offline-render bracket, any host that deactivates around transport
|
||||
state — pays a disk read and a decode that nothing about activation requires. **Activation
|
||||
currently means two things at once**: "the audio thread may run" and "the decoded `SampleData`
|
||||
is (re)built." Dynamic latency is simply the first feature that makes the cycle
|
||||
user-triggerable.
|
||||
**Why it was declined rather than taken.** `drag_` has **42 references across 13 shell TUs**
|
||||
(measured over `src/shell/instrument/*.cpp`; the declaration in the header is additional), and
|
||||
every input TU both writes it and branches on it. Extracting it is a real refactor of the
|
||||
editor's input half, not a header move — and doing it inside a wave whose subject is the MASTER
|
||||
deck would have put an unrelated high-blast-radius change in the same diff. Declining was right;
|
||||
leaving it unrecorded was not.
|
||||
|
||||
**Intended fix.** Separate the two lifetimes: keep the decoded `SampleData` alive across a
|
||||
deactivate and rebuild only the voice state on reactivate. The mechanism already exists in this
|
||||
file — `rebuildVoiceEngine` performs exactly that shape (drain-slot swap around the
|
||||
already-decoded `SampleData`, no bank re-read, no WAV re-decode) for voice-count and voice-mode
|
||||
edits. This is a lifetime split, not a new mechanism.
|
||||
**The shape a fix would take.** A `DragState` type owning the kind plus its anchor payload,
|
||||
with the input TUs mutating it through named transitions rather than assigning `drag_` and its
|
||||
anchors independently — which is also what would let the invariant "an anchor is only readable
|
||||
while its own `DragKind` is in flight" be enforced rather than observed. `editor_interaction.h`
|
||||
already holds the `DragKind` vocabulary and is the natural home.
|
||||
|
||||
**The constraint the fix MUST handle.** The deactivate's destruction is deliberate and its
|
||||
reason is documented at the call site: a surviving `live_` would be displaced into the drain
|
||||
slot on reactivate and *"resurrect stale sustained voices as ghosts."* **Voice state must still
|
||||
die across the cycle** — only the decoded PCM survives, and those are two different lifetimes
|
||||
currently collapsed into one. Second constraint: `setActive(true)` is also the non-editor
|
||||
legacy-lift trigger for a pre-v10 blob (its opportunistic `refreshRefsFromBank` copies refs in
|
||||
once the bank blob is readable), so a path that skips the bridge read must keep that lift
|
||||
reachable — the comment at `:90-96` records the residual load-order race it exists to cover.
|
||||
**Priority / risk.** Low, but the margin is the clock: the next surface that adds two members to
|
||||
the header takes it over the ceiling, and at that point the seam gets chosen under time pressure
|
||||
by whoever is unlucky. Take it before that, not after.
|
||||
|
||||
**Priority / risk.** Low; deferred by ruling. Nothing is incorrect today, only wasteful, and
|
||||
Daniel has explicitly accepted the user-visible consequence (held notes cut on a limiter
|
||||
toggle). **Trigger conditions — revisit when any one of these holds:** (a) a second
|
||||
latency-changing control appears, so the cycle stops being a once-per-patch event; (b) the
|
||||
limiter enable is ever wanted automatable, which `docs/product/parameter-automation.md` §3.8
|
||||
currently forbids *because* of this cost; or (c) the re-decode is observed to be perceptible in
|
||||
REAPER — Γ-W1-T2's review records that observation for exactly this purpose.
|
||||
|
||||
**Done looks like.** A host-driven deactivate/reactivate cycle costs no disk I/O and no WAV
|
||||
decode; sounding voices are still destroyed across it, with no ghost-resurrection regression;
|
||||
a pre-v10 blob still lifts; and `getLatencySamples()` still derives from persisted state rather
|
||||
than from a transient the deactivate cleared.
|
||||
**Done looks like.** `reasampler_editor.h` is back under the ceiling with room; no TU assigns
|
||||
`drag_` and an anchor as two independent writes; and the transitions are named where the
|
||||
`DragKind` catalogue already lives.
|
||||
|
||||
## `Sample::sourceMode` has no value meaning "produced by the instrument"
|
||||
|
||||
|
||||
@@ -50,9 +50,8 @@ own width formula, not carried over from a prior measurement. The stale geometry
|
||||
when off, the lookahead when on, reported to the host's PDC. This is **routine VST3
|
||||
behaviour**; the `restartComponent(kLatencyChanged)` it costs is the normal contract, and
|
||||
the deactivate/reactivate the flag mandates is **accepted** — the toggle is a patch-design
|
||||
gesture. The only reason the cycle is expensive at all is that **our** `setActive` re-decodes
|
||||
the WAV, which is a latent improvement filed in `docs/TODO.md`, not a design constraint.
|
||||
§3.1.1.
|
||||
gesture. The cycle used to be expensive only because **our** `setActive` re-decoded the WAV;
|
||||
Γ-W3 decoupled the two lifetimes, so it no longer does. §3.1.1.
|
||||
- **The cortex limiter does not clear the bar** — §3.5. Read it, take nothing.
|
||||
- **Loop gets an explicit enable on the chrome row** (Γ-F4), and the four-mark grammar
|
||||
sits under it. The core finding behind the re-approach: three identical bars draw a
|
||||
@@ -560,31 +559,28 @@ plugins — lookahead limiters, linear-phase EQs and oversampling processors all
|
||||
REAPER handles it as a matter of course. The deactivate/reactivate is the *normal* cost of
|
||||
the flag, and for a typical plugin it is cheap: `setActive` allocates and frees buffers.
|
||||
|
||||
**What makes it expensive here is entirely our own design, in one line.**
|
||||
`ReaSamplerProcessor::setActive` is deliberately destructive in both directions
|
||||
(`reasampler_processor.cpp:85-109`):
|
||||
**What made it expensive here was entirely our own design, in one line** — and Γ-W3 removed
|
||||
that line. `ReaSamplerProcessor::setActive` was deliberately destructive in both directions:
|
||||
|
||||
- `setActive(true)` calls `reloadInstrument()` (`:89-97`) — **a bridge read and a full WAV
|
||||
re-decode**, plus a fresh engine. This is the expensive half, and no part of it is required
|
||||
by the SDK: it is there because activation was the convenient trigger for a reload, not
|
||||
because activation implies one.
|
||||
- `setActive(false)` frees `live_`, `draining_` **and** the graveyard (`:98-107`), so every
|
||||
sounding voice dies. The comment there explains why that is correct and must not be
|
||||
softened casually: a surviving `live_` would be displaced into the drain slot on reactivate
|
||||
and *"resurrect stale sustained voices as ghosts."*
|
||||
- `setActive(true)` called `reloadInstrument()` — **a bridge read and a full WAV re-decode**,
|
||||
plus a fresh engine. That was the expensive half, and no part of it was required by the SDK:
|
||||
it was there because activation was the convenient trigger for a reload, not because
|
||||
activation implies one.
|
||||
- `setActive(false)` frees `live_`, `draining_` **and** the graveyard, so every sounding voice
|
||||
dies. That half is correct and must not be softened casually: a surviving `live_` would be
|
||||
displaced into the drain slot on reactivate and *"resurrect stale sustained voices as
|
||||
ghosts."*
|
||||
|
||||
**So the cost is ours, and it is ours to reduce.** The reduction is **decoupling the reload
|
||||
from activation** — keeping the decoded `SampleData` alive across a deactivate while still
|
||||
destroying voice state, which is exactly the shape `rebuildVoiceEngine`'s drain-slot swap
|
||||
already implements for voice-count edits. **That is a latent improvement with a clear trigger
|
||||
condition, filed in `docs/TODO.md` ("Decouple the instrument reload from VST3 activation") —
|
||||
not a reason to abandon dynamic latency, and not scheduled in this phase.**
|
||||
**The cost was ours, and it has been reduced (Γ-W3 — see §7.11).** The deactivate now parks the
|
||||
decoded `SampleData` and the reactivate rebuilds only the voice state around it, through the
|
||||
same drain-slot swap `rebuildVoiceEngine` uses for voice-count edits. An activation cycle costs
|
||||
no disk read and no decode; an instance with nothing decoded still takes the full reload, which
|
||||
is where the pre-v10 legacy lift lives.
|
||||
|
||||
**The honest cost of the toggle today, stated plainly:** every sounding note stops and the
|
||||
sample is re-decoded from disk. **Daniel has accepted it** (Γ-F6): *"Toggling the limiter
|
||||
killing the voices isn't a deal breaker though, the limiter will either be on or off on its
|
||||
instance, toggling during playback is not a use case."* There is no fallback design and no
|
||||
measurement gate.
|
||||
**The honest cost of the toggle, stated plainly:** every sounding note stops. **Daniel has
|
||||
accepted it** (Γ-F6): *"Toggling the limiter killing the voices isn't a deal breaker though,
|
||||
the limiter will either be on or off on its instance, toggling during playback is not a use
|
||||
case."* There is no fallback design and no measurement gate.
|
||||
|
||||
#### The standing scar, and why this is nonetheless not the forbidden change
|
||||
|
||||
@@ -660,10 +656,9 @@ What is in scope alongside it — and what each is actually for:
|
||||
in the **not-automatable** class, and it is emphatically not the plugin's `kIsBypass`
|
||||
parameter either.
|
||||
- **Observe what REAPER does, and record it — as evidence, not as a gate.** Whether notes
|
||||
cut, whether the re-decode is perceptible, whether transport hiccups, is DAW-observable
|
||||
only. Record it in Γ-W1-T2's review because it is the trigger-condition evidence for the
|
||||
`docs/TODO.md` decoupling entry. **No outcome changes the design**; Γ-F6 is closed either
|
||||
way.
|
||||
cut and whether transport hiccups is DAW-observable only. The re-decode half of that
|
||||
question is gone (§7.11), so what remains to observe is the voice cut alone. **No outcome
|
||||
changes the design**; Γ-F6 is closed either way.
|
||||
|
||||
### 3.2 The meter
|
||||
|
||||
@@ -1401,13 +1396,18 @@ squarely on `ReaSamplerProcessor::setActive`, which is deliberately destructive
|
||||
directions. **Those four are hygiene against the `kIoChanged` scar (§3.1.1), not a hedge
|
||||
against the flag itself** — Γ-F6 is ruled and the restart ships.
|
||||
|
||||
**7.11 — `setActive` conflates two lifetimes, and dynamic latency is the first feature that
|
||||
makes a user notice.** Activation currently means both "the audio thread may run" and "the
|
||||
decoded `SampleData` is (re)built" (`reasampler_processor.cpp:89-97`). Phase Γ does **not**
|
||||
separate them — Γ-F6 accepts the cost — but the conflation is now a named, filed improvement
|
||||
(`docs/TODO.md`, "Decouple the instrument reload from VST3 activation") rather than an
|
||||
unremarked property. **Do not restructure `setActive` inside this phase**; its destructive
|
||||
shape is deliberate and its reasoning is documented at the call site.
|
||||
**7.11 — `setActive` conflated two lifetimes; it no longer does (LANDED, Γ-W3).** Activation
|
||||
used to mean both "the audio thread may run" and "the decoded `SampleData` is (re)built", so
|
||||
every host-driven cycle paid a bridge read and a full WAV decode. The two are now separate:
|
||||
`setActive(false)` parks the decoded sample and destroys the voice state, `setActive(true)`
|
||||
rebuilds the voices around the parked sample through the drain-slot swap `rebuildVoiceEngine`
|
||||
already used. **This section's earlier instruction — "do not restructure `setActive` inside
|
||||
this phase" — was superseded by Daniel's ruling that this track does it**; the deactivate's
|
||||
destruction of voice state is still deliberate (a surviving `live_` would resurrect stale
|
||||
sustained voices as ghosts) and only the PCM survives. Nothing parked routes the activation
|
||||
back through the full reload, which is what keeps the pre-v10 legacy lift reachable. Γ-F6 is
|
||||
untouched: dynamic latency ships and the deactivate/reactivate is still the accepted cost —
|
||||
it is simply a much cheaper one.
|
||||
|
||||
---
|
||||
|
||||
@@ -1427,7 +1427,7 @@ ceiling.
|
||||
| **Γ-F3** | Does the log taper raise the 2 s stage-time ceiling? | **REVERSED, same day. Ruled first "not in this phase — stays 2.0 s"; then Daniel: _"extend the stage lengths to 10s."_ The ceiling moves 2.0 → 10.0 in Γ-W1-T1.** The reversal's cause is Ruling 1: parameters now ship in-phase, so the ceiling is a one-way door that has to be walked through *before* them. | **§4.3.1** (new), §4.3; `docs/TODO.md` entry discharged |
|
||||
| **Γ-F4** | Explicit loop enable? | **Yes — on the CHROME ROW.** Not a deck cell; loop is a waveform-overlay concept and has no deck. | **§6.4** (new), §6.5, §7.9 |
|
||||
| **Γ-F5** | MASTER's reserved slot: one cell or two? | **One cell.** Two would spend 60 of the 82 px headroom on an unnamed control and freeze row 1 forever. | **§1.6** (new), §1.4 |
|
||||
| **Γ-F6** | Is the `kLatencyChanged` deactivate/reactivate acceptable as the cost of the toggle? | **Yes — ship dynamic latency as ruled.** No constant-latency fallback, no measurement gate. *Corrected this doc's analysis: the cost is self-inflicted, not SDK-imposed.* | **§3.1.1** (rewritten), §7.10, §7.11, `docs/TODO.md` |
|
||||
| **Γ-F6** | Is the `kLatencyChanged` deactivate/reactivate acceptable as the cost of the toggle? | **Yes — ship dynamic latency as ruled.** No constant-latency fallback, no measurement gate. *Corrected this doc's analysis: the cost is self-inflicted, not SDK-imposed.* | **§3.1.1** (rewritten), §7.10, §7.11; `docs/TODO.md` decoupling entry discharged in Γ-W3 |
|
||||
| **Γ-F7** | VST3 parameter ORDER: signal flow, or the editor's visual rows? | **Signal flow** — *"signal flow order."* The frozen id numbering and the presentation index both follow the deck's own rule; the visual layout is too mobile to freeze against. | **§8.3**; `parameter-automation.md` §6.4 (argument) and §6.2 (the 44-id table) |
|
||||
|
||||
Three of these corrected this doc rather than confirming it, and all three corrections are
|
||||
@@ -1473,7 +1473,8 @@ reintroduced:
|
||||
than just counting:
|
||||
|
||||
1. **§3.1.1 was rewritten, not annotated.** Its prior framing — dynamic latency as exotic and
|
||||
expensive — was wrong. Dynamic PDC is routine; the expense is our reload-on-activate.
|
||||
expensive — was wrong. Dynamic PDC is routine; the expense was our reload-on-activate, and
|
||||
Γ-W3 removed it (§7.11).
|
||||
2. **The measurement gate was dropped.** Γ-W1-T2's first deliverable is the limiter, not a
|
||||
spike. What remains is an *observation* recorded in review as evidence for the deferred
|
||||
improvement — it gates nothing.
|
||||
|
||||
@@ -202,8 +202,8 @@ The consequence for this doc is concrete and it is a **subtraction from the para
|
||||
> latency, and the vendored SDK defines `restartComponent(kLatencyChanged)` as *"the host
|
||||
> has to deactivate and reactivate the plug-in"*
|
||||
> (`pluginterfaces/vst/ivsteditcontroller.h:105-108`). In this plugin a deactivate frees
|
||||
> every sounding voice and a reactivate re-decodes the WAV. **An automation lane toggling
|
||||
> that parameter would deactivate the plugin on every flip.**
|
||||
> every sounding voice. **An automation lane toggling that parameter would deactivate the
|
||||
> plugin on every flip.**
|
||||
|
||||
Two corollaries the parameter work must carry rather than rediscover:
|
||||
|
||||
@@ -211,7 +211,7 @@ Two corollaries the parameter work must carry rather than rediscover:
|
||||
binding it to `kIsBypass` would hand the host a control that restarts the component.
|
||||
- **Latency reporting must be derived from persisted state, not from a transient.** The SDK
|
||||
states the new latency is what `getLatencySamples` returns *after* `setActive(true)` — and
|
||||
this plugin's `setActive(false)` frees essentially everything. Whatever holds the limiter
|
||||
this plugin's `setActive(false)` destroys the whole voice state. Whatever holds the limiter
|
||||
flag must survive that cycle.
|
||||
|
||||
Full reasoning, the SDK quotes, and the required verification steps are in
|
||||
@@ -221,12 +221,13 @@ There is no constant-reported-latency fallback — that option is closed, not sh
|
||||
**this section does not shrink to a footnote and the limiter enable does not become
|
||||
automatable.** Plan against the not-automatable classification; it is settled.
|
||||
|
||||
**One future condition could reopen it, and it is worth knowing about.** The restart is only
|
||||
expensive because *this plugin's* `setActive(true)` re-decodes the WAV — not because the SDK
|
||||
requires it. `docs/TODO.md` ("Decouple the instrument reload from VST3 activation") files that
|
||||
reduction, and **"the limiter enable is wanted automatable" is one of its named trigger
|
||||
conditions.** If the parameter work genuinely needs that lane, the answer is to do the
|
||||
decoupling first, not to re-litigate the classification.
|
||||
**The decoupling that was filed against this section has LANDED (Γ-W3), and it changes the
|
||||
cost but not the classification.** `setActive(true)` no longer re-decodes the WAV: the decoded
|
||||
sample now survives a deactivate and only the voice state is rebuilt
|
||||
(`instrument-control-surface.md` §7.11). So a flip costs a voice rebuild rather than a disk
|
||||
read plus a decode — but **the deactivate still frees every sounding voice**, which is the
|
||||
ground the not-automatable classification actually rests on. Plan against not-automatable; if
|
||||
the parameter work wants that lane, the question to answer is the voice cut, not the decode.
|
||||
|
||||
---
|
||||
|
||||
|
||||
Reference in New Issue
Block a user