Start improving the startup/shutdown code paths

* Add some docs
* Comment out stuff we don't need right now
* Make startup/shutdown work with a simple test case and simple aspect

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

4 files changed, 72 insertions, 43 deletions

diff --git a/src/core/aspects/qabstractaspect.cpp b/src/core/aspects/qabstractaspect.cpp
index f34c55308..a35d860c6 100644
--- a/src/core/aspects/qabstractaspect.cpp
+++ b/src/core/aspects/qabstractaspect.cpp
@@ -46,6 +46,7 @@
 #include <Qt3DCore/private/qscene_p.h>
 #include <Qt3DCore/private/qnodevisitor_p.h>
 #include <Qt3DCore/qscenepropertychange.h>
+#include <Qt3DCore/private/corelogging_p.h>
 
 QT_BEGIN_NAMESPACE
 
@@ -191,6 +192,7 @@ void QAbstractAspectPrivate::clearBackendNode(QNode *frontend) const
 
 void QAbstractAspectPrivate::registerAspect(QEntity *rootObject)
 {
+    qCDebug(Aspects) << Q_FUNC_INFO << "rootObject =" << rootObject;
     Q_Q(QAbstractAspect);
     if (rootObject == m_root)
         return;
diff --git a/src/core/aspects/qaspectengine.cpp b/src/core/aspects/qaspectengine.cpp
index f159710f9..8b7c4a667 100644
--- a/src/core/aspects/qaspectengine.cpp
+++ b/src/core/aspects/qaspectengine.cpp
@@ -117,7 +117,14 @@ QAspectEngine::QAspectEngine(QObject *parent)
 QAspectEngine::~QAspectEngine()
 {
     Q_D(QAspectEngine);
+
+    // Shutdown the simulation loop by setting an empty scene
     setRootEntity(QEntityPtr());
+
+    // Exit the main loop
+    d->m_aspectThread->aspectManager()->quit();
+    d->m_aspectThread->wait();
+
     delete d->m_aspectThread;
     delete d->m_postman;
     delete d->m_scene;
@@ -142,6 +149,13 @@ void QAspectEnginePrivate::initialize()
                               Q_ARG(Qt3DCore::QScene*, m_scene));
 }
 
