diff --git a/AutomatedTesting/Gem/PythonTests/atom_renderer/CMakeLists.txt b/AutomatedTesting/Gem/PythonTests/atom_renderer/CMakeLists.txt index f056623ecd..992420904c 100644 --- a/AutomatedTesting/Gem/PythonTests/atom_renderer/CMakeLists.txt +++ b/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() diff --git a/AutomatedTesting/Gem/PythonTests/atom_renderer/atom_hydra_scripts/TestAssets/ShaderAssetBuilder/DependencyValidation.azsl.txt b/AutomatedTesting/Gem/PythonTests/atom_renderer/atom_hydra_scripts/TestAssets/ShaderAssetBuilder/DependencyValidation.azsl.txt new file mode 100644 index 0000000000..c0b64f40b7 --- /dev/null +++ b/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 + +#include "Test1Color.azsli" +#include + +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; +} diff --git a/AutomatedTesting/Gem/PythonTests/atom_renderer/atom_hydra_scripts/TestAssets/ShaderAssetBuilder/DependencyValidation.shader.txt b/AutomatedTesting/Gem/PythonTests/atom_renderer/atom_hydra_scripts/TestAssets/ShaderAssetBuilder/DependencyValidation.shader.txt new file mode 100644 index 0000000000..b0eac1783e --- /dev/null +++ b/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" + } + ] + } + +} diff --git a/AutomatedTesting/Gem/PythonTests/atom_renderer/atom_hydra_scripts/TestAssets/ShaderAssetBuilder/Test1Color.azsli.txt b/AutomatedTesting/Gem/PythonTests/atom_renderer/atom_hydra_scripts/TestAssets/ShaderAssetBuilder/Test1Color.azsli.txt new file mode 100644 index 0000000000..7d097beafb --- /dev/null +++ b/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); +} diff --git a/AutomatedTesting/Gem/PythonTests/atom_renderer/atom_hydra_scripts/TestAssets/ShaderAssetBuilder/Test2Color.azsli.txt b/AutomatedTesting/Gem/PythonTests/atom_renderer/atom_hydra_scripts/TestAssets/ShaderAssetBuilder/Test2Color.azsli.txt new file mode 100644 index 0000000000..2ef946b947 --- /dev/null +++ b/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; +} diff --git a/AutomatedTesting/Gem/PythonTests/atom_renderer/atom_hydra_scripts/TestAssets/ShaderAssetBuilder/Test3Color.azsli.txt b/AutomatedTesting/Gem/PythonTests/atom_renderer/atom_hydra_scripts/TestAssets/ShaderAssetBuilder/Test3Color.azsli.txt new file mode 100644 index 0000000000..73b0cca434 --- /dev/null +++ b/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; +} diff --git a/AutomatedTesting/Gem/PythonTests/atom_renderer/atom_hydra_scripts/hydra_ShaderAssetBuilder_RecompilesShaderAsChainOfDependenciesChanges.py b/AutomatedTesting/Gem/PythonTests/atom_renderer/atom_hydra_scripts/hydra_ShaderAssetBuilder_RecompilesShaderAsChainOfDependenciesChanges.py new file mode 100644 index 0000000000..a05420d960 --- /dev/null +++ b/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) \ No newline at end of file diff --git a/AutomatedTesting/Gem/PythonTests/atom_renderer/test_Atom_ShaderBuildPipelineSuite.py b/AutomatedTesting/Gem/PythonTests/atom_renderer/test_Atom_ShaderBuildPipelineSuite.py new file mode 100644 index 0000000000..9ef93ea238 --- /dev/null +++ b/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 \ No newline at end of file diff --git a/Code/Editor/GameExporter.cpp b/Code/Editor/GameExporter.cpp index 1fc980fdac..00dc3d8de1 100644 --- a/Code/Editor/GameExporter.cpp +++ b/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(terrainAabb.GetXExtent() / terrainGridResolution.GetX()); root->setAttr("HeightmapSize", compiledHeightmapSize); diff --git a/Code/Framework/AzFramework/AzFramework/Archive/Archive.cpp b/Code/Framework/AzFramework/AzFramework/Archive/Archive.cpp index 012d713cf5..1891c50d10 100644 --- a/Code/Framework/AzFramework/AzFramework/Archive/Archive.cpp +++ b/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) diff --git a/Code/Framework/AzFramework/AzFramework/Terrain/TerrainDataRequestBus.cpp b/Code/Framework/AzFramework/AzFramework/Terrain/TerrainDataRequestBus.cpp index 947d3821ab..1fb29cfa30 100644 --- a/Code/Framework/AzFramework/AzFramework/Terrain/TerrainDataRequestBus.cpp +++ b/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) ; } diff --git a/Code/Framework/AzFramework/AzFramework/Terrain/TerrainDataRequestBus.h b/Code/Framework/AzFramework/AzFramework/Terrain/TerrainDataRequestBus.h index 08e238434e..92eb28a110 100644 --- a/Code/Framework/AzFramework/AzFramework/Terrain/TerrainDataRequestBus.h +++ b/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, diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/ViewportSelection/EditorTransformComponentSelection.cpp b/Code/Framework/AzToolsFramework/AzToolsFramework/ViewportSelection/EditorTransformComponentSelection.cpp index 5a7b097e29..064fbb9da6 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/ViewportSelection/EditorTransformComponentSelection.cpp +++ b/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(EntityIdList(), s_entityBoxSelectUndoRedoDesc); + AZStd::make_unique(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(CreateManipulatorCommandStateFromSelf(), s_manipulatorUndoRedoName); + AZStd::make_unique(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(EntityIdList(), s_entitiesDeselectUndoRedoDesc); + auto selectionCommand = AZStd::make_unique(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(nextEntityIds, s_entityDeselectUndoRedoDesc); + auto selectionCommand = AZStd::make_unique(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(nextEntityIds, s_entitySelectUndoRedoDesc); + ScopedUndoBatch undoBatch(EntitySelectUndoRedoDesc); + auto selectionCommand = AZStd::make_unique(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(CreateManipulatorCommandStateFromSelf(), s_manipulatorUndoRedoName); + AZStd::make_unique(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(CreateManipulatorCommandStateFromSelf(), s_manipulatorUndoRedoName); + AZStd::make_unique(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(nextEntityIds, s_selectAllEntitiesUndoRedoDesc); + auto selectionCommand = AZStd::make_unique(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(CreateManipulatorCommandStateFromSelf(), s_manipulatorUndoRedoName); + AZStd::make_unique(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(nextEntityIds, s_invertSelectionUndoRedoDesc); + auto selectionCommand = AZStd::make_unique(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(EntityIdList(), s_duplicateUndoRedoDesc); + ScopedUndoBatch undoBatch(DuplicateUndoRedoDesc); + auto selectionCommand = AZStd::make_unique(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(CreateManipulatorCommandStateFromSelf(), s_manipulatorUndoRedoName); + AZStd::make_unique(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(CreateManipulatorCommandStateFromSelf(), s_manipulatorUndoRedoName); + AZStd::make_unique(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(CreateManipulatorCommandStateFromSelf(), s_manipulatorUndoRedoName); + AZStd::make_unique(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(CreateManipulatorCommandStateFromSelf(), s_manipulatorUndoRedoName); + AZStd::make_unique(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(CreateManipulatorCommandStateFromSelf(), s_manipulatorUndoRedoName); + AZStd::make_unique(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(CreateManipulatorCommandStateFromSelf(), s_manipulatorUndoRedoName); + AZStd::make_unique(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(CreateManipulatorCommandStateFromSelf(), s_manipulatorUndoRedoName); + AZStd::make_unique(CreateManipulatorCommandStateFromSelf(), ManipulatorUndoRedoName); manipulatorCommand->SetManipulatorAfter(EntityManipulatorCommand::State()); diff --git a/Code/Framework/AzToolsFramework/Tests/EditorTransformComponentSelectionTests.cpp b/Code/Framework/AzToolsFramework/Tests/EditorTransformComponentSelectionTests.cpp index 241ee3f576..72cafe5cd5 100644 --- a/Code/Framework/AzToolsFramework/Tests/EditorTransformComponentSelectionTests.cpp +++ b/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; diff --git a/Code/LauncherUnified/Launcher.cpp b/Code/LauncherUnified/Launcher.cpp index de4c496b2a..27aab074ef 100644 --- a/Code/LauncherUnified/Launcher.cpp +++ b/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; } } } diff --git a/Code/Legacy/CrySystem/AZCoreLogSink.h b/Code/Legacy/CrySystem/AZCoreLogSink.h index e824c4c172..a54a78157c 100644 --- a/Code/Legacy/CrySystem/AZCoreLogSink.h +++ b/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(); diff --git a/Code/Legacy/CrySystem/LevelSystem/LevelSystem.cpp b/Code/Legacy/CrySystem/LevelSystem/LevelSystem.cpp index a0338941b5..c5d364a025 100644 --- a/Code/Legacy/CrySystem/LevelSystem/LevelSystem.cpp +++ b/Code/Legacy/CrySystem/LevelSystem/LevelSystem.cpp @@ -233,6 +233,7 @@ CLevelSystem::CLevelSystem(ISystem* pSystem, const char* levelsFolder) //------------------------------------------------------------------------ CLevelSystem::~CLevelSystem() { + UnloadLevel(); } //------------------------------------------------------------------------ diff --git a/Code/Legacy/CrySystem/System.cpp b/Code/Legacy/CrySystem/System.cpp index 4a9d9ece5f..2e18e13841 100644 --- a/Code/Legacy/CrySystem/System.cpp +++ b/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 diff --git a/Code/Tools/RemoteConsole/Core/RemoteConsoleCore.cpp b/Code/Tools/RemoteConsole/Core/RemoteConsoleCore.cpp index d0e9e6a760..0a8d9d8eb7 100644 --- a/Code/Tools/RemoteConsole/Core/RemoteConsoleCore.cpp +++ b/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)) diff --git a/Code/Tools/Standalone/Source/Editor/LuaEditor.cpp b/Code/Tools/Standalone/Source/Editor/LuaEditor.cpp index 89ab8207ff..f6d905515b 100644 --- a/Code/Tools/Standalone/Source/Editor/LuaEditor.cpp +++ b/Code/Tools/Standalone/Source/Editor/LuaEditor.cpp @@ -35,6 +35,10 @@ int main(int argc, char* argv[]) { AZ::AllocatorInstance::Create(); } + if (!AZ::AllocatorInstance::IsReady()) + { + AZ::AllocatorInstance::Create(); + } AZStd::unique_ptr fileIO = AZStd::unique_ptr(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::IsReady()) + { + AZ::AllocatorInstance::Destroy(); + } if (AZ::AllocatorInstance::IsReady()) { AZ::AllocatorInstance::Destroy(); diff --git a/Gems/Atom/Asset/Shader/Code/CMakeLists.txt b/Gems/Atom/Asset/Shader/Code/CMakeLists.txt index c0bdfd4a8b..efbedb0a38 100644 --- a/Gems/Atom/Asset/Shader/Code/CMakeLists.txt +++ b/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 ) diff --git a/Gems/Atom/Asset/Shader/Code/Source/Editor/ShaderAssetBuilder.cpp b/Gems/Atom/Asset/Shader/Code/Source/Editor/ShaderAssetBuilder.cpp index 6673da2be3..2babba1ecc 100644 --- a/Gems/Atom/Asset/Shader/Code/Source/Editor/ShaderAssetBuilder.cpp +++ b/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& 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& includedFiles, AZStd::string_view normalizedRelativePath, AZStd::string_view currentFolderPath, const AZStd::vector& 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 ' 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& includeDirectories, + const ShaderBuilderUtility::IncludedFilesParser& includedFilesParser, AZStd::unordered_set& 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 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); diff --git a/Gems/Atom/Asset/Shader/Code/Source/Editor/ShaderBuilderUtility.cpp b/Gems/Atom/Asset/Shader/Code/Source/Editor/ShaderBuilderUtility.cpp index e47dd55a97..eb91d9e866 100644 --- a/Gems/Atom/Asset/Shader/Code/Source/Editor/ShaderBuilderUtility.cpp +++ b/Gems/Atom/Asset/Shader/Code/Source/Editor/ShaderBuilderUtility.cpp @@ -20,6 +20,7 @@ #include #include #include +#include #include @@ -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 IncludedFilesParser::ParseStringAndGetIncludedFiles(AZStd::string_view haystack) const + { + AZStd::vector 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::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 diff --git a/Gems/Atom/Asset/Shader/Code/Source/Editor/ShaderBuilderUtility.h b/Gems/Atom/Asset/Shader/Code/Source/Editor/ShaderBuilderUtility.h index c000ba9df6..5d45ade9cb 100644 --- a/Gems/Atom/Asset/Shader/Code/Source/Editor/ShaderBuilderUtility.h +++ b/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 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::string> ParseFileAndGetIncludedFiles(AZStd::string_view sourceFilePath) const; + + private: + AZStd::regex m_includeRegex; + }; + } // ShaderBuilderUtility namespace } // ShaderBuilder namespace } // AZ diff --git a/Gems/Atom/Asset/Shader/Code/Tests/ShaderBuilderUtilityTests.cpp b/Gems/Atom/Asset/Shader/Code/Tests/ShaderBuilderUtilityTests.cpp new file mode 100644 index 0000000000..d477060d04 --- /dev/null +++ b/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 +#include + +#include "Common/ShaderBuilderTestFixture.h" + +#include + +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 \n" + "// #include \n" + "blah # include \"valid_file3.azsli\"\n" + "bar include \n" + "foo # include \"a/directory/valid-file5.azsli\"\n" + "# include \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); + diff --git a/Gems/Atom/Asset/Shader/Code/atom_asset_shader_builders_tests_files.cmake b/Gems/Atom/Asset/Shader/Code/atom_asset_shader_builders_tests_files.cmake index 033b399478..ab8df70bdb 100644 --- a/Gems/Atom/Asset/Shader/Code/atom_asset_shader_builders_tests_files.cmake +++ b/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 ) diff --git a/Gems/Atom/Bootstrap/Code/Source/BootstrapSystemComponent.cpp b/Gems/Atom/Bootstrap/Code/Source/BootstrapSystemComponent.cpp index 97faa7380f..ddd88a1f52 100644 --- a/Gems/Atom/Bootstrap/Code/Source/BootstrapSystemComponent.cpp +++ b/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(); + m_defaultFrameworkScene->UnsetSubsystem(m_defaultScene); } m_defaultScene = nullptr; diff --git a/Gems/Atom/Feature/Common/Assets/Shaders/PostProcessing/FastDepthAwareBlurHor.azsl b/Gems/Atom/Feature/Common/Assets/Shaders/PostProcessing/FastDepthAwareBlurHor.azsl index 64f6b3a4b5..a7dfad1c24 100644 --- a/Gems/Atom/Feature/Common/Assets/Shaders/PostProcessing/FastDepthAwareBlurHor.azsl +++ b/Gems/Atom/Feature/Common/Assets/Shaders/PostProcessing/FastDepthAwareBlurHor.azsl @@ -55,7 +55,7 @@ int GetLdsIndex(int2 ldsPosition) // --- Common file start --- -// #include +// include ('#' 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 diff --git a/Gems/Atom/Feature/Common/Assets/Shaders/PostProcessing/FastDepthAwareBlurVer.azsl b/Gems/Atom/Feature/Common/Assets/Shaders/PostProcessing/FastDepthAwareBlurVer.azsl index cfb49f5911..b4f230c99c 100644 --- a/Gems/Atom/Feature/Common/Assets/Shaders/PostProcessing/FastDepthAwareBlurVer.azsl +++ b/Gems/Atom/Feature/Common/Assets/Shaders/PostProcessing/FastDepthAwareBlurVer.azsl @@ -55,7 +55,7 @@ int GetLdsIndex(int2 ldsPosition) // --- Common file start --- -// #include +// include ('#' 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 diff --git a/Gems/Atom/Feature/Common/Assets/Shaders/PostProcessing/SMAA.azsli b/Gems/Atom/Feature/Common/Assets/Shaders/PostProcessing/SMAA.azsli index 8c03816b26..a32b56857c 100644 --- a/Gems/Atom/Feature/Common/Assets/Shaders/PostProcessing/SMAA.azsli +++ b/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 diff --git a/Gems/AtomLyIntegration/CommonFeatures/Assets/PostProcess/default.postfxlayercategories b/Gems/AtomLyIntegration/CommonFeatures/Assets/PostProcess/default.postfxlayercategories index 33adccfd33..d6bffb0e6a 100644 --- a/Gems/AtomLyIntegration/CommonFeatures/Assets/PostProcess/default.postfxlayercategories +++ b/Gems/AtomLyIntegration/CommonFeatures/Assets/PostProcess/default.postfxlayercategories @@ -1,17 +1,30 @@ + - - + + - - + + + + + + + + + + - + + + + + diff --git a/Gems/HttpRequestor/Code/Source/HttpRequestManager.cpp b/Gems/HttpRequestor/Code/Source/HttpRequestManager.cpp index 8ed40c1599..523d84f0f5 100644 --- a/Gems/HttpRequestor/Code/Source/HttpRequestManager.cpp +++ b/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()) diff --git a/Gems/PhysX/Code/Tests/PhysXTestUtil.h b/Gems/PhysX/Code/Tests/PhysXTestUtil.h index b1ba23600a..8e81b9cb89 100644 --- a/Gems/PhysX/Code/Tests/PhysXTestUtil.h +++ b/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 {}; } diff --git a/Gems/Terrain/Assets/Shaders/Terrain/TerrainCommon.azsli b/Gems/Terrain/Assets/Shaders/Terrain/TerrainCommon.azsli index 6e489796d7..18b85bbd43 100644 --- a/Gems/Terrain/Assets/Shaders/Terrain/TerrainCommon.azsli +++ b/Gems/Terrain/Assets/Shaders/Terrain/TerrainCommon.azsli @@ -11,16 +11,16 @@ ShaderResourceGroup ObjectSrg : SRG_PerObject { Texture2D 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) diff --git a/Gems/Terrain/Code/CMakeLists.txt b/Gems/Terrain/Code/CMakeLists.txt index b4a35edbbf..0feacdaf71 100644 --- a/Gems/Terrain/Code/CMakeLists.txt +++ b/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() diff --git a/Gems/Terrain/Code/Source/Components/TerrainHeightGradientListComponent.cpp b/Gems/Terrain/Code/Source/Components/TerrainHeightGradientListComponent.cpp index d64f660d90..4d0cb6576d 100644 --- a/Gems/Terrain/Code/Source/Components/TerrainHeightGradientListComponent.cpp +++ b/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( diff --git a/Gems/Terrain/Code/Source/Components/TerrainHeightGradientListComponent.h b/Gems/Terrain/Code/Source/Components/TerrainHeightGradientListComponent.h index 7635680815..509f003afa 100644 --- a/Gems/Terrain/Code/Source/Components/TerrainHeightGradientListComponent.h +++ b/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 diff --git a/Gems/Terrain/Code/Source/Components/TerrainLayerSpawnerComponent.cpp b/Gems/Terrain/Code/Source/Components/TerrainLayerSpawnerComponent.cpp index e17dbd0e93..245c98ccac 100644 --- a/Gems/Terrain/Code/Source/Components/TerrainLayerSpawnerComponent.cpp +++ b/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()); diff --git a/Gems/Terrain/Code/Source/Components/TerrainLayerSpawnerComponent.h b/Gems/Terrain/Code/Source/Components/TerrainLayerSpawnerComponent.h index 3f8e72e1b8..c7398bf93e 100644 --- a/Gems/Terrain/Code/Source/Components/TerrainLayerSpawnerComponent.h +++ b/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; diff --git a/Gems/Terrain/Code/Source/Components/TerrainWorldComponent.cpp b/Gems/Terrain/Code/Source/Components/TerrainWorldComponent.cpp index 669a8f4b02..d8b7308ef7 100644 --- a/Gems/Terrain/Code/Source/Components/TerrainWorldComponent.cpp +++ b/Gems/Terrain/Code/Source/Components/TerrainWorldComponent.cpp @@ -13,6 +13,7 @@ #include #include #include +#include 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() diff --git a/Gems/Terrain/Code/Source/Components/TerrainWorldDebuggerComponent.cpp b/Gems/Terrain/Code/Source/Components/TerrainWorldDebuggerComponent.cpp index d7504295c2..89ffa6364f 100644 --- a/Gems/Terrain/Code/Source/Components/TerrainWorldDebuggerComponent.cpp +++ b/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 (*--*--*--). diff --git a/Gems/Terrain/Code/Source/TerrainSystem/TerrainSystem.cpp b/Gems/Terrain/Code/Source/TerrainSystem/TerrainSystem.cpp index a271d624f5..674af376e4 100644 --- a/Gems/Terrain/Code/Source/TerrainSystem/TerrainSystem.cpp +++ b/Gems/Terrain/Code/Source/TerrainSystem/TerrainSystem.cpp @@ -9,6 +9,7 @@ #include #include #include +#include #include #include @@ -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 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 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->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 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 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 lock(m_areaMutex); - m_registeredAreas.clear(); - } - - const AZ::RPI::Scene* scene = AZ::RPI::RPISystemInterface::Get()->GetDefaultScene().get(); - auto terrainFeatureProcessor = scene->GetFeatureProcessor(); - if (terrainFeatureProcessor) - { - terrainFeatureProcessor->RemoveTerrainData(); - } -} - void TerrainSystem::RegisterArea(AZ::EntityId areaId) { AZStd::unique_lock 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 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( @@ -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; diff --git a/Gems/Terrain/Code/Source/TerrainSystem/TerrainSystem.h b/Gems/Terrain/Code/Source/TerrainSystem/TerrainSystem.h index a75685fb12..c52165da6b 100644 --- a/Gems/Terrain/Code/Source/TerrainSystem/TerrainSystem.h +++ b/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; diff --git a/Gems/Terrain/Code/Source/TerrainSystem/TerrainSystemBus.h b/Gems/Terrain/Code/Source/TerrainSystem/TerrainSystemBus.h index cb41ba9957..1ba63a8f84 100644 --- a/Gems/Terrain/Code/Source/TerrainSystem/TerrainSystemBus.h +++ b/Gems/Terrain/Code/Source/TerrainSystem/TerrainSystemBus.h @@ -16,6 +16,8 @@ #include #include +#include + 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; - /** - * 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; /** * 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; diff --git a/Gems/Terrain/Code/Tests/LayerSpawnerTests.cpp b/Gems/Terrain/Code/Tests/LayerSpawnerTests.cpp index 91d6a26f75..f0533322c8 100644 --- a/Gems/Terrain/Code/Tests/LayerSpawnerTests.cpp +++ b/Gems/Terrain/Code/Tests/LayerSpawnerTests.cpp @@ -17,6 +17,10 @@ #include +using ::testing::NiceMock; +using ::testing::AtLeast; +using ::testing::_; + class LayerSpawnerComponentTest : public ::testing::Test { @@ -26,7 +30,7 @@ protected: AZStd::unique_ptr m_entity; Terrain::TerrainLayerSpawnerComponent* m_layerSpawnerComponent; UnitTest::MockBoxShapeComponent* m_shapeComponent; - AZStd::unique_ptr m_terrainSystem; + AZStd::unique_ptr> 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(); - m_terrainSystem->Activate(); + m_terrainSystem = AZStd::make_unique>(); } }; @@ -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(); } diff --git a/Gems/Terrain/Code/Tests/TerrainMocks.h b/Gems/Terrain/Code/Tests/TerrainMocks.h index 674104e26b..03a2eccf1d 100644 --- a/Gems/Terrain/Code/Tests/TerrainMocks.h +++ b/Gems/Terrain/Code/Tests/TerrainMocks.h @@ -7,7 +7,10 @@ */ #pragma once +#include + #include +#include #include 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)); + }; + } diff --git a/Gems/Terrain/Code/Tests/TerrainSystemTest.cpp b/Gems/Terrain/Code/Tests/TerrainSystemTest.cpp new file mode 100644 index 0000000000..3e557c66a6 --- /dev/null +++ b/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 +#include + +#include + +#include +#include + +using ::testing::AtLeast; +using ::testing::NiceMock; + +class TerrainSystemTest : public ::testing::Test +{ +protected: + AZ::ComponentApplication m_app; + + AZStd::unique_ptr m_entity; + AZStd::unique_ptr 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(); + m_entity->Init(); + + ASSERT_TRUE(m_entity); + } + + void ResetEntity() + { + m_entity->Deactivate(); + m_entity->Reset(); + } +}; + +TEST_F(TerrainSystemTest, TrivialCreateDestroy) +{ + m_terrainSystem = AZStd::make_unique(); +} + +TEST_F(TerrainSystemTest, TrivialActivateDeactivate) +{ + m_terrainSystem = AZStd::make_unique(); + m_terrainSystem->Activate(); + m_terrainSystem->Deactivate(); +} + +TEST_F(TerrainSystemTest, CreateEventsCalledOnActivation) +{ + NiceMock mockTerrainListener; + EXPECT_CALL(mockTerrainListener, OnTerrainDataCreateBegin()).Times(AtLeast(1)); + EXPECT_CALL(mockTerrainListener, OnTerrainDataCreateEnd()).Times(AtLeast(1)); + + m_terrainSystem = AZStd::make_unique(); + m_terrainSystem->Activate(); +} + +TEST_F(TerrainSystemTest, DestroyEventsCalledOnDeactivation) +{ + NiceMock mockTerrainListener; + EXPECT_CALL(mockTerrainListener, OnTerrainDataDestroyBegin()).Times(AtLeast(1)); + EXPECT_CALL(mockTerrainListener, OnTerrainDataDestroyEnd()).Times(AtLeast(1)); + + m_terrainSystem = AZStd::make_unique(); + m_terrainSystem->Activate(); + m_terrainSystem->Deactivate(); +} + + diff --git a/Gems/Terrain/Code/Tests/TerrainTest.cpp b/Gems/Terrain/Code/Tests/TerrainTest.cpp index 9b47c91a31..40217ff9bc 100644 --- a/Gems/Terrain/Code/Tests/TerrainTest.cpp +++ b/Gems/Terrain/Code/Tests/TerrainTest.cpp @@ -8,24 +8,4 @@ #include -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); diff --git a/Gems/Terrain/Code/terrain_tests_files.cmake b/Gems/Terrain/Code/terrain_tests_files.cmake index b44f143f3b..6d1cf97fd9 100644 --- a/Gems/Terrain/Code/terrain_tests_files.cmake +++ b/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 )