From 13c824c63d715dc01cafc5492907850d85214804 Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Wed, 5 Aug 2026 20:40:45 -0400 Subject: [PATCH] Collapse the FX-park drain's writes into one undo point instead of one per TrackFX_SetOffline --- docs/VERIFICATION.md | 4 ++ src/shell/view/CLAUDE.md | 36 +++++++---- src/shell/view/view_fx_park.cpp | 104 ++++++++++++++++++++++++-------- src/shell/view/view_fx_park.h | 29 +++++++++ tests/test_view_fx_park.cpp | 32 +++++++++- 5 files changed, 164 insertions(+), 41 deletions(-) diff --git a/docs/VERIFICATION.md b/docs/VERIFICATION.md index d3ad8ad..02c4fe0 100644 --- a/docs/VERIFICATION.md +++ b/docs/VERIFICATION.md @@ -48,6 +48,10 @@ Checks for Θ, Ξ, Ψ, Ε, Ρ, Γ, and Ω work that no unit test can close. Buil - [ ] Attempt a mode switch while the transport is playing, then while recording — both refuse, visibly (`docs/COMPLETED.md` §"Ψ-W1-T2") - [ ] Click the footer mode segment, save, reopen the project — the mode persisted (`docs/COMPLETED.md` §"Ψ-W1-T2") - [ ] `[verify — DAW]` Reproduce the strand: project with FX in both modes, switch to Design, save, close, reopen, toggle to Arrange. Toggle back and forth several more times — the arrangement's FX must NOT converge on permanently offline/hidden. A track the tool refuses to park is NAMED in the console, and the same refusal is not reprinted on a later reapply unless the refused set changed (`src/shell/view/view_fx_park.h`'s `decidePark` / `reportRefusedParks`) +- [ ] `[verify — DAW]` **The undo collapse.** In a project with several FX across several tracks, switch modes once and count the undo points REAPER shows (Ctrl-Z tooltip / undo history): expect exactly TWO — `ReaSampler: Design View FX park` above `ReaSampler: activate view` — never one per FX (`src/shell/view/view_fx_park.h`'s `fxParkUndoClose`) +- [ ] `[verify — DAW]` Then press Ctrl-Z once: every FX re-onlines in that one step and the flags stay parked. A second Ctrl-Z rolls the flags and the ext state back. The FX point must sit ABOVE the switch point, never below it and never folded into it (`src/shell/view/CLAUDE.md` §Invariants — the observed undo semantics) +- [ ] `[verify — DAW]` Open a project saved in Design mode and check the undo history: the load-tick reapply writes no FX state (they are already offline), so it must add NO undo point at all — the discard form. A `ReaSampler: Design View FX park` entry appearing on a plain project open means the discard is not working (`src/shell/view/view_fx_park.h`'s `fxParkUndoClose`) +- [ ] `[verify — DAW]` Whether `Undo_EndBlock2` tolerates a `ReaProject*` that closed between it and its `Undo_BeginBlock2`. The drain closes its block unconditionally on the project-gone abort path — an unbalanced block would swallow every later action into one point, which is the worse failure — but the SDK header promises nothing here. Reproduce by closing a project tab while a mode switch's FX drain is in flight (`src/shell/view/view_fx_park.cpp`'s `FxParkUndoBlock`) - [ ] `[verify — DAW]` For a track the console names as left unparked, follow the message's recovery IN FULL: re-enable its TCP/mixer visibility, main send and FX enable, **and set every FX in its chain online** (FX enable is the chain bypass — it does not bring an individually offlined FX back). Then switch modes twice: it parks and restores normally, with no further console line. Do the flag half ONLY on a second such track and confirm it is still refused — the FX half of the trust test, and the reason the message spells the step out (unit-tested only as a decision, never against a live chain) ## Actions and drops diff --git a/src/shell/view/CLAUDE.md b/src/shell/view/CLAUDE.md index a165484..481ed50 100644 --- a/src/shell/view/CLAUDE.md +++ b/src/shell/view/CLAUDE.md @@ -71,17 +71,24 @@ decide membership or mode rules. so the new mode paints first. The deferral changes only WHEN the plugins move — they still unload and re-instantiate, and un-persisted internal state is still lost. What it does change is the undo record, and REAPER's behaviour there is - now OBSERVED, not assumed: the drain's `TrackFX_SetOffline` calls (the only - `TrackFX_SetOffline` in the tree) run outside any undo block, and REAPER mints - ONE implicit undo point per call — a small project measured 10 points for one - switch, 1 for the switch itself and 9 for its FX. Ctrl-Z walks them one at a - time, each step re-onlining a single FX, so one Ctrl-Z after a switch does not - bring the plugins back. It also puts the switch point BELOW every FX point: an - undo deep enough to restore the driven flags and roll the `"reasampler"` ext - state back has already re-onlined every FX, so flags, FX and snapshot move - together. The per-FX fragmentation is itself a defect to be closed by collapsing - the drain's writes into one undo point; nothing here depends on them staying - separate. **The idle tick is not the only drain point.** Any path that + now OBSERVED, not assumed. What was measured, before the fix below: a + `TrackFX_SetOffline` made outside any undo block mints ONE implicit undo point + per call, and a small project showed 10 points for one switch — 1 for the switch + itself and 9 for its FX — which Ctrl-Z then walked one FX at a time. **The drain + now opens its own undo block** (`fxParkUndoClose`, `UNDO_STATE_FX` only, no other + domain), so one drain is one point and one switch costs two: the switch's, then + the drain's. A drain that wrote no FX state closes with REAPER's discard form + and mints nothing at all, so a plain project open costs no undo point + (`fxParkUndoClose` owns why). The ordering property the observation established is DELIBERATELY PRESERVED: the + drain's block is its own and never the switch's, so the FX point still sits ABOVE + the switch point, and an undo deep enough to restore the driven flags and roll + the `"reasampler"` ext state back has already re-onlined every FX — flags, FX and + snapshot still move together. Folding the FX writes into the switch's own block, + or making them undo-silent, would break that and is not an option here. So ONE + Ctrl-Z after a switch now re-onlines every FX at once, and a second rolls the + flags and the ext state back — two steps rather than the measured ten, and never + one, which is the standing price of the drain being a separate act. + **The idle tick is not the only drain point.** Any path that serializes the view model drains synchronously first (`persistViewState`, `render_in_place`), because a save landing between a restore's synchronous flag writes and its drain would record offline FX beside a model that no longer @@ -134,7 +141,7 @@ applies the resulting lane state to live tracks. ## Modules - `view` — Design View shell: snapshots flag values before parking, drives hide + CPU-park on inactive-mode leaves (`B_SHOWINTCP`/`B_SHOWINMIXER`/`B_MAINSEND`/`I_FXEN`, with per-FX offline deferred to `view_fx_park`), restores from snapshot. Owns the one discriminator (`target != active`) that separates a real switch from a reapply, and with it both the playback gate (`transportBlocksModeSwitch`) and the solo cache/clear/restore seams. **Never touches master or `B_MUTE`.** -- `view_fx_park` — the park surface: the `TrackFX_GetFXGUID` identity read snapshot/park/restore share, the deferred intent queue that keeps `TrackFX_SetOffline` off the switch's synchronous path (at most one intent per track GUID, latest wins, an intent landing on its own pending inverse cancels it), the idle-tick drain `main.cpp`'s `OnTimer` calls, the per-track park decision (`decidePark` over "does the model hold a snapshot" × "does the chain already read parked", with `decideParkForTrack` owning the live reads BOTH halves of that second term needs, and `trackFlagParm` the ONE `Flag` → REAPER-parameter mapping snapshot's read, park's read and restore's write all address), and the refusal report (`reportRefusedParks`, which names the tracks and holds the last-reported set so a reapply does not reprint it). **The drain owns no model state.** A snapshot is dropped where the restore is PLANNED — the flags are back at their captured values from that moment, and a model that still described the track as parked would let a persist or a reapply inside the drain window replan a restore over whatever the user changed since. What the deferral costs instead is that the live FX chain stops being a trustworthy snapshot source while an intent is pending: a park that CANCELS a pending restore takes the pre-park FX states from that restore's ops (`preParkFxFromCancelledRestore`), because the chain still reads the parked values and the cancel means no drain will ever fix them. +- `view_fx_park` — the park surface: the `TrackFX_GetFXGUID` identity read snapshot/park/restore share, the deferred intent queue that keeps `TrackFX_SetOffline` off the switch's synchronous path (at most one intent per track GUID, latest wins, an intent landing on its own pending inverse cancels it), the idle-tick drain `main.cpp`'s `OnTimer` calls and the ONE undo point it collapses its writes into (`fxParkUndoClose` — it opens that block itself, so no caller may drain while holding one), the per-track park decision (`decidePark` over "does the model hold a snapshot" × "does the chain already read parked", with `decideParkForTrack` owning the live reads BOTH halves of that second term needs, and `trackFlagParm` the ONE `Flag` → REAPER-parameter mapping snapshot's read, park's read and restore's write all address), and the refusal report (`reportRefusedParks`, which names the tracks and holds the last-reported set so a reapply does not reprint it). **The drain owns no model state.** A snapshot is dropped where the restore is PLANNED — the flags are back at their captured values from that moment, and a model that still described the track as parked would let a persist or a reapply inside the drain window replan a restore over whatever the user changed since. What the deferral costs instead is that the live FX chain stops being a trustworthy snapshot source while an intent is pending: a park that CANCELS a pending restore takes the pre-park FX states from that restore's ops (`preParkFxFromCancelledRestore`), because the chain still reads the parked values and the cancel means no drain will ever fix them. - `view_solo` — the `I_SOLO` read/write pair behind the per-mode solo surface, plus `clearTrackSolos`/`restoreTrackSolos`, the outgoing-clear and incoming-replay entry points `view` drives them through. Holds no policy: what to cache, clear, or replay is `core/view/solo_cache`. ## Gotchas @@ -163,7 +170,10 @@ applies the resulting lane state to live tracks. FX enqueue too. Full contract at `discardDeferredFxParks`. - **Which routes actually reach that no-snapshot-plus-parked-chain pair.** An undo of a mode switch (Ctrl-Z) is NOT one of them: per the observed undo semantics - above, the flags and the FX come back at the same step. Whether the ext-state + above, the drain's point sits above the switch's, so every FX is back online by + the step that rolls the flags and the ext state back — and the step in between + (FX online, flags still parked) still holds the model's snapshot, which decides + `ParkOnly`, not a refusal. Whether the ext-state snapshot comes back with them is INFERENCE, not observed, and untested in the REDO direction: `[verify — DAW]` does a redo that re-parks a track's flags/FX also restore a `view_state` that is snapshot-free for it (matching a fresh diff --git a/src/shell/view/view_fx_park.cpp b/src/shell/view/view_fx_park.cpp index fbe2e1f..f365ffa 100644 --- a/src/shell/view/view_fx_park.cpp +++ b/src/shell/view/view_fx_park.cpp @@ -23,10 +23,15 @@ #define REAPERAPI_WANT_TrackFX_GetFXGUID #define REAPERAPI_WANT_TrackFX_GetOffline #define REAPERAPI_WANT_TrackFX_SetOffline +#define REAPERAPI_WANT_Undo_BeginBlock2 +#define REAPERAPI_WANT_Undo_EndBlock2 #define REAPERAPI_WANT_ValidatePtr2 #define REAPERAPI_WANT_guidToString #include "reaper_plugin_functions.h" +static_assert(reasampler::kFxParkUndoMask == UNDO_STATE_FX, + "the SDK-free copy of the drain's undo mask has drifted from UNDO_STATE_FX"); + namespace reasampler { namespace { @@ -58,30 +63,70 @@ void adoptOwner(ReaProject* proj) { g_owner = p; } +// Returns whether it actually wrote — the drain's undo point is discarded unless +// something did (fxParkUndoClose). +// // [verify — DAW] TrackFX_SetOffline unloads and re-instantiates the plugin, so // writing a state that already holds is assumed to cost the same unload/reload // as a real change — the header documents no internal short-circuit either way. // The compare is cheap and correct regardless; only the size of what it saves is // unconfirmed. -void setOfflineIfChanged(MediaTrack* tr, int fx, bool offline) { - if (TrackFX_GetOffline(tr, fx) == offline) return; +bool setOfflineIfChanged(MediaTrack* tr, int fx, bool offline) { + if (TrackFX_GetOffline(tr, fx) == offline) return false; TrackFX_SetOffline(tr, fx, offline); + return true; } -void applyPark(MediaTrack* tr) { +bool applyPark(MediaTrack* tr) { + bool wrote = false; const int fxCount = TrackFX_GetCount(tr); - for (int fx = 0; fx < fxCount; ++fx) setOfflineIfChanged(tr, fx, true); + for (int fx = 0; fx < fxCount; ++fx) + if (setOfflineIfChanged(tr, fx, true)) wrote = true; + return wrote; } +struct RestoreOutcome { + FxRestoreDrops drops; + bool wrote = false; +}; + // Restores per-FX offline from the plan verbatim — never a blanket "online". // Which live FX each captured state belongs to is resolveFxRestore's call, and // what it could not place comes back for the caller to report. -FxRestoreDrops applyRestore(MediaTrack* tr, const std::vector& ops) { +RestoreOutcome applyRestore(MediaTrack* tr, const std::vector& ops) { const FxRestoreResolution res = resolveFxRestore(ops, liveFxGuids(tr)); - for (const FxOfflineWrite& w : res.writes) setOfflineIfChanged(tr, w.fxIndex, w.offline); - return res.drops; + RestoreOutcome out; + for (const FxOfflineWrite& w : res.writes) + if (setOfflineIfChanged(tr, w.fxIndex, w.offline)) out.wrote = true; + out.drops = res.drops; + return out; } +// ONE drain is ONE undo point (see src/shell/view/CLAUDE.md for the measured +// per-FX fragmentation this closes). The block is deliberately the drain's OWN +// and never the switch's — the FX point has to stay ABOVE the switch's. +// +// Closing is unconditional on every exit, the abort path included: an unbalanced +// block would swallow every later action into one point, which is far worse than +// [verify — DAW] whatever Undo_EndBlock2 does with a ReaProject* that has closed +// since Undo_BeginBlock2 took it. +class FxParkUndoBlock { +public: + explicit FxParkUndoBlock(ReaProject* proj) : proj_(proj) { Undo_BeginBlock2(proj_); } + ~FxParkUndoBlock() { + const FxParkUndoClose close = fxParkUndoClose(wrote_); + Undo_EndBlock2(proj_, close.label, close.mask); + } + FxParkUndoBlock(const FxParkUndoBlock&) = delete; + FxParkUndoBlock& operator=(const FxParkUndoBlock&) = delete; + + void noteWrite() { wrote_ = true; } + +private: + ReaProject* proj_; + bool wrote_ = false; +}; + // The FX's own durable identity, braced exactly like the track GUID keys. Empty // when REAPER reports none — an FX we cannot name is one we cannot restore, and // fx_offline treats it that way rather than guessing at its slot. Lifetime is @@ -221,27 +266,34 @@ void drainDeferredFxParks() { FxRestoreDrops drops; int dropTracks = 0; - for (const FxParkIntent& intent : draining) { - auto it = byGuid.find(intent.guid); - if (it == byGuid.end()) continue; // track deleted since the switch — prune + { + // Scoped so the block closes before the console report below: a report is + // not a project write and must not join what a Ctrl-Z rolls back, the same + // ordering applyMode gives reportRefusedParks. + FxParkUndoBlock undo(proj); + for (const FxParkIntent& intent : draining) { + auto it = byGuid.find(intent.guid); + if (it == byGuid.end()) continue; // track deleted since the switch — prune - // Re-validated PER INTENT, not once above: under the same pumping - // assumption, a project closed or a track deleted between two applies - // leaves the handle resolved above dangling — a use-after-free, not a - // pruned intent. Same gate capture_realtime_shell's teardown uses; a null - // first argument validates the ReaProject* itself (SDK: proj is ignored - // when the pointer is a project). - if (!ValidatePtr2(nullptr, proj, "ReaProject*")) return; - if (!ValidatePtr2(proj, it->second, "MediaTrack*")) continue; + // Re-validated PER INTENT, not once above: under the same pumping + // assumption, a project closed or a track deleted between two applies + // leaves the handle resolved above dangling — a use-after-free, not a + // pruned intent. Same gate capture_realtime_shell's teardown uses; a null + // first argument validates the ReaProject* itself (SDK: proj is ignored + // when the pointer is a project). + if (!ValidatePtr2(nullptr, proj, "ReaProject*")) return; + if (!ValidatePtr2(proj, it->second, "MediaTrack*")) continue; - if (intent.park) { - applyPark(it->second); - continue; - } - const FxRestoreDrops d = applyRestore(it->second, intent.restoreOps); - if (d.total() > 0) { - drops.add(d); - ++dropTracks; + if (intent.park) { + if (applyPark(it->second)) undo.noteWrite(); + continue; + } + const RestoreOutcome r = applyRestore(it->second, intent.restoreOps); + if (r.wrote) undo.noteWrite(); + if (r.drops.total() > 0) { + drops.add(r.drops); + ++dropTracks; + } } } diff --git a/src/shell/view/view_fx_park.h b/src/shell/view/view_fx_park.h index be46868..f0a0d45 100644 --- a/src/shell/view/view_fx_park.h +++ b/src/shell/view/view_fx_park.h @@ -263,6 +263,29 @@ PreParkFx snapshotFxOffline(MediaTrack* tr, const std::vector& canc std::vector deferFxPark(ReaProject* proj, const std::string& guid); void deferFxRestore(ReaProject* proj, const std::string& guid, std::vector ops); +// -- the drain's own undo point ----------------------------------------------- +// +// UNDO_STATE_FX and nothing else: per-FX offline is all the drain writes. The +// value is repeated here rather than included so this header stays SDK-free; +// view_fx_park.cpp static_asserts it against the macro (reaper_plugin.h:1542). +inline constexpr int kFxParkUndoMask = 2; + +// Undo_EndBlock2's two arguments for ONE drain. An empty label with a zero mask +// is REAPER's discard form — view.cpp's lane-mint path uses the same idiom — and +// discarding matters here rather than merely tidying: a project-load reapply +// plans a park for every inactive leaf, and a project saved parked already holds +// every one of those FX offline, so a block opened unconditionally would mint an +// empty undo point on each open. +struct FxParkUndoClose { + const char* label = ""; + int mask = 0; +}; + +inline FxParkUndoClose fxParkUndoClose(bool wroteAnyFx) { + if (!wroteAnyFx) return {}; + return FxParkUndoClose{"ReaSampler: Design View FX park", kFxParkUndoMask}; +} + // Applies every pending intent. Touches NO model state — a restore's snapshot is // dropped where the restore is planned (applyMode). Discards the queue unapplied // if the project it was enqueued against is no longer current (close / switch); @@ -278,6 +301,12 @@ void deferFxRestore(ReaProject* proj, const std::string& guid, std::vectorpark); } +// -- the drain's undo point -------------------------------------------------- + +static void testADrainThatWroteFxClosesItsBlockAsOneNamedFxPoint() { + const FxParkUndoClose close = fxParkUndoClose(true); + + // 2 is UNDO_STATE_FX (reaper_plugin.h:1542), pinned as a literal here and + // static_asserted against the macro in view_fx_park.cpp. The drain writes + // per-FX offline and nothing else, so any wider mask would make it marshal + // track config or items it never touched. + CHECK(close.mask == 2); + CHECK(close.label != nullptr && std::string(close.label) == + "ReaSampler: Design View FX park"); +} + +static void testADrainThatWroteNothingClosesItsBlockAsADiscard() { + // The project-load reapply: a park is planned for every inactive leaf, and a + // project saved parked already holds every one of those FX offline. Nothing is + // written, so the block must leave no undo point behind at all. + const FxParkUndoClose close = fxParkUndoClose(false); + + CHECK(close.mask == 0); + CHECK(close.label != nullptr && std::string(close.label).empty()); +} + int main() { testParkEnqueuesOneIntentCarryingNoOps(); testParkReportsNothingCancelledWhenNoIntentWasPending(); @@ -442,6 +467,9 @@ int main() { testIntentsArrivingDuringADrainSurviveIt(); testAReEntrantParkCancelsOnlyWhatIsStillPending(); + testADrainThatWroteFxClosesItsBlockAsOneNamedFxPoint(); + testADrainThatWroteNothingClosesItsBlockAsADiscard(); + if (g_fail == 0) std::printf("All tests passed.\n"); return g_fail ? 1 : 0; }