Scene3D: Properly handle shutdowns with QtQuick Loaders

Shutdown procedure reworked for regular Scene3D use cases.
Added a Qt3D.Sceme3D logging category in case there's ever
a need to troubleshoot similar issues.

Change-Id: I275a312c498a50aa1ceefc9a51e20351f268c698
Reviewed-by: Alexandr Akulich <akulichalexander@gmail.com>
Reviewed-by: Sean Harmer <sean.harmer@kdab.com>

12 files changed, 233 insertions, 43 deletions

diff --git a/src/core/aspects/qaspectengine.cpp b/src/core/aspects/qaspectengine.cpp
index e2ff6d860..f5855b760 100644
--- a/src/core/aspects/qaspectengine.cpp
+++ b/src/core/aspects/qaspectengine.cpp
@@ -170,6 +170,12 @@ void QAspectEngine::shutdown()
 
     // Wait for thread to exit
     d->m_aspectThread->wait();
+
+    qCDebug(Aspects) << Q_FUNC_INFO << "deleting aspects";
+    // Deletes aspects in the same thread as the one they were created in
+    qDeleteAll(d->m_aspects);
+
+    qCDebug(Aspects) << Q_FUNC_INFO << "Shutdown complete";
 }
 
 void QAspectEngine::setData(const QVariantMap &data)
diff --git a/src/core/aspects/qaspectmanager.cpp b/src/core/aspects/qaspectmanager.cpp
index 98d92cb7b..e5c461631 100644
--- a/src/core/aspects/qaspectmanager.cpp
+++ b/src/core/aspects/qaspectmanager.cpp
@@ -98,7 +98,7 @@ void QAspectManager::shutdown()
         aspect->onCleanup();
         m_changeArbiter->unregisterSceneObserver(aspect);
     }
-    qDeleteAll(m_aspects);
+    // Aspects must be deleted in the Thread they were created in
 }
 
 void QAspectManager::setRootEntity(Qt3D::QEntity *root)
diff --git a/src/core/aspects/qaspectthread.cpp b/src/core/aspects/qaspectthread.cpp
index e0f948923..0b1a969d3 100644
--- a/src/core/aspects/qaspectthread.cpp
+++ b/src/core/aspects/qaspectthread.cpp
@@ -80,6 +80,8 @@ void QAspectThread::run()
 
     // Clean up
     m_aspectManager->shutdown();
+
+    // Delete the aspect manager while we're still in the thread
     delete m_aspectManager;
 
     qCDebug(Aspects) << "Exiting void QAspectThread::run()";
