Merge branch 'development' of https://github.com/o3de/o3de into daimini/dpiIssues/floatingWidgetFlickerBetweenScreens

AutomatedTesting/Gem/PythonTests/atom_renderer/CMakeLists.txt

@@ -46,4 +46,15 @@ if(PAL_TRAIT_BUILD_HOST_TOOLS AND PAL_TRAIT_BUILD_TESTS_SUPPORTED AND AutomatedT
             AutomatedTesting.Assets
             Editor
     )
+    ly_add_pytest(
+        NAME AutomatedTesting::AtomRenderer_HydraTests_ShaderBuildPipeline
+        TEST_SUITE main
+        PATH ${CMAKE_CURRENT_LIST_DIR}/test_Atom_ShaderBuildPipelineSuite.py
+        TEST_SERIAL
+        TIMEOUT 600
+        RUNTIME_DEPENDENCIES
+            AssetProcessor
+            AutomatedTesting.Assets
+            Editor
+    )
 endif()

AutomatedTesting/Gem/PythonTests/atom_renderer/atom_hydra_scripts/TestAssets/ShaderAssetBuilder/DependencyValidation.azsl.txt

@@ -0,0 +1,55 @@
+/*
+ * Copyright (c) Contributors to the Open 3D Engine Project.
+ * For complete copyright and license terms please see the LICENSE at the root of this distribution.
+ *
+ * SPDX-License-Identifier: Apache-2.0 OR MIT
+ *
+ */
+
+ /*
+    This is a dummy shader used to validate detection of "#included files"
+ */
+
+#include <Atom/Features/SrgSemantics.azsli>
+
+#include "Test1Color.azsli"
+#include <Test3Color.azsli>
+
+ShaderResourceGroup DummySrg : SRG_PerDraw
+{
+    float4 m_color;
+}
+
+struct VSInput
+{
+    float3 m_position : POSITION;
+    float4 m_color : COLOR0;
+};
+
+struct VSOutput
+{
+    float4 m_position : SV_Position;
+    float4 m_color : COLOR0;
+};
+
+VSOutput MainVS(VSInput vsInput)
+{
+    VSOutput OUT;
+    OUT.m_position = float4(vsInput.m_position, 1.0);
+    OUT.m_color = vsInput.m_color;
+    return OUT;
+}
+
+struct PSOutput
+{
+    float4 m_color : SV_Target0;
+};
+
+PSOutput MainPS(VSOutput vsOutput)
+{
+    PSOutput OUT;
+
+    OUT.m_color = GetTest1Color(DummySrg::m_color) + GetTest3Color(DummySrg::m_color);
+
+    return OUT;
+}

AutomatedTesting/Gem/PythonTests/atom_renderer/atom_hydra_scripts/TestAssets/ShaderAssetBuilder/DependencyValidation.shader.txt

@@ -0,0 +1,26 @@
+// This is a dummy shader used to validate detection of "#included files"
+{
+    "Source" : "DependencyValidation.azsl",
+
+    "DepthStencilState" : { 
+        "Depth" : { "Enable" : false, "CompareFunc" : "GreaterEqual" }
+    },
+
+    "DrawList" : "forward",
+
+    "ProgramSettings":
+    {
+      "EntryPoints":
+      [
+        {
+          "name": "MainVS",
+          "type": "Vertex"
+        },
+        {
+          "name": "MainPS",
+          "type": "Fragment"
+        }
+      ]
+    }
+    
+}

AutomatedTesting/Gem/PythonTests/atom_renderer/atom_hydra_scripts/TestAssets/ShaderAssetBuilder/Test1Color.azsli.txt

@@ -0,0 +1,18 @@
+/*
+ * Copyright (c) Contributors to the Open 3D Engine Project.
+ * For complete copyright and license terms please see the LICENSE at the root of this distribution.
+ *
+ * SPDX-License-Identifier: Apache-2.0 OR MIT
+ *
+ */
+
+ /*
+    This is a dummy shader used to validate detection of "#included files"
+ */
+
+#include "Test2Color.azsli"
+
+float4 GetTest1Color(float4 color)
+{
+    return color + GetTest2Color(color);
+}

AutomatedTesting/Gem/PythonTests/atom_renderer/atom_hydra_scripts/TestAssets/ShaderAssetBuilder/Test2Color.azsli.txt

@@ -0,0 +1,16 @@
+/*
+ * Copyright (c) Contributors to the Open 3D Engine Project.
+ * For complete copyright and license terms please see the LICENSE at the root of this distribution.
+ *
+ * SPDX-License-Identifier: Apache-2.0 OR MIT
+ *
+ */
+
+ /*
+    This is a dummy shader used to validate detection of "#included files"
+ */
+
+float4 GetTest2Color(float4 color)
+{
+    return color * 0.5; 
+}

AutomatedTesting/Gem/PythonTests/atom_renderer/atom_hydra_scripts/TestAssets/ShaderAssetBuilder/Test3Color.azsli.txt

@@ -0,0 +1,16 @@
+/*
+ * Copyright (c) Contributors to the Open 3D Engine Project.
+ * For complete copyright and license terms please see the LICENSE at the root of this distribution.
+ *
+ * SPDX-License-Identifier: Apache-2.0 OR MIT
+ *
+ */
+
+ /*
+    This is a dummy shader used to validate detection of "#included files"
+ */
+
+float4 GetTest3Color(float4 color)
+{
+    return color * 0.13; 
+}

AutomatedTesting/Gem/PythonTests/atom_renderer/atom_hydra_scripts/hydra_ShaderAssetBuilder_RecompilesShaderAsChainOfDependenciesChanges.py

@@ -0,0 +1,188 @@
+"""
+Copyright (c) Contributors to the Open 3D Engine Project.
+For complete copyright and license terms please see the LICENSE at the root of this distribution.
+
+SPDX-License-Identifier: Apache-2.0 OR MIT
+
+"""
+
+import os
+import shutil
+
+def _copy_file(src_file, src_path, target_file, target_path):
+    # type: (str, str, str, str) -> None
+    """
+    Copies the [src_file] located in [src_path] to the [target_file] located at [target_path].
+    Leaves the [target_file] unlocked for reading and writing privileges
+    :param src_file: The source file to copy (file name)
+    :param src_path: The source file's path
+    :param target_file: The target file to copy into (file name)
+    :param target_path: The target file's path
+    :return: None
+    """
+    target_file_path = os.path.join(target_path, target_file)
+    src_file_path = os.path.join(src_path, src_file)
+    if os.path.exists(target_file_path):
+        fs.unlock_file(target_file_path)
+    shutil.copyfile(src_file_path, target_file_path)
+
+def _copy_tmp_files_in_order(src_directory, file_list, dst_directory, wait_time_in_between = 0.0):
+    # type: (str, list, str, float) -> None
+    """
+    This function assumes that for each file name listed in @file_list
+    there's file named "@filename.txt" which the original source file
+    but they will be copied with just the @filename (.txt removed).
+    """
+    for filename in file_list:
+        src_name = f"{filename}.txt"
+        _copy_file(src_name, src_directory, filename, dst_directory)
+        if wait_time_in_between > 0.0: 
+            print(f"Created {filename} in {dst_directory}")
+            general.idle_wait(wait_time_in_between)
+
+
+def _remove_file(src_file, src_path):
+    # type: (str, str) -> None
+    """
+    Removes the [src_file] located in [src_path].
+    :param src_file: The source file to copy (file name)
+    :param src_path: The source file's path
+    :return: None
+    """
+    src_file_path = os.path.join(src_path, src_file)
+    if os.path.exists(src_file_path):
+        fs.unlock_file(src_file_path)
+        os.remove(src_file_path)
+
+
+def _remove_files(directory, file_list):
+    for filename in file_list:
+        _remove_file(filename, directory)
+
+
+def _asset_exists(cache_relative_path):
+    asset_id = azasset.AssetCatalogRequestBus(azbus.Broadcast, "GetAssetIdByPath", cache_relative_path, azmath.Uuid(), False)
+    return asset_id.is_valid()
+
+# List of results that we want to check, this is not 100% necessary but it's a good
+# practice to make it easier to debug tests.
+# Here we define a tuple of tests
+class Results():
+    azshader_was_removed          = ("azshader was removed",        "Failed to remove azshader")
+    azshader_was_compiled         = ("azshader was compiled",       "Failed to compile azshader")
+
+
+def ShaderAssetBuilder_RecompilesShaderAsChainOfDependenciesChanges():
+    """
+    This test validates [ATOM-5441] Shader Builders May Fail When Multiple New Files Are Added
+    It creates source assets to compile a particular shader.
+    1- The first phase generates the source assets out of order and slowly. The AP should
+       wakeup each time one of the source dependencies appears but will fail each time. Only when the
+       last dependency appears then the shader should build successfully. 
+    2- The second phase is similar as above, except that all source assets will be created
+       at once and We also expect that in the end the shader is built successfully.
+    """
+    # Required for automated tests
+    helper.init_idle()
+
+    game_root_path = os.path.normpath(general.get_game_folder())
+    game_asset_path = os.path.join(game_root_path, "Assets")
+
+    base_dir = os.path.dirname(__file__)
+    src_assets_subdir = os.path.join(base_dir, "TestAssets", "ShaderAssetBuilder") 
+
+    with Tracer() as error_tracer:
+        # The script drives the execution of the test, to return the flow back to the editor,
+        # we will tick it one time
+        general.idle_wait_frames(1)
+ 
+        # This is the order in which the source assets should be deployed
+        # to avoid source dependency issues with the old MCPP-based CreateJobs. 
+        file_list = [
+            "Test2Color.azsli",
+            "Test3Color.azsli",
+            "Test1Color.azsli",
+            "DependencyValidation.azsl",
+            "DependencyValidation.shader"
+        ]
+
+        reverse_file_list = file_list[::-1]
+
+        # Remove files in reverse order
+        _remove_files(game_asset_path, reverse_file_list)
+
+        # Wait here until the azshader doesn't exist anymore.
+        azshader_name = "assets/dependencyvalidation.azshader"
+        helper.wait_for_condition(lambda: not _asset_exists(azshader_name), 5.0)
+
+        Report.critical_result(Results.azshader_was_removed, not _asset_exists(azshader_name))
+
+        _copy_tmp_files_in_order(src_assets_subdir, file_list, game_asset_path, 1.0)
+
+        # Give enough time to AP to compile the shader
+        helper.wait_for_condition(lambda: _asset_exists(azshader_name), 60.0)
+
+        Report.critical_result(Results.azshader_was_compiled, _asset_exists(azshader_name))
+
+        # The first part was about compiling the shader under normal conditions.
+        # Let's remove the files from the previous phase and will proceed
+        # to make the source files visible to the AP in reverse order. The
+        # ShaderAssetBuilder will only succeed when the last file becomes visible.
+        _remove_files(game_asset_path, reverse_file_list)
+        helper.wait_for_condition(lambda: not _asset_exists(azshader_name), 5.0)
+        Report.critical_result(Results.azshader_was_removed, not _asset_exists(azshader_name))
+
+        # Remark, if you are running this test manually from the Editor with "pyRunFile",
+        # You'll notice how the AP issues notifications that it fails to compile the shader
+        # as the source files are being copied to the "Assets" subfolder.
+        # Those errors are OK and also expected because We need the AP to wake up as each
+        # reported source dependency exists. Once the last file is copied then all source
+        # dependencies are fully satisfied and the shader should compile successfully.
+        # And this summarizes the importance of this Test: The previous version
+        # of ShaderAssetBuilder::CreateJobs was incapable of compiling the shader under the conditions
+        # presented in this test, but with the new version of ShaderAssetBuilder::CreateJobs, which
+        # doesn't use MCPP for #include files discovery, it should eventually compile the shader
+        # once all the source files are in place.
+        _copy_tmp_files_in_order(src_assets_subdir, reverse_file_list, game_asset_path, 3.0)
+
+        # Give enough time to AP to compile the shader
+        helper.wait_for_condition(lambda: _asset_exists(azshader_name), 60.0)
+
+        Report.critical_result(Results.azshader_was_compiled, _asset_exists(azshader_name))
+
+        # The last phase of the test puts stress on potential race conditions
+        # when all required files appear as soon as possible.
+
+        # First Clean up.
+        # Remove left over files.
+        _remove_files(game_asset_path, reverse_file_list)
+        helper.wait_for_condition(lambda: not _asset_exists(azshader_name), 5.0)
+        Report.critical_result(Results.azshader_was_removed, not _asset_exists(azshader_name))
+
+        # Now let's copy all the source files to the "Assets" folder as fast as possible.
+        _copy_tmp_files_in_order(src_assets_subdir, reverse_file_list, game_asset_path)
+
+        # Give enough time to AP to compile the shader
+        helper.wait_for_condition(lambda: _asset_exists(azshader_name), 60.0)
+
+        Report.critical_result(Results.azshader_was_compiled, _asset_exists(azshader_name))
+
+        # All good, let's cleanup leftover files before closing the test.
+        _remove_files(game_asset_path, reverse_file_list)
+        helper.wait_for_condition(lambda: not _asset_exists(azshader_name), 5.0)
+
+
+if __name__ == "__main__":
+    # All exposed python bindings are in azlmbr
+    import azlmbr.legacy.general as general
+    import azlmbr.bus as azbus
+    import azlmbr.asset as azasset
+    import azlmbr.math as azmath
+
+    # Import report and test helper utilities
+    from editor_python_test_tools.utils import Report
+    from editor_python_test_tools.utils import TestHelper as helper
+    from editor_python_test_tools.utils import Tracer
+    import ly_test_tools.environment.file_system as fs
+
+    Report.start_test(ShaderAssetBuilder_RecompilesShaderAsChainOfDependenciesChanges)

AutomatedTesting/Gem/PythonTests/atom_renderer/test_Atom_ShaderBuildPipelineSuite.py

@@ -0,0 +1,19 @@
+"""
+Copyright (c) Contributors to the Open 3D Engine Project.
+For complete copyright and license terms please see the LICENSE at the root of this distribution.
+
+SPDX-License-Identifier: Apache-2.0 OR MIT
+
+Main suite tests for the Shader Build Pipeline.
+"""
+import pytest
+from ly_test_tools import LAUNCHERS
+from ly_test_tools.o3de.editor_test import EditorTestSuite, EditorSingleTest
+
+@pytest.mark.parametrize("project", ["AutomatedTesting"])
+@pytest.mark.parametrize("launcher_platform", ['windows_editor'])
+class TestShaderBuildPipelineMain(EditorTestSuite):
+    """Holds tests for Shader Build Pipeline validation"""
+
+    class ShaderAssetBuilder_RecompilesShaderAsChainOfDependenciesChanges(EditorSingleTest):
+        from .atom_hydra_scripts import hydra_ShaderAssetBuilder_RecompilesShaderAsChainOfDependenciesChanges as test_module

Code/Editor/GameExporter.cpp

@@ -318,7 +318,7 @@ void CGameExporter::ExportLevelInfo(const QString& path)
     root->setAttr("Name", levelName.toUtf8().data());
     auto terrain = AzFramework::Terrain::TerrainDataRequestBus::FindFirstHandler();
     const AZ::Aabb terrainAabb = terrain ? terrain->GetTerrainAabb() : AZ::Aabb::CreateFromPoint(AZ::Vector3::CreateZero());
-    const AZ::Vector2 terrainGridResolution = terrain ? terrain->GetTerrainGridResolution() : AZ::Vector2::CreateOne();
+    const AZ::Vector2 terrainGridResolution = terrain ? terrain->GetTerrainHeightQueryResolution() : AZ::Vector2::CreateOne();
     const int compiledHeightmapSize = static_cast<int>(terrainAabb.GetXExtent() / terrainGridResolution.GetX());
     root->setAttr("HeightmapSize", compiledHeightmapSize);
 

Code/Framework/AzFramework/AzFramework/Archive/Archive.cpp

@@ -2185,7 +2185,7 @@ namespace AZ::IO
         AZStd::unique_lock lock(m_archiveMutex);
         if (pArchive)
         {
-            AZ_TracePrintf("Archive", "Closing Archive file: %s", pArchive->GetFullPath());
+            AZ_TracePrintf("Archive", "Closing Archive file: %s\n", pArchive->GetFullPath());
         }
         ArchiveArray::iterator it;
         if (m_arrArchives.size() < 16)

Code/Framework/AzFramework/AzFramework/Terrain/TerrainDataRequestBus.cpp

@@ -51,7 +51,8 @@ namespace AzFramework
                     ->Event("GetNormal", &AzFramework::Terrain::TerrainDataRequestBus::Events::GetNormal)
                     ->Event("GetNormalFromFloats", &AzFramework::Terrain::TerrainDataRequestBus::Events::GetNormalFromFloats)
                     ->Event("GetTerrainAabb", &AzFramework::Terrain::TerrainDataRequestBus::Events::GetTerrainAabb)
-                    ->Event("GetTerrainGridResolution", &AzFramework::Terrain::TerrainDataRequestBus::Events::GetTerrainGridResolution)
+                    ->Event("GetTerrainHeightQueryResolution",
+                        &AzFramework::Terrain::TerrainDataRequestBus::Events::GetTerrainHeightQueryResolution)
                     ;
 
             }

Code/Framework/AzFramework/AzFramework/Terrain/TerrainDataRequestBus.h

@@ -59,8 +59,11 @@ namespace AzFramework
             static AZ::Vector3 GetDefaultTerrainNormal() { return AZ::Vector3::CreateAxisZ(); }
 
             // System-level queries to understand world size and resolution
-            virtual AZ::Vector2 GetTerrainGridResolution() const = 0;
+            virtual AZ::Vector2 GetTerrainHeightQueryResolution() const = 0;
+            virtual void SetTerrainHeightQueryResolution(AZ::Vector2 queryResolution) = 0;
+
             virtual AZ::Aabb GetTerrainAabb() const = 0;
+            virtual void SetTerrainAabb(const AZ::Aabb& worldBounds) = 0;
 
             //! Returns terrains height in meters at location x,y.
             //! @terrainExistsPtr: Can be nullptr. If != nullptr then, if there's no terrain at location x,y or location x,y is inside a terrain HOLE then *terrainExistsPtr will become false,

