From 99e1fdd46e227cde5412a21496db593ef6219648 Mon Sep 17 00:00:00 2001
From: Andrew Meadows <andrew@highfidelity.io>
Date: Sat, 10 Jan 2015 07:44:26 -0800
Subject: [PATCH] fix for EntityServer crash
adding EntityItem::_element backpointer for easier add/remove logic
---
.../src/EntityTreeRenderer.cpp | 2 +
libraries/entities/src/EntityItem.cpp | 3 +
libraries/entities/src/EntityItem.h | 4 ++
libraries/entities/src/EntityTreeElement.cpp | 13 ++++-
.../entities/src/MovingEntitiesOperator.cpp | 15 +++--
.../entities/src/UpdateEntityOperator.cpp | 57 ++++++++-----------
6 files changed, 55 insertions(+), 39 deletions(-)
diff --git a/libraries/entities-renderer/src/EntityTreeRenderer.cpp b/libraries/entities-renderer/src/EntityTreeRenderer.cpp
index 47e9237ddc..3518e7b75c 100644
--- a/libraries/entities-renderer/src/EntityTreeRenderer.cpp
+++ b/libraries/entities-renderer/src/EntityTreeRenderer.cpp
@@ -98,7 +98,9 @@ void EntityTreeRenderer::init() {
}
QScriptValue EntityTreeRenderer::loadEntityScript(const EntityItemID& entityItemID) {
+ _tree->lockForRead();
EntityItem* entity = static_cast<EntityTree*>(_tree)->findEntityByEntityItemID(entityItemID);
+ _tree->unlock();
return loadEntityScript(entity);
}
diff --git a/libraries/entities/src/EntityItem.cpp b/libraries/entities/src/EntityItem.cpp
index a744d39f05..6917dc1e7d 100644
--- a/libraries/entities/src/EntityItem.cpp
+++ b/libraries/entities/src/EntityItem.cpp
@@ -95,6 +95,7 @@ EntityItem::EntityItem(const EntityItemID& entityItemID) {
_physicsInfo = NULL;
_dirtyFlags = 0;
_changedOnServer = 0;
+ _element = NULL;
initFromEntityItemID(entityItemID);
}
@@ -110,6 +111,7 @@ EntityItem::EntityItem(const EntityItemID& entityItemID, const EntityItemPropert
_physicsInfo = NULL;
_dirtyFlags = 0;
_changedOnServer = 0;
+ _element = NULL;
initFromEntityItemID(entityItemID);
setProperties(properties);
}
@@ -117,6 +119,7 @@ EntityItem::EntityItem(const EntityItemID& entityItemID, const EntityItemPropert
EntityItem::~EntityItem() {
// be sure to clean up _physicsInfo before calling this dtor
assert(_physicsInfo == NULL);
+ assert(_element == NULL);
}
EntityPropertyFlags EntityItem::getEntityProperties(EncodeBitstreamParams& params) const {
diff --git a/libraries/entities/src/EntityItem.h b/libraries/entities/src/EntityItem.h
index e52be1707a..6d422ab257 100644
--- a/libraries/entities/src/EntityItem.h
+++ b/libraries/entities/src/EntityItem.h
@@ -39,6 +39,7 @@ class EntityTreeElementExtraEncodeData;
/// 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 {
+ friend class EntityTreeElement;
public:
enum EntityDirtyFlags {
@@ -301,6 +302,7 @@ public:
void* getPhysicsInfo() const { return _physicsInfo; }
void setPhysicsInfo(void* data) { _physicsInfo = data; }
+ EntityTreeElement* getElement() const { return _element; }
protected:
virtual void initFromEntityItemID(const EntityItemID& entityItemID); // maybe useful to allow subclasses to init
@@ -362,6 +364,8 @@ protected:
// DirtyFlags are set whenever a property changes that the EntitySimulation needs to know about.
uint32_t _dirtyFlags; // things that have changed from EXTERNAL changes (via script or packet) but NOT from simulation
+
+ EntityTreeElement* _element; // back pointer to containing Element
};
diff --git a/libraries/entities/src/EntityTreeElement.cpp b/libraries/entities/src/EntityTreeElement.cpp
index 5f072164a0..4ced5fe844 100644
--- a/libraries/entities/src/EntityTreeElement.cpp
+++ b/libraries/entities/src/EntityTreeElement.cpp
@@ -682,6 +682,7 @@ void EntityTreeElement::cleanupEntities() {
uint16_t numberOfEntities = _entityItems->size();
for (uint16_t i = 0; i < numberOfEntities; i++) {
EntityItem* entity = (*_entityItems)[i];
+ entity->_element = NULL;
delete entity;
}
_entityItems->clear();
@@ -693,6 +694,7 @@ bool EntityTreeElement::removeEntityWithEntityItemID(const EntityItemID& id) {
for (uint16_t i = 0; i < numberOfEntities; i++) {
if ((*_entityItems)[i]->getEntityItemID() == id) {
foundEntity = true;
+ (*_entityItems)[i]->_element = NULL;
_entityItems->removeAt(i);
break;
}
@@ -701,7 +703,13 @@ bool EntityTreeElement::removeEntityWithEntityItemID(const EntityItemID& id) {
}
bool EntityTreeElement::removeEntityItem(EntityItem* entity) {
- return _entityItems->removeAll(entity) > 0;
+ int numEntries = _entityItems->removeAll(entity);
+ if (numEntries > 0) {
+ assert(entity->_element == this);
+ entity->_element = NULL;
+ return true;
+ }
+ return false;
}
@@ -808,7 +816,10 @@ int EntityTreeElement::readElementDataFromBuffer(const unsigned char* data, int
}
void EntityTreeElement::addEntityItem(EntityItem* entity) {
+ assert(entity);
+ assert(entity->_element == NULL);
_entityItems->push_back(entity);
+ entity->_element = this;
}
// will average a "common reduced LOD view" from the the child elements...
diff --git a/libraries/entities/src/MovingEntitiesOperator.cpp b/libraries/entities/src/MovingEntitiesOperator.cpp
index 1ec67967b6..48ba8e4ec2 100644
--- a/libraries/entities/src/MovingEntitiesOperator.cpp
+++ b/libraries/entities/src/MovingEntitiesOperator.cpp
@@ -179,7 +179,7 @@ bool MovingEntitiesOperator::preRecursion(OctreeElement* element) {
// If this is one of the old elements we're looking for, then ask it to remove the old entity
if (!details.oldFound && entityTreeElement == details.oldContainingElement) {
- entityTreeElement->removeEntityItem(details.entity);
+ // DO NOT remove the entity here. It will be removed when added to the destination element.
_foundOldCount++;
//details.oldFound = true; // TODO: would be nice to add this optimization
if (_wantDebug) {
@@ -193,8 +193,15 @@ bool MovingEntitiesOperator::preRecursion(OctreeElement* element) {
// If this element is the best fit for the new bounds of this entity then add the entity to the element
if (!details.newFound && entityTreeElement->bestFitBounds(details.newCube)) {
EntityItemID entityItemID = details.entity->getEntityItemID();
- entityTreeElement->addEntityItem(details.entity);
- _tree->setContainingElement(entityItemID, entityTreeElement);
+ // remove from the old before adding
+ EntityTreeElement* oldElement = details.entity->getElement();
+ if (oldElement != entityTreeElement) {
+ if (oldElement) {
+ oldElement->removeEntityItem(details.entity);
+ }
+ entityTreeElement->addEntityItem(details.entity);
+ _tree->setContainingElement(entityItemID, entityTreeElement);
+ }
_foundNewCount++;
//details.newFound = true; // TODO: would be nice to add this optimization
if (_wantDebug) {
@@ -227,7 +234,7 @@ bool MovingEntitiesOperator::postRecursion(OctreeElement* element) {
- // It's not OK to prune if we have the potential of deleting the original containig element.
+ // It's not OK to prune if we have the potential of deleting the original containing element
// because if we prune the containing element then new might end up reallocating the same memory later
// and that will confuse our logic.
//
diff --git a/libraries/entities/src/UpdateEntityOperator.cpp b/libraries/entities/src/UpdateEntityOperator.cpp
index 9cee11d73c..c85c4235de 100644
--- a/libraries/entities/src/UpdateEntityOperator.cpp
+++ b/libraries/entities/src/UpdateEntityOperator.cpp
@@ -231,18 +231,19 @@ bool UpdateEntityOperator::preRecursion(OctreeElement* element) {
qDebug() << " *** REMOVING from ELEMENT ***";
}
- entityTreeElement->removeEntityItem(_existingEntity); // NOTE: only removes the entity, doesn't delete it
+ // the entity knows what element it's in, so we remove it from that one
+ // NOTE: we know we haven't yet added it to its new element because _removeOld is true
+ EntityTreeElement* oldElement = _existingEntity->getElement();
+ oldElement->removeEntityItem(_existingEntity);
+ _tree->setContainingElement(_entityItemID, NULL);
- // If we haven't yet found the new location, then we need to
- // make sure to remove our entity to element map, because for
- // now we're not in that map
- if (!_foundNew) {
- _tree->setContainingElement(_entityItemID, NULL);
-
- if (_wantDebug) {
- qDebug() << " *** REMOVING from MAP ***";
- }
+ if (oldElement != _containingElement) {
+ qDebug() << "WARNING entity moved during UpdateEntityOperator recursion";
+ assert(! _containingElement->removeEntityItem(_existingEntity));
+ }
+ if (_wantDebug) {
+ qDebug() << " *** REMOVING from MAP ***";
}
}
_foundOld = true;
@@ -263,7 +264,6 @@ bool UpdateEntityOperator::preRecursion(OctreeElement* element) {
qDebug() << " entityTreeElement->bestFitBounds(_newEntityBox)=" << entityTreeElement->bestFitBounds(_newEntityBox);
}
-
// If this element is the best fit for the new entity properties, then add/or update it
if (entityTreeElement->bestFitBounds(_newEntityBox)) {
@@ -271,33 +271,14 @@ bool UpdateEntityOperator::preRecursion(OctreeElement* element) {
qDebug() << " *** THIS ELEMENT IS BEST FIT ***";
}
+ EntityTreeElement* oldElement = _existingEntity->getElement();
// if we are the existing containing element, then we can just do the update of the entity properties
- if (entityTreeElement == _containingElement) {
+ if (entityTreeElement == oldElement) {
if (_wantDebug) {
qDebug() << " *** This is the same OLD ELEMENT ***";
}
- // TODO: We shouldn't be in a remove old case and also be the new best fit. This indicates that
- // we have some kind of a logic error in this operator. But, it can handle it properly by setting
- // the new properties for the entity and moving on. Still going to output a warning that if we
- // see consistently we will want to address this.
- if (_removeOld) {
- qDebug() << "UNEXPECTED - UpdateEntityOperator - "
- "we thought we needed to removeOld, but the old entity is our best fit.";
- _removeOld = false;
-
- // if we thought we were supposed to remove the old item, and we already did, then we need
- // to repair this case.
- if (_foundOld) {
- if (_wantDebug) {
- qDebug() << " *** REPAIRING PREVIOUS REMOVAL from ELEMENT and MAP ***";
- }
- entityTreeElement->addEntityItem(_existingEntity);
- _tree->setContainingElement(_entityItemID, entityTreeElement);
- }
- }
-
// set the entity properties and mark our element as changed.
_existingEntity->setProperties(_properties);
if (_wantDebug) {
@@ -305,14 +286,22 @@ bool UpdateEntityOperator::preRecursion(OctreeElement* element) {
}
} else {
// otherwise, this is an add case.
+ if (oldElement) {
+ oldElement->removeEntityItem(_existingEntity);
+ if (oldElement != _containingElement) {
+ qDebug() << "WARNING entity moved during UpdateEntityOperator recursion";
+ }
+ }
entityTreeElement->addEntityItem(_existingEntity);
- _existingEntity->setProperties(_properties); // still need to update the properties!
_tree->setContainingElement(_entityItemID, entityTreeElement);
+
+ _existingEntity->setProperties(_properties); // still need to update the properties!
if (_wantDebug) {
qDebug() << " *** ADDING ENTITY to ELEMENT and MAP and SETTING PROPERTIES ***";
}
}
- _foundNew = true; // we found the new item!
+ _foundNew = true; // we found the new element
+ _removeOld = false; // and it has already been removed from the old
} else {
keepSearching = true;
}