[prev in list] [next in list] [prev in thread] [next in thread]
List: koffice-devel
Subject: Re: Review Request: Huge facelift to KisPainter in the bit blit
From: "Boudewijn Rempt" <boud () valdyas ! org>
Date: 2010-09-14 7:49:16
Message-ID: 20100914074916.6181.77867 () vidsolbach ! de
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://svn.reviewboard.kde.org/r/5318/#review7595
-----------------------------------------------------------
Ship it!
I especially like the mistake you spotted when trying to resize a fixed paint device: \
that's something we shouldn't do indeed. Looks good all over -- please commit!
- Boudewijn
On 2010-09-13 00:16:58, Pentalis wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://svn.reviewboard.kde.org/r/5318/
> -----------------------------------------------------------
>
> (Updated 2010-09-13 00:16:58)
>
>
> Review request for KOffice.
>
>
> Summary
> -------
>
> First an introduction (this is important) --->
>
> A few hours ago I read a sentence full of truth:
> "It's harder to read code than to write it."
> (source ---> http://www.joelonsoftware.com/articles/fog0000000069.html)
>
> I don't like it when it takes more effort to understand the wheel than to invent it \
> all over again. So I'm taking the challenge of giving Krita's code a facelift, \
> starting with KisPainter's bit blit functions which are the first ones I actually \
> managed to grasp (as a consequence of having to solve a nasty bug).
> Here follows an opinionated description of why I made most of the changes I made:
>
> KisPainter functions shouldn't try to be smarter than the plugin programmer. They \
> shouldn't fall back to safe behavior when they are requested to do nonsense. When I \
> was just learning to write paintops and I cluelessly tried to do things, I tended \
> to feed nonsense to KisPainter and I still got results. The problem was that it \
> took me a while to realize why what I did was stupid. What I really needed was a \
> good crash right on my face (and better documentation), like for example when I \
> tried to use a selection which wasn't Alpha 8 colorspace. That was a good crash. It \
> taught me something very important, really fast. So first I changed the first 3 \
> lines of all functions to Q_ASSERTS instead of returns. But to my dismay I got \
> crashes everywhere. Then I discovered the cause. Then I placed a nice comment right \
> there to save every new contributor the hassle of figuring why those things are \
> If-returns instead of Q_ASSERTS.
> Then I bumped to a bunch of code like this:
> // In case of COMPOSITE_COPY restricting bitblt to extent can
> // have unexpected behavior since it would reduce the area that
> // is copied.
> if (d->compositeOp->id() != COMPOSITE_COPY) {
> srcRect &= srcdev->extent();
> }
>
> dx += srcRect.x() - sx;
> dy += srcRect.y() - sy;
> sx = srcRect.x();
> sy = srcRect.y();
> sw = srcRect.width();
> sh = srcRect.height();
>
> Why do we ask the user (of the function) to provide the QRect encompassing an area \
> to be blitted if we're going to immediately re-size it to srcdev->extent()?. It \
> makes no sense, it doesn't protect memory, and it causes "surprising" behavior (in \
> the negative sense): you tell the function to do something and then it turns around \
> and does something else. So I went and deleted all those blocks of code and \
> replaced one of them (in bitBltWithFixedSelection) with a nice explanation of the \
> reasoning, such that all newcomers who see KisPainter's code don't have to spend \
> hours thinking of all the implications of this again (or feel dumbfounded looking \
> at cryptic code).
> Then I looked at the documentation and recalled how little help it was me back \
> then; I learned more about the functions by copying other people's code (I couldn't \
> read KisPainter's code, it was chinese to me, so... copying and essay-error were my \
> only options). I don't want any other student or contributor to experience that \
> ever again so I completely replaced all of the bit blit function documentation \
> entries for something more thorough and hand-holding.
> Also I renamed "sw" and "sh" which were horribly confusing names, for the more \
> self-evident "width" and "height". Of course, months later I discovered why "sw" \
> and "sh" made sense. But if variable names are going to take that long to grasp, \
> then they're not being helpful. The more the code feels like a novel, the faster \
> you'll have people contributing instead of scratching their heads wondering what \
> they're seeing. Names matter, It's not bikeshedding (*ahem* ;-) ).
> Other name changes:
> bitBltFixedSelection --> bitBltWithFixedSelection (clearer)
> bltFixed ---> bltFixedWithFixedSelection (much more clear)
>
> Finally, apart from many other comments I left here and there. And a SNEAKY \
> copyright notice, I changed the way the 2 renamed functions work. I added arguments \
> to control the relative position of the rectangle on the fixed selection, and a \
> convenience function that assumes srcx (old sx), srcy (old sy), selx and sely as 0 \
> (they were equal to sx and sy back then), for people who like tidy code.
> With respect to the actual code, I have only 1 thing to warn:
>
> How do I do it such that the new bltFixed, bltFixedWithFixedSelection and \
> bitBltFixedSelection really behave as the documentation (that I made) says?, They \
> behave as expected when all the 4 source and selection coordinates are 0 (srcx, \
> srcy, selx, sely). And all the current paintops except DuplicateOp use it that way \
> so IT DOESN'T BREAK ANYTHING (even the DuplicateOp). But I'm sure that the "sel" \
> coordinates are not doing their job and I don't know how to best make them do it. \
> If I can't get them to do that job, I better remove the version with srcx, srcy, \
> selx and sely, and leave only the version which assumes them all to be 0 (which is \
> still good enough for all paintops we have, even DuplicateOp, I have just not \
> patched it yet).
> Also I fixed the KisPainterTest to not break with the new changes to \
> bitBltFixedSelection (now bitBltWithFixedSelection)
>
> All that's left is to improve the patch based on criticism, and for me to find a \
> way for selx and sely to work as intended (and make a UNIT TEST to prove it and \
> trace it; this is work for another day though).
> I made this post long because I didn't have time to make it shorter. (Sleepy to \
> death).
> Regards.
>
>
> Diffs
> -----
>
> /trunk/koffice/krita/image/kis_painter.h 1173868
> /trunk/koffice/krita/image/kis_painter.cc 1173868
> /trunk/koffice/krita/image/tests/kis_painter_test.cpp 1173868
> /trunk/koffice/krita/plugins/paintops/defaultpaintops/duplicate/kis_duplicateop.cpp \
> 1174156
> /trunk/koffice/krita/plugins/paintops/defaultpaintops/smudge/kis_smudgeop.cpp \
> 1174156
> /trunk/koffice/krita/plugins/paintops/deform/kis_deform_paintop.cpp 1174156
> /trunk/koffice/krita/plugins/paintops/hatching/kis_hatching_paintop.cpp 1174156
>
> Diff: http://svn.reviewboard.kde.org/r/5318/diff
>
>
> Testing
> -------
>
> The patch of course includes a fix to all the paintops that used the renamed \
> functions. I tested it with my tablet and with KisPainterTest and it's stable and \
> doesn't break, so it could be immediately applied without negative consequences to \
> stability.
>
> Thanks,
>
> Pentalis
>
>
[Attachment #5 (text/html)]
<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 \
solid;"> <tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://svn.reviewboard.kde.org/r/5318/">http://svn.reviewboard.kde.org/r/5318/</a>
</td>
</tr>
</table>
<br />
<p>Ship it!</p>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I especially like the \
mistake you spotted when trying to resize a fixed paint device: that's something \
we shouldn't do indeed. Looks good all over -- please commit!</pre> <br />
<p>- Boudewijn</p>
<br />
<p>On September 13th, 2010, 12:16 a.m., Pentalis wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" \
style="background-image: \
url('http://svn.reviewboard.kde.orgrb/images/review_request_box_top_bg.png'); \
background-position: left top; background-repeat: repeat-x; border: 1px black \
solid;"> <tr>
<td>
<div>Review request for KOffice.</div>
<div>By Pentalis.</div>
<p style="color: grey;"><i>Updated 2010-09-13 00:16:58</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: \
1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">First an introduction (this is important) --->
A few hours ago I read a sentence full of truth:
"It's harder to read code than to write it."
(source ---> http://www.joelonsoftware.com/articles/fog0000000069.html)
I don't like it when it takes more effort to understand the wheel than to invent \
it all over again. So I'm taking the challenge of giving Krita's code a \
facelift, starting with KisPainter's bit blit functions which are the first ones \
I actually managed to grasp (as a consequence of having to solve a nasty bug).
Here follows an opinionated description of why I made most of the changes I made:
KisPainter functions shouldn't try to be smarter than the plugin programmer. They \
shouldn't fall back to safe behavior when they are requested to do nonsense. When \
I was just learning to write paintops and I cluelessly tried to do things, I tended \
to feed nonsense to KisPainter and I still got results. The problem was that it took \
me a while to realize why what I did was stupid. What I really needed was a good \
crash right on my face (and better documentation), like for example when I tried to \
use a selection which wasn't Alpha 8 colorspace. That was a good crash. It taught \
me something very important, really fast. So first I changed the first 3 lines of all \
functions to Q_ASSERTS instead of returns. But to my dismay I got crashes everywhere. \
Then I discovered the cause. Then I placed a nice comment right there to save every \
new contributor the hassle of figuring why those things are If-returns instead of \
Q_ASSERTS.
Then I bumped to a bunch of code like this:
// In case of COMPOSITE_COPY restricting bitblt to extent can
// have unexpected behavior since it would reduce the area that
// is copied.
if (d->compositeOp->id() != COMPOSITE_COPY) {
srcRect &= srcdev->extent();
}
dx += srcRect.x() - sx;
dy += srcRect.y() - sy;
sx = srcRect.x();
sy = srcRect.y();
sw = srcRect.width();
sh = srcRect.height();
Why do we ask the user (of the function) to provide the QRect encompassing an area to \
be blitted if we're going to immediately re-size it to srcdev->extent()?. It \
makes no sense, it doesn't protect memory, and it causes "surprising" \
behavior (in the negative sense): you tell the function to do something and then it \
turns around and does something else. So I went and deleted all those blocks of code \
and replaced one of them (in bitBltWithFixedSelection) with a nice explanation of the \
reasoning, such that all newcomers who see KisPainter's code don't have to \
spend hours thinking of all the implications of this again (or feel dumbfounded \
looking at cryptic code).
Then I looked at the documentation and recalled how little help it was me back then; \
I learned more about the functions by copying other people's code (I couldn't \
read KisPainter's code, it was chinese to me, so... copying and essay-error were \
my only options). I don't want any other student or contributor to experience \
that ever again so I completely replaced all of the bit blit function documentation \
entries for something more thorough and hand-holding.
Also I renamed "sw" and "sh" which were horribly confusing names, \
for the more self-evident "width" and "height". Of course, months \
later I discovered why "sw" and "sh" made sense. But if variable \
names are going to take that long to grasp, then they're not being helpful. The \
more the code feels like a novel, the faster you'll have people contributing \
instead of scratching their heads wondering what they're seeing. Names matter, \
It's not bikeshedding (*ahem* ;-) ).
Other name changes:
bitBltFixedSelection --> bitBltWithFixedSelection (clearer)
bltFixed ---> bltFixedWithFixedSelection (much more clear)
Finally, apart from many other comments I left here and there. And a SNEAKY copyright \
notice, I changed the way the 2 renamed functions work. I added arguments to control \
the relative position of the rectangle on the fixed selection, and a convenience \
function that assumes srcx (old sx), srcy (old sy), selx and sely as 0 (they were \
equal to sx and sy back then), for people who like tidy code.
With respect to the actual code, I have only 1 thing to warn:
How do I do it such that the new bltFixed, bltFixedWithFixedSelection and \
bitBltFixedSelection really behave as the documentation (that I made) says?, They \
behave as expected when all the 4 source and selection coordinates are 0 (srcx, srcy, \
selx, sely). And all the current paintops except DuplicateOp use it that way so IT \
DOESN'T BREAK ANYTHING (even the DuplicateOp). But I'm sure that the \
"sel" coordinates are not doing their job and I don't know how to best \
make them do it. If I can't get them to do that job, I better remove the version \
with srcx, srcy, selx and sely, and leave only the version which assumes them all to \
be 0 (which is still good enough for all paintops we have, even DuplicateOp, I have \
just not patched it yet).
Also I fixed the KisPainterTest to not break with the new changes to \
bitBltFixedSelection (now bitBltWithFixedSelection)
All that's left is to improve the patch based on criticism, and for me to find a \
way for selx and sely to work as intended (and make a UNIT TEST to prove it and trace \
it; this is work for another day though).
I made this post long because I didn't have time to make it shorter. (Sleepy to \
death).
Regards.
</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: \
1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">The patch of course includes a fix to all the paintops that used the \
renamed functions. I tested it with my tablet and with KisPainterTest and it's \
stable and doesn't break, so it could be immediately applied without negative \
consequences to stability.</pre> </td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>/trunk/koffice/krita/image/kis_painter.h <span style="color: \
grey">(1173868)</span></li>
<li>/trunk/koffice/krita/image/kis_painter.cc <span style="color: \
grey">(1173868)</span></li>
<li>/trunk/koffice/krita/image/tests/kis_painter_test.cpp <span style="color: \
grey">(1173868)</span></li>
<li>/trunk/koffice/krita/plugins/paintops/defaultpaintops/duplicate/kis_duplicateop.cpp \
<span style="color: grey">(1174156)</span></li>
<li>/trunk/koffice/krita/plugins/paintops/defaultpaintops/smudge/kis_smudgeop.cpp \
<span style="color: grey">(1174156)</span></li>
<li>/trunk/koffice/krita/plugins/paintops/deform/kis_deform_paintop.cpp <span \
style="color: grey">(1174156)</span></li>
<li>/trunk/koffice/krita/plugins/paintops/hatching/kis_hatching_paintop.cpp <span \
style="color: grey">(1174156)</span></li>
</ul>
<p><a href="http://svn.reviewboard.kde.org/r/5318/diff/" style="margin-left: \
3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>
_______________________________________________
koffice-devel mailing list
koffice-devel@kde.org
https://mail.kde.org/mailman/listinfo/koffice-devel
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic