From kde-kimageshop Tue Sep 29 00:10:45 2009 From: Sven Langkamp Date: Tue, 29 Sep 2009 00:10:45 +0000 To: kde-kimageshop Subject: Re: A first part of the layers/masks patch Message-Id: <478b087a0909281710y34c66598h85ea88cc28986dde () mail ! gmail ! com> X-MARC-Message: https://marc.info/?l=kde-kimageshop&m=125418307608632 MIME-Version: 1 Content-Type: multipart/mixed; boundary="--===============0138411065==" --===============0138411065== Content-Type: multipart/alternative; boundary=00504502d4b3e6bd080474ac3efb --00504502d4b3e6bd080474ac3efb Content-Type: text/plain; charset=ISO-8859-1 On Fri, Sep 25, 2009 at 9:25 PM, Dmitry Kazakov 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 --00504502d4b3e6bd080474ac3efb Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable
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/chan= ge rects of filters inside masks (at best).

How it is now:
update= Projection() and ALL the stuff about masks AND projections is
moved to KisLayer. Specialized layer can operate with only a few
functio= ns those are not connected with a projection much. Every child
can overl= oad these functions:

original() - returns a paint device where the l= ayer stores it's
image. NOTE: This is not the same with projection() as the latter one
in= cludes original() with all the masks applied.

repaintOriginal() - ca= lled by updateProjection when original should be
repainted.

OPTIONAL:
A layer CAN (not should) overload:

needProjection() - if a layer nee= ds an always-in-memory projection
(e.g. KisPaintLayer needs one for indi= rect painting)

copyOriginalToProjection() - used in couple with the = previous one. A
layer can enhance original during painting on a projection
(e.g. indirec= t 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<= br>application of a mask-stack we first get to know what need rect we
want to copy from original() device.

Rect calculating algorithm is q= uite overwhelming. It's implemented in
two functions: masksChangeRec= t and masksNeedRect.
First we get a change rect of a DESTINATION PROJECT= ION 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 thi= s 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=A0 you simply you get a slack if you paint on a layer w= ith
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
ins= tead of destination).


2) The second task for this patch was to u= nify adjustment and
generator layers. The point is, they both use select= ions, that means
they both should work in the same way.

How it was before:
KisAdju= stmentLayer and KisGeneratorLayer had code duplications for
selections s= tuff.

How it is now:
All the selection-related stuff is moved to = a separate class -
KisSelectionBasedLayer. Both problematic classes are derived from this
b= ase class. Speaking truly, KisAdjustmentLayer class has very few code
in= side now.

BUGFIX:
Selections on selection based layers was not wo= rking 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 dup= lications and misuses was connected to
selections (again).

How it was before:
KisFilterMaks, KisGenerato= rMask, KisTransparencyMask - they all had
their own implementation
How it is now:
All the selection-related stuff is moved to KisMask cla= ss. Here is an
extract form a commit message:

=A0=A0=A0 Refactoring for masks
=A0=A0=A0 Now masks utilize selections in a uniform way. KisMask is the o= nly
=A0=A0=A0 place that knows anything about mask's selection[0]. I= t's descendant's do
=A0=A0=A0 very simple task, they just decorate the rect with the desired=A0=A0=A0 effect[1]. The task made by KisTransparencyMask fell back to eve= n more
=A0=A0=A0 trivial routine - it just needs to copy source image to= destination,
=A0=A0=A0 without any modifications[2].

=A0=A0=A0 Another significan= t change accumulated by this commit is a small
=A0=A0=A0 cleanup of KisM= askManager. There are still many FIXME's for it. The
=A0=A0=A0 most = annoying is code duplication [3] of KActions for masks.

=A0=A0=A0 [0] - KisMask::apply()
=A0=A0=A0 [1] - virtual KisMask::de= corateRect()
=A0=A0=A0 [2] - KisTransparencyMask::decorateRect()
=A0= =A0=A0 [3] - KisMaskManager's actions for mask functions are duplicated= in
=A0=A0=A0 KisLayerBox.

As you can see, this commit accumulates some cleanups for
KisMaskMan= ager too. Most of them remove code duplications and uniform
the code. Mo= re 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 u= ser 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
alre= ady here.


*) Still present bugs:

a) [the worst one] Paint= ing on any alpha mask do not work because of
alpha() colorspace issue i was described before in this maillist. You
ca= n paint if your mask is transparent, but the brush isn't. This is
no= t an issue of an algorithm. This is an issue of the alpha()
colorspace a= nd 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 paintingon 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 t= hink) is
that KisSelectionToolHelper class uses KisLayerSP type for inte= rnal
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) =3D> c) There is no(!) way to paint on alpha-channels/masks
curre= ntly. You simply CAN'T change the mask you created.

The only way to create a mask now is the following:
=A0=A0=A0 i) mak= e a vector selection first
=A0=A0=A0 ii) create a=A0 mask - it'll de= rive 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
devic= e instead of the destination one

e) Drag&Drop is broken in KisLa= yerBox. Just try to change an order of
masks with a mouse - you can easi= ly lift it up above the root
layer. Of course,=A0 your next action will cause krita to crash.

f) = Curves filter DO NOT(!) work. I guess, there is some bug in
KoColorTrans= formation 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 sil= ly string comparison
algorithm. Why?! Every node has a specially designe= d method for this!

h) KActions for mask-manipulation functions are d= uplicated 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 b= rush 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/i= mg23/3918/kritafiltermask.png

--00504502d4b3e6bd080474ac3efb-- --===============0138411065== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ kimageshop mailing list kimageshop@kde.org https://mail.kde.org/mailman/listinfo/kimageshop --===============0138411065==--