A first part of the layers/masks patch

[Dmitry Kazakov-2]

Hi, all!

Today i've managed to merge my patch with current trunk code. So, if you are interested in it you can take a look at it:

This is a singlepatch version. It can be easily applied to trunk without conflicts. It's good for testing, not for reading:
http://dimula73.narod.ru/01_layers_masks_refactoring_singlepatch.diff

And this is a patchset of changes, convenient for reading and investigation. It can't be applied to trunk without a couple of conflicts. That's why i suggest it only for reading.
http://dimula73.narod.ru/layers-masks-refactor-patchset.tar.gz

I'm too sleepy to write about the changes this patch introduces 8) I'll do it in a day or two.


PS:
For Boud
You have to update your copy of a patchset to make ./image/tests compile :)

PPS:
For Lukas
This part still doesn't fix fickering :)

PPPS:
For Sven and Cyrille
But seem to fix 205210. But it should be tested.

--
Dmitry Kazakov

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

[Dmitry Kazakov-2]

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.


--
Dmitry Kazakov

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

[Cyrille Berger]

Hello,

Bah I tried to apply it but got a lot of stuff that didn't apply cleanly :(
Anyway, it brings a lot of nice changes, so I think it would be a good idea to
apply it sooner than latter.

> *) 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!

> btw, a,b,c,d,e,f are good, well-fed release blockers.
hum no. With the exception of e), if it is indeed easy to trigger the crash,
that I can't reproduce before the patch.

about d) As far as I can see, the blur filter do write on dst, what makes you
think it doesn't ?

for a,b,c) it doesn't work perfectly, but it's not *that* broken.

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

[Cyrille Berger]

On Friday 25 September 2009, Cyrille Berger wrote:
> for a,b,c) it doesn't work perfectly, but it's not that broken.
Read a) and c), I have fixed the eraser composite. And will have a look at why
opacity isn't respected tomorrow.

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

On Fri, Sep 25, 2009 at 9:25 PM, Dmitry Kazakov <dimula73@...> 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)

Yes, all selection tools will not work on the mask but on the selection. So even if you have selected the mask all changes with selection tools will go to the global selection.
 

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.

We might want to kill the actions in the mask menu. See the other thread.
 

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

[Dmitry Kazakov-2]

On Sat, Sep 26, 2009 at 12:00 AM, Cyrille Berger <cberger@...> wrote:

Hello,

Bah I tried to apply it but got a lot of stuff that didn't apply cleanly :(

Did you use a singlepatch version? Maybe trunk have already changed - i'll try to update in the evening.

Anyway, it brings a lot of nice changes, so I think it would be a good idea to
apply it sooner than latter.

> btw, a,b,c,d,e,f are good, well-fed release blockers.

hum no. With the exception of e), if it is indeed easy to trigger the crash,
that I can't reproduce before the patch.

Not working Curves is not good either.
 

about d) As far as I can see, the blur filter do write on dst, what makes you
think it doesn't ?

I added src->convertToQImage(0).save("foobar.png"); right after the call =)
More than that, there is another bug (double application of blur in filters dialog) caused by this.
 

for a,b,c) it doesn't work perfectly, but it's not *that* broken.

