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

The undo/redo system consumes a lot of RAM #883

Closed
OverloadedOrama opened this issue Jul 2, 2023 · 4 comments
Closed

The undo/redo system consumes a lot of RAM #883

OverloadedOrama opened this issue Jul 2, 2023 · 4 comments
Labels
bug Something isn't working help wanted Extra attention is needed

Comments

@OverloadedOrama
Copy link
Member

OverloadedOrama commented Jul 2, 2023

Pixelorama version:
v0.11.1-dev, happens in all previous versions as well

OS/device including version:
opneSUSE Tumbleweed, but occurs on all operating systems and hardware

Issue description:
The undo/redo system stores each change the user makes to the cel data (by drawing, applying effects etc) as separate Imageobjects for each undo state. For example, let's say we have a 6400x6400 project with a single cel. This alone consumes 6400x6400x4 ~= 163.84MB of memory. Drawing a single pixel, will store another 6400x6400 to memory via the undo/redo system, thus increasing the memory usage with another ~163MB. This is extremely inefficient, since only a single pixel has been changed. A workaround to clear the undo history is to either close and restart Pixelorama, or close that project, and the memory will be freed.

Ideas and proposals on how the system can be improved are welcome!

Steps to reproduce:
Follow the example above, and have a system activity/task manager utility open to measure Pixelorama's memory usage.

@OverloadedOrama OverloadedOrama added bug Something isn't working help wanted Extra attention is needed labels Jul 2, 2023
@OverloadedOrama OverloadedOrama pinned this issue Jul 2, 2023
@AlphinAlbukhari
Copy link
Contributor

An idea that comes to my mind is by writing the undo history into a file
For example, for every 20 step, we'll save our previous history into a file then clear our undo history in memory. If the user is undoing and trying to access the freed history, we will read and load that saved history

@MatteoPiovanelli-Laser
Copy link
Contributor

Launching pixelorama from the godot editor, my test: on a 6400x6400 canvas, draw 10 separate (more or less) vertical lines with the pencil tool at thickness 50.
Screenshot with the Windows Resource Monitor to show the used memory.

Code from master branch: 2+GB
image

Code from my proposal: around 600MB
image

The trick is compressing the hex data in the image, and decompressing it when that "state" needs to be restored.
I only applied this to

func commit_undo() -> void:
, because I don't know the other operations that may be affected by do/undo enough to claim the same principle should apply. Moreover, I did not try to optimize this further: for example, I only used the default compression method.

Other specific do/undo operations could be clever, and track the methods they should apply, at least in the "do" direction, rather than storing the entire images.

@MatteoPiovanelli-Laser
Copy link
Contributor

As discussed on discord, the cleaner way to handle this would be to have "proper" do/undo methods to reapply/revert changes everywhere.

The assumption I'm starting from is that were add_do_method and add_undo_method are being used, the software isn't (necessarily) placing a lot of stuff in memory, because it can invoke a method to handle changes; on the other hand add_do_property and add_undo_property can fill memory fast, because they'll often add the whole Image in RAM twice. Of course, it's also possible to store large objects (e.g. all the image data) when passing it to the method calls, so using those is not guarantee of better memory performances.

For the sake of discussion, I'll write here the places where I found project.undo_redo being used to fill the undo/redo stack (in the order given by the editor). I ignore calls to

project.undo_redo.add_undo_method(Global, "undo_or_redo", true)
project.undo_redo.add_do_method(Global, "undo_or_redo", false)

