diff --git a/Gems/Atom/RPI/Code/Include/Atom/RPI.Edit/Material/MaterialPropertyId.h b/Gems/Atom/RPI/Code/Include/Atom/RPI.Edit/Material/MaterialPropertyId.h index 77d49f3407..4b6d78c092 100644 --- a/Gems/Atom/RPI/Code/Include/Atom/RPI.Edit/Material/MaterialPropertyId.h +++ b/Gems/Atom/RPI/Code/Include/Atom/RPI.Edit/Material/MaterialPropertyId.h @@ -9,6 +9,7 @@ #pragma once #include +#include namespace AZ { @@ -16,27 +17,28 @@ namespace AZ { class MaterialAsset; - //! Utility for building material property names consisting of a group name and a property sub-name. - //! Represented as "[groupName].[propertyName]". - //! The group name is optional, in which case the ID will just be "[propertyName]". + //! Utility for building material property IDs. + //! These IDs are represented like "groupA.groupB.[...].propertyName". + //! The groups are optional, in which case the full property ID will just be like "propertyName". class MaterialPropertyId { public: static bool IsValidName(AZStd::string_view name); static bool IsValidName(const AZ::Name& name); - //! Creates a MaterialPropertyId from a full name string like "[groupName].[propertyName]" or just "[propertyName]" + //! Creates a MaterialPropertyId from a full name string like "groupA.groupB.[...].propertyName" or just "propertyName". + //! Also checks the name for validity. static MaterialPropertyId Parse(AZStd::string_view fullPropertyId); MaterialPropertyId() = default; + explicit MaterialPropertyId(AZStd::string_view propertyName); MaterialPropertyId(AZStd::string_view groupName, AZStd::string_view propertyName); MaterialPropertyId(const Name& groupName, const Name& propertyName); + MaterialPropertyId(const AZStd::array_view names); AZ_DEFAULT_COPY_MOVE(MaterialPropertyId); - const Name& GetGroupName() const; - const Name& GetPropertyName() const; - const Name& GetFullName() const; + operator const Name&() const; //! Returns a pointer to the full name ("[groupName].[propertyName]"). //! This is included for convenience so it can be used for error messages in the same way an AZ::Name is used. @@ -52,8 +54,6 @@ namespace AZ private: Name m_fullName; - Name m_groupName; - Name m_propertyName; }; } // namespace RPI diff --git a/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialPropertyId.cpp b/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialPropertyId.cpp index e74ec3e6e0..1be5c4485c 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialPropertyId.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialPropertyId.cpp @@ -27,63 +27,84 @@ namespace AZ bool MaterialPropertyId::IsValid() const { - const bool groupNameIsValid = m_groupName.IsEmpty() || IsValidName(m_groupName); - const bool propertyNameIsValid = IsValidName(m_propertyName); - return groupNameIsValid && propertyNameIsValid; + return !m_fullName.IsEmpty(); } MaterialPropertyId MaterialPropertyId::Parse(AZStd::string_view fullPropertyId) { AZStd::vector tokens; - AzFramework::StringFunc::Tokenize(fullPropertyId.data(), tokens, '.', true, true); + AzFramework::StringFunc::Tokenize(fullPropertyId, tokens, '.', true, true); - if (tokens.size() == 1) + if (tokens.empty()) { - return MaterialPropertyId{"", tokens[0]}; + AZ_Error("MaterialPropertyId", false, "Property ID is empty.", fullPropertyId.data()); + return MaterialPropertyId{}; } - else if (tokens.size() == 2) + + for (const auto& token : tokens) + { + if (!IsValidName(token)) + { + AZ_Error("MaterialPropertyId", false, "Property ID '%.*s' is not a valid identifier.", AZ_STRING_ARG(fullPropertyId)); + return MaterialPropertyId{}; + } + } + + MaterialPropertyId id; + id.m_fullName = fullPropertyId; + return id; + } + + MaterialPropertyId::MaterialPropertyId(AZStd::string_view propertyName) + { + if (!IsValidName(propertyName)) { - return MaterialPropertyId{tokens[0], tokens[1]}; + AZ_Error("MaterialPropertyId", false, "Property name '%.*s' is not a valid identifier.", AZ_STRING_ARG(propertyName)); } else { - AZ_Error("MaterialPropertyId", false, "Property ID '%s' is not a valid identifier.", fullPropertyId.data()); - return MaterialPropertyId{}; + m_fullName = propertyName; } } MaterialPropertyId::MaterialPropertyId(AZStd::string_view groupName, AZStd::string_view propertyName) - : MaterialPropertyId(Name{groupName}, Name{propertyName}) - { - } - - MaterialPropertyId::MaterialPropertyId(const Name& groupName, const Name& propertyName) { - AZ_Error("MaterialPropertyId", groupName.IsEmpty() || IsValidName(groupName), "Group name '%s' is not a valid identifier.", groupName.GetCStr()); - AZ_Error("MaterialPropertyId", IsValidName(propertyName), "Property name '%s' is not a valid identifier.", propertyName.GetCStr()); - m_groupName = groupName; - m_propertyName = propertyName; - if (groupName.IsEmpty()) + if (!IsValidName(groupName)) { - m_fullName = m_propertyName.GetStringView(); + AZ_Error("MaterialPropertyId", false, "Group name '%.*s' is not a valid identifier.", AZ_STRING_ARG(groupName)); + } + else if (!IsValidName(propertyName)) + { + AZ_Error("MaterialPropertyId", false, "Property name '%.*s' is not a valid identifier.", AZ_STRING_ARG(propertyName)); } else { - m_fullName = AZStd::string::format("%s.%s", m_groupName.GetCStr(), m_propertyName.GetCStr()); + m_fullName = AZStd::string::format("%.*s.%.*s", AZ_STRING_ARG(groupName), AZ_STRING_ARG(propertyName)); } } - - const Name& MaterialPropertyId::GetGroupName() const + + MaterialPropertyId::MaterialPropertyId(const Name& groupName, const Name& propertyName) + : MaterialPropertyId(groupName.GetStringView(), propertyName.GetStringView()) { - return m_groupName; } - - const Name& MaterialPropertyId::GetPropertyName() const + + MaterialPropertyId::MaterialPropertyId(const AZStd::array_view names) { - return m_propertyName; + for (const auto& name : names) + { + if (!IsValidName(name)) + { + AZ_Error("MaterialPropertyId", false, "'%s' is not a valid identifier.", name.c_str()); + return; + } + } + + AZStd::string fullName; + AzFramework::StringFunc::Join(fullName, names.begin(), names.end(), "."); + m_fullName = fullName; } - const Name& MaterialPropertyId::GetFullName() const + MaterialPropertyId::operator const Name&() const { return m_fullName; } 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 87c064f571..ddc2bc842b 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialTypeSourceData.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialTypeSourceData.cpp @@ -400,7 +400,7 @@ namespace AZ continue; } - materialTypeAssetCreator.BeginMaterialProperty(propertyId.GetFullName(), property.m_dataType); + materialTypeAssetCreator.BeginMaterialProperty(propertyId, property.m_dataType); if (property.m_dataType == MaterialPropertyDataType::Enum) { @@ -454,18 +454,18 @@ namespace AZ if (result == MaterialUtils::GetImageAssetResult::Missing) { materialTypeAssetCreator.ReportError( - "Material property '%s': Could not find the image '%s'", propertyId.GetFullName().GetCStr(), + "Material property '%s': Could not find the image '%s'", propertyId.GetCStr(), property.m_value.GetValue().data()); } else { - materialTypeAssetCreator.SetPropertyValue(propertyId.GetFullName(), imageAsset); + materialTypeAssetCreator.SetPropertyValue(propertyId, imageAsset); } } break; case MaterialPropertyDataType::Enum: { - MaterialPropertyIndex propertyIndex = materialTypeAssetCreator.GetMaterialPropertiesLayout()->FindPropertyIndex(propertyId.GetFullName()); + MaterialPropertyIndex propertyIndex = materialTypeAssetCreator.GetMaterialPropertiesLayout()->FindPropertyIndex(propertyId); const MaterialPropertyDescriptor* propertyDescriptor = materialTypeAssetCreator.GetMaterialPropertiesLayout()->GetPropertyDescriptor(propertyIndex); AZ::Name enumName = AZ::Name(property.m_value.GetValue()); @@ -476,12 +476,12 @@ namespace AZ } else { - materialTypeAssetCreator.SetPropertyValue(propertyId.GetFullName(), enumValue); + materialTypeAssetCreator.SetPropertyValue(propertyId, enumValue); } } break; default: - materialTypeAssetCreator.SetPropertyValue(propertyId.GetFullName(), property.m_value); + materialTypeAssetCreator.SetPropertyValue(propertyId, property.m_value); break; } } diff --git a/Gems/Atom/RPI/Code/Tests/Material/MaterialPropertyIdTests.cpp b/Gems/Atom/RPI/Code/Tests/Material/MaterialPropertyIdTests.cpp new file mode 100644 index 0000000000..2b729ed8ef --- /dev/null +++ b/Gems/Atom/RPI/Code/Tests/Material/MaterialPropertyIdTests.cpp @@ -0,0 +1,131 @@ +/* + * 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 +#include +#include + +namespace UnitTest +{ + using namespace AZ; + using namespace RPI; + + class MaterialPropertyIdTests + : public RPITestFixture + { + }; + + TEST_F(MaterialPropertyIdTests, TestConstructWithPropertyName) + { + MaterialPropertyId id{"color"}; + EXPECT_TRUE(id.IsValid()); + EXPECT_STREQ(id.GetCStr(), "color"); + AZ::Name idCastedToName = id; + EXPECT_EQ(idCastedToName, AZ::Name{"color"}); + } + + TEST_F(MaterialPropertyIdTests, TestConstructWithPropertyName_BadName) + { + ErrorMessageFinder errorMessageFinder; + errorMessageFinder.AddExpectedErrorMessage("not a valid identifier"); + + MaterialPropertyId id{"color?"}; + EXPECT_FALSE(id.IsValid()); + + errorMessageFinder.CheckExpectedErrorsFound(); + } + + TEST_F(MaterialPropertyIdTests, TestConstructWithTwoNames) + { + MaterialPropertyId id{"baseColor", "factor"}; + EXPECT_TRUE(id.IsValid()); + EXPECT_STREQ(id.GetCStr(), "baseColor.factor"); + AZ::Name idCastedToName = id; + EXPECT_EQ(idCastedToName, AZ::Name{"baseColor.factor"}); + } + + TEST_F(MaterialPropertyIdTests, TestConstructWithTwoNames_BadGroupName) + { + ErrorMessageFinder errorMessageFinder; + errorMessageFinder.AddExpectedErrorMessage("not a valid identifier"); + + MaterialPropertyId id{"layer1.baseColor", "factor"}; + EXPECT_FALSE(id.IsValid()); + + errorMessageFinder.CheckExpectedErrorsFound(); + } + + TEST_F(MaterialPropertyIdTests, TestConstructWithTwoNames_BadPropertyName) + { + ErrorMessageFinder errorMessageFinder; + errorMessageFinder.AddExpectedErrorMessage("not a valid identifier"); + + MaterialPropertyId id{"baseColor", ".factor"}; + EXPECT_FALSE(id.IsValid()); + + errorMessageFinder.CheckExpectedErrorsFound(); + } + + TEST_F(MaterialPropertyIdTests, TestConstructWithMultipleNames) + { + AZStd::vector names{"layer1", "clearCoat", "normal", "factor"}; + MaterialPropertyId id{names}; + EXPECT_TRUE(id.IsValid()); + EXPECT_STREQ(id.GetCStr(), "layer1.clearCoat.normal.factor"); + AZ::Name idCastedToName = id; + EXPECT_EQ(idCastedToName, AZ::Name{"layer1.clearCoat.normal.factor"}); + } + + TEST_F(MaterialPropertyIdTests, TestConstructWithMultipleNames_BadName) + { + ErrorMessageFinder errorMessageFinder; + errorMessageFinder.AddExpectedErrorMessage("not a valid identifier"); + + AZStd::vector names{"layer1", "clear-coat", "normal", "factor"}; + MaterialPropertyId id{names}; + EXPECT_FALSE(id.IsValid()); + + errorMessageFinder.CheckExpectedErrorsFound(); + } + + TEST_F(MaterialPropertyIdTests, TestParse) + { + MaterialPropertyId id = MaterialPropertyId::Parse("layer1.clearCoat.normal.factor"); + EXPECT_TRUE(id.IsValid()); + EXPECT_STREQ(id.GetCStr(), "layer1.clearCoat.normal.factor"); + AZ::Name idCastedToName = id; + EXPECT_EQ(idCastedToName, AZ::Name{"layer1.clearCoat.normal.factor"}); + } + + TEST_F(MaterialPropertyIdTests, TestParse_BadName) + { + ErrorMessageFinder errorMessageFinder; + errorMessageFinder.AddExpectedErrorMessage("not a valid identifier"); + + MaterialPropertyId id = MaterialPropertyId::Parse("layer1.clearCoat.normal,factor"); + EXPECT_FALSE(id.IsValid()); + + errorMessageFinder.CheckExpectedErrorsFound(); + } + + TEST_F(MaterialPropertyIdTests, TestNameValidity) + { + EXPECT_TRUE(MaterialPropertyId::IsValidName("a")); + EXPECT_TRUE(MaterialPropertyId::IsValidName("z")); + EXPECT_TRUE(MaterialPropertyId::IsValidName("A")); + EXPECT_TRUE(MaterialPropertyId::IsValidName("Z")); + EXPECT_TRUE(MaterialPropertyId::IsValidName("_")); + EXPECT_TRUE(MaterialPropertyId::IsValidName("m_layer10bazBAZ")); + EXPECT_FALSE(MaterialPropertyId::IsValidName("")); + EXPECT_FALSE(MaterialPropertyId::IsValidName("1layer")); + EXPECT_FALSE(MaterialPropertyId::IsValidName("base-color")); + EXPECT_FALSE(MaterialPropertyId::IsValidName("base.color")); + EXPECT_FALSE(MaterialPropertyId::IsValidName("base/color")); + } +} diff --git a/Gems/Atom/RPI/Code/atom_rpi_tests_files.cmake b/Gems/Atom/RPI/Code/atom_rpi_tests_files.cmake index 5d67948ac8..57ee71e412 100644 --- a/Gems/Atom/RPI/Code/atom_rpi_tests_files.cmake +++ b/Gems/Atom/RPI/Code/atom_rpi_tests_files.cmake @@ -37,6 +37,7 @@ set(FILES Tests/Material/MaterialSourceDataTests.cpp Tests/Material/MaterialFunctorTests.cpp Tests/Material/MaterialFunctorSourceDataSerializerTests.cpp + Tests/Material/MaterialPropertyIdTests.cpp Tests/Material/MaterialPropertyValueSourceDataTests.cpp Tests/Material/MaterialTests.cpp Tests/Model/ModelTests.cpp diff --git a/Gems/Atom/Tools/MaterialEditor/Code/Source/Document/MaterialDocument.cpp b/Gems/Atom/Tools/MaterialEditor/Code/Source/Document/MaterialDocument.cpp index 641d586ea7..8f25e41827 100644 --- a/Gems/Atom/Tools/MaterialEditor/Code/Source/Document/MaterialDocument.cpp +++ b/Gems/Atom/Tools/MaterialEditor/Code/Source/Document/MaterialDocument.cpp @@ -589,7 +589,7 @@ namespace MaterialEditor const MaterialPropertyId propertyId(groupName, propertyName); - const auto it = m_properties.find(propertyId.GetFullName()); + const auto it = m_properties.find(propertyId); if (it != m_properties.end() && propertyFilter(it->second)) { MaterialPropertyValue propertyValue = AtomToolsFramework::ConvertToRuntimeType(it->second.GetValue()); @@ -597,7 +597,7 @@ namespace MaterialEditor { if (!AtomToolsFramework::ConvertToExportFormat(exportPath, propertyId.GetFullName(), propertyDefinition, propertyValue)) { - AZ_Error("MaterialDocument", false, "Material document property could not be converted: '%s' in '%s'.", propertyId.GetFullName().GetCStr(), m_absolutePath.c_str()); + AZ_Error("MaterialDocument", false, "Material document property could not be converted: '%s' in '%s'.", propertyId.GetCStr(), m_absolutePath.c_str()); result = false; return false; } @@ -783,7 +783,7 @@ namespace MaterialEditor AtomToolsFramework::DynamicPropertyConfig propertyConfig; // Assign id before conversion so it can be used in dynamic description - propertyConfig.m_id = MaterialPropertyId(groupName, propertyName).GetCStr(); + propertyConfig.m_id = MaterialPropertyId(groupName, propertyName); const auto& propertyIndex = m_materialAsset->GetMaterialPropertiesLayout()->FindPropertyIndex(propertyConfig.m_id); const bool propertyIndexInBounds = propertyIndex.IsValid() && propertyIndex.GetIndex() < m_materialAsset->GetPropertyValues().size(); @@ -854,7 +854,7 @@ namespace MaterialEditor propertyConfig = {}; propertyConfig.m_dataType = AtomToolsFramework::DynamicPropertyType::String; - propertyConfig.m_id = MaterialPropertyId(UvGroupName, shaderInput).GetCStr(); + propertyConfig.m_id = MaterialPropertyId(UvGroupName, shaderInput); propertyConfig.m_name = shaderInput; propertyConfig.m_displayName = shaderInput; propertyConfig.m_groupName = "UV Sets"; diff --git a/Gems/Atom/Tools/MaterialEditor/Code/Source/Window/MaterialInspector/MaterialInspector.cpp b/Gems/Atom/Tools/MaterialEditor/Code/Source/Window/MaterialInspector/MaterialInspector.cpp index 7790b9bef5..0af051894d 100644 --- a/Gems/Atom/Tools/MaterialEditor/Code/Source/Window/MaterialInspector/MaterialInspector.cpp +++ b/Gems/Atom/Tools/MaterialEditor/Code/Source/Window/MaterialInspector/MaterialInspector.cpp @@ -152,7 +152,7 @@ namespace MaterialEditor AtomToolsFramework::DynamicProperty property; AtomToolsFramework::AtomToolsDocumentRequestBus::EventResult( property, m_documentId, &AtomToolsFramework::AtomToolsDocumentRequestBus::Events::GetProperty, - AZ::RPI::MaterialPropertyId(groupName, uvNamePair.m_shaderInput.ToString()).GetFullName()); + AZ::RPI::MaterialPropertyId(groupName, uvNamePair.m_shaderInput.ToString())); group.m_properties.push_back(property); property.SetValue(property.GetConfig().m_parentValue); @@ -189,7 +189,7 @@ namespace MaterialEditor AtomToolsFramework::DynamicProperty property; AtomToolsFramework::AtomToolsDocumentRequestBus::EventResult( property, m_documentId, &AtomToolsFramework::AtomToolsDocumentRequestBus::Events::GetProperty, - AZ::RPI::MaterialPropertyId(groupName, propertyDefinition.m_name).GetFullName()); + AZ::RPI::MaterialPropertyId(groupName, propertyDefinition.m_name)); group.m_properties.push_back(property); } } diff --git a/Gems/AtomLyIntegration/CommonFeatures/Code/Source/Material/EditorMaterialComponentInspector.cpp b/Gems/AtomLyIntegration/CommonFeatures/Code/Source/Material/EditorMaterialComponentInspector.cpp index bfa9f1d36f..b20fe5802d 100644 --- a/Gems/AtomLyIntegration/CommonFeatures/Code/Source/Material/EditorMaterialComponentInspector.cpp +++ b/Gems/AtomLyIntegration/CommonFeatures/Code/Source/Material/EditorMaterialComponentInspector.cpp @@ -310,7 +310,7 @@ namespace AZ AtomToolsFramework::DynamicPropertyConfig propertyConfig; // Assign id before conversion so it can be used in dynamic description - propertyConfig.m_id = AZ::RPI::MaterialPropertyId(groupName, propertyDefinition.m_name).GetFullName(); + propertyConfig.m_id = AZ::RPI::MaterialPropertyId(groupName, propertyDefinition.m_name); AtomToolsFramework::ConvertToPropertyConfig(propertyConfig, propertyDefinition); diff --git a/Gems/AtomLyIntegration/CommonFeatures/Code/Source/Material/EditorMaterialComponentUtil.cpp b/Gems/AtomLyIntegration/CommonFeatures/Code/Source/Material/EditorMaterialComponentUtil.cpp index d3e125426c..f46876fdfb 100644 --- a/Gems/AtomLyIntegration/CommonFeatures/Code/Source/Material/EditorMaterialComponentUtil.cpp +++ b/Gems/AtomLyIntegration/CommonFeatures/Code/Source/Material/EditorMaterialComponentUtil.cpp @@ -116,7 +116,7 @@ namespace AZ editData.m_materialTypeSourceData.EnumerateProperties([&](const AZStd::string& groupName, const AZStd::string& propertyName, const auto& propertyDefinition){ const AZ::RPI::MaterialPropertyId propertyId(groupName, propertyName); const AZ::RPI::MaterialPropertyIndex propertyIndex = - editData.m_materialAsset->GetMaterialPropertiesLayout()->FindPropertyIndex(propertyId.GetFullName()); + editData.m_materialAsset->GetMaterialPropertiesLayout()->FindPropertyIndex(propertyId); AZ::RPI::MaterialPropertyValue propertyValue = editData.m_materialAsset->GetPropertyValues()[propertyIndex.GetIndex()]; @@ -128,7 +128,7 @@ namespace AZ } // Check for and apply any property overrides before saving property values - auto propertyOverrideItr = editData.m_materialPropertyOverrideMap.find(propertyId.GetFullName()); + auto propertyOverrideItr = editData.m_materialPropertyOverrideMap.find(propertyId); if (propertyOverrideItr != editData.m_materialPropertyOverrideMap.end()) { propertyValue = AZ::RPI::MaterialPropertyValue::FromAny(propertyOverrideItr->second); diff --git a/Gems/AtomLyIntegration/CommonFeatures/Code/Source/Material/EditorMaterialModelUvNameMapInspector.cpp b/Gems/AtomLyIntegration/CommonFeatures/Code/Source/Material/EditorMaterialModelUvNameMapInspector.cpp index 0cf6410fc5..53d7980fdc 100644 --- a/Gems/AtomLyIntegration/CommonFeatures/Code/Source/Material/EditorMaterialModelUvNameMapInspector.cpp +++ b/Gems/AtomLyIntegration/CommonFeatures/Code/Source/Material/EditorMaterialModelUvNameMapInspector.cpp @@ -96,7 +96,7 @@ namespace AZ const AZStd::string materialUvName = m_materialUvNames[i].m_uvName.GetStringView(); propertyConfig.m_dataType = AtomToolsFramework::DynamicPropertyType::Enum; - propertyConfig.m_id = AZ::RPI::MaterialPropertyId(groupName, shaderInput).GetFullName(); + propertyConfig.m_id = AZ::RPI::MaterialPropertyId(groupName, shaderInput); propertyConfig.m_name = shaderInput; propertyConfig.m_displayName = materialUvName; propertyConfig.m_description = shaderInput; @@ -248,7 +248,7 @@ namespace AZ const AZStd::string materialUvName = m_materialUvNames[i].m_uvName.GetStringView(); propertyConfig.m_dataType = AtomToolsFramework::DynamicPropertyType::Enum; - propertyConfig.m_id = AZ::RPI::MaterialPropertyId(groupName, shaderInput).GetFullName(); + propertyConfig.m_id = AZ::RPI::MaterialPropertyId(groupName, shaderInput); propertyConfig.m_name = shaderInput; propertyConfig.m_displayName = materialUvName; propertyConfig.m_description = shaderInput;