+/*!
+    \internal
+
+    Called when we unset the root entity. Causes the QAspectManager's simulation
+    loop to be exited. The main loop should keep processing events ready
+    to start up the simulation again with a new root entity.
+*/
 void QAspectEnginePrivate::shutdown()
 {
     qCDebug(Aspects) << Q_FUNC_INFO;
@@ -150,20 +164,6 @@ void QAspectEnginePrivate::shutdown()
     m_scene->setArbiter(Q_NULLPTR);
     QChangeArbiter *arbiter = m_aspectThread->aspectManager()->changeArbiter();
     QChangeArbiter::destroyUnmanagedThreadLocalChangeQueue(arbiter);
-
-    // Tell the aspect thread to exit
-    // This will return only after the aspectManager has
-    // exited its exec loop
-    m_aspectThread->aspectManager()->quit();
-
-    // Wait for thread to exit
-    m_aspectThread->wait();
-
-    qCDebug(Aspects) << Q_FUNC_INFO << "deleting aspects";
-    // Deletes aspects in the same thread as the one they were created in
-    qDeleteAll(m_aspects);
-
-    qCDebug(Aspects) << Q_FUNC_INFO << "Shutdown complete";
 }
 
 /*!
@@ -278,7 +278,7 @@ QVariant QAspectEngine::executeCommand(const QString &command)
 
 void QAspectEngine::setRootEntity(QEntityPtr root)
 {
-    qCDebug(Aspects) << "Setting scene root on aspect manager";
+    qCDebug(Aspects) << Q_FUNC_INFO << "root =" << root;
     Q_D(QAspectEngine);
     if (d->m_root == root)
         return;
@@ -290,8 +290,10 @@ void QAspectEngine::setRootEntity(QEntityPtr root)
     // free any related resources
     d->m_root = root;
 
-    if (shutdownNeeded)
+    if (shutdownNeeded) {
         d->shutdown();
+        d->m_aspectThread->aspectManager()->exitSimulationLoop();
+    }
 
     // Do we actually have a new scene?
     if (!d->m_root)
@@ -311,13 +313,16 @@ void QAspectEngine::setRootEntity(QEntityPtr root)
     d->initNodeTree(root.data());
 
     // Finally, tell the aspects about the new scene object tree. This is done
-    // in a blocking manner to allow the backends to get synchronized before the
+    // in a blocking manner to allow the aspects to get synchronized before the
     // main thread starts triggering potentially more notifications
+    qCDebug(Aspects) << "Begin setting scene root on aspect manager";
     QMetaObject::invokeMethod(d->m_aspectThread->aspectManager(),
                               "setRootEntity",
                               Qt::BlockingQueuedConnection,
                               Q_ARG(Qt3DCore::QEntity *, root.data()));
     qCDebug(Aspects) << "Done setting scene root on aspect manager";
+
+    d->m_aspectThread->aspectManager()->enterSimulationLoop();
 }
 
 QEntityPtr QAspectEngine::rootEntity() const
diff --git a/src/core/aspects/qaspectmanager.cpp b/src/core/aspects/qaspectmanager.cpp
index c43d2a4bc..7ea94e63d 100644
--- a/src/core/aspects/qaspectmanager.cpp
+++ b/src/core/aspects/qaspectmanager.cpp
@@ -77,8 +77,8 @@ QAspectManager::QAspectManager(QObject *parent)
     , m_waitForQuit(0)
 {
     qRegisterMetaType<QSurface *>("QSurface*");
-    m_runMainLoop.fetchAndStoreOrdered(0);
-    m_terminated.fetchAndStoreOrdered(0);
+    m_runSimulationLoop.fetchAndStoreOrdered(0);
+    m_runMainLoop.fetchAndStoreOrdered(1);
     qCDebug(Aspects) << Q_FUNC_INFO;
 }
 
@@ -89,11 +89,29 @@ QAspectManager::~QAspectManager()
     delete m_scheduler;
 }
 
+void QAspectManager::enterSimulationLoop()
+{
+    qCDebug(Aspects) << Q_FUNC_INFO;
+    m_runSimulationLoop.fetchAndStoreOrdered(1);
+}
+
+void QAspectManager::exitSimulationLoop()
+{
+    qCDebug(Aspects) << Q_FUNC_INFO;
+    m_runSimulationLoop.fetchAndStoreOrdered(0);
+}
+
 bool QAspectManager::isShuttingDown() const
 {
-    return !m_runMainLoop.load();
+    return !m_runSimulationLoop.load();
 }
 
+/*!
+    \internal
+
+    Called by the QAspectThread's run() method immediately after the manager
+    has been created
+*/
 void QAspectManager::initialize()
 {
     qCDebug(Aspects) << Q_FUNC_INFO;
@@ -102,6 +120,12 @@ void QAspectManager::initialize()
     m_changeArbiter->initialize(m_jobManager);
 }
 
+/*!
+    \internal
+
+    Called by the QAspectThread's run() method immediately after the manager's
+    exec() function has returned.
+*/
 void QAspectManager::shutdown()
 {
     qCDebug(Aspects) << Q_FUNC_INFO;
@@ -131,7 +155,6 @@ void QAspectManager::setRootEntity(Qt3DCore::QEntity *root)
     if (m_root) {
         for (QAbstractAspect *aspect : qAsConst(m_aspects))
             aspect->d_func()->registerAspect(m_root);
-        m_runMainLoop.fetchAndStoreOrdered(1);
     }
 }
 
@@ -164,9 +187,9 @@ void QAspectManager::exec()
     // Gentlemen, start your engines
     QEventLoop eventLoop;
 
