diff --git a/Gems/Atom/RPI/Code/Include/Atom/RPI.Edit/Material/MaterialSourceData.h b/Gems/Atom/RPI/Code/Include/Atom/RPI.Edit/Material/MaterialSourceData.h index deeace0d41..9557d51849 100644 --- a/Gems/Atom/RPI/Code/Include/Atom/RPI.Edit/Material/MaterialSourceData.h +++ b/Gems/Atom/RPI/Code/Include/Atom/RPI.Edit/Material/MaterialSourceData.h @@ -66,6 +66,7 @@ namespace AZ //! Checks the material type version and potentially applies a series of property changes (most common are simple property renames) //! based on the MaterialTypeAsset's version update procedure. + //! @return true if any changes were applied bool ApplyVersionUpdates(); //! Creates a MaterialAsset from the MaterialSourceData content. 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 fd5a57fa8d..7aede97301 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 @@ -43,7 +43,9 @@ namespace AZ uint32_t GetVersion() const; void SetVersion(uint32_t toVersion); - void ApplyVersionUpdates(MaterialAsset& materialAsset) const; + //! Apply version updates to the given material asset. + //! @return true if any changes were made + bool ApplyVersionUpdates(MaterialAsset& materialAsset) const; using Actions = AZStd::vector; const Actions& GetActions() const; 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 e43095f639..c3a63fa883 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialSourceData.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialSourceData.cpp @@ -80,9 +80,16 @@ namespace AZ { return false; } - + MaterialTypeSourceData materialTypeSourceData = materialTypeSourceDataOutcome.TakeValue(); + if (m_materialTypeVersion == materialTypeSourceData.m_version) + { + return false; + } + + bool changesWereApplied = false; + // Note that the only kind of property update currently supported is rename... for (auto& groupPair : m_properties) @@ -97,6 +104,7 @@ namespace AZ if (materialTypeSourceData.ApplyPropertyRenames(propertyId, m_materialTypeVersion)) { newPropertyMap[propertyId.GetPropertyName().GetStringView()] = propertyPair.second; + changesWereApplied = true; } else { @@ -107,8 +115,16 @@ namespace AZ propertyMap = newPropertyMap; } - m_materialTypeVersion = materialTypeSourceData.m_version; + if (changesWereApplied) + { + 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.", + m_materialTypeVersion, m_materialType.c_str(), materialTypeSourceData.m_version); + } + m_materialTypeVersion = materialTypeSourceData.m_version; + return true; } diff --git a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialAsset.cpp b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialAsset.cpp index ac740107f2..aa188bead6 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialAsset.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialAsset.cpp @@ -105,11 +105,18 @@ namespace AZ AZStd::array_view MaterialAsset::GetPropertyValues() const { + // If property names are included, they are used to re-arrange the property value list to align with the + // MaterialPropertiesLayout. This realignment would be necessary if the material type is updated with + // a new property layout, and a corresponding material is not reprocessed by the AP and continues using the + // old property layout. if (!m_propertyNames.empty()) { const uint32_t materialTypeVersion = m_materialTypeAsset->GetVersion(); if (m_materialTypeVersion < materialTypeVersion) { + // It is possible that the material type has had some properties renamed. If that's the case, and this material + // is still referencing the old property layout, we need to apply any auto updates to rename those properties + // before using them to realign the property values. const_cast(this)->ApplyVersionUpdates(); } @@ -196,11 +203,29 @@ namespace AZ void MaterialAsset::ApplyVersionUpdates() { + if (m_materialTypeVersion == m_materialTypeAsset->GetVersion()) + { + return; + } + + bool changesWereApplied = false; + for (int i = 0; i < aznumeric_cast(m_materialTypeAsset->GetVersion() - m_materialTypeVersion); ++i) { const auto& versionUpdate = m_materialTypeAsset->GetMaterialVersionUpdate(m_materialTypeVersion + i + 1); - versionUpdate.ApplyVersionUpdates(*this); + if (versionUpdate.ApplyVersionUpdates(*this)) + { + changesWereApplied = true; + } + } + + if (changesWereApplied) + { + AZ_Warning("MaterialAsset", 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.", + m_materialTypeVersion, m_materialTypeAsset.ToString().c_str(), m_materialTypeAsset->GetVersion()); } m_materialTypeVersion = m_materialTypeAsset->GetVersion(); 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 de85d4230e..48ad095eba 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialVersionUpdate.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialVersionUpdate.cpp @@ -51,8 +51,10 @@ namespace AZ m_toVersion = toVersion; } - void MaterialVersionUpdate::ApplyVersionUpdates(MaterialAsset& materialAsset) const + bool MaterialVersionUpdate::ApplyVersionUpdates(MaterialAsset& materialAsset) const { + bool changesWereApplied = false; + for (auto& propertyName : materialAsset.m_propertyNames) { for (const auto& action : m_actions) @@ -60,9 +62,12 @@ namespace AZ if (propertyName == action.m_fromPropertyId) { propertyName = action.m_toPropertyId; + changesWereApplied = true; } } } + + return changesWereApplied; } const AZ::RPI::MaterialVersionUpdate::Actions& MaterialVersionUpdate::GetActions() const diff --git a/Gems/Atom/RPI/Code/Tests/Material/MaterialAssetTests.cpp b/Gems/Atom/RPI/Code/Tests/Material/MaterialAssetTests.cpp index df45e81ddf..d376ca06b7 100644 --- a/Gems/Atom/RPI/Code/Tests/Material/MaterialAssetTests.cpp +++ b/Gems/Atom/RPI/Code/Tests/Material/MaterialAssetTests.cpp @@ -10,6 +10,7 @@ #include #include #include +#include #include #include @@ -64,15 +65,9 @@ namespace UnitTest RPITestFixture::TearDown(); } - void UpgradeAndValidateMaterialAsset(Data::Asset materialAsset, Data::Asset upgradedMaterialTypeAsset) + void ReplaceMaterialType(Data::Asset materialAsset, Data::Asset upgradedMaterialTypeAsset) { - // Set materialTypeAsset to the upgraded version. - EXPECT_EQ(1, materialAsset->m_materialTypeVersion); materialAsset->m_materialTypeAsset = upgradedMaterialTypeAsset; - materialAsset->ApplyVersionUpdates(); - - EXPECT_EQ(2, materialAsset->m_materialTypeVersion); - EXPECT_EQ(AZ::Name{ "MyBoolNext" }, materialAsset->m_propertyNames[0]); } }; @@ -215,6 +210,10 @@ namespace UnitTest TEST_F(MaterialAssetTests, UpgradeMaterialAsset) { + // Here we test the main way that a material asset upgrade would be applied at runtime: A material type is updated to + // both rename a property *and* change the order in which properties appear in the layout. In this case, the new name + // must be identified and then that new name is used to find the appropriate index in the property layout. + auto materialSrgLayout = CreateCommonTestMaterialSrgLayout(); auto shaderAsset = CreateTestShaderAsset(Uuid::CreateRandom(), materialSrgLayout); @@ -223,16 +222,20 @@ namespace UnitTest MaterialTypeAssetCreator materialTypeCreator; materialTypeCreator.Begin(Uuid::CreateRandom()); materialTypeCreator.AddShader(shaderAsset); - AddMaterialPropertyForSrg(materialTypeCreator, Name{ "MyBool" }, MaterialPropertyDataType::Bool, Name{ "m_bool" }); - materialTypeCreator.SetPropertyValue(Name{ "MyBool" }, true); + AddMaterialPropertyForSrg(materialTypeCreator, Name{ "MyInt" }, MaterialPropertyDataType::Int, Name{ "m_int" }); + AddMaterialPropertyForSrg(materialTypeCreator, Name{ "MyUInt" }, MaterialPropertyDataType::UInt, Name{ "m_uint" }); + AddMaterialPropertyForSrg(materialTypeCreator, Name{ "MyFloat" }, MaterialPropertyDataType::Float, Name{ "m_float" }); EXPECT_TRUE(materialTypeCreator.End(testMaterialTypeAssetV1)); // Construct the material asset with materialTypeAsset version 1 Data::AssetId assetId(Uuid::CreateRandom()); MaterialAssetCreator creator; - creator.Begin(assetId, *testMaterialTypeAssetV1); - creator.SetPropertyValue(Name{ "MyBool" }, true); + const bool includePropertyNames = true; + creator.Begin(assetId, *testMaterialTypeAssetV1, includePropertyNames); + creator.SetPropertyValue(Name{ "MyInt" }, 7); + creator.SetPropertyValue(Name{ "MyUInt" }, 8u); + creator.SetPropertyValue(Name{ "MyFloat" }, 9.0f); Data::Asset materialAsset; EXPECT_TRUE(creator.End(materialAsset)); @@ -240,8 +243,8 @@ namespace UnitTest MaterialVersionUpdate versionUpdate(2); versionUpdate.AddAction(MaterialVersionUpdate::RenamePropertyAction( { - Name{ "MyBool" }, - Name{ "MyBoolNext" } + Name{ "MyInt" }, + Name{ "MyIntRenamed" } })); Data::Asset testMaterialTypeAssetV2; @@ -249,12 +252,33 @@ namespace UnitTest materialTypeCreator.SetVersion(versionUpdate.GetVersion()); materialTypeCreator.AddVersionUpdate(versionUpdate.GetVersion(), versionUpdate); materialTypeCreator.AddShader(shaderAsset); - AddMaterialPropertyForSrg(materialTypeCreator, Name{ "MyBoolNext" }, MaterialPropertyDataType::Bool, Name{ "m_bool" }); - materialTypeCreator.SetPropertyValue(Name{ "MyBoolNext" }, true); + // Now we add the properties in a different order from before, and use the new name for MyInt. + AddMaterialPropertyForSrg(materialTypeCreator, Name{ "MyUInt" }, MaterialPropertyDataType::UInt, Name{ "m_uint" }); + AddMaterialPropertyForSrg(materialTypeCreator, Name{ "MyFloat" }, MaterialPropertyDataType::Float, Name{ "m_float" }); + AddMaterialPropertyForSrg(materialTypeCreator, Name{ "MyIntRenamed" }, MaterialPropertyDataType::Int, Name{ "m_int" }); EXPECT_TRUE(materialTypeCreator.End(testMaterialTypeAssetV2)); - // Upgrade the material asset with the materialTypeAsset v2 and verify - UpgradeAndValidateMaterialAsset(materialAsset, testMaterialTypeAssetV2); + // This is our way of faking the idea that an old version of the MaterialAsset could be loaded with a new version of the MaterialTypeAsset. + ReplaceMaterialType(materialAsset, testMaterialTypeAssetV2); + + // This can find errors and warnings, we are looking for a warning when the version update is applied + ErrorMessageFinder warningFinder; + warningFinder.AddExpectedErrorMessage("Automatic updates are available. Consider updating the .material source file"); + + // Even though this material was created using the old version of the material type, it's property values should get automatically + // updated to align with the new property layout in the latest MaterialTypeAsset. + MaterialPropertyIndex myIntIndex = materialAsset->GetMaterialPropertiesLayout()->FindPropertyIndex(Name{"MyIntRenamed"}); + EXPECT_EQ(2, myIntIndex.GetIndex()); + EXPECT_EQ(7, materialAsset->GetPropertyValues()[myIntIndex.GetIndex()].GetValue()); + + warningFinder.CheckExpectedErrorsFound(); + + // Since the MaterialAsset has already been updated, and the warning reported once, we should not see the "consider updating" + // warning reported again on subsequent property accesses. + warningFinder.Reset(); + myIntIndex = materialAsset->GetMaterialPropertiesLayout()->FindPropertyIndex(Name{"MyIntRenamed"}); + EXPECT_EQ(2, myIntIndex.GetIndex()); + EXPECT_EQ(7, materialAsset->GetPropertyValues()[myIntIndex.GetIndex()].GetValue()); } TEST_F(MaterialAssetTests, Error_NoBegin) diff --git a/Gems/Atom/RPI/Code/Tests/Material/MaterialSourceDataTests.cpp b/Gems/Atom/RPI/Code/Tests/Material/MaterialSourceDataTests.cpp index a6ce6504fb..6a6947e7c7 100644 --- a/Gems/Atom/RPI/Code/Tests/Material/MaterialSourceDataTests.cpp +++ b/Gems/Atom/RPI/Code/Tests/Material/MaterialSourceDataTests.cpp @@ -10,6 +10,7 @@ #include #include #include +#include #include #include @@ -725,7 +726,10 @@ namespace UnitTest EXPECT_EQ(1, 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"); 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. @@ -739,6 +743,10 @@ namespace UnitTest EXPECT_TRUE(AZ::Color(0.1f, 0.2f, 0.3f, 1.0f).IsClose(testColor, 0.01)); 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)