But still not usable =(

--
Dmitry Kazakov

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

[Dmitry Kazakov-2]

Yes, all selection tools will not work on the mask but on the selection. So even if you have selected the mask all changes with selection tools will go to the global selection.

Exactly!

 

We might want to kill the actions in the mask menu. See the other thread.

We should kill them in layerBox and share them with mask manager.
 

--
Dmitry Kazakov

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

[Cyrille Berger]

On Saturday 26 September 2009, Dmitry Kazakov wrote:
> > for a,b,c) it doesn't work perfectly, but it's not that broken.
>
> But still not usable =(
Well now, I have fix all the issue with the alpha colorspace (I have added
alpha darken). All it remains to do is to make mask support the indirect
painting interface.

It doesn't use the gray color, but this is by design, it means you have to use
the opacity setting (this is something we have been debating in krita for
years, wether opacity belongs in the tool or in the color selector).
Gradient work if you choose a gradient with transparency, in a future release,
it could be worth investigating adding an option to the gradient tool for
working on the transparent channel instead of colors (I think it might have a
broader use).

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

On Saturday 26 September 2009, Dmitry Kazakov wrote:

> > Yes, all selection tools will not work on the mask but on the selection.
> > So even if you have selected the mask all changes with selection tools
> > will go to the global selection.
>
> Exactly!
>
> > We might want to kill the actions in the mask menu. See the other thread.
>
> We should kill them in layerBox and share them with mask manager.
>

Actually, the actions aren't implemented in the layerbox, but in the node
model. The layerbox only refers the the node manager (at least, it was that
way when I implemented it, I should check again.) But generally speaking, I
prefer the gui to be in the layerbox.

--
Boudewijn Rempt | http://www.valdyas.org
_______________________________________________
kimageshop mailing list
kimageshop@...
https://mail.kde.org/mailman/listinfo/kimageshop

On Saturday 26 September 2009, Cyrille Berger wrote:
> On Saturday 26 September 2009, Dmitry Kazakov wrote:
> > > for a,b,c) it doesn't work perfectly, but it's not that broken.
> >
> > But still not usable =(
>
> Well now, I have fix all the issue with the alpha colorspace (I have added
> alpha darken). All it remains to do is to make mask support the indirect
> painting interface.
>

Cool! A nice surprise when I woke up this morning.

> It doesn't use the gray color, but this is by design, it means you have to
>  use the opacity setting (this is something we have been debating in krita
>  for years, wether opacity belongs in the tool or in the color selector).
>  Gradient work if you choose a gradient with transparency, in a future
>  release, it could be worth investigating adding an option to the gradient
>  tool for working on the transparent channel instead of colors (I think it
>  might have a broader use).

I agree and it woud be easy to implement as well.

--
Boudewijn Rempt | http://www.valdyas.org
_______________________________________________
kimageshop mailing list
kimageshop@...
https://mail.kde.org/mailman/listinfo/kimageshop

On Wednesday 23 September 2009, Dmitry Kazakov wrote:

> This is a singlepatch version. It can be easily applied to trunk without
> conflicts. It's good for testing, not for reading:
> http://dimula73.narod.ru/01_layers_masks_refactoring_singlepatch.diff

There are a couple of stylistic issues I'm not too happy with, like
rearranging the order of methods in a header and some renaming. Right now, for
someone who's been working with Krita for a longer time, confusion is
guaranteed between the term 'projection; and the new term 'cache'.

These big refactors go better if we keep the renaming to the end, I think. I'm
still digesting your other mail, it's as long as the patch!
--
Boudewijn Rempt | http://www.valdyas.org
_______________________________________________
kimageshop mailing list
kimageshop@...
https://mail.kde.org/mailman/listinfo/kimageshop

On Friday 25 September 2009, 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.

So, that's what used to be called paintDevice(), right? I don't think the
rename adds much clarity.

> 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)

Ok, that's clear.

> 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.

That sounds good.

> 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).

That's impossible, the src paint device is const. I just checked the blur
filter and it doesn't touch the src paint device.

> 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
>

Ok -- I think you should apply this patch, if all unittests still run. The two
really important things after this are:

* fix projection
* fix pyramid


> 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.
>

Cool!

>
> *) 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.

This has been fixed by Cyrille :-).
 
> 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)

Yes, that's something that should be fixed. The original idea for selections
was different from what we have now: I wanted to have no selection tools at
all, and have the ordinary tools work on local selection masks and on the
global selection, with the global selection appearing in the layerbox as a
node. That would have made it possible, for instance, to drag any shape onto
the selection. Of course, tools like select contiguous area (bucket fill) or
select similar pixels would have to be implemented in a way that makes sense
for both ordinary layers and selections.

That is still my goal. The current state is not a release blocker, as far as I
am concerned, we'll have to rethink selection handling anway. And restore the
mask visualization, too :-).

