diff --git a/assignment-client/src/avatars/ScriptableAvatar.cpp b/assignment-client/src/avatars/ScriptableAvatar.cpp index 929cd68f7c4..a1db33fbc87 100644 --- a/assignment-client/src/avatars/ScriptableAvatar.cpp +++ b/assignment-client/src/avatars/ScriptableAvatar.cpp @@ -30,7 +30,7 @@ #include -ScriptableAvatar::ScriptableAvatar(): _scriptEngine(newScriptEngine()) { +ScriptableAvatar::ScriptableAvatar() { _clientTraitsHandler.reset(new ClientTraitsHandler(this)); static std::once_flag once; std::call_once(once, [] { @@ -344,7 +344,9 @@ AvatarEntityMap ScriptableAvatar::getAvatarEntityDataInternal(bool allProperties EntityItemProperties properties = entity->getProperties(desiredProperties); QByteArray blob; - EntityItemProperties::propertiesToBlob(*_scriptEngine, sessionID, properties, blob, allProperties); + _helperScriptEngine.run( [&] { + EntityItemProperties::propertiesToBlob(*_helperScriptEngine.get(), sessionID, properties, blob, allProperties); + }); data[id] = blob; } }); @@ -368,8 +370,12 @@ void ScriptableAvatar::setAvatarEntityData(const AvatarEntityMap& avatarEntityDa while (dataItr != avatarEntityData.end()) { EntityItemProperties properties; const QByteArray& blob = dataItr.value(); - if (!blob.isNull() && EntityItemProperties::blobToProperties(*_scriptEngine, blob, properties)) { - newProperties[dataItr.key()] = properties; + if (!blob.isNull()) { + _helperScriptEngine.run([&] { + if (EntityItemProperties::blobToProperties(*_helperScriptEngine.get(), blob, properties)) { + newProperties[dataItr.key()] = properties; + } + }); } ++dataItr; } @@ -448,9 +454,16 @@ void ScriptableAvatar::updateAvatarEntity(const QUuid& entityID, const QByteArra EntityItemPointer entity; EntityItemProperties properties; - if (!EntityItemProperties::blobToProperties(*_scriptEngine, entityData, properties)) { - // entityData is corrupt - return; + { + // TODO: checking how often this happens and what is the performance impact of having the script engine on separate thread + // If it's happening often, a method to move HelperScriptEngine into the current thread would be a good idea + bool result = _helperScriptEngine.runWithResult ( [&]() { + return EntityItemProperties::blobToProperties(*_helperScriptEngine.get(), entityData, properties); + }); + if (!result) { + // entityData is corrupt + return; + } } std::map::iterator itr = _entities.find(entityID); diff --git a/assignment-client/src/avatars/ScriptableAvatar.h b/assignment-client/src/avatars/ScriptableAvatar.h index 192de31a269..7e79d25cd0f 100644 --- a/assignment-client/src/avatars/ScriptableAvatar.h +++ b/assignment-client/src/avatars/ScriptableAvatar.h @@ -21,6 +21,7 @@ #include #include "model-networking/ModelCache.h" #include "Rig.h" +#include /*@jsdoc * The Avatar API is used to manipulate scriptable avatars on the domain. This API is a subset of the @@ -228,7 +229,7 @@ public slots: QHash _fstJointIndices; ///< 1-based, since zero is returned for missing keys QStringList _fstJointNames; ///< in order of depth-first traversal QUrl _skeletonModelFilenameURL; // This contains URL from filename field in fst file - mutable ScriptEnginePointer _scriptEngine; + mutable HelperScriptEngine _helperScriptEngine; std::map _entities; /// Loads the joint indices, names from the FST file (if any) diff --git a/interface/src/avatar/MyAvatar.cpp b/interface/src/avatar/MyAvatar.cpp index 21ac211e78c..5d3d6f0d1f6 100644 --- a/interface/src/avatar/MyAvatar.cpp +++ b/interface/src/avatar/MyAvatar.cpp @@ -1740,10 +1740,11 @@ void MyAvatar::handleChangedAvatarEntityData() { blobFailed = true; // blob doesn't exist return; } - std::lock_guard guard(_scriptEngineLock); - if (!EntityItemProperties::blobToProperties(*_scriptEngine, itr.value(), properties)) { - blobFailed = true; // blob is corrupt - } + _helperScriptEngine.run( [&] { + if (!EntityItemProperties::blobToProperties(*_helperScriptEngine.get(), itr.value(), properties)) { + blobFailed = true; // blob is corrupt + } + }); }); if (blobFailed) { // remove from _cachedAvatarEntityBlobUpdatesToSkip just in case: @@ -1776,10 +1777,11 @@ void MyAvatar::handleChangedAvatarEntityData() { skip = true; return; } - std::lock_guard guard(_scriptEngineLock); - if (!EntityItemProperties::blobToProperties(*_scriptEngine, itr.value(), properties)) { - skip = true; - } + _helperScriptEngine.run( [&] { + if (!EntityItemProperties::blobToProperties(*_helperScriptEngine.get(), itr.value(), properties)) { + skip = true; + } + }); }); if (!skip && canRezAvatarEntites) { sanitizeAvatarEntityProperties(properties); @@ -1884,10 +1886,9 @@ bool MyAvatar::updateStaleAvatarEntityBlobs() const { if (found) { ++numFound; QByteArray blob; - { - std::lock_guard guard(_scriptEngineLock); - EntityItemProperties::propertiesToBlob(*_scriptEngine, getID(), properties, blob); - } + _helperScriptEngine.run( [&] { + EntityItemProperties::propertiesToBlob(*_helperScriptEngine.get(), getID(), properties, blob); + }); _avatarEntitiesLock.withWriteLock([&] { _cachedAvatarEntityBlobs[id] = blob; }); @@ -1948,10 +1949,9 @@ AvatarEntityMap MyAvatar::getAvatarEntityData() const { EntityItemProperties properties = entity->getProperties(desiredProperties); QByteArray blob; - { - std::lock_guard guard(_scriptEngineLock); - EntityItemProperties::propertiesToBlob(*_scriptEngine, getID(), properties, blob, true); - } + _helperScriptEngine.run( [&] { + EntityItemProperties::propertiesToBlob(*_helperScriptEngine.get(), getID(), properties, blob, true); + }); data[entityID] = blob; } @@ -2093,9 +2093,6 @@ void MyAvatar::avatarEntityDataToJson(QJsonObject& root) const { } void MyAvatar::loadData() { - if (!_scriptEngine) { - _scriptEngine = newScriptEngine(); - } getHead()->setBasePitch(_headPitchSetting.get()); _yawSpeed = _yawSpeedSetting.get(_yawSpeed); @@ -2704,11 +2701,10 @@ QVariantList MyAvatar::getAvatarEntitiesVariant() { QVariantMap avatarEntityData; avatarEntityData["id"] = entityID; EntityItemProperties entityProperties = entity->getProperties(desiredProperties); - { - std::lock_guard guard(_scriptEngineLock); - ScriptValue scriptProperties = EntityItemPropertiesToScriptValue(_scriptEngine.get(), entityProperties); + _helperScriptEngine.run( [&] { + ScriptValue scriptProperties = EntityItemPropertiesToScriptValue(_helperScriptEngine.get(), entityProperties); avatarEntityData["properties"] = scriptProperties.toVariant(); - } + }); avatarEntitiesData.append(QVariant(avatarEntityData)); } } diff --git a/interface/src/avatar/MyAvatar.h b/interface/src/avatar/MyAvatar.h index 4198deba84f..60c07ad42cb 100644 --- a/interface/src/avatar/MyAvatar.h +++ b/interface/src/avatar/MyAvatar.h @@ -28,6 +28,7 @@ #include #include #include +#include #include #include #include @@ -3102,8 +3103,10 @@ private slots: mutable std::set _staleCachedAvatarEntityBlobs; // // keep a ScriptEngine around so we don't have to instantiate on the fly (these are very slow to create/delete) - mutable std::mutex _scriptEngineLock; - ScriptEnginePointer _scriptEngine { nullptr }; + // TODO: profile if it performs better when script engine is on avatar thread or on its own thread + // Own thread is safer from deadlocks + mutable HelperScriptEngine _helperScriptEngine; + bool _needToSaveAvatarEntitySettings { false }; bool _reactionTriggers[NUM_AVATAR_TRIGGER_REACTIONS] { false, false }; diff --git a/libraries/baking/src/MaterialBaker.cpp b/libraries/baking/src/MaterialBaker.cpp index 5cb108721d0..631a10fd11f 100644 --- a/libraries/baking/src/MaterialBaker.cpp +++ b/libraries/baking/src/MaterialBaker.cpp @@ -36,8 +36,7 @@ MaterialBaker::MaterialBaker(const QString& materialData, bool isURL, const QStr _isURL(isURL), _destinationPath(destinationPath), _bakedOutputDir(bakedOutputDir), - _textureOutputDir(bakedOutputDir + "/materialTextures/" + QString::number(materialNum++)), - _scriptEngine(newScriptEngine()) + _textureOutputDir(bakedOutputDir + "/materialTextures/" + QString::number(materialNum++)) { } @@ -214,16 +213,20 @@ void MaterialBaker::outputMaterial() { if (_materialResource->parsedMaterials.networkMaterials.size() == 1) { auto networkMaterial = _materialResource->parsedMaterials.networkMaterials.begin(); auto scriptableMaterial = scriptable::ScriptableMaterial(networkMaterial->second); - QVariant materialVariant = - scriptable::scriptableMaterialToScriptValue(_scriptEngine.get(), scriptableMaterial).toVariant(); - json.insert("materials", QJsonDocument::fromVariant(materialVariant).object()); + _helperScriptEngine.run( [&] { + QVariant materialVariant = + scriptable::scriptableMaterialToScriptValue(_helperScriptEngine.get(), scriptableMaterial).toVariant(); + json.insert("materials", QJsonDocument::fromVariant(materialVariant).object()); + }); } else { QJsonArray materialArray; for (auto networkMaterial : _materialResource->parsedMaterials.networkMaterials) { auto scriptableMaterial = scriptable::ScriptableMaterial(networkMaterial.second); - QVariant materialVariant = - scriptable::scriptableMaterialToScriptValue(_scriptEngine.get(), scriptableMaterial).toVariant(); - materialArray.append(QJsonDocument::fromVariant(materialVariant).object()); + _helperScriptEngine.run( [&] { + QVariant materialVariant = + scriptable::scriptableMaterialToScriptValue(_helperScriptEngine.get(), scriptableMaterial).toVariant(); + materialArray.append(QJsonDocument::fromVariant(materialVariant).object()); + }); } json.insert("materials", materialArray); } diff --git a/libraries/baking/src/MaterialBaker.h b/libraries/baking/src/MaterialBaker.h index 129b36aa8fd..4fd8948b037 100644 --- a/libraries/baking/src/MaterialBaker.h +++ b/libraries/baking/src/MaterialBaker.h @@ -21,6 +21,7 @@ #include "TextureBaker.h" #include "baking/TextureFileNamer.h" +#include #include #include @@ -72,7 +73,7 @@ private slots: QString _textureOutputDir; QString _bakedMaterialData; - ScriptEnginePointer _scriptEngine; + HelperScriptEngine _helperScriptEngine; static std::function _getNextOvenWorkerThreadOperator; TextureFileNamer _textureFileNamer; diff --git a/libraries/entities/src/EntityTree.cpp b/libraries/entities/src/EntityTree.cpp index fb7fbe64998..a28cf0b7904 100644 --- a/libraries/entities/src/EntityTree.cpp +++ b/libraries/entities/src/EntityTree.cpp @@ -2553,11 +2553,10 @@ bool EntityTree::writeToMap(QVariantMap& entityDescription, OctreeElementPointer } entityDescription["DataVersion"] = _persistDataVersion; entityDescription["Id"] = _persistID; - const std::lock_guard scriptLock(scriptEngineMutex); - RecurseOctreeToMapOperator theOperator(entityDescription, element, scriptEngine.get(), skipDefaultValues, - skipThoseWithBadParents, _myAvatar); - withReadLock([&] { - recurseTreeWithOperator(&theOperator); + _helperScriptEngine.run( [&] { + RecurseOctreeToMapOperator theOperator(entityDescription, element, _helperScriptEngine.get(), skipDefaultValues, + skipThoseWithBadParents, _myAvatar); + withReadLock([&] { recurseTreeWithOperator(&theOperator); }); }); return true; } @@ -2728,11 +2727,10 @@ bool EntityTree::readFromMap(QVariantMap& map, const bool isImport) { } EntityItemProperties properties; - { - const std::lock_guard scriptLock(scriptEngineMutex); - ScriptValue entityScriptValue = variantMapToScriptValue(entityMap, *scriptEngine); + _helperScriptEngine.run( [&] { + ScriptValue entityScriptValue = variantMapToScriptValue(entityMap, *_helperScriptEngine.get()); EntityItemPropertiesFromScriptValueIgnoreReadOnly(entityScriptValue, properties); - } + }); EntityItemID entityItemID; if (entityMap.contains("id")) { @@ -2881,13 +2879,12 @@ bool EntityTree::readFromMap(QVariantMap& map, const bool isImport) { } bool EntityTree::writeToJSON(QString& jsonString, const OctreeElementPointer& element) { - const std::lock_guard scriptLock(scriptEngineMutex); - RecurseOctreeToJSONOperator theOperator(element, scriptEngine.get(), jsonString); - withReadLock([&] { - recurseTreeWithOperator(&theOperator); - }); + _helperScriptEngine.run( [&] { + RecurseOctreeToJSONOperator theOperator(element, _helperScriptEngine.get(), jsonString); + withReadLock([&] { recurseTreeWithOperator(&theOperator); }); - jsonString = theOperator.getJson(); + jsonString = theOperator.getJson(); + }); return true; } diff --git a/libraries/entities/src/EntityTree.h b/libraries/entities/src/EntityTree.h index d5c3a74eb77..c93a3b15270 100644 --- a/libraries/entities/src/EntityTree.h +++ b/libraries/entities/src/EntityTree.h @@ -15,6 +15,7 @@ #include #include +#include #include #include @@ -387,8 +388,7 @@ class EntityTree : public Octree, public SpatialParentTree { MovingEntitiesOperator& moveOperator, bool force, bool tellServer); // Script engine for writing entity tree data to and from JSON - std::mutex scriptEngineMutex; - ScriptEnginePointer scriptEngine{ newScriptEngine() }; + HelperScriptEngine _helperScriptEngine; }; void convertGrabUserDataToProperties(EntityItemProperties& properties); diff --git a/libraries/script-engine/src/HelperScriptEngine.cpp b/libraries/script-engine/src/HelperScriptEngine.cpp new file mode 100644 index 00000000000..313fac74daa --- /dev/null +++ b/libraries/script-engine/src/HelperScriptEngine.cpp @@ -0,0 +1,31 @@ +// +// HelperScriptEngine.h +// libraries/script-engine/src/HelperScriptEngine.h +// +// Created by dr Karol Suprynowicz on 2024/04/28. +// Copyright 2024 Overte e.V. +// +// Distributed under the Apache License, Version 2.0. +// See the accompanying file LICENSE or http://www.apache.org/licenses/LICENSE-2.0.html +// + +#include "HelperScriptEngine.h" + +HelperScriptEngine::HelperScriptEngine() { + std::lock_guard lock(_scriptEngineLock); + _scriptEngine = newScriptEngine(); + _scriptEngineThread.reset(new QThread()); + _scriptEngine->setThread(_scriptEngineThread.get()); + _scriptEngineThread->start(); +} + +HelperScriptEngine::~HelperScriptEngine() { + std::lock_guard lock(_scriptEngineLock); + if (_scriptEngine) { + if (_scriptEngineThread) { + _scriptEngineThread->quit(); + _scriptEngineThread->wait(); + } + _scriptEngine.reset(); + } +} diff --git a/libraries/script-engine/src/HelperScriptEngine.h b/libraries/script-engine/src/HelperScriptEngine.h new file mode 100644 index 00000000000..53d2e893068 --- /dev/null +++ b/libraries/script-engine/src/HelperScriptEngine.h @@ -0,0 +1,65 @@ +// +// HelperScriptEngine.h +// libraries/script-engine/src/HelperScriptEngine.h +// +// Created by dr Karol Suprynowicz on 2024/04/28. +// Copyright 2024 Overte e.V. +// +// Distributed under the Apache License, Version 2.0. +// See the accompanying file LICENSE or http://www.apache.org/licenses/LICENSE-2.0.html +// + +#ifndef overte_HelperScriptEngine_h +#define overte_HelperScriptEngine_h + +#include +#include "QThread" + +#include "ScriptEngine.h" + +/** + * @brief Provides a wrapper around script engine that does not have ScriptManager + * + * HelperScriptEngine is used for performing smaller tasks, like for example conversions between entity + * properties and JSON data. + * For thread safety all accesses to helper script engine need to be done either through HelperScriptEngine::run() + * or HelperScriptEngine::runWithResult(). + * + */ + + +class HelperScriptEngine { +public: + HelperScriptEngine(); + ~HelperScriptEngine(); + + template + inline void run(F&& f) { + std::lock_guard guard(_scriptEngineLock); + f(); + } + + template + inline T runWithResult(F&& f) { + T result; + { + std::lock_guard guard(_scriptEngineLock); + result = f(); + } + return result; + } + + /** + * @brief Returns pointer to the script engine + * + * This function should be used only inside HelperScriptEngine::run() or HelperScriptEngine::runWithResult() + */ + ScriptEngine* get() { return _scriptEngine.get(); }; + ScriptEnginePointer getShared() { return _scriptEngine; }; +private: + std::mutex _scriptEngineLock; + ScriptEnginePointer _scriptEngine { nullptr }; + std::shared_ptr _scriptEngineThread { nullptr }; +}; + +#endif //overte_HelperScriptEngine_h