AudioProcessorValueTreeState & UndoManager usage


#13

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


#14

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…


#15

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?


#16

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:


#17

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


#18

great, thanks!


#19

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


#20

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


#21

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?


#22

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.


#23

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.


#24

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


#25

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.


AudioProcessorValueTreeState Undo behaviour - Juce demo plugin example
AudioProcessorValueTreeState - Presets & undo/redo
Redo issue with PluginStateValueTree. SetPropertyAction & excludeListener
#26

thanks tom! everything works just fine!


#27

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()?


#28

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?


#29

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.


#30

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

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

#31

Thanks for this! I noticed this introduced a regression where initializing an AudioProcessorValueTreeState with a null UndoManager will throw assertion failures due to the lack of a null check around the undoManager here https://github.com/WeAreROLI/JUCE/commit/2e51654958df26d9c374e961758180cc842e0dfa#diff-1ec75d23db30b69cffab9854fa6697f4R127

EDIT: Hrm, I’m not sure what the right behavior is or if I’m missing something obvious, but I keep hitting this assertion error whenever I change any param with param.setValue(newValue). The AudioProcessorValueTreeState is being initialized with a a non-null AudioProcessor and UndoManager. Having trouble getting to the bottom of this one.


#32

It turns out I was missing this step of the AudioProcessorValueTreeState setup:

state.state = ValueTree(Identifier("My-Identifier"));

@t0m before your commit, I would get no assertion errors at all in my project because the state.isValid() check in copyValueToValueTree() was always returning false, and was thus never descending into the state.setPropertyExcludingListener call. With your changes, it does make the setProperty... call even if the state is invalid (initialized with only the default constructor and given no explicit identifier).

Basically, with the new changes, if someone (a newb like me) is just starting a project and throws a default AudioProcessorValueTreeState constructor on their AudioProcessor, it will throw errors, whereas before the behavior was, “ok, you’ve done that. I won’t complain but I also won’t be doing anything useful.” Now, in that scenario we get obscure-looking errors. I’ll leave it to the team to decide if this is desired, but I would suggest that if this is an undesirable state for an AudioProcessorValueTreeState to be in, that you make it harder, or even impossible, to get into this state by, say, requiring an identifier and initializing the internal ValueTree state in the constructor or something like that. Barring this, if you’d rather allow the bad state but “fail-fast” with an assert error, I would suggest a helpful warning message that mentions explicitly initializing the internal ValueTree state.

Again, thanks for addressing these issues!