Skip to content

Commit

Permalink
Fix painting clipping glitches with fractional scaling
Browse files Browse the repository at this point in the history
In QPainter, clipping can only be done on whole pixels. The various
ways of specifying a clipping rectangle to the QPainter API have been
inconsistent in how fractional rectangles (either specified directly,
or as a result of fractional scaling) are mapped (rounded) to integer
coordinates.

Also, the mappings have not made sure to keep the edge-to-edge
property of clip rects under scaling. This is particularly important
when scaling QRegions with multiple rects, as QRegion is designed on
the assumption that an area can be described as a set of edge-to-edge
rects.

The fix rounds a clip rect identically with a fill rect. (Indeed, a
followup plan would be to merge QRasterPaintEngine's
toNormalizedFillRect() with the rectangle rounding function in this
commit).

Notably, a QRectF clip is now interpreted the same as a QPainterPath
clip describing the same area.

This modifies d9cc149

Task-number: QTBUG-100329
Fixes: QTBUG-95957
Task-number: QTBUG-100343
Pick-to: 6.3
Change-Id: Iaae6464b9b17f8bf3adc69007f6ef8d623bf2c80
Reviewed-by: Volker Hilsheimer <[email protected]>
  • Loading branch information
aavit authored and vohi committed Mar 2, 2022
1 parent 27fae72 commit 423b650
Show file tree
Hide file tree
Showing 7 changed files with 130 additions and 19 deletions.
7 changes: 3 additions & 4 deletions src/gui/kernel/qhighdpiscaling_p.h
Original file line number Diff line number Diff line change
Expand Up @@ -219,10 +219,9 @@ inline QRegion scale(const QRegion &region, qreal scaleFactor, QPoint origin = Q
if (!QHighDpiScaling::isActive())
return region;

QRegion scaled;
for (const QRect &rect : region)
scaled += scale(QRectF(rect), scaleFactor, origin).toRect();
return scaled;
QRegion scaled = region.translated(-origin);
scaled = QTransform::fromScale(scaleFactor, scaleFactor).map(scaled);
return scaled.translated(origin);
}

template <typename T>
Expand Down
15 changes: 15 additions & 0 deletions src/gui/painting/qmath_p.h
Original file line number Diff line number Diff line change
Expand Up @@ -53,12 +53,27 @@

#include <qmath.h>
#include <private/qglobal_p.h>
#include <qtransform.h>

QT_BEGIN_NAMESPACE

static const qreal Q_PI = qreal(M_PI); // pi
static const qreal Q_MM_PER_INCH = 25.4;

inline QRect qt_mapFillRect(const QRectF &rect, const QTransform &xf)
{
// Only for xf <= scaling or 90 degree rotations
Q_ASSERT(xf.type() <= QTransform::TxScale
|| (xf.type() == QTransform::TxRotate && qFuzzyIsNull(xf.m11()) && qFuzzyIsNull(xf.m22())));
// Transform the corners instead of the rect to avoid hitting numerical accuracy limit
// when transforming topleft and size separately and adding afterwards,
// as that can sometimes be slightly off around the .5 point, leading to wrong rounding
QPoint pt1 = xf.map(rect.topLeft()).toPoint();
QPoint pt2 = xf.map(rect.bottomRight()).toPoint();
// Normalize and adjust for the QRect vs. QRectF bottomright
return QRect::span(pt1, pt2).adjusted(0, 0, -1, -1);
}

QT_END_NAMESPACE

#endif // QMATH_P_H
6 changes: 3 additions & 3 deletions src/gui/painting/qpaintengine_raster.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@
#include <qpainterpath.h>
#include <qdebug.h>
#include <qbitmap.h>
#include <qmath.h>
#include "qmath_p.h"
#include <qrandom.h>

// #include <private/qdatabuffer_p.h>
Expand Down Expand Up @@ -1195,7 +1195,7 @@ void QRasterPaintEngine::clip(const QVectorPath &path, Qt::ClipOperation op)
#endif
const qreal *points = path.points();
QRectF r(points[0], points[1], points[4]-points[0], points[5]-points[1]);
if (setClipRectInDeviceCoords(s->matrix.mapRect(r).toAlignedRect(), op))
if (setClipRectInDeviceCoords(qt_mapFillRect(r, s->matrix), op))
return;
}
}
Expand Down Expand Up @@ -1255,7 +1255,7 @@ void QRasterPaintEngine::clip(const QRect &rect, Qt::ClipOperation op)
QPaintEngineEx::clip(rect, op);
return;

} else if (!setClipRectInDeviceCoords(s->matrix.mapRect(QRectF(rect)).toRect(), op)) {
} else if (!setClipRectInDeviceCoords(qt_mapFillRect(rect, s->matrix), op)) {
QPaintEngineEx::clip(rect, op);
return;
}
Expand Down
6 changes: 3 additions & 3 deletions src/gui/painting/qtransform.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@
#include "qpainterpath.h"
#include "qpainterpath_p.h"
#include "qvariant.h"
#include <qmath.h>
#include "qmath_p.h"
#include <qnumeric.h>

