From 86270339d8967de8c983cd4ab96982f347652d0f Mon Sep 17 00:00:00 2001 From: Mike Balfour <82224783+mbalfour-amzn@users.noreply.github.com> Date: Thu, 28 Oct 2021 16:17:38 -0500 Subject: [PATCH] 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 | 3 +- .../TerrainHeightGradientListComponent.cpp | 8 +++-- .../TerrainLayerSpawnerComponent.cpp | 8 ++++- .../TerrainSurfaceGradientListComponent.cpp | 4 ++- .../Source/TerrainSystem/TerrainSystem.cpp | 36 ++++++++++++++----- .../Code/Source/TerrainSystem/TerrainSystem.h | 4 ++- .../Source/TerrainSystem/TerrainSystemBus.h | 2 +- Gems/Terrain/Code/Tests/LayerSpawnerTests.cpp | 4 +-- .../Tests/TerrainHeightGradientListTests.cpp | 2 +- 9 files changed, 52 insertions(+), 19 deletions(-) diff --git a/Gems/Terrain/Code/Mocks/Terrain/MockTerrain.h b/Gems/Terrain/Code/Mocks/Terrain/MockTerrain.h index 53d93be8ea..f9607fdafb 100644 --- a/Gems/Terrain/Code/Mocks/Terrain/MockTerrain.h +++ b/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 diff --git a/Gems/Terrain/Code/Source/Components/TerrainHeightGradientListComponent.cpp b/Gems/Terrain/Code/Source/Components/TerrainHeightGradientListComponent.cpp index 01ad0bb77f..231d5abc28 100644 --- a/Gems/Terrain/Code/Source/Components/TerrainHeightGradientListComponent.cpp +++ b/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() diff --git a/Gems/Terrain/Code/Source/Components/TerrainLayerSpawnerComponent.cpp b/Gems/Terrain/Code/Source/Components/TerrainLayerSpawnerComponent.cpp index 00ed9c004f..c3803c25e8 100644 --- a/Gems/Terrain/Code/Source/Components/TerrainLayerSpawnerComponent.cpp +++ b/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::HeightData | Terrain::SurfaceData) + ); } } diff --git a/Gems/Terrain/Code/Source/Components/TerrainSurfaceGradientListComponent.cpp b/Gems/Terrain/Code/Source/Components/TerrainSurfaceGradientListComponent.cpp index 5b76f15d74..748221d69e 100644 --- a/Gems/Terrain/Code/Source/Components/TerrainSurfaceGradientListComponent.cpp +++ b/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 diff --git a/Gems/Terrain/Code/Source/TerrainSystem/TerrainSystem.cpp b/Gems/Terrain/Code/Source/TerrainSystem/TerrainSystem.cpp index fa1d483a5d..8d39340b06 100644 --- a/Gems/Terrain/Code/Source/TerrainSystem/TerrainSystem.cpp +++ b/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 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( - changeMask | AzFramework::Terrain::TerrainDataNotifications::TerrainDataChangedMask::Settings); + changeMask = static_cast(changeMask | Terrain::TerrainDataChangedMask::Settings); } if (m_terrainHeightDirty) { - changeMask = static_cast( - changeMask | AzFramework::Terrain::TerrainDataNotifications::TerrainDataChangedMask::HeightData); + changeMask = static_cast(changeMask | Terrain::TerrainDataChangedMask::HeightData); + } + + if (m_terrainSurfacesDirty) + { + changeMask = static_cast(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( diff --git a/Gems/Terrain/Code/Source/TerrainSystem/TerrainSystem.h b/Gems/Terrain/Code/Source/TerrainSystem/TerrainSystem.h index 956424f048..022cd218cc 100644 --- a/Gems/Terrain/Code/Source/TerrainSystem/TerrainSystem.h +++ b/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; diff --git a/Gems/Terrain/Code/Source/TerrainSystem/TerrainSystemBus.h b/Gems/Terrain/Code/Source/TerrainSystem/TerrainSystemBus.h index cda6d65a1e..013d82d94d 100644 --- a/Gems/Terrain/Code/Source/TerrainSystem/TerrainSystemBus.h +++ b/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; diff --git a/Gems/Terrain/Code/Tests/LayerSpawnerTests.cpp b/Gems/Terrain/Code/Tests/LayerSpawnerTests.cpp index 0ca5e7d99a..3778ba860f 100644 --- a/Gems/Terrain/Code/Tests/LayerSpawnerTests.cpp +++ b/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(); diff --git a/Gems/Terrain/Code/Tests/TerrainHeightGradientListTests.cpp b/Gems/Terrain/Code/Tests/TerrainHeightGradientListTests.cpp index c314e4e968..ec500d6ada 100644 --- a/Gems/Terrain/Code/Tests/TerrainHeightGradientListTests.cpp +++ b/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);