Merge branch 'optimize_get_land' into 'master'

Optimize Storage::getLand calls

See merge request OpenMW/openmw!3379

apps/openmw/mwrender/landmanager.cpp

@@ -19,9 +19,8 @@ namespace MWRender
 
     osg::ref_ptr<ESMTerrain::LandObject> LandManager::getLand(ESM::ExteriorCellLocation cellIndex)
     {
-        const osg::ref_ptr<osg::Object> obj = mCache->getRefFromObjectCache(cellIndex);
+        if (const std::optional<osg::ref_ptr<osg::Object>> obj = mCache->getRefFromObjectCacheOrNone(cellIndex))
-        if (obj != nullptr)
+            return static_cast<ESMTerrain::LandObject*>(obj->get());
-            return static_cast<ESMTerrain::LandObject*>(obj.get());
 
         const MWBase::World& world = *MWBase::Environment::get().getWorld();
         osg::ref_ptr<ESMTerrain::LandObject> landObj = nullptr;
@@ -29,16 +28,14 @@ namespace MWRender
         if (ESM::isEsm4Ext(cellIndex.mWorldspace))
         {
             const ESM4::Land* land = world.getStore().get<ESM4::Land>().search(cellIndex);
-            if (land == nullptr)
+            if (land != nullptr)
-                return nullptr;
+                landObj = new ESMTerrain::LandObject(*land, mLoadFlags);
-            landObj = new ESMTerrain::LandObject(*land, mLoadFlags);
         }
         else
         {
             const ESM::Land* land = world.getStore().get<ESM::Land>().search(cellIndex.mX, cellIndex.mY);
-            if (land == nullptr)
+            if (land != nullptr)
-                return nullptr;
+                landObj = new ESMTerrain::LandObject(*land, mLoadFlags);
-            landObj = new ESMTerrain::LandObject(*land, mLoadFlags);
         }
 
         mCache->addEntryToObjectCache(cellIndex, landObj.get());

components/esmterrain/storage.cpp

@@ -1,6 +1,7 @@
 #include "storage.hpp"
 
 #include <algorithm>
+#include <optional>
 #include <stdexcept>
 
 #include <osg/Image>
@@ -43,8 +44,45 @@ namespace ESMTerrain
     class LandCache
     {
     public:
-        typedef std::map<ESM::ExteriorCellLocation, osg::ref_ptr<const LandObject>> Map;
+        explicit LandCache(int offsetX, int offsetY, std::size_t size)
-        Map mMap;
+            : mOffsetX(offsetX)
+            , mOffsetY(offsetY)
+            , mSize(size)
+            , mValues(size * size)
+        {
+        }
+
+        std::optional<const LandObject*> find(int x, int y) const
+        {
+            const std::size_t index = getIndex(x, y);
+            if (const auto& value = mValues[index])
+                return value->get();
+            return std::nullopt;
+        }
+
+        void insert(int x, int y, osg::ref_ptr<const LandObject>&& value)
+        {
+            const std::size_t index = getIndex(x, y);
+            mValues[index] = std::move(value);
+        }
+
+    private:
+        int mOffsetX;
+        int mOffsetY;
+        std::size_t mSize;
+        std::vector<std::optional<osg::ref_ptr<const LandObject>>> mValues;
+
+        std::size_t getIndex(int x, int y) const
+        {
+            return normalizeCoordinate(x, mOffsetX) * mSize + normalizeCoordinate(y, mOffsetY);
+        }
+
+        std::size_t normalizeCoordinate(int value, int offset) const
+        {
+            assert(value >= offset);
+            assert(value < offset + static_cast<int>(mSize));
+            return static_cast<std::size_t>(value - offset);
+        }
     };
 
     LandObject::LandObject(const ESM4::Land& land, int loadFlags)
@@ -217,13 +255,12 @@ namespace ESMTerrain
         normals.resize(numVerts * numVerts);
         colours.resize(numVerts * numVerts);
 
-        LandCache cache;
-
         const bool alteration = useAlteration();
         const int landSizeInUnits = ESM::getCellSize(worldspace);
         const osg::Vec2f origin = center - osg::Vec2f(size, size) * 0.5f;
         const int startCellX = static_cast<int>(std::floor(origin.x()));
         const int startCellY = static_cast<int>(std::floor(origin.y()));
+        LandCache cache(startCellX - 1, startCellY - 1, static_cast<std::size_t>(std::ceil(size)) + 2);
         std::pair lastCell{ startCellX, startCellY };
         const LandObject* land = getLand(ESM::ExteriorCellLocation(startCellX, startCellY, worldspace), cache);
         const ESM::LandData* heightData = nullptr;
@@ -361,7 +398,7 @@ namespace ESMTerrain
         const std::size_t blendmapImageSize = blendmapSize * imageScaleFactor;
 
         std::vector<UniqueTextureId> textureIds(blendmapSize * blendmapSize);
-        LandCache cache;
+        LandCache cache(startCellX - 1, startCellY - 1, static_cast<std::size_t>(std::ceil(chunkSize)) + 2);
         std::pair lastCell{ startCellX, startCellY };
         const LandObject* land = getLand(ESM::ExteriorCellLocation(startCellX, startCellY, worldspace), cache);
 
@@ -514,14 +551,12 @@ namespace ESMTerrain
 
     const LandObject* Storage::getLand(ESM::ExteriorCellLocation cellLocation, LandCache& cache)
     {
-        LandCache::Map::iterator found = cache.mMap.find(cellLocation);
+        if (const auto land = cache.find(cellLocation.mX, cellLocation.mY))
-        if (found != cache.mMap.end())
+            return *land;
-            return found->second;
+        osg::ref_ptr<const LandObject> land = getLand(cellLocation);
-        else
+        const LandObject* result = land.get();
-        {
+        cache.insert(cellLocation.mX, cellLocation.mY, std::move(land));
-            found = cache.mMap.insert(std::make_pair(cellLocation, getLand(cellLocation))).first;
+        return result;
-            return found->second;
-        }
     }
 
     void Storage::adjustColor(int col, int row, const ESM::LandData* heightData, osg::Vec4ub& color) const {}

components/resource/objectcache.hpp

@@ -26,6 +26,7 @@
 
 #include <map>
 #include <mutex>
+#include <optional>
 #include <string>
 
 namespace osg
@@ -82,7 +83,8 @@ namespace Resource
                 {
                     if (oitr->second.second <= expiryTime)
                     {
-                        objectsToRemove.push_back(oitr->second.first);
+                        if (oitr->second.first != nullptr)
+                            objectsToRemove.push_back(oitr->second.first);
                         _objectCache.erase(oitr++);
                     }
                     else
@@ -127,6 +129,15 @@ namespace Resource
                 return nullptr;
         }
 
+        std::optional<osg::ref_ptr<osg::Object>> getRefFromObjectCacheOrNone(const KeyType& key)
+        {
+            const std::lock_guard<std::mutex> lock(_objectCacheMutex);
+            const auto it = _objectCache.find(key);
+            if (it == _objectCache.end())
+                return std::nullopt;
+            return it->second.first;
+        }
+
         /** Check if an object is in the cache, and if it is, update its usage time stamp. */
         bool checkInObjectCache(const KeyType& key, double timeStamp)
         {