From 31754216c1a6f2e82a621370c68a0c5174835996 Mon Sep 17 00:00:00 2001 From: David Rowland Date: Thu, 17 Sep 2026 11:17:31 +0100 Subject: [PATCH] Clips: Stopped AudioClipBase's constructor making undoable writes (fixes #415) A clip can be constructed whilst an undo is in progress - undoing a record or a clip deletion re-adds the clip's state, which rebuilds the Clip from valueTreeChildAdded inside the undo transaction. Any write made through the Edit's UndoManager at that point re-enters UndoManager::perform, which refuses it, asserts, and silently discards the change. CachedValue still updates its cached copy, so the object and the state then disagree until the Edit is reloaded. Made the constructor's corrections and migrations non-undoable: - checkFadeLengthsForOverrun() now clamps fadeIn/fadeOut with a null UndoManager; it's derived geometry, not user data, and setFadeIn/setFadeOut still do their own clamping on the user-facing paths - the pan limit, the legacy timeStretch migration and TimeStretcher::checkModeIsAvailable() likewise - the LOOPINFO child is created with a null UndoManager, so a refused perform() can't leave LoopInfo referring to a detached tree Co-Authored-By: Claude Opus 5 --- .../model/clips/tracktion_AudioClipBase.cpp | 22 +++++-- .../model/clips/tracktion_Clip.cpp | 61 +++++++++++++++++++ 2 files changed, 77 insertions(+), 6 deletions(-) diff --git a/modules/tracktion_engine/model/clips/tracktion_AudioClipBase.cpp b/modules/tracktion_engine/model/clips/tracktion_AudioClipBase.cpp index b13f36e7ed3..f70dd440012 100644 --- a/modules/tracktion_engine/model/clips/tracktion_AudioClipBase.cpp +++ b/modules/tracktion_engine/model/clips/tracktion_AudioClipBase.cpp @@ -205,10 +205,16 @@ class ProxyGeneratorJob : public AudioProxyGenerator::GeneratorJob //============================================================================== AudioClipBase::AudioClipBase (const juce::ValueTree& v, EditItemID id, Type t, ClipOwner& targetParent) : Clip (v, targetParent, id, t), - loopInfo (edit.engine, state.getOrCreateChildWithName (IDs::LOOPINFO, getUndoManager()), getUndoManager()), + loopInfo (edit.engine, state.getOrCreateChildWithName (IDs::LOOPINFO, nullptr), getUndoManager()), pluginList (edit), lastProxy (edit.engine) { + // N.B. Nothing in this constructor may write to the state via the UndoManager. + // A clip can be constructed whilst an undo is in progress (undoing a record or a + // clip deletion re-adds the clip's state), and UndoManager::perform refuses to + // re-enter, so any undoable write made here is silently discarded. + // Corrections to derived geometry and old-Edit migrations are made with a null + // UndoManager instead. auto um = getUndoManager(); level->dbGain.referTo (state, IDs::gain, um); @@ -245,9 +251,9 @@ AudioClipBase::AudioClipBase (const juce::ValueTree& v, EditItemID id, Type t, C // Keep this in to handle old edits.. if (state.getProperty (IDs::timeStretch)) - timeStretchMode = juce::VariantConverter::fromVar (state.getProperty (IDs::stretchMode)); + timeStretchMode.setValue (juce::VariantConverter::fromVar (state.getProperty (IDs::stretchMode)), nullptr); - timeStretchMode = TimeStretcher::checkModeIsAvailable (timeStretchMode); + timeStretchMode.setValue (TimeStretcher::checkModeIsAvailable (timeStretchMode), nullptr); autoPitch.referTo (state, IDs::autoPitch, um); autoPitchMode.referTo (state, IDs::autoPitchMode, um); @@ -256,7 +262,7 @@ AudioClipBase::AudioClipBase (const juce::ValueTree& v, EditItemID id, Type t, C isReversed.referTo (state, IDs::isReversed, um); autoDetectBeats.referTo (state, IDs::autoDetectBeats, um); - level->pan = juce::jlimit (-1.0f, 1.0f, static_cast (level->pan.get())); + level->pan.setValue (juce::jlimit (-1.0f, 1.0f, static_cast (level->pan.get())), nullptr); checkFadeLengthsForOverrun(); useClipLaunchQuantisation.referTo (state, IDs::useClipLaunchQuantisation, um); @@ -750,8 +756,12 @@ void AudioClipBase::checkFadeLengthsForOverrun() if (fadeIn + fadeOut > len) { const double scale = len / (fadeIn + fadeOut); - fadeIn = fadeIn * scale; - fadeOut = fadeOut * scale; + + // This is derived geometry, not user data, and it can be corrected whilst a clip is + // being constructed. If that happens during an undo, an undoable write would re-enter + // UndoManager::perform, which refuses it and silently discards the correction. + fadeIn.setValue (fadeIn * scale, nullptr); + fadeOut.setValue (fadeOut * scale, nullptr); } // also check the auto fades diff --git a/modules/tracktion_engine/model/clips/tracktion_Clip.cpp b/modules/tracktion_engine/model/clips/tracktion_Clip.cpp index e7f1e729e78..8fa7c6bf07d 100644 --- a/modules/tracktion_engine/model/clips/tracktion_Clip.cpp +++ b/modules/tracktion_engine/model/clips/tracktion_Clip.cpp @@ -879,6 +879,67 @@ TEST_SUITE("tracktion_engine") CHECK (clip->getPosition().time == positionBeforeTrim.time); } } + + TEST_CASE("Fade length correction isn't undoable") + { + auto& engine = *tracktion::engine::Engine::getEngines()[0]; + auto edit = Edit::createSingleTrackEdit (engine); + auto track = getAudioTracks (*edit)[0]; + auto& undoManager = edit->getUndoManager(); + + auto clip = dynamic_cast (insertNewClip (*track, TrackItem::Type::wave, { 2_tp, 3_tp })); + REQUIRE (clip != nullptr); + + // A comp remnant can be left with fades that are longer than the clip itself + const auto length = TimeDuration::fromSeconds (0.00999999999999979); + auto clipState = clip->state; + clipState.setProperty (IDs::length, length.inSeconds(), nullptr); + clipState.setProperty (IDs::fadeIn, 0.0, nullptr); + clipState.setProperty (IDs::fadeOut, 0.02, nullptr); + + SUBCASE ("Correcting the fades during construction isn't undoable") + { + track->state.removeChild (clipState, nullptr); + undoManager.clearUndoHistory(); + undoManager.beginNewTransaction(); + + track->state.addChild (clipState, -1, nullptr); + const double correctedFadeOut = clipState[IDs::fadeOut]; + CHECK (correctedFadeOut == doctest::Approx (length.inSeconds())); + + // Undoing back past the clip's construction mustn't revert the correction + while (undoManager.canUndo()) + undoManager.undo(); + + CHECK (double (clipState[IDs::fadeOut]) == doctest::Approx (correctedFadeOut)); + } + + SUBCASE ("Fades are corrected when a clip is rebuilt during an undo") + { + undoManager.clearUndoHistory(); + undoManager.beginNewTransaction(); + clip->removeFromParent(); + REQUIRE (track->getClips().isEmpty()); + + undoManager.beginNewTransaction(); + undoManager.undo(); + + REQUIRE (track->getClips().size() == 1); + auto rebuiltClip = dynamic_cast (track->getClips()[0]); + REQUIRE (rebuiltClip != nullptr); + CHECK (rebuiltClip->getFadeIn() + rebuiltClip->getFadeOut() <= length); + + // The correction has to reach the state, not just the CachedValue, or it's + // lost the next time the Edit is loaded + const double fadeInInState = rebuiltClip->state[IDs::fadeIn]; + const double fadeOutInState = rebuiltClip->state[IDs::fadeOut]; + CHECK (fadeInInState + fadeOutInState <= length.inSeconds()); + CHECK (fadeOutInState == doctest::Approx (length.inSeconds())); + + // The undo history must still be intact + CHECK (undoManager.canRedo()); + } + } } } // namespace tracktion::inline engine