diff --git a/src/core/nodes/qnode.cpp b/src/core/nodes/qnode.cpp
index 1ea092687..c1dcce90d 100644
--- a/src/core/nodes/qnode.cpp
+++ b/src/core/nodes/qnode.cpp
@@ -557,6 +557,9 @@ QNodeList QNode::childrenNodes() const
 */
 QNode *QNode::clone(QNode *node)
 {
+    if (node == Q_NULLPTR)
+        return Q_NULLPTR;
+
     static int clearLock = 0;
     clearLock++;
 
diff --git a/src/quick3d/imports/scene3d/importsscene3d.pro b/src/quick3d/imports/scene3d/importsscene3d.pro
index 074f60810..a2ccf25af 100644
--- a/src/quick3d/imports/scene3d/importsscene3d.pro
+++ b/src/quick3d/imports/scene3d/importsscene3d.pro
@@ -6,11 +6,13 @@ QT += qml quick 3dcore 3drenderer 3dinput
 
 HEADERS += \
     qtquickscene3dplugin.h \
-    scene3ditem.h
+    scene3dlogging_p.h \
+    scene3ditem_p.h
 
 SOURCES += \
     qtquickscene3dplugin.cpp \
-    scene3ditem.cpp
+    scene3ditem.cpp \
+    scene3dlogging.cpp
 
 OTHER_FILES += qmldir
 
diff --git a/src/quick3d/imports/scene3d/qtquickscene3dplugin.cpp b/src/quick3d/imports/scene3d/qtquickscene3dplugin.cpp
index 51874d128..65836bdcf 100644
--- a/src/quick3d/imports/scene3d/qtquickscene3dplugin.cpp
+++ b/src/quick3d/imports/scene3d/qtquickscene3dplugin.cpp
@@ -36,13 +36,13 @@
 
 #include <QtQml>
 #include "qtquickscene3dplugin.h"
-#include "scene3ditem.h"
+#include "scene3ditem_p.h"
 
 QT_BEGIN_NAMESPACE
 
 void QtQuickScene3DPlugin::registerTypes(const char *uri)
 {
-    qmlRegisterType<Scene3DItem>(uri, 2, 0, "Scene3D");
+    qmlRegisterType<Qt3D::Scene3DItem>(uri, 2, 0, "Scene3D");
 }
 
 QT_END_NAMESPACE
diff --git a/src/quick3d/imports/scene3d/scene3ditem.cpp b/src/quick3d/imports/scene3d/scene3ditem.cpp
index 14d8e9591..68dfb2bea 100644
--- a/src/quick3d/imports/scene3d/scene3ditem.cpp
+++ b/src/quick3d/imports/scene3d/scene3ditem.cpp
@@ -34,7 +34,8 @@
 **
 ****************************************************************************/
 
-#include "scene3ditem.h"
+#include "scene3ditem_p.h"
+#include "scene3dlogging_p.h"
 
 #include <Qt3DCore/QAspectEngine>
 #include <Qt3DRenderer/QRenderAspect>
@@ -50,10 +51,12 @@
 #include <QSGMaterial>
 #include <QSGNode>
 #include <QOpenGLFunctions>
-#include <QSemaphore>
+#include <QThread>
 
 QT_BEGIN_NAMESPACE
 
+namespace Qt3D {
+
 class ContextSaver
 {
 public:
@@ -93,17 +96,52 @@ class Scene3DSGNode;
 
     \li The window is closed
 
-    \li This triggers the sceneGraphInvalidatedSignal which the Scene3DRenderer
+    \li This triggers the windowsChanged signal which the Scene3DRenderer
     uses to perform the necessary cleanups in the QSGRenderThread (destroys
-    DebugLogger ...) with the shutdown slot.
+    DebugLogger ...) with the shutdown slot (queued connection).
 
     \li The destroyed signal of the window is also connected to the
     Scene3DRenderer. When triggered in the context of the main thread, the
-    cleanup slot is called. A semaphore is used to make sure that the shutdown
-    slots has completed. Then the Scene3DRenderer calls delete upon himself which
-    also deletes the aspect engine in its dtor, still in the main thread.
+    cleanup slot is called.
+
+    There is an alternate shutdown procedure in case the QQuickItem is
+    destroyed with an active window which can happen in the case where the
+    Scene3D is used with a QtQuick Loader
 
+    In that case the shutdown procedure goes the same except that the destroyed
+    signal of the window is not called. Therefore the cleanup method is invoked
+    to properly destroy the aspect engine.
  */
+
+// Lives in the main thread
+// Used to delete the Qt3D aspect engine in the main thread
+class Scene3DCleaner : public QObject
+{
+    Q_OBJECT
+public:
+    Scene3DCleaner()
+        : QObject()
+        , m_renderer(Q_NULLPTR)
+    {}
+
+    ~Scene3DCleaner()
+    {
+        qCDebug(Scene3D) << Q_FUNC_INFO << QThread::currentThread();
+    }
+
+    void setRenderer(Scene3DRenderer *renderer)
+    {
+        m_renderer = renderer;
+    }
+
+public Q_SLOTS:
+    void cleanup();
+
+private:
+    Scene3DRenderer *m_renderer;
+};
+
+// Lives in the aspect thread (QSGRenderThread)
 class Scene3DRenderer : public QObject
 {
     Q_OBJECT
@@ -118,15 +156,14 @@ public:
         , m_multisampledFBO(Q_NULLPTR)
         , m_finalFBO(Q_NULLPTR)
         , m_texture(Q_NULLPTR)
-        , m_destructionSemaphore(0)
         , m_enableMultisampledFBO(true)
     {
         Q_CHECK_PTR(m_item);
         Q_CHECK_PTR(m_item->window());
 
-        QObject::connect(m_item->window(), SIGNAL(beforeRendering()), this, SLOT(render()), Qt::DirectConnection);
-        QObject::connect(m_item->window(), &QQuickWindow::sceneGraphInvalidated, this, &Scene3DRenderer::shutdown, Qt::DirectConnection);
-        QObject::connect(m_item->window(), &QQuickWindow::destroyed, this, &Scene3DRenderer::cleanup, Qt::DirectConnection);
+        QObject::connect(m_item->window(), &QQuickWindow::beforeRendering, this, &Scene3DRenderer::render, Qt::DirectConnection);
+        QObject::connect(m_item, &QQuickItem::windowChanged, this, &Scene3DRenderer::onWindowChangedQueued, Qt::QueuedConnection);
+
         ContextSaver saver;
 
         QVariantMap data;
@@ -141,8 +178,7 @@ public:
     // The Scene3DRender is delete by itself with the cleanup slot
     ~Scene3DRenderer()
     {
-        // Delete aspect engine
-        delete m_aspectEngine;
+        qCDebug(Scene3D) << Q_FUNC_INFO << QThread::currentThread();
     }
 
     QOpenGLFramebufferObject *createMultisampledFramebufferObject(const QSize &size)
@@ -167,37 +203,59 @@ public:
 
     void setSGNode(Scene3DSGNode *node) Q_DECL_NOEXCEPT;
 
+    void setCleanerHelper(Scene3DCleaner *cleaner)
+    {
+        m_cleaner = cleaner;
+        if (m_cleaner) {
+            // Window closed case
+            QObject::connect(m_item->window(), &QQuickWindow::destroyed, m_cleaner, &Scene3DCleaner::cleanup);
+            m_cleaner->setRenderer(this);
+        }
+    }
+
 public Q_SLOTS:
     void render();
 
     // Executed in the QtQuick render thread.
     void shutdown()
     {
+        qCDebug(Scene3D) << Q_FUNC_INFO << QThread::currentThread();
+
+        // Set to null so that subsequent calls to render
+        // would return early
+        m_item = Q_NULLPTR;
+
+        // Shutdown the Renderer Aspect while the OpenGL context
+        // is still valid
         m_renderAspect->renderShutdown();
-        // Allow the cleanup to proceed
-        m_destructionSemaphore.release(1);
     }
 
-    // Executed in the main thread
-    void cleanup()
+    // SGThread
+    void onWindowChangedQueued(QQuickWindow *w)
     {
-        m_destructionSemaphore.acquire(1);
-
-        // Delete ourself
-        delete this;
+        if (w == Q_NULLPTR) {
+            qCDebug(Scene3D) << Q_FUNC_INFO << QThread::currentThread();
+            shutdown();
+            // Will only trigger something with the Loader case
+            // The window closed cases is handled by the window's destroyed
+            // signal
+            QMetaObject::invokeMethod(m_cleaner, "cleanup");
+        }
     }
 
 private:
     Scene3DItem *m_item; // Will be released by the QQuickWindow/QML Engine
-    Qt3D::QAspectEngine *m_aspectEngine; // We are responsible for not leaking it
+    Qt3D::QAspectEngine *m_aspectEngine; // Will be released by the Scene3DRendererCleaner
     Qt3D::QRenderAspect *m_renderAspect; // Will be released by the aspectEngine
     QScopedPointer<QOpenGLFramebufferObject> m_multisampledFBO;
     QScopedPointer<QOpenGLFramebufferObject> m_finalFBO;
     QScopedPointer<QSGTexture> m_texture;
     Scene3DSGNode *m_node; // Will be released by the QtQuick SceneGraph
-    QSemaphore m_destructionSemaphore;
+    Scene3DCleaner *m_cleaner;
     QSize m_lastSize;
     bool m_enableMultisampledFBO;
+
+    friend class Scene3DCleaner;
 };
 
 /*!
@@ -318,10 +376,12 @@ public:
         setMaterial(&m_material);
         setOpaqueMaterial(&m_opaqueMaterial);
         setGeometry(&m_geometry);
+        qCDebug(Scene3D) << Q_FUNC_INFO << QThread::currentThread();
     }
 
     ~Scene3DSGNode()
     {
+        qCDebug(Scene3D) << Q_FUNC_INFO << QThread::currentThread();
         // The Scene3DSGNode is deleted by the QSGRenderThread when the SceneGraph
         // is terminated.
     }
@@ -370,11 +430,12 @@ private:
  */
 
 Scene3DItem::Scene3DItem(QQuickItem *parent)
-    : QQuickItem(parent),
-      m_entity(Q_NULLPTR),
-      m_aspectEngine(new Qt3D::QAspectEngine()),
-      m_renderAspect(new Qt3D::QRenderAspect(Qt3D::QRenderAspect::Synchronous)),
-      m_renderer(Q_NULLPTR)
+    : QQuickItem(parent)
+    , m_entity(Q_NULLPTR)
+    , m_aspectEngine(new Qt3D::QAspectEngine())
+    , m_renderAspect(new Qt3D::QRenderAspect(Qt3D::QRenderAspect::Synchronous))
+    , m_renderer(Q_NULLPTR)
+    , m_rendererCleaner(new Scene3DCleaner())
 {
     setFlag(QQuickItem::ItemHasContents, true);
     setAcceptedMouseButtons(Qt::MouseButtonMask);
@@ -450,8 +511,10 @@ QSGNode *Scene3DItem::updatePaintNode(QSGNode *node, QQuickItem::UpdatePaintNode
         node = Q_NULLPTR;
     }
 
-    if (m_renderer == Q_NULLPTR)
+    if (m_renderer == Q_NULLPTR) {
         m_renderer = new Scene3DRenderer(this, m_aspectEngine, m_renderAspect);
+        m_renderer->setCleanerHelper(m_rendererCleaner);
+    }
 
     Scene3DSGNode *fboNode = new Scene3DSGNode();
     fboNode->setRect(boundingRect());
@@ -469,7 +532,9 @@ void Scene3DRenderer::setSGNode(Scene3DSGNode *node) Q_DECL_NOEXCEPT
 void Scene3DRenderer::render()
 {
     if (!m_item || !m_item->window())
-        return ;
+        return;
+
+    QQuickWindow *window = m_item->window();
 
     if (m_aspectEngine->rootEntity() != m_item->entity())
         scheduleRootEntityChange();
@@ -491,7 +556,7 @@ void Scene3DRenderer::render()
 
     if (m_finalFBO.isNull() || forceRecreate) {
         m_finalFBO.reset(createFramebufferObject(m_item->boundingRect().size().toSize()));
-        m_texture.reset(m_item->window()->createTextureFromId(m_finalFBO->texture(), m_finalFBO->size(), QQuickWindow::TextureHasAlphaChannel));
+        m_texture.reset(window->createTextureFromId(m_finalFBO->texture(), m_finalFBO->size(), QQuickWindow::TextureHasAlphaChannel));
         m_node->setTexture(m_texture.data());
     }
 
@@ -533,13 +598,13 @@ void Scene3DRenderer::render()
 
     // Reset the state used by the Qt Quick scenegraph to avoid any
     // interference when rendering the rest of the UI.
-    m_item->window()->resetOpenGLState();
+    window->resetOpenGLState();
 
     // Mark material as dirty to request a new frame
     m_node->markDirty(QSGNode::DirtyMaterial);
 
     // Request next frame
-    m_item->window()->update();
+    window->update();
 }
 
 inline static bool isPowerOfTwo(int x)
@@ -579,6 +644,16 @@ void Scene3DSGMaterialShader::updateState(const RenderState &state, QSGMaterial
         program()->setUniformValue(m_opacityId, state.opacity());
 }
 
+void Scene3DCleaner::cleanup()
+{
+    Q_ASSERT(m_renderer);
+    delete m_renderer->m_aspectEngine;
+    m_renderer->m_aspectEngine = Q_NULLPTR;
+    m_renderer->deleteLater();
+    deleteLater();
+}
+
+} // Qt3D
 
 QT_END_NAMESPACE
 
diff --git a/src/quick3d/imports/scene3d/scene3ditem.h b/src/quick3d/imports/scene3d/scene3ditem_p.h
index 7506892c9..d1f5b3389 100644
--- a/src/quick3d/imports/scene3d/scene3ditem.h
+++ b/src/quick3d/imports/scene3d/scene3ditem_p.h
@@ -43,12 +43,12 @@ QT_BEGIN_NAMESPACE
 
 namespace Qt3D
 {
-    class QAspectEngine;
-    class QEntity;
-    class QRenderAspect;
-}
+class QAspectEngine;
+class QEntity;
+class QRenderAspect;
 
 class Scene3DRenderer;
+class Scene3DCleaner;
 
 class Scene3DItem : public QQuickItem
 {
@@ -83,8 +83,11 @@ private:
     Qt3D::QAspectEngine *m_aspectEngine;
     Qt3D::QRenderAspect *m_renderAspect;
     Scene3DRenderer *m_renderer;
+    Scene3DCleaner *m_rendererCleaner;
 };
 
+} // Qt3D
+
 QT_END_NAMESPACE
 
 #endif
diff --git a/src/quick3d/imports/scene3d/scene3dlogging.cpp b/src/quick3d/imports/scene3d/scene3dlogging.cpp
new file mode 100644
index 000000000..344d9250d
--- /dev/null
+++ b/src/quick3d/imports/scene3d/scene3dlogging.cpp
@@ -0,0 +1,47 @@
+/****************************************************************************
+**
+** Copyright (C) 2015 Klaralvdalens Datakonsult AB (KDAB).
+** Contact: http://www.qt-project.org/legal
+**
+** This file is part of the Qt3D module of the Qt Toolkit.
+**
+** $QT_BEGIN_LICENSE:LGPL3$
+** 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 The Qt Company. For licensing terms
+** and conditions see http://www.qt.io/terms-conditions. For further
+** information use the contact form at http://www.qt.io/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 3 as published by the Free Software
+** Foundation and appearing in the file LICENSE.LGPLv3 included in the
+** packaging of this file. Please review the following information to
+** ensure the GNU Lesser General Public License version 3 requirements
+** will be met: https://www.gnu.org/licenses/lgpl.html.
+**
+** GNU General Public License Usage
+** Alternatively, this file may be used under the terms of the GNU
+** General Public License version 2.0 or later 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 2.0 requirements will be
+** met: http://www.gnu.org/licenses/gpl-2.0.html.
+**
+** $QT_END_LICENSE$
+**
+****************************************************************************/
+
+#include "scene3dlogging_p.h"
+
+QT_BEGIN_NAMESPACE
+
+namespace Qt3D {
+
+Q_LOGGING_CATEGORY(Scene3D, "Qt3D.Scene3D")
+
+} // Qt3D
+
+QT_END_NAMESPACE
diff --git a/src/quick3d/imports/scene3d/scene3dlogging_p.h b/src/quick3d/imports/scene3d/scene3dlogging_p.h
new file mode 100644
index 000000000..2bec6f8f1
--- /dev/null
+++ b/src/quick3d/imports/scene3d/scene3dlogging_p.h
@@ -0,0 +1,52 @@
+/****************************************************************************
+**
+** Copyright (C) 2015 Klaralvdalens Datakonsult AB (KDAB).
+** Contact: http://www.qt-project.org/legal
+**
+** This file is part of the Qt3D module of the Qt Toolkit.
+**
+** $QT_BEGIN_LICENSE:LGPL3$
+** 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 The Qt Company. For licensing terms
+** and conditions see http://www.qt.io/terms-conditions. For further
+** information use the contact form at http://www.qt.io/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 3 as published by the Free Software
+** Foundation and appearing in the file LICENSE.LGPLv3 included in the
+** packaging of this file. Please review the following information to
+** ensure the GNU Lesser General Public License version 3 requirements
+** will be met: https://www.gnu.org/licenses/lgpl.html.
+**
+** GNU General Public License Usage
+** Alternatively, this file may be used under the terms of the GNU
+** General Public License version 2.0 or later 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 2.0 requirements will be
+** met: http://www.gnu.org/licenses/gpl-2.0.html.
+**
+** $QT_END_LICENSE$
+**
+****************************************************************************/
+
+#ifndef QT3D_SCENE3DLOGGING_P_H
+#define QT3D_SCENE3DLOGGING_P_H
+
+#include <QLoggingCategory>
+
+QT_BEGIN_NAMESPACE
+
+namespace Qt3D {
+
+Q_DECLARE_LOGGING_CATEGORY(Scene3D)
+
+} // Qt3D
+
+QT_END_NAMESPACE
+
+#endif // QT3D_SCENE3DLOGGING_P_H
diff --git a/src/render/backend/renderer.cpp b/src/render/backend/renderer.cpp
index e71c60ce0..bb5b99c0e 100644
--- a/src/render/backend/renderer.cpp
+++ b/src/render/backend/renderer.cpp
@@ -414,6 +414,7 @@ void Renderer::shutdown()
         // Clean up the graphics context
         m_graphicsContext.reset(Q_NULLPTR);
         m_surface = Q_NULLPTR;
+        qCDebug(Backend) << Q_FUNC_INFO << "Renderer properly shutdown";
     }
 }
 
diff --git a/src/render/backend/renderer_p.h b/src/render/backend/renderer_p.h
index 2b25720e7..fc3c55fc9 100644
--- a/src/render/backend/renderer_p.h
+++ b/src/render/backend/renderer_p.h
@@ -47,7 +47,6 @@
 #include <QHash>
 #include <QMatrix4x4>
 #include <QObject>
-#include <QTimer>
 
 #include <QOpenGLShaderProgram>
 #include <QOpenGLVertexArrayObject>