AudioProcessorValueTreeState & UndoManager usage

Not really sure where you’re getting that idea from… There are lots of solid commercial field-tested plugins out there using it, including our own Equator, which I know has been heavily used by many thousands of people for a couple of years, and which has undo/redo.

UndoManager is very mature. I’m explicitly mentioning AudioProcessorValueTreeState usage of it.

I’ve made now another test on my personal machine with clean JUCE 5 (master 7e959).

Plain “plug-in” with only gain slider and undo redo buttons.
For the sake of keeping it short I’m only showing the important editor parts.

That’s my constructor:

addAndMakeVisible(undoBtn);
addAndMakeVisible(redoBtn);
addAndMakeVisible(gainSlider);

undoBtn.addListener(this);
redoBtn.addListener(this);

gainAttach = new AudioProcessorValueTreeState::SliderAttachment(p.params,"gain",gainSlider);

setSize (400, 200);

That’s my a simple listener for the buttons:

void AudioProcessorValueTreeUndoAudioProcessorEditor::buttonClicked (Button* btn)
{
    UndoManager* undoMgt = processor.params.undoManager;

    if (btn == &undoBtn)
        if (undoMgt->canUndo()) undoMgt->undo();

    if (btn == &redoBtn)
        if (undoMgt->canRedo()) undoMgt->redo();
}

On the processor side,
Constructor inits:

,params(*this, &undoManager)
{
    params.createAndAddParameter("gain", "Gain", "gain", NormalisableRange<float>(0.0,1.0), 1.0, nullptr, nullptr);
    params.state = ValueTree( Identifier("undoTest"));
} 

And you get those behaviors:

  1. Click Undo just when running it first time will assert (UndoManager::perform:126) as:

             jassertfalse;  // don't call perform() recursively from the UndoableAction::perform()
                        // or undo() methods, or else these actions will be discarded!
    

I can “overcome” those assertions by adding on our constructor:

undoManager.clearUndoHistory();
  1. Now it pretty much “works”. but there are more undoable/redoable actions than what I’d expect from the plug-in…
  • start the plugin
  • move slider
  • undo
  • Try to redo, it’ll fail.
  • start the plugin
  • move slider
  • move slider
  • Try undo, it’ll consider both moves as a single transaction…

I didn’t try equator but if you compare this to other plug-ins undo/redo. this isn’t a common workflow. or am I’m implementing it wrong?

1 Like

note that “Redo” does not currently work correctly with AudioProcessorValueTreeState :

2 Likes

That’s why I’ve asked about the status of UndoManager within AudioProcessorValueTree context…
Because from my experiments it seems safest undo/redo with current state of it would be for me to implement UndoManager separately from AudioProcessorValueTree.

So unless I’m not implementing it wrong I’m confused with @jules saying it is being used with Equator/field-tested plugins.

Again,

  • we’ve used UndoManager with our current parameter engine and it’s working well.
  • we’re now using UndoManager with a ValueTree and it’s also seems to be doing well.

I’m only asking about AudioProcessorValueTree and UndoManager…

2 Likes

Hi,

in my AudioProcessor I have a AudioProcessorValueTreeState and and UndoManager. I pass the undoManager into the AudioProcessorValueTreeStates constructor and create all my parameters with state.createAndAddParameter. In my plugin editor, I use AudioProcessorValueTreeState::SliderAttachment. I want a new undo transaction to start, each time a new slider-drag begins, so I do

slider.onDragStart = [&](){
    std::clog << "new transaction:" << slider.getName() << std::endl;
    filter->undoManager.beginNewTransaction(slider.getName());
};

I also have two buttons labeled ‘undo’ and ‘redo’. that trigger undoManager.undo() and undoManager.redo() respectively.

All this works quite nicely, there are however some oddities:

  1. canUndo() returns true, even at the very beginning. This is because flushParameterValuesToValueTree() is called by some timer and perform()s SetPropertyActions on the undo manager. because this is done via a timer, I can’t even do undoManager.clearUndoHistory(); in the constructor of my AudioProcessor
  2. while undo works as expected, redo sometimes is messed up by flushParameterValuesToValueTree as well. If the timing is ‘just right’ (i.e. just wrong), flushParameterValuesToValueTree() is called right after I press my undo button and this pushes new actions into the undo manager and this trashes the redoable actions.

@jules: are the AudioProcessorValueTreeState & UndoManager supposed to work together? Is this a tested/supported scenario? Is there an example somewhere?

1 Like

This is a known issue, and @t0m has said it will be fixed in a future overhaul to AudioProcessorValueTreeState:

I (and others) have worked around it by adding a small delay to clear the undo history shortly after object creation, which while a total hack seems to clear it up. See my workaround at line 54:

You can give AudioProcessorUndoAttachment a try instead, it works better for AudioProcessorValueTreeState than AudioProcessorValueTreeState’s built-in UndoManager support.

6 Likes

great, thanks!

Thanks a ton, I will! How is your gist licensed?

Public domain :slight_smile: Or if that definition doesn’t work for you then BSD license.

2 Likes

I’m a bit confused, in the description it says that AudioProcessorUndoAttachment is not intended for AudioProcessorValueTreeState based projects, but you’re saying that it should work fine?

You’re right. @yairadix should maybe edit this comment. it was very early before we had a product with AudioProcessorValueTreeState released.

TL;DR:
The implantation on the gist is agnostic to your parameter system. we use it on older products and newer with great success.

Few caveats to remember:

  • it based on AudioProcessorListener. I’m not sure about future changes as seen here . but it simply keeps track on any parameter changes within the plug-in (not from the host, which is a good thing).

  • you should use begin/end gestures properly for sliders to get a proper behavior. (same goes with AudioProcessorValueTree from my experience).

  • A/B switch or single transaction for entire states aren’t being registered that way.

I wrote that description naively assuming that AudioProcessorValueTreeState's own undo attachment should work fine. Nowadays, knowing that it doesn’t work well I recommend AudioProcessorUndoAttachment everywhere.

3 Likes

It’s working great so far with my AudioProcessorValueTreeState setup!! Bunches and bunches of thanks Yair! :slight_smile:

https://github.com/WeAreROLI/JUCE/commit/2e51654958df26d9c374e961758180cc842e0dfa

This commit should address the issues using an UndoManager with an AudioProcessorValueTreeState.

  • An undoable action is no longer pushed onto the stack when the plug-in is loaded
  • Redoing actions works correctly
  • New transactions are started when you interact with an attachment

Since this alters the behaviour of the UndoManager this is technically a breaking change, but I doubt anyone was relying upon the old way of doing things.

6 Likes

thanks tom! everything works just fine!

HI.
Working with undomanager and valueTreeState (latest version), i have a randomiser function that changes the values of multiple parameters (notifying host). Undo manager undos all the changes made with just one undo() which is convenient. I thought it would undo them one by one, now I am curious how does it work since im not doing any perform() or beginNewTransaction()?

I dont expect it to work correctly if you don’t call beginNewTransaction() when the randomizer button is clicked.

it is probably undoing all the changes you made since the last beginNewTransaction() (so since you previously interact with a slider or something?). also, if you don’t call beginNewTransaction, then successive clicks on your randomiser button will get grouped together no?

Yes you’re right. All of them gets into one group and i can call beginNewTransaction() after each randomize action to separate them. So only changing sliders that are attached will create a single transaction, and if we want to call setValueNotifyingHost() ourselves we should call beginNewTransaction() after that. Got it.

no, you have to call beginNewTransaction() before the action, not after.

void buttonClicked (Button*)
{
    beginNewTransaction();
    setValueNotifyingHost()
}
1 Like