> a),b) => c) There is no(!) way to paint on alpha-channels/masks
> currently. You simply CAN'T change the mask you created.

Fixed by Cyrille :P And even before that fix, I could select areas on a mask
using the filled circle or rectangle tool without problems other than the
projection not being updated.

> 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

Gaussian or ordinary blur? In any case, I don't see that happening, given that
the src paint device is const. And what happens in

void KisBlurFilter::process()

is

    KisConvolutionPainter painter(dst, dstInfo.selection());
    painter.applyMatrix(kernel, src, srcTopLeft, dstTopLeft, size,
BORDER_REPEAT);

so to me it looks as if the filter reads from src and writes to dst.

> 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.

Unfixable, this is a Qt problem. And we don't show the root layer by default,
which helps.

> 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!

I think Cyrille just fix that.

> [some design bugs]
> g) KisNodeManager::allowAsChild uses some silly string comparison
> algorithm. Why?! Every node has a specially designed method for this!

Because that predates the newer method? You have to realize that the Krita
code is about 10 years old. You will find historical artefacts all over the
place.

> h) KActions for mask-manipulation functions are duplicated in
> KisMaskManager and in KisLayerBox. They must be moved to
> KisMaskManager.

No, they aren't. The gui in the layerbox defers to the nodemanager for the
executions of the user actions.

>
> PS:
> btw, a,b,c,d,e,f are good, well-fed release blockers.
>


--
Boudewijn Rempt | http://www.valdyas.org
_______________________________________________
kimageshop mailing list
kimageshop@...
https://mail.kde.org/mailman/listinfo/kimageshop

[Dmitry Kazakov-2]

On Sat, Sep 26, 2009 at 10:14 AM, Cyrille Berger <cberger@...> wrote:

On Saturday 26 September 2009, Dmitry Kazakov wrote:
> > for a,b,c) it doesn't work perfectly, but it's not that broken.
>
> But still not usable =(

Well now, I have fix all the issue with the alpha colorspace (I have added
alpha darken). All it remains to do is to make mask support the indirect
painting interface.

Well, no. You've not fixed that. It's just a workaround.

Testcase:
1) Create any mask (e.g. transparency mask)
2) Paint something on a mask to get transparency

Let's imagine after these steps you decide to make some rect visible again, what are you going to do? In a good editor you just select this rect with selection and fill it with a white color (or any semi-transparent one(!)). You CAN'T do the same with krita, as far as i can see. Yes, you can use pixel eraser tool (that is not so obvious, btw), but what if your rect is 1024x1024 size? Are you gonna fill it with a small brush?
 
That's why i repeat again, we must use grayscale-like system for transparency masks. Current discreet selection system is a dirty hack.

 

It doesn't use the gray color, but this is by design, it means you have to use
the opacity setting (this is something we have been debating in krita for
years, wether opacity belongs in the tool or in the color selector).
Gradient work if you choose a gradient with transparency, in a future release,
it could be worth investigating adding an option to the gradient tool for
working on the transparent channel instead of colors (I think it might have a
broader use).

Do not create additional abstraction for a user. He knows what the color is, this is RGB values. The user DOES NOT know anything about our INTERNAL representation of the color as rgbA. RGB != RGBA. He knows (or should know) _nothing_ about alpha channel of the brush or of the layer. It's editor's job to think about alpha, not user's one!


PS:
Sorry for brute words. I don't know how to explain it. I repeat it again and again...
We have a weird selections system and you don't even want to make it better.


--
Dmitry Kazakov

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

[Dmitry Kazakov-2]

On Sat, Sep 26, 2009 at 10:14 AM, Cyrille Berger <> wrote:

