SurfaceTagWeights optimization (#7436)

* Add comparison operators to SurfaceTagWeight.

Signed-off-by: Mike Balfour <82224783+mbalfour-amzn@users.noreply.github.com>

* Changed AddSurfaceTagWeight to always combine weights.
This simplifies the API a bit and defines the behavior if someone ever tries to add a duplicate tag.

Signed-off-by: Mike Balfour <82224783+mbalfour-amzn@users.noreply.github.com>

* Added benchmarks for measuring the performance-critical APIs.

Signed-off-by: Mike Balfour <82224783+mbalfour-amzn@users.noreply.github.com>

* Changed SurfaceTagWeights to a fixed_vector.

Signed-off-by: Mike Balfour <82224783+mbalfour-amzn@users.noreply.github.com>

Code/Framework/AzFramework/AzFramework/SurfaceData/SurfaceData.h

@@ -29,6 +29,19 @@ namespace AzFramework::SurfaceData
         {
         }
 
+        //! Equality comparison operator for SurfaceTagWeight.
+        bool operator==(const SurfaceTagWeight& rhs) const
+        {
+            return (m_surfaceType == rhs.m_surfaceType) && (m_weight == rhs.m_weight);
+        }
+
+        //! Inequality comparison operator for SurfaceTagWeight.
+        bool operator!=(const SurfaceTagWeight& rhs) const
+        {
+            return !(*this == rhs);
+        }
+
+
         AZ::Crc32 m_surfaceType = AZ::Crc32(Constants::s_unassignedTagName);
         float m_weight = 0.0f; //! A Value in the range [0.0f .. 1.0f]
 

Gems/GradientSignal/Code/Source/Components/GradientSurfaceDataComponent.cpp

@@ -251,7 +251,7 @@ namespace GradientSignal
                         const float value = m_gradientSampler.GetValue(sampleParams);
                         if (value >= m_configuration.m_thresholdMin && value <= m_configuration.m_thresholdMax)
                         {
-                            weights.AddSurfaceWeightsIfGreater(m_configuration.m_modifierTags, value);
+                            weights.AddSurfaceTagWeights(m_configuration.m_modifierTags, value);
                         }
                     }
                 });

Gems/SurfaceData/Code/Include/SurfaceData/SurfaceDataTypes.h

@@ -24,9 +24,17 @@ namespace SurfaceData
     using SurfaceTagVector = AZStd::vector<SurfaceTag>;
 
     //! SurfaceTagWeights stores a collection of surface tags and weights.
