From 3b424bd5450716bfe35a5bf4b72a16025e07cf43 Mon Sep 17 00:00:00 2001 From: santorac <55155825+santorac@users.noreply.github.com> Date: Tue, 24 Aug 2021 15:40:47 -0700 Subject: [PATCH 1/3] Fixed a shader hot reload issue where the new root shader variant asset was not getting saved in the Shader object during OnAssetReloaded, it was only saved during OnAssetReady. MaterialHotReloadTest 06_VerticalPattern now passes. Signed-off-by: santorac <55155825+santorac@users.noreply.github.com> --- .../RPI/Code/Source/RPI.Public/Shader/Shader.cpp | 2 ++ .../Code/Source/RPI.Reflect/Shader/ShaderAsset.cpp | 14 +++----------- 2 files changed, 5 insertions(+), 11 deletions(-) diff --git a/Gems/Atom/RPI/Code/Source/RPI.Public/Shader/Shader.cpp b/Gems/Atom/RPI/Code/Source/RPI.Public/Shader/Shader.cpp index f6dcd02804..4d7acb0ea4 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Public/Shader/Shader.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Public/Shader/Shader.cpp @@ -175,6 +175,8 @@ namespace AZ /// ShaderVariantFinderNotificationBus overrides void Shader::OnShaderVariantAssetReady(Data::Asset shaderVariantAsset, bool isError) { + ShaderReloadDebugTracker::ScopedSection reloadSection("{%p}->Shader::OnShaderVariantAssetReady %s", this, shaderVariantAsset.GetHint().c_str()); + AZ_Assert(shaderVariantAsset, "Reloaded ShaderVariantAsset is null"); const ShaderVariantStableId stableId = shaderVariantAsset->GetStableId(); diff --git a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Shader/ShaderAsset.cpp b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Shader/ShaderAsset.cpp index adeb564675..81b9705e41 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Shader/ShaderAsset.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Shader/ShaderAsset.cpp @@ -587,6 +587,8 @@ namespace AZ { Data::Asset shaderVariantAsset = { asset.GetAs(), AZ::Data::AssetLoadBehavior::PreLoad }; AZ_Assert(shaderVariantAsset->GetStableId() == RootShaderVariantStableId, "Was expecting to update the root variant"); + SupervariantIndex supervariantIndex = GetSupervariantIndexFromAssetId(asset.GetId()); + GetCurrentShaderApiData().m_supervariants[supervariantIndex.GetIndex()].m_rootShaderVariantAsset = asset; ShaderReloadNotificationBus::Event(GetId(), &ShaderReloadNotificationBus::Events::OnShaderAssetReinitialized, Data::Asset{ this, AZ::Data::AssetLoadBehavior::PreLoad } ); } @@ -607,17 +609,7 @@ namespace AZ // so it continues using the old ShaderVariantAsset instead of the new one. // The OnAssetReady bus function is called automatically whenever a connection to AssetBus is made, so listening to this gives // us the opportunity to assign the appropriate ShaderVariantAsset. - - ShaderReloadNotificationBus::Event(GetId(), &ShaderReloadNotificationBus::Events::OnShaderAssetReinitialized, Data::Asset{ this, AZ::Data::AssetLoadBehavior::PreLoad } ); - - Data::Asset shaderVariantAsset = { asset.GetAs(), AZ::Data::AssetLoadBehavior::PreLoad }; - AZ_Assert(shaderVariantAsset->GetStableId() == RootShaderVariantStableId, - "Was expecting to update the root variant"); - SupervariantIndex supervariantIndex = GetSupervariantIndexFromAssetId(asset.GetId()); - GetCurrentShaderApiData().m_supervariants[supervariantIndex.GetIndex()].m_rootShaderVariantAsset = asset; - - ShaderReloadNotificationBus::Event(GetId(), &ShaderReloadNotificationBus::Events::OnShaderAssetReinitialized, Data::Asset{ this, AZ::Data::AssetLoadBehavior::PreLoad } ); - + ShaderReloadDebugTracker::ScopedSection reloadSection("{%p}->ShaderAsset::OnAssetReady %s", this, asset.GetHint().c_str()); ReinitializeRootShaderVariant(asset); } From 7aa24fd58f3bc43625c0b84b94f0c37c49905e26 Mon Sep 17 00:00:00 2001 From: santorac <55155825+santorac@users.noreply.github.com> Date: Thu, 26 Aug 2021 00:26:35 -0700 Subject: [PATCH 2/3] Fixed shader variant hot reload which was failing due to mismatched timestamps. The ShaderAsset was using microseconds and the ShaderVariantAsset was using system ticks. Since ticks will always be higher than microseconds, stale variants were not prevented from being used. Signed-off-by: santorac <55155825+santorac@users.noreply.github.com> --- .../Editor/AzslShaderBuilderSystemComponent.cpp | 4 ++-- .../Code/Source/Editor/ShaderAssetBuilder.cpp | 17 +++++++++-------- .../Source/Editor/ShaderVariantAssetBuilder.cpp | 6 ++++-- 3 files changed, 15 insertions(+), 12 deletions(-) diff --git a/Gems/Atom/Asset/Shader/Code/Source/Editor/AzslShaderBuilderSystemComponent.cpp b/Gems/Atom/Asset/Shader/Code/Source/Editor/AzslShaderBuilderSystemComponent.cpp index 16cebef6ac..cacb310918 100644 --- a/Gems/Atom/Asset/Shader/Code/Source/Editor/AzslShaderBuilderSystemComponent.cpp +++ b/Gems/Atom/Asset/Shader/Code/Source/Editor/AzslShaderBuilderSystemComponent.cpp @@ -81,7 +81,7 @@ namespace AZ // Register Shader Asset Builder AssetBuilderSDK::AssetBuilderDesc shaderAssetBuilderDescriptor; shaderAssetBuilderDescriptor.m_name = "Shader Asset Builder"; - shaderAssetBuilderDescriptor.m_version = 103; // ATOM-15058 + shaderAssetBuilderDescriptor.m_version = 104; // ATOM-15871 // .shader file changes trigger rebuilds shaderAssetBuilderDescriptor.m_patterns.push_back(AssetBuilderSDK::AssetBuilderPattern( AZStd::string::format("*.%s", RPI::ShaderSourceData::Extension), AssetBuilderSDK::AssetBuilderPattern::PatternType::Wildcard)); shaderAssetBuilderDescriptor.m_busId = azrtti_typeid(); @@ -96,7 +96,7 @@ namespace AZ shaderVariantAssetBuilderDescriptor.m_name = "Shader Variant Asset Builder"; // Both "Shader Variant Asset Builder" and "Shader Asset Builder" produce ShaderVariantAsset products. If you update // ShaderVariantAsset you will need to update BOTH version numbers, not just "Shader Variant Asset Builder". - shaderVariantAssetBuilderDescriptor.m_version = 24; // ATOM-15978 + shaderVariantAssetBuilderDescriptor.m_version = 25; // ATOM-15871 shaderVariantAssetBuilderDescriptor.m_patterns.push_back(AssetBuilderSDK::AssetBuilderPattern(AZStd::string::format("*.%s", RPI::ShaderVariantListSourceData::Extension), AssetBuilderSDK::AssetBuilderPattern::PatternType::Wildcard)); shaderVariantAssetBuilderDescriptor.m_busId = azrtti_typeid(); shaderVariantAssetBuilderDescriptor.m_createJobFunction = AZStd::bind(&ShaderVariantAssetBuilder::CreateJobs, &m_shaderVariantAssetBuilder, AZStd::placeholders::_1, AZStd::placeholders::_2); diff --git a/Gems/Atom/Asset/Shader/Code/Source/Editor/ShaderAssetBuilder.cpp b/Gems/Atom/Asset/Shader/Code/Source/Editor/ShaderAssetBuilder.cpp index 87eb6dc97d..2ebcb981fa 100644 --- a/Gems/Atom/Asset/Shader/Code/Source/Editor/ShaderAssetBuilder.cpp +++ b/Gems/Atom/Asset/Shader/Code/Source/Editor/ShaderAssetBuilder.cpp @@ -43,6 +43,7 @@ #include #include #include +#include #include "AzslCompiler.h" #include "ShaderVariantAssetBuilder.h" @@ -236,7 +237,9 @@ namespace AZ void ShaderAssetBuilder::ProcessJob(const AssetBuilderSDK::ProcessJobRequest& request, AssetBuilderSDK::ProcessJobResponse& response) const { - const AZStd::sys_time_t startTime = AZStd::GetTimeNowTicks(); + AZ::Debug::Timer timer; + timer.Stamp(); + AZStd::string shaderFullPath; AzFramework::StringFunc::Path::ConstructFull(request.m_watchFolder.c_str(), request.m_sourceFile.c_str(), shaderFullPath, true); // Save .shader file name (no extension and no parent directory path) @@ -283,6 +286,8 @@ namespace AZ } } + AZ_TracePrintf(ShaderAssetBuilderName, "Build Timestamp %zu", shaderAssetBuildTimestamp); + auto supervariantList = ShaderBuilderUtility::GetSupervariantListFromShaderSourceData(shaderSourceData); RPI::ShaderAssetCreator shaderAssetCreator; @@ -579,7 +584,7 @@ namespace AZ request.m_platformInfo, buildOptions.m_compilerArguments, request.m_tempDirPath, - startTime, + shaderAssetBuildTimestamp, shaderSourceData, *shaderOptionGroupLayout.get(), shaderEntryPoints, @@ -660,12 +665,8 @@ namespace AZ } response.m_resultCode = AssetBuilderSDK::ProcessJobResult_Success; - - const AZStd::sys_time_t endTime = AZStd::GetTimeNowTicks(); - const AZStd::sys_time_t deltaTime = endTime - startTime; - const float elapsedTimeSeconds = (float)(deltaTime) / (float)AZStd::GetTimeTicksPerSecond(); - - AZ_TracePrintf(ShaderAssetBuilderName, "Finished processing %s in %.2f seconds\n", request.m_sourceFile.c_str(), elapsedTimeSeconds); + + AZ_TracePrintf(ShaderAssetBuilderName, "Finished processing %s in %.2f seconds\n", request.m_sourceFile.c_str(), timer.GetDeltaTimeInSeconds()); ShaderBuilderUtility::LogProfilingData(ShaderAssetBuilderName, shaderFileName); } diff --git a/Gems/Atom/Asset/Shader/Code/Source/Editor/ShaderVariantAssetBuilder.cpp b/Gems/Atom/Asset/Shader/Code/Source/Editor/ShaderVariantAssetBuilder.cpp index 0e995992b9..8fcc22808b 100644 --- a/Gems/Atom/Asset/Shader/Code/Source/Editor/ShaderVariantAssetBuilder.cpp +++ b/Gems/Atom/Asset/Shader/Code/Source/Editor/ShaderVariantAssetBuilder.cpp @@ -743,7 +743,6 @@ namespace AZ void ShaderVariantAssetBuilder::ProcessShaderVariantJob(const AssetBuilderSDK::ProcessJobRequest& request, AssetBuilderSDK::ProcessJobResponse& response) const { - const AZStd::sys_time_t startTime = AZStd::GetTimeNowTicks(); AssetBuilderSDK::JobCancelListener jobCancelListener(request.m_jobId); AZStd::string fullPath; @@ -777,6 +776,9 @@ namespace AZ response.m_resultCode = AssetBuilderSDK::ProcessJobResult_Success; return; } + + const AZStd::sys_time_t shaderVariantAssetBuildTimestamp = AZStd::GetTimeNowMicroSecond(); + AZ_TracePrintf(ShaderVariantAssetBuilderName, "Build Timestamp %zu", shaderVariantAssetBuildTimestamp); auto supervariantList = ShaderBuilderUtility::GetSupervariantListFromShaderSourceData(shaderSourceDescriptor); @@ -911,7 +913,7 @@ namespace AZ ShaderVariantCreationContext shaderVariantCreationContext = { *shaderPlatformInterface, request.m_platformInfo, buildOptions.m_compilerArguments, request.m_tempDirPath, - startTime, + shaderVariantAssetBuildTimestamp, shaderSourceDescriptor, *shaderOptionGroupLayout.get(), shaderEntryPoints, From ccd648c60000f7c54f2af891e37760919fbc840e Mon Sep 17 00:00:00 2001 From: santorac <55155825+santorac@users.noreply.github.com> Date: Thu, 26 Aug 2021 08:51:28 -0700 Subject: [PATCH 3/3] Removed unnecessary print statements Signed-off-by: santorac <55155825+santorac@users.noreply.github.com> --- .../Asset/Shader/Code/Source/Editor/ShaderAssetBuilder.cpp | 4 +--- .../Shader/Code/Source/Editor/ShaderVariantAssetBuilder.cpp | 1 - 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/Gems/Atom/Asset/Shader/Code/Source/Editor/ShaderAssetBuilder.cpp b/Gems/Atom/Asset/Shader/Code/Source/Editor/ShaderAssetBuilder.cpp index 2ebcb981fa..8ec7733d07 100644 --- a/Gems/Atom/Asset/Shader/Code/Source/Editor/ShaderAssetBuilder.cpp +++ b/Gems/Atom/Asset/Shader/Code/Source/Editor/ShaderAssetBuilder.cpp @@ -286,8 +286,6 @@ namespace AZ } } - AZ_TracePrintf(ShaderAssetBuilderName, "Build Timestamp %zu", shaderAssetBuildTimestamp); - auto supervariantList = ShaderBuilderUtility::GetSupervariantListFromShaderSourceData(shaderSourceData); RPI::ShaderAssetCreator shaderAssetCreator; @@ -666,7 +664,7 @@ namespace AZ response.m_resultCode = AssetBuilderSDK::ProcessJobResult_Success; - AZ_TracePrintf(ShaderAssetBuilderName, "Finished processing %s in %.2f seconds\n", request.m_sourceFile.c_str(), timer.GetDeltaTimeInSeconds()); + AZ_TracePrintf(ShaderAssetBuilderName, "Finished processing %s in %.3f seconds\n", request.m_sourceFile.c_str(), timer.GetDeltaTimeInSeconds()); ShaderBuilderUtility::LogProfilingData(ShaderAssetBuilderName, shaderFileName); } diff --git a/Gems/Atom/Asset/Shader/Code/Source/Editor/ShaderVariantAssetBuilder.cpp b/Gems/Atom/Asset/Shader/Code/Source/Editor/ShaderVariantAssetBuilder.cpp index 8fcc22808b..5dc1267dbb 100644 --- a/Gems/Atom/Asset/Shader/Code/Source/Editor/ShaderVariantAssetBuilder.cpp +++ b/Gems/Atom/Asset/Shader/Code/Source/Editor/ShaderVariantAssetBuilder.cpp @@ -778,7 +778,6 @@ namespace AZ } const AZStd::sys_time_t shaderVariantAssetBuildTimestamp = AZStd::GetTimeNowMicroSecond(); - AZ_TracePrintf(ShaderVariantAssetBuilderName, "Build Timestamp %zu", shaderVariantAssetBuildTimestamp); auto supervariantList = ShaderBuilderUtility::GetSupervariantListFromShaderSourceData(shaderSourceDescriptor);