On Saturday 26 September 2009, Dmitry Kazakov wrote:
> > for a,b,c) it doesn't work perfectly, but it's not that broken.
>
> But still not usable =(

Well now, I have fix all the issue with the alpha colorspace (I have added
alpha darken). All it remains to do is to make mask support the indirect
painting interface.

Btw, i've taken a look at the krita after your fixes. It really good and fixes some issues with selections.

I don't wanna say these fixes are not good, i just mean that the whole discreet system should be abolished and you or anyone else simply should not spend time on a brunch with no future. It's better to spend time on creating a new one.


--
Dmitry Kazakov

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

[Dmitry Kazakov-2]

On Sat, Sep 26, 2009 at 10:17 AM, Boudewijn Rempt <boud@...> wrote:

On Saturday 26 September 2009, Dmitry Kazakov wrote:

> > Yes, all selection tools will not work on the mask but on the selection.
> > So even if you have selected the mask all changes with selection tools
> > will go to the global selection.
>
> Exactly!
>
> > We might want to kill the actions in the mask menu. See the other thread.
>
> We should kill them in layerBox and share them with mask manager.
>

Actually, the actions aren't implemented in the layerbox, but in the node
model. The layerbox only refers the the node manager (at least, it was that
way when I implemented it, I should check again.) But generally speaking, I
prefer the gui to be in the layerbox.

What we have now:

KisMaskManager - has it's own KActions for all the mask operations
KisLayerManager - has it's own KActions for all the layer operations
KisLayerBox - has it's own KActions for all the layer ans mask operations (and some of them are created twice even)
This means that "Filter Layer..." menu-item has three(!) clones.

You don't think it's normal, do you?


As i remember KAction is created exactly to ensure that an action is created ONCE only. We just break this idea.

How it should be, i think:
KisMaskManager - has it's own KActions for all the mask operations
KisLayerManager - has it's own KActions for all the layer operations
KisLayerBox - just gets pointers for actions from KisMaskManager and KisLayerManager. It must not create any KAction related to masks or layers.


PS:

By the way, we are getting  "dividends" from this actions-duplication already. Filter masks have different names in menubar and in layerbox's context-menu.
"Filter Mask..." against "Effect Mask...".

And don't even try to simply change a string in KisMaskManager - this will be just another "dirty hack" that will help us to make our codebase completely unmaintainable. =(

--
Dmitry Kazakov

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

[Dmitry Kazakov-2]

> It doesn't use the gray color, but this is by design, it means you have to
>  use the opacity setting (this is something we have been debating in krita
>  for years, wether opacity belongs in the tool or in the color selector).
>  Gradient work if you choose a gradient with transparency, in a future
>  release, it could be worth investigating adding an option to the gradient
>  tool for working on the transparent channel instead of colors (I think it
>  might have a broader use).

I agree and it woud be easy to implement as well.

What is easy for a programmer is a complete disaster for a user. Programmer should think about the user first, not about the implementation.


--
Dmitry Kazakov

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

[Dmitry Kazakov-2]

On Sat, Sep 26, 2009 at 10:44 AM, Boudewijn Rempt <boud@...> wrote:

On Wednesday 23 September 2009, Dmitry Kazakov wrote:

> This is a singlepatch version. It can be easily applied to trunk without
> conflicts. It's good for testing, not for reading:
> http://dimula73.narod.ru/01_layers_masks_refactoring_singlepatch.diff

Yes, recent commits broke it. I'm compiling newly merged one.

 

There are a couple of stylistic issues I'm not too happy with, like
rearranging the order of methods in a header and some renaming.

I just wanted to make them more readable by grouping resembling methods.

 

Right now, for
someone who's been working with Krita for a longer time, confusion is
guaranteed between the term 'projection; and the new term 'cache'.

Btw, there is no new term 'cache' =) Do you mean KisGroupLayer::resetCache()?
I changed it from resetProjection to resetCache because KisAdjustmentLayer had already had similar method. They both should have the same name. And their name can't be resetProjection because now they reset 'original' paint device instead of 'projection'

I'll write about projections, originals and painDevices in a reply to your next mail.
 

These big refactors go better if we keep the renaming to the end, I think. I'm
still digesting your other mail, it's as long as the patch!

=)

--
Dmitry Kazakov

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

[Cyrille Berger]

On Saturday 26 September 2009, Dmitry Kazakov wrote:

