'Re: A first part of the layers/masks patch'

[prev in list] [next in list] [prev in thread] [next in thread] 

List: kde-kimageshop
Subject: Re: A first part of the layers/masks patch
From: Sven Langkamp <sven.langkamp () gmail ! com>
Date: 2009-09-29 0:10:45
Message-ID: 478b087a0909281710y34c66598h85ea88cc28986dde () mail ! gmail ! com
[Download RAW message or body]

[Attachment #2 (multipart/alternative)]

On Fri, Sep 25, 2009 at 9:25 PM, Dmitry Kazakov <dimula73@gmail.com> wrote:

> What is done in the patch.
>
>
> 1) Now all the layers apply their own masks in a uniform way.
>
> How it was before:
> Every type of layer (paint, adjustment,etc) had it's own
> updateProjection() function. And EVERY layer implemented it's own
> cycle of applying masks. No need to say that they all differed. And
> most of them was broken, because they didn't take into account
> need/change rects of filters inside masks (at best).
>
> How it is now:
> updateProjection() and ALL the stuff about masks AND projections is
> moved to KisLayer. Specialized layer can operate with only a few
> functions those are not connected with a projection much. Every child
> can overload these functions:
>
> original() - returns a paint device where the layer stores it's
> image. NOTE: This is not the same with projection() as the latter one
> includes original() with all the masks applied.
>
> repaintOriginal() - called by updateProjection when original should be
> repainted.
>
> OPTIONAL:
> A layer CAN (not should) overload:
>
> needProjection() - if a layer needs an always-in-memory projection
> (e.g. KisPaintLayer needs one for indirect painting)
>
> copyOriginalToProjection() - used in couple with the previous one. A
> layer can enhance original during painting on a projection
> (e.g. indirect painting, see KisPaintDevice's method)
>
>
> This all means that a KisLayer's child shouldn't do any work to create
> a projection. It's created by KisLayer in a deferred way (when masks
> are present)
>
> Masks application strategy changed too (and quite much).
> Now changeRect()/needRect() functions are part of KisNode. And before
> application of a mask-stack we first get to know what need rect we
> want to copy from original() device.
>
> Rect calculating algorithm is quite overwhelming. It's implemented in
> two functions: masksChangeRect and masksNeedRect.
> First we get a change rect of a DESTINATION PROJECTION with a first
> function in a bottom-up(!) way. Then we calculate needRects for all
> the masks in a top-down(!) way with a second function, and by the end
> of this process we get a rect that we should copy from original()
> device to a temporary device for applying filters.
>
> BUGFIX:
> This part of a patch fixes a bug with a blur-filter-mask. In current
> version  you simply you get a slack if you paint on a layer with
> blur-mask present (i'm not sure you can check it up now, because
> blur-filter has an internal bug - it writes the result to the source
> instead of destination).
>
>
> 2) The second task for this patch was to unify adjustment and
> generator layers. The point is, they both use selections, that means
> they both should work in the same way.
>
> How it was before:
> KisAdjustmentLayer and KisGeneratorLayer had code duplications for
> selections stuff.
>
> How it is now:
> All the selection-related stuff is moved to a separate class -
> KisSelectionBasedLayer. Both problematic classes are derived from this
> base class. Speaking truly, KisAdjustmentLayer class has very few code
> inside now.
>
> BUGFIX:
> Selections on selection based layers was not working at all
> before. Now the the code applies selections (if you manage to
> create one ;) (see * for more)) to the layers well.
>
>
> 3) The third task was to clean masks classes. We had much code
> duplication there too. Most duplications and misuses was connected to
> selections (again).
>
> How it was before:
> KisFilterMaks, KisGeneratorMask, KisTransparencyMask - they all had
> their own implementation
>
> How it is now:
> All the selection-related stuff is moved to KisMask class. Here is an
> extract form a commit message:
>
>     Refactoring for masks
>
>     Now masks utilize selections in a uniform way. KisMask is the only
>     place that knows anything about mask's selection[0]. It's descendant's
> do
>     very simple task, they just decorate the rect with the desired
>     effect[1]. The task made by KisTransparencyMask fell back to even more
>     trivial routine - it just needs to copy source image to destination,
>     without any modifications[2].
>
>     Another significant change accumulated by this commit is a small
>     cleanup of KisMaskManager. There are still many FIXME's for it. The
>     most annoying is code duplication [3] of KActions for masks.
>
>     [0] - KisMask::apply()
>     [1] - virtual KisMask::decorateRect()
>     [2] - KisTransparencyMask::decorateRect()
>     [3] - KisMaskManager's actions for mask functions are duplicated in
>     KisLayerBox.
>
> As you can see, this commit accumulates some cleanups for
> KisMaskManager too. Most of them remove code duplications and uniform
> the code. More than that [and this is the only place where i worry
> about the patch] this fix adds a new method(!) to KoDocument. It's
> called undoLastCommand() - that is an equivalent to a user-undo
> command. It is added to be able to cancel creation of an filter mask
> in case a user presses 'Cancel' button in a creation dialog. Please
> review this change!
>
> BUGFIXES:
> - cancelling creation of the filter mask now do not break undo stack
> - selections (see *) on filter masks work well now
>
>
> 4) Cleaning selections infrastructure, made me create a benchmark for
> different selection application algorithms. This benchmark is placed
> in ./image/tests/kis_filter_selections_benchmark.cpp. I wrote about it
> already here.
>
>
> *) Still present bugs:
>
> a) [the worst one] Painting on any alpha mask do not work because of
> alpha() colorspace issue i was described before in this maillist. You
> can paint if your mask is transparent, but the brush isn't. This is
> not an issue of an algorithm. This is an issue of the alpha()
> colorspace and the fact that we use brush's alpha channel for painting
> alpha image-channel (tha latter assumption is completely wrong(!)).
>
> Solution: Brush's dab should use grayscale colorspace during painting
> on alpha-mask/selection. KisSelection should use grayscale, or a
> special alpha_special() colorspace to work with grayscale channel of a
> dab.
>
> b) Vector selection tools DO NOT work on masks (And i'm not sure they
> work on adjustment layers). The reason of the first fact (i think) is
> that KisSelectionToolHelper class uses KisLayerSP type for internal
> storage of a paint device. It should be changed to KisNodeSP to fix
> that (of course alongside some other changes in logics of this class)
>
> a),b) => c) There is no(!) way to paint on alpha-channels/masks
> currently. You simply CAN'T change the mask you created.
>
> The only way to create a mask now is the following:
>     i) make a vector selection first
>     ii) create a  mask - it'll derive this selection for it's own
>
> [some minor bugs not dependent on the patch]
> d) Blur filter works wrong. It writes filtered image to the source
> device instead of the destination one
>
> e) Drag&Drop is broken in KisLayerBox. Just try to change an order of
> masks with a mouse - you can easily lift it up above the root
> layer. Of course,  your next action will cause krita to crash.
>
> f) Curves filter DO NOT(!) work. I guess, there is some bug in
> KoColorTransformation as every curve works as a brightness/contrast
> curve instead of a channel curve. Just try it out!
>
>
> [some design bugs]
> g) KisNodeManager::allowAsChild uses some silly string comparison
> algorithm. Why?! Every node has a specially designed method for this!
>
> h) KActions for mask-manipulation functions are duplicated in
> KisMaskManager and in KisLayerBox. They must be moved to
> KisMaskManager.
>
> PS:
> btw, a,b,c,d,e,f are good, well-fed release blockers.
>

