Made material property auto-rename procedure apply to Material Component

Merge pull request #4905 from aws-lumberyard-dev/Atom/santorac/MaterialPropertyRenameInMaterialComponent

This ensures that an material property overrides and any gameplay scripts that work with property overrides can get the benefit of the material type version update procedure.

I added an ApplyPropertyRenames function to MaterialTypeAsset very similar to the one in MaterialTypeSourceData.
Updated the MaterialAssignment class to apply any property renames when it discovers the old name doesn't work. This will be written to disk when the level or prefab is saved.

AutomatedTesting/Materials/MinimalBlue.material

@@ -0,0 +1,17 @@
+{
+    "description": "",
+    "parentMaterial": "",
+    "materialType": "TestData/Materials/Types/MinimalPBR.materialtype",
+    "materialTypeVersion": 3,
+    "properties": {
+        "settings": {
+            "color": [
+                0.08522164076566696,
+                0.11898985505104065,
+                1.0,
+                1.0
+            ],
+            "roughness": 0.33000001311302185
+        }
+    }
+}

Gems/Atom/Feature/Common/Code/Source/Material/MaterialAssignment.cpp

@@ -141,7 +141,26 @@ namespace AZ
                 {
                     if (!propertyPair.second.empty())
                     {
-                        const auto& materialPropertyIndex = m_materialInstance->FindPropertyIndex(propertyPair.first);
+                        bool wasRenamed = false;
+                        Name newName;
+                        RPI::MaterialPropertyIndex materialPropertyIndex = m_materialInstance->FindPropertyIndex(propertyPair.first, &wasRenamed, &newName);
+
+                        // FindPropertyIndex will have already reported a message about what the old and new names are. Here we just add some extra info to help the user resolve it.
+                        AZ_Warning("MaterialAssignment", !wasRenamed,
+                            "Consider running \"Apply Automatic Property Updates\" to use the latest property names.",
+                            propertyPair.first.GetCStr(),
+                            newName.GetCStr());
+
+                        if (wasRenamed && m_propertyOverrides.find(newName) != m_propertyOverrides.end())
+                        {
+                            materialPropertyIndex.Reset();
+                            
+                            AZ_Warning("MaterialAssignment", false,
+                                "Material property '%s' has been renamed to '%s', and a property override exists for both. The one with the old name will be ignored.",
+                                propertyPair.first.GetCStr(),
+                                newName.GetCStr());
+                        }
+
                         if (!materialPropertyIndex.IsNull())
                         {
                             m_materialInstance->SetPropertyValue(

Gems/Atom/RPI/Code/Include/Atom/RPI.Public/Material/Material.h

@@ -71,7 +71,9 @@ namespace AZ
             virtual ~Material();
 
             //! Finds the material property index from the material property ID
-            MaterialPropertyIndex FindPropertyIndex(const Name& propertyId) const;
+            //! @param wasRenamed optional parameter that is set to true if @propertyId is an old name and an automatic rename was applied to find the index.
+            //! @param newName optional parameter that is set to the new property name, if the property was renamed.
+            MaterialPropertyIndex FindPropertyIndex(const Name& propertyId, bool* wasRenamed = nullptr, Name* newName = nullptr) const;
 
             //! Sets the value of a material property. The template data type must match the property's data type.
             //! @return true if property value was changed

Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialTypeAsset.h

@@ -129,6 +129,10 @@ namespace AZ
  
             const AZStd::vector<MaterialVersionUpdate>& GetMaterialVersionUpdateList() const { return m_materialVersionUpdates; }
 
+            //! Possibly renames @propertyId based on the material version update steps.
+            //! @return true if the property was renamed
+            bool ApplyPropertyRenames(AZ::Name& propertyId) const;
+
         private:
             bool PostLoadInit() override;
 

Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialVersionUpdate.h

@@ -44,6 +44,10 @@ namespace AZ
 
             uint32_t GetVersion() const;
             void SetVersion(uint32_t toVersion);
+            
+            //! Possibly renames @propertyId based on the material version update steps.
+            //! @return true if the property was renamed
+            bool ApplyPropertyRenames(AZ::Name& propertyId) const;
 
             //! Apply version updates to the given material asset.
             //! @return true if any changes were made

Gems/Atom/RPI/Code/Source/RPI.Public/Material/Material.cpp

@@ -386,9 +386,39 @@ namespace AZ
             return m_currentChangeId;
         }
 
-        MaterialPropertyIndex Material::FindPropertyIndex(const Name& propertyId) const
+        MaterialPropertyIndex Material::FindPropertyIndex(const Name& propertyId, bool* wasRenamed, Name* newName) const
         {
-            return m_layout->FindPropertyIndex(propertyId);
+            if (wasRenamed)
+            {
+                *wasRenamed = false;
+            }
+
+            MaterialPropertyIndex index = m_layout->FindPropertyIndex(propertyId);
+            if (!index.IsValid())
+            {
+                Name renamedId = propertyId;
+                
+                if (m_materialAsset->GetMaterialTypeAsset()->ApplyPropertyRenames(renamedId))
+                {                                
+                    index = m_layout->FindPropertyIndex(renamedId);
+
+                    if (wasRenamed)
+                    {
+                        *wasRenamed = true;
+                    }
+
+                    if (newName)
+                    {
+                        *newName = renamedId;
+                    }
+
+                    AZ_Warning("Material", false,
+                        "Material property '%s' has been renamed to '%s'. Consider updating the corresponding source data.",
+                        propertyId.GetCStr(),
+                        renamedId.GetCStr());
+                }
+            }
+            return index;
         }
 
         template<typename Type>

Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialTypeAsset.cpp

@@ -169,6 +169,22 @@ namespace AZ
             return m_version;
         }
 
+
+        bool MaterialTypeAsset::ApplyPropertyRenames(AZ::Name& propertyId) const
+        {
+            bool renamed = false;
+
+            for (const auto& versionUpdates : m_materialVersionUpdates)
+            {
+                if (versionUpdates.ApplyPropertyRenames(propertyId))
+                {
+                    renamed = true;
+                }
+            }
+
+            return renamed;
+        }
+
         void MaterialTypeAsset::SetReady()
         {
             m_status = AssetStatus::Ready;

Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialVersionUpdate.cpp

@@ -56,6 +56,22 @@ namespace AZ
         {
             m_toVersion = toVersion;
         }
+        
+        bool MaterialVersionUpdate::ApplyPropertyRenames(AZ::Name& propertyId) const
+        {
+            bool renamed = false;
+
+            for (const auto& action : m_actions)
+            {
+                if (action.m_fromPropertyId == propertyId)
+                {
+                    propertyId = action.m_toPropertyId;
+                    renamed = true;
+                }
+            }
+
+            return renamed;
+        }
 
         bool MaterialVersionUpdate::ApplyVersionUpdates(MaterialAsset& materialAsset) const
         {

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

@@ -857,4 +857,35 @@ namespace UnitTest
         EXPECT_EQ(material->GetPropertyValue<int32_t>(material->FindPropertyIndex(Name{ "MyInt" })), -7);
         EXPECT_EQ(srgData->GetConstant<int32_t>(srgData->FindShaderInputConstantIndex(Name{ "m_int" })), -7);
     }
+
+    TEST_F(MaterialTests, TestFindPropertyIndexUsingOldName)
+    {
+        MaterialTypeAssetCreator materialTypeCreator;
+        materialTypeCreator.Begin(Uuid::CreateRandom());
+        materialTypeCreator.AddShader(m_testMaterialShaderAsset);
+        AddCommonTestMaterialProperties(materialTypeCreator);
+        materialTypeCreator.SetVersion(2);
+        MaterialVersionUpdate versionUpdate(2);
+        versionUpdate.AddAction(MaterialVersionUpdate::RenamePropertyAction({Name{ "OldName" },Name{ "MyInt" }}));
+        materialTypeCreator.AddVersionUpdate(versionUpdate);
+        materialTypeCreator.End(m_testMaterialTypeAsset);
+
+        MaterialAssetCreator materialCreator;
+        materialCreator.Begin(Uuid::CreateRandom(), *m_testMaterialTypeAsset);
+        materialCreator.End(m_testMaterialAsset);
+
+        Data::Instance<Material> material = Material::FindOrCreate(m_testMaterialAsset);
+
+        bool wasRenamed = false;
+        Name newName;
+        MaterialPropertyIndex indexFromOldName = material->FindPropertyIndex(Name{"OldName"}, &wasRenamed, &newName);
+        EXPECT_TRUE(wasRenamed);
+        EXPECT_EQ(newName, Name{"MyInt"});
+        
+        MaterialPropertyIndex indexFromNewName = material->FindPropertyIndex(Name{"MyInt"}, &wasRenamed, &newName);
+        EXPECT_FALSE(wasRenamed);
+
+        EXPECT_EQ(indexFromOldName, indexFromNewName);
+    }
+
 }

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

@@ -1041,5 +1041,88 @@ namespace UnitTest
         EXPECT_FALSE(materialTypeAsset->GetShaderCollection()[1].MaterialOwnsShaderOption(Name{"o_globalOption_inShaderB"}));
     }
 
+    TEST_F(MaterialTypeAssetTests, ApplyPropertyRenames)
+    {
+        Data::Asset<MaterialTypeAsset> materialTypeAsset;
+
+        auto addRenameAction = [](MaterialVersionUpdate& versionUpdate, const char* from, const char* to)
+        {
+            versionUpdate.AddAction(MaterialVersionUpdate::RenamePropertyAction(
+                {
+                    Name{ from },
+                    Name{ to }
+                }));
+        };
+
+        MaterialTypeAssetCreator materialTypeCreator;
+        materialTypeCreator.Begin(Uuid::CreateRandom());
+
+        // Version updates
+        materialTypeCreator.SetVersion(10);
+
+        MaterialVersionUpdate versionUpdate2(2);
+        addRenameAction(versionUpdate2, "general.fooA", "general.fooB");
+        materialTypeCreator.AddVersionUpdate(versionUpdate2);
+
+        MaterialVersionUpdate versionUpdate4(4);
+        addRenameAction(versionUpdate4, "general.barA", "general.barB");
+        materialTypeCreator.AddVersionUpdate(versionUpdate4);
+
+        MaterialVersionUpdate versionUpdate6(6);
+        addRenameAction(versionUpdate6, "general.fooB", "general.fooC");
+        addRenameAction(versionUpdate6, "general.barB", "general.barC");
+        materialTypeCreator.AddVersionUpdate(versionUpdate6);
+
+        MaterialVersionUpdate versionUpdate7(7);
+        addRenameAction(versionUpdate7, "general.bazA", "otherGroup.bazB");
+        materialTypeCreator.AddVersionUpdate(versionUpdate7);
+        
+        materialTypeCreator.BeginMaterialProperty(Name{ "general.fooC" }, MaterialPropertyDataType::Bool);
+        materialTypeCreator.EndMaterialProperty();
+        materialTypeCreator.BeginMaterialProperty(Name{ "general.barC" }, MaterialPropertyDataType::Bool);
+        materialTypeCreator.EndMaterialProperty();
+        materialTypeCreator.BeginMaterialProperty(Name{ "otherGroup.bazB" }, MaterialPropertyDataType::Bool);
+        materialTypeCreator.EndMaterialProperty();
+
+        EXPECT_TRUE(materialTypeCreator.End(materialTypeAsset));
+
+        AZ::Name propertyId;
+
+        propertyId = AZ::Name{"doesNotExist"};
+        EXPECT_FALSE(materialTypeAsset->ApplyPropertyRenames(propertyId));
+        EXPECT_STREQ(propertyId.GetCStr(), "doesNotExist");
+
+        propertyId = AZ::Name{"general.fooA"};
+        EXPECT_TRUE(materialTypeAsset->ApplyPropertyRenames(propertyId));
+        EXPECT_STREQ(propertyId.GetCStr(), "general.fooC");
+
+        propertyId = AZ::Name{"general.fooB"};
+        EXPECT_TRUE(materialTypeAsset->ApplyPropertyRenames(propertyId));
+        EXPECT_STREQ(propertyId.GetCStr(), "general.fooC");
+
+        propertyId = AZ::Name{"general.fooC"};
+        EXPECT_FALSE(materialTypeAsset->ApplyPropertyRenames(propertyId));
+        EXPECT_STREQ(propertyId.GetCStr(), "general.fooC");
+
+        propertyId = AZ::Name{"general.barA"};
+        EXPECT_TRUE(materialTypeAsset->ApplyPropertyRenames(propertyId));
+        EXPECT_STREQ(propertyId.GetCStr(), "general.barC");
+
+        propertyId = AZ::Name{"general.barB"};
+        EXPECT_TRUE(materialTypeAsset->ApplyPropertyRenames(propertyId));
+        EXPECT_STREQ(propertyId.GetCStr(), "general.barC");
+
+        propertyId = AZ::Name{"general.barC"};
+        EXPECT_FALSE(materialTypeAsset->ApplyPropertyRenames(propertyId));
+        EXPECT_STREQ(propertyId.GetCStr(), "general.barC");
+
+        propertyId = AZ::Name{"general.bazA"};
+        EXPECT_TRUE(materialTypeAsset->ApplyPropertyRenames(propertyId));
+        EXPECT_STREQ(propertyId.GetCStr(), "otherGroup.bazB");
+
+        propertyId = AZ::Name{"otherGroup.bazB"};
+        EXPECT_FALSE(materialTypeAsset->ApplyPropertyRenames(propertyId));
+        EXPECT_STREQ(propertyId.GetCStr(), "otherGroup.bazB");
+    }
 }
 

