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..c79ceb39ba 100644 --- a/Gems/Atom/Feature/Common/Code/Source/Material/MaterialAssignment.cpp +++ b/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( diff --git a/Gems/Atom/RPI/Code/Include/Atom/RPI.Public/Material/Material.h b/Gems/Atom/RPI/Code/Include/Atom/RPI.Public/Material/Material.h index d8a0cf0e7d..72c8616a73 100644 --- a/Gems/Atom/RPI/Code/Include/Atom/RPI.Public/Material/Material.h +++ b/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 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.Public/Material/Material.cpp b/Gems/Atom/RPI/Code/Source/RPI.Public/Material/Material.cpp index c87646e17d..050ae47749 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Public/Material/Material.cpp +++ b/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 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/MaterialTests.cpp b/Gems/Atom/RPI/Code/Tests/Material/MaterialTests.cpp index f202747bf6..9c3e08ee08 100644 --- a/Gems/Atom/RPI/Code/Tests/Material/MaterialTests.cpp +++ b/Gems/Atom/RPI/Code/Tests/Material/MaterialTests.cpp @@ -857,4 +857,35 @@ namespace UnitTest EXPECT_EQ(material->GetPropertyValue(material->FindPropertyIndex(Name{ "MyInt" })), -7); EXPECT_EQ(srgData->GetConstant(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::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); + } + } 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"); + } } diff --git a/Gems/AtomLyIntegration/CommonFeatures/Code/Include/AtomLyIntegration/CommonFeatures/Material/MaterialComponentBus.h b/Gems/AtomLyIntegration/CommonFeatures/Code/Include/AtomLyIntegration/CommonFeatures/Material/MaterialComponentBus.h index ed7f1564ac..293080367d 100644 --- a/Gems/AtomLyIntegration/CommonFeatures/Code/Include/AtomLyIntegration/CommonFeatures/Material/MaterialComponentBus.h +++ b/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 diff --git a/Gems/AtomLyIntegration/CommonFeatures/Code/Source/Material/EditorMaterialComponent.cpp b/Gems/AtomLyIntegration/CommonFeatures/Code/Source/Material/EditorMaterialComponent.cpp index a3ae61b14b..94beeb2430 100644 --- a/Gems/AtomLyIntegration/CommonFeatures/Code/Source/Material/EditorMaterialComponent.cpp +++ b/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) diff --git a/Gems/AtomLyIntegration/CommonFeatures/Code/Source/Material/EditorMaterialComponentInspector.cpp b/Gems/AtomLyIntegration/CommonFeatures/Code/Source/Material/EditorMaterialComponentInspector.cpp index b6875e2e9c..9af3b41b12 100644 --- a/Gems/AtomLyIntegration/CommonFeatures/Code/Source/Material/EditorMaterialComponentInspector.cpp +++ b/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> 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) diff --git a/Gems/AtomLyIntegration/CommonFeatures/Code/Source/Material/MaterialComponentController.cpp b/Gems/AtomLyIntegration/CommonFeatures/Code/Source/Material/MaterialComponentController.cpp index 4c26042a26..07082a87d5 100644 --- a/Gems/AtomLyIntegration/CommonFeatures/Code/Source/Material/MaterialComponentController.cpp +++ b/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> 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) { diff --git a/Gems/AtomLyIntegration/CommonFeatures/Code/Source/Material/MaterialComponentController.h b/Gems/AtomLyIntegration/CommonFeatures/Code/Source/Material/MaterialComponentController.h index 1594456a31..ec542c377c 100644 --- a/Gems/AtomLyIntegration/CommonFeatures/Code/Source/Material/MaterialComponentController.h +++ b/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;