A first part of the layers/masks patch

[Dmitry Kazakov]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/kimageshop/attachments/20090926/456cfc82/attachment-0001.htm