Gems/AtomLyIntegration/CommonFeatures/Code/Include/AtomLyIntegration/CommonFeatures/Material/MaterialComponentBus.h

@@ -43,6 +43,9 @@ namespace AZ
             virtual void ClearInvalidMaterialOverrides() = 0;
             //! Repair materials that reference missing assets by assigning the default asset
             virtual void RepairInvalidMaterialOverrides() = 0;
+            //! Repair material property overrides that reference missing properties by auto-renaming them where possible
+            //! @return the number of properties that were updated
+            virtual uint32_t ApplyAutomaticPropertyUpdates() = 0;
             //! Set default material override
             virtual void SetDefaultMaterialOverride(const AZ::Data::AssetId& materialAssetId) = 0;
             //! Get default material override

Gems/AtomLyIntegration/CommonFeatures/Code/Source/Material/EditorMaterialComponent.cpp

@@ -240,6 +240,19 @@ namespace AZ
                 UpdateMaterialSlots();
             });
             action->setToolTip("Repair materials that reference missing assets by assigning the default asset.");
+            
+            action = menu->addAction("Apply Automatic Property Updates", [this]() {
+                AzToolsFramework::ScopedUndoBatch undoBatch("Applying automatic property updates.");
+                SetDirty();
+
+                uint32_t propertiesUpdated = 0;
+                MaterialComponentRequestBus::EventResult(propertiesUpdated, GetEntityId(), &MaterialComponentRequestBus::Events::ApplyAutomaticPropertyUpdates);
+
+                AZ_Printf("EditorMaterialComponent", "Updated %u property(s).", propertiesUpdated);
+
+                UpdateMaterialSlots();
+            });
+            action->setToolTip("Repair material property overrides that reference missing properties by auto-renaming them where possible.");
         }
 
         void EditorMaterialComponent::SetPrimaryAsset(const AZ::Data::AssetId& assetId)

