Merge pull request #10476 from sethalves/fix-imported-children-querybox

fix up queryAACubes before sending imported entities to server

libraries/entities/src/AddEntityOperator.cpp

@@ -27,10 +27,6 @@ AddEntityOperator::AddEntityOperator(EntityTreePointer tree, EntityItemPointer n
 
     bool success;
     auto queryCube = _newEntity->getQueryAACube(success);
-    if (!success) {
-        _newEntity->markAncestorMissing(true);
-    }
-
     _newEntityBox = queryCube.clamp((float)(-HALF_TREE_SCALE), (float)HALF_TREE_SCALE);
 }
 

libraries/entities/src/EntityItem.cpp

@@ -1374,7 +1374,7 @@ bool EntityItem::setProperties(const EntityItemProperties& properties) {
         somethingChanged = true;
     }
 
-    // Now check the sub classes 
+    // Now check the sub classes
     somethingChanged |= setSubClassProperties(properties);
 
     // Finally notify if change detected
@@ -1606,11 +1606,19 @@ void EntityItem::updateRegistrationPoint(const glm::vec3& value) {
 void EntityItem::updatePosition(const glm::vec3& value) {
     if (getLocalPosition() != value) {
         setLocalPosition(value);
+
+        EntityTreePointer tree = getTree();
         markDirtyFlags(Simulation::DIRTY_POSITION);
+        if (tree) {
+            tree->entityChanged(getThisPointer());
+        }
         forEachDescendant([&](SpatiallyNestablePointer object) {
             if (object->getNestableType() == NestableType::Entity) {
                 EntityItemPointer entity = std::static_pointer_cast<EntityItem>(object);
                 entity->markDirtyFlags(Simulation::DIRTY_POSITION);
+                if (tree) {
+                    tree->entityChanged(entity);
+                }
             }
         });
     }
@@ -1622,6 +1630,11 @@ void EntityItem::updateParentID(const QUuid& value) {
         // children are forced to be kinematic
         // may need to not collide with own avatar
         markDirtyFlags(Simulation::DIRTY_MOTION_TYPE | Simulation::DIRTY_COLLISION_GROUP);
+
+        EntityTreePointer tree = getTree();
+        if (tree) {
+            tree->addToNeedsParentFixupList(getThisPointer());
+        }
     }
 }
 

libraries/entities/src/EntityItem.h

@@ -187,7 +187,7 @@ public:
 
     const Transform getTransformToCenter(bool& success) const;
 
-    inline void requiresRecalcBoxes();
+    void requiresRecalcBoxes();
 
     // Hyperlink related getters and setters
     QString getHref() const;
@@ -476,6 +476,8 @@ public:
 
     virtual bool getMeshes(MeshProxyList& result) { return true; }
 
+    virtual void locationChanged(bool tellPhysics = true) override;
+
 protected:
 
     void setSimulated(bool simulated) { _simulated = simulated; }
@@ -483,7 +485,6 @@ protected:
     const QByteArray getDynamicDataInternal() const;
     void setDynamicDataInternal(QByteArray dynamicData);
 
-    virtual void locationChanged(bool tellPhysics = true) override;
     virtual void dimensionsChanged() override;
 
     EntityTypes::EntityType _type;

libraries/entities/src/EntityTree.cpp

@@ -122,16 +122,15 @@ void EntityTree::postAddEntity(EntityItemPointer entity) {
         _simulation->addEntity(entity);
     }
 
-    if (!entity->isParentIDValid()) {
+    if (!entity->getParentID().isNull()) {
-        QWriteLocker locker(&_missingParentLock);
+        addToNeedsParentFixupList(entity);
-        _missingParent.append(entity);
     }
 
     _isDirty = true;
     emit addingEntity(entity->getEntityItemID());
 
     // find and hook up any entities with this entity as a (previously) missing parent
-    fixupMissingParents();
+    fixupNeedsParentFixups();
 }
 
 bool EntityTree::updateEntity(const EntityItemID& entityID, const EntityItemProperties& properties, const SharedNodePointer& senderNode) {
@@ -291,13 +290,11 @@ bool EntityTree::updateEntityWithElement(EntityItemPointer entity, const EntityI
             bool success;
             AACube queryCube = childEntity->getQueryAACube(success);
             if (!success) {
-                QWriteLocker locker(&_missingParentLock);
+                addToNeedsParentFixupList(childEntity);
-                _missingParent.append(childEntity);
                 continue;
             }
-            if (!childEntity->isParentIDValid()) {
+            if (!childEntity->getParentID().isNull()) {
-                QWriteLocker locker(&_missingParentLock);
+                addToNeedsParentFixupList(childEntity);
-                _missingParent.append(childEntity);
             }
 
             UpdateEntityOperator theChildOperator(getThisPointer(), containingElement, childEntity, queryCube);
@@ -383,11 +380,8 @@ EntityItemPointer EntityTree::addEntity(const EntityItemID& entityID, const Enti
         // Recurse the tree and store the entity in the correct tree element
         AddEntityOperator theOperator(getThisPointer(), result);
         recurseTreeWithOperator(&theOperator);
-        if (result->getAncestorMissing()) {
+        if (!result->getParentID().isNull()) {
-            // we added the entity, but didn't know about all its ancestors, so it went into the wrong place.
+            addToNeedsParentFixupList(result);
-            // add it to a list of entities needing to be fixed once their parents are known.
-            QWriteLocker locker(&_missingParentLock);
-            _missingParent.append(result);
         }
 
         postAddEntity(result);
@@ -1254,60 +1248,68 @@ void EntityTree::entityChanged(EntityItemPointer entity) {
     }
 }
 
-void EntityTree::fixupMissingParents() {
+
+void EntityTree::fixupNeedsParentFixups() {
     MovingEntitiesOperator moveOperator(getThisPointer());
 
-    QList<EntityItemPointer> missingParents;
+    QWriteLocker locker(&_needsParentFixupLock);
-    {
+
-        QWriteLocker locker(&_missingParentLock);
+    QMutableVectorIterator<EntityItemWeakPointer> iter(_needsParentFixup);
-        QMutableVectorIterator<EntityItemWeakPointer> iter(_missingParent);
+    while (iter.hasNext()) {
-        while (iter.hasNext()) {
+        EntityItemWeakPointer entityWP = iter.next();
-            EntityItemWeakPointer entityWP = iter.next();
+        EntityItemPointer entity = entityWP.lock();
-            EntityItemPointer entity = entityWP.lock();
+        if (!entity) {
-            if (entity) {
+            // entity was deleted before we found its parent
-                if (entity->isParentIDValid()) {
+            iter.remove();
-                    iter.remove();
+            continue;
-                } else {
+        }
-                    missingParents.append(entity);
+
-                }
+        entity->requiresRecalcBoxes();
-            } else {
+        bool queryAACubeSuccess { false };
-                // entity was deleted before we found its parent.
+        bool maxAACubeSuccess { false };
-                iter.remove();
+        AACube newCube = entity->getQueryAACube(queryAACubeSuccess);
+        if (queryAACubeSuccess) {
+            // make sure queryAACube encompasses maxAACube
+            AACube maxAACube = entity->getMaximumAACube(maxAACubeSuccess);
+            if (maxAACubeSuccess && !newCube.contains(maxAACube)) {
+                newCube = maxAACube;
             }
         }
-    }
 
-    for (EntityItemPointer entity : missingParents) {
+        bool doMove = false;
-        if (entity) {
+        if (entity->isParentIDValid() && maxAACubeSuccess) { // maxAACubeSuccess of true means all ancestors are known
-            bool queryAACubeSuccess;
+            iter.remove(); // this entity is all hooked up; we can remove it from the list
-            AACube newCube = entity->getQueryAACube(queryAACubeSuccess);
+            // this entity's parent was previously not known, and now is.  Update its location in the EntityTree...
-            if (queryAACubeSuccess) {
+            doMove = true;
-                // make sure queryAACube encompasses maxAACube
+            // the bounds on the render-item may need to be updated, the rigid body in the physics engine may
-                bool maxAACubeSuccess;
+            // need to be moved.
-                AACube maxAACube = entity->getMaximumAACube(maxAACubeSuccess);
+            entity->markDirtyFlags(Simulation::DIRTY_MOTION_TYPE |
-                if (maxAACubeSuccess && !newCube.contains(maxAACube)) {
+                                   Simulation::DIRTY_COLLISION_GROUP |
-                    newCube = maxAACube;
+                                   Simulation::DIRTY_TRANSFORM);
+            entityChanged(entity);
+            entity->forEachDescendant([&](SpatiallyNestablePointer object) {
+                if (object->getNestableType() == NestableType::Entity) {
+                    EntityItemPointer descendantEntity = std::static_pointer_cast<EntityItem>(object);
+                    descendantEntity->markDirtyFlags(Simulation::DIRTY_MOTION_TYPE |
+                                                     Simulation::DIRTY_COLLISION_GROUP |
+                                                     Simulation::DIRTY_TRANSFORM);
+                    entityChanged(descendantEntity);
                 }
+            });
+            entity->locationChanged(true);
+        } else if (getIsServer() && _avatarIDs.contains(entity->getParentID())) {
+            // this is a child of an avatar, which the entity server will never have
+            // a SpatiallyNestable object for.  Add it to a list for cleanup when the avatar leaves.
+            if (!_childrenOfAvatars.contains(entity->getParentID())) {
+                _childrenOfAvatars[entity->getParentID()] = QSet<EntityItemID>();
             }
+            _childrenOfAvatars[entity->getParentID()] += entity->getEntityItemID();
+            doMove = true;
+            iter.remove(); // and pull it out of the list
+        }
 
-            bool doMove = false;
+        if (queryAACubeSuccess && doMove) {
-            if (entity->isParentIDValid()) {
+            moveOperator.addEntityToMoveList(entity, newCube);
-                // this entity's parent was previously not known, and now is.  Update its location in the EntityTree...
-                doMove = true;
-            } else if (getIsServer() && _avatarIDs.contains(entity->getParentID())) {
-                // this is a child of an avatar, which the entity server will never have
-                // a SpatiallyNestable object for.  Add it to a list for cleanup when the avatar leaves.
-                if (!_childrenOfAvatars.contains(entity->getParentID())) {
-                    _childrenOfAvatars[entity->getParentID()] = QSet<EntityItemID>();
-                }
-                _childrenOfAvatars[entity->getParentID()] += entity->getEntityItemID();
-                doMove = true;
-            }
-
-            if (queryAACubeSuccess && doMove) {
-                moveOperator.addEntityToMoveList(entity, newCube);
-                entity->markAncestorMissing(false);
-            }
         }
     }
 
@@ -1324,8 +1326,13 @@ void EntityTree::deleteDescendantsOfAvatar(QUuid avatarID) {
     }
 }
 
+void EntityTree::addToNeedsParentFixupList(EntityItemPointer entity) {
+    QWriteLocker locker(&_needsParentFixupLock);
+    _needsParentFixup.append(entity);
+}
+
 void EntityTree::update() {
-    fixupMissingParents();
+    fixupNeedsParentFixups();
     if (_simulation) {
         withWriteLock([&] {
             _simulation->updateEntities();
@@ -1626,31 +1633,61 @@ QByteArray EntityTree::remapActionDataIDs(QByteArray actionData, QHash<EntityIte
 QVector<EntityItemID> EntityTree::sendEntities(EntityEditPacketSender* packetSender, EntityTreePointer localTree,
                                                float x, float y, float z) {
     SendEntitiesOperationArgs args;
-    args.packetSender = packetSender;
     args.ourTree = this;
     args.otherTree = localTree;
     args.root = glm::vec3(x, y, z);
-    // If this is called repeatedly (e.g., multiple pastes with the same data), the new elements will clash unless we use new identifiers.
+    // If this is called repeatedly (e.g., multiple pastes with the same data), the new elements will clash unless we
-    // We need to keep a map so that we can map parent identifiers correctly.
+    // use new identifiers.  We need to keep a map so that we can map parent identifiers correctly.
     QHash<EntityItemID, EntityItemID> map;
     args.map = &map;
     withReadLock([&] {
         recurseTreeWithOperation(sendEntitiesOperation, &args);
     });
-    packetSender->releaseQueuedMessages();
 
-    // the values from map are used as the list of successfully "sent" entities.  If some didn't actually make it,
+    // The values from map are used as the list of successfully "sent" entities.  If some didn't actually make it,
     // pull them out.  Bogus entries could happen if part of the imported data makes some reference to an entity
-    // that isn't in the data being imported.
+    // that isn't in the data being imported.  For those that made it, fix up their queryAACubes and send an
+    // add-entity packet to the server.
+
+    // fix the queryAACubes of any children that were read in before their parents, get them into the correct element
+    MovingEntitiesOperator moveOperator(localTree);
     QHash<EntityItemID, EntityItemID>::iterator i = map.begin();
     while (i != map.end()) {
         EntityItemID newID = i.value();
-        if (localTree->findEntityByEntityItemID(newID)) {
+        EntityItemPointer entity = localTree->findEntityByEntityItemID(newID);
+        if (entity) {
+            if (!entity->getParentID().isNull()) {
+                addToNeedsParentFixupList(entity);
+            }
+            entity->forceQueryAACubeUpdate();
+            moveOperator.addEntityToMoveList(entity, entity->getQueryAACube());
             i++;
         } else {
             i = map.erase(i);
         }
     }
+    if (moveOperator.hasMovingEntities()) {
+        PerformanceTimer perfTimer("recurseTreeWithOperator");
+        localTree->recurseTreeWithOperator(&moveOperator);
+    }
+
+    // send add-entity packets to the server
+    i = map.begin();
+    while (i != map.end()) {
+        EntityItemID newID = i.value();
+        EntityItemPointer entity = localTree->findEntityByEntityItemID(newID);
+        if (entity) {
+            // queue the packet to send to the server
+            entity->computePuffedQueryAACube();
+            EntityItemProperties properties = entity->getProperties();
+            properties.markAllChanged(); // so the entire property set is considered new, since we're making a new entity
+            packetSender->queueEditEntityMessage(PacketType::EntityAdd, localTree, newID, properties);
+            i++;
+        } else {
+            i = map.erase(i);
+        }
+    }
+    packetSender->releaseQueuedMessages();
 
     return map.values().toVector();
 }
@@ -1704,14 +1741,9 @@ bool EntityTree::sendEntitiesOperation(OctreeElementPointer element, void* extra
         // set creation time to "now" for imported entities
         properties.setCreated(usecTimestampNow());
 
-        properties.markAllChanged(); // so the entire property set is considered new, since we're making a new entity
-
         EntityTreeElementPointer entityTreeElement = std::static_pointer_cast<EntityTreeElement>(element);
         EntityTreePointer tree = entityTreeElement->getTree();
 
-        // queue the packet to send to the server
-        args->packetSender->queueEditEntityMessage(PacketType::EntityAdd, tree, newID, properties);
-
         // also update the local tree instantly (note: this is not our tree, but an alternate tree)
         if (args->otherTree) {
             args->otherTree->withWriteLock([&] {

libraries/entities/src/EntityTree.h

@@ -53,7 +53,6 @@ public:
     glm::vec3 root;
     EntityTree* ourTree;
     EntityTreePointer otherTree;
-    EntityEditPacketSender* packetSender;
     QHash<EntityItemID, EntityItemID>* map;
 };
 
@@ -272,6 +271,8 @@ public:
     void forgetAvatarID(QUuid avatarID) { _avatarIDs -= avatarID; }
     void deleteDescendantsOfAvatar(QUuid avatarID);
 
+    void addToNeedsParentFixupList(EntityItemPointer entity);
+
     void notifyNewCollisionSoundURL(const QString& newCollisionSoundURL, const EntityItemID& entityID);
 
     static const float DEFAULT_MAX_TMP_ENTITY_LIFETIME;
@@ -354,9 +355,9 @@ protected:
     quint64 _maxEditDelta = 0;
     quint64 _treeResetTime = 0;
 
-    void fixupMissingParents(); // try to hook members of _missingParent to parent instances
+    void fixupNeedsParentFixups(); // try to hook members of _needsParentFixup to parent instances
-    QVector<EntityItemWeakPointer> _missingParent; // entites with a parentID but no (yet) known parent instance
+    QVector<EntityItemWeakPointer> _needsParentFixup; // entites with a parentID but no (yet) known parent instance
-    mutable QReadWriteLock _missingParentLock;
+    mutable QReadWriteLock _needsParentFixupLock;
 
     // we maintain a list of avatarIDs to notice when an entity is a child of one.
     QSet<QUuid> _avatarIDs; // IDs of avatars connected to entity server

libraries/entities/src/EntityTreeElement.cpp

@@ -208,7 +208,7 @@ void EntityTreeElement::elementEncodeComplete(EncodeBitstreamParams& params) con
 
             // why would this ever fail???
             // If we've encoding this element before... but we're coming back a second time in an attempt to
-            // encoud our parent... this might happen.
+            // encode our parent... this might happen.
             if (extraEncodeData->contains(childElement.get())) {
                 EntityTreeElementExtraEncodeDataPointer childExtraEncodeData
                     = std::static_pointer_cast<EntityTreeElementExtraEncodeData>((*extraEncodeData)[childElement.get()]);
@@ -981,6 +981,7 @@ int EntityTreeElement::readElementDataFromBuffer(const unsigned char* data, int
                 //    3) remember the old cube for the entity so we can mark it as dirty
                 if (entityItem) {
                     QString entityScriptBefore = entityItem->getScript();
+                    QUuid parentIDBefore = entityItem->getParentID();
                     QString entityServerScriptsBefore = entityItem->getServerScripts();
                     quint64 entityScriptTimestampBefore = entityItem->getScriptTimestamp();
                     bool bestFitBefore = bestFitEntityBounds(entityItem);
@@ -1018,6 +1019,11 @@ int EntityTreeElement::readElementDataFromBuffer(const unsigned char* data, int
                         _myTree->emitEntityServerScriptChanging(entityItemID, reload); // the entity server script has changed
                     }
 
+                    QUuid parentIDAfter = entityItem->getParentID();
+                    if (parentIDBefore != parentIDAfter) {
+                        _myTree->addToNeedsParentFixupList(entityItem);
+                    }
+
                 } else {
                     entityItem = EntityTypes::constructEntityItem(dataAt, bytesLeftToRead, args);
                     if (entityItem) {

libraries/shared/src/SpatiallyNestable.cpp

@@ -946,12 +946,13 @@ AACube SpatiallyNestable::getMaximumAACube(bool& success) const {
     return AACube(getPosition(success) - glm::vec3(defaultAACubeSize / 2.0f), defaultAACubeSize);
 }
 
-void SpatiallyNestable::checkAndAdjustQueryAACube() {
+bool SpatiallyNestable::checkAndAdjustQueryAACube() {
     bool success;
     AACube maxAACube = getMaximumAACube(success);
     if (success && (!_queryAACubeSet || !_queryAACube.contains(maxAACube))) {
         setQueryAACube(maxAACube);
     }
+    return success;
 }
 
 void SpatiallyNestable::setQueryAACube(const AACube& queryAACube) {

libraries/shared/src/SpatiallyNestable.h

@@ -102,11 +102,12 @@ public:
     virtual glm::vec3 getParentAngularVelocity(bool& success) const;
 
     virtual AACube getMaximumAACube(bool& success) const;
-    virtual void checkAndAdjustQueryAACube();
+    virtual bool checkAndAdjustQueryAACube();
     virtual bool computePuffedQueryAACube();
 
     virtual void setQueryAACube(const AACube& queryAACube);
     virtual bool queryAABoxNeedsUpdate() const;
+    void forceQueryAACubeUpdate() { _queryAACubeSet = false; }
     virtual AACube getQueryAACube(bool& success) const;
     virtual AACube getQueryAACube() const;
 
@@ -157,9 +158,6 @@ public:
 
     SpatiallyNestablePointer getThisPointer() const;
 
-    void markAncestorMissing(bool value) { _missingAncestor = value; }
-    bool getAncestorMissing() { return _missingAncestor; }
-
     void forEachChild(std::function<void(SpatiallyNestablePointer)> actor);
     void forEachDescendant(std::function<void(SpatiallyNestablePointer)> actor);
 
@@ -206,7 +204,6 @@ protected:
     mutable AACube _queryAACube;
     mutable bool _queryAACubeSet { false };
 
-    bool _missingAncestor { false };
     quint64 _scaleChanged { 0 };
     quint64 _translationChanged { 0 };
     quint64 _rotationChanged { 0 };