From b064081a3cca106a9cf2a74c379fb59088d04251 Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Thu, 6 Aug 2026 16:50:43 -0400 Subject: [PATCH] Fix five Wave-1 pure-layer FX/park review findings: resume-point stall, stale doc comments, implicit mode read, keying reset --- src/core/view/applied_state.h | 12 ++++--- src/core/view/fx_state.cpp | 31 ++++++++++------- src/core/view/fx_state.h | 28 +++++++++++++--- src/core/view/view_mode_model.cpp | 4 ++- tests/test_applied_state.cpp | 9 +++-- tests/test_fx_state.cpp | 55 ++++++++++++++++++++++--------- 6 files changed, 101 insertions(+), 38 deletions(-) diff --git a/src/core/view/applied_state.h b/src/core/view/applied_state.h index 5194f22..21221ae 100644 --- a/src/core/view/applied_state.h +++ b/src/core/view/applied_state.h @@ -1,9 +1,13 @@ #pragma once // A comparable fingerprint of what an apply put into the project, so a reconcile -// can ask "does the model differ from what we applied?" rather than "did an undo -// happen?". The second question is not the same one: an undo that touched -// nothing we drive still answers yes, and repairing on it re-enables state the -// user disabled by hand. +// can ask "did our applied output drift from what a restore would drive it back +// to?" rather than "did an undo happen?". The second question is not the same +// one: an undo that touched nothing we drive still answers yes, and repairing on +// it re-enables state the user disabled by hand. Narrower still than "did the +// model change?": membership, lane ownership and the solo cache are excluded by +// design (see AppliedState below), so an edit to any of those changes what a +// future apply must drive while leaving this fingerprint bit-identical. Not a +// sole reapply trigger for anything membership can affect. #include #include diff --git a/src/core/view/fx_state.cpp b/src/core/view/fx_state.cpp index 986fae0..00e4073 100644 --- a/src/core/view/fx_state.cpp +++ b/src/core/view/fx_state.cpp @@ -7,15 +7,22 @@ namespace reasampler::view { namespace { -// Everything up to and including the first op naming `resumeAfter` is already -// done. Not found ⇒ nothing dropped: the identity left the chain between ticks, -// and re-walking a compare-before-write list beats trusting a cursor that no -// longer points at anything. +// Everything up to and including the resume point is already done. An empty +// `resumeAfterFxGuid` is ambiguous by itself — "nothing done yet" and "the last +// completed op had no identity" look identical — so an empty guid falls back to +// matching by slot instead; `resumeAfterSlot < 0` is what actually means +// "nothing done yet". Not found ⇒ nothing dropped: the resume point left the +// chain between ticks, and re-walking a compare-before-write list beats +// trusting a cursor that no longer points at anything. std::vector remainingAfter(const std::vector& ops, - const std::string& resumeAfter) { - if (resumeAfter.empty()) return ops; + const std::string& resumeAfterFxGuid, + int resumeAfterSlot) { + if (resumeAfterFxGuid.empty() && resumeAfterSlot < 0) return ops; for (std::size_t i = 0; i < ops.size(); ++i) { - if (ops[i].fxGuid == resumeAfter) + const bool isResumePoint = resumeAfterFxGuid.empty() + ? ops[i].slot == resumeAfterSlot + : ops[i].fxGuid == resumeAfterFxGuid; + if (isResumePoint) return std::vector(ops.begin() + static_cast(i) + 1, ops.end()); } @@ -54,15 +61,15 @@ std::vector fxRestoreTargets(const ViewModeModel& model, } std::vector desiredFxState(const ViewModeModel& model, + const std::string& targetMode, const std::set& liveTrackGuids) { std::vector online; std::vector offline; - const std::string& activeMode = model.activeModeId(); for (const auto& [guid, snap] : model.snapshots()) { // std::map ⇒ GUID order if (liveTrackGuids.count(guid) == 0) continue; // dead track — nothing to write - if (!model.leafBelongsToMode(guid, activeMode)) { + if (!model.leafBelongsToMode(guid, targetMode)) { offline.push_back(FxTrackWork{guid, FxWorkKind::Offline, {}, {}}); continue; } @@ -82,10 +89,12 @@ FxRestoreResolution resolveFxWork(const FxTrackWork& work, const std::vector& liveFxGuids) { if (work.kind == FxWorkKind::Offline) { return resolveFxRestore( - remainingAfter(offlineTargets(work.trackGuid, liveFxGuids), work.resumeAfterFxGuid), + remainingAfter(offlineTargets(work.trackGuid, liveFxGuids), work.resumeAfterFxGuid, + work.resumeAfterSlot), liveFxGuids); } - return resolveFxRestore(remainingAfter(work.targets, work.resumeAfterFxGuid), liveFxGuids); + return resolveFxRestore( + remainingAfter(work.targets, work.resumeAfterFxGuid, work.resumeAfterSlot), liveFxGuids); } } // namespace reasampler::view diff --git a/src/core/view/fx_state.h b/src/core/view/fx_state.h index 849835c..c74e242 100644 --- a/src/core/view/fx_state.h +++ b/src/core/view/fx_state.h @@ -32,14 +32,28 @@ struct FxTrackWork { // read it, so resolveFxWork synthesises the targets from what it is handed. std::vector targets; - // Identity of the last FX this item's drain completed; empty ⇒ none yet. An - // optimisation only: every write is compare-before-write, so an identity that - // has since left the chain safely restarts the item rather than stranding it. + // Identity of the last FX this item's drain completed; empty means EITHER + // nothing has completed yet OR the completed FX itself carried no identity + // (REAPER reports no GUID for it, or it came from a Slot-keyed record) — see + // resumeAfterSlot below, which disambiguates the two. This is load-bearing + // for forward progress, not an optimisation: without the disambiguation, an + // unidentified FX would look like "none yet" and the drain would restart at + // the top of the list every tick, forever. Compare-before-write makes every + // write idempotent, so an identity that has since left the chain safely + // restarts the item rather than stranding it. std::string resumeAfterFxGuid; + // Position of the last completed op, consulted only when resumeAfterFxGuid + // is empty, to tell "resumed past an unidentified FX at this slot" apart + // from "nothing done yet" (-1). Every FxOfflineOp carries its slot + // regardless of keying, so this is available for both Online and Offline + // work. + int resumeAfterSlot = -1; + bool operator==(const FxTrackWork& o) const { return trackGuid == o.trackGuid && kind == o.kind && targets == o.targets && - resumeAfterFxGuid == o.resumeAfterFxGuid; + resumeAfterFxGuid == o.resumeAfterFxGuid && + resumeAfterSlot == o.resumeAfterSlot; } }; @@ -51,7 +65,13 @@ struct FxTrackWork { // opportunistic reclaim that may be deferred indefinitely without losing // anything. Within a kind, GUID order — deterministic, so a drain's position in // the list survives a re-derivation. +// +// `targetMode` is explicit, mirroring planToggle, rather than read from +// `model.activeModeId()`: applyMode sets the active mode LAST, after its park +// and restore loops, so a caller reading it from inside or before that block +// would classify every track against the OUTGOING mode instead. std::vector desiredFxState(const ViewModeModel& model, + const std::string& targetMode, const std::set& liveTrackGuids); // One track's Online targets from its park record's FX half; empty when the diff --git a/src/core/view/view_mode_model.cpp b/src/core/view/view_mode_model.cpp index 5537e9c..1e2ea88 100644 --- a/src/core/view/view_mode_model.cpp +++ b/src/core/view/view_mode_model.cpp @@ -278,7 +278,9 @@ void ViewModeModel::releaseSnapshotFx(const std::string& guid) { if (it == snapshots_.end()) return; it->second.fxHeld = false; it->second.fxOffline.clear(); - it->second.fxKeying = FxKeying::Identity; + // fxKeying is left as captured: the record's own keying, not a claim about + // one it never had. Harmless today (fxOffline is empty right above), but + // whoever reads fxKeying after this point should see the truth. eraseIfSpent(snapshots_, it); } diff --git a/tests/test_applied_state.cpp b/tests/test_applied_state.cpp index ccf0c52..e36dceb 100644 --- a/tests/test_applied_state.cpp +++ b/tests/test_applied_state.cpp @@ -102,10 +102,15 @@ static void testEditsTheApplyDidNotDriveAreNotDifferences() { ViewModeModel vm = parkedPair(); const AppliedState before = appliedState(vm); - vm.membership().tag("{OTHER}", kDesignModeId); + // {T1} is already parked (tagged Design, active mode Arrange); retagging it + // INTO the active mode changes what the next apply must drive (release it) + // while its park record — and so the fingerprint — is untouched. Pinning + // this on a GUID actually in the parked set is the point: an unrelated GUID + // would pass even if membership were folded into the fingerprint by mistake. + vm.membership().tag("{T1}", kArrangeModeId); CHECK(appliedState(vm) == before); - vm.membership().setShowBoth("{OTHER}", true); + vm.membership().setShowBoth("{T1}", true); CHECK(appliedState(vm) == before); vm.lanes().setManaged("{T1}", "reasampler:design", kDesignModeId); diff --git a/tests/test_fx_state.cpp b/tests/test_fx_state.cpp index 06e8964..c20dadc 100644 --- a/tests/test_fx_state.cpp +++ b/tests/test_fx_state.cpp @@ -63,7 +63,7 @@ static void testActiveTrackIsOwedOnlineInactiveIsOwedOffline() { vm.storeSnapshot("{P}", recordWithFx({{"{FX-P}", 0}})); // Active mode is Arrange: {T} (Design) stays parked, {P} is coming back. - const std::vector work = desiredFxState(vm, {"{T}", "{P}"}); + const std::vector work = desiredFxState(vm, kArrangeModeId, {"{T}", "{P}"}); CHECK(work.size() == 2); CHECK(workFor(work, "{P}") && workFor(work, "{P}")->kind == FxWorkKind::Online); CHECK(workFor(work, "{T}") && workFor(work, "{T}")->kind == FxWorkKind::Offline); @@ -86,7 +86,7 @@ static void testOnlineWorkSortsBeforeOfflineWork() { vm.storeSnapshot(g, recordWithFx({{"{FX}", 0}})); const std::vector work = - desiredFxState(vm, {"{A-park}", "{B-park}", "{Y-back}", "{Z-back}"}); + desiredFxState(vm, kArrangeModeId, {"{A-park}", "{B-park}", "{Y-back}", "{Z-back}"}); CHECK(work.size() == 4); // Onlining restores state the user owns and is 2-6x the cost of offlining; // offlining is opportunistic reclaim and may be deferred indefinitely. @@ -97,7 +97,8 @@ static void testOnlineWorkSortsBeforeOfflineWork() { // Deterministic: a re-derivation yields the identical list, so a drain's // position in it survives one. - CHECK(desiredFxState(vm, {"{A-park}", "{B-park}", "{Y-back}", "{Z-back}"}) == work); + CHECK(desiredFxState(vm, kArrangeModeId, {"{A-park}", "{B-park}", "{Y-back}", "{Z-back}"}) == + work); } static void testUserOfflinedPluginIsNeverBlanketOnlined() { @@ -106,7 +107,7 @@ static void testUserOfflinedPluginIsNeverBlanketOnlined() { ViewModeModel vm = parkedWithThreeFx(); CHECK(vm.setActiveMode(kDesignModeId)); - const std::vector work = desiredFxState(vm, {"{T}"}); + const std::vector work = desiredFxState(vm, kDesignModeId, {"{T}"}); CHECK(work.size() == 1); CHECK(work[0].kind == FxWorkKind::Online); @@ -124,14 +125,14 @@ static void testReleasedFxHalfIsOwedNothing() { CHECK(vm.setActiveMode(kDesignModeId)); vm.releaseSnapshotFx("{T}"); - CHECK(desiredFxState(vm, {"{T}"}).empty()); + CHECK(desiredFxState(vm, kDesignModeId, {"{T}"}).empty()); CHECK(fxRestoreTargets(vm, "{T}").empty()); // ...and the mirror: a record whose FLAG half is spent still owes its FX. ViewModeModel vm2 = parkedWithThreeFx(); CHECK(vm2.setActiveMode(kDesignModeId)); vm2.releaseSnapshotFlags("{T}"); - const std::vector work = desiredFxState(vm2, {"{T}"}); + const std::vector work = desiredFxState(vm2, kDesignModeId, {"{T}"}); CHECK(work.size() == 1); CHECK(work.size() == 1 && work[0].targets.size() == 3); } @@ -139,20 +140,20 @@ static void testReleasedFxHalfIsOwedNothing() { static void testDeadAndUnparkedTracksAreOwedNothing() { ViewModeModel vm = parkedWithThreeFx(); // The track is gone from the project: there is nothing to write to. - CHECK(desiredFxState(vm, {}).empty()); + CHECK(desiredFxState(vm, kArrangeModeId, {}).empty()); // A track with no park record is owed nothing either way — that is also how a // REFUSED park stays refused: it wrote no record, so no reclaim targets it. ViewModeModel bare; bare.membership().tag("{U}", kDesignModeId); - CHECK(desiredFxState(bare, {"{U}"}).empty()); + CHECK(desiredFxState(bare, kArrangeModeId, {"{U}"}).empty()); } // -- resolveFxWork: against the chain as it stands ---------------------------- static void testOfflineWorkTargetsEveryLiveSlot() { ViewModeModel vm = parkedWithThreeFx(); // active mode Arrange ⇒ {T} parks - const std::vector work = desiredFxState(vm, {"{T}"}); + const std::vector work = desiredFxState(vm, kArrangeModeId, {"{T}"}); CHECK(work.size() == 1); if (work.empty()) return; @@ -168,7 +169,7 @@ static void testOfflineWorkTargetsEveryLiveSlot() { static void testReorderRestoreLandsEachPluginItsOwnState() { ViewModeModel vm = parkedWithThreeFx(); CHECK(vm.setActiveMode(kDesignModeId)); - const std::vector work = desiredFxState(vm, {"{T}"}); + const std::vector work = desiredFxState(vm, kDesignModeId, {"{T}"}); CHECK(work.size() == 1); if (work.empty()) return; @@ -184,7 +185,7 @@ static void testReorderRestoreLandsEachPluginItsOwnState() { static void testDeletedFxIsDroppedExplicitly() { ViewModeModel vm = parkedWithThreeFx(); CHECK(vm.setActiveMode(kDesignModeId)); - const std::vector work = desiredFxState(vm, {"{T}"}); + const std::vector work = desiredFxState(vm, kDesignModeId, {"{T}"}); CHECK(work.size() == 1); if (work.empty()) return; @@ -200,7 +201,7 @@ static void testDeletedFxIsDroppedExplicitly() { static void testAddedFxIsLeftAlone() { ViewModeModel vm = parkedWithThreeFx(); CHECK(vm.setActiveMode(kDesignModeId)); - const std::vector work = desiredFxState(vm, {"{T}"}); + const std::vector work = desiredFxState(vm, kDesignModeId, {"{T}"}); CHECK(work.size() == 1); if (work.empty()) return; @@ -228,7 +229,7 @@ static void testSlotKeyedRecordStillRestoresByPosition() { CHECK(targets.size() == 3); CHECK(!targets.empty() && targets[0].keying == FxKeying::Slot); - const std::vector work = desiredFxState(vm, {"{T}"}); + const std::vector work = desiredFxState(vm, kDesignModeId, {"{T}"}); CHECK(work.size() == 1); if (work.empty()) return; const FxRestoreResolution res = resolveFxWork(work[0], {"{FX-X}", "{FX-Y}", "{FX-Z}"}); @@ -244,7 +245,7 @@ static void testSlotKeyedRecordStillRestoresByPosition() { static void testResumePointSkipsWhatTheDrainAlreadyDid() { ViewModeModel vm = parkedWithThreeFx(); CHECK(vm.setActiveMode(kDesignModeId)); - std::vector work = desiredFxState(vm, {"{T}"}); + std::vector work = desiredFxState(vm, kDesignModeId, {"{T}"}); CHECK(work.size() == 1); if (work.empty()) return; @@ -263,7 +264,7 @@ static void testResumePointSkipsWhatTheDrainAlreadyDid() { static void testResumePointThatLeftTheChainRestartsRatherThanStrands() { ViewModeModel vm = parkedWithThreeFx(); CHECK(vm.setActiveMode(kDesignModeId)); - std::vector work = desiredFxState(vm, {"{T}"}); + std::vector work = desiredFxState(vm, kDesignModeId, {"{T}"}); CHECK(work.size() == 1); if (work.empty()) return; @@ -277,7 +278,7 @@ static void testResumePointThatLeftTheChainRestartsRatherThanStrands() { // Same rule on the offline side, where the cursor keys into the live chain. ViewModeModel parked = parkedWithThreeFx(); // Arrange active ⇒ {T} parks - std::vector offlineWork = desiredFxState(parked, {"{T}"}); + std::vector offlineWork = desiredFxState(parked, kArrangeModeId, {"{T}"}); CHECK(offlineWork.size() == 1); if (offlineWork.empty()) return; offlineWork[0].resumeAfterFxGuid = "{FX-A}"; @@ -286,6 +287,27 @@ static void testResumePointThatLeftTheChainRestartsRatherThanStrands() { CHECK(resolveFxWork(offlineWork[0], {"{FX-A}", "{FX-B}", "{FX-C}"}).writes.size() == 3); } +static void testResumeAfterUnidentifiedFxMakesForwardProgress() { + // The drain completed slot 1, an FX REAPER reports no GUID for. Before the + // slot-cursor fix, an empty resumeAfterFxGuid was indistinguishable from + // "nothing done yet" and this item would restart at slot 0 every tick, + // forever — the non-terminating drain this test pins against. + ViewModeModel parked = parkedWithThreeFx(); // Arrange active ⇒ {T} parks (Offline, slot-keyed) + std::vector work = desiredFxState(parked, kArrangeModeId, {"{T}"}); + CHECK(work.size() == 1); + if (work.empty()) return; + + work[0].resumeAfterFxGuid = ""; + work[0].resumeAfterSlot = 1; + const FxRestoreResolution res = + resolveFxWork(work[0], {"{FX-A}", "", "{FX-B}", "{FX-C}"}); + CHECK(res.writes.size() == 2); // slots 2 and 3 only — 0 and 1 already done + CHECK(!anyWriteAt(res, 0)); + CHECK(!anyWriteAt(res, 1)); + CHECK(anyWriteAt(res, 2)); + CHECK(anyWriteAt(res, 3)); +} + int main() { testActiveTrackIsOwedOnlineInactiveIsOwedOffline(); testOnlineWorkSortsBeforeOfflineWork(); @@ -301,6 +323,7 @@ int main() { testResumePointSkipsWhatTheDrainAlreadyDid(); testResumePointThatLeftTheChainRestartsRatherThanStrands(); + testResumeAfterUnidentifiedFxMakesForwardProgress(); if (g_fail == 0) std::printf("All tests passed.\n"); return g_fail ? 1 : 0;