Code/Framework/AzToolsFramework/AzToolsFramework/ViewportSelection/EditorTransformComponentSelection.cpp

@@ -86,59 +86,60 @@ namespace AzToolsFramework
         "Sticky select implies a single click will not change selection with an entity already selected");
 
     // strings related to new viewport interaction model (EditorTransformComponentSelection)
-    static const char* const s_togglePivotTitleRightClick = "Toggle pivot";
-    static const char* const s_togglePivotTitleEditMenu = "Toggle Pivot Location";
-    static const char* const s_togglePivotDesc = "Toggle pivot location";
-    static const char* const s_manipulatorUndoRedoName = "Manipulator Adjustment";
-    static const char* const s_lockSelectionTitle = "Lock Selection";
-    static const char* const s_lockSelectionDesc = "Lock the selected entities so that they can't be selected in the viewport";
-    static const char* const s_hideSelectionTitle = "Hide Selection";
-    static const char* const s_hideSelectionDesc = "Hide the selected entities so that they don't appear in the viewport";
-    static const char* const s_unlockAllTitle = "Unlock All Entities";
-    static const char* const s_unlockAllDesc = "Unlock all entities the level";
-    static const char* const s_showAllTitle = "Show All";
-    static const char* const s_showAllDesc = "Show all entities so that they appear in the viewport";
-    static const char* const s_selectAllTitle = "Select All";
-    static const char* const s_selectAllDesc = "Select all entities";
-    static const char* const s_invertSelectionTitle = "Invert Selection";
-    static const char* const s_invertSelectionDesc = "Invert the current entity selection";
-    static const char* const s_duplicateTitle = "Duplicate";
-    static const char* const s_duplicateDesc = "Duplicate selected entities";
-    static const char* const s_deleteTitle = "Delete";
-    static const char* const s_deleteDesc = "Delete selected entities";
-    static const char* const s_resetEntityTransformTitle = "Reset Entity Transform";
-    static const char* const s_resetEntityTransformDesc = "Reset transform based on manipulator mode";
-    static const char* const s_resetManipulatorTitle = "Reset Manipulator";
-    static const char* const s_resetManipulatorDesc = "Reset the manipulator to recenter it on the selected entity";
-    static const char* const s_resetTransformLocalTitle = "Reset Transform (Local)";
-    static const char* const s_resetTransformLocalDesc = "Reset transform to local space";
-    static const char* const s_resetTransformWorldTitle = "Reset Transform (World)";
-    static const char* const s_resetTransformWorldDesc = "Reset transform to world space";
+    static const char* const TogglePivotTitleRightClick = "Toggle pivot";
+    static const char* const TogglePivotTitleEditMenu = "Toggle Pivot Location";
+    static const char* const TogglePivotDesc = "Toggle pivot location";
+    static const char* const ManipulatorUndoRedoName = "Manipulator Adjustment";
+    static const char* const LockSelectionTitle = "Lock Selection";
+    static const char* const LockSelectionDesc = "Lock the selected entities so that they can't be selected in the viewport";
+    static const char* const HideSelectionTitle = "Hide Selection";
+    static const char* const HideSelectionDesc = "Hide the selected entities so that they don't appear in the viewport";
+    static const char* const UnlockAllTitle = "Unlock All Entities";
+    static const char* const UnlockAllDesc = "Unlock all entities the level";
+    static const char* const ShowAllTitle = "Show All";
+    static const char* const ShowAllDesc = "Show all entities so that they appear in the viewport";
+    static const char* const SelectAllTitle = "Select All";
+    static const char* const SelectAllDesc = "Select all entities";
+    static const char* const InvertSelectionTitle = "Invert Selection";
+    static const char* const InvertSelectionDesc = "Invert the current entity selection";
+    static const char* const DuplicateTitle = "Duplicate";
+    static const char* const DuplicateDesc = "Duplicate selected entities";
+    static const char* const DeleteTitle = "Delete";
+    static const char* const DeleteDesc = "Delete selected entities";
+    static const char* const ResetEntityTransformTitle = "Reset Entity Transform";
+    static const char* const ResetEntityTransformDesc = "Reset transform based on manipulator mode";
+    static const char* const ResetManipulatorTitle = "Reset Manipulator";
+    static const char* const ResetManipulatorDesc = "Reset the manipulator to recenter it on the selected entity";
+    static const char* const ResetTransformLocalTitle = "Reset Transform (Local)";
+    static const char* const ResetTransformLocalDesc = "Reset transform to local space";
+    static const char* const ResetTransformWorldTitle = "Reset Transform (World)";
+    static const char* const ResetTransformWorldDesc = "Reset transform to world space";
 
-    static const char* const s_entityBoxSelectUndoRedoDesc = "Box Select Entities";
-    static const char* const s_entityDeselectUndoRedoDesc = "Deselect Entity";
-    static const char* const s_entitiesDeselectUndoRedoDesc = "Deselect Entities";
-    static const char* const s_entitySelectUndoRedoDesc = "Select Entity";
-    static const char* const s_dittoManipulatorUndoRedoDesc = "Ditto Manipulator";
-    static const char* const s_resetManipulatorTranslationUndoRedoDesc = "Reset Manipulator Translation";
-    static const char* const s_resetManipulatorOrientationUndoRedoDesc = "Reset Manipulator Orientation";
-    static const char* const s_dittoEntityOrientationIndividualUndoRedoDesc = "Ditto orientation individual";
-    static const char* const s_dittoEntityOrientationGroupUndoRedoDesc = "Ditto orientation group";
-    static const char* const s_resetTranslationToParentUndoRedoDesc = "Reset translation to parent";
-    static const char* const s_resetOrientationToParentUndoRedoDesc = "Reset orientation to parent";
-    static const char* const s_dittoTranslationGroupUndoRedoDesc = "Ditto translation group";
-    static const char* const s_dittoTranslationIndividualUndoRedoDesc = "Ditto translation individual";
-    static const char* const s_dittoScaleIndividualWorldUndoRedoDesc = "Ditto scale individual world";
-    static const char* const s_dittoScaleIndividualLocalUndoRedoDesc = "Ditto scale individual local";
-    static const char* const s_snapToWorldGridUndoRedoDesc = "Snap to world grid";
-    static const char* const s_showAllEntitiesUndoRedoDesc = s_showAllTitle;
-    static const char* const s_lockSelectionUndoRedoDesc = s_lockSelectionTitle;
-    static const char* const s_hideSelectionUndoRedoDesc = s_hideSelectionTitle;
-    static const char* const s_unlockAllUndoRedoDesc = s_unlockAllTitle;
-    static const char* const s_selectAllEntitiesUndoRedoDesc = s_selectAllTitle;
-    static const char* const s_invertSelectionUndoRedoDesc = s_invertSelectionTitle;
-    static const char* const s_duplicateUndoRedoDesc = s_duplicateTitle;
-    static const char* const s_deleteUndoRedoDesc = s_deleteTitle;
+    static const char* const EntityBoxSelectUndoRedoDesc = "Box Select Entities";
+    static const char* const EntityDeselectUndoRedoDesc = "Deselect Entity";
+    static const char* const EntitiesDeselectUndoRedoDesc = "Deselect Entities";
+    static const char* const ChangeEntitySelectionUndoRedoDesc = "Change Selected Entity";
+    static const char* const EntitySelectUndoRedoDesc = "Select Entity";
+    static const char* const DittoManipulatorUndoRedoDesc = "Ditto Manipulator";
+    static const char* const ResetManipulatorTranslationUndoRedoDesc = "Reset Manipulator Translation";
+    static const char* const ResetManipulatorOrientationUndoRedoDesc = "Reset Manipulator Orientation";
+    static const char* const DittoEntityOrientationIndividualUndoRedoDesc = "Ditto orientation individual";
+    static const char* const DittoEntityOrientationGroupUndoRedoDesc = "Ditto orientation group";
+    static const char* const ResetTranslationToParentUndoRedoDesc = "Reset translation to parent";
+    static const char* const ResetOrientationToParentUndoRedoDesc = "Reset orientation to parent";
+    static const char* const DittoTranslationGroupUndoRedoDesc = "Ditto translation group";
+    static const char* const DittoTranslationIndividualUndoRedoDesc = "Ditto translation individual";
+    static const char* const DittoScaleIndividualWorldUndoRedoDesc = "Ditto scale individual world";
+    static const char* const DittoScaleIndividualLocalUndoRedoDesc = "Ditto scale individual local";
+    static const char* const SnapToWorldGridUndoRedoDesc = "Snap to world grid";
+    static const char* const ShowAllEntitiesUndoRedoDesc = ShowAllTitle;
+    static const char* const LockSelectionUndoRedoDesc = LockSelectionTitle;
+    static const char* const HideSelectionUndoRedoDesc = HideSelectionTitle;
+    static const char* const UnlockAllUndoRedoDesc = UnlockAllTitle;
+    static const char* const SelectAllEntitiesUndoRedoDesc = SelectAllTitle;
+    static const char* const InvertSelectionUndoRedoDesc = InvertSelectionTitle;
+    static const char* const DuplicateUndoRedoDesc = DuplicateTitle;
+    static const char* const DeleteUndoRedoDesc = DeleteTitle;
 
     static const char* const TransformModeClusterTranslateTooltip = "Switch to translate mode";
     static const char* const TransformModeClusterRotateTooltip = "Switch to rotate mode";
@@ -148,14 +149,14 @@ namespace AzToolsFramework
     static const char* const SpaceClusterLocalTooltip = "Toggle local space lock";
     static const char* const SnappingClusterSnapToWorldTooltip = "Snap selected entities to the world space grid";
 
-    static const AZ::Color s_fadedXAxisColor = AZ::Color(AZ::u8(200), AZ::u8(127), AZ::u8(127), AZ::u8(255));
-    static const AZ::Color s_fadedYAxisColor = AZ::Color(AZ::u8(127), AZ::u8(190), AZ::u8(127), AZ::u8(255));
-    static const AZ::Color s_fadedZAxisColor = AZ::Color(AZ::u8(120), AZ::u8(120), AZ::u8(180), AZ::u8(255));
+    static const AZ::Color FadedXAxisColor = AZ::Color(AZ::u8(200), AZ::u8(127), AZ::u8(127), AZ::u8(255));
+    static const AZ::Color FadedYAxisColor = AZ::Color(AZ::u8(127), AZ::u8(190), AZ::u8(127), AZ::u8(255));
+    static const AZ::Color FadedZAxisColor = AZ::Color(AZ::u8(120), AZ::u8(120), AZ::u8(180), AZ::u8(255));
 
-    static const AZ::Color s_pickedOrientationColor = AZ::Color(0.0f, 1.0f, 0.0f, 1.0f);
-    static const AZ::Color s_selectedEntityAabbColor = AZ::Color(0.6f, 0.6f, 0.6f, 0.4f);
+    static const AZ::Color PickedOrientationColor = AZ::Color(0.0f, 1.0f, 0.0f, 1.0f);
+    static const AZ::Color SelectedEntityAabbColor = AZ::Color(0.6f, 0.6f, 0.6f, 0.4f);
 
-    static const float s_pivotSize = 0.075f; // the size of the pivot (box) to render when selected
+    static const float PivotSize = 0.075f; // the size of the pivot (box) to render when selected
 
     // data passed to manipulators when processing mouse interactions
     // m_entityIds should be sorted based on the entity hierarchy
@@ -1107,7 +1108,7 @@ namespace AzToolsFramework
             {
                 // begin selection undo/redo command
                 entityBoxSelectData->m_boxSelectSelectionCommand =
-                    AZStd::make_unique<SelectionCommand>(EntityIdList(), s_entityBoxSelectUndoRedoDesc);
+                    AZStd::make_unique<SelectionCommand>(EntityIdList(), EntityBoxSelectUndoRedoDesc);
                 // grab currently selected entities
                 entityBoxSelectData->m_selectedEntityIdsBeforeBoxSelect = m_selectedEntityIds;
             });
@@ -1131,7 +1132,7 @@ namespace AzToolsFramework
                 if (!entityBoxSelectData->m_potentialDeselectedEntityIds.empty() ||
                     !entityBoxSelectData->m_potentialSelectedEntityIds.empty())
                 {
-                    ScopedUndoBatch undoBatch(s_entityBoxSelectUndoRedoDesc);
+                    ScopedUndoBatch undoBatch(EntityBoxSelectUndoRedoDesc);
 
                     // restore manipulator overrides when undoing
                     if (m_entityIdManipulators.m_manipulators && m_selectedEntityIds.empty())
@@ -1174,7 +1175,7 @@ namespace AzToolsFramework
                 }
 
                 debugDisplay.DepthTestOff();
-                debugDisplay.SetColor(s_selectedEntityAabbColor);
+                debugDisplay.SetColor(SelectedEntityAabbColor);
 
                 for (AZ::EntityId entityId : entityBoxSelectData->m_potentialSelectedEntityIds)
                 {
@@ -1222,7 +1223,7 @@ namespace AzToolsFramework
         {
             // check here if translation or orientation override are set
             m_manipulatorMoveCommand =
-                AZStd::make_unique<EntityManipulatorCommand>(CreateManipulatorCommandStateFromSelf(), s_manipulatorUndoRedoName);
+                AZStd::make_unique<EntityManipulatorCommand>(CreateManipulatorCommandStateFromSelf(), ManipulatorUndoRedoName);
         }
     }
 
@@ -1696,7 +1697,7 @@ namespace AzToolsFramework
 
         if (!UndoRedoOperationInProgress())
         {
-            ScopedUndoBatch undoBatch(s_entitiesDeselectUndoRedoDesc);
+            ScopedUndoBatch undoBatch(EntitiesDeselectUndoRedoDesc);
 
             // restore manipulator overrides when undoing
             if (m_entityIdManipulators.m_manipulators)
@@ -1706,7 +1707,7 @@ namespace AzToolsFramework
 
             // select must happen after to ensure in the undo/redo step the selection command
             // happens before the manipulator command
-            auto selectionCommand = AZStd::make_unique<SelectionCommand>(EntityIdList(), s_entitiesDeselectUndoRedoDesc);
+            auto selectionCommand = AZStd::make_unique<SelectionCommand>(EntityIdList(), EntitiesDeselectUndoRedoDesc);
             selectionCommand->SetParent(undoBatch.GetUndoBatch());
             selectionCommand.release();
         }
@@ -1732,7 +1733,7 @@ namespace AzToolsFramework
 
                     const auto nextEntityIds = EntityIdVectorFromContainer(m_selectedEntityIds);
 
-                    ScopedUndoBatch undoBatch(s_entityDeselectUndoRedoDesc);
+                    ScopedUndoBatch undoBatch(EntityDeselectUndoRedoDesc);
 
                     // store manipulator state when removing last entity from selection
                     if (m_entityIdManipulators.m_manipulators && nextEntityIds.empty())
@@ -1740,7 +1741,7 @@ namespace AzToolsFramework
                         CreateEntityManipulatorDeselectCommand(undoBatch);
                     }
 
-                    auto selectionCommand = AZStd::make_unique<SelectionCommand>(nextEntityIds, s_entityDeselectUndoRedoDesc);
+                    auto selectionCommand = AZStd::make_unique<SelectionCommand>(nextEntityIds, EntityDeselectUndoRedoDesc);
                     selectionCommand->SetParent(undoBatch.GetUndoBatch());
                     selectionCommand.release();
 
@@ -1755,8 +1756,8 @@ namespace AzToolsFramework
 
                     const auto nextEntityIds = EntityIdVectorFromContainer(m_selectedEntityIds);
 
-                    ScopedUndoBatch undoBatch(s_entitySelectUndoRedoDesc);
-                    auto selectionCommand = AZStd::make_unique<SelectionCommand>(nextEntityIds, s_entitySelectUndoRedoDesc);
+                    ScopedUndoBatch undoBatch(EntitySelectUndoRedoDesc);
+                    auto selectionCommand = AZStd::make_unique<SelectionCommand>(nextEntityIds, EntitySelectUndoRedoDesc);
                     selectionCommand->SetParent(undoBatch.GetUndoBatch());
                     selectionCommand.release();
 
@@ -1772,6 +1773,13 @@ namespace AzToolsFramework
 
     void EditorTransformComponentSelection::ChangeSelectedEntity(const AZ::EntityId entityId)
     {
+        AZ_Assert(
+            !UndoRedoOperationInProgress(),
+            "ChangeSelectedEntity called from undo/redo operation - this is unexpected and not currently supported");
+
+        // ensure deselect/select is tracked as an atomic undo/redo operation
+        ScopedUndoBatch undoBatch(ChangeEntitySelectionUndoRedoDesc);
+
         DeselectEntities();
         SelectDeselect(entityId);
     }
@@ -1799,7 +1807,7 @@ namespace AzToolsFramework
                 const AZ::Transform& worldFromLocal = m_entityDataCache->GetVisibleEntityTransform(*entityIndex);
                 const AZ::Vector3 boxPosition = worldFromLocal.TransformPoint(CalculateCenterOffset(entityId, m_pivotMode));
                 const AZ::Vector3 scaledSize =
