<div class="gmail_quote">On Sat, Sep 26, 2009 at 1:33 PM, Boudewijn Rempt <span dir="ltr"><></span> wrote:<br><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<div class="im">On Friday 25 September 2009, Dmitry Kazakov wrote:<br>
> What is done in the patch.<br>
><br>
><br>
> 1) Now all the layers apply their own masks in a uniform way.<br>
><br>
> How it was before:<br>
> Every type of layer (paint, adjustment,etc) had it's own<br>
> updateProjection() function. And EVERY layer implemented it's own<br>
> cycle of applying masks. No need to say that they all differed. And<br>
> most of them was broken, because they didn't take into account<br>
> need/change rects of filters inside masks (at best).<br>
><br>
> How it is now:<br>
> updateProjection() and ALL the stuff about masks AND projections is<br>
> moved to KisLayer. Specialized layer can operate with only a few<br>
> functions those are not connected with a projection much. Every child<br>
> can overload these functions:<br>
><br>
> original() - returns a paint device where the layer stores it's<br>
> image. NOTE: This is not the same with projection() as the latter one<br>
> includes original() with all the masks applied.<br>
<br>
</div>So, that's what used to be called paintDevice(), right? I don't think the<br>
rename adds much clarity.<br></blockquote><div><br>No. Well, "not always". <br><br>paintDevice() - is a device where tools are painting on, e.g. selections of the adjustment layers.<br><br>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.<br>
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:<br><br>KisPaintDeviceSP KisSelectionBasedLayer::original() const<br>
{<br> return m_d->paintDevice;<br>}<br>KisPaintDeviceSP KisSelectionBasedLayer::paintDevice() const<br>{<br> return m_d->selection->getOrCreatePixelSelection();<br>}<br> <br></div><div><br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<div class="im">
> repaintOriginal() - called by updateProjection when original should be<br>
> repainted.<br>
><br>
> OPTIONAL:<br>
> A layer CAN (not should) overload:<br>
><br>
> needProjection() - if a layer needs an always-in-memory projection<br>
> (e.g. KisPaintLayer needs one for indirect painting)<br>
><br>
> copyOriginalToProjection() - used in couple with the previous one. A<br>
> layer can enhance original during painting on a projection<br>
> (e.g. indirect painting, see KisPaintDevice's method)<br>
><br>
><br>
> This all means that a KisLayer's child shouldn't do any work to create<br>
> a projection. It's created by KisLayer in a deferred way (when masks<br>
> are present)<br>
<br>
</div>Ok, that's clear.<br>
<div class="im"><br>
> Masks application strategy changed too (and quite much).<br>
> 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<br>
> want to copy from original() device.<br>
><br>
> Rect calculating algorithm is quite overwhelming. It's implemented in<br>
> two functions: masksChangeRect and masksNeedRect.<br>
> First we get a change rect of a DESTINATION PROJECTION with a first<br>
> function in a bottom-up(!) way. Then we calculate needRects for all<br>
> the masks in a top-down(!) way with a second function, and by the end<br>
> of this process we get a rect that we should copy from original()<br>
> device to a temporary device for applying filters.<br>
<br>
</div>That sounds good.<br>
<div class="im"><br>
> BUGFIX:<br>
> This part of a patch fixes a bug with a blur-filter-mask. In current<br>
> version you simply you get a slack if you paint on a layer with<br>
> blur-mask present (i'm not sure you can check it up now, because<br>
> blur-filter has an internal bug - it writes the result to the source<br>
> instead of destination).<br>
<br>
</div>That's impossible, the src paint device is const.</blockquote><div>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?<br>
</div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"> I just checked the blur<br>
filter and it doesn't touch the src paint device.<br></blockquote><div><br>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?<br>
</div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<div><div></div><div class="h5"><br>
> 2) The second task for this patch was to unify adjustment and<br>
> generator layers. The point is, they both use selections, that means<br>
> they both should work in the same way.<br>
><br>
> How it was before:<br>
> KisAdjustmentLayer and KisGeneratorLayer had code duplications for<br>
> selections stuff.<br>
><br>
> How it is now:<br>
> All the selection-related stuff is moved to a separate class -<br>
> KisSelectionBasedLayer. Both problematic classes are derived from this<br>
> base class. Speaking truly, KisAdjustmentLayer class has very few code<br>
> inside now.<br>
><br>
> BUGFIX:<br>
> Selections on selection based layers was not working at all<br>
> before. Now the the code applies selections (if you manage to<br>
> create one ;) (see * for more)) to the layers well.<br>
><br>
><br>
> 3) The third task was to clean masks classes. We had much code<br>
> duplication there too. Most duplications and misuses was connected to<br>
> selections (again).<br>
><br>
> How it was before:<br>
> KisFilterMaks, KisGeneratorMask, KisTransparencyMask - they all had<br>
> their own implementation<br>
><br>
> How it is now:<br>
> All the selection-related stuff is moved to KisMask class. Here is an<br>
> extract form a commit message:<br>
><br>
> Refactoring for masks<br>
><br>
> Now masks utilize selections in a uniform way. KisMask is the only<br>
> place that knows anything about mask's selection[0]. It's descendant's<br>
> do<br>
> very simple task, they just decorate the rect with the desired<br>
> effect[1]. The task made by KisTransparencyMask fell back to even more<br>
> trivial routine - it just needs to copy source image to destination,<br>
> without any modifications[2].<br>
><br>
> Another significant change accumulated by this commit is a small<br>
> cleanup of KisMaskManager. There are still many FIXME's for it. The<br>
> most annoying is code duplication [3] of KActions for masks.<br>
><br>
> [0] - KisMask::apply()<br>
> [1] - virtual KisMask::decorateRect()<br>
> [2] - KisTransparencyMask::decorateRect()<br>
> [3] - KisMaskManager's actions for mask functions are duplicated in<br>
> KisLayerBox.<br>
><br>
> As you can see, this commit accumulates some cleanups for<br>
> KisMaskManager too. Most of them remove code duplications and uniform<br>
> the code. More than that [and this is the only place where i worry<br>
> about the patch] this fix adds a new method(!) to KoDocument. It's<br>
> called undoLastCommand() - that is an equivalent to a user-undo<br>
> command. It is added to be able to cancel creation of an filter mask<br>
> in case a user presses 'Cancel' button in a creation dialog. Please<br>
> review this change!<br>
><br>
> BUGFIXES:<br>
> - cancelling creation of the filter mask now do not break undo stack<br>
> - selections (see *) on filter masks work well now<br>
><br>
<br>
</div></div>Ok -- I think you should apply this patch, if all unittests still run.</blockquote><div><br>Well, the build and they run. But there are some fails. Most of them are connected to a design.<br><br> </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
The two<br>
really important things after this are:<br>
<br>
* fix projection<br></blockquote><div></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
* fix pyramid<br></blockquote><div><br>Well, i'm going to write a todo list, so shortly now:<br><br>The refactoring will contain three patches.<br>1st: Fixes masks and introduces needRect/changeRect infrastructure for layers/masks.<br>
2nd: Creates bottom-up recursive update strategy that works with need/change rects<br>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.<br>
<br>And only after the third patch projection will not have flickering anymore.<br>The pyramid (more exactly, KisCanvas2) will use the same scheduler for threading scaling.<br><br><br> </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<div class="im">
> 4) Cleaning selections infrastructure, made me create a benchmark for<br>
> different selection application algorithms. This benchmark is placed<br>
> in ./image/tests/kis_filter_selections_benchmark.cpp. I wrote about it<br>
> already here.<br>
><br>
<br>
</div>Cool!<br>
<div class="im"><br>
><br>
> *) Still present bugs:<br>
><br>
> a) [the worst one] Painting on any alpha mask do not work because of<br>
> alpha() colorspace issue i was described before in this maillist. You<br>
> can paint if your mask is transparent, but the brush isn't. This is<br>
> not an issue of an algorithm. This is an issue of the alpha()<br>
> colorspace and the fact that we use brush's alpha channel for painting<br>
> alpha image-channel (tha latter assumption is completely wrong(!)).<br>
><br>
> Solution: Brush's dab should use grayscale colorspace during painting<br>
> on alpha-mask/selection. KisSelection should use grayscale, or a<br>
> special alpha_special() colorspace to work with grayscale channel of a<br>
> dab.<br>
<br>
</div>This has been fixed by Cyrille :-).<br></blockquote><div><br>Not really =(<br> </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<div class="im"><br>
> b) Vector selection tools DO NOT work on masks (And i'm not sure they<br>
> work on adjustment layers). The reason of the first fact (i think) is<br>
> that KisSelectionToolHelper class uses KisLayerSP type for internal<br>
> storage of a paint device. It should be changed to KisNodeSP to fix<br>
> that (of course alongside some other changes in logics of this class)<br>
<br>
</div>Yes, that's something that should be fixed. The original idea for selections<br>
was different from what we have now: I wanted to have no selection tools at<br>
all, and have the ordinary tools work on local selection masks and on the<br>
global selection, with the global selection appearing in the layerbox as a<br>
node. That would have made it possible, for instance, to drag any shape onto<br>
the selection. Of course, tools like select contiguous area (bucket fill) or<br>
select similar pixels would have to be implemented in a way that makes sense<br>
for both ordinary layers and selections.<br>
<br>
That is still my goal. The current state is not a release blocker, as far as I<br>
am concerned, we'll have to rethink selection handling anway. And restore the<br>
mask visualization, too :-).<br>
<div class="im"><br>
> a),b) => c) There is no(!) way to paint on alpha-channels/masks<br>
> currently. You simply CAN'T change the mask you created.<br>
<br>
</div>Fixed by Cyrille :P And even before that fix, I could select areas on a mask<br>
using the filled circle or rectangle tool without problems other than the<br>
projection not being updated.<br></blockquote><div><br>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.<br>
<br>Sorry.<br>(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. <br>
<br>PS_#1:<br>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"?<br>
<br> </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<div class="im">
> The only way to create a mask now is the following:<br>
> i) make a vector selection first<br>
> ii) create a mask - it'll derive this selection for it's own<br>
><br>
> [some minor bugs not dependent on the patch]<br>
> d) Blur filter works wrong. It writes filtered image to the source<br>
> device instead of the destination one<br>
<br>
</div>Gaussian or ordinary blur? In any case, I don't see that happening, given that<br>
the src paint device is const. And what happens in<br>
<br>
void KisBlurFilter::process()<br>
<br>
is<br>
<br>
KisConvolutionPainter painter(dst, dstInfo.selection());<br>
painter.applyMatrix(kernel, src, srcTopLeft, dstTopLeft, size,<br>
BORDER_REPEAT);<br>
<br>
so to me it looks as if the filter reads from src and writes to dst.<br></blockquote><div><br>Ok, i'll take a look at it. It doesn't work for me now. <br></div><div><br> </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<div class="im">
> e) Drag&Drop is broken in KisLayerBox. Just try to change an order of<br>
> masks with a mouse - you can easily lift it up above the root<br>
> layer. Of course, your next action will cause krita to crash.<br>
<br>
</div>Unfixable, this is a Qt problem.</blockquote><div>I don't think so. Doesn't it have an ability to control where we can drag an item to?<br><br> </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
And we don't show the root layer by default,<br>
which helps.<br></blockquote><div><br>Sorry, "above it's parent layer to the empty place with no parent"<br><br> </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<div class="im"><br>
> f) Curves filter DO NOT(!) work. I guess, there is some bug in<br>
> KoColorTransformation as every curve works as a brightness/contrast<br>
> curve instead of a channel curve. Just try it out!<br>
<br>
</div>I think Cyrille just fix that.<br>
<div class="im"><br>
> [some design bugs]<br>
> g) KisNodeManager::allowAsChild uses some silly string comparison<br>
> algorithm. Why?! Every node has a specially designed method for this!<br>
<br>
</div>Because that predates the newer method? You have to realize that the Krita<br>
code is about 10 years old. You will find historical artefacts all over the<br>
place.<br></blockquote><div><br>Cleaning?<br><br> </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<div class="im">
> h) KActions for mask-manipulation functions are duplicated in<br>
> KisMaskManager and in KisLayerBox. They must be moved to<br>
> KisMaskManager.<br>
<br>
</div>No, they aren't. The gui in the layerbox defers to the nodemanager for the<br>
executions of the user actions.<br></blockquote><div><br>It breaks KAction paradigm. I've written about it before.<br></div></div><br clear="all"><br>-- <br>Dmitry Kazakov<br>