Merge pull request #4983 from aws-lumberyard-dev/Atom/santorac/MoveDoubleSided

Moved DoubleSided Flag Out Of Opacity Group

The double-sided flag was hidden inside the opacity property group, and only exposed when you set the opacity to non-opaque. Here we move doubleSided from opacity to general making it easier to find and set.

Also...
- Fixed a bug with the material version auto-update, where it could rename properties but could not move them to another group.
- Added another "default" texture map to the Common/Feature gem that is directional, so better for understanding UV/tangent space. These were copied from the AtomLyIntegration gem.

Gems/Atom/Feature/Common/Assets/Materials/Types/EnhancedPBR.materialtype

@@ -1,6 +1,14 @@
 {
     "description": "Material Type with properties used to define Enhanced PBR, a metallic-roughness Physically-Based Rendering (PBR) material shading model, with advanced features like subsurface scattering, transmission, and anisotropy.",
-    "version": 3,
+    "version": 4,
+    "versionUpdates": [
+        {
+            "toVersion": 4,
+            "actions": [
+                {"op": "rename", "from": "opacity.doubleSided", "to": "general.doubleSided"}
+            ]
+        }
+    ],
     "propertyLayout": {
         "groups": [
             {
@@ -92,6 +100,12 @@
         ],
         "properties": {
             "general": [
+                {
+                    "name": "doubleSided",
+                    "displayName": "Double-sided",
+                    "description": "Whether to render back-faces or just front-faces.",
+                    "type": "Bool"
+                },
                 {
                     "name": "applySpecularAA",
                     "displayName": "Apply Specular AA",
@@ -709,12 +723,6 @@
                         "name": "m_opacityFactor"
                     }
                 },
-                {
-                    "name": "doubleSided",
-                    "displayName": "Double-sided",
-                    "description": "Whether to render back-faces or just front-faces.",
-                    "type": "Bool"
-                },
                 {
                     "name": "alphaAffectsSpecular",
                     "displayName": "Alpha affects specular",

Gems/Atom/Feature/Common/Assets/Materials/Types/StandardPBR.materialtype

@@ -1,6 +1,14 @@
 {
     "description": "Material Type with properties used to define Standard PBR, a metallic-roughness Physically-Based Rendering (PBR) material shading model.",
-    "version": 3,
+    "version": 4,
+    "versionUpdates": [
+        {
+            "toVersion": 4,
+            "actions": [
+                {"op": "rename", "from": "opacity.doubleSided", "to": "general.doubleSided"}
+            ]
+        }
+    ],
     "propertyLayout": {
         "groups": [
             {
@@ -72,6 +80,12 @@
         ],
         "properties": {
             "general": [
+                {
+                    "name": "doubleSided",
+                    "displayName": "Double-sided",
+                    "description": "Whether to render back-faces or just front-faces.",
+                    "type": "Bool"
+                },
                 {
                     "name": "applySpecularAA",
                     "displayName": "Apply Specular AA",
@@ -650,12 +664,6 @@
                         "name": "m_opacityFactor"
                     }
                 },
-                {
-                    "name": "doubleSided",
-                    "displayName": "Double-sided",
-                    "description": "Whether to render back-faces or just front-faces.",
-                    "type": "Bool"
-                },
                 {
                     "name": "alphaAffectsSpecular",
                     "displayName": "Alpha affects specular",

Gems/Atom/Feature/Common/Assets/Materials/Types/StandardPBR_HandleOpacityDoubleSided.lua

@@ -10,14 +10,14 @@
 ----------------------------------------------------------------------------------------------------
 
 function GetMaterialPropertyDependencies()
-    return {"opacity.doubleSided"}
+    return {"general.doubleSided"}
 end
  
 ForwardPassIndex = 0
 ForwardPassEdsIndex = 1
 
 function Process(context)
-    local doubleSided = context:GetMaterialPropertyValue_bool("opacity.doubleSided")
+    local doubleSided = context:GetMaterialPropertyValue_bool("general.doubleSided")
     local lastShader = context:GetShaderCount() - 1;
 
     if(doubleSided) then

Gems/Atom/Feature/Common/Assets/Textures/Default/checker_basecolor.tif

@@ -0,0 +1,3 @@
+version https://git-lfs.github.com/spec/v1
+oid sha256:57d6744696768f9fb8a5fe5fee9aa36fee1eb87a9dbc1e60d4a35ed3c39d68e6
+size 810620

Gems/Atom/Feature/Common/Assets/Textures/Default/checker_uv_basecolor.png

@@ -0,0 +1,3 @@
+version https://git-lfs.github.com/spec/v1
+oid sha256:513f47f6fea5105f603170a8881b7e3b1cd2c4258636d64a6399c725032b500d
+size 38689

Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialSourceData.cpp

@@ -93,31 +93,28 @@ namespace AZ
 
             // Note that the only kind of property update currently supported is rename...
 
+            PropertyGroupMap newPropertyGroups;
             for (auto& groupPair : m_properties)
             {
                 PropertyMap& propertyMap = groupPair.second;
 
-                PropertyMap newPropertyMap;
-
                 for (auto& propertyPair : propertyMap)
                 {
                     MaterialPropertyId propertyId{groupPair.first, propertyPair.first};
+
                     if (materialTypeSourceData.ApplyPropertyRenames(propertyId, m_materialTypeVersion))
                     {
-                        newPropertyMap[propertyId.GetPropertyName().GetStringView()] = propertyPair.second;
                         changesWereApplied = true;
                     }
-                    else
-                    {
-                        newPropertyMap[propertyPair.first] = propertyPair.second;
-                    }
+                    
+                    newPropertyGroups[propertyId.GetGroupName().GetStringView()][propertyId.GetPropertyName().GetStringView()] = propertyPair.second;
                 }
-
-                propertyMap = newPropertyMap;
             }
 
             if (changesWereApplied)
             {
+                m_properties = AZStd::move(newPropertyGroups);
+
                 AZ_Warning("MaterialSourceData", false,
                     "This material is based on version '%u' of '%s', but the material type is now at version '%u'. "
                     "Automatic updates are available. Consider updating the .material source file.",

Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialTypeSourceData.cpp

@@ -164,16 +164,14 @@ namespace AZ
         const MaterialTypeSourceData::PropertyDefinition* MaterialTypeSourceData::FindProperty(AZStd::string_view groupName, AZStd::string_view propertyName, uint32_t materialTypeVersion) const
         {
             auto groupIter = m_propertyLayout.m_properties.find(groupName);
-            if (groupIter == m_propertyLayout.m_properties.end())
+            if (groupIter != m_propertyLayout.m_properties.end())
             {
-                return nullptr;
-            }
-
-            for (const PropertyDefinition& property : groupIter->second)
-            {
-                if (property.m_name == propertyName)
+                for (const PropertyDefinition& property : groupIter->second)
                 {
-                    return &property;
+                    if (property.m_name == propertyName)
+                    {
+                        return &property;
+                    }
                 }
             }
 
@@ -185,16 +183,14 @@ namespace AZ
             // Do the search again with the new names
 
             groupIter = m_propertyLayout.m_properties.find(propertyId.GetGroupName().GetStringView());
-            if (groupIter == m_propertyLayout.m_properties.end())
+            if (groupIter != m_propertyLayout.m_properties.end())
             {
-                return nullptr;
-            }
-
-            for (const PropertyDefinition& property : groupIter->second)
-            {
-                if (property.m_name == propertyId.GetPropertyName().GetStringView())
+                for (const PropertyDefinition& property : groupIter->second)
                 {
-                    return &property;
+                    if (property.m_name == propertyId.GetPropertyName().GetStringView())
+                    {
+                        return &property;
+                    }
                 }
             }
 

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

@@ -105,6 +105,13 @@ namespace UnitTest
                                     {"op": "rename", "from": "general.testColorNameB", "to": "general.testColorNameC"}
                                 ]
                             },
+                            {
+                                "toVersion": 6,
+                                "actions": [
+                                    {"op": "rename", "from": "oldGroup.MyFloat", "to": "general.MyFloat"},
+                                    {"op": "rename", "from": "oldGroup.MyIntOldName", "to": "general.MyInt"}
+                                ]
+                            },
                             {
                                 "toVersion": 10,
                                 "actions": [
@@ -751,6 +758,72 @@ namespace UnitTest
         material.ApplyVersionUpdates();
     }
     
+    TEST_F(MaterialSourceDataTests, Load_MaterialTypeVersionUpdate_MovePropertiesToAnotherGroup)
+    {
+        const AZStd::string inputJson = R"(
+        {
+            "materialType": "@exefolder@/Temp/test.materialtype",
+            "materialTypeVersion": 3,
+            "properties": {
+                "oldGroup": {
+                    "MyFloat": 1.2,
+                    "MyIntOldName": 5
+                }
+            }
+        }
+        )";
+
+        MaterialSourceData material;
+        JsonTestResult loadResult = LoadTestDataFromJson(material, inputJson);
+
+        EXPECT_EQ(AZ::JsonSerializationResult::Tasks::ReadField, loadResult.m_jsonResultCode.GetTask());
+        EXPECT_EQ(AZ::JsonSerializationResult::Processing::Completed, loadResult.m_jsonResultCode.GetProcessing());
+
+        // Initially, the loaded material data will match the .material file exactly. This gives us the accurate representation of
+        // what's actually saved on disk.
+        
+        EXPECT_NE(material.m_properties["oldGroup"].find("MyFloat"), material.m_properties["oldGroup"].end());
+        EXPECT_NE(material.m_properties["oldGroup"].find("MyIntOldName"), material.m_properties["oldGroup"].end());
+        EXPECT_EQ(material.m_properties["general"].find("MyFloat"), material.m_properties["general"].end());
+        EXPECT_EQ(material.m_properties["general"].find("MyInt"), material.m_properties["general"].end());
+
+        float myFloat = material.m_properties["oldGroup"]["MyFloat"].m_value.GetValue<float>();
+        EXPECT_EQ(myFloat, 1.2f);
+        
+        int32_t myInt = material.m_properties["oldGroup"]["MyIntOldName"].m_value.GetValue<int32_t>();
+        EXPECT_EQ(myInt, 5);
+
+        EXPECT_EQ(3, material.m_materialTypeVersion);
+
+        // Then we force the material data to update to the latest material type version specification
+        ErrorMessageFinder warningFinder; // Note this finds errors and warnings, and we're looking for a warning.
+        warningFinder.AddExpectedErrorMessage("Automatic updates are available. Consider updating the .material source file");
+        warningFinder.AddExpectedErrorMessage("This material is based on version '3'");
+        warningFinder.AddExpectedErrorMessage("material type is now at version '10'");
+        material.ApplyVersionUpdates();
+        warningFinder.CheckExpectedErrorsFound();
+
+        // Now the material data should match the latest material type.
+        // Look for the property under the latest name in the material type, not the name used in the .material file.
+        
+        EXPECT_EQ(material.m_properties["oldGroup"].find("MyFloat"), material.m_properties["oldGroup"].end());
+        EXPECT_EQ(material.m_properties["oldGroup"].find("MyIntOldName"), material.m_properties["oldGroup"].end());
+        EXPECT_NE(material.m_properties["general"].find("MyFloat"), material.m_properties["general"].end());
+        EXPECT_NE(material.m_properties["general"].find("MyInt"), material.m_properties["general"].end());
+        
+        myFloat = material.m_properties["general"]["MyFloat"].m_value.GetValue<float>();
+        EXPECT_EQ(myFloat, 1.2f);
+        
+        myInt = material.m_properties["general"]["MyInt"].m_value.GetValue<int32_t>();
+        EXPECT_EQ(myInt, 5);
+
+        EXPECT_EQ(10, material.m_materialTypeVersion);
+
+        // Calling ApplyVersionUpdates() again should not report the warning again, since the material has already been updated.
+        warningFinder.Reset();
+        material.ApplyVersionUpdates();
+    }
+    
     TEST_F(MaterialSourceDataTests, Load_MaterialTypeVersionPartialUpdate)
     {
         // This case is similar to Load_MaterialTypeVersionUpdate but we start at a later

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

@@ -1346,7 +1346,8 @@ namespace UnitTest
                     {
                         "toVersion": 7,
                         "actions": [
-                            { "op": "rename", "from": "general.bazA", "to": "otherGroup.bazB" }
+                            { "op": "rename", "from": "general.bazA", "to": "otherGroup.bazB" },
+                            { "op": "rename", "from": "onlyOneProperty.bopA", "to": "otherGroup.bopB" } // This tests a group 'onlyOneProperty' that no longer exists in the material type
                         ]
                     }
                 ],
@@ -1370,6 +1371,10 @@ namespace UnitTest
                             {
                                 "name": "bazB",
                                 "type": "Float"
+                            },
+                            {
+                                "name": "bopB",
+                                "type": "Float"
                             }
                         ]
                     }
@@ -1386,13 +1391,16 @@ namespace UnitTest
         const MaterialTypeSourceData::PropertyDefinition* foo = materialType.FindProperty("general", "fooC");
         const MaterialTypeSourceData::PropertyDefinition* bar = materialType.FindProperty("general", "barC");
         const MaterialTypeSourceData::PropertyDefinition* baz = materialType.FindProperty("otherGroup", "bazB");
+        const MaterialTypeSourceData::PropertyDefinition* bop = materialType.FindProperty("otherGroup", "bopB");
         
         EXPECT_TRUE(foo);
         EXPECT_TRUE(bar);
         EXPECT_TRUE(baz);
+        EXPECT_TRUE(bop);
         EXPECT_EQ(foo->m_name, "fooC");
         EXPECT_EQ(bar->m_name, "barC");
         EXPECT_EQ(baz->m_name, "bazB");
+        EXPECT_EQ(bop->m_name, "bopB");
 
         // Now try doing the property lookup using old versions of the name and make sure the same property can be found
 
@@ -1401,12 +1409,15 @@ namespace UnitTest
         EXPECT_EQ(bar, materialType.FindProperty("general", "barA"));
         EXPECT_EQ(bar, materialType.FindProperty("general", "barB"));
         EXPECT_EQ(baz, materialType.FindProperty("general", "bazA"));
+        EXPECT_EQ(bop, materialType.FindProperty("onlyOneProperty", "bopA"));
         
         EXPECT_EQ(nullptr, materialType.FindProperty("general", "fooX"));
         EXPECT_EQ(nullptr, materialType.FindProperty("general", "barX"));
         EXPECT_EQ(nullptr, materialType.FindProperty("general", "bazX"));
         EXPECT_EQ(nullptr, materialType.FindProperty("general", "bazB"));
         EXPECT_EQ(nullptr, materialType.FindProperty("otherGroup", "bazA"));
+        EXPECT_EQ(nullptr, materialType.FindProperty("onlyOneProperty", "bopB"));
+        EXPECT_EQ(nullptr, materialType.FindProperty("otherGroup", "bopA"));
     }
     
     TEST_F(MaterialTypeSourceDataTests, FindPropertyUsingOldName_Error_UnsupportedVersionUpdate)

Gems/Atom/TestData/TestData/Materials/StandardPbrTestCases/009_Opacity_Opaque_DoubleSided.material

@@ -0,0 +1,14 @@
+{
+    "description": "",
+    "materialType": "Materials/Types/StandardPBR.materialtype",
+    "parentMaterial": "",
+    "propertyLayoutVersion": 3,
+    "properties": {
+        "baseColor": {
+            "textureMap": "Textures/Default/checker_uv_basecolor.png"
+        },
+        "general": {
+            "doubleSided": true
+        }
+    }
+}

Gems/Atom/TestData/TestData/Objects/tube.fbx

@@ -0,0 +1,3 @@
+version https://git-lfs.github.com/spec/v1
+oid sha256:b2ecc32cd3052f3cb5836c8be7bf5cba54d98f46e6a0eeac95aaef00a123411a
+size 27340

Gems/Atom/Tools/MaterialEditor/Assets/MaterialEditor/ViewportModels/_dev_shaderball_00_basecolor.png

@@ -1,3 +1,3 @@
 version https://git-lfs.github.com/spec/v1
-oid sha256:93a7e033d9fb0fcac221647322bde03716643d789390f79078c4fcc37ecfd005
-size 68327
+oid sha256:513f47f6fea5105f603170a8881b7e3b1cd2c4258636d64a6399c725032b500d
+size 38689