Merge branch 'fix_terrain_cache' into 'master'

Terrain cache fixes and optimizations

Closes #7557

See merge request OpenMW/openmw!3388

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 #6149: Dehardcode Lua API_REVISION

apps/openmw/mwworld/scene.cpp

@@ -834,9 +834,6 @@ namespace MWWorld
         mPreloader = std::make_unique<CellPreloader>(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);

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)
     {
     }
 

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)
+                if ((itr->second.mValue != nullptr && itr->second.mValue->referenceCount() > 1)
-                    || itr->second.second == 0.0)
+                    || itr->second.mLastUsage == 0.0)
-                    itr->second.second = referenceTime;
+                    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.first != nullptr)
+                        if (oitr->second.mValue != nullptr)
-                            objectsToRemove.push_back(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<std::mutex> 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<std::mutex> 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<std::mutex> 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<std::mutex> lock(_objectCacheMutex);
             for (typename ObjectCacheMap::iterator itr = _objectCache.begin(); itr != _objectCache.end(); ++itr)
             {
-                osg::Object* object = itr->second.first.get();
+                if (osg::Object* object = itr->second.mValue.get())
-                if (object)
                 {
                     osg::Node* node = dynamic_cast<osg::Node*>(object);
                     if (node)
@@ -185,7 +184,7 @@ namespace Resource
         {
             std::lock_guard<std::mutex> 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. */
@@ -195,11 +194,26 @@ namespace Resource
             return _objectCache.size();
         }
 
+        template <class K>
+        std::optional<std::pair<KeyType, osg::ref_ptr<osg::Object>>> lowerBound(K&& key)
+        {
+            const std::lock_guard<std::mutex> lock(_objectCacheMutex);
+            const auto it = _objectCache.lower_bound(std::forward<K>(key));
+            if (it == _objectCache.end())
+                return std::nullopt;
+            return std::pair(it->first, it->second.mValue);
+        }
+
     protected:
+        struct Item
+        {
+            osg::ref_ptr<osg::Object> mValue;
+            double mLastUsage;
+        };
+
         virtual ~GenericObjectCache() {}
 
-        typedef std::pair<osg::ref_ptr<osg::Object>, double> ObjectTimeStampPair;
+        using ObjectCacheMap = std::map<KeyType, Item, std::less<>>;
-        typedef std::map<KeyType, ObjectTimeStampPair> ObjectCacheMap;
 
         ObjectCacheMap _objectCache;
         mutable std::mutex _objectCacheMutex;

components/resource/resourcemanager.hpp

@@ -3,6 +3,8 @@
 
 #include <osg/ref_ptr>
 
+#include <components/settings/values.hpp>
+
 #include "objectcache.hpp"
 
 namespace VFS
@@ -23,11 +25,11 @@ namespace Resource
     {
     public:
         virtual ~BaseResourceManager() = default;
-        virtual void updateCache(double referenceTime) {}
+        virtual void updateCache(double referenceTime) = 0;
-        virtual void clearCache() {}
+        virtual void clearCache() = 0;
-        virtual void setExpiryDelay(double expiryDelay) {}
+        virtual void setExpiryDelay(double expiryDelay) = 0;
-        virtual void reportStats(unsigned int frameNumber, osg::Stats* stats) const {}
+        virtual void reportStats(unsigned int frameNumber, osg::Stats* stats) const = 0;
-        virtual void releaseGLObjects(osg::State* state) {}
+        virtual void releaseGLObjects(osg::State* state) = 0;
     };
 
     /// @brief Base class for managers that require a virtual file system and object cache.
@@ -39,10 +41,11 @@ namespace Resource
     public:
         typedef GenericObjectCache<KeyType> 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)
         {
         }
 
@@ -59,7 +62,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; }
@@ -77,10 +80,15 @@ namespace Resource
     class ResourceManager : public GenericResourceManager<std::string>
     {
     public:
-        ResourceManager(const VFS::Manager* vfs)
+        explicit ResourceManager(const VFS::Manager* vfs)
             : GenericResourceManager<std::string>(vfs)
         {
         }
+
+        explicit ResourceManager(const VFS::Manager* vfs, double expiryDelay)
+            : GenericResourceManager<std::string>(vfs, expiryDelay)
+        {
+        }
     };
 
 }

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<ChunkId>(nullptr)
+        : GenericResourceManager<ChunkKey>(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<osg::Object> mFoundTemplate;
-    };
-
     osg::ref_ptr<osg::Node> 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<unsigned char>(lodFlags >> (4 * 4));
