Bugfixes and cleanups for the picking code

- Removed the pickRegistry and related code, because it is not needed
- Fixed picking when navigation is turned off
- Introduced a new autotest for picking

Change-Id: Ib43789383760902f42542b45cfe49342f4ffa090
Reviewed-by: Sergio Ahumada <sergio.ahumada@digia.com>

7 files changed, 279 insertions, 63 deletions

diff --git a/src/imports/threed/viewport.cpp b/src/imports/threed/viewport.cpp
index 9e506f283..49a3deccd 100644
--- a/src/imports/threed/viewport.cpp
+++ b/src/imports/threed/viewport.cpp
@@ -236,9 +236,7 @@ public:
     bool panning;
     QPointF startPan;
     QPointF lastPan;
-    QPointF lastPick;
     QObject *lastObject;
-    bool needsPick;
     QVector3D startEye;
     QVector3D startCenter;
     QVector3D startUpVector;
@@ -249,15 +247,7 @@ public:
     bool pickingRenderInitialized;
     QList<PickEvent *> pickEventQueue;
 
-    // INVARIANT: PickEvents are always either in the queue, or in the registry, never
-    // in both.  Here "never" means "not outside of sections guarded by the lock".
-    //
-    // Serializing PickEvents across the signal/slot boundary is dicey, since they are
-    // not a value type.  Instead keep our own registry of them here, and forward the
-    // id values instead.  Should also make dispatch faster.
-    QMap<quint64, PickEvent *> pickEventRegistry;
-
-    // This lock is for the registry and for the pick event queue itself.  All accesses
+    // This lock is for the pick event queue itself.  All accesses
     // to that data structure must be guarded by this lock.
     QMutexMaybeLocker::Lock pickEventQueueLock;
 
@@ -270,7 +260,6 @@ public:
     void setDefaults(QGLPainter *painter);
     void setRenderSettings(QGLPainter *painter);
     void getOverflow(QMouseEvent *e);
-    PickEvent *takeFromRegistry(quint64 id);
 };
 
 ViewportPrivate::ViewportPrivate()
@@ -294,9 +283,7 @@ ViewportPrivate::ViewportPrivate()
     , panning(false)
     , startPan(-1, -1)
     , lastPan(-1, -1)
-    , lastPick(-1, -1)
     , lastObject(0)
-    , needsPick(true)
     , panModifiers(Qt::NoModifier)
     , renderMode(Viewport::UnknownRender)
     , directRenderInitialized(false)
@@ -312,7 +299,6 @@ ViewportPrivate::~ViewportPrivate()
 {
     delete pickFbo;
     qDeleteAll(pickEventQueue);
-    qDeleteAll(pickEventRegistry);
 }
 
 void ViewportPrivate::setDefaults(QGLPainter *painter)
@@ -372,20 +358,6 @@ void ViewportPrivate::setRenderSettings(QGLPainter *painter)
     glClear(GL_COLOR_BUFFER_BIT | GL_DEPTH_BUFFER_BIT);
 }
 
