From a4a1c3860f2f858e7b834cf918fb2b90a1de8116 Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Mon, 3 Aug 2026 12:39:22 -0400 Subject: [PATCH 1/3] Defer Design View's per-FX park to an idle tick; honest undo mask, compare-before-write, PreventUIRefresh bracket, O(1) handle resolve --- docs/TODO.md | 14 +- src/app/CMakeLists.txt | 7 + src/app/main.cpp | 7 + src/shell/view/CLAUDE.md | 18 +- src/shell/view/view.cpp | 301 +++++++++++++++----------------- src/shell/view/view_fx_park.cpp | 157 +++++++++++++++++ src/shell/view/view_fx_park.h | 95 ++++++++++ tests/test_view_fx_park.cpp | 139 +++++++++++++++ 8 files changed, 568 insertions(+), 170 deletions(-) create mode 100644 src/shell/view/view_fx_park.cpp create mode 100644 src/shell/view/view_fx_park.h create mode 100644 tests/test_view_fx_park.cpp diff --git a/docs/TODO.md b/docs/TODO.md index 87fa831..947a354 100644 --- a/docs/TODO.md +++ b/docs/TODO.md @@ -868,9 +868,11 @@ the golden test literals pin would roll a format change and a codec extraction together, which is the riskier order. **Also over the bar, blocked differently.** `src/shell/view/view.cpp` measures -**642 lines** (verified this pass). Its seam is blocked not by a private-state/friend -question but by a build file another team owns: `src/shell/view/` has no -`CMakeLists.txt` of its own today. +**625 lines** (re-measured after the deferred FX-park split took `fxGuidString`, +`liveFxGuids` and the park/restore FX writes out into `view_fx_park`). Its remaining +seam is blocked not by a private-state/friend question but by a build file another +team owns: `src/shell/view/` has no `CMakeLists.txt` of its own today — a new TU +there costs one `target_sources` line in `src/app/CMakeLists.txt` instead. **Priority / risk.** Not stated. @@ -881,8 +883,8 @@ unblocked once the build-file ownership question is resolved. ## FX-GUID stability for `restoreFxOffline` is unverified in the DAW -**Context.** The Design View park/restore FX keying (`restoreFxOffline`, -`src/shell/view/view.cpp`) rests on `TrackFX_GetFXGUID` returning an identity that +**Context.** The Design View park/restore FX keying (`applyRestore`, +`src/shell/view/view_fx_park.cpp`) rests on `TrackFX_GetFXGUID` returning an identity that survives a chain reorder while a track is parked. SWS issue #802 reports that after `SNM_MoveOrRemoveTrackFX` reorders a chain, the FXID lines do not follow the plugin (`SNM_PreObjectState()` → `RemoveAllIds()`) — if that still holds, an SWS-driven @@ -893,7 +895,7 @@ operation this keying targets. instances of the same plugin. **Already flagged in code — this entry is the tracked home, not a restatement.** -There is a `[verify — DAW]` marker at `fxGuidString` in `src/shell/view/view.cpp` and +There is a `[verify — DAW]` marker at `fxGuidString` in `src/shell/view/view_fx_park.cpp` and a note in `src/shell/view/CLAUDE.md`'s Gotchas; point at them rather than restating them in full. diff --git a/src/app/CMakeLists.txt b/src/app/CMakeLists.txt index 2669575..e2473d6 100644 --- a/src/app/CMakeLists.txt +++ b/src/app/CMakeLists.txt @@ -38,6 +38,7 @@ add_library(reaper_reasampler MODULE ${LICE_SRC} ${REASAMPLER_SRC_DIR}/shell/capture/insert.cpp ${REASAMPLER_SRC_DIR}/shell/view/view.cpp + ${REASAMPLER_SRC_DIR}/shell/view/view_fx_park.cpp ${REASAMPLER_SRC_DIR}/shell/view/view_solo.cpp ${REASAMPLER_SRC_DIR}/shell/capture/track_guid.cpp ${REASAMPLER_SRC_DIR}/shell/capture/provenance_shell.cpp @@ -59,6 +60,12 @@ target_link_libraries(reaper_reasampler PRIVATE json wire file_bytes bank_model # link graph free of the voice engine — a link edge to it here means the design drifted. target_include_directories(reaper_reasampler PRIVATE ${SDK_INC} ${WDL_INC}) +# The deferred FX-park queue's re-entrancy rule is pure (header-inline, no REAPER +# types), so it is CTest-covered like a core/ module. Declared here rather than in a +# src/shell/view/CMakeLists.txt because that directory deliberately has none — its +# TUs are compiled into this target directly. +reasampler_test(view_fx_park LINK fx_offline) + # Bank-package import: the promptless verb plus its action skin. Kept as its own # appended block rather than merged into the lists above, so the two package # directions stay textually independent. diff --git a/src/app/main.cpp b/src/app/main.cpp index 45d5c02..ca7b52b 100644 --- a/src/app/main.cpp +++ b/src/app/main.cpp @@ -38,6 +38,7 @@ #include "shell/panel/panel_window.h" // panel lifecycle (init/toggle/open-query/shutdown) #include "shell/persist/session.h" // ReaSamplerSession #include "shell/view/view.h" // reconcileManagedLanes / applyMode +#include "shell/view/view_fx_park.h" // the mode switch's deferred FX park namespace capture = reasampler::capture; @@ -172,6 +173,12 @@ static void OnTimer() g_session.poll(); + // A mode switch applies its visibility/routing writes synchronously and leaves + // the per-FX offline work here, so the new mode paints before the plugins + // unload. Drained AFTER poll() so a project switch discards the queue instead + // of applying it to the project that replaced it; idle cost is one empty test. + reasampler::drainDeferredFxParks(g_session.view()); + // persist stays MODEL-ONLY (loads the saved view model but does not apply // visibility, to avoid coupling persist to the view shell); poll() raises a // one-shot load signal that we drain here to reapply the SAVED active mode so a diff --git a/src/shell/view/CLAUDE.md b/src/shell/view/CLAUDE.md index b8ab91a..e4f719d 100644 --- a/src/shell/view/CLAUDE.md +++ b/src/shell/view/CLAUDE.md @@ -45,9 +45,16 @@ decide membership or mode rules. mode ids — no absolute paths, no index positions). - **Documented caveat:** offlined FX re-instantiate when a track returns to the active mode — stateful plugins (convolution, loaded samplers, tail-holding - effects) re-initialize on return (possible load hitch, un-persisted internal - state lost). Accepted cost of the CPU reclaim; surfaced at the toggle affordance - (tooltip). + effects) re-initialize on return (load hitch, un-persisted internal state lost). + Accepted cost of the CPU reclaim; surfaced at the toggle affordance (tooltip). + **The hitch no longer sits on the switch's synchronous path:** per-FX + offline/online is enqueued and applied on a later idle tick (`view_fx_park`), + 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: the offline writes land outside + the switch's undo block, so the tool no longer re-drives them on an undo or a + redo — what a Ctrl-Z then leaves the chain at is REAPER's own FX-state record, + `[verify — DAW]`. - **Show-both semantics:** a per-track "pin visible across modes" flag re-enables processing whenever shown. A show-both leaf appears in every mode's visible set and is never parked — its driven flags stay at snapshot/restored values, FX @@ -85,7 +92,8 @@ 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` + per-FX offline), 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` — 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 per-FX offline 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), and the idle-tick drain `main.cpp`'s `OnTimer` calls. The drain is also where a consumed snapshot is cleared — not where the restore was planned — so a second switch arriving before the first drained re-parks against the still-true captured state instead of re-capturing parked values. - `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 @@ -105,7 +113,7 @@ applies the resulting lane state to live tracks. the solo cache inherits it rather than introducing it. Not fixed here. - `fx_offline`'s identity keying (`TrackFX_GetFXGUID`) assumes the GUID stays attached to its plugin across a chain mutation while parked. That is - `[verify — DAW]` (see `fxGuidString` in `view.cpp`) and SWS issue #802 is a + `[verify — DAW]` (see `fxGuidString` in `view_fx_park.cpp`) and SWS issue #802 is a known reason it might not hold: `SNM_MoveOrRemoveTrackFX` reportedly leaves the FXID lines behind on reorder rather than moving them with the plugin. If confirmed, an SWS-driven reorder of a parked track's chain — not a native diff --git a/src/shell/view/view.cpp b/src/shell/view/view.cpp index f12520c..4f48614 100644 --- a/src/shell/view/view.cpp +++ b/src/shell/view/view.cpp @@ -10,6 +10,7 @@ #include #include #include +#include #include #include @@ -18,6 +19,7 @@ #include "core/view/lane_keys.h" #include "core/view/solo_cache.h" #include "shell/capture/track_guid.h" +#include "shell/view/view_fx_park.h" #include "shell/view/view_solo.h" #include "core/view/view_tree.h" @@ -28,12 +30,8 @@ #define REAPERAPI_WANT_GetMediaTrackInfo_Value #define REAPERAPI_WANT_SetMediaTrackInfo_Value #define REAPERAPI_WANT_GetSetMediaTrackInfo_String -#define REAPERAPI_WANT_ShowConsoleMsg -#define REAPERAPI_WANT_TrackFX_GetCount -#define REAPERAPI_WANT_TrackFX_GetFXGUID +#define REAPERAPI_WANT_PreventUIRefresh #define REAPERAPI_WANT_TrackFX_GetOffline -#define REAPERAPI_WANT_TrackFX_SetOffline -#define REAPERAPI_WANT_guidToString #define REAPERAPI_WANT_Undo_BeginBlock2 #define REAPERAPI_WANT_Undo_EndBlock2 #define REAPERAPI_WANT_TrackList_AdjustWindows @@ -64,6 +62,26 @@ constexpr int kFreeModeFixedLanes = 2; // deliberately excluded, nothing is moving there. constexpr int kTransportMoving = 1 | 4; +// The domains one apply actually writes: track config/routing (visibility, +// B_MAINSEND, I_SOLO), item lane assignment, and the ext-state block +// (UNDO_STATE_MISCCFG covers extension state). NOT UNDO_STATE_ALL, which +// includes UNDO_STATE_FX and so makes every undo record carry every track's FX +// state — a cost that scales with the project's plugin count rather than with +// what the switch changed. FX is OR'd back in per apply, only when an FX-domain +// flag really moved. +constexpr int kApplyUndoMask = UNDO_STATE_TRACKCFG | UNDO_STATE_ITEMS | UNDO_STATE_MISCCFG; + +// Holds REAPER's UI refresh off for the write phase; the deliberate rebuild +// (TrackList_AdjustWindows + UpdateArrange) runs after it releases. RAII because +// an unbalanced pair leaves the user's UI frozen with no way back — the SDK's +// own warning at reaper_plugin_functions.h:5581. +struct UiRefreshHold { + UiRefreshHold() { PreventUIRefresh(1); } + ~UiRefreshHold() { PreventUIRefresh(-1); } + UiRefreshHold(const UiRefreshHold&) = delete; + UiRefreshHold& operator=(const UiRefreshHold&) = delete; +}; + // C_LANESCOLLAPSED=2: render a tool-split track like a normal single-lane // track showing only the playing lane (SDK: 1=collapsed, 2=hidden-lanes-exist // but displays as non-fixed-lane). @@ -102,7 +120,7 @@ const char* flagParm(Flag f) { // construction. Also caches each MediaTrack* by GUID for later resolve(). std::vector readFolderEntries( ReaProject* proj, - std::vector>& handleByGuid) { + TrackHandles& handleByGuid) { std::vector entries; int count = CountTracks(proj); entries.reserve(static_cast(count)); @@ -119,46 +137,21 @@ std::vector readFolderEntries( return entries; } -MediaTrack* resolve(const std::vector>& handleByGuid, - const std::string& guid) { - for (const auto& kv : handleByGuid) { - if (kv.first == guid) return kv.second; - } - return nullptr; // stale/deleted GUID — pruned by being skipped +// Every park, restore, parent and lane op resolves its GUID; a linear scan made +// that O(T²) on the one path whose cost the user waits on. The vector form +// survives beside it because view_solo's writers take one pass over it. +using TrackByGuid = std::unordered_map; + +TrackByGuid indexHandles(const TrackHandles& handleByGuid) { + TrackByGuid byGuid; + byGuid.reserve(handleByGuid.size()); + for (const auto& kv : handleByGuid) byGuid.emplace(kv.first, kv.second); + return byGuid; } -// 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 -// settled: the string copy is taken immediately and the GUID* is never held -// past this call (reaper_plugin_functions.h:7348 documents no null contract for -// TrackFX_GetFXGUID; treating null as "no identity" is the safe read). -// -// [verify — DAW] STABILITY across a chain mutation is not settled the same way: -// confirm the GUID for one FX instance survives a native drag-reorder, an SWS -// move (SNM_MoveOrRemoveTrackFX — SWS issue #802 reports the FXID lines do not -// follow the plugin after that call, i.e. wrong-plugin restores or mass drops -// through fx_offline on that path specifically), a save/reload round trip, and -// two live instances of one plugin type staying distinguishable. See -// src/shell/view/CLAUDE.md's Gotchas for the SWS-path risk this leaves open. -std::string fxGuidString(MediaTrack* tr, int fx) { - GUID* g = TrackFX_GetFXGUID(tr, fx); - if (!g) return {}; - char buf[64] = {0}; // guidToString needs a >=64-char destination (SDK contract) - guidToString(g, buf); - return std::string(buf); -} - -// The chain as it stands now: identity by current slot. Snapshot, park and -// restore all address FX through this one plain 0..TrackFX_GetCount-1 -// enumeration — never the 0x1000000/0x2000000 input-FX or container forms — so -// whatever it covers, all three cover identically. -std::vector liveFxGuids(MediaTrack* tr) { - const int fxCount = TrackFX_GetCount(tr); - std::vector guids; - guids.reserve(static_cast(fxCount)); - for (int fx = 0; fx < fxCount; ++fx) guids.push_back(fxGuidString(tr, fx)); - return guids; +MediaTrack* resolve(const TrackByGuid& byGuid, const std::string& guid) { + auto it = byGuid.find(guid); + return it == byGuid.end() ? nullptr : it->second; // stale/deleted GUID — pruned by being skipped } // Captures prior driven-flag state before parking. Never reads B_MUTE/I_SOLO; @@ -179,30 +172,25 @@ TrackSnapshot snapshotTrack(MediaTrack* tr) { return snap; // fxKeying stays Identity — a live capture always knows the chain } -void applyFlags(MediaTrack* tr, const std::vector& flags) { +// A reapply (tag/untag, project load) re-plans every flag it already applied, so +// most of what it writes is a value the track already holds; the read is the +// cheap half of the pair, and it also keeps those no-op writes out of the undo +// record's mask. +bool writeIfChanged(MediaTrack* tr, const char* parm, double value) { + if (GetMediaTrackInfo_Value(tr, parm) == value) return false; + SetMediaTrackInfo_Value(tr, parm, value); + return true; +} + +void applyFlags(MediaTrack* tr, const std::vector& flags, int& undoMask) { for (const TrackFlagOp& op : flags) { - SetMediaTrackInfo_Value(tr, flagParm(op.flag), static_cast(op.value)); + if (!writeIfChanged(tr, flagParm(op.flag), static_cast(op.value))) continue; + // I_FXEN is the only FX-domain flag driven here, so it is the only one + // whose undo record has to carry UNDO_STATE_FX. + if (op.flag == Flag::FxEnable) undoMask |= UNDO_STATE_FX; } } -// The pure park plan leaves fxOffline empty by design; expand it here from the -// live FX count. -void parkFxOffline(MediaTrack* tr) { - int fxCount = TrackFX_GetCount(tr); - for (int fx = 0; fx < fxCount; ++fx) { - TrackFX_SetOffline(tr, fx, true); - } -} - -// Restores per-FX offline from the snapshot 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 restoreFxOffline(MediaTrack* tr, const std::vector& fxOffline) { - const FxRestoreResolution res = resolveFxRestore(fxOffline, liveFxGuids(tr)); - for (const FxOfflineWrite& w : res.writes) TrackFX_SetOffline(tr, w.fxIndex, w.offline); - return res.drops; -} - // Managed-lane application: the pure planner keys LanePlayOps by the lane's // DURABLE name; REAPER's C_LANEPLAYS:N is keyed by current ordinal, which // renumbers on reorder. So every write here re-resolves durable key -> current @@ -238,7 +226,7 @@ std::map managedLaneOrdinals(MediaTrack* tr) { void applyLanePlays(MediaTrack* tr, int laneIdx, int lanePlays) { char parm[32]; std::snprintf(parm, sizeof(parm), "C_LANEPLAYS:%d", laneIdx); - SetMediaTrackInfo_Value(tr, parm, static_cast(lanePlays)); + writeIfChanged(tr, parm, static_cast(lanePlays)); } // Groups ops by track, reconciles each op's durable laneKey to the track's @@ -246,7 +234,7 @@ void applyLanePlays(MediaTrack* tr, int laneIdx, int lanePlays) { // enables fixed-lane mode on any track carrying a managed lane, and drives // C_LANEPLAYS. UpdateTimeline() is the caller's job when this returns true // (SDK: required after an I_FREEMODE change). -bool applyLaneOps(const std::vector>& handleByGuid, +bool applyLaneOps(const TrackByGuid& handleByGuid, const std::vector& lanes) { if (lanes.empty()) return false; @@ -312,7 +300,7 @@ std::string itemModeFromMembership(const ViewModeModel& model, const std::string // there regardless of name). std::vector readLaneTracks( const ViewModeModel& model, - const std::vector>& handleByGuid) { + const TrackHandles& handleByGuid) { std::vector tracks; tracks.reserve(handleByGuid.size()); for (const auto& [guid, tr] : handleByGuid) { @@ -358,7 +346,7 @@ bool assignItemToLane(MediaTrack* tr, MediaItem* it, int laneOrdinal) { // managed-eligible items; I_NUMFIXEDLANES is only ever GROWN, never shrunk, // so a user's existing manual lanes are never renamed or reassigned. bool applyMintPlan(ViewModeModel& model, const LaneMintPlan& plan, - const std::vector>& handleByGuid) { + const TrackByGuid& handleByGuid) { bool changed = false; std::map> mintsByTrack; @@ -436,8 +424,9 @@ bool applyMode(ViewModeModel& model, const std::string& targetModeId, ReaProject return false; // same fail-closed shape as the mode-exists guard above } - std::vector> handleByGuid; + TrackHandles handleByGuid; std::vector entries = readFolderEntries(proj, handleByGuid); + const TrackByGuid trackByGuid = indexHandles(handleByGuid); FolderTree tree = buildFolderTree(entries); // Prune snapshots for tracks no longer in the live enumeration before @@ -455,81 +444,6 @@ bool applyMode(ViewModeModel& model, const std::string& targetModeId, ReaProject TogglePlan plan = model.planToggle(tree, targetModeId); - Undo_BeginBlock2(proj); - - // DISJOIN THE SOLO SURFACES. Both this clear and the replay after setActiveMode - // ride the existing undo block — one mode toggle stays one Ctrl-Z. - if (realSwitch) { - model.soloCache().store(outgoingModeId, outgoingSolo); - clearTrackSolos(handleByGuid, outgoingSolo); - } - - // PARK: snapshot before mutating, store into the model, then apply. - for (const TrackPlan& tp : plan.park) { - if (tp.flags.empty()) continue; // every op in a TrackPlan targets one track - const std::string& guid = tp.flags.front().guid; - MediaTrack* tr = resolve(handleByGuid, guid); - if (!tr) continue; // stale GUID — prune - - // Snapshot ONCE, at first park: a snapshot already present means the - // track is still parked from a prior apply, so its live flags are the - // parked values — recapturing would overwrite the true pre-park state - // with zeros and a later restore would hide it for good. Restore - // clears the snapshot, so the next genuine park recaptures fresh state. - if (model.snapshot(guid) == nullptr) - model.storeSnapshot(guid, snapshotTrack(tr)); - applyFlags(tr, tp.flags); - parkFxOffline(tr); - } - - // RESTORE: apply verbatim, then drop the consumed snapshot. - FxRestoreDrops fxDrops; - int fxDropTracks = 0; - for (const TrackPlan& tp : plan.restore) { - if (tp.flags.empty()) continue; - const std::string& guid = tp.flags.front().guid; - MediaTrack* tr = resolve(handleByGuid, guid); - if (!tr) continue; // stale GUID — prune - - applyFlags(tr, tp.flags); - const FxRestoreDrops drops = restoreFxOffline(tr, tp.fxOffline); - if (drops.total() > 0) { - fxDrops.add(drops); - ++fxDropTracks; - } - model.clearSnapshot(guid); - } - - // Captured FX state that could not be applied is REPORTED. Silence here would - // read to the user as "restore worked" while an FX sat at whatever state the - // park left it in. Sent with the "!SHOW:" prefix (reaper_plugin_functions.h:6536) - // so it never force-opens the console window: applyMode's reapply path also - // runs unattended on project load (see the reconcile comment above), and this - // one call site can't tell that case apart from an interactive toggle/tag-edit - // reapply — both call in with target == active — so splitting loud-on-toggle - // from quiet-on-load would need a flag threaded from every caller, several of - // which are outside this change. Quiet-always is the safe default: the message - // still lands in the console for whoever opens it, on every path. - const std::string fxDropMsg = describeFxRestoreDrops(fxDrops, fxDropTracks); - if (!fxDropMsg.empty()) ShowConsoleMsg(("!SHOW:" + fxDropMsg).c_str()); - - // MANAGED LANES: drive C_LANEPLAYS so the active mode's lane plays+shows - // and every other managed lane is silenced+hidden. Empty for a D1-only - // project, leaving that behavior byte-identical. - const bool laneModeChanged = applyLaneOps(handleByGuid, plan.lanes); - - // PARENT VISIBILITY (never parked): recomputed every toggle, never - // snapshotted. Only the two visibility flags — never mainSend/FX on a parent. - std::set visible = model.visibleTracks(tree, targetModeId); - for (const FolderNode& node : tree.nodes) { - if (!node.isParent) continue; - MediaTrack* tr = resolve(handleByGuid, node.guid); - if (!tr) continue; // stale GUID — prune - double show = visible.count(node.guid) ? 1.0 : 0.0; - SetMediaTrackInfo_Value(tr, "B_SHOWINTCP", show); - SetMediaTrackInfo_Value(tr, "B_SHOWINMIXER", show); - } - // Target is guaranteed registered (checked at entry); fall back to the id // defensively if that ever changes. const Mode* targetMode = model.modes().query(targetModeId); @@ -537,16 +451,81 @@ bool applyMode(ViewModeModel& model, const std::string& targetModeId, ReaProject "ReaSampler: activate " + (targetMode ? targetMode->displayName : targetModeId) + " view"; - model.setActiveMode(targetModeId); + int undoMask = kApplyUndoMask; + bool laneModeChanged = false; - // Replay + consume, before the single relayout below picks the change up. Dropped - // against `visible` (computed above for parent visibility), not the park plan: a - // folder parent can be hidden without being parked, and a hidden track must not - // receive a replayed solo it carries no visible control to undo. - if (realSwitch) { - if (const std::map* cached = model.soloCache().query(targetModeId)) { - restoreTrackSolos(handleByGuid, *cached, liveGuids, visible); - model.soloCache().clear(targetModeId); + Undo_BeginBlock2(proj); + { + UiRefreshHold uiHold; // every write below lands with the TCP/MCP frozen + + // DISJOIN THE SOLO SURFACES. Both this clear and the replay after + // setActiveMode ride the existing undo block — one mode toggle stays one Ctrl-Z. + if (realSwitch) { + model.soloCache().store(outgoingModeId, outgoingSolo); + clearTrackSolos(handleByGuid, outgoingSolo); + } + + // PARK: snapshot before mutating, store into the model, then apply. The + // per-FX offline half is deferred (view_fx_park) — it is the expensive + // half and nothing about the new mode's appearance waits on it. + for (const TrackPlan& tp : plan.park) { + if (tp.flags.empty()) continue; // every op in a TrackPlan targets one track + const std::string& guid = tp.flags.front().guid; + MediaTrack* tr = resolve(trackByGuid, guid); + if (!tr) continue; // stale GUID — prune + + // Snapshot ONCE, at first park: a snapshot already present means the + // track is still parked from a prior apply (or its deferred restore + // has not landed yet), so its live flags are the parked values — + // recapturing would overwrite the true pre-park state with zeros and + // a later restore would hide it for good. + if (model.snapshot(guid) == nullptr) + model.storeSnapshot(guid, snapshotTrack(tr)); + applyFlags(tr, tp.flags, undoMask); + deferFxPark(proj, guid); + } + + // RESTORE: flags verbatim now, per-FX offline on the drain, which is also + // where the consumed snapshot is dropped. + for (const TrackPlan& tp : plan.restore) { + if (tp.flags.empty()) continue; + const std::string& guid = tp.flags.front().guid; + MediaTrack* tr = resolve(trackByGuid, guid); + if (!tr) continue; // stale GUID — prune + + applyFlags(tr, tp.flags, undoMask); + deferFxRestore(proj, guid, tp.fxOffline); + } + + // MANAGED LANES: drive C_LANEPLAYS so the active mode's lane plays+shows + // and every other managed lane is silenced+hidden. Empty for a D1-only + // project, leaving that behavior byte-identical. + laneModeChanged = applyLaneOps(trackByGuid, plan.lanes); + + // PARENT VISIBILITY (never parked): recomputed every toggle, never + // snapshotted. Only the two visibility flags — never mainSend/FX on a parent. + std::set visible = model.visibleTracks(tree, targetModeId); + for (const FolderNode& node : tree.nodes) { + if (!node.isParent) continue; + MediaTrack* tr = resolve(trackByGuid, node.guid); + if (!tr) continue; // stale GUID — prune + const double show = visible.count(node.guid) ? 1.0 : 0.0; + writeIfChanged(tr, "B_SHOWINTCP", show); + writeIfChanged(tr, "B_SHOWINMIXER", show); + } + + model.setActiveMode(targetModeId); + + // Replay + consume, before the single relayout below picks the change up. + // Dropped against `visible` (computed above for parent visibility), not the + // park plan: a folder parent can be hidden without being parked, and a + // hidden track must not receive a replayed solo it carries no visible + // control to undo. + if (realSwitch) { + if (const std::map* cached = model.soloCache().query(targetModeId)) { + restoreTrackSolos(handleByGuid, *cached, liveGuids, visible); + model.soloCache().clear(targetModeId); + } } } @@ -560,12 +539,12 @@ bool applyMode(ViewModeModel& model, const std::string& targetModeId, ReaProject // fixed lanes this apply (SDK requirement for I_FREEMODE changes). if (laneModeChanged) UpdateTimeline(); - Undo_EndBlock2(proj, undoLabel.c_str(), -1); + Undo_EndBlock2(proj, undoLabel.c_str(), undoMask); return true; } bool mintManagedLanes(ViewModeModel& model, ReaProject* proj) { - std::vector> handleByGuid; + TrackHandles handleByGuid; std::vector entries = readFolderEntries(proj, handleByGuid); // The tree is needed to detect a content-bearing folder derived-visible in // >1 mode, exactly as applyMode builds it. @@ -575,8 +554,10 @@ bool mintManagedLanes(ViewModeModel& model, ReaProject* proj) { const LaneMintPlan plan = planLaneMinting(model, tree, tracks); if (plan.empty()) return false; // nothing to mint — no Undo point for a no-op tick + const TrackByGuid trackByGuid = indexHandles(handleByGuid); + Undo_BeginBlock2(proj); - const bool changed = applyMintPlan(model, plan, handleByGuid); + const bool changed = applyMintPlan(model, plan, trackByGuid); if (!changed) { // Plan was non-empty but every write was already satisfied — discard @@ -599,17 +580,19 @@ bool mintManagedLanes(ViewModeModel& model, ReaProject* proj) { // here — it would re-park/restore whole tracks and recompute parent // visibility, which a lane-only mint must not touch. const TogglePlan togglePlan = model.planToggle(FolderTree{}, model.activeModeId()); - applyLaneOps(handleByGuid, togglePlan.lanes); + applyLaneOps(trackByGuid, togglePlan.lanes); UpdateTimeline(); // a split happened this call — refresh is owed UpdateArrange(); - Undo_EndBlock2(proj, "ReaSampler: separate cross-mode content into lanes", -1); + // Lane minting writes track lane config and item lane assignment and nothing + // else — never an FX state — so it takes the same honest mask applyMode does. + Undo_EndBlock2(proj, "ReaSampler: separate cross-mode content into lanes", kApplyUndoMask); return true; } void reconcileManagedLanes(ViewModeModel& model, ReaProject* proj) { - std::vector> handleByGuid; + TrackHandles handleByGuid; readFolderEntries(proj, handleByGuid); // populates handleByGuid (tree unused here) // Pure read of REAPER state (no lane created, no I_FREEMODE/I_NUMFIXEDLANES/ diff --git a/src/shell/view/view_fx_park.cpp b/src/shell/view/view_fx_park.cpp new file mode 100644 index 0000000..95190aa --- /dev/null +++ b/src/shell/view/view_fx_park.cpp @@ -0,0 +1,157 @@ +// See view_fx_park.h. Compiled into the reaper_reasampler module; includes +// reaper_plugin_functions.h without REAPERAPI_IMPLEMENT (main.cpp owns that). +// The queue's rule is pure (header, test_view_fx_park.cpp); this file owns the +// REAPER reads/writes and the ordering against the live enumeration. + +#include "shell/view/view_fx_park.h" + +#include +#include +#include + +#include "core/view/view_mode_model.h" +#include "shell/capture/track_guid.h" + +#define REAPERAPI_MINIMAL +#define REAPERAPI_WANT_CountTracks +#define REAPERAPI_WANT_EnumProjects +#define REAPERAPI_WANT_GetTrack +#define REAPERAPI_WANT_ShowConsoleMsg +#define REAPERAPI_WANT_TrackFX_GetCount +#define REAPERAPI_WANT_TrackFX_GetFXGUID +#define REAPERAPI_WANT_TrackFX_GetOffline +#define REAPERAPI_WANT_TrackFX_SetOffline +#define REAPERAPI_WANT_guidToString +#include "reaper_plugin_functions.h" + +namespace reasampler { + +namespace { + +FxParkQueue g_queue; +ReaProject* g_owner = nullptr; // the project the pending intents were enqueued against + +ReaProject* currentProject() { return EnumProjects(-1, nullptr, 0); } + +void adoptOwner(ReaProject* proj) { + ReaProject* p = proj ? proj : currentProject(); + if (p == g_owner) return; + g_queue.clear(); // intents planned against another project are never replayed here + g_owner = p; +} + +// TrackFX_SetOffline unloads and re-instantiates the plugin, so writing a state +// that already holds is not a no-op — it is the expensive half of a mode switch +// spent on nothing. +void setOfflineIfChanged(MediaTrack* tr, int fx, bool offline) { + if (TrackFX_GetOffline(tr, fx) == offline) return; + TrackFX_SetOffline(tr, fx, offline); +} + +void applyPark(MediaTrack* tr) { + const int fxCount = TrackFX_GetCount(tr); + for (int fx = 0; fx < fxCount; ++fx) setOfflineIfChanged(tr, fx, true); +} + +// 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) { + const FxRestoreResolution res = resolveFxRestore(ops, liveFxGuids(tr)); + for (const FxOfflineWrite& w : res.writes) setOfflineIfChanged(tr, w.fxIndex, w.offline); + return res.drops; +} + +// 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 +// settled: the string copy is taken immediately and the GUID* is never held +// past this call (reaper_plugin_functions.h:7348 documents no null contract for +// TrackFX_GetFXGUID; treating null as "no identity" is the safe read). +// +// [verify — DAW] STABILITY across a chain mutation is not settled the same way: +// confirm the GUID for one FX instance survives a native drag-reorder, an SWS +// move (SNM_MoveOrRemoveTrackFX — SWS issue #802 reports the FXID lines do not +// follow the plugin after that call, i.e. wrong-plugin restores or mass drops +// through fx_offline on that path specifically), a save/reload round trip, and +// two live instances of one plugin type staying distinguishable. See +// src/shell/view/CLAUDE.md's Gotchas for the SWS-path risk this leaves open. +std::string fxGuidString(MediaTrack* tr, int fx) { + GUID* g = TrackFX_GetFXGUID(tr, fx); + if (!g) return {}; + char buf[64] = {0}; // guidToString needs a >=64-char destination (SDK contract) + guidToString(g, buf); + return std::string(buf); +} + +} // namespace + +std::vector liveFxGuids(MediaTrack* tr) { + const int fxCount = TrackFX_GetCount(tr); + std::vector guids; + guids.reserve(static_cast(fxCount)); + for (int fx = 0; fx < fxCount; ++fx) guids.push_back(fxGuidString(tr, fx)); + return guids; +} + +void deferFxPark(ReaProject* proj, const std::string& guid) { + adoptOwner(proj); + g_queue.park(guid); +} + +void deferFxRestore(ReaProject* proj, const std::string& guid, std::vector ops) { + adoptOwner(proj); + g_queue.restore(guid, std::move(ops)); +} + +void drainDeferredFxParks(ViewModeModel& model) { + if (g_queue.empty()) return; + + if (g_owner != currentProject()) { + // The project the intents were planned against was closed or switched + // away from: its tracks are not ours to write and its handles may be + // gone. Discard rather than apply. + g_queue.clear(); + g_owner = nullptr; + return; + } + + std::unordered_map byGuid; + const int count = CountTracks(g_owner); + byGuid.reserve(static_cast(count)); + for (int i = 0; i < count; ++i) { + MediaTrack* tr = GetTrack(g_owner, i); + if (!tr) continue; + std::string guid = guidString(tr); + if (!guid.empty()) byGuid.emplace(std::move(guid), tr); + } + + FxRestoreDrops drops; + int dropTracks = 0; + for (const FxParkIntent& intent : g_queue.pending()) { + auto it = byGuid.find(intent.guid); + if (it == byGuid.end()) continue; // track deleted since the switch — prune + if (intent.park) { + applyPark(it->second); + continue; + } + const FxRestoreDrops d = applyRestore(it->second, intent.restoreOps); + if (d.total() > 0) { + drops.add(d); + ++dropTracks; + } + model.clearSnapshot(intent.guid); + } + + g_queue.clear(); + + // Captured FX state that could not be applied is REPORTED — silence would read + // to the user as "restore worked" while an FX sat at whatever state the park + // left it in. The "!SHOW:" prefix (reaper_plugin_functions.h:6536) keeps it from + // force-opening the console: this drain also runs behind an unattended + // project-load reapply, and cannot tell that case from an interactive toggle. + const std::string fxDropMsg = describeFxRestoreDrops(drops, dropTracks); + if (!fxDropMsg.empty()) ShowConsoleMsg(("!SHOW:" + fxDropMsg).c_str()); +} + +} // namespace reasampler diff --git a/src/shell/view/view_fx_park.h b/src/shell/view/view_fx_park.h new file mode 100644 index 0000000..2377277 --- /dev/null +++ b/src/shell/view/view_fx_park.h @@ -0,0 +1,95 @@ +#pragma once +// Design View's per-FX offline surface: the FX-identity read that snapshot, +// park and restore all address FX through, the deferred intent queue that keeps +// TrackFX_SetOffline off the mode switch's synchronous path, and the idle-tick +// drain that applies it. See src/shell/view/CLAUDE.md's FX-parking caveat. + +#include +#include + +#include "core/view/fx_offline.h" + +// Forward-declared to keep this header SDK-free; the .cpp includes the real SDK header. +class MediaTrack; +class ReaProject; + +namespace reasampler { + +class ViewModeModel; + +// The chain as it stands now: identity by current slot. Snapshot, park and +// restore all address FX through this one plain 0..TrackFX_GetCount-1 +// enumeration — never the 0x1000000/0x2000000 input-FX or container forms — so +// whatever it covers, all three cover identically. +std::vector liveFxGuids(MediaTrack* tr); + +// One deferred per-FX intent for one track. A park carries no ops (every live +// slot goes offline); a restore carries the planned ops verbatim. +struct FxParkIntent { + std::string guid; + bool park = false; + std::vector restoreOps; +}; + +// The queue's re-entrancy rule, pure so it can be asserted without a DAW: at +// most ONE intent per track GUID, and the latest one wins. Park and restore are +// inverses, so an intent landing on its own pending inverse CANCELS it rather +// than stacking — the queued work never ran, so the track already holds the +// state the newcomer asks for, and replaying both would be both slower and +// observably wrong. +class FxParkQueue { +public: + void park(const std::string& guid) { + FxParkIntent* held = find(guid); + if (!held) { + pending_.push_back(FxParkIntent{guid, true, {}}); + } else if (!held->park) { + erase(held); + } + } + + void restore(const std::string& guid, std::vector ops) { + FxParkIntent* held = find(guid); + if (!held) { + pending_.push_back(FxParkIntent{guid, false, std::move(ops)}); + } else if (held->park) { + erase(held); + } else { + held->restoreOps = std::move(ops); + } + } + + // Enqueue order, which is apply order: park before restore within one + // switch, as the synchronous body already orders them. + const std::vector& pending() const { return pending_; } + bool empty() const { return pending_.empty(); } + void clear() { pending_.clear(); } + +private: + FxParkIntent* find(const std::string& guid) { + for (FxParkIntent& i : pending_) + if (i.guid == guid) return &i; + return nullptr; + } + void erase(FxParkIntent* held) { + pending_.erase(pending_.begin() + (held - pending_.data())); + } + + std::vector pending_; +}; + +// Enqueue against `proj` (nullptr = current project). An enqueue naming a +// different project than the pending intents discards those unapplied. +void deferFxPark(ReaProject* proj, const std::string& guid); +void deferFxRestore(ReaProject* proj, const std::string& guid, std::vector ops); + +// Applies every pending intent, then clears each drained restore's snapshot +// from `model` — the snapshot is consumed when the restore actually lands, not +// when it was planned, so a second switch arriving first re-parks against the +// still-true captured state instead of re-capturing parked values. Discards the +// queue unapplied if the project it was enqueued against is no longer current +// (close / switch); an intent whose track is gone is pruned. Idle cost is one +// empty-queue test. +void drainDeferredFxParks(ViewModeModel& model); + +} // namespace reasampler diff --git a/tests/test_view_fx_park.cpp b/tests/test_view_fx_park.cpp new file mode 100644 index 0000000..edc4e41 --- /dev/null +++ b/tests/test_view_fx_park.cpp @@ -0,0 +1,139 @@ +// Standalone tests for the deferred FX-park queue's re-entrancy rule — no +// REAPER, no test framework. +// +// The property under test: a mode switch leaves its per-FX offline work here, so +// a second switch arriving before the first drained must leave every track in the +// state the SECOND switch specifies — never the first's, never both replayed. + +#include "../src/shell/view/view_fx_park.h" + +#include +#include +#include + +using namespace reasampler; + +static int g_fail = 0; +#define CHECK(cond) do { if(!(cond)) { \ + std::printf("FAIL line %d: %s\n", __LINE__, #cond); ++g_fail; } } while(0) + +// -- helpers ----------------------------------------------------------------- + +// The ops applyMode hands a restore: one per FX captured in the track's snapshot. +static std::vector ops(const std::string& fxGuid, bool offline) { + return {FxOfflineOp{"{TRACK}", FxKeying::Identity, fxGuid, 0, offline}}; +} + +static const FxParkIntent* intentFor(const FxParkQueue& q, const std::string& guid) { + for (const FxParkIntent& i : q.pending()) + if (i.guid == guid) return &i; + return nullptr; +} + +// -- tests ------------------------------------------------------------------- + +static void testParkEnqueuesOneIntentCarryingNoOps() { + FxParkQueue q; + q.park("{A}"); + + CHECK(q.pending().size() == 1); + const FxParkIntent* held = intentFor(q, "{A}"); + CHECK(held != nullptr); + CHECK(held && held->park); + CHECK(held && held->restoreOps.empty()); +} + +static void testRestoreOnAnUndrainedParkCancelsRatherThanStacks() { + // The park never ran, so the track's FX still hold their captured state — + // exactly what the restore would write. Replaying both would unload every + // plugin only to reload it. + FxParkQueue q; + q.park("{A}"); + q.restore("{A}", ops("{FX}", false)); + + CHECK(q.empty()); +} + +static void testParkOnAnUndrainedRestoreCancelsRatherThanStacks() { + // The mirror case: the restore never ran, so the FX are still parked offline, + // which is where the new park wants them. + FxParkQueue q; + q.restore("{A}", ops("{FX}", false)); + q.park("{A}"); + + CHECK(q.empty()); +} + +static void testRepeatedParkStaysOneIntent() { + FxParkQueue q; + q.park("{A}"); + q.park("{A}"); + q.park("{A}"); + + CHECK(q.pending().size() == 1); + CHECK(intentFor(q, "{A}") && intentFor(q, "{A}")->park); +} + +static void testLaterRestoreReplacesTheEarlierOnesOps() { + FxParkQueue q; + q.restore("{A}", ops("{OLD}", false)); + q.restore("{A}", ops("{NEW}", true)); + + CHECK(q.pending().size() == 1); + const FxParkIntent* held = intentFor(q, "{A}"); + CHECK(held && !held->park); + CHECK(held && held->restoreOps.size() == 1); + CHECK(held && held->restoreOps.front().fxGuid == "{NEW}"); + CHECK(held && held->restoreOps.front().offline); +} + +static void testOneTracksCancelLeavesEveryOtherTrackAlone() { + FxParkQueue q; + q.park("{A}"); + q.park("{B}"); + q.park("{C}"); + q.restore("{B}", ops("{FX}", false)); // cancels B only + + CHECK(q.pending().size() == 2); + CHECK(intentFor(q, "{A}") != nullptr); + CHECK(intentFor(q, "{B}") == nullptr); + CHECK(intentFor(q, "{C}") != nullptr); + // Order survives the middle erase: the drain applies in enqueue order. + CHECK(q.pending()[0].guid == "{A}"); + CHECK(q.pending()[1].guid == "{C}"); +} + +static void testCancelledTrackCanBeQueuedAgain() { + // Two rapid switches then a third: the third is the one that must land. + FxParkQueue q; + q.park("{A}"); + q.restore("{A}", ops("{FX}", false)); + q.park("{A}"); + + CHECK(q.pending().size() == 1); + CHECK(intentFor(q, "{A}") && intentFor(q, "{A}")->park); +} + +static void testClearDropsEverythingPending() { + FxParkQueue q; + q.park("{A}"); + q.restore("{B}", ops("{FX}", true)); + q.clear(); + + CHECK(q.empty()); + CHECK(q.pending().empty()); +} + +int main() { + testParkEnqueuesOneIntentCarryingNoOps(); + testRestoreOnAnUndrainedParkCancelsRatherThanStacks(); + testParkOnAnUndrainedRestoreCancelsRatherThanStacks(); + testRepeatedParkStaysOneIntent(); + testLaterRestoreReplacesTheEarlierOnesOps(); + testOneTracksCancelLeavesEveryOtherTrackAlone(); + testCancelledTrackCanBeQueuedAgain(); + testClearDropsEverythingPending(); + + if (g_fail == 0) std::printf("All tests passed.\n"); + return g_fail ? 1 : 0; +} From 7169d7f22b79ed354a6467d0de6d5c9b9b7eee7c Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Mon, 3 Aug 2026 13:35:30 -0400 Subject: [PATCH 2/3] Clear the park snapshot where the restore is planned, not where it drains; make the drain re-entrant and reload-aware --- docs/TODO.md | 2 +- src/app/main.cpp | 18 +++-- src/shell/view/CLAUDE.md | 9 ++- src/shell/view/view.cpp | 94 +++++++++++------------ src/shell/view/view_fx_park.cpp | 57 ++++++++++---- src/shell/view/view_fx_park.h | 79 +++++++++++++++---- tests/test_view_fx_park.cpp | 130 ++++++++++++++++++++++++++++++-- 7 files changed, 298 insertions(+), 91 deletions(-) diff --git a/docs/TODO.md b/docs/TODO.md index 947a354..1585b3d 100644 --- a/docs/TODO.md +++ b/docs/TODO.md @@ -881,7 +881,7 @@ there costs one `target_sources` line in `src/app/CMakeLists.txt` instead. sidestepped), dropping the file under the ~600-line ceiling; `view.cpp`'s own path is unblocked once the build-file ownership question is resolved. -## FX-GUID stability for `restoreFxOffline` is unverified in the DAW +## FX-GUID stability for `applyRestore` is unverified in the DAW **Context.** The Design View park/restore FX keying (`applyRestore`, `src/shell/view/view_fx_park.cpp`) rests on `TrackFX_GetFXGUID` returning an identity that diff --git a/src/app/main.cpp b/src/app/main.cpp index ca7b52b..140ca94 100644 --- a/src/app/main.cpp +++ b/src/app/main.cpp @@ -173,12 +173,6 @@ static void OnTimer() g_session.poll(); - // A mode switch applies its visibility/routing writes synchronously and leaves - // the per-FX offline work here, so the new mode paints before the plugins - // unload. Drained AFTER poll() so a project switch discards the queue instead - // of applying it to the project that replaced it; idle cost is one empty test. - reasampler::drainDeferredFxParks(g_session.view()); - // persist stays MODEL-ONLY (loads the saved view model but does not apply // visibility, to avoid coupling persist to the view shell); poll() raises a // one-shot load signal that we drain here to reapply the SAVED active mode so a @@ -187,12 +181,24 @@ static void OnTimer() // reapply so re-arm and model restore ride the one load event (otherwise // pre-existing tracks can be mis-detected as "new" and mass-tagged). if (g_session.consumeLoadSignal()) { + // Every path that raises this signal — open, project switch, recycled + // pointer, undo/redo state restore — replaced the model the pending FX + // intents were planned against, so they are discarded rather than applied + // to the project that replaced it. This is the queue's ONLY guard against + // a recycled ReaProject*, which a pointer compare cannot see. + reasampler::discardDeferredFxParks(); reasampler::bankPanelNotifyProjectLoaded(); // Reconcile lane ownership against the live project's lanes (P_LANENAME, // the cross-session source of truth) BEFORE reapplying visibility. Never // re-mints, never mass-tags. reasampler::reconcileManagedLanes(g_session.view(), nullptr); reasampler::applyMode(g_session.view(), g_session.view().activeModeId(), nullptr); + } else { + // A mode switch applies its visibility/routing writes synchronously and + // leaves the per-FX offline work here, so the new mode paints before the + // plugins unload. Idle cost is one empty test. Skipped on a load tick so + // the reapply's own intents defer one tick like any other switch's. + reasampler::drainDeferredFxParks(); } reasampler::bankPanelRefresh(); // cheap fingerprint compare; no-op when unchanged/closed diff --git a/src/shell/view/CLAUDE.md b/src/shell/view/CLAUDE.md index e4f719d..bd80068 100644 --- a/src/shell/view/CLAUDE.md +++ b/src/shell/view/CLAUDE.md @@ -55,6 +55,13 @@ decide membership or mode rules. the switch's undo block, so the tool no longer re-drives them on an undo or a redo — what a Ctrl-Z then leaves the chain at is REAPER's own FX-state record, `[verify — DAW]`. +- **Stated DEVIATION — the undo mask does not keep FX out of a real switch.** The + apply mask (`kApplyUndoMask`) drops `UNDO_STATE_FX` and ORs it back in when a + driven flag in that domain moved; the only such flag is `I_FXEN`, which every + park writes. So any switch that parks at least one track still makes REAPER + marshal the project-wide FX chunk into its undo record. The saving is real only + on a reapply and on a no-op switch. Narrowing it further would mean not carrying + `I_FXEN` in the undo record at all, which would break "one switch is one Ctrl-Z." - **Show-both semantics:** a per-track "pin visible across modes" flag re-enables processing whenever shown. A show-both leaf appears in every mode's visible set and is never parked — its driven flags stay at snapshot/restored values, FX @@ -93,7 +100,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 per-FX offline 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), and the idle-tick drain `main.cpp`'s `OnTimer` calls. The drain is also where a consumed snapshot is cleared — not where the restore was planned — so a second switch arriving before the first drained re-parks against the still-true captured state instead of re-capturing parked values. +- `view_fx_park` — the per-FX offline 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), and the idle-tick drain `main.cpp`'s `OnTimer` calls. **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 diff --git a/src/shell/view/view.cpp b/src/shell/view/view.cpp index 4f48614..2bc9466 100644 --- a/src/shell/view/view.cpp +++ b/src/shell/view/view.cpp @@ -31,7 +31,6 @@ #define REAPERAPI_WANT_SetMediaTrackInfo_Value #define REAPERAPI_WANT_GetSetMediaTrackInfo_String #define REAPERAPI_WANT_PreventUIRefresh -#define REAPERAPI_WANT_TrackFX_GetOffline #define REAPERAPI_WANT_Undo_BeginBlock2 #define REAPERAPI_WANT_Undo_EndBlock2 #define REAPERAPI_WANT_TrackList_AdjustWindows @@ -69,6 +68,10 @@ constexpr int kTransportMoving = 1 | 4; // state — a cost that scales with the project's plugin count rather than with // what the switch changed. FX is OR'd back in per apply, only when an FX-domain // flag really moved. +// [verify — DAW] INFERRED, not documented: UNDO_STATE_TRACKCFG reads +// "track/master vol/pan/routing" (reaper_plugin.h:1541) and names neither +// B_SHOWINTCP/B_SHOWINMIXER nor the fixed-lane properties (assumed to ride +// UNDO_STATE_ITEMS, :1543). Wrong ⇒ a Ctrl-Z restores less than the switch did. constexpr int kApplyUndoMask = UNDO_STATE_TRACKCFG | UNDO_STATE_ITEMS | UNDO_STATE_MISCCFG; // Holds REAPER's UI refresh off for the write phase; the deliberate rebuild @@ -117,7 +120,7 @@ const char* flagParm(Flag f) { // The master track is not enumerated by GetTrack (index space excludes it), // so it can never enter the tree — the master-untouched invariant holds by -// construction. Also caches each MediaTrack* by GUID for later resolve(). +// construction. std::vector readFolderEntries( ReaProject* proj, TrackHandles& handleByGuid) { @@ -156,26 +159,25 @@ MediaTrack* resolve(const TrackByGuid& byGuid, const std::string& guid) { // Captures prior driven-flag state before parking. Never reads B_MUTE/I_SOLO; // ints preserve whatever REAPER reported (TrackSnapshot's defensive contract). -TrackSnapshot snapshotTrack(MediaTrack* tr) { +// The FX half is snapshotFxOffline's — only it knows when the live chain has +// stopped being a trustworthy source. +TrackSnapshot snapshotTrack(MediaTrack* tr, const std::vector& cancelledRestore) { TrackSnapshot snap; snap.showInTcp = static_cast(GetMediaTrackInfo_Value(tr, "B_SHOWINTCP")); snap.showInMixer = static_cast(GetMediaTrackInfo_Value(tr, "B_SHOWINMIXER")); snap.mainSend = static_cast(GetMediaTrackInfo_Value(tr, "B_MAINSEND")); snap.fxEnable = static_cast(GetMediaTrackInfo_Value(tr, "I_FXEN")); - const std::vector guids = liveFxGuids(tr); - snap.fxOffline.reserve(guids.size()); - for (std::size_t fx = 0; fx < guids.size(); ++fx) { - snap.fxOffline.push_back( - FxOfflineState{guids[fx], TrackFX_GetOffline(tr, static_cast(fx)) ? 1 : 0}); - } - return snap; // fxKeying stays Identity — a live capture always knows the chain + PreParkFx fx = snapshotFxOffline(tr, cancelledRestore); + snap.fxOffline = std::move(fx.states); + snap.fxKeying = fx.keying; + return snap; } // A reapply (tag/untag, project load) re-plans every flag it already applied, so -// most of what it writes is a value the track already holds; the read is the -// cheap half of the pair, and it also keeps those no-op writes out of the undo -// record's mask. +// most of what it writes is a value the track already holds. The read is ASSUMED +// cheaper than the write it elides (unmeasured); what it certainly does is keep +// those no-op writes out of the undo record's mask. bool writeIfChanged(MediaTrack* tr, const char* parm, double value) { if (GetMediaTrackInfo_Value(tr, parm) == value) return false; SetMediaTrackInfo_Value(tr, parm, value); @@ -191,11 +193,10 @@ void applyFlags(MediaTrack* tr, const std::vector& flags, int& undo } } -// Managed-lane application: the pure planner keys LanePlayOps by the lane's -// DURABLE name; REAPER's C_LANEPLAYS:N is keyed by current ordinal, which -// renumbers on reorder. So every write here re-resolves durable key -> current -// ordinal first. A lane whose name lacks the managed prefix never enters this -// map and so can never be driven. +// Managed-lane application: LanePlayOps are keyed by the lane's DURABLE name but +// C_LANEPLAYS:N by current ordinal, which renumbers on reorder — so every write +// re-resolves durable key -> ordinal first, and a lane whose name lacks the +// managed prefix never enters the map and so can never be driven. // Lane `laneIdx`'s durable name (P_LANENAME:n) on `tr`, or empty if unnamed / // unavailable (non-fixed-lane track). @@ -268,12 +269,10 @@ bool applyLaneOps(const TrackByGuid& handleByGuid, return touchedFreeMode; } -// Managed-lane minting: the DECISION (which tracks split, which lanes, which -// item goes where) is planLaneMinting; this shell only reads live per-item -// mode+lane state, calls it, and applies the resulting writes. +// Managed-lane minting: the DECISION (which tracks split, which lanes, which item +// goes where) is planLaneMinting's; this shell only reads, calls and applies. -// Maps every item GUID on `tr` to its handle in one pass (avoids a per-item -// re-scan in the assign loop). +// One pass, so the assign loop needs no per-item re-scan. std::map itemHandlesByGuid(MediaTrack* tr) { std::map byGuid; const int itemCount = CountTrackMediaItems(tr); @@ -295,9 +294,8 @@ std::string itemModeFromMembership(const ViewModeModel& model, const std::string return *modes.begin(); } -// Builds the per-track LaneItem picture the pure decision consumes. Manual- -// lane reads are skipped on a non-fixed-lane track (isOnManualLane is false -// there regardless of name). +// The per-track LaneItem picture the pure decision consumes. Manual-lane reads +// are skipped on a non-fixed-lane track (isOnManualLane is false there anyway). std::vector readLaneTracks( const ViewModeModel& model, const TrackHandles& handleByGuid) { @@ -329,9 +327,8 @@ std::vector readLaneTracks( return tracks; } -// Idempotent: writes I_FIXEDLANE only when it differs from the item's current -// lane. Non-destructive — only this reversible flag is written, never a move -// in time or across tracks. +// Idempotent, and non-destructive: only this reversible flag is written, never +// a move in time or across tracks. bool assignItemToLane(MediaTrack* tr, MediaItem* it, int laneOrdinal) { const int current = static_cast(GetMediaItemInfo_Value(it, "I_FIXEDLANE")); if (current == laneOrdinal) return false; // already there — no-op @@ -465,28 +462,26 @@ bool applyMode(ViewModeModel& model, const std::string& targetModeId, ReaProject clearTrackSolos(handleByGuid, outgoingSolo); } - // PARK: snapshot before mutating, store into the model, then apply. The - // per-FX offline half is deferred (view_fx_park) — it is the expensive - // half and nothing about the new mode's appearance waits on it. + // PARK: snapshot before mutating. The per-FX offline half is deferred + // (view_fx_park) — the expensive half, and the new mode's appearance + // does not wait on it. for (const TrackPlan& tp : plan.park) { if (tp.flags.empty()) continue; // every op in a TrackPlan targets one track const std::string& guid = tp.flags.front().guid; MediaTrack* tr = resolve(trackByGuid, guid); if (!tr) continue; // stale GUID — prune - // Snapshot ONCE, at first park: a snapshot already present means the - // track is still parked from a prior apply (or its deferred restore - // has not landed yet), so its live flags are the parked values — - // recapturing would overwrite the true pre-park state with zeros and - // a later restore would hide it for good. + // Enqueued FIRST: a park landing on this track's own pending restore + // cancels it, and those ops are then the only surviving record of the + // pre-park FX state. Snapshot ONCE — a snapshot already present means + // the track is still parked, so its live flags read as parked. + const std::vector cancelled = deferFxPark(proj, guid); if (model.snapshot(guid) == nullptr) - model.storeSnapshot(guid, snapshotTrack(tr)); + model.storeSnapshot(guid, snapshotTrack(tr, cancelled)); applyFlags(tr, tp.flags, undoMask); - deferFxPark(proj, guid); } - // RESTORE: flags verbatim now, per-FX offline on the drain, which is also - // where the consumed snapshot is dropped. + // RESTORE: flags verbatim now, per-FX offline on the drain. for (const TrackPlan& tp : plan.restore) { if (tp.flags.empty()) continue; const std::string& guid = tp.flags.front().guid; @@ -495,11 +490,15 @@ bool applyMode(ViewModeModel& model, const std::string& targetModeId, ReaProject applyFlags(tr, tp.flags, undoMask); deferFxRestore(proj, guid, tp.fxOffline); + // Consumed HERE, not on the drain: from this line the flags are back + // at their captured values, and a persist or reapply landing inside + // the drain window must not replan a restore over what the user has + // changed since. + model.clearSnapshot(guid); } - // MANAGED LANES: drive C_LANEPLAYS so the active mode's lane plays+shows - // and every other managed lane is silenced+hidden. Empty for a D1-only - // project, leaving that behavior byte-identical. + // MANAGED LANES: the active mode's lane plays+shows, every other managed + // lane is silenced+hidden. Empty on a D1-only project — byte-identical there. laneModeChanged = applyLaneOps(trackByGuid, plan.lanes); // PARENT VISIBILITY (never parked): recomputed every toggle, never @@ -530,8 +529,8 @@ bool applyMode(ViewModeModel& model, const std::string& targetModeId, ReaProject } // Force REAPER to rebuild the TCP/MCP now rather than on the next user - // interaction: TrackList_AdjustWindows(false) does the full relayout owed - // when tracks appear/disappear; UpdateArrange() repaints. + // interaction — AdjustWindows(false) is the full relayout tracks appearing + // and disappearing owes. TrackList_AdjustWindows(false); UpdateArrange(); @@ -586,7 +585,8 @@ bool mintManagedLanes(ViewModeModel& model, ReaProject* proj) { UpdateArrange(); // Lane minting writes track lane config and item lane assignment and nothing - // else — never an FX state — so it takes the same honest mask applyMode does. + // else — never an FX state — so it takes the same honest mask applyMode does + // (kApplyUndoMask, whose [verify — DAW] on lane-property domain binds here too). Undo_EndBlock2(proj, "ReaSampler: separate cross-mode content into lanes", kApplyUndoMask); return true; } diff --git a/src/shell/view/view_fx_park.cpp b/src/shell/view/view_fx_park.cpp index 95190aa..22f99cf 100644 --- a/src/shell/view/view_fx_park.cpp +++ b/src/shell/view/view_fx_park.cpp @@ -9,7 +9,6 @@ #include #include -#include "core/view/view_mode_model.h" #include "shell/capture/track_guid.h" #define REAPERAPI_MINIMAL @@ -40,9 +39,11 @@ void adoptOwner(ReaProject* proj) { g_owner = p; } -// TrackFX_SetOffline unloads and re-instantiates the plugin, so writing a state -// that already holds is not a no-op — it is the expensive half of a mode switch -// spent on nothing. +// [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; TrackFX_SetOffline(tr, fx, offline); @@ -94,9 +95,24 @@ std::vector liveFxGuids(MediaTrack* tr) { return guids; } -void deferFxPark(ReaProject* proj, const std::string& guid) { +PreParkFx snapshotFxOffline(MediaTrack* tr, const std::vector& cancelled) { + PreParkFx carried = preParkFxFromCancelledRestore(cancelled); + if (!carried.states.empty()) return carried; + + // Nothing was cancelled, so the chain reads true. fxKeying stays Identity — + // a live capture always knows the chain. + PreParkFx live; + const std::vector guids = liveFxGuids(tr); + live.states.reserve(guids.size()); + for (std::size_t fx = 0; fx < guids.size(); ++fx) + live.states.push_back( + FxOfflineState{guids[fx], TrackFX_GetOffline(tr, static_cast(fx)) ? 1 : 0}); + return live; +} + +std::vector deferFxPark(ReaProject* proj, const std::string& guid) { adoptOwner(proj); - g_queue.park(guid); + return g_queue.park(guid); } void deferFxRestore(ReaProject* proj, const std::string& guid, std::vector ops) { @@ -104,23 +120,35 @@ void deferFxRestore(ReaProject* proj, const std::string& guid, std::vector draining = g_queue.take(); + std::unordered_map byGuid; - const int count = CountTracks(g_owner); + const int count = CountTracks(proj); byGuid.reserve(static_cast(count)); for (int i = 0; i < count; ++i) { - MediaTrack* tr = GetTrack(g_owner, i); + MediaTrack* tr = GetTrack(proj, i); if (!tr) continue; std::string guid = guidString(tr); if (!guid.empty()) byGuid.emplace(std::move(guid), tr); @@ -128,7 +156,7 @@ void drainDeferredFxParks(ViewModeModel& model) { FxRestoreDrops drops; int dropTracks = 0; - for (const FxParkIntent& intent : g_queue.pending()) { + for (const FxParkIntent& intent : draining) { auto it = byGuid.find(intent.guid); if (it == byGuid.end()) continue; // track deleted since the switch — prune if (intent.park) { @@ -140,11 +168,8 @@ void drainDeferredFxParks(ViewModeModel& model) { drops.add(d); ++dropTracks; } - model.clearSnapshot(intent.guid); } - g_queue.clear(); - // Captured FX state that could not be applied is REPORTED — silence would read // to the user as "restore worked" while an FX sat at whatever state the park // left it in. The "!SHOW:" prefix (reaper_plugin_functions.h:6536) keeps it from diff --git a/src/shell/view/view_fx_park.h b/src/shell/view/view_fx_park.h index 2377277..ad054f3 100644 --- a/src/shell/view/view_fx_park.h +++ b/src/shell/view/view_fx_park.h @@ -15,8 +15,6 @@ class ReaProject; namespace reasampler { -class ViewModeModel; - // The chain as it stands now: identity by current slot. Snapshot, park and // restore all address FX through this one plain 0..TrackFX_GetCount-1 // enumeration — never the 0x1000000/0x2000000 input-FX or container forms — so @@ -39,15 +37,24 @@ struct FxParkIntent { // observably wrong. class FxParkQueue { public: - void park(const std::string& guid) { + // Returns the ops of a pending restore this park CANCELLED, empty otherwise. + // The caller needs them: that restore never ran, so the live chain still + // reads the PARKED offline states and is no longer a source for a fresh + // pre-park snapshot (see preParkFxFromCancelledRestore). + std::vector park(const std::string& guid) { FxParkIntent* held = find(guid); if (!held) { pending_.push_back(FxParkIntent{guid, true, {}}); - } else if (!held->park) { - erase(held); + return {}; } + if (held->park) return {}; + std::vector cancelled = std::move(held->restoreOps); + erase(held); + return cancelled; } + // A restore cancelling a pending park is COMPLETE at that point — the park + // never ran, so no drain will ever come for this GUID. void restore(const std::string& guid, std::vector ops) { FxParkIntent* held = find(guid); if (!held) { @@ -65,6 +72,17 @@ public: bool empty() const { return pending_.empty(); } void clear() { pending_.clear(); } + // Detaches everything pending, leaving the queue able to accept intents + // enqueued WHILE the caller applies what it took. Applying loads/unloads + // plugins, which pumps the message loop, so a re-entrant switch can enqueue + // mid-apply: iterating the live queue would dangle on the push_back, and + // clearing it afterwards would discard whatever arrived during the apply. + std::vector take() { + std::vector taken; + taken.swap(pending_); + return taken; + } + private: FxParkIntent* find(const std::string& guid) { for (FxParkIntent& i : pending_) @@ -78,18 +96,49 @@ private: std::vector pending_; }; +// The FX half of a snapshot, with the keying it must be read back under. +struct PreParkFx { + std::vector states; + FxKeying keying = FxKeying::Identity; +}; + +// The FX half a fresh pre-park snapshot must carry when the park CANCELLED a +// pending restore: those ops are the only surviving record of the pre-park +// state, because the chain still reads the parked values until that restore +// drains — and it never will, the cancel dropped it. Empty in (nothing was +// cancelled) means the caller reads the live chain instead. The restore's own +// keying travels with it so a slot-keyed snapshot lifted from a legacy +// view_state does not silently become an identity-keyed one with no identities. +inline PreParkFx preParkFxFromCancelledRestore(const std::vector& cancelled) { + PreParkFx out; + if (cancelled.empty()) return out; + out.keying = cancelled.front().keying; + out.states.reserve(cancelled.size()); + for (const FxOfflineOp& op : cancelled) + out.states.push_back(FxOfflineState{op.fxGuid, op.offline ? 1 : 0}); + return out; +} + +// The FX half of a fresh pre-park snapshot for `tr`: whatever the accompanying +// park cancelled, else a live read of the chain. +PreParkFx snapshotFxOffline(MediaTrack* tr, const std::vector& cancelled); + // Enqueue against `proj` (nullptr = current project). An enqueue naming a -// different project than the pending intents discards those unapplied. -void deferFxPark(ReaProject* proj, const std::string& guid); +// different project than the pending intents discards those unapplied. The park +// returns whatever pending restore it cancelled, per FxParkQueue::park. +std::vector deferFxPark(ReaProject* proj, const std::string& guid); void deferFxRestore(ReaProject* proj, const std::string& guid, std::vector ops); -// Applies every pending intent, then clears each drained restore's snapshot -// from `model` — the snapshot is consumed when the restore actually lands, not -// when it was planned, so a second switch arriving first re-parks against the -// still-true captured state instead of re-capturing parked values. Discards the -// queue unapplied if the project it was enqueued against is no longer current -// (close / switch); an intent whose track is gone is pruned. Idle cost is one -// empty-queue test. -void drainDeferredFxParks(ViewModeModel& model); +// 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); +// an intent whose track is gone is pruned. Idle cost is one empty-queue test. +void drainDeferredFxParks(); + +// Drops every pending intent without applying it. Called when the model the +// intents were planned against has been replaced (project load/switch, undo/redo +// state restore) — applying them then would write the pre-reload plan over the +// project that replaced it. +void discardDeferredFxParks(); } // namespace reasampler diff --git a/tests/test_view_fx_park.cpp b/tests/test_view_fx_park.cpp index edc4e41..a132ffd 100644 --- a/tests/test_view_fx_park.cpp +++ b/tests/test_view_fx_park.cpp @@ -1,9 +1,10 @@ -// Standalone tests for the deferred FX-park queue's re-entrancy rule — no -// REAPER, no test framework. +// Standalone tests for the deferred FX-park queue's re-entrancy rule and the +// snapshot-lifecycle contract that rides on it — no REAPER, no test framework. // -// The property under test: a mode switch leaves its per-FX offline work here, so -// a second switch arriving before the first drained must leave every track in the -// state the SECOND switch specifies — never the first's, never both replayed. +// The properties under test: a mode switch leaves its per-FX offline work here, +// so a second switch arriving before the first drained must leave every track in +// the state the SECOND switch specifies — never the first's, never both replayed; +// and a cancel must not strand the pre-park FX state it was the last record of. #include "../src/shell/view/view_fx_park.h" @@ -43,6 +44,17 @@ static void testParkEnqueuesOneIntentCarryingNoOps() { CHECK(held && held->restoreOps.empty()); } +static void testParkReportsNothingCancelledWhenNoIntentWasPending() { + FxParkQueue q; + CHECK(q.park("{A}").empty()); +} + +static void testParkOnItsOwnPendingParkReportsNothingCancelled() { + FxParkQueue q; + q.park("{A}"); + CHECK(q.park("{A}").empty()); +} + static void testRestoreOnAnUndrainedParkCancelsRatherThanStacks() { // The park never ran, so the track's FX still hold their captured state — // exactly what the restore would write. Replaying both would unload every @@ -124,8 +136,109 @@ static void testClearDropsEverythingPending() { CHECK(q.pending().empty()); } +// -- snapshot lifecycle ------------------------------------------------------ + +static void testParkHandsBackTheOpsOfTheRestoreItCancelled() { + // The cancelled restore is the LAST record of the pre-park FX state: the + // track's chain still reads the parked values (the restore never ran), and + // the cancel means no drain will ever put them back. A park that drops these + // snapshots the park's own offline zeros as if they were the user's state. + FxParkQueue q; + q.restore("{A}", ops("{FX}", false)); + + const std::vector cancelled = q.park("{A}"); + + CHECK(cancelled.size() == 1); + CHECK(cancelled.size() == 1 && cancelled.front().fxGuid == "{FX}"); + CHECK(cancelled.size() == 1 && !cancelled.front().offline); + CHECK(q.empty()); // annihilated: the chain already holds what the park wants +} + +static void testCancelledRestoreOpsBecomeTheFreshSnapshotsFxHalf() { + std::vector cancelled = ops("{ONE}", true); + cancelled.push_back(FxOfflineOp{"{TRACK}", FxKeying::Identity, "{TWO}", 1, false}); + + const PreParkFx fx = preParkFxFromCancelledRestore(cancelled); + + CHECK(fx.keying == FxKeying::Identity); + CHECK(fx.states.size() == 2); + CHECK(fx.states.size() == 2 && fx.states[0].fxGuid == "{ONE}" && fx.states[0].offline == 1); + CHECK(fx.states.size() == 2 && fx.states[1].fxGuid == "{TWO}" && fx.states[1].offline == 0); +} + +static void testNothingCancelledLeavesTheFxHalfToTheCaller() { + const PreParkFx fx = preParkFxFromCancelledRestore({}); + + CHECK(fx.states.empty()); // caller reads the live chain instead + CHECK(fx.keying == FxKeying::Identity); +} + +static void testSlotKeyedRestoreDoesNotBecomeIdentityKeyedWithNoIdentities() { + // A snapshot lifted from a pre-identity view_state is slot-keyed and carries + // no fxGuid. Re-labelling it Identity would make resolveFxRestore drop every + // entry as unidentified instead of writing it by slot. + std::vector cancelled = { + FxOfflineOp{"{TRACK}", FxKeying::Slot, "", 0, true}, + FxOfflineOp{"{TRACK}", FxKeying::Slot, "", 1, false}, + }; + + const PreParkFx fx = preParkFxFromCancelledRestore(cancelled); + + CHECK(fx.keying == FxKeying::Slot); + CHECK(fx.states.size() == 2); + CHECK(fx.states.size() == 2 && fx.states[0].offline == 1 && fx.states[1].offline == 0); +} + +// -- re-entrancy ------------------------------------------------------------- + +static void testTakeDetachesEverythingAndLeavesTheQueueEmpty() { + FxParkQueue q; + q.park("{A}"); + q.restore("{B}", ops("{FX}", true)); + + const std::vector taken = q.take(); + + CHECK(taken.size() == 2); + CHECK(taken.size() == 2 && taken[0].guid == "{A}" && taken[0].park); + CHECK(taken.size() == 2 && taken[1].guid == "{B}" && !taken[1].park); + CHECK(q.empty()); +} + +static void testIntentsArrivingDuringADrainSurviveIt() { + // Applying an intent loads/unloads plugins, which pumps the message loop, so + // a switch can re-enter and enqueue mid-drain. Those intents belong to the + // NEXT drain — the one in progress must neither see them nor discard them. + FxParkQueue q; + q.park("{A}"); + + const std::vector draining = q.take(); + q.restore("{B}", ops("{FX}", false)); // arrives while {A} is being applied + + CHECK(draining.size() == 1); + CHECK(draining.size() == 1 && draining.front().guid == "{A}"); + CHECK(q.pending().size() == 1); + CHECK(intentFor(q, "{B}") != nullptr); +} + +static void testAReEntrantParkCancelsOnlyWhatIsStillPending() { + // {A}'s restore was already taken for the in-flight drain, so a park arriving + // mid-drain has nothing to cancel — it must queue as a fresh park rather than + // silently annihilate against an intent that has already been applied. + FxParkQueue q; + q.restore("{A}", ops("{FX}", false)); + q.take(); + + const std::vector cancelled = q.park("{A}"); + + CHECK(cancelled.empty()); + CHECK(q.pending().size() == 1); + CHECK(intentFor(q, "{A}") && intentFor(q, "{A}")->park); +} + int main() { testParkEnqueuesOneIntentCarryingNoOps(); + testParkReportsNothingCancelledWhenNoIntentWasPending(); + testParkOnItsOwnPendingParkReportsNothingCancelled(); testRestoreOnAnUndrainedParkCancelsRatherThanStacks(); testParkOnAnUndrainedRestoreCancelsRatherThanStacks(); testRepeatedParkStaysOneIntent(); @@ -133,6 +246,13 @@ int main() { testOneTracksCancelLeavesEveryOtherTrackAlone(); testCancelledTrackCanBeQueuedAgain(); testClearDropsEverythingPending(); + testParkHandsBackTheOpsOfTheRestoreItCancelled(); + testCancelledRestoreOpsBecomeTheFreshSnapshotsFxHalf(); + testNothingCancelledLeavesTheFxHalfToTheCaller(); + testSlotKeyedRestoreDoesNotBecomeIdentityKeyedWithNoIdentities(); + testTakeDetachesEverythingAndLeavesTheQueueEmpty(); + testIntentsArrivingDuringADrainSurviveIt(); + testAReEntrantParkCancelsOnlyWhatIsStillPending(); if (g_fail == 0) std::printf("All tests passed.\n"); return g_fail ? 1 : 0; From 78e5f06928b1814c6f0c9eb74827fcdbcf4101b0 Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Mon, 3 Aug 2026 14:08:28 -0400 Subject: [PATCH 3/3] Drain deferred FX parks before the view model is serialized; re-validate track handles per intent; pin the restore-plan round trip --- src/app/CMakeLists.txt | 6 ++- src/shell/actions/design_view_actions.cpp | 7 +++ src/shell/capture/render_in_place.cpp | 2 + src/shell/view/CLAUDE.md | 15 ++++++- src/shell/view/view.cpp | 6 +-- src/shell/view/view_fx_park.cpp | 30 ++++++++++++- src/shell/view/view_fx_park.h | 43 ++++++++++++++++-- tests/test_view_fx_park.cpp | 53 +++++++++++++++++++++-- 8 files changed, 147 insertions(+), 15 deletions(-) diff --git a/src/app/CMakeLists.txt b/src/app/CMakeLists.txt index e2473d6..bfae2bd 100644 --- a/src/app/CMakeLists.txt +++ b/src/app/CMakeLists.txt @@ -63,8 +63,10 @@ target_include_directories(reaper_reasampler PRIVATE ${SDK_INC} ${WDL_INC}) # The deferred FX-park queue's re-entrancy rule is pure (header-inline, no REAPER # types), so it is CTest-covered like a core/ module. Declared here rather than in a # src/shell/view/CMakeLists.txt because that directory deliberately has none — its -# TUs are compiled into this target directly. -reasampler_test(view_fx_park LINK fx_offline) +# TUs are compiled into this target directly. view_mode_model is linked for +# makeRestorePlan alone: the cancel path's round trip is a contract between the two, +# and pinning it against hand-built ops would not catch a change to either half. +reasampler_test(view_fx_park LINK fx_offline view_mode_model) # Bank-package import: the promptless verb plus its action skin. Kept as its own # appended block rather than merged into the lists above, so the two package diff --git a/src/shell/actions/design_view_actions.cpp b/src/shell/actions/design_view_actions.cpp index a6f45ca..6b5d914 100644 --- a/src/shell/actions/design_view_actions.cpp +++ b/src/shell/actions/design_view_actions.cpp @@ -26,6 +26,7 @@ #include "shell/capture/track_guid.h" // shared MediaTrack* -> canonical GUID key #include "shell/panel/panel_window.h" // bankPanelInvalidate — footer toggle repaint #include "shell/view/view.h" // applyMode + mintManagedLanes (D2 shell) +#include "shell/view/view_fx_park.h" // drainDeferredFxParks — see persistViewState #define REAPERAPI_MINIMAL #define REAPERAPI_WANT_CountSelectedTracks @@ -138,6 +139,12 @@ std::vector selectedRetagItems() { // the project is unsaved, prompts Save-As first (mirrors the flow capture uses) — // DAW-ONLY: Main_SaveProject(proj, true) blocks until the dialog is dismissed. void persistViewState() { + // BEFORE the model is serialized, and before the Save-As below can write a + // .rpp: a deferred FX restore leaves the chain offline while the model has + // already dropped the snapshot that would replan it. Why that combination is + // unrecoverable on reopen is at drainDeferredFxParks. + drainDeferredFxParks(); + if (!g_session->view().membership().empty()) { ReaProject* proj = EnumProjects(-1, nullptr, 0); if (proj) { diff --git a/src/shell/capture/render_in_place.cpp b/src/shell/capture/render_in_place.cpp index cbe0da0..e0434aa 100644 --- a/src/shell/capture/render_in_place.cpp +++ b/src/shell/capture/render_in_place.cpp @@ -22,6 +22,7 @@ #include "shell/panel/panel_input.h" // bankPanelTailSetting #include "shell/persist/session.h" #include "shell/view/view.h" // applyMode / mintManagedLanes +#include "shell/view/view_fx_park.h" // drainDeferredFxParks #define REAPERAPI_MINIMAL #define REAPERAPI_WANT_CountTrackMediaItems @@ -210,6 +211,7 @@ void RunRenderTrackInPlace(ReaSamplerSession& session) { // Persist outside the block. The offline render's own save gate already forced a // saved project, so the Save-As-guarded persist the Design View actions need // cannot have anything to prompt for here. + drainDeferredFxParks(); // the reapply above may have deferred a restore — see the contract there session.saveToActiveProject(); if (!placed) { diff --git a/src/shell/view/CLAUDE.md b/src/shell/view/CLAUDE.md index bd80068..caa8743 100644 --- a/src/shell/view/CLAUDE.md +++ b/src/shell/view/CLAUDE.md @@ -54,7 +54,14 @@ decide membership or mode rules. lost. What it does change is the undo record: the offline writes land outside the switch's undo block, so the tool no longer re-drives them on an undo or a redo — what a Ctrl-Z then leaves the chain at is REAPER's own FX-state record, - `[verify — DAW]`. + `[verify — DAW]`. **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 + carries the snapshot to replan them — unrecoverable on reopen. So an + action-driven switch does pay the FX hitch before it returns; the repaint and + the undo block have both closed by then, which is what the deferral was for. + Any new caller that reapplies a mode and then persists inherits this obligation. - **Stated DEVIATION — the undo mask does not keep FX out of a real switch.** The apply mask (`kApplyUndoMask`) drops `UNDO_STATE_FX` and ORs it back in when a driven flag in that domain moved; the only such flag is `I_FXEN`, which every @@ -118,6 +125,12 @@ applies the resulting lane state to live tracks. under whatever mode id is active at that point, not the one the user undid back to. Pre-existing: `snapshots_` already carries this same model-vs-undo split; the solo cache inherits it rather than introducing it. Not fixed here. +- An undo/redo also DISCARDS every pending FX intent (`discardDeferredFxParks`), + which is not the pure loss it reads as: the same tick reapplies the active mode + over the reloaded model, re-planning a park for every inactive leaf, so parked + FX converge on the following drain. The one case that does not self-heal is a + track whose reloaded model carries no snapshot — nothing plans its restore, so + FX left offline stay offline. Full contract at `discardDeferredFxParks`. - `fx_offline`'s identity keying (`TrackFX_GetFXGUID`) assumes the GUID stays attached to its plugin across a chain mutation while parked. That is `[verify — DAW]` (see `fxGuidString` in `view_fx_park.cpp`) and SWS issue #802 is a diff --git a/src/shell/view/view.cpp b/src/shell/view/view.cpp index 2bc9466..e06cd3f 100644 --- a/src/shell/view/view.cpp +++ b/src/shell/view/view.cpp @@ -36,8 +36,7 @@ #define REAPERAPI_WANT_TrackList_AdjustWindows #define REAPERAPI_WANT_UpdateArrange #define REAPERAPI_WANT_UpdateTimeline -// Lane minting (D2 Wave 3): item-side lane reads/writes to assign each item to -// its mode's managed lane. +// Lane minting: item-side lane reads/writes assigning each item to its mode's lane. #define REAPERAPI_WANT_CountTrackMediaItems #define REAPERAPI_WANT_GetTrackMediaItem #define REAPERAPI_WANT_GetMediaItemInfo_Value @@ -474,7 +473,8 @@ bool applyMode(ViewModeModel& model, const std::string& targetModeId, ReaProject // Enqueued FIRST: a park landing on this track's own pending restore // cancels it, and those ops are then the only surviving record of the // pre-park FX state. Snapshot ONCE — a snapshot already present means - // the track is still parked, so its live flags read as parked. + // the track is still parked, so recapturing would overwrite the true + // pre-park state with zeros and a later restore would hide it for good. const std::vector cancelled = deferFxPark(proj, guid); if (model.snapshot(guid) == nullptr) model.storeSnapshot(guid, snapshotTrack(tr, cancelled)); diff --git a/src/shell/view/view_fx_park.cpp b/src/shell/view/view_fx_park.cpp index 22f99cf..467a80e 100644 --- a/src/shell/view/view_fx_park.cpp +++ b/src/shell/view/view_fx_park.cpp @@ -20,6 +20,7 @@ #define REAPERAPI_WANT_TrackFX_GetFXGUID #define REAPERAPI_WANT_TrackFX_GetOffline #define REAPERAPI_WANT_TrackFX_SetOffline +#define REAPERAPI_WANT_ValidatePtr2 #define REAPERAPI_WANT_guidToString #include "reaper_plugin_functions.h" @@ -29,6 +30,17 @@ namespace { FxParkQueue g_queue; ReaProject* g_owner = nullptr; // the project the pending intents were enqueued against +bool g_draining = false; + +// Makes "one drain at a time" explicit rather than implied by the call sites. +// RAII because an apply can throw and a stuck flag would silence the queue for +// the rest of the session. +struct DrainScope { + DrainScope() { g_draining = true; } + ~DrainScope() { g_draining = false; } + DrainScope(const DrainScope&) = delete; + DrainScope& operator=(const DrainScope&) = delete; +}; ReaProject* currentProject() { return EnumProjects(-1, nullptr, 0); } @@ -127,6 +139,7 @@ void discardDeferredFxParks() { void drainDeferredFxParks() { if (g_queue.empty()) return; + if (g_draining) return; // re-entered mid-apply: those intents are the outer drain's next pass if (g_owner != currentProject()) { // The project the intents were planned against was closed or switched @@ -139,9 +152,12 @@ void drainDeferredFxParks() { } ReaProject* const proj = g_owner; + const DrainScope scope; - // Detached before the first write: applying pumps the message loop (plugins - // load and unload), so a re-entrant switch can enqueue while this runs. + // Detached before the first write: [verify — DAW] applying is ASSUMED to pump + // the message loop (plugins load and unload), so a re-entrant switch can + // enqueue while this runs. Everything defensive below rests on that one + // assumption; each piece is correct regardless of whether it holds. const std::vector draining = g_queue.take(); std::unordered_map byGuid; @@ -159,6 +175,16 @@ void drainDeferredFxParks() { 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; + if (intent.park) { applyPark(it->second); continue; diff --git a/src/shell/view/view_fx_park.h b/src/shell/view/view_fx_park.h index ad054f3..aebbf6d 100644 --- a/src/shell/view/view_fx_park.h +++ b/src/shell/view/view_fx_park.h @@ -41,6 +41,11 @@ public: // The caller needs them: that restore never ran, so the live chain still // reads the PARKED offline states and is no longer a source for a fresh // pre-park snapshot (see preParkFxFromCancelledRestore). + // + // The empty return is a LOAD-BEARING sentinel that deliberately conflates + // "nothing was cancelled" with "cancelled a restore carrying no ops": a + // zero-op restore held no FX state to hand back, so falling through to a + // live chain read is the same answer, not a worse one. std::vector park(const std::string& guid) { FxParkIntent* held = find(guid); if (!held) { @@ -73,10 +78,12 @@ public: void clear() { pending_.clear(); } // Detaches everything pending, leaving the queue able to accept intents - // enqueued WHILE the caller applies what it took. Applying loads/unloads - // plugins, which pumps the message loop, so a re-entrant switch can enqueue - // mid-apply: iterating the live queue would dangle on the push_back, and - // clearing it afterwards would discard whatever arrived during the apply. + // enqueued WHILE the caller applies what it took. [verify — DAW] applying + // loads/unloads plugins, which is ASSUMED to pump the message loop, so a + // re-entrant switch can enqueue mid-apply: iterating the live queue would + // dangle on the push_back, and clearing it afterwards would discard + // whatever arrived during the apply. The detach is correct either way; only + // the need for it is unconfirmed. std::vector take() { std::vector taken; taken.swap(pending_); @@ -109,6 +116,17 @@ struct PreParkFx { // cancelled) means the caller reads the live chain instead. The restore's own // keying travels with it so a slot-keyed snapshot lifted from a legacy // view_state does not silently become an identity-keyed one with no identities. +// +// FAITHFUL TO THE SNAPSHOT, NOT THE CHAIN. The ops describe the chain as it was +// at the ORIGINAL park; applyPark enumerates it again at drain time. So an FX +// added while the track was parked (a floating FX-chain window, ReaScript) is +// absent from this reconstruction yet IS offlined by the cancelling park's +// drain — and so never comes back online. Rare, and recoverable by hand in the +// FX chain, but specific to the deferral. +// +// `offline` is already boolean by the time it arrives: makeRestorePlan narrowed +// FxOfflineState's defensive int to FxOfflineOp's bool, so this widening back to +// int restores the type, not lost information. inline PreParkFx preParkFxFromCancelledRestore(const std::vector& cancelled) { PreParkFx out; if (cancelled.empty()) return out; @@ -133,12 +151,29 @@ void deferFxRestore(ReaProject* proj, const std::string& guid, std::vector #include #include @@ -189,6 +191,48 @@ static void testSlotKeyedRestoreDoesNotBecomeIdentityKeyedWithNoIdentities() { CHECK(fx.states.size() == 2 && fx.states[0].offline == 1 && fx.states[1].offline == 0); } +// -- the makeRestorePlan <-> preParkFxFromCancelledRestore round trip --------- +// +// The cancel path's whole premise is that a planned restore's ops are a LOSSLESS +// carrier of the snapshot's FX half. makeRestorePlan is their only producer, so +// the real claim is that the pair composes to the identity on (fxOffline, +// fxKeying). Asserting it against hand-built ops would let a change to +// makeRestorePlan's field mapping or op ordering pass with every test green. + +static void testRestorePlanOpsRebuildTheIdentityKeyedSnapshotVerbatim() { + TrackSnapshot snap; + snap.fxKeying = FxKeying::Identity; + snap.fxOffline = {FxOfflineState{"{ONE}", 1}, FxOfflineState{"{TWO}", 0}, + FxOfflineState{"{THREE}", 1}}; + + const TrackPlan plan = makeRestorePlan("{TRACK}", snap); + const PreParkFx rebuilt = preParkFxFromCancelledRestore(plan.fxOffline); + + CHECK(rebuilt.keying == FxKeying::Identity); + // Three DISTINCT entries, compared as a sequence: a dropped fxGuid, a flipped + // offline, or a reordering each fail here. + CHECK(rebuilt.states == snap.fxOffline); +} + +static void testRestorePlanOpsRebuildTheSlotKeyedSnapshotVerbatim() { + TrackSnapshot snap; + snap.fxKeying = FxKeying::Slot; + snap.fxOffline = {FxOfflineState{"", 0}, FxOfflineState{"", 1}, FxOfflineState{"", 1}}; + + const TrackPlan plan = makeRestorePlan("{TRACK}", snap); + + // Slot keying addresses by POSITION, so the identity holds only while the op + // at index i carries slot i. + CHECK(plan.fxOffline.size() == 3); + CHECK(plan.fxOffline.size() == 3 && plan.fxOffline[0].slot == 0 && + plan.fxOffline[1].slot == 1 && plan.fxOffline[2].slot == 2); + + const PreParkFx rebuilt = preParkFxFromCancelledRestore(plan.fxOffline); + + CHECK(rebuilt.keying == FxKeying::Slot); + CHECK(rebuilt.states == snap.fxOffline); +} + // -- re-entrancy ------------------------------------------------------------- static void testTakeDetachesEverythingAndLeavesTheQueueEmpty() { @@ -205,9 +249,10 @@ static void testTakeDetachesEverythingAndLeavesTheQueueEmpty() { } static void testIntentsArrivingDuringADrainSurviveIt() { - // Applying an intent loads/unloads plugins, which pumps the message loop, so - // a switch can re-enter and enqueue mid-drain. Those intents belong to the - // NEXT drain — the one in progress must neither see them nor discard them. + // [verify — DAW] applying an intent loads/unloads plugins, which is ASSUMED to + // pump the message loop, so a switch can re-enter and enqueue mid-drain. Those + // intents belong to the NEXT drain — the one in progress must neither see them + // nor discard them. FxParkQueue q; q.park("{A}"); @@ -250,6 +295,8 @@ int main() { testCancelledRestoreOpsBecomeTheFreshSnapshotsFxHalf(); testNothingCancelledLeavesTheFxHalfToTheCaller(); testSlotKeyedRestoreDoesNotBecomeIdentityKeyedWithNoIdentities(); + testRestorePlanOpsRebuildTheIdentityKeyedSnapshotVerbatim(); + testRestorePlanOpsRebuildTheSlotKeyedSnapshotVerbatim(); testTakeDetachesEverythingAndLeavesTheQueueEmpty(); testIntentsArrivingDuringADrainSurviveIt(); testAReEntrantParkCancelsOnlyWhatIsStillPending();