-        ChunkId id = std::make_tuple(center, lod, lodFlags);
+
-        osg::ref_ptr<osg::Object> obj = mCache->getRefFromObjectCache(id);
+        const ChunkKey key{ .mCenter = center, .mLod = lod, .mLodFlags = lodFlags };
-        if (obj)
+        if (osg::ref_ptr<osg::Object> obj = mCache->getRefFromObjectCache(key))
             return static_cast<osg::Node*>(obj.get());
-        else
+
-        {
+        const TerrainDrawable* templateGeometry = nullptr;
-            FindChunkTemplate find;
+        const TemplateKey templateKey{ .mCenter = center, .mLod = lod };
-            find.mId = id;
+        const auto pair = mCache->lowerBound(templateKey);
-            mCache->call(find);
+        if (pair.has_value() && templateKey == TemplateKey{ .mCenter = pair->first.mCenter, .mLod = pair->first.mLod })
-            TerrainDrawable* templateGeometry
+            templateGeometry = static_cast<const TerrainDrawable*>(pair->second.get());
-                = find.mFoundTemplate ? static_cast<TerrainDrawable*>(find.mFoundTemplate.get()) : nullptr;
+
-            osg::ref_ptr<osg::Node> node = createChunk(size, center, lod, lodFlags, compile, templateGeometry);
+        osg::ref_ptr<osg::Node> node = createChunk(size, center, lod, lodFlags, compile, templateGeometry);
-            mCache->addEntryToObjectCache(id, node.get());
+        mCache->addEntryToObjectCache(key, node.get());
-            return node;
+        return node;
-        }
     }
 
     void ChunkManager::reportStats(unsigned int frameNumber, osg::Stats* stats) const
@@ -80,14 +68,14 @@ namespace Terrain
 
     void ChunkManager::clearCache()
     {
-        GenericResourceManager<ChunkId>::clearCache();
+        GenericResourceManager<ChunkKey>::clearCache();
 
         mBufferCache.clearCache();
     }
 
     void ChunkManager::releaseGLObjects(osg::State* state)
     {
-        GenericResourceManager<ChunkId>::releaseGLObjects(state);
+        GenericResourceManager<ChunkKey>::releaseGLObjects(state);
         mBufferCache.releaseGLObjects(state);
     }
 
@@ -202,7 +190,7 @@ namespace Terrain
     }
 
     osg::ref_ptr<osg::Node> 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<TerrainDrawable> geometry(new TerrainDrawable);
 

components/terrain/chunkmanager.hpp

@@ -28,10 +28,51 @@ namespace Terrain
     class CompositeMap;
     class TerrainDrawable;
 
-    typedef std::tuple<osg::Vec2f, unsigned char, unsigned int> 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<ChunkId>, public QuadTreeWorld::ChunkManager
+    class ChunkManager : public Resource::GenericResourceManager<ChunkKey>, public QuadTreeWorld::ChunkManager
     {
     public:
         ChunkManager(Storage* storage, Resource::SceneManager* sceneMgr, TextureManager* textureManager,
@@ -55,7 +96,7 @@ namespace Terrain
 
     private:
         osg::ref_ptr<osg::Node> 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<osg::Texture2D> createCompositeMapRTT();
 

components/terrain/quadtreeworld.cpp

@@ -544,37 +544,16 @@ namespace Terrain
         vd->setViewPoint(viewPoint);
         vd->setActiveGrid(grid);
 
-        for (unsigned int pass = 0; pass < 3; ++pass)
+        DefaultLodCallback lodCallback(mLodFactor, mMinSize, mViewDistance, grid, cellWorldSize);
+        mRootNode->traverseNodes(vd, viewPoint, &lodCallback);
+
+        reporter.addTotal(vd->getNumEntries());
+
+        for (unsigned int i = 0, n = vd->getNumEntries(); i < n && !abort; ++i)
         {
-            unsigned int startEntry = vd->getNumEntries();
+            ViewDataEntry& entry = vd->getEntry(i);
-
+            loadRenderingNode(entry, vd, cellWorldSize, grid, true);
-            float distanceModifier = 0.f;
+            reporter.addProgress(1);
-            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);
-            }
-
-            for (unsigned int i = startEntry; i < vd->getNumEntries() && !abort; ++i)
-            {
-                ViewDataEntry& entry = vd->getEntry(i);
-
-                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
-            }
         }
     }
 

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: