Resolve PR comments. Add unit tests.

Signed-off-by: Robin <rbarrand@amazon.com>
monroegm-disable-blank-issue-2
Robin 4 years ago
parent 59031d2f17
commit 2607b3471a

@ -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<VersionUpdatesRenameOperationDefinition>;
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<GroupDefinition> m_groups;

@ -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 <typename iteratorType>
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();

@ -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;

@ -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).

@ -11,13 +11,18 @@
#include <AzCore/std/containers/vector.h>
#include <AzCore/std/containers/map.h>
#include <AzCore/std/string/string.h>
#include <AzCore/Name/Name.h>
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<AZStd::string, AZStd::string> m_argsMap;
AZ::Name m_operation;
AZStd::unordered_map<AZ::Name, AZ::Name> m_argsMap;
Action() = default;
Action(const AZStd::string& operation, const AZStd::initializer_list<AZStd::pair<AZStd::string, AZStd::string>>& args);
void AddArgs(const AZStd::string& key, const AZStd::string& argument);
Action(const AZ::Name& operation, const AZStd::initializer_list<AZStd::pair<AZ::Name, AZ::Name>>& args);
void AddArg(const AZ::Name& key, const AZ::Name& argument);
};
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<Action>;
const Actions& GetActions() const;
void AddAction(const Action& action);
private:
uint32_t m_toVersion;
Actions m_actions;
};

@ -95,8 +95,7 @@ namespace AZ
;
serializeContext->Class<PropertyLayout>()
->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);
}

@ -6,6 +6,8 @@
*
*/
#pragma optimize("", off)
#include <Atom/RPI.Reflect/Material/MaterialAsset.h>
#include <Atom/RPI.Reflect/Material/MaterialPropertiesLayout.h>
#include <Atom/RPI.Reflect/Material/MaterialFunctor.h>
@ -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<MaterialAsset*>(this)->RenamePropertyNames();
const_cast<uint32_t&>(m_materialTypeVersion) = materialTypeVersion;
const_cast<MaterialAsset*>(this)->ApplyVersionUpdates();
}
if (m_isDirty)
@ -194,50 +195,16 @@ namespace AZ
m_isDirty = false;
}
template <typename iteratorType>
void MaterialAsset::RenamePropertyNames(iteratorType start, iteratorType end)
void MaterialAsset::ApplyVersionUpdates()
{
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<AZStd::map<AZStd::string, AZStd::string>> versionToFrom;
const bool ascending = m_materialTypeVersion < m_materialTypeAsset->GetVersion();
for (int i = 0; i < AZStd::abs(static_cast<int>(m_materialTypeVersion - m_materialTypeAsset->GetVersion())); ++i)
for (int i = 0; i < static_cast<int>(m_materialTypeAsset->GetVersion() - m_materialTypeVersion); ++i)
{
const auto& versionUpdate = m_materialTypeAsset->GetMaterialVersionUpdate((m_materialTypeVersion < m_materialTypeAsset->GetVersion() ? m_materialTypeVersion : m_materialTypeAsset->GetVersion()) + i + 1);
const auto& versionUpdate = m_materialTypeAsset->GetMaterialVersionUpdate(m_materialTypeVersion + 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<Data::AssetData> asset)
@ -287,3 +254,5 @@ namespace AZ
} // namespace RPI
} // namespace AZ
#pragma optimize("", on)

@ -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()

@ -102,18 +102,18 @@ namespace AZ
bool MaterialTypeAssetCreator::ValidateMaterialVersion()
{
AZStd::vector<AZStd::string> renamedPropertieNames;
AZStd::vector<AZ::Name> 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;
}

@ -7,6 +7,7 @@
*/
#include <Atom/RPI.Reflect/Material/MaterialVersionUpdate.h>
#include <Atom/RPI.Reflect/Material/MaterialAsset.h>
#include <AzCore/Serialization/SerializeContext.h>
namespace AZ
@ -40,16 +41,59 @@ namespace AZ
{
}
MaterialVersionUpdate::Action::Action(const AZStd::string& operation, const AZStd::initializer_list<AZStd::pair<AZStd::string, AZStd::string>>& 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<AZ::Name, AZ::Name> renameToFrom;
for (const auto& action : m_actions)
{
const AZ::Name from = action.m_argsMap.find(AZ::Name{ "from" })->second;
const AZ::Name to = action.m_argsMap.find(AZ::Name{ "to" })->second;
renameToFrom[from] = to;
}
// apply rename actions
for (auto& propertyName : materialAsset.m_propertyNames)
{
const auto toFromIterator = renameToFrom.find(propertyName);
if (toFromIterator != renameToFrom.end())
{
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<AZStd::pair<AZ::Name, AZ::Name>>& 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;
}

@ -63,6 +63,17 @@ namespace UnitTest
RPITestFixture::TearDown();
}
void UpgradeAndValidateMaterialAsset(Data::Asset<MaterialAsset> materialAsset, Data::Asset<MaterialTypeAsset> 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<Data::Asset<ImageAsset>>(), streamingImageAsset);
}
TEST_F(MaterialAssetTests, UpgradeMaterialAsset)
{
auto materialSrgLayout = CreateCommonTestMaterialSrgLayout();
auto shaderAsset = CreateTestShaderAsset(Uuid::CreateRandom(), materialSrgLayout);
Data::Asset<MaterialTypeAsset> 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> 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<MaterialTypeAsset> 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());

@ -14,6 +14,7 @@
#include <Material/MaterialAssetTestUtils.h>
#include <Atom/RPI.Reflect/Material/MaterialTypeAssetCreator.h>
#include <Atom/RPI.Reflect/Material/MaterialVersionUpdate.h>
#include <Atom/RPI.Reflect/Material/MaterialFunctor.h>
#include <Atom/RPI.Reflect/Material/MaterialPropertiesLayout.h>
#include <Atom/RPI.Public/Shader/ShaderResourceGroup.h>
@ -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> 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)
{

@ -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);

@ -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

@ -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;

Loading…
Cancel
Save