koffice/krita/image

[Dmitry Kazakov]

On Fri, Oct 2, 2009 at 6:00 PM, Boudewijn Rempt <boud at valdyas.org> wrote:

> On Friday 02 October 2009, Dmitry Kazakov wrote:
>
> > It depends on a type of a bug inside convolution painter. If it reads out
> > of dst device, then we'll get the same border-effect problem that i have
> > been fixing for about a month. If it just uses dst as a default value,
> > it'll work, but we can get even more subtle problems in the future, like
> a
> > difference between changeRect of a filter and rc-value transmitted to the
> > decorateRect() function.
>
> Since originally the paint device we were writing to was empty, that
> shouldn't
> matter. Most filters will overwrite everything, for the others it won't
> matter, should it?
>

Do you mean that for most of the filters this bitBlt() is simply a waste of
time, right? ;)

We do already have several bitBlts during one merge operation, i think this
one is superfluous.
If a filter doesn't want to write to some area he should declare this with
changeRect() function, but not just not write there.




>
> > damn!
>
> Please work from a basic assumption of competence in the rest of the team,
> i.e., assume we know what we are doing with the code we have been working
> on
> for years. I'd like this project to stay a friendly place.
>

Sorry. I was wrong.

I just want Krita's code to be clean and understandable. And i don't want to
see any crutches there. I think if some (old) code is broken it should be
fixed or disabled. And we mustn't break any new code too, only for the
reason of "giving a crutch to the old one".
I'm continuously trying to explain this idea... And it seems that i fail...
:(



>  > Well, Boud, isn't it easier to fix a bug in convolution painter than to
> > create a garbage from our KisLayer/Mask code?
>
> No, definitely not. I took a look at the code of the convolution painter
> and
> couldn't find any reason for this behaviour.
>

Have you got the proof of wrong behavior of convolution painter?



> Besides, this commit makes the way the filter mask applies filters
> consistent
> with the way the manual filter application works and so offers a better
> starting point for refactoring after the release.
>

hmm.. What do you mean by "manual filter application"? I thought if we apply
a filter directly through Filters-menu they are added as a TemporaryMask and
merged to the layer then, aren't they?



>  > PS:
> > I simply can't understand, why are you going to fix KisLayer and KisMask,
> >  if you know exactly that real problem is inside KisConvolutionPainter?
>
> I do not know what the problem is in the convolution painter.
>

=)


>  > I don't understand something, do i?
>
> Besides, there is nothing in the api of KisFilter that explicitly declares
> that filters always need to write all pixels, changed an unchanged to dst,
> so
> dst should be prepared with a copy of src.
>

Quite illogic, i think. Every process-unit has in-point and out-point. It
reads from in- and writes to out-point. And no pre-assumptions.



> Whether that is a good api or not, I don't have an opinion about: I'll let
> you
> and Cyrille discuss that. We have had a single-paint device api before, and
> that didn't work out either.
>

I don't think it's good to have one, especially if we continue using bitBlt
for selections.
In such a case we'll have four loops through entire rc instead of three.

"double-paintdevice" loops:
1-st - bitBlt'ing an image in KisLayer::applyMasks
2-nd - KisFilter's loop ( src ----filtering----> dst )
3-rd - bitBlt'ing in KisMask::apply() to take a selection into account

"single-paintdevice" loops:
1-st - bitBlt'ing an image in KisLayer::applyMasks
2-nd - bitBlt'in in KisImage to leave an original copy untouched by a filter
(for applying a selection then)
3-rd - KisFilter's loop ( src ----filtering----> src )
4-th- bitBlt'ing in KisMask::apply() to take a selection into account



-- 
Dmitry Kazakov
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/kimageshop/attachments/20091002/07138309/attachment-0001.htm