Checkpoint before GenericThread.

interface/src/Application.cpp

@@ -771,7 +771,7 @@ void Application::cleanupBeforeQuit() {
 
     // first stop all timers directly or by invokeMethod
     // depending on what thread they run in
-    _avatarUpdate->stop();
+    _avatarUpdate->terminate();
     locationUpdateTimer->stop();
     balanceUpdateTimer->stop();
     identityPacketTimer->stop();
@@ -2453,6 +2453,7 @@ void Application::init() {
     connect(_myAvatar, &MyAvatar::newCollisionSoundURL, DependencyManager::get<SoundCache>().data(), &SoundCache::getSound);
    
     _avatarUpdate = new AvatarUpdate();
+    _avatarUpdate->initialize(_avatarUpdate->isToBeThreaded());
 }
 
 void Application::closeMirrorView() {
@@ -2884,7 +2885,7 @@ void Application::update(float deltaTime) {
         _overlays.update(deltaTime);
     }
 
-    _avatarUpdate->avatarUpdateIfSynchronous();
+    _avatarUpdate->synchronousProcess();
 
     {
         PerformanceTimer perfTimer("emitSimulating");

interface/src/avatar/AvatarUpdate.cpp

@@ -15,73 +15,69 @@
 #include "AvatarUpdate.h"
 #include "InterfaceLogging.h"
 
-enum UpdateType {
+// GenericThread accepts an optional "parent" argument, defaulting to nullptr.
-    Synchronous = 1,
+// This is odd, because the moveToThread() in GenericThread::initialize() will
-    Timer,
+// become a no-op if the instance ever inits QObject(someNonNullParentQObject).
-    Thread
+// (The only clue will be a log message "QObject::moveToThread: Cannot move objects with a parent",
-};
+//  and things will end up in the same thread that created this instance. Hillarity ensues.)
-
+// As it turns out, all the other subclasses of GenericThread (at this time) do not init
-AvatarUpdate::AvatarUpdate() : _lastAvatarUpdate(0) {
+// GenericThread(parent), so things work as expected. Here we explicitly init GenericThread(nullptr)
+// so that there is no confusion and no chance for a hillarious thread debugging session.
+AvatarUpdate::AvatarUpdate() : QObject(nullptr), _timer(nullptr),  _lastAvatarUpdate(0), _thread(nullptr)/*FIXME remove*/ {
+    setObjectName("Avatar Update"); // GenericThread::initialize uses this to set the thread name.
     Settings settings;
-    int type = settings.value("AvatarUpdateType", UpdateType::Synchronous).toInt();
+    const int DEFAULT_TARGET_AVATAR_SIMRATE = 60;
-    _targetSimrate = settings.value("AvatarUpdateTargetSimrate", 60).toInt();
+    _targetSimrate = settings.value("AvatarUpdateTargetSimrate", DEFAULT_TARGET_AVATAR_SIMRATE).toInt();
-    qCDebug(interfaceapp) << "AvatarUpdate using" << type << "at" << _targetSimrate << "sims/second";
+    _type = settings.value("AvatarUpdateType", UpdateType::Synchronous).toInt();
-    switch (type) {
+    qCDebug(interfaceapp) << "AvatarUpdate using" << _type << "at" << _targetSimrate << "sims/second";
-        case UpdateType::Synchronous:
-            _timer = nullptr;
-            break;
-        case UpdateType::Timer:
-            initTimer();
-            break;
-        default:
-            initThread();
-            break;
-    }
 }
-void AvatarUpdate::stop() {
+// We could have the constructor call initialize(), but GenericThread::initialize can take parameters.
-    if (!_timer) {
+// Keeping it separately called by the client allows that client to pass those without our
-        return;
+// constructor needing to know about them.
+
+// GenericThread::terimate() calls terminating() only when _isThreaded, so it's not good enough
+// to just override terminating(). Instead, we extend terminate();
+void AvatarUpdate::terminate() {
+    if (_timer) {
+        _timer->stop();
     }
-    _timer->stop();
+    // FIXME: GenericThread::terminate();
-    if (_avatarThread) {
+    if (_thread) {
-        return;
+        _thread->quit();
     }
-    _avatarThread->quit();
 }
 
+// QTimer runs in the thread that starts it.
+// threadRoutine() is called once within the separate thread (if any),
+// or on each main loop update via synchronousProcess();
+void AvatarUpdate::threadRoutine() {
+    if (!_timer && (_type != UpdateType::Synchronous)) {
+        initTimer();
+    }
+    if (!_timer) {
+        process();
+    }
+}
 void AvatarUpdate::initTimer() {
     const qint64 AVATAR_UPDATE_INTERVAL_MSECS = 1000 / _targetSimrate;
     _timer = new QTimer(this);
-    connect(_timer, &QTimer::timeout, this, &AvatarUpdate::avatarUpdate);
+    connect(_timer, &QTimer::timeout, this, &AvatarUpdate::process);
     _timer->start(AVATAR_UPDATE_INTERVAL_MSECS);
 }
-void AvatarUpdate::initThread() {
-    _avatarThread = new QThread();
-    _avatarThread->setObjectName("Avatar Update Thread");
-    this->moveToThread(_avatarThread);
-    connect(_avatarThread, &QThread::started, this, &AvatarUpdate::initTimer);
-    _avatarThread->start();
-}
 
-// There are a couple of ways we could do this.
+void AvatarUpdate::synchronousProcess() {
-// Right now, the goals are:
+
-// 1. allow development to switch between UpdateType
-// 2. minimize changes everwhere, particularly outside of Avatars.
-// As an example of the latter, we could make Application::isHMDMode() thread safe, but in this case
-// we just made AvatarUpdate::isHMDMode() thread safe.
-void AvatarUpdate::avatarUpdateIfSynchronous() {
     // Keep our own updated value, so that our asynchronous code can consult it.
     _isHMDMode = Application::getInstance()->isHMDMode();
+
     if (_updateBillboard) {
         Application::getInstance()->getMyAvatar()->doUpdateBillboard();
     }
-    if (_timer) {
+
-        return;
+    threadRoutine();
-    }
-    avatarUpdate();
 }
 
-void AvatarUpdate::avatarUpdate() {
+ bool AvatarUpdate::process() {
     PerformanceTimer perfTimer("AvatarUpdate");
     quint64 now = usecTimestampNow();
     float deltaTime = (now - _lastAvatarUpdate) / (float) USECS_PER_SECOND;
@@ -95,4 +91,19 @@ void AvatarUpdate::avatarUpdate() {
     Application::getInstance()->updateMyAvatarLookAtPosition();
     // Sample hardware, update view frustum if needed, and send avatar data to mixer/nodes
     DependencyManager::get<AvatarManager>()->updateMyAvatar(deltaTime);
-}
+    return true;
+}
+
+// To be removed with GenericThread
+void AvatarUpdate::initialize(bool isThreaded) { //fixme remove
+    if (isThreaded) {
+        initThread();
+    }
+}
+void AvatarUpdate::initThread() {
+    _thread = new QThread();
+    _thread->setObjectName(objectName());
+    this->moveToThread(_thread);
+    connect(_thread, &QThread::started, this, &AvatarUpdate::threadRoutine);
+    _thread->start();
+}

interface/src/avatar/AvatarUpdate.h

@@ -15,27 +15,54 @@
 #include <QtCore/QObject>
 #include <QTimer>
 
+// There are a couple of ways we could do this.
+// Right now, the goals are:
+// 1. allow development to switch between UpdateType
+//   e.g.: see uses of UpdateType.
+// 2. minimize changes everwhere, particularly outside of Avatars.
+//   e.g.: we could make Application::isHMDMode() thread safe, but for now we just made AvatarUpdate::isHMDMode() thread safe.
+
+
 // Home for the avatarUpdate operations (e.g., whether on a separate thread, pipelined in various ways, etc.)
-// TODO: become GenericThread
 // This might get folded into AvatarManager.
 class AvatarUpdate : public QObject {
     Q_OBJECT
 public:
     AvatarUpdate();
-    void avatarUpdateIfSynchronous();
+    void synchronousProcess();
-    bool isHMDMode() { return _isHMDMode; }
     void setRequestBillboardUpdate(bool needsUpdate) { _updateBillboard = needsUpdate; }
-    void stop();
+    void terminate(); // Extends GenericThread::terminate to also kill timer.
+
 private:
+    virtual bool process(); // No reason for other classes to invoke this.
     void initTimer();
-    void initThread();
-    void avatarUpdate();
     int _targetSimrate;
-    bool _isHMDMode;
     bool _updateBillboard;
     QTimer* _timer;
-    QThread* _avatarThread;
     quint64 _lastAvatarUpdate;
+
+    // Goes away when we get rid of the ability to switch back and forth in settings:
+    enum UpdateType {
+        Synchronous = 1,
+        Timer,
+        Thread
+    };
+    int _type;
+public:
+    bool isToBeThreaded() const { return _type == UpdateType::Thread; } // Currently set by constructor from settings.
+    void threadRoutine();
+
+    // Goes away when using GenericThread:
+    void initialize(bool isThreaded);
+private:
+    QThread* _thread;
+    void initThread();
+
+    // Goes away if Applicaiton::isHMDMode() is made thread safe:
+public:
+    bool isHMDMode() { return _isHMDMode; }
+private:
+    bool _isHMDMode;
 };