PR feedback + refactored some code duplication in tests

Signed-off-by: Sergey Pereslavtsev <pereslav@amazon.com>

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

@@ -84,7 +84,7 @@ namespace Terrain
                     ->Attribute(AZ::Edit::Attributes::Visibility, AZ::Edit::PropertyVisibility::ShowChildrenOnly)
                     ->Attribute(AZ::Edit::Attributes::AutoExpand, true)
                     ->DataElement(AZ::Edit::UIHandlers::Default, &TerrainPhysicsColliderConfig::m_defaultMaterialSelection,
-                        "Default Surface Physics Material", "Select a material to be used by maps surfaces by default")
+                        "Default Surface Physics Material", "Select a material to be used by unmapped surfaces by default")
                     ->DataElement(
                         AZ::Edit::UIHandlers::Default, &TerrainPhysicsColliderConfig::m_surfaceMaterialMappings,
                         "Surface to Material Mappings", "Maps surfaces to physics materials")

Gems/Terrain/Code/Tests/TerrainPhysicsColliderTests.cpp

@@ -46,10 +46,17 @@ protected:
         appDesc.m_stackRecordLevels = 20;
 
         m_app.Create(appDesc);
+
+        CreateEntity();
+
+        m_boxComponent = m_entity->CreateComponent<UnitTest::MockAxisAlignedBoxShapeComponent>();
+        m_app.RegisterComponentDescriptor(m_boxComponent->CreateDescriptor());
     }
 
     void TearDown() override
     {
+        m_entity.reset();
+
         m_app.Destroy();
     }
 
@@ -61,12 +68,9 @@ protected:
         m_entity->Init();
     }
 
-    void AddTerrainPhysicsColliderAndShapeComponentToEntity()
+    void AddTerrainPhysicsColliderToEntity(const Terrain::TerrainPhysicsColliderConfig& configuration)
     {
-        m_boxComponent = m_entity->CreateComponent<UnitTest::MockAxisAlignedBoxShapeComponent>();
-        m_app.RegisterComponentDescriptor(m_boxComponent->CreateDescriptor());
-
-        m_colliderComponent = m_entity->CreateComponent<Terrain::TerrainPhysicsColliderComponent>(Terrain::TerrainPhysicsColliderConfig());
+        m_colliderComponent = m_entity->CreateComponent<Terrain::TerrainPhysicsColliderComponent>(configuration);
         m_app.RegisterComponentDescriptor(m_colliderComponent->CreateDescriptor());
     }
 
