Create Task
Maniphest T10901

Better undo/redo for Krita
Open, Needs TriagePublic
Actions

Description

  1. Snapshot Docker.
    1. Users should be able to save the current state of document as a snapshot. Use KisDocument::clone() to obtain a (relatively) shallow copy of the document.
    2. The docker should display a list of snapshots (shallow copies) of the document, and allow users to replace the current document with any of the snapshots taken.
    3. A user manual should be written on how to use this docker. After finishing the docker, a tutorial video should be recorded.
  2. Copy-on-Write Vector Layers.
    1. Current implementations of vector layers:
      1. Data are stored using d-pointers that are not shared
      2. Subclasses’ d-pointers are inherited from those of the parent class
      3. Have at least two constructors for each class to prevent creating multiple data instances
        1. ChildClass() : ParentClass(new ChildClassPrivate(this))
        2. ChildClass(ChildClassPrivate * dd) : ParentClass(dd)
      4. All shapes are descended from KoShape, including KisShapeLayer itself
    2. Possible implementations:
      1. Similar implementation to the one in KisTile, which manually calls KisTileData::acquire() when an edit is needed.
      2. Qt’s implicit sharing, using the Qt class QSharedDataPointer. This class is reentrant (guaranteed to be safe when different threads access different instances). This is the preferred method. However, this is not thread-safe and may cause problems if the same instance is accessed from multiple threads. More work needs to be done to ensure thread-safety.
      3. C++11’s shared_ptr, as demonstrated by Sean Parent. However, in KDE guidelines, Qt libraries are preferred over std. This should be attempted only if QSharedDataPointer cannot work.

Timeline

Week 1-4 (May 27 - June 23)
Write core code to replace the current document with another KisDocument
Unit test for replacing the current document with another KisDocument
This involves emitting signals to GUI
Probably begin writing the snapshot docker

Week 5 (June 24 - June 30)
Snapshot Docker
Write user manuals
Make a tutorial/demo video

Week 6 (July 1 - July 7)
Build a testing release, and collect feedback from the community
Possible questions:
Is snapshot taking helpful to your workflow?
Is it efficient?
Other features wanted?
Improve the docker using the feedback

Week 7-8 (July 8 - July 21)
Start working on Copy-on-Write vector layers
The re-written classes should have the same API as the old ones, where possible.
Start using implicit sharing for members of KoShapePrivate
What have been stored using pointers may be now stored using shallow copies, for example, QSharedPointer<KoShapeStrokeModel> stroke could be replaced with KoShapeStrokeModel stroke
Make api documents on how implicit-sharing works for these components, and possibly a “guideline” or example on how to use implicit sharing in other classes.

Week 9-10 (July 22 - August 4)
Implicit sharing for the KoShape hierarchy
According to the discussion above, I may need to replace Qt’s macro with code written by myself in all descendents
Write and run unit tests
Try to merge the changes into master

Week 11 (August 5 - August 11)
Write undo commands that switch between states to replace the original ones that stores actions

Week 12 + Remaining 1 day (August 12 - August 19)
Used for buffering

Further Discussions

See also: https://phabricator.kde.org/T11969

Discussions with Dmitry on #krita, Sat 9 Nov 2019. All times in UTC-5.

<tusooa> dmitryK|log: do you mean https://github.com/arximboldi/lager this
         library?                                                       [10:11]
<dmitryK|log> tusooa: yes                                               [10:13]
<dmitryK|log> tusooa: I didn't look at it deeply
<dmitryK|log> tusooa: it is more for GUI controls, but some of its ideas might
              be useful for shapes as well
<tusooa> oh                                                             [10:14]
<tusooa> what do you mean by "limiting shapes editing to GUI only sounds
         really wrong."?
<tusooa> i think i meant the reverse (gui threads only read shapes)     [10:15]
<dmitryK|log> tusooa: I mean that shapes editing should be also allowed in the
              background threads
<tusooa> in stroke?
<dmitryK|log> tusooa: and gui threads shoul dbe able to read some (old) copy
              of them
<dmitryK|log> tusooa: yes, in stroke
<tusooa> that is exactly what i intended to say lol
<dmitryK|log> well, sorry :)
<tusooa> np -))                                                         [10:16]
<tusooa> "access" -> "read"
<tusooa> dmitryK|log: and, what do you mean by the "original problem"?
<tusooa> btw "KisDescendent<const KoShape>" is inspired by the pattern
         introduced by Sean Parent                                      [10:17]
<dmitryK|log> tusooa: I mean that we don't have a proper general solution to
              it in Krita at all :)                                     [10:21]
