From 46061eb30212ffbf2793a97f73ac96690f32ecaf Mon Sep 17 00:00:00 2001 From: santorac <55155825+santorac@users.noreply.github.com> Date: Thu, 21 Oct 2021 00:07:36 -0700 Subject: [PATCH] Simplified the code around MaterialAsset::ApplyVersionUpdates() Signed-off-by: santorac <55155825+santorac@users.noreply.github.com> --- .../RPI.Reflect/Material/MaterialTypeAsset.h | 8 +- .../Material/MaterialTypeAssetCreator.h | 2 +- .../Material/MaterialVersionUpdate.h | 1 - .../Material/MaterialTypeSourceData.cpp | 4 +- .../RPI.Reflect/Material/MaterialAsset.cpp | 12 +-- .../Material/MaterialTypeAsset.cpp | 8 +- .../Material/MaterialTypeAssetCreator.cpp | 38 ++++++--- .../Material/MaterialVersionUpdate.cpp | 2 - .../Tests/Material/MaterialAssetTests.cpp | 2 +- .../Tests/Material/MaterialTypeAssetTests.cpp | 78 ++++++++++++++++++- 10 files changed, 119 insertions(+), 36 deletions(-) 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 d0f0609c02..9bc0e018d3 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 @@ -126,10 +126,8 @@ namespace AZ //! Returns the version of the MaterialTypeAsset. uint32_t GetVersion() const; - //! Returns the toVersion update containing the actions to perform. If a toVersion - //! is not defined in m_materialVersionUpdatesMap, a material toVersion with the specified - //! version but empty actions will be returned. - MaterialVersionUpdate GetMaterialVersionUpdate(uint32_t toVersion) const; + + const AZStd::vector& GetMaterialVersionUpdateList() const { return m_materialVersionUpdates; } private: bool PostLoadInit() override; @@ -175,7 +173,7 @@ namespace AZ uint32_t m_version = 1; //! Contains actions to perform for each material update version. - MaterialVersionUpdateMap m_materialVersionUpdateMap; + AZStd::vector m_materialVersionUpdates; }; class MaterialTypeAssetHandler : public AssetHandler diff --git a/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialTypeAssetCreator.h b/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialTypeAssetCreator.h index 64327ecd03..5e5f94da6d 100644 --- a/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialTypeAssetCreator.h +++ b/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialTypeAssetCreator.h @@ -41,7 +41,7 @@ namespace AZ //! Sets the version of the MaterialTypeAsset void SetVersion(uint32_t version); //! Adds a version update object into the MaterialTypeAsset - void AddVersionUpdate(uint32_t toVersion, const MaterialVersionUpdate& materialVersionUpdate); + void AddVersionUpdate(const MaterialVersionUpdate& materialVersionUpdate); //! Indicates that this MaterialType will own the specified shader option. //! Material-owned shader options can be connected to material properties (either directly or through functors). 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 7aede97301..0acd1d4432 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 @@ -56,6 +56,5 @@ namespace AZ Actions m_actions; }; - using MaterialVersionUpdateMap = AZStd::map; } // namespace RPI } // namespace AZ 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 31b34da092..8c8bad8c0c 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialTypeSourceData.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialTypeSourceData.cpp @@ -358,7 +358,7 @@ namespace AZ for (const auto& versionUpdate : m_versionUpdates) { - MaterialVersionUpdate materialVersionUpdate; + MaterialVersionUpdate materialVersionUpdate{versionUpdate.m_toVersion}; for (const auto& action : versionUpdate.m_actions) { if (action.m_operation == rename.GetStringView()) @@ -373,7 +373,7 @@ namespace AZ materialTypeAssetCreator.ReportWarning("Unsupported material version update operation '%s'", action.m_operation.c_str()); } } - materialTypeAssetCreator.AddVersionUpdate(versionUpdate.m_toVersion, materialVersionUpdate); + materialTypeAssetCreator.AddVersionUpdate(materialVersionUpdate); } } 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 aa188bead6..2b7c1c58c7 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialAsset.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialAsset.cpp @@ -210,13 +210,15 @@ namespace AZ bool changesWereApplied = false; - for (int i = 0; i < aznumeric_cast(m_materialTypeAsset->GetVersion() - m_materialTypeVersion); ++i) + for (const MaterialVersionUpdate& versionUpdate : m_materialTypeAsset->GetMaterialVersionUpdateList()) { - const auto& versionUpdate = m_materialTypeAsset->GetMaterialVersionUpdate(m_materialTypeVersion + i + 1); - - if (versionUpdate.ApplyVersionUpdates(*this)) + if (m_materialTypeVersion < versionUpdate.GetVersion()) { - changesWereApplied = true; + if (versionUpdate.ApplyVersionUpdates(*this)) + { + changesWereApplied = true; + m_materialTypeVersion = versionUpdate.GetVersion(); + } } } 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 a876028599..522ba74119 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialTypeAsset.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialTypeAsset.cpp @@ -47,7 +47,7 @@ namespace AZ serializeContext->Class() ->Version(5) // Material version update ->Field("Version", &MaterialTypeAsset::m_version) - ->Field("VersionUpdates", &MaterialTypeAsset::m_materialVersionUpdateMap) + ->Field("VersionUpdates", &MaterialTypeAsset::m_materialVersionUpdates) ->Field("ShaderCollection", &MaterialTypeAsset::m_shaderCollection) ->Field("MaterialFunctors", &MaterialTypeAsset::m_materialFunctors) ->Field("MaterialSrgShaderIndex", &MaterialTypeAsset::m_materialSrgShaderIndex) @@ -169,12 +169,6 @@ namespace AZ return m_version; } - MaterialVersionUpdate MaterialTypeAsset::GetMaterialVersionUpdate(uint32_t toVersion) const - { - const auto it = m_materialVersionUpdateMap.find(toVersion); - return it != m_materialVersionUpdateMap.end() ? it->second : MaterialVersionUpdate(toVersion); - } - void MaterialTypeAsset::SetReady() { m_status = AssetStatus::Ready; diff --git a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialTypeAssetCreator.cpp b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialTypeAssetCreator.cpp index 3839976004..46086dfecc 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialTypeAssetCreator.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialTypeAssetCreator.cpp @@ -102,18 +102,38 @@ namespace AZ bool MaterialTypeAssetCreator::ValidateMaterialVersion() { - AZStd::vector renamedPropertyNames; - const auto& materialVersionUpdate = m_asset->GetMaterialVersionUpdate(m_asset->m_version); - for (const auto& action : materialVersionUpdate.GetActions()) + if (m_asset->m_materialVersionUpdates.empty()) + { + return true; + } + + uint32_t prevVersion = 0; + for(const MaterialVersionUpdate& versionUpdate : m_asset->m_materialVersionUpdates) + { + if (versionUpdate.GetVersion() <= prevVersion) + { + ReportError("Version updates are not sequential. See version update '%u'.", versionUpdate.GetVersion()); + return false; + } + + if (versionUpdate.GetVersion() > m_asset->m_version) + { + ReportError("Version updates go beyond the current material type version. See version update '%u'.", versionUpdate.GetVersion()); + return false; + } + + prevVersion = versionUpdate.GetVersion(); + } + + const auto& lastMaterialVersionUpdate = m_asset->m_materialVersionUpdates.back(); + for (const auto& action : lastMaterialVersionUpdate.GetActions()) { const auto propertyIndex = m_asset->m_materialPropertiesLayout->FindPropertyIndex(AZ::Name{ action.m_toPropertyId }); if (!propertyIndex.IsValid()) { - ReportError(AZStd::string::format( - "Renamed property '%s' not found in material property layout. Check that the property name has been " + ReportError("Renamed property '%s' not found in material property layout. Check that the property name has been " "upgraded to the correct version", - action.m_toPropertyId.GetCStr()) - .c_str()); + action.m_toPropertyId.GetCStr()); return false; } @@ -150,9 +170,9 @@ namespace AZ m_asset->m_version = version; } - void MaterialTypeAssetCreator::AddVersionUpdate(uint32_t toVersion, const MaterialVersionUpdate& materialVersionUpdate) + void MaterialTypeAssetCreator::AddVersionUpdate(const MaterialVersionUpdate& materialVersionUpdate) { - m_asset->m_materialVersionUpdateMap[toVersion] = materialVersionUpdate; + m_asset->m_materialVersionUpdates.push_back(materialVersionUpdate); } void MaterialTypeAssetCreator::ClaimShaderOptionOwnership(const Name& shaderOptionName) 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 48ad095eba..bf503f3c67 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialVersionUpdate.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialVersionUpdate.cpp @@ -31,8 +31,6 @@ namespace AZ ->Field("ToVersion", &MaterialVersionUpdate::m_toVersion) ->Field("Actions", &MaterialVersionUpdate::m_actions) ; - - serializeContext->RegisterGenericType(); } } diff --git a/Gems/Atom/RPI/Code/Tests/Material/MaterialAssetTests.cpp b/Gems/Atom/RPI/Code/Tests/Material/MaterialAssetTests.cpp index d376ca06b7..cf8b55d581 100644 --- a/Gems/Atom/RPI/Code/Tests/Material/MaterialAssetTests.cpp +++ b/Gems/Atom/RPI/Code/Tests/Material/MaterialAssetTests.cpp @@ -250,7 +250,7 @@ namespace UnitTest Data::Asset testMaterialTypeAssetV2; materialTypeCreator.Begin(Uuid::CreateRandom()); materialTypeCreator.SetVersion(versionUpdate.GetVersion()); - materialTypeCreator.AddVersionUpdate(versionUpdate.GetVersion(), versionUpdate); + materialTypeCreator.AddVersionUpdate(versionUpdate); materialTypeCreator.AddShader(shaderAsset); // 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" }); diff --git a/Gems/Atom/RPI/Code/Tests/Material/MaterialTypeAssetTests.cpp b/Gems/Atom/RPI/Code/Tests/Material/MaterialTypeAssetTests.cpp index 7dd2734ee4..81b723ec04 100644 --- a/Gems/Atom/RPI/Code/Tests/Material/MaterialTypeAssetTests.cpp +++ b/Gems/Atom/RPI/Code/Tests/Material/MaterialTypeAssetTests.cpp @@ -162,7 +162,7 @@ namespace UnitTest Name{ "EnableSpecialPass" } })); materialTypeCreator.SetVersion(versionUpdate.GetVersion()); - materialTypeCreator.AddVersionUpdate(versionUpdate.GetVersion(), versionUpdate); + materialTypeCreator.AddVersionUpdate(versionUpdate); // Built-in shader @@ -501,7 +501,7 @@ namespace UnitTest }); } - TEST_F(MaterialTypeAssetTests, Error_InvalidMaterialVersionUpdate) + TEST_F(MaterialTypeAssetTests, Error_InvalidMaterialVersionUpdate_WrongName) { Data::Asset materialTypeAsset; @@ -518,7 +518,7 @@ namespace UnitTest Name{ "InvalidPropertyName" } })); materialTypeCreator.SetVersion(versionUpdate.GetVersion()); - materialTypeCreator.AddVersionUpdate(versionUpdate.GetVersion(), versionUpdate); + materialTypeCreator.AddVersionUpdate(versionUpdate); materialTypeCreator.AddShader(m_testShaderAsset); materialTypeCreator.BeginMaterialProperty(Name{ "EnableSpecialPass" }, MaterialPropertyDataType::Bool); @@ -529,6 +529,78 @@ namespace UnitTest AZ_TEST_STOP_ASSERTTEST(1); EXPECT_EQ(1, materialTypeCreator.GetErrorCount()); } + + TEST_F(MaterialTypeAssetTests, Error_InvalidMaterialVersionUpdate_WrongOrder) + { + MaterialTypeAssetCreator materialTypeCreator; + materialTypeCreator.Begin(Uuid::CreateRandom()); + + materialTypeCreator.SetVersion(4); + materialTypeCreator.AddShader(m_testShaderAsset); + materialTypeCreator.BeginMaterialProperty(Name{ "d" }, MaterialPropertyDataType::Bool); + materialTypeCreator.EndMaterialProperty(); + + ErrorMessageFinder errorMessageFinder; + errorMessageFinder.AddExpectedErrorMessage("Version updates are not sequential. See version update '3'"); + + { + MaterialVersionUpdate versionUpdate(2); + versionUpdate.AddAction(MaterialVersionUpdate::RenamePropertyAction({Name{ "a" },Name{ "b" }})); + materialTypeCreator.AddVersionUpdate(versionUpdate); + } + + { + MaterialVersionUpdate versionUpdate(4); + versionUpdate.AddAction(MaterialVersionUpdate::RenamePropertyAction({Name{ "b" },Name{ "c" }})); + materialTypeCreator.AddVersionUpdate(versionUpdate); + } + + { + MaterialVersionUpdate versionUpdate(3); + versionUpdate.AddAction(MaterialVersionUpdate::RenamePropertyAction({Name{ "c" },Name{ "d" }})); + materialTypeCreator.AddVersionUpdate(versionUpdate); + } + + Data::Asset materialTypeAsset; + EXPECT_FALSE(materialTypeCreator.End(materialTypeAsset)); + + errorMessageFinder.CheckExpectedErrorsFound(); + + EXPECT_EQ(1, materialTypeCreator.GetErrorCount()); + } + + TEST_F(MaterialTypeAssetTests, Error_InvalidMaterialVersionUpdate_GoesTooFar) + { + MaterialTypeAssetCreator materialTypeCreator; + materialTypeCreator.Begin(Uuid::CreateRandom()); + + materialTypeCreator.SetVersion(3); + materialTypeCreator.AddShader(m_testShaderAsset); + materialTypeCreator.BeginMaterialProperty(Name{ "d" }, MaterialPropertyDataType::Bool); + materialTypeCreator.EndMaterialProperty(); + + ErrorMessageFinder errorMessageFinder; + errorMessageFinder.AddExpectedErrorMessage("Version updates go beyond the current material type version. See version update '4'"); + + { + MaterialVersionUpdate versionUpdate(2); + versionUpdate.AddAction(MaterialVersionUpdate::RenamePropertyAction({Name{ "a" },Name{ "b" }})); + materialTypeCreator.AddVersionUpdate(versionUpdate); + } + + { + MaterialVersionUpdate versionUpdate(4); + versionUpdate.AddAction(MaterialVersionUpdate::RenamePropertyAction({Name{ "b" },Name{ "c" }})); + materialTypeCreator.AddVersionUpdate(versionUpdate); + } + + Data::Asset materialTypeAsset; + EXPECT_FALSE(materialTypeCreator.End(materialTypeAsset)); + + errorMessageFinder.CheckExpectedErrorsFound(); + + EXPECT_EQ(1, materialTypeCreator.GetErrorCount()); + } TEST_F(MaterialTypeAssetTests, MaterialTypeWithNoSRGOrProperties) {