diff --git a/libraries/script-engine/src/ScriptEngine.cpp b/libraries/script-engine/src/ScriptEngine.cpp index 738e313f8f..9ee2c374c5 100644 --- a/libraries/script-engine/src/ScriptEngine.cpp +++ b/libraries/script-engine/src/ScriptEngine.cpp @@ -485,10 +485,10 @@ void ScriptEngine::removeEventHandler(const EntityItemID& entityID, const QStrin return; } RegisteredEventHandlers& handlersOnEntity = _registeredHandlers[entityID]; - QScriptValueList& handlersForEvent = handlersOnEntity[eventName]; + CallbackList& handlersForEvent = handlersOnEntity[eventName]; // QScriptValue does not have operator==(), so we can't use QList::removeOne and friends. So iterate. for (int i = 0; i < handlersForEvent.count(); ++i) { - if (handlersForEvent[i].equals(handler)) { + if (handlersForEvent[i].function.equals(handler)) { handlersForEvent.removeAt(i); return; // Design choice: since comparison is relatively expensive, just remove the first matching handler. } @@ -516,6 +516,13 @@ void ScriptEngine::addEventHandler(const EntityItemID& entityID, const QString& // Connect up ALL the handlers to the global entities object's signals. // (We could go signal by signal, or even handler by handler, but I don't think the efficiency is worth the complexity.) auto entities = DependencyManager::get(); + // Bug? These handlers are deleted when entityID is deleted, which is nice. + // But if they are created by an entity script on a different entity, should they also be deleted when the entity script unloads? + // E.g., suppose a bow has an entity script that causes arrows to be created with a potential lifetime greater than the bow, + // and that the entity script adds (e.g., collision) handlers to the arrows. Should those handlers fire if the bow is unloaded? + // Also, what about when the entity script is REloaded? + // For now, we are leaving them around. Changing that would require some non-trivial digging around to find the + // handlers that were added while a given currentEntityIdentifier was in place. I don't think this is dangerous. Just perhaps unexpected. -HRS connect(entities.data(), &EntityScriptingInterface::deletingEntity, this, [this](const EntityItemID& entityID) { _registeredHandlers.remove(entityID); }); @@ -563,8 +570,9 @@ void ScriptEngine::addEventHandler(const EntityItemID& entityID, const QString& if (!_registeredHandlers.contains(entityID)) { _registeredHandlers[entityID] = RegisteredEventHandlers(); } - QScriptValueList& handlersForEvent = _registeredHandlers[entityID][eventName]; - handlersForEvent << handler; // Note that the same handler can be added many times. See removeEntityEventHandler(). + CallbackList& handlersForEvent = _registeredHandlers[entityID][eventName]; + CallbackData handlerData = {handler, currentEntityIdentifier}; + handlersForEvent << handlerData; // Note that the same handler can be added many times. See removeEntityEventHandler(). } @@ -705,13 +713,30 @@ void ScriptEngine::run() { // NOTE: This is private because it must be called on the same thread that created the timers, which is why // we want to only call it in our own run "shutdown" processing. void ScriptEngine::stopAllTimers() { - QMutableHashIterator i(_timerFunctionMap); + QMutableHashIterator i(_timerFunctionMap); while (i.hasNext()) { i.next(); QTimer* timer = i.key(); stopTimer(timer); } } +void ScriptEngine::stopAllTimersForEntityScript(const EntityItemID& entityID) { + // We could maintain a separate map of entityID => QTimer, but someone will have to prove to me that it's worth the complexity. -HRS + QVector toDelete; + QMutableHashIterator i(_timerFunctionMap); + while (i.hasNext()) { + i.next(); + if (i.value().definingEntityIdentifier != entityID) { + continue; + } + QTimer* timer = i.key(); + toDelete << timer; // don't delete while we're iterating. save it. + } + for (auto timer:toDelete) { // now reap 'em + stopTimer(timer); + } + +} void ScriptEngine::stop() { if (!_isFinished) { @@ -743,13 +768,14 @@ void ScriptEngine::callAnimationStateHandler(QScriptValue callback, AnimVariantM QScriptValue javascriptParameters = parameters.animVariantMapToScriptValue(this, names, useNames); QScriptValueList callingArguments; callingArguments << javascriptParameters; + assert(currentEntityIdentifier.isInvalidID()); // No animation state handlers from entity scripts. QScriptValue result = callback.call(QScriptValue(), callingArguments); resultHandler(result); } void ScriptEngine::timerFired() { QTimer* callingTimer = reinterpret_cast(sender()); - QScriptValue timerFunction = _timerFunctionMap.value(callingTimer); + CallbackData timerData = _timerFunctionMap.value(callingTimer); if (!callingTimer->isActive()) { // this timer is done, we can kill it @@ -758,11 +784,12 @@ void ScriptEngine::timerFired() { } // call the associated JS function, if it exists - if (timerFunction.isValid()) { - timerFunction.call(); + if (timerData.function.isValid()) { + callWithEnvironment(timerData.definingEntityIdentifier, timerData.function, timerData.function, QScriptValueList()); } } + QObject* ScriptEngine::setupTimerWithInterval(const QScriptValue& function, int intervalMS, bool isSingleShot) { // create the timer, add it to the map, and start it QTimer* newTimer = new QTimer(this); @@ -773,7 +800,8 @@ QObject* ScriptEngine::setupTimerWithInterval(const QScriptValue& function, int // make sure the timer stops when the script does connect(this, &ScriptEngine::scriptEnding, newTimer, &QTimer::stop); - _timerFunctionMap.insert(newTimer, function); + CallbackData timerData = {function, currentEntityIdentifier}; + _timerFunctionMap.insert(newTimer, timerData); newTimer->start(intervalMS); return newTimer; @@ -859,6 +887,7 @@ void ScriptEngine::include(const QStringList& includeFiles, QScriptValue callbac } BatchLoader* loader = new BatchLoader(urls); + EntityItemID capturedEntityIdentifier = currentEntityIdentifier; auto evaluateScripts = [=](const QMap& data) { auto parentURL = _parentURL; @@ -870,13 +899,16 @@ void ScriptEngine::include(const QStringList& includeFiles, QScriptValue callbac // Set the parent url so that path resolution will be relative // to this script's url during its initial evaluation _parentURL = url.toString(); - QScriptValue result = evaluate(contents, url.toString()); + auto operation = [&]() { + evaluate(contents, url.toString()); + }; + doWithEnvironment(capturedEntityIdentifier, operation); } } _parentURL = parentURL; if (callback.isFunction()) { - QScriptValue(callback).call(); + callWithEnvironment(capturedEntityIdentifier, QScriptValue(callback), QScriptValue(), QScriptValueList()); } loader->deleteLater(); @@ -917,6 +949,11 @@ void ScriptEngine::load(const QString& loadFile) { << "loadFile:" << loadFile << "parent script:" << getFilename(); return; // bail early } + if (!currentEntityIdentifier.isInvalidID()) { + qCWarning(scriptengine) << "Script.load() from entity script is ignored... " + << "loadFile:" << loadFile << "parent script:" << getFilename(); + return; // bail early + } QUrl url = resolvePath(loadFile); if (_isReloading) { @@ -933,7 +970,7 @@ 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::forwardHandlerCall(const EntityItemID& entityID, const QString& eventName, QScriptValueList eventHanderArgs) { +void ScriptEngine::forwardHandlerCall(const EntityItemID& entityID, const QString& eventName, QScriptValueList eventHandlerArgs) { if (QThread::currentThread() != thread()) { qDebug() << "*** ERROR *** ScriptEngine::forwardHandlerCall() called on wrong thread [" << QThread::currentThread() << "], invoking on correct thread [" << thread() << "]"; assert(false); @@ -946,10 +983,13 @@ void ScriptEngine::forwardHandlerCall(const EntityItemID& entityID, const QStrin if (!handlersOnEntity.contains(eventName)) { return; } - QScriptValueList handlersForEvent = handlersOnEntity[eventName]; + CallbackList handlersForEvent = handlersOnEntity[eventName]; if (!handlersForEvent.isEmpty()) { for (int i = 0; i < handlersForEvent.count(); ++i) { - handlersForEvent[i].call(QScriptValue(), eventHanderArgs); + // handlersForEvent[i] can tonain many handlers that may have each been added by different interface or entity scripts, + // and the entity scripts may be for entities other than the one this is a handler for. + // Fortunately, the definingEntityIdentifier captured the entity script id (if any) when the handler was added. + callWithEnvironment(handlersForEvent[i].definingEntityIdentifier, handlersForEvent[i].function, QScriptValue(), eventHandlerArgs); } } } @@ -1055,9 +1095,13 @@ void ScriptEngine::entityScriptContentAvailable(const EntityItemID& entityID, co QString file = QUrl(scriptOrURL).toLocalFile(); lastModified = (quint64)QFileInfo(file).lastModified().toMSecsSinceEpoch(); } + QScriptValue entityScriptConstructor, entityScriptObject; + auto initialization = [&]{ + entityScriptConstructor = evaluate(contents, fileName); + entityScriptObject = entityScriptConstructor.construct(); + }; + doWithEnvironment(entityID, initialization); - QScriptValue entityScriptConstructor = evaluate(contents, fileName); - QScriptValue entityScriptObject = entityScriptConstructor.construct(); EntityScriptDetails newDetails = { scriptOrURL, entityScriptObject, lastModified }; _entityScripts[entityID] = newDetails; if (isURL) { @@ -1087,6 +1131,7 @@ void ScriptEngine::unloadEntityScript(const EntityItemID& entityID) { if (_entityScripts.contains(entityID)) { callEntityScriptMethod(entityID, "unload"); _entityScripts.remove(entityID); + stopAllTimersForEntityScript(entityID); } } @@ -1142,6 +1187,32 @@ void ScriptEngine::refreshFileScript(const EntityItemID& entityID) { recurseGuard = false; } +// Execute operation in the appropriate context for (the possibly empty) entityID. +// Even if entityID is supplied as currentEntityIdentifier, this still documents the source +// of the code being executed (e.g., if we ever sandbox different entity scripts, or provide different +// global values for different entity scripts). +void ScriptEngine::doWithEnvironment(const EntityItemID& entityID, std::function operation) { + EntityItemID oldIdentifier = currentEntityIdentifier; + currentEntityIdentifier = entityID; + +#if DEBUG_CURRENT_ENTITY + QScriptValue oldData = this->globalObject().property("debugEntityID"); + this->globalObject().setProperty("debugEntityID", entityID.toScriptValue(this)); // Make the entityID available to javascript as a global. + operation(); + this->globalObject().setProperty("debugEntityID", oldData); +#else + operation(); +#endif + + currentEntityIdentifier = oldIdentifier; +} +void ScriptEngine::callWithEnvironment(const EntityItemID& entityID, QScriptValue function, QScriptValue thisObject, QScriptValueList args) { + auto operation = [&]() { + function.call(thisObject, args); + }; + doWithEnvironment(entityID, operation); +} + void ScriptEngine::callEntityScriptMethod(const EntityItemID& entityID, const QString& methodName, const QStringList& params) { if (QThread::currentThread() != thread()) { #ifdef THREAD_DEBUGGING @@ -1168,7 +1239,7 @@ void ScriptEngine::callEntityScriptMethod(const EntityItemID& entityID, const QS QScriptValueList args; args << entityID.toScriptValue(this); args << qScriptValueFromSequence(this, params); - entityScript.property(methodName).call(entityScript, args); + callWithEnvironment(entityID, entityScript.property(methodName), entityScript, args); } } @@ -1200,7 +1271,7 @@ void ScriptEngine::callEntityScriptMethod(const EntityItemID& entityID, const QS QScriptValueList args; args << entityID.toScriptValue(this); args << event.toScriptValue(this); - entityScript.property(methodName).call(entityScript, args); + callWithEnvironment(entityID, entityScript.property(methodName), entityScript, args); } } } @@ -1234,7 +1305,7 @@ void ScriptEngine::callEntityScriptMethod(const EntityItemID& entityID, const QS args << entityID.toScriptValue(this); args << otherID.toScriptValue(this); args << collisionToScriptValue(this, collision); - entityScript.property(methodName).call(entityScript, args); + callWithEnvironment(entityID, entityScript.property(methodName), entityScript, args); } } } diff --git a/libraries/script-engine/src/ScriptEngine.h b/libraries/script-engine/src/ScriptEngine.h index f89998d9f0..91d4c3f5a5 100644 --- a/libraries/script-engine/src/ScriptEngine.h +++ b/libraries/script-engine/src/ScriptEngine.h @@ -42,7 +42,14 @@ const QString NO_SCRIPT(""); const unsigned int SCRIPT_DATA_CALLBACK_USECS = floor(((1.0f / 60.0f) * 1000 * 1000) + 0.5f); -typedef QHash RegisteredEventHandlers; +class CallbackData { +public: + QScriptValue function; + EntityItemID definingEntityIdentifier; +}; + +typedef QList CallbackList; +typedef QHash RegisteredEventHandlers; class EntityScriptDetails { public: @@ -169,7 +176,7 @@ protected: std::atomic _isRunning { false }; int _evaluatesPending { 0 }; bool _isInitialized { false }; - QHash _timerFunctionMap; + QHash _timerFunctionMap; QSet _includedURLs; bool _wantSignals { true }; QHash _entityScripts; @@ -181,6 +188,7 @@ protected: bool evaluatePending() const { return _evaluatesPending > 0; } void timerFired(); void stopAllTimers(); + void stopAllTimersForEntityScript(const EntityItemID& entityID); void refreshFileScript(const EntityItemID& entityID); void setParentURL(const QString& parentURL) { _parentURL = parentURL; } @@ -203,6 +211,10 @@ protected: void forwardHandlerCall(const EntityItemID& entityID, const QString& eventName, QScriptValueList eventHanderArgs); Q_INVOKABLE void entityScriptContentAvailable(const EntityItemID& entityID, const QString& scriptOrURL, const QString& contents, bool isURL, bool success); + EntityItemID currentEntityIdentifier {}; // Contains the defining entity script entity id during execution, if any. Empty for interface script execution. + void doWithEnvironment(const EntityItemID& entityID, std::function operation); + void callWithEnvironment(const EntityItemID& entityID, QScriptValue function, QScriptValue thisObject, QScriptValueList args); + friend class ScriptEngines; static std::atomic _stoppingAllScripts; };