-PickEvent *ViewportPrivate::takeFromRegistry(quint64 id)
-{
-    PickEvent *pick = 0;
-    QMutexMaybeLocker locker(&pickEventQueueLock);
-    QMap<quint64, PickEvent*>::iterator it = pickEventRegistry.find(id);
-    if (it != pickEventRegistry.end())
-    {
-        pick = it.value();
-        pickEventRegistry.erase(it);
-    }
-    locker.unlock();
-    return pick;
-}
-
 const int Viewport::FBO_SIZE = 8;
 
 /*!
@@ -939,7 +911,6 @@ void Viewport::render(QGLPainter *painter)
     // May've been set by early draw
     glDisable(GL_CULL_FACE);
 
-    d->needsPick = true;
     draw(painter);
 
     // May've been set by one of the items
@@ -1194,8 +1165,6 @@ void Viewport::objectForPoint()
             QMutexMaybeLocker locker(&d->pickEventQueueLock);
             if (d->pickEventQueue.size() > 0)
                 p = d->pickEventQueue.takeFirst();
-            if (p)
-                d->pickEventRegistry.insert(p->id(), p);
             locker.unlock();
         }
         if (!p)
@@ -1205,8 +1174,10 @@ void Viewport::objectForPoint()
         // Check the viewport boundaries in case a mouse move has
         // moved the pointer outside the window.
         QRectF rect = boundingRect();
-        if (!rect.contains(pt))
+        if (!rect.contains(pt)) {
+            delete p;
             continue;
+        }
 
         if (!d->pickFbo)
         {
@@ -1226,8 +1197,6 @@ void Viewport::objectForPoint()
                 painter.setPicking(false);
                 objectId = painter.pickObject(pt.x() / winToFboRatioW, pt.y() / winToFboRatioH);
                 d->setDefaults(&painter);
-                d->needsPick = false;
-                d->lastPick = pt;
             } else {
                 qWarning() << "Warning: unable to paint into fbo, picking will be unavailable";
                 continue;
@@ -1237,7 +1206,7 @@ void Viewport::objectForPoint()
         d->lastObject = obj;
         p->setObject(obj);
         QMetaMethod m = metaObject()->method(p->callback());
-        m.invoke(this, Qt::QueuedConnection, Q_ARG(quint64, p->id()));
+        m.invoke(this, Qt::QueuedConnection, Q_ARG(void*, p));
     }
 }
 
@@ -1277,7 +1246,7 @@ void Viewport::mousePressEvent(QMouseEvent *e)
     static int processMousePressInvocation = -1;
     if (processMousePressInvocation == -1)
     {
-        processMousePressInvocation = metaObject()->indexOfMethod("processMousePress(quint64)");
+        processMousePressInvocation = metaObject()->indexOfMethod("processMousePress(PickEvent*)");
         Q_ASSERT(processMousePressInvocation != -1);
     }
     if (!d->panning && d->picking)
@@ -1285,6 +1254,8 @@ void Viewport::mousePressEvent(QMouseEvent *e)
         PickEvent * p = initiatePick(e);
         if (p)
             p->setCallback(processMousePressInvocation);
+        e->setAccepted(true); //This is necessary, otherwise we won't get a realese event
+        return;
     }
     if (d->navigation && e->button() == Qt::LeftButton)
     {
@@ -1298,10 +1269,8 @@ void Viewport::mousePressEvent(QMouseEvent *e)
     e->setAccepted(true);
 }
 
-void Viewport::processMousePress(quint64 eventId)
+void Viewport::processMousePress(PickEvent *pick)
 {
-    QScopedPointer<PickEvent> pick(d->takeFromRegistry(eventId));
-    Q_ASSERT(pick.data());
     QObject *object = pick->object();
     QMouseEvent *e = pick->event();
     if (d->pressedObject)
@@ -1328,10 +1297,12 @@ void Viewport::processMousePress(quint64 eventId)
                           e->modifiers());
         QCoreApplication::sendEvent(object, &event);
     }
-    else if (d->navigation && e->button() == Qt::LeftButton)
+
+    if (d->navigation && e->button() == Qt::LeftButton)
     {
         processNavEvent(e);
     }
+    delete pick;
 }
 
 void Viewport::processNavEvent(QMouseEvent *e)
@@ -1352,7 +1323,7 @@ void Viewport::mouseReleaseEvent(QMouseEvent *e)
     static int processMouseReleaseInvocation = -1;
     if (processMouseReleaseInvocation == -1)
     {
-        processMouseReleaseInvocation = metaObject()->indexOfMethod("processMouseRelease(quint64)");
+        processMouseReleaseInvocation = metaObject()->indexOfMethod("processMouseRelease(PickEvent*)");
         Q_ASSERT(processMouseReleaseInvocation != -1);
     }
     if (d->panning && e->button() == Qt::LeftButton) {
@@ -1368,10 +1339,8 @@ void Viewport::mouseReleaseEvent(QMouseEvent *e)
     }
 }
 
-void Viewport::processMouseRelease(quint64 eventId)
+void Viewport::processMouseRelease(PickEvent *pick)
 {
-    QScopedPointer<PickEvent> pick(d->takeFromRegistry(eventId));
-    Q_ASSERT(pick.data());
     Q_ASSERT(d->pressedObject);
     QObject *object = pick->object();
     QMouseEvent *e = pick->event();
@@ -1407,6 +1376,7 @@ void Viewport::processMouseRelease(quint64 eventId)
              e->screenPos(), e->button(), e->buttons(), e->modifiers());
         QCoreApplication::sendEvent(pressed, &event);
     }
+    delete pick;
 }
 
 /*!
@@ -1417,7 +1387,7 @@ void Viewport::mouseDoubleClickEvent(QMouseEvent *e)
     static int processMouseDoubleClickInvocation = -1;
     if (processMouseDoubleClickInvocation == -1)
     {
-        processMouseDoubleClickInvocation = metaObject()->indexOfMethod("processMouseDoubleClick(quint64)");
+        processMouseDoubleClickInvocation = metaObject()->indexOfMethod("processMouseDoubleClick(PickEvent*)");
         Q_ASSERT(processMouseDoubleClickInvocation != -1);
     }
     if (d->picking) {
@@ -1428,10 +1398,8 @@ void Viewport::mouseDoubleClickEvent(QMouseEvent *e)
     QQuickItem::mouseDoubleClickEvent(e);
 }
 
-void Viewport::processMouseDoubleClick(quint64 eventId)
+void Viewport::processMouseDoubleClick(PickEvent *pick)
 {
-    QScopedPointer<PickEvent> pick(d->takeFromRegistry(eventId));
-    Q_ASSERT(pick.data());
     QObject *object = pick->object();
     QMouseEvent *e = pick->event();
     if (object) {
@@ -1442,6 +1410,7 @@ void Viewport::processMouseDoubleClick(quint64 eventId)
         QCoreApplication::sendEvent(object, &event);
         e->setAccepted(true);
     }
+    delete pick;
 }
 
 /*!
@@ -1481,7 +1450,7 @@ void Viewport::mouseMoveEvent(QMouseEvent *e)
     static int processMouseMoveInvocation = -1;
     if (processMouseMoveInvocation == -1)
     {
-        processMouseMoveInvocation = metaObject()->indexOfMethod("processMouseMove(quint64)");
+        processMouseMoveInvocation = metaObject()->indexOfMethod("processMouseMove(PickEvent*)");
         Q_ASSERT(processMouseMoveInvocation != -1);
     }
     if (d->panning) {
@@ -1516,10 +1485,8 @@ void Viewport::mouseMoveEvent(QMouseEvent *e)
     e->setAccepted(true);
 }
 
-void Viewport::processMouseMove(quint64 eventId)
+void Viewport::processMouseMove(PickEvent *pick)
 {
-    QScopedPointer<PickEvent> pick(d->takeFromRegistry(eventId));
-    Q_ASSERT(pick.data());
     QObject *object = pick->object();
     QMouseEvent *e = pick->event();
     if (d->pressedObject) {
@@ -1547,8 +1514,10 @@ void Viewport::processMouseMove(quint64 eventId)
         d->enteredObject = 0;
     } else {
         QQuickItem::mouseMoveEvent(e);
+        delete pick;
         return;
     }
+    delete pick;
 }
 
 /*!
@@ -1683,7 +1652,7 @@ bool Viewport::hoverEvent(QHoverEvent *e)
     static int processMouseHoverInvocation = -1;
     if (processMouseHoverInvocation == -1)
     {
-        processMouseHoverInvocation = metaObject()->indexOfMethod("processMouseHover(quint64)");
+        processMouseHoverInvocation = metaObject()->indexOfMethod("processMouseHover(PickEvent*)");
         Q_ASSERT(processMouseHoverInvocation != -1);
     }
     if (!d->panning && d->picking) {
@@ -1699,10 +1668,8 @@ bool Viewport::hoverEvent(QHoverEvent *e)
     return false;
 }
 
-void Viewport::processMouseHover(quint64 eventId)
+void Viewport::processMouseHover(PickEvent *pick)
 {
-    QScopedPointer<PickEvent> pick(d->takeFromRegistry(eventId));
-    Q_ASSERT(pick.data());
     QObject *object = pick->object();
     QMouseEvent *e = pick->event();
 
@@ -1730,6 +1697,7 @@ void Viewport::processMouseHover(quint64 eventId)
         sendLeaveEvent(d->enteredObject);
         d->enteredObject = 0;
     }
+    delete pick;
 }
 
 // Zoom in and out according to the change in wheel delta.
diff --git a/src/imports/threed/viewport.h b/src/imports/threed/viewport.h
index bc586c3ed..73e4c7d66 100644
--- a/src/imports/threed/viewport.h
+++ b/src/imports/threed/viewport.h
@@ -163,11 +163,11 @@ private:
     void setupPickPaint(QGLPainter *painter, const QPointF &pt);
     bool mouseMoveOverflow(QMouseEvent *e) const;
 
-    Q_INVOKABLE void processMousePress(quint64 eventId);
-    Q_INVOKABLE void processMouseRelease(quint64);
-    Q_INVOKABLE void processMouseDoubleClick(quint64);
-    Q_INVOKABLE void processMouseMove(quint64);
-    Q_INVOKABLE void processMouseHover(quint64);
+    Q_INVOKABLE void processMousePress(PickEvent *event);
+    Q_INVOKABLE void processMouseRelease(PickEvent *event);
+    Q_INVOKABLE void processMouseDoubleClick(PickEvent *event);
+    Q_INVOKABLE void processMouseMove(PickEvent *event);
+    Q_INVOKABLE void processMouseHover(PickEvent *event);
 
     void processNavEvent(QMouseEvent *event);
 
diff --git a/tests/auto/auto.pro b/tests/auto/auto.pro
index 3aac8a00f..46114c1c1 100644
--- a/tests/auto/auto.pro
+++ b/tests/auto/auto.pro
@@ -5,5 +5,7 @@ SUBDIRS = threed \
 qtHaveModule(qml): SUBDIRS += imports
 qtHaveModule(qmltest) {
     SUBDIRS += qml3d
-    !win32 : SUBDIRS += qml3d_visual
+
+    !win32 : SUBDIRS += qml3d_visual \
+                        qml3d_cpp
 }
diff --git a/tests/auto/qml3d_cpp/picking/data/tst_picking.qml b/tests/auto/qml3d_cpp/picking/data/tst_picking.qml
new file mode 100644
index 000000000..c158e857f
--- /dev/null
+++ b/tests/auto/qml3d_cpp/picking/data/tst_picking.qml
@@ -0,0 +1,98 @@
+/***************************************************************************
+**
+** Copyright (C) 2011 - 2013 Research In Motion
+** Contact: http://www.qt-project.org/legal
+**
+** This file is part of the Qt3D module of the Qt Toolkit.
+**
+** $QT_BEGIN_LICENSE:LGPL$
+** Commercial License Usage
+** Licensees holding valid commercial Qt licenses may use this file in
+** accordance with the commercial license agreement provided with the
+** Software or, alternatively, in accordance with the terms contained in
+** a written agreement between you and Digia.  For licensing terms and
+** conditions see http://qt.digia.com/licensing.  For further information
+** use the contact form at http://qt.digia.com/contact-us.
+**
+** GNU Lesser General Public License Usage
+** Alternatively, this file may be used under the terms of the GNU Lesser
+** General Public License version 2.1 as published by the Free Software
+** Foundation and appearing in the file LICENSE.LGPL included in the
+** packaging of this file.  Please review the following information to
+** ensure the GNU Lesser General Public License version 2.1 requirements
+** will be met: http://www.gnu.org/licenses/old-licenses/lgpl-2.1.html.
+**
+** In addition, as a special exception, Digia gives you certain additional
+** rights.  These rights are described in the Digia Qt LGPL Exception
+** version 1.1, included in the file LGPL_EXCEPTION.txt in this package.
+**
+** GNU General Public License Usage
+** Alternatively, this file may be used under the terms of the GNU
+** General Public License version 3.0 as published by the Free Software
+** Foundation and appearing in the file LICENSE.GPL included in the
+** packaging of this file.  Please review the following information to
+** ensure the GNU General Public License version 3.0 requirements will be
+** met: http://www.gnu.org/copyleft/gpl.html.
+**
+**
+** $QT_END_LICENSE$
+**
+****************************************************************************/
+
+import Qt3D 2.0
+import Qt3D.Shapes 2.0
+import QtQuick 2.0
+
+Rectangle
+{
+    id: topLevel
+    width: 480; height: 480
+
+    property string pickedObjectClicked;
+    property string pickedObjectPressed;
+    property string pickedObjectReleased;
+    property string hoveredObject;
+
+    property alias navigation: viewport.navigation
+
+    Viewport {
+        id: viewport
+        width: 480; height: 480
+        picking: true
+
+        Quad {
+            id: fullScreenQuad
+
+            transform: [
+                Rotation3D {
+                    axis: Qt.vector3d(1,0,0);
+                    angle: 90
+                }
+            ]
+            position: Qt.vector3d(0, 1, 0)
+
+            onClicked: topLevel.pickedObjectClicked = "first"
+            onPressed: topLevel.pickedObjectPressed = "first"
+            onReleased:  topLevel.pickedObjectReleased = "first"
+            onHoverEnter: hoveredObject= "first"
+            onHoverLeave: hoveredObject= ""
+        }
+
+        Quad {
+            id: smallerQuad
+
+            transform: [
+                Rotation3D {
+                    axis: Qt.vector3d(1,0,0);
+                    angle: 90
+                }
+            ]
+
+            position: Qt.vector3d(0, -1, 0)
+
+            onClicked: topLevel.pickedObjectClicked = "second"
+            onPressed: topLevel.pickedObjectPressed = "second"
+            onReleased:topLevel.pickedObjectReleased = "second"
+        }
+    }
+}
diff --git a/tests/auto/qml3d_cpp/picking/picking.pro b/tests/auto/qml3d_cpp/picking/picking.pro
new file mode 100644
index 000000000..80a81a7e5
--- /dev/null
+++ b/tests/auto/qml3d_cpp/picking/picking.pro
@@ -0,0 +1,12 @@
+TARGET = tst_qml3d_cpp
+CONFIG += testcase
+TEMPLATE=app
+QT += testlib 3d 3dquick
+QT += qml quick gui-private
+
+SOURCES += tst_picking.cpp
+DEFINES += QT_DISABLE_DEPRECATED_BEFORE=0
+
+OTHER_FILES += data/tst_picking.qml
+
+TESTDATA = $$OTHER_FILES
diff --git a/tests/auto/qml3d_cpp/picking/tst_picking.cpp b/tests/auto/qml3d_cpp/picking/tst_picking.cpp
new file mode 100644
index 000000000..f9ab2ec99
--- /dev/null
+++ b/tests/auto/qml3d_cpp/picking/tst_picking.cpp
@@ -0,0 +1,134 @@
+/***************************************************************************
+**
+** Copyright (C) 2011 - 2013 Research In Motion
+** Contact: http://www.qt-project.org/legal
+**
+** This file is part of the Qt3D module of the Qt Toolkit.
+**
+** $QT_BEGIN_LICENSE:LGPL$
+** Commercial License Usage
+** Licensees holding valid commercial Qt licenses may use this file in
+** accordance with the commercial license agreement provided with the
+** Software or, alternatively, in accordance with the terms contained in
+** a written agreement between you and Digia.  For licensing terms and
+** conditions see http://qt.digia.com/licensing.  For further information
+** use the contact form at http://qt.digia.com/contact-us.
+**
+** GNU Lesser General Public License Usage
+** Alternatively, this file may be used under the terms of the GNU Lesser
+** General Public License version 2.1 as published by the Free Software
+** Foundation and appearing in the file LICENSE.LGPL included in the
+** packaging of this file.  Please review the following information to
+** ensure the GNU Lesser General Public License version 2.1 requirements
+** will be met: http://www.gnu.org/licenses/old-licenses/lgpl-2.1.html.
+**
+** In addition, as a special exception, Digia gives you certain additional
+** rights.  These rights are described in the Digia Qt LGPL Exception
+** version 1.1, included in the file LGPL_EXCEPTION.txt in this package.
+**
+** GNU General Public License Usage
+** Alternatively, this file may be used under the terms of the GNU
+** General Public License version 3.0 as published by the Free Software
+** Foundation and appearing in the file LICENSE.GPL included in the
+** packaging of this file.  Please review the following information to
+** ensure the GNU General Public License version 3.0 requirements will be
+** met: http://www.gnu.org/copyleft/gpl.html.
+**
+**
+** $QT_END_LICENSE$
+**
+****************************************************************************/
+
+#include <QtTest/QtTest>
+#include <QtQuick/qquickview.h>
+#include <QtQml/qqmlengine.h>
+#include <QtQml/qqmlcontext.h>
+#include <QQuickItem>
+#include <qpa/qwindowsysteminterface.h>
+
+class tst_Picking : public QObject
+{
+    Q_OBJECT
+public:
+    tst_Picking() {}
+    ~tst_Picking() {}
+
+private slots:
+    void initTestCase();
+    void cleanupTestCase();
+    void testWithNavigation();
+    void testWithoutNavigation();
+    void testMove();
+
+private:
+    void testMouse();
+
+private:
+    QQuickItem *rootObject;
+    QQuickView *window;
+};
+
+void tst_Picking::initTestCase()
+{
+    window = new QQuickView(0);
+    window->setSource(QUrl::fromLocalFile(QFINDTESTDATA("data/tst_picking.qml")));
+    window->setGeometry(0,0,480,480);
+    window->show();
+    QTest::qWaitForWindowExposed(window);
+    rootObject = window->rootObject();
+}
+
+void tst_Picking::cleanupTestCase()
+{
+    delete window;
+}
+
+void tst_Picking::testWithNavigation()
+{
+    rootObject->setProperty("navigation", true);
+    testMouse();
+}
+
+void tst_Picking::testWithoutNavigation()
+{
+    rootObject->setProperty("navigation", false);
+    testMouse();
+}
+
+void tst_Picking::testMouse()
+{
+    QTest::mousePress(window, Qt::LeftButton, 0, QPoint(240, 90));
+    QTest::mouseRelease(window, Qt::LeftButton, 0, QPoint(240, 90), 300);
+
+    QTRY_COMPARE(rootObject->property("pickedObjectPressed").toString(), QString("first"));
+    QTRY_COMPARE(rootObject->property("pickedObjectClicked").toString(), QString("first"));
+    QTRY_COMPARE(rootObject->property("pickedObjectReleased").toString(), QString("first"));
+
+    QTest::mousePress(window, Qt::LeftButton, 0, QPoint(240, 320));
+    QTest::mouseRelease(window, Qt::LeftButton, 0, QPoint(240, 240),300);
+
+    QTRY_COMPARE(rootObject->property("pickedObjectPressed").toString(), QString("second"));
+    QTRY_COMPARE(rootObject->property("pickedObjectClicked").toString(), QString("first"));
+    QTRY_COMPARE(rootObject->property("pickedObjectReleased").toString(), QString("second"));
+
+    QTest::mousePress(window, Qt::LeftButton, 0, QPoint(20, 100));
+    QTest::mouseRelease(window, Qt::LeftButton, 0, QPoint(20, 100), 300);
+
+    QTRY_COMPARE(rootObject->property("pickedObjectPressed").toString(), QString("second"));
+    QTRY_COMPARE(rootObject->property("pickedObjectClicked").toString(), QString("first"));
+    QTRY_COMPARE(rootObject->property("pickedObjectReleased").toString(), QString("second"));
+}
+
+void tst_Picking::testMove()
+{
+    rootObject->setProperty("hoveredObject", "");
+    QTest::mouseMove(window, QPoint(240, 90));
+    QTRY_COMPARE(rootObject->property("hoveredObject").toString(), QString("first"));
+
+    QTest::mouseMove(window, QPoint(240, 200));
+    QTRY_COMPARE(rootObject->property("hoveredObject").toString(), QString(""));
+}
+
+QTEST_MAIN(tst_Picking)
+
+#include "tst_picking.moc"
diff --git a/tests/auto/qml3d_cpp/qml3d_cpp.pro b/tests/auto/qml3d_cpp/qml3d_cpp.pro
new file mode 100644
index 000000000..baf619016
--- /dev/null
+++ b/tests/auto/qml3d_cpp/qml3d_cpp.pro
@@ -0,0 +1,2 @@
+TEMPLATE = subdirs
+SUBDIRS = picking