From 9fd61907f51aa79d136f5cbf1cb6a7032b3654d8 Mon Sep 17 00:00:00 2001 From: Howard Stearns Date: Wed, 21 Oct 2015 20:50:07 -0700 Subject: [PATCH] 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 | 2 + interface/src/avatar/SkeletonModel.cpp | 1 - libraries/animation/src/AnimVariant.h | 6 ++- libraries/animation/src/AnimVariantMap.cpp | 7 ++- libraries/animation/src/Rig.cpp | 50 ++++++++------------ libraries/animation/src/Rig.h | 8 ++-- libraries/script-engine/src/ScriptEngine.cpp | 37 +++++++++++++-- libraries/script-engine/src/ScriptEngine.h | 5 ++ 8 files changed, 77 insertions(+), 39 deletions(-) diff --git a/interface/src/avatar/MyAvatar.h b/interface/src/avatar/MyAvatar.h index 49f6c33aca..1ce72cb33f 100644 --- a/interface/src/avatar/MyAvatar.h +++ b/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(); diff --git a/interface/src/avatar/SkeletonModel.cpp b/interface/src/avatar/SkeletonModel.cpp index 5c98d4cd9b..003677bf90 100644 --- a/interface/src/avatar/SkeletonModel.cpp +++ b/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(_owningAvatar); const FBXGeometry& geometry = _geometry->getFBXGeometry(); diff --git a/libraries/animation/src/AnimVariant.h b/libraries/animation/src/AnimVariant.h index 0a91c82d80..2d140bfa52 100644 --- a/libraries/animation/src/AnimVariant.h +++ b/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 _triggers; }; +Q_DECLARE_METATYPE(AnimVariantMap) + #endif // hifi_AnimVariant_h diff --git a/libraries/animation/src/AnimVariantMap.cpp b/libraries/animation/src/AnimVariantMap.cpp index f626e46c0f..0154c9698a 100644 --- a/libraries/animation/src/AnimVariantMap.cpp +++ b/libraries/animation/src/AnimVariantMap.cpp @@ -14,7 +14,7 @@ #include #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, diff --git a/libraries/animation/src/Rig.cpp b/libraries/animation/src/Rig.cpp index 215e0095c0..14aef88e2f 100644 --- a/libraries/animation/src/Rig.cpp +++ b/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) { diff --git a/libraries/animation/src/Rig.h b/libraries/animation/src/Rig.h index 3f68deef1a..c29bb6ad04 100644 --- a/libraries/animation/src/Rig.h +++ b/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__) */ diff --git a/libraries/script-engine/src/ScriptEngine.cpp b/libraries/script-engine/src/ScriptEngine.cpp index 76590f266b..0ec1a09d6d 100644 --- a/libraries/script-engine/src/ScriptEngine.cpp +++ b/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().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(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 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 argGenerator) { + if (checkThread()) { return; } - if (!_registeredHandlers.contains(entityID)) { return; } diff --git a/libraries/script-engine/src/ScriptEngine.h b/libraries/script-engine/src/ScriptEngine.h index 1d3986143a..77f1e2d5f1 100644 --- a/libraries/script-engine/src/ScriptEngine.h +++ b/libraries/script-engine/src/ScriptEngine.h @@ -21,6 +21,7 @@ #include #include +#include #include #include #include @@ -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 _entityScripts; private: + bool checkThread() const; void init(); QString getFilename() const; void waitTillDoneRunning();