> On Sat, Sep 26, 2009 at 10:14 AM, Cyrille Berger <cberger@...>wrote:
> > On Saturday 26 September 2009, Dmitry Kazakov wrote:
> > > > for a,b,c) it doesn't work perfectly, but it's not that broken.
> > >
> > > But still not usable =(
> >
> > Well now, I have fix all the issue with the alpha colorspace (I have
> > added alpha darken). All it remains to do is to make mask support the
> > indirect painting interface.
>
> Well, no. You've not fixed that. It's just a workaround.
>
> Testcase:
> 1) Create any mask (e.g. transparency mask)
> 2) Paint something on a mask to get transparency
>
> Let's imagine after these steps you decide to make some rect visible again,
> what are you going to do? In a good editor you just select this rect with
> selection and fill it with a white color (or any semi-transparent one(!)).

Why white ?

> You CAN'T do the same with krita, as far as i can see. Yes, you can use
> pixel eraser tool (that is not so obvious, btw), but what if your rect is
> 1024x1024 size?
No, I would say, there is a bug in the fill tool that needs to be fixed.

> Are you gonna fill it with a small brush?
you can use bigger brush....

> That's why i repeat again, we must use grayscale-like system for
> transparency masks. Current discreet selection system is a dirty hack.

Sorry, but I feel it's the other way around... How are you supposed to know
that white correspond to transparent ? (or is it black ?) While choosing
transparency for setting the transparency sounds more logical to me.

> > It doesn't use the gray color, but this is by design, it means you have
> > to use
> > the opacity setting (this is something we have been debating in krita for
> > years, wether opacity belongs in the tool or in the color selector).
> > Gradient work if you choose a gradient with transparency, in a future
> > release,
> > it could be worth investigating adding an option to the gradient tool for
> > working on the transparent channel instead of colors (I think it might
> > have a
> > broader use).
>
> Do not create additional abstraction for a user. He knows what the color
>  is, this is RGB values. The user DOES NOT know anything about our INTERNAL
>  representation of the color as rgbA. RGB != RGBA. He knows (or should
>  know) _nothing_ about alpha channel of the brush or of the layer. It's
>  editor's job to think about alpha, not user's one!

Hum ? alpha I would agree, but transparency... I sure hope the user would know
about transparency/opacity otherwise why would he want to use a transparency
mask in the first place ?
 
> PS:
> Sorry for brute words. I don't know how to explain it. I repeat it again
>  and again...
> We have a weird selections system and you don't even want to make it
>  better.
Sure I want, I just disagree with the assumption that color has a meaning for
selection.

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

[Cyrille Berger]

On Saturday 26 September 2009, Cyrille Berger wrote:
> > PS:
> > Sorry for brute words. I don't know how to explain it. I repeat it again
> >  and again...
> > We have a weird selections system and you don't even want to make it
> >  better.
>
> Sure I want, I just disagree with the assumption that color has a meaning
>  for  selection.
I also think we might want to discuss those issue with an interaction
designer, and probably discuss that at a live meeting. Because we are kind in
an impasse there.

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

[Dmitry Kazakov-2]

On Sat, Sep 26, 2009 at 1:33 PM, Boudewijn Rempt <> wrote:

On Friday 25 September 2009, 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.

So, that's what used to be called paintDevice(), right? I don't think the
rename adds much clarity.

No. Well, "not always".

paintDevice() - is a device where tools are painting on, e.g. selections of the adjustment layers.

paintDevice() and original() are the same for KisPaintLayer as the picture we paint on and a picture we show on a screen are the same.
But this is not right for KisSelectionBasedLayer (and it's child KisAdjustmentLayer). We paint on it's selection() paint device, but show on screen filtered image. That's why it has these methods:

KisPaintDeviceSP KisSelectionBasedLayer::original() const
{
    return m_d->paintDevice;
}
KisPaintDeviceSP KisSelectionBasedLayer::paintDevice() const
{
    return m_d->selection->getOrCreatePixelSelection();
}
 

> 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)

Ok, that's clear.

> 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.

That sounds good.

> 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).

That's impossible, the src paint device is const.

Btw, const KisSharedPtr does not mean that the object is constant too. That is a bug of shared pointers that we used to discuss on irc, don't know whether it has already been fixed. Has it?
 

I just checked the blur
filter and it doesn't touch the src paint device.

Yes, i can't reproduce it either now. I'll take a look at it. I always get empty dst device. Maybe the reason is that i pass it NULL selection?
 

> 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
>

Ok -- I think you should apply this patch, if all unittests still run.

Well, the build and they run. But there are some fails. Most of them are connected to a design.

 

The two
really important things after this are:

* fix projection

* fix pyramid

Well, i'm going to write a todo list, so shortly now:

The refactoring will contain three patches.
1st: Fixes masks and introduces needRect/changeRect infrastructure for layers/masks.
2nd: Creates bottom-up recursive update strategy that works with need/change rects
3rd: Using information, collected by new update strategy, creates an "update scheduler", that ensures that no two threads use the same rect at the same time.

And only after the third patch projection will not have flickering anymore.
The pyramid (more exactly, KisCanvas2) will use the same scheduler for threading scaling.


 

> 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.
>

Cool!

>
> *) 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.

This has been fixed by Cyrille :-).

Not really =(
 

> 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)

Yes, that's something that should be fixed. The original idea for selections
was different from what we have now: I wanted to have no selection tools at
all, and have the ordinary tools work on local selection masks and on the
global selection, with the global selection appearing in the layerbox as a
node. That would have made it possible, for instance, to drag any shape onto
the selection. Of course, tools like select contiguous area (bucket fill) or
select similar pixels would have to be implemented in a way that makes sense
for both ordinary layers and selections.

That is still my goal. The current state is not a release blocker, as far as I
am concerned, we'll have to rethink selection handling anway. And restore the
mask visualization, too :-).

> a),b) => c) There is no(!) way to paint on alpha-channels/masks
> currently. You simply CAN'T change the mask you created.

Fixed by Cyrille :P And even before that fix, I could select areas on a mask
using the filled circle or rectangle tool without problems other than the
projection not being updated.

Thanks to the hack that used to invert selection's paintDevice. Now you have to use an eraser to make a mask transparent, as my patch applies alpha channel directly.

Sorry.
(i've just find the way to make it transparent again! Eraser tool!) So i have to change the initial sentence: "there IS way to paint on masks, but you have to use eraser/brush to hide/show). Selection tools still do not work, but that is disputable, whether they should work _after_ creation of the mask.

PS_#1:
btw, i spent two weeks, to get to know that "Eraser" tool makes the opposite operation to a brush _on selections_. How do you think, how much time will user spend on this? And how to explain him that "the color of the brush makes no sense when painting on a mask"?

 

> 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

Gaussian or ordinary blur? In any case, I don't see that happening, given that
the src paint device is const. And what happens in

void KisBlurFilter::process()

is

   KisConvolutionPainter painter(dst, dstInfo.selection());
   painter.applyMatrix(kernel, src, srcTopLeft, dstTopLeft, size,
BORDER_REPEAT);

so to me it looks as if the filter reads from src and writes to dst.

Ok, i'll take a look at it. It doesn't work for me now.

 

> 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.

Unfixable, this is a Qt problem.

I don't think so. Doesn't it have an ability to control where we can drag an item to?

 

And we don't show the root layer by default,
which helps.

Sorry, "above it's parent layer to the empty place with no parent"

 

> 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!

I think Cyrille just fix that.

> [some design bugs]
> g) KisNodeManager::allowAsChild uses some silly string comparison
> algorithm. Why?! Every node has a specially designed method for this!

Because that predates the newer method? You have to realize that the Krita
code is about 10 years old. You will find historical artefacts all over the
place.

Cleaning?

 

> h) KActions for mask-manipulation functions are duplicated in
> KisMaskManager and in KisLayerBox. They must be moved to
> KisMaskManager.

No, they aren't. The gui in the layerbox defers to the nodemanager for the
executions of the user actions.

It breaks KAction paradigm. I've written about it before.

--
Dmitry Kazakov

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