Painting on masks is a bit weird as now the meaning of brush and eraser is
reversed. So before the patch I could paint transparency with the brush, now
I have to use the eraser.

Also painting on mask causes some artifacts:
http://img23.imageshack.us/img23/3918/kritafiltermask.png

[Attachment #5 (text/html)]

<div class="gmail_quote">On Fri, Sep 25, 2009 at 9:25 PM, Dmitry Kazakov &lt;<a \
href="mailto:dimula73@gmail.com">dimula73@gmail.com</a>&gt; wrote: <blockquote \
class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt \
0.8ex; padding-left: 1ex;"> What is done in the patch. 1) Now all the layers apply \
their own masks in a uniform way. How it was before: Every type of layer (paint, \
adjustment,etc) had it&#39;s own updateProjection() function. And EVERY layer implemented \
it&#39;s own

cycle of applying masks. No need to say that they all differed. And most of them was broken, \
because they didn&#39;t take into account need/change rects of filters inside masks (at \
best). How it is now: updateProjection() and ALL the stuff about masks AND \
projections is

moved to KisLayer. Specialized layer can operate with only a few functions those are not \
connected with a projection much. Every child can overload these \
functions: original() - returns a paint device where the layer stores it&#39;s

image. NOTE: This is not the same with projection() as the latter one includes original() \
with all the masks applied. repaintOriginal() - called by updateProjection when original \
should be repainted. OPTIONAL: 
A layer CAN (not should) overload: needProjection() - if a layer needs an \
always-in-memory projection (e.g. KisPaintLayer needs one for indirect \
painting) copyOriginalToProjection() - used in couple with the previous one. A