@@ -113,21 +117,17 @@ protected:
 TEST_F(TerrainPhysicsColliderComponentTest, ActivateEntityActivateSuccess)
 {
     // Check that the entity activates with a collider and the required shape attached.
-    CreateEntity();
-    AddTerrainPhysicsColliderAndShapeComponentToEntity();
+    AddTerrainPhysicsColliderToEntity(Terrain::TerrainPhysicsColliderConfig());
 
     m_entity->Activate();
     EXPECT_EQ(m_entity->GetState(), AZ::Entity::State::Active);
      
-    m_entity.reset();
 }
 
 TEST_F(TerrainPhysicsColliderComponentTest, TerrainPhysicsColliderTransformChangedNotifiesHeightfieldBus)
 {
     // Check that the HeightfieldBus is notified when the transform of the entity changes.
-    CreateEntity();
-
-    AddTerrainPhysicsColliderAndShapeComponentToEntity();
+    AddTerrainPhysicsColliderToEntity(Terrain::TerrainPhysicsColliderConfig());
 
     m_entity->Activate();
 
@@ -138,16 +138,12 @@ TEST_F(TerrainPhysicsColliderComponentTest, TerrainPhysicsColliderTransformChang
     LmbrCentral::ShapeComponentNotificationsBus::Event(
         m_entity->GetId(), &LmbrCentral::ShapeComponentNotificationsBus::Events::OnShapeChanged,
         LmbrCentral::ShapeComponentNotifications::ShapeChangeReasons::TransformChanged);
-
-    m_entity.reset();
 }
 
 TEST_F(TerrainPhysicsColliderComponentTest, TerrainPhysicsColliderShapeChangedNotifiesHeightfieldBus)
 {
     // Check that the Heightfield bus is notified when the shape component changes.
-    CreateEntity();
-
-    AddTerrainPhysicsColliderAndShapeComponentToEntity();
+    AddTerrainPhysicsColliderToEntity(Terrain::TerrainPhysicsColliderConfig());
 
     m_entity->Activate();
 
@@ -157,16 +153,12 @@ TEST_F(TerrainPhysicsColliderComponentTest, TerrainPhysicsColliderShapeChangedNo
     LmbrCentral::ShapeComponentNotificationsBus::Event(
         m_entity->GetId(), &LmbrCentral::ShapeComponentNotificationsBus::Events::OnShapeChanged,
         LmbrCentral::ShapeComponentNotifications::ShapeChangeReasons::ShapeChanged);
-
-    m_entity.reset();
 }
 
 TEST_F(TerrainPhysicsColliderComponentTest, TerrainPhysicsColliderReturnsAlignedRowBoundsCorrectly)
 {
     // Check that the heightfield grid size is correct when the shape bounds match the grid resolution.
-    CreateEntity();
-
-    AddTerrainPhysicsColliderAndShapeComponentToEntity();
+    AddTerrainPhysicsColliderToEntity(Terrain::TerrainPhysicsColliderConfig());
 
     m_entity->Activate();
 
@@ -188,17 +180,13 @@ TEST_F(TerrainPhysicsColliderComponentTest, TerrainPhysicsColliderReturnsAligned
     // With the bounds set at 0-1024 and a resolution of 1.0, the heightfield grid should be 1024x1024.
     EXPECT_EQ(cols, 1024);
     EXPECT_EQ(rows, 1024);
-
-    m_entity.reset();
 }
 
 TEST_F(TerrainPhysicsColliderComponentTest, TerrainPhysicsColliderExpandsMinBoundsCorrectly)
 {
     // Check that the heightfield grid is correctly expanded if the minimum value of the bounds needs expanding
     // to correctly encompass it.
-    CreateEntity();
-
-    AddTerrainPhysicsColliderAndShapeComponentToEntity();
+    AddTerrainPhysicsColliderToEntity(Terrain::TerrainPhysicsColliderConfig());
 
     m_entity->Activate();
 
@@ -221,17 +209,13 @@ TEST_F(TerrainPhysicsColliderComponentTest, TerrainPhysicsColliderExpandsMinBoun
     // the values returned would be 1023.
     EXPECT_EQ(cols, 1024);
     EXPECT_EQ(rows, 1024);
-
-    m_entity.reset();
 }
 
 TEST_F(TerrainPhysicsColliderComponentTest, TerrainPhysicsColliderExpandsMaxBoundsCorrectly)
 {
     // Check that the heightfield grid is correctly expanded if the maximum value of the bounds needs expanding
     // to correctly encompass it.
-    CreateEntity();
-
-    AddTerrainPhysicsColliderAndShapeComponentToEntity();
+    AddTerrainPhysicsColliderToEntity(Terrain::TerrainPhysicsColliderConfig());
 
     m_entity->Activate();
 
@@ -254,16 +238,12 @@ TEST_F(TerrainPhysicsColliderComponentTest, TerrainPhysicsColliderExpandsMaxBoun
     // the values returned would be 1023.
     EXPECT_EQ(cols, 1024);
     EXPECT_EQ(rows, 1024);
-
-    m_entity.reset();
 }
 
 TEST_F(TerrainPhysicsColliderComponentTest, TerrainPhysicsColliderGetHeightsReturnsHeights)
 {
     // Check that the TerrainPhysicsCollider returns a heightfield of the expected size.
-    CreateEntity();
-
-    AddTerrainPhysicsColliderAndShapeComponentToEntity();
+    AddTerrainPhysicsColliderToEntity(Terrain::TerrainPhysicsColliderConfig());
 
     m_entity->Activate();
 
@@ -298,16 +278,12 @@ TEST_F(TerrainPhysicsColliderComponentTest, TerrainPhysicsColliderGetHeightsRetu
     EXPECT_EQ(cols, 1024);
     EXPECT_EQ(rows, 1024);
     EXPECT_EQ(heights.size(), cols * rows);
-   
-    m_entity.reset();
 }
 
 TEST_F(TerrainPhysicsColliderComponentTest, TerrainPhysicsColliderReturnsRelativeHeightsCorrectly)
 {
     // Check that the values stored in the heightfield returned by the TerrainPhysicsCollider are correct.
-    CreateEntity();
-
-    AddTerrainPhysicsColliderAndShapeComponentToEntity();
+    AddTerrainPhysicsColliderToEntity(Terrain::TerrainPhysicsColliderConfig());
 
     m_entity->Activate();
 
@@ -341,18 +317,11 @@ TEST_F(TerrainPhysicsColliderComponentTest, TerrainPhysicsColliderReturnsRelativ
 
     const float expectedHeightValue = 16384.0f;
     EXPECT_NEAR(heights[0], expectedHeightValue, 0.01f);
-
-    m_entity->Reset();
 }
 
 TEST_F(TerrainPhysicsColliderComponentTest, TerrainPhysicsColliderReturnsMaterials)
 {
     // Check that the TerrainPhysicsCollider returns all the assigned materials.
-    CreateEntity();
-
-    m_boxComponent = m_entity->CreateComponent<UnitTest::MockAxisAlignedBoxShapeComponent>();
-    m_app.RegisterComponentDescriptor(m_boxComponent->CreateDescriptor());
-
     // Create two SurfaceTag/Material mappings and add them to the collider.
     Terrain::TerrainPhysicsColliderConfig config;
 
@@ -372,8 +341,7 @@ TEST_F(TerrainPhysicsColliderComponentTest, TerrainPhysicsColliderReturnsMateria
     mapping2.m_surfaceTag = tag2;
     config.m_surfaceMaterialMappings.emplace_back(mapping2);
 
-    m_colliderComponent = m_entity->CreateComponent<Terrain::TerrainPhysicsColliderComponent>(config);
-    m_app.RegisterComponentDescriptor(m_colliderComponent->CreateDescriptor());
+    AddTerrainPhysicsColliderToEntity(config);
 
     m_entity->Activate();
 
@@ -388,20 +356,12 @@ TEST_F(TerrainPhysicsColliderComponentTest, TerrainPhysicsColliderReturnsMateria
     EXPECT_EQ(materialList[0], defaultMaterial);
     EXPECT_EQ(materialList[1], mat1);
     EXPECT_EQ(materialList[2], mat2);
-
-    m_entity.reset();
 }
 
 TEST_F(TerrainPhysicsColliderComponentTest, TerrainPhysicsColliderReturnsMaterialsWhenNotMapped)
 {
     // Check that the TerrainPhysicsCollider returns a default material when no surfaces are mapped.
-    CreateEntity();
-
-    m_boxComponent = m_entity->CreateComponent<UnitTest::MockAxisAlignedBoxShapeComponent>();
-    m_app.RegisterComponentDescriptor(m_boxComponent->CreateDescriptor());
-
-    m_colliderComponent = m_entity->CreateComponent<Terrain::TerrainPhysicsColliderComponent>();
-    m_app.RegisterComponentDescriptor(m_colliderComponent->CreateDescriptor());
+    AddTerrainPhysicsColliderToEntity(Terrain::TerrainPhysicsColliderConfig());
 
     m_entity->Activate();
 
@@ -414,18 +374,11 @@ TEST_F(TerrainPhysicsColliderComponentTest, TerrainPhysicsColliderReturnsMateria
 
     Physics::MaterialId defaultMaterial = Physics::MaterialId();
     EXPECT_EQ(materialList[0], defaultMaterial);
-
-    m_entity.reset();
 }
 
 TEST_F(TerrainPhysicsColliderComponentTest, TerrainPhysicsColliderGetHeightsAndMaterialsReturnsCorrectly)
 {
     // Check that the TerrainPhysicsCollider returns a heightfield of the expected size.
-    CreateEntity();
-
-    m_boxComponent = m_entity->CreateComponent<UnitTest::MockAxisAlignedBoxShapeComponent>();
-    m_app.RegisterComponentDescriptor(m_boxComponent->CreateDescriptor());
-
     // Create two SurfaceTag/Material mappings and add them to the collider.
     Terrain::TerrainPhysicsColliderConfig config;
 
@@ -445,8 +398,7 @@ TEST_F(TerrainPhysicsColliderComponentTest, TerrainPhysicsColliderGetHeightsAndM
     mapping2.m_surfaceTag = tag2;
     config.m_surfaceMaterialMappings.emplace_back(mapping2);
 
-    m_colliderComponent = m_entity->CreateComponent<Terrain::TerrainPhysicsColliderComponent>(config);
-    m_app.RegisterComponentDescriptor(m_colliderComponent->CreateDescriptor());
+    AddTerrainPhysicsColliderToEntity(config);
 
     m_entity->Activate();
 
@@ -460,15 +412,10 @@ TEST_F(TerrainPhysicsColliderComponentTest, TerrainPhysicsColliderGetHeightsAndM
     const float mockHeight = 32768.0f;
     AZ::Vector2 mockHeightResolution = AZ::Vector2(1.0f);
 
-    AzFramework::SurfaceData::SurfaceTagWeight return1;
-    return1.m_surfaceType = tag1;
-    return1.m_weight = 1.0f;
+    AzFramework::SurfaceData::SurfaceTagWeight tagWeight1(tag1, 1.0f);
+    AzFramework::SurfaceData::SurfaceTagWeight tagWeight2(tag2, 1.0f);
 
-    AzFramework::SurfaceData::SurfaceTagWeight return2;
-    return2.m_surfaceType = tag2;
-    return2.m_weight = 1.0f;
-
-    AzFramework::SurfaceData::SurfaceTagWeightList surfaceTags = { return1, return2 };
+    AzFramework::SurfaceData::SurfaceTagWeightList surfaceTags = { tagWeight1, tagWeight2 };
 
     NiceMock<UnitTest::MockTerrainDataRequests> terrainListener;
     ON_CALL(terrainListener, GetTerrainHeightQueryResolution).WillByDefault(Return(mockHeightResolution));
@@ -499,17 +446,10 @@ TEST_F(TerrainPhysicsColliderComponentTest, TerrainPhysicsColliderGetHeightsAndM
     // Check an entry from the second half of the list
     EXPECT_EQ(heightsAndMaterials[256 * 128].m_materialIndex, 2);
     EXPECT_NEAR(heightsAndMaterials[256 * 128].m_height, expectedHeightValue, 0.01f);
-
-    m_entity.reset();
 }
 
 TEST_F(TerrainPhysicsColliderComponentTest, TerrainPhysicsColliderDefaultMaterialAssignedWhenTagHasNoMapping)
 {
-    CreateEntity();
-
-    m_boxComponent = m_entity->CreateComponent<UnitTest::MockAxisAlignedBoxShapeComponent>();
-    m_app.RegisterComponentDescriptor(m_boxComponent->CreateDescriptor());
-
     // Create two SurfaceTag/Material mappings and add them to the collider.
     Terrain::TerrainPhysicsColliderConfig config;
 
@@ -526,9 +466,7 @@ TEST_F(TerrainPhysicsColliderComponentTest, TerrainPhysicsColliderDefaultMateria
     config.m_defaultMaterialSelection.SetMaterialId(defaultSurfaceMaterial);
 
     // Intentionally don't set the mapping for "tag2". It's expected the default material will substitute.
-
-    m_colliderComponent = m_entity->CreateComponent<Terrain::TerrainPhysicsColliderComponent>(config);
-    m_app.RegisterComponentDescriptor(m_colliderComponent->CreateDescriptor());
+    AddTerrainPhysicsColliderToEntity(config);
 
     m_entity->Activate();
 
@@ -554,15 +492,10 @@ TEST_F(TerrainPhysicsColliderComponentTest, TerrainPhysicsColliderDefaultMateria
     const float mockHeight = 32768.0f;
     AZ::Vector2 mockHeightResolution = AZ::Vector2(1.0f);
 
-    AzFramework::SurfaceData::SurfaceTagWeight return1;
-    return1.m_surfaceType = tag1;
-    return1.m_weight = 1.0f;
+    AzFramework::SurfaceData::SurfaceTagWeight tagWeight1(tag1, 1.0f);
+    AzFramework::SurfaceData::SurfaceTagWeight tagWeight2(tag2, 1.0f);
 
-    AzFramework::SurfaceData::SurfaceTagWeight return2;
-    return2.m_surfaceType = tag2;
-    return2.m_weight = 1.0f;
-
-    AzFramework::SurfaceData::SurfaceTagWeightList surfaceTags = { return1, return2 };
+    AzFramework::SurfaceData::SurfaceTagWeightList surfaceTags = { tagWeight1, tagWeight2 };
 
     NiceMock<UnitTest::MockTerrainDataRequests> terrainListener;
     ON_CALL(terrainListener, GetTerrainHeightQueryResolution).WillByDefault(Return(mockHeightResolution));
@@ -591,6 +524,70 @@ TEST_F(TerrainPhysicsColliderComponentTest, TerrainPhysicsColliderDefaultMateria
         // This should point to the default material (0) since we don't have a mapping for "tag2"
         EXPECT_EQ(heightsAndMaterials[256 * 128].m_materialIndex, 0);
     }
-
-    m_entity.reset();
+}
+
+TEST_F(TerrainPhysicsColliderComponentTest, TerrainPhysicsColliderDefaultMaterialAssignedWhenNoMappingsExist)
+{
+    // Create only the default material with no mapping for the tags. It's expected the default material will be assigned to both tags.
+    Terrain::TerrainPhysicsColliderConfig config;    
+    const Physics::MaterialId defaultSurfaceMaterial = Physics::MaterialId::Create();
+    config.m_defaultMaterialSelection.SetMaterialId(defaultSurfaceMaterial);
+    AddTerrainPhysicsColliderToEntity(config);
+
+    m_entity->Activate();
+
+    // Validate material list is generated with the default material
+    {
+        AZStd::vector<Physics::MaterialId> materialList;
+        Physics::HeightfieldProviderRequestsBus::EventResult(
+            materialList, m_entity->GetId(), &Physics::HeightfieldProviderRequestsBus::Events::GetMaterialList);
+
+        EXPECT_EQ(materialList.size(), 1);
+        EXPECT_EQ(materialList[0], defaultSurfaceMaterial);
+    }
+
+    const AZ::Vector3 boundsMin = AZ::Vector3(0.0f);
+    const AZ::Vector3 boundsMax = AZ::Vector3(256.0f, 256.0f, 32768.0f);
+
+    NiceMock<UnitTest::MockShapeComponentRequests> boxShape(m_entity->GetId());
+    const AZ::Aabb bounds = AZ::Aabb::CreateFromMinMax(boundsMin, boundsMax);
+    ON_CALL(boxShape, GetEncompassingAabb).WillByDefault(Return(bounds));
+
+    const float mockHeight = 32768.0f;
+    AZ::Vector2 mockHeightResolution = AZ::Vector2(1.0f);
+
+    const SurfaceData::SurfaceTag tag1 = SurfaceData::SurfaceTag("tag1");
+    AzFramework::SurfaceData::SurfaceTagWeight tagWeight1(tag1, 1.0f);
+
+    const SurfaceData::SurfaceTag tag2 = SurfaceData::SurfaceTag("tag2");
+    AzFramework::SurfaceData::SurfaceTagWeight tagWeight2(tag2, 1.0f);
+
+    AzFramework::SurfaceData::SurfaceTagWeightList surfaceTags = { tagWeight1, tagWeight2 };
+
+    NiceMock<UnitTest::MockTerrainDataRequests> terrainListener;
+    ON_CALL(terrainListener, GetTerrainHeightQueryResolution).WillByDefault(Return(mockHeightResolution));
+    ON_CALL(terrainListener, ProcessSurfacePointsFromRegion).WillByDefault(
+        [this, mockHeight, &surfaceTags](const AZ::Aabb& inRegion, const AZ::Vector2& stepSize,
+            AzFramework::Terrain::SurfacePointRegionFillCallback perPositionCallback,
+            [[maybe_unused]] AzFramework::Terrain::TerrainDataRequests::Sampler sampleFilter)
+    {
+        ProcessRegionLoop(inRegion, stepSize, perPositionCallback, &surfaceTags, mockHeight);
+    }
+    );
+
+    // Validate material indices 
+    {
+        AZStd::vector<Physics::HeightMaterialPoint> heightsAndMaterials;
+        Physics::HeightfieldProviderRequestsBus::EventResult(
+            heightsAndMaterials, m_entity->GetId(), &Physics::HeightfieldProviderRequestsBus::Events::GetHeightsAndMaterials);
+
+        // We set the bounds to 256, so check that the correct number of entries are present.
+        EXPECT_EQ(heightsAndMaterials.size(), 256 * 256);
+
+        // Check an entry from the first half of the returned list. Should be the default material index 0.
+        EXPECT_EQ(heightsAndMaterials[0].m_materialIndex, 0);
+
+        // Check an entry from the second half of the list. Should be the default material index 0.
+        EXPECT_EQ(heightsAndMaterials[256 * 128].m_materialIndex, 0);
+    }
 }