From a4a1c3860f2f858e7b834cf918fb2b90a1de8116 Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Mon, 3 Aug 2026 12:39:22 -0400 Subject: [PATCH] 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; +}