From bd8c53550a5e00f89f4f89107c0a7bd8c24cac52 Mon Sep 17 00:00:00 2001 From: mrieggeramzn <61609885+mrieggeramzn@users.noreply.github.com> Date: Tue, 14 Sep 2021 17:12:17 -0700 Subject: [PATCH 1/2] Replacing the old esm shadow blur with faster blur (#4095) * Replacing the old gaussian blur with a much faster, better quality kawase blur Signed-off-by: mrieggeramzn * Removing atomtesting outdated msg Signed-off-by: mrieggeramzn * Some recommendations from Tommy Signed-off-by: mrieggeramzn * Adding early termination from previous gaussian filtering algorithm that kawase replaces Signed-off-by: mrieggeramzn * Removing pcf method Signed-off-by: mrieggeramzn * removing the old blur and adding in the new kawase blur into .cmake file Signed-off-by: mrieggeramzn --- .../Common/Assets/Passes/EsmShadowmaps.pass | 19 +-- .../Assets/Passes/FilterDepthHorizontal.pass | 72 ---------- ...pthVertical.pass => KawaseShadowBlur.pass} | 10 +- .../Assets/Passes/PassTemplates.azasset | 14 +- .../Math/GaussianFilterFloatHorizontal.azsl | 60 -------- .../Math/GaussianFilterFloatHorizontal.shader | 16 --- .../Math/GaussianFilterFloatVertical.azsl | 60 -------- .../Shaders/Shadow/KawaseShadowBlur.azsl | 132 ++++++++++++++++++ .../KawaseShadowBlur.shader} | 2 +- .../atom_feature_common_asset_files.cmake | 9 +- .../Source/CoreLights/EsmShadowmapsPass.cpp | 52 ++++--- .../Source/CoreLights/EsmShadowmapsPass.h | 14 +- 12 files changed, 201 insertions(+), 259 deletions(-) delete mode 100644 Gems/Atom/Feature/Common/Assets/Passes/FilterDepthHorizontal.pass rename Gems/Atom/Feature/Common/Assets/Passes/{FilterDepthVertical.pass => KawaseShadowBlur.pass} (88%) delete mode 100644 Gems/Atom/Feature/Common/Assets/Shaders/Math/GaussianFilterFloatHorizontal.azsl delete mode 100644 Gems/Atom/Feature/Common/Assets/Shaders/Math/GaussianFilterFloatHorizontal.shader delete mode 100644 Gems/Atom/Feature/Common/Assets/Shaders/Math/GaussianFilterFloatVertical.azsl create mode 100644 Gems/Atom/Feature/Common/Assets/Shaders/Shadow/KawaseShadowBlur.azsl rename Gems/Atom/Feature/Common/Assets/Shaders/{Math/GaussianFilterFloatVertical.shader => Shadow/KawaseShadowBlur.shader} (79%) diff --git a/Gems/Atom/Feature/Common/Assets/Passes/EsmShadowmaps.pass b/Gems/Atom/Feature/Common/Assets/Passes/EsmShadowmaps.pass index 27b777d549..a3bd6604dc 100644 --- a/Gems/Atom/Feature/Common/Assets/Passes/EsmShadowmaps.pass +++ b/Gems/Atom/Feature/Common/Assets/Passes/EsmShadowmaps.pass @@ -40,9 +40,11 @@ } ] }, + { - "Name": "HorizontalGaussianFilter", - "TemplateName": "FilterDepthHorizontalTemplate", + "Name": "KawaseBlur0", + "TemplateName": "KawaseShadowBlurTemplate", + "Enabled": true, "Connections": [ { "LocalSlot": "Input", @@ -54,26 +56,27 @@ ] }, { - "Name": "VerticalGaussianFiter", - "TemplateName": "FilterDepthVerticalTemplate", + "Name": "KawaseBlur1", + "TemplateName": "KawaseShadowBlurTemplate", + "Enabled": true, "Connections": [ { "LocalSlot": "Input", "AttachmentRef": { - "Pass": "HorizontalGaussianFilter", + "Pass": "KawaseBlur0", "Attachment": "Output" } } ] - } + } ], "Connections": [ { "LocalSlot": "EsmShadowmaps", "AttachmentRef": { - "Pass": "VerticalGaussianFiter", + "Pass": "KawaseBlur1", "Attachment": "Output" - } + } } ] } diff --git a/Gems/Atom/Feature/Common/Assets/Passes/FilterDepthHorizontal.pass b/Gems/Atom/Feature/Common/Assets/Passes/FilterDepthHorizontal.pass deleted file mode 100644 index 2b2ae40fdc..0000000000 --- a/Gems/Atom/Feature/Common/Assets/Passes/FilterDepthHorizontal.pass +++ /dev/null @@ -1,72 +0,0 @@ -{ - "Type": "JsonSerialization", - "Version": 1, - "ClassName": "PassAsset", - "ClassData": { - "PassTemplate": { - "Name": "FilterDepthHorizontalTemplate", - "PassClass": "ComputePass", - "Slots": [ - { - "Name": "Input", - "SlotType": "Input", - "ShaderInputName": "m_inputImage", - "ScopeAttachmentUsage": "Shader", - "LoadStoreAction": { - "LoadAction": "Load", - "StoreAction": "DontCare" - }, - "ImageViewDesc": { - "IsArray": 1 - } - }, - { - "Name": "Output", - "SlotType": "Output", - "ShaderInputName": "m_outputImage", - "ScopeAttachmentUsage": "Shader", - "LoadStoreAction": { - "LoadAction": "DontCare", - "StoreAction": "Store" - }, - "ImageViewDesc": { - "IsArray": 1 - } - } - ], - "PassData": { - "$type": "ComputePassData", - "ShaderAsset": { - "FilePath": "Shaders/Math/GaussianFilterFloatHorizontal.shader" - } - }, - "ImageAttachments": [ - { - "Name": "HorizontalFiltered", - "SizeSource": { - "Source": { - "Pass": "This", - "Attachment": "Input" - } - }, - "ArraySizeSource": { - "Pass": "This", - "Attachment": "Input" - }, - "ImageDescriptor": { - "Format": "R32_FLOAT" - } - } - ], - "Connections": [ - { - "localSlot": "Output", - "AttachmentRef": { - "Pass": "This", - "Attachment": "HorizontalFiltered" - } - } - ] - } - } -} diff --git a/Gems/Atom/Feature/Common/Assets/Passes/FilterDepthVertical.pass b/Gems/Atom/Feature/Common/Assets/Passes/KawaseShadowBlur.pass similarity index 88% rename from Gems/Atom/Feature/Common/Assets/Passes/FilterDepthVertical.pass rename to Gems/Atom/Feature/Common/Assets/Passes/KawaseShadowBlur.pass index 5d8a4de21b..46ef8ba8ae 100644 --- a/Gems/Atom/Feature/Common/Assets/Passes/FilterDepthVertical.pass +++ b/Gems/Atom/Feature/Common/Assets/Passes/KawaseShadowBlur.pass @@ -4,7 +4,7 @@ "ClassName": "PassAsset", "ClassData": { "PassTemplate": { - "Name": "FilterDepthVerticalTemplate", + "Name": "KawaseShadowBlurTemplate", "PassClass": "ComputePass", "Slots": [ { @@ -28,7 +28,7 @@ "LoadStoreAction": { "LoadAction": "DontCare", "StoreAction": "Store" - }, + }, "ImageViewDesc": { "IsArray": 1 } @@ -37,12 +37,12 @@ "PassData": { "$type": "ComputePassData", "ShaderAsset": { - "FilePath": "Shaders/Math/GaussianFilterFloatVertical.shader" + "FilePath": "Shaders/Shadow/KawaseShadowBlur.shader" } }, "ImageAttachments": [ { - "Name": "VerticalFiltered", + "Name": "FilteredImage", "SizeSource": { "Source": { "Pass": "This", @@ -63,7 +63,7 @@ "localSlot": "Output", "AttachmentRef": { "Pass": "This", - "Attachment": "VerticalFiltered" + "Attachment": "FilteredImage" } } ] diff --git a/Gems/Atom/Feature/Common/Assets/Passes/PassTemplates.azasset b/Gems/Atom/Feature/Common/Assets/Passes/PassTemplates.azasset index 57d35fb48d..8b7d6a8438 100644 --- a/Gems/Atom/Feature/Common/Assets/Passes/PassTemplates.azasset +++ b/Gems/Atom/Feature/Common/Assets/Passes/PassTemplates.azasset @@ -184,14 +184,6 @@ "Name": "DepthOfFieldWriteFocusDepthFromGpuTemplate", "Path": "Passes/DepthOfFieldWriteFocusDepthFromGpu.pass" }, - { - "Name": "FilterDepthHorizontalTemplate", - "Path": "Passes/FilterDepthHorizontal.pass" - }, - { - "Name": "FilterDepthVerticalTemplate", - "Path": "Passes/FilterDepthVertical.pass" - }, { "Name": "EsmShadowmapsTemplate", "Path": "Passes/EsmShadowmaps.pass" @@ -503,7 +495,11 @@ { "Name": "LowEndPipelineTemplate", "Path": "Passes/LowEndPipeline.pass" - } + }, + { + "Name": "KawaseShadowBlurTemplate", + "Path": "Passes/KawaseShadowBlur.pass" + } ] } } diff --git a/Gems/Atom/Feature/Common/Assets/Shaders/Math/GaussianFilterFloatHorizontal.azsl b/Gems/Atom/Feature/Common/Assets/Shaders/Math/GaussianFilterFloatHorizontal.azsl deleted file mode 100644 index 353f08427d..0000000000 --- a/Gems/Atom/Feature/Common/Assets/Shaders/Math/GaussianFilterFloatHorizontal.azsl +++ /dev/null @@ -1,60 +0,0 @@ -/* - * 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 - * - */ - -// [GFX TODO][ATOM-3365] optimization using intermediary results in groupshared memory. - -#include -#include -#include - -[numthreads(16,16,1)] -void MainCS(uint3 dispatchId: SV_DispatchThreadID) -{ - const float3 inputSize = GetImageSize(FilterPassSrg::m_inputImage); - const float3 outputSize = GetImageSize(FilterPassSrg::m_outputImage); - - const uint shadowmapIndex = GetShadowmapIndex( - FilterPassSrg::m_shadowmapIndexTable, - dispatchId, - inputSize.x); - // Early return if thread is outside of shadowmaps. - if (shadowmapIndex == ~0) - { - return; - } - const FilterParameter filterParameter = FilterPassSrg::m_filterParameters[shadowmapIndex]; - const uint shadowmapSize = filterParameter.m_shadowmapSize; - // Early return if filter is disabled. - if (!filterParameter.m_isEnabled || shadowmapSize <= 1) - { - return; // early return if filter parameter is empty. - } - - const uint sourceMin = filterParameter.m_shadowmapOriginInSlice.x; - const uint sourceMax = sourceMin + shadowmapSize - 1; - - uint filterTableSize = 0; - FilterPassSrg::m_filterTable.GetDimensions(filterTableSize); - if (filterTableSize == 0 || filterParameter.m_parameterCount == 0) - { - return; // If filter parameter is empty, early return. - } - - // [GFX TODO][ATOM-5676] pass proper source min/max for each shadowmap - const float result = FilteredFloat( - dispatchId, - FilterPassSrg::m_inputImage, - uint2(1, 0), // horizontal - sourceMin, - sourceMax, - FilterPassSrg::m_filterTable, - filterParameter.m_parameterOffset, - filterParameter.m_parameterCount); - - FilterPassSrg::m_outputImage[dispatchId].r = result; -} diff --git a/Gems/Atom/Feature/Common/Assets/Shaders/Math/GaussianFilterFloatHorizontal.shader b/Gems/Atom/Feature/Common/Assets/Shaders/Math/GaussianFilterFloatHorizontal.shader deleted file mode 100644 index 6998fd000c..0000000000 --- a/Gems/Atom/Feature/Common/Assets/Shaders/Math/GaussianFilterFloatHorizontal.shader +++ /dev/null @@ -1,16 +0,0 @@ -{ - "Source" : "GaussianFilterFloatHorizontal", - - "DrawList" : "shadow", - - "ProgramSettings": - { - "EntryPoints": - [ - { - "name": "MainCS", - "type": "Compute" - } - ] - } -} diff --git a/Gems/Atom/Feature/Common/Assets/Shaders/Math/GaussianFilterFloatVertical.azsl b/Gems/Atom/Feature/Common/Assets/Shaders/Math/GaussianFilterFloatVertical.azsl deleted file mode 100644 index 11c7d04bb1..0000000000 --- a/Gems/Atom/Feature/Common/Assets/Shaders/Math/GaussianFilterFloatVertical.azsl +++ /dev/null @@ -1,60 +0,0 @@ -/* - * 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 - * - */ - -// [GFX TODO][ATOM-3365] optimization using intermediary results in groupshared memory. - -#include -#include -#include - -[numthreads(16,16,1)] -void MainCS(uint3 dispatchId: SV_DispatchThreadID) -{ - const float3 inputSize = GetImageSize(FilterPassSrg::m_inputImage); - const float3 outputSize = GetImageSize(FilterPassSrg::m_outputImage); - - const uint shadowmapIndex = GetShadowmapIndex( - FilterPassSrg::m_shadowmapIndexTable, - dispatchId, - inputSize.x); - // Early return if thread is outside of shadowmaps. - if (shadowmapIndex == ~0) - { - return; - } - const FilterParameter filterParameter = FilterPassSrg::m_filterParameters[shadowmapIndex]; - const uint shadowmapSize = filterParameter.m_shadowmapSize; - // Early return if filter is disabled. - if (!filterParameter.m_isEnabled || shadowmapSize <= 1) - { - return; // early return if filter parameter is empty. - } - - const uint sourceMin = filterParameter.m_shadowmapOriginInSlice.y; - const uint sourceMax = sourceMin + shadowmapSize - 1; - - uint filterTableSize = 0; - FilterPassSrg::m_filterTable.GetDimensions(filterTableSize); - if (filterTableSize == 0 || filterParameter.m_parameterCount == 0) - { - return; // If filter parameter is empty, early return. - } - - // [GFX TODO][ATOM-5676] pass proper source min/max for each shadowmap - const float result = FilteredFloat( - dispatchId, - FilterPassSrg::m_inputImage, - uint2(0, 1), // vertical - sourceMin, - sourceMax, - FilterPassSrg::m_filterTable, - filterParameter.m_parameterOffset, - filterParameter.m_parameterCount); - - FilterPassSrg::m_outputImage[dispatchId].r = result; -} diff --git a/Gems/Atom/Feature/Common/Assets/Shaders/Shadow/KawaseShadowBlur.azsl b/Gems/Atom/Feature/Common/Assets/Shaders/Shadow/KawaseShadowBlur.azsl new file mode 100644 index 0000000000..9b480a4e40 --- /dev/null +++ b/Gems/Atom/Feature/Common/Assets/Shaders/Shadow/KawaseShadowBlur.azsl @@ -0,0 +1,132 @@ +/* + * 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 + * + */ + +// [GFX TODO][ATOM-3365] optimization using intermediary results in groupshared memory. + +// This shader blurs the ESM results using a multi-pass kawase filter. +// It should generally be faster than separable gaussian blur +// https://software.intel.com/content/www/us/en/develop/blogs/an-investigation-of-fast-real-time-gpu-based-image-blur-algorithms.html + +#include +#include +#include +#include + +ShaderResourceGroup FilterPassSrg : SRG_PerPass +{ + // This shader filters multiple images with distinct filter parameters. + // So, the input and output are arrays of texture2Ds. + Texture2DArray m_inputImage; + RWTexture2DArray m_outputImage; + + // This can convert a coordinate in an atlas to + // the shadowmap index. + Buffer m_shadowmapIndexTable; + + // This contains parameters related to filtering. + StructuredBuffer m_filterParameters; + + // x and y contain the inverse of the texture map resolution, z contains the kawase iteration + // i.e. a two pass kawase blur passes in 0 for the 1st pass and 1 for the second pass + float4 m_rcpResolutionAndIteration; + + Sampler LinearSampler + { + MinFilter = Linear; + MagFilter = Linear; + MipFilter = Linear; + AddressU = Clamp; + AddressV = Clamp; + AddressW = Clamp; + }; +} + +void CalculateBlurBoundaries(const uint shadowmapIndex, out float2 sourceMinTex, out float2 sourceMaxTex) +{ + const float2 rcpPixelSize = FilterPassSrg::m_rcpResolutionAndIteration.xy; + + const FilterParameter filterParameter = FilterPassSrg::m_filterParameters[shadowmapIndex]; + const uint shadowmapSize = filterParameter.m_shadowmapSize; + + // location of the shadow bounds in texels + const uint2 sourceMinPixel = filterParameter.m_shadowmapOriginInSlice.xy; + const uint2 sourceMaxPixel = sourceMinPixel + shadowmapSize - 1; + + // location of the shadow bounds in uv space + sourceMinTex = (sourceMinPixel + 0.5f) * rcpPixelSize; + sourceMaxTex = (sourceMaxPixel + 0.5f) * rcpPixelSize; +} + +float AccumulateShadowSamples(Texture2DArray tex, float3 texCoord, SamplerState s) +{ + float4 values = tex.GatherRed(s, texCoord); + float result = values.x + values.y + values.z + values.w; + return result; +} + +[numthreads(16,16,1)] +void MainCS(uint3 dispatchId: SV_DispatchThreadID) +{ + const float inputSize = GetImageSize(FilterPassSrg::m_inputImage).x; + const uint shadowmapIndex = GetShadowmapIndex( + FilterPassSrg::m_shadowmapIndexTable, + dispatchId, + inputSize); + + // Early return if thread is outside of shadowmaps. + if (shadowmapIndex == ~0) + { + return; + } + + const FilterParameter filterParameter = FilterPassSrg::m_filterParameters[shadowmapIndex]; + const uint shadowmapSize = filterParameter.m_shadowmapSize; + // Early return if filter is disabled. + if (!filterParameter.m_isEnabled || shadowmapSize <= 1) + { + return; // early return if filter parameter is empty. + } + + const float2 rcpPixelSize = FilterPassSrg::m_rcpResolutionAndIteration.xy; + const float blurIteration = FilterPassSrg::m_rcpResolutionAndIteration.z; + + float2 sourceMinTex, sourceMaxTex; + CalculateBlurBoundaries(shadowmapIndex, sourceMinTex, sourceMaxTex); + + const float2 halfRcpPixelSize = rcpPixelSize / 2.0f; + const float2 dUV = rcpPixelSize.xy * blurIteration + halfRcpPixelSize.xy; + const float2 texCoord = (dispatchId.xy + 0.5f) * rcpPixelSize; + + const float3 texCoordSamples[4] = { + float3(texCoord.x - dUV.x, texCoord.y - dUV.y, dispatchId.z), + float3(texCoord.x - dUV.x, texCoord.y + dUV.y, dispatchId.z), + float3(texCoord.x + dUV.x, texCoord.y - dUV.y, dispatchId.z), + float3(texCoord.x + dUV.x, texCoord.y + dUV.y, dispatchId.z), + }; + + float accumulatedBlur = 0; + float numSamplesAccumulated = 0; + for(int i = 0 ; i < 4; ++i) + { + if (texCoordSamples[i].x >= sourceMinTex.x && + texCoordSamples[i].y >= sourceMinTex.y && + texCoordSamples[i].x < sourceMaxTex.x && + texCoordSamples[i].y < sourceMaxTex.y) + { + // we should be tapping the location directly in between 4 adjacent texels + accumulatedBlur += AccumulateShadowSamples(FilterPassSrg::m_inputImage, texCoordSamples[i], FilterPassSrg::LinearSampler); + numSamplesAccumulated += 4; + } + } + + if (numSamplesAccumulated > 0) + { + float result = accumulatedBlur / numSamplesAccumulated; + FilterPassSrg::m_outputImage[dispatchId].r = result; + } +} diff --git a/Gems/Atom/Feature/Common/Assets/Shaders/Math/GaussianFilterFloatVertical.shader b/Gems/Atom/Feature/Common/Assets/Shaders/Shadow/KawaseShadowBlur.shader similarity index 79% rename from Gems/Atom/Feature/Common/Assets/Shaders/Math/GaussianFilterFloatVertical.shader rename to Gems/Atom/Feature/Common/Assets/Shaders/Shadow/KawaseShadowBlur.shader index db303e1ea7..dacacdafff 100644 --- a/Gems/Atom/Feature/Common/Assets/Shaders/Math/GaussianFilterFloatVertical.shader +++ b/Gems/Atom/Feature/Common/Assets/Shaders/Shadow/KawaseShadowBlur.shader @@ -1,5 +1,5 @@ { - "Source" : "GaussianFilterFloatVertical", + "Source" : "KawaseShadowBlur", "DrawList" : "shadow", diff --git a/Gems/Atom/Feature/Common/Assets/atom_feature_common_asset_files.cmake b/Gems/Atom/Feature/Common/Assets/atom_feature_common_asset_files.cmake index e43498c55d..8435625833 100644 --- a/Gems/Atom/Feature/Common/Assets/atom_feature_common_asset_files.cmake +++ b/Gems/Atom/Feature/Common/Assets/atom_feature_common_asset_files.cmake @@ -137,8 +137,6 @@ set(FILES Passes/FastDepthAwareBlur.pass Passes/FastDepthAwareBlurHor.pass Passes/FastDepthAwareBlurVer.pass - Passes/FilterDepthHorizontal.pass - Passes/FilterDepthVertical.pass Passes/Forward.pass Passes/ForwardCheckerboard.pass Passes/ForwardMSAA.pass @@ -146,6 +144,7 @@ set(FILES Passes/FullscreenCopy.pass Passes/FullscreenOutputOnly.pass Passes/ImGui.pass + Passes/KawaseShadowBlur.pass Passes/LightAdaptationParent.pass Passes/LightCulling.pass Passes/LightCullingHeatmap.pass @@ -331,10 +330,6 @@ set(FILES Shaders/LightCulling/LightCullingTilePrepare.shader Shaders/LuxCore/RenderTexture.azsl Shaders/LuxCore/RenderTexture.shader - Shaders/Math/GaussianFilterFloatHorizontal.azsl - Shaders/Math/GaussianFilterFloatHorizontal.shader - Shaders/Math/GaussianFilterFloatVertical.azsl - Shaders/Math/GaussianFilterFloatVertical.shader Shaders/MorphTargets/MorphTargetCS.azsl Shaders/MorphTargets/MorphTargetCS.shader Shaders/MorphTargets/MorphTargetSRG.azsli @@ -457,6 +452,8 @@ set(FILES Shaders/ScreenSpace/DeferredFog.shader Shaders/Shadow/DepthExponentiation.azsl Shaders/Shadow/DepthExponentiation.shader + Shaders/Shadow/KawaseShadowBlur.azsl + Shaders/Shadow/KawaseShadowBlur.shader Shaders/Shadow/Shadowmap.azsl Shaders/Shadow/Shadowmap.shader Shaders/SkinnedMesh/LinearSkinningCS.azsl diff --git a/Gems/Atom/Feature/Common/Code/Source/CoreLights/EsmShadowmapsPass.cpp b/Gems/Atom/Feature/Common/Code/Source/CoreLights/EsmShadowmapsPass.cpp index 6948eb598e..99eaa8a919 100644 --- a/Gems/Atom/Feature/Common/Code/Source/CoreLights/EsmShadowmapsPass.cpp +++ b/Gems/Atom/Feature/Common/Code/Source/CoreLights/EsmShadowmapsPass.cpp @@ -130,32 +130,17 @@ namespace AZ const AZStd::array_view>& children = GetChildren(); AZ_Assert(children.size() == EsmChildPassKindCount, "[EsmShadowmapsPass '%s'] The count of children is wrong.", GetPathName().GetCStr()); - for (uint32_t index = 0; index < EsmChildPassKindCount; ++index) + for (uint32_t childPassIndex = 0; childPassIndex < EsmChildPassKindCount; ++childPassIndex) { - RPI::ComputePass* child = azrtti_cast(children[index].get()); + RPI::ComputePass* child = azrtti_cast(children[childPassIndex].get()); AZ_Assert(child, "[EsmShadowmapsPass '%s'] A child does not compute.", GetPathName().GetCStr()); Data::Instance srg = child->GetShaderResourceGroup(); - if (m_shadowmapIndexTableBufferIndices[index].IsNull()) + SetBlurParameters(srg, childPassIndex); + if (childPassIndex >= aznumeric_cast(EsmChildPassKind::KawaseBlur0)) { - m_shadowmapIndexTableBufferIndices[index] = srg->FindShaderInputBufferIndex(Name("m_shadowmapIndexTable")); - } - srg->SetBuffer(m_shadowmapIndexTableBufferIndices[index], m_shadowmapIndexTableBuffer); - - if (m_filterParameterBufferIndices[index].IsNull()) - { - m_filterParameterBufferIndices[index] = srg->FindShaderInputBufferIndex(Name("m_filterParameters")); - } - srg->SetBuffer(m_filterParameterBufferIndices[index], m_filterParameterBuffer); - - if (index != static_cast(EsmChildPassKind::Exponentiation)) - { - if (m_filterTableBufferIndices[index].IsNull()) - { - m_filterTableBufferIndices[index] = srg->FindShaderInputBufferIndex(Name("m_filterTable")); - } - srg->SetBuffer(m_filterTableBufferIndices[index], m_filterTableBuffer); + SetKawaseBlurSpecificParameters(srg, childPassIndex - aznumeric_cast(EsmChildPassKind::KawaseBlur0)); } child->SetTargetThreadCounts( @@ -165,5 +150,32 @@ namespace AZ } } + void EsmShadowmapsPass::SetBlurParameters(Data::Instance srg, const uint32_t childPassIndex) + { + if (m_shadowmapIndexTableBufferIndices[childPassIndex].IsNull()) + { + m_shadowmapIndexTableBufferIndices[childPassIndex] = srg->FindShaderInputBufferIndex(Name("m_shadowmapIndexTable")); + } + srg->SetBuffer(m_shadowmapIndexTableBufferIndices[childPassIndex], m_shadowmapIndexTableBuffer); + + if (m_filterParameterBufferIndices[childPassIndex].IsNull()) + { + m_filterParameterBufferIndices[childPassIndex] = srg->FindShaderInputBufferIndex(Name("m_filterParameters")); + } + srg->SetBuffer(m_filterParameterBufferIndices[childPassIndex], m_filterParameterBuffer); + } + + void EsmShadowmapsPass::SetKawaseBlurSpecificParameters(Data::Instance srg, uint32_t kawaseBlurIndex) + { + if (m_kawaseBlurConstantIndices[kawaseBlurIndex].IsNull()) + { + m_kawaseBlurConstantIndices[kawaseBlurIndex] = srg->FindShaderInputConstantIndex(Name("m_rcpResolutionAndIteration")); + } + const AZ::Vector4 data( + 1.0f / m_shadowmapImageSize.m_width, 1.0f / m_shadowmapImageSize.m_height, aznumeric_cast(kawaseBlurIndex), 0.0f); + + srg->SetConstant(m_kawaseBlurConstantIndices[kawaseBlurIndex], data); + } + } // namespace Render } // namespace AZ diff --git a/Gems/Atom/Feature/Common/Code/Source/CoreLights/EsmShadowmapsPass.h b/Gems/Atom/Feature/Common/Code/Source/CoreLights/EsmShadowmapsPass.h index 236855c8c5..6e5e1aa311 100644 --- a/Gems/Atom/Feature/Common/Code/Source/CoreLights/EsmShadowmapsPass.h +++ b/Gems/Atom/Feature/Common/Code/Source/CoreLights/EsmShadowmapsPass.h @@ -21,12 +21,17 @@ namespace AZ { + namespace RPI + { + class ShaderResourceGroup; + } + namespace Render { AZ_ENUM_CLASS_WITH_UNDERLYING_TYPE(EsmChildPassKind, uint32_t, (Exponentiation, 0), - HorizontalFilter, - VerticalFilter); + KawaseBlur0, + KawaseBlur1); //! This pass outputs filtered shadowmap images used in ESM. //! ESM is an abbreviation of Exponential Shadow Maps. @@ -88,6 +93,9 @@ namespace AZ void FrameBeginInternal(FramePrepareParams params) override; void UpdateChildren(); + // Parameters for both the depth exponentiation pass along with the kawase blur passes + void SetBlurParameters(Data::Instance srg, const uint32_t childPassIndex); + void SetKawaseBlurSpecificParameters(Data::Instance srg, const uint32_t kawaseBlurIndex); bool m_computationEnabled = false; Name m_lightTypeName; @@ -102,6 +110,8 @@ namespace AZ Data::Instance m_shadowmapIndexTableBuffer; AZStd::array m_filterParameterBufferIndices; Data::Instance m_filterParameterBuffer; + + AZStd::array m_kawaseBlurConstantIndices; }; } // namespace Render } // namespace AZ From ce72e32cfc640af8029194578fe1ecf4766c8d49 Mon Sep 17 00:00:00 2001 From: lumberyard-employee-dm <56135373+lumberyard-employee-dm@users.noreply.github.com> Date: Tue, 14 Sep 2021 19:21:29 -0500 Subject: [PATCH 2/2] Fixed issue where two Paths could compare equal to each other, but hash differently (#4126) * Fixed issue where two Paths could compare equal to each other, but hash differently This issue is caused by the Path comparison logic using the path separator of the left path in a comparison of two paths(left and right) to determine whether the PathComparison is case-sensitive or not. The logic has been updated to only perform a non-case-sensitive path comparison if both paths are using the WindowsPathSeperator of `\` Also fixed issue with the Hashing algorihtm of the Path class to always hash the root directory as if it is `/`. This allows a path of "C:\foo" and "C:/foo" to hash to the equivalent value. Signed-off-by: lumberyard-employee-dm <56135373+lumberyard-employee-dm@users.noreply.github.com> * MS Build Tools 14.29 workaround around suppressing warnings using the external header feature Signed-off-by: lumberyard-employee-dm <56135373+lumberyard-employee-dm@users.noreply.github.com> --- Code/Framework/AzCore/AzCore/IO/Path/Path.h | 4 +- Code/Framework/AzCore/AzCore/IO/Path/Path.inl | 55 +++++--------- .../AzCore/AzCore/IO/Path/PathParser.inl | 56 ++++++++++++-- .../AzCore/Tests/IO/Path/PathTests.cpp | 76 +++++++++++++++++++ 4 files changed, 148 insertions(+), 43 deletions(-) diff --git a/Code/Framework/AzCore/AzCore/IO/Path/Path.h b/Code/Framework/AzCore/AzCore/IO/Path/Path.h index c0c4b1c974..24f26daa51 100644 --- a/Code/Framework/AzCore/AzCore/IO/Path/Path.h +++ b/Code/Framework/AzCore/AzCore/IO/Path/Path.h @@ -273,7 +273,7 @@ namespace AZ::IO // If the path input = 'D:bar', then the new PathIterable parts = [D:, 'bar' ] static constexpr void AppendNormalPathParts(PathIterable& pathIterableResult, const AZ::IO::PathView& path) noexcept; - constexpr int compare_string_view(AZStd::string_view other) const; + constexpr int ComparePathView(const PathView& other) const; constexpr AZStd::string_view root_name_view() const; constexpr AZStd::string_view root_directory_view() const; constexpr AZStd::string_view root_path_raw_view() const; @@ -480,6 +480,8 @@ namespace AZ::IO // compare //! Performs a compare of each of the path parts for equivalence //! Each part of the path is compare using string comparison + //! If both *this path and the input path uses the WindowsPathSeparator + //! then a non-case sensitive compare is performed //! Ex: Comparing "test/foo" against "test/fop" returns -1; //! Path separators of the contained path string aren't compared //! Ex. Comparing "C:/test\foo" against C:\test/foo" returns 0; diff --git a/Code/Framework/AzCore/AzCore/IO/Path/Path.inl b/Code/Framework/AzCore/AzCore/IO/Path/Path.inl index 40cbf6f46b..0147ad3356 100644 --- a/Code/Framework/AzCore/AzCore/IO/Path/Path.inl +++ b/Code/Framework/AzCore/AzCore/IO/Path/Path.inl @@ -224,15 +224,15 @@ namespace AZ::IO // compare constexpr int PathView::Compare(const PathView& other) const noexcept { - return compare_string_view(other.m_path); + return ComparePathView(other); } constexpr int PathView::Compare(AZStd::string_view pathView) const noexcept { - return compare_string_view(pathView); + return ComparePathView(PathView(pathView, m_preferred_separator)); } constexpr int PathView::Compare(const value_type* path) const noexcept { - return compare_string_view(path); + return ComparePathView(PathView(path, m_preferred_separator)); } constexpr AZStd::fixed_string PathView::FixedMaxPathString() const noexcept @@ -398,10 +398,10 @@ namespace AZ::IO return true; } - constexpr int PathView::compare_string_view(AZStd::string_view pathView) const + constexpr int PathView::ComparePathView(const PathView& other) const { auto lhsPathParser = parser::PathParser::CreateBegin(m_path, m_preferred_separator); - auto rhsPathParser = parser::PathParser::CreateBegin(pathView, m_preferred_separator); + auto rhsPathParser = parser::PathParser::CreateBegin(other.m_path, other.m_preferred_separator); if (int res = CompareRootName(&lhsPathParser, &rhsPathParser); res != 0) { @@ -476,6 +476,8 @@ namespace AZ::IO template constexpr void PathView::MakeRelativeTo(PathResultType& pathResult, const AZ::IO::PathView& path, const AZ::IO::PathView& base) { + const bool exactCaseCompare = path.m_preferred_separator == PosixPathSeparator + || base.m_preferred_separator == PosixPathSeparator; { // perform root-name/root-directory mismatch checks auto pathParser = parser::PathParser::CreateBegin(path.m_path, path.m_preferred_separator); @@ -487,7 +489,7 @@ namespace AZ::IO }; if (pathParser.InRootName() && pathParserBase.InRootName()) { - if (int res = Internal::ComparePathSegment(*pathParser, *pathParserBase, pathParser.m_preferred_separator); + if (int res = Internal::ComparePathSegment(*pathParser, *pathParserBase, exactCaseCompare); res != 0) { pathResult.m_path = AZStd::string_view{}; @@ -519,7 +521,7 @@ namespace AZ::IO auto pathParser = parser::PathParser::CreateBegin(path.m_path, path.m_preferred_separator); auto pathParserBase = parser::PathParser::CreateBegin(base.m_path, base.m_preferred_separator); while (pathParser && pathParserBase && pathParser.m_parser_state == pathParserBase.m_parser_state && - Internal::ComparePathSegment(*pathParser, *pathParserBase, pathParser.m_preferred_separator) == 0) + Internal::ComparePathSegment(*pathParser, *pathParserBase, exactCaseCompare) == 0) { ++pathParser; ++pathParserBase; @@ -1080,25 +1082,25 @@ namespace AZ::IO template constexpr int BasicPath::Compare(const PathView& other) const noexcept { - return static_cast(*this).compare_string_view(other.m_path); + return static_cast(*this).ComparePathView(other); } template constexpr int BasicPath::Compare(const string_type& pathString) const { - return static_cast(*this).compare_string_view(pathString); + return static_cast(*this).ComparePathView(PathView(pathString, m_preferred_separator)); } template constexpr int BasicPath::Compare(AZStd::string_view pathView) const noexcept { - return static_cast(*this).compare_string_view(pathView); + return static_cast(*this).ComparePathView(pathView); } template constexpr int BasicPath::Compare(const value_type* pathString) const noexcept { - return static_cast(*this).compare_string_view(pathString); + return static_cast(*this).ComparePathView(pathString); } // decomposition @@ -1330,10 +1332,12 @@ namespace AZ::IO // PathView::LexicallyRelative is not being used as it returns a FixedMaxPath // which has a limitation that it requires the relative path to fit within // an AZ::IO::MaxPathLength buffer - auto ComparePathPart = [pathSeparator = m_preferred_separator]( + const bool exactCaseCompare = m_preferred_separator == PosixPathSeparator + || base.m_preferred_separator == PosixPathSeparator; + auto ComparePathPart = [exactCaseCompare]( const PathIterable::PartKindPair& left, const PathIterable::PartKindPair& right) -> bool { - return Internal::ComparePathSegment(left.first, right.first, pathSeparator) == 0; + return Internal::ComparePathSegment(left.first, right.first, exactCaseCompare) == 0; }; const PathIterable thisPathParts = GetNormalPathParts(*this); @@ -1471,37 +1475,16 @@ namespace AZStd template <> struct hash { - /// Path is using FNV-1a algorithm 64 bit version. - static size_t hash_path(AZStd::string_view pathSegment, const char pathSeparator) - { - size_t hash = 14695981039346656037ULL; - constexpr size_t fnvPrime = 1099511628211ULL; - - for (const char first : pathSegment) - { - hash ^= static_cast((pathSeparator == AZ::IO::PosixPathSeparator) - ? first : tolower(first)); - hash *= fnvPrime; - } - return hash; - } - size_t operator()(const AZ::IO::PathView& pathToHash) noexcept { auto pathParser = AZ::IO::parser::PathParser::CreateBegin(pathToHash.Native(), pathToHash.m_preferred_separator); - size_t hash_value = 0; - while (pathParser) - { - AZStd::hash_combine(hash_value, hash_path(*pathParser, pathToHash.m_preferred_separator)); - ++pathParser; - } - return hash_value; + return AZ::IO::parser::HashPath(pathParser); } }; template struct hash> { - const size_t operator()(const AZ::IO::BasicPath& pathToHash) noexcept + size_t operator()(const AZ::IO::BasicPath& pathToHash) noexcept { return AZStd::hash{}(pathToHash); } diff --git a/Code/Framework/AzCore/AzCore/IO/Path/PathParser.inl b/Code/Framework/AzCore/AzCore/IO/Path/PathParser.inl index 3ab2c4376c..b19c518ff9 100644 --- a/Code/Framework/AzCore/AzCore/IO/Path/PathParser.inl +++ b/Code/Framework/AzCore/AzCore/IO/Path/PathParser.inl @@ -183,13 +183,12 @@ namespace AZ::IO::Internal return IsAbsolute(pathView.begin(), pathView.end(), preferredSeparator); } - // Compares path segments using either Posix or Windows path rules based on the path separator in use - // Posix paths perform a case-sensitive comparison, while Windows paths perform a case-insensitive comparison - static int ComparePathSegment(AZStd::string_view left, AZStd::string_view right, char pathSeparator) + // Compares path segments using either Posix or Windows path rules based on the exactCaseCompare option + static int ComparePathSegment(AZStd::string_view left, AZStd::string_view right, bool exactCaseCompare) { const size_t maxCharsToCompare = (AZStd::min)(left.size(), right.size()); - int charCompareResult = pathSeparator == PosixPathSeparator + int charCompareResult = exactCaseCompare ? maxCharsToCompare ? strncmp(left.data(), right.data(), maxCharsToCompare) : 0 : maxCharsToCompare ? azstrnicmp(left.data(), right.data(), maxCharsToCompare) : 0; return charCompareResult == 0 @@ -594,7 +593,10 @@ namespace AZ::IO::parser { return pathParser->InRootName() ? **pathParser : ""; }; - int res = Internal::ComparePathSegment(GetRootName(lhsPathParser), GetRootName(rhsPathParser), lhsPathParser->m_preferred_separator); + + const bool exactCaseCompare = lhsPathParser->m_preferred_separator == PosixPathSeparator + || rhsPathParser->m_preferred_separator == PosixPathSeparator; + int res = Internal::ComparePathSegment(GetRootName(lhsPathParser), GetRootName(rhsPathParser), exactCaseCompare); ConsumeRootName(lhsPathParser); ConsumeRootName(rhsPathParser); return res; @@ -621,9 +623,11 @@ namespace AZ::IO::parser auto& lhsPathParser = *lhsPathParserPtr; auto& rhsPathParser = *rhsPathParserPtr; + const bool exactCaseCompare = lhsPathParser.m_preferred_separator == PosixPathSeparator + || rhsPathParser.m_preferred_separator == PosixPathSeparator; while (lhsPathParser && rhsPathParser) { - if (int res = Internal::ComparePathSegment(*lhsPathParser, *rhsPathParser, lhsPathParser.m_preferred_separator); + if (int res = Internal::ComparePathSegment(*lhsPathParser, *rhsPathParser, exactCaseCompare); res != 0) { return res; @@ -646,6 +650,46 @@ namespace AZ::IO::parser return 0; } + //path.hash + /// Path is using FNV-1a algorithm 64 bit version. + inline size_t HashSegment(AZStd::string_view pathSegment, bool hashExactPath) + { + size_t hash = 14695981039346656037ULL; + constexpr size_t fnvPrime = 1099511628211ULL; + + for (const char first : pathSegment) + { + hash ^= static_cast(hashExactPath ? first : tolower(first)); + hash *= fnvPrime; + } + return hash; + } + constexpr size_t HashPath(PathParser& pathParser) + { + size_t hash_value = 0; + const bool hashExactPath = pathParser.m_preferred_separator == AZ::IO::PosixPathSeparator; + while (pathParser) + { + switch (pathParser.m_parser_state) + { + case PS_InRootName: + case PS_InFilenames: + AZStd::hash_combine(hash_value, HashSegment(*pathParser, hashExactPath)); + break; + case PS_InRootDir: + // Only hash the PosixPathSeparator when a root directory is seen + // This makes the hash consistent for root directories path of C:\ and C:/ + AZStd::hash_combine(hash_value, HashSegment("/", hashExactPath)); + break; + default: + // The BeforeBegin and AtEnd states contain no segments to hash + break; + } + ++pathParser; + } + return hash_value; + } + constexpr int DetermineLexicalElementCount(PathParser pathParser) { int count = 0; diff --git a/Code/Framework/AzCore/Tests/IO/Path/PathTests.cpp b/Code/Framework/AzCore/Tests/IO/Path/PathTests.cpp index dbdb4fee78..cf212aa34c 100644 --- a/Code/Framework/AzCore/Tests/IO/Path/PathTests.cpp +++ b/Code/Framework/AzCore/Tests/IO/Path/PathTests.cpp @@ -213,6 +213,82 @@ namespace UnitTest AZStd::tuple(R"(foO/Bar)", "foo/bar") )); + + struct PathHashCompareParams + { + AZ::IO::PathView m_testPath{}; + ::testing::Matcher m_compareMatcher; + ::testing::Matcher m_hashMatcher; + }; + + class PathHashCompareFixture + : public ScopedAllocatorSetupFixture + , public ::testing::WithParamInterface + {}; + + // Verifies that two paths that compare equal has their hash value compare equal + TEST_P(PathHashCompareFixture, PathsWhichCompareEqual_HashesToSameValue_Succeeds) + { + auto&& [testPath1, compareMatcher, hashMatcher] = GetParam(); + + // Compare path using parameterized Matcher + EXPECT_THAT(testPath1, compareMatcher); + // Compare hash using parameterized Matcher + const size_t testPath1Hash = AZStd::hash{}(testPath1); +AZ_PUSH_DISABLE_WARNING(4296, "-Wunknown-warning-option") + EXPECT_THAT(testPath1Hash, hashMatcher); +AZ_POP_DISABLE_WARNING + } + + INSTANTIATE_TEST_CASE_P( + HashPathCompareValidation, + PathHashCompareFixture, + ::testing::Values( + PathHashCompareParams{ AZ::IO::PathView("C:/test/foo", AZ::IO::WindowsPathSeparator), + testing::Eq(AZ::IO::PathView(R"(c:\test/foo)", AZ::IO::WindowsPathSeparator)), + testing::Eq(AZStd::hash{}(AZ::IO::PathView(R"(c:\test/foo)", AZ::IO::WindowsPathSeparator))) }, + PathHashCompareParams{ AZ::IO::PathView("/test/foo", AZ::IO::WindowsPathSeparator), + testing::Eq(AZ::IO::PathView(R"(/test/FOO)", AZ::IO::WindowsPathSeparator)), + testing::Eq(AZStd::hash{}(AZ::IO::PathView(R"(/test/FOO)", AZ::IO::WindowsPathSeparator))) }, + PathHashCompareParams{ AZ::IO::PathView("C:/test/foo", AZ::IO::WindowsPathSeparator), + testing::Eq(AZ::IO::PathView(R"(c:\test/foo)", AZ::IO::WindowsPathSeparator)), + testing::Eq(AZStd::hash{}(AZ::IO::PathView(R"(c:\test/foo)", AZ::IO::WindowsPathSeparator))) }, + PathHashCompareParams{ AZ::IO::PathView("C:/test/foo", AZ::IO::PosixPathSeparator), + testing::Ne(AZ::IO::PathView(R"(c:\test/foo)", AZ::IO::WindowsPathSeparator)), + testing::Ne(AZStd::hash{}(AZ::IO::PathView(R"(c:\test/foo)", AZ::IO::WindowsPathSeparator))) }, + PathHashCompareParams{ AZ::IO::PathView(R"(C:\test\foo)", AZ::IO::WindowsPathSeparator), + testing::Ne(AZ::IO::PathView(R"(c:/test/foo)", AZ::IO::PosixPathSeparator)), + testing::Ne(AZStd::hash{}(AZ::IO::PathView(R"(c:/test/foo)", AZ::IO::PosixPathSeparator))) }, + PathHashCompareParams{ AZ::IO::PathView("/test/aoo", AZ::IO::WindowsPathSeparator), + testing::Eq(AZ::IO::PathView(R"(/test/AOO)", AZ::IO::WindowsPathSeparator)), + testing::Eq(AZStd::hash{}(AZ::IO::PathView(R"(/test/AOO)", AZ::IO::WindowsPathSeparator))) }, + PathHashCompareParams{ AZ::IO::PathView("/test/aoo", AZ::IO::PosixPathSeparator), + testing::Gt(AZ::IO::PathView(R"(/test/AOO)", AZ::IO::WindowsPathSeparator)), + testing::Eq(AZStd::hash{}(AZ::IO::PathView(R"(/test/AOO)", AZ::IO::WindowsPathSeparator))) }, + PathHashCompareParams{ AZ::IO::PathView("/test/aoo", AZ::IO::WindowsPathSeparator), + testing::Gt(AZ::IO::PathView(R"(/test/AOO)", AZ::IO::PosixPathSeparator)), + testing::Ne(AZStd::hash{}(AZ::IO::PathView(R"(/test/AOO)", AZ::IO::PosixPathSeparator))) }, + PathHashCompareParams{ AZ::IO::PathView("/test/AOO", AZ::IO::PosixPathSeparator), + testing::Lt(AZ::IO::PathView(R"(/test/aoo)", AZ::IO::WindowsPathSeparator)), + testing::Ne(AZStd::hash{}(AZ::IO::PathView(R"(/test/aoo)", AZ::IO::WindowsPathSeparator))) }, + PathHashCompareParams{ AZ::IO::PathView("/test/AOO", AZ::IO::WindowsPathSeparator), + testing::Lt(AZ::IO::PathView(R"(/test/aoo)", AZ::IO::PosixPathSeparator)), + testing::Eq(AZStd::hash{}(AZ::IO::PathView(R"(/test/aoo)", AZ::IO::PosixPathSeparator))) }, + // Paths with different character values, comparison based on path separator + PathHashCompareParams{ AZ::IO::PathView("/test/BOO", AZ::IO::PosixPathSeparator), + testing::Le(AZ::IO::PathView(R"(/test/aoo)", AZ::IO::WindowsPathSeparator)), + testing::Ne(AZStd::hash{}(AZ::IO::PathView(R"(/test/aoo)", AZ::IO::WindowsPathSeparator))) }, + PathHashCompareParams{ AZ::IO::PathView("/test/BOO", AZ::IO::WindowsPathSeparator), + testing::Ge(AZ::IO::PathView(R"(/test/aoo)", AZ::IO::WindowsPathSeparator)), + testing::Ne(AZStd::hash{}(AZ::IO::PathView(R"(/test/aoo)", AZ::IO::WindowsPathSeparator))) }, + PathHashCompareParams{ AZ::IO::PathView("/test/aoo", AZ::IO::WindowsPathSeparator), + testing::Le(AZ::IO::PathView(R"(/test/Boo)", AZ::IO::WindowsPathSeparator)), + testing::Ne(AZStd::hash{}(AZ::IO::PathView(R"(/test/Boo)", AZ::IO::WindowsPathSeparator))) }, + PathHashCompareParams{ AZ::IO::PathView("/test/aoo", AZ::IO::PosixPathSeparator), + testing::Ge(AZ::IO::PathView(R"(/test/Boo)", AZ::IO::WindowsPathSeparator)), + testing::Ne(AZStd::hash{}(AZ::IO::PathView(R"(/test/Boo)", AZ::IO::WindowsPathSeparator))) } + )); + class PathSingleParamFixture : public ScopedAllocatorSetupFixture , public ::testing::WithParamInterface>