[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-kimageshop
Subject: Re: koffice/krita/image
From: Dmitry Kazakov <dimula73 () gmail ! com>
Date: 2009-10-03 14:38:54
Message-ID: ae32c1ef0910030738s1fa48ea0x59462655fdf81cc8 () mail ! gmail ! com
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
On Sat, Oct 3, 2009 at 12:12 PM, Boudewijn Rempt <boud@valdyas.org> wrote:
> On Friday 02 October 2009, Dmitry Kazakov wrote:
>
> > Do you mean that for most of the filters this bitBlt() is simply a waste
> of
> > time, right? ;)
>
> Yes. Unfortunately, most of the filters _are_ convolution filters
like colorsfilters, right? :p
> or otherwise
> broken (like the old artistic filters). I briefly considered, way back, to
> add
> a flag to indicate this sort of brokenness, needsCopy()
no-no-no =) Just fix that.
> or something like
> that, but decided that that would just be compounding the problem.
>
> > We do already have several bitBlts during one merge operation, i think
> this
> > one is superfluous.
>
> It makes blur work in 2.1, so it's pretty useful :-)
>
> > If a filter doesn't want to write to some area he should declare this
> with
> > changeRect() function, but not just not write there.
>
> The area where a filter doesn't write isn't always a rect. Look at the
> funny
> results of the raindrops filter we had previously:
> http://rempt.xs4all.nl/fading/index.cgi/hacking/krita/amusing.comments.
>
The filter can check whether dst==src and optimize it's work... Just an
idea...
> > > > damn!
>
> > 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".
>
> That depends a lot on the phase of the project: we're now trying to
> stabilize
> 2.1 and get it ready for release.
The code should be clean and tidy all the time, not depending on the phase
of the Moon =) Otherwise it will never be good.
And, i think, there is no reason to be stabilizing code for several months,
and then refactor it completely right after release to make it unstable
again. There is no reason to make the same work twice. The code should be
clean since the beginning.
> That means really big refactorings are
> really risky, so I'm not prepared to go for a refactoring of the filter
> api.
> After 2.1 is branched, I'll happily consider any refactoring scheme again.
>
Filter api is absolutely ok. KisConvolutionPainter simply has a bug, and you
are suggesting to fix a mask to hide this problem.
>
> I'm totally happy with placing a big "XXX!!! Why do we need this copy! Fix
> after 2.1" in both places where we do this copy.
>
Please do this.
> > I'm continuously trying to explain this idea... And it seems that i
> fail...
>
> Well, sometimes you're expressing yourself a little forcefully, which leads
> to
> friction. And remember the prime rule of software development: there is
> always
> life after the release!
>
"Never put off till tomorrow what you can do today". Hiding a problem is
even worse.
> > Have you got the proof of wrong behavior of convolution painter?
>
> I haven't yet written a unittest for it, no. I was too busy actually fixing
> other bugs.
>
> > 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?
>
> KisFilterHandler defers to KisFilterJob which applies the filter directly.
> We
> had the idea once to simply swap projection and original if there were no
> other masks than the temporary mask, but that was never implemented. Also,
> if
> we ever implement region-of-interest recomposition (i.e., recomposite only
> the
> visible area) that will fail.
>
We should join "manual" application and masks application. That is another
code duplication.
--
Dmitry Kazakov
[Attachment #5 (text/html)]
<br><br><div class="gmail_quote">On Sat, Oct 3, 2009 at 12:12 PM, Boudewijn Rempt \
<span dir="ltr"><<a href="mailto:boud@valdyas.org">boud@valdyas.org</a>></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 02 \
October 2009, Dmitry Kazakov wrote:<br> <br>
</div><div class="im">> Do you mean that for most of the filters this bitBlt() is \
simply a waste of<br> > time, right? ;)<br>
<br>
</div>Yes. Unfortunately, most of the filters _are_ convolution \
filters</blockquote><div><br>like colorsfilters, right? :p<br> </div><blockquote \
class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt \
0pt 0.8ex; padding-left: 1ex;"> or otherwise<br>
broken (like the old artistic filters). I briefly considered, way back, to add<br>
a flag to indicate this sort of brokenness, needsCopy()</blockquote><div><br>no-no-no \
=) Just fix that.<br> </div><blockquote class="gmail_quote" style="border-left: 1px \
solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"> or \
something like<br> that, but decided that that would just be compounding the \
problem.<br> <div class="im"><br>
> We do already have several bitBlts during one merge operation, i think this<br>
> one is superfluous.<br>
<br>
</div>It makes blur work in 2.1, so it's pretty useful :-)<br>
<div class="im"><br>
> If a filter doesn't want to write to some area he should declare this \
with<br> > changeRect() function, but not just not write there.<br>
<br>
</div>The area where a filter doesn't write isn't always a rect. Look at the \
funny<br> results of the raindrops filter we had previously:<br>
<a href="http://rempt.xs4all.nl/fading/index.cgi/hacking/krita/amusing.comments" \
target="_blank">http://rempt.xs4all.nl/fading/index.cgi/hacking/krita/amusing.comments</a>.<br></blockquote><div><br>The \
filter can check whether dst==src and optimize it's work... Just an idea...<br> \
</div><div><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;"> > > \
> damn!<br> <div class="im"><br>
> I just want Krita's code to be clean and understandable. And i don't \
want<br> > to see any crutches there. I think if some (old) code is broken it \
should<br> > be fixed or disabled. And we mustn't break any new code too, \
only for the<br> > reason of "giving a crutch to the old one".<br>
<br>
</div>That depends a lot on the phase of the project: we're now trying to \
stabilize<br> 2.1 and get it ready for release. </blockquote><div><br>The code should \
be clean and tidy all the time, not depending on the phase of the Moon =) Otherwise \
it will never be good.<br>And, i think, there is no reason to be stabilizing code for \
several months, and then refactor it completely right after release to make it \
unstable again. There is no reason to make the same work twice. The code should be \
clean since the beginning.<br> </div><blockquote class="gmail_quote" \
style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; \
padding-left: 1ex;">That means really big refactorings are<br> really risky, so \
I'm not prepared to go for a refactoring of the filter api.<br> After 2.1 is \
branched, I'll happily consider any refactoring scheme \
again.<br></blockquote><div><br>Filter api is absolutely ok. KisConvolutionPainter \
simply has a bug, and you are suggesting to fix a mask to hide this problem.<br> \
</div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, \
204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"> <br>
I'm totally happy with placing a big "XXX!!! Why do we need this copy! \
Fix<br> after 2.1" in both places where we do this \
copy.<br></blockquote><div><br>Please do this.<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">
> I'm continuously trying to explain this idea... And it seems that i \
fail...<br> <br>
</div>Well, sometimes you're expressing yourself a little forcefully, which leads \
to<br> friction. And remember the prime rule of software development: there is \
always<br> life after the release!<br></blockquote><div><br>"Never put off till \
tomorrow what you can do today". Hiding a problem is even worse.<br> \
</div><div> </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">
> Have you got the proof of wrong behavior of convolution painter?<br>
<br>
</div>I haven't yet written a unittest for it, no. I was too busy actually \
fixing<br> other bugs.<br>
<div class="im"><br>
> hmm.. What do you mean by "manual filter application"? I thought if \
we<br> > apply a filter directly through Filters-menu they are added as a<br>
> TemporaryMask and merged to the layer then, aren't they?<br>
<br>
</div>KisFilterHandler defers to KisFilterJob which applies the filter directly. \
We<br> had the idea once to simply swap projection and original if there were no<br>
other masks than the temporary mask, but that was never implemented. Also, if<br>
we ever implement region-of-interest recomposition (i.e., recomposite only the<br>
visible area) that will fail.<br></blockquote><div><br>We should join \
"manual" application and masks application. That is another code \
duplication.<br><br></div></div><br>-- <br>Dmitry Kazakov<br>
_______________________________________________
kimageshop mailing list
kimageshop@kde.org
https://mail.kde.org/mailman/listinfo/kimageshop
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic