From b8fcd6d3ba0a3a91879ae7a98b22f75c852433c1 Mon Sep 17 00:00:00 2001 From: elsid Date: Sun, 11 Jul 2021 14:43:52 +0200 Subject: [PATCH 1/4] Manage work item lifetime on the client side Instead of explicit work queue stop before any possibly used engine manager is destructed. Based on an assumption that any engine manager can be destructed independently from the work queue destruction. This model is already used in CellPreloader that conflicts with explicit work queue stop. After the work queue is requested to be stopped, any client waiting for a not started work item to be done will wait forever because the work item is dropped from the queue. Therefore either clients should not wait for own work items to be completed in destructor or the work queue should not drop items before clients are destructed. Other approaches are possible but are not considered due to increasing complexity. CellPreloader already tries to wait for all created work items to be done so keep it that way and extend the model to AsyncScreenCaptureOperation and Scene. Additionally abort all scheduled work items when owner is destructed. This prevents a long exit when multiple screenshots are scheduled right before exiting the game. --- apps/openmw/engine.cpp | 3 ++- apps/openmw/engine.hpp | 3 ++- apps/openmw/mwworld/scene.cpp | 24 +++++++++++++++++++- apps/openmw/mwworld/scene.hpp | 9 ++++++++ components/sceneutil/screencapture.cpp | 31 +++++++++++++++++++++++++- components/sceneutil/screencapture.hpp | 9 ++++++++ 6 files changed, 75 insertions(+), 4 deletions(-) diff --git a/apps/openmw/engine.cpp b/apps/openmw/engine.cpp index f554821b3d..b0047fd1df 100644 --- a/apps/openmw/engine.cpp +++ b/apps/openmw/engine.cpp @@ -427,7 +427,8 @@ OMW::Engine::Engine(Files::ConfigurationManager& configurationManager) OMW::Engine::~Engine() { - mWorkQueue->stop(); + if (mScreenCaptureOperation != nullptr) + mScreenCaptureOperation->stop(); mEnvironment.cleanup(); diff --git a/apps/openmw/engine.hpp b/apps/openmw/engine.hpp index 180e06bcbc..290fd890a6 100644 --- a/apps/openmw/engine.hpp +++ b/apps/openmw/engine.hpp @@ -21,6 +21,7 @@ namespace Resource namespace SceneUtil { class WorkQueue; + class AsyncScreenCaptureOperation; } namespace VFS @@ -67,7 +68,7 @@ namespace OMW boost::filesystem::path mResDir; osg::ref_ptr mViewer; osg::ref_ptr mScreenCaptureHandler; - osg::ref_ptr mScreenCaptureOperation; + osg::ref_ptr mScreenCaptureOperation; std::string mCellName; std::vector mContentFiles; std::vector mGroundcoverFiles; diff --git a/apps/openmw/mwworld/scene.cpp b/apps/openmw/mwworld/scene.cpp index 7d3a6c7893..c43ee76d1b 100644 --- a/apps/openmw/mwworld/scene.cpp +++ b/apps/openmw/mwworld/scene.cpp @@ -3,6 +3,7 @@ #include #include #include +#include #include #include @@ -867,6 +868,11 @@ namespace MWWorld Scene::~Scene() { + for (const osg::ref_ptr& v : mWorkItems) + v->abort(); + + for (const osg::ref_ptr& v : mWorkItems) + v->waitTillDone(); } bool Scene::hasCellChanged() const @@ -1061,6 +1067,9 @@ namespace MWWorld void doWork() override { + if (mAborted) + return; + try { mSceneManager->getTemplate(mMesh); @@ -1069,9 +1078,16 @@ namespace MWWorld { } } + + void abort() override + { + mAborted = true; + } + private: std::string mMesh; Resource::SceneManager* mSceneManager; + std::atomic_bool mAborted {false}; }; void Scene::preload(const std::string &mesh, bool useAnim) @@ -1081,7 +1097,13 @@ namespace MWWorld mesh_ = Misc::ResourceHelpers::correctActorModelPath(mesh_, mRendering.getResourceSystem()->getVFS()); if (!mRendering.getResourceSystem()->getSceneManager()->checkLoaded(mesh_, mRendering.getReferenceTime())) - mRendering.getWorkQueue()->addWorkItem(new PreloadMeshItem(mesh_, mRendering.getResourceSystem()->getSceneManager())); + { + osg::ref_ptr item(new PreloadMeshItem(mesh_, mRendering.getResourceSystem()->getSceneManager())); + mRendering.getWorkQueue()->addWorkItem(item); + const auto isDone = [] (const osg::ref_ptr& v) { return v->isDone(); }; + mWorkItems.erase(std::remove_if(mWorkItems.begin(), mWorkItems.end(), isDone), mWorkItems.end()); + mWorkItems.emplace_back(std::move(item)); + } } void Scene::preloadCells(float dt) diff --git a/apps/openmw/mwworld/scene.hpp b/apps/openmw/mwworld/scene.hpp index bc9c2386bb..75c070dd1e 100644 --- a/apps/openmw/mwworld/scene.hpp +++ b/apps/openmw/mwworld/scene.hpp @@ -3,6 +3,7 @@ #include #include +#include #include "ptr.hpp" #include "globals.hpp" @@ -10,6 +11,7 @@ #include #include #include +#include #include @@ -49,6 +51,11 @@ namespace MWPhysics class PhysicsSystem; } +namespace SceneUtil +{ + class WorkItem; +} + namespace MWWorld { class Player; @@ -91,6 +98,8 @@ namespace MWWorld std::set mPagedRefs; + std::vector> mWorkItems; + void insertCell (CellStore &cell, Loading::Listener* loadingListener, bool onlyObjects, bool test = false); osg::Vec2i mCurrentGridCenter; diff --git a/components/sceneutil/screencapture.cpp b/components/sceneutil/screencapture.cpp index 47119f3644..99cb535326 100644 --- a/components/sceneutil/screencapture.cpp +++ b/components/sceneutil/screencapture.cpp @@ -15,6 +15,7 @@ #include #include #include +#include namespace { @@ -32,6 +33,9 @@ namespace void doWork() override { + if (mAborted) + return; + try { (*mImpl)(*mImage, mContextId); @@ -42,10 +46,16 @@ namespace } } + void abort() override + { + mAborted = true; + } + private: const osg::ref_ptr mImpl; const osg::ref_ptr mImage; const unsigned int mContextId; + std::atomic_bool mAborted {false}; }; } @@ -130,8 +140,27 @@ namespace SceneUtil assert(mImpl != nullptr); } + AsyncScreenCaptureOperation::~AsyncScreenCaptureOperation() + { + stop(); + } + + void AsyncScreenCaptureOperation::stop() + { + for (const osg::ref_ptr& item : *mWorkItems.lockConst()) + item->abort(); + + for (const osg::ref_ptr& item : *mWorkItems.lockConst()) + item->waitTillDone(); + } + void AsyncScreenCaptureOperation::operator()(const osg::Image& image, const unsigned int context_id) { - mQueue->addWorkItem(new ScreenCaptureWorkItem(mImpl, image, context_id)); + osg::ref_ptr item(new ScreenCaptureWorkItem(mImpl, image, context_id)); + mQueue->addWorkItem(item); + const auto isDone = [] (const osg::ref_ptr& v) { return v->isDone(); }; + const auto workItems = mWorkItems.lock(); + workItems->erase(std::remove_if(workItems->begin(), workItems->end(), isDone), workItems->end()); + workItems->emplace_back(std::move(item)); } } diff --git a/components/sceneutil/screencapture.hpp b/components/sceneutil/screencapture.hpp index 6395d989d8..87e396b020 100644 --- a/components/sceneutil/screencapture.hpp +++ b/components/sceneutil/screencapture.hpp @@ -1,10 +1,13 @@ #ifndef OPENMW_COMPONENTS_SCENEUTIL_SCREENCAPTURE_H #define OPENMW_COMPONENTS_SCENEUTIL_SCREENCAPTURE_H +#include + #include #include #include +#include namespace osg { @@ -14,6 +17,7 @@ namespace osg namespace SceneUtil { class WorkQueue; + class WorkItem; std::string writeScreenshotToFile(const std::string& screenshotPath, const std::string& screenshotFormat, const osg::Image& image); @@ -38,11 +42,16 @@ namespace SceneUtil AsyncScreenCaptureOperation(osg::ref_ptr queue, osg::ref_ptr impl); + ~AsyncScreenCaptureOperation(); + + void stop(); + void operator()(const osg::Image& image, const unsigned int context_id) override; private: const osg::ref_ptr mQueue; const osg::ref_ptr mImpl; + Misc::ScopeGuarded>> mWorkItems; }; } From eece47f70e1f65891d101d42f1f445e71ad22a50 Mon Sep 17 00:00:00 2001 From: elsid Date: Sat, 10 Jul 2021 12:11:48 +0200 Subject: [PATCH 2/4] Avoid copying osg::ref_ptr when adding or removing item from work queue Copy constructor does refcounting, and move constructor doesn't. --- apps/openmw/mwrender/renderingmanager.cpp | 2 +- components/sceneutil/workqueue.cpp | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/apps/openmw/mwrender/renderingmanager.cpp b/apps/openmw/mwrender/renderingmanager.cpp index e497fdecd2..016149ed90 100644 --- a/apps/openmw/mwrender/renderingmanager.cpp +++ b/apps/openmw/mwrender/renderingmanager.cpp @@ -492,7 +492,7 @@ namespace MWRender workItem->mTextures.emplace_back("textures/_land_default.dds"); - mWorkQueue->addWorkItem(workItem); + mWorkQueue->addWorkItem(std::move(workItem)); } double RenderingManager::getReferenceTime() const diff --git a/components/sceneutil/workqueue.cpp b/components/sceneutil/workqueue.cpp index 3c1df80ac4..81c9bee29c 100644 --- a/components/sceneutil/workqueue.cpp +++ b/components/sceneutil/workqueue.cpp @@ -74,9 +74,9 @@ void WorkQueue::addWorkItem(osg::ref_ptr item, bool front) std::unique_lock lock(mMutex); if (front) - mQueue.push_front(item); + mQueue.push_front(std::move(item)); else - mQueue.push_back(item); + mQueue.push_back(std::move(item)); mCondition.notify_one(); } @@ -89,7 +89,7 @@ osg::ref_ptr WorkQueue::removeWorkItem() } if (!mQueue.empty()) { - osg::ref_ptr item = mQueue.front(); + osg::ref_ptr item = std::move(mQueue.front()); mQueue.pop_front(); return item; } From d4a2dab9d93b5b9903112abca6888c68dda06079 Mon Sep 17 00:00:00 2001 From: elsid Date: Sat, 10 Jul 2021 12:19:59 +0200 Subject: [PATCH 3/4] Remove redundant else --- components/sceneutil/workqueue.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/components/sceneutil/workqueue.cpp b/components/sceneutil/workqueue.cpp index 81c9bee29c..0c68c61921 100644 --- a/components/sceneutil/workqueue.cpp +++ b/components/sceneutil/workqueue.cpp @@ -93,8 +93,7 @@ osg::ref_ptr WorkQueue::removeWorkItem() mQueue.pop_front(); return item; } - else - return nullptr; + return nullptr; } unsigned int WorkQueue::getNumItems() const From 153cd9a20cf1891f9a953870d1b04d591740a5f3 Mon Sep 17 00:00:00 2001 From: elsid Date: Sat, 10 Jul 2021 13:02:28 +0200 Subject: [PATCH 4/4] Avoid redundant search for existing element --- apps/openmw/mwworld/cellpreloader.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/openmw/mwworld/cellpreloader.cpp b/apps/openmw/mwworld/cellpreloader.cpp index 44afde22ae..a2167c562f 100644 --- a/apps/openmw/mwworld/cellpreloader.cpp +++ b/apps/openmw/mwworld/cellpreloader.cpp @@ -317,7 +317,7 @@ namespace MWWorld if (found->second.mWorkItem) { found->second.mWorkItem->abort(); - mUnrefQueue->push(mPreloadCells[cell].mWorkItem); + mUnrefQueue->push(std::move(found->second.mWorkItem)); } mPreloadCells.erase(found);