Gems/AtomLyIntegration/CommonFeatures/Code/Source/Material/EditorMaterialComponentInspector.cpp

@@ -352,6 +352,25 @@ namespace AZ
                     m_editData.m_materialPropertyOverrideMap, m_entityId, &MaterialComponentRequestBus::Events::GetPropertyOverrides,
                     m_materialAssignmentId);
 
+                // Apply any automatic property renames so that the material inspector will be properly initialized with the right values
+                // for properties that have new names.
+                {
+                    AZStd::vector<AZStd::pair<Name, Name>> renamedProperties;
+                    for (auto& propertyOverridePair : m_editData.m_materialPropertyOverrideMap)
+                    {
+                        Name name = propertyOverridePair.first;
+                        if (m_materialInstance->GetAsset()->GetMaterialTypeAsset()->ApplyPropertyRenames(name))
+                        {
+                            renamedProperties.emplace_back(propertyOverridePair.first, name);
+                        }
+                    }
+                    for (const auto& [oldName, newName] : renamedProperties)
+                    {
+                        m_editData.m_materialPropertyOverrideMap[newName] = m_editData.m_materialPropertyOverrideMap[oldName];
+                        m_editData.m_materialPropertyOverrideMap.erase(oldName);
+                    }
+                }
+
                 for (auto& group : m_groups)
                 {
                     for (auto& property : group.second.m_properties)

Gems/AtomLyIntegration/CommonFeatures/Code/Source/Material/MaterialComponentController.cpp

@@ -49,6 +49,7 @@ namespace AZ
                     ->Event("ClearIncompatibleMaterialOverrides", &MaterialComponentRequestBus::Events::ClearIncompatibleMaterialOverrides)
                     ->Event("ClearInvalidMaterialOverrides", &MaterialComponentRequestBus::Events::ClearInvalidMaterialOverrides)
                     ->Event("RepairInvalidMaterialOverrides", &MaterialComponentRequestBus::Events::RepairInvalidMaterialOverrides)
+                    ->Event("ApplyAutomaticPropertyUpdates", &MaterialComponentRequestBus::Events::ApplyAutomaticPropertyUpdates)
                     ->Event("SetMaterialOverride", &MaterialComponentRequestBus::Events::SetMaterialOverride)
                     ->Event("GetMaterialOverride", &MaterialComponentRequestBus::Events::GetMaterialOverride)
                     ->Event("ClearMaterialOverride", &MaterialComponentRequestBus::Events::ClearMaterialOverride)
@@ -406,6 +407,37 @@ namespace AZ
             }
             LoadMaterials();
         }
