From e84ab1fee7f44a28ee82793f83b0b27d04d28c09 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B8rn=20Erik=20Nilsen?= Date: Wed, 6 Oct 2010 14:19:44 +0200 Subject: QGraphicsItem device coordinate cache unefficient in portrait mode Problem was that we always invalidated the cache whenever the item was rotated. This is however not required for simple rotations such as 90, 180 and 270 degrees. This commit also removes the somewhat arbitrary logic which takes the desktop size into account. We now use the viewport size instead. Auto test included. Task-number: QT-3779 Reviewed-by: yoann --- src/gui/graphicsview/qgraphicsscene.cpp | 76 ++++++++++++++++++++------ tests/auto/qgraphicsitem/tst_qgraphicsitem.cpp | 76 ++++++++++++++++++++++++++ 2 files changed, 134 insertions(+), 18 deletions(-) diff --git a/src/gui/graphicsview/qgraphicsscene.cpp b/src/gui/graphicsview/qgraphicsscene.cpp index e58b93c..a0015dc 100644 --- a/src/gui/graphicsview/qgraphicsscene.cpp +++ b/src/gui/graphicsview/qgraphicsscene.cpp @@ -4362,6 +4362,50 @@ static void _q_paintIntoCache(QPixmap *pix, QGraphicsItem *item, const QRegion & } } +// Copied from qpaintengine_vg.cpp +// Returns true for 90, 180, and 270 degree rotations. +static inline bool transformIsSimple(const QTransform& transform) +{ + QTransform::TransformationType type = transform.type(); + if (type == QTransform::TxNone || type == QTransform::TxTranslate) { + return true; + } else if (type == QTransform::TxScale) { + // Check for 0 and 180 degree rotations. + // (0 might happen after 4 rotations of 90 degrees). + qreal m11 = transform.m11(); + qreal m12 = transform.m12(); + qreal m21 = transform.m21(); + qreal m22 = transform.m22(); + if (m12 == 0.0f && m21 == 0.0f) { + if (m11 == 1.0f && m22 == 1.0f) + return true; // 0 degrees + else if (m11 == -1.0f && m22 == -1.0f) + return true; // 180 degrees. + if(m11 == 1.0f && m22 == -1.0f) + return true; // 0 degrees inverted y. + else if(m11 == -1.0f && m22 == 1.0f) + return true; // 180 degrees inverted y. + } + } else if (type == QTransform::TxRotate) { + // Check for 90, and 270 degree rotations. + qreal m11 = transform.m11(); + qreal m12 = transform.m12(); + qreal m21 = transform.m21(); + qreal m22 = transform.m22(); + if (m11 == 0.0f && m22 == 0.0f) { + if (m12 == 1.0f && m21 == -1.0f) + return true; // 90 degrees. + else if (m12 == -1.0f && m21 == 1.0f) + return true; // 270 degrees. + else if (m12 == -1.0f && m21 == -1.0f) + return true; // 90 degrees inverted y. + else if (m12 == 1.0f && m21 == 1.0f) + return true; // 270 degrees inverted y. + } + } + return false; +} + /*! \internal @@ -4530,32 +4574,28 @@ void QGraphicsScenePrivate::drawItemHelper(QGraphicsItem *item, QPainter *painte if (invertable) diff *= painter->worldTransform(); deviceData->lastTransform = painter->worldTransform(); - if (!invertable - || diff.type() > QTransform::TxTranslate - || painter->worldTransform().type() > QTransform::TxScale) { + bool allowPartialCacheExposure = false; + bool simpleTransform = invertable && diff.type() <= QTransform::TxTranslate + && transformIsSimple(painter->worldTransform()); + if (!simpleTransform) { pixModified = true; itemCache->allExposed = true; itemCache->exposed.clear(); + deviceData->cacheIndent = QPoint(); pix = QPixmap(); + } else { + allowPartialCacheExposure = deviceData->cacheIndent != QPoint(); } - // ### This is a pretty bad way to determine when to start partial - // exposure for DeviceCoordinateCache but it's the least intrusive - // approach for now. -#if 0 - // Only if the device rect isn't fully contained. - bool allowPartialCacheExposure = !viewRect.contains(deviceRect); -#else - // Only if deviceRect is 20% taller or wider than the desktop. - bool allowPartialCacheExposure = false; - if (widget) { - QRect desktopRect = QApplication::desktop()->availableGeometry(widget); - allowPartialCacheExposure = (desktopRect.width() * 1.2 < deviceRect.width() - || desktopRect.height() * 1.2 < deviceRect.height()); + // Allow partial cache exposure if the device rect isn't fully contained and + // deviceRect is 20% taller or wider than the viewRect. + if (!allowPartialCacheExposure && !viewRect.contains(deviceRect)) { + allowPartialCacheExposure = (viewRect.width() * 1.2 < deviceRect.width()) + || (viewRect.height() * 1.2 < deviceRect.height()); } -#endif + QRegion scrollExposure; - if (deviceData->cacheIndent != QPoint() || allowPartialCacheExposure) { + if (allowPartialCacheExposure) { // Part of pixmap is drawn. Either device contains viewrect (big // item covers whole screen) or parts of device are outside the // viewport. In either case the device rect must be the intersect diff --git a/tests/auto/qgraphicsitem/tst_qgraphicsitem.cpp b/tests/auto/qgraphicsitem/tst_qgraphicsitem.cpp index 25ec040..2901dd5 100644 --- a/tests/auto/qgraphicsitem/tst_qgraphicsitem.cpp +++ b/tests/auto/qgraphicsitem/tst_qgraphicsitem.cpp @@ -445,6 +445,7 @@ private slots: void textItem_shortcuts(); void scroll(); void stopClickFocusPropagation(); + void deviceCoordinateCache_simpleRotations(); // task specific tests below me void task141694_textItemEnsureVisible(); @@ -10560,6 +10561,81 @@ void tst_QGraphicsItem::stopClickFocusPropagation() QVERIFY(itemWithFocus->hasFocus()); } +void tst_QGraphicsItem::deviceCoordinateCache_simpleRotations() +{ + // Make sure we don't invalidate the cache when applying simple + // (90, 180, 270, 360) rotation transforms to the item. + QGraphicsRectItem *item = new QGraphicsRectItem(0, 0, 300, 200); + item->setBrush(Qt::red); + item->setCacheMode(QGraphicsItem::DeviceCoordinateCache); + + QGraphicsScene scene; + scene.setSceneRect(0, 0, 300, 200); + scene.addItem(item); + + MyGraphicsView view(&scene); + view.show(); + QTest::qWaitForWindowShown(&view); + QTRY_VERIFY(view.repaints > 0); + + QGraphicsItemCache *itemCache = QGraphicsItemPrivate::get(item)->extraItemCache(); + Q_ASSERT(itemCache); + QPixmapCache::Key currentKey = itemCache->deviceData.value(view.viewport()).key; + + // Trigger an update and verify that the cache is unchanged. + QPixmapCache::Key oldKey = currentKey; + view.reset(); + view.viewport()->update(); + QTRY_VERIFY(view.repaints > 0); + currentKey = itemCache->deviceData.value(view.viewport()).key; + QCOMPARE(currentKey, oldKey); + + // Check 90, 180, 270 and 360 degree rotations. + for (int angle = 90; angle <= 360; angle += 90) { + // Rotate item and verify that the cache was invalidated. + oldKey = currentKey; + view.reset(); + QTransform transform; + transform.translate(150, 100); + transform.rotate(angle); + transform.translate(-150, -100); + item->setTransform(transform); + QTRY_VERIFY(view.repaints > 0); + currentKey = itemCache->deviceData.value(view.viewport()).key; + QVERIFY(currentKey != oldKey); + + // IMPORTANT PART: + // Trigger an update and verify that the cache is unchanged. + oldKey = currentKey; + view.reset(); + view.viewport()->update(); + QTRY_VERIFY(view.repaints > 0); + currentKey = itemCache->deviceData.value(view.viewport()).key; + QCOMPARE(currentKey, oldKey); + } + + // 45 degree rotation. + oldKey = currentKey; + view.reset(); + QTransform transform; + transform.translate(150, 100); + transform.rotate(45); + transform.translate(-150, -100); + item->setTransform(transform); + QTRY_VERIFY(view.repaints > 0); + currentKey = itemCache->deviceData.value(view.viewport()).key; + QVERIFY(currentKey != oldKey); + + // Trigger an update and verify that the cache was invalidated. + // We should always invalidate the cache for non-trivial transforms. + oldKey = currentKey; + view.reset(); + view.viewport()->update(); + QTRY_VERIFY(view.repaints > 0); + currentKey = itemCache->deviceData.value(view.viewport()).key; + QVERIFY(currentKey != oldKey); +} + void tst_QGraphicsItem::QTBUG_5418_textItemSetDefaultColor() { struct Item : public QGraphicsTextItem -- cgit v0.12 From a9a35d40f14751d0d9a0c82696c4b5cd0090bbea Mon Sep 17 00:00:00 2001 From: Pierre Rossi Date: Wed, 6 Oct 2010 15:46:33 +0200 Subject: Doc: fix description of the expected behavior for QGraphicsItem::cursor It was fixed for setCursor in 600ff0193c9bfac4d2b40960766002e8b81aca22. Reviewed-by: Alexis --- src/gui/graphicsview/qgraphicsitem.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/gui/graphicsview/qgraphicsitem.cpp b/src/gui/graphicsview/qgraphicsitem.cpp index fc44a44..60cd020 100644 --- a/src/gui/graphicsview/qgraphicsitem.cpp +++ b/src/gui/graphicsview/qgraphicsitem.cpp @@ -2125,7 +2125,7 @@ void QGraphicsItem::setToolTip(const QString &toolTip) \snippet doc/src/snippets/code/src_gui_graphicsview_qgraphicsitem.cpp 2 - If no cursor has been set, the parent's cursor is used. + If no cursor has been set, the cursor of the item beneath is used. \sa setCursor(), hasCursor(), unsetCursor(), QWidget::cursor, QApplication::overrideCursor() -- cgit v0.12