-                    AZ::Vector3(s_pivotSize) * CalculateScreenToWorldMultiplier(worldFromLocal.GetTranslation(), cameraState);
+                    AZ::Vector3(PivotSize) * CalculateScreenToWorldMultiplier(worldFromLocal.GetTranslation(), cameraState);
 
                 if (AabbIntersectMouseRay(
                         mouseInteraction.m_mouseInteraction,
@@ -2002,10 +2010,10 @@ namespace AzToolsFramework
     {
         if (m_entityIdManipulators.m_manipulators)
         {
-            ScopedUndoBatch undoBatch(s_dittoManipulatorUndoRedoDesc);
+            ScopedUndoBatch undoBatch(DittoManipulatorUndoRedoDesc);
 
             auto manipulatorCommand =
-                AZStd::make_unique<EntityManipulatorCommand>(CreateManipulatorCommandStateFromSelf(), s_manipulatorUndoRedoName);
+                AZStd::make_unique<EntityManipulatorCommand>(CreateManipulatorCommandStateFromSelf(), ManipulatorUndoRedoName);
 
             if (entityId.IsValid())
             {
@@ -2131,7 +2139,7 @@ namespace AzToolsFramework
         {
             AZ_PROFILE_FUNCTION(AzToolsFramework);
 
-            ScopedUndoBatch undoBatch(s_lockSelectionUndoRedoDesc);
+            ScopedUndoBatch undoBatch(LockSelectionUndoRedoDesc);
 
             if (m_entityIdManipulators.m_manipulators)
             {
@@ -2151,7 +2159,7 @@ namespace AzToolsFramework
 
         // lock selection
         AddAction(
-            m_actions, { QKeySequence(Qt::Key_L) }, LockSelection, s_lockSelectionTitle, s_lockSelectionDesc,
+            m_actions, { QKeySequence(Qt::Key_L) }, LockSelection, LockSelectionTitle, LockSelectionDesc,
             [lockUnlock]()
             {
                 lockUnlock(true);
@@ -2159,7 +2167,7 @@ namespace AzToolsFramework
 
         // unlock selection
         AddAction(
-            m_actions, { QKeySequence(Qt::CTRL + Qt::Key_L) }, UnlockSelection, s_lockSelectionTitle, s_lockSelectionDesc,
+            m_actions, { QKeySequence(Qt::CTRL + Qt::Key_L) }, UnlockSelection, LockSelectionTitle, LockSelectionDesc,
             [lockUnlock]()
             {
                 lockUnlock(false);
@@ -2169,7 +2177,7 @@ namespace AzToolsFramework
         {
             AZ_PROFILE_FUNCTION(AzToolsFramework);
 
-            ScopedUndoBatch undoBatch(s_hideSelectionUndoRedoDesc);
+            ScopedUndoBatch undoBatch(HideSelectionUndoRedoDesc);
 
             if (m_entityIdManipulators.m_manipulators)
             {
@@ -2189,7 +2197,7 @@ namespace AzToolsFramework
 
         // hide selection
         AddAction(
-            m_actions, { QKeySequence(Qt::Key_H) }, HideSelection, s_hideSelectionTitle, s_hideSelectionDesc,
+            m_actions, { QKeySequence(Qt::Key_H) }, HideSelection, HideSelectionTitle, HideSelectionDesc,
             [showHide]()
             {
                 showHide(false);
@@ -2197,7 +2205,7 @@ namespace AzToolsFramework
 
         // show selection
         AddAction(
-            m_actions, { QKeySequence(Qt::CTRL + Qt::Key_H) }, ShowSelection, s_hideSelectionTitle, s_hideSelectionDesc,
+            m_actions, { QKeySequence(Qt::CTRL + Qt::Key_H) }, ShowSelection, HideSelectionTitle, HideSelectionDesc,
             [showHide]()
             {
                 showHide(true);
@@ -2205,12 +2213,12 @@ namespace AzToolsFramework
 
         // unlock all entities in the level/scene
         AddAction(
-            m_actions, { QKeySequence(Qt::CTRL + Qt::SHIFT + Qt::Key_L) }, UnlockAll, s_unlockAllTitle, s_unlockAllDesc,
+            m_actions, { QKeySequence(Qt::CTRL + Qt::SHIFT + Qt::Key_L) }, UnlockAll, UnlockAllTitle, UnlockAllDesc,
             []()
             {
                 AZ_PROFILE_FUNCTION(AzToolsFramework);
 
-                ScopedUndoBatch undoBatch(s_unlockAllUndoRedoDesc);
+                ScopedUndoBatch undoBatch(UnlockAllUndoRedoDesc);
 
                 EnumerateEditorEntities(
                     [](AZ::EntityId entityId)
@@ -2222,12 +2230,12 @@ namespace AzToolsFramework
 
         // show all entities in the level/scene
         AddAction(
-            m_actions, { QKeySequence(Qt::CTRL + Qt::SHIFT + Qt::Key_H) }, ShowAll, s_showAllTitle, s_showAllDesc,
+            m_actions, { QKeySequence(Qt::CTRL + Qt::SHIFT + Qt::Key_H) }, ShowAll, ShowAllTitle, ShowAllDesc,
             []()
             {
                 AZ_PROFILE_FUNCTION(AzToolsFramework);
 
-                ScopedUndoBatch undoBatch(s_showAllEntitiesUndoRedoDesc);
+                ScopedUndoBatch undoBatch(ShowAllEntitiesUndoRedoDesc);
 
                 EnumerateEditorEntities(
                     [](AZ::EntityId entityId)
@@ -2239,17 +2247,17 @@ namespace AzToolsFramework
 
         // select all entities in the level/scene
         AddAction(
-            m_actions, { QKeySequence(Qt::CTRL + Qt::Key_A) }, SelectAll, s_selectAllTitle, s_selectAllDesc,
+            m_actions, { QKeySequence(Qt::CTRL + Qt::Key_A) }, SelectAll, SelectAllTitle, SelectAllDesc,
             [this]()
             {
                 AZ_PROFILE_FUNCTION(AzToolsFramework);
 
-                ScopedUndoBatch undoBatch(s_selectAllEntitiesUndoRedoDesc);
+                ScopedUndoBatch undoBatch(SelectAllEntitiesUndoRedoDesc);
 
                 if (m_entityIdManipulators.m_manipulators)
                 {
                     auto manipulatorCommand =
-                        AZStd::make_unique<EntityManipulatorCommand>(CreateManipulatorCommandStateFromSelf(), s_manipulatorUndoRedoName);
+                        AZStd::make_unique<EntityManipulatorCommand>(CreateManipulatorCommandStateFromSelf(), ManipulatorUndoRedoName);
 
                     // note, nothing will change that the manipulatorCommand needs to keep track
                     // for after so no need to call SetManipulatorAfter
@@ -2269,7 +2277,7 @@ namespace AzToolsFramework
 
                 auto nextEntityIds = EntityIdVectorFromContainer(m_selectedEntityIds);
 
-                auto selectionCommand = AZStd::make_unique<SelectionCommand>(nextEntityIds, s_selectAllEntitiesUndoRedoDesc);
+                auto selectionCommand = AZStd::make_unique<SelectionCommand>(nextEntityIds, SelectAllEntitiesUndoRedoDesc);
                 selectionCommand->SetParent(undoBatch.GetUndoBatch());
                 selectionCommand.release();
 
@@ -2279,17 +2287,17 @@ namespace AzToolsFramework
 
         // invert current selection
         AddAction(
-            m_actions, { QKeySequence(Qt::CTRL + Qt::SHIFT + Qt::Key_I) }, InvertSelect, s_invertSelectionTitle, s_invertSelectionDesc,
+            m_actions, { QKeySequence(Qt::CTRL + Qt::SHIFT + Qt::Key_I) }, InvertSelect, InvertSelectionTitle, InvertSelectionDesc,
             [this]()
             {
                 AZ_PROFILE_FUNCTION(AzToolsFramework);
 
-                ScopedUndoBatch undoBatch(s_invertSelectionUndoRedoDesc);
+                ScopedUndoBatch undoBatch(InvertSelectionUndoRedoDesc);
 
                 if (m_entityIdManipulators.m_manipulators)
                 {
                     auto manipulatorCommand =
-                        AZStd::make_unique<EntityManipulatorCommand>(CreateManipulatorCommandStateFromSelf(), s_manipulatorUndoRedoName);
+                        AZStd::make_unique<EntityManipulatorCommand>(CreateManipulatorCommandStateFromSelf(), ManipulatorUndoRedoName);
 
                     // note, nothing will change that the manipulatorCommand needs to keep track
                     // for after so no need to call SetManipulatorAfter
@@ -2316,7 +2324,7 @@ namespace AzToolsFramework
 
                 auto nextEntityIds = EntityIdVectorFromContainer(entityIds);
 
-                auto selectionCommand = AZStd::make_unique<SelectionCommand>(nextEntityIds, s_invertSelectionUndoRedoDesc);
+                auto selectionCommand = AZStd::make_unique<SelectionCommand>(nextEntityIds, InvertSelectionUndoRedoDesc);
                 selectionCommand->SetParent(undoBatch.GetUndoBatch());
                 selectionCommand.release();
 
@@ -2326,7 +2334,7 @@ namespace AzToolsFramework
 
         // duplicate selection
         AddAction(
-            m_actions, { QKeySequence(Qt::CTRL + Qt::Key_D) }, DuplicateSelect, s_duplicateTitle, s_duplicateDesc,
+            m_actions, { QKeySequence(Qt::CTRL + Qt::Key_D) }, DuplicateSelect, DuplicateTitle, DuplicateDesc,
             []()
             {
                 AZ_PROFILE_FUNCTION(AzToolsFramework);
@@ -2338,8 +2346,8 @@ namespace AzToolsFramework
                     QApplication::focusWidget()->clearFocus();
                 }
 
-                ScopedUndoBatch undoBatch(s_duplicateUndoRedoDesc);
-                auto selectionCommand = AZStd::make_unique<SelectionCommand>(EntityIdList(), s_duplicateUndoRedoDesc);
+                ScopedUndoBatch undoBatch(DuplicateUndoRedoDesc);
+                auto selectionCommand = AZStd::make_unique<SelectionCommand>(EntityIdList(), DuplicateUndoRedoDesc);
                 selectionCommand->SetParent(undoBatch.GetUndoBatch());
                 selectionCommand.release();
 
@@ -2351,12 +2359,12 @@ namespace AzToolsFramework
 
         // delete selection
         AddAction(
-            m_actions, { QKeySequence(Qt::Key_Delete) }, DeleteSelect, s_deleteTitle, s_deleteDesc,
+            m_actions, { QKeySequence(Qt::Key_Delete) }, DeleteSelect, DeleteTitle, DeleteDesc,
             [this]()
             {
                 AZ_PROFILE_FUNCTION(AzToolsFramework);
 
-                ScopedUndoBatch undoBatch(s_deleteUndoRedoDesc);
+                ScopedUndoBatch undoBatch(DeleteUndoRedoDesc);
 
                 CreateEntityManipulatorDeselectCommand(undoBatch);
 
@@ -2375,14 +2383,14 @@ namespace AzToolsFramework
             });
 
         AddAction(
-            m_actions, { QKeySequence(Qt::Key_P) }, EditPivot, s_togglePivotTitleEditMenu, s_togglePivotDesc,
+            m_actions, { QKeySequence(Qt::Key_P) }, EditPivot, TogglePivotTitleEditMenu, TogglePivotDesc,
             [this]()
             {
                 ToggleCenterPivotSelection();
             });
 
         AddAction(
-            m_actions, { QKeySequence(Qt::Key_R) }, EditReset, s_resetEntityTransformTitle, s_resetEntityTransformDesc,
+            m_actions, { QKeySequence(Qt::Key_R) }, EditReset, ResetEntityTransformTitle, ResetEntityTransformDesc,
             [this]()
             {
                 switch (m_mode)
@@ -2400,11 +2408,11 @@ namespace AzToolsFramework
             });
 
         AddAction(
-            m_actions, { QKeySequence(Qt::CTRL + Qt::Key_R) }, EditResetManipulator, s_resetManipulatorTitle, s_resetManipulatorDesc,
+            m_actions, { QKeySequence(Qt::CTRL + Qt::Key_R) }, EditResetManipulator, ResetManipulatorTitle, ResetManipulatorDesc,
             AZStd::bind(AZStd::mem_fn(&EditorTransformComponentSelection::DelegateClearManipulatorOverride), this));
 
         AddAction(
-            m_actions, { QKeySequence(Qt::ALT + Qt::Key_R) }, EditResetLocal, s_resetTransformLocalTitle, s_resetTransformLocalDesc,
+            m_actions, { QKeySequence(Qt::ALT + Qt::Key_R) }, EditResetLocal, ResetTransformLocalTitle, ResetTransformLocalDesc,
             [this]()
             {
                 switch (m_mode)
@@ -2422,7 +2430,7 @@ namespace AzToolsFramework
             });
 
         AddAction(
-            m_actions, { QKeySequence(Qt::SHIFT + Qt::Key_R) }, EditResetWorld, s_resetTransformWorldTitle, s_resetTransformWorldDesc,
+            m_actions, { QKeySequence(Qt::SHIFT + Qt::Key_R) }, EditResetWorld, ResetTransformWorldTitle, ResetTransformWorldDesc,
             [this]()
             {
                 switch (m_mode)
@@ -2431,7 +2439,7 @@ namespace AzToolsFramework
                     {
                         // begin an undo batch so operations inside CopyOrientation... and
                         // DelegateClear... are grouped into a single undo/redo
-                        ScopedUndoBatch undoBatch{ s_resetTransformWorldTitle };
+                        ScopedUndoBatch undoBatch{ ResetTransformWorldTitle };
                         CopyOrientationToSelectedEntitiesIndividual(AZ::Quaternion::CreateIdentity());
                         ClearManipulatorOrientationOverride();
                     }
@@ -2685,7 +2693,7 @@ namespace AzToolsFramework
 
         const AZStd::array snapAxes = { AZ::Vector3::CreateAxisX(), AZ::Vector3::CreateAxisY(), AZ::Vector3::CreateAxisZ() };
 
-        ScopedUndoBatch undoBatch(s_snapToWorldGridUndoRedoDesc);
+        ScopedUndoBatch undoBatch(SnapToWorldGridUndoRedoDesc);
         for (const AZ::EntityId& entityId : m_selectedEntityIds)
         {
             ScopedUndoBatch::MarkEntityDirty(entityId);
@@ -2870,10 +2878,10 @@ namespace AzToolsFramework
 
         if (m_entityIdManipulators.m_manipulators)
         {
-            ScopedUndoBatch undoBatch(s_resetManipulatorTranslationUndoRedoDesc);
+            ScopedUndoBatch undoBatch(ResetManipulatorTranslationUndoRedoDesc);
 
             auto manipulatorCommand =
-                AZStd::make_unique<EntityManipulatorCommand>(CreateManipulatorCommandStateFromSelf(), s_manipulatorUndoRedoName);
+                AZStd::make_unique<EntityManipulatorCommand>(CreateManipulatorCommandStateFromSelf(), ManipulatorUndoRedoName);
 
             m_pivotOverrideFrame.ResetPickedTranslation();
             m_pivotOverrideFrame.m_pickedEntityIdOverride.SetInvalid();
@@ -2896,10 +2904,10 @@ namespace AzToolsFramework
 
         if (m_entityIdManipulators.m_manipulators)
         {
-            ScopedUndoBatch undoBatch{ s_resetManipulatorOrientationUndoRedoDesc };
+            ScopedUndoBatch undoBatch{ ResetManipulatorOrientationUndoRedoDesc };
 
             auto manipulatorCommand =
-                AZStd::make_unique<EntityManipulatorCommand>(CreateManipulatorCommandStateFromSelf(), s_manipulatorUndoRedoName);
+                AZStd::make_unique<EntityManipulatorCommand>(CreateManipulatorCommandStateFromSelf(), ManipulatorUndoRedoName);
 
             m_pivotOverrideFrame.ResetPickedOrientation();
             m_pivotOverrideFrame.m_pickedEntityIdOverride.SetInvalid();
@@ -2961,13 +2969,13 @@ namespace AzToolsFramework
 
         if (m_entityIdManipulators.m_manipulators)
         {
-            ScopedUndoBatch undoBatch(s_dittoTranslationGroupUndoRedoDesc);
+            ScopedUndoBatch undoBatch(DittoTranslationGroupUndoRedoDesc);
 
             // store previous translation manipulator position
             const AZ::Vector3 previousPivotTranslation = m_entityIdManipulators.m_manipulators->GetLocalTransform().GetTranslation();
 
             auto manipulatorCommand =
-                AZStd::make_unique<EntityManipulatorCommand>(CreateManipulatorCommandStateFromSelf(), s_manipulatorUndoRedoName);
+                AZStd::make_unique<EntityManipulatorCommand>(CreateManipulatorCommandStateFromSelf(), ManipulatorUndoRedoName);
 
             // refresh the transform pivot override if it's set
             if (m_pivotOverrideFrame.m_translationOverride)
@@ -3017,10 +3025,10 @@ namespace AzToolsFramework
 
         if (m_entityIdManipulators.m_manipulators)
         {
-            ScopedUndoBatch undoBatch(s_dittoTranslationIndividualUndoRedoDesc);
+            ScopedUndoBatch undoBatch(DittoTranslationIndividualUndoRedoDesc);
 
             auto manipulatorCommand =
-                AZStd::make_unique<EntityManipulatorCommand>(CreateManipulatorCommandStateFromSelf(), s_manipulatorUndoRedoName);
+                AZStd::make_unique<EntityManipulatorCommand>(CreateManipulatorCommandStateFromSelf(), ManipulatorUndoRedoName);
 
             // refresh the transform pivot override if it's set
             if (m_pivotOverrideFrame.m_translationOverride)
@@ -3055,7 +3063,7 @@ namespace AzToolsFramework
     {
         AZ_PROFILE_FUNCTION(AzToolsFramework);
 
-        ScopedUndoBatch undoBatch(s_dittoScaleIndividualWorldUndoRedoDesc);
+        ScopedUndoBatch undoBatch(DittoScaleIndividualWorldUndoRedoDesc);
 
         ManipulatorEntityIds manipulatorEntityIds;
         BuildSortedEntityIdVectorFromEntityIdMap(m_entityIdManipulators.m_lookups, manipulatorEntityIds.m_entityIds);
@@ -3089,7 +3097,7 @@ namespace AzToolsFramework
     {
         AZ_PROFILE_FUNCTION(AzToolsFramework);
 
-        ScopedUndoBatch undoBatch(s_dittoScaleIndividualLocalUndoRedoDesc);
+        ScopedUndoBatch undoBatch(DittoScaleIndividualLocalUndoRedoDesc);
 
         ManipulatorEntityIds manipulatorEntityIds;
         BuildSortedEntityIdVectorFromEntityIdMap(m_entityIdManipulators.m_lookups, manipulatorEntityIds.m_entityIds);
@@ -3110,10 +3118,10 @@ namespace AzToolsFramework
 
         if (m_entityIdManipulators.m_manipulators)
         {
-            ScopedUndoBatch undoBatch{ s_dittoEntityOrientationIndividualUndoRedoDesc };
+            ScopedUndoBatch undoBatch{ DittoEntityOrientationIndividualUndoRedoDesc };
 
             auto manipulatorCommand =
-                AZStd::make_unique<EntityManipulatorCommand>(CreateManipulatorCommandStateFromSelf(), s_manipulatorUndoRedoName);
+                AZStd::make_unique<EntityManipulatorCommand>(CreateManipulatorCommandStateFromSelf(), ManipulatorUndoRedoName);
 
             ManipulatorEntityIds manipulatorEntityIds;
             BuildSortedEntityIdVectorFromEntityIdMap(m_entityIdManipulators.m_lookups, manipulatorEntityIds.m_entityIds);
@@ -3148,10 +3156,10 @@ namespace AzToolsFramework
 
         if (m_entityIdManipulators.m_manipulators)
         {
-            ScopedUndoBatch undoBatch(s_dittoEntityOrientationGroupUndoRedoDesc);
+            ScopedUndoBatch undoBatch(DittoEntityOrientationGroupUndoRedoDesc);
 
             auto manipulatorCommand =
-                AZStd::make_unique<EntityManipulatorCommand>(CreateManipulatorCommandStateFromSelf(), s_manipulatorUndoRedoName);
+                AZStd::make_unique<EntityManipulatorCommand>(CreateManipulatorCommandStateFromSelf(), ManipulatorUndoRedoName);
 
             ManipulatorEntityIds manipulatorEntityIds;
             BuildSortedEntityIdVectorFromEntityIdMap(m_entityIdManipulators.m_lookups, manipulatorEntityIds.m_entityIds);
@@ -3194,7 +3202,7 @@ namespace AzToolsFramework
     {
         AZ_PROFILE_FUNCTION(AzToolsFramework);
 
-        ScopedUndoBatch undoBatch(s_resetOrientationToParentUndoRedoDesc);
+        ScopedUndoBatch undoBatch(ResetOrientationToParentUndoRedoDesc);
         for (const auto& entityIdLookup : m_entityIdManipulators.m_lookups)
         {
             ScopedUndoBatch::MarkEntityDirty(entityIdLookup.first);
@@ -3215,7 +3223,7 @@ namespace AzToolsFramework
 
         if (m_entityIdManipulators.m_manipulators)
         {
-            ScopedUndoBatch undoBatch(s_resetTranslationToParentUndoRedoDesc);
+            ScopedUndoBatch undoBatch(ResetTranslationToParentUndoRedoDesc);
 
             ManipulatorEntityIds manipulatorEntityIds;
             BuildSortedEntityIdVectorFromEntityIdMap(m_entityIdManipulators.m_lookups, manipulatorEntityIds.m_entityIds);
@@ -3251,7 +3259,7 @@ namespace AzToolsFramework
     void EditorTransformComponentSelection::PopulateEditorGlobalContextMenu(
         QMenu* menu, [[maybe_unused]] const AZ::Vector2& point, [[maybe_unused]] int flags)
     {
-        QAction* action = menu->addAction(QObject::tr(s_togglePivotTitleRightClick));
+        QAction* action = menu->addAction(QObject::tr(TogglePivotTitleRightClick));
         QObject::connect(
             action, &QAction::triggered, action,
             [this]()
@@ -3329,15 +3337,15 @@ namespace AzToolsFramework
                 : 1.0f;
         };
 
-        display.SetColor(s_fadedXAxisColor);
+        display.SetColor(FadedXAxisColor);
         display.DrawLine(
             transform.GetTranslation(),
             transform.GetTranslation() + transform.GetBasisX().GetNormalizedSafe() * axisLength * axisFlip(AZ::Vector3::CreateAxisX()));
-        display.SetColor(s_fadedYAxisColor);
+        display.SetColor(FadedYAxisColor);
         display.DrawLine(
             transform.GetTranslation(),
             transform.GetTranslation() + transform.GetBasisY().GetNormalizedSafe() * axisLength * axisFlip(AZ::Vector3::CreateAxisY()));
-        display.SetColor(s_fadedZAxisColor);
+        display.SetColor(FadedZAxisColor);
         display.DrawLine(
             transform.GetTranslation(),
             transform.GetTranslation() + transform.GetBasisZ().GetNormalizedSafe() * axisLength * axisFlip(AZ::Vector3::CreateAxisZ()));
@@ -3416,11 +3424,11 @@ namespace AzToolsFramework
                     CalculatePivotTranslation(m_pivotOverrideFrame.m_pickedEntityIdOverride, m_pivotMode));
 
                 const float scaledSize =
-                    s_pivotSize * CalculateScreenToWorldMultiplier(pickedEntityWorldTransform.GetTranslation(), cameraState);
+                    PivotSize * CalculateScreenToWorldMultiplier(pickedEntityWorldTransform.GetTranslation(), cameraState);
 
                 debugDisplay.DepthWriteOff();
                 debugDisplay.DepthTestOff();
-                debugDisplay.SetColor(s_pickedOrientationColor);
+                debugDisplay.SetColor(PickedOrientationColor);
 
                 debugDisplay.DrawWireSphere(pickedEntityWorldTransform.GetTranslation(), scaledSize);
 
@@ -3462,7 +3470,7 @@ namespace AzToolsFramework
                     const AZ::Vector3 boxPosition = worldFromLocal.TransformPoint(CalculateCenterOffset(entityId, m_pivotMode));
 
                     const AZ::Vector3 scaledSize =
-                        AZ::Vector3(s_pivotSize) * CalculateScreenToWorldMultiplier(worldFromLocal.GetTranslation(), cameraState);
+                        AZ::Vector3(PivotSize) * CalculateScreenToWorldMultiplier(worldFromLocal.GetTranslation(), cameraState);
 
                     const AZ::Color hiddenNormal[] = { AzFramework::ViewportColors::SelectedColor,
                                                        AzFramework::ViewportColors::HiddenColor };
@@ -3708,7 +3716,7 @@ namespace AzToolsFramework
         if (m_entityIdManipulators.m_manipulators)
         {
             auto manipulatorCommand =
-                AZStd::make_unique<EntityManipulatorCommand>(CreateManipulatorCommandStateFromSelf(), s_manipulatorUndoRedoName);
+                AZStd::make_unique<EntityManipulatorCommand>(CreateManipulatorCommandStateFromSelf(), ManipulatorUndoRedoName);
 
             manipulatorCommand->SetManipulatorAfter(EntityManipulatorCommand::State());
 

Code/Framework/AzToolsFramework/Tests/EditorTransformComponentSelectionTests.cpp

@@ -866,9 +866,7 @@ namespace UnitTest
         EXPECT_THAT(selectedEntitiesAfter, UnorderedElementsAre(m_entityId1));
     }
 
-    TEST_F(
-        EditorTransformComponentSelectionViewportPickingManipulatorTestFixture,
-        BoxSelectWithNoInitialSelectionAddsEntitiesToSelection)
+    TEST_F(EditorTransformComponentSelectionViewportPickingManipulatorTestFixture, BoxSelectWithNoInitialSelectionAddsEntitiesToSelection)
     {
         AzToolsFramework::ed_viewportStickySelect = true;
 
@@ -989,6 +987,30 @@ namespace UnitTest
         EXPECT_TRUE(selectedEntitiesAfter.empty());
     }
 
+    TEST_F(EditorTransformComponentSelectionViewportPickingManipulatorTestFixture, UnstickyUndoOperationForChangeInSelectionIsAtomic)
+    {
+        AzToolsFramework::ed_viewportStickySelect = false;
+
+        PositionEntities();
+        PositionCamera(m_cameraState);
+
+        AzToolsFramework::SelectEntity(m_entityId1);
+
+        // calculate the position in screen space of the second entity
+        const auto entity2ScreenPosition = AzFramework::WorldToScreen(m_entity2WorldTranslation, m_cameraState);
+
+        // single click select entity2
+        m_actionDispatcher->CameraState(m_cameraState)->MousePosition(entity2ScreenPosition)->MouseLButtonDown()->MouseLButtonUp();
+
+        // undo action
+        AzToolsFramework::ToolsApplicationRequestBus::Broadcast(&AzToolsFramework::ToolsApplicationRequestBus::Events::UndoPressed);
+
+        // entity1 is selected after undo
+        using ::testing::UnorderedElementsAre;
+        auto selectedEntitiesAfter = SelectedEntities();
+        EXPECT_THAT(selectedEntitiesAfter, UnorderedElementsAre(m_entityId1));
+    }
+
     using EditorTransformComponentSelectionManipulatorTestFixture =
         IndirectCallManipulatorViewportInteractionFixtureMixin<EditorTransformComponentSelectionFixture>;
 

Code/LauncherUnified/Launcher.cpp

@@ -232,13 +232,17 @@ namespace
         // our frame time to be managed by AzGameFramework::GameApplication
         // instead, which probably isn't going to happen anytime soon given
         // how many things depend on the ITimer interface).
-        bool continueRunning = true;
         ISystem* system = gEnv ? gEnv->pSystem : nullptr;
-        while (continueRunning)
+        while (!gameApplication.WasExitMainLoopRequested())
         {
             // Pump the system event loop
             gameApplication.PumpSystemEventLoopUntilEmpty();
 
+            if (gameApplication.WasExitMainLoopRequested())
+            {
+                break;
+            }
+
             // Update the AzFramework system tick bus
             gameApplication.TickSystem();
 
@@ -256,9 +260,6 @@ namespace
             {
                 system->UpdatePostTickBus();
             }
-
-            // Check for quit requests
-            continueRunning = !gameApplication.WasExitMainLoopRequested() && continueRunning;
         }
     }
 }

Code/Legacy/CrySystem/AZCoreLogSink.h

@@ -31,6 +31,11 @@ class AZCoreLogSink
     : public AZ::Debug::TraceMessageBus::Handler
 {
 public:
+    ~AZCoreLogSink()
+    {
+        Disconnect();
+    }
+
     inline static void Connect()
     {
         GetInstance().m_ignoredAsserts = new IgnoredAssertMap();

Code/Legacy/CrySystem/LevelSystem/LevelSystem.cpp

@@ -233,6 +233,7 @@ CLevelSystem::CLevelSystem(ISystem* pSystem, const char* levelsFolder)
 //------------------------------------------------------------------------
 CLevelSystem::~CLevelSystem()
 {
+    UnloadLevel();
 }
 
 //------------------------------------------------------------------------

Code/Legacy/CrySystem/System.cpp

@@ -548,29 +548,6 @@ void CSystem::Quit()
         logger->Flush();
     }
 
-    /*
-    * TODO: This call to _exit, _Exit, TerminateProcess etc. needs to
-    * eventually be removed. This causes an extremely early exit before we
-    * actually perform cleanup. When this gets called most managers are
-    * simply never deleted and we leave it to the OS to clean up our mess
-    * which is just really bad practice. However there are LOTS of issues
-    * with shutdown at the moment. Removing this will simply cause
-    * a crash when either the Editor or Launcher initiate shutdown. Both
-    * applications crash differently too. Bugs will be logged about those
-    * issues.
-    */
-#if defined(AZ_RESTRICTED_PLATFORM)
-#define AZ_RESTRICTED_SECTION SYSTEM_CPP_SECTION_4
-#include AZ_RESTRICTED_FILE(System_cpp)
-#endif
-#if defined(AZ_RESTRICTED_SECTION_IMPLEMENTED)
-#undef AZ_RESTRICTED_SECTION_IMPLEMENTED
-#elif defined(WIN32) || defined(WIN64)
-    TerminateProcess(GetCurrentProcess(), m_env.retCode);
-#else
-    exit(m_env.retCode);
-#endif
-
 #ifdef WIN32
     //Post a WM_QUIT message to the Win32 api which causes the message loop to END
     //This is not the same as handling a WM_DESTROY event which destroys a window

Code/Tools/RemoteConsole/Core/RemoteConsoleCore.cpp

@@ -239,6 +239,11 @@ void SRemoteServer::Run()
 
     while (m_bAcceptClients)
     {
+        AZTIMEVAL timeout { 1, 0 };
+        if (!AZ::AzSock::IsRecvPending(m_socket, &timeout))
+        {
+            continue;
+        }
         AZ::AzSock::AzSocketAddress clientAddress;
         sClient = AZ::AzSock::Accept(m_socket, clientAddress);
         if (!m_bAcceptClients || !AZ::AzSock::IsAzSocketValid(sClient))

Code/Tools/Standalone/Source/Editor/LuaEditor.cpp

@@ -35,6 +35,10 @@ int main(int argc, char* argv[])
         {
             AZ::AllocatorInstance<AZ::OSAllocator>::Create();
         }
+        if (!AZ::AllocatorInstance<AZ::SystemAllocator>::IsReady())
+        {
+            AZ::AllocatorInstance<AZ::SystemAllocator>::Create();
+        }
 
         AZStd::unique_ptr<AZ::IO::LocalFileIO> fileIO = AZStd::unique_ptr<AZ::IO::LocalFileIO>(aznew AZ::IO::LocalFileIO());
         AZ::IO::FileIOBase::SetInstance(fileIO.get());
@@ -70,6 +74,10 @@ int main(int argc, char* argv[])
         // if its in GUI mode or not.
     }
 
+    if (AZ::AllocatorInstance<AZ::SystemAllocator>::IsReady())
+    {
+        AZ::AllocatorInstance<AZ::SystemAllocator>::Destroy();
+    }
     if (AZ::AllocatorInstance<AZ::OSAllocator>::IsReady())
     {
         AZ::AllocatorInstance<AZ::OSAllocator>::Destroy();

Gems/Atom/Asset/Shader/Code/CMakeLists.txt

@@ -65,6 +65,7 @@ ly_add_target(
             AZ::AzFramework
             AZ::AzToolsFramework
             Gem::Atom_RHI.Edit
+            Gem::Atom_RPI.Edit
             Gem::Atom_RPI.Public
 )
 

Gems/Atom/Asset/Shader/Code/Source/Editor/ShaderAssetBuilder.cpp

@@ -61,10 +61,99 @@ namespace AZ
         static constexpr char ShaderAssetBuilderName[] = "ShaderAssetBuilder";
         static constexpr uint32_t ShaderAssetBuildTimestampParam = 0;
 
+        //! The search will start in @currentFolderPath.
+        //! if the file is not found then it searches in order of appearence in @includeDirectories.
+        //! If the search yields no existing file it returns an empty string.
+        static AZStd::string DiscoverFullPath(AZStd::string_view normalizedRelativePath, AZStd::string_view currentFolderPath, const AZStd::vector<AZStd::string>& includeDirectories)
+        {
+            AZStd::string fullPath;
+            AzFramework::StringFunc::Path::Join(currentFolderPath.data(), normalizedRelativePath.data(), fullPath);
+            if (AZ::IO::SystemFile::Exists(fullPath.c_str()))
+            {
+                return fullPath;
+            }
+
+            for (const auto &includeDir : includeDirectories)
+            {
+                AzFramework::StringFunc::Path::Join(includeDir.c_str(), normalizedRelativePath.data(), fullPath);
+                if (AZ::IO::SystemFile::Exists(fullPath.c_str()))
+                {
+                    return fullPath;
+                }
+            }
+
+            return "";
+        }
+
+        // Appends to @includedFiles normalized paths of possible future locations of the file @normalizedRelativePath.
+        // The future locations are each directory listed in @includeDirectories joined with @normalizedRelativePath.
+        // This function is called when an included file doesn't exist but We need to declare source dependency so a .shader
+        // asset is rebuilt when the missing file appears in the future.
+        static void AppendListOfPossibleFutureLocations(AZStd::unordered_set<AZStd::string>& includedFiles, AZStd::string_view normalizedRelativePath, AZStd::string_view currentFolderPath, const AZStd::vector<AZStd::string>& includeDirectories)
+        {
+            AZStd::string fullPath;
+            AzFramework::StringFunc::Path::Join(currentFolderPath.data(), normalizedRelativePath.data(), fullPath);
+            includedFiles.insert(fullPath);
+            for (const auto &includeDir : includeDirectories)
+            {
+                AzFramework::StringFunc::Path::Join(includeDir.c_str(), normalizedRelativePath.data(), fullPath);
+                includedFiles.insert(fullPath);
+            }
+        }
+
+        //! Parses, using depth-first recursive approach, azsl files. Looks for '#include <foo/bar/blah.h>' or '#include "foo/bar/blah.h"' lines
+        //! and in turn parses the included files.
+        //! The included files are searched in the directories listed in @includeDirectories. Basically it's a similar approach
+        //! as how most C-preprocessors would find included files.
+        static void GetListOfIncludedFiles(AZStd::string_view sourceFilePath, const AZStd::vector<AZStd::string>& includeDirectories,
+            const ShaderBuilderUtility::IncludedFilesParser& includedFilesParser, AZStd::unordered_set<AZStd::string>& includedFiles)
+        {
+            auto outcome = includedFilesParser.ParseFileAndGetIncludedFiles(sourceFilePath);
+            if (!outcome.IsSuccess())
+            {
+                AZ_Warning(ShaderAssetBuilderName, false, outcome.GetError().c_str());
+                return;
+            }
+
+            // Cache the path of the folder where @sourceFilePath is located.
+            AZStd::string sourceFileFolderPath;
+            {
+                AZStd::string drive;
+                AzFramework::StringFunc::Path::Split(sourceFilePath.data(), &drive, &sourceFileFolderPath);
+                if (!drive.empty())
+                {
+                    AzFramework::StringFunc::Path::Join(drive.c_str(), sourceFileFolderPath.c_str(), sourceFileFolderPath);
+                }
+            }
+
+            auto listOfRelativePaths = outcome.TakeValue();
+            for (auto relativePath : listOfRelativePaths)
+            {
+                auto fullPath = DiscoverFullPath(relativePath, sourceFileFolderPath, includeDirectories);
+                if (fullPath.empty())
+                {
+                    // The file doesn't exist in any of the includeDirectories. It doesn't exist in @sourceFileFolderPath either.
+                    // The file may appear in the future in one of those directories, We must build an exhaustive list
+                    // of full file paths where the file may appear in the future.
+                    AppendListOfPossibleFutureLocations(includedFiles, relativePath, sourceFileFolderPath, includeDirectories);
+                    continue;
+                }
+
+                // Add the file to the list and keep parsing recursively.
+                if (includedFiles.count(fullPath))
+                {
+                    continue;
+                }
+                includedFiles.insert(fullPath);
+                GetListOfIncludedFiles(fullPath, includeDirectories, includedFilesParser, includedFiles);
+            }
+        }
+
         void ShaderAssetBuilder::CreateJobs(const AssetBuilderSDK::CreateJobsRequest& request, AssetBuilderSDK::CreateJobsResponse& response) const
         {
             AZStd::string fullPath;
             AzFramework::StringFunc::Path::ConstructFull(request.m_watchFolder.data(), request.m_sourceFile.data(), fullPath, true);
+            ShaderBuilderUtility::IncludedFilesParser includedFilesParser;
 
             AZ_TracePrintf(ShaderAssetBuilderName, "CreateJobs for Shader \"%s\"\n", fullPath.data());
 
@@ -90,36 +179,6 @@ namespace AZ
 
             AZStd::string azslFullPath;
             ShaderBuilderUtility::GetAbsolutePathToAzslFile(fullPath, shaderSourceData.m_source, azslFullPath);
-            if (!IO::FileIOBase::GetInstance()->Exists(azslFullPath.c_str()))
-            {
-                AZ_Error(
-                    ShaderAssetBuilderName, false, "Shader program listed as the source entry does not exist: %s.", azslFullPath.c_str());
-                response.m_result = AssetBuilderSDK::CreateJobsResultCode::Failed;
-                return;
-            }
-
-
-            GlobalBuildOptions buildOptions = ReadBuildOptions(ShaderAssetBuilderName);
-
-            // [GFX TODO] [ATOM-14966] In principle, based on macro definitions, included files can change per supervariant.
-            //                         So, the list of source asset dependencies must be collected by running MCPP on each supervariant.
-            //                         For now, we will run MCPP only once because CreateJobs() should be as light as possible.
-            // 
-            // Regardless of the PlatformInfo and enabled ShaderPlatformInterfaces, the azsl file will be preprocessed
-            // with the sole purpose of extracting all included files. For each included file a SourceDependency will be declared.
-            PreprocessorData output;
-            buildOptions.m_compilerArguments.Merge(shaderSourceData.m_compiler);
-            PreprocessFile(azslFullPath, output, buildOptions.m_preprocessorSettings, true, true);
-            for (auto includePath : output.includedPaths)
-            {
-                // m_sourceFileDependencyList does not support paths with "." or ".." for relative lookup, but the preprocessor
-                // may produce path strings like "C:/a/b/c/../../d/file.azsli" so we have to normalize
-                AzFramework::StringFunc::Path::Normalize(includePath);
-
-                AssetBuilderSDK::SourceFileDependency includeFileDependency;
-                includeFileDependency.m_sourceFileDependencyPath = includePath;
-                response.m_sourceFileDependencyList.emplace_back(includeFileDependency);
-            }
 
             {
                 // Add the AZSL as source dependency
@@ -128,6 +187,26 @@ namespace AZ
                 response.m_sourceFileDependencyList.emplace_back(azslFileDependency);
             }
 
+            if (!IO::FileIOBase::GetInstance()->Exists(azslFullPath.c_str()))
+            {
+                AZ_Error(
+                    ShaderAssetBuilderName, false, "Shader program listed as the source entry does not exist: %s.", azslFullPath.c_str());
+                // Treat as success, so when the azsl file shows up the AP will try to recompile.
+                response.m_result = AssetBuilderSDK::CreateJobsResultCode::Success;
+                return;
+            }
+
+            GlobalBuildOptions buildOptions = ReadBuildOptions(ShaderAssetBuilderName);
+
+            AZStd::unordered_set<AZStd::string> includedFiles;
+            GetListOfIncludedFiles(azslFullPath, buildOptions.m_preprocessorSettings.m_projectIncludePaths, includedFilesParser, includedFiles);
+            for (auto includePath : includedFiles)
+            {
+                AssetBuilderSDK::SourceFileDependency includeFileDependency;
+                includeFileDependency.m_sourceFileDependencyPath = includePath;
+                response.m_sourceFileDependencyList.emplace_back(includeFileDependency);
+            }
+
             for (const AssetBuilderSDK::PlatformInfo& platformInfo : request.m_enabledPlatforms)
             {
                 AZ_TraceContext("For platform", platformInfo.m_identifier.data());
@@ -149,6 +228,10 @@ namespace AZ
                 response.m_createJobOutputs.push_back(jobDescriptor);
             }  // for all request.m_enabledPlatforms
 
+            const AZStd::sys_time_t createJobsEndStamp = AZStd::GetTimeNowMicroSecond();
+            const u64 createJobDurationMicros = createJobsEndStamp - shaderAssetBuildTimestamp;
+            AZ_TracePrintf(ShaderAssetBuilderName, "CreateJobs for %s took %llu microseconds", fullPath.c_str(), createJobDurationMicros );
+
             response.m_result = AssetBuilderSDK::CreateJobsResultCode::Success;
         }
 
@@ -286,6 +369,13 @@ namespace AZ
                     return;
                 }
             }
+            else
+            {
+                // CreateJobs was not successful if there's no timestamp property in m_jobParameters.
+                response.m_resultCode = AssetBuilderSDK::ProcessJobResult_Failed;
+                AZ_Assert(false, "Missing ShaderAssetBuildTimestampParam");
+                return;
+            }
 
             auto supervariantList = ShaderBuilderUtility::GetSupervariantListFromShaderSourceData(shaderSourceData);
 

Gems/Atom/Asset/Shader/Code/Source/Editor/ShaderBuilderUtility.cpp

@@ -20,6 +20,7 @@
 #include <AzCore/IO/IOUtils.h>
 #include <AzCore/IO/FileIO.h>
 #include <AzCore/Settings/SettingsRegistryMergeUtils.h>
+#include <AzCore/std/string/regex.h>
 
 #include <AzCore/Serialization/Json/JsonUtils.h>
 
@@ -814,6 +815,51 @@ namespace AZ
                 return success;
             }
 
+            IncludedFilesParser::IncludedFilesParser()
+            {
+                AZStd::regex regex(R"(#\s*include\s+[<|"]([\w|/|\\|\.|-]+)[>|"])", AZStd::regex::ECMAScript);
+                m_includeRegex.swap(regex);
+            }
+
+            AZStd::vector<AZStd::string> IncludedFilesParser::ParseStringAndGetIncludedFiles(AZStd::string_view haystack) const
+            {
+                AZStd::vector<AZStd::string> listOfFilePaths;
+                AZStd::smatch match;
+                AZStd::string::const_iterator searchStart(haystack.cbegin());
+                while (AZStd::regex_search(searchStart, haystack.cend(), match, m_includeRegex))
+                {
+                    if (match.size() > 1)
+                    {
+                        AZStd::string relativeFilePath(match[1].str().c_str());
+                        AzFramework::StringFunc::Path::Normalize(relativeFilePath);
+                        listOfFilePaths.push_back(relativeFilePath);
+                    }
+                    searchStart = match.suffix().first;
+                }
+                return listOfFilePaths;
+            }
+
+            AZ::Outcome<AZStd::vector<AZStd::string>, AZStd::string> IncludedFilesParser::ParseFileAndGetIncludedFiles(AZStd::string_view sourceFilePath) const
+            {
+                AZ::IO::FileIOStream stream(sourceFilePath.data(), AZ::IO::OpenMode::ModeRead);
+                if (!stream.IsOpen())
+                {
+                    return AZ::Failure(AZStd::string::format("\"%s\" source file could not be opened.", sourceFilePath.data()));
+                }
+
+                if (!stream.CanRead())
+                {
+                    return AZ::Failure(AZStd::string::format("\"%s\" source file could not be read.", sourceFilePath.data()));
+                }
+
+                AZStd::string hayStack;
+                hayStack.resize_no_construct(stream.GetLength());
+                stream.Read(stream.GetLength(), hayStack.data());
+
+                auto listOfFilePaths = ParseStringAndGetIncludedFiles(hayStack);
+                return AZ::Success(AZStd::move(listOfFilePaths));
+            }
+
         }  // namespace ShaderBuilderUtility
     } // namespace ShaderBuilder
 } // AZ

Gems/Atom/Asset/Shader/Code/Source/Editor/ShaderBuilderUtility.h

@@ -141,6 +141,29 @@ namespace AZ
                 const uint32_t rhiUniqueIndex, const AZStd::string& platformIdentifier, const AZStd::string& shaderJsonPath,
                 const uint32_t supervariantIndex, RPI::ShaderAssetSubId shaderAssetSubId);
 
+
+            class IncludedFilesParser
+            {
+            public:
+                IncludedFilesParser();
+                ~IncludedFilesParser() = default;
+
+                //! This static function was made public for testability purposes only.
+                //! Parses the string @haystack, looking for "#include file" lines with a regular expression.
+                //! Returns the list of relative paths as included by the file.
+                //! REMARK: The algorithm may over prescribe what files to include because it doesn't discern between comments, etc.
+                //!         Also, a #include line may be protected by #ifdef macros but this algorithm doesn't care.
+                //! Over prescribing is not a real problem, albeit potential waste in processing. Under prescribing would be a real problem.
+                AZStd::vector<AZStd::string> ParseStringAndGetIncludedFiles(AZStd::string_view haystack) const;
+
+                //! This static function was made public for testability purposes only.
+                //! Opens the file @sourceFilePath, loads the content into a string and returns ParseStringAndGetIncludedFiles(content)
+                AZ::Outcome<AZStd::vector<AZStd::string>, AZStd::string> ParseFileAndGetIncludedFiles(AZStd::string_view sourceFilePath) const;
+
+            private:
+                AZStd::regex m_includeRegex;
+            };
+
         }  // ShaderBuilderUtility namespace
     } // ShaderBuilder namespace
 } // AZ

Gems/Atom/Asset/Shader/Code/Tests/ShaderBuilderUtilityTests.cpp

@@ -0,0 +1,86 @@
+/*
+ * Copyright (c) Contributors to the Open 3D Engine Project.
+ * For complete copyright and license terms please see the LICENSE at the root of this distribution.
+ *
+ * SPDX-License-Identifier: Apache-2.0 OR MIT
+ *
+ */
+
+#include <AzTest/AzTest.h>
+#include <AzCore/UnitTest/TestTypes.h>
+
+#include "Common/ShaderBuilderTestFixture.h"
+
+#include <ShaderBuilderUtility.h>
+
+namespace UnitTest
+{
+    using namespace AZ;
+
+    // The main purpose of this class is to test ShaderBuilderUtility functions
+    class ShaderBuilderUtilityTests : public ShaderBuilderTestFixture
+    {
+    }; // class ShaderBuilderUtilityTests
+
+
+    TEST_F(ShaderBuilderUtilityTests, IncludedFilesParser_ParseStringAndGetIncludedFiles)
+    {
+        AZStd::string haystack(
+            "Some content to parse\n"
+            "#include <valid_file1.azsli>\n"
+            "// #include <valid_file2.azsli>\n"
+            "blah # include \"valid_file3.azsli\"\n"
+            "bar include <a\\dire-ctory\\invalid-file4.azsli>\n"
+            "foo #   include \"a/directory/valid-file5.azsli\"\n"
+            "# include <a\\dire-ctory\\valid-file6.azsli>\n"
+            "#includ \"a\\dire-ctory\\invalid-file7.azsli\"\n"
+        );
+
+        AZ::ShaderBuilder::ShaderBuilderUtility::IncludedFilesParser includedFilesParser;
+        auto fileList = includedFilesParser.ParseStringAndGetIncludedFiles(haystack);
+        EXPECT_EQ(fileList.size(), 5);
+
+        auto it = AZStd::find(fileList.begin(), fileList.end(), "valid_file1.azsli");
+        EXPECT_TRUE(it != fileList.end());
+
+        it = AZStd::find(fileList.begin(), fileList.end(), "valid_file2.azsli");
+        EXPECT_TRUE(it != fileList.end());
+
+        it = AZStd::find(fileList.begin(), fileList.end(), "valid_file3.azsli");
+        EXPECT_TRUE(it != fileList.end());
+
+        // Remark: From now on We must normalize because internally AZ::ShaderBuilder::ShaderBuilderUtility::IncludedFilesParser
+        // always returns normalized paths.
+        {
+            AZStd::string fileName("a\\dire-ctory\\invalid-file4.azsli");
+            AzFramework::StringFunc::Path::Normalize(fileName);
+            it = AZStd::find(fileList.begin(), fileList.end(), fileName);
+            EXPECT_TRUE(it == fileList.end());
+        }
+
+        {
+            AZStd::string fileName("a\\directory\\valid-file5.azsli");
+            AzFramework::StringFunc::Path::Normalize(fileName);
+            it = AZStd::find(fileList.begin(), fileList.end(), fileName);
+            EXPECT_TRUE(it != fileList.end());
+        }
+
+        {
+            AZStd::string fileName("a\\dire-ctory\\valid-file6.azsli");
+            AzFramework::StringFunc::Path::Normalize(fileName);
+            it = AZStd::find(fileList.begin(), fileList.end(), fileName);
+            EXPECT_TRUE(it != fileList.end());
+        }
+
+        {
+            AZStd::string fileName("a\\dire-ctory\\invalid-file7.azsli");
+            AzFramework::StringFunc::Path::Normalize(fileName);
+            it = AZStd::find(fileList.begin(), fileList.end(), fileName);
+            EXPECT_TRUE(it == fileList.end());
+        }
+    }
+
+} //namespace UnitTest
+
+//AZ_UNIT_TEST_HOOK(DEFAULT_UNIT_TEST_ENV);
+

Gems/Atom/Asset/Shader/Code/atom_asset_shader_builders_tests_files.cmake

@@ -11,4 +11,5 @@ set(FILES
     Tests/Common/ShaderBuilderTestFixture.cpp
     Tests/SupervariantCmdArgumentTests.cpp
     Tests/McppBinderTests.cpp
+    Tests/ShaderBuilderUtilityTests.cpp
 )

Gems/Atom/Bootstrap/Code/Source/BootstrapSystemComponent.cpp

@@ -386,7 +386,7 @@ namespace AZ
                     // Unbind m_defaultScene to the GameEntityContext's AzFramework::Scene
                     if (m_defaultFrameworkScene)
                     {
-                        m_defaultFrameworkScene->UnsetSubsystem<RPI::Scene>();
+                        m_defaultFrameworkScene->UnsetSubsystem(m_defaultScene);
                     }
 
                     m_defaultScene = nullptr;

Gems/Atom/Feature/Common/Assets/Shaders/PostProcessing/FastDepthAwareBlurHor.azsl

@@ -55,7 +55,7 @@ int GetLdsIndex(int2 ldsPosition)
 
 // --- Common file start ---
 
-// #include <Atom/Features/PostProcessing/FastDepthAwareBlurCommon.azsli>
+// include <Atom/Features/PostProcessing/FastDepthAwareBlurCommon.azsli> ('#' symbol before 'include' was removed on purpose to avoid parsing this azsli file during ShaderAssetBuilder::CreateJobs)
 // This include fails with the asset processor when generating the .shader for this file
 // Everything below this is copy pasted from FastDepthAwareBlurCommon.azsli up until the
 // "Common file end" marker below

Gems/Atom/Feature/Common/Assets/Shaders/PostProcessing/FastDepthAwareBlurVer.azsl

@@ -55,7 +55,7 @@ int GetLdsIndex(int2 ldsPosition)
 
 // --- Common file start ---
 
-// #include <Atom/Features/PostProcessing/FastDepthAwareBlurCommon.azsli>
+// include <Atom/Features/PostProcessing/FastDepthAwareBlurCommon.azsli> ('#' symbol before 'include' was removed on purpose to avoid parsing this azsli file during ShaderAssetBuilder::CreateJobs)
 // This include fails with the asset processor when generating the .shader for this file
 // Everything below this is copy pasted from FastDepthAwareBlurCommon.azsli up until the
 // "Common file end" marker below

Gems/Atom/Feature/Common/Assets/Shaders/PostProcessing/SMAA.azsli

@@ -157,7 +157,7 @@
  *         #define SMAA_RT_METRICS float4(1.0 / 1280.0, 1.0 / 720.0, 1280.0, 720.0)
  *         #define SMAA_HLSL_4
  *         #define SMAA_PRESET_HIGH
- *         #include "SMAA.h"
+ *         include "SMAA.h" ('#' symbol before 'include' was removed on purpose to avoid parsing this azsli file during ShaderAssetBuilder::CreateJobs)
  *
  *     Note that SMAA_RT_METRICS doesn't need to be a macro, it can be a
  *     uniform variable. The code is designed to minimize the impact of not

Gems/AtomLyIntegration/CommonFeatures/Assets/PostProcess/default.postfxlayercategories

@@ -1,17 +1,30 @@
 <ObjectStream version="3">
 	<Class name="EditorPostFxLayerCategoriesAsset" type="{A18B1B11-4C1E-4C1B-9643-178E8ED27019}">
 		<Class name="AZStd::map" field="Layer Categories" type="{5B4970CC-26DE-51A3-83E7-1BC3A7E8E3C3}">
+
 			<Class name="AZStd::pair" field="element" type="{279D3EEE-F9CE-57FA-986E-D24E57211795}">
-				<Class name="AZStd::string" field="value1" value="Camera" type="{03AAAB3F-5C47-5A66-9EBC-D5FA4DB353C9}"/>
-				<Class name="int" field="value2" value="100" type="{72039442-EB38-4D42-A1AD-CB68F7E0EEF6}"/>
+				<Class name="AZStd::string" field="value1" value="FrontEnd" type="{03AAAB3F-5C47-5A66-9EBC-D5FA4DB353C9}"/>
+				<Class name="int" field="value2" value="1000000" type="{72039442-EB38-4D42-A1AD-CB68F7E0EEF6}"/>
 			</Class>
 			<Class name="AZStd::pair" field="element" type="{279D3EEE-F9CE-57FA-986E-D24E57211795}">
-				<Class name="AZStd::string" field="value1" value="Level" type="{03AAAB3F-5C47-5A66-9EBC-D5FA4DB353C9}"/>
-				<Class name="int" field="value2" value="1000" type="{72039442-EB38-4D42-A1AD-CB68F7E0EEF6}"/>
+				<Class name="AZStd::string" field="value1" value="Cinematics" type="{03AAAB3F-5C47-5A66-9EBC-D5FA4DB353C9}"/>
+				<Class name="int" field="value2" value="2000000" type="{72039442-EB38-4D42-A1AD-CB68F7E0EEF6}"/>
+			</Class>
+			<Class name="AZStd::pair" field="element" type="{279D3EEE-F9CE-57FA-986E-D24E57211795}">
+				<Class name="AZStd::string" field="value1" value="Gameplay" type="{03AAAB3F-5C47-5A66-9EBC-D5FA4DB353C9}"/>
+				<Class name="int" field="value2" value="3000000" type="{72039442-EB38-4D42-A1AD-CB68F7E0EEF6}"/>
+			</Class>
+			<Class name="AZStd::pair" field="element" type="{279D3EEE-F9CE-57FA-986E-D24E57211795}">
+				<Class name="AZStd::string" field="value1" value="Camera" type="{03AAAB3F-5C47-5A66-9EBC-D5FA4DB353C9}"/>
+				<Class name="int" field="value2" value="4000000" type="{72039442-EB38-4D42-A1AD-CB68F7E0EEF6}"/>
 			</Class>
 			<Class name="AZStd::pair" field="element" type="{279D3EEE-F9CE-57FA-986E-D24E57211795}">
 				<Class name="AZStd::string" field="value1" value="Volume" type="{03AAAB3F-5C47-5A66-9EBC-D5FA4DB353C9}"/>
-				<Class name="int" field="value2" value="10" type="{72039442-EB38-4D42-A1AD-CB68F7E0EEF6}"/>
+				<Class name="int" field="value2" value="5000000" type="{72039442-EB38-4D42-A1AD-CB68F7E0EEF6}"/>
+			</Class>
+			<Class name="AZStd::pair" field="element" type="{279D3EEE-F9CE-57FA-986E-D24E57211795}">
+				<Class name="AZStd::string" field="value1" value="Level" type="{03AAAB3F-5C47-5A66-9EBC-D5FA4DB353C9}"/>
+				<Class name="int" field="value2" value="6000000" type="{72039442-EB38-4D42-A1AD-CB68F7E0EEF6}"/>
 			</Class>
 		</Class>
 	</Class>

Gems/HttpRequestor/Code/Source/HttpRequestManager.cpp

@@ -35,7 +35,6 @@ namespace HttpRequestor
         desc.m_name = s_loggingName;
         desc.m_cpuId = AFFINITY_MASK_USERTHREADS;
         m_runThread = true;
-        // Shutdown will be handled by the InitializationManager - no need to call in the destructor
         AWSNativeSDKInit::InitializationManager::InitAwsApi();
         auto function = AZStd::bind(&Manager::ThreadFunction, this);
         m_thread = AZStd::thread(function, &desc);
@@ -43,7 +42,7 @@ namespace HttpRequestor
 
     Manager::~Manager()
     {
-        // NativeSDK Shutdown does not need to be called here - will be taken care of by the InitializationManager
+        AWSNativeSDKInit::InitializationManager::Shutdown();
         m_runThread = false;
         m_requestConditionVar.notify_all();
         if (m_thread.joinable())

Gems/PhysX/Code/Tests/PhysXTestUtil.h

@@ -93,9 +93,26 @@ namespace PhysX
 
         ////////////////////////////////////////////////////////////////////////
         // TerrainDataRequestBus interface dummy implementation
-        AZ::Vector2 GetTerrainGridResolution() const override { return {}; }
-        AZ::Aabb GetTerrainAabb() const override { return {}; }
-        float GetHeight(AZ::Vector3, Sampler, bool*) const override { return {}; }
+        AZ::Vector2 GetTerrainHeightQueryResolution() const override
+        {
+            return {};
+        }
+        void SetTerrainHeightQueryResolution([[maybe_unused]] AZ::Vector2 queryResolution) override
+        {
+        }
+
+        AZ::Aabb GetTerrainAabb() const override
+        {
+            return {};
+        }
+        void SetTerrainAabb([[maybe_unused]] const AZ::Aabb& worldBounds) override
+        {
+        }
+
+        float GetHeight(AZ::Vector3, Sampler, bool*) const override
+        {
+            return {};
+        }
         float GetHeightFromFloats(float, float, Sampler, bool*) const override { return {}; }
         AzFramework::SurfaceData::SurfaceTagWeight GetMaxSurfaceWeight(AZ::Vector3, Sampler, bool*) const override { return {}; }
         AzFramework::SurfaceData::SurfaceTagWeight GetMaxSurfaceWeightFromFloats(float, float, Sampler, bool*) const override { return {}; }

Gems/Terrain/Assets/Shaders/Terrain/TerrainCommon.azsli

@@ -11,16 +11,16 @@ ShaderResourceGroup ObjectSrg : SRG_PerObject
 {
     Texture2D<float> m_heightmapImage;
     
-    Sampler LinearSampler
+    Sampler PointSampler
     {
-        MinFilter = Linear;
-        MagFilter = Linear;
-        MipFilter = Linear;
+        MinFilter = Point;
+        MagFilter = Point;
+        MipFilter = Point;
         AddressU = Clamp;
         AddressV = Clamp;
         AddressW = Clamp;
     };
-    
+
     row_major float3x4 m_modelToWorld;
 
     struct TerrainData
@@ -57,8 +57,8 @@ float4x4 GetObject_WorldMatrix()
 
 float GetHeight(float2 origUv)
 {
-    float2 uv = clamp(origUv, 0.0f, 1.0f);
-    return ObjectSrg::m_terrainData.m_heightScale * (ObjectSrg::m_heightmapImage.SampleLevel(ObjectSrg::LinearSampler, uv, 0).r - 0.5f);
+    float2 uv = clamp(origUv + (ObjectSrg::m_terrainData.m_uvStep * 0.5f), 0.0f, 1.0f);
+    return ObjectSrg::m_terrainData.m_heightScale * (ObjectSrg::m_heightmapImage.SampleLevel(ObjectSrg::PointSampler, uv, 0).r - 0.5f);
 }
 
 float4 GetTerrainProjectedPosition(ObjectSrg::TerrainData terrainData, float2 vertexPosition, float2 uv)

Gems/Terrain/Code/CMakeLists.txt

@@ -83,15 +83,36 @@ endif()
 ################################################################################
 # See if globally, tests are supported
 if(PAL_TRAIT_BUILD_TESTS_SUPPORTED)
-    # We globally support tests, see if we support tests on this platform for Terrain.Static
-    if(PAL_TRAIT_TERRAIN_TEST_SUPPORTED)
-        # We support Terrain.Tests on this platform, add Terrain.Tests target which depends on Terrain.Static
+    ly_add_target(
+        NAME Terrain.Tests ${PAL_TRAIT_TEST_TARGET_TYPE}
+        NAMESPACE Gem
+        FILES_CMAKE
+            terrain_files.cmake
+            terrain_tests_files.cmake
+        INCLUDE_DIRECTORIES
+            PRIVATE
+                Tests
+                Source
+        BUILD_DEPENDENCIES
+            PRIVATE
+                AZ::AzTest
+                AZ::AzFramework
+                Gem::Terrain.Static
+    )
+
+    # Add Terrain.Tests to googletest
+    ly_add_googletest(
+        NAME Gem::Terrain.Tests
+    )
+
+    # If we are a host platform we want to add tools test like editor tests here
+    if(PAL_TRAIT_BUILD_HOST_TOOLS)
+        # We support Terrain.Editor.Tests on this platform, add Terrain.Editor.Tests target which depends on Terrain.Editor
         ly_add_target(
-            NAME Terrain.Tests ${PAL_TRAIT_TEST_TARGET_TYPE}
+            NAME Terrain.Editor.Tests ${PAL_TRAIT_TEST_TARGET_TYPE}
             NAMESPACE Gem
             FILES_CMAKE
-                terrain_files.cmake
-                terrain_tests_files.cmake
+                terrain_editor_tests_files.cmake
             INCLUDE_DIRECTORIES
                 PRIVATE
                     Tests
@@ -99,40 +120,12 @@ if(PAL_TRAIT_BUILD_TESTS_SUPPORTED)
             BUILD_DEPENDENCIES
                 PRIVATE
                     AZ::AzTest
-                    AZ::AzFramework
-                    Gem::Terrain.Static
+                    Gem::Terrain.Editor
         )
 
-        # Add Terrain.Tests to googletest
+        # Add Terrain.Editor.Tests to googletest
         ly_add_googletest(
-            NAME Gem::Terrain.Tests
+            NAME Gem::Terrain.Editor.Tests
         )
     endif()
-
-    # If we are a host platform we want to add tools test like editor tests here
-    if(PAL_TRAIT_BUILD_HOST_TOOLS)
-        # We are a host platform, see if Editor tests are supported on this platform
-        if(PAL_TRAIT_TERRAIN_EDITOR_TEST_SUPPORTED)
-            # We support Terrain.Editor.Tests on this platform, add Terrain.Editor.Tests target which depends on Terrain.Editor
-            ly_add_target(
-                NAME Terrain.Editor.Tests ${PAL_TRAIT_TEST_TARGET_TYPE}
-                NAMESPACE Gem
-                FILES_CMAKE
-                    terrain_editor_tests_files.cmake
-                INCLUDE_DIRECTORIES
-                    PRIVATE
-                        Tests
-                        Source
-                BUILD_DEPENDENCIES
-                    PRIVATE
-                        AZ::AzTest
-                        Gem::Terrain.Editor
-            )
-
-            # Add Terrain.Editor.Tests to googletest
-            ly_add_googletest(
-                NAME Gem::Terrain.Editor.Tests
-            )
-        endif()
-    endif()
 endif()

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

@@ -166,14 +166,20 @@ namespace Terrain
     }
 
     void TerrainHeightGradientListComponent::GetHeight(
-        const AZ::Vector3& inPosition, AZ::Vector3& outPosition, [[maybe_unused]] Sampler sampleFilter = Sampler::DEFAULT)
+        const AZ::Vector3& inPosition,
+        AZ::Vector3& outPosition,
+        [[maybe_unused]] AzFramework::Terrain::TerrainDataRequests::Sampler sampleFilter =
+            AzFramework::Terrain::TerrainDataRequests::Sampler::DEFAULT)
     {
         const float height = GetHeight(inPosition.GetX(), inPosition.GetY());
         outPosition.SetZ(height);
     }
 
     void TerrainHeightGradientListComponent::GetNormal(
-        const AZ::Vector3& inPosition, AZ::Vector3& outNormal, [[maybe_unused]] Sampler sampleFilter = Sampler::DEFAULT)
+        const AZ::Vector3& inPosition,
+        AZ::Vector3& outNormal,
+        [[maybe_unused]] AzFramework::Terrain::TerrainDataRequests::Sampler sampleFilter =
+            AzFramework::Terrain::TerrainDataRequests::Sampler::DEFAULT)
     {
         const float x = inPosition.GetX();
         const float y = inPosition.GetY();
@@ -206,7 +212,7 @@ namespace Terrain
         // Get the height range of the entire world
         m_cachedHeightQueryResolution = AZ::Vector2(1.0f);
         AzFramework::Terrain::TerrainDataRequestBus::BroadcastResult(
-            m_cachedHeightQueryResolution, &AzFramework::Terrain::TerrainDataRequestBus::Events::GetTerrainGridResolution);
+            m_cachedHeightQueryResolution, &AzFramework::Terrain::TerrainDataRequestBus::Events::GetTerrainHeightQueryResolution);
 
         AZ::Aabb worldBounds = AZ::Aabb::CreateNull();
         AzFramework::Terrain::TerrainDataRequestBus::BroadcastResult(

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

@@ -64,8 +64,14 @@ namespace Terrain
         TerrainHeightGradientListComponent() = default;
         ~TerrainHeightGradientListComponent() = default;
 
-        void GetHeight(const AZ::Vector3& inPosition, AZ::Vector3& outPosition, Sampler sampleFilter) override;
-        void GetNormal(const AZ::Vector3& inPosition, AZ::Vector3& outNormal, Sampler sampleFilter) override;
+        void GetHeight(
+            const AZ::Vector3& inPosition,
+            AZ::Vector3& outPosition,
+            AzFramework::Terrain::TerrainDataRequests::Sampler sampleFilter) override;
+        void GetNormal(
+            const AZ::Vector3& inPosition,
+            AZ::Vector3& outNormal,
+            AzFramework::Terrain::TerrainDataRequests::Sampler sampleFilter) override;
 
         //////////////////////////////////////////////////////////////////////////
         // AZ::Component interface implementation

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

@@ -104,7 +104,6 @@ namespace Terrain
     {
         AZ::TransformNotificationBus::Handler::BusConnect(GetEntityId());
         LmbrCentral::ShapeComponentNotificationsBus::Handler::BusConnect(GetEntityId());
-        TerrainAreaRequestBus::Handler::BusConnect(GetEntityId());
         TerrainSpawnerRequestBus::Handler::BusConnect(GetEntityId());
 
         TerrainSystemServiceRequestBus::Broadcast(&TerrainSystemServiceRequestBus::Events::RegisterArea, GetEntityId());
@@ -114,7 +113,6 @@ namespace Terrain
     {
         TerrainSystemServiceRequestBus::Broadcast(&TerrainSystemServiceRequestBus::Events::UnregisterArea, GetEntityId());
         TerrainSpawnerRequestBus::Handler::BusDisconnect();
-        TerrainAreaRequestBus::Handler::BusDisconnect();
         LmbrCentral::ShapeComponentNotificationsBus::Handler::BusDisconnect();
         AZ::TransformNotificationBus::Handler::BusDisconnect();
         
@@ -161,11 +159,6 @@ namespace Terrain
         return m_configuration.m_useGroundPlane;
     }
 
-    void TerrainLayerSpawnerComponent::RegisterArea()
-    {
-        TerrainSystemServiceRequestBus::Broadcast(&TerrainSystemServiceRequestBus::Events::RegisterArea, GetEntityId());
-    }
-
     void TerrainLayerSpawnerComponent::RefreshArea()
     {
         TerrainSystemServiceRequestBus::Broadcast(&TerrainSystemServiceRequestBus::Events::RefreshArea, GetEntityId());

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

@@ -58,7 +58,6 @@ namespace Terrain
         : public AZ::Component
         , private AZ::TransformNotificationBus::Handler
         , private LmbrCentral::ShapeComponentNotificationsBus::Handler
-        , private Terrain::TerrainAreaRequestBus::Handler
         , private Terrain::TerrainSpawnerRequestBus::Handler
     {
     public:
@@ -81,6 +80,7 @@ namespace Terrain
         bool ReadInConfig(const AZ::ComponentConfig* baseConfig) override;
         bool WriteOutConfig(AZ::ComponentConfig* outBaseConfig) const override;
 
+    protected:
         //////////////////////////////////////////////////////////////////////////
         // AZ::TransformNotificationBus::Handler
         void OnTransformChanged(const AZ::Transform& local, const AZ::Transform& world) override;
@@ -92,8 +92,7 @@ namespace Terrain
         void GetPriority(AZ::u32& outLayer, AZ::u32& outPriority) override;
         bool GetUseGroundPlane() override;
         
-        void RegisterArea() override;
-        void RefreshArea() override;
+        void RefreshArea();
 
     private:
         TerrainLayerSpawnerConfig m_configuration;

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

@@ -13,6 +13,7 @@
 #include <AzCore/RTTI/BehaviorContext.h>
 #include <AzCore/Serialization/EditContext.h>
 #include <AzCore/Serialization/SerializeContext.h>
+#include <AzFramework/Terrain/TerrainDataRequestBus.h>
 
 namespace Terrain
 {
@@ -85,17 +86,16 @@ namespace Terrain
 
     void TerrainWorldComponent::Activate()
     {
-        TerrainSystemServiceRequestBus::Broadcast(
-            &TerrainSystemServiceRequestBus::Events::SetWorldBounds,
-            AZ::Aabb::CreateFromMinMax(m_configuration.m_worldMin, m_configuration.m_worldMax)
-        );
-        TerrainSystemServiceRequestBus::Broadcast(
-            &TerrainSystemServiceRequestBus::Events::SetHeightQueryResolution, m_configuration.m_heightQueryResolution);
-
         // Currently, the Terrain System Component owns the Terrain System instance because the Terrain World component gets recreated
         // every time an entity is added or removed to a level.  If this ever changes, the Terrain System ownership could move into
         // the level component.
         TerrainSystemServiceRequestBus::Broadcast(&TerrainSystemServiceRequestBus::Events::Activate);
+
+        AzFramework::Terrain::TerrainDataRequestBus::Broadcast(
+            &AzFramework::Terrain::TerrainDataRequestBus::Events::SetTerrainAabb,
+            AZ::Aabb::CreateFromMinMax(m_configuration.m_worldMin, m_configuration.m_worldMax));
+        AzFramework::Terrain::TerrainDataRequestBus::Broadcast(
+            &AzFramework::Terrain::TerrainDataRequestBus::Events::SetTerrainHeightQueryResolution, m_configuration.m_heightQueryResolution);
     }
 
     void TerrainWorldComponent::Deactivate()

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

@@ -171,7 +171,7 @@ namespace Terrain
                 // Determine how far to draw in each direction in world space based on our MaxSectorsToDraw
                 AZ::Vector2 queryResolution = AZ::Vector2(1.0f);
                 AzFramework::Terrain::TerrainDataRequestBus::BroadcastResult(
-                    queryResolution, &AzFramework::Terrain::TerrainDataRequests::GetTerrainGridResolution);
+                    queryResolution, &AzFramework::Terrain::TerrainDataRequests::GetTerrainHeightQueryResolution);
                 AZ::Vector3 viewDistance(
                     queryResolution.GetX() * SectorSizeInGridPoints * sqrtf(MaxSectorsToDraw),
                     queryResolution.GetY() * SectorSizeInGridPoints * sqrtf(MaxSectorsToDraw),
@@ -214,7 +214,7 @@ namespace Terrain
 
         AZ::Vector2 queryResolution = AZ::Vector2(1.0f);
         AzFramework::Terrain::TerrainDataRequestBus::BroadcastResult(
-            queryResolution, &AzFramework::Terrain::TerrainDataRequests::GetTerrainGridResolution);
+            queryResolution, &AzFramework::Terrain::TerrainDataRequests::GetTerrainHeightQueryResolution);
 
         // Calculate the world size of each sector.  Note that this size actually ends at the last point, not the last square.
         // So for example, the sector size for 3 points will go from (*--*--*) even though it will be used to draw (*--*--*--).

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

@@ -9,6 +9,7 @@
 #include <TerrainSystem/TerrainSystem.h>
 #include <AzCore/std/parallel/shared_mutex.h>
 #include <SurfaceData/SurfaceDataTypes.h>
+#include <SurfaceData/SurfaceDataSystemRequestBus.h>
 #include <LmbrCentral/Shape/ShapeComponentBus.h>
 
 #include <Atom/RPI.Public/Scene.h>
@@ -53,7 +54,7 @@ TerrainSystem::TerrainSystem()
     m_currentSettings.m_worldBounds = AZ::Aabb::CreateNull();
 
     m_requestedSettings = m_currentSettings;
-    m_requestedSettings.m_worldBounds = AZ::Aabb::CreateFromMinMax(AZ::Vector3(0.0f, 0.0f, 0.0f), AZ::Vector3(4096.0f, 4096.0f, 2048.0f));
+    m_requestedSettings.m_worldBounds = AZ::Aabb::CreateFromMinMax(AZ::Vector3(-512.0f), AZ::Vector3(512.0f));
 }
 
 TerrainSystem::~TerrainSystem()
@@ -66,23 +67,76 @@ TerrainSystem::~TerrainSystem()
 
 void TerrainSystem::Activate()
 {
-    m_requestedSettings.m_systemActive = true;
+    AzFramework::Terrain::TerrainDataNotificationBus::Broadcast(
+        &AzFramework::Terrain::TerrainDataNotificationBus::Events::OnTerrainDataCreateBegin);
+
+    m_dirtyRegion = AZ::Aabb::CreateNull();
+    m_terrainHeightDirty = true;
     m_terrainSettingsDirty = true;
+    m_requestedSettings.m_systemActive = true;
+
+    {
+        AZStd::shared_lock<AZStd::shared_mutex> lock(m_areaMutex);
+        m_registeredAreas.clear();
+    }
+
+    AzFramework::Terrain::TerrainDataRequestBus::Handler::BusConnect();
+
+    // Register any terrain spawners that were already active before the terrain system activated.
+    auto enumerationCallback = [&]([[maybe_unused]] Terrain::TerrainSpawnerRequests* terrainSpawner) -> bool
+    {
+        AZ::EntityId areaId = *(Terrain::TerrainSpawnerRequestBus::GetCurrentBusId());
+        RegisterArea(areaId);
+
+        // Keep Enumerating
+        return true;
+    };
+    Terrain::TerrainSpawnerRequestBus::EnumerateHandlers(enumerationCallback);
+
+    AzFramework::Terrain::TerrainDataNotificationBus::Broadcast(
+        &AzFramework::Terrain::TerrainDataNotificationBus::Events::OnTerrainDataCreateEnd);
 }
 
 void TerrainSystem::Deactivate()
 {
-    m_requestedSettings.m_systemActive = false;
+    AzFramework::Terrain::TerrainDataNotificationBus::Broadcast(
+        &AzFramework::Terrain::TerrainDataNotificationBus::Events::OnTerrainDataDestroyBegin);
+
+    AzFramework::Terrain::TerrainDataRequestBus::Handler::BusDisconnect();
+
+    {
+        AZStd::shared_lock<AZStd::shared_mutex> lock(m_areaMutex);
+        m_registeredAreas.clear();
+    }
+
+    m_dirtyRegion = AZ::Aabb::CreateNull();
+    m_terrainHeightDirty = true;
     m_terrainSettingsDirty = true;
+    m_requestedSettings.m_systemActive = false;
+
+    if (auto rpi = AZ::RPI::RPISystemInterface::Get(); rpi)
+    {
+        if (auto defaultScene = rpi->GetDefaultScene(); defaultScene)
+        {
+            const AZ::RPI::Scene* scene = defaultScene.get();
+            if (auto terrainFeatureProcessor = scene->GetFeatureProcessor<TerrainFeatureProcessor>(); terrainFeatureProcessor)
+            {
+                terrainFeatureProcessor->RemoveTerrainData();
+            }
+        }
+    }
+
+    AzFramework::Terrain::TerrainDataNotificationBus::Broadcast(
+        &AzFramework::Terrain::TerrainDataNotificationBus::Events::OnTerrainDataDestroyEnd);
 }
 
-void TerrainSystem::SetWorldBounds(const AZ::Aabb& worldBounds)
+void TerrainSystem::SetTerrainAabb(const AZ::Aabb& worldBounds)
 {   
     m_requestedSettings.m_worldBounds = worldBounds;
     m_terrainSettingsDirty = true;
 }
 
-void TerrainSystem::SetHeightQueryResolution(AZ::Vector2 queryResolution)
+void TerrainSystem::SetTerrainHeightQueryResolution(AZ::Vector2 queryResolution)
 {
     m_requestedSettings.m_heightQueryResolution = queryResolution;
     m_terrainSettingsDirty = true;
@@ -93,13 +147,15 @@ AZ::Aabb TerrainSystem::GetTerrainAabb() const
     return m_currentSettings.m_worldBounds;
 }
 
-AZ::Vector2 TerrainSystem::GetTerrainGridResolution() const
+AZ::Vector2 TerrainSystem::GetTerrainHeightQueryResolution() const
 {
     return m_currentSettings.m_heightQueryResolution;
 }
 
-float TerrainSystem::GetHeightSynchronous(float x, float y) const
+float TerrainSystem::GetHeightSynchronous(float x, float y, Sampler sampler, bool* terrainExistsPtr) const
 {
+    bool terrainExists = false;
+
     AZ::Vector3 inPosition((float)x, (float)y, m_currentSettings.m_worldBounds.GetMin().GetZ());
     AZ::Vector3 outPosition((float)x, (float)y, m_currentSettings.m_worldBounds.GetMin().GetZ());
 
@@ -111,67 +167,77 @@ float TerrainSystem::GetHeightSynchronous(float x, float y) const
         if (areaBounds.Contains(inPosition))
         {
             Terrain::TerrainAreaHeightRequestBus::Event(
-                areaId, &Terrain::TerrainAreaHeightRequestBus::Events::GetHeight, inPosition, outPosition,
-                Terrain::TerrainAreaHeightRequestBus::Events::Sampler::DEFAULT);
+                areaId, &Terrain::TerrainAreaHeightRequestBus::Events::GetHeight, inPosition, outPosition, sampler);
+
+            terrainExists = true;
+
+            break;
         }
     }
 
+    if (terrainExistsPtr)
+    {
+        *terrainExistsPtr = terrainExists;
+    }
+
     return AZ::GetClamp(
         outPosition.GetZ(), m_currentSettings.m_worldBounds.GetMin().GetZ(), m_currentSettings.m_worldBounds.GetMax().GetZ());
 }
 
-float TerrainSystem::GetHeight(AZ::Vector3 position, [[maybe_unused]] Sampler sampler, [[maybe_unused]] bool* terrainExistsPtr) const
+float TerrainSystem::GetHeight(AZ::Vector3 position, Sampler sampler, bool* terrainExistsPtr) const
 {
-    if (terrainExistsPtr)
+    return GetHeightSynchronous(position.GetX(), position.GetY(), sampler, terrainExistsPtr);
+}
+
+float TerrainSystem::GetHeightFromFloats(float x, float y, Sampler sampler, bool* terrainExistsPtr) const
+{
+    return GetHeightSynchronous(x, y, sampler, terrainExistsPtr);
+}
+
+bool TerrainSystem::GetIsHoleFromFloats(float x, float y, Sampler sampler) const
+{
+    bool terrainExists = false;
+    GetHeightSynchronous(x, y, sampler, &terrainExists);
+    return !terrainExists;
+}
+
+AZ::Vector3 TerrainSystem::GetNormalSynchronous(float x, float y, Sampler sampler, bool* terrainExistsPtr) const
+{
+    bool terrainExists = false;
+
+    AZ::Vector3 inPosition((float)x, (float)y, m_currentSettings.m_worldBounds.GetMin().GetZ());
+    AZ::Vector3 outNormal = AZ::Vector3::CreateAxisZ();
+
+    AZStd::shared_lock<AZStd::shared_mutex> lock(m_areaMutex);
+
+    for (auto& [areaId, areaBounds] : m_registeredAreas)
     {
-        *terrainExistsPtr = true;
+        inPosition.SetZ(areaBounds.GetMin().GetZ());
+        if (areaBounds.Contains(inPosition))
+        {
+            Terrain::TerrainAreaHeightRequestBus::Event(
+                areaId, &Terrain::TerrainAreaHeightRequestBus::Events::GetNormal, inPosition, outNormal, sampler);
+            terrainExists = true;
+            break;
+        }
     }
 
-    return GetHeightSynchronous(position.GetX(), position.GetY());
-}
-
-float TerrainSystem::GetHeightFromFloats(
-    float x, float y, [[maybe_unused]] Sampler sampler, [[maybe_unused]] bool* terrainExistsPtr) const
-{
     if (terrainExistsPtr)
     {
-        *terrainExistsPtr = true;
+        *terrainExistsPtr = terrainExists;
     }
 
-    return GetHeightSynchronous(x, y);
+    return outNormal;
 }
 
-bool TerrainSystem::GetIsHoleFromFloats(
-    [[maybe_unused]] float x, [[maybe_unused]] float y, [[maybe_unused]] Sampler sampleFilter) const
+AZ::Vector3 TerrainSystem::GetNormal(AZ::Vector3 position, Sampler sampler, bool* terrainExistsPtr) const
 {
-    return false;
+    return GetNormalSynchronous(position.GetX(), position.GetY(), sampler, terrainExistsPtr);
 }
 
-AZ::Vector3 TerrainSystem::GetNormalSynchronous([[maybe_unused]] float x, [[maybe_unused]] float y) const
+AZ::Vector3 TerrainSystem::GetNormalFromFloats(float x, float y, Sampler sampler, bool* terrainExistsPtr) const
 {
-    return AZ::Vector3::CreateAxisZ();
-}
-
-AZ::Vector3 TerrainSystem::GetNormal(
-    AZ::Vector3 position, [[maybe_unused]] Sampler sampleFilter, [[maybe_unused]] bool* terrainExistsPtr) const
-{
-    if (terrainExistsPtr)
-    {
-        *terrainExistsPtr = true;
-    }
-
-    return GetNormalSynchronous(position.GetX(), position.GetY());
-}
-
-AZ::Vector3 TerrainSystem::GetNormalFromFloats(
-    float x, float y, [[maybe_unused]] Sampler sampleFilter, [[maybe_unused]] bool* terrainExistsPtr) const
-{
-    if (terrainExistsPtr)
-    {
-        *terrainExistsPtr = true;
-    }
-
-    return GetNormalSynchronous(x, y);
+    return GetNormalSynchronous(x, y, sampler, terrainExistsPtr);
 }
 
 
@@ -298,35 +364,6 @@ void TerrainSystem::ProcessSurfacePointsFromRegion(const AZ::Aabb& inRegion, con
 }
 */
 
-void TerrainSystem::SystemActivate()
-{
-    {
-        AZStd::shared_lock<AZStd::shared_mutex> lock(m_areaMutex);
-        m_registeredAreas.clear();
-    }
-
-    AzFramework::Terrain::TerrainDataRequestBus::Handler::BusConnect();
-
-    TerrainAreaRequestBus::Broadcast(&TerrainAreaRequestBus::Events::RegisterArea);
-}
-
-void TerrainSystem::SystemDeactivate()
-{
-    AzFramework::Terrain::TerrainDataRequestBus::Handler::BusDisconnect();
-
-    {
-        AZStd::shared_lock<AZStd::shared_mutex> lock(m_areaMutex);
-        m_registeredAreas.clear();
-    }
-
-    const AZ::RPI::Scene* scene = AZ::RPI::RPISystemInterface::Get()->GetDefaultScene().get();
-    auto terrainFeatureProcessor = scene->GetFeatureProcessor<TerrainFeatureProcessor>();
-    if (terrainFeatureProcessor)
-    {
-        terrainFeatureProcessor->RemoveTerrainData();
-    }
-}
-
 void TerrainSystem::RegisterArea(AZ::EntityId areaId)
 {
     AZStd::unique_lock<AZStd::shared_mutex> lock(m_areaMutex);
@@ -383,6 +420,7 @@ void TerrainSystem::OnTick(float /*deltaTime*/, AZ::ScriptTimePoint /*time*/)
 
     if (m_terrainSettingsDirty)
     {
+        terrainSettingsChanged = true;
         m_terrainSettingsDirty = false;
 
         // This needs to happen before the "system active" check below, because activating the system will cause the various
@@ -393,24 +431,12 @@ void TerrainSystem::OnTick(float /*deltaTime*/, AZ::ScriptTimePoint /*time*/)
             m_dirtyRegion.AddAabb(m_requestedSettings.m_worldBounds);
             m_terrainHeightDirty = true;
             m_currentSettings.m_worldBounds = m_requestedSettings.m_worldBounds;
-            terrainSettingsChanged = true;
         }
 
         if (m_requestedSettings.m_heightQueryResolution != m_currentSettings.m_heightQueryResolution)
         {
             m_dirtyRegion = AZ::Aabb::CreateNull();
             m_terrainHeightDirty = true;
-            terrainSettingsChanged = true;
-        }
-
-        if (m_requestedSettings.m_systemActive != m_currentSettings.m_systemActive)
-        {
-            m_requestedSettings.m_systemActive ? SystemActivate() : SystemDeactivate();
-
-            // Null dirty region will be interpreted as updating everything
-            m_dirtyRegion = AZ::Aabb::CreateNull();
-            m_terrainHeightDirty = true;
-            terrainSettingsChanged = true;
         }
 
         m_currentSettings = m_requestedSettings;
@@ -420,6 +446,14 @@ void TerrainSystem::OnTick(float /*deltaTime*/, AZ::ScriptTimePoint /*time*/)
     {
         AZStd::shared_lock<AZStd::shared_mutex> lock(m_areaMutex);
 
+        // 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
+        // that create false detection of cyclic dependencies when multiple requests occur on different threads simultaneously.
+        // (One case where this was previously able to occur was in rapid updating of the Preview widget on the
+        // GradientSurfaceDataComponent in the Editor when moving the threshold sliders back and forth rapidly)
+        auto& surfaceDataContext = SurfaceData::SurfaceDataSystemRequestBus::GetOrCreateContext(false);
+        typename SurfaceData::SurfaceDataSystemRequestBus::Context::DispatchLockGuard scopeLock(surfaceDataContext.m_contextMutex);
+
         AZ::Transform transform = AZ::Transform::CreateTranslation(m_currentSettings.m_worldBounds.GetCenter());
 
         uint32_t width = aznumeric_cast<uint32_t>(
@@ -449,7 +483,8 @@ void TerrainSystem::OnTick(float /*deltaTime*/, AZ::ScriptTimePoint /*time*/)
                     }
 
                     AZ::Vector3 outPosition;
-                    const Terrain::TerrainAreaHeightRequests::Sampler sampleFilter = Terrain::TerrainAreaHeightRequests::Sampler::DEFAULT;
+                    const AzFramework::Terrain::TerrainDataRequestBus::Events::Sampler sampleFilter =
+                        AzFramework::Terrain::TerrainDataRequestBus::Events::Sampler::DEFAULT;
 
                     Terrain::TerrainAreaHeightRequestBus::Event(
                         areaId, &Terrain::TerrainAreaHeightRequestBus::Events::GetHeight, inPosition, outPosition, sampleFilter);
@@ -476,6 +511,14 @@ void TerrainSystem::OnTick(float /*deltaTime*/, AZ::ScriptTimePoint /*time*/)
 
     if (terrainSettingsChanged || m_terrainHeightDirty)
     {
+        // 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
+        // that create false detection of cyclic dependencies when multiple requests occur on different threads simultaneously.
+        // (One case where this was previously able to occur was in rapid updating of the Preview widget on the
+        // GradientSurfaceDataComponent in the Editor when moving the threshold sliders back and forth rapidly)
+        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;
 

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

@@ -42,10 +42,6 @@ namespace Terrain
 
         ///////////////////////////////////////////
         // TerrainSystemServiceRequestBus::Handler Impl
-
-        void SetWorldBounds(const AZ::Aabb& worldBounds) override;
-        void SetHeightQueryResolution(AZ::Vector2 queryResolution) override;
-
         void Activate() override;
         void Deactivate() override;
 
@@ -55,8 +51,12 @@ namespace Terrain
 
         ///////////////////////////////////////////
         // TerrainDataRequestBus::Handler Impl
-        AZ::Vector2 GetTerrainGridResolution() const override;
+        AZ::Vector2 GetTerrainHeightQueryResolution() const override;
+        void SetTerrainHeightQueryResolution(AZ::Vector2 queryResolution) override;
+
         AZ::Aabb GetTerrainAabb() const override;
+        void SetTerrainAabb(const AZ::Aabb& worldBounds) override;
+
 
         //! Returns terrains height in meters at location x,y.
         //! @terrainExistsPtr: Can be nullptr. If != nullptr then, if there's no terrain at location x,y or location x,y is inside a terrain
@@ -94,15 +94,12 @@ namespace Terrain
             float x, float y, Sampler sampleFilter = Sampler::BILINEAR, bool* terrainExistsPtr = nullptr) const override;
 
     private:
-        float GetHeightSynchronous(float x, float y) const;
-        AZ::Vector3 GetNormalSynchronous(float x, float y) const;
+        float GetHeightSynchronous(float x, float y, Sampler sampler, bool* terrainExistsPtr) const;
+        AZ::Vector3 GetNormalSynchronous(float x, float y, Sampler sampler, bool* terrainExistsPtr) const;
 
         // AZ::TickBus::Handler overrides ...
         void OnTick(float deltaTime, AZ::ScriptTimePoint time) override;
 
-        void SystemActivate();
-        void SystemDeactivate();
-
         struct TerrainSystemSettings
         {
             AZ::Aabb m_worldBounds;

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

@@ -16,6 +16,8 @@
 #include <AzCore/EBus/EBus.h>
 #include <AzCore/Component/ComponentBus.h>
 
+#include <AzFramework/Terrain/TerrainDataRequestBus.h>
+
 namespace Terrain
 {
     /**
@@ -39,9 +41,6 @@ namespace Terrain
         virtual void Activate() = 0;
         virtual void Deactivate() = 0;
 
-        virtual void SetWorldBounds(const AZ::Aabb& worldBounds) = 0;
-        virtual void SetHeightQueryResolution(AZ::Vector2 queryResolution) = 0;
-
         // register an area to override terrain
         virtual void RegisterArea(AZ::EntityId areaId) = 0;
         virtual void UnregisterArea(AZ::EntityId areaId) = 0;
@@ -50,27 +49,6 @@ namespace Terrain
 
     using TerrainSystemServiceRequestBus = AZ::EBus<TerrainSystemServiceRequests>;
 
-    /**
-    * A bus to signal the life times of terrain areas
-    * Note: all the API are meant to be queued events
-    */
-    class TerrainAreaRequests
-        : public AZ::ComponentBus
-    {
-    public:
-        ////////////////////////////////////////////////////////////////////////
-        // EBusTraits
-        using MutexType = AZStd::recursive_mutex;
-        ////////////////////////////////////////////////////////////////////////
-
-        virtual ~TerrainAreaRequests() = default;
-
-        virtual void RegisterArea() = 0;
-        virtual void RefreshArea() = 0;
-
-    };
-
-    using TerrainAreaRequestBus = AZ::EBus<TerrainAreaRequests>;
 
     /**
     * A bus to signal the life times of terrain areas
@@ -87,15 +65,6 @@ namespace Terrain
 
         virtual ~TerrainAreaHeightRequests() = default;
 
-        enum class Sampler
-        {
-            BILINEAR,   // Get the value at the requested location, using terrain sample grid to bilinear filter between sample grid points
-            CLAMP,      // Clamp the input point to the terrain sample grid, then get the exact value
-            EXACT,       // Directly get the value at the location, regardless of terrain sample grid density
-
-            DEFAULT = BILINEAR
-        };
-
         enum SurfacePointDataMask
         {
             POSITION = 0x01,
@@ -107,8 +76,16 @@ namespace Terrain
 
         // Synchronous single input location.  The Vector3 input position versions are defined to ignore the input Z value.
 
-        virtual void GetHeight(const AZ::Vector3& inPosition, AZ::Vector3& outPosition, Sampler sampleFilter = Sampler::DEFAULT) = 0;
-        virtual void GetNormal(const AZ::Vector3& inPosition, AZ::Vector3& outNormal, Sampler sampleFilter = Sampler::DEFAULT) = 0;
+        virtual void GetHeight(
+            const AZ::Vector3& inPosition,
+            AZ::Vector3& outPosition,
+            AzFramework::Terrain::TerrainDataRequests::Sampler sampleFilter =
+                AzFramework::Terrain::TerrainDataRequests::Sampler::DEFAULT) = 0;
+        virtual void GetNormal(
+            const AZ::Vector3& inPosition,
+            AZ::Vector3& outNormal,
+            AzFramework::Terrain::TerrainDataRequests::Sampler sampleFilter =
+                AzFramework::Terrain::TerrainDataRequests::Sampler::DEFAULT) = 0;
     };
 
     using TerrainAreaHeightRequestBus = AZ::EBus<TerrainAreaHeightRequests>;

Gems/Terrain/Code/Tests/LayerSpawnerTests.cpp

@@ -17,6 +17,10 @@
 
 #include <TerrainMocks.h>
 
+using ::testing::NiceMock;
+using ::testing::AtLeast;
+using ::testing::_;
+
 class LayerSpawnerComponentTest
     : public ::testing::Test
 {
@@ -26,7 +30,7 @@ protected:
     AZStd::unique_ptr<AZ::Entity> m_entity;
     Terrain::TerrainLayerSpawnerComponent* m_layerSpawnerComponent;
     UnitTest::MockBoxShapeComponent* m_shapeComponent;
-    AZStd::unique_ptr<UnitTest::MockTerrainSystem> m_terrainSystem;
+    AZStd::unique_ptr<NiceMock<UnitTest::MockTerrainSystemService>> m_terrainSystem;
 
     void SetUp() override
     {
@@ -40,10 +44,8 @@ protected:
 
     void TearDown() override
     {
-        if (m_terrainSystem)
-        {
-            m_terrainSystem->Deactivate();
-        }
+        m_entity.reset();
+        m_terrainSystem.reset();
         m_app.Destroy();
     }
 
@@ -72,16 +74,9 @@ protected:
         ASSERT_TRUE(m_shapeComponent);
     }
 
-    void ResetEntity()
-    {
-        m_entity->Deactivate();
-        m_entity->Reset();
-    }
-
     void CreateMockTerrainSystem()
     {
-        m_terrainSystem = AZStd::make_unique<UnitTest::MockTerrainSystem>();
-        m_terrainSystem->Activate();
+        m_terrainSystem = AZStd::make_unique<NiceMock<UnitTest::MockTerrainSystemService>>();
     }
 };
 
@@ -93,7 +88,7 @@ TEST_F(LayerSpawnerComponentTest, ActivatEntityActivateSuccess)
     m_entity->Activate();
     EXPECT_EQ(m_entity->GetState(), AZ::Entity::State::Active);
      
-    ResetEntity();
+    m_entity->Deactivate();
 }
 
 TEST_F(LayerSpawnerComponentTest, LayerSpawnerDefaultValuesCorrect)
@@ -115,7 +110,7 @@ TEST_F(LayerSpawnerComponentTest, LayerSpawnerDefaultValuesCorrect)
 
     EXPECT_TRUE(useGroundPlane);
 
-    ResetEntity();
+    m_entity->Deactivate();
 }
 
 TEST_F(LayerSpawnerComponentTest, LayerSpawnerConfigValuesCorrect)
@@ -147,7 +142,7 @@ TEST_F(LayerSpawnerComponentTest, LayerSpawnerConfigValuesCorrect)
 
     EXPECT_FALSE(useGroundPlane);
 
-    ResetEntity();
+    m_entity->Deactivate();
 }
 
 TEST_F(LayerSpawnerComponentTest, LayerSpawnerRegisterAreaUpdatesTerrainSystem)
@@ -156,14 +151,14 @@ TEST_F(LayerSpawnerComponentTest, LayerSpawnerRegisterAreaUpdatesTerrainSystem)
 
     CreateMockTerrainSystem();
 
+    // The Activate call should register the area.
+    EXPECT_CALL(*m_terrainSystem, RegisterArea(_)).Times(1);
+
     AddLayerSpawnerAndShapeComponentToEntity();
 
     m_entity->Activate();
 
-    // The Activate call should have registered the area.
-    EXPECT_EQ(1, m_terrainSystem->m_registerAreaCalledCount);
-
-    ResetEntity();
+    m_entity->Deactivate();
 }
 
 TEST_F(LayerSpawnerComponentTest, LayerSpawnerUnregisterAreaUpdatesTerrainSystem)
@@ -172,16 +167,14 @@ TEST_F(LayerSpawnerComponentTest, LayerSpawnerUnregisterAreaUpdatesTerrainSystem
 
     CreateMockTerrainSystem();
 
+    // The Deactivate call should unregister the area.
+    EXPECT_CALL(*m_terrainSystem, UnregisterArea(_)).Times(1);
+
     AddLayerSpawnerAndShapeComponentToEntity();
 
     m_entity->Activate();
 
-    m_layerSpawnerComponent->Deactivate();
-
-    // The Deactivate call should have unregistered the area.
-    EXPECT_EQ(1, m_terrainSystem->m_unregisterAreaCalledCount);
-
-    ResetEntity();
+    m_entity->Deactivate();
 }
 
 TEST_F(LayerSpawnerComponentTest, LayerSpawnerTransformChangedUpdatesTerrainSystem)
@@ -190,6 +183,9 @@ TEST_F(LayerSpawnerComponentTest, LayerSpawnerTransformChangedUpdatesTerrainSyst
 
     CreateMockTerrainSystem();
 
+    // The TransformChanged call should refresh the area.
+    EXPECT_CALL(*m_terrainSystem, RefreshArea(_)).Times(1);
+
     AddLayerSpawnerAndShapeComponentToEntity();
 
     m_entity->Activate();
@@ -197,9 +193,7 @@ TEST_F(LayerSpawnerComponentTest, LayerSpawnerTransformChangedUpdatesTerrainSyst
     AZ::TransformNotificationBus::Event(
         m_entity->GetId(), &AZ::TransformNotificationBus::Events::OnTransformChanged, AZ::Transform(), AZ::Transform());
 
-    EXPECT_EQ(1, m_terrainSystem->m_refreshAreaCalledCount);
-
-    ResetEntity();
+    m_entity->Deactivate();
 }
 
 TEST_F(LayerSpawnerComponentTest, LayerSpawnerShapeChangedUpdatesTerrainSystem)
@@ -208,6 +202,9 @@ TEST_F(LayerSpawnerComponentTest, LayerSpawnerShapeChangedUpdatesTerrainSystem)
 
     CreateMockTerrainSystem();
 
+    // The ShapeChanged call should refresh the area.
+    EXPECT_CALL(*m_terrainSystem, RefreshArea(_)).Times(1);
+
     AddLayerSpawnerAndShapeComponentToEntity();
 
     m_entity->Activate();
@@ -216,7 +213,5 @@ TEST_F(LayerSpawnerComponentTest, LayerSpawnerShapeChangedUpdatesTerrainSystem)
         m_entity->GetId(), &LmbrCentral::ShapeComponentNotificationsBus::Events::OnShapeChanged,
         LmbrCentral::ShapeComponentNotifications::ShapeChangeReasons::ShapeChanged);
 
-    EXPECT_EQ(1, m_terrainSystem->m_refreshAreaCalledCount);
-
-    ResetEntity();
+    m_entity->Deactivate();
 }

Gems/Terrain/Code/Tests/TerrainMocks.h

@@ -7,7 +7,10 @@
  */
 #pragma once
 
+#include <gmock/gmock.h>
+
 #include <AzCore/Component/ComponentApplication.h>
+#include <AzFramework/Terrain/TerrainDataRequestBus.h>
 #include <LmbrCentral/Shape/ShapeComponentBus.h>
 
 namespace UnitTest
@@ -62,44 +65,45 @@ namespace UnitTest
         }
     };
 
-    class MockTerrainSystem : private Terrain::TerrainSystemServiceRequestBus::Handler
+    class MockTerrainSystemService : private Terrain::TerrainSystemServiceRequestBus::Handler
     {
     public:
-        void Activate() override
+        MockTerrainSystemService()
         {
             Terrain::TerrainSystemServiceRequestBus::Handler::BusConnect();
         }
 
-        void Deactivate() override
+        ~MockTerrainSystemService()
         {
             Terrain::TerrainSystemServiceRequestBus::Handler::BusDisconnect();
         }
 
-        void SetWorldBounds([[maybe_unused]] const AZ::Aabb& worldBounds) override
-        {
-        }
+        MOCK_METHOD0(Activate, void());
+        MOCK_METHOD0(Deactivate, void());
 
-        void SetHeightQueryResolution([[maybe_unused]] AZ::Vector2 queryResolution) override
-        {
-        }
-
-        void RegisterArea([[maybe_unused]] AZ::EntityId areaId) override
-        {
-            m_registerAreaCalledCount++;
-        }
-
-        void UnregisterArea([[maybe_unused]] AZ::EntityId areaId) override
-        {
-            m_unregisterAreaCalledCount++;
-        }
-
-        void RefreshArea([[maybe_unused]] AZ::EntityId areaId) override
-        {
-            m_refreshAreaCalledCount++;
-        }
-
-        int m_registerAreaCalledCount = 0;
-        int m_refreshAreaCalledCount = 0;
-        int m_unregisterAreaCalledCount = 0;
+        MOCK_METHOD1(RegisterArea, void(AZ::EntityId areaId));
+        MOCK_METHOD1(UnregisterArea, void(AZ::EntityId areaId));
+        MOCK_METHOD1(RefreshArea, void(AZ::EntityId areaId));
     };
+
+    class MockTerrainDataNotificationListener : public AzFramework::Terrain::TerrainDataNotificationBus::Handler
+    {
+    public:
+        MockTerrainDataNotificationListener()
+        {
+            AzFramework::Terrain::TerrainDataNotificationBus::Handler::BusConnect();
+        }
+
+        ~MockTerrainDataNotificationListener()
+        {
+            AzFramework::Terrain::TerrainDataNotificationBus::Handler::BusDisconnect();
+        }
+
+        MOCK_METHOD0(OnTerrainDataCreateBegin, void());
+        MOCK_METHOD0(OnTerrainDataCreateEnd, void());
+        MOCK_METHOD0(OnTerrainDataDestroyBegin, void());
+        MOCK_METHOD0(OnTerrainDataDestroyEnd, void());
+        MOCK_METHOD2(OnTerrainDataChanged, void(const AZ::Aabb& dirtyRegion, TerrainDataChangedMask dataChangedMask));
+    };
+
 }

Gems/Terrain/Code/Tests/TerrainSystemTest.cpp

@@ -0,0 +1,92 @@
+/*
+ * Copyright (c) Contributors to the Open 3D Engine Project.
+ * For complete copyright and license terms please see the LICENSE at the root of this distribution.
+ *
+ * SPDX-License-Identifier: Apache-2.0 OR MIT
+ *
+ */
+
+#include <AzCore/Component/ComponentApplication.h>
+#include <AzCore/Memory/MemoryComponent.h>
+
+#include <TerrainSystem/TerrainSystem.h>
+
+#include <AzTest/AzTest.h>
+#include <TerrainMocks.h>
+
+using ::testing::AtLeast;
+using ::testing::NiceMock;
+
+class TerrainSystemTest : public ::testing::Test
+{
+protected:
+    AZ::ComponentApplication m_app;
+
+    AZStd::unique_ptr<AZ::Entity> m_entity;
+    AZStd::unique_ptr<Terrain::TerrainSystem> m_terrainSystem;
+
+    void SetUp() override
+    {
+        AZ::ComponentApplication::Descriptor appDesc;
+        appDesc.m_memoryBlocksByteSize = 20 * 1024 * 1024;
+        appDesc.m_recordingMode = AZ::Debug::AllocationRecords::RECORD_NO_RECORDS;
+        appDesc.m_stackRecordLevels = 20;
+
+        m_app.Create(appDesc);
+    }
+
+    void TearDown() override
+    {
+        m_terrainSystem.reset();
+        m_app.Destroy();
+    }
+
+    void CreateEntity()
+    {
+        m_entity = AZStd::make_unique<AZ::Entity>();
+        m_entity->Init();
+
+        ASSERT_TRUE(m_entity);
+    }
+
+    void ResetEntity()
+    {
+        m_entity->Deactivate();
+        m_entity->Reset();
+    }
+};
+
+TEST_F(TerrainSystemTest, TrivialCreateDestroy)
+{
+    m_terrainSystem = AZStd::make_unique<Terrain::TerrainSystem>();
+}
+
+TEST_F(TerrainSystemTest, TrivialActivateDeactivate)
+{
+    m_terrainSystem = AZStd::make_unique<Terrain::TerrainSystem>();
+    m_terrainSystem->Activate();
+    m_terrainSystem->Deactivate();
+}
+
+TEST_F(TerrainSystemTest, CreateEventsCalledOnActivation)
+{
+    NiceMock<UnitTest::MockTerrainDataNotificationListener> mockTerrainListener;
+    EXPECT_CALL(mockTerrainListener, OnTerrainDataCreateBegin()).Times(AtLeast(1));
+    EXPECT_CALL(mockTerrainListener, OnTerrainDataCreateEnd()).Times(AtLeast(1));
+
+    m_terrainSystem = AZStd::make_unique<Terrain::TerrainSystem>();
+    m_terrainSystem->Activate();
+}
+
+TEST_F(TerrainSystemTest, DestroyEventsCalledOnDeactivation)
+{
+    NiceMock<UnitTest::MockTerrainDataNotificationListener> mockTerrainListener;
+    EXPECT_CALL(mockTerrainListener, OnTerrainDataDestroyBegin()).Times(AtLeast(1));
+    EXPECT_CALL(mockTerrainListener, OnTerrainDataDestroyEnd()).Times(AtLeast(1));
+
+    m_terrainSystem = AZStd::make_unique<Terrain::TerrainSystem>();
+    m_terrainSystem->Activate();
+    m_terrainSystem->Deactivate();
+}
+
+

Gems/Terrain/Code/Tests/TerrainTest.cpp

@@ -8,24 +8,4 @@
 
 #include <AzTest/AzTest.h>
 
-class TerrainTest
-    : public ::testing::Test
-{
-protected:
-    void SetUp() override
-    {
-
-    }
-
-    void TearDown() override
-    {
-
-    }
-};
-
-TEST_F(TerrainTest, SanityTest)
-{
-    ASSERT_TRUE(true);
-}
-
 AZ_UNIT_TEST_HOOK(DEFAULT_UNIT_TEST_ENV);

Gems/Terrain/Code/terrain_tests_files.cmake

@@ -9,5 +9,6 @@
 set(FILES
     Tests/TerrainMocks.h
     Tests/TerrainTest.cpp
+    Tests/TerrainSystemTest.cpp
     Tests/LayerSpawnerTests.cpp
 )