From 59031d2f17aeba28f3a01b8384ddc51a7e0c5496 Mon Sep 17 00:00:00 2001 From: rbarrand Date: Tue, 21 Sep 2021 15:35:59 -0700 Subject: [PATCH 01/15] Enable material version updates. Signed-off-by: Robin --- .../Material/MaterialTypeSourceData.h | 28 ++++++++ .../Atom/RPI.Reflect/Material/MaterialAsset.h | 9 +++ .../RPI.Reflect/Material/MaterialTypeAsset.h | 14 ++++ .../Material/MaterialTypeAssetCreator.h | 6 ++ .../Material/MaterialVersionUpdate.h | 48 ++++++++++++++ .../Material/MaterialTypeSourceData.cpp | 35 +++++++++- .../RPI.Reflect/Material/MaterialAsset.cpp | 64 ++++++++++++++++++- .../Material/MaterialAssetCreator.cpp | 2 + .../Material/MaterialTypeAsset.cpp | 16 ++++- .../Material/MaterialTypeAssetCreator.cpp | 41 +++++++++++- .../Material/MaterialVersionUpdate.cpp | 57 +++++++++++++++++ .../RPI/Code/atom_rpi_reflect_files.cmake | 2 + 12 files changed, 316 insertions(+), 6 deletions(-) create mode 100644 Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialVersionUpdate.h create mode 100644 Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialVersionUpdate.cpp diff --git a/Gems/Atom/RPI/Code/Include/Atom/RPI.Edit/Material/MaterialTypeSourceData.h b/Gems/Atom/RPI/Code/Include/Atom/RPI.Edit/Material/MaterialTypeSourceData.h index 7019289466..cca7d41e91 100644 --- a/Gems/Atom/RPI/Code/Include/Atom/RPI.Edit/Material/MaterialTypeSourceData.h +++ b/Gems/Atom/RPI/Code/Include/Atom/RPI.Edit/Material/MaterialTypeSourceData.h @@ -119,6 +119,29 @@ namespace AZ using PropertyList = AZStd::vector; + struct VersionUpdatesRenameOperationDefinition + { + AZ_TYPE_INFO(AZ::RPI::MaterialTypeSourceData::VersionUpdatesRenameOperationDefinition, "{F2295489-E15A-46CC-929F-8D42DEDBCF14}"); + + AZStd::string m_operation; + + AZStd::string m_renameFrom; + AZStd::string m_renameTo; + }; + + // TODO: Support script operations. We will only be supporting rename for now. + using VersionUpdateActions = AZStd::vector; + + struct VersionUpdateDefinition + { + AZ_TYPE_INFO(AZ::RPI::MaterialTypeSourceData::VersionUpdateDefinition, "{2C9D3B91-0585-4BC9-91D2-4CF0C71BC4B7}"); + + uint32_t m_toVersion; + VersionUpdateActions m_actions; + }; + + using VersionUpdates = AZStd::vector; + struct PropertyLayout { AZ_TYPE_INFO(AZ::RPI::MaterialTypeSourceData::PropertyLayout, "{AE53CF3F-5C3B-44F5-B2FB-306F0EB06393}"); @@ -135,6 +158,11 @@ namespace AZ AZStd::string m_description; + //! Version 1 is the default and should not contain any version update. + uint32_t m_version = 1; + + VersionUpdates m_versionUpdates; + PropertyLayout m_propertyLayout; //! A list of shader variants that are always used at runtime; they cannot be turned off diff --git a/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialAsset.h b/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialAsset.h index 6ca3bca652..2d42cc13ea 100644 --- a/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialAsset.h +++ b/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialAsset.h @@ -119,6 +119,11 @@ namespace AZ //! from m_materialTypeAsset. void RealignPropertyValuesAndNames(); + //! Renames properties in m_propertyNames based on the MaterialTypeAsset's version update. + void RenamePropertyNames(); + template + void RenamePropertyNames(iteratorType start, iteratorType end); + //! Called by asset creators to assign the asset to a ready state. void SetReady(); @@ -143,6 +148,10 @@ namespace AZ //! If empty, this implies that m_propertyValues is aligned with the entries in m_materialPropertiesLayout. AZStd::vector m_propertyNames; + //! The materialTypeVersion this materialAsset was based of. If the versions do not match at runtime when a + //! materialTypeAsset is loaded, an update will be performed on m_propertyNames if populated. + uint32_t m_materialTypeVersion = 1; + //! A flag to determine if m_propertyValues needs to be aligned with MaterialPropertiesLayout. Set to true whenever //! m_materialTypeAsset is reinitializing. bool m_isDirty = true; 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 e705a8041b..ca7de621a4 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 @@ -18,6 +18,7 @@ #include #include #include +#include namespace AZ { @@ -123,6 +124,13 @@ namespace AZ //! Returns a map from the UV shader inputs to a custom name. MaterialUvNameMap GetUvNameMap() const; + //! Returns the version of the MaterialTypeAsset. + uint32_t GetVersion() const; + //! Returns the version update containing the actions to perform. If a version + //! is not defined in m_materialVersionUpdatesMap, a material version with the specified + //! version but empty actions will be returned. + const MaterialVersionUpdate GetMaterialVersionUpdate(uint32_t version) const; + private: bool PostLoadInit() override; @@ -162,6 +170,12 @@ namespace AZ //! Index in @m_shaderCollection of the shader asset that contains the ObjectSrg. uint32_t m_objectSrgShaderIndex = InvalidShaderIndex; + //! The version of this MaterialTypeAsset. If the version is greater than 1, actions performed + //! to update this MaterialTypeAsset will be in m_materialVersionUpdateMap + uint32_t m_version = 1; + + //! Contains actions to perform for each material update version. + MaterialVersionUpdateMap m_materialVersionUpdateMap; }; 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 70488245a2..953b201960 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 @@ -38,6 +38,11 @@ namespace AZ void AddShader(const AZ::Data::Asset& shaderAsset, const ShaderVariantId& shaderVaraintId = ShaderVariantId{}, const AZ::Name& shaderTag = Uuid::CreateRandom().ToString()); void AddShader(const AZ::Data::Asset& shaderAsset, const AZ::Name& shaderTag); + //! Sets the version of the MaterialTypeAsset + void SetVersion(uint32_t version); + //! Adds a version update object into the MaterialTypeAsset + void AddVersionUpdate(uint32_t toVersion, 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). //! They cannot be accessed externally (for example, through the Material::SetSystemShaderOption() function). @@ -112,6 +117,7 @@ namespace AZ //! Saves the per-material SRG layout in m_shaderResourceGroupLayout for easier access void CacheMaterialSrgLayout(); + bool ValidateMaterialVersion(); bool ValidateBeginMaterialProperty(); bool ValidateEndMaterialProperty(); 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 new file mode 100644 index 0000000000..9147ec8d82 --- /dev/null +++ b/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialVersionUpdate.h @@ -0,0 +1,48 @@ +/* + * Copyright (c) Contributors to the Open 3D Engine Project. + * For complete copyright and license terms please see the LICENSE at the root of this distribution. + * + * SPDX-License-Identifier: Apache-2.0 OR MIT + * + */ +#pragma once + +#include +#include +#include +#include + +namespace AZ +{ + namespace RPI + { + struct MaterialVersionUpdate + { + AZ_TYPE_INFO(AZ::RPI::MaterialVersionUpdate, "{B36E7712-AED8-46AA-AFE0-01F8F884C44A}"); + + static void Reflect(ReflectContext* context); + + struct Action + { + AZ_TYPE_INFO(AZ::RPI::MaterialVersionUpdate::Action, "{A1FBEB19-EA05-40F0-9700-57D048DF572B}"); + + AZStd::string m_operation; + AZStd::map m_argsMap; + + Action() = default; + Action(const AZStd::string& operation, const AZStd::initializer_list>& args); + void AddArgs(const AZStd::string& key, const AZStd::string& argument); + }; + + MaterialVersionUpdate() = default; + MaterialVersionUpdate(uint32_t toVersion); + + uint32_t m_toVersion; + + using Actions = AZStd::vector; + 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 5a95cb35a6..c8ce26d4e7 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialTypeSourceData.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialTypeSourceData.cpp @@ -14,6 +14,7 @@ #include #include #include +#include #include #include #include @@ -58,6 +59,23 @@ namespace AZ serializeContext->RegisterGenericType(); + serializeContext->Class() + ->Version(1) + ->Field("op", &VersionUpdatesRenameOperationDefinition::m_operation) + ->Field("from", &VersionUpdatesRenameOperationDefinition::m_renameFrom) + ->Field("to", &VersionUpdatesRenameOperationDefinition::m_renameTo) + ; + + serializeContext->RegisterGenericType(); + + serializeContext->Class() + ->Version(1) + ->Field("toVersion", &VersionUpdateDefinition::m_toVersion) + ->Field("actions", &VersionUpdateDefinition::m_actions) + ; + + serializeContext->RegisterGenericType(); + serializeContext->Class() ->Version(1) ->Field("id", &GroupDefinition::m_nameId) @@ -86,8 +104,10 @@ namespace AZ serializeContext->RegisterGenericType(); serializeContext->Class() - ->Version(3) + ->Version(4) ->Field("description", &MaterialTypeSourceData::m_description) + ->Field("version", &MaterialTypeSourceData::m_version) + ->Field("versionUpdates", &MaterialTypeSourceData::m_versionUpdates) ->Field("propertyLayout", &MaterialTypeSourceData::m_propertyLayout) ->Field("shaders", &MaterialTypeSourceData::m_shaderCollection) ->Field("functors", &MaterialTypeSourceData::m_materialFunctorSourceData) @@ -290,6 +310,19 @@ namespace AZ materialTypeAssetCreator.SetElevateWarnings(elevateWarnings); materialTypeAssetCreator.Begin(assetId); + // Set materialtype version and add each version update object into MaterialTypeAsset. + materialTypeAssetCreator.SetVersion(m_version); + for (const auto& versionUpdate : m_versionUpdates) + { + MaterialVersionUpdate materialVersionUpdate; + for (const auto& action : versionUpdate.m_actions) + { + materialVersionUpdate.m_actions.push_back( + MaterialVersionUpdate::Action("rename", { { "from", action.m_renameFrom }, { "to", action.m_renameTo } })); + } + materialTypeAssetCreator.AddVersionUpdate(versionUpdate.m_toVersion, materialVersionUpdate); + } + // Used to gather all the UV streams used in this material type from its shaders in alphabetical order. auto semanticComp = [](const RHI::ShaderSemantic& lhs, const RHI::ShaderSemantic& rhs) -> bool { 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 c3e0221192..eefa259ed3 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialAsset.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialAsset.cpp @@ -9,6 +9,7 @@ #include #include #include +#include #include #include @@ -31,8 +32,9 @@ namespace AZ if (auto* serializeContext = azrtti_cast(context)) { serializeContext->Class() - ->Version(10) + ->Version(11) // Material version update ->Field("materialTypeAsset", &MaterialAsset::m_materialTypeAsset) + ->Field("materialTypeVersion", &MaterialAsset::m_materialTypeVersion) ->Field("propertyValues", &MaterialAsset::m_propertyValues) ->Field("propertyNames", &MaterialAsset::m_propertyNames) ; @@ -102,9 +104,19 @@ namespace AZ AZStd::array_view MaterialAsset::GetPropertyValues() const { - if (!m_propertyNames.empty() && m_isDirty) + if (!m_propertyNames.empty()) { - const_cast(this)->RealignPropertyValuesAndNames(); + const uint32_t materialTypeVersion = m_materialTypeAsset->GetVersion(); + if (m_materialTypeVersion != materialTypeVersion) + { + const_cast(this)->RenamePropertyNames(); + const_cast(m_materialTypeVersion) = materialTypeVersion; + } + + if (m_isDirty) + { + const_cast(this)->RealignPropertyValuesAndNames(); + } } return m_propertyValues; @@ -182,6 +194,52 @@ namespace AZ m_isDirty = false; } + template + void MaterialAsset::RenamePropertyNames(iteratorType start, iteratorType end) + { + for (iteratorType it = start; it != end; ++it) + { + for (auto& propertyName : m_propertyNames) + { + const auto toFromIterator = it->find(propertyName.GetCStr()); + if (toFromIterator != it->end()) + { + propertyName = AZ::Name{ toFromIterator->second }; + } + } + } + } + + void MaterialAsset::RenamePropertyNames() + { + // construct toFrom map in ascending order of version. + AZStd::vector> versionToFrom; + + const bool ascending = m_materialTypeVersion < m_materialTypeAsset->GetVersion(); + for (int i = 0; i < AZStd::abs(static_cast(m_materialTypeVersion - m_materialTypeAsset->GetVersion())); ++i) + { + const auto& versionUpdate = m_materialTypeAsset->GetMaterialVersionUpdate((m_materialTypeVersion < m_materialTypeAsset->GetVersion() ? m_materialTypeVersion : m_materialTypeAsset->GetVersion()) + i + 1); + + versionToFrom.push_back(); + for (const auto& action : versionUpdate.m_actions) + { + const AZStd::string from = ascending ? action.m_argsMap.find("from")->second : action.m_argsMap.find("to")->second; + const AZStd::string to = ascending ? action.m_argsMap.find("to")->second : action.m_argsMap.find("from")->second; + + versionToFrom[i][from] = to; + } + } + + if (ascending) + { + RenamePropertyNames(versionToFrom.cbegin(), versionToFrom.cend()); + } + else + { + RenamePropertyNames(versionToFrom.crbegin(), versionToFrom.crend()); + } + } + void MaterialAsset::ReinitializeMaterialTypeAsset(Data::Asset asset) { Data::Asset newMaterialTypeAsset = { asset.GetAs(), AZ::Data::AssetLoadBehavior::PreLoad }; diff --git a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialAssetCreator.cpp b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialAssetCreator.cpp index 79d19c15a2..b62a91a98d 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialAssetCreator.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialAssetCreator.cpp @@ -23,6 +23,7 @@ namespace AZ if (ValidateIsReady()) { m_asset->m_materialTypeAsset = parentMaterial.m_materialTypeAsset; + m_asset->m_materialTypeVersion = m_asset->m_materialTypeAsset->GetVersion(); if (!m_asset->m_materialTypeAsset) { @@ -69,6 +70,7 @@ namespace AZ ReportError("MaterialTypeAsset is null"); return; } + m_asset->m_materialTypeVersion = m_asset->m_materialTypeAsset->GetVersion(); m_materialPropertiesLayout = m_asset->GetMaterialPropertiesLayout(); if (includeMaterialPropertyNames) 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 913c3206fb..9209942f28 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialTypeAsset.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialTypeAsset.cpp @@ -37,6 +37,7 @@ namespace AZ void MaterialTypeAsset::Reflect(ReflectContext* context) { + MaterialVersionUpdate::Reflect(context); UvNamePair::Reflect(context); if (auto* serializeContext = azrtti_cast(context)) @@ -44,7 +45,9 @@ namespace AZ serializeContext->RegisterGenericType(); serializeContext->Class() - ->Version(4) // ATOM-15472 + ->Version(5) // Material version update + ->Field("Version", &MaterialTypeAsset::m_version) + ->Field("VersionUpdates", &MaterialTypeAsset::m_materialVersionUpdateMap) ->Field("ShaderCollection", &MaterialTypeAsset::m_shaderCollection) ->Field("MaterialFunctors", &MaterialTypeAsset::m_materialFunctors) ->Field("MaterialSrgShaderIndex", &MaterialTypeAsset::m_materialSrgShaderIndex) @@ -161,6 +164,17 @@ namespace AZ return m_uvNameMap; } + uint32_t MaterialTypeAsset::GetVersion() const + { + return m_version; + } + + const AZ::RPI::MaterialVersionUpdate MaterialTypeAsset::GetMaterialVersionUpdate(uint32_t version) const + { + const auto it = m_materialVersionUpdateMap.find(version); + return it != m_materialVersionUpdateMap.end() ? it->second : MaterialVersionUpdate(version); + } + 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 4405e835d6..f10ae42266 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialTypeAssetCreator.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialTypeAssetCreator.cpp @@ -38,7 +38,7 @@ namespace AZ bool MaterialTypeAssetCreator::End(Data::Asset& result) { - if (!ValidateIsReady() || !ValidateEndMaterialProperty()) + if (!ValidateIsReady() || !ValidateEndMaterialProperty() || !ValidateMaterialVersion()) { return false; } @@ -100,6 +100,35 @@ namespace AZ } } + bool MaterialTypeAssetCreator::ValidateMaterialVersion() + { + AZStd::vector renamedPropertieNames; + const auto& materialVersionUpdate = m_asset->GetMaterialVersionUpdate(m_asset->m_version); + for (const auto& action : materialVersionUpdate.m_actions) + { + const auto it = action.m_argsMap.find("to"); + if (it != action.m_argsMap.end()) + { + renamedPropertieNames.push_back(it->second); + } + } + + for (const auto& propertyName : renamedPropertieNames) + { + const auto propertyIndex = m_asset->m_materialPropertiesLayout->FindPropertyIndex(AZ::Name{ propertyName }); + 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.c_str()) + .c_str()); + return false; + } + } + return true; + } + void MaterialTypeAssetCreator::AddShader(const AZ::Data::Asset& shaderAsset, const ShaderVariantId& shaderVaraintId, const AZ::Name& shaderTag) { if (ValidateIsReady() && ValidateNotNull(shaderAsset, "ShaderAsset")) @@ -123,6 +152,16 @@ namespace AZ AddShader(shaderAsset, ShaderVariantId{}, shaderTag); } + void MaterialTypeAssetCreator::SetVersion(uint32_t version) + { + m_asset->m_version = version; + } + + void MaterialTypeAssetCreator::AddVersionUpdate(uint32_t toVersion, MaterialVersionUpdate materialVersionUpdate) + { + m_asset->m_materialVersionUpdateMap[toVersion] = materialVersionUpdate; + } + void MaterialTypeAssetCreator::ClaimShaderOptionOwnership(const Name& shaderOptionName) { bool optionFound = false; diff --git a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialVersionUpdate.cpp b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialVersionUpdate.cpp new file mode 100644 index 0000000000..fb2143d020 --- /dev/null +++ b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialVersionUpdate.cpp @@ -0,0 +1,57 @@ +/* + * Copyright (c) Contributors to the Open 3D Engine Project. + * For complete copyright and license terms please see the LICENSE at the root of this distribution. + * + * SPDX-License-Identifier: Apache-2.0 OR MIT + * + */ + +#include +#include + +namespace AZ +{ + namespace RPI + { + void MaterialVersionUpdate::Reflect(ReflectContext* context) + { + if (auto* serializeContext = azrtti_cast(context)) + { + serializeContext->Class() + ->Version(1) + ->Field("ArgsMap", &Action::m_argsMap) + ->Field("Operation", &Action::m_operation) + ; + + serializeContext->RegisterGenericType(); + + serializeContext->Class() + ->Version(1) + ->Field("ToVersion", &MaterialVersionUpdate::m_toVersion) + ->Field("Actions", &MaterialVersionUpdate::m_actions) + ; + + serializeContext->RegisterGenericType(); + } + } + + MaterialVersionUpdate::MaterialVersionUpdate(uint32_t toVersion) + : m_toVersion(toVersion) + { + } + + MaterialVersionUpdate::Action::Action(const AZStd::string& operation, const AZStd::initializer_list>& args) + : m_operation(operation) + { + for (const auto& arg : args) + { + AddArgs(arg.first, arg.second); + } + } + + void MaterialVersionUpdate::Action::AddArgs(const AZStd::string& key, const AZStd::string& argument) + { + m_argsMap[key] = argument; + } + } // namespace RPI +} // namespace AZ diff --git a/Gems/Atom/RPI/Code/atom_rpi_reflect_files.cmake b/Gems/Atom/RPI/Code/atom_rpi_reflect_files.cmake index 49c7231fed..4f0e432511 100644 --- a/Gems/Atom/RPI/Code/atom_rpi_reflect_files.cmake +++ b/Gems/Atom/RPI/Code/atom_rpi_reflect_files.cmake @@ -61,6 +61,7 @@ set(FILES Include/Atom/RPI.Reflect/Material/MaterialTypeAssetCreator.h Include/Atom/RPI.Reflect/Material/ShaderCollection.h Include/Atom/RPI.Reflect/Material/MaterialFunctor.h + Include/Atom/RPI.Reflect/Material/MaterialVersionUpdate.h Include/Atom/RPI.Reflect/Pass/ComputePassData.h Include/Atom/RPI.Reflect/Pass/CopyPassData.h Include/Atom/RPI.Reflect/Pass/DownsampleMipChainPassData.h @@ -141,6 +142,7 @@ set(FILES Source/RPI.Reflect/Material/MaterialTypeAssetCreator.cpp Source/RPI.Reflect/Material/ShaderCollection.cpp Source/RPI.Reflect/Material/MaterialFunctor.cpp + Source/RPI.Reflect/Material/MaterialVersionUpdate.cpp Source/RPI.Reflect/Pass/PassAsset.cpp Source/RPI.Reflect/Pass/PassAttachmentReflect.cpp Source/RPI.Reflect/Pass/PassRequest.cpp From 2607b3471aa38bfb92fd9a378b43fa96162ad24c Mon Sep 17 00:00:00 2001 From: Robin Date: Mon, 27 Sep 2021 23:30:41 -0700 Subject: [PATCH 02/15] Resolve PR comments. Add unit tests. Signed-off-by: Robin --- .../Material/MaterialTypeSourceData.h | 6 +-- .../Atom/RPI.Reflect/Material/MaterialAsset.h | 9 ++-- .../RPI.Reflect/Material/MaterialTypeAsset.h | 6 +-- .../Material/MaterialTypeAssetCreator.h | 2 +- .../Material/MaterialVersionUpdate.h | 29 +++++++--- .../Material/MaterialTypeSourceData.cpp | 8 +-- .../RPI.Reflect/Material/MaterialAsset.cpp | 53 ++++--------------- .../Material/MaterialTypeAsset.cpp | 6 +-- .../Material/MaterialTypeAssetCreator.cpp | 14 ++--- .../Material/MaterialVersionUpdate.cpp | 50 +++++++++++++++-- .../Tests/Material/MaterialAssetTests.cpp | 53 +++++++++++++++++++ .../Tests/Material/MaterialTypeAssetTests.cpp | 37 ++++++++++++- .../Material/MaterialTypeSourceDataTests.cpp | 19 +++++-- .../Code/Source/Document/MaterialDocument.cpp | 3 -- .../Material/EditorMaterialComponentUtil.cpp | 1 - 15 files changed, 208 insertions(+), 88 deletions(-) diff --git a/Gems/Atom/RPI/Code/Include/Atom/RPI.Edit/Material/MaterialTypeSourceData.h b/Gems/Atom/RPI/Code/Include/Atom/RPI.Edit/Material/MaterialTypeSourceData.h index cca7d41e91..6d7c9b556c 100644 --- a/Gems/Atom/RPI/Code/Include/Atom/RPI.Edit/Material/MaterialTypeSourceData.h +++ b/Gems/Atom/RPI/Code/Include/Atom/RPI.Edit/Material/MaterialTypeSourceData.h @@ -129,7 +129,8 @@ namespace AZ AZStd::string m_renameTo; }; - // TODO: Support script operations. We will only be supporting rename for now. + // TODO: Support script operations--At that point, we'll likely need to replace VersionUpdatesRenameOperationDefinition with a more generic + // data structure that has a custom JSON serialize. We will only be supporting rename for now. using VersionUpdateActions = AZStd::vector; struct VersionUpdateDefinition @@ -146,9 +147,6 @@ namespace AZ { AZ_TYPE_INFO(AZ::RPI::MaterialTypeSourceData::PropertyLayout, "{AE53CF3F-5C3B-44F5-B2FB-306F0EB06393}"); - //! Indicates the version of the set of available properties. Can be used to detect materials that might need to be updated. - uint32_t m_version = 0; - //! List of groups that will contain the available properties AZStd::vector m_groups; diff --git a/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialAsset.h b/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialAsset.h index 2d42cc13ea..e14ff30f87 100644 --- a/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialAsset.h +++ b/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialAsset.h @@ -22,6 +22,7 @@ namespace UnitTest { class MaterialTests; + class MaterialAssetTests; } namespace AZ @@ -42,10 +43,12 @@ namespace AZ , public MaterialReloadNotificationBus::Handler , public AssetInitBus::Handler { + friend class MaterialVersionUpdate; friend class MaterialAssetCreator; friend class MaterialAssetHandler; friend class MaterialAssetCreatorCommon; friend class UnitTest::MaterialTests; + friend class UnitTest::MaterialAssetTests; public: AZ_RTTI(MaterialAsset, "{522C7BE0-501D-463E-92C6-15184A2B7AD8}", AZ::Data::AssetData); @@ -119,10 +122,8 @@ namespace AZ //! from m_materialTypeAsset. void RealignPropertyValuesAndNames(); - //! Renames properties in m_propertyNames based on the MaterialTypeAsset's version update. - void RenamePropertyNames(); - template - void RenamePropertyNames(iteratorType start, iteratorType end); + //! Renames properties in m_propertyNames based on the MaterialTypeAsset's version update. Note that only version upgrades are supported. + void ApplyVersionUpdates(); //! Called by asset creators to assign the asset to a ready state. void SetReady(); 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 ca7de621a4..d0f0609c02 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,10 @@ namespace AZ //! Returns the version of the MaterialTypeAsset. uint32_t GetVersion() const; - //! Returns the version update containing the actions to perform. If a version - //! is not defined in m_materialVersionUpdatesMap, a material version with the specified + //! 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. - const MaterialVersionUpdate GetMaterialVersionUpdate(uint32_t version) const; + MaterialVersionUpdate GetMaterialVersionUpdate(uint32_t toVersion) const; private: bool PostLoadInit() override; 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 953b201960..64327ecd03 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, MaterialVersionUpdate materialVersionUpdate); + void AddVersionUpdate(uint32_t toVersion, 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 9147ec8d82..ee15ebfdcf 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 @@ -11,13 +11,18 @@ #include #include #include +#include namespace AZ { namespace RPI { - struct MaterialVersionUpdate + class MaterialAsset; + + // This class contains a toVersion and a list of actions to specify what operations were performed to upgrade a materialType. + class MaterialVersionUpdate { + public: AZ_TYPE_INFO(AZ::RPI::MaterialVersionUpdate, "{B36E7712-AED8-46AA-AFE0-01F8F884C44A}"); static void Reflect(ReflectContext* context); @@ -26,20 +31,28 @@ namespace AZ { AZ_TYPE_INFO(AZ::RPI::MaterialVersionUpdate::Action, "{A1FBEB19-EA05-40F0-9700-57D048DF572B}"); - AZStd::string m_operation; - AZStd::map m_argsMap; + AZ::Name m_operation; + AZStd::unordered_map m_argsMap; Action() = default; - Action(const AZStd::string& operation, const AZStd::initializer_list>& args); - void AddArgs(const AZStd::string& key, const AZStd::string& argument); + Action(const AZ::Name& operation, const AZStd::initializer_list>& args); + void AddArg(const AZ::Name& key, const AZ::Name& argument); }; - MaterialVersionUpdate() = default; - MaterialVersionUpdate(uint32_t toVersion); + explicit MaterialVersionUpdate() = default; + explicit MaterialVersionUpdate(uint32_t toVersion); - uint32_t m_toVersion; + uint32_t GetVersion() const; + void SetVersion(uint32_t toVersion); + + void ApplyVersionUpdates(MaterialAsset& materialAsset) const; using Actions = AZStd::vector; + const Actions& GetActions() const; + void AddAction(const Action& action); + + private: + uint32_t m_toVersion; Actions m_actions; }; 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 c8ce26d4e7..4d9d40cde2 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialTypeSourceData.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialTypeSourceData.cpp @@ -95,8 +95,7 @@ namespace AZ ; serializeContext->Class() - ->Version(1) - ->Field("version", &PropertyLayout::m_version) + ->Version(2) ->Field("groups", &PropertyLayout::m_groups) ->Field("properties", &PropertyLayout::m_properties) ; @@ -317,8 +316,9 @@ namespace AZ MaterialVersionUpdate materialVersionUpdate; for (const auto& action : versionUpdate.m_actions) { - materialVersionUpdate.m_actions.push_back( - MaterialVersionUpdate::Action("rename", { { "from", action.m_renameFrom }, { "to", action.m_renameTo } })); + materialVersionUpdate.AddAction(MaterialVersionUpdate::Action(AZ::Name{ "rename" }, { + { AZ::Name{ "from" }, AZ::Name{ action.m_renameFrom } }, + { AZ::Name{ "to" }, AZ::Name{ action.m_renameTo } } })); } materialTypeAssetCreator.AddVersionUpdate(versionUpdate.m_toVersion, 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 eefa259ed3..1695319a64 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialAsset.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialAsset.cpp @@ -6,6 +6,8 @@ * */ +#pragma optimize("", off) + #include #include #include @@ -107,10 +109,9 @@ namespace AZ if (!m_propertyNames.empty()) { const uint32_t materialTypeVersion = m_materialTypeAsset->GetVersion(); - if (m_materialTypeVersion != materialTypeVersion) + if (m_materialTypeVersion < materialTypeVersion) { - const_cast(this)->RenamePropertyNames(); - const_cast(m_materialTypeVersion) = materialTypeVersion; + const_cast(this)->ApplyVersionUpdates(); } if (m_isDirty) @@ -194,50 +195,16 @@ namespace AZ m_isDirty = false; } - template - void MaterialAsset::RenamePropertyNames(iteratorType start, iteratorType end) + void MaterialAsset::ApplyVersionUpdates() { - for (iteratorType it = start; it != end; ++it) + for (int i = 0; i < static_cast(m_materialTypeAsset->GetVersion() - m_materialTypeVersion); ++i) { - for (auto& propertyName : m_propertyNames) - { - const auto toFromIterator = it->find(propertyName.GetCStr()); - if (toFromIterator != it->end()) - { - propertyName = AZ::Name{ toFromIterator->second }; - } - } - } - } + const auto& versionUpdate = m_materialTypeAsset->GetMaterialVersionUpdate(m_materialTypeVersion + i + 1); - void MaterialAsset::RenamePropertyNames() - { - // construct toFrom map in ascending order of version. - AZStd::vector> versionToFrom; - - const bool ascending = m_materialTypeVersion < m_materialTypeAsset->GetVersion(); - for (int i = 0; i < AZStd::abs(static_cast(m_materialTypeVersion - m_materialTypeAsset->GetVersion())); ++i) - { - const auto& versionUpdate = m_materialTypeAsset->GetMaterialVersionUpdate((m_materialTypeVersion < m_materialTypeAsset->GetVersion() ? m_materialTypeVersion : m_materialTypeAsset->GetVersion()) + i + 1); - - versionToFrom.push_back(); - for (const auto& action : versionUpdate.m_actions) - { - const AZStd::string from = ascending ? action.m_argsMap.find("from")->second : action.m_argsMap.find("to")->second; - const AZStd::string to = ascending ? action.m_argsMap.find("to")->second : action.m_argsMap.find("from")->second; - - versionToFrom[i][from] = to; - } + versionUpdate.ApplyVersionUpdates(*this); } - if (ascending) - { - RenamePropertyNames(versionToFrom.cbegin(), versionToFrom.cend()); - } - else - { - RenamePropertyNames(versionToFrom.crbegin(), versionToFrom.crend()); - } + m_materialTypeVersion = m_materialTypeAsset->GetVersion(); } void MaterialAsset::ReinitializeMaterialTypeAsset(Data::Asset asset) @@ -287,3 +254,5 @@ namespace AZ } // namespace RPI } // namespace AZ + +#pragma optimize("", on) 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 9209942f28..a876028599 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialTypeAsset.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialTypeAsset.cpp @@ -169,10 +169,10 @@ namespace AZ return m_version; } - const AZ::RPI::MaterialVersionUpdate MaterialTypeAsset::GetMaterialVersionUpdate(uint32_t version) const + MaterialVersionUpdate MaterialTypeAsset::GetMaterialVersionUpdate(uint32_t toVersion) const { - const auto it = m_materialVersionUpdateMap.find(version); - return it != m_materialVersionUpdateMap.end() ? it->second : MaterialVersionUpdate(version); + const auto it = m_materialVersionUpdateMap.find(toVersion); + return it != m_materialVersionUpdateMap.end() ? it->second : MaterialVersionUpdate(toVersion); } void MaterialTypeAsset::SetReady() 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 f10ae42266..79336692ec 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,18 @@ namespace AZ bool MaterialTypeAssetCreator::ValidateMaterialVersion() { - AZStd::vector renamedPropertieNames; + AZStd::vector renamedPropertyNames; const auto& materialVersionUpdate = m_asset->GetMaterialVersionUpdate(m_asset->m_version); - for (const auto& action : materialVersionUpdate.m_actions) + for (const auto& action : materialVersionUpdate.GetActions()) { - const auto it = action.m_argsMap.find("to"); + const auto it = action.m_argsMap.find(AZ::Name{ "to" }); if (it != action.m_argsMap.end()) { - renamedPropertieNames.push_back(it->second); + renamedPropertyNames.push_back(it->second); } } - for (const auto& propertyName : renamedPropertieNames) + for (const auto& propertyName : renamedPropertyNames) { const auto propertyIndex = m_asset->m_materialPropertiesLayout->FindPropertyIndex(AZ::Name{ propertyName }); if (!propertyIndex.IsValid()) @@ -121,7 +121,7 @@ namespace AZ 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.c_str()) + propertyName.GetCStr()) .c_str()); return false; } @@ -157,7 +157,7 @@ namespace AZ m_asset->m_version = version; } - void MaterialTypeAssetCreator::AddVersionUpdate(uint32_t toVersion, MaterialVersionUpdate materialVersionUpdate) + void MaterialTypeAssetCreator::AddVersionUpdate(uint32_t toVersion, const MaterialVersionUpdate& materialVersionUpdate) { m_asset->m_materialVersionUpdateMap[toVersion] = materialVersionUpdate; } 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 fb2143d020..faa571e403 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialVersionUpdate.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialVersionUpdate.cpp @@ -7,6 +7,7 @@ */ #include +#include #include namespace AZ @@ -40,16 +41,59 @@ namespace AZ { } - MaterialVersionUpdate::Action::Action(const AZStd::string& operation, const AZStd::initializer_list>& args) + uint32_t MaterialVersionUpdate::GetVersion() const + { + return m_toVersion; + } + + void MaterialVersionUpdate::SetVersion(uint32_t toVersion) + { + m_toVersion = toVersion; + } + + 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()) + { + propertyName = AZ::Name{ toFromIterator->second }; + } + } + } + + const AZ::RPI::MaterialVersionUpdate::Actions& MaterialVersionUpdate::GetActions() const + { + return m_actions; + } + + void MaterialVersionUpdate::AddAction(const Action& 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) { - AddArgs(arg.first, arg.second); + AddArg(arg.first, arg.second); } } - void MaterialVersionUpdate::Action::AddArgs(const AZStd::string& key, const AZStd::string& argument) + void MaterialVersionUpdate::Action::AddArg(const AZ::Name& key, const AZ::Name& argument) { m_argsMap[key] = argument; } diff --git a/Gems/Atom/RPI/Code/Tests/Material/MaterialAssetTests.cpp b/Gems/Atom/RPI/Code/Tests/Material/MaterialAssetTests.cpp index ce223ceb35..3191797ded 100644 --- a/Gems/Atom/RPI/Code/Tests/Material/MaterialAssetTests.cpp +++ b/Gems/Atom/RPI/Code/Tests/Material/MaterialAssetTests.cpp @@ -63,6 +63,17 @@ namespace UnitTest RPITestFixture::TearDown(); } + + void UpgradeAndValidateMaterialAsset(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]); + } }; TEST_F(MaterialAssetTests, Basic) @@ -202,6 +213,48 @@ namespace UnitTest EXPECT_EQ(serializedAsset->GetPropertyValues()[8].GetValue>(), streamingImageAsset); } + TEST_F(MaterialAssetTests, UpgradeMaterialAsset) + { + auto materialSrgLayout = CreateCommonTestMaterialSrgLayout(); + + auto shaderAsset = CreateTestShaderAsset(Uuid::CreateRandom(), materialSrgLayout); + + Data::Asset testMaterialTypeAssetV1; + MaterialTypeAssetCreator materialTypeCreator; + materialTypeCreator.Begin(Uuid::CreateRandom()); + materialTypeCreator.AddShader(shaderAsset); + AddMaterialPropertyForSrg(materialTypeCreator, Name{ "MyBool" }, MaterialPropertyDataType::Bool, Name{ "m_bool" }); + materialTypeCreator.SetPropertyValue(Name{ "MyBool" }, true); + 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); + Data::Asset materialAsset; + EXPECT_TRUE(creator.End(materialAsset)); + + // 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" } } })); + + Data::Asset testMaterialTypeAssetV2; + materialTypeCreator.Begin(Uuid::CreateRandom()); + 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); + EXPECT_TRUE(materialTypeCreator.End(testMaterialTypeAssetV2)); + + // Upgrade the material asset with the materialTypeAsset v2 and verify + UpgradeAndValidateMaterialAsset(materialAsset, testMaterialTypeAssetV2); + } + TEST_F(MaterialAssetTests, Error_NoBegin) { Data::AssetId assetId(Uuid::CreateRandom()); diff --git a/Gems/Atom/RPI/Code/Tests/Material/MaterialTypeAssetTests.cpp b/Gems/Atom/RPI/Code/Tests/Material/MaterialTypeAssetTests.cpp index b9774c84d9..6329ef8d48 100644 --- a/Gems/Atom/RPI/Code/Tests/Material/MaterialTypeAssetTests.cpp +++ b/Gems/Atom/RPI/Code/Tests/Material/MaterialTypeAssetTests.cpp @@ -14,6 +14,7 @@ #include #include +#include #include #include #include @@ -153,6 +154,14 @@ namespace UnitTest MaterialTypeAssetCreator materialTypeCreator; materialTypeCreator.Begin(assetId); + // Version updates + MaterialVersionUpdate versionUpdate(2); + versionUpdate.AddAction(MaterialVersionUpdate::Action(AZ::Name{ "rename" }, { + { Name{ "from" }, Name{ "EnableSpecialPassPrev" } }, + { Name{ "to" }, Name{ "EnableSpecialPass" } } })); + materialTypeCreator.SetVersion(versionUpdate.GetVersion()); + materialTypeCreator.AddVersionUpdate(versionUpdate.GetVersion(), versionUpdate); + // Built-in shader materialTypeCreator.AddShader(m_testShaderAsset); @@ -198,7 +207,7 @@ namespace UnitTest { EXPECT_EQ(m_testMaterialSrgLayout, materialTypeAsset->GetMaterialSrgLayout()); EXPECT_EQ(5, materialTypeAsset->GetMaterialPropertiesLayout()->GetPropertyCount()); - + //EXPECT_EQ(2, materialTypeAsset->GetVersion()); // Check aliased properties const MaterialPropertyIndex colorIndex = materialTypeAsset->GetMaterialPropertiesLayout()->FindPropertyIndex(Name{ "MyColor" }); @@ -490,6 +499,32 @@ namespace UnitTest }); } + TEST_F(MaterialTypeAssetTests, Error_InvalidMaterialVersionUpdate) + { + Data::Asset materialTypeAsset; + + Data::AssetId assetId(Uuid::CreateRandom()); + + MaterialTypeAssetCreator materialTypeCreator; + materialTypeCreator.Begin(assetId); + + // Invalid version updates + MaterialVersionUpdate versionUpdate(2); + versionUpdate.AddAction(MaterialVersionUpdate::Action(AZ::Name{ "rename" }, { + { Name{ "from" }, Name{ "EnableSpecialPassPrev" } }, + { Name{ "to" }, Name{ "InvalidPropertyName" } } })); + materialTypeCreator.SetVersion(versionUpdate.GetVersion()); + materialTypeCreator.AddVersionUpdate(versionUpdate.GetVersion(), versionUpdate); + materialTypeCreator.AddShader(m_testShaderAsset); + + materialTypeCreator.BeginMaterialProperty(Name{ "EnableSpecialPass" }, MaterialPropertyDataType::Bool); + materialTypeCreator.EndMaterialProperty(); + + AZ_TEST_START_ASSERTTEST; + EXPECT_FALSE(materialTypeCreator.End(materialTypeAsset)); + AZ_TEST_STOP_ASSERTTEST(1); + EXPECT_EQ(1, materialTypeCreator.GetErrorCount()); + } TEST_F(MaterialTypeAssetTests, MaterialTypeWithNoSRGOrProperties) { diff --git a/Gems/Atom/RPI/Code/Tests/Material/MaterialTypeSourceDataTests.cpp b/Gems/Atom/RPI/Code/Tests/Material/MaterialTypeSourceDataTests.cpp index 179dc7c966..7766c1f8d8 100644 --- a/Gems/Atom/RPI/Code/Tests/Material/MaterialTypeSourceDataTests.cpp +++ b/Gems/Atom/RPI/Code/Tests/Material/MaterialTypeSourceDataTests.cpp @@ -978,8 +978,16 @@ namespace UnitTest const AZStd::string inputJson = R"( { "description": "This is a general description about the material", + "version": 2, + "versionUpdates": [ + { + "toVersion": 2, + "actions": [ + { "op": "rename", "from": "groupA.fooPrev", "to": "groupA.foo" } + ] + } + ], "propertyLayout": { - "version": 2, "groups": [ { "id": "groupA", @@ -1061,9 +1069,12 @@ namespace UnitTest JsonTestResult loadResult = LoadTestDataFromJson(material, inputJson); EXPECT_EQ(material.m_description, "This is a general description about the material"); - - EXPECT_EQ(material.m_propertyLayout.m_version, 2); - + EXPECT_EQ(material.m_version, 2); + EXPECT_EQ(material.m_versionUpdates.size(), 1); + EXPECT_EQ(material.m_versionUpdates[0].m_toVersion, 2); + EXPECT_EQ(material.m_versionUpdates[0].m_actions[0].m_operation, "rename"); + EXPECT_EQ(material.m_versionUpdates[0].m_actions[0].m_renameFrom, "groupA.fooPrev"); + EXPECT_EQ(material.m_versionUpdates[0].m_actions[0].m_renameTo, "groupA.foo"); EXPECT_EQ(material.m_propertyLayout.m_groups.size(), 2); EXPECT_TRUE(material.FindGroup("groupA") != nullptr); EXPECT_TRUE(material.FindGroup("groupB") != nullptr); diff --git a/Gems/Atom/Tools/MaterialEditor/Code/Source/Document/MaterialDocument.cpp b/Gems/Atom/Tools/MaterialEditor/Code/Source/Document/MaterialDocument.cpp index d032f35e38..ab8f9ff1a4 100644 --- a/Gems/Atom/Tools/MaterialEditor/Code/Source/Document/MaterialDocument.cpp +++ b/Gems/Atom/Tools/MaterialEditor/Code/Source/Document/MaterialDocument.cpp @@ -231,7 +231,6 @@ namespace MaterialEditor // create source data from properties MaterialSourceData sourceData; - sourceData.m_propertyLayoutVersion = m_materialTypeSourceData.m_propertyLayout.m_version; sourceData.m_materialType = m_materialSourceData.m_materialType; sourceData.m_parentMaterial = m_materialSourceData.m_parentMaterial; @@ -303,7 +302,6 @@ namespace MaterialEditor // create source data from properties MaterialSourceData sourceData; - sourceData.m_propertyLayoutVersion = m_materialTypeSourceData.m_propertyLayout.m_version; sourceData.m_materialType = m_materialSourceData.m_materialType; sourceData.m_parentMaterial = m_materialSourceData.m_parentMaterial; @@ -374,7 +372,6 @@ namespace MaterialEditor // create source data from properties MaterialSourceData sourceData; - sourceData.m_propertyLayoutVersion = m_materialTypeSourceData.m_propertyLayout.m_version; sourceData.m_materialType = m_materialSourceData.m_materialType; // Only assign a parent path if the source was a .material diff --git a/Gems/AtomLyIntegration/CommonFeatures/Code/Source/Material/EditorMaterialComponentUtil.cpp b/Gems/AtomLyIntegration/CommonFeatures/Code/Source/Material/EditorMaterialComponentUtil.cpp index e7059ccdf7..65ddef47e5 100644 --- a/Gems/AtomLyIntegration/CommonFeatures/Code/Source/Material/EditorMaterialComponentUtil.cpp +++ b/Gems/AtomLyIntegration/CommonFeatures/Code/Source/Material/EditorMaterialComponentUtil.cpp @@ -99,7 +99,6 @@ namespace AZ { // Construct the material source data object that will be exported AZ::RPI::MaterialSourceData exportData; - exportData.m_propertyLayoutVersion = editData.m_materialTypeSourceData.m_propertyLayout.m_version; // Converting absolute material paths to relative paths bool result = false; From 503e565a5313c238d5f51ce0e75ee72d3f1dc48e Mon Sep 17 00:00:00 2001 From: Robin Date: Mon, 27 Sep 2021 23:37:58 -0700 Subject: [PATCH 03/15] Remove pragma optimize off. Signed-off-by: Robin --- .../RPI/Code/Source/RPI.Reflect/Material/MaterialAsset.cpp | 4 ---- 1 file changed, 4 deletions(-) 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 1695319a64..c838ab2794 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialAsset.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialAsset.cpp @@ -6,8 +6,6 @@ * */ -#pragma optimize("", off) - #include #include #include @@ -254,5 +252,3 @@ namespace AZ } // namespace RPI } // namespace AZ - -#pragma optimize("", on) From ca06e2e82d19b2b08f2a163ea8fd4ae4bd4ab792 Mon Sep 17 00:00:00 2001 From: Robin Date: Mon, 27 Sep 2021 23:44:13 -0700 Subject: [PATCH 04/15] Update reflection version markers. Signed-off-by: Robin --- .../Code/Source/RPI.Edit/Material/MaterialTypeSourceData.cpp | 4 ++-- Gems/Atom/RPI/Code/Tests/Material/MaterialTypeAssetTests.cpp | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) 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 4d9d40cde2..48b2c43435 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialTypeSourceData.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialTypeSourceData.cpp @@ -95,7 +95,7 @@ namespace AZ ; serializeContext->Class() - ->Version(2) + ->Version(2) // Material Version Update ->Field("groups", &PropertyLayout::m_groups) ->Field("properties", &PropertyLayout::m_properties) ; @@ -103,7 +103,7 @@ namespace AZ serializeContext->RegisterGenericType(); serializeContext->Class() - ->Version(4) + ->Version(4) // Material Version Update ->Field("description", &MaterialTypeSourceData::m_description) ->Field("version", &MaterialTypeSourceData::m_version) ->Field("versionUpdates", &MaterialTypeSourceData::m_versionUpdates) diff --git a/Gems/Atom/RPI/Code/Tests/Material/MaterialTypeAssetTests.cpp b/Gems/Atom/RPI/Code/Tests/Material/MaterialTypeAssetTests.cpp index 6329ef8d48..8341a8e382 100644 --- a/Gems/Atom/RPI/Code/Tests/Material/MaterialTypeAssetTests.cpp +++ b/Gems/Atom/RPI/Code/Tests/Material/MaterialTypeAssetTests.cpp @@ -207,7 +207,7 @@ namespace UnitTest { EXPECT_EQ(m_testMaterialSrgLayout, materialTypeAsset->GetMaterialSrgLayout()); EXPECT_EQ(5, materialTypeAsset->GetMaterialPropertiesLayout()->GetPropertyCount()); - //EXPECT_EQ(2, materialTypeAsset->GetVersion()); + EXPECT_EQ(2, materialTypeAsset->GetVersion()); // Check aliased properties const MaterialPropertyIndex colorIndex = materialTypeAsset->GetMaterialPropertiesLayout()->FindPropertyIndex(Name{ "MyColor" }); From acff45446a068bcce2ba88cb711b1b9da3a0ae65 Mon Sep 17 00:00:00 2001 From: Robin Date: Wed, 29 Sep 2021 10:04:49 -0700 Subject: [PATCH 05/15] Resolve PR comments. Signed-off-by: Robin --- .../Material/MaterialTypeSourceData.cpp | 19 ++++++++++++------- .../RPI.Reflect/Material/MaterialAsset.cpp | 2 +- 2 files changed, 13 insertions(+), 8 deletions(-) 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 48b2c43435..6fe4fa3412 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialTypeSourceData.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialTypeSourceData.cpp @@ -311,16 +311,21 @@ namespace AZ // Set materialtype version and add each version update object into MaterialTypeAsset. materialTypeAssetCreator.SetVersion(m_version); - for (const auto& versionUpdate : m_versionUpdates) { - MaterialVersionUpdate materialVersionUpdate; - for (const auto& action : versionUpdate.m_actions) + 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.AddAction(MaterialVersionUpdate::Action(AZ::Name{ "rename" }, { - { AZ::Name{ "from" }, AZ::Name{ action.m_renameFrom } }, - { AZ::Name{ "to" }, AZ::Name{ action.m_renameTo } } })); + MaterialVersionUpdate materialVersionUpdate; + for (const auto& action : versionUpdate.m_actions) + { + materialVersionUpdate.AddAction(MaterialVersionUpdate::Action(rename, { + { from, AZ::Name{ action.m_renameFrom } }, + { to, AZ::Name{ action.m_renameTo } } })); + } + materialTypeAssetCreator.AddVersionUpdate(versionUpdate.m_toVersion, materialVersionUpdate); } - materialTypeAssetCreator.AddVersionUpdate(versionUpdate.m_toVersion, materialVersionUpdate); } // Used to gather all the UV streams used in this material type from its shaders in alphabetical order. 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 c838ab2794..cc460f4234 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialAsset.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialAsset.cpp @@ -195,7 +195,7 @@ namespace AZ void MaterialAsset::ApplyVersionUpdates() { - for (int i = 0; i < static_cast(m_materialTypeAsset->GetVersion() - m_materialTypeVersion); ++i) + for (int i = 0; i < aznumeric_cast(m_materialTypeAsset->GetVersion() - m_materialTypeVersion); ++i) { const auto& versionUpdate = m_materialTypeAsset->GetMaterialVersionUpdate(m_materialTypeVersion + i + 1); From a0b1dec929e336b43aadaabb99d0e95ccabf7bfc Mon Sep 17 00:00:00 2001 From: santorac <55155825+santorac@users.noreply.github.com> Date: Wed, 20 Oct 2021 15:31:01 -0700 Subject: [PATCH 06/15] Added support for material version updates in MaterialSourceData. This is necessary for tools like Material Editor and Asset Processor to work with the latest property names. - Added MaterialSourceData::ApplyVersionUpdates() for updating the properties. This should be called by tools after loading the MaterialSourceData. (But can be omitted if a tool wants to read the data exactly as it appears in the .material file). - Updated MaterialTypeSourceData::FindProperty to support applying version update renames, including a ApplyPropertyRenames utility function, which are necessary for MaterialSourceData to be able to find the necessary property definitons while loading. - Added a new context struct to JsonMaterialPropertyValueSerializer for passing down the material type version number, to help with applying property renames. - Renamed the .material file format "propertyLayoutVersion" to "materialTypeVersion" which is more accurate. This shouldn't hurt existing data as this field wasn't actually used for anything before. - Updated Material Editor to again store the material type version number in .material files. MaterialSourceDataTests updates... - Updated to include both a .materialtype file and a MaterialTypeAsset for the test material type. Both are used by the MaterialTypeSourceData class. - The default test material type now includes some version update steps; these are only used for version update tests and won't impact the other test functions. - Updated the path for storing temp files to disk, to just be in a "temp" folder in the exe path. (Originally they were saved to the gem folder near MaterialSourceDataTests.cpp, but at some point someone changed it to be under the exe folder, so there's no reason to use the full gem path anymore). MaterialTypeSourceDataTests updates... - Moved some code that was accidentally added to LoadAllFieldsUsingOldFormat but should have been in LoadAndStoreJson_AllFields. - Added test cases for unsupported version update operations Signed-off-by: santorac <55155825+santorac@users.noreply.github.com> --- Code/Framework/AzCore/AzCore/Utils/Utils.cpp | 2 +- .../MaterialPropertyValueSerializer.h | 7 + .../RPI.Edit/Material/MaterialSourceData.h | 6 +- .../Material/MaterialTypeSourceData.h | 13 +- .../Atom/RPI.Reflect/Material/MaterialAsset.h | 3 +- .../RPI.Builders/Material/MaterialBuilder.cpp | 5 + .../MaterialPropertyValueSerializer.cpp | 4 +- .../RPI.Edit/Material/MaterialSourceData.cpp | 39 +++ .../Material/MaterialSourceDataSerializer.cpp | 9 +- .../Material/MaterialTypeSourceData.cpp | 73 +++++- .../RPI/Code/Tests/Common/AssetSystemStub.cpp | 10 +- .../Material/MaterialSourceDataTests.cpp | 244 +++++++++++++++--- .../Material/MaterialTypeSourceDataTests.cpp | 177 ++++++++++++- .../Code/Source/Document/MaterialDocument.cpp | 9 + 14 files changed, 535 insertions(+), 66 deletions(-) diff --git a/Code/Framework/AzCore/AzCore/Utils/Utils.cpp b/Code/Framework/AzCore/AzCore/Utils/Utils.cpp index 2031d14d08..e6bfd78806 100644 --- a/Code/Framework/AzCore/AzCore/Utils/Utils.cpp +++ b/Code/Framework/AzCore/AzCore/Utils/Utils.cpp @@ -120,7 +120,7 @@ namespace AZ::Utils AZ::Outcome WriteFile(AZStd::string_view content, AZStd::string_view filePath) { AZ::IO::FixedMaxPath filePathFixed = filePath; // Because FileIOStream requires a null-terminated string - AZ::IO::FileIOStream stream(filePathFixed.c_str(), AZ::IO::OpenMode::ModeWrite); + AZ::IO::FileIOStream stream(filePathFixed.c_str(), AZ::IO::OpenMode::ModeWrite | AZ::IO::OpenMode::ModeCreatePath); bool success = false; diff --git a/Gems/Atom/RPI/Code/Include/Atom/RPI.Edit/Material/MaterialPropertyValueSerializer.h b/Gems/Atom/RPI/Code/Include/Atom/RPI.Edit/Material/MaterialPropertyValueSerializer.h index befbb7c990..29371618cf 100644 --- a/Gems/Atom/RPI/Code/Include/Atom/RPI.Edit/Material/MaterialPropertyValueSerializer.h +++ b/Gems/Atom/RPI/Code/Include/Atom/RPI.Edit/Material/MaterialPropertyValueSerializer.h @@ -24,6 +24,13 @@ namespace AZ AZ_RTTI(AZ::RPI::JsonMaterialPropertyValueSerializer, "{A52B1ED8-C849-4269-9AA7-9D0814D2EC59}", BaseJsonSerializer); AZ_CLASS_ALLOCATOR_DECL; + //! A LoadContext object must be passed down to the serializer via JsonDeserializerContext::GetMetadata().Add(...) + struct LoadContext + { + AZ_TYPE_INFO(JsonMaterialPropertyValueSerializer::LoadContext, "{5E0A891A-27F6-4AD7-88A5-B9EA50F88B45}"); + uint32_t m_materialTypeVersion; //!< The version number from the .materialtype file + }; + JsonSerializationResult::Result Load(void* outputValue, const Uuid& outputValueTypeId, const rapidjson::Value& inputValue, JsonDeserializerContext& context) override; 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 02607a7954..deeace0d41 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 @@ -50,7 +50,7 @@ namespace AZ AZStd::string m_parentMaterial; //!< The immediate parent of this material - uint32_t m_propertyLayoutVersion = 0; //!< The version of the property layout, defined in the material type, which was used to configure this material + uint32_t m_materialTypeVersion = 0; //!< The version of the material type that was used to configure this material struct Property { @@ -64,6 +64,10 @@ namespace AZ PropertyGroupMap m_properties; + //! 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. + bool ApplyVersionUpdates(); + //! Creates a MaterialAsset from the MaterialSourceData content. //! @param assetId ID for the MaterialAsset //! @param materialSourceFilePath Indicates the path of the .material file that the MaterialSourceData represents. Used for resolving file-relative paths. diff --git a/Gems/Atom/RPI/Code/Include/Atom/RPI.Edit/Material/MaterialTypeSourceData.h b/Gems/Atom/RPI/Code/Include/Atom/RPI.Edit/Material/MaterialTypeSourceData.h index 44ac926ecf..99eb15db24 100644 --- a/Gems/Atom/RPI/Code/Include/Atom/RPI.Edit/Material/MaterialTypeSourceData.h +++ b/Gems/Atom/RPI/Code/Include/Atom/RPI.Edit/Material/MaterialTypeSourceData.h @@ -13,6 +13,7 @@ #include #include #include +#include namespace AZ { @@ -179,7 +180,12 @@ namespace AZ const GroupDefinition* FindGroup(AZStd::string_view groupName) const; - const PropertyDefinition* FindProperty(AZStd::string_view groupName, AZStd::string_view propertyName) const; + //! Searches for a specific property. + //! Note this function can find properties using old versions of the property name; in that case, + //! the name in the returned PropertyDefinition* will not match the @propertyName that was searched for. + //! @param materialTypeVersion indicates the version number of the property name being passed in. Only renames above this version number will be applied. + //! @return the requested property, or null if it could not be found + const PropertyDefinition* FindProperty(AZStd::string_view groupName, AZStd::string_view propertyName, uint32_t materialTypeVersion = 0) const; //! Construct a complete list of group definitions, including implicit groups, arranged in the same order as the source data //! Groups with the same name will be consolidated into a single entry @@ -205,6 +211,11 @@ namespace AZ bool ConvertPropertyValueToSourceDataFormat(const PropertyDefinition& propertyDefinition, MaterialPropertyValue& propertyValue) const; Outcome> CreateMaterialTypeAsset(Data::AssetId assetId, AZStd::string_view materialTypeSourceFilePath = "", bool elevateWarnings = true) const; + + //! Possibly renames @propertyId based on the material version update steps. + //! @param materialTypeVersion indicates the version number of the property name being passed in. Only renames above this version number will be applied. + //! @return true if the property was renamed + bool ApplyPropertyRenames(MaterialPropertyId& propertyId, uint32_t materialTypeVersion = 0) const; }; //! The wrapper class for derived material functors. diff --git a/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialAsset.h b/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialAsset.h index e14ff30f87..2a1de6debd 100644 --- a/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialAsset.h +++ b/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialAsset.h @@ -122,7 +122,8 @@ namespace AZ //! from m_materialTypeAsset. void RealignPropertyValuesAndNames(); - //! Renames properties in m_propertyNames based on the MaterialTypeAsset's version update. Note that only version upgrades are supported. + //! 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. void ApplyVersionUpdates(); //! Called by asset creators to assign the asset to a ready state. diff --git a/Gems/Atom/RPI/Code/Source/RPI.Builders/Material/MaterialBuilder.cpp b/Gems/Atom/RPI/Code/Source/RPI.Builders/Material/MaterialBuilder.cpp index 86ea8ab688..0680c5c428 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Builders/Material/MaterialBuilder.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Builders/Material/MaterialBuilder.cpp @@ -287,6 +287,11 @@ namespace AZ return {}; } + if (!material.GetValue().ApplyVersionUpdates()) + { + return {}; + } + auto materialAssetOutcome = material.GetValue().CreateMaterialAsset(Uuid::CreateRandom(), materialSourceFilePath, true); if (!materialAssetOutcome.IsSuccess()) { diff --git a/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialPropertyValueSerializer.cpp b/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialPropertyValueSerializer.cpp index 3b2d36451a..5e04365ffb 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialPropertyValueSerializer.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialPropertyValueSerializer.cpp @@ -62,6 +62,8 @@ namespace AZ return context.Report(JsonSerializationResult::Tasks::ReadField, JsonSerializationResult::Outcomes::Catastrophic, "Material type reference not found."); } + const JsonMaterialPropertyValueSerializer::LoadContext* loadContext = context.GetMetadata().Find(); + // Construct the full property name (groupName.propertyName) by parsing it from the JSON path string. size_t startPropertyName = context.GetPath().Get().rfind('/'); size_t startGroupName = context.GetPath().Get().rfind('/', startPropertyName-1); @@ -70,7 +72,7 @@ namespace AZ JSR::ResultCode result(JSR::Tasks::ReadField); - auto propertyDefinition = materialType->FindProperty(groupName, propertyName); + auto propertyDefinition = materialType->FindProperty(groupName, propertyName, loadContext->m_materialTypeVersion); if (!propertyDefinition) { AZStd::string message = AZStd::string::format("Property '%.*s.%.*s' not found in material type.", AZ_STRING_ARG(groupName), AZ_STRING_ARG(propertyName)); 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 f697f33a3f..e43095f639 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialSourceData.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialSourceData.cpp @@ -72,6 +72,45 @@ namespace AZ materialAssetCreator.SetPropertyValue(propertyId, entry.second); } } + + bool MaterialSourceData::ApplyVersionUpdates() + { + auto materialTypeSourceDataOutcome = MaterialUtils::LoadMaterialTypeSourceData(m_materialType); + if (!materialTypeSourceDataOutcome.IsSuccess()) + { + return false; + } + + MaterialTypeSourceData materialTypeSourceData = materialTypeSourceDataOutcome.TakeValue(); + + // Note that the only kind of property update currently supported is rename... + + for (auto& groupPair : m_properties) + { + PropertyMap& propertyMap = groupPair.second; + + PropertyMap newPropertyMap; + + for (auto& propertyPair : propertyMap) + { + MaterialPropertyId propertyId{groupPair.first, propertyPair.first}; + if (materialTypeSourceData.ApplyPropertyRenames(propertyId, m_materialTypeVersion)) + { + newPropertyMap[propertyId.GetPropertyName().GetStringView()] = propertyPair.second; + } + else + { + newPropertyMap[propertyPair.first] = propertyPair.second; + } + } + + propertyMap = newPropertyMap; + } + + m_materialTypeVersion = materialTypeSourceData.m_version; + + return true; + } Outcome > MaterialSourceData::CreateMaterialAsset(Data::AssetId assetId, AZStd::string_view materialSourceFilePath, bool elevateWarnings, bool includeMaterialPropertyNames) const { diff --git a/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialSourceDataSerializer.cpp b/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialSourceDataSerializer.cpp index 5e4af07aae..cdd434e894 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialSourceDataSerializer.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialSourceDataSerializer.cpp @@ -8,6 +8,7 @@ #include #include +#include #include #include #include @@ -47,7 +48,7 @@ namespace AZ result.Combine(ContinueLoadingFromJsonObjectField(&materialSourceData->m_description, azrtti_typeid(), inputValue, "description", context)); result.Combine(ContinueLoadingFromJsonObjectField(&materialSourceData->m_materialType, azrtti_typeid(), inputValue, "materialType", context)); result.Combine(ContinueLoadingFromJsonObjectField(&materialSourceData->m_parentMaterial, azrtti_typeid(), inputValue, "parentMaterial", context)); - result.Combine(ContinueLoadingFromJsonObjectField(&materialSourceData->m_propertyLayoutVersion, azrtti_typeid(), inputValue, "propertyLayoutVersion", context)); + result.Combine(ContinueLoadingFromJsonObjectField(&materialSourceData->m_materialTypeVersion, azrtti_typeid(), inputValue, "materialTypeVersion", context)); if (materialSourceData->m_materialType.empty()) { @@ -118,6 +119,10 @@ namespace AZ context.GetMetadata().Add(AZStd::move(materialTypeData)); + JsonMaterialPropertyValueSerializer::LoadContext materialPropertyValueLoadContext; + materialPropertyValueLoadContext.m_materialTypeVersion = materialSourceData->m_materialTypeVersion; + context.GetMetadata().Add(materialPropertyValueLoadContext); + result.Combine(ContinueLoadingFromJsonObjectField(&materialSourceData->m_properties, azrtti_typeid(), inputValue, "properties", context)); if (result.GetProcessing() == JsonSerializationResult::Processing::Completed) @@ -148,7 +153,7 @@ namespace AZ resultCode.Combine(ContinueStoringToJsonObjectField(outputValue, "description", &materialSourceData->m_description, nullptr, azrtti_typeid(), context)); resultCode.Combine(ContinueStoringToJsonObjectField(outputValue, "materialType", &materialSourceData->m_materialType, nullptr, azrtti_typeid(), context)); resultCode.Combine(ContinueStoringToJsonObjectField(outputValue, "parentMaterial", &materialSourceData->m_parentMaterial, nullptr, azrtti_typeid(), context)); - resultCode.Combine(ContinueStoringToJsonObjectField(outputValue, "propertyLayoutVersion", &materialSourceData->m_propertyLayoutVersion, nullptr, azrtti_typeid(), context)); + resultCode.Combine(ContinueStoringToJsonObjectField(outputValue, "materialTypeVersion", &materialSourceData->m_materialTypeVersion, nullptr, azrtti_typeid(), context)); resultCode.Combine(ContinueStoringToJsonObjectField(outputValue, "properties", &materialSourceData->m_properties, nullptr, azrtti_typeid(), context)); return context.Report(resultCode, "Processed material."); 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 5e2ceacafc..ea2e499972 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialTypeSourceData.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialTypeSourceData.cpp @@ -129,7 +129,38 @@ namespace AZ return nullptr; } - const MaterialTypeSourceData::PropertyDefinition* MaterialTypeSourceData::FindProperty(AZStd::string_view groupName, AZStd::string_view propertyName) const + bool MaterialTypeSourceData::ApplyPropertyRenames(MaterialPropertyId& propertyId, uint32_t materialTypeVersion) const + { + bool renamed = false; + + for (const VersionUpdateDefinition& versionUpdate : m_versionUpdates) + { + if (materialTypeVersion >= versionUpdate.m_toVersion) + { + continue; + } + + for (const VersionUpdatesRenameOperationDefinition& action : versionUpdate.m_actions) + { + if (action.m_operation == "rename") + { + if (action.m_renameFrom == propertyId.GetFullName().GetStringView()) + { + propertyId = MaterialPropertyId::Parse(action.m_renameTo); + renamed = true; + } + } + else + { + AZ_Warning("Material source data", false, "Unsupported material version update operation '%s'", action.m_operation.c_str()); + } + } + } + + return renamed; + } + + const MaterialTypeSourceData::PropertyDefinition* MaterialTypeSourceData::FindProperty(AZStd::string_view groupName, AZStd::string_view propertyName, uint32_t materialTypeVersion) const { auto groupIter = m_propertyLayout.m_properties.find(groupName); if (groupIter == m_propertyLayout.m_properties.end()) @@ -145,6 +176,27 @@ namespace AZ } } + // Property has not been found, try looking for renames in the version history + + MaterialPropertyId propertyId = MaterialPropertyId{groupName, propertyName}; + ApplyPropertyRenames(propertyId, materialTypeVersion); + + // Do the search again with the new names + + groupIter = m_propertyLayout.m_properties.find(propertyId.GetGroupName().GetStringView()); + if (groupIter == m_propertyLayout.m_properties.end()) + { + return nullptr; + } + + for (const PropertyDefinition& property : groupIter->second) + { + if (property.m_name == propertyId.GetPropertyName().GetStringView()) + { + return &property; + } + } + return nullptr; } @@ -302,17 +354,24 @@ namespace AZ // Set materialtype version and add each version update object into MaterialTypeAsset. 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" }; + 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; for (const auto& action : versionUpdate.m_actions) { - materialVersionUpdate.AddAction(MaterialVersionUpdate::Action(rename, { - { from, AZ::Name{ action.m_renameFrom } }, - { to, AZ::Name{ action.m_renameTo } } })); + if (action.m_operation == rename.GetStringView()) + { + materialVersionUpdate.AddAction(MaterialVersionUpdate::Action(rename, { + { from, AZ::Name{ action.m_renameFrom } }, + { to, AZ::Name{ action.m_renameTo } } })); + } + else + { + materialTypeAssetCreator.ReportWarning("Unsupported material version update operation '%s'", action.m_operation.c_str()); + } } materialTypeAssetCreator.AddVersionUpdate(versionUpdate.m_toVersion, materialVersionUpdate); } diff --git a/Gems/Atom/RPI/Code/Tests/Common/AssetSystemStub.cpp b/Gems/Atom/RPI/Code/Tests/Common/AssetSystemStub.cpp index a5511edea1..31a46e9a6f 100644 --- a/Gems/Atom/RPI/Code/Tests/Common/AssetSystemStub.cpp +++ b/Gems/Atom/RPI/Code/Tests/Common/AssetSystemStub.cpp @@ -7,6 +7,7 @@ */ #include +#include namespace UnitTest { @@ -36,12 +37,17 @@ namespace UnitTest // Because GetSourceInfoBySourcePath should always return 0 for the sub-id, since it's about the source file not product file. sourceInfo.m_assetInfo.m_assetId.m_subId = 0; - m_sourceInfoMap.emplace(sourcePath, sourceInfo); + AZStd::string normalizedSourcePath = sourcePath; + AzFramework::StringFunc::Path::Normalize(normalizedSourcePath); + m_sourceInfoMap.emplace(normalizedSourcePath, sourceInfo); } bool AssetSystemStub::GetSourceInfoBySourcePath(const char* sourcePath, AZ::Data::AssetInfo& assetInfo, AZStd::string& watchFolder) { - auto iter = m_sourceInfoMap.find(sourcePath); + AZStd::string normalizedSourcePath = sourcePath; + AzFramework::StringFunc::Path::Normalize(normalizedSourcePath); + + auto iter = m_sourceInfoMap.find(normalizedSourcePath); if (iter != m_sourceInfoMap.end()) { diff --git a/Gems/Atom/RPI/Code/Tests/Material/MaterialSourceDataTests.cpp b/Gems/Atom/RPI/Code/Tests/Material/MaterialSourceDataTests.cpp index dd3b3b2711..a6ce6504fb 100644 --- a/Gems/Atom/RPI/Code/Tests/Material/MaterialSourceDataTests.cpp +++ b/Gems/Atom/RPI/Code/Tests/Material/MaterialSourceDataTests.cpp @@ -17,6 +17,7 @@ #include #include #include +#include #include #include @@ -60,22 +61,72 @@ namespace UnitTest localFileIO->SetAlias("@exefolder@", rootPath); m_testMaterialSrgLayout = CreateCommonTestMaterialSrgLayout(); - m_testShaderAsset = CreateTestShaderAsset(Uuid::CreateRandom(), m_testMaterialSrgLayout); + m_assetSystemStub.RegisterSourceInfo("@exefolder@/Temp/test.shader", m_testShaderAsset.GetId()); - MaterialTypeAssetCreator materialTypeCreator; - materialTypeCreator.Begin(Uuid::CreateRandom()); - materialTypeCreator.AddShader(m_testShaderAsset); - AddCommonTestMaterialProperties(materialTypeCreator, "general."); - materialTypeCreator.End(m_testMaterialTypeAsset); + // The MaterialSourceData relies on both MaterialTypeSourceData and MaterialTypeAsset. We have to make sure the + // .materialtype file is present on disk, and that the MaterialTypeAsset is available through the asset database stub... + + const char* materialTypeJson = R"( + { + "version": 10, + "propertyLayout": { + "properties": { + "general": [ + {"name": "MyBool", "type": "bool"}, + {"name": "MyInt", "type": "Int"}, + {"name": "MyUInt", "type": "UInt"}, + {"name": "MyFloat", "type": "Float"}, + {"name": "MyFloat2", "type": "Vector2"}, + {"name": "MyFloat3", "type": "Vector3"}, + {"name": "MyFloat4", "type": "Vector4"}, + {"name": "MyColor", "type": "Color"}, + {"name": "MyImage", "type": "Image"}, + {"name": "MyEnum", "type": "Enum", "enumValues": ["Enum0", "Enum1", "Enum2"], "defaultValue": "Enum0"} + ] + } + }, + "shaders": [ + { + "file": "@exefolder@/Temp/test.shader" + } + ], + "versionUpdates": [ + { + "toVersion": 2, + "actions": [ + {"op": "rename", "from": "general.testColorNameA", "to": "general.testColorNameB"} + ] + }, + { + "toVersion": 4, + "actions": [ + {"op": "rename", "from": "general.testColorNameB", "to": "general.testColorNameC"} + ] + }, + { + "toVersion": 10, + "actions": [ + {"op": "rename", "from": "general.testColorNameC", "to": "general.MyColor"} + ] + } + ] + } + )"; + + AZ::Utils::WriteFile(materialTypeJson, "@exefolder@/Temp/test.materialtype"); + + MaterialTypeSourceData materialTypeSourceData; + LoadTestDataFromJson(materialTypeSourceData, materialTypeJson); + m_testMaterialTypeAsset = materialTypeSourceData.CreateMaterialTypeAsset(Uuid::CreateRandom()).TakeValue(); // Since this test doesn't actually instantiate a Material, it won't need to instantiate this ImageAsset, so all we // need is an asset reference with a valid ID. m_testImageAsset = Data::Asset{ Data::AssetId{Uuid::CreateRandom(), StreamingImageAsset::GetImageAssetSubId()}, azrtti_typeid() }; // Register the test assets with the AssetSystemStub so CreateMaterialAsset() can use AssetUtils. - m_assetSystemStub.RegisterSourceInfo("test.materialtype", m_testMaterialTypeAsset.GetId()); - m_assetSystemStub.RegisterSourceInfo("test.streamingimage", m_testImageAsset.GetId()); + m_assetSystemStub.RegisterSourceInfo("@exefolder@/Temp/test.materialtype", m_testMaterialTypeAsset.GetId()); + m_assetSystemStub.RegisterSourceInfo("@exefolder@/Temp/test.streamingimage", m_testImageAsset.GetId()); } void TearDown() override @@ -88,12 +139,12 @@ namespace UnitTest RPITestFixture::TearDown(); } }; - + void AddPropertyGroup(MaterialSourceData& material, AZStd::string_view groupName) { material.m_properties.insert(groupName); } - + void AddProperty(MaterialSourceData& material, AZStd::string_view groupName, AZStd::string_view propertyName, const MaterialPropertyValue& anyValue) { material.m_properties[groupName][propertyName].m_value = anyValue; @@ -103,7 +154,7 @@ namespace UnitTest { MaterialSourceData sourceData; - sourceData.m_materialType = "test.materialtype"; + sourceData.m_materialType = "@exefolder@/Temp/test.materialtype"; AddPropertyGroup(sourceData, "general"); AddProperty(sourceData, "general", "MyBool", true); AddProperty(sourceData, "general", "MyInt", -10); @@ -113,7 +164,7 @@ namespace UnitTest AddProperty(sourceData, "general", "MyFloat2", AZ::Vector2(2.1f, 2.2f)); AddProperty(sourceData, "general", "MyFloat3", AZ::Vector3(3.1f, 3.2f, 3.3f)); AddProperty(sourceData, "general", "MyFloat4", AZ::Vector4(4.1f, 4.2f, 4.3f, 4.4f)); - AddProperty(sourceData, "general", "MyImage", AZStd::string("test.streamingimage")); + AddProperty(sourceData, "general", "MyImage", AZStd::string("@exefolder@/Temp/test.streamingimage")); AddProperty(sourceData, "general", "MyEnum", AZStd::string("Enum1")); auto materialAssetOutcome = sourceData.CreateMaterialAsset(Uuid::CreateRandom(), "", true); @@ -139,7 +190,7 @@ namespace UnitTest EXPECT_STREQ(a.m_materialType.data(), b.m_materialType.data()); EXPECT_STREQ(a.m_description.data(), b.m_description.data()); EXPECT_STREQ(a.m_parentMaterial.data(), b.m_parentMaterial.data()); - EXPECT_EQ(a.m_propertyLayoutVersion, b.m_propertyLayoutVersion); + EXPECT_EQ(a.m_materialTypeVersion, b.m_materialTypeVersion); EXPECT_EQ(a.m_properties.size(), b.m_properties.size()); for (auto& groupA : a.m_properties) @@ -170,7 +221,7 @@ namespace UnitTest auto& propertyA = propertyIterA.second; auto& propertyB = propertyIterB->second; - + bool typesMatch = propertyA.m_value.GetTypeId() == propertyB.m_value.GetTypeId(); EXPECT_TRUE(typesMatch); if (typesMatch) @@ -229,8 +280,8 @@ namespace UnitTest " } \n" "} \n"; - const char* materialTypeFilePath = "@exefolder@/Gems/Atom/RPI/Code/Tests/Material/Temp/roundTripTest.materialtype"; - + const char* materialTypeFilePath = "@exefolder@/Temp/roundTripTest.materialtype"; + AZ::IO::FileIOStream file; EXPECT_TRUE(file.Open(materialTypeFilePath, AZ::IO::OpenMode::ModeWrite | AZ::IO::OpenMode::ModeCreatePath)); file.Write(strlen(materialTypeJson), materialTypeJson); @@ -240,7 +291,7 @@ namespace UnitTest sourceDataOriginal.m_materialType = materialTypeFilePath; sourceDataOriginal.m_parentMaterial = materialTypeFilePath; sourceDataOriginal.m_description = "This is a description"; - sourceDataOriginal.m_propertyLayoutVersion = 7; + sourceDataOriginal.m_materialTypeVersion = 7; AddPropertyGroup(sourceDataOriginal, "groupA"); AddProperty(sourceDataOriginal, "groupA", "MyBool", true); AddProperty(sourceDataOriginal, "groupA", "MyInt", -10); @@ -252,14 +303,14 @@ namespace UnitTest AddPropertyGroup(sourceDataOriginal, "groupC"); AddProperty(sourceDataOriginal, "groupC", "MyFloat4", AZ::Vector4(4.1f, 4.2f, 4.3f, 4.4f)); AddProperty(sourceDataOriginal, "groupC", "MyColor", AZ::Color{0.1f, 0.2f, 0.3f, 0.4f}); - AddProperty(sourceDataOriginal, "groupC", "MyImage", AZStd::string("test.streamingimage")); + AddProperty(sourceDataOriginal, "groupC", "MyImage", AZStd::string("@exefolder@/Temp/test.streamingimage")); AZStd::string sourceDataSerialized; JsonTestResult storeResult = StoreTestDataToJson(sourceDataOriginal, sourceDataSerialized); MaterialSourceData sourceDataCopy; JsonTestResult loadResult = LoadTestDataFromJson(sourceDataCopy, sourceDataSerialized); - + CheckEqual(sourceDataOriginal, sourceDataCopy); } @@ -277,10 +328,10 @@ namespace UnitTest ] } } - } + } )"; - const char* materialTypeFilePath = "@exefolder@/Gems/Atom/RPI/Code/Tests/Material/Temp/simpleMaterialType.materialtype"; + const char* materialTypeFilePath = "@exefolder@/Temp/simpleMaterialType.materialtype"; AZ::IO::FileIOStream file; EXPECT_TRUE(file.Open(materialTypeFilePath, AZ::IO::OpenMode::ModeWrite | AZ::IO::OpenMode::ModeCreatePath)); @@ -296,7 +347,7 @@ namespace UnitTest "testColor": [0.1,0.2,0.3] } }, - "materialType": "@exefolder@/Gems/Atom/RPI/Code/Tests/Material/Temp/simpleMaterialType.materialtype" + "materialType": "@exefolder@/Temp/simpleMaterialType.materialtype" } )"; @@ -330,7 +381,7 @@ namespace UnitTest { const AZStd::string inputJson = R"( { - "propertyLayoutVersion": 1, + "materialTypeVersion": 1, "properties": { "baseColor": { "color": [1.0,1.0,1.0] @@ -354,7 +405,7 @@ namespace UnitTest const AZStd::string inputJson = R"( { "materialType": "DoesNotExist.materialtype", - "propertyLayoutVersion": 1, + "materialTypeVersion": 1, "properties": { "baseColor": { "color": [1.0,1.0,1.0] @@ -387,10 +438,10 @@ namespace UnitTest ] } } - } + } )"; - const char* materialTypeFilePath = "@exefolder@/Gems/Atom/RPI/Code/Tests/Material/Temp/simpleMaterialType.materialtype"; + const char* materialTypeFilePath = "@exefolder@/Temp/simpleMaterialType.materialtype"; AZ::IO::FileIOStream file; EXPECT_TRUE(file.Open(materialTypeFilePath, AZ::IO::OpenMode::ModeWrite | AZ::IO::OpenMode::ModeCreatePath)); @@ -399,8 +450,8 @@ namespace UnitTest const AZStd::string inputJson = R"( { - "materialType": "@exefolder@/Gems/Atom/RPI/Code/Tests/Material/Temp/simpleMaterialType.materialtype", - "propertyLayoutVersion": 1, + "materialType": "@exefolder@/Temp/simpleMaterialType.materialtype", + "materialTypeVersion": 1, "properties": { "general": { "testColor": [1.0,1.0,1.0] @@ -433,10 +484,10 @@ namespace UnitTest ] } } - } + } )"; - const char* materialTypeFilePath = "@exefolder@/Gems/Atom/RPI/Code/Tests/Material/Temp/simpleMaterialType.materialtype"; + const char* materialTypeFilePath = "@exefolder@/Temp/simpleMaterialType.materialtype"; AZ::IO::FileIOStream file; EXPECT_TRUE(file.Open(materialTypeFilePath, AZ::IO::OpenMode::ModeWrite | AZ::IO::OpenMode::ModeCreatePath)); @@ -445,8 +496,8 @@ namespace UnitTest const AZStd::string inputJson = R"( { - "materialType": "@exefolder@/Gems/Atom/RPI/Code/Tests/Material/Temp/simpleMaterialType.materialtype", - "propertyLayoutVersion": 1, + "materialType": "@exefolder@/Temp/simpleMaterialType.materialtype", + "materialTypeVersion": 1, "properties": { "general": { "doesNotExist": [1.0,1.0,1.0] @@ -467,20 +518,20 @@ namespace UnitTest TEST_F(MaterialSourceDataTests, CreateMaterialAsset_MultiLevelDataInheritance) { MaterialSourceData sourceDataLevel1; - sourceDataLevel1.m_materialType = "test.materialtype"; + sourceDataLevel1.m_materialType = "@exefolder@/Temp/test.materialtype"; AddPropertyGroup(sourceDataLevel1, "general"); AddProperty(sourceDataLevel1, "general", "MyFloat", 1.5f); AddProperty(sourceDataLevel1, "general", "MyColor", AZ::Color{0.1f, 0.2f, 0.3f, 0.4f}); MaterialSourceData sourceDataLevel2; - sourceDataLevel2.m_materialType = "test.materialtype"; + sourceDataLevel2.m_materialType = "@exefolder@/Temp/test.materialtype"; sourceDataLevel2.m_parentMaterial = "level1.material"; AddPropertyGroup(sourceDataLevel2, "general"); AddProperty(sourceDataLevel2, "general", "MyColor", AZ::Color{0.15f, 0.25f, 0.35f, 0.45f}); AddProperty(sourceDataLevel2, "general", "MyFloat2", AZ::Vector2{4.1f, 4.2f}); MaterialSourceData sourceDataLevel3; - sourceDataLevel3.m_materialType = "test.materialtype"; + sourceDataLevel3.m_materialType = "@exefolder@/Temp/test.materialtype"; sourceDataLevel3.m_parentMaterial = "level2.material"; AddPropertyGroup(sourceDataLevel3, "general"); AddProperty(sourceDataLevel3, "general", "MyFloat", 3.5f); @@ -497,7 +548,7 @@ namespace UnitTest auto materialAssetLevel3 = sourceDataLevel3.CreateMaterialAsset(Uuid::CreateRandom(), "", true); EXPECT_TRUE(materialAssetLevel3.IsSuccess()); - + auto layout = m_testMaterialTypeAsset->GetMaterialPropertiesLayout(); MaterialPropertyIndex myFloat = layout->FindPropertyIndex(Name("general.MyFloat")); MaterialPropertyIndex myFloat2 = layout->FindPropertyIndex(Name("general.MyFloat2")); @@ -535,14 +586,14 @@ namespace UnitTest m_assetSystemStub.RegisterSourceInfo("otherBase.materialtype", otherMaterialType.GetId()); MaterialSourceData sourceDataLevel1; - sourceDataLevel1.m_materialType = "test.materialtype"; + sourceDataLevel1.m_materialType = "@exefolder@/Temp/test.materialtype"; MaterialSourceData sourceDataLevel2; - sourceDataLevel2.m_materialType = "test.materialtype"; + sourceDataLevel2.m_materialType = "@exefolder@/Temp/test.materialtype"; sourceDataLevel2.m_parentMaterial = "level1.material"; MaterialSourceData sourceDataLevel3; - sourceDataLevel3.m_materialType = "otherBase.materialtype"; + sourceDataLevel3.m_materialType = "@exefolder@/Temp/otherBase.materialtype"; sourceDataLevel3.m_parentMaterial = "level2.material"; auto materialAssetLevel1 = sourceDataLevel1.CreateMaterialAsset(Uuid::CreateRandom(), "", true); @@ -570,7 +621,7 @@ namespace UnitTest { MaterialSourceData sourceData; - sourceData.m_materialType = "test.materialtype"; + sourceData.m_materialType = "@exefolder@/Temp/test.materialtype"; AddPropertyGroup(sourceData, "general"); @@ -587,7 +638,7 @@ namespace UnitTest { MaterialSourceData sourceData; - sourceData.m_materialType = "test.materialtype"; + sourceData.m_materialType = "@exefolder@/Temp/test.materialtype"; AddPropertyGroup(sourceData, "general"); @@ -629,7 +680,7 @@ namespace UnitTest expectWarning([](MaterialSourceData& materialSourceData) { - AddProperty(materialSourceData, "general", "DoesNotExist", AZStd::string("test.streamingimage")); + AddProperty(materialSourceData, "general", "DoesNotExist", AZStd::string("@exefolder@/Temp/test.streamingimage")); }); // Missing image reference @@ -638,6 +689,115 @@ namespace UnitTest AddProperty(materialSourceData, "general", "MyImage", AZStd::string("doesNotExist.streamingimage")); }, 3); // Expect a 3rd error because AssetUtils reports its own assertion failure } + + + TEST_F(MaterialSourceDataTests, Load_MaterialTypeVersionUpdate) + { + const AZStd::string inputJson = R"( + { + "materialType": "@exefolder@/Temp/test.materialtype", + "materialTypeVersion": 1, + "properties": { + "general": { + "testColorNameA": [0.1, 0.2, 0.3] + } + } + } + )"; + + MaterialSourceData material; + JsonTestResult loadResult = LoadTestDataFromJson(material, inputJson); + + EXPECT_EQ(AZ::JsonSerializationResult::Tasks::ReadField, loadResult.m_jsonResultCode.GetTask()); + EXPECT_EQ(AZ::JsonSerializationResult::Processing::Completed, loadResult.m_jsonResultCode.GetProcessing()); + + // Initially, the loaded material data will match the .material file exactly. This gives us the accurate representation of + // what's actually saved on disk. + + EXPECT_NE(material.m_properties["general"].find("testColorNameA"), material.m_properties["general"].end()); + EXPECT_EQ(material.m_properties["general"].find("testColorNameB"), material.m_properties["general"].end()); + EXPECT_EQ(material.m_properties["general"].find("testColorNameC"), material.m_properties["general"].end()); + EXPECT_EQ(material.m_properties["general"].find("MyColor"), material.m_properties["general"].end()); + + AZ::Color testColor = material.m_properties["general"]["testColorNameA"].m_value.GetValue(); + EXPECT_TRUE(AZ::Color(0.1f, 0.2f, 0.3f, 1.0f).IsClose(testColor, 0.01)); + + EXPECT_EQ(1, material.m_materialTypeVersion); + + // Then we force the material data to update to the latest material type version specification + material.ApplyVersionUpdates(); + + // 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. + + EXPECT_EQ(material.m_properties["general"].find("testColorNameA"), material.m_properties["general"].end()); + EXPECT_EQ(material.m_properties["general"].find("testColorNameB"), material.m_properties["general"].end()); + EXPECT_EQ(material.m_properties["general"].find("testColorNameC"), material.m_properties["general"].end()); + EXPECT_NE(material.m_properties["general"].find("MyColor"), material.m_properties["general"].end()); + + testColor = material.m_properties["general"]["MyColor"].m_value.GetValue(); + EXPECT_TRUE(AZ::Color(0.1f, 0.2f, 0.3f, 1.0f).IsClose(testColor, 0.01)); + + EXPECT_EQ(10, material.m_materialTypeVersion); + } + + TEST_F(MaterialSourceDataTests, Load_MaterialTypeVersionPartialUpdate) + { + // This case is similar to Load_MaterialTypeVersionUpdate but we start at a later + // version so only some of the version updates are applied. + + const AZStd::string inputJson = R"( + { + "materialType": "@exefolder@/Temp/test.materialtype", + "materialTypeVersion": 3, + "properties": { + "general": { + "testColorNameB": [0.1, 0.2, 0.3] + } + } + } + )"; + + MaterialSourceData material; + JsonTestResult loadResult = LoadTestDataFromJson(material, inputJson); + + EXPECT_EQ(AZ::JsonSerializationResult::Tasks::ReadField, loadResult.m_jsonResultCode.GetTask()); + EXPECT_EQ(AZ::JsonSerializationResult::Processing::Completed, loadResult.m_jsonResultCode.GetProcessing()); + + material.ApplyVersionUpdates(); + + AZ::Color testColor = material.m_properties["general"]["MyColor"].m_value.GetValue(); + EXPECT_TRUE(AZ::Color(0.1f, 0.2f, 0.3f, 1.0f).IsClose(testColor, 0.01)); + + EXPECT_EQ(10, material.m_materialTypeVersion); + } + + TEST_F(MaterialSourceDataTests, Load_Error_MaterialTypeVersionUpdateWithMismatchedVersion) + { + const AZStd::string inputJson = R"( + { + "materialType": "@exefolder@/Temp/test.materialtype", + "materialTypeVersion": 3, // At this version, the property should be testColorNameB not testColorNameA + "properties": { + "general": { + "testColorNameA": [0.1, 0.2, 0.3] + } + } + } + )"; + + MaterialSourceData material; + JsonTestResult loadResult = LoadTestDataFromJson(material, inputJson); + + loadResult.ContainsMessage("/properties/general/testColorNameA", "Property 'general.testColorNameA' not found in material type."); + + EXPECT_FALSE(material.m_properties["general"]["testColorNameA"].m_value.IsValid()); + + material.ApplyVersionUpdates(); + + EXPECT_FALSE(material.m_properties["general"]["MyColor"].m_value.IsValid()); + } + } diff --git a/Gems/Atom/RPI/Code/Tests/Material/MaterialTypeSourceDataTests.cpp b/Gems/Atom/RPI/Code/Tests/Material/MaterialTypeSourceDataTests.cpp index 778fca5402..709d6b84ec 100644 --- a/Gems/Atom/RPI/Code/Tests/Material/MaterialTypeSourceDataTests.cpp +++ b/Gems/Atom/RPI/Code/Tests/Material/MaterialTypeSourceDataTests.cpp @@ -10,6 +10,7 @@ #include #include #include +#include #include #include @@ -1070,7 +1071,12 @@ namespace UnitTest EXPECT_EQ(material.m_description, "This is a general description about the material"); - EXPECT_EQ(material.m_propertyLayout.m_version, 2); + EXPECT_EQ(material.m_version, 2); + EXPECT_EQ(material.m_versionUpdates.size(), 1); + EXPECT_EQ(material.m_versionUpdates[0].m_toVersion, 2); + EXPECT_EQ(material.m_versionUpdates[0].m_actions[0].m_operation, "rename"); + EXPECT_EQ(material.m_versionUpdates[0].m_actions[0].m_renameFrom, "groupA.fooPrev"); + EXPECT_EQ(material.m_versionUpdates[0].m_actions[0].m_renameTo, "groupA.foo"); EXPECT_EQ(material.m_propertyLayout.m_groups.size(), 2); EXPECT_TRUE(material.FindGroup("groupA") != nullptr); @@ -1215,12 +1221,7 @@ namespace UnitTest JsonTestResult loadResult = LoadTestDataFromJson(material, inputJson); EXPECT_EQ(material.m_description, "This is a general description about the material"); - EXPECT_EQ(material.m_version, 2); - EXPECT_EQ(material.m_versionUpdates.size(), 1); - EXPECT_EQ(material.m_versionUpdates[0].m_toVersion, 2); - EXPECT_EQ(material.m_versionUpdates[0].m_actions[0].m_operation, "rename"); - EXPECT_EQ(material.m_versionUpdates[0].m_actions[0].m_renameFrom, "groupA.fooPrev"); - EXPECT_EQ(material.m_versionUpdates[0].m_actions[0].m_renameTo, "groupA.foo"); + EXPECT_EQ(material.m_propertyLayout.m_groups.size(), 2); EXPECT_TRUE(material.FindGroup("groupA") != nullptr); EXPECT_TRUE(material.FindGroup("groupB") != nullptr); @@ -1277,7 +1278,6 @@ namespace UnitTest { "description": "", "propertyLayout": { - "version": 2, "groups": [ { "name": "general", @@ -1316,4 +1316,165 @@ namespace UnitTest CheckPropertyValue>(materialTypeAsset, Name{ "general.absolute" }, m_testImageAsset2); CheckPropertyValue>(materialTypeAsset, Name{ "general.relative" }, m_testImageAsset2); } + + + TEST_F(MaterialTypeSourceDataTests, FindPropertyUsingOldName) + { + const AZStd::string inputJson = R"( + { + "version": 10, + "versionUpdates": [ + { + "toVersion": 2, + "actions": [ + { "op": "rename", "from": "general.fooA", "to": "general.fooB" } + ] + }, + { + "toVersion": 4, + "actions": [ + { "op": "rename", "from": "general.barA", "to": "general.barB" } + ] + }, + { + "toVersion": 6, + "actions": [ + { "op": "rename", "from": "general.fooB", "to": "general.fooC" }, + { "op": "rename", "from": "general.barB", "to": "general.barC" } + ] + }, + { + "toVersion": 7, + "actions": [ + { "op": "rename", "from": "general.bazA", "to": "otherGroup.bazB" } + ] + } + ], + "propertyLayout": { + "properties": { + "general": [ + { + "name": "fooC", + "type": "Bool" + }, + { + "name": "barC", + "type": "Float" + } + ], + "otherGroup": [ + { + "name": "dontMindMe", + "type": "Bool" + }, + { + "name": "bazB", + "type": "Float" + } + ] + } + } + } + )"; + + MaterialTypeSourceData materialType; + JsonTestResult loadResult = LoadTestDataFromJson(materialType, inputJson); + + EXPECT_EQ(materialType.m_version, 10); + + // First find the properties using their correct current names + const MaterialTypeSourceData::PropertyDefinition* foo = materialType.FindProperty("general", "fooC"); + const MaterialTypeSourceData::PropertyDefinition* bar = materialType.FindProperty("general", "barC"); + const MaterialTypeSourceData::PropertyDefinition* baz = materialType.FindProperty("otherGroup", "bazB"); + + EXPECT_TRUE(foo); + EXPECT_TRUE(bar); + EXPECT_TRUE(baz); + EXPECT_EQ(foo->m_name, "fooC"); + EXPECT_EQ(bar->m_name, "barC"); + EXPECT_EQ(baz->m_name, "bazB"); + + // Now try doing the property lookup using old versions of the name and make sure the same property can be found + + EXPECT_EQ(foo, materialType.FindProperty("general", "fooA")); + EXPECT_EQ(foo, materialType.FindProperty("general", "fooB")); + EXPECT_EQ(bar, materialType.FindProperty("general", "barA")); + EXPECT_EQ(bar, materialType.FindProperty("general", "barB")); + EXPECT_EQ(baz, materialType.FindProperty("general", "bazA")); + + EXPECT_EQ(nullptr, materialType.FindProperty("general", "fooX")); + EXPECT_EQ(nullptr, materialType.FindProperty("general", "barX")); + EXPECT_EQ(nullptr, materialType.FindProperty("general", "bazX")); + EXPECT_EQ(nullptr, materialType.FindProperty("general", "bazB")); + EXPECT_EQ(nullptr, materialType.FindProperty("otherGroup", "bazA")); + } + + TEST_F(MaterialTypeSourceDataTests, FindPropertyUsingOldName_Error_UnsupportedVersionUpdate) + { + const AZStd::string inputJson = R"( + { + "version": 10, + "versionUpdates": [ + { + "toVersion": 2, + "actions": [ + { "op": "notRename", "from": "general.fooA", "to": "general.fooB" } + ] + } + ], + "propertyLayout": { + "properties": { + "general": [ + { + "name": "fooB", + "type": "Bool" + } + ] + } + } + } + )"; + + MaterialTypeSourceData materialType; + JsonTestResult loadResult = LoadTestDataFromJson(materialType, inputJson); + + ErrorMessageFinder errorMessageFinder; + errorMessageFinder.AddExpectedErrorMessage("Unsupported material version update operation 'notRename'"); + + + const MaterialTypeSourceData::PropertyDefinition* foo = materialType.FindProperty("general", "fooA"); + + EXPECT_EQ(nullptr, foo); + + errorMessageFinder.CheckExpectedErrorsFound(); + } + + TEST_F(MaterialTypeSourceDataTests, CreateMaterialTypeAsset_Error_UnsupportedVersionUpdate) + { + MaterialTypeSourceData sourceData; + + MaterialTypeSourceData::PropertyDefinition propertySource; + propertySource.m_name = "a"; + propertySource.m_dataType = MaterialPropertyDataType::Int; + propertySource.m_value = 0; + sourceData.m_propertyLayout.m_properties["general"].push_back(propertySource); + + sourceData.m_version = 2; + + MaterialTypeSourceData::VersionUpdateDefinition versionUpdate; + versionUpdate.m_toVersion = 2; + MaterialTypeSourceData::VersionUpdatesRenameOperationDefinition updateAction; + updateAction.m_operation = "operationNotKnown"; + versionUpdate.m_actions.push_back(updateAction); + sourceData.m_versionUpdates.push_back(versionUpdate); + + ErrorMessageFinder errorMessageFinder; + errorMessageFinder.AddExpectedErrorMessage("Unsupported material version update operation 'operationNotKnown'"); + errorMessageFinder.AddIgnoredErrorMessage("Failed to build MaterialTypeAsset", true); + + auto materialTypeOutcome = sourceData.CreateMaterialTypeAsset(Uuid::CreateRandom()); + EXPECT_FALSE(materialTypeOutcome.IsSuccess()); + + errorMessageFinder.CheckExpectedErrorsFound(); + } } diff --git a/Gems/Atom/Tools/MaterialEditor/Code/Source/Document/MaterialDocument.cpp b/Gems/Atom/Tools/MaterialEditor/Code/Source/Document/MaterialDocument.cpp index 2aca8ce1f2..51164a70f4 100644 --- a/Gems/Atom/Tools/MaterialEditor/Code/Source/Document/MaterialDocument.cpp +++ b/Gems/Atom/Tools/MaterialEditor/Code/Source/Document/MaterialDocument.cpp @@ -232,6 +232,9 @@ namespace MaterialEditor MaterialSourceData sourceData; sourceData.m_materialType = m_materialSourceData.m_materialType; sourceData.m_parentMaterial = m_materialSourceData.m_parentMaterial; + + AZ_Assert(m_materialAsset && m_materialAsset->GetMaterialTypeAsset(), "When IsOpen() is true, these assets should not be null."); + sourceData.m_materialTypeVersion = m_materialAsset->GetMaterialTypeAsset()->GetVersion(); // Force save data to store forward slashes AzFramework::StringFunc::Replace(sourceData.m_materialType, "\\", "/"); @@ -303,6 +306,9 @@ namespace MaterialEditor MaterialSourceData sourceData; sourceData.m_materialType = m_materialSourceData.m_materialType; sourceData.m_parentMaterial = m_materialSourceData.m_parentMaterial; + + AZ_Assert(m_materialAsset && m_materialAsset->GetMaterialTypeAsset(), "When IsOpen() is true, these assets should not be null."); + sourceData.m_materialTypeVersion = m_materialAsset->GetMaterialTypeAsset()->GetVersion(); // Force save data to store forward slashes AzFramework::StringFunc::Replace(sourceData.m_materialType, "\\", "/"); @@ -372,6 +378,9 @@ namespace MaterialEditor // create source data from properties MaterialSourceData sourceData; sourceData.m_materialType = m_materialSourceData.m_materialType; + + AZ_Assert(m_materialAsset && m_materialAsset->GetMaterialTypeAsset(), "When IsOpen() is true, these assets should not be null."); + sourceData.m_materialTypeVersion = m_materialAsset->GetMaterialTypeAsset()->GetVersion(); // Only assign a parent path if the source was a .material if (AzFramework::StringFunc::Path::IsExtension(m_relativePath.c_str(), MaterialSourceData::Extension)) 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 07/15] 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); From 1633ced656f6cec6096103d29a46e7f6d4a444d7 Mon Sep 17 00:00:00 2001 From: santorac <55155825+santorac@users.noreply.github.com> Date: Wed, 20 Oct 2021 23:36:53 -0700 Subject: [PATCH 08/15] Reports warnings when a material version auto update is applied, notifying the user they should update their source data. Also improved the MaterialAssetTests UpgradeMaterialAsset() to focus on testing the inputs and outputs of the class rather than the private internal data. Signed-off-by: santorac <55155825+santorac@users.noreply.github.com> --- .../RPI.Edit/Material/MaterialSourceData.h | 1 + .../Material/MaterialVersionUpdate.h | 4 +- .../RPI.Edit/Material/MaterialSourceData.cpp | 20 ++++++- .../RPI.Reflect/Material/MaterialAsset.cpp | 27 ++++++++- .../Material/MaterialVersionUpdate.cpp | 7 ++- .../Tests/Material/MaterialAssetTests.cpp | 58 +++++++++++++------ .../Material/MaterialSourceDataTests.cpp | 8 +++ 7 files changed, 103 insertions(+), 22 deletions(-) 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) 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 09/15] 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) { From dfcf88265b0736e78e66817e481825a6f1bae153 Mon Sep 17 00:00:00 2001 From: santorac <55155825+santorac@users.noreply.github.com> Date: Thu, 21 Oct 2021 01:28:08 -0700 Subject: [PATCH 10/15] Fixed a MaterialBuilder issue where material version updates were incorrectly reporting failure in some cases. Signed-off-by: santorac <55155825+santorac@users.noreply.github.com> --- .../Atom/RPI.Edit/Material/MaterialSourceData.h | 10 ++++++++-- .../Source/RPI.Builders/Material/MaterialBuilder.cpp | 2 +- .../Source/RPI.Edit/Material/MaterialSourceData.cpp | 11 ++++++----- 3 files changed, 15 insertions(+), 8 deletions(-) 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 9557d51849..a8bf790ad8 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 @@ -64,10 +64,16 @@ namespace AZ PropertyGroupMap m_properties; + enum class ApplyVersionUpdatesResult + { + Failed, + NoUpdates, + UpdatesApplied + }; + //! 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(); + ApplyVersionUpdatesResult ApplyVersionUpdates(AZStd::string_view materialSourceFilePath); //! Creates a MaterialAsset from the MaterialSourceData content. //! @param assetId ID for the MaterialAsset diff --git a/Gems/Atom/RPI/Code/Source/RPI.Builders/Material/MaterialBuilder.cpp b/Gems/Atom/RPI/Code/Source/RPI.Builders/Material/MaterialBuilder.cpp index 0680c5c428..02fd11bdff 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Builders/Material/MaterialBuilder.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Builders/Material/MaterialBuilder.cpp @@ -287,7 +287,7 @@ namespace AZ return {}; } - if (!material.GetValue().ApplyVersionUpdates()) + if (MaterialSourceData::ApplyVersionUpdatesResult::Failed == material.GetValue().ApplyVersionUpdates(materialSourceFilePath)) { return {}; } 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 c3a63fa883..9d44963942 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialSourceData.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialSourceData.cpp @@ -73,19 +73,20 @@ namespace AZ } } - bool MaterialSourceData::ApplyVersionUpdates() + MaterialSourceData::ApplyVersionUpdatesResult MaterialSourceData::ApplyVersionUpdates(AZStd::string_view materialSourceFilePath) { - auto materialTypeSourceDataOutcome = MaterialUtils::LoadMaterialTypeSourceData(m_materialType); + AZStd::string materialTypeFullPath = AssetUtils::ResolvePathReference(materialSourceFilePath, m_materialType); + auto materialTypeSourceDataOutcome = MaterialUtils::LoadMaterialTypeSourceData(materialTypeFullPath); if (!materialTypeSourceDataOutcome.IsSuccess()) { - return false; + return ApplyVersionUpdatesResult::Failed; } MaterialTypeSourceData materialTypeSourceData = materialTypeSourceDataOutcome.TakeValue(); if (m_materialTypeVersion == materialTypeSourceData.m_version) { - return false; + return ApplyVersionUpdatesResult::NoUpdates; } bool changesWereApplied = false; @@ -125,7 +126,7 @@ namespace AZ m_materialTypeVersion = materialTypeSourceData.m_version; - return true; + return changesWereApplied ? ApplyVersionUpdatesResult::UpdatesApplied : ApplyVersionUpdatesResult::NoUpdates; } Outcome > MaterialSourceData::CreateMaterialAsset(Data::AssetId assetId, AZStd::string_view materialSourceFilePath, bool elevateWarnings, bool includeMaterialPropertyNames) const From f83b6594c9680d54fa82f9ed9ea1cbce03638ba6 Mon Sep 17 00:00:00 2001 From: santorac <55155825+santorac@users.noreply.github.com> Date: Thu, 21 Oct 2021 11:05:47 -0700 Subject: [PATCH 11/15] Updated MaterialTypeSourceData to force users to move the "version" indicator to the new location at the top level of the json document. (It's a simple enough change to make manually, and making .materialtype is an uncommon workflow, so not worth doing this automatically). Signed-off-by: santorac <55155825+santorac@users.noreply.github.com> --- .../ReflectionProbeVisualization.materialtype | 2 +- .../Special/ShadowCatcher.materialtype | 2 +- .../Materials/Types/EnhancedPBR.materialtype | 2 +- .../Assets/Materials/Types/Skin.materialtype | 2 +- .../Types/StandardMultilayerPBR.materialtype | 2 +- .../Materials/Types/StandardPBR.materialtype | 4 +-- .../RPI.Edit/Material/MaterialSourceData.h | 3 ++- .../Material/MaterialTypeSourceData.h | 3 +++ .../RPI.Builders/Material/MaterialBuilder.cpp | 2 +- .../Material/MaterialSourceDataSerializer.cpp | 4 +-- .../Material/MaterialTypeSourceData.cpp | 10 +++++++ .../Material/MaterialTypeSourceDataTests.cpp | 26 +++++++++++++++++++ .../Materials/Types/AutoBrick.materialtype | 2 +- .../Materials/Types/MinimalPBR.materialtype | 2 +- .../Materials/Terrain/PbrTerrain.materialtype | 2 +- .../Terrain/TerrainMacroMaterial.materialtype | 2 +- 16 files changed, 55 insertions(+), 15 deletions(-) diff --git a/Gems/Atom/Feature/Common/Assets/Materials/ReflectionProbe/ReflectionProbeVisualization.materialtype b/Gems/Atom/Feature/Common/Assets/Materials/ReflectionProbe/ReflectionProbeVisualization.materialtype index 17209771e5..30062205a8 100644 --- a/Gems/Atom/Feature/Common/Assets/Materials/ReflectionProbe/ReflectionProbeVisualization.materialtype +++ b/Gems/Atom/Feature/Common/Assets/Materials/ReflectionProbe/ReflectionProbeVisualization.materialtype @@ -1,7 +1,7 @@ { "description": "Base material for the reflection probe visualization model.", + "version": 1, "propertyLayout": { - "version": 1, "properties": { "general": [ { diff --git a/Gems/Atom/Feature/Common/Assets/Materials/Special/ShadowCatcher.materialtype b/Gems/Atom/Feature/Common/Assets/Materials/Special/ShadowCatcher.materialtype index 74246f85db..d05f03c9a9 100644 --- a/Gems/Atom/Feature/Common/Assets/Materials/Special/ShadowCatcher.materialtype +++ b/Gems/Atom/Feature/Common/Assets/Materials/Special/ShadowCatcher.materialtype @@ -1,7 +1,7 @@ { "description": "Base material for the reflection probe visualization model.", + "version": 1, "propertyLayout": { - "version": 1, "properties": { "settings": [ { diff --git a/Gems/Atom/Feature/Common/Assets/Materials/Types/EnhancedPBR.materialtype b/Gems/Atom/Feature/Common/Assets/Materials/Types/EnhancedPBR.materialtype index bed3b69c4c..f4bcfb2673 100644 --- a/Gems/Atom/Feature/Common/Assets/Materials/Types/EnhancedPBR.materialtype +++ b/Gems/Atom/Feature/Common/Assets/Materials/Types/EnhancedPBR.materialtype @@ -1,7 +1,7 @@ { "description": "Material Type with properties used to define Enhanced PBR, a metallic-roughness Physically-Based Rendering (PBR) material shading model, with advanced features like subsurface scattering, transmission, and anisotropy.", + "version": 3, "propertyLayout": { - "version": 3, "groups": [ { "name": "baseColor", diff --git a/Gems/Atom/Feature/Common/Assets/Materials/Types/Skin.materialtype b/Gems/Atom/Feature/Common/Assets/Materials/Types/Skin.materialtype index e4da9c6022..e2a05aa916 100644 --- a/Gems/Atom/Feature/Common/Assets/Materials/Types/Skin.materialtype +++ b/Gems/Atom/Feature/Common/Assets/Materials/Types/Skin.materialtype @@ -1,7 +1,7 @@ { "description": "Material Type tailored for rendering skin, with support for blended wrinkle maps that work with animated vertex blend shapes.", + "version": 3, "propertyLayout": { - "version": 3, "groups": [ { "name": "baseColor", diff --git a/Gems/Atom/Feature/Common/Assets/Materials/Types/StandardMultilayerPBR.materialtype b/Gems/Atom/Feature/Common/Assets/Materials/Types/StandardMultilayerPBR.materialtype index 5fa0dcb217..107b525ae4 100644 --- a/Gems/Atom/Feature/Common/Assets/Materials/Types/StandardMultilayerPBR.materialtype +++ b/Gems/Atom/Feature/Common/Assets/Materials/Types/StandardMultilayerPBR.materialtype @@ -1,7 +1,7 @@ { "description": "Similar to StandardPBR but supports multiple layers blended together.", + "version": 3, "propertyLayout": { - "version": 3, "groups": [ { "name": "blend", diff --git a/Gems/Atom/Feature/Common/Assets/Materials/Types/StandardPBR.materialtype b/Gems/Atom/Feature/Common/Assets/Materials/Types/StandardPBR.materialtype index 6eb82b85ae..84b4c29fcb 100644 --- a/Gems/Atom/Feature/Common/Assets/Materials/Types/StandardPBR.materialtype +++ b/Gems/Atom/Feature/Common/Assets/Materials/Types/StandardPBR.materialtype @@ -1,7 +1,7 @@ { "description": "Material Type with properties used to define Standard PBR, a metallic-roughness Physically-Based Rendering (PBR) material shading model.", + "version": 3, "propertyLayout": { - "version": 3, "groups": [ { "name": "baseColor", @@ -152,7 +152,7 @@ ], "baseColor": [ { - "name": "color", + "name": "colorX", "displayName": "Color", "description": "Color is displayed as sRGB but the values are stored as linear color.", "type": "Color", 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 a8bf790ad8..a67477f061 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 @@ -73,7 +73,8 @@ 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. - ApplyVersionUpdatesResult ApplyVersionUpdates(AZStd::string_view materialSourceFilePath); + //! @param materialSourceFilePath Indicates the path of the .material file that the MaterialSourceData represents. Used for resolving file-relative paths. + ApplyVersionUpdatesResult ApplyVersionUpdates(AZStd::string_view materialSourceFilePath = ""); //! Creates a MaterialAsset from the MaterialSourceData content. //! @param assetId ID for the MaterialAsset diff --git a/Gems/Atom/RPI/Code/Include/Atom/RPI.Edit/Material/MaterialTypeSourceData.h b/Gems/Atom/RPI/Code/Include/Atom/RPI.Edit/Material/MaterialTypeSourceData.h index 99eb15db24..1234b15f95 100644 --- a/Gems/Atom/RPI/Code/Include/Atom/RPI.Edit/Material/MaterialTypeSourceData.h +++ b/Gems/Atom/RPI/Code/Include/Atom/RPI.Edit/Material/MaterialTypeSourceData.h @@ -147,6 +147,9 @@ namespace AZ struct PropertyLayout { AZ_TYPE_INFO(AZ::RPI::MaterialTypeSourceData::PropertyLayout, "{AE53CF3F-5C3B-44F5-B2FB-306F0EB06393}"); + + //! This field is unused, and has been replaced by MaterialTypeSourceData::m_version below. It is kept for legacy file compatibility to suppress warnings and errors. + uint32_t m_versionOld = 0; //! List of groups that will contain the available properties AZStd::vector m_groups; diff --git a/Gems/Atom/RPI/Code/Source/RPI.Builders/Material/MaterialBuilder.cpp b/Gems/Atom/RPI/Code/Source/RPI.Builders/Material/MaterialBuilder.cpp index 02fd11bdff..e9cebf29c7 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Builders/Material/MaterialBuilder.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Builders/Material/MaterialBuilder.cpp @@ -47,7 +47,7 @@ namespace AZ { AssetBuilderSDK::AssetBuilderDesc materialBuilderDescriptor; materialBuilderDescriptor.m_name = JobKey; - materialBuilderDescriptor.m_version = 109; // Changed "id" to "name" in serialization + materialBuilderDescriptor.m_version = 110; // Material version auto update feature materialBuilderDescriptor.m_patterns.push_back(AssetBuilderSDK::AssetBuilderPattern("*.material", AssetBuilderSDK::AssetBuilderPattern::PatternType::Wildcard)); materialBuilderDescriptor.m_patterns.push_back(AssetBuilderSDK::AssetBuilderPattern("*.materialtype", AssetBuilderSDK::AssetBuilderPattern::PatternType::Wildcard)); materialBuilderDescriptor.m_busId = azrtti_typeid(); diff --git a/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialSourceDataSerializer.cpp b/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialSourceDataSerializer.cpp index cdd434e894..2a504fc345 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialSourceDataSerializer.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialSourceDataSerializer.cpp @@ -46,8 +46,8 @@ namespace AZ } result.Combine(ContinueLoadingFromJsonObjectField(&materialSourceData->m_description, azrtti_typeid(), inputValue, "description", context)); - result.Combine(ContinueLoadingFromJsonObjectField(&materialSourceData->m_materialType, azrtti_typeid(), inputValue, "materialType", context)); result.Combine(ContinueLoadingFromJsonObjectField(&materialSourceData->m_parentMaterial, azrtti_typeid(), inputValue, "parentMaterial", context)); + result.Combine(ContinueLoadingFromJsonObjectField(&materialSourceData->m_materialType, azrtti_typeid(), inputValue, "materialType", context)); result.Combine(ContinueLoadingFromJsonObjectField(&materialSourceData->m_materialTypeVersion, azrtti_typeid(), inputValue, "materialTypeVersion", context)); if (materialSourceData->m_materialType.empty()) @@ -151,8 +151,8 @@ namespace AZ JSR::ResultCode resultCode(JSR::Tasks::ReadField); resultCode.Combine(ContinueStoringToJsonObjectField(outputValue, "description", &materialSourceData->m_description, nullptr, azrtti_typeid(), context)); - resultCode.Combine(ContinueStoringToJsonObjectField(outputValue, "materialType", &materialSourceData->m_materialType, nullptr, azrtti_typeid(), context)); resultCode.Combine(ContinueStoringToJsonObjectField(outputValue, "parentMaterial", &materialSourceData->m_parentMaterial, nullptr, azrtti_typeid(), context)); + resultCode.Combine(ContinueStoringToJsonObjectField(outputValue, "materialType", &materialSourceData->m_materialType, nullptr, azrtti_typeid(), context)); resultCode.Combine(ContinueStoringToJsonObjectField(outputValue, "materialTypeVersion", &materialSourceData->m_materialTypeVersion, nullptr, azrtti_typeid(), context)); resultCode.Combine(ContinueStoringToJsonObjectField(outputValue, "properties", &materialSourceData->m_properties, nullptr, azrtti_typeid(), context)); 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 8c8bad8c0c..74250647c3 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialTypeSourceData.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialTypeSourceData.cpp @@ -86,6 +86,7 @@ namespace AZ serializeContext->Class() ->Version(2) // Material Version Update + ->Field("version", &PropertyLayout::m_versionOld) ->Field("groups", &PropertyLayout::m_groups) ->Field("properties", &PropertyLayout::m_properties) ; @@ -351,6 +352,15 @@ namespace AZ materialTypeAssetCreator.SetElevateWarnings(elevateWarnings); materialTypeAssetCreator.Begin(assetId); + if (m_propertyLayout.m_versionOld != 0) + { + materialTypeAssetCreator.ReportError( + "The field '/propertyLayout/version' is deprecated and moved to '/version'. " + "Please edit this material type source file and move the '\"version\": %u' setting up one level.", + m_propertyLayout.m_versionOld); + return Failure(); + } + // Set materialtype version and add each version update object into MaterialTypeAsset. materialTypeAssetCreator.SetVersion(m_version); { diff --git a/Gems/Atom/RPI/Code/Tests/Material/MaterialTypeSourceDataTests.cpp b/Gems/Atom/RPI/Code/Tests/Material/MaterialTypeSourceDataTests.cpp index 709d6b84ec..b811e630d0 100644 --- a/Gems/Atom/RPI/Code/Tests/Material/MaterialTypeSourceDataTests.cpp +++ b/Gems/Atom/RPI/Code/Tests/Material/MaterialTypeSourceDataTests.cpp @@ -1477,4 +1477,30 @@ namespace UnitTest errorMessageFinder.CheckExpectedErrorsFound(); } + + TEST_F(MaterialTypeSourceDataTests, CreateMaterialTypeAsset_Error_VersionInWrongLocation) + { + // The version field used to be under the propertyLayout section, but it has been moved up to the top level. + // If any users have their own custom .materialtype with an older format that has the version in the wrong place + // then we will report an error with instructions to move it to the correct location. + + ErrorMessageFinder errorMessageFinder; + errorMessageFinder.AddExpectedErrorMessage("The field '/propertyLayout/version' is deprecated and moved to '/version'. Please edit this material type source file and move the '\"version\": 4' setting up one level"); + + const AZStd::string inputJson = R"( + { + "propertyLayout": { + "version": 4 + } + } + )"; + + MaterialTypeSourceData materialType; + JsonTestResult loadResult = LoadTestDataFromJson(materialType, inputJson); + + auto materialTypeOutcome = materialType.CreateMaterialTypeAsset(Uuid::CreateRandom()); + EXPECT_FALSE(materialTypeOutcome.IsSuccess()); + + errorMessageFinder.CheckExpectedErrorsFound(); + } } diff --git a/Gems/Atom/TestData/TestData/Materials/Types/AutoBrick.materialtype b/Gems/Atom/TestData/TestData/Materials/Types/AutoBrick.materialtype index 00f11663f7..cf0bffa058 100644 --- a/Gems/Atom/TestData/TestData/Materials/Types/AutoBrick.materialtype +++ b/Gems/Atom/TestData/TestData/Materials/Types/AutoBrick.materialtype @@ -1,7 +1,7 @@ { "description": "This is an example of a custom material type using Atom's PBR shading model: procedurally generated brick or tile.", + "version": 3, "propertyLayout": { - "version": 3, "groups": [ { "name": "shape", diff --git a/Gems/Atom/TestData/TestData/Materials/Types/MinimalPBR.materialtype b/Gems/Atom/TestData/TestData/Materials/Types/MinimalPBR.materialtype index 81ebd63c28..5d99737576 100644 --- a/Gems/Atom/TestData/TestData/Materials/Types/MinimalPBR.materialtype +++ b/Gems/Atom/TestData/TestData/Materials/Types/MinimalPBR.materialtype @@ -1,7 +1,7 @@ { "description": "Base Material with properties used to define Standard PBR, a metallic-roughness Physically-Based Rendering (PBR) material shading model.", + "version": 3, "propertyLayout": { - "version": 3, "groups": [ { "name": "settings", diff --git a/Gems/Terrain/Assets/Materials/Terrain/PbrTerrain.materialtype b/Gems/Terrain/Assets/Materials/Terrain/PbrTerrain.materialtype index ec04412fe6..01b862c4f8 100644 --- a/Gems/Terrain/Assets/Materials/Terrain/PbrTerrain.materialtype +++ b/Gems/Terrain/Assets/Materials/Terrain/PbrTerrain.materialtype @@ -1,7 +1,7 @@ { "description": "A material for rendering terrain with a physically-based rendering (PBR) material shading model.", + "version": 1, "propertyLayout": { - "version": 1, "groups": [ { "id": "settings", diff --git a/Gems/Terrain/Assets/Materials/Terrain/TerrainMacroMaterial.materialtype b/Gems/Terrain/Assets/Materials/Terrain/TerrainMacroMaterial.materialtype index 3cdab8da10..17769ffb92 100644 --- a/Gems/Terrain/Assets/Materials/Terrain/TerrainMacroMaterial.materialtype +++ b/Gems/Terrain/Assets/Materials/Terrain/TerrainMacroMaterial.materialtype @@ -1,7 +1,7 @@ { "description": "A material for providing terrain with low-fidelity color and normals. This material will get blended with surface detail materials.", + "version": 1, "propertyLayout": { - "version": 1, "groups": [ { "name": "baseColor", From f4d5a75574098ed9e67f6badf29ddf9e95de9d3e Mon Sep 17 00:00:00 2001 From: santorac <55155825+santorac@users.noreply.github.com> Date: Thu, 21 Oct 2021 12:13:45 -0700 Subject: [PATCH 12/15] Fixed an issue where the wrong version number was being reported in a warning message. It showed the latest version instead of the original version number. Signed-off-by: santorac <55155825+santorac@users.noreply.github.com> --- .../Code/Source/RPI.Edit/Material/MaterialSourceData.cpp | 2 +- .../RPI/Code/Source/RPI.Reflect/Material/MaterialAsset.cpp | 6 ++++-- Gems/Atom/RPI/Code/Tests/Material/MaterialAssetTests.cpp | 2 ++ .../RPI/Code/Tests/Material/MaterialSourceDataTests.cpp | 2 ++ 4 files changed, 9 insertions(+), 3 deletions(-) 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 9d44963942..5aed6b2993 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialSourceData.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialSourceData.cpp @@ -119,7 +119,7 @@ namespace AZ if (changesWereApplied) { AZ_Warning("MaterialSourceData", false, - "This material is based on version %u of '%s', but the material type is now at version %u. " + "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); } 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 2b7c1c58c7..e9d8a42641 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialAsset.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialAsset.cpp @@ -208,6 +208,8 @@ namespace AZ return; } + const uint32_t originalVersion = m_materialTypeVersion; + bool changesWereApplied = false; for (const MaterialVersionUpdate& versionUpdate : m_materialTypeAsset->GetMaterialVersionUpdateList()) @@ -225,9 +227,9 @@ namespace AZ if (changesWereApplied) { AZ_Warning("MaterialAsset", false, - "This material is based on version %u of '%s', but the material type is now at version %u. " + "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()); + originalVersion, m_materialTypeAsset.ToString().c_str(), m_materialTypeAsset->GetVersion()); } m_materialTypeVersion = m_materialTypeAsset->GetVersion(); diff --git a/Gems/Atom/RPI/Code/Tests/Material/MaterialAssetTests.cpp b/Gems/Atom/RPI/Code/Tests/Material/MaterialAssetTests.cpp index cf8b55d581..58a852f176 100644 --- a/Gems/Atom/RPI/Code/Tests/Material/MaterialAssetTests.cpp +++ b/Gems/Atom/RPI/Code/Tests/Material/MaterialAssetTests.cpp @@ -264,6 +264,8 @@ namespace UnitTest // 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"); + warningFinder.AddExpectedErrorMessage("This material is based on version '1'"); + warningFinder.AddExpectedErrorMessage("material type is now at version '2'"); // 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. diff --git a/Gems/Atom/RPI/Code/Tests/Material/MaterialSourceDataTests.cpp b/Gems/Atom/RPI/Code/Tests/Material/MaterialSourceDataTests.cpp index 6a6947e7c7..acce52ae8e 100644 --- a/Gems/Atom/RPI/Code/Tests/Material/MaterialSourceDataTests.cpp +++ b/Gems/Atom/RPI/Code/Tests/Material/MaterialSourceDataTests.cpp @@ -728,6 +728,8 @@ namespace UnitTest // 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"); + warningFinder.AddExpectedErrorMessage("This material is based on version '1'"); + warningFinder.AddExpectedErrorMessage("material type is now at version '10'"); material.ApplyVersionUpdates(); warningFinder.CheckExpectedErrorsFound(); From b32a6a1369a28d19c2af3865c8e81f52fbc88e50 Mon Sep 17 00:00:00 2001 From: santorac <55155825+santorac@users.noreply.github.com> Date: Thu, 21 Oct 2021 12:52:28 -0700 Subject: [PATCH 13/15] Reverted accidental change to StandardPBR.material type's color property name. Signed-off-by: santorac <55155825+santorac@users.noreply.github.com> --- .../Common/Assets/Materials/Types/StandardPBR.materialtype | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Gems/Atom/Feature/Common/Assets/Materials/Types/StandardPBR.materialtype b/Gems/Atom/Feature/Common/Assets/Materials/Types/StandardPBR.materialtype index 84b4c29fcb..7527a7658a 100644 --- a/Gems/Atom/Feature/Common/Assets/Materials/Types/StandardPBR.materialtype +++ b/Gems/Atom/Feature/Common/Assets/Materials/Types/StandardPBR.materialtype @@ -152,7 +152,7 @@ ], "baseColor": [ { - "name": "colorX", + "name": "color", "displayName": "Color", "description": "Color is displayed as sRGB but the values are stored as linear color.", "type": "Color", From 7ffcbe080e57d236e3b765b1f8213696fdc2bf1a Mon Sep 17 00:00:00 2001 From: santorac <55155825+santorac@users.noreply.github.com> Date: Thu, 21 Oct 2021 13:23:18 -0700 Subject: [PATCH 14/15] Updated Material Editor's MaterialDocument to apply version updates to the materials that it opens. Signed-off-by: santorac <55155825+santorac@users.noreply.github.com> --- .../Code/Source/Document/MaterialDocument.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/Gems/Atom/Tools/MaterialEditor/Code/Source/Document/MaterialDocument.cpp b/Gems/Atom/Tools/MaterialEditor/Code/Source/Document/MaterialDocument.cpp index 51164a70f4..98af749261 100644 --- a/Gems/Atom/Tools/MaterialEditor/Code/Source/Document/MaterialDocument.cpp +++ b/Gems/Atom/Tools/MaterialEditor/Code/Source/Document/MaterialDocument.cpp @@ -685,6 +685,12 @@ namespace MaterialEditor return false; } m_materialTypeSourceData = materialTypeOutcome.GetValue(); + + if (MaterialSourceData::ApplyVersionUpdatesResult::Failed == m_materialSourceData.ApplyVersionUpdates(m_absolutePath)) + { + AZ_Error("MaterialDocument", false, "Material source data could not be auto updated to the latest version of the material type: '%s'.", m_materialSourceData.m_materialType.c_str()); + return false; + } } else if (AzFramework::StringFunc::Path::IsExtension(m_absolutePath.c_str(), MaterialTypeSourceData::Extension)) { From a4321baecc8b28dcec9a73eb3eb32480249b8af3 Mon Sep 17 00:00:00 2001 From: santorac <55155825+santorac@users.noreply.github.com> Date: Thu, 21 Oct 2021 17:00:06 -0700 Subject: [PATCH 15/15] Hopefully fix a Jenkins build failure about "function cannot access 'AZ::RPI::MaterialVersionUpdate::RenamePropertyAction::m_fromPropertyId'" Signed-off-by: santorac <55155825+santorac@users.noreply.github.com> --- .../RPI.Reflect/Material/MaterialVersionUpdate.h | 2 ++ .../Material/MaterialVersionUpdate.cpp | 16 ++++++++++++---- 2 files changed, 14 insertions(+), 4 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 0acd1d4432..eb71ad9cb7 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 @@ -32,6 +32,8 @@ namespace AZ struct RenamePropertyAction { AZ_TYPE_INFO(AZ::RPI::MaterialVersionUpdate::RenameAction, "{A1FBEB19-EA05-40F0-9700-57D048DF572B}"); + + static void Reflect(ReflectContext* context); AZ::Name m_fromPropertyId; AZ::Name m_toPropertyId; 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 bf503f3c67..b387e502e2 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialVersionUpdate.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialVersionUpdate.cpp @@ -14,16 +14,24 @@ namespace AZ { namespace RPI { - void MaterialVersionUpdate::Reflect(ReflectContext* context) + void MaterialVersionUpdate::RenamePropertyAction::Reflect(ReflectContext* context) { if (auto* serializeContext = azrtti_cast(context)) { - serializeContext->Class() + serializeContext->Class() ->Version(1) - ->Field("From", &MaterialVersionUpdate::RenamePropertyAction::m_fromPropertyId) - ->Field("To", &MaterialVersionUpdate::RenamePropertyAction::m_toPropertyId) + ->Field("From", &RenamePropertyAction::m_fromPropertyId) + ->Field("To", &RenamePropertyAction::m_toPropertyId) ; + } + } + void MaterialVersionUpdate::Reflect(ReflectContext* context) + { + MaterialVersionUpdate::RenamePropertyAction::Reflect(context); + + if (auto* serializeContext = azrtti_cast(context)) + { serializeContext->RegisterGenericType(); serializeContext->Class()