Added terrain surface data notifications (#5067)

* Fix notifications for surface data changes.
Separated the notifications from the surface component and the height component to add a reason to a RefreshArea request.  This makes it possible to distinguish between surface changes and height changes and provide the appropriate OnTerrainDataChanged flags.

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

* Reworked to use a changeMask instead of separate calls.

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

* PR Feedback
Judicious use of "using" to reduce a bunch of bulky namespaces.

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

Gems/Terrain/Code/Mocks/Terrain/MockTerrain.h

@@ -34,7 +34,8 @@ namespace UnitTest
 
         MOCK_METHOD1(RegisterArea, void(AZ::EntityId areaId));
         MOCK_METHOD1(UnregisterArea, void(AZ::EntityId areaId));
-        MOCK_METHOD1(RefreshArea, void(AZ::EntityId areaId));
+        MOCK_METHOD2(RefreshArea,
+            void(AZ::EntityId areaId, AzFramework::Terrain::TerrainDataNotifications::TerrainDataChangedMask changeMask));
     };
 
     class MockTerrainDataNotificationListener : public AzFramework::Terrain::TerrainDataNotificationBus::Handler

Gems/Terrain/Code/Source/Components/TerrainHeightGradientListComponent.cpp

@@ -119,7 +119,9 @@ namespace Terrain
         LmbrCentral::DependencyNotificationBus::Handler::BusDisconnect();
 
         // Since this height data will no longer exist, notify the terrain system to refresh the area.
-        TerrainSystemServiceRequestBus::Broadcast(&TerrainSystemServiceRequestBus::Events::RefreshArea, GetEntityId());
+        TerrainSystemServiceRequestBus::Broadcast(
+            &TerrainSystemServiceRequestBus::Events::RefreshArea, GetEntityId(),
+            AzFramework::Terrain::TerrainDataNotifications::HeightData);
     }
 
     bool TerrainHeightGradientListComponent::ReadInConfig(const AZ::ComponentConfig* baseConfig)
@@ -176,7 +178,9 @@ namespace Terrain
     void TerrainHeightGradientListComponent::OnCompositionChanged()
     {
         RefreshMinMaxHeights();
-        TerrainSystemServiceRequestBus::Broadcast(&TerrainSystemServiceRequestBus::Events::RefreshArea, GetEntityId());
+        TerrainSystemServiceRequestBus::Broadcast(
+            &TerrainSystemServiceRequestBus::Events::RefreshArea, GetEntityId(),
+            AzFramework::Terrain::TerrainDataNotifications::HeightData);
     }
 
     void TerrainHeightGradientListComponent::RefreshMinMaxHeights()

Gems/Terrain/Code/Source/Components/TerrainLayerSpawnerComponent.cpp

@@ -157,6 +157,12 @@ namespace Terrain
 
     void TerrainLayerSpawnerComponent::RefreshArea()
     {
-        TerrainSystemServiceRequestBus::Broadcast(&TerrainSystemServiceRequestBus::Events::RefreshArea, GetEntityId());
+        using Terrain = AzFramework::Terrain::TerrainDataNotifications;
+
+        // Notify the terrain system that the entire layer has changed, so both height and surface data can be affected.
+        TerrainSystemServiceRequestBus::Broadcast(
+            &TerrainSystemServiceRequestBus::Events::RefreshArea, GetEntityId(),
+            static_cast<Terrain::TerrainDataChangedMask>(Terrain::HeightData | Terrain::SurfaceData)
+        );
     }
 }

Gems/Terrain/Code/Source/Components/TerrainSurfaceGradientListComponent.cpp

@@ -184,7 +184,9 @@ namespace Terrain
 
     void TerrainSurfaceGradientListComponent::OnCompositionChanged()
     {
-        TerrainSystemServiceRequestBus::Broadcast(&TerrainSystemServiceRequestBus::Events::RefreshArea, GetEntityId());
+        TerrainSystemServiceRequestBus::Broadcast(
+            &TerrainSystemServiceRequestBus::Events::RefreshArea, GetEntityId(),
+            AzFramework::Terrain::TerrainDataNotifications::SurfaceData);
     }
 
 } // namespace Terrain

Gems/Terrain/Code/Source/TerrainSystem/TerrainSystem.cpp

@@ -76,6 +76,7 @@ void TerrainSystem::Activate()
     m_dirtyRegion = AZ::Aabb::CreateNull();
     m_terrainHeightDirty = true;
     m_terrainSettingsDirty = true;
