Fixed a bug where material version updates didn't support moving a property from one group to another.

Signed-off-by: santorac <55155825+santorac@users.noreply.github.com>

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
+                    
-                    {
+                    newPropertyGroups[propertyId.GetGroupName().GetStringView()][propertyId.GetPropertyName().GetStringView()] = propertyPair.second;
-                        newPropertyMap[propertyPair.first] = 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)
-            }
-
-            for (const PropertyDefinition& property : groupIter->second)
-            {
-                if (property.m_name == propertyName)
                 {
-                    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)
-            }
-
-            for (const PropertyDefinition& property : groupIter->second)
-            {
-                if (property.m_name == propertyId.GetPropertyName().GetStringView())
                 {
-                    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)