Call back to Javascript asynchronously, so that we don't block and the script's engine doesn't have thread conflicts.

interface/src/avatar/MyAvatar.h

@@ -118,6 +118,8 @@ public:
     Q_INVOKABLE void addAnimationStateHandler(QScriptValue handler, QScriptValue propertiesList) { _rig->addAnimationStateHandler(handler, propertiesList); }
     // Removes a handler previously added by addAnimationStateHandler.
     Q_INVOKABLE void removeAnimationStateHandler(QScriptValue handler) { _rig->removeAnimationStateHandler(handler); }
+    // Processes a handler result. Not really for user code, but used by invokeAnimationCallback.
+    Q_INVOKABLE void animationStateHandlerResult(QScriptValue handler, QScriptValue result) { _rig->animationStateHandlerResult(handler, result); }
 
     // get/set avatar data
     void saveData();

interface/src/avatar/SkeletonModel.cpp

@@ -112,7 +112,6 @@ const float PALM_PRIORITY = DEFAULT_PRIORITY;
 // Called within Model::simulate call, below.
 void SkeletonModel::updateRig(float deltaTime, glm::mat4 parentTransform) {
     Head* head = _owningAvatar->getHead();
-    _rig->cleanupAnimationStateHandler();
     if (_owningAvatar->isMyAvatar()) {
         MyAvatar* myAvatar = static_cast<MyAvatar*>(_owningAvatar);
         const FBXGeometry& geometry = _geometry->getFBXGeometry();

libraries/animation/src/AnimVariant.h

@@ -158,12 +158,14 @@ public:
     void setTrigger(const QString& key) { _triggers.insert(key); }
     void clearTriggers() { _triggers.clear(); }
 
+    void clearMap() { _map.clear(); }
     bool hasKey(const QString& key) const { return _map.find(key) != _map.end(); }
 
     // Answer a Plain Old Javascript Object (for the given engine) all of our values set as properties.
-    QScriptValue animVariantMapToScriptValue(QScriptEngine* engine);
+    QScriptValue animVariantMapToScriptValue(QScriptEngine* engine) const;
     // Side-effect us with the value of object's own properties. (No inherited properties.)
     void animVariantMapFromScriptValue(const QScriptValue& object);
+    void copyVariantsFrom(const AnimVariantMap& other);
 
 #ifdef NDEBUG
     void dump() const {
@@ -203,4 +205,6 @@ protected:
     std::set<QString> _triggers;
 };
 
+Q_DECLARE_METATYPE(AnimVariantMap)
+
 #endif // hifi_AnimVariant_h

libraries/animation/src/AnimVariantMap.cpp

@@ -14,7 +14,7 @@
 #include <RegisteredMetaTypes.h>
 #include "AnimVariant.h"
 
-QScriptValue AnimVariantMap::animVariantMapToScriptValue(QScriptEngine* engine) {
+QScriptValue AnimVariantMap::animVariantMapToScriptValue(QScriptEngine* engine) const {
     QScriptValue target = engine->newObject();
     for (auto& pair : _map) {
         switch (pair.second.getType()) {
@@ -43,6 +43,11 @@ QScriptValue AnimVariantMap::animVariantMapToScriptValue(QScriptEngine* engine)
     }
     return target;
 }
+void AnimVariantMap::copyVariantsFrom(const AnimVariantMap& other) {
+    for (auto& pair : other._map) {
+        _map[pair.first] = pair.second;
+    }
+}
 void AnimVariantMap::animVariantMapFromScriptValue(const QScriptValue& source) {
     // POTENTIAL OPTIMIZATION: cache the types we've seen. I.e, keep a dictionary mapping property names to an enumeration of types.
     // Whenever we identify a new outbound type in animVariantMapToScriptValue above, or a new inbound type in the code that follows here,

libraries/animation/src/Rig.cpp

@@ -605,40 +605,32 @@ void Rig::computeMotionAnimationState(float deltaTime, const glm::vec3& worldPos
     _lastPosition = worldPosition;
 }
 
-void Rig::addAnimationStateHandler(QScriptValue handler, QScriptValue propertiesList) {
+// Allow script to add/remove handlers and report results, from within their thread.
+// TODO: iterate multiple handlers, but with one shared arg.
+// TODO: fill the properties based on the union of requested properties. (Keep all properties objs and compute new union when add/remove handler.)
+void Rig::addAnimationStateHandler(QScriptValue handler, QScriptValue propertiesList) { // called in script thread
     _stateHandlers = handler;
 }
-void Rig::removeAnimationStateHandler(QScriptValue handler) {
-    _stateHandlersResultsToRemove = _stateHandlersResults;
-    _stateHandlers = _stateHandlersResults = QScriptValue();
+void Rig::removeAnimationStateHandler(QScriptValue handler) { // called in script thread
+    _stateHandlers = QScriptValue();
+    QMutexLocker locker(&_stateMutex); // guarding access to results
+    _stateHandlersResults.clearMap(); // TODO: When we have multiple handlers, we'll need to clear only his handler's results.
 }
-void Rig::cleanupAnimationStateHandler() {
-    if (!_stateHandlersResultsToRemove.isValid()) {
-        return;
-    }
-    QScriptValueIterator property(_stateHandlersResultsToRemove);
-    while (property.hasNext()) {
-        property.next();
-        _animVars.unset(property.name());
-    }
-    _stateHandlersResultsToRemove = QScriptValue();
+void Rig::animationStateHandlerResult(QScriptValue handler, QScriptValue result) { // called synchronously from script
+    // handler is currently ignored but might be used in storing individual results
+    QMutexLocker locker(&_stateMutex);
+    _stateHandlersResults.animVariantMapFromScriptValue(result); // Into our own copy.
 }
-void Rig::updateAnimationStateHandlers() {
-    if (!_stateHandlers.isValid()) {
-        return;
+void Rig::updateAnimationStateHandlers() { // called on avatar update thread (which may be main thread)
+    if (_stateHandlers.isValid()) {
+        // invokeMethod makes a copy of the args, and copies of AnimVariantMap do copy the underlying map, so this will correctly capture
+        // the state of _animVars and allow continued changes to _animVars in this thread without conflict.
+        QMetaObject::invokeMethod(_stateHandlers.engine(), "invokeAnimationCallback",  Qt::QueuedConnection,
+                                  Q_ARG(QScriptValue, _stateHandlers),
+                                  Q_ARG(AnimVariantMap, _animVars));
     }
-    // TODO: iterate multiple handlers, but with one shared arg.
-    // TODO: fill the properties based on the union of requested properties. (Keep all properties objs and compute new union when add/remove handler.)
-    // TODO: check QScriptEngine::hasUncaughtException()
-    // TODO: call asynchronously (through a signal on script), so that each script is single threaded, and so we never block here.
-    //       This will require inboundMaps to be kept in the list of per-handler data.
-    QScriptEngine* engine = _stateHandlers.engine();
-    QScriptValue outboundMap = _animVars.animVariantMapToScriptValue(engine);
-    QScriptValueList args;
-    args << outboundMap;
-    _stateHandlersResults = _stateHandlers.call(QScriptValue(), args);
-    _animVars.animVariantMapFromScriptValue(_stateHandlersResults);
-    //qCDebug(animation) << _animVars.lookup("foo", QString("not set"));
+    QMutexLocker locker(&_stateMutex); // as we examine/copy most recently computed state, if any. (Typically an earlier invocation.)
+    _animVars.copyVariantsFrom(_stateHandlersResults);
 }
 
 void Rig::updateAnimations(float deltaTime, glm::mat4 rootTransform) {

libraries/animation/src/Rig.h

@@ -202,7 +202,7 @@ public:
     bool disableHands {false}; // should go away with rig animation (and Rig::inverseKinematics)
     void addAnimationStateHandler(QScriptValue handler, QScriptValue propertiesList);
     void removeAnimationStateHandler(QScriptValue handler);
-    void cleanupAnimationStateHandler();
+    void animationStateHandlerResult(QScriptValue handler, QScriptValue result);
 
     bool getModelOffset(glm::vec3& modelOffsetOut) const;
 
@@ -248,9 +248,9 @@ public:
     float _rightHandOverlayAlpha = 0.0f;
 
 private:
-    QScriptValue _stateHandlers {};
-    QScriptValue _stateHandlersResults {};
-    QScriptValue _stateHandlersResultsToRemove {};
+    QScriptValue _stateHandlers;
+    AnimVariantMap _stateHandlersResults;
+    QMutex _stateMutex;
 };
 
 #endif /* defined(__hifi__Rig__) */

libraries/script-engine/src/ScriptEngine.cpp

@@ -258,6 +258,15 @@ void ScriptEngine::errorInLoadingScript(const QUrl& url) {
     }
 }
 
+// Even though we never pass AnimVariantMap directly to and from javascript, the queued invokeMethod of
+// invokeAnimationCallback requires that the type be registered.
+static QScriptValue animVarMapToScriptValue(QScriptEngine* engine, const AnimVariantMap& parameters) {
+    return parameters.animVariantMapToScriptValue(engine);
+}
+static void animVarMapFromScriptValue(const QScriptValue& value, AnimVariantMap& parameters) {
+    parameters.animVariantMapFromScriptValue(value);
+}
+
 void ScriptEngine::init() {
     if (_isInitialized) {
         return; // only initialize once
@@ -316,6 +325,7 @@ void ScriptEngine::init() {
     registerGlobalObject("Vec3", &_vec3Library);
     registerGlobalObject("Uuid", &_uuidLibrary);
     registerGlobalObject("AnimationCache", DependencyManager::get<AnimationCache>().data());
+    qScriptRegisterMetaType(this, animVarMapToScriptValue, animVarMapFromScriptValue);
 
     // constants
     globalObject().setProperty("TREE_SCALE", newVariant(QVariant(TREE_SCALE)));
@@ -718,6 +728,21 @@ void ScriptEngine::stop() {
     }
 }
 
+// Other threads can invoke this through invokeMethod, which causes the callback to be asynchronously executed in this script's thread.
+void ScriptEngine::invokeAnimationCallback(QScriptValue callback, AnimVariantMap parameters) {
+    checkThread();
+    QScriptValue javascriptParametgers = parameters.animVariantMapToScriptValue(this);
+    QScriptValueList callingArguments;
+    callingArguments << javascriptParametgers;
+    QScriptValue result = callback.call(QScriptValue(), callingArguments);
+    // We want to give the result back to the rig, but we don't have the rig or the avatar. But the global does.
+    // This is sort of like going through DependencyManager.get.
+    QScriptValue resultHandler = globalObject().property("MyAvatar").property("animationStateHandlerResult");
+    QScriptValueList resultArguments;
+    resultArguments << callback << result;
+    resultHandler.call(QScriptValue(), resultArguments); // Call it synchronously, on our own time and thread.
+}
+
 void ScriptEngine::timerFired() {
     QTimer* callingTimer = reinterpret_cast<QTimer*>(sender());
     QScriptValue timerFunction = _timerFunctionMap.value(callingTimer);
@@ -898,14 +923,20 @@ void ScriptEngine::load(const QString& loadFile) {
     }
 }
 
-// Look up the handler associated with eventName and entityID. If found, evalute the argGenerator thunk and call the handler with those args
-void ScriptEngine::generalHandler(const EntityItemID& entityID, const QString& eventName, std::function<QScriptValueList()> argGenerator) {
+bool ScriptEngine::checkThread() const {
     if (QThread::currentThread() != thread()) {
         qDebug() << "*** ERROR *** ScriptEngine::generalHandler() called on wrong thread [" << QThread::currentThread() << "], invoking on correct thread [" << thread() << "]";
         assert(false);
+        return true;
+    }
+    return false;
+}
+
+// Look up the handler associated with eventName and entityID. If found, evalute the argGenerator thunk and call the handler with those args
+void ScriptEngine::generalHandler(const EntityItemID& entityID, const QString& eventName, std::function<QScriptValueList()> argGenerator) {
+    if (checkThread()) {
         return;
     }
-
     if (!_registeredHandlers.contains(entityID)) {
         return;
     }

libraries/script-engine/src/ScriptEngine.h

@@ -21,6 +21,7 @@
 #include <QtScript/QScriptEngine>
 
 #include <AnimationCache.h>
+#include <AnimVariant.h>
 #include <AvatarData.h>
 #include <AvatarHashMap.h>
 #include <LimitedNodeList.h>
@@ -142,6 +143,9 @@ public:
     // NOTE - this is used by the TypedArray implemetation. we need to review this for thread safety
     ArrayBufferClass* getArrayBufferClass() { return _arrayBufferClass; }
 
+public slots:
+    void invokeAnimationCallback(QScriptValue callback, AnimVariantMap parameters);
+
 signals:
     void scriptLoaded(const QString& scriptFilename);
     void errorLoadingScript(const QString& scriptFilename);
@@ -169,6 +173,7 @@ protected:
     bool _wantSignals = true;
     QHash<EntityItemID, EntityScriptDetails> _entityScripts;
 private:
+    bool checkThread() const;
     void init();
     QString getFilename() const;
     void waitTillDoneRunning();