+    //! A surface tag can only appear once in the collection. Attempting to add it multiple times will always preserve the
+    //! highest weight value.
     class SurfaceTagWeights
     {
     public:
+        //! The maximum number of surface weights that we can store.
+        //! For performance reasons, we want to limit this so that we can preallocate the max size in advance.
+        //! The current number is chosen to be higher than expected needs, but small enough to avoid being excessively wasteful.
+        //! (Dynamic structures would end up taking more memory than what we're preallocating)
+        static inline constexpr size_t MaxSurfaceWeights = 16;
+
         SurfaceTagWeights() = default;
 
         //! Construct a collection of SurfaceTagWeights from the given SurfaceTagWeightList.
@@ -45,42 +53,68 @@ namespace SurfaceData
         //! @param weight - The weight to assign to each tag.
         void AssignSurfaceTagWeights(const SurfaceTagVector& tags, float weight);
 
-        //! Add a surface tag weight to this collection.
+        //! Add a surface tag weight to this collection. If the tag already exists, the higher weight will be preserved.
-        //! @param tag - The surface tag.
-        //! @param weight - The surface tag weight.
-        void AddSurfaceTagWeight(const AZ::Crc32 tag, const float weight);
-
-        //! Replace the surface tag weight with the new one if it's higher, or add it if the tag isn't found.
         //! (This method is intentionally inlined for its performance impact)
         //! @param tag - The surface tag.
         //! @param weight - The surface tag weight.
-        void AddSurfaceWeightIfGreater(const AZ::Crc32 tag, const float weight)
+        void AddSurfaceTagWeight(const AZ::Crc32 tag, const float weight)
         {
-            const auto maskItr = m_weights.find(tag);
+            for (auto weightItr = m_weights.begin(); weightItr != m_weights.end(); ++weightItr)
-            const float previousValue = maskItr != m_weights.end() ? maskItr->second : 0.0f;
+            {
-            m_weights[tag] = AZ::GetMax(weight, previousValue);
+                // Since we need to scan for duplicate surface types, store the entries sorted by surface type so that we can
+                // early-out once we pass the location for the entry instead of always searching every entry.
+                if (weightItr->m_surfaceType > tag)
+                {
+                    if (m_weights.size() != MaxSurfaceWeights)
+                    {
+                        // We didn't find the surface type, so add the new entry in sorted order.
+                        m_weights.insert(weightItr, { tag, weight });
+                    }
+                    else
+                    {
+                        AZ_Assert(false, "SurfaceTagWeights has reached max capacity, it cannot add a new tag / weight.");
+                    }
+                    return;
+                }
+                else if (weightItr->m_surfaceType == tag)
+                {
+                    // We found the surface type, so just keep the higher of the two weights.
+                    weightItr->m_weight = AZ::GetMax(weight, weightItr->m_weight);
+                    return;
+                }
+            }
+
+            // We didn't find the surface weight, and the sort order for it is at the end, so add it to the back of the list.
+            if (m_weights.size() != MaxSurfaceWeights)
+            {
+                m_weights.emplace_back(tag, weight);
+            }
+            else
+            {
+                AZ_Assert(false, "SurfaceTagWeights has reached max capacity, it cannot add a new tag / weight.");
+            }
         }
 
-        //! Replace the surface tag weight with the new one if it's higher, or add it if the tag isn't found.
+        //! Add surface tags and weights to this collection. If a tag already exists, the higher weight will be preserved.
         //! (This method is intentionally inlined for its performance impact)
         //! @param tags - The surface tags to replace/add.
         //! @param weight - The surface tag weight to use for each tag.
-        void AddSurfaceWeightsIfGreater(const SurfaceTagVector& tags, const float weight)
+        void AddSurfaceTagWeights(const SurfaceTagVector& tags, const float weight)
         {
             for (const auto& tag : tags)
             {
-                AddSurfaceWeightIfGreater(tag, weight);
+                AddSurfaceTagWeight(tag, weight);
             }
         }
 
-        //! Replace the surface tag weight with the new one if it's higher, or add it if the tag isn't found.
+        //! Add surface tags and weights to this collection. If a tag already exists, the higher weight will be preserved.
         //! (This method is intentionally inlined for its performance impact)
         //! @param weights - The surface tags and weights to replace/add.
-        void AddSurfaceWeightsIfGreater(const SurfaceTagWeights& weights)
+        void AddSurfaceTagWeights(const SurfaceTagWeights& weights)
         {
             for (const auto& [tag, weight] : weights.m_weights)
             {
-                AddSurfaceWeightIfGreater(tag, weight);
+                AddSurfaceTagWeight(tag, weight);
             }
         }
 
@@ -125,7 +159,7 @@ namespace SurfaceData
         //! Check to see if the collection contains the given tag.
         //! @param sampleTag - The tag to look for.
         //! @return True if the tag is found, false if it isn't.
-        bool HasMatchingTag(const AZ::Crc32& sampleTag) const;
+        bool HasMatchingTag(AZ::Crc32 sampleTag) const;
 
         //! Check to see if the collection contains the given tag with the given weight range.
         //! The range check is inclusive on both sides of the range: [weightMin, weightMax]
@@ -133,7 +167,7 @@ namespace SurfaceData
         //! @param weightMin - The minimum weight for this tag.
         //! @param weightMax - The maximum weight for this tag.
         //! @return True if the tag is found, false if it isn't.
-        bool HasMatchingTag(const AZ::Crc32& sampleTag, float weightMin, float weightMax) const;
+        bool HasMatchingTag(AZ::Crc32 sampleTag, float weightMin, float weightMax) const;
 
         //! Check to see if the collection contains any of the given tags.
         //! @param sampleTags - The tags to look for.
@@ -149,7 +183,12 @@ namespace SurfaceData
         bool HasAnyMatchingTags(const SurfaceTagVector& sampleTags, float weightMin, float weightMax) const;
 
     private:
-        AZStd::unordered_map<AZ::Crc32, float> m_weights;
+        //! Search for the given tag entry.
+        //! @param tag - The tag to search for.
+        //! @return The pointer to the tag that's found, or end() if it wasn't found.
+        const AzFramework::SurfaceData::SurfaceTagWeight* FindTag(AZ::Crc32 tag) const;
+
+        AZStd::fixed_vector<AzFramework::SurfaceData::SurfaceTagWeight, MaxSurfaceWeights> m_weights;
     };
 
     //! SurfacePointList stores a collection of surface point data, which consists of positions, normals, and surface tag weights.

