Review Request: Fix clone layers updating

[Torio Mlshi]

> On Nov. 1, 2011, 9:41 p.m., Dmitry Kazakov wrote:
> > The bug is a really good catch! I didn't even think about it. =)
> > 
> > But the problem is, this very solution will not work, I'm afraid. There is a couple of problems.
> > 1) setDirty() and refreshGraphAsync() do different things actually. setDirty() updates the node and all its parent nodes (that is the nodes above the dirty one). refreshGraphAsync first updates the subgraph of nodes below the dirty node, after that all the parent nodes (the work of setDirty()). Specifically for this solution, it won't work for moving group layers with children.
> > 2) every setDirty() call adds a separate job to the updater, so calling setDirty() for all the parent nodes will add too much excessive work.
> > 
> > 
> > As for working solution, atm I cannot tell exactly we can do here, but I have the following thoughts:
> > 1) There are already updates for clone layers in KisLayers::setDirty() so there is no need for additional method in KisNode.
> > 2) The real problem here is that refreshGraphAsync() does not (and should not) call to setDirty(), so the clones are not updated.
> > 3) There is one more problem possible: when the node above the dirty node is updated, its setDirty() is not called as well, so the clones of such node (it can be an issue for a group and filter layer only) will not be updated.
> > 4) The most sane solution, i think, would be to move the call to 'm_d->clonesList.setDirty(rect);' from setDirty() into something like updateProjection() call. But there are several problems possible like race conditions and deadlocks as the latter method is called asynchronously from several non-UI threads. I will check whether it is possible, but not earlier than Thursday.

I didn't think about this either, I just tried to use clones of group layers =)

1) What is the reason for updating nodes below the moving one? I've tried moving group layers and it worked.. And even if it doesn't work in some cases, the solution could be either to call setDirty from refreshGraphAsync (if it does this work anyway, just replace that part with setDirty call) or to call them both (this probably would lead to slowness) or to call it after moving stroke ends (this is worse because clone layers would not be updated while moving, but better from performace point of view).

2) Ok, then we could add some call like updateClones() and call it from setDirty to parents. The point for calling from KisNode (not KisLayer) is that it fixes not only problem of group layers' clones, but also problem of clones of layers with masks.

About moving clones updating to updateProjection -- may be it is better solution if it will work. But I'm not sure if it will be easier than fixing my solution (I've done some testing and it worked, so only problem that I see is potential performance problems; if it it does not work in some case, please write).


- Torio


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/103021/#review7845
-----------------------------------------------------------


On Nov. 1, 2011, 7:25 p.m., Torio Mlshi wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/103021/
> -----------------------------------------------------------
> 
> (Updated Nov. 1, 2011, 7:25 p.m.)
> 
> 
> Review request for Calligra.
> 
> 
> Description
> -------
> 
> This patch fixes issues, when clone layer wasn't updating despite its original was changed.
> 
> KisNode::setDirty now calls setDirty of parent to let its clones know about update. In MoveStrokeStrategy updating is now implemented via setDirty, so clone layers will know about update.
> 
> 
> Diffs
> -----
> 
>   krita/image/kis_node.cpp 52881e7 
>   krita/plugins/tools/defaulttools/strokes/move_stroke_strategy.cpp df8b6da 
> 
> Diff: http://git.reviewboard.kde.org/r/103021/diff/diff
> 
> 
> Testing
> -------
> 
> Normal moving (when no clones) seems to have the same performance. Of course, performance becomes low when many clones are involved.
> 
> Unit tests have same results for me.
> 
> 
> Thanks,
> 
> Torio Mlshi
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/calligra-devel/attachments/20111102/9e58aa6d/attachment.htm>