<tusooa> thread-safety?                                                 [10:22]
<tusooa> or what
<dmitryK|log> tusooa: yes
<dmitryK|log> tusooa: bascially, my recent refactorings of transform tool and
              move tool can partially address this problem
<dmitryK|log> tusooa: but this approach is not applicable to other tools
<Animtim> boud: about your question related to the blog post on
          Qt+android+openssl, it probably is relevant for you, except if you
          build using KDE's docker image; the article talks only about 5.13.1,
          but I found out the same applies to 5.12.5                    [10:23]
<dmitryK|log> tusooa: bascially, if the tool has "options widget" and does
              something in Kis*Tool::paint(), then it probaly reads from the
              image in unsafe way.
<Wolthera_laptop> Animtim: thanks for mentioning it!
<tusooa> ok                                                             [10:24]
<Animtim> :)
<dmitryK|log> tusooa: tiles engine guarantees that this access does not cause
              any crash, but the data read is still "undefined behavior"
<tusooa> i can see the case for DefaultTool
<dmitryK|log> tusooa: to fix this problem in MoveTool and TransformTool I had
              to move tool initialization into the stroke itself        [10:25]
<dmitryK|log> tusooa: and it is not very possible for Default Tool.
<dmitryK|log> tusooa: some other approach should be used for it
<tusooa> i mean the Tool Options Widget
<tusooa> i locked the canvas in updateActions to prevent it accessing shapes
         when there might be changes ongoing                            [10:26]
<dmitryK|log> tusooa: yes, everything related to flake is really affected :)
<tusooa> but that does not solve the whole problem
<dmitryK|log> yes
<tusooa> it seems we might need something mediating between tools and shapes
                                                                        [10:27]
<dmitryK|log> tusooa: ideally, it needs some higer level solution. Like
              read-copy-update. But I have no clear idea what exactly
<tusooa> especially to provide atomic/pseudo-atomic access
<tusooa> dmitryK|log: i think copy-update sounds viable                 [10:28]
<dmitryK|log> tusooa: well, actually, original idea of KoShapeManager could
              solve this issue :)
<dmitryK|log> tusooa: but now we have KoShapeDocumentController and all these
              weird classes around it                                   [10:29]
<tusooa> but doesn't that mean we would introduce "dummies" again?
<dmitryK|log> tusooa: well, what I can tell from those c++ talks...
<tusooa> um probably not if we just pass everything by value
<tusooa> wait i have not really seen KoShapeDocumentController          [10:30]
<tusooa> i only know KisShapeController, KoShapeController,
         KoShapeControllerBase                                          [10:31]
<tusooa> but that's already a mess -((
<dmitryK|log> tusooa: 1) tools should not use pointers to shapes. Only values;
              2) If a tool needs to save a pointer to a shape, it should use
              some "persistent index" (see QPersistentModelIndex for
              inspiration). 3) KoShapeManager should be a *the only* point,
              which is used for accessing shapes by the tools. 4)
              KoShapeManager can be swapped atomically after any operation; 5)
              No other controllers will exist                           [10:32]
<dmitryK|log> tusooa: sorry, I didn't remember their name. One of these
              classes was name KoShapeDocumentBase, and then was renamed into
              something like KoShapeControllerBase or somehting         [10:33]
<tusooa> ha, i see why one of the classes has the weird documentBase()
         function                                                       [10:34]
<tusooa> ok
<tusooa> (1)(2) makes sense to me
<dmitryK|log> tusooa: but all this "persistent index" idea is rather
              complicated. And that is why I was suggesting to look into Lager
              library
<tusooa> yea                                                            [10:35]
<dmitryK|log> tusooa: Lager is supposed to track changed in such objects and
              emit notifications properly
<tusooa> ok
<dmitryK|log> tusooa: all this talk gradualy comes down to a "functional
              programming" thing...                                     [10:36]
<tusooa> dmitryK|log: what do you mean by (4)
*** slangkamp (~quassel@200116b812c065001c7a2a322bda69e7.dip.versatel-1u1.de)
    has joined channel #krita
<dmitryK|log> tusooa: is RCU paradigm you need to swap something in the end of
              update operation
<dmitryK|log> tusooa: KoShapeManager (or its internals) is a good candidate
              for that
<dmitryK|log> *in RCU...
<tusooa> but don't see why we would want to swap KoShapeManager         [10:37]
<tusooa> isn't it just a controller class?
<tusooa> that persists?
<dmitryK|log> tusooa: well, I have no strong opinion about this
<tusooa> i mean we need to be able to swap things atomacally, that's true
                                                                        [10:38]
<dmitryK|log> tusooa: perhaps, if we swap into Lager library, then
              KoShapeManager will just stop to exist
