free-audio / clap

Audio Plugin API
https://cleveraudio.org/
MIT License
1.71k stars 98 forks source link

Undo v2.0 #402

Closed abique closed 2 months ago

abique commented 2 months ago

One thing that I didn't add yet is the implicit changes. For example when turning a knob, the host can already deal with the that change and create the undo step. Though I'm not sure if the parameters are the exception or if we need an more general approach to this.

baconpaul commented 2 months ago

So as I said on discord I think the requirement to have the binary deltas be stable across versions and instances is a mistake. We should revert https://github.com/free-audio/clap/pull/402/commits/e4c7447fb91ba4b963e893f10671fd240d0cc12c

  1. It conflates undo redo with incremental state
  2. It places a very high bar on plugin undo redo implementations

I think the api should include a way for the plugin to advertise whether its undo redo stack is valid per instance or across instances.

Implementing this api without that constraint is easy in surge. Adding that constraint means I wouldn’t bother or maybe would do something that just makes apply delta fail across instances always.

abique commented 2 months ago

@baconpaul the delta are currently optional. So you can have an implementation of the undo that doesn't require to provide any delta.

We could have a flag to express the forward compatibility of those deltas, at least that'd be a step to make the plugin developers aware of this.

The host, or at least Bitwig needs some strong properties on the deltas, and if the plugin's provided deltas don't honor those then it is better for the host to collect the plugin state (which is forward compatible).

baconpaul commented 2 months ago

so

I have a large state. A parameter change i have a small undo. But if I don't send you that undo you cache the entire state? That seems really wasteful no?

There's, I bet, a lot of plugins out there with undo implementations (including surge) where

  1. the undo state is way smaller than the state of the plugin but
  2. the undo is hard to make stream across instances reliably
abique commented 2 months ago

The host can workout a binary diff of the state. Also there's one more thing to elaborate in that spec, like to let the host create implicit undo state for parameter changes.

Most plugins have a relatively small state. For plugins with a large state, there's more value/reward in spending time on the delta format.

baconpaul commented 2 months ago

but then the redo is setting the entire state!

that's terrible! We turn off and restart our audio engine on a state reset. but don't on a parameter change.

abique commented 2 months ago

Please avoid hyperbolic figures of speech.

As I've written above, we also have to discuss a different path for the parameter changes as those changes are well understood by the host and the undo steps can be created by the host without the plugin calling complete_change(). The undo/redo for parameter change are as simple as a parameter change.

Yes, a state reset is an heavy operation and the host could keep both the state and the delta and pick the delta if its life time and version format permits it and fallback to state otherwise.

I'll add the life time info.

abique commented 2 months ago

I've added the delta lifetime.

I think it allows for the host to choose the best path according to the current options. @baconpaul thanks for insisting :+1:

abique commented 2 months ago

I added implicit changes to the extension.

baconpaul commented 2 months ago

Yes thank you Alex! That exactly addresses ny concern

my “that’s terrible” was indeed mis worded. What I meant was “that would make host undo way worse for our users than internal undo (which I think would be a terrible experience for them)” but with the suggested changes I can definitely code to this!!

appreciate you listening