-    // Enter the main loop
-    while (!m_terminated.load())
-    {
+    // Enter the engine loop
+    qCDebug(Aspects) << Q_FUNC_INFO << "***** Entering main loop *****";
+    while (m_runMainLoop.load()) {
         // Process any pending events, waiting for more to arrive if queue is empty
         eventLoop.processEvents(QEventLoop::WaitForMoreEvents, 16);
 
@@ -174,21 +197,22 @@ void QAspectManager::exec()
         QAbstractFrameAdvanceService *frameAdvanceService =
                 m_serviceLocator->service<QAbstractFrameAdvanceService>(QServiceLocator::FrameAdvanceService);
 
-        // Start the frameAdvanceService if we're about to enter the running loop
+        // Start the frameAdvanceService if we're about to enter the simulation loop
         bool needsShutdown = false;
-        if (m_runMainLoop.load()) {
+        if (m_runSimulationLoop.load()) {
             needsShutdown = true;
             frameAdvanceService->start();
 
-            // We are about to enter the main loop. Give aspects a chance to do any last
+            // We are about to enter the simulation loop. Give aspects a chance to do any last
             // pieces of initialization
-            qCDebug(Aspects) << "Calling onStartup() for each aspect";
+            qCDebug(Aspects) << "Calling onEngineStartup() for each aspect";
             for (QAbstractAspect *aspect : qAsConst(m_aspects))
                 aspect->onEngineStartup();
+            qCDebug(Aspects) << "Done calling onEngineStartup() for each aspect";
         }
 
-        // Only enter main render loop once the renderer and other aspects are initialized
-        while (m_runMainLoop.load()) {
+        // Only enter main simulation loop once the renderer and other aspects are initialized
+        while (m_runSimulationLoop.load()) {
             qint64 t = frameAdvanceService->waitForNextFrame();
 
             // Distribute accumulated changes. This includes changes sent from the frontend
@@ -215,17 +239,18 @@ void QAspectManager::exec()
 
             // Process any pending events
             eventLoop.processEvents();
-        }
+        } // End of simulation loop
 
         if (needsShutdown) {
             // Give aspects a chance to perform any shutdown actions. This may include unqueuing
             // any blocking work on the main thread that could potentially deadlock during shutdown.
-            qCDebug(Aspects) << "Calling onShutdown() for each aspect";
+            qCDebug(Aspects) << "Calling onEngineShutdown() for each aspect";
             for (QAbstractAspect *aspect : qAsConst(m_aspects))
                 aspect->onEngineShutdown();
+            qCDebug(Aspects) << "Done calling onEngineShutdown() for each aspect";
         }
-    }
-    qCDebug(Aspects) << Q_FUNC_INFO << "Exiting event loop";
+    } // End of main loop
+    qCDebug(Aspects) << Q_FUNC_INFO << "***** Exited main loop *****";
 
     m_waitForEndOfExecLoop.release(1);
     m_waitForQuit.acquire(1);
@@ -235,20 +260,14 @@ void QAspectManager::quit()
 {
     qCDebug(Aspects) << Q_FUNC_INFO;
 
+    Q_ASSERT_X(m_runSimulationLoop.load() == 0, "QAspectManagr::quit()", "Inner loop is still running");
     m_runMainLoop.fetchAndStoreOrdered(0);
-    m_terminated.fetchAndStoreOrdered(1);
-
-    // Allow the Aspect thread to proceed in case it's locked by the
-    // FrameAdvanceService <=> it is still in the running loop
-    QAbstractFrameAdvanceService *advanceFrameService = m_serviceLocator->service<QAbstractFrameAdvanceService>(QServiceLocator::FrameAdvanceService);
-    if (advanceFrameService)
-        advanceFrameService->stop();
 
     // We need to wait for the QAspectManager exec loop to terminate
     m_waitForEndOfExecLoop.acquire(1);
     m_waitForQuit.release(1);
 
-    qCDebug(Aspects) << Q_FUNC_INFO << "Exited event loop";
+    qCDebug(Aspects) << Q_FUNC_INFO << "Exiting";
 }
 
 const QList<QAbstractAspect *> &QAspectManager::aspects() const
diff --git a/src/core/aspects/qaspectmanager_p.h b/src/core/aspects/qaspectmanager_p.h
index 5275b0217..643da2311 100644
--- a/src/core/aspects/qaspectmanager_p.h
+++ b/src/core/aspects/qaspectmanager_p.h
@@ -80,6 +80,9 @@ public:
     explicit QAspectManager(QObject *parent = 0);
     ~QAspectManager();
 
+    void enterSimulationLoop();
+    void exitSimulationLoop();
+
     bool isShuttingDown() const;
 
 public Q_SLOTS:
@@ -104,8 +107,8 @@ private:
     QScheduler *m_scheduler;
     QAbstractAspectJobManager *m_jobManager;
     QChangeArbiter *m_changeArbiter;
+    QAtomicInt m_runSimulationLoop;
     QAtomicInt m_runMainLoop;
-    QAtomicInt m_terminated;
     QScopedPointer<QServiceLocator> m_serviceLocator;
     QSemaphore m_waitForEndOfExecLoop;
     QSemaphore m_waitForQuit;