From 423b6509382c3bcc36eca78ced0254bfb463d017 Mon Sep 17 00:00:00 2001 From: Eirik Aavitsland Date: Fri, 21 Jan 2022 08:05:29 +0100 Subject: [PATCH] Fix painting clipping glitches with fractional scaling 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 d9cc1499954829faf9486fb72056e29f1bad58e3 Task-number: QTBUG-100329 Fixes: QTBUG-95957 Task-number: QTBUG-100343 Pick-to: 6.3 Change-Id: Iaae6464b9b17f8bf3adc69007f6ef8d623bf2c80 Reviewed-by: Volker Hilsheimer --- src/gui/kernel/qhighdpiscaling_p.h | 7 +- src/gui/painting/qmath_p.h | 15 +++ src/gui/painting/qpaintengine_raster.cpp | 6 +- src/gui/painting/qtransform.cpp | 6 +- src/opengl/qopenglpaintengine.cpp | 2 +- .../gui/painting/qpainter/tst_qpainter.cpp | 104 +++++++++++++++++- tests/auto/gui/qopengl/tst_qopengl.cpp | 9 +- 7 files changed, 130 insertions(+), 19 deletions(-) diff --git a/src/gui/kernel/qhighdpiscaling_p.h b/src/gui/kernel/qhighdpiscaling_p.h index 89856570ce6..85527fe5983 100644 --- a/src/gui/kernel/qhighdpiscaling_p.h +++ b/src/gui/kernel/qhighdpiscaling_p.h @@ -219,10 +219,9 @@ inline QRegion scale(const QRegion ®ion, 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 diff --git a/src/gui/painting/qmath_p.h b/src/gui/painting/qmath_p.h index 0191f8381f0..6691665f419 100644 --- a/src/gui/painting/qmath_p.h +++ b/src/gui/painting/qmath_p.h @@ -53,12 +53,27 @@ #include #include +#include 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 diff --git a/src/gui/painting/qpaintengine_raster.cpp b/src/gui/painting/qpaintengine_raster.cpp index b044b485df8..6fbedabdbdd 100644 --- a/src/gui/painting/qpaintengine_raster.cpp +++ b/src/gui/painting/qpaintengine_raster.cpp @@ -49,7 +49,7 @@ #include #include #include -#include +#include "qmath_p.h" #include // #include @@ -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; } } @@ -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; } diff --git a/src/gui/painting/qtransform.cpp b/src/gui/painting/qtransform.cpp index 9d4f28c8954..c3189582078 100644 --- a/src/gui/painting/qtransform.cpp +++ b/src/gui/painting/qtransform.cpp @@ -45,7 +45,7 @@ #include "qpainterpath.h" #include "qpainterpath_p.h" #include "qvariant.h" -#include +#include "qmath_p.h" #include #include @@ -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 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); } diff --git a/src/opengl/qopenglpaintengine.cpp b/src/opengl/qopenglpaintengine.cpp index c385b4c62b6..98ab71dad5a 100644 --- a/src/opengl/qopenglpaintengine.cpp +++ b/src/opengl/qopenglpaintengine.cpp @@ -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; } diff --git a/tests/auto/gui/painting/qpainter/tst_qpainter.cpp b/tests/auto/gui/painting/qpainter/tst_qpainter.cpp index cfcaf5b67e1..c8bcd2f030f 100644 --- a/tests/auto/gui/painting/qpainter/tst_qpainter.cpp +++ b/tests/auto/gui/painting/qpainter/tst_qpainter.cpp @@ -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(); @@ -155,6 +158,8 @@ private slots: void clipBoundingRect(); void transformedClip(); + void scaledClipConsistency_data(); + void scaledClipConsistency(); void setOpacity_data(); void setOpacity(); @@ -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() { @@ -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)); } @@ -4583,6 +4589,96 @@ void tst_QPainter::transformedClip() } } +void tst_QPainter::scaledClipConsistency_data() +{ + QTest::addColumn("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 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() diff --git a/tests/auto/gui/qopengl/tst_qopengl.cpp b/tests/auto/gui/qopengl/tst_qopengl.cpp index e2e7f153a0f..a0a1ecca177 100644 --- a/tests/auto/gui/qopengl/tst_qopengl.cpp +++ b/tests/auto/gui/qopengl/tst_qopengl.cpp @@ -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() { @@ -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