because those are used everywhere to make sure the UI is refreshed properly (as far as I understand it, at least).

  1. Autoload/OpenSave.gd
    1.1. open_image_as_spritesheet_layer is using add_do_method and add_undo_method.
    1.2. open_image_at_cel is using add_do_property and add_undo_property, passing an Image as parameter.
    1.3. open_image_as_new_frame is using add_do_method and add_undo_method.
    1.4. open_image_as_new_layer is using add_do_method and add_undo_method.
  2. Autoload/Global.gd: provide methods invoked elsewhere.
  3. Autoload/DrawingAlgos.gd
    3.1. scale_image is using add_do_property and add_undo_property, passing the image data as Draw was doing.
    3.2. center is using add_do_property and add_undo_property, passing the image data as Draw was doing.
    3.3. crop_image is using add_do_property and add_undo_property, passing the image data as Draw was doing.
    3.4. resize_canvas is using add_do_property and add_undo_property, passing the image data as Draw was doing.
    3.5. general_do_scale is using add_do_property, but only using numbers, so its memory impact looks to be minimal.
    3.6. general_undo_scale is using add_undo_property, but only using numbers, so its memory impact looks to be minimal.
  4. Classes/ImageEffect.gd
    4.1. _commit_undo is using add_do_property and add_undo_property, passing the image data as Draw was doing.
  5. Tools/Move.gd
    5.1. commit_undo is using add_do_property and add_undo_property, passing the image data as Draw was doing.
  6. Tools/Draw.gd
    6.1. commit_undo is the method that just got changed. Many tools extend this, so they use this implicitly. It may make sense to look at each independently.
  7. Tools/Bucket.gd
    7.1. commit_undo is using add_do_property and add_undo_property, passing the image data as Draw was doing.
  8. UI/Buttons/BrushesPopup.gd
    8.1. remove_brush uses both kinds of techniques. For properties it add an Array of brushes.
  9. UI/Canvas/Selection.gd
    9.1. commit_undo is using add_do_property and add_undo_property, adding a vector, but also the data for a SelectionMap (that extends Image).
  10. UI/Dialogs/ImageEffects/FlipImageDialog.gd
    10.1. _commit_undo is using add_do_property and add_undo_property, adding a vector, but also the data for a SelectionMap (that extends Image) as well as data for the image.
  11. UI/PerspectiveEditor/PerspectiveEditor.gd
    11.1. _on_AddPoint_pressed is using add_do_method and add_undo_method.
    11.2. delete_point is using add_do_method and add_undo_method.
  12. UI/Recorder/Recorder.gd
    12.1. disconnect_undo and connect_undo are both connecting an handler to capture frames. I would disregard this here.
  13. UI/Timeline/LayerButton.gd
    13.1. drop_data is using add_do_method and add_undo_method.
  14. UI/Timeline/FrameTagDialog.gd
    14.1. _on_TagOptions_confirmed is using add_do_method and add_undo_method.
    14.2. _on_TagOptions_custom_action is using add_do_method and add_undo_method.
  15. UI/Timeline/FrameProperties.gd
    15.1. _on_FrameProperties_confirmed is using add_do_property and add_undo_property, but only using numbers, so its memory impact looks to be minimal.
  16. UI/Timeline/FrameButton.gd
    16.1. change_frame_order is using add_do_method and add_undo_method.
    16.2. drop_data is using add_do_method and add_undo_method.
  17. UI/Timeline/CelButton.gd
    17.1. _on_PopupMenu_id_pressed is using add_do_method and add_undo_method.
    17.2. _delete_cel_content is using add_do_method and add_undo_method. It is passing the result of cel.get_content(), that in some cases may be the entire Image (if I'm reading things correctly).
    17.3. drop_data is using add_do_method and add_undo_method.
  18. UI/Timeline/AnimationTimeline.gd
    18.1. add_frame is using add_do_method and add_undo_method.
    18.2. delete_frames is using add_do_method and add_undo_method. It's also using add_do_property and add_undo_property, but only using numbers and apparently small objects.
    18.3. copy_frames is using add_do_method and add_undo_method. It's also using add_do_property and add_undo_property, but only using numbers and apparently small objects.
    18.4. reverse_frames is using add_do_method and add_undo_method.
    18.5. add_layer is using add_do_method and add_undo_method.
    18.6. _on_CloneLayer_pressed is using add_do_method and add_undo_method.
    18.7. _on_RemoveLayer_pressed is using add_do_method and add_undo_method.
    18.8. change_layer_order is using add_do_method and add_undo_method.
    18.9. copy_frames is using add_do_method and add_undo_method. It's also using add_do_property and add_undo_property, and I think it's passing entire images.

@OverloadedOrama
Copy link
Member Author

Continuing from the initial work made in #890:
e625485 - compresses image data of image effects, bucket tool and selection transformations.
4f5f37a - does not store image data of the move tool, just a difference vector and on undo/redo just move the pixels of the image based on that vector
e22794e - compresses image data of the mirror image effect
9279a8e - compresses image data of scaling and centering (in DrawingAlgos.gd)
2c5ece5 - compresses image data of replacing cels (when importing an image) and merging down layers.
e6d5329 - compresses the image data of the selections themselves (since they are stored as images)

With all these optimizations, I think this issue can be considered resolved now. The next big RAM optimization that could be done is that empty cels still consume a lot of memory. Not entirely sure how this can be solved though, perhaps by dynamically rescaling images based on how many pixels are drawn. This system could theoretically allow for layers with different sizes, but that's a discussion for another issue.

A negative side effect coming from saving compressed image data in undo/redo is that drawing on large canvases is noticeably laggier, talking about sizes like 6400x6400, at least on my machine. Perhaps a better way can be found that is both not so CPU intensive and also consumes less RAM, but again, that's probably a discussion for another issue.

These changes I made are for the v1.0 branch, but I will also port them to v0.11.4 soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants