A first part of the layers/masks patch

[Boudewijn Rempt]

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