History

Fixing some entity race condition crashes

Brad Davis committed 11 years ago

Commit aa989c31 parent c4c6e7236 files · +1364 −1192Browse files

Showing the changes to libraries/entities/src/EntityScriptingInterface.cpp (1 of 36 files). Show all

Unified · Split

libraries/entities/src/EntityScriptingInterface.cpp+142−138
@@ -72,23 +72,23 @@ QUuid EntityScriptingInterface::addEntity(const EntityItemProperties& properties · 71 unmodified lines
7272 // If we have a local entity tree set, then also update it.
7373 bool success = true;
7474 if (_entityTree) {
75− _entityTree->lockForWrite();
76− EntityItemPointer entity = _entityTree->addEntity(id, propertiesWithSimID);
77− if (entity) {
78− // This Node is creating a new object. If it's in motion, set this Node as the simulator.
79− auto nodeList = DependencyManager::get<NodeList>();
80− const QUuid myNodeID = nodeList->getSessionUUID();
81− propertiesWithSimID.setSimulationOwner(myNodeID, SCRIPT_EDIT_SIMULATION_PRIORITY);
75+ _entityTree->withWriteLock([&] {
76+ EntityItemPointer entity = _entityTree->addEntity(id, propertiesWithSimID);
77+ if (entity) {
78+ // This Node is creating a new object. If it's in motion, set this Node as the simulator.
79+ auto nodeList = DependencyManager::get<NodeList>();
80+ const QUuid myNodeID = nodeList->getSessionUUID();
81+ propertiesWithSimID.setSimulationOwner(myNodeID, SCRIPT_EDIT_SIMULATION_PRIORITY);
8282
83− // and make note of it now, so we can act on it right away.
84− entity->setSimulationOwner(myNodeID, SCRIPT_EDIT_SIMULATION_PRIORITY);
83+ // and make note of it now, so we can act on it right away.
84+ entity->setSimulationOwner(myNodeID, SCRIPT_EDIT_SIMULATION_PRIORITY);
8585
86− entity->setLastBroadcast(usecTimestampNow());
87− } else {
88− qCDebug(entities) << "script failed to add new Entity to local Octree";
89− success = false;
90− }
91− _entityTree->unlock();
86+ entity->setLastBroadcast(usecTimestampNow());
87+ } else {
88+ qCDebug(entities) << "script failed to add new Entity to local Octree";
89+ success = false;
90+ }
91+ });
9292 }
9393
9494 // queue the packet
@@ -102,28 +102,26 @@ QUuid EntityScriptingInterface::addEntity(const EntityItemProperties& properties · 7 unmodified lines
102102 EntityItemProperties EntityScriptingInterface::getEntityProperties(QUuid identity) {
103103 EntityItemProperties results;
104104 if (_entityTree) {
105− _entityTree->lockForRead();
106−
107− EntityItemPointer entity = _entityTree->findEntityByEntityItemID(EntityItemID(identity));
108−
109− if (entity) {
110− results = entity->getProperties();
111−
112− // TODO: improve sitting points and naturalDimensions in the future,
113− // for now we've included the old sitting points model behavior for entity types that are models
114− // we've also added this hack for setting natural dimensions of models
115− if (entity->getType() == EntityTypes::Model) {
116− const FBXGeometry* geometry = _entityTree->getGeometryForEntity(entity);
117− if (geometry) {
118− results.setSittingPoints(geometry->sittingPoints);
119− Extents meshExtents = geometry->getUnscaledMeshExtents();
120− results.setNaturalDimensions(meshExtents.maximum - meshExtents.minimum);
121− results.calculateNaturalPosition(meshExtents.minimum, meshExtents.maximum);
105+ _entityTree->withReadLock([&] {
106+ EntityItemPointer entity = _entityTree->findEntityByEntityItemID(EntityItemID(identity));
107+ if (entity) {
108+ results = entity->getProperties();
109+
110+ // TODO: improve sitting points and naturalDimensions in the future,
111+ // for now we've included the old sitting points model behavior for entity types that are models
112+ // we've also added this hack for setting natural dimensions of models
113+ if (entity->getType() == EntityTypes::Model) {
114+ const FBXGeometry* geometry = _entityTree->getGeometryForEntity(entity);
115+ if (geometry) {
116+ results.setSittingPoints(geometry->sittingPoints);
117+ Extents meshExtents = geometry->getUnscaledMeshExtents();
118+ results.setNaturalDimensions(meshExtents.maximum - meshExtents.minimum);
119+ results.calculateNaturalPosition(meshExtents.minimum, meshExtents.maximum);
120+ }
122121 }
123− }
124122
125− }
126− _entityTree->unlock();
123+ }
124+ });
127125 }
128126
129127 return results;
@@ -132,56 +130,59 @@ EntityItemProperties EntityScriptingInterface::getEntityProperties(QUuid identit · 2 unmodified lines
132130 QUuid EntityScriptingInterface::editEntity(QUuid id, EntityItemProperties properties) {
133131 EntityItemID entityID(id);
134132 // If we have a local entity tree set, then also update it.
135− if (_entityTree) {
136− _entityTree->lockForWrite();
137− bool updatedEntity = _entityTree->updateEntity(entityID, properties);
138− _entityTree->unlock();
133+ if (!_entityTree) {
134+ queueEntityMessage(PacketType::EntityEdit, entityID, properties);
135+ return id;
136+ }
139137
140− if (updatedEntity) {
141− _entityTree->lockForRead();
142− EntityItemPointer entity = _entityTree->findEntityByEntityItemID(entityID);
143− if (entity) {
144− // make sure the properties has a type, so that the encode can know which properties to include
145− properties.setType(entity->getType());
146− bool hasTerseUpdateChanges = properties.hasTerseUpdateChanges();
147− bool hasPhysicsChanges = properties.hasMiscPhysicsChanges() || hasTerseUpdateChanges;
148− if (hasPhysicsChanges) {
149− auto nodeList = DependencyManager::get<NodeList>();
150− const QUuid myNodeID = nodeList->getSessionUUID();
151−
152− if (entity->getSimulatorID() == myNodeID) {
153− // we think we already own the simulation, so make sure to send ALL TerseUpdate properties
154− if (hasTerseUpdateChanges) {
155− entity->getAllTerseUpdateProperties(properties);
156− }
157− // TODO: if we knew that ONLY TerseUpdate properties have changed in properties AND the object
158− // is dynamic AND it is active in the physics simulation then we could chose to NOT queue an update
159− // and instead let the physics simulation decide when to send a terse update. This would remove
160− // the "slide-no-rotate" glitch (and typical a double-update) that we see during the "poke rolling
161− // balls" test. However, even if we solve this problem we still need to provide a "slerp the visible
162− // proxy toward the true physical position" feature to hide the final glitches in the remote watcher's
163− // simulation.
164−
165− if (entity->getSimulationPriority() < SCRIPT_EDIT_SIMULATION_PRIORITY) {
166− // we re-assert our simulation ownership at a higher priority
167− properties.setSimulationOwner(myNodeID,
168− glm::max(entity->getSimulationPriority(), SCRIPT_EDIT_SIMULATION_PRIORITY));
169− }
170− } else {
171− // we make a bid for simulation ownership
172− properties.setSimulationOwner(myNodeID, SCRIPT_EDIT_SIMULATION_PRIORITY);
173− entity->flagForOwnership();
138+ bool updatedEntity = false;
139+ _entityTree->withWriteLock([&] {
140+ updatedEntity = _entityTree->updateEntity(entityID, properties);
141+ });
142+
143+
144+ if (!updatedEntity) {
145+ return QUuid();
146+ }
147+
148+ _entityTree->withReadLock([&] {
149+ EntityItemPointer entity = _entityTree->findEntityByEntityItemID(entityID);
150+ if (entity) {
151+ // make sure the properties has a type, so that the encode can know which properties to include
152+ properties.setType(entity->getType());
153+ bool hasTerseUpdateChanges = properties.hasTerseUpdateChanges();
154+ bool hasPhysicsChanges = properties.hasMiscPhysicsChanges() || hasTerseUpdateChanges;
155+ if (hasPhysicsChanges) {
156+ auto nodeList = DependencyManager::get<NodeList>();
157+ const QUuid myNodeID = nodeList->getSessionUUID();
158+
159+ if (entity->getSimulatorID() == myNodeID) {
160+ // we think we already own the simulation, so make sure to send ALL TerseUpdate properties
161+ if (hasTerseUpdateChanges) {
162+ entity->getAllTerseUpdateProperties(properties);
163+ }
164+ // TODO: if we knew that ONLY TerseUpdate properties have changed in properties AND the object
165+ // is dynamic AND it is active in the physics simulation then we could chose to NOT queue an update
166+ // and instead let the physics simulation decide when to send a terse update. This would remove
167+ // the "slide-no-rotate" glitch (and typical a double-update) that we see during the "poke rolling
168+ // balls" test. However, even if we solve this problem we still need to provide a "slerp the visible
169+ // proxy toward the true physical position" feature to hide the final glitches in the remote watcher's
170+ // simulation.
171+
172+ if (entity->getSimulationPriority() < SCRIPT_EDIT_SIMULATION_PRIORITY) {
173+ // we re-assert our simulation ownership at a higher priority
174+ properties.setSimulationOwner(myNodeID,
175+ glm::max(entity->getSimulationPriority(), SCRIPT_EDIT_SIMULATION_PRIORITY));
174176 }
177+ } else {
178+ // we make a bid for simulation ownership
179+ properties.setSimulationOwner(myNodeID, SCRIPT_EDIT_SIMULATION_PRIORITY);
180+ entity->flagForOwnership();
175181 }
176− entity->setLastBroadcast(usecTimestampNow());
177182 }
178− _entityTree->unlock();
179− queueEntityMessage(PacketType::EntityEdit, entityID, properties);
180− return id;
183+ entity->setLastBroadcast(usecTimestampNow());
181184 }
182− return QUuid();
183− }
184−
185+ });
185186 queueEntityMessage(PacketType::EntityEdit, entityID, properties);
186187 return id;
187188 }
@@ -192,18 +193,16 @@ void EntityScriptingInterface::deleteEntity(QUuid id) { · 4 unmodified lines
192193
193194 // If we have a local entity tree set, then also update it.
194195 if (_entityTree) {
195− _entityTree->lockForWrite();
196−
197− EntityItemPointer entity = _entityTree->findEntityByEntityItemID(entityID);
198− if (entity) {
199− if (entity->getLocked()) {
200− shouldDelete = false;
201− } else {
202− _entityTree->deleteEntity(entityID);
196+ _entityTree->withWriteLock([&] {
197+ EntityItemPointer entity = _entityTree->findEntityByEntityItemID(entityID);
198+ if (entity) {
199+ if (entity->getLocked()) {
200+ shouldDelete = false;
201+ } else {
202+ _entityTree->deleteEntity(entityID);
203+ }
203204 }
204− }
205−
206− _entityTree->unlock();
205+ });
207206 }
208207
209208 // 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) { · 5 unmodified lines
215214 QUuid EntityScriptingInterface::findClosestEntity(const glm::vec3& center, float radius) const {
216215 EntityItemID result;
217216 if (_entityTree) {
218− _entityTree->lockForRead();
219− EntityItemPointer closestEntity = _entityTree->findClosestEntity(center, radius);
220− _entityTree->unlock();
217+ EntityItemPointer closestEntity;
218+ _entityTree->withReadLock([&] {
219+ closestEntity = _entityTree->findClosestEntity(center, radius);
220+ });
221221 if (closestEntity) {
222222 result = closestEntity->getEntityItemID();
223223 }
@@ -228,19 +228,19 @@ QUuid EntityScriptingInterface::findClosestEntity(const glm::vec3& center, float · 4 unmodified lines
228228
229229 void EntityScriptingInterface::dumpTree() const {
230230 if (_entityTree) {
231− _entityTree->lockForRead();
232− _entityTree->dumpTree();
233− _entityTree->unlock();
231+ _entityTree->withReadLock([&] {
232+ _entityTree->dumpTree();
233+ });
234234 }
235235 }
236236
237237 QVector<QUuid> EntityScriptingInterface::findEntities(const glm::vec3& center, float radius) const {
238238 QVector<QUuid> result;
239239 if (_entityTree) {
240− _entityTree->lockForRead();
241240 QVector<EntityItemPointer> entities;
242− _entityTree->findEntities(center, radius, entities);
243− _entityTree->unlock();
241+ _entityTree->withReadLock([&] {
242+ _entityTree->findEntities(center, radius, entities);
243+ });
244244
245245 foreach (EntityItemPointer entity, entities) {
246246 result << entity->getEntityItemID();
@@ -252,11 +252,11 @@ QVector<QUuid> EntityScriptingInterface::findEntities(const glm::vec3& center, f · 5 unmodified lines
252252 QVector<QUuid> EntityScriptingInterface::findEntitiesInBox(const glm::vec3& corner, const glm::vec3& dimensions) const {
253253 QVector<QUuid> result;
254254 if (_entityTree) {
255− _entityTree->lockForRead();
256− AABox box(corner, dimensions);
257255 QVector<EntityItemPointer> entities;
258− _entityTree->findEntities(box, entities);
259− _entityTree->unlock();
256+ _entityTree->withReadLock([&] {
257+ AABox box(corner, dimensions);
258+ _entityTree->findEntities(box, entities);
259+ });
260260
261261 foreach (EntityItemPointer entity, entities) {
262262 result << entity->getEntityItemID();
@@ -432,9 +432,10 @@ bool EntityScriptingInterface::setVoxels(QUuid entityID, · 169 unmodified lines
432432 }
433433
434434 auto polyVoxEntity = std::dynamic_pointer_cast<PolyVoxEntityItem>(entity);
435− _entityTree->lockForWrite();
436− bool result = actor(*polyVoxEntity);
437− _entityTree->unlock();
435+ bool result;
436+ _entityTree->withWriteLock([&] {
437+ result = actor(*polyVoxEntity);
438+ });
438439 return result;
439440 }
440441
@@ -457,15 +458,17 @@ bool EntityScriptingInterface::setPoints(QUuid entityID, std::function<bool(Line · 16 unmodified lines
457458 auto now = usecTimestampNow();
458459
459460 auto lineEntity = std::static_pointer_cast<LineEntityItem>(entity);
460− _entityTree->lockForWrite();
461− bool success = actor(*lineEntity);
462− entity->setLastEdited(now);
463− entity->setLastBroadcast(now);
464− _entityTree->unlock();
465−
466− _entityTree->lockForRead();
467− EntityItemProperties properties = entity->getProperties();
468− _entityTree->unlock();
461+ bool success;
462+ _entityTree->withWriteLock([&] {
463+ success = actor(*lineEntity);
464+ entity->setLastEdited(now);
465+ entity->setLastBroadcast(now);
466+ });
467+
468+ EntityItemProperties properties;
469+ _entityTree->withReadLock([&] {
470+ properties = entity->getProperties();
471+ });
469472
470473 properties.setLinePointsDirty();
471474 properties.setLastEdited(now);
@@ -543,32 +546,33 @@ bool EntityScriptingInterface::actionWorker(const QUuid& entityID, · 71 unmodified lines
543546 return false;
544547 }
545548
546− _entityTree->lockForWrite();
547−
548− EntitySimulation* simulation = _entityTree->getSimulation();
549− EntityItemPointer entity = _entityTree->findEntityByEntityItemID(entityID);
550− if (!entity) {
551− qDebug() << "actionWorker -- unknown entity" << entityID;
552− _entityTree->unlock();
553− return false;
554− }
549+ EntityItemPointer entity;
550+ bool success;
551+ _entityTree->withWriteLock([&] {
552+ EntitySimulation* simulation = _entityTree->getSimulation();
553+ EntityItemPointer entity = _entityTree->findEntityByEntityItemID(entityID);
554+ if (!entity) {
555+ qDebug() << "actionWorker -- unknown entity" << entityID;
556+ return;
557+ }
555558
556− if (!simulation) {
557− qDebug() << "actionWorker -- no simulation" << entityID;
558− _entityTree->unlock();
559− return false;
560− }
559+ if (!simulation) {
560+ qDebug() << "actionWorker -- no simulation" << entityID;
561+ return;
562+ }
561563
562− bool success = actor(simulation, entity);
563− if (success) {
564− _entityTree->entityChanged(entity);
565− }
566− _entityTree->unlock();
564+ success = actor(simulation, entity);
565+ if (success) {
566+ _entityTree->entityChanged(entity);
567+ }
568+ });
567569
568570 // transmit the change
569− _entityTree->lockForRead();
570− EntityItemProperties properties = entity->getProperties();
571− _entityTree->unlock();
571+ EntityItemProperties properties;
572+ _entityTree->withReadLock([&] {
573+ properties = entity->getProperties();
574+ });
575+
572576 properties.setActionDataDirty();
573577 auto now = usecTimestampNow();
574578 properties.setLastEdited(now);