#include <private/qbezier_p.h>
Expand Down Expand Up @@ -1486,12 +1486,12 @@ QRegion QTransform::map(const QRegion &r) const
QRegion res;
if (m11() < 0 || m22() < 0) {
for (const QRect &rect : r)
res += mapRect(QRectF(rect)).toRect();
res += qt_mapFillRect(QRectF(rect), *this);
} else {
QVarLengthArray<QRect, 32> rects;
rects.reserve(r.rectCount());
for (const QRect &rect : r) {
QRect nr = mapRect(QRectF(rect)).toRect();
QRect nr = qt_mapFillRect(QRectF(rect), *this);
if (!nr.isEmpty())
rects.append(nr);
}
Expand Down
2 changes: 1 addition & 1 deletion src/opengl/qopenglpaintengine.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2526,7 +2526,7 @@ void QOpenGL2PaintEngineEx::clip(const QVectorPath &path, Qt::ClipOperation op)
&& qFuzzyIsNull(state()->matrix.m11())
&& qFuzzyIsNull(state()->matrix.m22())))
{
state()->rectangleClip = state()->rectangleClip.intersected(state()->matrix.mapRect(rect).toAlignedRect());
state()->rectangleClip &= qt_mapFillRect(rect, state()->matrix);
d->updateClipScissorTest();
return;
}
Expand Down
104 changes: 100 additions & 4 deletions tests/auto/gui/painting/qpainter/tst_qpainter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,9 @@ Q_OBJECT
public:
tst_QPainter();

enum ClipType { ClipRect, ClipRectF, ClipRegionSingle, ClipRegionMulti, ClipPathR, ClipPath };
Q_ENUM(ClipType);

private slots:
void cleanupTestCase();
void getSetCheck();
Expand Down Expand Up @@ -155,6 +158,8 @@ private slots:

void clipBoundingRect();
void transformedClip();
void scaledClipConsistency_data();
void scaledClipConsistency();

void setOpacity_data();
void setOpacity();
Expand Down Expand Up @@ -1752,10 +1757,11 @@ void tst_QPainter::setClipRect()

/*
Verify that the clipping works correctly.
The red outline should be covered by the blue rect on top and left,
while it should be clipped on the right and bottom and thus the red outline be visible
Just like fillRect, cliprect should snap rightwards and downwards in case of .5 coordinates.
The red outline should be covered by the blue rect on top,
while it should be clipped on the other edges and thus the red outline be visible
See: QTBUG-83229
See: QTBUG-83229, modified by QTBUG-100329
*/
void tst_QPainter::clipRect()
{
Expand All @@ -1781,7 +1787,7 @@ void tst_QPainter::clipRect()
p.end();

QCOMPARE(image.pixelColor(clipRect.left() + 1, clipRect.top()), QColor(Qt::blue));
QCOMPARE(image.pixelColor(clipRect.left(), clipRect.top() + 1), QColor(Qt::blue));
QCOMPARE(image.pixelColor(clipRect.left(), clipRect.top() + 1), QColor(Qt::red));
QCOMPARE(image.pixelColor(clipRect.left() + 1, clipRect.bottom()), QColor(Qt::red));
QCOMPARE(image.pixelColor(clipRect.right(), clipRect.top() + 1), QColor(Qt::red));
}
Expand Down Expand Up @@ -4583,6 +4589,96 @@ void tst_QPainter::transformedClip()
}
}

void tst_QPainter::scaledClipConsistency_data()
{
QTest::addColumn<ClipType>("clipType");

QTest::newRow("clipRect") << ClipRect;
QTest::newRow("clipRectF") << ClipRectF;
QTest::newRow("clipRegionSingle") << ClipRegionSingle;
QTest::newRow("clipRegionMulti") << ClipRegionMulti;
QTest::newRow("clipPathR") << ClipPathR;
QTest::newRow("clipPath") << ClipPath;
}

