The undo/redo system consumes a lot of RAM
OverloadedOrama opened this issue · 4 comments
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 Image
objects 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.
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
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 my proposal: around 600MB
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
Line 201 in e195e32
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.
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).
- Autoload/OpenSave.gd
1.1.open_image_as_spritesheet_layer
is usingadd_do_method
andadd_undo_method
.
1.2.open_image_at_cel
is usingadd_do_property
andadd_undo_property
, passing an Image as parameter.
1.3.open_image_as_new_frame
is usingadd_do_method
andadd_undo_method
.
1.4.open_image_as_new_layer
is usingadd_do_method
andadd_undo_method
. - Autoload/Global.gd: provide methods invoked elsewhere.
- Autoload/DrawingAlgos.gd
3.1.scale_image
is usingadd_do_property
andadd_undo_property
, passing the image data as Draw was doing.
3.2.center
is usingadd_do_property
andadd_undo_property
, passing the image data as Draw was doing.
3.3.crop_image
is usingadd_do_property
andadd_undo_property
, passing the image data as Draw was doing.
3.4.resize_canvas
is usingadd_do_property
andadd_undo_property
, passing the image data as Draw was doing.
3.5.general_do_scale
is usingadd_do_property
, but only using numbers, so its memory impact looks to be minimal.
3.6.general_undo_scale
is usingadd_undo_property
, but only using numbers, so its memory impact looks to be minimal. - Classes/ImageEffect.gd
4.1._commit_undo
is usingadd_do_property
andadd_undo_property
, passing the image data as Draw was doing. - Tools/Move.gd
5.1.commit_undo
is usingadd_do_property
andadd_undo_property
, passing the image data as Draw was doing. - 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. - Tools/Bucket.gd
7.1.commit_undo
is usingadd_do_property
andadd_undo_property
, passing the image data as Draw was doing. - UI/Buttons/BrushesPopup.gd
8.1.remove_brush
uses both kinds of techniques. For properties it add anArray
of brushes. - UI/Canvas/Selection.gd
9.1.commit_undo
is usingadd_do_property
andadd_undo_property
, adding a vector, but also the data for aSelectionMap
(that extendsImage
). - UI/Dialogs/ImageEffects/FlipImageDialog.gd
10.1._commit_undo
is usingadd_do_property
andadd_undo_property
, adding a vector, but also the data for aSelectionMap
(that extendsImage
) as well as data for the image. - UI/PerspectiveEditor/PerspectiveEditor.gd
11.1._on_AddPoint_pressed
is usingadd_do_method
andadd_undo_method
.
11.2.delete_point
is usingadd_do_method
andadd_undo_method
. - UI/Recorder/Recorder.gd
12.1.disconnect_undo
andconnect_undo
are both connecting an handler to capture frames. I would disregard this here. - UI/Timeline/LayerButton.gd
13.1.drop_data
is usingadd_do_method
andadd_undo_method
. - UI/Timeline/FrameTagDialog.gd
14.1._on_TagOptions_confirmed
is usingadd_do_method
andadd_undo_method
.
14.2._on_TagOptions_custom_action
is usingadd_do_method
andadd_undo_method
. - UI/Timeline/FrameProperties.gd
15.1._on_FrameProperties_confirmed
is usingadd_do_property
andadd_undo_property
, but only using numbers, so its memory impact looks to be minimal. - UI/Timeline/FrameButton.gd
16.1.change_frame_order
is usingadd_do_method
andadd_undo_method
.
16.2.drop_data
is usingadd_do_method
andadd_undo_method
. - UI/Timeline/CelButton.gd
17.1._on_PopupMenu_id_pressed
is usingadd_do_method
andadd_undo_method
.
17.2._delete_cel_content
is usingadd_do_method
andadd_undo_method
. It is passing the result ofcel.get_content()
, that in some cases may be the entire Image (if I'm reading things correctly).
17.3.drop_data
is usingadd_do_method
andadd_undo_method
. - UI/Timeline/AnimationTimeline.gd
18.1.add_frame
is usingadd_do_method
andadd_undo_method
.
18.2.delete_frames
is usingadd_do_method
andadd_undo_method
. It's also usingadd_do_property
andadd_undo_property
, but only using numbers and apparently small objects.
18.3.copy_frames
is usingadd_do_method
andadd_undo_method
. It's also usingadd_do_property
andadd_undo_property
, but only using numbers and apparently small objects.
18.4.reverse_frames
is usingadd_do_method
andadd_undo_method
.
18.5.add_layer
is usingadd_do_method
andadd_undo_method
.
18.6._on_CloneLayer_pressed
is usingadd_do_method
andadd_undo_method
.
18.7._on_RemoveLayer_pressed
is usingadd_do_method
andadd_undo_method
.
18.8.change_layer_order
is usingadd_do_method
andadd_undo_method
.
18.9.copy_frames
is usingadd_do_method
andadd_undo_method
. It's also usingadd_do_property
andadd_undo_property
, and I think it's passing entire images.
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.