Merge pull request #7328 from aws-lumberyard-dev/Atom/santorac/ShaderOptionValueUtils

Minor cleanup of some shader option related unit tests.

Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Shader/ShaderOptionGroupLayout.h

@@ -14,6 +14,7 @@
 #include <Atom/RHI.Reflect/NameIdReflectionMap.h>
 
 #include <AzCore/std/smart_ptr/intrusive_base.h>
+#include <AzCore/std/containers/span.h>
 #include <AzCore/Memory/PoolAllocator.h>
 #include <AzCore/Serialization/SerializeContext.h>
 #include <AzCore/Utils/TypeHash.h>
@@ -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<const AZStd::string_view> enumNames);
+        ShaderOptionValues CreateEnumShaderOptionValues(AZStd::initializer_list<AZStd::string_view> 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<RPI::ShaderOptionValuePair>& nameIndexList,
+                                   const ShaderOptionValues& nameIndexList,
-                                   const Name& defaultValue);
+                                   const Name& defaultValue = {});
 
             AZ_DEFAULT_COPY_MOVE(ShaderOptionDescriptor);
 

Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Shader/ShaderOptionTypes.h

@@ -18,6 +18,7 @@ namespace AZ
         using ShaderOptionIndex = RHI::Handle<uint32_t, class ShaderOptionIndexNamespace>;  //!< ShaderOption index in the group layout
         using ShaderOptionValue = RHI::Handle<uint32_t, class ShaderOptionValueNamespace>;  //!< Numerical representation for a single value in the ShaderOption
         using ShaderOptionValuePair = AZStd::pair<Name/*valueName*/, ShaderOptionValue>;  //!< Provides a string representation for a ShaderOptionValue
+        using ShaderOptionValues = AZStd::vector<ShaderOptionValuePair>; //!< List of possible values for a shader option
 
         enum class ShaderOptionType : uint32_t
         {

Gems/Atom/RPI/Code/Source/RPI.Reflect/Shader/ShaderOptionGroupLayout.cpp

@@ -35,6 +35,35 @@ namespace AZ
             default: return "<Unknown>";
             }
         }
+        
+        ShaderOptionValues CreateEnumShaderOptionValues(AZStd::span<const AZStd::string_view> 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<AZStd::string_view> 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<RPI::ShaderOptionValuePair> 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<RPI::ShaderOptionValuePair>& 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();

Gems/Atom/RPI/Code/Tests/Material/LuaMaterialFunctorTests.cpp

@@ -70,18 +70,9 @@ namespace UnitTest
 
         AZ::RPI::Ptr<AZ::RPI::ShaderOptionGroupLayout> CreateCommonTestShaderOptionsLayout()
         {
-            AZStd::vector<RPI::ShaderOptionValuePair> boolOptionValues;
+            AZStd::vector<RPI::ShaderOptionValuePair> boolOptionValues = CreateBoolShaderOptionValues();
-            boolOptionValues.push_back({Name("False"),  RPI::ShaderOptionValue(0)});
+            AZStd::vector<RPI::ShaderOptionValuePair> intRangeOptionValues = CreateIntRangeShaderOptionValues(0, 15);
-            boolOptionValues.push_back({Name("True"), RPI::ShaderOptionValue(1)});
+            AZStd::vector<RPI::ShaderOptionValuePair> qualityOptionValues = CreateEnumShaderOptionValues({"Quality::Low", "Quality::Medium", "Quality::High"});
-
-            AZStd::vector<RPI::ShaderOptionValuePair> intRangeOptionValues;
-            intRangeOptionValues.push_back({Name("0"),  RPI::ShaderOptionValue(0)});
-            intRangeOptionValues.push_back({Name("15"), RPI::ShaderOptionValue(15)});
-
-            AZStd::vector<RPI::ShaderOptionValuePair> 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)});
 
             AZ::RPI::Ptr<AZ::RPI::ShaderOptionGroupLayout> shaderOptions = RPI::ShaderOptionGroupLayout::Create();
             shaderOptions->AddShaderOption(ShaderOptionDescriptor{Name{"o_bool"}, ShaderOptionType::Boolean, 0, 0, boolOptionValues, Name{"False"}});

