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 system improvements #1419

Open
wants to merge 23 commits into
base: main
Choose a base branch
from
Open

Conversation

ergeysay
Copy link
Contributor

@ergeysay ergeysay commented Jun 21, 2022

Ref: #792

This PR partially implements a refactoring of the undo system I was talking about in my previous PR.

You can see it in action:

ArmorPaint-1.mp4

Here, I create a couple of fill layers, put them in a group, add masks to each of them and then I merge down the group to the bottom layer. Then I repeatedly undo and redo this action.

More specifically, this PR implements an alternative approach to record history: instead of calling a History.{action}() before calling an action, now we call History.begin{action}() and History.end() inside the action() method itself.

The goal is to allow actions to record undo steps not only for themselves but also for any nested actions which greatly simplifies implementing undo and redo operations for complex actions such as Merge Down. The system also allows for correct recording of undo steps for recursive actions.

However, there is still room for improvement.

A single biggest issue is that this approach greatly increases number of the undo steps being recorded. This is why I increased default maximum number of undo steps to 32. Since each undo step has an associated layer, this increases memory requirements since there is currently no way to swap the layer data to disk.

Please let me know if you have any questions.

@ergeysay
Copy link
Contributor Author

@luboslenco hello! Could you please let me know what you think about this PR? I have several more ideas on history system improvements which build on ideas from this PR.

@luboslenco
Copy link
Member

Thanks a ton, it's super awesome - happy to merge!

This is why I increased default maximum number of undo steps to 32.

Hm I would leave the default at 4 for now and raise it asap once we improve the memory management, as this increases video memory use from 400MB to 1700MB. 4 by default definitely sucks - should be still enough to undo at least the most recent action in most cases until better memory management is implemented?

We could also expose some UI control directly in the history tab to make it easier for user to raise it manually.

@ergeysay
Copy link
Contributor Author

ergeysay commented Jun 28, 2022

Hm I would leave the default at 4 for now and raise it asap once we improve the memory management
I agree! The problem with the extraneous memory consumption is the glaring issue with these changes.

I've been thinking on how to improve on this and there are two solutions which I would like to discuss before starting the implementation.

The options are not mutually exclusive, and I believe both of them should be implemented to achieve maximal efficiency, but implementing any one of them should allow to increase maximum number of undo steps while keeping the same memory requirements.

Option 1

The first option is to implement paging system for layers like this:

  1. Allocate a fixed number of layers during application startup (this number can be made user-configurable)
  2. Get the platform-specific path for temporary files
  3. When the history system wants to save layer data for a step, page out the least recently used undo layer to the temporary directory
  4. When the history system needs to get layer data that was paged out, page it in

Overall, this seems quite straightforward. Obviously, there are cases when the system can reach an incorrect state (for example, there's no space left on the device to page out a layer), but these can be dealt with on a case-by-case basis.

Option 2

The second option is to prune the intermediate steps.

As of this PR, the history system records all of the intermediate states for all layers involved in any operation.

While this behavior is correct it is also obviously suboptimal since the history system only cares about two states: before and after an action. Which means it is possible to optimize memory usage by keeping just "before" and "after" states, generated by the series of recorded sub-actions. The pruning operation will maintain correctness since it will only be using already existing in-between states.

What's important is that it will allow us to keep the external history interface proposed in this PR intact, all the magic will be done internally.

This option is much more complex than option 1, but I believe that having it will benefit the project going forward.

Conclusion

I propose to implement option 1 in a separate PR, then merge this one and implement option 2 after that.

The reason is that having only 4 undo steps (and undo layers) is far too limiting. For example, merging two groups with two layers in each needs at least 4 undo layers just to preserve the contents of the layers, and there could be more than one such operation in a working session. Currently, the number of undo layers is tied to the maximum number of undo steps, which can lead to situations where undo layers are either under-utilized (and they just waste memory) or over-utilized (and there is not enough of them to preserve correctness).

Implementing option 1 will allow to have arbitrary number of undo steps while keeping memory requirements constrained and the history system will be able to use virtually unlimited number of layers.

What do you think?

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.

2 participants