From 3441c026377bc2d5d9a7b5c0bcde21a458a15983 Mon Sep 17 00:00:00 2001 From: santorac <55155825+santorac@users.noreply.github.com> Date: Tue, 1 Feb 2022 11:42:50 -0800 Subject: [PATCH] Minor cleanup of some shader option related unit tests. Added new utility functions for easily creating the value set for a ShaderOptionDescriptor. Made ShaderOptionDescriptor default value optional, picking the first available value as the default ... by default. Signed-off-by: santorac <55155825+santorac@users.noreply.github.com> --- .../Shader/ShaderOptionGroupLayout.h | 14 ++++++-- .../RPI.Reflect/Shader/ShaderOptionTypes.h | 1 + .../Shader/ShaderOptionGroupLayout.cpp | 36 ++++++++++++++++++- .../Material/LuaMaterialFunctorTests.cpp | 15 ++------ .../Tests/Material/MaterialFunctorTests.cpp | 5 +-- .../RPI/Code/Tests/Material/MaterialTests.cpp | 15 ++------ .../Tests/Material/MaterialTypeAssetTests.cpp | 19 +++------- .../Material/MaterialTypeSourceDataTests.cpp | 19 +++------- .../RPI/Code/Tests/Shader/ShaderTests.cpp | 22 ++++++------ 9 files changed, 74 insertions(+), 72 deletions(-) diff --git a/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Shader/ShaderOptionGroupLayout.h b/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Shader/ShaderOptionGroupLayout.h index 396b6d50f0..27c8fff001 100644 --- a/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Shader/ShaderOptionGroupLayout.h +++ b/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Shader/ShaderOptionGroupLayout.h @@ -14,6 +14,7 @@ #include #include +#include #include #include #include @@ -44,6 +45,12 @@ namespace AZ bool m_bakeEmptyAsDefault = false; }; + //! Creates a list of shader option values that can be used to construct a ShaderOptionDescriptor. + ShaderOptionValues CreateEnumShaderOptionValues(AZStd::span enumNames); + ShaderOptionValues CreateEnumShaderOptionValues(AZStd::initializer_list enumNames); + ShaderOptionValues CreateBoolShaderOptionValues(); + ShaderOptionValues CreateIntRangeShaderOptionValues(uint32_t min, uint32_t max); + //! Describes a shader option to the ShaderOptionGroupLayout class. Maps a shader option //! to a set of bits in a mask in order to facilitate packing values into a mask to //! form a ShaderKey. @@ -62,15 +69,16 @@ namespace AZ //! @param optionType Type hint for the option - bool, enum, integer range, etc. //! @param bitOffset Bit offset must match the ShaderOptionGroupLayout where this Option will be added //! @param order The order (rank) of the shader option. Must be unique within a group. Lower order is higher priority. - //! @param nameIndexList List of valid (valueName, value) pairs for this Option + //! @param nameIndexList List of valid (valueName, value) pairs for this Option. See "Create*ShaderOptionValues" utility functions above. //! @param defaultValue Default value name, which must also be in the nameIndexList. In the cases where the list //! defines a range (IntegerRange for instance) defaultValue must be within the range instead. + //! If omitted, the first entry in @nameIndexList will be used. ShaderOptionDescriptor(const Name& name, const ShaderOptionType& optionType, uint32_t bitOffset, uint32_t order, - const AZStd::vector& nameIndexList, - const Name& defaultValue); + const ShaderOptionValues& nameIndexList, + const Name& defaultValue = {}); AZ_DEFAULT_COPY_MOVE(ShaderOptionDescriptor); diff --git a/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Shader/ShaderOptionTypes.h b/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Shader/ShaderOptionTypes.h index e8be930a22..427f70890c 100644 --- a/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Shader/ShaderOptionTypes.h +++ b/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Shader/ShaderOptionTypes.h @@ -18,6 +18,7 @@ namespace AZ using ShaderOptionIndex = RHI::Handle; //!< ShaderOption index in the group layout using ShaderOptionValue = RHI::Handle; //!< Numerical representation for a single value in the ShaderOption using ShaderOptionValuePair = AZStd::pair; //!< Provides a string representation for a ShaderOptionValue + using ShaderOptionValues = AZStd::vector; //!< List of possible values for a shader option enum class ShaderOptionType : uint32_t { diff --git a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Shader/ShaderOptionGroupLayout.cpp b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Shader/ShaderOptionGroupLayout.cpp index 7e9f62e560..17f5faec98 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Shader/ShaderOptionGroupLayout.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Shader/ShaderOptionGroupLayout.cpp @@ -35,6 +35,35 @@ namespace AZ default: return ""; } } + + ShaderOptionValues CreateEnumShaderOptionValues(AZStd::span enumNames) + { + ShaderOptionValues values; + values.reserve(enumNames.size()); + for (size_t i = 0; i < enumNames.size(); ++i) + { + values.emplace_back(Name{enumNames[i]}, i); + } + return values; + } + + ShaderOptionValues CreateEnumShaderOptionValues(AZStd::initializer_list enumNames) + { + return CreateEnumShaderOptionValues(AZStd::span(enumNames.begin(), enumNames.end())); + } + + ShaderOptionValues CreateBoolShaderOptionValues() + { + return CreateEnumShaderOptionValues({"False", "True"}); + } + + ShaderOptionValues CreateIntRangeShaderOptionValues(uint32_t min, uint32_t max) + { + AZStd::vector intOptionRange; + intOptionRange.push_back({Name{AZStd::string::format("%u", min)}, RPI::ShaderOptionValue{min}}); + intOptionRange.push_back({Name{AZStd::string::format("%u", max)}, RPI::ShaderOptionValue{max}}); + return intOptionRange; + } void ShaderOptionGroupHints::Reflect(ReflectContext* context) { @@ -90,7 +119,7 @@ namespace AZ const ShaderOptionType& optionType, uint32_t bitOffset, uint32_t order, - const AZStd::vector& nameIndexList, + const ShaderOptionValues& nameIndexList, const Name& defaultValue) : m_name{name} @@ -102,6 +131,11 @@ namespace AZ for (auto pair : nameIndexList) { // Registers the pair in the lookup table AddValue(pair.first, pair.second); + + if (m_defaultValue.IsEmpty()) + { + m_defaultValue = pair.first; + } } uint32_t numValues = (m_type == ShaderOptionType::IntegerRange) ? (m_maxValue.GetIndex() - m_minValue.GetIndex() + 1) : (uint32_t) nameIndexList.size(); diff --git a/Gems/Atom/RPI/Code/Tests/Material/LuaMaterialFunctorTests.cpp b/Gems/Atom/RPI/Code/Tests/Material/LuaMaterialFunctorTests.cpp index 5016f8303e..15f66916a6 100644 --- a/Gems/Atom/RPI/Code/Tests/Material/LuaMaterialFunctorTests.cpp +++ b/Gems/Atom/RPI/Code/Tests/Material/LuaMaterialFunctorTests.cpp @@ -70,18 +70,9 @@ namespace UnitTest AZ::RPI::Ptr CreateCommonTestShaderOptionsLayout() { - AZStd::vector boolOptionValues; - boolOptionValues.push_back({Name("False"), RPI::ShaderOptionValue(0)}); - boolOptionValues.push_back({Name("True"), RPI::ShaderOptionValue(1)}); - - AZStd::vector intRangeOptionValues; - intRangeOptionValues.push_back({Name("0"), RPI::ShaderOptionValue(0)}); - intRangeOptionValues.push_back({Name("15"), RPI::ShaderOptionValue(15)}); - - AZStd::vector qualityOptionValues; - qualityOptionValues.push_back({Name("Quality::Low"), RPI::ShaderOptionValue(0)}); - qualityOptionValues.push_back({Name("Quality::Medium"), RPI::ShaderOptionValue(1)}); - qualityOptionValues.push_back({Name("Quality::High"), RPI::ShaderOptionValue(2)}); + AZStd::vector boolOptionValues = CreateBoolShaderOptionValues(); + AZStd::vector intRangeOptionValues = CreateIntRangeShaderOptionValues(0, 15); + AZStd::vector qualityOptionValues = CreateEnumShaderOptionValues({"Quality::Low", "Quality::Medium", "Quality::High"}); AZ::RPI::Ptr shaderOptions = RPI::ShaderOptionGroupLayout::Create(); shaderOptions->AddShaderOption(ShaderOptionDescriptor{Name{"o_bool"}, ShaderOptionType::Boolean, 0, 0, boolOptionValues, Name{"False"}}); diff --git a/Gems/Atom/RPI/Code/Tests/Material/MaterialFunctorTests.cpp b/Gems/Atom/RPI/Code/Tests/Material/MaterialFunctorTests.cpp index 3373646c6e..08049f3393 100644 --- a/Gems/Atom/RPI/Code/Tests/Material/MaterialFunctorTests.cpp +++ b/Gems/Atom/RPI/Code/Tests/Material/MaterialFunctorTests.cpp @@ -127,9 +127,7 @@ namespace UnitTest { using namespace AZ::RPI; - AZStd::vector boolOptionValues; - boolOptionValues.push_back({Name("False"), RPI::ShaderOptionValue(0)}); - boolOptionValues.push_back({Name("True"), RPI::ShaderOptionValue(1)}); + AZStd::vector boolOptionValues = CreateBoolShaderOptionValues(); AZ::RPI::Ptr shaderOptions = RPI::ShaderOptionGroupLayout::Create(); shaderOptions->AddShaderOption(ShaderOptionDescriptor{Name{"o_optionA"}, ShaderOptionType::Boolean, 0, 0, boolOptionValues, Name{"False"}}); @@ -138,7 +136,6 @@ namespace UnitTest shaderOptions->Finalize(); Data::Asset materialTypeAsset; - //Data::Asset materialAsset; // Note we don't actually need any properties or functors in the material type. We just need to set up some sample data // structures that we can pass to the functors below, especially the shader with shader options. diff --git a/Gems/Atom/RPI/Code/Tests/Material/MaterialTests.cpp b/Gems/Atom/RPI/Code/Tests/Material/MaterialTests.cpp index 569f8f6df0..02b78d5fd2 100644 --- a/Gems/Atom/RPI/Code/Tests/Material/MaterialTests.cpp +++ b/Gems/Atom/RPI/Code/Tests/Material/MaterialTests.cpp @@ -389,18 +389,9 @@ namespace UnitTest Ptr CreateTestOptionsLayout() { - AZStd::vector enumOptionValues; - enumOptionValues.push_back({Name("Low"), RPI::ShaderOptionValue(0)}); - enumOptionValues.push_back({Name("Med"), RPI::ShaderOptionValue(1)}); - enumOptionValues.push_back({Name("High"), RPI::ShaderOptionValue(2)}); - - AZStd::vector boolOptionValues; - boolOptionValues.push_back({Name("False"), RPI::ShaderOptionValue(0)}); - boolOptionValues.push_back({Name("True"), RPI::ShaderOptionValue(1)}); - - AZStd::vector rangeOptionValues; - rangeOptionValues.push_back({Name("1"), RPI::ShaderOptionValue(1)}); - rangeOptionValues.push_back({Name("10"), RPI::ShaderOptionValue(10)}); + AZStd::vector enumOptionValues = CreateEnumShaderOptionValues({"Low", "Med", "High"}); + AZStd::vector boolOptionValues = CreateBoolShaderOptionValues(); + AZStd::vector rangeOptionValues = CreateIntRangeShaderOptionValues(1, 10); Ptr shaderOptions = ShaderOptionGroupLayout::Create(); uint32_t order = 0; diff --git a/Gems/Atom/RPI/Code/Tests/Material/MaterialTypeAssetTests.cpp b/Gems/Atom/RPI/Code/Tests/Material/MaterialTypeAssetTests.cpp index 382ab4e149..0fbbcf76c3 100644 --- a/Gems/Atom/RPI/Code/Tests/Material/MaterialTypeAssetTests.cpp +++ b/Gems/Atom/RPI/Code/Tests/Material/MaterialTypeAssetTests.cpp @@ -103,18 +103,9 @@ namespace UnitTest m_testMaterialSrgLayout = CreateCommonTestMaterialSrgLayout(); - AZStd::vector boolOptionValues; - boolOptionValues.push_back({Name{"False"}, RPI::ShaderOptionValue{0}}); - boolOptionValues.push_back({Name{"True"}, RPI::ShaderOptionValue{1}}); - - AZStd::vector enumOptionValues; - enumOptionValues.push_back({Name{"Low"}, RPI::ShaderOptionValue{0}}); - enumOptionValues.push_back({Name{"Med"}, RPI::ShaderOptionValue{1}}); - enumOptionValues.push_back({Name{"High"}, RPI::ShaderOptionValue{2}}); - - AZStd::vector intOptionRange; - intOptionRange.push_back({Name{"0"}, RPI::ShaderOptionValue{0}}); - intOptionRange.push_back({Name{"8"}, RPI::ShaderOptionValue{8}}); + AZStd::vector boolOptionValues = CreateBoolShaderOptionValues(); + AZStd::vector enumOptionValues = CreateEnumShaderOptionValues({"Low", "Med", "High"}); + AZStd::vector intOptionRange = CreateIntRangeShaderOptionValues(0, 8); m_testShaderOptionsLayout = ShaderOptionGroupLayout::Create(); uint32_t order = 0; @@ -977,9 +968,7 @@ namespace UnitTest { // Create shaders... - AZStd::vector boolOptionValues; - boolOptionValues.push_back({Name("False"), RPI::ShaderOptionValue(0)}); - boolOptionValues.push_back({Name("True"), RPI::ShaderOptionValue(1)}); + AZStd::vector boolOptionValues = CreateBoolShaderOptionValues(); Ptr optionsForShaderA = ShaderOptionGroupLayout::Create(); optionsForShaderA->AddShaderOption(ShaderOptionDescriptor{Name{"o_globalOption_inBothShaders"}, ShaderOptionType::Boolean, 0, 0, boolOptionValues, Name{"False"}}); diff --git a/Gems/Atom/RPI/Code/Tests/Material/MaterialTypeSourceDataTests.cpp b/Gems/Atom/RPI/Code/Tests/Material/MaterialTypeSourceDataTests.cpp index 206073e96a..948a56f557 100644 --- a/Gems/Atom/RPI/Code/Tests/Material/MaterialTypeSourceDataTests.cpp +++ b/Gems/Atom/RPI/Code/Tests/Material/MaterialTypeSourceDataTests.cpp @@ -289,10 +289,7 @@ namespace UnitTest m_testMaterialSrgLayout->AddShaderInput(RHI::ShaderInputImageDescriptor{ Name{ "m_image" }, RHI::ShaderInputImageAccess::Read, RHI::ShaderInputImageType::Image2D, 1, 1 }); EXPECT_TRUE(m_testMaterialSrgLayout->Finalize()); - AZStd::vector optionValues; - optionValues.push_back({Name("Low"), RPI::ShaderOptionValue(0)}); - optionValues.push_back({Name("Med"), RPI::ShaderOptionValue(1)}); - optionValues.push_back({Name("High"), RPI::ShaderOptionValue(2)}); + AZStd::vector optionValues = CreateEnumShaderOptionValues({"Low", "Med", "High"}); Ptr shaderOptions = ShaderOptionGroupLayout::Create(); uint32_t order = 0; @@ -703,10 +700,7 @@ namespace UnitTest { // Set up the shaders... - AZStd::vector optionValues; - optionValues.push_back({Name("Low"), RPI::ShaderOptionValue(0)}); - optionValues.push_back({Name("Med"), RPI::ShaderOptionValue(1)}); - optionValues.push_back({Name("High"), RPI::ShaderOptionValue(2)}); + AZStd::vector optionValues = CreateEnumShaderOptionValues({"Low", "Med", "High"}); Ptr shaderOptions = ShaderOptionGroupLayout::Create(); uint32_t order = 0; @@ -1072,10 +1066,7 @@ namespace UnitTest { // Setup the shader... - AZStd::vector optionValues; - optionValues.push_back({ Name("Low"), RPI::ShaderOptionValue(0) }); - optionValues.push_back({ Name("Med"), RPI::ShaderOptionValue(1) }); - optionValues.push_back({ Name("High"), RPI::ShaderOptionValue(2) }); + AZStd::vector optionValues = CreateEnumShaderOptionValues({"Low", "Med", "High"}); uint32_t order = 0; @@ -1402,9 +1393,7 @@ namespace UnitTest layeredMaterialSrgLayout->AddShaderInput(RHI::ShaderInputConstantDescriptor{ Name{ "m_blendFactor" }, 4, 4, 0 }); layeredMaterialSrgLayout->Finalize(); - AZStd::vector boolOptionValues; - boolOptionValues.push_back({Name("False"), RPI::ShaderOptionValue(0)}); - boolOptionValues.push_back({Name("True"), RPI::ShaderOptionValue(1)}); + AZStd::vector boolOptionValues = CreateBoolShaderOptionValues(); Ptr shaderOptionsLayout = ShaderOptionGroupLayout::Create(); uint32_t order = 0; shaderOptionsLayout->AddShaderOption(ShaderOptionDescriptor{Name{"o_layer2_clearCoat_enable"}, ShaderOptionType::Boolean, 0, order++, boolOptionValues, Name{"False"}}); diff --git a/Gems/Atom/RPI/Code/Tests/Shader/ShaderTests.cpp b/Gems/Atom/RPI/Code/Tests/Shader/ShaderTests.cpp index b30fd0ec94..4fc5f74f3b 100644 --- a/Gems/Atom/RPI/Code/Tests/Shader/ShaderTests.cpp +++ b/Gems/Atom/RPI/Code/Tests/Shader/ShaderTests.cpp @@ -793,16 +793,6 @@ namespace UnitTest EXPECT_FALSE(success); errorMessageFinder.CheckExpectedErrorsFound(); - // Add shader option with an empty default value. - errorMessageFinder.Reset(); - errorMessageFinder.AddExpectedErrorMessage("invalid default value"); - AZStd::vector list5; - list5.push_back({ Name("0"), RPI::ShaderOptionValue(0) }); // 1+ bit - list5.push_back({ Name("1"), RPI::ShaderOptionValue(1) }); // ... - success = shaderOptionGroupLayout->AddShaderOption(AZ::RPI::ShaderOptionDescriptor{ Name{"Invalid"}, intRangeType, 16, order++, list5, Name() }); - EXPECT_FALSE(success); - errorMessageFinder.CheckExpectedErrorsFound(); - // Add shader option with an invalid default int value. errorMessageFinder.Reset(); errorMessageFinder.AddExpectedErrorMessage("invalid default value"); @@ -886,6 +876,18 @@ namespace UnitTest EXPECT_FALSE(shaderOptionGroupLayout->FindShaderOptionIndex(Name{ "Invalid" }).IsValid()); } + + TEST_F(ShaderTests, ImplicitDefaultValue) + { + // Add shader option with no default value. + + RPI::Ptr shaderOptionGroupLayout = RPI::ShaderOptionGroupLayout::Create(); + + AZStd::vector values = AZ::RPI::CreateEnumShaderOptionValues({"A", "B", "C"}); + bool success = shaderOptionGroupLayout->AddShaderOption(AZ::RPI::ShaderOptionDescriptor{ Name{"NoDefaultSpecified"}, RPI::ShaderOptionType::Enumeration, 0, 0, values }); + EXPECT_TRUE(success); + EXPECT_STREQ("A", shaderOptionGroupLayout->GetShaderOptions().back().GetDefaultValue().GetCStr()); + } TEST_F(ShaderTests, ShaderOptionGroupTest) {