From 1979ee1491a2e8065a00e998b2b357445528c6c4 Mon Sep 17 00:00:00 2001
From: Bo Svensson <90132211+bosvensson1@users.noreply.github.com>
Date: Thu, 4 Nov 2021 15:54:47 +0000
Subject: [PATCH] refactors hashed std::map (#3199)

We currently apply a strange algorithm to `LightManager::mStateSetCache`. For some reason this algorithm inserts hashed keys into `std::map` in a way that fails to handle hash collisions and exhibits worse lookup complexity than `std::unordered_map`. With this PR we just use `std::unordered_map` here.
---
 components/sceneutil/lightmanager.cpp | 42 +++++++++++++++++----------
 components/sceneutil/lightmanager.hpp |  8 +++--
 2 files changed, 33 insertions(+), 17 deletions(-)

diff --git a/components/sceneutil/lightmanager.cpp b/components/sceneutil/lightmanager.cpp
index f38fd80d26..a5ce448b75 100644
--- a/components/sceneutil/lightmanager.cpp
+++ b/components/sceneutil/lightmanager.cpp
@@ -1,6 +1,8 @@
 #include "lightmanager.hpp"
 
 #include <array>
+#include <algorithm>
+#include <iterator>
 
 #include <osg/BufferObject>
 #include <osg/BufferIndexBinding>
@@ -1158,29 +1160,39 @@ namespace SceneUtil
         return mSun;
     }
 
+    size_t LightManager::HashLightIdList::operator()(const LightIdList& lightIdList) const
+    {
+        size_t hash = 0;
+        for (size_t i = 0; i < lightIdList.size(); ++i)
+            Misc::hashCombine(hash, lightIdList[i]);
+        return hash;
+    }
+
     osg::ref_ptr<osg::StateSet> LightManager::getLightListStateSet(const LightList& lightList, size_t frameNum, const osg::RefMatrix* viewMatrix)
     {
         // possible optimization: return a StateSet containing all requested lights plus some extra lights (if a suitable one exists)
-        size_t hash = 0;
-        for (size_t i = 0; i < lightList.size(); ++i)
+
+        if (getLightingMethod() == LightingMethod::SingleUBO)
         {
-            auto id = lightList[i]->mLightSource->getId();
-            Misc::hashCombine(hash, id);
+            for (size_t i = 0; i < lightList.size(); ++i)
+            {
+                auto id = lightList[i]->mLightSource->getId();
+                if (getLightIndexMap(frameNum).find(id) != getLightIndexMap(frameNum).end())
+                    continue;
 
-            if (getLightingMethod() != LightingMethod::SingleUBO)
-                continue;
-
-            if (getLightIndexMap(frameNum).find(id) != getLightIndexMap(frameNum).end())
-                continue;
-
-            int index = getLightIndexMap(frameNum).size() + 1;
-            updateGPUPointLight(index, lightList[i]->mLightSource, frameNum, viewMatrix);
-            getLightIndexMap(frameNum).emplace(lightList[i]->mLightSource->getId(), index);
+                int index = getLightIndexMap(frameNum).size() + 1;
+                updateGPUPointLight(index, lightList[i]->mLightSource, frameNum, viewMatrix);
+                getLightIndexMap(frameNum).emplace(id, index);
+            }
         }
 
         auto& stateSetCache = mStateSetCache[frameNum%2];
 
-        auto found = stateSetCache.find(hash);
+        LightIdList lightIdList;
+        lightIdList.reserve(lightList.size());
+        std::transform(lightList.begin(), lightList.end(), std::back_inserter(lightIdList), [] (const LightSourceViewBound* l) { return l->mLightSource->getId(); });
+
+        auto found = stateSetCache.find(lightIdList);
         if (found != stateSetCache.end())
         {
             mStateSetGenerator->update(found->second, lightList, frameNum);
@@ -1188,7 +1200,7 @@ namespace SceneUtil
         }
 
         auto stateset = mStateSetGenerator->generate(lightList, frameNum);
-        stateSetCache.emplace(hash, stateset);
+        stateSetCache.emplace(lightIdList, stateset);
         return stateset;
     }
 
diff --git a/components/sceneutil/lightmanager.hpp b/components/sceneutil/lightmanager.hpp
index b518a4723c..4a7dc7dbe7 100644
--- a/components/sceneutil/lightmanager.hpp
+++ b/components/sceneutil/lightmanager.hpp
@@ -207,8 +207,12 @@ namespace SceneUtil
         using LightSourceViewBoundCollection = std::vector<LightSourceViewBound>;
         std::map<osg::observer_ptr<osg::Camera>, LightSourceViewBoundCollection> mLightsInViewSpace;
 
-        // < Light list hash , StateSet >
-        using LightStateSetMap = std::map<size_t, osg::ref_ptr<osg::StateSet>>;
+        using LightIdList = std::vector<int>;
+        struct HashLightIdList
+        {
+            size_t operator()(const LightIdList&) const;
+        };
+        using LightStateSetMap = std::unordered_map<LightIdList, osg::ref_ptr<osg::StateSet>, HashLightIdList>;
         LightStateSetMap mStateSetCache[2];
 
         std::vector<osg::ref_ptr<osg::StateAttribute>> mDummies;