void tst_QPainter::scaledClipConsistency()
{
QFETCH(ClipType, clipType);

const QList<QRect> clipRects = {
// Varying odd and even coordinates and width/height
QRect(1, 1, 7, 8),
QRect(8, 0, 8, 9),
QRect(0, 9, 8, 7),
QRect(8, 9, 8, 7),
};
// Assert that these are edge to edge:
QPointF center = QRectF(clipRects[0]).bottomRight();
Q_ASSERT(QRectF(clipRects[1]).bottomLeft() == center);
Q_ASSERT(QRectF(clipRects[2]).topRight() == center);
Q_ASSERT(QRectF(clipRects[3]).topLeft() == center);

QRegion multiRegion;
for (const QRect &clipRect : clipRects)
multiRegion += clipRect;

QColor fillColor(Qt::black);
fillColor.setAlphaF(0.5);

for (int i = 100; i <= 300; i++) {
qreal dpr = qreal(i) / 100.0;
QImage img(QSize(16, 16) * dpr, QImage::Format_RGB32);
img.fill(Qt::white);
img.setDevicePixelRatio(dpr);

for (const QRect &clipRect : clipRects) {
QPainter p(&img);
switch (clipType) {
case ClipRect:
p.setClipRect(clipRect);
break;
case ClipRectF:
p.setClipRect(QRectF(clipRect));
break;
case ClipRegionSingle:
p.setClipRegion(QRegion(clipRect));
break;
case ClipRegionMulti:
p.setClipRegion(multiRegion);
break;
case ClipPath:
p.rotate(0.001); // Avoid the path being optimized to a rectf
Q_FALLTHROUGH();
case ClipPathR: {
QPainterPath path;
path.addRect(clipRect); // Will be recognized and converted back to a rectf
p.setClipPath(path);
break;
}
default:
Q_ASSERT(false);
break;
}
p.fillRect(p.window(), fillColor);
if (clipType == ClipRegionMulti)
break; // once is enough, we're not using the clipRect anyway
}

int qtWidth = img.width() / 4;
int qtHeight = img.height() / 4;
QPoint imgCenter = img.rect().center();
const QRgb targetColor = img.pixel(qtWidth, qtHeight);

// Test that there are no gaps or overlaps where the cliprects meet
for (int offset = -2; offset <= 2; offset++) {
QCOMPARE(img.pixel(imgCenter.x() + offset, qtHeight), targetColor);
QCOMPARE(img.pixel(imgCenter.x() + offset, img.height() - qtHeight), targetColor);
QCOMPARE(img.pixel(qtWidth, imgCenter.y() + offset), targetColor);
QCOMPARE(img.pixel(img.width() - qtWidth, imgCenter.y() + offset), targetColor);
}
}
}

#if defined(Q_OS_MAC)
// Only Mac supports sub pixel positions in raster engine currently
void tst_QPainter::drawText_subPixelPositionsInRaster_qtbug5053()
Expand Down
9 changes: 5 additions & 4 deletions tests/auto/gui/qopengl/tst_qopengl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1695,10 +1695,11 @@ void tst_QOpenGL::nullTextureInitializtion()

/*
Verify that the clipping works correctly.
The red outline should be covered by the blue rect on top and left,
while it should be clipped on the right and bottom and thus the red outline be visible
Just like fillRect, cliprect should snap rightwards and downwards in case of .5 coordinates.
The red outline should be covered by the blue rect on top,
while it should be clipped on the other edges and thus the red outline be visible
See: QTBUG-83229
See: QTBUG-83229, modified by QTBUG-100329
*/
void tst_QOpenGL::clipRect()
{
Expand Down Expand Up @@ -1754,7 +1755,7 @@ void tst_QOpenGL::clipRect()
QCOMPARE(fb.size(), size);

QCOMPARE(fb.pixelColor(clipRect.left() + 1, clipRect.top()), QColor(Qt::blue));
QCOMPARE(fb.pixelColor(clipRect.left(), clipRect.top() + 1), QColor(Qt::blue));
QCOMPARE(fb.pixelColor(clipRect.left(), clipRect.top() + 1), QColor(Qt::red));
QCOMPARE(fb.pixelColor(clipRect.left() + 1, clipRect.bottom()), QColor(Qt::red));

// Enable this once QTBUG-85286 is fixed
Expand Down

0 comments on commit 423b650

Please sign in to comment.