Faster undo/redo keeping doc::Objects in memory (#4860)#5437
Conversation
aseprite-bot
left a comment
There was a problem hiding this comment.
clang-tidy made some suggestions
| void LayerImage::suspendObject() | ||
| { | ||
| CelIterator it = getCelBegin(); | ||
| CelIterator end = getCelEnd(); |
There was a problem hiding this comment.
warning: variable 'end' of type 'CelIterator' (aka '__normal_iterator<doc::Cel **, std::vector<doc::Cel *, std::allocator<doc::Cel *>>>') can be declared 'const' [misc-const-correctness]
| CelIterator end = getCelEnd(); | |
| CelIterator const end = getCelEnd(); |
| { | ||
| Layer::restoreObject(); | ||
| CelIterator it = getCelBegin(); | ||
| CelIterator end = getCelEnd(); |
There was a problem hiding this comment.
warning: variable 'end' of type 'CelIterator' (aka '__normal_iterator<doc::Cel **, std::vector<doc::Cel *, std::allocator<doc::Cel *>>>') can be declared 'const' [misc-const-correctness]
| CelIterator end = getCelEnd(); | |
| CelIterator const end = getCelEnd(); |
5242107 to
cc15775
Compare
cc15775 to
91e1d83
Compare
aseprite-bot
left a comment
There was a problem hiding this comment.
clang-tidy made some suggestions
|
|
||
| int ImageImplBase::getMemSize() const | ||
| { | ||
| return sizeof(ImageImplBase) + size_t(m_stream ? (size_t)m_stream->tellp() : 0) + |
There was a problem hiding this comment.
warning: redundant explicit casting to the same type 'size_t' (aka 'unsigned long') as the sub-expression, remove this casting [readability-redundant-casting]
| return sizeof(ImageImplBase) + size_t(m_stream ? (size_t)m_stream->tellp() : 0) + | |
| return sizeof(ImageImplBase) + (m_stream ? (size_t)m_stream->tellp() : 0) + |
|
|
||
| #include "base/buffer.h" | ||
| #include "doc/blend_funcs.h" | ||
| #include "doc/image.h" |
There was a problem hiding this comment.
warning: circular header file dependency detected while including 'image.h', please check the include path [misc-header-include-cycle]
#include "doc/image.h"
^'image.h' included from here
|
|
||
| class ImageImplBase : public Image { | ||
| protected: | ||
| ImageBufferPtr m_buffer; |
There was a problem hiding this comment.
warning: member variable 'm_buffer' has protected visibility [misc-non-private-member-variables-in-classes]
ImageBufferPtr m_buffer;
^| class ImageImplBase : public Image { | ||
| protected: | ||
| ImageBufferPtr m_buffer; | ||
| std::unique_ptr<std::stringstream> m_stream; |
There was a problem hiding this comment.
warning: member variable 'm_stream' has protected visibility [misc-non-private-member-variables-in-classes]
std::unique_ptr<std::stringstream> m_stream;
^| m_rows = (address_t*)m_buffer->buffer(); | ||
| m_bits = (address_t)(m_buffer->buffer() + for_rows); | ||
|
|
||
| auto addr = (uint8_t*)m_bits; |
There was a problem hiding this comment.
warning: 'auto addr' can be declared as 'auto *addr' [readability-qualified-auto]
| auto addr = (uint8_t*)m_bits; | |
| auto *addr = (uint8_t*)m_bits; |
91e1d83 to
529d978
Compare
529d978 to
f3fa844
Compare
f3fa844 to
27a2aea
Compare
09f9fbe to
f4d2e4c
Compare
…ite#4860) We can just store the ID of each object into an alternative "suspended ID" to keep the object in memory outside the current doc objects collection, and easily bring it back when we undo an operation that needs the object alive. Anyway this is tricky and we have to fix some situations. E.g. when removing a Cel, the Cel::id() might be required when we trigger some notification, but we cannot remove the Cel first from the sprite as we need to count the number of links (we need the related sprite/layer for this). We have to fix these issues yet (e.g. having a links counter in the CelData). This patch introduces more memory usage for doc::Image, as we are just keeping images in memory, they are uncompressed in the undo history, but before we were compressing/decompressing with write/read_image() functions. We can fix this later compressing the data of suspended images in a background thread or something similar.
We have added a CelData reference counter (CelData::m_refs) to known how many cels are referencing/linked with the same CelData in the sprite. It makes the Cel::links() member function impl a lot simpler: without the need to access the parent layer to iterate all cels to count all links.
f4d2e4c to
731a3fb
Compare
We also save the bounds of the cel as it's required to restore the bounds when we restore the image if we SetCelImage(cel, nullptr) and then undo.
We also save the bounds of the cel as it's required to restore the bounds when we restore the image if we SetCelImage(cel, nullptr) and then undo.
We can just store the ID of each object into an alternative "suspended ID" to keep the object in memory outside the current doc objects collection, and easily bring it back when we undo an operation that needs the object alive.
Anyway this is tricky and we have to fix some situations. E.g. when removing a Cel, the Cel::id() might be required when we trigger some notification, but we cannot remove the Cel first from the sprite as we need to count the number of links (we need the related sprite/layer for this).
We have to fix these issues yet (e.g. having a links counter in the CelData).
This patch introduces more memory usage for doc::Image, as we are just keeping images in memory, they are uncompressed in the undo history, but before we were compressing/decompressing with write/read_image() functions. We can fix this later compressing the data of suspended images in a background thread or something similar.
Implements part of #4860