layer can enhance original during painting on a projection (e.g. indirect painting, see \
KisPaintDevice&#39;s method) This all means that a KisLayer&#39;s child \
shouldn&#39;t do any work to create a projection. It&#39;s created by KisLayer in a deferred \
way (when masks

are present) Masks application strategy changed too (and quite much). Now \
changeRect()/needRect() functions are part of KisNode. And before application of a \
mask-stack we first get to know what need rect we

want to copy from original() device. Rect calculating algorithm is quite overwhelming. \
It&#39;s implemented in two functions: masksChangeRect and masksNeedRect. First we get a \
change rect of a DESTINATION PROJECTION with a first

function in a bottom-up(!) way. Then we calculate needRects for all the masks in a \
top-down(!) way with a second function, and by the end of this process we get a rect that we \
should copy from original() device to a temporary device for applying filters.

BUGFIX: This part of a patch fixes a bug with a blur-filter-mask. In current version \
you simply you get a slack if you paint on a layer with blur-mask present (i&#39;m not sure \
you can check it up now, because

blur-filter has an internal bug - it writes the result to the source instead of \
destination). 2) The second task for this patch was to unify adjustment \
and generator layers. The point is, they both use selections, that means

they both should work in the same way.<br><br>How it was before:<br>KisAdjustmentLayer and \
KisGeneratorLayer had code duplications for<br>selections stuff.<br><br>How it is now:<br>All \
the selection-related stuff is moved to a separate class -<br>

KisSelectionBasedLayer. Both problematic classes are derived from this base class. Speaking \
truly, KisAdjustmentLayer class has very few code inside now. BUGFIX: Selections \
on selection based layers was not working at all

before. Now the the code applies selections (if you manage to create one ;) (see * for \
more)) to the layers well. 3) The third task was to clean masks classes. We had much \
code duplication there too. Most duplications and misuses was connected to

selections (again). How it was before: KisFilterMaks, KisGeneratorMask, \
KisTransparencyMask - they all had their own implementation How it is now: All the \
selection-related stuff is moved to KisMask class. Here is an

extract form a commit message: Refactoring for masks Now masks utilize \
selections in a uniform way. KisMask is the only place that knows anything about \
mask&#39;s selection[0]. It&#39;s descendant&#39;s do

    very simple task, they just decorate the rect with the desired<br>    effect[1]. The task \
made by KisTransparencyMask fell back to even more<br>    trivial routine - it just needs to \
copy source image to destination,<br>

    without any modifications[2].<br><br>    Another significant change accumulated by this \