+    m_terrainSurfacesDirty = true;
     m_requestedSettings.m_systemActive = true;
 
     {
@@ -115,6 +116,7 @@ void TerrainSystem::Deactivate()
     m_dirtyRegion = AZ::Aabb::CreateNull();
     m_terrainHeightDirty = true;
     m_terrainSettingsDirty = true;
+    m_terrainSurfacesDirty = true;
     m_requestedSettings.m_systemActive = false;
 
     AzFramework::Terrain::TerrainDataNotificationBus::Broadcast(
@@ -549,6 +551,7 @@ void TerrainSystem::RegisterArea(AZ::EntityId areaId)
     m_registeredAreas[areaId] = aabb;
     m_dirtyRegion.AddAabb(aabb);
     m_terrainHeightDirty = true;
+    m_terrainSurfacesDirty = true;
 }
 
 void TerrainSystem::UnregisterArea(AZ::EntityId areaId)
@@ -567,14 +570,17 @@ void TerrainSystem::UnregisterArea(AZ::EntityId areaId)
             {
                 m_dirtyRegion.AddAabb(aabb);
                 m_terrainHeightDirty = true;
+                m_terrainSurfacesDirty = true;
                 return true;
             }
             return false;
         });
 }
 
-void TerrainSystem::RefreshArea(AZ::EntityId areaId)
+void TerrainSystem::RefreshArea(AZ::EntityId areaId, AzFramework::Terrain::TerrainDataNotifications::TerrainDataChangedMask changeMask)
 {
+    using Terrain = AzFramework::Terrain::TerrainDataNotifications;
+
     AZStd::unique_lock<AZStd::shared_mutex> lock(m_areaMutex);
 
     auto areaAabb = m_registeredAreas.find(areaId);
@@ -588,11 +594,18 @@ void TerrainSystem::RefreshArea(AZ::EntityId areaId)
     expandedAabb.AddAabb(newAabb);
 
     m_dirtyRegion.AddAabb(expandedAabb);
-    m_terrainHeightDirty = true;
+
+    // Keep track of which types of data have changed so that we can send out the appropriate notifications later.
+
+    m_terrainHeightDirty = m_terrainHeightDirty || ((changeMask & Terrain::HeightData) == Terrain::HeightData);
+
+    m_terrainSurfacesDirty = m_terrainSurfacesDirty || ((changeMask & Terrain::SurfaceData) == Terrain::SurfaceData);
 }
 
 void TerrainSystem::OnTick(float /*deltaTime*/, AZ::ScriptTimePoint /*time*/)
 {
+    using Terrain = AzFramework::Terrain::TerrainDataNotifications;
+
     bool terrainSettingsChanged = false;
 
     if (m_terrainSettingsDirty)
@@ -607,6 +620,7 @@ void TerrainSystem::OnTick(float /*deltaTime*/, AZ::ScriptTimePoint /*time*/)
             m_dirtyRegion = m_currentSettings.m_worldBounds;
             m_dirtyRegion.AddAabb(m_requestedSettings.m_worldBounds);
             m_terrainHeightDirty = true;
+            m_terrainSurfacesDirty = true;
             m_currentSettings.m_worldBounds = m_requestedSettings.m_worldBounds;
         }
 
@@ -614,12 +628,13 @@ void TerrainSystem::OnTick(float /*deltaTime*/, AZ::ScriptTimePoint /*time*/)
         {
             m_dirtyRegion = AZ::Aabb::CreateNull();
             m_terrainHeightDirty = true;
+            m_terrainSurfacesDirty = true;
         }
 
         m_currentSettings = m_requestedSettings;
     }
 
