From 46d45a2d43b0be8457ae79ac41df64a93f0697c4 Mon Sep 17 00:00:00 2001 From: matsukaze Date: Mon, 2 Jun 2014 05:55:58 -0400 Subject: [PATCH] Job #19700 BUG: Crash in NodeBounds::draw() fixed. QMap and QReadWriteLock are orthogonal. Combined them into a wrapper class called NodeToJurisdictionMap, replacing typedef. This allows us to avoid changing method signatures wherever NodeToJurisdictionMap is used. The lock is bound with the map and is available to all clients of the NodeToJurisdictionMap. The lock allows multi-threaded access to the map. Fixed compiler warning in NodeBounds.cpp regarding loss of precision during conversion of double to GLfloat. --- interface/src/Application.cpp | 25 +++++++++++++++++++ interface/src/ui/NodeBounds.cpp | 12 ++++++--- interface/src/ui/OctreeStatsDialog.cpp | 3 +++ libraries/octree/src/JurisdictionMap.h | 3 ++- .../octree/src/OctreeEditPacketSender.cpp | 6 +++++ libraries/octree/src/OctreeHeadlessViewer.cpp | 11 ++++++++ 6 files changed, 56 insertions(+), 4 deletions(-) diff --git a/interface/src/Application.cpp b/interface/src/Application.cpp index 0e731fde79..48a4f64f72 100644 --- a/interface/src/Application.cpp +++ b/interface/src/Application.cpp @@ -3116,9 +3116,15 @@ void Application::domainChanged(const QString& domainHostname) { _environment.resetToDefault(); // reset our node to stats and node to jurisdiction maps... since these must be changing... + _voxelServerJurisdictions.lockForWrite(); _voxelServerJurisdictions.clear(); + _voxelServerJurisdictions.unlock(); + _octreeServerSceneStats.clear(); + + _particleServerJurisdictions.lockForWrite(); _particleServerJurisdictions.clear(); + _particleServerJurisdictions.unlock(); // reset the particle renderer _particles.clear(); @@ -3157,10 +3163,12 @@ void Application::nodeKilled(SharedNodePointer node) { if (node->getType() == NodeType::VoxelServer) { QUuid nodeUUID = node->getUUID(); // see if this is the first we've heard of this node... + _voxelServerJurisdictions.lockForRead(); if (_voxelServerJurisdictions.find(nodeUUID) != _voxelServerJurisdictions.end()) { unsigned char* rootCode = _voxelServerJurisdictions[nodeUUID].getRootOctalCode(); VoxelPositionSize rootDetails; voxelDetailsForCode(rootCode, rootDetails); + _voxelServerJurisdictions.unlock(); qDebug("voxel server going away...... v[%f, %f, %f, %f]", rootDetails.x, rootDetails.y, rootDetails.z, rootDetails.s); @@ -3177,8 +3185,10 @@ void Application::nodeKilled(SharedNodePointer node) { } // If the voxel server is going away, remove it from our jurisdiction map so we don't send voxels to a dead server + _voxelServerJurisdictions.lockForWrite(); _voxelServerJurisdictions.erase(_voxelServerJurisdictions.find(nodeUUID)); } + _voxelServerJurisdictions.unlock(); // also clean up scene stats for that server _octreeSceneStatsLock.lockForWrite(); @@ -3190,10 +3200,12 @@ void Application::nodeKilled(SharedNodePointer node) { } else if (node->getType() == NodeType::ParticleServer) { QUuid nodeUUID = node->getUUID(); // see if this is the first we've heard of this node... + _particleServerJurisdictions.lockForRead(); if (_particleServerJurisdictions.find(nodeUUID) != _particleServerJurisdictions.end()) { unsigned char* rootCode = _particleServerJurisdictions[nodeUUID].getRootOctalCode(); VoxelPositionSize rootDetails; voxelDetailsForCode(rootCode, rootDetails); + _particleServerJurisdictions.unlock(); qDebug("particle server going away...... v[%f, %f, %f, %f]", rootDetails.x, rootDetails.y, rootDetails.z, rootDetails.s); @@ -3210,8 +3222,10 @@ void Application::nodeKilled(SharedNodePointer node) { } // If the particle server is going away, remove it from our jurisdiction map so we don't send voxels to a dead server + _particleServerJurisdictions.lockForWrite(); _particleServerJurisdictions.erase(_particleServerJurisdictions.find(nodeUUID)); } + _particleServerJurisdictions.unlock(); // also clean up scene stats for that server _octreeSceneStatsLock.lockForWrite(); @@ -3224,10 +3238,12 @@ void Application::nodeKilled(SharedNodePointer node) { QUuid nodeUUID = node->getUUID(); // see if this is the first we've heard of this node... + _modelServerJurisdictions.lockForRead(); if (_modelServerJurisdictions.find(nodeUUID) != _modelServerJurisdictions.end()) { unsigned char* rootCode = _modelServerJurisdictions[nodeUUID].getRootOctalCode(); VoxelPositionSize rootDetails; voxelDetailsForCode(rootCode, rootDetails); + _modelServerJurisdictions.unlock(); qDebug("model server going away...... v[%f, %f, %f, %f]", rootDetails.x, rootDetails.y, rootDetails.z, rootDetails.s); @@ -3244,8 +3260,10 @@ void Application::nodeKilled(SharedNodePointer node) { } // If the model server is going away, remove it from our jurisdiction map so we don't send voxels to a dead server + _modelServerJurisdictions.lockForWrite(); _modelServerJurisdictions.erase(_modelServerJurisdictions.find(nodeUUID)); } + _modelServerJurisdictions.unlock(); // also clean up scene stats for that server _octreeSceneStatsLock.lockForWrite(); @@ -3315,7 +3333,10 @@ int Application::parseOctreeStats(const QByteArray& packet, const SharedNodePoin serverType = "Model"; } + jurisdiction->lockForRead(); if (jurisdiction->find(nodeUUID) == jurisdiction->end()) { + jurisdiction->unlock(); + qDebug("stats from new %s server... [%f, %f, %f, %f]", qPrintable(serverType), rootDetails.x, rootDetails.y, rootDetails.z, rootDetails.s); @@ -3329,6 +3350,8 @@ int Application::parseOctreeStats(const QByteArray& packet, const SharedNodePoin _voxelFades.push_back(fade); _voxelFadesLock.unlock(); } + } 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 @@ -3336,7 +3359,9 @@ int Application::parseOctreeStats(const QByteArray& packet, const SharedNodePoin // details from the OctreeSceneStats to construct the JurisdictionMap JurisdictionMap jurisdictionMap; jurisdictionMap.copyContents(temp.getJurisdictionRoot(), temp.getJurisdictionEndNodes()); + jurisdiction->lockForWrite(); (*jurisdiction)[nodeUUID] = jurisdictionMap; + jurisdiction->unlock(); } return statsMessageLength; } diff --git a/interface/src/ui/NodeBounds.cpp b/interface/src/ui/NodeBounds.cpp index f0e4a25f49..12a93095cd 100644 --- a/interface/src/ui/NodeBounds.cpp +++ b/interface/src/ui/NodeBounds.cpp @@ -70,14 +70,16 @@ void NodeBounds::draw() { } QUuid nodeUUID = node->getUUID(); + serverJurisdictions->lockForRead(); if (serverJurisdictions->find(nodeUUID) != serverJurisdictions->end()) { - const JurisdictionMap& map = serverJurisdictions->value(nodeUUID); + const JurisdictionMap& map = (*serverJurisdictions)[nodeUUID]; unsigned char* rootCode = map.getRootOctalCode(); if (rootCode) { VoxelPositionSize rootDetails; voxelDetailsForCode(rootCode, rootDetails); + serverJurisdictions->unlock(); glm::vec3 location(rootDetails.x, rootDetails.y, rootDetails.z); location *= (float)TREE_SCALE; @@ -123,7 +125,11 @@ void NodeBounds::draw() { selectedCenter = center; selectedScale = scaleFactor; } + } else { + serverJurisdictions->unlock(); } + } else { + serverJurisdictions->unlock(); } } @@ -136,7 +142,7 @@ void NodeBounds::draw() { float red, green, blue; getColorForNodeType(selectedNode->getType(), red, green, blue); - glColor4f(red, green, blue, 0.2); + glColor4f(red, green, blue, 0.2f); glutSolidCube(1.0); glPopMatrix(); @@ -229,7 +235,7 @@ void NodeBounds::drawOverlay() { int mouseX = application->getMouseX(), mouseY = application->getMouseY(), textWidth = widthText(TEXT_SCALE, 0, _overlayText); - glColor4f(0.4, 0.4, 0.4, 0.6); + glColor4f(0.4f, 0.4f, 0.4f, 0.6f); renderBevelCornersRect(mouseX + MOUSE_OFFSET, mouseY - TEXT_HEIGHT - PADDING, textWidth + (2 * PADDING), TEXT_HEIGHT + (2 * PADDING), BACKGROUND_BEVEL); drawText(mouseX + MOUSE_OFFSET + PADDING, mouseY, TEXT_SCALE, ROTATION, FONT, _overlayText, TEXT_COLOR); diff --git a/interface/src/ui/OctreeStatsDialog.cpp b/interface/src/ui/OctreeStatsDialog.cpp index bd2abba325..3296d8ccb2 100644 --- a/interface/src/ui/OctreeStatsDialog.cpp +++ b/interface/src/ui/OctreeStatsDialog.cpp @@ -281,8 +281,10 @@ 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 { const JurisdictionMap& map = serverJurisdictions[nodeUUID]; @@ -305,6 +307,7 @@ void OctreeStatsDialog::showOctreeServersOfType(int& serverCount, NodeType_t ser } else { serverDetails << " jurisdiction has no rootCode"; } // root code + serverJurisdictions.unlock(); } // jurisdiction // now lookup stats details for this server... diff --git a/libraries/octree/src/JurisdictionMap.h b/libraries/octree/src/JurisdictionMap.h index b4174e6432..9820045321 100644 --- a/libraries/octree/src/JurisdictionMap.h +++ b/libraries/octree/src/JurisdictionMap.h @@ -18,6 +18,7 @@ #include #include +#include #include @@ -82,7 +83,7 @@ private: /// Map between node IDs and their reported JurisdictionMap. Typically used by classes that need to know which nodes are /// managing which jurisdictions. -typedef QMap NodeToJurisdictionMap; +class NodeToJurisdictionMap : public QMap, public QReadWriteLock {}; typedef QMap::iterator NodeToJurisdictionMapIterator; diff --git a/libraries/octree/src/OctreeEditPacketSender.cpp b/libraries/octree/src/OctreeEditPacketSender.cpp index 80f75e9278..898c41de08 100644 --- a/libraries/octree/src/OctreeEditPacketSender.cpp +++ b/libraries/octree/src/OctreeEditPacketSender.cpp @@ -71,9 +71,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(); } hasServers = true; } @@ -185,8 +187,10 @@ void OctreeEditPacketSender::queuePacketToNodes(unsigned char* buffer, ssize_t l 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(); if (isMyJurisdiction) { queuePacketToNode(nodeUUID, buffer, length); } @@ -236,12 +240,14 @@ void OctreeEditPacketSender::queueOctreeEditMessage(PacketType type, unsigned ch 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(codeColorBuffer, CHECK_NODE_ONLY) == JurisdictionMap::WITHIN); } else { isMyJurisdiction = false; } + _serverJurisdictions->unlock(); } if (isMyJurisdiction) { EditPacketBuffer& packetBuffer = _pendingEditPackets[nodeUUID]; diff --git a/libraries/octree/src/OctreeHeadlessViewer.cpp b/libraries/octree/src/OctreeHeadlessViewer.cpp index 5833cb4170..2372d85b0e 100644 --- a/libraries/octree/src/OctreeHeadlessViewer.cpp +++ b/libraries/octree/src/OctreeHeadlessViewer.cpp @@ -45,9 +45,11 @@ void OctreeHeadlessViewer::queryOctree() { qDebug() << "---------------"; qDebug() << "_jurisdictionListener=" << _jurisdictionListener; qDebug() << "Jurisdictions..."; + jurisdictions.lockForRead(); for (NodeToJurisdictionMapIterator i = jurisdictions.begin(); i != jurisdictions.end(); ++i) { qDebug() << i.key() << ": " << &i.value(); } + jurisdictions.unlock(); qDebug() << "---------------"; } @@ -85,7 +87,9 @@ 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 { const JurisdictionMap& map = (jurisdictions)[nodeUUID]; @@ -95,6 +99,7 @@ void OctreeHeadlessViewer::queryOctree() { if (rootCode) { VoxelPositionSize rootDetails; voxelDetailsForCode(rootCode, rootDetails); + jurisdictions.unlock(); AACube serverBounds(glm::vec3(rootDetails.x, rootDetails.y, rootDetails.z), rootDetails.s); serverBounds.scale(TREE_SCALE); @@ -103,6 +108,8 @@ void OctreeHeadlessViewer::queryOctree() { if (serverFrustumLocation != ViewFrustum::OUTSIDE) { inViewServers++; } + } else { + jurisdictions.unlock(); } } } @@ -148,7 +155,9 @@ 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) { qDebug() << "no known jurisdiction for node " << *node << ", assume it's visible."; @@ -161,6 +170,7 @@ void OctreeHeadlessViewer::queryOctree() { if (rootCode) { VoxelPositionSize rootDetails; voxelDetailsForCode(rootCode, rootDetails); + jurisdictions.unlock(); AACube serverBounds(glm::vec3(rootDetails.x, rootDetails.y, rootDetails.z), rootDetails.s); serverBounds.scale(TREE_SCALE); @@ -171,6 +181,7 @@ void OctreeHeadlessViewer::queryOctree() { inView = false; } } else { + jurisdictions.unlock(); if (wantExtraDebugging) { qDebug() << "Jurisdiction without RootCode for node " << *node << ". That's unusual!"; }