From ce3faa2916a0b9c56ba67940c3013d5203bd1329 Mon Sep 17 00:00:00 2001 From: Seth Alves Date: Wed, 11 May 2016 18:47:20 -0700 Subject: [PATCH 1/7] fix logic in UpdateEntityOperator --- libraries/entities/src/UpdateEntityOperator.cpp | 7 ++----- libraries/entities/src/UpdateEntityOperator.h | 1 - 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/libraries/entities/src/UpdateEntityOperator.cpp b/libraries/entities/src/UpdateEntityOperator.cpp index 94599496b0..cfd7498cdf 100644 --- a/libraries/entities/src/UpdateEntityOperator.cpp +++ b/libraries/entities/src/UpdateEntityOperator.cpp @@ -23,7 +23,6 @@ UpdateEntityOperator::UpdateEntityOperator(EntityTreePointer tree, _foundOld(false), _foundNew(false), _removeOld(false), - _dontMove(false), // assume we'll be moving _changeTime(usecTimestampNow()), _oldEntityCube(), _newEntityCube(), @@ -46,12 +45,11 @@ UpdateEntityOperator::UpdateEntityOperator(EntityTreePointer tree, // If our new properties don't have bounds details (no change to position, etc) or if this containing element would // be the best fit for our new properties, then just do the new portion of the store pass, since the change path will // be the same for both parts of the update - bool oldElementBestFit = _containingElement->bestFitBounds(newQueryAACube); + bool oldElementBestFit = _containingElement->bestFitBounds(_oldEntityBox); // For some reason we've seen a case where the original containing element isn't a best fit for the old properties // in this case we want to move it, even if the properties haven't changed. if (!oldElementBestFit) { - _newEntityCube = _oldEntityCube; _removeOld = true; // our properties are going to move us, so remember this for later processing if (_wantDebug) { @@ -59,14 +57,13 @@ UpdateEntityOperator::UpdateEntityOperator(EntityTreePointer tree, } } else { _foundOld = true; - _newEntityCube = _oldEntityCube; - _dontMove = true; if (_wantDebug) { qCDebug(entities) << " **** TYPICAL NO MOVE CASE **** oldElementBestFit:" << oldElementBestFit; } } + _newEntityCube = newQueryAACube; _newEntityBox = _newEntityCube.clamp((float)-HALF_TREE_SCALE, (float)HALF_TREE_SCALE); // clamp to domain bounds if (_wantDebug) { diff --git a/libraries/entities/src/UpdateEntityOperator.h b/libraries/entities/src/UpdateEntityOperator.h index ea6faabe3e..8b5bbdf135 100644 --- a/libraries/entities/src/UpdateEntityOperator.h +++ b/libraries/entities/src/UpdateEntityOperator.h @@ -37,7 +37,6 @@ private: bool _foundOld; bool _foundNew; bool _removeOld; - bool _dontMove; quint64 _changeTime; AACube _oldEntityCube; From 63073301ee9735821af9301ee51402b2a763eb1c Mon Sep 17 00:00:00 2001 From: Seth Alves Date: Thu, 12 May 2016 11:16:42 -0700 Subject: [PATCH 2/7] try again to fix update logic --- .../entities/src/UpdateEntityOperator.cpp | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/libraries/entities/src/UpdateEntityOperator.cpp b/libraries/entities/src/UpdateEntityOperator.cpp index cfd7498cdf..a48f3f7198 100644 --- a/libraries/entities/src/UpdateEntityOperator.cpp +++ b/libraries/entities/src/UpdateEntityOperator.cpp @@ -42,6 +42,9 @@ UpdateEntityOperator::UpdateEntityOperator(EntityTreePointer tree, _oldEntityCube = _existingEntity->getQueryAACube(); _oldEntityBox = _oldEntityCube.clamp((float)-HALF_TREE_SCALE, (float)HALF_TREE_SCALE); // clamp to domain bounds + _newEntityCube = newQueryAACube; + _newEntityBox = _newEntityCube.clamp((float)-HALF_TREE_SCALE, (float)HALF_TREE_SCALE); // clamp to domain bounds + // If our new properties don't have bounds details (no change to position, etc) or if this containing element would // be the best fit for our new properties, then just do the new portion of the store pass, since the change path will // be the same for both parts of the update @@ -49,23 +52,19 @@ UpdateEntityOperator::UpdateEntityOperator(EntityTreePointer tree, // For some reason we've seen a case where the original containing element isn't a best fit for the old properties // in this case we want to move it, even if the properties haven't changed. - if (!oldElementBestFit) { + if (oldElementBestFit) { + if (_wantDebug) { + qCDebug(entities) << " **** TYPICAL NO MOVE CASE **** oldElementBestFit:" << oldElementBestFit; + } + } else { + _oldEntityBox = _existingEntity->getElement()->getAACube(); _removeOld = true; // our properties are going to move us, so remember this for later processing if (_wantDebug) { qCDebug(entities) << " **** UNUSUAL CASE **** no changes, but not best fit... consider it a move.... **"; } - } else { - _foundOld = true; - - if (_wantDebug) { - qCDebug(entities) << " **** TYPICAL NO MOVE CASE **** oldElementBestFit:" << oldElementBestFit; - } } - _newEntityCube = newQueryAACube; - _newEntityBox = _newEntityCube.clamp((float)-HALF_TREE_SCALE, (float)HALF_TREE_SCALE); // clamp to domain bounds - if (_wantDebug) { qCDebug(entities) << " _entityItemID:" << _entityItemID; qCDebug(entities) << " _containingElementCube:" << _containingElementCube; From dd2a29aace35ac6a30c9be6a9ba854a7e5839dae Mon Sep 17 00:00:00 2001 From: Seth Alves Date: Thu, 12 May 2016 18:05:28 -0700 Subject: [PATCH 3/7] when a ModelEntityItem moves, also update its meta-render-item --- .../src/RenderableModelEntityItem.cpp | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/libraries/entities-renderer/src/RenderableModelEntityItem.cpp b/libraries/entities-renderer/src/RenderableModelEntityItem.cpp index c4ac9b09e5..e13c2eac98 100644 --- a/libraries/entities-renderer/src/RenderableModelEntityItem.cpp +++ b/libraries/entities-renderer/src/RenderableModelEntityItem.cpp @@ -807,6 +807,16 @@ void RenderableModelEntityItem::locationChanged(bool tellPhysics) { if (_model && _model->isActive()) { _model->setRotation(getRotation()); _model->setTranslation(getPosition()); + + auto myMetaItemCopy = _myMetaItem; + + void* key = (void*)this; + AbstractViewStateInterface::instance()->pushPostUpdateLambda(key, [_myMetaItem]() { + render::ScenePointer scene = AbstractViewStateInterface::instance()->getMain3DScene(); + render::PendingChanges pendingChanges; + pendingChanges.updateItem(myMetaItemCopy, [](RenderableModelEntityItemMeta& data){}); + scene->enqueuePendingChanges(pendingChanges); + }); } } From 79141e38698f38226ae266337f4700292ed87958 Mon Sep 17 00:00:00 2001 From: Seth Alves Date: Thu, 12 May 2016 17:52:40 -0700 Subject: [PATCH 4/7] dry up some code --- .../src/RenderableModelEntityItem.cpp | 43 ++++++++----------- .../src/RenderableModelEntityItem.h | 2 + 2 files changed, 19 insertions(+), 26 deletions(-) diff --git a/libraries/entities-renderer/src/RenderableModelEntityItem.cpp b/libraries/entities-renderer/src/RenderableModelEntityItem.cpp index e13c2eac98..d6f8eb23c8 100644 --- a/libraries/entities-renderer/src/RenderableModelEntityItem.cpp +++ b/libraries/entities-renderer/src/RenderableModelEntityItem.cpp @@ -162,6 +162,19 @@ void RenderableModelEntityItem::remapTextures() { } } +void RenderableModelEntityItem::doInitialModelSimulation() { + _model->setScaleToFit(true, getDimensions()); + _model->setSnapModelToRegistrationPoint(true, getRegistrationPoint()); + _model->setRotation(getRotation()); + _model->setTranslation(getPosition()); + { + PerformanceTimer perfTimer("_model->simulate"); + _model->simulate(0.0f); + } + _needsInitialSimulation = false; +} + + // TODO: we need a solution for changes to the postion/rotation/etc of a model... // this current code path only addresses that in this setup case... not the changing/moving case bool RenderableModelEntityItem::readyToAddToScene(RenderArgs* renderArgs) { @@ -172,22 +185,12 @@ bool RenderableModelEntityItem::readyToAddToScene(RenderArgs* renderArgs) { getModel(renderer); } if (renderArgs && _model && _needsInitialSimulation && _model->isActive() && _model->isLoaded()) { - _model->setScaleToFit(true, getDimensions()); - _model->setSnapModelToRegistrationPoint(true, getRegistrationPoint()); - _model->setRotation(getRotation()); - _model->setTranslation(getPosition()); - // make sure to simulate so everything gets set up correctly for rendering - { - PerformanceTimer perfTimer("_model->simulate"); - _model->simulate(0.0f); - } - _needsInitialSimulation = false; - + doInitialModelSimulation(); _model->renderSetup(renderArgs); } bool ready = !_needsInitialSimulation && _model && _model->readyToAddToScene(renderArgs); - return ready; + return ready; } class RenderableModelEntityItemMeta { @@ -348,18 +351,7 @@ void RenderableModelEntityItem::updateModelBounds() { _model->getRotation() != getRotation() || _model->getRegistrationPoint() != getRegistrationPoint()) && _model->isActive() && _dimensionsInitialized) { - _model->setScaleToFit(true, dimensions); - _model->setSnapModelToRegistrationPoint(true, getRegistrationPoint()); - _model->setRotation(getRotation()); - _model->setTranslation(getPosition()); - - // make sure to simulate so everything gets set up correctly for rendering - { - PerformanceTimer perfTimer("_model->simulate"); - _model->simulate(0.0f); - } - - _needsInitialSimulation = false; + doInitialModelSimulation(); _needsJointSimulation = false; } } @@ -592,8 +584,7 @@ bool RenderableModelEntityItem::isReadyToComputeShape() { if (_needsInitialSimulation) { // the _model's offset will be wrong until _needsInitialSimulation is false PerformanceTimer perfTimer("_model->simulate"); - _model->simulate(0.0f); - _needsInitialSimulation = false; + doInitialModelSimulation(); } return true; diff --git a/libraries/entities-renderer/src/RenderableModelEntityItem.h b/libraries/entities-renderer/src/RenderableModelEntityItem.h index 59208d209d..5fd767c6ee 100644 --- a/libraries/entities-renderer/src/RenderableModelEntityItem.h +++ b/libraries/entities-renderer/src/RenderableModelEntityItem.h @@ -38,6 +38,8 @@ public: EntityPropertyFlags& propertyFlags, bool overwriteLocalData, bool& somethingChanged) override; + void doInitialModelSimulation(); + virtual bool readyToAddToScene(RenderArgs* renderArgs = nullptr); virtual bool addToScene(EntityItemPointer self, std::shared_ptr scene, render::PendingChanges& pendingChanges) override; virtual void removeFromScene(EntityItemPointer self, std::shared_ptr scene, render::PendingChanges& pendingChanges) override; From 574709824e924cba5da16a1ab474f48ec1ad4830 Mon Sep 17 00:00:00 2001 From: Seth Alves Date: Fri, 13 May 2016 09:41:19 -0700 Subject: [PATCH 5/7] oops --- libraries/entities-renderer/src/RenderableModelEntityItem.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libraries/entities-renderer/src/RenderableModelEntityItem.cpp b/libraries/entities-renderer/src/RenderableModelEntityItem.cpp index d6f8eb23c8..6273b9f8df 100644 --- a/libraries/entities-renderer/src/RenderableModelEntityItem.cpp +++ b/libraries/entities-renderer/src/RenderableModelEntityItem.cpp @@ -802,7 +802,7 @@ void RenderableModelEntityItem::locationChanged(bool tellPhysics) { auto myMetaItemCopy = _myMetaItem; void* key = (void*)this; - AbstractViewStateInterface::instance()->pushPostUpdateLambda(key, [_myMetaItem]() { + AbstractViewStateInterface::instance()->pushPostUpdateLambda(key, [myMetaItemCopy]() { render::ScenePointer scene = AbstractViewStateInterface::instance()->getMain3DScene(); render::PendingChanges pendingChanges; pendingChanges.updateItem(myMetaItemCopy, [](RenderableModelEntityItemMeta& data){}); From 970e7ca17d8ff6617d4ea2dca038141d301612b8 Mon Sep 17 00:00:00 2001 From: Seth Alves Date: Fri, 13 May 2016 13:32:00 -0700 Subject: [PATCH 6/7] don't crash when a moving model entity is deleted --- .../src/RenderableModelEntityItem.cpp | 21 +++++++++++++++---- .../src/RenderableModelEntityItem.h | 2 ++ libraries/render/src/render/Scene.cpp | 5 +++++ 3 files changed, 24 insertions(+), 4 deletions(-) diff --git a/libraries/entities-renderer/src/RenderableModelEntityItem.cpp b/libraries/entities-renderer/src/RenderableModelEntityItem.cpp index e13c2eac98..a7e14b4c07 100644 --- a/libraries/entities-renderer/src/RenderableModelEntityItem.cpp +++ b/libraries/entities-renderer/src/RenderableModelEntityItem.cpp @@ -808,13 +808,26 @@ void RenderableModelEntityItem::locationChanged(bool tellPhysics) { _model->setRotation(getRotation()); _model->setTranslation(getPosition()); - auto myMetaItemCopy = _myMetaItem; - void* key = (void*)this; - AbstractViewStateInterface::instance()->pushPostUpdateLambda(key, [_myMetaItem]() { + std::weak_ptr weakSelf = + std::static_pointer_cast(getThisPointer()); + + AbstractViewStateInterface::instance()->pushPostUpdateLambda(key, [weakSelf]() { + auto self = weakSelf.lock(); + if (!self) { + return; + } + + render::ItemID myMetaItem = self->getMetaRenderItem(); + + if (myMetaItem == render::Item::INVALID_ITEM_ID) { + return; + } + render::ScenePointer scene = AbstractViewStateInterface::instance()->getMain3DScene(); render::PendingChanges pendingChanges; - pendingChanges.updateItem(myMetaItemCopy, [](RenderableModelEntityItemMeta& data){}); + + pendingChanges.updateItem(myMetaItem); scene->enqueuePendingChanges(pendingChanges); }); } diff --git a/libraries/entities-renderer/src/RenderableModelEntityItem.h b/libraries/entities-renderer/src/RenderableModelEntityItem.h index 59208d209d..9b21743395 100644 --- a/libraries/entities-renderer/src/RenderableModelEntityItem.h +++ b/libraries/entities-renderer/src/RenderableModelEntityItem.h @@ -87,6 +87,8 @@ public: bool hasRenderAnimation() const { return !_renderAnimationProperties.getURL().isEmpty(); } const QString& getRenderAnimationURL() const { return _renderAnimationProperties.getURL(); } + render::ItemID getMetaRenderItem() { return _myMetaItem; } + private: QVariantMap parseTexturesToMap(QString textures); void remapTextures(); diff --git a/libraries/render/src/render/Scene.cpp b/libraries/render/src/render/Scene.cpp index e091b4842c..0b1d8fb55f 100644 --- a/libraries/render/src/render/Scene.cpp +++ b/libraries/render/src/render/Scene.cpp @@ -157,6 +157,11 @@ void Scene::updateItems(const ItemIDs& ids, UpdateFunctors& functors) { auto updateFunctor = functors.begin(); for (auto updateID : ids) { + if (updateID == Item::INVALID_ITEM_ID) { + updateFunctor++; + continue; + } + // Access the true item auto& item = _items[updateID]; auto oldCell = item.getCell(); From d77b3bf59a6edec6b557628c15e33cc83f3d8fa2 Mon Sep 17 00:00:00 2001 From: Seth Alves Date: Mon, 16 May 2016 14:12:05 -0700 Subject: [PATCH 7/7] fix some comments and debug pritns --- libraries/entities/src/UpdateEntityOperator.cpp | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/libraries/entities/src/UpdateEntityOperator.cpp b/libraries/entities/src/UpdateEntityOperator.cpp index a48f3f7198..84f801b059 100644 --- a/libraries/entities/src/UpdateEntityOperator.cpp +++ b/libraries/entities/src/UpdateEntityOperator.cpp @@ -45,23 +45,17 @@ UpdateEntityOperator::UpdateEntityOperator(EntityTreePointer tree, _newEntityCube = newQueryAACube; _newEntityBox = _newEntityCube.clamp((float)-HALF_TREE_SCALE, (float)HALF_TREE_SCALE); // clamp to domain bounds - // If our new properties don't have bounds details (no change to position, etc) or if this containing element would - // be the best fit for our new properties, then just do the new portion of the store pass, since the change path will - // be the same for both parts of the update + // set oldElementBestFit true if the entity was in the correct element before this operator was run. bool oldElementBestFit = _containingElement->bestFitBounds(_oldEntityBox); // For some reason we've seen a case where the original containing element isn't a best fit for the old properties // in this case we want to move it, even if the properties haven't changed. - if (oldElementBestFit) { - if (_wantDebug) { - qCDebug(entities) << " **** TYPICAL NO MOVE CASE **** oldElementBestFit:" << oldElementBestFit; - } - } else { + if (!oldElementBestFit) { _oldEntityBox = _existingEntity->getElement()->getAACube(); _removeOld = true; // our properties are going to move us, so remember this for later processing if (_wantDebug) { - qCDebug(entities) << " **** UNUSUAL CASE **** no changes, but not best fit... consider it a move.... **"; + qCDebug(entities) << " **** UNUSUAL CASE **** not best fit.... **"; } }