Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Undo v2.0 #402

Merged
merged 16 commits into from
Apr 23, 2024
Merged

Undo v2.0 #402

merged 16 commits into from
Apr 23, 2024

Conversation

abique
Copy link
Contributor

@abique abique commented Apr 18, 2024

No description provided.

@abique
Copy link
Contributor Author

abique commented Apr 18, 2024

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
Copy link
Collaborator

baconpaul commented Apr 18, 2024

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 e4c7447

  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
Copy link
Contributor Author

abique commented Apr 19, 2024

@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
Copy link
Collaborator

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
Copy link
Contributor Author

abique commented Apr 19, 2024

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
Copy link
Collaborator

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
Copy link
Contributor Author

abique commented Apr 19, 2024

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
Copy link
Contributor Author

abique commented Apr 19, 2024

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 👍

@abique
Copy link
Contributor Author

abique commented Apr 19, 2024

I added implicit changes to the extension.

@baconpaul
Copy link
Collaborator

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

@abique abique merged commit a10c3ec into free-audio:next Apr 23, 2024
4 checks passed
@abique abique deleted the undo-v2 branch April 23, 2024 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants