Picking: restore early checks

5.8.0 removed early checks because it broke hover.

Restoring early checks by looking for at least one enabled
object picker that requires hover support.

Added a flag to job that can be set by pickers when they
are created, deleted or some property changed. This is
used to lazily update the state flags for the early
termination checks.

Also removed the dirty flag handling as it was not used
and never cleared.

Task-number: QTBUG-57611
Change-Id: Ib5c60290ee016d8c1fd3f24f7ebacff412fec239
Reviewed-by: Sean Harmer <sean.harmer@kdab.com>

6 files changed, 89 insertions, 59 deletions

diff --git a/src/render/jobs/pickboundingvolumejob.cpp b/src/render/jobs/pickboundingvolumejob.cpp
index 2ce6eb92e..8af99dfc3 100644
--- a/src/render/jobs/pickboundingvolumejob.cpp
+++ b/src/render/jobs/pickboundingvolumejob.cpp
@@ -104,6 +104,7 @@ PickBoundingVolumeJob::PickBoundingVolumeJob()
     , m_node(nullptr)
     , m_frameGraphRoot(nullptr)
     , m_renderSettings(nullptr)
+    , m_pickersDirty(true)
 {
     SET_JOB_RUN_STAT_TYPE(this, JobTypes::PickBoundingVolume, 0);
 }
@@ -150,17 +151,55 @@ bool PickBoundingVolumeJob::runHelper()
     if (mouseEvents.empty())
         return false;
 
-    // bail out early if no picker is enabled
-    bool oneEnabledAtLeast = false;
-    for (auto handle: m_manager->objectPickerManager()->activeHandles()) {
-        if (m_manager->objectPickerManager()->data(handle)->isEnabled()) {
-            oneEnabledAtLeast = true;
-            break;
+    // Quickly look which picker settings we've got
+    if (m_pickersDirty) {
+        m_pickersDirty = false;
+        m_oneEnabledAtLeast = false;
+        m_oneHoverAtLeast = false;
+
+        for (auto handle : m_manager->objectPickerManager()->activeHandles()) {
+            auto picker = m_manager->objectPickerManager()->data(handle);
+            m_oneEnabledAtLeast |= picker->isEnabled();
+            m_oneHoverAtLeast |= picker->isHoverEnabled();
+            if (m_oneEnabledAtLeast && m_oneHoverAtLeast)
+                break;
         }
     }
-    if (!oneEnabledAtLeast)
+
+    // bail out early if no picker is enabled
+    if (!m_oneEnabledAtLeast)
         return false;
 
+    bool hasMoveEvent = false;
+    bool hasOtherEvent = false;
+    // Quickly look which types of events we've got
+    for (const QMouseEvent &event : mouseEvents) {
+        const bool isMove = (event.type() == QEvent::MouseMove);
+        hasMoveEvent |= isMove;
+        hasOtherEvent |= !isMove;
+    }
+
+    // In the case we have a move event, find if we actually have
+    // an object picker that cares about these
+    if (!hasOtherEvent) {
+        // Retrieve the last used object picker
+        ObjectPicker *lastCurrentPicker = m_manager->objectPickerManager()->data(m_currentPicker);
+
+        // The only way to set lastCurrentPicker is to click
+        // so we can return since if we're there it means we
+        // have only move events. But keep on if hover support
+        // is needed
+        if (lastCurrentPicker == nullptr && !m_oneHoverAtLeast)
+            return false;
+
+        const bool caresAboutMove = (hasMoveEvent &&
+                                      (m_oneHoverAtLeast ||
+                                        (lastCurrentPicker && lastCurrentPicker->isDragEnabled())));
+        // Early return if the current object picker doesn't care about move events
+        if (!caresAboutMove)
+            return false;
+    }
+
     PickingUtils::ViewportCameraAreaGatherer vcaGatherer;
     // TO DO: We could cache this and only gather when we know the FrameGraph tree has changed
     const QVector<PickingUtils::ViewportCameraAreaTriplet> vcaTriplets = vcaGatherer.gather(m_frameGraphRoot);
@@ -238,6 +277,11 @@ void PickBoundingVolumeJob::setManagers(NodeManagers *manager)
     m_manager = manager;
 }
 
+void PickBoundingVolumeJob::markPickersDirty()
+{
+    m_pickersDirty = true;
+}
+
 void PickBoundingVolumeJob::run()
 {
     Q_ASSERT(m_frameGraphRoot && m_renderSettings && m_node && m_manager);
diff --git a/src/render/jobs/pickboundingvolumejob_p.h b/src/render/jobs/pickboundingvolumejob_p.h
index 913a5ace0..8d0d01f48 100644
--- a/src/render/jobs/pickboundingvolumejob_p.h
+++ b/src/render/jobs/pickboundingvolumejob_p.h
@@ -88,6 +88,8 @@ public:
     void setFrameGraphRoot(FrameGraphNode *frameGraphRoot);
     void setRenderSettings(RenderSettings *settings);
     void setManagers(NodeManagers *manager);
+    void markPickersDirty();
+    bool pickersDirty() const { return m_pickersDirty; }
 
     static QRay3D intersectionRay(const QPoint &pos,
                                   const QMatrix4x4 &viewMatrix,
@@ -112,6 +114,9 @@ private:
     FrameGraphNode *m_frameGraphRoot;
     RenderSettings *m_renderSettings;
     QList<QMouseEvent> m_pendingMouseEvents;
+    bool m_pickersDirty;
+    bool m_oneEnabledAtLeast;
+    bool m_oneHoverAtLeast;
 
     void viewMatrixForCamera(Qt3DCore::QNodeId cameraId,
                              QMatrix4x4 &viewMatrix,
diff --git a/src/render/picking/objectpicker.cpp b/src/render/picking/objectpicker.cpp
index eff6e3b3c..b85bdc723 100644
--- a/src/render/picking/objectpicker.cpp
+++ b/src/render/picking/objectpicker.cpp
@@ -39,6 +39,7 @@
 
 #include "objectpicker_p.h"
 #include "qpickevent.h"
+#include "renderer_p.h"
 #include <Qt3DRender/qobjectpicker.h>
 #include <Qt3DRender/private/qobjectpicker_p.h>
 #include <Qt3DRender/qattribute.h>
@@ -52,7 +53,6 @@ namespace Render {
 
 ObjectPicker::ObjectPicker()
     : BackendNode(QBackendNode::ReadWrite)
-    , m_isDirty(false)
     , m_isPressed(false)
     , m_hoverEnabled(false)
     , m_dragEnabled(false)
@@ -61,15 +61,16 @@ ObjectPicker::ObjectPicker()
 
 ObjectPicker::~ObjectPicker()
 {
+    notifyJob();
 }
 
 void ObjectPicker::cleanup()
 {
     BackendNode::setEnabled(false);
-    m_isDirty = false;
     m_isPressed = false;
     m_hoverEnabled = false;
     m_dragEnabled = false;
+    notifyJob();
 }
 
 void ObjectPicker::initializeFromPeer(const Qt3DCore::QNodeCreatedChangeBasePtr &change)
@@ -78,47 +79,44 @@ void ObjectPicker::initializeFromPeer(const Qt3DCore::QNodeCreatedChangeBasePtr
     const auto &data = typedChange->data;
     m_hoverEnabled = data.hoverEnabled;
     m_dragEnabled = data.dragEnabled;
-    m_isDirty = true;
+    notifyJob();
+}
+
+void ObjectPicker::notifyJob()
+{
+    if (m_renderer && m_renderer->pickBoundingVolumeJob())
+        qSharedPointerCast<PickBoundingVolumeJob>(m_renderer->pickBoundingVolumeJob())->markPickersDirty();
 }
 
 void ObjectPicker::sceneChangeEvent(const Qt3DCore::QSceneChangePtr &e)
 {
     if (e->type() == Qt3DCore::PropertyUpdated) {
         const Qt3DCore::QPropertyUpdatedChangePtr propertyChange = qSharedPointerCast<Qt3DCore::QPropertyUpdatedChange>(e);
+        bool notifyPickJob = false;
 
         if (propertyChange->propertyName() == QByteArrayLiteral("hoverEnabled")) {
             m_hoverEnabled = propertyChange->value().toBool();
-            m_isDirty = true;
+            notifyPickJob = true;
         } else if (propertyChange->propertyName() == QByteArrayLiteral("dragEnabled")) {
             m_dragEnabled = propertyChange->value().toBool();
-            m_isDirty = true;
+            notifyPickJob = true;
+        } else if (propertyChange->propertyName() == QByteArrayLiteral("enabled")) {
+            notifyPickJob = true;
+            // actual value change handled in BackendNode::sceneChangeEvent
         }
+
         markDirty(AbstractRenderer::AllDirty);
+        notifyJob();
     }
 
     BackendNode::sceneChangeEvent(e);
 }
 
-bool ObjectPicker::isDirty() const
-{
-    return m_isDirty;
-}
-
 bool ObjectPicker::isPressed() const
 {
     return m_isPressed;
 }
 
-void ObjectPicker::unsetDirty()
-{
-    m_isDirty = false;
-}
-
-void ObjectPicker::makeDirty()
-{
-    m_isDirty = true;
-}
-
 bool ObjectPicker::isHoverEnabled() const
 {
     return m_hoverEnabled;
diff --git a/src/render/picking/objectpicker_p.h b/src/render/picking/objectpicker_p.h
index f116aa332..2c2cc361e 100644
--- a/src/render/picking/objectpicker_p.h
+++ b/src/render/picking/objectpicker_p.h
@@ -70,10 +70,7 @@ public:
 
     void cleanup();
     void sceneChangeEvent(const Qt3DCore::QSceneChangePtr &e) Q_DECL_FINAL;
-    bool isDirty() const;
     bool isPressed() const;
-    void unsetDirty();
-    void makeDirty();
     bool isHoverEnabled() const;
     bool isDragEnabled() const;
 
@@ -86,8 +83,8 @@ public:
 
 private:
     void initializeFromPeer(const Qt3DCore::QNodeCreatedChangeBasePtr &change) Q_DECL_FINAL;
+    void notifyJob();
 
-    bool m_isDirty;
     bool m_isPressed;
     bool m_hoverEnabled;
     bool m_dragEnabled;
diff --git a/tests/auto/render/objectpicker/tst_objectpicker.cpp b/tests/auto/render/objectpicker/tst_objectpicker.cpp
index c42ec60c9..c1b06ccd8 100644
--- a/tests/auto/render/objectpicker/tst_objectpicker.cpp
+++ b/tests/auto/render/objectpicker/tst_objectpicker.cpp
@@ -54,7 +54,6 @@ private Q_SLOTS:
         // THEN
         QVERIFY(!objectPicker.peerId().isNull());
         QCOMPARE(objectPicker.isHoverEnabled(), true);
-        QCOMPARE(objectPicker.isDirty(), true);
     }
 
     void checkInitialAndCleanedUpState()
@@ -65,7 +64,6 @@ private Q_SLOTS:
         // THEN
         QVERIFY(objectPicker.peerId().isNull());
         QCOMPARE(objectPicker.isHoverEnabled(), false);
-        QCOMPARE(objectPicker.isDirty(), false);
 
         // GIVEN
         Qt3DRender::QObjectPicker picker;
@@ -77,31 +75,26 @@ private Q_SLOTS:
 
         // THEN
         QCOMPARE(objectPicker.isHoverEnabled(), false);
-        QCOMPARE(objectPicker.isDirty(), false);
     }
 
     void checkPropertyChanges()
     {
         // GIVEN
-        Qt3DRender::Render::ObjectPicker objectPicker;
         TestRenderer renderer;
-        objectPicker.setRenderer(&renderer);
-
-        QVERIFY(!objectPicker.isDirty());
-
-        // WHEN
-        Qt3DCore::QPropertyUpdatedChangePtr updateChange(new Qt3DCore::QPropertyUpdatedChange(Qt3DCore::QNodeId()));
-        updateChange->setValue(true);
-        updateChange->setPropertyName("hoverEnabled");
-        objectPicker.sceneChangeEvent(updateChange);
-
-        // THEN
-        QCOMPARE(objectPicker.isHoverEnabled(), true);
-        QVERIFY(objectPicker.isDirty());
-        QVERIFY(renderer.dirtyBits() != 0);
-
-        objectPicker.unsetDirty();
-        QVERIFY(!objectPicker.isDirty());
+        {
+            Qt3DRender::Render::ObjectPicker objectPicker;
+            objectPicker.setRenderer(&renderer);
+
+            // WHEN
+            Qt3DCore::QPropertyUpdatedChangePtr updateChange(new Qt3DCore::QPropertyUpdatedChange(Qt3DCore::QNodeId()));
+            updateChange->setValue(true);
+            updateChange->setPropertyName("hoverEnabled");
+            objectPicker.sceneChangeEvent(updateChange);
+
+            // THEN
+            QCOMPARE(objectPicker.isHoverEnabled(), true);
+            QVERIFY(renderer.dirtyBits() != 0);
+        }
     }
 
     void checkBackendPropertyNotifications()
@@ -111,7 +104,6 @@ private Q_SLOTS:
         Qt3DRender::Render::ObjectPicker objectPicker;
         Qt3DCore::QBackendNodePrivate::get(&objectPicker)->setArbiter(&arbiter);
         Qt3DRender::QPickEventPtr event(new Qt3DRender::QPickEvent);
-        QVERIFY(!objectPicker.isDirty());
 
         // WHEN
         objectPicker.onPressed(event);
diff --git a/tests/auto/render/pickboundingvolumejob/tst_pickboundingvolumejob.cpp b/tests/auto/render/pickboundingvolumejob/tst_pickboundingvolumejob.cpp
index 7e885435b..3e752e6c3 100644
--- a/tests/auto/render/pickboundingvolumejob/tst_pickboundingvolumejob.cpp
+++ b/tests/auto/render/pickboundingvolumejob/tst_pickboundingvolumejob.cpp
@@ -438,8 +438,6 @@ private Q_SLOTS:
 
     void checkEarlyReturnWhenMoveEventsAndNoCurrentPickers()
     {
-        QSKIP("Disabled following removal of early return checks");
-
         // GIVEN
         QmlSceneReader sceneReader(QUrl("qrc:/testscene_dragenabled.qml"));
         QScopedPointer<Qt3DCore::QNode> root(qobject_cast<Qt3DCore::QNode *>(sceneReader.root()));
@@ -541,15 +539,11 @@ private Q_SLOTS:
 
     void checkEarlyReturnWhenMoveEventsAndDragDisabledPickers_data()
     {
-        QSKIP("Disabled following removal of early return checks");
-
         generateAllPickingSettingsCombinations();
     }
 
     void checkEarlyReturnWhenMoveEventsAndDragDisabledPickers()
     {
-        QSKIP("Disabled following removal of early return checks");
-
         // GIVEN
         QmlSceneReader sceneReader(QUrl("qrc:/testscene_dragdisabled.qml"));
         QScopedPointer<Qt3DCore::QNode> root(qobject_cast<Qt3DCore::QNode *>(sceneReader.root()));