From c6b209ace0d24ff6fa1bc1d2f4a5c4ce04fbd90b Mon Sep 17 00:00:00 2001 From: santorac <55155825+santorac@users.noreply.github.com> Date: Thu, 21 Oct 2021 23:18:08 -0700 Subject: [PATCH] Made material property auto-rename procedure apply to Material Component at runtime. 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. Signed-off-by: santorac <55155825+santorac@users.noreply.github.com> --- .../Materials/MinimalBlue.material | 17 ++++ .../Source/Material/MaterialAssignment.cpp | 33 +++++++- .../RPI.Reflect/Material/MaterialTypeAsset.h | 4 + .../Material/MaterialVersionUpdate.h | 4 + .../Material/MaterialTypeAsset.cpp | 16 ++++ .../Material/MaterialVersionUpdate.cpp | 16 ++++ .../Tests/Material/MaterialTypeAssetTests.cpp | 83 +++++++++++++++++++ 7 files changed, 172 insertions(+), 1 deletion(-) create mode 100644 AutomatedTesting/Materials/MinimalBlue.material diff --git a/AutomatedTesting/Materials/MinimalBlue.material b/AutomatedTesting/Materials/MinimalBlue.material new file mode 100644 index 0000000000..7310a90250 --- /dev/null +++ b/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 + } + } +} \ No newline at end of file diff --git a/Gems/Atom/Feature/Common/Code/Source/Material/MaterialAssignment.cpp b/Gems/Atom/Feature/Common/Code/Source/Material/MaterialAssignment.cpp index 8984ec160b..f8a8d17c55 100644 --- a/Gems/Atom/Feature/Common/Code/Source/Material/MaterialAssignment.cpp +++ b/Gems/Atom/Feature/Common/Code/Source/Material/MaterialAssignment.cpp @@ -137,11 +137,33 @@ namespace AZ if (m_materialInstance->CanCompile()) { + AZStd::vector> renamedProperties; + for (const auto& propertyPair : m_propertyOverrides) { if (!propertyPair.second.empty()) { - const auto& materialPropertyIndex = m_materialInstance->FindPropertyIndex(propertyPair.first); + RPI::MaterialPropertyIndex materialPropertyIndex = m_materialInstance->FindPropertyIndex(propertyPair.first); + + // If we didn't find the name, check to see if there was a rename that should be used. + if (materialPropertyIndex.IsNull()) + { + Name propertyId = propertyPair.first; + + if (m_materialInstance->GetAsset()->GetMaterialTypeAsset()->ApplyPropertyRenames(propertyId)) + { + renamedProperties.emplace_back(propertyPair.first, propertyId); + + // We should be able to find the property now, using the new name + materialPropertyIndex = m_materialInstance->FindPropertyIndex(propertyId); + + AZ_Warning("MaterialAssignment", false, + "Material property '%s' has been renamed to '%s', and a material override references the old name. Save the level or prefab to permanently apply this change.", + propertyPair.first.GetCStr(), + propertyId.GetCStr()); + } + } + if (!materialPropertyIndex.IsNull()) { m_materialInstance->SetPropertyValue( @@ -150,6 +172,15 @@ namespace AZ } } + // Now that we're done looping over all the overrides, it's safe to apply any renames that were scheduled + for (auto& pair : renamedProperties) + { + const Name& oldName = pair.first; + const Name& newName = pair.second; + m_propertyOverrides[newName] = m_propertyOverrides[oldName]; + m_propertyOverrides.erase(oldName); + } + return m_materialInstance->Compile(); } diff --git a/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialTypeAsset.h b/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialTypeAsset.h index 9bc0e018d3..f194d84263 100644 --- a/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialTypeAsset.h +++ b/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialTypeAsset.h @@ -129,6 +129,10 @@ namespace AZ const AZStd::vector& 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; diff --git a/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialVersionUpdate.h b/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialVersionUpdate.h index eb71ad9cb7..3ecfe7b5e0 100644 --- a/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialVersionUpdate.h +++ b/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 diff --git a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialTypeAsset.cpp b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialTypeAsset.cpp index 522ba74119..76634201eb 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialTypeAsset.cpp +++ b/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; diff --git a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialVersionUpdate.cpp b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialVersionUpdate.cpp index b387e502e2..f5dfe9e80c 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialVersionUpdate.cpp +++ b/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 { diff --git a/Gems/Atom/RPI/Code/Tests/Material/MaterialTypeAssetTests.cpp b/Gems/Atom/RPI/Code/Tests/Material/MaterialTypeAssetTests.cpp index 81b723ec04..382ab4e149 100644 --- a/Gems/Atom/RPI/Code/Tests/Material/MaterialTypeAssetTests.cpp +++ b/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; + + 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"); + } }