Gems/SurfaceData/Code/Source/Components/SurfaceDataColliderComponent.cpp

@@ -259,7 +259,7 @@ namespace SurfaceData
                         if (DoRayTrace(position, queryPointOnly, hitPosition, hitNormal))
                         {
                             // If the query point collides with the volume, add all our modifier tags with a weight of 1.0f.
-                            weights.AddSurfaceWeightsIfGreater(m_configuration.m_modifierTags, 1.0f);
+                            weights.AddSurfaceTagWeights(m_configuration.m_modifierTags, 1.0f);
                         }
                     }
                 });

Gems/SurfaceData/Code/Source/Components/SurfaceDataShapeComponent.cpp

@@ -179,7 +179,7 @@ namespace SurfaceData
                         if (m_shapeBounds.Contains(position) && shape->IsPointInside(position))
                         {
                             // If the point is inside our shape, add all our modifier tags with a weight of 1.0f.
-                            weights.AddSurfaceWeightsIfGreater(m_configuration.m_modifierTags, 1.0f);
+                            weights.AddSurfaceTagWeights(m_configuration.m_modifierTags, 1.0f);
                         }
                     });
                 });

Gems/SurfaceData/Code/Source/SurfaceDataTypes.cpp

@@ -14,28 +14,21 @@ namespace SurfaceData
     void SurfaceTagWeights::AssignSurfaceTagWeights(const AzFramework::SurfaceData::SurfaceTagWeightList& weights)
     {
         m_weights.clear();
-        m_weights.reserve(weights.size());
         for (auto& weight : weights)
         {
-            m_weights.emplace(weight.m_surfaceType, weight.m_weight);
+            AddSurfaceTagWeight(weight.m_surfaceType, weight.m_weight);
         }
     }
 
     void SurfaceTagWeights::AssignSurfaceTagWeights(const SurfaceTagVector& tags, float weight)
     {
         m_weights.clear();
-        m_weights.reserve(tags.size());
         for (auto& tag : tags)
         {
-            m_weights[tag] = weight;
+            AddSurfaceTagWeight(tag.operator AZ::Crc32(), weight);
         }
     }
 
-    void SurfaceTagWeights::AddSurfaceTagWeight(const AZ::Crc32 tag, const float value)
-    {
-        m_weights[tag] = value;
-    }
-
     void SurfaceTagWeights::Clear()
     {
         m_weights.clear();
@@ -52,7 +45,7 @@ namespace SurfaceData
         weights.reserve(m_weights.size());
         for (auto& weight : m_weights)
         {
-            weights.emplace_back(weight.first, weight.second);
+            weights.emplace_back(weight);
         }
         return weights;
     }
@@ -65,17 +58,8 @@ namespace SurfaceData
             return false;
         }
 
-        for (auto& weight : m_weights)
+        // The lists are stored in sorted order, so we can compare every entry in order for equivalence.
-        {
+        return (m_weights == rhs.m_weights);
-            auto rhsWeight = rhs.m_weights.find(weight.first);
-            if ((rhsWeight == rhs.m_weights.end()) || (rhsWeight->second != weight.second))
-            {
-                return false;
-            }
-        }
-
-        // All the entries matched, and the lists are the same size, so they're equal.
-        return true;
     }
 
     bool SurfaceTagWeights::SurfaceWeightsAreEqual(const AzFramework::SurfaceData::SurfaceTagWeightList& compareWeights) const
@@ -92,7 +76,7 @@ namespace SurfaceData
                 compareWeights.begin(), compareWeights.end(),
                 [weight](const AzFramework::SurfaceData::SurfaceTagWeight& compareWeight) -> bool
                 {
-                    return (weight.first == compareWeight.m_surfaceType) && (weight.second == compareWeight.m_weight);
+                    return (weight == compareWeight);
                 });
 
             // If we didn't find a match, they're not equal.
@@ -119,9 +103,9 @@ namespace SurfaceData
 
     bool SurfaceTagWeights::HasValidTags() const
     {
-        for (const auto& sourceTag : m_weights)
+        for (const auto& weight : m_weights)
         {
-            if (sourceTag.first != Constants::s_unassignedTagCrc)
+            if (weight.m_surfaceType != Constants::s_unassignedTagCrc)
             {
                 return true;
             }
@@ -129,9 +113,9 @@ namespace SurfaceData
         return false;
     }
 
-    bool SurfaceTagWeights::HasMatchingTag(const AZ::Crc32& sampleTag) const
+    bool SurfaceTagWeights::HasMatchingTag(AZ::Crc32 sampleTag) const
     {
-        return m_weights.find(sampleTag) != m_weights.end();
+        return FindTag(sampleTag) != m_weights.end();
     }
 
     bool SurfaceTagWeights::HasAnyMatchingTags(const SurfaceTagVector& sampleTags) const
@@ -147,10 +131,10 @@ namespace SurfaceData
         return false;
     }
 
-    bool SurfaceTagWeights::HasMatchingTag(const AZ::Crc32& sampleTag, float weightMin, float weightMax) const
+    bool SurfaceTagWeights::HasMatchingTag(AZ::Crc32 sampleTag, float weightMin, float weightMax) const
     {
-        auto maskItr = m_weights.find(sampleTag);
+        auto weightEntry = FindTag(sampleTag);
-        return maskItr != m_weights.end() && weightMin <= maskItr->second && weightMax >= maskItr->second;
+        return weightEntry != m_weights.end() && weightMin <= weightEntry->m_weight && weightMax >= weightEntry->m_weight;
     }
 
     bool SurfaceTagWeights::HasAnyMatchingTags(const SurfaceTagVector& sampleTags, float weightMin, float weightMax) const
@@ -166,7 +150,25 @@ namespace SurfaceData
         return false;
     }
 
+    const AzFramework::SurfaceData::SurfaceTagWeight* SurfaceTagWeights::FindTag(AZ::Crc32 tag) const
+    {
+        for (auto weightItr = m_weights.begin(); weightItr != m_weights.end(); ++weightItr)
+        {
+            if (weightItr->m_surfaceType == tag)
+            {
+                // Found the tag, return a pointer to the entry.
+                return weightItr;
+            }
+            else if (weightItr->m_surfaceType > tag)
+            {
+                // Our list is stored in sorted order by surfaceType, so early-out if our values get too high.
+                break;
+            }
+        }
 
+        // The tag wasn't found, so return end().
+        return m_weights.end();
+    }
 
 
     SurfacePointList::SurfacePointList(AZStd::initializer_list<const AzFramework::SurfaceData::SurfacePoint> surfacePoints)
@@ -192,7 +194,7 @@ namespace SurfaceData
             if (m_surfacePositionList[index].IsClose(position) && m_surfaceNormalList[index].IsClose(normal))
             {
                 // consolidate points with similar attributes by adding masks/weights to the similar point instead of adding a new one.
-                m_surfaceWeightsList[index].AddSurfaceWeightsIfGreater(masks);
+                m_surfaceWeightsList[index].AddSurfaceTagWeights(masks);
                 return;
             }
             else if (m_surfacePositionList[index].GetZ() < position.GetZ())

Gems/SurfaceData/Code/Tests/SurfaceDataBenchmarks.cpp

@@ -12,6 +12,7 @@
 #include <AzCore/Component/Entity.h>
 #include <AzCore/Debug/Profiler.h>
 #include <AzCore/Memory/PoolAllocator.h>
+#include <AzCore/Math/Random.h>
 #include <AzCore/Math/Vector2.h>
 #include <AzCore/std/containers/array.h>
 #include <AzCore/std/containers/span.h>
@@ -270,6 +271,88 @@ namespace UnitTest
         ->Arg( 2048 )
         ->Unit(::benchmark::kMillisecond);
 
