diff --git a/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialSourceData.cpp b/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialSourceData.cpp index 5aed6b2993..1467b017d5 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialSourceData.cpp +++ b/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.", diff --git a/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialTypeSourceData.cpp b/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialTypeSourceData.cpp index 74250647c3..08f57c7cd3 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialTypeSourceData.cpp +++ b/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; + } } } diff --git a/Gems/Atom/RPI/Code/Tests/Material/MaterialSourceDataTests.cpp b/Gems/Atom/RPI/Code/Tests/Material/MaterialSourceDataTests.cpp index acce52ae8e..fa8eed35de 100644 --- a/Gems/Atom/RPI/Code/Tests/Material/MaterialSourceDataTests.cpp +++ b/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(); + EXPECT_EQ(myFloat, 1.2f); + + int32_t myInt = material.m_properties["oldGroup"]["MyIntOldName"].m_value.GetValue(); + 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(); + EXPECT_EQ(myFloat, 1.2f); + + myInt = material.m_properties["general"]["MyInt"].m_value.GetValue(); + 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 diff --git a/Gems/Atom/RPI/Code/Tests/Material/MaterialTypeSourceDataTests.cpp b/Gems/Atom/RPI/Code/Tests/Material/MaterialTypeSourceDataTests.cpp index b811e630d0..2fe7b1dbe5 100644 --- a/Gems/Atom/RPI/Code/Tests/Material/MaterialTypeSourceDataTests.cpp +++ b/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)