<tusooa> um
<dmitryK|log> tusooa: because Lager is bascially an entity that updates
              tracking                                                  [10:39]
<tusooa> i haven't finished reading its documentation
<dmitryK|log> tusooa: and KoShapeManager also does updates tracking :)
<raghukamath> sometimes convrting colors crashes krita
<dmitryK|log> tusooa: watch videos of the author, he is really a nice guy :)
<tusooa> sure will do
<dmitryK|log> raghukamath: in master? or in 4.                          [10:40]
<raghukamath> master
<raghukamath> but it can't be reproduced
<raghukamath> i have only encountered it twice
<Wolthera_laptop> race condition, maybe?
<dmitryK|log> raghukamath: report a backtrace, I've merged the this week
<dmitryK|log> raghukamath: which sha1
<dmitryK|log> ?
<tusooa> dmitryK|log: it monitors changes in shapes and forwards them to the
         canvas right?
<raghukamath> 7104885                                                   [10:41]
<dmitryK|log> tusooa: KoShapeManager, yes
<dmitryK|log> tusooa: Lager does the same thing. And it can also aggregate the
              updates
<raghukamath> 7104885cf0904dbfe03cd04346a3b46d2547effb
<raghukamath> i'll update
<tusooa> shapes notice KoShapeManager that they have been modified --> shape
         manager compresses it and make the canvas do updates
<tusooa> i will read the doc                                            [10:42]
<tusooa> dmitryK|log: thank you for the insights

Related Objects
Search...

StatusAssignedTask
 OpentusooawT10901 Better undo/redo for Krita
 ResolvedtusooawT10991 Snapshot Docker
tusooaw created this task.May 8 2019, 4:42 PM
tusooaw updated the task description. (Show Details)May 8 2019, 4:45 PM
tusooaw added a comment.Jun 20 2019, 4:15 AM

Discussion on q-pointers.

The KoShapePrivate hierarchy uses q-pointers, which is a barrier to making it derive from QSharedData. (because the data are shared by many KoShapes and we do not know which one we are calling from)

Possible solutions:

  • Refactor out all the q-pointers and pass the KoShape pointer every time we call a function in KoShapePrivate
  • Refactor out all the q-pointers and put all methods using q-pointers into KoShape class
  • Make the shared data an inner layer of KoShapePrivate and access properties using d->s->propertyName (what if a descendent class accesses the d-pointers?)
tusooaw added a comment.Jun 20 2019, 9:32 AM

Discussion on derived d-pointers.

Currently the KoShape hierarchy uses derived d-pointers. E.g. KoShapeContainerPrivate is derived from KoShapePrivate. Since there is public inheritance, it makes it possible for KoShapeContainer class to access things declared in KoShapePrivate class (should this be allowed?).

Do we really need derived d-pointers? Would it be better to let the derived classes have their own d-pointers? There does not seem to be any benefit to use the derived ones, except for saving some memory. And yet, using multiple d-pointers seems to allow less deep-copying.

dkazakov added a subscriber: dkazakov.Jun 20 2019, 10:45 AM

About q-pointers.

The basic idea of Qt's d/q-pointers is to avoid private members and methods being exposed in a header file (and, therefore in a class' binary representation). It provides us with two features:

  1. binary backward compatibility (we can update private methods without breaking ABI)
  2. reduce compilation time (we don't need to recompile library (and client) code when changing private members

In our case point 1) is of no importance to us. Point 2) is interesting, but is not a "must have".

Looking at the real code, we have very few private methods in KoShapePrivate and its descendants, so I guess we can just refactor out q-pointers completely, that is:

Refactor out all the q-pointers and put all methods using q-pointers into KoShape class

dkazakov added a comment.Jun 20 2019, 10:58 AM
In T10901#189466, @tusooaw wrote:

Discussion on derived d-pointers.

Deriving d-pointers is also a part of "binary compatibility" and "private members exposure" problem. Basically, it is a tricky way to support 'protected' members and methods in an environment where you cannot write into a header. If all the private members are derived, then it is enough to have only one Private *p_ptr in a header, and all the descendants will happily use it. But it they are not derived, then every class of hierarchy will have to expose its own Private member.

Would it be better to let the derived classes have their own d-pointers?

I'd say, it is possible. In Krita (not Ko) code we use exactly this way. We declare QScopedPointer<Private> m_d on every level of class hierarchy and expose protected methods via a header. It is not much binary compatible, but we don't care.

Do we really need derived d-pointers?

The only benefit is a little bit better compilation time, so, no, there is no strong requirement on keeping d-pointers derived. The only trouble is, it might be a bit complicated to refactor all these classes. If you manage to do that, that would be very good :)

