From 836d018de75a88b2f44d444b7284b9c9640d47a4 Mon Sep 17 00:00:00 2001 From: santorac <55155825+santorac@users.noreply.github.com> Date: Wed, 20 Oct 2021 15:53:22 -0700 Subject: [PATCH] Simplified MaterialVersionUpdate code to focus just on renaming, since that's the only operation we currently support. We shouldn't add more complexity until additional operations need to be supported. Also rearranged some logic to simplify the code that loops over rename actions, avoiding making unnecessary additional maps. Signed-off-by: santorac <55155825+santorac@users.noreply.github.com> --- .../Material/MaterialVersionUpdate.h | 18 ++++---- .../Material/MaterialTypeSourceData.cpp | 10 ++--- .../Material/MaterialTypeAssetCreator.cpp | 17 +++----- .../Material/MaterialVersionUpdate.cpp | 41 ++++--------------- .../Tests/Material/MaterialAssetTests.cpp | 8 ++-- .../Tests/Material/MaterialTypeAssetTests.cpp | 16 +++++--- 6 files changed, 42 insertions(+), 68 deletions(-) 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 ee15ebfdcf..fd5a57fa8d 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 @@ -27,16 +27,14 @@ namespace AZ static void Reflect(ReflectContext* context); - struct Action + // At this time, the only supported operation is rename. If/when we add more actions in the future, + // we'll need to improve this, possibly with some virtual interface or union data. + struct RenamePropertyAction { - AZ_TYPE_INFO(AZ::RPI::MaterialVersionUpdate::Action, "{A1FBEB19-EA05-40F0-9700-57D048DF572B}"); + AZ_TYPE_INFO(AZ::RPI::MaterialVersionUpdate::RenameAction, "{A1FBEB19-EA05-40F0-9700-57D048DF572B}"); - AZ::Name m_operation; - AZStd::unordered_map m_argsMap; - - Action() = default; - Action(const AZ::Name& operation, const AZStd::initializer_list>& args); - void AddArg(const AZ::Name& key, const AZ::Name& argument); + AZ::Name m_fromPropertyId; + AZ::Name m_toPropertyId; }; explicit MaterialVersionUpdate() = default; @@ -47,9 +45,9 @@ namespace AZ void ApplyVersionUpdates(MaterialAsset& materialAsset) const; - using Actions = AZStd::vector; + using Actions = AZStd::vector; const Actions& GetActions() const; - void AddAction(const Action& action); + void AddAction(const RenamePropertyAction& action); private: uint32_t m_toVersion; 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 ea2e499972..31b34da092 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialTypeSourceData.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialTypeSourceData.cpp @@ -355,8 +355,7 @@ namespace AZ materialTypeAssetCreator.SetVersion(m_version); { const AZ::Name rename = AZ::Name{ "rename" }; - const AZ::Name from = AZ::Name{ "from" }; - const AZ::Name to = AZ::Name{ "to" }; + for (const auto& versionUpdate : m_versionUpdates) { MaterialVersionUpdate materialVersionUpdate; @@ -364,9 +363,10 @@ namespace AZ { if (action.m_operation == rename.GetStringView()) { - materialVersionUpdate.AddAction(MaterialVersionUpdate::Action(rename, { - { from, AZ::Name{ action.m_renameFrom } }, - { to, AZ::Name{ action.m_renameTo } } })); + materialVersionUpdate.AddAction(MaterialVersionUpdate::RenamePropertyAction{ + AZ::Name{ action.m_renameFrom }, + AZ::Name{ action.m_renameTo } + }); } else { 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 79336692ec..3839976004 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialTypeAssetCreator.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialTypeAssetCreator.cpp @@ -105,27 +105,20 @@ namespace AZ AZStd::vector renamedPropertyNames; const auto& materialVersionUpdate = m_asset->GetMaterialVersionUpdate(m_asset->m_version); for (const auto& action : materialVersionUpdate.GetActions()) - { - const auto it = action.m_argsMap.find(AZ::Name{ "to" }); - if (it != action.m_argsMap.end()) - { - renamedPropertyNames.push_back(it->second); - } - } - - for (const auto& propertyName : renamedPropertyNames) - { - const auto propertyIndex = m_asset->m_materialPropertiesLayout->FindPropertyIndex(AZ::Name{ propertyName }); + { + 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 " "upgraded to the correct version", - propertyName.GetCStr()) + action.m_toPropertyId.GetCStr()) .c_str()); return false; } + } + return true; } 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 faa571e403..de85d4230e 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialVersionUpdate.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialVersionUpdate.cpp @@ -18,10 +18,10 @@ namespace AZ { if (auto* serializeContext = azrtti_cast(context)) { - serializeContext->Class() + serializeContext->Class() ->Version(1) - ->Field("ArgsMap", &Action::m_argsMap) - ->Field("Operation", &Action::m_operation) + ->Field("From", &MaterialVersionUpdate::RenamePropertyAction::m_fromPropertyId) + ->Field("To", &MaterialVersionUpdate::RenamePropertyAction::m_toPropertyId) ; serializeContext->RegisterGenericType(); @@ -53,23 +53,14 @@ namespace AZ void MaterialVersionUpdate::ApplyVersionUpdates(MaterialAsset& materialAsset) const { - // collect all renames within this version update - AZStd::unordered_map renameToFrom; - for (const auto& action : m_actions) - { - const AZ::Name from = action.m_argsMap.find(AZ::Name{ "from" })->second; - const AZ::Name to = action.m_argsMap.find(AZ::Name{ "to" })->second; - - renameToFrom[from] = to; - } - - // apply rename actions for (auto& propertyName : materialAsset.m_propertyNames) { - const auto toFromIterator = renameToFrom.find(propertyName); - if (toFromIterator != renameToFrom.end()) + for (const auto& action : m_actions) { - propertyName = AZ::Name{ toFromIterator->second }; + if (propertyName == action.m_fromPropertyId) + { + propertyName = action.m_toPropertyId; + } } } } @@ -79,23 +70,9 @@ namespace AZ return m_actions; } - void MaterialVersionUpdate::AddAction(const Action& action) + void MaterialVersionUpdate::AddAction(const RenamePropertyAction& action) { m_actions.push_back(action); } - - MaterialVersionUpdate::Action::Action(const AZ::Name& operation, const AZStd::initializer_list>& args) - : m_operation(operation) - { - for (const auto& arg : args) - { - AddArg(arg.first, arg.second); - } - } - - void MaterialVersionUpdate::Action::AddArg(const AZ::Name& key, const AZ::Name& argument) - { - m_argsMap[key] = argument; - } } // namespace RPI } // namespace AZ diff --git a/Gems/Atom/RPI/Code/Tests/Material/MaterialAssetTests.cpp b/Gems/Atom/RPI/Code/Tests/Material/MaterialAssetTests.cpp index 3191797ded..df45e81ddf 100644 --- a/Gems/Atom/RPI/Code/Tests/Material/MaterialAssetTests.cpp +++ b/Gems/Atom/RPI/Code/Tests/Material/MaterialAssetTests.cpp @@ -238,9 +238,11 @@ namespace UnitTest // Prepare material type asset version 2 with the update actions MaterialVersionUpdate versionUpdate(2); - versionUpdate.AddAction(MaterialVersionUpdate::Action(AZ::Name{ "rename" }, { - { Name{ "from" }, Name{ "MyBool" } }, - { Name{ "to" }, Name{ "MyBoolNext" } } })); + versionUpdate.AddAction(MaterialVersionUpdate::RenamePropertyAction( + { + Name{ "MyBool" }, + Name{ "MyBoolNext" } + })); Data::Asset testMaterialTypeAssetV2; materialTypeCreator.Begin(Uuid::CreateRandom()); diff --git a/Gems/Atom/RPI/Code/Tests/Material/MaterialTypeAssetTests.cpp b/Gems/Atom/RPI/Code/Tests/Material/MaterialTypeAssetTests.cpp index 8341a8e382..7dd2734ee4 100644 --- a/Gems/Atom/RPI/Code/Tests/Material/MaterialTypeAssetTests.cpp +++ b/Gems/Atom/RPI/Code/Tests/Material/MaterialTypeAssetTests.cpp @@ -156,9 +156,11 @@ namespace UnitTest // Version updates MaterialVersionUpdate versionUpdate(2); - versionUpdate.AddAction(MaterialVersionUpdate::Action(AZ::Name{ "rename" }, { - { Name{ "from" }, Name{ "EnableSpecialPassPrev" } }, - { Name{ "to" }, Name{ "EnableSpecialPass" } } })); + versionUpdate.AddAction(MaterialVersionUpdate::RenamePropertyAction( + { + Name{ "EnableSpecialPassPrev" }, + Name{ "EnableSpecialPass" } + })); materialTypeCreator.SetVersion(versionUpdate.GetVersion()); materialTypeCreator.AddVersionUpdate(versionUpdate.GetVersion(), versionUpdate); @@ -510,9 +512,11 @@ namespace UnitTest // Invalid version updates MaterialVersionUpdate versionUpdate(2); - versionUpdate.AddAction(MaterialVersionUpdate::Action(AZ::Name{ "rename" }, { - { Name{ "from" }, Name{ "EnableSpecialPassPrev" } }, - { Name{ "to" }, Name{ "InvalidPropertyName" } } })); + versionUpdate.AddAction(MaterialVersionUpdate::RenamePropertyAction( + { + Name{ "EnableSpecialPassPrev" }, + Name{ "InvalidPropertyName" } + })); materialTypeCreator.SetVersion(versionUpdate.GetVersion()); materialTypeCreator.AddVersionUpdate(versionUpdate.GetVersion(), versionUpdate); materialTypeCreator.AddShader(m_testShaderAsset);