Add RFC: Undo/Redo System

[Programatic]

Description

Create a undo system that is capable of recording the state of OBS actions to
easily revert the state forwards or backwards in time.

Motivation

When working with large and intricate processes - whether it be scenes, sources,
etc. - accidents are bound to happen. Currently, there is no way to undo
committed actions. As a result, work can easily be lost at the expense of hours
of effort.

View RFC

View Prototype

[admshao]

Alright, after a long time, i finally could review the RFC and i have a few points to bring up.

Also, like many other applications, the undo/redo keys should be binded to Ctrl-Z and Ctrl-Y

For me should be Ctrl-Z and Ctrl-Shift-Z :) So which one should be the default? Is it even big of an "issue" to have a custom setting just to swap that or user should just change the default hotkey in the Hotkeys Tab?

Saving the entire state of OBS would use up more memory than necessary when a small action is committed. In order to prevent this, only the state of the object the action was committed on would be saved, alongside the action committed

100% agree here and i know the linked Prototype is just a general concept. While an actual implementation we could store as little as just a vec3 or matrix4 while in complex Scene Collections we could have group items and nested scenes with lots of plugins so the "diff" concept will be harder.

Scope

Filters are sources as well as Transitions, but just for the sake of clarification, Shouldn't we include Filters as well?

Context

Should we consider the selected source as a narrowing function for the Undo/Redo? As well as providing per Scene Undo/Redo + General "Per Scene Collection" Stack?

Just for the sake of simplicity I'll make this example with edits only, but things go crazy on real world usage of the program (I.e: hundreds of scenes)

Ex: In Scene A: User makes Edit1 to Source 1 (action 1 or a1), Edit2 to Source 1(action 2 or a2), Edit1 to Source 2(a3) and Edit3 to Source 1(a4).

IMHO, If no source is selected, the Undo function would have to do reverse a4, a3, a2 and finally a1. While if Source 1 is selected, the Undo function would have to check and only do a4, a2 and a1.
While if Source 2 is selected, it would only have a3.

Those were the thoughts I had when implemented a prototype on my local fork a while ago.

[Fishrock123]

What does 'local area' mean here?

[Fishrock123]

Should this include filters?

[WizardCM]

Absolutely.

[Fishrock123]

It should probably be decided in advance how this should act between profiles?

Anyways this is my number 1 gripe with OBS and since I have a bunch of time on my hands for now, I'd certainly be willing to help implement this, if other people's hands are full.

[Fishrock123]

For me should be Ctrl-Z and Ctrl-Shift-Z :) So which one should be the default? Is it even big of an "issue" to have a custom setting just to swap that or user should just change the default hotkey in the Hotkeys Tab?

Also seconding CTRL-SHIFT-Z, but honestly I think redo should bind to both by default.

[Fishrock123]

Should we consider the selected source as a narrowing function for the Undo/Redo? As well as providing per Scene Undo/Redo + General "Per Scene Collection" Stack?

I'm unsure how much work it would be to prototype it, but I wonder if the best way wouldn't be to try both and see which feels the most natural to people? I can see up and down sides to both...

[admshao]

Help is really welcome @Fishrock123! Once most of the topics are discussed and agreed upon, i can coordinate a branch for us to implement that

[Programatic]

Yes, help will be greatly appreciated as there are a lot of different components that will have to be saved. Currently, the scope only reflects immediate big picture items, but it for sure should include a lot of the small customizations that a user would expect it to have otherwise, I can imagine a lot of people getting angry very quickly because it ls not clear exactly which elements are being saved.

