From 3bc9e8c98de63c7063ad65285e41a58fdf890472 Mon Sep 17 00:00:00 2001 From: Brad Davis Date: Thu, 10 Sep 2015 14:12:55 -0700 Subject: [PATCH] Fixing some entity race condition crashes --- .../octree/OctreeInboundPacketProcessor.cpp | 18 +- .../src/octree/OctreeSendThread.cpp | 146 +++++----- examples/tests/entityEditStressTest.js | 175 +++++++++++ examples/tests/timerStressTest.js | 68 +++++ interface/src/Application.cpp | 211 +++++++------- interface/src/Application.h | 3 - interface/src/avatar/AvatarActionHold.cpp | 105 ++++--- interface/src/avatar/AvatarManager.cpp | 26 +- interface/src/avatar/AvatarManager.h | 11 +- interface/src/ui/OctreeStatsDialog.cpp | 127 ++++---- .../src/EntityTreeRenderer.cpp | 42 ++- libraries/entities/src/EntityItem.cpp | 213 ++++---------- libraries/entities/src/EntityItem.h | 12 +- .../entities/src/EntityScriptingInterface.cpp | 274 +++++++++--------- libraries/entities/src/EntitySimulation.cpp | 23 +- libraries/entities/src/EntitySimulation.h | 28 +- libraries/entities/src/EntityTree.cpp | 129 ++++----- .../entities/src/EntityTreeHeadlessViewer.cpp | 5 +- libraries/octree/src/JurisdictionMap.h | 5 +- libraries/octree/src/Octree.cpp | 132 +++------ libraries/octree/src/Octree.h | 23 +- .../octree/src/OctreeEditPacketSender.cpp | 36 +-- libraries/octree/src/OctreeHeadlessViewer.cpp | 91 +++--- libraries/octree/src/OctreePersistThread.cpp | 23 +- libraries/octree/src/OctreeRenderer.cpp | 66 ++--- libraries/octree/src/OctreeSceneStats.h | 5 +- libraries/physics/src/EntityMotionState.cpp | 16 +- libraries/physics/src/ObjectAction.h | 11 +- libraries/physics/src/ObjectActionOffset.cpp | 116 ++++---- libraries/physics/src/ObjectActionSpring.cpp | 156 +++++----- .../physics/src/PhysicalEntitySimulation.cpp | 45 +-- .../physics/src/PhysicalEntitySimulation.h | 27 +- libraries/physics/src/PhysicsEngine.cpp | 12 +- libraries/physics/src/PhysicsEngine.h | 12 +- .../shared/src/shared/ReadWriteLockable.cpp | 10 + .../shared/src/shared/ReadWriteLockable.h | 62 ++++ 36 files changed, 1272 insertions(+), 1192 deletions(-) create mode 100644 examples/tests/entityEditStressTest.js create mode 100644 examples/tests/timerStressTest.js create mode 100644 libraries/shared/src/shared/ReadWriteLockable.cpp create mode 100644 libraries/shared/src/shared/ReadWriteLockable.h diff --git a/assignment-client/src/octree/OctreeInboundPacketProcessor.cpp b/assignment-client/src/octree/OctreeInboundPacketProcessor.cpp index a023b9a7fd..3a131e8a1a 100644 --- a/assignment-client/src/octree/OctreeInboundPacketProcessor.cpp +++ b/assignment-client/src/octree/OctreeInboundPacketProcessor.cpp @@ -156,20 +156,20 @@ void OctreeInboundPacketProcessor::processPacket(QSharedPointer packet packet->pos(), maxSize); } - quint64 startLock = usecTimestampNow(); - _myServer->getOctree()->lockForWrite(); - quint64 startProcess = usecTimestampNow(); - int editDataBytesRead = - _myServer->getOctree()->processEditPacketData(*packet, editData, maxSize, sendingNode); + quint64 startProcess, startLock = usecTimestampNow(); + int editDataBytesRead; + _myServer->getOctree()->withWriteLock([&] { + startProcess = usecTimestampNow(); + editDataBytesRead = + _myServer->getOctree()->processEditPacketData(*packet, editData, maxSize, sendingNode); + }); + quint64 endProcess = usecTimestampNow(); if (debugProcessPacket) { qDebug() << "OctreeInboundPacketProcessor::processPacket() after processEditPacketData()..." - << "editDataBytesRead=" << editDataBytesRead; + << "editDataBytesRead=" << editDataBytesRead; } - _myServer->getOctree()->unlock(); - quint64 endProcess = usecTimestampNow(); - editsInPacket++; quint64 thisProcessTime = endProcess - startProcess; quint64 thisLockWaitTime = startProcess - startLock; diff --git a/assignment-client/src/octree/OctreeSendThread.cpp b/assignment-client/src/octree/OctreeSendThread.cpp index 417df3204a..9b528973a1 100644 --- a/assignment-client/src/octree/OctreeSendThread.cpp +++ b/assignment-client/src/octree/OctreeSendThread.cpp @@ -416,83 +416,83 @@ int OctreeSendThread::packetDistributor(OctreeQueryNode* nodeData, bool viewFrus if (!nodeData->elementBag.isEmpty()) { quint64 lockWaitStart = usecTimestampNow(); - _myServer->getOctree()->lockForRead(); - quint64 lockWaitEnd = usecTimestampNow(); - lockWaitElapsedUsec = (float)(lockWaitEnd - lockWaitStart); - quint64 encodeStart = usecTimestampNow(); + _myServer->getOctree()->withReadLock([&]{ + quint64 lockWaitEnd = usecTimestampNow(); + lockWaitElapsedUsec = (float)(lockWaitEnd - lockWaitStart); + quint64 encodeStart = usecTimestampNow(); - OctreeElement* subTree = nodeData->elementBag.extract(); + OctreeElement* subTree = nodeData->elementBag.extract(); - /* TODO: Looking for a way to prevent locking and encoding a tree that is not - // going to result in any packets being sent... - // - // If our node is root, and the root hasn't changed, and our view hasn't changed, - // and we've already seen at least one duplicate packet, then we probably don't need - // to lock the tree and encode, because the result should be that no bytes will be - // encoded, and this will be a duplicate packet from the last one we sent... - OctreeElement* root = _myServer->getOctree()->getRoot(); - bool skipEncode = false; - if ( - (subTree == root) - && (nodeData->getLastRootTimestamp() == root->getLastChanged()) - && !viewFrustumChanged - && (nodeData->getDuplicatePacketCount() > 0) - ) { - qDebug() << "is root, root not changed, view not changed, already seen a duplicate!" - << "Can we skip it?"; - skipEncode = true; - } - */ - - bool wantOcclusionCulling = nodeData->getWantOcclusionCulling(); - CoverageMap* coverageMap = wantOcclusionCulling ? &nodeData->map : IGNORE_COVERAGE_MAP; - - float octreeSizeScale = nodeData->getOctreeSizeScale(); - int boundaryLevelAdjustClient = nodeData->getBoundaryLevelAdjust(); - - int boundaryLevelAdjust = boundaryLevelAdjustClient + (viewFrustumChanged && nodeData->getWantLowResMoving() - ? LOW_RES_MOVING_ADJUST : NO_BOUNDARY_ADJUST); - - EncodeBitstreamParams params(INT_MAX, &nodeData->getCurrentViewFrustum(), wantColor, - WANT_EXISTS_BITS, DONT_CHOP, wantDelta, lastViewFrustum, - wantOcclusionCulling, coverageMap, boundaryLevelAdjust, octreeSizeScale, - nodeData->getLastTimeBagEmpty(), - isFullScene, &nodeData->stats, _myServer->getJurisdiction(), - &nodeData->extraEncodeData); - - // TODO: should this include the lock time or not? This stat is sent down to the client, - // it seems like it may be a good idea to include the lock time as part of the encode time - // are reported to client. Since you can encode without the lock - nodeData->stats.encodeStarted(); - - bytesWritten = _myServer->getOctree()->encodeTreeBitstream(subTree, &_packetData, nodeData->elementBag, params); - - quint64 encodeEnd = usecTimestampNow(); - encodeElapsedUsec = (float)(encodeEnd - encodeStart); - - // If after calling encodeTreeBitstream() there are no nodes left to send, then we know we've - // sent the entire scene. We want to know this below so we'll actually write this content into - // the packet and send it - completedScene = nodeData->elementBag.isEmpty(); - - // if we're trying to fill a full size packet, then we use this logic to determine if we have a DIDNT_FIT case. - if (_packetData.getTargetSize() == MAX_OCTREE_PACKET_DATA_SIZE) { - if (_packetData.hasContent() && bytesWritten == 0 && - params.stopReason == EncodeBitstreamParams::DIDNT_FIT) { - lastNodeDidntFit = true; + /* TODO: Looking for a way to prevent locking and encoding a tree that is not + // going to result in any packets being sent... + // + // If our node is root, and the root hasn't changed, and our view hasn't changed, + // and we've already seen at least one duplicate packet, then we probably don't need + // to lock the tree and encode, because the result should be that no bytes will be + // encoded, and this will be a duplicate packet from the last one we sent... + OctreeElement* root = _myServer->getOctree()->getRoot(); + bool skipEncode = false; + if ( + (subTree == root) + && (nodeData->getLastRootTimestamp() == root->getLastChanged()) + && !viewFrustumChanged + && (nodeData->getDuplicatePacketCount() > 0) + ) { + qDebug() << "is root, root not changed, view not changed, already seen a duplicate!" + << "Can we skip it?"; + skipEncode = true; } - } else { - // in compressed mode and we are trying to pack more... and we don't care if the _packetData has - // content or not... because in this case even if we were unable to pack any data, we want to drop - // below to our sendNow logic, but we do want to track that we attempted to pack extra - extraPackingAttempts++; - if (bytesWritten == 0 && params.stopReason == EncodeBitstreamParams::DIDNT_FIT) { - lastNodeDidntFit = true; - } - } + */ - nodeData->stats.encodeStopped(); - _myServer->getOctree()->unlock(); + bool wantOcclusionCulling = nodeData->getWantOcclusionCulling(); + CoverageMap* coverageMap = wantOcclusionCulling ? &nodeData->map : IGNORE_COVERAGE_MAP; + + float octreeSizeScale = nodeData->getOctreeSizeScale(); + int boundaryLevelAdjustClient = nodeData->getBoundaryLevelAdjust(); + + int boundaryLevelAdjust = boundaryLevelAdjustClient + (viewFrustumChanged && nodeData->getWantLowResMoving() + ? LOW_RES_MOVING_ADJUST : NO_BOUNDARY_ADJUST); + + EncodeBitstreamParams params(INT_MAX, &nodeData->getCurrentViewFrustum(), wantColor, + WANT_EXISTS_BITS, DONT_CHOP, wantDelta, lastViewFrustum, + wantOcclusionCulling, coverageMap, boundaryLevelAdjust, octreeSizeScale, + nodeData->getLastTimeBagEmpty(), + isFullScene, &nodeData->stats, _myServer->getJurisdiction(), + &nodeData->extraEncodeData); + + // TODO: should this include the lock time or not? This stat is sent down to the client, + // it seems like it may be a good idea to include the lock time as part of the encode time + // are reported to client. Since you can encode without the lock + nodeData->stats.encodeStarted(); + + bytesWritten = _myServer->getOctree()->encodeTreeBitstream(subTree, &_packetData, nodeData->elementBag, params); + + quint64 encodeEnd = usecTimestampNow(); + encodeElapsedUsec = (float)(encodeEnd - encodeStart); + + // If after calling encodeTreeBitstream() there are no nodes left to send, then we know we've + // sent the entire scene. We want to know this below so we'll actually write this content into + // the packet and send it + completedScene = nodeData->elementBag.isEmpty(); + + // if we're trying to fill a full size packet, then we use this logic to determine if we have a DIDNT_FIT case. + if (_packetData.getTargetSize() == MAX_OCTREE_PACKET_DATA_SIZE) { + if (_packetData.hasContent() && bytesWritten == 0 && + params.stopReason == EncodeBitstreamParams::DIDNT_FIT) { + lastNodeDidntFit = true; + } + } else { + // in compressed mode and we are trying to pack more... and we don't care if the _packetData has + // content or not... because in this case even if we were unable to pack any data, we want to drop + // below to our sendNow logic, but we do want to track that we attempted to pack extra + extraPackingAttempts++; + if (bytesWritten == 0 && params.stopReason == EncodeBitstreamParams::DIDNT_FIT) { + lastNodeDidntFit = true; + } + } + + nodeData->stats.encodeStopped(); + }); } else { // If the bag was empty then we didn't even attempt to encode, and so we know the bytesWritten were 0 bytesWritten = 0; diff --git a/examples/tests/entityEditStressTest.js b/examples/tests/entityEditStressTest.js new file mode 100644 index 0000000000..2d3c8ad0e1 --- /dev/null +++ b/examples/tests/entityEditStressTest.js @@ -0,0 +1,175 @@ +// entityEditStressTest.js +// +// Created by Seiji Emery on 8/31/15 +// Copyright 2015 High Fidelity, Inc. +// +// Stress tests the client + server-side entity trees by spawning huge numbers of entities in +// close proximity to your avatar and updating them continuously (ie. applying position edits), +// with the intent of discovering crashes and other bugs related to the entity, scripting, +// rendering, networking, and/or physics subsystems. +// +// This script was originally created to find + diagnose an a clientside crash caused by improper +// locking of the entity tree, but can be reused for other purposes. +// +// Distributed under the Apache License, Version 2.0. +// See the accompanying file LICENSE or http://www.apache.org/licenses/LICENSE-2.0.html +// + +var NUM_ENTITIES = 20000; // number of entities to spawn +var ENTITY_SPAWN_LIMIT = 1000; +var ENTITY_SPAWN_INTERVAL = 0.1; + +var UPDATE_INTERVAL = 0.05; // Re-randomize the entity's position every x seconds / ms +var ENTITY_LIFETIME = 30; // Entity timeout (when/if we crash, we need the entities to delete themselves) +var KEEPALIVE_INTERVAL = 5; // Refreshes the timeout every X seconds + +var RADIUS = 5.0; // Spawn within this radius (square) +var Y_OFFSET = 1.5; // Spawn at an offset below the avatar +var TEST_ENTITY_NAME = "EntitySpawnTest"; + +(function () { + this.makeEntity = function (properties) { + var entity = Entities.addEntity(properties); + // print("spawning entity: " + JSON.stringify(properties)); + + return { + update: function (properties) { + Entities.editEntity(entity, properties); + }, + destroy: function () { + Entities.deleteEntity(entity) + }, + getAge: function () { + return Entities.getEntityProperties(entity).age; + } + }; + } + + this.randomPositionXZ = function (center, radius) { + return { + x: center.x + (Math.random() * radius * 2.0) - radius, + y: center.y, + z: center.z + (Math.random() * radius * 2.0) - radius + }; + } + this.randomColor = function () { + var shade = Math.floor(Math.random() * 255); + var hue = Math.floor(Math.random() * (255 - shade)); + + return { + red: shade + hue, + green: shade, + blue: shade + }; + } + this.randomDimensions = function () { + return { + x: 0.1 + Math.random() * 0.5, + y: 0.1 + Math.random() * 0.1, + z: 0.1 + Math.random() * 0.5 + }; + } +})(); + +(function () { + var entities = []; + var entitiesToCreate = 0; + var entitiesSpawned = 0; + + + function clear () { + var ids = Entities.findEntities(MyAvatar.position, 50); + var that = this; + ids.forEach(function(id) { + var properties = Entities.getEntityProperties(id); + if (properties.name == TEST_ENTITY_NAME) { + Entities.deleteEntity(id); + } + }, this); + } + + function createEntities () { + print("Creating " + NUM_ENTITIES + " entities (UPDATE_INTERVAL = " + UPDATE_INTERVAL + ", KEEPALIVE_INTERVAL = " + KEEPALIVE_INTERVAL + ")"); + entitiesToCreate = NUM_ENTITIES; + Script.update.connect(spawnEntities); + } + + var spawnTimer = 0.0; + function spawnEntities (dt) { + if (entitiesToCreate <= 0) { + Script.update.disconnect(spawnEntities); + print("Finished spawning entities"); + } + else if ((spawnTimer -= dt) < 0.0){ + spawnTimer = ENTITY_SPAWN_INTERVAL; + + var n = Math.min(entitiesToCreate, ENTITY_SPAWN_LIMIT); + print("Spawning " + n + " entities (" + (entitiesSpawned += n) + ")"); + + entitiesToCreate -= n; + + var center = MyAvatar.position; + center.y -= Y_OFFSET; + + for (; n > 0; --n) { + entities.push(makeEntity({ + type: "Box", + name: TEST_ENTITY_NAME, + position: randomPositionXZ(center, RADIUS), + color: randomColor(), + dimensions: randomDimensions(), + lifetime: ENTITY_LIFETIME + })); + } + } + } + + function despawnEntities () { + print("despawning entities"); + entities.forEach(function (entity) { + entity.destroy(); + }); + entities = []; + } + + var keepAliveTimer = 0.0; + var updateTimer = 0.0; + + // Runs the following entity updates: + // a) refreshes the timeout interval every KEEPALIVE_INTERVAL seconds, and + // b) re-randomizes its position every UPDATE_INTERVAL seconds. + // This should be sufficient to crash the client until the entity tree bug is fixed (and thereafter if it shows up again). + function updateEntities (dt) { + var updateLifetime = ((keepAliveTimer -= dt) < 0.0) ? ((keepAliveTimer = KEEPALIVE_INTERVAL), true) : false; + var updateProperties = ((updateTimer -= dt) < 0.0) ? ((updateTimer = UPDATE_INTERVAL), true) : false; + + if (updateLifetime || updateProperties) { + var center = MyAvatar.position; + center.y -= Y_OFFSET; + + entities.forEach((updateLifetime && updateProperties && function (entity) { + entity.update({ + lifetime: entity.getAge() + ENTITY_LIFETIME, + position: randomPositionXZ(center, RADIUS) + }); + }) || (updateLifetime && function (entity) { + entity.update({ + lifetime: entity.getAge() + ENTITY_LIFETIME + }); + }) || (updateProperties && function (entity) { + entity.update({ + position: randomPositionXZ(center, RADIUS) + }); + }) || null, this); + } + } + + function init () { + Script.update.disconnect(init); + clear(); + createEntities(); + Script.update.connect(updateEntities); + Script.scriptEnding.connect(despawnEntities); + } + Script.update.connect(init); +})(); \ No newline at end of file diff --git a/examples/tests/timerStressTest.js b/examples/tests/timerStressTest.js new file mode 100644 index 0000000000..e9e1732f5c --- /dev/null +++ b/examples/tests/timerStressTest.js @@ -0,0 +1,68 @@ +// createBoxes.js +// part of bubblewand +// +// Created by James B. Pollack @imgntn -- 09/07/2015 +// Copyright 2015 High Fidelity, Inc. +// +// Loads a wand model and attaches the bubble wand behavior. +// Distributed under the Apache License, Version 2.0. +// See the accompanying file LICENSE or http://www.apache.org/licenses/LICENSE-2.0.html + + + +Script.include("https://raw.githubusercontent.com/highfidelity/hifi/master/examples/utilities.js"); +Script.include("https://raw.githubusercontent.com/highfidelity/hifi/master/examples/libraries/utils.js"); + +var bubbleModel = 'http://hifi-public.s3.amazonaws.com/james/bubblewand/models/bubble/bubble.fbx?' + randInt(0, 10000);; +//var scriptURL'http://hifi-public.s3.amazonaws.com/james/bubblewand/scripts/wand.js?'+randInt(0,10000); + +//for local testing +//var scriptURL = "http://localhost:8080/scripts/setRecurringTimeout.js?" + randInt(0, 10000); + + +var scriptURL='http://hifi-public.s3.amazonaws.com/james/debug/timeouts/setRecurringTimeout.js?'+ randInt(0, 10000); +//create the wand in front of the avatar + +var boxes=[]; +var TEST_ENTITY_NAME = "TimerScript"; + + +var TOTAL_ENTITIES = 100; +for (var i = 0; i < TOTAL_ENTITIES; i++) { + var box = Entities.addEntity({ + type: "Box", + name: "" + position: { + x: randInt(0, 100) - 50 + MyAvatar.position.x, + y: randInt(0, 100) - 50 + MyAvatar.position.x, + z: randInt(0, 100) - 50 + MyAvatar.position.x, + }, + dimensions: { + x: 1, + y: 1, + z: 1, + }, + color: { + red: 255, + green: 0, + blue: 0, + }, + //must be enabled to be grabbable in the physics engine + collisionsWillMove: true, + shapeType: 'box', + lifetime:60, + script: scriptURL + }); + boxes.push(box) +} + + +function cleanup() { + while (boxes.length > 0) { + Entities.deleteEntity(boxes.pop()); + + } +} + + +Script.scriptEnding.connect(cleanup); \ No newline at end of file diff --git a/interface/src/Application.cpp b/interface/src/Application.cpp index 07c1b4e38d..e39cfdd133 100644 --- a/interface/src/Application.cpp +++ b/interface/src/Application.cpp @@ -862,9 +862,7 @@ void Application::emptyLocalCache() { Application::~Application() { EntityTree* tree = _entities.getTree(); - tree->lockForWrite(); - _entities.getTree()->setSimulation(NULL); - tree->unlock(); + tree->setSimulation(NULL); _octreeProcessor.terminate(); _entityEditSender.terminate(); @@ -886,7 +884,9 @@ Application::~Application() { // remove avatars from physics engine DependencyManager::get()->clearOtherAvatars(); - _physicsEngine.deleteObjects(DependencyManager::get()->getObjectsToDelete()); + VectorOfMotionStates motionStates; + DependencyManager::get()->getObjectsToDelete(motionStates); + _physicsEngine.deleteObjects(motionStates); DependencyManager::destroy(); DependencyManager::destroy(); @@ -2861,46 +2861,39 @@ void Application::update(float deltaTime) { PerformanceTimer perfTimer("physics"); _myAvatar->relayDriveKeysToCharacterController(); - _entitySimulation.lock(); - _physicsEngine.deleteObjects(_entitySimulation.getObjectsToDelete()); - _entitySimulation.unlock(); - _entities.getTree()->lockForWrite(); - _entitySimulation.lock(); - _physicsEngine.addObjects(_entitySimulation.getObjectsToAdd()); - _entitySimulation.unlock(); - _entities.getTree()->unlock(); + static VectorOfMotionStates motionStates; + _entitySimulation.getObjectsToDelete(motionStates); + _physicsEngine.deleteObjects(motionStates); - _entities.getTree()->lockForWrite(); - _entitySimulation.lock(); - VectorOfMotionStates stillNeedChange = _physicsEngine.changeObjects(_entitySimulation.getObjectsToChange()); - _entitySimulation.setObjectsToChange(stillNeedChange); - _entitySimulation.unlock(); - _entities.getTree()->unlock(); + _entities.getTree()->withWriteLock([&] { + _entitySimulation.getObjectsToAdd(motionStates); + _physicsEngine.addObjects(motionStates); + + _entitySimulation.getObjectsToChange(motionStates); + VectorOfMotionStates stillNeedChange = _physicsEngine.changeObjects(motionStates); + _entitySimulation.setObjectsToChange(stillNeedChange); + }); - _entitySimulation.lock(); _entitySimulation.applyActionChanges(); - _entitySimulation.unlock(); AvatarManager* avatarManager = DependencyManager::get().data(); - _physicsEngine.deleteObjects(avatarManager->getObjectsToDelete()); - _physicsEngine.addObjects(avatarManager->getObjectsToAdd()); - _physicsEngine.changeObjects(avatarManager->getObjectsToChange()); - - _entities.getTree()->lockForWrite(); - _physicsEngine.stepSimulation(); - _entities.getTree()->unlock(); + avatarManager->getObjectsToDelete(motionStates); + _physicsEngine.deleteObjects(motionStates); + avatarManager->getObjectsToAdd(motionStates); + _physicsEngine.addObjects(motionStates); + avatarManager->getObjectsToChange(motionStates); + _physicsEngine.changeObjects(motionStates); + _entities.getTree()->withWriteLock([&] { + _physicsEngine.stepSimulation(); + }); + if (_physicsEngine.hasOutgoingChanges()) { - _entities.getTree()->lockForWrite(); - _entitySimulation.lock(); - _entitySimulation.handleOutgoingChanges(_physicsEngine.getOutgoingChanges(), _physicsEngine.getSessionID()); - _entitySimulation.unlock(); - _entities.getTree()->unlock(); - - _entities.getTree()->lockForWrite(); - avatarManager->handleOutgoingChanges(_physicsEngine.getOutgoingChanges()); - _entities.getTree()->unlock(); + _entities.getTree()->withWriteLock([&] { + _entitySimulation.handleOutgoingChanges(_physicsEngine.getOutgoingChanges(), _physicsEngine.getSessionID()); + avatarManager->handleOutgoingChanges(_physicsEngine.getOutgoingChanges()); + }); auto collisionEvents = _physicsEngine.getCollisionEvents(); avatarManager->handleCollisionEvents(collisionEvents); @@ -3019,27 +3012,21 @@ int Application::sendNackPackets() { return; } - _octreeSceneStatsLock.lockForRead(); + QSet missingSequenceNumbers; + _octreeServerSceneStats.withReadLock([&] { + // retreive octree scene stats of this node + if (_octreeServerSceneStats.find(nodeUUID) == _octreeServerSceneStats.end()) { + return; + } + // get sequence number stats of node, prune its missing set, and make a copy of the missing set + SequenceNumberStats& sequenceNumberStats = _octreeServerSceneStats[nodeUUID].getIncomingOctreeSequenceNumberStats(); + sequenceNumberStats.pruneMissingSet(); + missingSequenceNumbers = sequenceNumberStats.getMissingSet(); + }); - // retreive octree scene stats of this node - if (_octreeServerSceneStats.find(nodeUUID) == _octreeServerSceneStats.end()) { - _octreeSceneStatsLock.unlock(); - return; - } - - // get sequence number stats of node, prune its missing set, and make a copy of the missing set - SequenceNumberStats& sequenceNumberStats = _octreeServerSceneStats[nodeUUID].getIncomingOctreeSequenceNumberStats(); - sequenceNumberStats.pruneMissingSet(); - const QSet missingSequenceNumbers = sequenceNumberStats.getMissingSet(); - - _octreeSceneStatsLock.unlock(); - // construct nack packet(s) for this node - auto it = missingSequenceNumbers.constBegin(); - while (it != missingSequenceNumbers.constEnd()) { - OCTREE_PACKET_SEQUENCE missingNumber = *it; + foreach(const OCTREE_PACKET_SEQUENCE& missingNumber, missingSequenceNumbers) { nackPacketList.writePrimitive(missingNumber); - ++it; } if (nackPacketList.getNumPackets()) { @@ -3772,13 +3759,13 @@ void Application::clearDomainOctreeDetails() { // reset the environment so that we don't erroneously end up with multiple // reset our node to stats and node to jurisdiction maps... since these must be changing... - _entityServerJurisdictions.lockForWrite(); - _entityServerJurisdictions.clear(); - _entityServerJurisdictions.unlock(); + _entityServerJurisdictions.withWriteLock([&] { + _entityServerJurisdictions.clear(); + }); - _octreeSceneStatsLock.lockForWrite(); - _octreeServerSceneStats.clear(); - _octreeSceneStatsLock.unlock(); + _octreeServerSceneStats.withWriteLock([&] { + _octreeServerSceneStats.clear(); + }); // reset the model renderer _entities.clear(); @@ -3848,29 +3835,30 @@ void Application::nodeKilled(SharedNodePointer node) { QUuid nodeUUID = node->getUUID(); // see if this is the first we've heard of this node... - _entityServerJurisdictions.lockForRead(); - if (_entityServerJurisdictions.find(nodeUUID) != _entityServerJurisdictions.end()) { + _entityServerJurisdictions.withReadLock([&] { + if (_entityServerJurisdictions.find(nodeUUID) == _entityServerJurisdictions.end()) { + return; + } + unsigned char* rootCode = _entityServerJurisdictions[nodeUUID].getRootOctalCode(); VoxelPositionSize rootDetails; voxelDetailsForCode(rootCode, rootDetails); - _entityServerJurisdictions.unlock(); qCDebug(interfaceapp, "model server going away...... v[%f, %f, %f, %f]", - (double)rootDetails.x, (double)rootDetails.y, (double)rootDetails.z, (double)rootDetails.s); + (double)rootDetails.x, (double)rootDetails.y, (double)rootDetails.z, (double)rootDetails.s); // If the model server is going away, remove it from our jurisdiction map so we don't send voxels to a dead server - _entityServerJurisdictions.lockForWrite(); - _entityServerJurisdictions.erase(_entityServerJurisdictions.find(nodeUUID)); - } - _entityServerJurisdictions.unlock(); + _entityServerJurisdictions.withWriteLock([&] { + _entityServerJurisdictions.erase(_entityServerJurisdictions.find(nodeUUID)); + }); + }); // also clean up scene stats for that server - _octreeSceneStatsLock.lockForWrite(); - if (_octreeServerSceneStats.find(nodeUUID) != _octreeServerSceneStats.end()) { - _octreeServerSceneStats.erase(nodeUUID); - } - _octreeSceneStatsLock.unlock(); - + _octreeServerSceneStats.withWriteLock([&] { + if (_octreeServerSceneStats.find(nodeUUID) != _octreeServerSceneStats.end()) { + _octreeServerSceneStats.erase(nodeUUID); + } + }); } else if (node->getType() == NodeType::AvatarMixer) { // our avatar mixer has gone away - clear the hash of avatars DependencyManager::get()->clearOtherAvatars(); @@ -3888,12 +3876,12 @@ void Application::trackIncomingOctreePacket(NLPacket& packet, SharedNodePointer const QUuid& nodeUUID = sendingNode->getUUID(); // now that we know the node ID, let's add these stats to the stats for that node... - _octreeSceneStatsLock.lockForWrite(); - if (_octreeServerSceneStats.find(nodeUUID) != _octreeServerSceneStats.end()) { - OctreeSceneStats& stats = _octreeServerSceneStats[nodeUUID]; - stats.trackIncomingOctreePacket(packet, wasStatsPacket, sendingNode->getClockSkewUsec()); - } - _octreeSceneStatsLock.unlock(); + _octreeServerSceneStats.withWriteLock([&] { + if (_octreeServerSceneStats.find(nodeUUID) != _octreeServerSceneStats.end()) { + OctreeSceneStats& stats = _octreeServerSceneStats[nodeUUID]; + stats.trackIncomingOctreePacket(packet, wasStatsPacket, sendingNode->getClockSkewUsec()); + } + }); } } @@ -3907,43 +3895,42 @@ int Application::processOctreeStats(NLPacket& packet, SharedNodePointer sendingN const QUuid& nodeUUID = sendingNode->getUUID(); // now that we know the node ID, let's add these stats to the stats for that node... - _octreeSceneStatsLock.lockForWrite(); - - OctreeSceneStats& octreeStats = _octreeServerSceneStats[nodeUUID]; - statsMessageLength = octreeStats.unpackFromPacket(packet); - - _octreeSceneStatsLock.unlock(); + _octreeServerSceneStats.withWriteLock([&] { + OctreeSceneStats& octreeStats = _octreeServerSceneStats[nodeUUID]; + statsMessageLength = octreeStats.unpackFromPacket(packet); - // see if this is the first we've heard of this node... - NodeToJurisdictionMap* jurisdiction = NULL; - QString serverType; - if (sendingNode->getType() == NodeType::EntityServer) { - jurisdiction = &_entityServerJurisdictions; - serverType = "Entity"; - } + // see if this is the first we've heard of this node... + NodeToJurisdictionMap* jurisdiction = NULL; + QString serverType; + if (sendingNode->getType() == NodeType::EntityServer) { + jurisdiction = &_entityServerJurisdictions; + serverType = "Entity"; + } - jurisdiction->lockForRead(); - if (jurisdiction->find(nodeUUID) == jurisdiction->end()) { - jurisdiction->unlock(); - - VoxelPositionSize rootDetails; - voxelDetailsForCode(octreeStats.getJurisdictionRoot(), rootDetails); + jurisdiction->withReadLock([&] { + if (jurisdiction->find(nodeUUID) != jurisdiction->end()) { + return; + } - qCDebug(interfaceapp, "stats from new %s server... [%f, %f, %f, %f]", + VoxelPositionSize rootDetails; + voxelDetailsForCode(octreeStats.getJurisdictionRoot(), rootDetails); + + qCDebug(interfaceapp, "stats from new %s server... [%f, %f, %f, %f]", qPrintable(serverType), (double)rootDetails.x, (double)rootDetails.y, (double)rootDetails.z, (double)rootDetails.s); - } else { - jurisdiction->unlock(); - } - // store jurisdiction details for later use - // This is bit of fiddling is because JurisdictionMap assumes it is the owner of the values used to construct it - // but OctreeSceneStats thinks it's just returning a reference to its contents. So we need to make a copy of the - // details from the OctreeSceneStats to construct the JurisdictionMap - JurisdictionMap jurisdictionMap; - jurisdictionMap.copyContents(octreeStats.getJurisdictionRoot(), octreeStats.getJurisdictionEndNodes()); - jurisdiction->lockForWrite(); - (*jurisdiction)[nodeUUID] = jurisdictionMap; - jurisdiction->unlock(); + }); + // store jurisdiction details for later use + // This is bit of fiddling is because JurisdictionMap assumes it is the owner of the values used to construct it + // but OctreeSceneStats thinks it's just returning a reference to its contents. So we need to make a copy of the + // details from the OctreeSceneStats to construct the JurisdictionMap + JurisdictionMap jurisdictionMap; + jurisdictionMap.copyContents(octreeStats.getJurisdictionRoot(), octreeStats.getJurisdictionEndNodes()); + jurisdiction->withWriteLock([&] { + (*jurisdiction)[nodeUUID] = jurisdictionMap; + }); + }); + + return statsMessageLength; } diff --git a/interface/src/Application.h b/interface/src/Application.h index 6056323aa9..bc540f947f 100644 --- a/interface/src/Application.h +++ b/interface/src/Application.h @@ -253,8 +253,6 @@ public: bool importSVOFromURL(const QString& urlString); NodeToOctreeSceneStats* getOcteeSceneStats() { return &_octreeServerSceneStats; } - void lockOctreeSceneStats() { _octreeSceneStatsLock.lockForRead(); } - void unlockOctreeSceneStats() { _octreeSceneStatsLock.unlock(); } ToolWindow* getToolWindow() { return _toolWindow ; } @@ -612,7 +610,6 @@ private: NodeToJurisdictionMap _entityServerJurisdictions; NodeToOctreeSceneStats _octreeServerSceneStats; - QReadWriteLock _octreeSceneStatsLock; ControllerScriptingInterface _controllerScriptingInterface; QPointer _logDialog; diff --git a/interface/src/avatar/AvatarActionHold.cpp b/interface/src/avatar/AvatarActionHold.cpp index 0c5145b596..df0cd5aaf0 100644 --- a/interface/src/avatar/AvatarActionHold.cpp +++ b/interface/src/avatar/AvatarActionHold.cpp @@ -44,43 +44,42 @@ void AvatarActionHold::updateActionWorker(float deltaTimeStep) { return; } - auto myAvatar = DependencyManager::get()->getMyAvatar(); - - if (!tryLockForRead()) { - // don't risk hanging the thread running the physics simulation - return; - } - - glm::vec3 palmPosition; - glm::quat palmRotation; - if (_hand == "right") { - palmPosition = myAvatar->getRightPalmPosition(); - palmRotation = myAvatar->getRightPalmRotation(); - } else { - palmPosition = myAvatar->getLeftPalmPosition(); - palmRotation = myAvatar->getLeftPalmRotation(); - } - - auto rotation = palmRotation * _relativeRotation; - auto offset = rotation * _relativePosition; - auto position = palmPosition + offset; - unlock(); - - if (!tryLockForWrite()) { - return; - } - - if (_positionalTarget != position || _rotationalTarget != rotation) { - auto ownerEntity = _ownerEntity.lock(); - if (ownerEntity) { - ownerEntity->setActionDataDirty(true); + glm::quat rotation; + glm::vec3 position; + glm::vec3 offset; + bool gotLock = withTryReadLock([&]{ + auto myAvatar = DependencyManager::get()->getMyAvatar(); + glm::vec3 palmPosition; + glm::quat palmRotation; + if (_hand == "right") { + palmPosition = myAvatar->getRightPalmPosition(); + palmRotation = myAvatar->getRightPalmRotation(); + } else { + palmPosition = myAvatar->getLeftPalmPosition(); + palmRotation = myAvatar->getLeftPalmRotation(); } - _positionalTarget = position; - _rotationalTarget = rotation; - } - unlock(); - ObjectActionSpring::updateActionWorker(deltaTimeStep); + rotation = palmRotation * _relativeRotation; + offset = rotation * _relativePosition; + position = palmPosition + offset; + }); + + if (gotLock) { + result = withTryWriteLock([&]{ + if (_positionalTarget != position || _rotationalTarget != rotation) { + auto ownerEntity = _ownerEntity.lock(); + if (ownerEntity) { + ownerEntity->setActionDataDirty(true); + } + _positionalTarget = position; + _rotationalTarget = rotation; + } + }); + } + + if (result) { + ObjectActionSpring::updateActionWorker(deltaTimeStep); + } } @@ -117,18 +116,18 @@ bool AvatarActionHold::updateArguments(QVariantMap arguments) { || relativeRotation != _relativeRotation || timeScale != _linearTimeScale || hand != _hand) { - lockForWrite(); - _relativePosition = relativePosition; - _relativeRotation = relativeRotation; - const float MIN_TIMESCALE = 0.1f; - _linearTimeScale = glm::min(MIN_TIMESCALE, timeScale); - _angularTimeScale = _linearTimeScale; - _hand = hand; + withWriteLock([&] { + _relativePosition = relativePosition; + _relativeRotation = relativeRotation; + const float MIN_TIMESCALE = 0.1f; + _linearTimeScale = glm::min(MIN_TIMESCALE, timeScale); + _angularTimeScale = _linearTimeScale; + _hand = hand; - _mine = true; - _active = true; - activateBody(); - unlock(); + _mine = true; + _active = true; + activateBody(); + }); } return true; } @@ -136,17 +135,17 @@ bool AvatarActionHold::updateArguments(QVariantMap arguments) { QVariantMap AvatarActionHold::getArguments() { QVariantMap arguments; - lockForRead(); - if (_mine) { + withReadLock([&]{ + if (!_mine) { + arguments = ObjectActionSpring::getArguments(); + return; + } + arguments["relativePosition"] = glmToQMap(_relativePosition); arguments["relativeRotation"] = glmToQMap(_relativeRotation); arguments["timeScale"] = _linearTimeScale; arguments["hand"] = _hand; - } else { - unlock(); - return ObjectActionSpring::getArguments(); - } - unlock(); + }); return arguments; } diff --git a/interface/src/avatar/AvatarManager.cpp b/interface/src/avatar/AvatarManager.cpp index 67668a549d..1790682e56 100644 --- a/interface/src/avatar/AvatarManager.cpp +++ b/interface/src/avatar/AvatarManager.cpp @@ -242,37 +242,33 @@ QVector AvatarManager::getLocalLights() const { return _localLights; } -VectorOfMotionStates& AvatarManager::getObjectsToDelete() { - _tempMotionStates.clear(); - _tempMotionStates.swap(_motionStatesToDelete); - return _tempMotionStates; +void AvatarManager::getObjectsToDelete(VectorOfMotionStates& result) { + result.clear(); + result.swap(_motionStatesToDelete); } -VectorOfMotionStates& AvatarManager::getObjectsToAdd() { - _tempMotionStates.clear(); - +void AvatarManager::getObjectsToAdd(VectorOfMotionStates& result) { + result.clear(); for (auto motionState : _motionStatesToAdd) { - _tempMotionStates.push_back(motionState); + result.push_back(motionState); } _motionStatesToAdd.clear(); - return _tempMotionStates; } -VectorOfMotionStates& AvatarManager::getObjectsToChange() { - _tempMotionStates.clear(); +void AvatarManager::getObjectsToChange(VectorOfMotionStates& result) { + result.clear(); for (auto state : _avatarMotionStates) { if (state->_dirtyFlags > 0) { - _tempMotionStates.push_back(state); + result.push_back(state); } } - return _tempMotionStates; } -void AvatarManager::handleOutgoingChanges(VectorOfMotionStates& motionStates) { +void AvatarManager::handleOutgoingChanges(const VectorOfMotionStates& motionStates) { // TODO: extract the MyAvatar results once we use a MotionState for it. } -void AvatarManager::handleCollisionEvents(CollisionEvents& collisionEvents) { +void AvatarManager::handleCollisionEvents(const CollisionEvents& collisionEvents) { for (Collision collision : collisionEvents) { // TODO: Current physics uses null idA or idB for non-entities. The plan is to handle MOTIONSTATE_TYPE_AVATAR, // and then MOTIONSTATE_TYPE_MYAVATAR. As it is, this code only covers the case of my avatar (in which case one diff --git a/interface/src/avatar/AvatarManager.h b/interface/src/avatar/AvatarManager.h index 853bd4799a..277e931419 100644 --- a/interface/src/avatar/AvatarManager.h +++ b/interface/src/avatar/AvatarManager.h @@ -52,11 +52,11 @@ public: Q_INVOKABLE void setLocalLights(const QVector& localLights); Q_INVOKABLE QVector getLocalLights() const; - VectorOfMotionStates& getObjectsToDelete(); - VectorOfMotionStates& getObjectsToAdd(); - VectorOfMotionStates& getObjectsToChange(); - void handleOutgoingChanges(VectorOfMotionStates& motionStates); - void handleCollisionEvents(CollisionEvents& collisionEvents); + void getObjectsToDelete(VectorOfMotionStates& motionStates); + void getObjectsToAdd(VectorOfMotionStates& motionStates); + void getObjectsToChange(VectorOfMotionStates& motionStates); + void handleOutgoingChanges(const VectorOfMotionStates& motionStates); + void handleCollisionEvents(const CollisionEvents& collisionEvents); void updateAvatarPhysicsShape(const QUuid& id); @@ -87,7 +87,6 @@ private: SetOfAvatarMotionStates _avatarMotionStates; SetOfMotionStates _motionStatesToAdd; VectorOfMotionStates _motionStatesToDelete; - VectorOfMotionStates _tempMotionStates; }; Q_DECLARE_METATYPE(AvatarManager::LocalLight) diff --git a/interface/src/ui/OctreeStatsDialog.cpp b/interface/src/ui/OctreeStatsDialog.cpp index 7e6111bd83..c2d72de9d6 100644 --- a/interface/src/ui/OctreeStatsDialog.cpp +++ b/interface/src/ui/OctreeStatsDialog.cpp @@ -196,30 +196,30 @@ void OctreeStatsDialog::paintEvent(QPaintEvent* event) { unsigned long totalInternal = 0; unsigned long totalLeaves = 0; - Application::getInstance()->lockOctreeSceneStats(); NodeToOctreeSceneStats* sceneStats = Application::getInstance()->getOcteeSceneStats(); - for(NodeToOctreeSceneStatsIterator i = sceneStats->begin(); i != sceneStats->end(); i++) { - //const QUuid& uuid = i->first; - OctreeSceneStats& stats = i->second; - serverCount++; + sceneStats->withReadLock([&] { + for (NodeToOctreeSceneStatsIterator i = sceneStats->begin(); i != sceneStats->end(); i++) { + //const QUuid& uuid = i->first; + OctreeSceneStats& stats = i->second; + serverCount++; - // calculate server node totals - totalNodes += stats.getTotalElements(); - totalInternal += stats.getTotalInternal(); - totalLeaves += stats.getTotalLeaves(); - - // Sending mode - if (serverCount > 1) { - sendingMode << ","; + // calculate server node totals + totalNodes += stats.getTotalElements(); + totalInternal += stats.getTotalInternal(); + totalLeaves += stats.getTotalLeaves(); + + // Sending mode + if (serverCount > 1) { + sendingMode << ","; + } + if (stats.isMoving()) { + sendingMode << "M"; + movingServerCount++; + } else { + sendingMode << "S"; + } } - if (stats.isMoving()) { - sendingMode << "M"; - movingServerCount++; - } else { - sendingMode << "S"; - } - } - Application::getInstance()->unlockOctreeSceneStats(); + }); sendingMode << " - " << serverCount << " servers"; if (movingServerCount > 0) { sendingMode << " "; @@ -398,45 +398,44 @@ void OctreeStatsDialog::showOctreeServersOfType(int& serverCount, NodeType_t ser // lookup our nodeUUID in the jurisdiction map, if it's missing then we're // missing at least one jurisdiction - serverJurisdictions.lockForRead(); - if (serverJurisdictions.find(nodeUUID) == serverJurisdictions.end()) { - serverDetails << " unknown jurisdiction "; - serverJurisdictions.unlock(); - } else { + serverJurisdictions.withReadLock([&] { + if (serverJurisdictions.find(nodeUUID) == serverJurisdictions.end()) { + serverDetails << " unknown jurisdiction "; + return; + } const JurisdictionMap& map = serverJurisdictions[nodeUUID]; - + unsigned char* rootCode = map.getRootOctalCode(); - + if (rootCode) { QString rootCodeHex = octalCodeToHexString(rootCode); - + VoxelPositionSize rootDetails; voxelDetailsForCode(rootCode, rootDetails); AACube serverBounds(glm::vec3(rootDetails.x, rootDetails.y, rootDetails.z), rootDetails.s); serverDetails << " jurisdiction: " - << qPrintable(rootCodeHex) - << " [" - << rootDetails.x << ", " - << rootDetails.y << ", " - << rootDetails.z << ": " - << rootDetails.s << "] "; + << qPrintable(rootCodeHex) + << " [" + << rootDetails.x << ", " + << rootDetails.y << ", " + << rootDetails.z << ": " + << rootDetails.s << "] "; } else { serverDetails << " jurisdiction has no rootCode"; } // root code - serverJurisdictions.unlock(); - } // jurisdiction + }); // now lookup stats details for this server... if (_extraServerDetails[serverCount-1] != LESS) { - Application::getInstance()->lockOctreeSceneStats(); NodeToOctreeSceneStats* sceneStats = Application::getInstance()->getOcteeSceneStats(); - if (sceneStats->find(nodeUUID) != sceneStats->end()) { - OctreeSceneStats& stats = sceneStats->at(nodeUUID); - - switch (_extraServerDetails[serverCount-1]) { + sceneStats->withReadLock([&] { + if (sceneStats->find(nodeUUID) != sceneStats->end()) { + OctreeSceneStats& stats = sceneStats->at(nodeUUID); + + switch (_extraServerDetails[serverCount - 1]) { case MOST: { - extraDetails << "
" ; - + extraDetails << "
"; + float lastFullEncode = stats.getLastFullTotalEncodeTime() / USECS_PER_MSEC; float lastFullSend = stats.getLastFullElapsedTime() / USECS_PER_MSEC; float lastFullSendInSeconds = stats.getLastFullElapsedTime() / USECS_PER_SECOND; @@ -445,20 +444,20 @@ void OctreeStatsDialog::showOctreeServersOfType(int& serverCount, NodeType_t ser if (lastFullSendInSeconds > 0) { lastFullPPS = lastFullPackets / lastFullSendInSeconds; } - + QString lastFullEncodeString = locale.toString(lastFullEncode); QString lastFullSendString = locale.toString(lastFullSend); QString lastFullPacketsString = locale.toString(lastFullPackets); QString lastFullBytesString = locale.toString((uint)stats.getLastFullTotalBytes()); QString lastFullPPSString = locale.toString(lastFullPPS); - + extraDetails << "
" << "Last Full Scene... " << "Encode: " << qPrintable(lastFullEncodeString) << " ms " << "Send: " << qPrintable(lastFullSendString) << " ms " << - "Packets: " << qPrintable(lastFullPacketsString) << " " << + "Packets: " << qPrintable(lastFullPacketsString) << " " << "Bytes: " << qPrintable(lastFullBytesString) << " " << - "Rate: " << qPrintable(lastFullPPSString) << " PPS"; - + "Rate: " << qPrintable(lastFullPPSString) << " PPS"; + for (int i = 0; i < OctreeSceneStats::ITEM_COUNT; i++) { OctreeSceneStats::Item item = (OctreeSceneStats::Item)(i); OctreeSceneStats::ItemInfo& itemInfo = stats.getItemInfo(item); @@ -469,14 +468,14 @@ void OctreeStatsDialog::showOctreeServersOfType(int& serverCount, NodeType_t ser QString totalString = locale.toString((uint)stats.getTotalElements()); QString internalString = locale.toString((uint)stats.getTotalInternal()); QString leavesString = locale.toString((uint)stats.getTotalLeaves()); - + serverDetails << "
" << "Node UUID: " << qPrintable(nodeUUID.toString()) << " "; - + serverDetails << "
" << "Elements: " << qPrintable(totalString) << " total " << qPrintable(internalString) << " internal " << qPrintable(leavesString) << " leaves "; - + QString incomingPacketsString = locale.toString((uint)stats.getIncomingPackets()); QString incomingBytesString = locale.toString((uint)stats.getIncomingBytes()); QString incomingWastedBytesString = locale.toString((uint)stats.getIncomingWastedBytes()); @@ -487,12 +486,12 @@ void OctreeStatsDialog::showOctreeServersOfType(int& serverCount, NodeType_t ser QString incomingEarlyString = locale.toString((uint)seqStats.getEarly()); QString incomingLikelyLostString = locale.toString((uint)seqStats.getLost()); QString incomingRecovered = locale.toString((uint)seqStats.getRecovered()); - + int clockSkewInMS = node->getClockSkewUsec() / (int)USECS_PER_MSEC; QString incomingFlightTimeString = locale.toString((int)stats.getIncomingFlightTimeAverage()); QString incomingPingTimeString = locale.toString(node->getPingMs()); QString incomingClockSkewString = locale.toString(clockSkewInMS); - + serverDetails << "
" << "Incoming Packets: " << qPrintable(incomingPacketsString) << "/ Lost: " << qPrintable(incomingLikelyLostString) << "/ Recovered: " << qPrintable(incomingRecovered); @@ -501,36 +500,36 @@ void OctreeStatsDialog::showOctreeServersOfType(int& serverCount, NodeType_t ser "/ Early: " << qPrintable(incomingEarlyString) << "/ Late: " << qPrintable(incomingLateString) << "/ Unreasonable: " << qPrintable(incomingUnreasonableString); - + serverDetails << "
" << " Average Flight Time: " << qPrintable(incomingFlightTimeString) << " msecs"; - + serverDetails << "
" << " Average Ping Time: " << qPrintable(incomingPingTimeString) << " msecs"; - + serverDetails << "
" << " Average Clock Skew: " << qPrintable(incomingClockSkewString) << " msecs"; - + serverDetails << "
" << "Incoming" << - " Bytes: " << qPrintable(incomingBytesString) << + " Bytes: " << qPrintable(incomingBytesString) << " Wasted Bytes: " << qPrintable(incomingWastedBytesString); - + serverDetails << extraDetails.str(); - if (_extraServerDetails[serverCount-1] == MORE) { + if (_extraServerDetails[serverCount - 1] == MORE) { linkDetails << " " << " [most...]"; linkDetails << " " << " [less...]"; } else { linkDetails << " " << " [less...]"; linkDetails << " " << " [least...]"; } - + } break; case LESS: { // nothing } break; + } } - } - Application::getInstance()->unlockOctreeSceneStats(); + }); } else { linkDetails << " " << " [more...]"; linkDetails << " " << " [most...]"; diff --git a/libraries/entities-renderer/src/EntityTreeRenderer.cpp b/libraries/entities-renderer/src/EntityTreeRenderer.cpp index fa8c0eb633..5ced01e982 100644 --- a/libraries/entities-renderer/src/EntityTreeRenderer.cpp +++ b/libraries/entities-renderer/src/EntityTreeRenderer.cpp @@ -341,16 +341,17 @@ void EntityTreeRenderer::checkEnterLeaveEntities() { QVector entitiesContainingAvatar; // find the entities near us - _tree->lockForRead(); // don't let someone else change our tree while we search - static_cast(_tree)->findEntities(avatarPosition, radius, foundEntities); + // don't let someone else change our tree while we search + _tree->withReadLock([&] { + static_cast(_tree)->findEntities(avatarPosition, radius, foundEntities); - // create a list of entities that actually contain the avatar's position - foreach(EntityItemPointer entity, foundEntities) { - if (entity->contains(avatarPosition)) { - entitiesContainingAvatar << entity->getEntityItemID(); + // create a list of entities that actually contain the avatar's position + foreach(EntityItemPointer entity, foundEntities) { + if (entity->contains(avatarPosition)) { + entitiesContainingAvatar << entity->getEntityItemID(); + } } - } - _tree->unlock(); + }); // Note: at this point we don't need to worry about the tree being locked, because we only deal with // EntityItemIDs from here. The loadEntityScript() method is robust against attempting to load scripts @@ -501,22 +502,19 @@ void EntityTreeRenderer::render(RenderArgs* renderArgs) { if (_tree && !_shuttingDown) { renderArgs->_renderer = this; + _tree->withReadLock([&] { + // Whenever you're in an intersection between zones, we will always choose the smallest zone. + _bestZone = NULL; // NOTE: Is this what we want? + _bestZoneVolume = std::numeric_limits::max(); - _tree->lockForRead(); + // FIX ME: right now the renderOperation does the following: + // 1) determining the best zone (not really rendering) + // 2) render the debug cell details + // we should clean this up + _tree->recurseTreeWithOperation(renderOperation, renderArgs); - // Whenever you're in an intersection between zones, we will always choose the smallest zone. - _bestZone = NULL; // NOTE: Is this what we want? - _bestZoneVolume = std::numeric_limits::max(); - - // FIX ME: right now the renderOperation does the following: - // 1) determining the best zone (not really rendering) - // 2) render the debug cell details - // we should clean this up - _tree->recurseTreeWithOperation(renderOperation, renderArgs); - - applyZonePropertiesToScene(_bestZone); - - _tree->unlock(); + applyZonePropertiesToScene(_bestZone); + }); } deleteReleasedModels(); // seems like as good as any other place to do some memory cleanup } diff --git a/libraries/entities/src/EntityItem.cpp b/libraries/entities/src/EntityItem.cpp index 9fa6ccac65..c4ef8cd910 100644 --- a/libraries/entities/src/EntityItem.cpp +++ b/libraries/entities/src/EntityItem.cpp @@ -1508,15 +1508,16 @@ void EntityItem::clearSimulationOwnership() { bool EntityItem::addAction(EntitySimulation* simulation, EntityActionPointer action) { - lockForWrite(); - checkWaitingToRemove(simulation); + bool result; + withWriteLock([&] { + checkWaitingToRemove(simulation); - bool result = addActionInternal(simulation, action); - if (!result) { - removeActionInternal(action->getID()); - } + bool result = addActionInternal(simulation, action); + if (!result) { + removeActionInternal(action->getID()); + } + }); - unlock(); return result; } @@ -1543,33 +1544,33 @@ bool EntityItem::addActionInternal(EntitySimulation* simulation, EntityActionPoi } bool EntityItem::updateAction(EntitySimulation* simulation, const QUuid& actionID, const QVariantMap& arguments) { - lockForWrite(); - checkWaitingToRemove(simulation); + bool success = false; + withWriteLock([&] { + checkWaitingToRemove(simulation); - if (!_objectActions.contains(actionID)) { - unlock(); - return false; - } - EntityActionPointer action = _objectActions[actionID]; + if (!_objectActions.contains(actionID)) { + return; + } - bool success = action->updateArguments(arguments); - if (success) { - _allActionsDataCache = serializeActions(success); - _dirtyFlags |= EntityItem::DIRTY_PHYSICS_ACTIVATION; - } else { - qDebug() << "EntityItem::updateAction failed"; - } + EntityActionPointer action = _objectActions[actionID]; - unlock(); + success = action->updateArguments(arguments); + if (success) { + _allActionsDataCache = serializeActions(success); + _dirtyFlags |= EntityItem::DIRTY_PHYSICS_ACTIVATION; + } else { + qDebug() << "EntityItem::updateAction failed"; + } + }); return success; } bool EntityItem::removeAction(EntitySimulation* simulation, const QUuid& actionID) { - lockForWrite(); - checkWaitingToRemove(simulation); - - bool success = removeActionInternal(actionID); - unlock(); + bool success = false; + withWriteLock([&] { + checkWaitingToRemove(simulation); + success = removeActionInternal(actionID); + }); return success; } @@ -1598,29 +1599,29 @@ bool EntityItem::removeActionInternal(const QUuid& actionID, EntitySimulation* s } bool EntityItem::clearActions(EntitySimulation* simulation) { - lockForWrite(); - QHash::iterator i = _objectActions.begin(); - while (i != _objectActions.end()) { - const QUuid id = i.key(); - EntityActionPointer action = _objectActions[id]; - i = _objectActions.erase(i); - action->setOwnerEntity(nullptr); - action->removeFromSimulation(simulation); - } - // empty _serializedActions means no actions for the EntityItem - _actionsToRemove.clear(); - _allActionsDataCache.clear(); - _dirtyFlags |= EntityItem::DIRTY_PHYSICS_ACTIVATION; - unlock(); + withWriteLock([&] { + QHash::iterator i = _objectActions.begin(); + while (i != _objectActions.end()) { + const QUuid id = i.key(); + EntityActionPointer action = _objectActions[id]; + i = _objectActions.erase(i); + action->setOwnerEntity(nullptr); + action->removeFromSimulation(simulation); + } + // empty _serializedActions means no actions for the EntityItem + _actionsToRemove.clear(); + _allActionsDataCache.clear(); + _dirtyFlags |= EntityItem::DIRTY_PHYSICS_ACTIVATION; + }); return true; } void EntityItem::deserializeActions() { assertUnlocked(); - lockForWrite(); - deserializeActionsInternal(); - unlock(); + withWriteLock([&] { + deserializeActionsInternal(); + }); } @@ -1694,9 +1695,9 @@ void EntityItem::checkWaitingToRemove(EntitySimulation* simulation) { void EntityItem::setActionData(QByteArray actionData) { assertUnlocked(); - lockForWrite(); - setActionDataInternal(actionData); - unlock(); + withWriteLock([&] { + setActionDataInternal(actionData); + }); } void EntityItem::setActionDataInternal(QByteArray actionData) { @@ -1750,119 +1751,23 @@ const QByteArray EntityItem::getActionDataInternal() const { } const QByteArray EntityItem::getActionData() const { + QByteArray result; assertUnlocked(); - lockForRead(); - auto result = getActionDataInternal(); - unlock(); + withReadLock([&] { + result = getActionDataInternal(); + }); return result; } QVariantMap EntityItem::getActionArguments(const QUuid& actionID) const { QVariantMap result; - lockForRead(); + withReadLock([&] { + if (_objectActions.contains(actionID)) { + EntityActionPointer action = _objectActions[actionID]; + result = action->getArguments(); + result["type"] = EntityActionInterface::actionTypeToString(action->getType()); + } + }); - if (_objectActions.contains(actionID)) { - EntityActionPointer action = _objectActions[actionID]; - result = action->getArguments(); - result["type"] = EntityActionInterface::actionTypeToString(action->getType()); - } - unlock(); return result; } - - - -#define ENABLE_LOCKING 1 - -#ifdef ENABLE_LOCKING -void EntityItem::lockForRead() const { - _lock.lockForRead(); -} - -bool EntityItem::tryLockForRead() const { - return _lock.tryLockForRead(); -} - -void EntityItem::lockForWrite() const { - _lock.lockForWrite(); -} - -bool EntityItem::tryLockForWrite() const { - return _lock.tryLockForWrite(); -} - -void EntityItem::unlock() const { - _lock.unlock(); -} - -bool EntityItem::isLocked() const { - bool readSuccess = tryLockForRead(); - if (readSuccess) { - unlock(); - } - bool writeSuccess = tryLockForWrite(); - if (writeSuccess) { - unlock(); - } - if (readSuccess && writeSuccess) { - return false; // if we can take both kinds of lock, there was no previous lock - } - return true; // either read or write failed, so there is some lock in place. -} - - -bool EntityItem::isWriteLocked() const { - bool readSuccess = tryLockForRead(); - if (readSuccess) { - unlock(); - return false; - } - bool writeSuccess = tryLockForWrite(); - if (writeSuccess) { - unlock(); - return false; - } - return true; // either read or write failed, so there is some lock in place. -} - - -bool EntityItem::isUnlocked() const { - // this can't be sure -- this may get unlucky and hit locks from other threads. what we're actually trying - // to discover is if *this* thread hasn't locked the EntityItem. Try repeatedly to take both kinds of lock. - bool readSuccess = false; - for (int i=0; i<80; i++) { - readSuccess = tryLockForRead(); - if (readSuccess) { - unlock(); - break; - } - QThread::usleep(200); - } - - bool writeSuccess = false; - if (readSuccess) { - for (int i=0; i<80; i++) { - writeSuccess = tryLockForWrite(); - if (writeSuccess) { - unlock(); - break; - } - QThread::usleep(300); - } - } - - if (readSuccess && writeSuccess) { - return true; // if we can take both kinds of lock, there was no previous lock - } - return false; -} -#else -void EntityItem::lockForRead() const { } -bool EntityItem::tryLockForRead() const { return true; } -void EntityItem::lockForWrite() const { } -bool EntityItem::tryLockForWrite() const { return true; } -void EntityItem::unlock() const { } -bool EntityItem::isLocked() const { return true; } -bool EntityItem::isWriteLocked() const { return true; } -bool EntityItem::isUnlocked() const { return true; } -#endif diff --git a/libraries/entities/src/EntityItem.h b/libraries/entities/src/EntityItem.h index 54a524b5a2..c08b385426 100644 --- a/libraries/entities/src/EntityItem.h +++ b/libraries/entities/src/EntityItem.h @@ -88,7 +88,7 @@ const float ACTIVATION_ANGULAR_VELOCITY_DELTA = 0.03f; /// EntityItem class this is the base class for all entity types. It handles the basic properties and functionality available /// to all other entity types. In particular: postion, size, rotation, age, lifetime, velocity, gravity. You can not instantiate /// one directly, instead you must only construct one of it's derived classes with additional features. -class EntityItem : public std::enable_shared_from_this { +class EntityItem : public std::enable_shared_from_this, public ReadWriteLockable { // These two classes manage lists of EntityItem pointers and must be able to cleanup pointers when an EntityItem is deleted. // To make the cleanup robust each EntityItem has backpointers to its manager classes (which are only ever set/cleared by // the managers themselves, hence they are fiends) whose NULL status can be used to determine which managers still need to @@ -512,16 +512,6 @@ protected: void checkWaitingToRemove(EntitySimulation* simulation = nullptr); mutable QSet _actionsToRemove; mutable bool _actionDataDirty = false; - - mutable QReadWriteLock _lock; - void lockForRead() const; - bool tryLockForRead() const; - void lockForWrite() const; - bool tryLockForWrite() const; - void unlock() const; - bool isLocked() const; - bool isWriteLocked() const; - bool isUnlocked() const; }; #endif // hifi_EntityItem_h diff --git a/libraries/entities/src/EntityScriptingInterface.cpp b/libraries/entities/src/EntityScriptingInterface.cpp index a735129a28..2c1753c8e7 100644 --- a/libraries/entities/src/EntityScriptingInterface.cpp +++ b/libraries/entities/src/EntityScriptingInterface.cpp @@ -72,23 +72,23 @@ QUuid EntityScriptingInterface::addEntity(const EntityItemProperties& properties // If we have a local entity tree set, then also update it. bool success = true; if (_entityTree) { - _entityTree->lockForWrite(); - EntityItemPointer entity = _entityTree->addEntity(id, propertiesWithSimID); - if (entity) { - // This Node is creating a new object. If it's in motion, set this Node as the simulator. - auto nodeList = DependencyManager::get(); - const QUuid myNodeID = nodeList->getSessionUUID(); - propertiesWithSimID.setSimulationOwner(myNodeID, SCRIPT_EDIT_SIMULATION_PRIORITY); + _entityTree->withWriteLock([&] { + EntityItemPointer entity = _entityTree->addEntity(id, propertiesWithSimID); + if (entity) { + // This Node is creating a new object. If it's in motion, set this Node as the simulator. + auto nodeList = DependencyManager::get(); + const QUuid myNodeID = nodeList->getSessionUUID(); + propertiesWithSimID.setSimulationOwner(myNodeID, SCRIPT_EDIT_SIMULATION_PRIORITY); - // and make note of it now, so we can act on it right away. - entity->setSimulationOwner(myNodeID, SCRIPT_EDIT_SIMULATION_PRIORITY); + // and make note of it now, so we can act on it right away. + entity->setSimulationOwner(myNodeID, SCRIPT_EDIT_SIMULATION_PRIORITY); - entity->setLastBroadcast(usecTimestampNow()); - } else { - qCDebug(entities) << "script failed to add new Entity to local Octree"; - success = false; - } - _entityTree->unlock(); + entity->setLastBroadcast(usecTimestampNow()); + } else { + qCDebug(entities) << "script failed to add new Entity to local Octree"; + success = false; + } + }); } // queue the packet @@ -102,28 +102,26 @@ QUuid EntityScriptingInterface::addEntity(const EntityItemProperties& properties EntityItemProperties EntityScriptingInterface::getEntityProperties(QUuid identity) { EntityItemProperties results; if (_entityTree) { - _entityTree->lockForRead(); + _entityTree->withReadLock([&] { + EntityItemPointer entity = _entityTree->findEntityByEntityItemID(EntityItemID(identity)); + if (entity) { + results = entity->getProperties(); - EntityItemPointer entity = _entityTree->findEntityByEntityItemID(EntityItemID(identity)); - - if (entity) { - results = entity->getProperties(); - - // TODO: improve sitting points and naturalDimensions in the future, - // for now we've included the old sitting points model behavior for entity types that are models - // we've also added this hack for setting natural dimensions of models - if (entity->getType() == EntityTypes::Model) { - const FBXGeometry* geometry = _entityTree->getGeometryForEntity(entity); - if (geometry) { - results.setSittingPoints(geometry->sittingPoints); - Extents meshExtents = geometry->getUnscaledMeshExtents(); - results.setNaturalDimensions(meshExtents.maximum - meshExtents.minimum); - results.calculateNaturalPosition(meshExtents.minimum, meshExtents.maximum); + // TODO: improve sitting points and naturalDimensions in the future, + // for now we've included the old sitting points model behavior for entity types that are models + // we've also added this hack for setting natural dimensions of models + if (entity->getType() == EntityTypes::Model) { + const FBXGeometry* geometry = _entityTree->getGeometryForEntity(entity); + if (geometry) { + results.setSittingPoints(geometry->sittingPoints); + Extents meshExtents = geometry->getUnscaledMeshExtents(); + results.setNaturalDimensions(meshExtents.maximum - meshExtents.minimum); + results.calculateNaturalPosition(meshExtents.minimum, meshExtents.maximum); + } } - } - } - _entityTree->unlock(); + } + }); } return results; @@ -132,56 +130,59 @@ EntityItemProperties EntityScriptingInterface::getEntityProperties(QUuid identit QUuid EntityScriptingInterface::editEntity(QUuid id, EntityItemProperties properties) { EntityItemID entityID(id); // If we have a local entity tree set, then also update it. - if (_entityTree) { - _entityTree->lockForWrite(); - bool updatedEntity = _entityTree->updateEntity(entityID, properties); - _entityTree->unlock(); + if (!_entityTree) { + queueEntityMessage(PacketType::EntityEdit, entityID, properties); + return id; + } - if (updatedEntity) { - _entityTree->lockForRead(); - EntityItemPointer entity = _entityTree->findEntityByEntityItemID(entityID); - if (entity) { - // make sure the properties has a type, so that the encode can know which properties to include - properties.setType(entity->getType()); - bool hasTerseUpdateChanges = properties.hasTerseUpdateChanges(); - bool hasPhysicsChanges = properties.hasMiscPhysicsChanges() || hasTerseUpdateChanges; - if (hasPhysicsChanges) { - auto nodeList = DependencyManager::get(); - const QUuid myNodeID = nodeList->getSessionUUID(); + bool updatedEntity = false; + _entityTree->withWriteLock([&] { + updatedEntity = _entityTree->updateEntity(entityID, properties); + }); - if (entity->getSimulatorID() == myNodeID) { - // we think we already own the simulation, so make sure to send ALL TerseUpdate properties - if (hasTerseUpdateChanges) { - entity->getAllTerseUpdateProperties(properties); - } - // TODO: if we knew that ONLY TerseUpdate properties have changed in properties AND the object - // is dynamic AND it is active in the physics simulation then we could chose to NOT queue an update - // and instead let the physics simulation decide when to send a terse update. This would remove - // the "slide-no-rotate" glitch (and typical a double-update) that we see during the "poke rolling - // balls" test. However, even if we solve this problem we still need to provide a "slerp the visible - // proxy toward the true physical position" feature to hide the final glitches in the remote watcher's - // simulation. - if (entity->getSimulationPriority() < SCRIPT_EDIT_SIMULATION_PRIORITY) { - // we re-assert our simulation ownership at a higher priority - properties.setSimulationOwner(myNodeID, - glm::max(entity->getSimulationPriority(), SCRIPT_EDIT_SIMULATION_PRIORITY)); - } - } else { - // we make a bid for simulation ownership - properties.setSimulationOwner(myNodeID, SCRIPT_EDIT_SIMULATION_PRIORITY); - entity->flagForOwnership(); - } - } - entity->setLastBroadcast(usecTimestampNow()); - } - _entityTree->unlock(); - queueEntityMessage(PacketType::EntityEdit, entityID, properties); - return id; - } + if (!updatedEntity) { return QUuid(); } + _entityTree->withReadLock([&] { + EntityItemPointer entity = _entityTree->findEntityByEntityItemID(entityID); + if (entity) { + // make sure the properties has a type, so that the encode can know which properties to include + properties.setType(entity->getType()); + bool hasTerseUpdateChanges = properties.hasTerseUpdateChanges(); + bool hasPhysicsChanges = properties.hasMiscPhysicsChanges() || hasTerseUpdateChanges; + if (hasPhysicsChanges) { + auto nodeList = DependencyManager::get(); + const QUuid myNodeID = nodeList->getSessionUUID(); + + if (entity->getSimulatorID() == myNodeID) { + // we think we already own the simulation, so make sure to send ALL TerseUpdate properties + if (hasTerseUpdateChanges) { + entity->getAllTerseUpdateProperties(properties); + } + // TODO: if we knew that ONLY TerseUpdate properties have changed in properties AND the object + // is dynamic AND it is active in the physics simulation then we could chose to NOT queue an update + // and instead let the physics simulation decide when to send a terse update. This would remove + // the "slide-no-rotate" glitch (and typical a double-update) that we see during the "poke rolling + // balls" test. However, even if we solve this problem we still need to provide a "slerp the visible + // proxy toward the true physical position" feature to hide the final glitches in the remote watcher's + // simulation. + + if (entity->getSimulationPriority() < SCRIPT_EDIT_SIMULATION_PRIORITY) { + // we re-assert our simulation ownership at a higher priority + properties.setSimulationOwner(myNodeID, + glm::max(entity->getSimulationPriority(), SCRIPT_EDIT_SIMULATION_PRIORITY)); + } + } else { + // we make a bid for simulation ownership + properties.setSimulationOwner(myNodeID, SCRIPT_EDIT_SIMULATION_PRIORITY); + entity->flagForOwnership(); + } + } + entity->setLastBroadcast(usecTimestampNow()); + } + }); queueEntityMessage(PacketType::EntityEdit, entityID, properties); return id; } @@ -192,18 +193,16 @@ void EntityScriptingInterface::deleteEntity(QUuid id) { // If we have a local entity tree set, then also update it. if (_entityTree) { - _entityTree->lockForWrite(); - - EntityItemPointer entity = _entityTree->findEntityByEntityItemID(entityID); - if (entity) { - if (entity->getLocked()) { - shouldDelete = false; - } else { - _entityTree->deleteEntity(entityID); + _entityTree->withWriteLock([&] { + EntityItemPointer entity = _entityTree->findEntityByEntityItemID(entityID); + if (entity) { + if (entity->getLocked()) { + shouldDelete = false; + } else { + _entityTree->deleteEntity(entityID); + } } - } - - _entityTree->unlock(); + }); } // if at this point, we know the id, and we should still delete the entity, send the update to the entity server @@ -215,9 +214,10 @@ void EntityScriptingInterface::deleteEntity(QUuid id) { QUuid EntityScriptingInterface::findClosestEntity(const glm::vec3& center, float radius) const { EntityItemID result; if (_entityTree) { - _entityTree->lockForRead(); - EntityItemPointer closestEntity = _entityTree->findClosestEntity(center, radius); - _entityTree->unlock(); + EntityItemPointer closestEntity; + _entityTree->withReadLock([&] { + closestEntity = _entityTree->findClosestEntity(center, radius); + }); if (closestEntity) { result = closestEntity->getEntityItemID(); } @@ -228,19 +228,19 @@ QUuid EntityScriptingInterface::findClosestEntity(const glm::vec3& center, float void EntityScriptingInterface::dumpTree() const { if (_entityTree) { - _entityTree->lockForRead(); - _entityTree->dumpTree(); - _entityTree->unlock(); + _entityTree->withReadLock([&] { + _entityTree->dumpTree(); + }); } } QVector EntityScriptingInterface::findEntities(const glm::vec3& center, float radius) const { QVector result; if (_entityTree) { - _entityTree->lockForRead(); QVector entities; - _entityTree->findEntities(center, radius, entities); - _entityTree->unlock(); + _entityTree->withReadLock([&] { + _entityTree->findEntities(center, radius, entities); + }); foreach (EntityItemPointer entity, entities) { result << entity->getEntityItemID(); @@ -252,11 +252,11 @@ QVector EntityScriptingInterface::findEntities(const glm::vec3& center, f QVector EntityScriptingInterface::findEntitiesInBox(const glm::vec3& corner, const glm::vec3& dimensions) const { QVector result; if (_entityTree) { - _entityTree->lockForRead(); - AABox box(corner, dimensions); QVector entities; - _entityTree->findEntities(box, entities); - _entityTree->unlock(); + _entityTree->withReadLock([&] { + AABox box(corner, dimensions); + _entityTree->findEntities(box, entities); + }); foreach (EntityItemPointer entity, entities) { result << entity->getEntityItemID(); @@ -432,9 +432,10 @@ bool EntityScriptingInterface::setVoxels(QUuid entityID, } auto polyVoxEntity = std::dynamic_pointer_cast(entity); - _entityTree->lockForWrite(); - bool result = actor(*polyVoxEntity); - _entityTree->unlock(); + bool result; + _entityTree->withWriteLock([&] { + result = actor(*polyVoxEntity); + }); return result; } @@ -457,15 +458,17 @@ bool EntityScriptingInterface::setPoints(QUuid entityID, std::function(entity); - _entityTree->lockForWrite(); - bool success = actor(*lineEntity); - entity->setLastEdited(now); - entity->setLastBroadcast(now); - _entityTree->unlock(); + bool success; + _entityTree->withWriteLock([&] { + success = actor(*lineEntity); + entity->setLastEdited(now); + entity->setLastBroadcast(now); + }); - _entityTree->lockForRead(); - EntityItemProperties properties = entity->getProperties(); - _entityTree->unlock(); + EntityItemProperties properties; + _entityTree->withReadLock([&] { + properties = entity->getProperties(); + }); properties.setLinePointsDirty(); properties.setLastEdited(now); @@ -543,32 +546,33 @@ bool EntityScriptingInterface::actionWorker(const QUuid& entityID, return false; } - _entityTree->lockForWrite(); + EntityItemPointer entity; + bool success; + _entityTree->withWriteLock([&] { + EntitySimulation* simulation = _entityTree->getSimulation(); + EntityItemPointer entity = _entityTree->findEntityByEntityItemID(entityID); + if (!entity) { + qDebug() << "actionWorker -- unknown entity" << entityID; + return; + } - EntitySimulation* simulation = _entityTree->getSimulation(); - EntityItemPointer entity = _entityTree->findEntityByEntityItemID(entityID); - if (!entity) { - qDebug() << "actionWorker -- unknown entity" << entityID; - _entityTree->unlock(); - return false; - } + if (!simulation) { + qDebug() << "actionWorker -- no simulation" << entityID; + return; + } - if (!simulation) { - qDebug() << "actionWorker -- no simulation" << entityID; - _entityTree->unlock(); - return false; - } - - bool success = actor(simulation, entity); - if (success) { - _entityTree->entityChanged(entity); - } - _entityTree->unlock(); + success = actor(simulation, entity); + if (success) { + _entityTree->entityChanged(entity); + } + }); // transmit the change - _entityTree->lockForRead(); - EntityItemProperties properties = entity->getProperties(); - _entityTree->unlock(); + EntityItemProperties properties; + _entityTree->withReadLock([&] { + properties = entity->getProperties(); + }); + properties.setActionDataDirty(); auto now = usecTimestampNow(); properties.setLastEdited(now); diff --git a/libraries/entities/src/EntitySimulation.cpp b/libraries/entities/src/EntitySimulation.cpp index 06f5023193..dc85657097 100644 --- a/libraries/entities/src/EntitySimulation.cpp +++ b/libraries/entities/src/EntitySimulation.cpp @@ -27,6 +27,7 @@ void EntitySimulation::setEntityTree(EntityTree* tree) { } void EntitySimulation::updateEntities() { + QMutexLocker lock(&_mutex); quint64 now = usecTimestampNow(); // these methods may accumulate entries in _entitiesToBeDeleted @@ -38,7 +39,7 @@ void EntitySimulation::updateEntities() { } void EntitySimulation::getEntitiesToDelete(VectorOfEntities& entitiesToDelete) { - + QMutexLocker lock(&_mutex); for (auto entity : _entitiesToDelete) { // this entity is still in its tree, so we insert into the external list entitiesToDelete.push_back(entity); @@ -145,6 +146,7 @@ void EntitySimulation::sortEntitiesThatMoved() { } void EntitySimulation::addEntity(EntityItemPointer entity) { + QMutexLocker lock(&_mutex); assert(entity); entity->deserializeActions(); if (entity->isMortal()) { @@ -168,6 +170,7 @@ void EntitySimulation::addEntity(EntityItemPointer entity) { } void EntitySimulation::removeEntity(EntityItemPointer entity) { + QMutexLocker lock(&_mutex); assert(entity); _entitiesToUpdate.remove(entity); _mortalEntities.remove(entity); @@ -181,6 +184,7 @@ void EntitySimulation::removeEntity(EntityItemPointer entity) { } void EntitySimulation::changeEntity(EntityItemPointer entity) { + QMutexLocker lock(&_mutex); assert(entity); if (!entity->_simulated) { // This entity was either never added to the simulation or has been removed @@ -232,6 +236,7 @@ void EntitySimulation::changeEntity(EntityItemPointer entity) { } void EntitySimulation::clearEntities() { + QMutexLocker lock(&_mutex); _mortalEntities.clear(); _nextExpiry = quint64(-1); _entitiesToUpdate.clear(); @@ -263,26 +268,24 @@ void EntitySimulation::moveSimpleKinematics(const quint64& now) { } void EntitySimulation::addAction(EntityActionPointer action) { - lock(); + QMutexLocker lock(&_mutex); _actionsToAdd += action; - unlock(); } void EntitySimulation::removeAction(const QUuid actionID) { - lock(); + QMutexLocker lock(&_mutex); _actionsToRemove += actionID; - unlock(); } void EntitySimulation::removeActions(QList actionIDsToRemove) { - lock(); - _actionsToRemove += actionIDsToRemove; - unlock(); + QMutexLocker lock(&_mutex); + foreach(QUuid uuid, actionIDsToRemove) { + _actionsToRemove.insert(uuid); + } } void EntitySimulation::applyActionChanges() { - lock(); + QMutexLocker lock(&_mutex); _actionsToAdd.clear(); _actionsToRemove.clear(); - unlock(); } diff --git a/libraries/entities/src/EntitySimulation.h b/libraries/entities/src/EntitySimulation.h index c1822abe77..a7bdfccf7b 100644 --- a/libraries/entities/src/EntitySimulation.h +++ b/libraries/entities/src/EntitySimulation.h @@ -47,22 +47,18 @@ public: EntitySimulation() : _mutex(QMutex::Recursive), _entityTree(NULL), _nextExpiry(quint64(-1)) { } virtual ~EntitySimulation() { setEntityTree(NULL); } - void lock() { _mutex.lock(); } - void unlock() { _mutex.unlock(); } - /// \param tree pointer to EntityTree which is stored internally void setEntityTree(EntityTree* tree); void updateEntities(); - friend class EntityTree; +// friend class EntityTree; virtual void addAction(EntityActionPointer action); virtual void removeAction(const QUuid actionID); virtual void removeActions(QList actionIDsToRemove); virtual void applyActionChanges(); -protected: // these only called by the EntityTree? /// \param entity pointer to EntityItem to be added /// \sideeffect sets relevant backpointers in entity, but maybe later when appropriate data structures are locked void addEntity(EntityItemPointer entity); @@ -79,6 +75,7 @@ protected: // these only called by the EntityTree? void clearEntities(); void moveSimpleKinematics(const quint64& now); +protected: // these only called by the EntityTree? public: @@ -90,7 +87,6 @@ signals: void entityCollisionWithEntity(const EntityItemID& idA, const EntityItemID& idB, const Collision& collision); protected: - // These pure virtual methods are protected because they are not to be called will-nilly. The base class // calls them in the right places. virtual void updateEntitiesInternal(const quint64& now) = 0; @@ -103,7 +99,15 @@ protected: void callUpdateOnEntitiesThatNeedIt(const quint64& now); void sortEntitiesThatMoved(); - QMutex _mutex; + QMutex _mutex{ QMutex::Recursive }; + + SetOfEntities _entitiesToSort; // entities moved by simulation (and might need resort in EntityTree) + SetOfEntities _simpleKinematicEntities; // entities undergoing non-colliding kinematic motion + QList _actionsToAdd; + QSet _actionsToRemove; + +private: + void moveSimpleKinematics(); // back pointer to EntityTree structure EntityTree* _entityTree; @@ -113,17 +117,11 @@ protected: SetOfEntities _allEntities; // tracks all entities added the simulation SetOfEntities _mortalEntities; // entities that have an expiry quint64 _nextExpiry; + + SetOfEntities _entitiesToUpdate; // entities that need to call EntityItem::update() - SetOfEntities _entitiesToSort; // entities moved by simulation (and might need resort in EntityTree) SetOfEntities _entitiesToDelete; // entities simulation decided needed to be deleted (EntityTree will actually delete) - SetOfEntities _simpleKinematicEntities; // entities undergoing non-colliding kinematic motion - private: - void moveSimpleKinematics(); - - protected: - QList _actionsToAdd; - QList _actionsToRemove; }; #endif // hifi_EntitySimulation_h diff --git a/libraries/entities/src/EntityTree.cpp b/libraries/entities/src/EntityTree.cpp index 2879468b5a..6e6cb989e5 100644 --- a/libraries/entities/src/EntityTree.cpp +++ b/libraries/entities/src/EntityTree.cpp @@ -50,9 +50,7 @@ void EntityTree::eraseAllOctreeElements(bool createNewRoot) { // this would be a good place to clean up our entities... if (_simulation) { - _simulation->lock(); _simulation->clearEntities(); - _simulation->unlock(); } foreach (EntityTreeElement* element, _entityToElementMap) { element->cleanupEntities(); @@ -80,9 +78,7 @@ void EntityTree::postAddEntity(EntityItemPointer entity) { assert(entity); // check to see if we need to simulate this entity.. if (_simulation) { - _simulation->lock(); _simulation->addEntity(entity); - _simulation->unlock(); } _isDirty = true; maybeNotifyNewCollisionSoundURL("", entity->getCollisionSoundURL()); @@ -209,9 +205,7 @@ bool EntityTree::updateEntityWithElement(EntityItemPointer entity, const EntityI if (newFlags) { if (_simulation) { if (newFlags & DIRTY_SIMULATION_FLAGS) { - _simulation->lock(); _simulation->changeEntity(entity); - _simulation->unlock(); } } else { // normally the _simulation clears ALL updateFlags, but since there is none we do it explicitly @@ -293,18 +287,18 @@ void EntityTree::maybeNotifyNewCollisionSoundURL(const QString& previousCollisio } void EntityTree::setSimulation(EntitySimulation* simulation) { - if (simulation) { - // assert that the simulation's backpointer has already been properly connected - assert(simulation->getEntityTree() == this); - } - if (_simulation && _simulation != simulation) { - // It's important to clearEntities() on the simulation since taht will update each - // EntityItem::_simulationState correctly so as to not confuse the next _simulation. - _simulation->lock(); - _simulation->clearEntities(); - _simulation->unlock(); - } - _simulation = simulation; + this->withWriteLock([&] { + if (simulation) { + // assert that the simulation's backpointer has already been properly connected + assert(simulation->getEntityTree() == this); + } + if (_simulation && _simulation != simulation) { + // It's important to clearEntities() on the simulation since taht will update each + // EntityItem::_simulationState correctly so as to not confuse the next _simulation. + _simulation->clearEntities(); + } + _simulation = simulation; + }); } void EntityTree::deleteEntity(const EntityItemID& entityID, bool force, bool ignoreWarnings) { @@ -383,9 +377,6 @@ void EntityTree::deleteEntities(QSet entityIDs, bool force, bool i void EntityTree::processRemovedEntities(const DeleteEntityOperator& theOperator) { const RemovedEntities& entities = theOperator.getEntities(); - if (_simulation) { - _simulation->lock(); - } foreach(const EntityToDeleteDetails& details, entities) { EntityItemPointer theEntity = details.entity; @@ -402,9 +393,6 @@ void EntityTree::processRemovedEntities(const DeleteEntityOperator& theOperator) _simulation->removeEntity(theEntity); } } - if (_simulation) { - _simulation->unlock(); - } } @@ -455,10 +443,10 @@ bool EntityTree::findNearPointOperation(OctreeElement* element, void* extraData) EntityItemPointer EntityTree::findClosestEntity(glm::vec3 position, float targetRadius) { FindNearPointArgs args = { position, targetRadius, false, NULL, FLT_MAX }; - lockForRead(); - // NOTE: This should use recursion, since this is a spatial operation - recurseTreeWithOperation(findNearPointOperation, &args); - unlock(); + withReadLock([&] { + // NOTE: This should use recursion, since this is a spatial operation + recurseTreeWithOperation(findNearPointOperation, &args); + }); return args.closestEntity; } @@ -710,33 +698,29 @@ void EntityTree::releaseSceneEncodeData(OctreeElementExtraEncodeData* extraEncod void EntityTree::entityChanged(EntityItemPointer entity) { if (_simulation) { - _simulation->lock(); _simulation->changeEntity(entity); - _simulation->unlock(); } } void EntityTree::update() { if (_simulation) { - lockForWrite(); - _simulation->lock(); - _simulation->updateEntities(); - VectorOfEntities pendingDeletes; - _simulation->getEntitiesToDelete(pendingDeletes); - _simulation->unlock(); + withWriteLock([&] { + _simulation->updateEntities(); + VectorOfEntities pendingDeletes; + _simulation->getEntitiesToDelete(pendingDeletes); - if (pendingDeletes.size() > 0) { - // translate into list of ID's - QSet idsToDelete; + if (pendingDeletes.size() > 0) { + // translate into list of ID's + QSet idsToDelete; - for (auto entity : pendingDeletes) { - idsToDelete.insert(entity->getEntityItemID()); + for (auto entity : pendingDeletes) { + idsToDelete.insert(entity->getEntityItemID()); + } + + // delete these things the roundabout way + deleteEntities(idsToDelete, true); } - - // delete these things the roundabout way - deleteEntities(idsToDelete, true); - } - unlock(); + }); } } @@ -856,36 +840,35 @@ void EntityTree::forgetEntitiesDeletedBefore(quint64 sinceTime) { // TODO: consider consolidating processEraseMessageDetails() and processEraseMessage() int EntityTree::processEraseMessage(NLPacket& packet, const SharedNodePointer& sourceNode) { - lockForWrite(); + withWriteLock([&] { + packet.seek(sizeof(OCTREE_PACKET_FLAGS) + sizeof(OCTREE_PACKET_SEQUENCE) + sizeof(OCTREE_PACKET_SENT_TIME)); - packet.seek(sizeof(OCTREE_PACKET_FLAGS) + sizeof(OCTREE_PACKET_SEQUENCE) + sizeof(OCTREE_PACKET_SENT_TIME)); + uint16_t numberOfIDs = 0; // placeholder for now + packet.readPrimitive(&numberOfIDs); - uint16_t numberOfIDs = 0; // placeholder for now - packet.readPrimitive(&numberOfIDs); - - if (numberOfIDs > 0) { - QSet entityItemIDsToDelete; + if (numberOfIDs > 0) { + QSet entityItemIDsToDelete; - for (size_t i = 0; i < numberOfIDs; i++) { + for (size_t i = 0; i < numberOfIDs; i++) { + + if (NUM_BYTES_RFC4122_UUID > packet.bytesLeftToRead()) { + qCDebug(entities) << "EntityTree::processEraseMessage().... bailing because not enough bytes in buffer"; + break; // bail to prevent buffer overflow + } + + QUuid entityID = QUuid::fromRfc4122(packet.readWithoutCopy(NUM_BYTES_RFC4122_UUID)); + + EntityItemID entityItemID(entityID); + entityItemIDsToDelete << entityItemID; + + if (wantEditLogging()) { + qCDebug(entities) << "User [" << sourceNode->getUUID() << "] deleting entity. ID:" << entityItemID; + } - if (NUM_BYTES_RFC4122_UUID > packet.bytesLeftToRead()) { - qCDebug(entities) << "EntityTree::processEraseMessage().... bailing because not enough bytes in buffer"; - break; // bail to prevent buffer overflow } - - QUuid entityID = QUuid::fromRfc4122(packet.readWithoutCopy(NUM_BYTES_RFC4122_UUID)); - - EntityItemID entityItemID(entityID); - entityItemIDsToDelete << entityItemID; - - if (wantEditLogging()) { - qCDebug(entities) << "User [" << sourceNode->getUUID() << "] deleting entity. ID:" << entityItemID; - } - + deleteEntities(entityItemIDsToDelete, true, true); } - deleteEntities(entityItemIDsToDelete, true, true); - } - unlock(); + }); return packet.pos(); } @@ -1043,9 +1026,9 @@ bool EntityTree::sendEntitiesOperation(OctreeElement* element, void* extraData) // also update the local tree instantly (note: this is not our tree, but an alternate tree) if (args->localTree) { - args->localTree->lockForWrite(); - args->localTree->addEntity(newID, properties); - args->localTree->unlock(); + args->localTree->withWriteLock([&] { + args->localTree->addEntity(newID, properties); + }); } } diff --git a/libraries/entities/src/EntityTreeHeadlessViewer.cpp b/libraries/entities/src/EntityTreeHeadlessViewer.cpp index 80c0a7fa7f..073aa8ec37 100644 --- a/libraries/entities/src/EntityTreeHeadlessViewer.cpp +++ b/libraries/entities/src/EntityTreeHeadlessViewer.cpp @@ -33,10 +33,9 @@ void EntityTreeHeadlessViewer::init() { void EntityTreeHeadlessViewer::update() { if (_tree) { EntityTree* tree = static_cast(_tree); - if (tree->tryLockForWrite()) { + tree->withTryWriteLock([&] { tree->update(); - tree->unlock(); - } + }); } } diff --git a/libraries/octree/src/JurisdictionMap.h b/libraries/octree/src/JurisdictionMap.h index b4294659d3..7cf7ebe6a9 100644 --- a/libraries/octree/src/JurisdictionMap.h +++ b/libraries/octree/src/JurisdictionMap.h @@ -18,7 +18,8 @@ #include #include -#include + +#include #include #include @@ -78,7 +79,7 @@ private: /// Map between node IDs and their reported JurisdictionMap. Typically used by classes that need to know which nodes are /// managing which jurisdictions. -class NodeToJurisdictionMap : public QMap, public QReadWriteLock {}; +class NodeToJurisdictionMap : public QMap, public ReadWriteLockable {}; typedef QMap::iterator NodeToJurisdictionMapIterator; diff --git a/libraries/octree/src/Octree.cpp b/libraries/octree/src/Octree.cpp index 54838ad019..0f0f307dcc 100644 --- a/libraries/octree/src/Octree.cpp +++ b/libraries/octree/src/Octree.cpp @@ -60,7 +60,6 @@ Octree::Octree(bool shouldReaverage) : _isDirty(true), _shouldReaverage(shouldReaverage), _stopImport(false), - _lock(QReadWriteLock::Recursive), _isViewing(false), _isServer(false) { @@ -461,9 +460,7 @@ void Octree::readBitstreamToTree(const unsigned char * bitstream, unsigned long void Octree::deleteOctreeElementAt(float x, float y, float z, float s) { unsigned char* octalCode = pointToOctalCode(x,y,z,s); - lockForWrite(); deleteOctalCodeFromTree(octalCode); - unlock(); delete[] octalCode; // cleanup memory } @@ -488,7 +485,9 @@ void Octree::deleteOctalCodeFromTree(const unsigned char* codeBuffer, bool colla args.deleteLastChild = false; args.pathChanged = false; - deleteOctalCodeFromTreeRecursion(_rootElement, &args); + withWriteLock([&] { + deleteOctalCodeFromTreeRecursion(_rootElement, &args); + }); } void Octree::deleteOctalCodeFromTreeRecursion(OctreeElement* element, void* extraData) { @@ -685,29 +684,15 @@ bool Octree::findRayIntersection(const glm::vec3& origin, const glm::vec3& direc intersectedObject, false, precisionPicking}; distance = FLT_MAX; - bool gotLock = false; - if (lockType == Octree::Lock) { - lockForRead(); - gotLock = true; - } else if (lockType == Octree::TryLock) { - gotLock = tryLockForRead(); - if (!gotLock) { - if (accurateResult) { - *accurateResult = false; // if user asked to accuracy or result, let them know this is inaccurate - } - return args.found; // if we wanted to tryLock, and we couldn't then just bail... - } - } - - recurseTreeWithOperation(findRayIntersectionOp, &args); - - if (gotLock) { - unlock(); - } + bool requireLock = lockType == Octree::Lock; + bool lockResult = withReadLock([&]{ + recurseTreeWithOperation(findRayIntersectionOp, &args); + }, requireLock); if (accurateResult) { - *accurateResult = true; // if user asked to accuracy or result, let them know this is accurate + *accurateResult = lockResult; // if user asked to accuracy or result, let them know this is accurate } + return args.found; } @@ -753,31 +738,16 @@ bool Octree::findSpherePenetration(const glm::vec3& center, float radius, glm::v NULL }; penetration = glm::vec3(0.0f, 0.0f, 0.0f); - bool gotLock = false; - if (lockType == Octree::Lock) { - lockForRead(); - gotLock = true; - } else if (lockType == Octree::TryLock) { - gotLock = tryLockForRead(); - if (!gotLock) { - if (accurateResult) { - *accurateResult = false; // if user asked to accuracy or result, let them know this is inaccurate - } - return args.found; // if we wanted to tryLock, and we couldn't then just bail... + bool requireLock = lockType == Octree::Lock; + bool lockResult = withReadLock([&]{ + recurseTreeWithOperation(findSpherePenetrationOp, &args); + if (penetratedObject) { + *penetratedObject = args.penetratedObject; } - } - - recurseTreeWithOperation(findSpherePenetrationOp, &args); - if (penetratedObject) { - *penetratedObject = args.penetratedObject; - } - - if (gotLock) { - unlock(); - } + }, requireLock); if (accurateResult) { - *accurateResult = true; // if user asked to accuracy or result, let them know this is accurate + *accurateResult = lockResult; // if user asked to accuracy or result, let them know this is accurate } return args.found; } @@ -856,40 +826,24 @@ bool Octree::findCapsulePenetration(const glm::vec3& start, const glm::vec3& end CapsuleArgs args = { start, end, radius, penetration, false }; penetration = glm::vec3(0.0f, 0.0f, 0.0f); - bool gotLock = false; - if (lockType == Octree::Lock) { - lockForRead(); - gotLock = true; - } else if (lockType == Octree::TryLock) { - gotLock = tryLockForRead(); - if (!gotLock) { - if (accurateResult) { - *accurateResult = false; // if user asked to accuracy or result, let them know this is inaccurate - } - return args.found; // if we wanted to tryLock, and we couldn't then just bail... - } - } - recurseTreeWithOperation(findCapsulePenetrationOp, &args); - - if (gotLock) { - unlock(); - } + bool requireLock = lockType == Octree::Lock; + bool lockResult = withReadLock([&]{ + recurseTreeWithOperation(findCapsulePenetrationOp, &args); + }, requireLock); if (accurateResult) { - *accurateResult = true; // if user asked to accuracy or result, let them know this is accurate + *accurateResult = lockResult; // if user asked to accuracy or result, let them know this is accurate } + return args.found; } bool Octree::findContentInCube(const AACube& cube, CubeList& cubes) { - if (!tryLockForRead()) { - return false; - } - ContentArgs args = { cube, &cubes }; - recurseTreeWithOperation(findContentInCubeOp, &args); - unlock(); - return true; + return withTryReadLock([&]{ + ContentArgs args = { cube, &cubes }; + recurseTreeWithOperation(findContentInCubeOp, &args); + }); } class GetElementEnclosingArgs { @@ -919,29 +873,15 @@ OctreeElement* Octree::getElementEnclosingPoint(const glm::vec3& point, Octree:: args.point = point; args.element = NULL; - bool gotLock = false; - if (lockType == Octree::Lock) { - lockForRead(); - gotLock = true; - } else if (lockType == Octree::TryLock) { - gotLock = tryLockForRead(); - if (!gotLock) { - if (accurateResult) { - *accurateResult = false; // if user asked to accuracy or result, let them know this is inaccurate - } - return args.element; // if we wanted to tryLock, and we couldn't then just bail... - } - } - - recurseTreeWithOperation(getElementEnclosingOperation, (void*)&args); - - if (gotLock) { - unlock(); - } + bool requireLock = lockType == Octree::Lock; + bool lockResult = withReadLock([&]{ + recurseTreeWithOperation(getElementEnclosingOperation, (void*)&args); + }, requireLock); if (accurateResult) { - *accurateResult = false; // if user asked to accuracy or result, let them know this is inaccurate + *accurateResult = lockResult; // if user asked to accuracy or result, let them know this is accurate } + return args.element; } @@ -2164,11 +2104,11 @@ void Octree::writeToSVOFile(const char* fileName, OctreeElement* element) { while (!elementBag.isEmpty()) { OctreeElement* subTree = elementBag.extract(); - lockForRead(); // do tree locking down here so that we have shorter slices and less thread contention EncodeBitstreamParams params(INT_MAX, IGNORE_VIEW_FRUSTUM, WANT_COLOR, NO_EXISTS_BITS); - params.extraEncodeData = &extraEncodeData; - bytesWritten = encodeTreeBitstream(subTree, &packetData, elementBag, params); - unlock(); + withReadLock([&] { + params.extraEncodeData = &extraEncodeData; + bytesWritten = encodeTreeBitstream(subTree, &packetData, elementBag, params); + }); // if the subTree couldn't fit, and so we should reset the packet and reinsert the element in our bag and try again if (bytesWritten == 0 && (params.stopReason == EncodeBitstreamParams::DIDNT_FIT)) { diff --git a/libraries/octree/src/Octree.h b/libraries/octree/src/Octree.h index 8651ce5622..cc1eed9735 100644 --- a/libraries/octree/src/Octree.h +++ b/libraries/octree/src/Octree.h @@ -13,7 +13,9 @@ #define hifi_Octree_h #include -#include +#include +#include +#include class CoverageMap; class ReadBitstreamToTreeParams; @@ -23,6 +25,8 @@ class OctreeElementBag; class OctreePacketData; class Shape; +#include +#include #include "JurisdictionMap.h" #include "ViewFrustum.h" @@ -31,10 +35,6 @@ class Shape; #include "OctreePacketData.h" #include "OctreeSceneStats.h" -#include -#include -#include - extern QVector PERSIST_EXTENSIONS; @@ -216,7 +216,7 @@ public: {} }; -class Octree : public QObject { +class Octree : public QObject, public ReadWriteLockable { Q_OBJECT public: Octree(bool shouldReaverage = false); @@ -289,17 +289,10 @@ public: void clearDirtyBit() { _isDirty = false; } void setDirtyBit() { _isDirty = true; } - // Octree does not currently handle its own locking, caller must use these to lock/unlock - void lockForRead() { _lock.lockForRead(); } - bool tryLockForRead() { return _lock.tryLockForRead(); } - void lockForWrite() { _lock.lockForWrite(); } - bool tryLockForWrite() { return _lock.tryLockForWrite(); } - void unlock() { _lock.unlock(); } // output hints from the encode process typedef enum { Lock, - TryLock, - NoLock + TryLock } lockType; bool findRayIntersection(const glm::vec3& origin, const glm::vec3& direction, @@ -409,8 +402,6 @@ protected: bool _shouldReaverage; bool _stopImport; - QReadWriteLock _lock; - bool _isViewing; bool _isServer; }; diff --git a/libraries/octree/src/OctreeEditPacketSender.cpp b/libraries/octree/src/OctreeEditPacketSender.cpp index 0d67e2391d..1be271cbdd 100644 --- a/libraries/octree/src/OctreeEditPacketSender.cpp +++ b/libraries/octree/src/OctreeEditPacketSender.cpp @@ -51,11 +51,11 @@ bool OctreeEditPacketSender::serversExist() const { if (_serverJurisdictions) { // lookup our nodeUUID in the jurisdiction map, if it's missing then we're // missing at least one jurisdiction - _serverJurisdictions->lockForRead(); - if ((*_serverJurisdictions).find(nodeUUID) == (*_serverJurisdictions).end()) { - atLeastOneJurisdictionMissing = true; - } - _serverJurisdictions->unlock(); + _serverJurisdictions->withReadLock([&] { + if ((*_serverJurisdictions).find(nodeUUID) == (*_serverJurisdictions).end()) { + atLeastOneJurisdictionMissing = true; + } + }); } hasServers = true; } @@ -178,10 +178,10 @@ void OctreeEditPacketSender::queuePacketToNodes(std::unique_ptr packet bool isMyJurisdiction = true; // we need to get the jurisdiction for this // here we need to get the "pending packet" for this server - _serverJurisdictions->lockForRead(); - const JurisdictionMap& map = (*_serverJurisdictions)[nodeUUID]; - isMyJurisdiction = (map.isMyJurisdiction(octCode, CHECK_NODE_ONLY) == JurisdictionMap::WITHIN); - _serverJurisdictions->unlock(); + _serverJurisdictions->withReadLock([&] { + const JurisdictionMap& map = (*_serverJurisdictions)[nodeUUID]; + isMyJurisdiction = (map.isMyJurisdiction(octCode, CHECK_NODE_ONLY) == JurisdictionMap::WITHIN); + }); if (isMyJurisdiction) { // make a copy of this packet for this node and queue @@ -236,15 +236,15 @@ void OctreeEditPacketSender::queueOctreeEditMessage(PacketType type, QByteArray& } else if (_serverJurisdictions) { // we need to get the jurisdiction for this // here we need to get the "pending packet" for this server - _serverJurisdictions->lockForRead(); - if ((*_serverJurisdictions).find(nodeUUID) != (*_serverJurisdictions).end()) { - const JurisdictionMap& map = (*_serverJurisdictions)[nodeUUID]; - isMyJurisdiction = (map.isMyJurisdiction(reinterpret_cast(editMessage.data()), - CHECK_NODE_ONLY) == JurisdictionMap::WITHIN); - } else { - isMyJurisdiction = false; - } - _serverJurisdictions->unlock(); + _serverJurisdictions->withReadLock([&] { + if ((*_serverJurisdictions).find(nodeUUID) != (*_serverJurisdictions).end()) { + const JurisdictionMap& map = (*_serverJurisdictions)[nodeUUID]; + isMyJurisdiction = (map.isMyJurisdiction(reinterpret_cast(editMessage.data()), + CHECK_NODE_ONLY) == JurisdictionMap::WITHIN); + } else { + isMyJurisdiction = false; + } + }); } if (isMyJurisdiction) { std::unique_ptr& bufferedPacket = _pendingEditPackets[nodeUUID]; diff --git a/libraries/octree/src/OctreeHeadlessViewer.cpp b/libraries/octree/src/OctreeHeadlessViewer.cpp index 529328a8c0..3a80180008 100644 --- a/libraries/octree/src/OctreeHeadlessViewer.cpp +++ b/libraries/octree/src/OctreeHeadlessViewer.cpp @@ -43,11 +43,11 @@ void OctreeHeadlessViewer::queryOctree() { qCDebug(octree) << "---------------"; qCDebug(octree) << "_jurisdictionListener=" << _jurisdictionListener; qCDebug(octree) << "Jurisdictions..."; - jurisdictions.lockForRead(); - for (NodeToJurisdictionMapIterator i = jurisdictions.begin(); i != jurisdictions.end(); ++i) { - qCDebug(octree) << i.key() << ": " << &i.value(); - } - jurisdictions.unlock(); + jurisdictions.withReadLock([&] { + for (NodeToJurisdictionMapIterator i = jurisdictions.begin(); i != jurisdictions.end(); ++i) { + qCDebug(octree) << i.key() << ": " << &i.value(); + } + }); qCDebug(octree) << "---------------"; } @@ -84,28 +84,30 @@ void OctreeHeadlessViewer::queryOctree() { // if we haven't heard from this voxel server, go ahead and send it a query, so we // can get the jurisdiction... - jurisdictions.lockForRead(); - if (jurisdictions.find(nodeUUID) == jurisdictions.end()) { - jurisdictions.unlock(); - unknownJurisdictionServers++; - } else { + VoxelPositionSize rootDetails; + bool foundRootDetails = false; + jurisdictions.withReadLock([&] { + if (jurisdictions.find(nodeUUID) == jurisdictions.end()) { + unknownJurisdictionServers++; + return; + } const JurisdictionMap& map = (jurisdictions)[nodeUUID]; unsigned char* rootCode = map.getRootOctalCode(); + if (!rootCode) { + return; + } - if (rootCode) { - VoxelPositionSize rootDetails; - voxelDetailsForCode(rootCode, rootDetails); - jurisdictions.unlock(); - AACube serverBounds(glm::vec3(rootDetails.x, rootDetails.y, rootDetails.z), rootDetails.s); + voxelDetailsForCode(rootCode, rootDetails); + foundRootDetails = true; + }); - ViewFrustum::location serverFrustumLocation = _viewFrustum.cubeInFrustum(serverBounds); + if (foundRootDetails) { + AACube serverBounds(glm::vec3(rootDetails.x, rootDetails.y, rootDetails.z), rootDetails.s); + ViewFrustum::location serverFrustumLocation = _viewFrustum.cubeInFrustum(serverBounds); - if (serverFrustumLocation != ViewFrustum::OUTSIDE) { - inViewServers++; - } - } else { - jurisdictions.unlock(); + if (serverFrustumLocation != ViewFrustum::OUTSIDE) { + inViewServers++; } } } @@ -149,35 +151,38 @@ void OctreeHeadlessViewer::queryOctree() { // if we haven't heard from this voxel server, go ahead and send it a query, so we // can get the jurisdiction... - jurisdictions.lockForRead(); - if (jurisdictions.find(nodeUUID) == jurisdictions.end()) { - jurisdictions.unlock(); - unknownView = true; // assume it's in view - if (wantExtraDebugging) { - qCDebug(octree) << "no known jurisdiction for node " << *node << ", assume it's visible."; + VoxelPositionSize rootDetails; + bool foundRootDetails = false; + jurisdictions.withReadLock([&] { + if (jurisdictions.find(nodeUUID) == jurisdictions.end()) { + unknownView = true; // assume it's in view + if (wantExtraDebugging) { + qCDebug(octree) << "no known jurisdiction for node " << *node << ", assume it's visible."; + } + return; } - } else { - const JurisdictionMap& map = (jurisdictions)[nodeUUID]; + const JurisdictionMap& map = (jurisdictions)[nodeUUID]; unsigned char* rootCode = map.getRootOctalCode(); - if (rootCode) { - VoxelPositionSize rootDetails; - voxelDetailsForCode(rootCode, rootDetails); - jurisdictions.unlock(); - AACube serverBounds(glm::vec3(rootDetails.x, rootDetails.y, rootDetails.z), rootDetails.s); - - ViewFrustum::location serverFrustumLocation = _viewFrustum.cubeInFrustum(serverBounds); - if (serverFrustumLocation != ViewFrustum::OUTSIDE) { - inView = true; - } else { - inView = false; - } - } else { - jurisdictions.unlock(); + if (!rootCode) { if (wantExtraDebugging) { qCDebug(octree) << "Jurisdiction without RootCode for node " << *node << ". That's unusual!"; } + return; + } + voxelDetailsForCode(rootCode, rootDetails); + foundRootDetails = true; + }); + + if (foundRootDetails) { + AACube serverBounds(glm::vec3(rootDetails.x, rootDetails.y, rootDetails.z), rootDetails.s); + + ViewFrustum::location serverFrustumLocation = _viewFrustum.cubeInFrustum(serverBounds); + if (serverFrustumLocation != ViewFrustum::OUTSIDE) { + inView = true; + } else { + inView = false; } } diff --git a/libraries/octree/src/OctreePersistThread.cpp b/libraries/octree/src/OctreePersistThread.cpp index bb6f62ef84..d700e19ac2 100644 --- a/libraries/octree/src/OctreePersistThread.cpp +++ b/libraries/octree/src/OctreePersistThread.cpp @@ -127,18 +127,17 @@ bool OctreePersistThread::process() { bool persistantFileRead; - _tree->lockForWrite(); - { + _tree->withWriteLock([&] { PerformanceWarning warn(true, "Loading Octree File", true); - + // First check to make sure "lock" file doesn't exist. If it does exist, then // our last save crashed during the save, and we want to load our most recent backup. QString lockFileName = _filename + ".lock"; - std::ifstream lockFile(qPrintable(lockFileName), std::ios::in|std::ios::binary|std::ios::ate); - if(lockFile.is_open()) { + std::ifstream lockFile(qPrintable(lockFileName), std::ios::in | std::ios::binary | std::ios::ate); + if (lockFile.is_open()) { qCDebug(octree) << "WARNING: Octree lock file detected at startup:" << lockFileName - << "-- Attempting to restore from previous backup file."; - + << "-- Attempting to restore from previous backup file."; + // This is where we should attempt to find the most recent backup and restore from // that file as our persist file. restoreFromMostRecentBackup(); @@ -151,8 +150,7 @@ bool OctreePersistThread::process() { persistantFileRead = _tree->readFromFile(qPrintable(_filename.toLocal8Bit())); _tree->pruneTree(); - } - _tree->unlock(); + }); quint64 loadDone = usecTimestampNow(); _loadTimeUSecs = loadDone - loadStarted; @@ -233,13 +231,12 @@ void OctreePersistThread::aboutToFinish() { void OctreePersistThread::persist() { if (_tree->isDirty()) { - _tree->lockForWrite(); - { + + _tree->withWriteLock([&] { qCDebug(octree) << "pruning Octree before saving..."; _tree->pruneTree(); qCDebug(octree) << "DONE pruning Octree before saving..."; - } - _tree->unlock(); + }); qCDebug(octree) << "persist operation calling backup..."; backup(); // handle backup if requested diff --git a/libraries/octree/src/OctreeRenderer.cpp b/libraries/octree/src/OctreeRenderer.cpp index ce9bd1e3fd..07cbcf3ec4 100644 --- a/libraries/octree/src/OctreeRenderer.cpp +++ b/libraries/octree/src/OctreeRenderer.cpp @@ -124,35 +124,35 @@ void OctreeRenderer::processDatagram(NLPacket& packet, SharedNodePointer sourceN // ask the VoxelTree to read the bitstream into the tree ReadBitstreamToTreeParams args(packetIsColored ? WANT_COLOR : NO_COLOR, WANT_EXISTS_BITS, NULL, sourceUUID, sourceNode, false, packet.getVersion()); - quint64 startLock = usecTimestampNow(); - + quint64 startUncompress, startLock = usecTimestampNow(); + quint64 startReadBitsteam, endReadBitsteam; // FIXME STUTTER - there may be an opportunity to bump this lock outside of the // loop to reduce the amount of locking/unlocking we're doing - _tree->lockForWrite(); - quint64 startUncompress = usecTimestampNow(); - - OctreePacketData packetData(packetIsCompressed); - packetData.loadFinalizedContent(reinterpret_cast(packet.getPayload() + packet.pos()), - sectionLength); - if (extraDebugging) { - qCDebug(octree, "OctreeRenderer::processDatagram() ... Got Packet Section" - " color:%s compressed:%s sequence: %u flight:%d usec size:%lld data:%lld" - " subsection:%d sectionLength:%d uncompressed:%d", - debug::valueOf(packetIsColored), debug::valueOf(packetIsCompressed), - sequence, flightTime, packet.getDataSize(), packet.bytesLeftToRead(), subsection, sectionLength, - packetData.getUncompressedSize()); - } - - if (extraDebugging) { - qCDebug(octree) << "OctreeRenderer::processDatagram() ******* START _tree->readBitstreamToTree()..."; - } - quint64 startReadBitsteam = usecTimestampNow(); - _tree->readBitstreamToTree(packetData.getUncompressedData(), packetData.getUncompressedSize(), args); - quint64 endReadBitsteam = usecTimestampNow(); - if (extraDebugging) { - qCDebug(octree) << "OctreeRenderer::processDatagram() ******* END _tree->readBitstreamToTree()..."; - } - _tree->unlock(); + _tree->withWriteLock([&] { + startUncompress = usecTimestampNow(); + + OctreePacketData packetData(packetIsCompressed); + packetData.loadFinalizedContent(reinterpret_cast(packet.getPayload() + packet.pos()), + sectionLength); + if (extraDebugging) { + qCDebug(octree, "OctreeRenderer::processDatagram() ... Got Packet Section" + " color:%s compressed:%s sequence: %u flight:%d usec size:%lld data:%lld" + " subsection:%d sectionLength:%d uncompressed:%d", + debug::valueOf(packetIsColored), debug::valueOf(packetIsCompressed), + sequence, flightTime, packet.getDataSize(), packet.bytesLeftToRead(), subsection, sectionLength, + packetData.getUncompressedSize()); + } + + if (extraDebugging) { + qCDebug(octree) << "OctreeRenderer::processDatagram() ******* START _tree->readBitstreamToTree()..."; + } + startReadBitsteam = usecTimestampNow(); + _tree->readBitstreamToTree(packetData.getUncompressedData(), packetData.getUncompressedSize(), args); + endReadBitsteam = usecTimestampNow(); + if (extraDebugging) { + qCDebug(octree) << "OctreeRenderer::processDatagram() ******* END _tree->readBitstreamToTree()..."; + } + }); // seek forwards in packet packet.seek(packet.pos() + sectionLength); @@ -218,17 +218,17 @@ bool OctreeRenderer::renderOperation(OctreeElement* element, void* extraData) { void OctreeRenderer::render(RenderArgs* renderArgs) { if (_tree) { renderArgs->_renderer = this; - _tree->lockForRead(); - _tree->recurseTreeWithOperation(renderOperation, renderArgs); - _tree->unlock(); + _tree->withReadLock([&] { + _tree->recurseTreeWithOperation(renderOperation, renderArgs); + }); } } void OctreeRenderer::clear() { if (_tree) { - _tree->lockForWrite(); - _tree->eraseAllOctreeElements(); - _tree->unlock(); + _tree->withWriteLock([&] { + _tree->eraseAllOctreeElements(); + }); } } diff --git a/libraries/octree/src/OctreeSceneStats.h b/libraries/octree/src/OctreeSceneStats.h index d840c0d6f6..45a7b2718a 100644 --- a/libraries/octree/src/OctreeSceneStats.h +++ b/libraries/octree/src/OctreeSceneStats.h @@ -15,6 +15,7 @@ #include #include +#include #include "JurisdictionMap.h" #include "OctreePacketData.h" @@ -281,7 +282,7 @@ private: /// Map between element IDs and their reported OctreeSceneStats. Typically used by classes that need to know which elements sent /// which octree stats -typedef std::map NodeToOctreeSceneStats; -typedef std::map::iterator NodeToOctreeSceneStatsIterator; +class NodeToOctreeSceneStats : public std::map, public ReadWriteLockable {}; +typedef NodeToOctreeSceneStats::iterator NodeToOctreeSceneStatsIterator; #endif // hifi_OctreeSceneStats_h diff --git a/libraries/physics/src/EntityMotionState.cpp b/libraries/physics/src/EntityMotionState.cpp index dc3c0ea4e8..c96d259914 100644 --- a/libraries/physics/src/EntityMotionState.cpp +++ b/libraries/physics/src/EntityMotionState.cpp @@ -33,22 +33,10 @@ const quint64 USECS_BETWEEN_OWNERSHIP_BIDS = USECS_PER_SECOND / 5; bool EntityMotionState::entityTreeIsLocked() const { EntityTreeElement* element = _entity ? _entity->getElement() : nullptr; EntityTree* tree = element ? element->getTree() : nullptr; - if (tree) { - bool readSuccess = tree->tryLockForRead(); - if (readSuccess) { - tree->unlock(); - } - bool writeSuccess = tree->tryLockForWrite(); - if (writeSuccess) { - tree->unlock(); - } - if (readSuccess && writeSuccess) { - return false; // if we can take either kind of lock, there was no tree lock. - } - return true; // either read or write failed, so there is some lock in place. - } else { + if (!tree) { return true; } + return tree->isLocked(); } #else bool entityTreeIsLocked() { diff --git a/libraries/physics/src/ObjectAction.h b/libraries/physics/src/ObjectAction.h index f27ed9ab07..4d91d0dbb1 100644 --- a/libraries/physics/src/ObjectAction.h +++ b/libraries/physics/src/ObjectAction.h @@ -17,12 +17,14 @@ #include +#include + #include "ObjectMotionState.h" #include "BulletUtil.h" #include "EntityActionInterface.h" -class ObjectAction : public btActionInterface, public EntityActionInterface { +class ObjectAction : public btActionInterface, public EntityActionInterface, public ReadWriteLockable { public: ObjectAction(EntityActionType type, const QUuid& id, EntityItemPointer ownerEntity); virtual ~ObjectAction(); @@ -57,14 +59,7 @@ protected: virtual void setAngularVelocity(glm::vec3 angularVelocity); virtual void activateBody(); - void lockForRead() { _lock.lockForRead(); } - bool tryLockForRead() { return _lock.tryLockForRead(); } - void lockForWrite() { _lock.lockForWrite(); } - bool tryLockForWrite() { return _lock.tryLockForWrite(); } - void unlock() { _lock.unlock(); } - private: - QReadWriteLock _lock; protected: bool _active; diff --git a/libraries/physics/src/ObjectActionOffset.cpp b/libraries/physics/src/ObjectActionOffset.cpp index a00bbbd418..2c1b391ba5 100644 --- a/libraries/physics/src/ObjectActionOffset.cpp +++ b/libraries/physics/src/ObjectActionOffset.cpp @@ -33,56 +33,49 @@ ObjectActionOffset::~ObjectActionOffset() { } void ObjectActionOffset::updateActionWorker(btScalar deltaTimeStep) { - if (!tryLockForRead()) { - // don't risk hanging the thread running the physics simulation - return; - } - - auto ownerEntity = _ownerEntity.lock(); - if (!ownerEntity) { - return; - } - - void* physicsInfo = ownerEntity->getPhysicsInfo(); - if (!physicsInfo) { - unlock(); - return; - } - - ObjectMotionState* motionState = static_cast(physicsInfo); - btRigidBody* rigidBody = motionState->getRigidBody(); - if (!rigidBody) { - unlock(); - qDebug() << "ObjectActionOffset::updateActionWorker no rigidBody"; - return; - } - - const float MAX_LINEAR_TIMESCALE = 600.0f; // 10 minutes is a long time - if (_positionalTargetSet && _linearTimeScale < MAX_LINEAR_TIMESCALE) { - glm::vec3 objectPosition = bulletToGLM(rigidBody->getCenterOfMassPosition()); - glm::vec3 springAxis = objectPosition - _pointToOffsetFrom; // from anchor to object - float distance = glm::length(springAxis); - if (distance > FLT_EPSILON) { - springAxis /= distance; // normalize springAxis - - // compute (critically damped) target velocity of spring relaxation - glm::vec3 offset = (distance - _linearDistance) * springAxis; - glm::vec3 targetVelocity = (-1.0f / _linearTimeScale) * offset; - - // compute current velocity and its parallel component - glm::vec3 currentVelocity = bulletToGLM(rigidBody->getLinearVelocity()); - glm::vec3 parallelVelocity = glm::dot(currentVelocity, springAxis) * springAxis; - - // we blend the parallel component with the spring's target velocity to get the new velocity - float blend = deltaTimeStep / _linearTimeScale; - if (blend > 1.0f) { - blend = 1.0f; - } - rigidBody->setLinearVelocity(glmToBullet(currentVelocity + blend * (targetVelocity - parallelVelocity))); + withTryReadLock([&]{ + auto ownerEntity = _ownerEntity.lock(); + if (!ownerEntity) { + return; } - } - unlock(); + void* physicsInfo = ownerEntity->getPhysicsInfo(); + if (!physicsInfo) { + return; + } + + ObjectMotionState* motionState = static_cast(physicsInfo); + btRigidBody* rigidBody = motionState->getRigidBody(); + if (!rigidBody) { + qDebug() << "ObjectActionOffset::updateActionWorker no rigidBody"; + return; + } + + const float MAX_LINEAR_TIMESCALE = 600.0f; // 10 minutes is a long time + if (_positionalTargetSet && _linearTimeScale < MAX_LINEAR_TIMESCALE) { + glm::vec3 objectPosition = bulletToGLM(rigidBody->getCenterOfMassPosition()); + glm::vec3 springAxis = objectPosition - _pointToOffsetFrom; // from anchor to object + float distance = glm::length(springAxis); + if (distance > FLT_EPSILON) { + springAxis /= distance; // normalize springAxis + + // compute (critically damped) target velocity of spring relaxation + glm::vec3 offset = (distance - _linearDistance) * springAxis; + glm::vec3 targetVelocity = (-1.0f / _linearTimeScale) * offset; + + // compute current velocity and its parallel component + glm::vec3 currentVelocity = bulletToGLM(rigidBody->getLinearVelocity()); + glm::vec3 parallelVelocity = glm::dot(currentVelocity, springAxis) * springAxis; + + // we blend the parallel component with the spring's target velocity to get the new velocity + float blend = deltaTimeStep / _linearTimeScale; + if (blend > 1.0f) { + blend = 1.0f; + } + rigidBody->setLinearVelocity(glmToBullet(currentVelocity + blend * (targetVelocity - parallelVelocity))); + } + } + }); } @@ -112,25 +105,26 @@ bool ObjectActionOffset::updateArguments(QVariantMap arguments) { if (_pointToOffsetFrom != pointToOffsetFrom || _linearTimeScale != linearTimeScale || _linearDistance != linearDistance) { - lockForWrite(); - _pointToOffsetFrom = pointToOffsetFrom; - _linearTimeScale = linearTimeScale; - _linearDistance = linearDistance; - _positionalTargetSet = true; - _active = true; - activateBody(); - unlock(); + + withWriteLock([&] { + _pointToOffsetFrom = pointToOffsetFrom; + _linearTimeScale = linearTimeScale; + _linearDistance = linearDistance; + _positionalTargetSet = true; + _active = true; + activateBody(); + }); } return true; } QVariantMap ObjectActionOffset::getArguments() { QVariantMap arguments; - lockForRead(); - arguments["pointToOffsetFrom"] = glmToQMap(_pointToOffsetFrom); - arguments["linearTimeScale"] = _linearTimeScale; - arguments["linearDistance"] = _linearDistance; - unlock(); + withReadLock([&] { + arguments["pointToOffsetFrom"] = glmToQMap(_pointToOffsetFrom); + arguments["linearTimeScale"] = _linearTimeScale; + arguments["linearDistance"] = _linearDistance; + }); return arguments; } diff --git a/libraries/physics/src/ObjectActionSpring.cpp b/libraries/physics/src/ObjectActionSpring.cpp index 89e1d58dff..d163933299 100644 --- a/libraries/physics/src/ObjectActionSpring.cpp +++ b/libraries/physics/src/ObjectActionSpring.cpp @@ -38,74 +38,72 @@ ObjectActionSpring::~ObjectActionSpring() { } void ObjectActionSpring::updateActionWorker(btScalar deltaTimeStep) { - if (!tryLockForRead()) { + auto lockResult = withTryReadLock([&]{ + auto ownerEntity = _ownerEntity.lock(); + if (!ownerEntity) { + return; + } + + void* physicsInfo = ownerEntity->getPhysicsInfo(); + if (!physicsInfo) { + return; + } + ObjectMotionState* motionState = static_cast(physicsInfo); + btRigidBody* rigidBody = motionState->getRigidBody(); + if (!rigidBody) { + qDebug() << "ObjectActionSpring::updateActionWorker no rigidBody"; + return; + } + + const float MAX_TIMESCALE = 600.0f; // 10 min is a long time + if (_linearTimeScale < MAX_TIMESCALE) { + btVector3 offset = rigidBody->getCenterOfMassPosition() - glmToBullet(_positionalTarget); + float offsetLength = offset.length(); + btVector3 targetVelocity(0.0f, 0.0f, 0.0f); + + if (offsetLength > 0) { + float speed = (offsetLength > FLT_EPSILON) ? glm::min(offsetLength / _linearTimeScale, SPRING_MAX_SPEED) : 0.0f; + targetVelocity = (-speed / offsetLength) * offset; + } + + // this action is aggresively critically damped and defeats the current velocity + rigidBody->setLinearVelocity(targetVelocity); + } + + if (_angularTimeScale < MAX_TIMESCALE) { + btVector3 targetVelocity(0.0f, 0.0f, 0.0f); + + btQuaternion bodyRotation = rigidBody->getOrientation(); + auto alignmentDot = bodyRotation.dot(glmToBullet(_rotationalTarget)); + const float ALMOST_ONE = 0.99999f; + if (glm::abs(alignmentDot) < ALMOST_ONE) { + btQuaternion target = glmToBullet(_rotationalTarget); + if (alignmentDot < 0.0f) { + target = -target; + } + // if dQ is the incremental rotation that gets an object from Q0 to Q1 then: + // + // Q1 = dQ * Q0 + // + // solving for dQ gives: + // + // dQ = Q1 * Q0^ + btQuaternion deltaQ = target * bodyRotation.inverse(); + float angle = deltaQ.getAngle(); + const float MIN_ANGLE = 1.0e-4f; + if (angle > MIN_ANGLE) { + targetVelocity = (angle / _angularTimeScale) * deltaQ.getAxis(); + } + } + // this action is aggresively critically damped and defeats the current velocity + rigidBody->setAngularVelocity(targetVelocity); + } + }); + if (!lockResult) { // don't risk hanging the thread running the physics simulation qDebug() << "ObjectActionSpring::updateActionWorker lock failed"; return; } - - auto ownerEntity = _ownerEntity.lock(); - if (!ownerEntity) { - return; - } - - void* physicsInfo = ownerEntity->getPhysicsInfo(); - if (!physicsInfo) { - unlock(); - return; - } - ObjectMotionState* motionState = static_cast(physicsInfo); - btRigidBody* rigidBody = motionState->getRigidBody(); - if (!rigidBody) { - unlock(); - qDebug() << "ObjectActionSpring::updateActionWorker no rigidBody"; - return; - } - - const float MAX_TIMESCALE = 600.0f; // 10 min is a long time - if (_linearTimeScale < MAX_TIMESCALE) { - btVector3 offset = rigidBody->getCenterOfMassPosition() - glmToBullet(_positionalTarget); - float offsetLength = offset.length(); - btVector3 targetVelocity(0.0f, 0.0f, 0.0f); - - if (offsetLength > 0) { - float speed = (offsetLength > FLT_EPSILON) ? glm::min(offsetLength / _linearTimeScale, SPRING_MAX_SPEED) : 0.0f; - targetVelocity = (-speed / offsetLength) * offset; - } - - // this action is aggresively critically damped and defeats the current velocity - rigidBody->setLinearVelocity(targetVelocity); - } - - if (_angularTimeScale < MAX_TIMESCALE) { - btVector3 targetVelocity(0.0f, 0.0f, 0.0f); - - btQuaternion bodyRotation = rigidBody->getOrientation(); - auto alignmentDot = bodyRotation.dot(glmToBullet(_rotationalTarget)); - const float ALMOST_ONE = 0.99999f; - if (glm::abs(alignmentDot) < ALMOST_ONE) { - btQuaternion target = glmToBullet(_rotationalTarget); - if (alignmentDot < 0.0f) { - target = -target; - } - // if dQ is the incremental rotation that gets an object from Q0 to Q1 then: - // - // Q1 = dQ * Q0 - // - // solving for dQ gives: - // - // dQ = Q1 * Q0^ - btQuaternion deltaQ = target * bodyRotation.inverse(); - float angle = deltaQ.getAngle(); - const float MIN_ANGLE = 1.0e-4f; - if (angle > MIN_ANGLE) { - targetVelocity = (angle / _angularTimeScale) * deltaQ.getAxis(); - } - } - // this action is aggresively critically damped and defeats the current velocity - rigidBody->setAngularVelocity(targetVelocity); - } - unlock(); } const float MIN_TIMESCALE = 0.1f; @@ -144,29 +142,27 @@ bool ObjectActionSpring::updateArguments(QVariantMap arguments) { || rotationalTarget != _rotationalTarget || angularTimeScale != _angularTimeScale) { // something changed - lockForWrite(); - _positionalTarget = positionalTarget; - _linearTimeScale = glm::max(MIN_TIMESCALE, glm::abs(linearTimeScale)); - _rotationalTarget = rotationalTarget; - _angularTimeScale = glm::max(MIN_TIMESCALE, glm::abs(angularTimeScale)); - _active = true; - activateBody(); - unlock(); + withWriteLock([&] { + _positionalTarget = positionalTarget; + _linearTimeScale = glm::max(MIN_TIMESCALE, glm::abs(linearTimeScale)); + _rotationalTarget = rotationalTarget; + _angularTimeScale = glm::max(MIN_TIMESCALE, glm::abs(angularTimeScale)); + _active = true; + activateBody(); + }); } return true; } QVariantMap ObjectActionSpring::getArguments() { QVariantMap arguments; - lockForRead(); + withReadLock([&] { + arguments["linearTimeScale"] = _linearTimeScale; + arguments["targetPosition"] = glmToQMap(_positionalTarget); - arguments["linearTimeScale"] = _linearTimeScale; - arguments["targetPosition"] = glmToQMap(_positionalTarget); - - arguments["targetRotation"] = glmToQMap(_rotationalTarget); - arguments["angularTimeScale"] = _angularTimeScale; - - unlock(); + arguments["targetRotation"] = glmToQMap(_rotationalTarget); + arguments["angularTimeScale"] = _angularTimeScale; + }); return arguments; } diff --git a/libraries/physics/src/PhysicalEntitySimulation.cpp b/libraries/physics/src/PhysicalEntitySimulation.cpp index 97cf6a549a..bee2a4d83d 100644 --- a/libraries/physics/src/PhysicalEntitySimulation.cpp +++ b/libraries/physics/src/PhysicalEntitySimulation.cpp @@ -121,8 +121,9 @@ void PhysicalEntitySimulation::clearEntitiesInternal() { // end EntitySimulation overrides -VectorOfMotionStates& PhysicalEntitySimulation::getObjectsToDelete() { - _tempVector.clear(); +void PhysicalEntitySimulation::getObjectsToDelete(VectorOfMotionStates& result) { + result.clear(); + QMutexLocker lock(&_mutex); for (auto stateItr : _pendingRemoves) { EntityMotionState* motionState = &(*stateItr); _pendingChanges.remove(motionState); @@ -134,14 +135,14 @@ VectorOfMotionStates& PhysicalEntitySimulation::getObjectsToDelete() { entity->setPhysicsInfo(nullptr); motionState->clearObjectBackPointer(); } - _tempVector.push_back(motionState); + result.push_back(motionState); } _pendingRemoves.clear(); - return _tempVector; } -VectorOfMotionStates& PhysicalEntitySimulation::getObjectsToAdd() { - _tempVector.clear(); +void PhysicalEntitySimulation::getObjectsToAdd(VectorOfMotionStates& result) { + result.clear(); + QMutexLocker lock(&_mutex); SetOfEntities::iterator entityItr = _pendingAdds.begin(); while (entityItr != _pendingAdds.end()) { EntityItemPointer entity = *entityItr; @@ -160,7 +161,7 @@ VectorOfMotionStates& PhysicalEntitySimulation::getObjectsToAdd() { EntityMotionState* motionState = new EntityMotionState(shape, entity); entity->setPhysicsInfo(static_cast(motionState)); _physicalObjects.insert(motionState); - _tempVector.push_back(motionState); + result.push_back(motionState); entityItr = _pendingAdds.erase(entityItr); } else { //qDebug() << "Warning! Failed to generate new shape for entity." << entity->getName(); @@ -170,26 +171,27 @@ VectorOfMotionStates& PhysicalEntitySimulation::getObjectsToAdd() { ++entityItr; } } - return _tempVector; } -void PhysicalEntitySimulation::setObjectsToChange(VectorOfMotionStates& objectsToChange) { +void PhysicalEntitySimulation::setObjectsToChange(const VectorOfMotionStates& objectsToChange) { + QMutexLocker lock(&_mutex); for (auto object : objectsToChange) { _pendingChanges.insert(static_cast(object)); } } -VectorOfMotionStates& PhysicalEntitySimulation::getObjectsToChange() { - _tempVector.clear(); +void PhysicalEntitySimulation::getObjectsToChange(VectorOfMotionStates& result) { + result.clear(); + QMutexLocker lock(&_mutex); for (auto stateItr : _pendingChanges) { EntityMotionState* motionState = &(*stateItr); - _tempVector.push_back(motionState); + result.push_back(motionState); } _pendingChanges.clear(); - return _tempVector; } -void PhysicalEntitySimulation::handleOutgoingChanges(VectorOfMotionStates& motionStates, const QUuid& sessionID) { +void PhysicalEntitySimulation::handleOutgoingChanges(const VectorOfMotionStates& motionStates, const QUuid& sessionID) { + QMutexLocker lock(&_mutex); // walk the motionStates looking for those that correspond to entities for (auto stateItr : motionStates) { ObjectMotionState* state = &(*stateItr); @@ -231,7 +233,7 @@ void PhysicalEntitySimulation::handleOutgoingChanges(VectorOfMotionStates& motio } } -void PhysicalEntitySimulation::handleCollisionEvents(CollisionEvents& collisionEvents) { +void PhysicalEntitySimulation::handleCollisionEvents(const CollisionEvents& collisionEvents) { for (auto collision : collisionEvents) { // NOTE: The collision event is always aligned such that idA is never NULL. // however idB may be NULL. @@ -244,29 +246,28 @@ void PhysicalEntitySimulation::handleCollisionEvents(CollisionEvents& collisionE void PhysicalEntitySimulation::addAction(EntityActionPointer action) { if (_physicsEngine) { - lock(); + // FIXME put fine grain locking into _physicsEngine + QMutexLocker lock(&_mutex); const QUuid& actionID = action->getID(); if (_physicsEngine->getActionByID(actionID)) { qDebug() << "warning -- PhysicalEntitySimulation::addAction -- adding an action that was already in _physicsEngine"; } - unlock(); EntitySimulation::addAction(action); } } void PhysicalEntitySimulation::applyActionChanges() { if (_physicsEngine) { - lock(); - foreach (QUuid actionToRemove, _actionsToRemove) { + // FIXME put fine grain locking into _physicsEngine + QMutexLocker lock(&_mutex); + foreach(QUuid actionToRemove, _actionsToRemove) { _physicsEngine->removeAction(actionToRemove); } - _actionsToRemove.clear(); foreach (EntityActionPointer actionToAdd, _actionsToAdd) { if (!_actionsToRemove.contains(actionToAdd->getID())) { _physicsEngine->addAction(actionToAdd); } } - _actionsToAdd.clear(); - unlock(); } + EntitySimulation::applyActionChanges(); } diff --git a/libraries/physics/src/PhysicalEntitySimulation.h b/libraries/physics/src/PhysicalEntitySimulation.h index 7599c7d1b5..8fe77b6343 100644 --- a/libraries/physics/src/PhysicalEntitySimulation.h +++ b/libraries/physics/src/PhysicalEntitySimulation.h @@ -32,25 +32,25 @@ public: void init(EntityTree* tree, PhysicsEngine* engine, EntityEditPacketSender* packetSender); - virtual void addAction(EntityActionPointer action); - virtual void applyActionChanges(); + virtual void addAction(EntityActionPointer action) override; + virtual void applyActionChanges() override; protected: // only called by EntitySimulation // overrides for EntitySimulation - virtual void updateEntitiesInternal(const quint64& now); - virtual void addEntityInternal(EntityItemPointer entity); - virtual void removeEntityInternal(EntityItemPointer entity); - virtual void changeEntityInternal(EntityItemPointer entity); - virtual void clearEntitiesInternal(); + virtual void updateEntitiesInternal(const quint64& now) override; + virtual void addEntityInternal(EntityItemPointer entity) override; + virtual void removeEntityInternal(EntityItemPointer entity) override; + virtual void changeEntityInternal(EntityItemPointer entity) override; + virtual void clearEntitiesInternal() override; public: - VectorOfMotionStates& getObjectsToDelete(); - VectorOfMotionStates& getObjectsToAdd(); - void setObjectsToChange(VectorOfMotionStates& objectsToChange); - VectorOfMotionStates& getObjectsToChange(); + void getObjectsToDelete(VectorOfMotionStates& result); + void getObjectsToAdd(VectorOfMotionStates& result); + void setObjectsToChange(const VectorOfMotionStates& objectsToChange); + void getObjectsToChange(VectorOfMotionStates& result); - void handleOutgoingChanges(VectorOfMotionStates& motionStates, const QUuid& sessionID); - void handleCollisionEvents(CollisionEvents& collisionEvents); + void handleOutgoingChanges(const VectorOfMotionStates& motionStates, const QUuid& sessionID); + void handleCollisionEvents(const CollisionEvents& collisionEvents); EntityEditPacketSender* getPacketSender() { return _entityPacketSender; } @@ -64,7 +64,6 @@ private: SetOfEntityMotionStates _outgoingChanges; // EntityMotionStates for which we need to send updates to entity-server SetOfMotionStates _physicalObjects; // MotionStates of entities in PhysicsEngine - VectorOfMotionStates _tempVector; // temporary array reference, valid immediately after getObjectsToRemove() (and friends) PhysicsEngine* _physicsEngine = nullptr; EntityEditPacketSender* _entityPacketSender = nullptr; diff --git a/libraries/physics/src/PhysicsEngine.cpp b/libraries/physics/src/PhysicsEngine.cpp index 040055b313..f91cd9c0fb 100644 --- a/libraries/physics/src/PhysicsEngine.cpp +++ b/libraries/physics/src/PhysicsEngine.cpp @@ -153,7 +153,7 @@ void PhysicsEngine::removeObject(ObjectMotionState* object) { _dynamicsWorld->removeRigidBody(body); } -void PhysicsEngine::deleteObjects(VectorOfMotionStates& objects) { +void PhysicsEngine::deleteObjects(const VectorOfMotionStates& objects) { for (auto object : objects) { removeObject(object); @@ -168,7 +168,7 @@ void PhysicsEngine::deleteObjects(VectorOfMotionStates& objects) { } // Same as above, but takes a Set instead of a Vector. Should only be called during teardown. -void PhysicsEngine::deleteObjects(SetOfMotionStates& objects) { +void PhysicsEngine::deleteObjects(const SetOfMotionStates& objects) { for (auto object : objects) { btRigidBody* body = object->getRigidBody(); removeObject(object); @@ -182,13 +182,13 @@ void PhysicsEngine::deleteObjects(SetOfMotionStates& objects) { } } -void PhysicsEngine::addObjects(VectorOfMotionStates& objects) { +void PhysicsEngine::addObjects(const VectorOfMotionStates& objects) { for (auto object : objects) { addObject(object); } } -VectorOfMotionStates PhysicsEngine::changeObjects(VectorOfMotionStates& objects) { +VectorOfMotionStates PhysicsEngine::changeObjects(const VectorOfMotionStates& objects) { VectorOfMotionStates stillNeedChange; for (auto object : objects) { uint32_t flags = object->getIncomingDirtyFlags() & DIRTY_PHYSICS_FLAGS; @@ -331,7 +331,7 @@ void PhysicsEngine::updateContactMap() { } } -CollisionEvents& PhysicsEngine::getCollisionEvents() { +const CollisionEvents& PhysicsEngine::getCollisionEvents() { const uint32_t CONTINUE_EVENT_FILTER_FREQUENCY = 10; _collisionEvents.clear(); @@ -377,7 +377,7 @@ CollisionEvents& PhysicsEngine::getCollisionEvents() { return _collisionEvents; } -VectorOfMotionStates& PhysicsEngine::getOutgoingChanges() { +const VectorOfMotionStates& PhysicsEngine::getOutgoingChanges() { BT_PROFILE("copyOutgoingChanges"); _dynamicsWorld->synchronizeMotionStates(); _hasOutgoingChanges = false; diff --git a/libraries/physics/src/PhysicsEngine.h b/libraries/physics/src/PhysicsEngine.h index 67b38323cc..49501fc5e6 100644 --- a/libraries/physics/src/PhysicsEngine.h +++ b/libraries/physics/src/PhysicsEngine.h @@ -57,10 +57,10 @@ public: void addObject(ObjectMotionState* motionState); void removeObject(ObjectMotionState* motionState); - void deleteObjects(VectorOfMotionStates& objects); - void deleteObjects(SetOfMotionStates& objects); // only called during teardown - void addObjects(VectorOfMotionStates& objects); - VectorOfMotionStates changeObjects(VectorOfMotionStates& objects); + void deleteObjects(const VectorOfMotionStates& objects); + void deleteObjects(const SetOfMotionStates& objects); // only called during teardown + void addObjects(const VectorOfMotionStates& objects); + VectorOfMotionStates changeObjects(const VectorOfMotionStates& objects); void reinsertObject(ObjectMotionState* object); void stepSimulation(); @@ -69,10 +69,10 @@ public: bool hasOutgoingChanges() const { return _hasOutgoingChanges; } /// \return reference to list of changed MotionStates. The list is only valid until beginning of next simulation loop. - VectorOfMotionStates& getOutgoingChanges(); + const VectorOfMotionStates& getOutgoingChanges(); /// \return reference to list of Collision events. The list is only valid until beginning of next simulation loop. - CollisionEvents& getCollisionEvents(); + const CollisionEvents& getCollisionEvents(); /// \brief prints timings for last frame if stats have been requested. void dumpStatsIfNecessary(); diff --git a/libraries/shared/src/shared/ReadWriteLockable.cpp b/libraries/shared/src/shared/ReadWriteLockable.cpp new file mode 100644 index 0000000000..04b71cf64f --- /dev/null +++ b/libraries/shared/src/shared/ReadWriteLockable.cpp @@ -0,0 +1,10 @@ +// +// Created by Bradley Austin Davis on 2015/09/10 +// Copyright 2013-2015 High Fidelity, Inc. +// +// Distributed under the Apache License, Version 2.0. +// See the accompanying file LICENSE or http://www.apache.org/licenses/LICENSE-2.0.html +// + +#include "ReadWriteLockable.h" + diff --git a/libraries/shared/src/shared/ReadWriteLockable.h b/libraries/shared/src/shared/ReadWriteLockable.h new file mode 100644 index 0000000000..a7d30d562b --- /dev/null +++ b/libraries/shared/src/shared/ReadWriteLockable.h @@ -0,0 +1,62 @@ +// +// Created by Bradley Austin Davis on 2015/09/10 +// Copyright 2013-2015 High Fidelity, Inc. +// +// Distributed under the Apache License, Version 2.0. +// See the accompanying file LICENSE or http://www.apache.org/licenses/LICENSE-2.0.html +// + +#pragma once +#ifndef hifi_ReadWriteLockable_h +#define hifi_ReadWriteLockable_h +#include + +class ReadWriteLockable { +public: + template + bool withWriteLock(F f, bool require = true) { + if (!require) { + bool result = _lock.tryLockForWrite(); + if (result) { + f(); + _lock.unlock(); + } + return result; + } + + QWriteLocker locker(&_lock); + f(); + return true; + } + + template + bool withTryWriteLock(F f) { + return withWriteLock(f, false); + } + + template + bool withReadLock(F f, bool require = true) const { + if (!require) { + bool result = _lock.tryLockForRead(); + if (result) { + f(); + _lock.unlock(); + } + return result; + } + + QReadLocker locker(&_lock); + f(); + return true; + } + + template + bool withTryReadLock(F f) const { + return withReadLock(f, false); + } + +private: + mutable QReadWriteLock _lock{ QReadWriteLock::Recursive }; +}; + +#endif