commit is a small<br>    cleanup of KisMaskManager. There are still many FIXME&#39;s for it. \
The<br>    most annoying is code duplication [3] of KActions for masks.<br>

<br>    [0] - KisMask::apply()<br>    [1] - virtual KisMask::decorateRect()<br>    [2] - \
KisTransparencyMask::decorateRect()<br>    [3] - KisMaskManager&#39;s actions for mask \
functions are duplicated in<br>    KisLayerBox.<br>

As you can see, this commit accumulates some cleanups for KisMaskManager too. Most of \
them remove code duplications and uniform the code. More than that [and this is the only \
place where i worry about the patch] this fix adds a new method(!) to KoDocument. \
It&#39;s

called undoLastCommand() - that is an equivalent to a user-undo command. It is added to be \
able to cancel creation of an filter mask in case a user presses &#39;Cancel&#39; button in \
a creation dialog. Please review this change!

<br>BUGFIXES:<br>- cancelling creation of the filter mask now do not break undo stack<br>- \
selections (see *) on filter masks work well now<br><br><br>4) Cleaning selections \
infrastructure, made me create a benchmark for<br>

different selection application algorithms. This benchmark is placed in \
./image/tests/kis_filter_selections_benchmark.cpp. I wrote about it already \
here. *) Still present bugs: a) [the worst one] Painting on any alpha mask do \
not work because of

alpha() colorspace issue i was described before in this maillist. You can paint if your mask \
is transparent, but the brush isn&#39;t. This is not an issue of an algorithm. This is an \
issue of the alpha() colorspace and the fact that we use brush&#39;s alpha channel for \
painting

alpha image-channel (tha latter assumption is completely wrong(!)). Solution: \
Brush&#39;s dab should use grayscale colorspace during painting on alpha-mask/selection. \
KisSelection should use grayscale, or a special alpha_special() colorspace to work with \
grayscale channel of a dab. b) Vector selection tools DO NOT work on masks (And \
i&#39;m not sure they work on adjustment layers). The reason of the first fact (i think) \
is that KisSelectionToolHelper class uses KisLayerSP type for internal

storage of a paint device. It should be changed to KisNodeSP to fix that (of course \
alongside some other changes in logics of this class) a),b) =&gt; c) There is no(!) way \
to paint on alpha-channels/masks currently. You simply CAN&#39;T change the mask you \
created.

<br>The only way to create a mask now is the following:<br>    i) make a vector selection \
first<br>    ii) create a  mask - it&#39;ll derive this selection for it&#39;s own<br><br>[some \
minor bugs not dependent on the patch]<br>

d) Blur filter works wrong. It writes filtered image to the source device instead of the \
destination one e) Drag&amp;Drop is broken in KisLayerBox. Just try to change an order \
of masks with a mouse - you can easily lift it up above the root

layer. Of course, your next action will cause krita to crash. f) Curves filter DO \
NOT(!) work. I guess, there is some bug in KoColorTransformation as every curve works as a \
brightness/contrast curve instead of a channel curve. Just try it out!

[some design bugs] g) KisNodeManager::allowAsChild uses some silly string \
comparison algorithm. Why?! Every node has a specially designed method for this! h) \
KActions for mask-manipulation functions are duplicated in

KisMaskManager and in KisLayerBox. They must be moved to KisMaskManager. PS: btw, \
a,b,c,d,e,f are good, well-fed release blockers. </blockquote><div> Painting on masks is \
a bit weird as now the meaning of brush and eraser is reversed. So before the patch I could \
paint transparency with the brush, now I have to use the eraser. Also painting on mask \
causes some artifacts: <a \
href="http://img23.imageshack.us/img23/3918/kritafiltermask.png">http://img23.imageshack.us/img23/3918/kritafiltermask.png</a> \
 </div></div>

_______________________________________________
kimageshop mailing list
kimageshop@kde.org
https://mail.kde.org/mailman/listinfo/kimageshop

[prev in list] [next in list] [prev in thread] [next in thread] 