Also on the comment of the local area, I will update the rfc tomorrow when I have time, buy the local area essentially means components directly related to the action taking place. For scenes, that means saving order and sources (although I believe I've found a better way to handle them) as the local area that needs to be reinflated when, say, undoing a delete.

[WizardCM]

It should probably be decided in advance how this should act between profiles?

We have a couple options, but firstly: current scope of undo/redo is purely Scene Collection related. Switching the Profile shouldn't affect the undo stack.

In terms of switching Scene Collections however, I would vote that saving to disk and discarding on next launch would make sense, in case the user switches scene collections and back, then realise they made a change they weren't happy with. This'd be consistent with Photoshop retaining the Undo stack for each file that's currently open.

Should we consider the selected source as a narrowing function for the Undo/Redo? As well as providing per Scene Undo/Redo + General "Per Scene Collection" Stack?

The challenge stems from "Sources" vs "Scene Item".
If you're modifying the transform, that's specific to the current scene and therefore per-scene Undo makes sense. However, if the item is in a group, this can get weird, as the changes will be reflected in multiple scenes, even though the changes "occurred" in one.
If you're modifying the properties, that will affect the same source in multiple locations, especially across multiple scenes.

Overall, I think a singular, general stack that's tied to the scene collection makes the most sense.

Currently, the scope only reflects immediate big picture items, but it for sure should include a lot of the small customizations that a user would expect it to have otherwise

I've documented the majority of the most important items for the scope. I'd say start with Delete. Scenes should be the first priority, as deleting a scene means deleting a large number of scene items, and potentially sources/groups too. Doing this, we can internally start writing functions for saving the undo stack of deleting sceneitem, group, and source, in that order.

[WizardCM]

I would go one step further, and actually name the undo actions, so that even if the first version doesn't track everything, the user has an idea of what will happen.

Photoshop example: "Undo New Layer"

This'd require translations for anything that can be undone. Could have a generic "Undo (%1)" translation, with %1 being replaced by the name of the action itself. This'd allow for easy extensibility in the future, and avoid translation duplication of the "Undo" word.

[WizardCM]

This assumes kind of "stack", allowing the user to essentially forge a "path" from one advanced state to another.

[WizardCM]

I agree that binding Redo to both Ctrl+Y and Ctrl+Shift+Z is the best option.

[WizardCM]

Keeping the memory footprint low is ideal, and I completely agree with only storing the exact values that changed. Are we thinking of a maximum size of the undo stack, either by number of actions or used memory?

[Fishrock123]

I'd think number of actions should probably be fine, even 100 might be enough.

[WizardCM]

Absolutely.

[WizardCM]

This scope needs to be significantly more specific for each of the items.

Scenes:

• Add
• Rename
• Reorder
• Delete

Sources/Scene Items:

• Add
• Properties (detailed scope here too, undo individual properties, or only Save/Cancel?)
• Rename
• Transform
  • Mouse drag: on mouse release
  • Keyboard: should this work on a timeframe where the keyboard moving the element should stop for at least 1 second to 'save' the undo
  • Transform submenu, each individual menu action trigger
  • Edit Transform dialog. Currently this sort-of saves a mini-undo-stack thanks to Qt, we want to keep this functionality, but allow bigger undos on Save/Cancel
• Reorder
  • Do we store how many rows it moved up/down, or where it was placed in relation to other sceneitems?
• Move into group
  • Keep in mind this also can do strange things to transform information, and it would likely be a good idea to store before/after of transform
• Delete

Filters:

• Add
• Properties (when the dialog is opened/closed, or when the user switches to the filter in the list?)
• Rename
• Reorder (if the attempt at adding drag & drop support is any indicator, this might get compicated)
• Delete

Audio Controls:

• Are we talking about just the Input Mixer, or Advanced Audio Properties too?
• I personally wouldn't see the need of undoing mute state or monitoring state
• Volume is a must

Transitions:

• Does this include creating/adding Quick Transitions? I vote that come later
• Add
• Properties (same question as Sources)
• Delete

[admshao]

Properties (detailed scope here too, undo individual properties, or only Save/Cancel?)

I would say just Save/Cancel.

Keyboard: should this work on a timeframe where the keyboard moving the element should stop for at least 1 second to 'save' the undo

Sounds about a good default! I think this might be an option for General Settings

Move into group. Keep in mind this also can do strange things to transform information, and it would likely be a good idea to store before/after of transform

Sounds tricky

Audio Controls:

I agree, probably just Volume is needed. All other controls we already have fine control

[Fishrock123]

Reorder

I think most times a reorder is to move an element specifically in-front of/behind other specific elements, so perhaps object-related like as in a linked list may be most appropriate, although that may be more complex than needed.

Delete

Just to clarify, this will need to hold as much info as possible on the deleted object.

[jp9000]

Just wanted to put an important note here, do not close or merge this, especially not until I've had a chance to comment on it. I'll comment on it in the near future.

[f2bacon]

im sorry but couldn't you steal a lil work from streamlabs?

[dodgepong]

If you're referring to implementation work, then no. Their fork uses a different UI framework.

If you're referring to design considerations, then possibly.

[f2bacon]

oh well then i'm worse than useless. sorry, and keep up the good work

[Fishrock123]

@jp9000 Could we please hear your thoughts on this soon?

[jp9000]

For undo/redo, I have a lot of experience writing these systems because I've written multiple in the past.

It seems the current proposal is basically an undo/redo specifically for source properties and scene changes. If a source is modified, push its json data in to a stack. This is all well and good, but doesn't really address things like transform, volume changes, and other things like that (if we want to address any of those things). An undo/redo system probably needs to be adaptable to any particular action we want to include, and we not yet have thought of all the actions that could be included in undo/redo. Do we want to exclude certain things or do we want to include every possible action?

Additionally, when considering what can constitute as an action to undo/redo, we need to consider the frequency of an action. A source can update every time you modify a single property, so you could for example have a full undo/redo action modify a single property of a source; actions such as modifying a slider value in the properties of a source or filter. If they dragged that slider back and forth, it will typically update at a high frequency, in real time. So in order to not "flood" the undo/redo stack with actions, you probably need to keep consolidating all such actions until either the user performs another unrelated action, or until the properties for that object are closed. This is why undo/redo could be a little bit more complicated than initially expected.

Thus, all property modifications that happen to an object in succession to one another should be consolidated in to a single action. "If previous action was a modification of properties to this object, and the properties window for this object is still open, change last undo/redo action rather than create a new undo/redo action" in order to not have a flooded undo/redo stack.

As for your example, using new/delete is almost always unnecessary. You should favor RAII and simple container objects when possible. You do not really need a linked list when you have things like circular buffers available to you in the standard library (std::deque). You do not even need to include references to existing objects or unnecessary data. You could simply find an object by its name (or maybe an identifier system might make it more appropriate), and it should be able to find that object in a table or list of some sort rather than even include any object pointer on an undo/redo stack. Personally I think that's safer than risking the need of "freeing" or "releasing" data that might be referenced in an undo/redo stack.

I will include a small source example of undo/redo handling I do from my most recent game editor I wrote as an example of what I'd normally do for something like undo/redo, it's something I do for my most recent game engine's level editor that handles any actions fairly well.

undo-stack.hpp
undo-stack.cpp

In this example, you'll note how simple undo/redo can be, yet these files are deceptively powerful. Of course, in my game engine I also have serializer objects which would probably need to be considered. Note that I use std::string objects as byte data buffers; they aren't actual readable strings, they're just byte data that utilize the std::string object as general byte data buffers. Of course, data could be stored as actual readable strings if we want, and the fact that we have json means that we could do just that. Unlike my game engine, we could just serialize all the data with json or something, and instead of my custom crpg::serializer objects that serialize in to byte data, that can be changed to something like,

typedef std::function<void(const json11::Json &obj)> undo_redo_cb;

This is just an idea. I hope I'm making sense. You'll notice the undo_stack::add_action function has five params. The first is the name of the action, so you can put it in the menu bar, like "Undo ['x']" or "Redo ['x']". The second and third parameters are functions that are called when you actually do the undo/redo action in order to perform the undo/redo. The fourth and fifth params are used to serialize the data to be sent to those functions but instead you could just use json11::Json objects or something for those params and serialize them beforehand.

So here would probably be how obs would probably look instead:
undo-stack-obs.hpp

Does this all make sense? But in general, as you can see the whole thing is super simple yet quite powerful.

[Programatic]

Thanks Jim! This makes perfect sense and I will try implementing it in my prototype. I guess I was trying to do a pseudo serialization using the built in obs_data objects, but ended up over complicating it. I ended up having to use manual deletes because I was struggling with the interconnected parts (it seems). This is just a lack of familiarity with the core of OBS, because from when I was testing it, RAII kept automatically deleting the objects I was storing causing consistent crashes (i think), and not using RAII fixed it. Perhaps using JSON serialization will help, but when I was storing the state of, say, the sources, when I tried loading them back, it would never register with OBS and nothing would happen. I tried using sourcegraph to trace through the source and find the distinction between sources and scenes, and found that they were way more combined than I thought. Could you direct me to some sources that would help unveil how exactly obs loads the sources/scenes and how they are differentiated.

[RytoEX]

This shouldn't include commits from other RFCs.

[Programatic]

Ok, my bad! I'll go back and update it to remove those commits

[Fenrirthviti]

Entering fast-track FCP for this, as the code has been merged, we have just not been keeping up with RFCs as much as we should be. If there are no objections by EOD, we will merge this.