[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-kimageshop
Subject: Re: Proposed change to KisPainter, fix for bug 246639
From: Dmitry Kazakov <dimula73 () gmail ! com>
Date: 2010-09-03 18:38:35
Message-ID: AANLkTikk3mkS00uZNpkmrNpgc2H+89=Ov7zGfCxLCiXp () mail ! gmail ! com
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
I'll take a look at this tomorrow morning =)
On Thu, Sep 2, 2010 at 3:31 PM, JL VT <> wrote:
> https://bugs.kde.org/show_bug.cgi?id=246639
>
> This bug had a basic solution which was mostly a workaround I did in
> the Smudge Brush and Hatching Brush.
> Slangkamp proposed that I generalized the solution. Much discussion
> was had and me and DmitryK concluded that the right solution was to
> change bitBltFixedSelection in KisPainter. This function is used only
> by 3 classes in all of Krita (SmudgeOp, HatchingOp, DuplicateOp), all
> of them were coded "believing" that the function fused the mask with
> the canvas selection, but such was not the case, only the mask was
> used.
> This patch changes this behavior and causes
> KisPainter::bitBltFixedSelection to first check if there is a
> selection in the canvas, if there is none, it works exactly as it used
> to (minus a memory leak it had); but if there is one, it fuses the
> canvas selection with the mask in the relevant area before blitting,
> thus respecting selections.
>
> Were this change accepted, I'd follow it by restoring the Hatching
> Brush and Smudge Brush to simply use KisPainter::bitBltFixedSelection
> without the bug workaround.
>
> Below is the new KisPainter::bitBltFixedSelection.
>
> PS: excuse the poor excuse of a "patch" to review, I haven't setup my
> system for SVN contributions, but I didn't want to waste the chance to
> let people review the code while I'm off reinstalling it and then
> sleeping.
>
>
> void KisPainter::bitBltFixedSelection(qint32 dx, qint32 dy, const
> KisPaintDeviceSP srcdev, const KisFixedPaintDeviceSP selection, qint32
> sx, qint32 sy, qint32 sw, qint32 sh)
> {
> if (sw == 0 || sh == 0) return;
> if (srcdev.isNull()) return;
> if (d->device.isNull()) return;
>
> Q_ASSERT(srcdev->pixelSize() == d->pixelSize);
> Q_ASSERT(selection->colorSpace() ==
> KoColorSpaceRegistry::instance()->alpha8());
>
> QRect srcRect = QRect(sx, sy, sw, sh);
> QRect dstRect = QRect(dx, dy, sw, sh);
>
> bool selectedness;
>
> // Check if there is a selection
> if (d->selection.isNull())
> selectedness = false;
> else if (d->selection->isDeselected())
> selectedness = false;
> else
> selectedness = true;
>
> // 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();
>
> if (srcRect.isEmpty())
> return;
>
> dx += srcRect.x() - sx;
> dy += srcRect.y() - sy;
>
> sx = srcRect.x();
> sy = srcRect.y();
> sw = srcRect.width();
> sh = srcRect.height();
>
> quint8 * srcBytes = new quint8[ sw * sh * srcdev->pixelSize() ];
> srcdev->readBytes(srcBytes, sx, sy, sw, sh);
>
> quint8 * dstBytes = new quint8[ sw * sh * d->device->pixelSize() ];
> d->device->readBytes(dstBytes, dx, dy, sw, sh);
>
> quint8 * mergedSelectionBytes = new quint8[ sw * sh *
> selection->pixelSize() ];
>
> if (!selectedness) {
> selection->readBytes(mergedSelectionBytes, 0, 0, sw, sh);
> }
> else {
> if (selection->pixelSize() != d->selection->pixelSize())
> qWarning("Error while performing
> KisPainter::bitBltFixedSelection, selection and mask had different
> pixel size, no merge performed");
> else {
> // Here selection->pixelSize() is known to be the same as
> d->selection->pixelSize(), both should be equal to 1
> quint32 totalPixels = sw * sh * selection->pixelSize();
>
> quint8 * selectionBytes = new quint8[ totalPixels ];
> d->selection->readBytes(selectionBytes, dx, dy, sw, sh);
>
> quint8 * maskBytes = new quint8[ totalPixels ];
> selection->readBytes(maskBytes, 0, 0, sw, sh);
>
> /* Some algebra-fu happens here, basically:
> z = x * y (conditional x * y < 1)
> But that's only valid when all numbers are between 0 and 1.
> For values between 0 and 255:
> z/255 = x/255 * y/255
> Conditional:
> x/255 * y/255 < 1 applying *255 on both sides results in
> x * y/255 < 255
> That explains the simplified formula used below.
> 255 is replaced by MAX_SELECTED
> z is replaced by mergedSelectionBytes
> and x and y by selectionBytes and maskBytes
> */
> for (quint32 i = 0; i < totalPixels; ++i) {
> if (selectionBytes[i] * maskBytes[i] / MAX_SELECTED <
> MAX_SELECTED )
> mergedSelectionBytes[i] = selectionBytes[i] *
> maskBytes[i] / MAX_SELECTED;
> else
> mergedSelectionBytes[i] = MAX_SELECTED;
> }
>
> delete [] selectionBytes;
> delete [] maskBytes;
> }
> }
>
> d->colorSpace->bitBlt(dstBytes,
> sw * d->device->pixelSize(),
> srcdev->colorSpace(),
> srcBytes,
> sw * srcdev->colorSpace()->pixelSize(),
> mergedSelectionBytes,
> sw * selection->pixelSize(),
> d->opacity,
> sh,
> sw,
> d->compositeOp,
> d->channelFlags);
>
> d->device->writeBytes(dstBytes, dx, dy, sw, sh);
>
> delete [] srcBytes;
> delete [] dstBytes;
> delete [] mergedSelectionBytes;
>
> addDirtyRect(QRect(dx, dy, sw, sh));
> }
> _______________________________________________
> kimageshop mailing list
> kimageshop@kde.org
> https://mail.kde.org/mailman/listinfo/kimageshop
>
--
Dmitry Kazakov
[Attachment #5 (text/html)]
I'll take a look at this tomorrow morning =)<br><br><br><div \
class="gmail_quote">On Thu, Sep 2, 2010 at 3:31 PM, JL VT <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;"> <a href="https://bugs.kde.org/show_bug.cgi?id=246639" \
target="_blank">https://bugs.kde.org/show_bug.cgi?id=246639</a><br> <br>
This bug had a basic solution which was mostly a workaround I did in<br>
the Smudge Brush and Hatching Brush.<br>
Slangkamp proposed that I generalized the solution. Much discussion<br>
was had and me and DmitryK concluded that the right solution was to<br>
change bitBltFixedSelection in KisPainter. This function is used only<br>
by 3 classes in all of Krita (SmudgeOp, HatchingOp, DuplicateOp), all<br>
of them were coded "believing" that the function fused the mask with<br>
the canvas selection, but such was not the case, only the mask was<br>
used.<br>
This patch changes this behavior and causes<br>
KisPainter::bitBltFixedSelection to first check if there is a<br>
selection in the canvas, if there is none, it works exactly as it used<br>
to (minus a memory leak it had); but if there is one, it fuses the<br>
canvas selection with the mask in the relevant area before blitting,<br>
thus respecting selections.<br>
<br>
Were this change accepted, I'd follow it by restoring the Hatching<br>
Brush and Smudge Brush to simply use KisPainter::bitBltFixedSelection<br>
without the bug workaround.<br>
<br>
Below is the new KisPainter::bitBltFixedSelection.<br>
<br>
PS: excuse the poor excuse of a "patch" to review, I haven't setup \
my<br> system for SVN contributions, but I didn't want to waste the chance to<br>
let people review the code while I'm off reinstalling it and then<br>
sleeping.<br>
<br>
<br>
void KisPainter::bitBltFixedSelection(qint32 dx, qint32 dy, const<br>
KisPaintDeviceSP srcdev, const KisFixedPaintDeviceSP selection, qint32<br>
sx, qint32 sy, qint32 sw, qint32 sh)<br>
{<br>
if (sw == 0 || sh == 0) return;<br>
if (srcdev.isNull()) return;<br>
if (d->device.isNull()) return;<br>
<br>
Q_ASSERT(srcdev->pixelSize() == d->pixelSize);<br>
Q_ASSERT(selection->colorSpace() ==<br>
KoColorSpaceRegistry::instance()->alpha8());<br>
<br>
QRect srcRect = QRect(sx, sy, sw, sh);<br>
QRect dstRect = QRect(dx, dy, sw, sh);<br>
<br>
bool selectedness;<br>
<br>
// Check if there is a selection<br>
if (d->selection.isNull())<br>
selectedness = false;<br>
else if (d->selection->isDeselected())<br>
selectedness = false;<br>
else<br>
selectedness = true;<br>
<br>
// In case of COMPOSITE_COPY restricting bitblt to extent can<br>
// have unexpected behavior since it would reduce the area that<br>
// is copied.<br>
if (d->compositeOp->id() != COMPOSITE_COPY)<br>
srcRect &= srcdev->extent();<br>
<br>
if (srcRect.isEmpty())<br>
return;<br>
<br>
dx += srcRect.x() - sx;<br>
dy += srcRect.y() - sy;<br>
<br>
sx = srcRect.x();<br>
sy = srcRect.y();<br>
sw = srcRect.width();<br>
sh = srcRect.height();<br>
<br>
quint8 * srcBytes = new quint8[ sw * sh * srcdev->pixelSize() ];<br>
srcdev->readBytes(srcBytes, sx, sy, sw, sh);<br>
<br>
quint8 * dstBytes = new quint8[ sw * sh * d->device->pixelSize() ];<br>
d->device->readBytes(dstBytes, dx, dy, sw, sh);<br>
<br>
quint8 * mergedSelectionBytes = new quint8[ sw * sh *<br>
selection->pixelSize() ];<br>
<br>
if (!selectedness) {<br>
selection->readBytes(mergedSelectionBytes, 0, 0, sw, sh);<br>
}<br>
else {<br>
if (selection->pixelSize() != d->selection->pixelSize())<br>
qWarning("Error while performing<br>
KisPainter::bitBltFixedSelection, selection and mask had different<br>
pixel size, no merge performed");<br>
else {<br>
// Here selection->pixelSize() is known to be the same as<br>
d->selection->pixelSize(), both should be equal to 1<br>
quint32 totalPixels = sw * sh * selection->pixelSize();<br>
<br>
quint8 * selectionBytes = new quint8[ totalPixels ];<br>
d->selection->readBytes(selectionBytes, dx, dy, sw, sh);<br>
<br>
quint8 * maskBytes = new quint8[ totalPixels ];<br>
selection->readBytes(maskBytes, 0, 0, sw, sh);<br>
<br>
/* Some algebra-fu happens here, basically:<br>
z = x * y (conditional x * y < 1)<br>
But that's only valid when all numbers are between 0 and \
1.<br> For values between 0 and 255:<br>
z/255 = x/255 * y/255<br>
Conditional:<br>
x/255 * y/255 < 1 applying *255 on both sides \
results in<br> x * y/255 < 255<br>
That explains the simplified formula used below.<br>
255 is replaced by MAX_SELECTED<br>
z is replaced by mergedSelectionBytes<br>
and x and y by selectionBytes and maskBytes<br>
*/<br>
for (quint32 i = 0; i < totalPixels; ++i) {<br>
if (selectionBytes[i] * maskBytes[i] / MAX_SELECTED <<br>
MAX_SELECTED )<br>
mergedSelectionBytes[i] = selectionBytes[i] *<br>
maskBytes[i] / MAX_SELECTED;<br>
else<br>
mergedSelectionBytes[i] = MAX_SELECTED;<br>
}<br>
<br>
delete [] selectionBytes;<br>
delete [] maskBytes;<br>
}<br>
}<br>
<br>
d->colorSpace->bitBlt(dstBytes,<br>
sw * d->device->pixelSize(),<br>
srcdev->colorSpace(),<br>
srcBytes,<br>
sw * \
srcdev->colorSpace()->pixelSize(),<br> mergedSelectionBytes,<br>
sw * selection->pixelSize(),<br>
d->opacity,<br>
sh,<br>
sw,<br>
d->compositeOp,<br>
d->channelFlags);<br>
<br>
d->device->writeBytes(dstBytes, dx, dy, sw, sh);<br>
<br>
delete [] srcBytes;<br>
delete [] dstBytes;<br>
delete [] mergedSelectionBytes;<br>
<br>
addDirtyRect(QRect(dx, dy, sw, sh));<br>
}<br>
_______________________________________________<br>
kimageshop mailing list<br>
<a href="mailto:kimageshop@kde.org">kimageshop@kde.org</a><br>
<a href="https://mail.kde.org/mailman/listinfo/kimageshop" \
target="_blank">https://mail.kde.org/mailman/listinfo/kimageshop</a><br> \
</blockquote></div><br><br clear="all"><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