-    if (terrainSettingsChanged || m_terrainHeightDirty)
+    if (terrainSettingsChanged || m_terrainHeightDirty || m_terrainSurfacesDirty)
     {
         // Block other threads from accessing the surface data bus while we are in GetValue (which may call into the SurfaceData bus).
         // We lock our surface data mutex *before* checking / setting "isRequestInProgress" so that we prevent race conditions
@@ -629,24 +644,27 @@ void TerrainSystem::OnTick(float /*deltaTime*/, AZ::ScriptTimePoint /*time*/)
         auto& surfaceDataContext = SurfaceData::SurfaceDataSystemRequestBus::GetOrCreateContext(false);
         typename SurfaceData::SurfaceDataSystemRequestBus::Context::DispatchLockGuard scopeLock(surfaceDataContext.m_contextMutex);
 
-        AzFramework::Terrain::TerrainDataNotifications::TerrainDataChangedMask changeMask =
-            AzFramework::Terrain::TerrainDataNotifications::TerrainDataChangedMask::None;
+        Terrain::TerrainDataChangedMask changeMask = Terrain::TerrainDataChangedMask::None;
 
         if (terrainSettingsChanged)
         {
-            changeMask = static_cast<AzFramework::Terrain::TerrainDataNotifications::TerrainDataChangedMask>(
-                changeMask | AzFramework::Terrain::TerrainDataNotifications::TerrainDataChangedMask::Settings);
+            changeMask = static_cast<Terrain::TerrainDataChangedMask>(changeMask | Terrain::TerrainDataChangedMask::Settings);
         }
         if (m_terrainHeightDirty)
         {
-            changeMask = static_cast<AzFramework::Terrain::TerrainDataNotifications::TerrainDataChangedMask>(
-                changeMask | AzFramework::Terrain::TerrainDataNotifications::TerrainDataChangedMask::HeightData);
+            changeMask = static_cast<Terrain::TerrainDataChangedMask>(changeMask | Terrain::TerrainDataChangedMask::HeightData);
+        }
+
+        if (m_terrainSurfacesDirty)
+        {
+            changeMask = static_cast<Terrain::TerrainDataChangedMask>(changeMask | Terrain::TerrainDataChangedMask::SurfaceData);
         }
 
         // Make sure to set these *before* calling OnTerrainDataChanged, since it's possible that subsystems reacting to that call will
         // cause the data to become dirty again.
         AZ::Aabb dirtyRegion = m_dirtyRegion;
         m_terrainHeightDirty = false;
+        m_terrainSurfacesDirty = false;
         m_dirtyRegion = AZ::Aabb::CreateNull();
 
         AzFramework::Terrain::TerrainDataNotificationBus::Broadcast(

Gems/Terrain/Code/Source/TerrainSystem/TerrainSystem.h

@@ -47,7 +47,8 @@ namespace Terrain
 
         void RegisterArea(AZ::EntityId areaId) override;
         void UnregisterArea(AZ::EntityId areaId) override;
-        void RefreshArea(AZ::EntityId areaId) override;
+        void RefreshArea(
+            AZ::EntityId areaId, AzFramework::Terrain::TerrainDataNotifications::TerrainDataChangedMask changeMask) override;
 
         ///////////////////////////////////////////
         // TerrainDataRequestBus::Handler Impl
@@ -164,6 +165,7 @@ namespace Terrain
 
         bool m_terrainSettingsDirty = true;
         bool m_terrainHeightDirty = false;
+        bool m_terrainSurfacesDirty = false;
         AZ::Aabb m_dirtyRegion;
 
         mutable AZStd::shared_mutex m_areaMutex;

Gems/Terrain/Code/Source/TerrainSystem/TerrainSystemBus.h

@@ -44,7 +44,7 @@ namespace Terrain
         // register an area to override terrain
         virtual void RegisterArea(AZ::EntityId areaId) = 0;
         virtual void UnregisterArea(AZ::EntityId areaId) = 0;
-        virtual void RefreshArea(AZ::EntityId areaId) = 0;
+        virtual void RefreshArea(AZ::EntityId areaId, AzFramework::Terrain::TerrainDataNotifications::TerrainDataChangedMask changeMask) = 0;
     };
 
     using TerrainSystemServiceRequestBus = AZ::EBus<TerrainSystemServiceRequests>;

Gems/Terrain/Code/Tests/LayerSpawnerTests.cpp

@@ -190,7 +190,7 @@ TEST_F(LayerSpawnerComponentTest, LayerSpawnerTransformChangedUpdatesTerrainSyst
     CreateMockTerrainSystem();
 
     // The TransformChanged call should refresh the area.
-    EXPECT_CALL(*m_terrainSystem, RefreshArea(_)).Times(1);
+    EXPECT_CALL(*m_terrainSystem, RefreshArea(_, _)).Times(1);
 
     AddLayerSpawnerAndShapeComponentToEntity();
 
@@ -211,7 +211,7 @@ TEST_F(LayerSpawnerComponentTest, LayerSpawnerShapeChangedUpdatesTerrainSystem)
     CreateMockTerrainSystem();
 
     // The ShapeChanged call should refresh the area.
-    EXPECT_CALL(*m_terrainSystem, RefreshArea(_)).Times(1);
+    EXPECT_CALL(*m_terrainSystem, RefreshArea(_, _)).Times(1);
 
     AddLayerSpawnerAndShapeComponentToEntity();
 

Gems/Terrain/Code/Tests/TerrainHeightGradientListTests.cpp

@@ -93,7 +93,7 @@ TEST_F(TerrainHeightGradientListComponentTest, TerrainHeightGradientRefreshesTer
     // As the TerrainHeightGradientListComponent subscribes to the dependency monitor, RefreshArea will be called twice:
     // once due to OnCompositionChanged being picked up by the the dependency monitor and resending the notification,
     // and once when the HeightGradientListComponent gets the OnCompositionChanged directly through the DependencyNotificationBus.
-    EXPECT_CALL(terrainSystem, RefreshArea(_)).Times(2);
+    EXPECT_CALL(terrainSystem, RefreshArea(_, _)).Times(2);
 
     LmbrCentral::DependencyNotificationBus::Event(m_entity->GetId(), &LmbrCentral::DependencyNotificationBus::Events::OnCompositionChanged);