+        
+        uint32_t MaterialComponentController::ApplyAutomaticPropertyUpdates()
+        {
+            uint32_t propertiesUpdated = 0;
+
+            for (auto& materialAssignmentPair : m_configuration.m_materials)
+            {
+                MaterialAssignment& materialAssignment = materialAssignmentPair.second;
+                
+                AZStd::vector<AZStd::pair<Name, Name>> renamedProperties;
+
+                for (const auto& propertyPair : materialAssignment.m_propertyOverrides)
+                {
+                    Name propertyId = propertyPair.first;
+
+                    if (materialAssignment.m_materialInstance->GetAsset()->GetMaterialTypeAsset()->ApplyPropertyRenames(propertyId))
+                    {
+                        renamedProperties.emplace_back(propertyPair.first, propertyId);
+                        ++propertiesUpdated;
+                    }
+                }
+                
+                for (const auto& [oldName, newName] : renamedProperties)
+                {
+                    materialAssignment.m_propertyOverrides[newName] = materialAssignment.m_propertyOverrides[oldName];
+                    materialAssignment.m_propertyOverrides.erase(oldName);
+                }
+            }
+
+            return propertiesUpdated;
+        }
 
         void MaterialComponentController::SetDefaultMaterialOverride(const AZ::Data::AssetId& materialAssetId)
         {

Gems/AtomLyIntegration/CommonFeatures/Code/Source/Material/MaterialComponentController.h

@@ -58,6 +58,7 @@ namespace AZ
             void ClearIncompatibleMaterialOverrides() override;
             void ClearInvalidMaterialOverrides() override;
             void RepairInvalidMaterialOverrides() override;
+            uint32_t ApplyAutomaticPropertyUpdates() override;
             void SetDefaultMaterialOverride(const AZ::Data::AssetId& materialAssetId) override;
             const AZ::Data::AssetId GetDefaultMaterialOverride() const override;
             void ClearDefaultMaterialOverride() override;