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;