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>
2021-10-20 15:53:22 -07:00
parent a0b1dec929
commit 836d018de7
6 changed files with 42 additions and 68 deletions
@@ -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<AZ::Name, AZ::Name> m_argsMap;
-
-                Action() = default;
-                Action(const AZ::Name& operation, const AZStd::initializer_list<AZStd::pair<AZ::Name, AZ::Name>>& 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<Action>;
+            using Actions = AZStd::vector<RenamePropertyAction>;
            const Actions& GetActions() const;
-            void AddAction(const Action& action);
+            void AddAction(const RenamePropertyAction& action);

        private:
            uint32_t m_toVersion;
@@ -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
                        {
@@ -105,27 +105,20 @@ namespace AZ
            AZStd::vector<AZ::Name> 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;
        }

@@ -18,10 +18,10 @@ namespace AZ
        {
            if (auto* serializeContext = azrtti_cast<SerializeContext*>(context))
            {
-                serializeContext->Class<MaterialVersionUpdate::Action>()
+                serializeContext->Class<MaterialVersionUpdate::RenamePropertyAction>()
                    ->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<MaterialVersionUpdate::Actions>();
@@ -53,23 +53,14 @@ namespace AZ

        void MaterialVersionUpdate::ApplyVersionUpdates(MaterialAsset& materialAsset) const
        {
-            // collect all renames within this version update
-            AZStd::unordered_map<AZ::Name, AZ::Name> 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<AZStd::pair<AZ::Name, AZ::Name>>& 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
@@ -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<MaterialTypeAsset> testMaterialTypeAssetV2;
        materialTypeCreator.Begin(Uuid::CreateRandom());
@@ -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);