From 3202900fd3c50fe55745d085ba65e170c6bb8f21 Mon Sep 17 00:00:00 2001 From: elsid Date: Sun, 27 Aug 2023 17:11:42 +0200 Subject: [PATCH 1/8] Make GenericResourceManager::setExpiryDelay final --- components/resource/resourcemanager.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/components/resource/resourcemanager.hpp b/components/resource/resourcemanager.hpp index fc5ecc7f03e..79632799e28 100644 --- a/components/resource/resourcemanager.hpp +++ b/components/resource/resourcemanager.hpp @@ -59,7 +59,7 @@ namespace Resource void clearCache() override { mCache->clear(); } /// How long to keep objects in cache after no longer being referenced. - void setExpiryDelay(double expiryDelay) override { mExpiryDelay = expiryDelay; } + void setExpiryDelay(double expiryDelay) final { mExpiryDelay = expiryDelay; } double getExpiryDelay() const { return mExpiryDelay; } const VFS::Manager* getVFS() const { return mVFS; } From b6a3d3c906c37631c814f0789a0b6f0eae637678 Mon Sep 17 00:00:00 2001 From: elsid Date: Sun, 27 Aug 2023 17:12:20 +0200 Subject: [PATCH 2/8] Make BaseResourceManager abstract --- components/resource/resourcemanager.hpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/components/resource/resourcemanager.hpp b/components/resource/resourcemanager.hpp index 79632799e28..1b91d0b412e 100644 --- a/components/resource/resourcemanager.hpp +++ b/components/resource/resourcemanager.hpp @@ -23,11 +23,11 @@ namespace Resource { public: virtual ~BaseResourceManager() = default; - virtual void updateCache(double referenceTime) {} - virtual void clearCache() {} - virtual void setExpiryDelay(double expiryDelay) {} - virtual void reportStats(unsigned int frameNumber, osg::Stats* stats) const {} - virtual void releaseGLObjects(osg::State* state) {} + virtual void updateCache(double referenceTime) = 0; + virtual void clearCache() = 0; + virtual void setExpiryDelay(double expiryDelay) = 0; + virtual void reportStats(unsigned int frameNumber, osg::Stats* stats) const = 0; + virtual void releaseGLObjects(osg::State* state) = 0; }; /// @brief Base class for managers that require a virtual file system and object cache. From 6d120f92e00b6846d0a6ddb5779017d33cd7f5d1 Mon Sep 17 00:00:00 2001 From: elsid Date: Mon, 28 Aug 2023 22:53:46 +0200 Subject: [PATCH 3/8] Lookup for terrain template using std::map::lower_bound instead of linear search --- components/resource/objectcache.hpp | 12 ++++++- components/terrain/chunkmanager.cpp | 46 ++++++++++--------------- components/terrain/chunkmanager.hpp | 47 ++++++++++++++++++++++++-- components/terrain/terraindrawable.hpp | 2 +- 4 files changed, 73 insertions(+), 34 deletions(-) diff --git a/components/resource/objectcache.hpp b/components/resource/objectcache.hpp index 3847129ed33..2895aaa3d23 100644 --- a/components/resource/objectcache.hpp +++ b/components/resource/objectcache.hpp @@ -195,11 +195,21 @@ namespace Resource return _objectCache.size(); } + template + std::optional>> lowerBound(K&& key) + { + const std::lock_guard lock(_objectCacheMutex); + const auto it = _objectCache.lower_bound(std::forward(key)); + if (it == _objectCache.end()) + return std::nullopt; + return std::pair(it->first, it->second.first); + } + protected: virtual ~GenericObjectCache() {} typedef std::pair, double> ObjectTimeStampPair; - typedef std::map ObjectCacheMap; + typedef std::map> ObjectCacheMap; ObjectCacheMap _objectCache; mutable std::mutex _objectCacheMutex; diff --git a/components/terrain/chunkmanager.cpp b/components/terrain/chunkmanager.cpp index 0364a0a4fdc..6273041f727 100644 --- a/components/terrain/chunkmanager.cpp +++ b/components/terrain/chunkmanager.cpp @@ -21,7 +21,7 @@ namespace Terrain ChunkManager::ChunkManager(Storage* storage, Resource::SceneManager* sceneMgr, TextureManager* textureManager, CompositeMapRenderer* renderer, ESM::RefId worldspace) - : GenericResourceManager(nullptr) + : GenericResourceManager(nullptr) , QuadTreeWorld::ChunkManager(worldspace) , mStorage(storage) , mSceneManager(sceneMgr) @@ -39,38 +39,26 @@ namespace Terrain mMultiPassRoot->setAttributeAndModes(material, osg::StateAttribute::ON); } - struct FindChunkTemplate - { - void operator()(ChunkId id, osg::Object* obj) - { - if (std::get<0>(id) == std::get<0>(mId) && std::get<1>(id) == std::get<1>(mId)) - mFoundTemplate = obj; - } - ChunkId mId; - osg::ref_ptr mFoundTemplate; - }; - osg::ref_ptr ChunkManager::getChunk(float size, const osg::Vec2f& center, unsigned char lod, unsigned int lodFlags, bool activeGrid, const osg::Vec3f& viewPoint, bool compile) { // Override lod with the vertexLodMod adjusted value. // TODO: maybe we can refactor this code by moving all vertexLodMod code into this class. lod = static_cast(lodFlags >> (4 * 4)); - ChunkId id = std::make_tuple(center, lod, lodFlags); - osg::ref_ptr obj = mCache->getRefFromObjectCache(id); - if (obj) + + const ChunkKey key{ .mCenter = center, .mLod = lod, .mLodFlags = lodFlags }; + if (osg::ref_ptr obj = mCache->getRefFromObjectCache(key)) return static_cast(obj.get()); - else - { - FindChunkTemplate find; - find.mId = id; - mCache->call(find); - TerrainDrawable* templateGeometry - = find.mFoundTemplate ? static_cast(find.mFoundTemplate.get()) : nullptr; - osg::ref_ptr node = createChunk(size, center, lod, lodFlags, compile, templateGeometry); - mCache->addEntryToObjectCache(id, node.get()); - return node; - } + + const TerrainDrawable* templateGeometry = nullptr; + const TemplateKey templateKey{ .mCenter = center, .mLod = lod }; + const auto pair = mCache->lowerBound(templateKey); + if (pair.has_value() && templateKey == TemplateKey{ .mCenter = pair->first.mCenter, .mLod = pair->first.mLod }) + templateGeometry = static_cast(pair->second.get()); + + osg::ref_ptr node = createChunk(size, center, lod, lodFlags, compile, templateGeometry); + mCache->addEntryToObjectCache(key, node.get()); + return node; } void ChunkManager::reportStats(unsigned int frameNumber, osg::Stats* stats) const @@ -80,14 +68,14 @@ namespace Terrain void ChunkManager::clearCache() { - GenericResourceManager::clearCache(); + GenericResourceManager::clearCache(); mBufferCache.clearCache(); } void ChunkManager::releaseGLObjects(osg::State* state) { - GenericResourceManager::releaseGLObjects(state); + GenericResourceManager::releaseGLObjects(state); mBufferCache.releaseGLObjects(state); } @@ -202,7 +190,7 @@ namespace Terrain } osg::ref_ptr ChunkManager::createChunk(float chunkSize, const osg::Vec2f& chunkCenter, unsigned char lod, - unsigned int lodFlags, bool compile, TerrainDrawable* templateGeometry) + unsigned int lodFlags, bool compile, const TerrainDrawable* templateGeometry) { osg::ref_ptr geometry(new TerrainDrawable); diff --git a/components/terrain/chunkmanager.hpp b/components/terrain/chunkmanager.hpp index 4238f01c8b2..a55dd15cc1f 100644 --- a/components/terrain/chunkmanager.hpp +++ b/components/terrain/chunkmanager.hpp @@ -28,10 +28,51 @@ namespace Terrain class CompositeMap; class TerrainDrawable; - typedef std::tuple ChunkId; // Center, Lod, Lod Flags + struct TemplateKey + { + osg::Vec2f mCenter; + unsigned char mLod; + }; + + inline auto tie(const TemplateKey& v) + { + return std::tie(v.mCenter, v.mLod); + } + + inline bool operator<(const TemplateKey& l, const TemplateKey& r) + { + return tie(l) < tie(r); + } + + inline bool operator==(const TemplateKey& l, const TemplateKey& r) + { + return tie(l) == tie(r); + } + + struct ChunkKey + { + osg::Vec2f mCenter; + unsigned char mLod; + unsigned mLodFlags; + }; + + inline auto tie(const ChunkKey& v) + { + return std::tie(v.mCenter, v.mLod, v.mLodFlags); + } + + inline bool operator<(const ChunkKey& l, const ChunkKey& r) + { + return tie(l) < tie(r); + } + + inline bool operator<(const ChunkKey& l, const TemplateKey& r) + { + return TemplateKey{ .mCenter = l.mCenter, .mLod = l.mLod } < r; + } /// @brief Handles loading and caching of terrain chunks - class ChunkManager : public Resource::GenericResourceManager, public QuadTreeWorld::ChunkManager + class ChunkManager : public Resource::GenericResourceManager, public QuadTreeWorld::ChunkManager { public: ChunkManager(Storage* storage, Resource::SceneManager* sceneMgr, TextureManager* textureManager, @@ -55,7 +96,7 @@ namespace Terrain private: osg::ref_ptr createChunk(float size, const osg::Vec2f& center, unsigned char lod, - unsigned int lodFlags, bool compile, TerrainDrawable* templateGeometry); + unsigned int lodFlags, bool compile, const TerrainDrawable* templateGeometry); osg::ref_ptr createCompositeMapRTT(); diff --git a/components/terrain/terraindrawable.hpp b/components/terrain/terraindrawable.hpp index b94f47df979..3ce846ad734 100644 --- a/components/terrain/terraindrawable.hpp +++ b/components/terrain/terraindrawable.hpp @@ -60,7 +60,7 @@ namespace Terrain const osg::BoundingBox& getWaterBoundingBox() const { return mWaterBoundingBox; } void setCompositeMap(CompositeMap* map) { mCompositeMap = map; } - CompositeMap* getCompositeMap() { return mCompositeMap; } + CompositeMap* getCompositeMap() const { return mCompositeMap; } void setCompositeMapRenderer(CompositeMapRenderer* renderer) { mCompositeMapRenderer = renderer; } private: From 52ab47771ce183495f81437a4fc4611ceb44a3d5 Mon Sep 17 00:00:00 2001 From: elsid Date: Sun, 27 Aug 2023 17:12:42 +0200 Subject: [PATCH 4/8] Initialize expiry delay for all GenericResourceManager instances --- apps/openmw/mwworld/scene.cpp | 3 --- components/resource/niffilemanager.cpp | 4 +++- components/resource/resourcemanager.hpp | 14 +++++++++++--- 3 files changed, 14 insertions(+), 7 deletions(-) diff --git a/apps/openmw/mwworld/scene.cpp b/apps/openmw/mwworld/scene.cpp index 78c8ccab1db..b5aff875bd2 100644 --- a/apps/openmw/mwworld/scene.cpp +++ b/apps/openmw/mwworld/scene.cpp @@ -834,9 +834,6 @@ namespace MWWorld mPreloader = std::make_unique(rendering.getResourceSystem(), physics->getShapeManager(), rendering.getTerrain(), rendering.getLandManager()); mPreloader->setWorkQueue(mRendering.getWorkQueue()); - - rendering.getResourceSystem()->setExpiryDelay(Settings::cells().mCacheExpiryDelay); - mPreloader->setExpiryDelay(Settings::cells().mPreloadCellExpiryDelay); mPreloader->setMinCacheSize(Settings::cells().mPreloadCellCacheMin); mPreloader->setMaxCacheSize(Settings::cells().mPreloadCellCacheMax); diff --git a/components/resource/niffilemanager.cpp b/components/resource/niffilemanager.cpp index ea95edf2aeb..5e457cdfaa9 100644 --- a/components/resource/niffilemanager.cpp +++ b/components/resource/niffilemanager.cpp @@ -32,7 +32,9 @@ namespace Resource }; NifFileManager::NifFileManager(const VFS::Manager* vfs) - : ResourceManager(vfs) + // NIF files aren't needed any more once the converted objects are cached in SceneManager / BulletShapeManager, + // so no point in using an expiry delay. + : ResourceManager(vfs, 0) { } diff --git a/components/resource/resourcemanager.hpp b/components/resource/resourcemanager.hpp index 1b91d0b412e..09b99faf285 100644 --- a/components/resource/resourcemanager.hpp +++ b/components/resource/resourcemanager.hpp @@ -3,6 +3,8 @@ #include +#include + #include "objectcache.hpp" namespace VFS @@ -39,10 +41,11 @@ namespace Resource public: typedef GenericObjectCache CacheType; - GenericResourceManager(const VFS::Manager* vfs) + explicit GenericResourceManager( + const VFS::Manager* vfs, double expiryDelay = Settings::cells().mCacheExpiryDelay) : mVFS(vfs) , mCache(new CacheType) - , mExpiryDelay(0.0) + , mExpiryDelay(expiryDelay) { } @@ -77,10 +80,15 @@ namespace Resource class ResourceManager : public GenericResourceManager { public: - ResourceManager(const VFS::Manager* vfs) + explicit ResourceManager(const VFS::Manager* vfs) : GenericResourceManager(vfs) { } + + explicit ResourceManager(const VFS::Manager* vfs, double expiryDelay) + : GenericResourceManager(vfs, expiryDelay) + { + } }; } From d3dca99a76a251ea4a1436d014d04a467ece02e7 Mon Sep 17 00:00:00 2001 From: elsid Date: Sun, 27 Aug 2023 16:03:47 +0200 Subject: [PATCH 5/8] Preload terrain in single pass Otherwise there is lodFlags mismatch because some of the neighbours are removed during preloading. This makes rendering culling create land chunk nodes again for the same position, lod because lodFlags are different. --- components/terrain/quadtreeworld.cpp | 37 ++++++---------------------- 1 file changed, 8 insertions(+), 29 deletions(-) diff --git a/components/terrain/quadtreeworld.cpp b/components/terrain/quadtreeworld.cpp index 8449f3df255..52edd96b9fa 100644 --- a/components/terrain/quadtreeworld.cpp +++ b/components/terrain/quadtreeworld.cpp @@ -544,37 +544,16 @@ namespace Terrain vd->setViewPoint(viewPoint); vd->setActiveGrid(grid); - for (unsigned int pass = 0; pass < 3; ++pass) - { - unsigned int startEntry = vd->getNumEntries(); - - float distanceModifier = 0.f; - if (pass == 1) - distanceModifier = 1024; - else if (pass == 2) - distanceModifier = -1024; - DefaultLodCallback lodCallback(mLodFactor, mMinSize, mViewDistance, grid, cellWorldSize, distanceModifier); - mRootNode->traverseNodes(vd, viewPoint, &lodCallback); - - if (pass == 0) - { - std::size_t progressTotal = 0; - for (unsigned int i = 0, n = vd->getNumEntries(); i < n; ++i) - progressTotal += vd->getEntry(i).mNode->getSize(); - - reporter.addTotal(progressTotal); - } + DefaultLodCallback lodCallback(mLodFactor, mMinSize, mViewDistance, grid, cellWorldSize); + mRootNode->traverseNodes(vd, viewPoint, &lodCallback); - for (unsigned int i = startEntry; i < vd->getNumEntries() && !abort; ++i) - { - ViewDataEntry& entry = vd->getEntry(i); + reporter.addTotal(vd->getNumEntries()); - loadRenderingNode(entry, vd, cellWorldSize, grid, true); - if (pass == 0) - reporter.addProgress(entry.mNode->getSize()); - vd->removeNodeFromIndex(entry.mNode); - entry.mNode = nullptr; // Clear node lest we break the neighbours search for the next pass - } + for (unsigned int i = 0, n = vd->getNumEntries(); i < n && !abort; ++i) + { + ViewDataEntry& entry = vd->getEntry(i); + loadRenderingNode(entry, vd, cellWorldSize, grid, true); + reporter.addProgress(1); } } From 5f4bd498cfcf25ac280c65823d36c1bc05451343 Mon Sep 17 00:00:00 2001 From: elsid Date: Sun, 27 Aug 2023 16:02:46 +0200 Subject: [PATCH 6/8] Move cached value into container to be removed --- components/resource/objectcache.hpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/components/resource/objectcache.hpp b/components/resource/objectcache.hpp index 2895aaa3d23..b5fb443ae3b 100644 --- a/components/resource/objectcache.hpp +++ b/components/resource/objectcache.hpp @@ -83,8 +83,8 @@ namespace Resource { if (oitr->second.second <= expiryTime) { - if (oitr->second.first != nullptr) - objectsToRemove.push_back(oitr->second.first); + if (oitr->second.mValue != nullptr) + objectsToRemove.push_back(std::move(oitr->second.first)); _objectCache.erase(oitr++); } else From 915a8df9424b45a2a46dbdaa515266d71a7a4953 Mon Sep 17 00:00:00 2001 From: elsid Date: Sun, 27 Aug 2023 16:03:14 +0200 Subject: [PATCH 7/8] Use struct for GenericObjectCache items --- components/resource/objectcache.hpp | 36 ++++++++++++++++------------- 1 file changed, 20 insertions(+), 16 deletions(-) diff --git a/components/resource/objectcache.hpp b/components/resource/objectcache.hpp index b5fb443ae3b..881729ffc45 100644 --- a/components/resource/objectcache.hpp +++ b/components/resource/objectcache.hpp @@ -62,9 +62,9 @@ namespace Resource { // If ref count is greater than 1, the object has an external reference. // If the timestamp is yet to be initialized, it needs to be updated too. - if ((itr->second.first != nullptr && itr->second.first->referenceCount() > 1) - || itr->second.second == 0.0) - itr->second.second = referenceTime; + if ((itr->second.mValue != nullptr && itr->second.mValue->referenceCount() > 1) + || itr->second.mLastUsage == 0.0) + itr->second.mLastUsage = referenceTime; } } @@ -81,10 +81,10 @@ namespace Resource typename ObjectCacheMap::iterator oitr = _objectCache.begin(); while (oitr != _objectCache.end()) { - if (oitr->second.second <= expiryTime) + if (oitr->second.mLastUsage <= expiryTime) { if (oitr->second.mValue != nullptr) - objectsToRemove.push_back(std::move(oitr->second.first)); + objectsToRemove.push_back(std::move(oitr->second.mValue)); _objectCache.erase(oitr++); } else @@ -106,7 +106,7 @@ namespace Resource void addEntryToObjectCache(const KeyType& key, osg::Object* object, double timestamp = 0.0) { std::lock_guard lock(_objectCacheMutex); - _objectCache[key] = ObjectTimeStampPair(object, timestamp); + _objectCache[key] = Item{ object, timestamp }; } /** Remove Object from cache.*/ @@ -124,7 +124,7 @@ namespace Resource std::lock_guard lock(_objectCacheMutex); typename ObjectCacheMap::iterator itr = _objectCache.find(key); if (itr != _objectCache.end()) - return itr->second.first; + return itr->second.mValue; else return nullptr; } @@ -135,7 +135,7 @@ namespace Resource const auto it = _objectCache.find(key); if (it == _objectCache.end()) return std::nullopt; - return it->second.first; + return it->second.mValue; } /** Check if an object is in the cache, and if it is, update its usage time stamp. */ @@ -145,7 +145,7 @@ namespace Resource typename ObjectCacheMap::iterator itr = _objectCache.find(key); if (itr != _objectCache.end()) { - itr->second.second = timeStamp; + itr->second.mLastUsage = timeStamp; return true; } else @@ -158,7 +158,7 @@ namespace Resource std::lock_guard lock(_objectCacheMutex); for (typename ObjectCacheMap::iterator itr = _objectCache.begin(); itr != _objectCache.end(); ++itr) { - osg::Object* object = itr->second.first.get(); + osg::Object* object = itr->second.mValue.get(); object->releaseGLObjects(state); } } @@ -169,8 +169,7 @@ namespace Resource std::lock_guard lock(_objectCacheMutex); for (typename ObjectCacheMap::iterator itr = _objectCache.begin(); itr != _objectCache.end(); ++itr) { - osg::Object* object = itr->second.first.get(); - if (object) + if (osg::Object* object = itr->second.mValue.get()) { osg::Node* node = dynamic_cast(object); if (node) @@ -185,7 +184,7 @@ namespace Resource { std::lock_guard lock(_objectCacheMutex); for (typename ObjectCacheMap::iterator it = _objectCache.begin(); it != _objectCache.end(); ++it) - f(it->first, it->second.first.get()); + f(it->first, it->second.mValue.get()); } /** Get the number of objects in the cache. */ @@ -202,14 +201,19 @@ namespace Resource const auto it = _objectCache.lower_bound(std::forward(key)); if (it == _objectCache.end()) return std::nullopt; - return std::pair(it->first, it->second.first); + return std::pair(it->first, it->second.mValue); } protected: + struct Item + { + osg::ref_ptr mValue; + double mLastUsage; + }; + virtual ~GenericObjectCache() {} - typedef std::pair, double> ObjectTimeStampPair; - typedef std::map> ObjectCacheMap; + using ObjectCacheMap = std::map>; ObjectCacheMap _objectCache; mutable std::mutex _objectCacheMutex; From 8bbaa57a1491be205e92df596784d805764ebff1 Mon Sep 17 00:00:00 2001 From: elsid Date: Tue, 29 Aug 2023 21:53:47 +0200 Subject: [PATCH 8/8] Add changelog record for #7557 --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5ee31b67623..0448e191880 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -70,6 +70,7 @@ Bug #7472: Crash when enchanting last projectiles Bug #7505: Distant terrain does not support sample size greater than cell size Bug #7553: Faction reaction loading is incorrect + Bug #7557: Terrain::ChunkManager::createChunk is called twice for the same position, lod on initial loading Feature #3537: Shader-based water ripples Feature #5492: Let rain and snow collide with statics Feature #6447: Add LOD support to Object Paging