From 23aa6267551c7db26b1c18f2bacfcf1be9e36e41 Mon Sep 17 00:00:00 2001 From: Anthony Thibault Date: Tue, 25 Oct 2016 15:28:37 -0700 Subject: [PATCH] Destroy render scene & engine before Application is destroyed Many render items/payloads contain smart pointers back to the objects that added them to the scene, including entity and avatar objects. Currently, those render items are destroyed when the scene is destroyed very late in the application life-cycle. There are rare crashes that can occur when these render items are destroyed. Possibly, due to them referencing objects that have already been destroyed via raw pointers. In an effort to eliminate these crashes, we now destroy the scene earlier, within Application::aboutToQuit() which is connected to the QCoreApplication::aboutToQuit signal. Also, we guard against null scene pointer dereferences. Any location that accesses the scene off the main thread, now checks the validity of the scene pointer. --- interface/src/Application.cpp | 6 ++++ interface/src/avatar/AvatarManager.cpp | 13 +++++--- .../src/EntityTreeRenderer.cpp | 30 +++++++++++++------ .../src/RenderableEntityItem.h | 11 +++++-- .../src/RenderableZoneEntityItem.cpp | 11 ++++--- libraries/render-utils/src/Model.cpp | 8 +++-- libraries/render/src/render/Scene.cpp | 4 +++ libraries/render/src/render/Scene.h | 2 +- 8 files changed, 62 insertions(+), 23 deletions(-) diff --git a/interface/src/Application.cpp b/interface/src/Application.cpp index 861e3a2246..7e37dd7455 100644 --- a/interface/src/Application.cpp +++ b/interface/src/Application.cpp @@ -1576,6 +1576,12 @@ void Application::cleanupBeforeQuit() { #endif DependencyManager::destroy(); + + // shutdown render engine + _main3DScene = nullptr; + _renderEngine = nullptr; + + qCDebug(interfaceapp) << "Application::cleanupBeforeQuit() complete"; } Application::~Application() { diff --git a/interface/src/avatar/AvatarManager.cpp b/interface/src/avatar/AvatarManager.cpp index 18856ff914..e7f3deeaea 100644 --- a/interface/src/avatar/AvatarManager.cpp +++ b/interface/src/avatar/AvatarManager.cpp @@ -36,6 +36,7 @@ #include "Application.h" #include "Avatar.h" #include "AvatarManager.h" +#include "InterfaceLogging.h" #include "Menu.h" #include "MyAvatar.h" #include "SceneScriptingInterface.h" @@ -208,11 +209,15 @@ AvatarSharedPointer AvatarManager::addAvatar(const QUuid& sessionUUID, const QWe auto rawRenderableAvatar = std::static_pointer_cast(newAvatar); render::ScenePointer scene = qApp->getMain3DScene(); - render::PendingChanges pendingChanges; - if (DependencyManager::get()->shouldRenderAvatars()) { - rawRenderableAvatar->addToScene(rawRenderableAvatar, scene, pendingChanges); + if (scene) { + render::PendingChanges pendingChanges; + if (DependencyManager::get()->shouldRenderAvatars()) { + rawRenderableAvatar->addToScene(rawRenderableAvatar, scene, pendingChanges); + } + scene->enqueuePendingChanges(pendingChanges); + } else { + qCWarning(interfaceapp) << "AvatarManager::addAvatar() : Unexpected null scene, possibly during application shutdown"; } - scene->enqueuePendingChanges(pendingChanges); return newAvatar; } diff --git a/libraries/entities-renderer/src/EntityTreeRenderer.cpp b/libraries/entities-renderer/src/EntityTreeRenderer.cpp index e0fc03897a..c842a78dc5 100644 --- a/libraries/entities-renderer/src/EntityTreeRenderer.cpp +++ b/libraries/entities-renderer/src/EntityTreeRenderer.cpp @@ -128,11 +128,15 @@ void EntityTreeRenderer::clear() { // remove all entities from the scene auto scene = _viewState->getMain3DScene(); - render::PendingChanges pendingChanges; - foreach(auto entity, _entitiesInScene) { - entity->removeFromScene(entity, scene, pendingChanges); + if (scene) { + render::PendingChanges pendingChanges; + foreach(auto entity, _entitiesInScene) { + entity->removeFromScene(entity, scene, pendingChanges); + } + scene->enqueuePendingChanges(pendingChanges); + } else { + qCWarning(entitiesrenderer) << "EntitityTreeRenderer::clear(), Unexpected null scene, possibly during application shutdown"; } - scene->enqueuePendingChanges(pendingChanges); _entitiesInScene.clear(); // reset the zone to the default (while we load the next scene) @@ -901,8 +905,12 @@ void EntityTreeRenderer::deletingEntity(const EntityItemID& entityID) { auto entity = _entitiesInScene.take(entityID); render::PendingChanges pendingChanges; auto scene = _viewState->getMain3DScene(); - entity->removeFromScene(entity, scene, pendingChanges); - scene->enqueuePendingChanges(pendingChanges); + if (scene) { + entity->removeFromScene(entity, scene, pendingChanges); + scene->enqueuePendingChanges(pendingChanges); + } else { + qCWarning(entitiesrenderer) << "EntityTreeRenderer::deletingEntity(), Unexpected null scene, possibly during application shutdown"; + } } } @@ -919,10 +927,14 @@ void EntityTreeRenderer::addEntityToScene(EntityItemPointer entity) { // here's where we add the entity payload to the scene render::PendingChanges pendingChanges; auto scene = _viewState->getMain3DScene(); - if (entity->addToScene(entity, scene, pendingChanges)) { - _entitiesInScene.insert(entity->getEntityItemID(), entity); + if (scene) { + if (entity->addToScene(entity, scene, pendingChanges)) { + _entitiesInScene.insert(entity->getEntityItemID(), entity); + } + scene->enqueuePendingChanges(pendingChanges); + } else { + qCWarning(entitiesrenderer) << "EntityTreeRenderer::addEntityToScene(), Unexpected null scene, possibly during application shutdown"; } - scene->enqueuePendingChanges(pendingChanges); } diff --git a/libraries/entities-renderer/src/RenderableEntityItem.h b/libraries/entities-renderer/src/RenderableEntityItem.h index 22b6264520..ec65bab1c8 100644 --- a/libraries/entities-renderer/src/RenderableEntityItem.h +++ b/libraries/entities-renderer/src/RenderableEntityItem.h @@ -15,6 +15,7 @@ #include #include #include "AbstractViewStateInterface.h" +#include "EntitiesRendererLogging.h" // These or the icon "name" used by the render item status value, they correspond to the atlas texture used by the DrawItemStatus @@ -79,10 +80,14 @@ public: render::PendingChanges pendingChanges; render::ScenePointer scene = AbstractViewStateInterface::instance()->getMain3DScene(); - pendingChanges.updateItem(_myItem, [](RenderableEntityItemProxy& data) { - }); + if (scene) { + pendingChanges.updateItem(_myItem, [](RenderableEntityItemProxy& data) { + }); - scene->enqueuePendingChanges(pendingChanges); + scene->enqueuePendingChanges(pendingChanges); + } else { + qCWarning(entitiesrenderer) << "SimpleRenderableEntityItem::notifyChanged(), Unexpected null scene, possibly during application shutdown"; + } } private: diff --git a/libraries/entities-renderer/src/RenderableZoneEntityItem.cpp b/libraries/entities-renderer/src/RenderableZoneEntityItem.cpp index 9aa52f5ad3..0215ce4d07 100644 --- a/libraries/entities-renderer/src/RenderableZoneEntityItem.cpp +++ b/libraries/entities-renderer/src/RenderableZoneEntityItem.cpp @@ -248,9 +248,12 @@ void RenderableZoneEntityItem::notifyBoundChanged() { } render::PendingChanges pendingChanges; render::ScenePointer scene = AbstractViewStateInterface::instance()->getMain3DScene(); + if (scene) { + pendingChanges.updateItem(_myMetaItem, [](RenderableZoneEntityItemMeta& data) { + }); - pendingChanges.updateItem(_myMetaItem, [](RenderableZoneEntityItemMeta& data) { - }); - - scene->enqueuePendingChanges(pendingChanges); + scene->enqueuePendingChanges(pendingChanges); + } else { + qCWarning(entitiesrenderer) << "RenderableZoneEntityItem::notifyBoundChanged(), Unexpected null scene, possibly during application shutdown"; + } } diff --git a/libraries/render-utils/src/Model.cpp b/libraries/render-utils/src/Model.cpp index 28a1c3d579..6e1f57778b 100644 --- a/libraries/render-utils/src/Model.cpp +++ b/libraries/render-utils/src/Model.cpp @@ -862,8 +862,12 @@ void Model::setURL(const QUrl& url) { { render::PendingChanges pendingChanges; render::ScenePointer scene = AbstractViewStateInterface::instance()->getMain3DScene(); - removeFromScene(scene, pendingChanges); - scene->enqueuePendingChanges(pendingChanges); + if (scene) { + removeFromScene(scene, pendingChanges); + scene->enqueuePendingChanges(pendingChanges); + } else { + qCWarning(renderutils) << "Model::setURL(), Unexpected null scene, possibly during application shutdown"; + } } _needsReload = true; diff --git a/libraries/render/src/render/Scene.cpp b/libraries/render/src/render/Scene.cpp index 0b1d8fb55f..6bc42ac656 100644 --- a/libraries/render/src/render/Scene.cpp +++ b/libraries/render/src/render/Scene.cpp @@ -48,6 +48,10 @@ Scene::Scene(glm::vec3 origin, float size) : _items.push_back(Item()); // add the itemID #0 to nothing } +Scene::~Scene() { + qDebug() << "Scene::~Scene()"; +} + ItemID Scene::allocateID() { // Just increment and return the proevious value initialized at 0 return _IDAllocator.fetch_add(1); diff --git a/libraries/render/src/render/Scene.h b/libraries/render/src/render/Scene.h index b7cfc367b8..6b57a22a36 100644 --- a/libraries/render/src/render/Scene.h +++ b/libraries/render/src/render/Scene.h @@ -55,7 +55,7 @@ typedef std::queue PendingChangesQueue; class Scene { public: Scene(glm::vec3 origin, float size); - ~Scene() {} + ~Scene(); // This call is thread safe, can be called from anywhere to allocate a new ID ItemID allocateID();