tusooaw added a comment.Jul 8 2019, 1:07 PM

On KoShapeController:

Currently the methods return a KUndo2Command that will do the changes. However, it should be possible to make them only do the changes, by taking a clone of the active layer before making undoable changes to it.

The original approach looks like:

KUndo2Command *command = shapeController->addShape(...);
doc->addCommand(command);

The new approach should look like:

KisNodeSP originalState = currentNode->clone();
shapeController->addShape(...);
// currentNode has been modified now

// KisNodeReplaceCommand is a post-exec command
// to avoid multiple kundo2_i18n`s we probably want to make a factory method for it?
KUndo2Command *cmd = new KisNodeReplaceCommand(kundo2_i18n(...), currentNode, originalState);
doc->addCommand(cmd);
tusooaw added a comment.Jul 8 2019, 1:15 PM

A note on KisNode::copyFromNode():

We are probably currently unable to "remove then add a copy" because the "original state" has to be a clone of the layer in the working document -- that will mess up with previous undo commands in the history because they cannot find the correct node.

copyFromNode() function should be no longer required as we get rid of undo commands--at that time the whole document will be replaced. See https://invent.kde.org/tusooaw/krita/commit/7c77e99a3c5037e49a44fbf033da6fdec84ed68a

tusooaw added a comment.Jul 25 2019, 8:06 AM

On the crash upon deletion of stroke strategy without creating an undo command:

It happens only when (1) we are not using debugger (so it is probably some timing issue); (2) the mouse is released immediately after pressing; and (3) no undo command is created.

https://invent.kde.org/snippets/335 indicates that the problem probably lies in KisCanvas2, whose m_d->canvasUpdateCompressor is of type KisSignalCompressor instead of the thread-safe one.

But I did also get another error message complaining from QObject::~QObject().

tusooaw added a comment.Jul 25 2019, 8:22 AM
In T10901#192951, @tusooaw wrote:

On the crash upon deletion of stroke strategy without creating an undo command:

It happens only when (1) we are not using debugger (so it is probably some timing issue); (2) the mouse is released immediately after pressing; and (3) no undo command is created.

https://invent.kde.org/snippets/335 indicates that the problem probably lies in KisCanvas2, whose m_d->canvasUpdateCompressor is of type KisSignalCompressor instead of the thread-safe one.

But I did also get another error message complaining from QObject::~QObject().

Probably two fixes:

  1. Make canvasUpdateCompressor in KisCanvas2 a KisThreadSafeSignalCompressor
  1. Split the canvas updating part into an update job (?)
tusooaw added a comment.Jul 26 2019, 3:14 AM

https://invent.kde.org/tusooaw/krita/commit/24355db3272a02230a14217da6023a953ec8f119

The crash does not come from KisCanvas2 but KisNode actually, as I said that it happens only when the undo command is not created (so KisNode is deleted by KisNodeReplaceBasedStrokeStrategy::Private's destructor, which is executed in the image thread (by the destructor of KisStroke)).

Ref: https://invent.kde.org/tusooaw/krita/blob/24355db3272a02230a14217da6023a953ec8f119/libs/global/kis_thread_safe_signal_compressor.h

tusooaw added a comment.Aug 9 2019, 7:52 AM
In T10901#193043, @tusooaw wrote:

https://invent.kde.org/tusooaw/krita/commit/24355db3272a02230a14217da6023a953ec8f119

The crash does not come from KisCanvas2 but KisNode actually, as I said that it happens only when the undo command is not created (so KisNode is deleted by KisNodeReplaceBasedStrokeStrategy::Private's destructor, which is executed in the image thread (by the destructor of KisStroke)).

Ref: https://invent.kde.org/tusooaw/krita/blob/24355db3272a02230a14217da6023a953ec8f119/libs/global/kis_thread_safe_signal_compressor.h

https://invent.kde.org/tusooaw/krita/commit/1bfd349378b4c1ca5c8f3ede6e465c66c94bb9c1

It comes from KoShapeManager's updateTreeCompressor actually.

We have run updateTree() before every action in KoShapeContainer, so the compressor is not really needed.

tusooaw updated the task description. (Show Details)Nov 9 2019, 4:25 PM
tusooaw updated the task description. (Show Details)Nov 9 2019, 7:50 PM
ramonmiranda added a subscriber: ramonmiranda.May 26 2020, 11:07 AM

Thanks Tusooa for your hard and useful work. I think Snapshot docker is a good addition for complex images.
For your week 6 if you think this could be doable:
I used that in PS long time ago and i remember there was a feature that allowed you to paint from history. (history mean ,snapshot) It is only a note if you are going to work more in this feature with more options. Right now in my tests works pretty well. Again Thanks