From 39ba06b84ed5c93c6a2c3a4dff2f3d3b4c214a6c Mon Sep 17 00:00:00 2001 From: Mike Balfour <82224783+mbalfour-amzn@users.noreply.github.com> Date: Tue, 23 Nov 2021 09:21:27 -0600 Subject: [PATCH] misc small terrain bugfixes (#5834) * Removed "chatty" profile markers. These three markers were making terrain refreshes take ~200% longer in profile builds. Signed-off-by: Mike Balfour <82224783+mbalfour-amzn@users.noreply.github.com> * Terrain surface data wasn't updating its AABB correctly. The code was making an invalid assumption that changes to the world bounds would result in an invalid dirtyRegion. It actually gets a valid region encapsulating the old and new bounds. The fix is an additional check to see if the terrain bounds changed. Signed-off-by: Mike Balfour <82224783+mbalfour-amzn@users.noreply.github.com> * Addressed previous PR feedback that terrainSystem ought to be a local variable in each test. Signed-off-by: Mike Balfour <82224783+mbalfour-amzn@users.noreply.github.com> --- .../Components/GradientTransformComponent.cpp | 2 - .../Components/ImageGradientComponent.cpp | 2 - .../GradientSignal/Code/Source/ImageAsset.cpp | 2 - .../TerrainSurfaceDataSystemComponent.cpp | 8 +- Gems/Terrain/Code/Tests/TerrainSystemTest.cpp | 73 +++++++++---------- 5 files changed, 40 insertions(+), 47 deletions(-) diff --git a/Gems/GradientSignal/Code/Source/Components/GradientTransformComponent.cpp b/Gems/GradientSignal/Code/Source/Components/GradientTransformComponent.cpp index 38936dc302..57de6f776a 100644 --- a/Gems/GradientSignal/Code/Source/Components/GradientTransformComponent.cpp +++ b/Gems/GradientSignal/Code/Source/Components/GradientTransformComponent.cpp @@ -324,8 +324,6 @@ namespace GradientSignal void GradientTransformComponent::TransformPositionToUVW(const AZ::Vector3& inPosition, AZ::Vector3& outUVW, const bool shouldNormalizeOutput, bool& wasPointRejected) const { - AZ_PROFILE_FUNCTION(Entity); - AZStd::lock_guard lock(m_cacheMutex); //transforming coordinate into "local" relative space of shape bounds diff --git a/Gems/GradientSignal/Code/Source/Components/ImageGradientComponent.cpp b/Gems/GradientSignal/Code/Source/Components/ImageGradientComponent.cpp index 6fe8d32484..021e083ea6 100644 --- a/Gems/GradientSignal/Code/Source/Components/ImageGradientComponent.cpp +++ b/Gems/GradientSignal/Code/Source/Components/ImageGradientComponent.cpp @@ -191,8 +191,6 @@ namespace GradientSignal float ImageGradientComponent::GetValue(const GradientSampleParams& sampleParams) const { - AZ_PROFILE_FUNCTION(Entity); - AZ::Vector3 uvw = sampleParams.m_position; bool wasPointRejected = false; diff --git a/Gems/GradientSignal/Code/Source/ImageAsset.cpp b/Gems/GradientSignal/Code/Source/ImageAsset.cpp index c67f86c6b8..7e73dc32d6 100644 --- a/Gems/GradientSignal/Code/Source/ImageAsset.cpp +++ b/Gems/GradientSignal/Code/Source/ImageAsset.cpp @@ -153,8 +153,6 @@ namespace GradientSignal float GetValueFromImageAsset(const AZ::Data::Asset& imageAsset, const AZ::Vector3& uvw, float tilingX, float tilingY, float defaultValue) { - AZ_PROFILE_FUNCTION(Entity); - if (imageAsset.IsReady()) { const auto& image = imageAsset.Get(); diff --git a/Gems/Terrain/Code/Source/Components/TerrainSurfaceDataSystemComponent.cpp b/Gems/Terrain/Code/Source/Components/TerrainSurfaceDataSystemComponent.cpp index 7395e66d06..65e8600d62 100644 --- a/Gems/Terrain/Code/Source/Components/TerrainSurfaceDataSystemComponent.cpp +++ b/Gems/Terrain/Code/Source/Components/TerrainSurfaceDataSystemComponent.cpp @@ -214,10 +214,10 @@ namespace Terrain { AZ_Assert((m_providerHandle != SurfaceData::InvalidSurfaceDataRegistryHandle), "Invalid surface data handle"); - // Our terrain was valid before and after, it just changed in some way. If we have a valid dirty region passed in - // then it's possible that the heightmap has been modified in the Editor. Otherwise, just notify that the entire - // terrain has changed in some way. - if (dirtyRegion.IsValid()) + // Our terrain was valid before and after, it just changed in some way. If we have a valid dirty region, and the terrain + // bounds themselves haven't changed, just notify that our terrain data has changed within the bounds. Otherwise, notify + // that the entire terrain provider needs to be updated, since it either has new bounds or the entire set of data is dirty. + if (dirtyRegion.IsValid() && m_terrainBounds.IsClose(terrainBoundsBeforeUpdate)) { SurfaceData::SurfaceDataSystemRequestBus::Broadcast( &SurfaceData::SurfaceDataSystemRequestBus::Events::RefreshSurfaceData, dirtyRegion); diff --git a/Gems/Terrain/Code/Tests/TerrainSystemTest.cpp b/Gems/Terrain/Code/Tests/TerrainSystemTest.cpp index 4d149e8527..c2f8c2620c 100644 --- a/Gems/Terrain/Code/Tests/TerrainSystemTest.cpp +++ b/Gems/Terrain/Code/Tests/TerrainSystemTest.cpp @@ -41,7 +41,6 @@ namespace UnitTest }; AZ::ComponentApplication m_app; - AZStd::unique_ptr m_terrainSystem; AZStd::unique_ptr> m_boxShapeRequests; AZStd::unique_ptr> m_shapeRequests; @@ -59,7 +58,6 @@ namespace UnitTest void TearDown() override { - m_terrainSystem.reset(); m_boxShapeRequests.reset(); m_shapeRequests.reset(); m_terrainAreaHeightRequests.reset(); @@ -96,16 +94,17 @@ namespace UnitTest // Create a terrain system with reasonable defaults for testing, but with the ability to override the defaults // on a test-by-test basis. - void CreateAndActivateTerrainSystem( + AZStd::unique_ptr CreateAndActivateTerrainSystem( AZ::Vector2 queryResolution = AZ::Vector2(1.0f), AZ::Aabb worldBounds = AZ::Aabb::CreateFromMinMax(AZ::Vector3(-128.0f), AZ::Vector3(128.0f))) { // Create the terrain system and give it one tick to fully initialize itself. - m_terrainSystem = AZStd::make_unique(); - m_terrainSystem->SetTerrainAabb(worldBounds); - m_terrainSystem->SetTerrainHeightQueryResolution(queryResolution); - m_terrainSystem->Activate(); + auto terrainSystem = AZStd::make_unique(); + terrainSystem->SetTerrainAabb(worldBounds); + terrainSystem->SetTerrainHeightQueryResolution(queryResolution); + terrainSystem->Activate(); AZ::TickBus::Broadcast(&AZ::TickBus::Events::OnTick, 0.f, AZ::ScriptTimePoint{}); + return terrainSystem; } AZStd::unique_ptr CreateAndActivateMockTerrainLayerSpawner( @@ -144,16 +143,16 @@ namespace UnitTest { // Trivially verify that the terrain system can successfully be constructed and destructed without errors. - m_terrainSystem = AZStd::make_unique(); + auto terrainSystem = AZStd::make_unique(); } TEST_F(TerrainSystemTest, TrivialActivateDeactivate) { // Verify that the terrain system can be activated and deactivated without errors. - m_terrainSystem = AZStd::make_unique(); - m_terrainSystem->Activate(); - m_terrainSystem->Deactivate(); + auto terrainSystem = AZStd::make_unique(); + terrainSystem->Activate(); + terrainSystem->Deactivate(); } TEST_F(TerrainSystemTest, CreateEventsCalledOnActivation) @@ -164,8 +163,8 @@ namespace UnitTest EXPECT_CALL(mockTerrainListener, OnTerrainDataCreateBegin()).Times(AtLeast(1)); EXPECT_CALL(mockTerrainListener, OnTerrainDataCreateEnd()).Times(AtLeast(1)); - m_terrainSystem = AZStd::make_unique(); - m_terrainSystem->Activate(); + auto terrainSystem = AZStd::make_unique(); + terrainSystem->Activate(); } TEST_F(TerrainSystemTest, DestroyEventsCalledOnDeactivation) @@ -176,9 +175,9 @@ namespace UnitTest EXPECT_CALL(mockTerrainListener, OnTerrainDataDestroyBegin()).Times(AtLeast(1)); EXPECT_CALL(mockTerrainListener, OnTerrainDataDestroyEnd()).Times(AtLeast(1)); - m_terrainSystem = AZStd::make_unique(); - m_terrainSystem->Activate(); - m_terrainSystem->Deactivate(); + auto terrainSystem = AZStd::make_unique(); + terrainSystem->Activate(); + terrainSystem->Deactivate(); } TEST_F(TerrainSystemTest, TerrainDoesNotExistWhenNoTerrainLayerSpawnersAreRegistered) @@ -190,9 +189,9 @@ namespace UnitTest // a normal facing up the Z axis. // Create and activate the terrain system with our testing defaults for world bounds and query resolution. - CreateAndActivateTerrainSystem(); + auto terrainSystem = CreateAndActivateTerrainSystem(); - AZ::Aabb worldBounds = m_terrainSystem->GetTerrainAabb(); + AZ::Aabb worldBounds = terrainSystem->GetTerrainAabb(); // Loop through several points within the world bounds, including on the edges, and verify that they all return false for // terrainExists with default heights and normals. @@ -203,17 +202,17 @@ namespace UnitTest AZ::Vector3 position(x, y, 0.0f); bool terrainExists = true; float height = - m_terrainSystem->GetHeight(position, AzFramework::Terrain::TerrainDataRequests::Sampler::EXACT, &terrainExists); + terrainSystem->GetHeight(position, AzFramework::Terrain::TerrainDataRequests::Sampler::EXACT, &terrainExists); EXPECT_FALSE(terrainExists); EXPECT_FLOAT_EQ(height, worldBounds.GetMin().GetZ()); terrainExists = true; AZ::Vector3 normal = - m_terrainSystem->GetNormal(position, AzFramework::Terrain::TerrainDataRequests::Sampler::EXACT, &terrainExists); + terrainSystem->GetNormal(position, AzFramework::Terrain::TerrainDataRequests::Sampler::EXACT, &terrainExists); EXPECT_FALSE(terrainExists); EXPECT_EQ(normal, AZ::Vector3::CreateAxisZ()); - bool isHole = m_terrainSystem->GetIsHoleFromFloats( + bool isHole = terrainSystem->GetIsHoleFromFloats( position.GetX(), position.GetY(), AzFramework::Terrain::TerrainDataRequests::Sampler::EXACT); EXPECT_TRUE(isHole); } @@ -242,7 +241,7 @@ namespace UnitTest // Verify that terrain exists within the layer spawner bounds, and doesn't exist outside of it. // Create and activate the terrain system with our testing defaults for world bounds and query resolution. - CreateAndActivateTerrainSystem(); + auto terrainSystem = CreateAndActivateTerrainSystem(); // Create a box that's twice as big as the layer spawner box. Loop through it and verify that points within the layer box contain // terrain and the expected height & normal values, and points outside the layer box don't contain terrain. @@ -255,9 +254,9 @@ namespace UnitTest { AZ::Vector3 position(x, y, 0.0f); bool heightQueryTerrainExists = false; - float height = m_terrainSystem->GetHeight( + float height = terrainSystem->GetHeight( position, AzFramework::Terrain::TerrainDataRequests::Sampler::EXACT, &heightQueryTerrainExists); - bool isHole = m_terrainSystem->GetIsHoleFromFloats( + bool isHole = terrainSystem->GetIsHoleFromFloats( position.GetX(), position.GetY(), AzFramework::Terrain::TerrainDataRequests::Sampler::EXACT); if (spawnerBox.Contains(AZ::Vector3(position.GetX(), position.GetY(), spawnerBox.GetMin().GetZ()))) @@ -297,7 +296,7 @@ namespace UnitTest // Create and activate the terrain system with our testing defaults for world bounds, and a query resolution that exactly matches // the frequency of our sine wave. If our height queries rely on the query resolution, we should always get a value of 0. const AZ::Vector2 queryResolution(frequencyMeters); - CreateAndActivateTerrainSystem(queryResolution); + auto terrainSystem = CreateAndActivateTerrainSystem(queryResolution); // Test an arbitrary set of points that should all produce non-zero heights with the EXACT sampler. They're not aligned with the // query resolution, or with the 0 points on the sine wave. @@ -307,7 +306,7 @@ namespace UnitTest AZ::Vector3 position(nonZeroPoint.GetX(), nonZeroPoint.GetY(), 0.0f); bool heightQueryTerrainExists = false; float height = - m_terrainSystem->GetHeight(position, AzFramework::Terrain::TerrainDataRequests::Sampler::EXACT, &heightQueryTerrainExists); + terrainSystem->GetHeight(position, AzFramework::Terrain::TerrainDataRequests::Sampler::EXACT, &heightQueryTerrainExists); // We've chosen a bunch of places on the sine wave that should return a non-zero positive or negative value. constexpr float epsilon = 0.0001f; @@ -322,7 +321,7 @@ namespace UnitTest AZ::Vector3 position(zeroPoint.GetX(), zeroPoint.GetY(), 0.0f); bool heightQueryTerrainExists = false; float height = - m_terrainSystem->GetHeight(position, AzFramework::Terrain::TerrainDataRequests::Sampler::EXACT, &heightQueryTerrainExists); + terrainSystem->GetHeight(position, AzFramework::Terrain::TerrainDataRequests::Sampler::EXACT, &heightQueryTerrainExists); constexpr float epsilon = 0.0001f; EXPECT_NEAR(height, 0.0f, epsilon); @@ -348,7 +347,7 @@ namespace UnitTest // Create and activate the terrain system with our testing defaults for world bounds, and a query resolution at 0.25 meter // intervals. const AZ::Vector2 queryResolution(0.25f); - CreateAndActivateTerrainSystem(queryResolution); + auto terrainSystem = CreateAndActivateTerrainSystem(queryResolution); // Test some points and verify that the results always go "downward", whether they're in positive or negative space. // (Z contains the the expected result for convenience). @@ -371,7 +370,7 @@ namespace UnitTest AZ::Vector3 position(testPoint.m_testLocation.GetX(), testPoint.m_testLocation.GetY(), 0.0f); bool heightQueryTerrainExists = false; float height = - m_terrainSystem->GetHeight(position, AzFramework::Terrain::TerrainDataRequests::Sampler::CLAMP, &heightQueryTerrainExists); + terrainSystem->GetHeight(position, AzFramework::Terrain::TerrainDataRequests::Sampler::CLAMP, &heightQueryTerrainExists); constexpr float epsilon = 0.0001f; EXPECT_NEAR(height, expectedHeight, epsilon); @@ -410,7 +409,7 @@ namespace UnitTest // Create and activate the terrain system with our testing defaults for world bounds, and a query resolution at 1 meter intervals. const AZ::Vector2 queryResolution(frequencyMeters); - CreateAndActivateTerrainSystem(queryResolution); + auto terrainSystem = CreateAndActivateTerrainSystem(queryResolution); // Test some points and verify that the results are the expected bilinear filtered result, // whether they're in positive or negative space. @@ -466,7 +465,7 @@ namespace UnitTest AZ::Vector3 position(testPoint.m_testLocation.GetX(), testPoint.m_testLocation.GetY(), 0.0f); bool heightQueryTerrainExists = false; - float height = m_terrainSystem->GetHeight( + float height = terrainSystem->GetHeight( position, AzFramework::Terrain::TerrainDataRequests::Sampler::BILINEAR, &heightQueryTerrainExists); // Verify that our height query returned the bilinear filtered result we expect. @@ -479,7 +478,7 @@ namespace UnitTest { // When there is more than one surface/weight defined, they should all be returned in descending weight order. - CreateAndActivateTerrainSystem(); + auto terrainSystem = CreateAndActivateTerrainSystem(); const AZ::Aabb aabb = AZ::Aabb::CreateFromMinMax(AZ::Vector3::CreateZero(), AZ::Vector3::CreateOne()); auto entity = CreateAndActivateMockTerrainLayerSpawner( @@ -508,11 +507,11 @@ namespace UnitTest AzFramework::SurfaceData::SurfaceTagWeightList outSurfaceWeights; // Asking for values outside the layer spawner bounds, should result in no results. - m_terrainSystem->GetSurfaceWeights(aabb.GetMax() + AZ::Vector3::CreateOne(), outSurfaceWeights); + terrainSystem->GetSurfaceWeights(aabb.GetMax() + AZ::Vector3::CreateOne(), outSurfaceWeights); EXPECT_TRUE(outSurfaceWeights.empty()); // Inside the layer spawner box should give us all of the added surface weights. - m_terrainSystem->GetSurfaceWeights(aabb.GetCenter(), outSurfaceWeights); + terrainSystem->GetSurfaceWeights(aabb.GetCenter(), outSurfaceWeights); EXPECT_EQ(outSurfaceWeights.size(), 3); @@ -531,7 +530,7 @@ namespace UnitTest TEST_F(TerrainSystemTest, GetMaxSurfaceWeightsReturnsBiggestValidSurfaceWeight) { - CreateAndActivateTerrainSystem(); + auto terrainSystem = CreateAndActivateTerrainSystem(); const AZ::Aabb aabb = AZ::Aabb::CreateFromMinMax(AZ::Vector3::CreateZero(), AZ::Vector3::CreateOne()); auto entity = CreateAndActivateMockTerrainLayerSpawner( @@ -562,12 +561,12 @@ namespace UnitTest // Asking for values outside the layer spawner bounds, should result in an invalid result. AzFramework::SurfaceData::SurfaceTagWeight tagWeight = - m_terrainSystem->GetMaxSurfaceWeight(aabb.GetMax() + AZ::Vector3::CreateOne()); + terrainSystem->GetMaxSurfaceWeight(aabb.GetMax() + AZ::Vector3::CreateOne()); EXPECT_EQ(tagWeight.m_surfaceType, AZ::Crc32(AzFramework::SurfaceData::Constants::s_unassignedTagName)); // Inside the layer spawner box should give us the highest weighted tag (tag1). - tagWeight = m_terrainSystem->GetMaxSurfaceWeight(aabb.GetCenter()); + tagWeight = terrainSystem->GetMaxSurfaceWeight(aabb.GetCenter()); EXPECT_EQ(tagWeight.m_surfaceType, tagWeight1.m_surfaceType); EXPECT_NEAR(tagWeight.m_weight, tagWeight1.m_weight, 0.01f);