+    BENCHMARK_DEFINE_F(SurfaceDataBenchmark, BM_AddSurfaceTagWeight)(benchmark::State& state)
+    {
+        AZ_PROFILE_FUNCTION(Entity);
+
+        AZ::Crc32 tags[SurfaceData::SurfaceTagWeights::MaxSurfaceWeights];
+        AZ::SimpleLcgRandom randomGenerator(1234567);
+
+        // Declare this outside the loop so that we aren't benchmarking creation and destruction.
+        SurfaceData::SurfaceTagWeights weights;
+
+        bool clearEachTime = state.range(0) > 0;
+
+        // Create a list of randomly-generated tag values.
+        for (auto& tag : tags)
+        {
+            tag = randomGenerator.GetRandom();
+        }
+
+        for (auto _ : state)
+        {
+            // We'll benchmark this two ways:
+            // 1. We clear each time, which means each AddSurfaceWeightIfGreater call will search the whole list then add.
+            // 2. We don't clear, which means that after the first run, AddSurfaceWeightIfGreater will always try to replace values.
+            if (clearEachTime)
+            {
+                weights.Clear();
+            }
+
+            // For each tag, try to add it with a random weight.
+            for (auto& tag : tags)
+            {
+                weights.AddSurfaceTagWeight(tag, randomGenerator.GetRandomFloat());
+            }
+        }
+    }
+
+    BENCHMARK_REGISTER_F(SurfaceDataBenchmark, BM_AddSurfaceTagWeight)
+        ->Arg(false)
+        ->Arg(true)
+        ->ArgName("ClearEachTime");
+
+    BENCHMARK_DEFINE_F(SurfaceDataBenchmark, BM_HasAnyMatchingTags_NoMatches)(benchmark::State& state)
+    {
+        AZ_PROFILE_FUNCTION(Entity);
+
+        AZ::Crc32 tags[SurfaceData::SurfaceTagWeights::MaxSurfaceWeights];
+        AZ::SimpleLcgRandom randomGenerator(1234567);
+
+        // Declare this outside the loop so that we aren't benchmarking creation and destruction.
+        SurfaceData::SurfaceTagWeights weights;
+
+        // Create a list of randomly-generated tag values.
+        for (auto& tag : tags)
+        {
+            // Specifically always set the last bit so that we can create comparison tags that won't match.
+            tag = randomGenerator.GetRandom() | 0x01;
+
+            // Add the tag to our weights list with a random weight.
+            weights.AddSurfaceTagWeight(tag, randomGenerator.GetRandomFloat());
+        }
+
+        // Create a set of similar comparison tags that won't match. We still want a random distribution of values though,
+        // because the SurfaceTagWeights might behave differently with ordered lists.
+        SurfaceData::SurfaceTagVector comparisonTags;
+        for (auto& tag : tags)
+        {
+            comparisonTags.emplace_back(tag ^ 0x01);
+        }
+
+        for (auto _ : state)
+        {
+            // Test to see if any of our tags match.
+            // All of comparison tags should get compared against all of the added tags.
+            bool result = weights.HasAnyMatchingTags(comparisonTags);
+            benchmark::DoNotOptimize(result);
+        }
+    }
+
+    BENCHMARK_REGISTER_F(SurfaceDataBenchmark, BM_HasAnyMatchingTags_NoMatches);
+
+
+
 #endif
 }
 

Gems/SurfaceData/Code/Tests/SurfaceDataTest.cpp

@@ -169,7 +169,7 @@ class MockSurfaceProvider
 
                     if (surfacePoints != m_GetSurfacePoints.end())
                     {
-                        weights.AddSurfaceWeightsIfGreater(m_tags, 1.0f);
+                        weights.AddSurfaceTagWeights(m_tags, 1.0f);
                     }
                 });
         }

Gems/Vegetation/Code/Source/AreaSystemComponent.cpp

@@ -1137,7 +1137,7 @@ namespace Vegetation
                     claimPoint.m_position = position;
                     claimPoint.m_normal = normal;
                     claimPoint.m_masks = masks;
-                    sectorInfo.m_baseContext.m_masks.AddSurfaceWeightsIfGreater(masks);
+                    sectorInfo.m_baseContext.m_masks.AddSurfaceTagWeights(masks);
                     return true;
                 });
         }