Gems/Atom/RPI/Code/Tests/Material/MaterialFunctorTests.cpp

@@ -127,9 +127,7 @@ namespace UnitTest
     {
         using namespace AZ::RPI;
 
-        AZStd::vector<RPI::ShaderOptionValuePair> boolOptionValues;
+        AZStd::vector<RPI::ShaderOptionValuePair> boolOptionValues = CreateBoolShaderOptionValues();
-        boolOptionValues.push_back({Name("False"),  RPI::ShaderOptionValue(0)});
-        boolOptionValues.push_back({Name("True"), RPI::ShaderOptionValue(1)});
 
         AZ::RPI::Ptr<AZ::RPI::ShaderOptionGroupLayout> 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> materialTypeAsset;
-        //Data::Asset<MaterialAsset> 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.

Gems/Atom/RPI/Code/Tests/Material/MaterialTests.cpp

@@ -389,18 +389,9 @@ namespace UnitTest
 
     Ptr<ShaderOptionGroupLayout> CreateTestOptionsLayout()
     {
-        AZStd::vector<RPI::ShaderOptionValuePair> enumOptionValues;
+        AZStd::vector<RPI::ShaderOptionValuePair> enumOptionValues = CreateEnumShaderOptionValues({"Low", "Med", "High"});
-        enumOptionValues.push_back({Name("Low"),  RPI::ShaderOptionValue(0)});
+        AZStd::vector<RPI::ShaderOptionValuePair> boolOptionValues = CreateBoolShaderOptionValues();
-        enumOptionValues.push_back({Name("Med"), RPI::ShaderOptionValue(1)});
+        AZStd::vector<RPI::ShaderOptionValuePair> rangeOptionValues = CreateIntRangeShaderOptionValues(1, 10);
-        enumOptionValues.push_back({Name("High"), RPI::ShaderOptionValue(2)});
-
-        AZStd::vector<RPI::ShaderOptionValuePair> boolOptionValues;
-        boolOptionValues.push_back({Name("False"),  RPI::ShaderOptionValue(0)});
-        boolOptionValues.push_back({Name("True"), RPI::ShaderOptionValue(1)});
-
-        AZStd::vector<RPI::ShaderOptionValuePair> rangeOptionValues;
-        rangeOptionValues.push_back({Name("1"),  RPI::ShaderOptionValue(1)});
-        rangeOptionValues.push_back({Name("10"), RPI::ShaderOptionValue(10)});
 
         Ptr<ShaderOptionGroupLayout> shaderOptions = ShaderOptionGroupLayout::Create();
         uint32_t order = 0;

Gems/Atom/RPI/Code/Tests/Material/MaterialTypeAssetTests.cpp

@@ -103,18 +103,9 @@ namespace UnitTest
 
             m_testMaterialSrgLayout = CreateCommonTestMaterialSrgLayout();
 
-            AZStd::vector<RPI::ShaderOptionValuePair> boolOptionValues;
+            AZStd::vector<RPI::ShaderOptionValuePair> boolOptionValues = CreateBoolShaderOptionValues();
-            boolOptionValues.push_back({Name{"False"},  RPI::ShaderOptionValue{0}});
+            AZStd::vector<RPI::ShaderOptionValuePair> enumOptionValues = CreateEnumShaderOptionValues({"Low", "Med", "High"});
-            boolOptionValues.push_back({Name{"True"}, RPI::ShaderOptionValue{1}});
+            AZStd::vector<RPI::ShaderOptionValuePair> intOptionRange = CreateIntRangeShaderOptionValues(0, 8);
-
-            AZStd::vector<RPI::ShaderOptionValuePair> 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<RPI::ShaderOptionValuePair> intOptionRange;
-            intOptionRange.push_back({Name{"0"},  RPI::ShaderOptionValue{0}});
-            intOptionRange.push_back({Name{"8"}, RPI::ShaderOptionValue{8}});
 
             m_testShaderOptionsLayout = ShaderOptionGroupLayout::Create();
             uint32_t order = 0;
@@ -977,9 +968,7 @@ namespace UnitTest
     {
         // Create shaders...
 
-        AZStd::vector<RPI::ShaderOptionValuePair> boolOptionValues;
+        AZStd::vector<RPI::ShaderOptionValuePair> boolOptionValues = CreateBoolShaderOptionValues();
-        boolOptionValues.push_back({Name("False"),  RPI::ShaderOptionValue(0)});
-        boolOptionValues.push_back({Name("True"), RPI::ShaderOptionValue(1)});
 
         Ptr<ShaderOptionGroupLayout> optionsForShaderA = ShaderOptionGroupLayout::Create();
         optionsForShaderA->AddShaderOption(ShaderOptionDescriptor{Name{"o_globalOption_inBothShaders"}, ShaderOptionType::Boolean, 0, 0, boolOptionValues, Name{"False"}});

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<RPI::ShaderOptionValuePair> optionValues;
+            AZStd::vector<RPI::ShaderOptionValuePair> optionValues = CreateEnumShaderOptionValues({"Low", "Med", "High"});
-            optionValues.push_back({Name("Low"),  RPI::ShaderOptionValue(0)});
-            optionValues.push_back({Name("Med"), RPI::ShaderOptionValue(1)});
-            optionValues.push_back({Name("High"), RPI::ShaderOptionValue(2)});
 
             Ptr<ShaderOptionGroupLayout> shaderOptions = ShaderOptionGroupLayout::Create();
             uint32_t order = 0;
@@ -703,10 +700,7 @@ namespace UnitTest
     {
         // Set up the shaders...
 
-        AZStd::vector<RPI::ShaderOptionValuePair> optionValues;
+        AZStd::vector<RPI::ShaderOptionValuePair> optionValues = CreateEnumShaderOptionValues({"Low", "Med", "High"});
-        optionValues.push_back({Name("Low"),  RPI::ShaderOptionValue(0)});
-        optionValues.push_back({Name("Med"), RPI::ShaderOptionValue(1)});
-        optionValues.push_back({Name("High"), RPI::ShaderOptionValue(2)});
 
         Ptr<ShaderOptionGroupLayout> shaderOptions = ShaderOptionGroupLayout::Create();
         uint32_t order = 0;
@@ -1072,10 +1066,7 @@ namespace UnitTest
     {
         // Setup the shader...
 
-        AZStd::vector<RPI::ShaderOptionValuePair> optionValues;
+        AZStd::vector<RPI::ShaderOptionValuePair> optionValues = CreateEnumShaderOptionValues({"Low", "Med", "High"});
-        optionValues.push_back({ Name("Low"),  RPI::ShaderOptionValue(0) });
-        optionValues.push_back({ Name("Med"), RPI::ShaderOptionValue(1) });
-        optionValues.push_back({ Name("High"), RPI::ShaderOptionValue(2) });
 
         uint32_t order = 0;
 
@@ -1402,9 +1393,7 @@ namespace UnitTest
         layeredMaterialSrgLayout->AddShaderInput(RHI::ShaderInputConstantDescriptor{ Name{ "m_blendFactor" }, 4, 4, 0 });
         layeredMaterialSrgLayout->Finalize();
         
-        AZStd::vector<RPI::ShaderOptionValuePair> boolOptionValues;
+        AZStd::vector<RPI::ShaderOptionValuePair> boolOptionValues = CreateBoolShaderOptionValues();
-        boolOptionValues.push_back({Name("False"),  RPI::ShaderOptionValue(0)});
-        boolOptionValues.push_back({Name("True"), RPI::ShaderOptionValue(1)});
         Ptr<ShaderOptionGroupLayout> shaderOptionsLayout = ShaderOptionGroupLayout::Create();
         uint32_t order = 0;
         shaderOptionsLayout->AddShaderOption(ShaderOptionDescriptor{Name{"o_layer2_clearCoat_enable"}, ShaderOptionType::Boolean, 0, order++, boolOptionValues, Name{"False"}});

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<RPI::ShaderOptionValuePair> 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<RPI::ShaderOptionGroupLayout> shaderOptionGroupLayout = RPI::ShaderOptionGroupLayout::Create();
+
+        AZStd::vector<RPI::ShaderOptionValuePair> 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)
     {