From fd47602945d1a4103a8f9d79cecc5c494cdea710 Mon Sep 17 00:00:00 2001 From: ZappoMan Date: Mon, 3 Mar 2014 12:49:34 -0800 Subject: [PATCH 1/3] various particle crash and deadlock fixes --- interface/src/ParticleTreeRenderer.cpp | 5 +---- libraries/octree/src/OctreeElement.cpp | 2 +- libraries/particles/src/ParticleTree.cpp | 2 ++ libraries/particles/src/ParticlesScriptingInterface.h | 2 -- 4 files changed, 4 insertions(+), 7 deletions(-) diff --git a/interface/src/ParticleTreeRenderer.cpp b/interface/src/ParticleTreeRenderer.cpp index d9d0849c94..59784c8c1f 100644 --- a/interface/src/ParticleTreeRenderer.cpp +++ b/interface/src/ParticleTreeRenderer.cpp @@ -33,10 +33,7 @@ void ParticleTreeRenderer::init() { void ParticleTreeRenderer::update() { if (_tree) { ParticleTree* tree = static_cast(_tree); - if (tree->tryLockForWrite()) { - tree->update(); - tree->unlock(); - } + tree->update(); } } diff --git a/libraries/octree/src/OctreeElement.cpp b/libraries/octree/src/OctreeElement.cpp index 0cb1922df4..67b96b4047 100644 --- a/libraries/octree/src/OctreeElement.cpp +++ b/libraries/octree/src/OctreeElement.cpp @@ -739,7 +739,7 @@ void OctreeElement::setChildAtIndex(int childIndex, OctreeElement* child) { _externalChildrenMemoryUsage += NUMBER_OF_CHILDREN * sizeof(OctreeElement*); } else if (previousChildCount == 2 && newChildCount == 1) { - assert(child); // we are removing a child, so this must be true! + assert(!child); // we are removing a child, so this must be true! OctreeElement* previousFirstChild = _children.external[firstIndex]; OctreeElement* previousSecondChild = _children.external[secondIndex]; delete[] _children.external; diff --git a/libraries/particles/src/ParticleTree.cpp b/libraries/particles/src/ParticleTree.cpp index b422b8e4e8..e257237f7a 100644 --- a/libraries/particles/src/ParticleTree.cpp +++ b/libraries/particles/src/ParticleTree.cpp @@ -487,7 +487,9 @@ void ParticleTree::update() { } // prune the tree... + lockForWrite(); recurseTreeWithOperation(pruneOperation, NULL); + unlock(); } diff --git a/libraries/particles/src/ParticlesScriptingInterface.h b/libraries/particles/src/ParticlesScriptingInterface.h index 3aa19629f0..ee21424f11 100644 --- a/libraries/particles/src/ParticlesScriptingInterface.h +++ b/libraries/particles/src/ParticlesScriptingInterface.h @@ -30,12 +30,10 @@ public: private slots: /// inbound slots for external collision systems void forwardParticleCollisionWithVoxel(const ParticleID& particleID, const VoxelDetail& voxel) { - qDebug() << "forwardParticleCollisionWithVoxel()"; emit particleCollisionWithVoxel(particleID, voxel); } void forwardParticleCollisionWithParticle(const ParticleID& idA, const ParticleID& idB) { - qDebug() << "forwardParticleCollisionWithParticle()"; emit particleCollisionWithParticle(idA, idB); } From 98bf85173ae8271b62d877ccf3bb2017548c2ae2 Mon Sep 17 00:00:00 2001 From: ZappoMan Date: Mon, 3 Mar 2014 13:54:07 -0800 Subject: [PATCH 2/3] more octree locking cleanup --- libraries/octree/src/Octree.cpp | 10 ++++++++-- libraries/octree/src/Octree.h | 12 ++++++------ libraries/particles/src/Particle.cpp | 2 +- libraries/particles/src/ParticleTree.cpp | 17 ++++++----------- libraries/particles/src/ParticleTree.h | 2 +- .../src/ParticlesScriptingInterface.cpp | 4 +--- 6 files changed, 23 insertions(+), 24 deletions(-) diff --git a/libraries/octree/src/Octree.cpp b/libraries/octree/src/Octree.cpp index d9a8ab0f74..b4935816b3 100644 --- a/libraries/octree/src/Octree.cpp +++ b/libraries/octree/src/Octree.cpp @@ -41,7 +41,8 @@ float boundaryDistanceForRenderLevel(unsigned int renderLevel, float voxelSizeSc Octree::Octree(bool shouldReaverage) : _isDirty(true), _shouldReaverage(shouldReaverage), - _stopImport(false) { + _stopImport(false), + _lock(QReadWriteLock::Recursive) { _rootNode = NULL; _isViewing = false; } @@ -551,7 +552,10 @@ OctreeElement* Octree::getOctreeElementAt(float x, float y, float z, float s) co OctreeElement* Octree::getOrCreateChildElementAt(float x, float y, float z, float s) { - return getRoot()->getOrCreateChildElementAt(x, y, z, s); + lockForWrite(); + OctreeElement* result = getRoot()->getOrCreateChildElementAt(x, y, z, s); + unlock(); + return result; } @@ -1478,10 +1482,12 @@ void Octree::writeToSVOFile(const char* fileName, OctreeElement* node) { while (!nodeBag.isEmpty()) { OctreeElement* subTree = nodeBag.extract(); + qDebug() << "waiting to lockForRead() to encode() for writeSVO..."; 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); bytesWritten = encodeTreeBitstream(subTree, &packetData, nodeBag, params); unlock(); + qDebug() << "unlock() after encode() for writeSVO..."; // if the subTree couldn't fit, and so we should reset the packet and reinsert the node 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 9db83d47b1..21a6929034 100644 --- a/libraries/octree/src/Octree.h +++ b/libraries/octree/src/Octree.h @@ -240,11 +240,11 @@ public: bool readFromSVOFile(const char* filename); // 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(); } + void lockForRead() { _lock.lockForRead(); } + bool tryLockForRead() { return _lock.tryLockForRead(); } + void lockForWrite() { _lock.lockForWrite(); } + bool tryLockForWrite() { return _lock.tryLockForWrite(); } + void unlock() { _lock.unlock(); } unsigned long getOctreeElementsCount(); @@ -329,7 +329,7 @@ protected: /// flushes out any Octal Codes that had to be queued void emptyDeleteQueue(); - QReadWriteLock lock; + QReadWriteLock _lock; /// This tree is receiving inbound viewer datagrams. bool _isViewing; diff --git a/libraries/particles/src/Particle.cpp b/libraries/particles/src/Particle.cpp index d9f0beb81a..b493ec6bc7 100644 --- a/libraries/particles/src/Particle.cpp +++ b/libraries/particles/src/Particle.cpp @@ -385,7 +385,7 @@ Particle Particle::fromEditPacket(const unsigned char* data, int length, int& pr } else { // look up the existing particle - const Particle* existingParticle = tree->findParticleByID(editID, true); + const Particle* existingParticle = tree->findParticleByID(editID); // copy existing properties before over-writing with new properties if (existingParticle) { diff --git a/libraries/particles/src/ParticleTree.cpp b/libraries/particles/src/ParticleTree.cpp index e257237f7a..30270b8fcf 100644 --- a/libraries/particles/src/ParticleTree.cpp +++ b/libraries/particles/src/ParticleTree.cpp @@ -99,8 +99,8 @@ void ParticleTree::storeParticle(const Particle& particle, const SharedNodePoint if (!args.found) { glm::vec3 position = particle.getPosition(); float size = std::max(MINIMUM_PARTICLE_ELEMENT_SIZE, particle.getRadius()); - ParticleTreeElement* element = (ParticleTreeElement*)getOrCreateChildElementAt(position.x, position.y, position.z, size); + ParticleTreeElement* element = (ParticleTreeElement*)getOrCreateChildElementAt(position.x, position.y, position.z, size); element->storeParticle(particle); } // what else do we need to do here to get reaveraging to work @@ -149,8 +149,8 @@ void ParticleTree::addParticle(const ParticleID& particleID, const ParticlePrope Particle particle(particleID, properties); glm::vec3 position = particle.getPosition(); float size = std::max(MINIMUM_PARTICLE_ELEMENT_SIZE, particle.getRadius()); + ParticleTreeElement* element = (ParticleTreeElement*)getOrCreateChildElementAt(position.x, position.y, position.z, size); - element->storeParticle(particle); _isDirty = true; @@ -370,17 +370,12 @@ bool ParticleTree::findByIDOperation(OctreeElement* element, void* extraData) { } -const Particle* ParticleTree::findParticleByID(uint32_t id, bool alreadyLocked) { +const Particle* ParticleTree::findParticleByID(uint32_t id) { FindByIDArgs args = { id, false, NULL }; - if (!alreadyLocked) { - //qDebug() << "ParticleTree::findParticleByID().... about to call lockForRead()...."; - lockForRead(); - //qDebug() << "ParticleTree::findParticleByID().... after call lockForRead()...."; - } + + lockForRead(); recurseTreeWithOperation(findByIDOperation, &args); - if (!alreadyLocked) { - unlock(); - } + unlock(); return args.foundParticle; } diff --git a/libraries/particles/src/ParticleTree.h b/libraries/particles/src/ParticleTree.h index f3b8f5183d..08e8e51a05 100644 --- a/libraries/particles/src/ParticleTree.h +++ b/libraries/particles/src/ParticleTree.h @@ -44,7 +44,7 @@ public: void addParticle(const ParticleID& particleID, const ParticleProperties& properties); void deleteParticle(const ParticleID& particleID); const Particle* findClosestParticle(glm::vec3 position, float targetRadius); - const Particle* findParticleByID(uint32_t id, bool alreadyLocked = false); + const Particle* findParticleByID(uint32_t id); /// finds all particles that touch a sphere /// \param center the center of the sphere diff --git a/libraries/particles/src/ParticlesScriptingInterface.cpp b/libraries/particles/src/ParticlesScriptingInterface.cpp index a25dde1b9e..c2376e8d72 100644 --- a/libraries/particles/src/ParticlesScriptingInterface.cpp +++ b/libraries/particles/src/ParticlesScriptingInterface.cpp @@ -33,9 +33,7 @@ ParticleID ParticlesScriptingInterface::addParticle(const ParticleProperties& pr // If we have a local particle tree set, then also update it. if (_particleTree) { - _particleTree->lockForWrite(); _particleTree->addParticle(id, properties); - _particleTree->unlock(); } return id; @@ -66,7 +64,7 @@ ParticleProperties ParticlesScriptingInterface::getParticleProperties(ParticleID } if (_particleTree) { _particleTree->lockForRead(); - const Particle* particle = _particleTree->findParticleByID(identity.id, true); + const Particle* particle = _particleTree->findParticleByID(identity.id); if (particle) { results.copyFromParticle(*particle); } else { From 3684359c5f7d2ff0d4f62f9b9e52687833bc1762 Mon Sep 17 00:00:00 2001 From: ZappoMan Date: Mon, 3 Mar 2014 13:57:05 -0800 Subject: [PATCH 3/3] removed some debug --- libraries/octree/src/Octree.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/libraries/octree/src/Octree.cpp b/libraries/octree/src/Octree.cpp index b4935816b3..0847590720 100644 --- a/libraries/octree/src/Octree.cpp +++ b/libraries/octree/src/Octree.cpp @@ -1482,12 +1482,10 @@ void Octree::writeToSVOFile(const char* fileName, OctreeElement* node) { while (!nodeBag.isEmpty()) { OctreeElement* subTree = nodeBag.extract(); - qDebug() << "waiting to lockForRead() to encode() for writeSVO..."; 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); bytesWritten = encodeTreeBitstream(subTree, &packetData, nodeBag, params); unlock(); - qDebug() << "unlock() after encode() for writeSVO..."; // if the subTree couldn't fit, and so we should reset the packet and reinsert the node in our bag and try again... if (bytesWritten == 0 && (params.stopReason == EncodeBitstreamParams::DIDNT_FIT)) {