From ffe1e2db00373ee58a0fa74fd697449d55c34a2c Mon Sep 17 00:00:00 2001 From: ZappoMan Date: Tue, 16 Sep 2014 09:07:22 -0700 Subject: [PATCH] fix crash in butterflies.js --- examples/butterflies.js | 7 +- .../entities/src/EntityItemProperties.cpp | 6 +- .../entities/src/EntityItemPropertiesMacros.h | 41 +++-- libraries/entities/src/EntityTree.cpp | 1 - .../entities/src/MovingEntitiesOperator.cpp | 15 +- .../entities/src/UpdateEntityOperator.cpp | 142 +++++++++++++++--- 6 files changed, 167 insertions(+), 45 deletions(-) diff --git a/examples/butterflies.js b/examples/butterflies.js index 6b15680fa0..e48095c345 100644 --- a/examples/butterflies.js +++ b/examples/butterflies.js @@ -13,6 +13,9 @@ // +var numButterflies = 20; + + function getRandomFloat(min, max) { return Math.random() * (max - min) + min; } @@ -73,7 +76,6 @@ function defineButterfly(entityID, targetPosition) { // Array of butterflies var butterflies = []; -var numButterflies = 20; function addButterfly() { // Decide the size of butterfly var color = { red: 100, green: 100, blue: 100 }; @@ -133,7 +135,8 @@ function updateButterflies(deltaTime) { // Update all the butterflies for (var i = 0; i < numButterflies; i++) { - entityID = butterflies[i].entityID; + entityID = Entities.identifyEntity(butterflies[i].entityID); + butterflies[i].entityID = entityID; var properties = Entities.getEntityProperties(entityID); if (properties.position.y > flockPosition.y + getRandomFloat(0.0,0.3)){ //0.3 //ceiling diff --git a/libraries/entities/src/EntityItemProperties.cpp b/libraries/entities/src/EntityItemProperties.cpp index 2ef307517f..f2f2483bb4 100644 --- a/libraries/entities/src/EntityItemProperties.cpp +++ b/libraries/entities/src/EntityItemProperties.cpp @@ -592,9 +592,9 @@ void EntityItemProperties::markAllChanged() { _glowLevelChanged = true; } -AACube EntityItemProperties::getMaximumAACubeInTreeUnits() const { +AACube EntityItemProperties::getMaximumAACubeInTreeUnits() const { AACube maxCube = getMaximumAACubeInMeters(); - maxCube.scale(1 / (float)TREE_SCALE); + maxCube.scale(1.0f / (float)TREE_SCALE); return maxCube; } @@ -611,7 +611,7 @@ AACube EntityItemProperties::getMaximumAACubeInMeters() const { glm::vec3 registrationPoint = (_dimensions * _registrationPoint); glm::vec3 registrationRemainder = (_dimensions * (glm::vec3(1.0f, 1.0f, 1.0f) - _registrationPoint)); glm::vec3 furthestExtentFromRegistration = glm::max(registrationPoint, registrationRemainder); - + // * we know that if you rotate in any direction you would create a sphere // that has a radius of the length of furthest extent from registration point float radius = glm::length(furthestExtentFromRegistration); diff --git a/libraries/entities/src/EntityItemPropertiesMacros.h b/libraries/entities/src/EntityItemPropertiesMacros.h index 46c78c09ea..16f883a36a 100644 --- a/libraries/entities/src/EntityItemPropertiesMacros.h +++ b/libraries/entities/src/EntityItemPropertiesMacros.h @@ -180,21 +180,25 @@ } \ } -#define COPY_PROPERTY_FROM_QSCRIPTVALUE_VEC3(P, S) \ - QScriptValue P = object.property(#P); \ - if (P.isValid()) { \ - QScriptValue x = P.property("x"); \ - QScriptValue y = P.property("y"); \ - QScriptValue z = P.property("z"); \ - if (x.isValid() && y.isValid() && z.isValid()) {\ - glm::vec3 newValue; \ - newValue.x = x.toVariant().toFloat(); \ - newValue.y = y.toVariant().toFloat(); \ - newValue.z = z.toVariant().toFloat(); \ - if (_defaultSettings || newValue != _##P) { \ - S(newValue); \ - } \ - } \ +#define COPY_PROPERTY_FROM_QSCRIPTVALUE_VEC3(P, S) \ + QScriptValue P = object.property(#P); \ + if (P.isValid()) { \ + QScriptValue x = P.property("x"); \ + QScriptValue y = P.property("y"); \ + QScriptValue z = P.property("z"); \ + if (x.isValid() && y.isValid() && z.isValid()) { \ + glm::vec3 newValue; \ + newValue.x = x.toVariant().toFloat(); \ + newValue.y = y.toVariant().toFloat(); \ + newValue.z = z.toVariant().toFloat(); \ + bool isValid = !glm::isnan(newValue.x) && \ + !glm::isnan(newValue.y) && \ + !glm::isnan(newValue.z); \ + if (isValid && \ + (_defaultSettings || newValue != _##P)) { \ + S(newValue); \ + } \ + } \ } #define COPY_PROPERTY_FROM_QSCRIPTVALUE_QUAT(P, S) \ @@ -210,7 +214,12 @@ newValue.y = y.toVariant().toFloat(); \ newValue.z = z.toVariant().toFloat(); \ newValue.w = w.toVariant().toFloat(); \ - if (_defaultSettings || newValue != _##P) { \ + bool isValid = !glm::isnan(newValue.x) && \ + !glm::isnan(newValue.y) && \ + !glm::isnan(newValue.z) && \ + !glm::isnan(newValue.w); \ + if (isValid && \ + (_defaultSettings || newValue != _##P)) { \ S(newValue); \ } \ } \ diff --git a/libraries/entities/src/EntityTree.cpp b/libraries/entities/src/EntityTree.cpp index 9a6e53b17a..58a34aeb60 100644 --- a/libraries/entities/src/EntityTree.cpp +++ b/libraries/entities/src/EntityTree.cpp @@ -501,7 +501,6 @@ int EntityTree::processEditPacketData(PacketType packetType, const unsigned char processedBytes = 0; break; } - return processedBytes; } diff --git a/libraries/entities/src/MovingEntitiesOperator.cpp b/libraries/entities/src/MovingEntitiesOperator.cpp index e6b48da232..045e07a910 100644 --- a/libraries/entities/src/MovingEntitiesOperator.cpp +++ b/libraries/entities/src/MovingEntitiesOperator.cpp @@ -60,8 +60,19 @@ void MovingEntitiesOperator::addEntityToMoveList(EntityItem* entity, const AACub qDebug() << " newCube:" << newCube; qDebug() << " oldCubeClamped:" << oldCubeClamped; qDebug() << " newCubeClamped:" << newCubeClamped; - qDebug() << " oldContainingElement:" << oldContainingElement->getAACube(); - qDebug() << " oldContainingElement->bestFitBounds(newCubeClamped):" << oldContainingElement->bestFitBounds(newCubeClamped); + if (oldContainingElement) { + qDebug() << " oldContainingElement:" << oldContainingElement->getAACube(); + qDebug() << " oldContainingElement->bestFitBounds(newCubeClamped):" + << oldContainingElement->bestFitBounds(newCubeClamped); + } else { + qDebug() << " WARNING NO OLD CONTAINING ELEMENT!!!"; + } + } + + if (!oldContainingElement) { + qDebug() << "UNEXPECTED!!!! attempting to move entity "<< entity->getEntityItemID() + << "that has no containing element. "; + return; // bail without adding. } // If the original containing element is the best fit for the requested newCube locations then diff --git a/libraries/entities/src/UpdateEntityOperator.cpp b/libraries/entities/src/UpdateEntityOperator.cpp index 284d8e9e24..2f70e96c60 100644 --- a/libraries/entities/src/UpdateEntityOperator.cpp +++ b/libraries/entities/src/UpdateEntityOperator.cpp @@ -36,6 +36,10 @@ UpdateEntityOperator::UpdateEntityOperator(EntityTree* tree, { // caller must have verified existence of containingElement and oldEntity assert(_containingElement && _existingEntity); + + if (_wantDebug) { + qDebug() << "UpdateEntityOperator::UpdateEntityOperator() -----------------------------"; + } // Here we have a choice to make, do we want to "tight fit" the actual minimum for the // entity into the the element, or do we want to use the entities "relaxed" bounds @@ -50,10 +54,19 @@ UpdateEntityOperator::UpdateEntityOperator(EntityTree* tree, if (_properties.containsPositionChange() && !_properties.containsDimensionsChange()) { glm::vec3 oldDimensionsInMeters = _existingEntity->getDimensions() * (float)TREE_SCALE; _properties.setDimensions(oldDimensionsInMeters); + + if (_wantDebug) { + qDebug() << " ** setting properties dimensions - had position change, no dimension change **"; + } + } if (!_properties.containsPositionChange() && _properties.containsDimensionsChange()) { glm::vec3 oldPositionInMeters = _existingEntity->getPosition() * (float)TREE_SCALE; _properties.setPosition(oldPositionInMeters); + + if (_wantDebug) { + qDebug() << " ** setting properties position - had dimensions change, no position change **"; + } } // If our new properties don't have bounds details (no change to position, etc) or if this containing element would @@ -64,6 +77,11 @@ UpdateEntityOperator::UpdateEntityOperator(EntityTree* tree, // if we don't have bounds properties, then use our old clamped box to determine best fit if (!_properties.containsBoundsProperties()) { oldElementBestFit = _containingElement->bestFitBounds(_oldEntityBox); + + if (_wantDebug) { + qDebug() << " ** old Element best fit - no dimensions change, no position change **"; + } + } // For some reason we've seen a case where the original containing element isn't a best fit for the old properties @@ -71,20 +89,36 @@ UpdateEntityOperator::UpdateEntityOperator(EntityTree* tree, if (!_properties.containsBoundsProperties() && !oldElementBestFit) { _newEntityCube = _oldEntityCube; _removeOld = true; // our properties are going to move us, so remember this for later processing + + if (_wantDebug) { + qDebug() << " **** UNUSUAL CASE **** no changes, but not best fit... consider it a move.... **"; + } + + } else if (!_properties.containsBoundsProperties() || oldElementBestFit) { _foundOld = true; _newEntityCube = _oldEntityCube; _dontMove = true; + + if (_wantDebug) { + qDebug() << " **** TYPICAL NO MOVE CASE ****"; + qDebug() << " _properties.containsBoundsProperties():" << _properties.containsBoundsProperties(); + qDebug() << " oldElementBestFit:" << oldElementBestFit; + } + } else { _newEntityCube = _properties.getMaximumAACubeInTreeUnits(); _removeOld = true; // our properties are going to move us, so remember this for later processing + + if (_wantDebug) { + qDebug() << " **** TYPICAL MOVE CASE ****"; + } } _newEntityBox = _newEntityCube.clamp(0.0f, 1.0f); // clamp to domain bounds if (_wantDebug) { - qDebug() << "UpdateEntityOperator::UpdateEntityOperator() -----------------------------"; qDebug() << " _entityItemID:" << _entityItemID; qDebug() << " _containingElementCube:" << _containingElementCube; qDebug() << " _oldEntityCube:" << _oldEntityCube; @@ -108,31 +142,30 @@ bool UpdateEntityOperator::subTreeContainsOldEntity(OctreeElement* element) { // so when we're searching the tree for the old element, we use the known cube for the known containing element bool elementContainsOldBox = element->getAACube().contains(_containingElementCube); - /* - bool elementContainsOldCube = element->getAACube().contains(_oldEntityCube); - qDebug() << "UpdateEntityOperator::subTreeContainsOldEntity()...."; - qDebug() << " element->getAACube()=" << element->getAACube(); - qDebug() << " _oldEntityCube=" << _oldEntityCube; - qDebug() << " _oldEntityBox=" << _oldEntityBox; - qDebug() << " elementContainsOldCube=" << elementContainsOldCube; - qDebug() << " elementContainsOldBox=" << elementContainsOldBox; - */ - + if (_wantDebug) { + bool elementContainsOldCube = element->getAACube().contains(_oldEntityCube); + qDebug() << "UpdateEntityOperator::subTreeContainsOldEntity()...."; + qDebug() << " element->getAACube()=" << element->getAACube(); + qDebug() << " _oldEntityCube=" << _oldEntityCube; + qDebug() << " _oldEntityBox=" << _oldEntityBox; + qDebug() << " elementContainsOldCube=" << elementContainsOldCube; + qDebug() << " elementContainsOldBox=" << elementContainsOldBox; + } return elementContainsOldBox; } bool UpdateEntityOperator::subTreeContainsNewEntity(OctreeElement* element) { bool elementContainsNewBox = element->getAACube().contains(_newEntityBox); - /* - bool elementContainsNewCube = element->getAACube().contains(_newEntityCube); - qDebug() << "UpdateEntityOperator::subTreeContainsNewEntity()...."; - qDebug() << " element->getAACube()=" << element->getAACube(); - qDebug() << " _newEntityCube=" << _newEntityCube; - qDebug() << " _newEntityBox=" << _newEntityBox; - qDebug() << " elementContainsNewCube=" << elementContainsNewCube; - qDebug() << " elementContainsNewBox=" << elementContainsNewBox; - */ + if (_wantDebug) { + bool elementContainsNewCube = element->getAACube().contains(_newEntityCube); + qDebug() << "UpdateEntityOperator::subTreeContainsNewEntity()...."; + qDebug() << " element->getAACube()=" << element->getAACube(); + qDebug() << " _newEntityCube=" << _newEntityCube; + qDebug() << " _newEntityBox=" << _newEntityBox; + qDebug() << " elementContainsNewCube=" << elementContainsNewCube; + qDebug() << " elementContainsNewBox=" << elementContainsNewBox; + } return elementContainsNewBox; } @@ -155,18 +188,42 @@ bool UpdateEntityOperator::preRecursion(OctreeElement* element) { bool subtreeContainsOld = subTreeContainsOldEntity(element); bool subtreeContainsNew = subTreeContainsNewEntity(element); + if (_wantDebug) { + qDebug() << "---- UpdateEntityOperator::preRecursion().... ----"; + qDebug() << " element=" << element->getAACube(); + qDebug() << " subtreeContainsOld=" << subtreeContainsOld; + qDebug() << " subtreeContainsNew=" << subtreeContainsNew; + qDebug() << " _foundOld=" << _foundOld; + qDebug() << " _foundNew=" << _foundNew; + } + // If we haven't yet found the old entity, and this subTreeContains our old // entity, then we need to keep searching. if (!_foundOld && subtreeContainsOld) { - + + if (_wantDebug) { + qDebug() << " OLD TREE CASE...."; + qDebug() << " entityTreeElement=" << entityTreeElement; + qDebug() << " _containingElement=" << _containingElement; + } + // If this is the element we're looking for, then ask it to remove the old entity // and we can stop searching. if (entityTreeElement == _containingElement) { + if (_wantDebug) { + qDebug() << " *** it's the OLD ELEMENT! ***"; + } + // If the containgElement IS NOT the best fit for the new entity properties // then we need to remove it, and the updateEntity below will store it in the // correct element. if (_removeOld) { + + if (_wantDebug) { + qDebug() << " *** REMOVING from ELEMENT ***"; + } + entityTreeElement->removeEntityItem(_existingEntity); // NOTE: only removes the entity, doesn't delete it // If we haven't yet found the new location, then we need to @@ -174,6 +231,11 @@ bool UpdateEntityOperator::preRecursion(OctreeElement* element) { // now we're not in that map if (!_foundNew) { _tree->setContainingElement(_entityItemID, NULL); + + if (_wantDebug) { + qDebug() << " *** REMOVING from MAP ***"; + } + } } _foundOld = true; @@ -187,11 +249,27 @@ bool UpdateEntityOperator::preRecursion(OctreeElement* element) { // entity, then we need to keep searching. if (!_foundNew && subtreeContainsNew) { + if (_wantDebug) { + qDebug() << " NEW TREE CASE...."; + qDebug() << " entityTreeElement=" << entityTreeElement; + qDebug() << " _containingElement=" << _containingElement; + 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)) { + if (_wantDebug) { + qDebug() << " *** THIS ELEMENT IS BEST FIT ***"; + } + // if we are the existing containing element, then we can just do the update of the entity properties if (entityTreeElement == _containingElement) { + + 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 @@ -201,21 +279,43 @@ bool UpdateEntityOperator::preRecursion(OctreeElement* element) { 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) { + qDebug() << " *** set properties ***"; + } } else { // otherwise, this is an add case. entityTreeElement->addEntityItem(_existingEntity); _existingEntity->setProperties(_properties); // still need to update the properties! _tree->setContainingElement(_entityItemID, entityTreeElement); + if (_wantDebug) { + qDebug() << " *** ADDING ENTITY to ELEMENT and MAP and SETTING PROPERTIES ***"; + } } _foundNew = true; // we found the new item! } else { keepSearching = true; } } + + if (_wantDebug) { + qDebug() << " FINAL --- keepSearching=" << keepSearching; + qDebug() << "--------------------------------------------------"; + } + return keepSearching; // if we haven't yet found it, keep looking }