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/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/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