From 4312c636afb458cb630a23100186150a6324240a Mon Sep 17 00:00:00 2001 From: santorac <55155825+santorac@users.noreply.github.com> Date: Wed, 26 Jan 2022 12:19:49 -0800 Subject: [PATCH] Got the RPI unit tests building and working again after merge. There were some incorrectly resolved conflicts that I had to re-resolve, especially in MaterialTypeSourceData::CreateMaterialTypeAsset. I removed support for property rename version updates in MaterialTypeSourceData (i.e. ApplyPropertyRenames) because MaterialSourceData serialization no longer loads material property definitions from the .materialtype file, per a recent change on the development branch. The unit tests were broken and it wasn't worth updating them since we don't need this functionality anymore. Material property renames and other version updates are now exclusively applied by the MaterialAsset class. Signed-off-by: santorac <55155825+santorac@users.noreply.github.com> --- .../Material/MaterialTypeSourceData.cpp | 327 +----------------- .../Material/MaterialSourceDataTests.cpp | 45 +-- .../Material/MaterialTypeSourceDataTests.cpp | 152 +------- 3 files changed, 38 insertions(+), 486 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 9e72e13b34..2504981273 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialTypeSourceData.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialTypeSourceData.cpp @@ -132,32 +132,6 @@ namespace AZ const float MaterialTypeSourceData::PropertyDefinition::DefaultMax = std::numeric_limits::max(); const float MaterialTypeSourceData::PropertyDefinition::DefaultStep = 0.1f; - bool MaterialTypeSourceData::ApplyPropertyRenames(MaterialPropertyId& propertyId) const - { - bool renamed = false; - - for (const VersionUpdateDefinition& versionUpdate : m_versionUpdates) - { - for (const VersionUpdatesRenameOperationDefinition& action : versionUpdate.m_actions) - { - if (action.m_operation == "rename") - { - if (action.m_renameFrom == propertyId.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; - } - /*static*/ MaterialTypeSourceData::PropertySet* MaterialTypeSourceData::PropertySet::AddPropertySet(AZStd::string_view name, AZStd::vector>& toPropertySetList) { auto iter = AZStd::find_if(toPropertySetList.begin(), toPropertySetList.end(), [name](const AZStd::unique_ptr& existingPropertySet) @@ -528,49 +502,6 @@ namespace AZ return groupDefinitions; } - // TODO: It looks like this function doesn't operate on MaterialTypeSourceData data, it belongs in MaterialUtils - bool MaterialTypeSourceData::ConvertPropertyValueToSourceDataFormat(const PropertyDefinition& propertyDefinition, MaterialPropertyValue& propertyValue) const - { - if (propertyDefinition.m_dataType == AZ::RPI::MaterialPropertyDataType::Enum && propertyValue.Is()) - { - const uint32_t index = propertyValue.GetValue(); - if (index >= propertyDefinition.m_enumValues.size()) - { - AZ_Error("Material source data", false, "Invalid value for material enum property: '%s'.", propertyDefinition.m_name.c_str()); - return false; - } - - propertyValue = propertyDefinition.m_enumValues[index]; - return true; - } - - // Image asset references must be converted from asset IDs to a relative source file path - if (propertyDefinition.m_dataType == AZ::RPI::MaterialPropertyDataType::Image && propertyValue.Is>()) - { - const Data::Asset& imageAsset = propertyValue.GetValue>(); - - Data::AssetInfo imageAssetInfo; - if (imageAsset.GetId().IsValid()) - { - bool result = false; - AZStd::string rootFilePath; - const AZStd::string platformName = ""; // Empty for default - AzToolsFramework::AssetSystemRequestBus::BroadcastResult(result, &AzToolsFramework::AssetSystem::AssetSystemRequest::GetAssetInfoById, - imageAsset.GetId(), imageAsset.GetType(), platformName, imageAssetInfo, rootFilePath); - if (!result) - { - AZ_Error("Material source data", false, "Image asset could not be found for property: '%s'.", propertyDefinition.m_name.c_str()); - return false; - } - } - - propertyValue = imageAssetInfo.m_relativePath; - return true; - } - - return true; - } - bool MaterialTypeSourceData::BuildPropertyList( const AZStd::string& materialTypeSourceFilePath, MaterialTypeAssetCreator& materialTypeAssetCreator, @@ -651,15 +582,20 @@ namespace AZ { case MaterialPropertyDataType::Image: { - Outcome> imageAssetResult = MaterialUtils::GetImageAssetReference(materialTypeSourceFilePath, property->m_value.GetValue()); + Data::Asset imageAsset; - if (imageAssetResult.IsSuccess()) + MaterialUtils::GetImageAssetResult result = MaterialUtils::GetImageAssetReference( + imageAsset, materialTypeSourceFilePath, property->m_value.GetValue()); + + if (result == MaterialUtils::GetImageAssetResult::Missing) { - materialTypeAssetCreator.SetPropertyValue(propertyId, imageAssetResult.GetValue()); + materialTypeAssetCreator.ReportError( + "Material property '%s': Could not find the image '%s'", propertyId.GetCStr(), + property->m_value.GetValue().data()); } else { - materialTypeAssetCreator.ReportError("Material property '%s': Could not find the image '%s'", propertyId.GetCStr(), property->m_value.GetValue().data()); + materialTypeAssetCreator.SetPropertyValue(propertyId, imageAsset); } } break; @@ -743,145 +679,6 @@ namespace AZ } - Outcome> MaterialTypeSourceData::CreateMaterialTypeAsset(Data::AssetId assetId, AZStd::string_view materialTypeSourceFilePath, bool elevateWarnings) const - { - MaterialTypeAssetCreator materialTypeAssetCreator; - materialTypeAssetCreator.SetElevateWarnings(elevateWarnings); - materialTypeAssetCreator.Begin(assetId); - - // 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 - { - return lhs.ToString() < rhs.ToString(); - }; - AZStd::set uvsInThisMaterialType(semanticComp); - - for (const ShaderVariantReferenceData& shaderRef : m_shaderCollection) - { - const auto& shaderFile = shaderRef.m_shaderFilePath; - auto shaderAssetResult = AssetUtils::LoadAsset(materialTypeSourceFilePath, shaderFile, 0); - - if (shaderAssetResult) - { - auto shaderAsset = shaderAssetResult.GetValue(); - auto optionsLayout = shaderAsset->GetShaderOptionGroupLayout(); - ShaderOptionGroup options{ optionsLayout }; - for (auto& iter : shaderRef.m_shaderOptionValues) - { - if (!options.SetValue(iter.first, iter.second)) - { - return Failure(); - } - } - - materialTypeAssetCreator.AddShader( - shaderAsset, options.GetShaderVariantId(), - shaderRef.m_shaderTag.IsEmpty() ? Uuid::CreateRandom().ToString() : shaderRef.m_shaderTag); - - // Gather UV names - const ShaderInputContract& shaderInputContract = shaderAsset->GetInputContract(); - for (const ShaderInputContract::StreamChannelInfo& channel : shaderInputContract.m_streamChannels) - { - const RHI::ShaderSemantic& semantic = channel.m_semantic; - - if (semantic.m_name.GetStringView().starts_with(RHI::ShaderSemantic::UvStreamSemantic)) - { - uvsInThisMaterialType.insert(semantic); - } - } - } - else - { - materialTypeAssetCreator.ReportError("Shader '%s' not found", shaderFile.data()); - return Failure(); - } - } - - for (const AZStd::unique_ptr& propertySet : m_propertyLayout.m_propertySets) - { - AZStd::vector propertyNameContext; - propertyNameContext.push_back(propertySet->m_name); - materialTypeAssetCreator.BeginMaterialProperty(propertyId.GetFullName(), property.m_dataType); - - if (!success) - { - return Failure(); - Data::Asset imageAsset; - - MaterialUtils::GetImageAssetResult result = MaterialUtils::GetImageAssetReference( - Outcome> imageAssetResult = MaterialUtils::GetImageAssetReference(materialTypeSourceFilePath, property.m_value.GetValue()); - if (imageAssetResult.IsSuccess()) - materialTypeAssetCreator.SetPropertyValue(propertyId.GetFullName(), imageAssetResult.GetValue()); - "Material property '%s': Could not find the image '%s'", propertyId.GetCStr(), - property.m_value.GetValue().data()); - materialTypeAssetCreator.ReportError("Material property '%s': Could not find the image '%s'", propertyId.GetFullName().GetCStr(), property.m_value.GetValue().data()); - MaterialPropertyIndex propertyIndex = materialTypeAssetCreator.GetMaterialPropertiesLayout()->FindPropertyIndex(propertyId.GetFullName()); - materialTypeAssetCreator.SetPropertyValue(propertyId.GetFullName(), enumValue); - materialTypeAssetCreator.SetPropertyValue(propertyId.GetFullName(), property.m_value); - } - } - - // We cannot create the MaterialFunctor until after all the properties are added because - // CreateFunctor() may need to look up properties in the MaterialPropertiesLayout - for (auto& functorData : m_materialFunctorSourceData) - { - MaterialFunctorSourceData::FunctorResult result = functorData->CreateFunctor( - MaterialFunctorSourceData::RuntimeContext( - materialTypeSourceFilePath, - materialTypeAssetCreator.GetMaterialPropertiesLayout(), - materialTypeAssetCreator.GetMaterialShaderResourceGroupLayout(), - materialTypeAssetCreator.GetShaderCollection() - ) - ); - - if (result.IsSuccess()) - { - Ptr& functor = result.GetValue(); - if (functor != nullptr) - { - materialTypeAssetCreator.AddMaterialFunctor(functor); - - for (const AZ::Name& optionName : functorData->GetActualSourceData()->GetShaderOptionDependencies()) - { - materialTypeAssetCreator.ClaimShaderOptionOwnership(optionName); - } - } - } - else - { - materialTypeAssetCreator.ReportError("Failed to create MaterialFunctor"); - return Failure(); - } - } - - // Only add the UV mapping related to this material type. - for (const auto& uvInput : uvsInThisMaterialType) - { - // We may have cases where the uv map is empty or inconsistent (exported from other projects), - // So we use semantic if mapping is not found. - auto iter = m_uvNameMap.find(uvInput.ToString()); - if (iter != m_uvNameMap.end()) - { - materialTypeAssetCreator.AddUvName(uvInput, Name(iter->second)); - } - else - { - materialTypeAssetCreator.AddUvName(uvInput, Name(uvInput.ToString())); - } - } - - Data::Asset materialTypeAsset; - if (materialTypeAssetCreator.End(materialTypeAsset)) - { - return Success(AZStd::move(materialTypeAsset)); - } - else - { - return Failure(); - } - } - - Outcome> MaterialTypeSourceData::CreateMaterialTypeAsset(Data::AssetId assetId, AZStd::string_view materialTypeSourceFilePath, bool elevateWarnings) const { MaterialTypeAssetCreator materialTypeAssetCreator; @@ -970,108 +767,16 @@ namespace AZ return Failure(); } } - - for (auto& groupIter : m_propertyLayout.m_properties) + + for (const AZStd::unique_ptr& propertySet : m_propertyLayout.m_propertySets) { - const AZStd::string& groupName = groupIter.first; + AZStd::vector propertyNameContext; + propertyNameContext.push_back(propertySet->m_name); + bool success = BuildPropertyList(materialTypeSourceFilePath, materialTypeAssetCreator, propertyNameContext, propertySet.get()); - for (const PropertyDefinition& property : groupIter.second) + if (!success) { - // Register the property... - - MaterialPropertyId propertyId{ groupName, property.m_name }; - - if (!propertyId.IsValid()) - { - materialTypeAssetCreator.ReportWarning("Cannot create material property with invalid ID '%s'.", propertyId.GetCStr()); - continue; - } - - materialTypeAssetCreator.BeginMaterialProperty(propertyId, property.m_dataType); - - if (property.m_dataType == MaterialPropertyDataType::Enum) - { - materialTypeAssetCreator.SetMaterialPropertyEnumNames(property.m_enumValues); - } - - for (auto& output : property.m_outputConnections) - { - switch (output.m_type) - { - case MaterialPropertyOutputType::ShaderInput: - materialTypeAssetCreator.ConnectMaterialPropertyToShaderInput(Name{ output.m_fieldName.data() }); - break; - case MaterialPropertyOutputType::ShaderOption: - if (output.m_shaderIndex >= 0) - { - materialTypeAssetCreator.ConnectMaterialPropertyToShaderOption(Name{ output.m_fieldName.data() }, output.m_shaderIndex); - } - else - { - materialTypeAssetCreator.ConnectMaterialPropertyToShaderOptions(Name{ output.m_fieldName.data() }); - } - break; - case MaterialPropertyOutputType::Invalid: - // Don't add any output mappings, this is the case when material functors are expected to process the property - break; - default: - AZ_Assert(false, "Unsupported MaterialPropertyOutputType"); - return Failure(); - } - } - - materialTypeAssetCreator.EndMaterialProperty(); - - // Parse and set the property's value... - if (!property.m_value.IsValid()) - { - AZ_Warning("Material source data", false, "Source data for material property value is invalid."); - } - else - { - switch (property.m_dataType) - { - case MaterialPropertyDataType::Image: - { - Data::Asset imageAsset; - - MaterialUtils::GetImageAssetResult result = MaterialUtils::GetImageAssetReference( - imageAsset, materialTypeSourceFilePath, property.m_value.GetValue()); - - if (result == MaterialUtils::GetImageAssetResult::Missing) - { - materialTypeAssetCreator.ReportError( - "Material property '%s': Could not find the image '%s'", propertyId.GetCStr(), - property.m_value.GetValue().data()); - } - else - { - materialTypeAssetCreator.SetPropertyValue(propertyId, imageAsset); - } - } - break; - case MaterialPropertyDataType::Enum: - { - MaterialPropertyIndex propertyIndex = materialTypeAssetCreator.GetMaterialPropertiesLayout()->FindPropertyIndex(propertyId); - const MaterialPropertyDescriptor* propertyDescriptor = materialTypeAssetCreator.GetMaterialPropertiesLayout()->GetPropertyDescriptor(propertyIndex); - - AZ::Name enumName = AZ::Name(property.m_value.GetValue()); - uint32_t enumValue = propertyDescriptor ? propertyDescriptor->GetEnumValue(enumName) : MaterialPropertyDescriptor::InvalidEnumValue; - if (enumValue == MaterialPropertyDescriptor::InvalidEnumValue) - { - materialTypeAssetCreator.ReportError("Enum value '%s' couldn't be found in the 'enumValues' list", enumName.GetCStr()); - } - else - { - materialTypeAssetCreator.SetPropertyValue(propertyId, enumValue); - } - } - break; - default: - materialTypeAssetCreator.SetPropertyValue(propertyId, property.m_value); - break; - } - } + return Failure(); } } diff --git a/Gems/Atom/RPI/Code/Tests/Material/MaterialSourceDataTests.cpp b/Gems/Atom/RPI/Code/Tests/Material/MaterialSourceDataTests.cpp index 808e312b71..1a3b46ac86 100644 --- a/Gems/Atom/RPI/Code/Tests/Material/MaterialSourceDataTests.cpp +++ b/Gems/Atom/RPI/Code/Tests/Material/MaterialSourceDataTests.cpp @@ -93,20 +93,23 @@ namespace UnitTest { "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"} - ] - } + "propertySets": [ + { + "name": "general", + "properties": [ + {"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": [ { @@ -580,18 +583,12 @@ namespace UnitTest errorMessageFinder.Reset(); errorMessageFinder.AddExpectedErrorMessage("Could not find asset [DoesNotExist.materialtype]"); - "properties": { - [ - { - "general": [ - "properties": [ - ] result = material.CreateMaterialAsset(AZ::Uuid::CreateRandom(), "test.material", AZ::RPI::MaterialAssetProcessingMode::PreBake, elevateWarnings); EXPECT_FALSE(result.IsSuccess()); errorMessageFinder.CheckExpectedErrorsFound(); errorMessageFinder.Reset(); - EXPECT_TRUE(loadResult.ContainsMessage("[simpleMaterialType.materialtype]/propertyLayout/properties", "Successfully read")); + errorMessageFinder.AddExpectedErrorMessage("Could not find asset [DoesNotExist.materialtype]"); errorMessageFinder.AddIgnoredErrorMessage("Failed to create material type asset ID", true); result = material.CreateMaterialAssetFromSourceData(AZ::Uuid::CreateRandom(), "test.material", elevateWarnings); EXPECT_FALSE(result.IsSuccess()); @@ -600,12 +597,6 @@ namespace UnitTest TEST_F(MaterialSourceDataTests, CreateMaterialAsset_MaterialPropertyNotFound) { - "properties": { - [ - { - "general": [ - "properties": [ - ] MaterialSourceData material; material.m_materialType = "@exefolder@/Temp/test.materialtype"; AddPropertyGroup(material, "general"); diff --git a/Gems/Atom/RPI/Code/Tests/Material/MaterialTypeSourceDataTests.cpp b/Gems/Atom/RPI/Code/Tests/Material/MaterialTypeSourceDataTests.cpp index 8e65b41e6c..f07e1da6c2 100644 --- a/Gems/Atom/RPI/Code/Tests/Material/MaterialTypeSourceDataTests.cpp +++ b/Gems/Atom/RPI/Code/Tests/Material/MaterialTypeSourceDataTests.cpp @@ -1911,157 +1911,13 @@ namespace UnitTest } - 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" }, - { "op": "rename", "from": "onlyOneProperty.bopA", "to": "otherGroup.bopB" } // This tests a group 'onlyOneProperty' that no longer exists in the material type - ] - } - ], - "propertyLayout": { - "properties": { - "general": [ - { - "name": "fooC", - "type": "Bool" - }, - { - "name": "barC", - "type": "Float" - } - ], - "otherGroup": [ - { - "name": "dontMindMe", - "type": "Bool" - }, - { - "name": "bazB", - "type": "Float" - }, - { - "name": "bopB", - "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"); - const MaterialTypeSourceData::PropertyDefinition* bop = materialType.FindProperty("otherGroup", "bopB"); - - EXPECT_TRUE(foo); - EXPECT_TRUE(bar); - EXPECT_TRUE(baz); - EXPECT_TRUE(bop); - EXPECT_EQ(foo->m_name, "fooC"); - EXPECT_EQ(bar->m_name, "barC"); - EXPECT_EQ(baz->m_name, "bazB"); - EXPECT_EQ(bop->m_name, "bopB"); - - // 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(bop, materialType.FindProperty("onlyOneProperty", "bopA")); - - 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")); - EXPECT_EQ(nullptr, materialType.FindProperty("onlyOneProperty", "bopB")); - EXPECT_EQ(nullptr, materialType.FindProperty("otherGroup", "bopA")); - } - - 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); + + MaterialTypeSourceData::PropertyDefinition* propertySource = sourceData.AddPropertySet("general")->AddProperty("a"); + propertySource->m_dataType = MaterialPropertyDataType::Int; + propertySource->m_value = 0; sourceData.m_version = 2;