From d9c646062b0521cc24cb54b120c1a17a9a726072 Mon Sep 17 00:00:00 2001 From: santorac <55155825+santorac@users.noreply.github.com> Date: Fri, 28 Jan 2022 13:45:58 -0800 Subject: [PATCH 01/14] Instrumented support for the JSON importer for material type files. I also noticed that JsonFileLoadContext was no longer used (see https://github.com/o3de/o3de/pull/7010) so I was able to remove all that code. Signed-off-by: santorac <55155825+santorac@users.noreply.github.com> --- .../RPI.Edit/Common/JsonFileLoadContext.h | 36 ----------- .../Include/Atom/RPI.Edit/Common/JsonUtils.h | 5 -- .../Atom/RPI.Edit/Material/MaterialUtils.h | 8 +-- .../RPI.Builders/Material/MaterialBuilder.cpp | 6 -- .../RPI.Edit/Common/JsonFileLoadContext.cpp | 39 ----------- .../MaterialFunctorSourceDataSerializer.cpp | 1 - .../RPI.Edit/Material/MaterialSourceData.cpp | 1 - .../RPI.Edit/Material/MaterialUtils.cpp | 25 ++++---- .../RPI/Code/Tests/Common/JsonTestUtils.h | 12 +--- .../Material/MaterialTypeSourceDataTests.cpp | 64 ++++++++++++++++++- Gems/Atom/RPI/Code/atom_rpi_edit_files.cmake | 2 - 11 files changed, 84 insertions(+), 115 deletions(-) delete mode 100644 Gems/Atom/RPI/Code/Include/Atom/RPI.Edit/Common/JsonFileLoadContext.h delete mode 100644 Gems/Atom/RPI/Code/Source/RPI.Edit/Common/JsonFileLoadContext.cpp diff --git a/Gems/Atom/RPI/Code/Include/Atom/RPI.Edit/Common/JsonFileLoadContext.h b/Gems/Atom/RPI/Code/Include/Atom/RPI.Edit/Common/JsonFileLoadContext.h deleted file mode 100644 index 90267010a7..0000000000 --- a/Gems/Atom/RPI/Code/Include/Atom/RPI.Edit/Common/JsonFileLoadContext.h +++ /dev/null @@ -1,36 +0,0 @@ -/* - * 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 - { - //! Settings for custom JSON serializers to get context about loading a file - class JsonFileLoadContext final - { - public: - AZ_TYPE_INFO(JsonFileLoadContext, "{314942B3-A74A-49D2-822D-CD56F8E3C0F8}"); - - void PushFilePath(AZStd::string path); - AZStd::string_view GetFilePath() const; - void PopFilePath(); - - private: - // Using vector instead of stack because stack doesn't have a copy constructor - AZStd::vector m_thisFilePath; - }; - - } // namespace RPI -} // namespace AZ diff --git a/Gems/Atom/RPI/Code/Include/Atom/RPI.Edit/Common/JsonUtils.h b/Gems/Atom/RPI/Code/Include/Atom/RPI.Edit/Common/JsonUtils.h index 9f4cdcf31b..5550013038 100644 --- a/Gems/Atom/RPI/Code/Include/Atom/RPI.Edit/Common/JsonUtils.h +++ b/Gems/Atom/RPI/Code/Include/Atom/RPI.Edit/Common/JsonUtils.h @@ -11,7 +11,6 @@ #include #include #include -#include #include namespace AZ @@ -52,13 +51,9 @@ namespace AZ rapidjson::Document& document = loadOutcome.GetValue(); - AZ::RPI::JsonFileLoadContext fileLoadContext; - fileLoadContext.PushFilePath(path); - AZ::JsonDeserializerSettings jsonSettings; AZ::RPI::JsonReportingHelper reportingHelper; reportingHelper.Attach(jsonSettings); - jsonSettings.m_metadata.Add(AZStd::move(fileLoadContext)); AZ::JsonSerialization::Load(objectData, document, jsonSettings); if (reportingHelper.ErrorsReported()) diff --git a/Gems/Atom/RPI/Code/Include/Atom/RPI.Edit/Material/MaterialUtils.h b/Gems/Atom/RPI/Code/Include/Atom/RPI.Edit/Material/MaterialUtils.h index 2fb55e5f40..6ab7132109 100644 --- a/Gems/Atom/RPI/Code/Include/Atom/RPI.Edit/Material/MaterialUtils.h +++ b/Gems/Atom/RPI/Code/Include/Atom/RPI.Edit/Material/MaterialUtils.h @@ -48,11 +48,11 @@ namespace AZ //! @return if resolving is successful. An error will be reported if it fails. bool ResolveMaterialPropertyEnumValue(const MaterialPropertyDescriptor* propertyDescriptor, const AZ::Name& enumName, MaterialPropertyValue& outResolvedValue); - //! Load material type from a json file. If the file path is relative, the loaded json document must be provided. + //! Load material type from a json file or document. //! Otherwise, it will use the passed in document first if not null, or load the json document from the path. - //! @param filePath a relative path if document is provided, an absolute path if document is not provided. - //! @param document the loaded json document. - AZ::Outcome LoadMaterialTypeSourceData(const AZStd::string& filePath, const rapidjson::Value* document = nullptr); + //! @param filePath path to the JSON file to load, unless the @document is already provided. In either case, this path will be used to resolve any relative file references. + //! @param document an optional already loaded json document. + AZ::Outcome LoadMaterialTypeSourceData(const AZStd::string& filePath, rapidjson::Document* document = nullptr); //! Utility function for custom JSON serializers to report results as "Skipped" when encountering keys that aren't recognized //! as part of the custom format. 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 d55f202c03..c81b91d777 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Builders/Material/MaterialBuilder.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Builders/Material/MaterialBuilder.cpp @@ -11,7 +11,6 @@ #include #include #include -#include #include #include #include @@ -139,11 +138,6 @@ namespace AZ JsonReportingHelper reportingHelper; reportingHelper.Attach(settings); - // This is required by some custom material serializers to support relative path references. - JsonFileLoadContext fileLoadContext; - fileLoadContext.PushFilePath(filePath); - settings.m_metadata.Add(fileLoadContext); - JsonSerialization::Load(material, value, settings); if (reportingHelper.ErrorsReported()) diff --git a/Gems/Atom/RPI/Code/Source/RPI.Edit/Common/JsonFileLoadContext.cpp b/Gems/Atom/RPI/Code/Source/RPI.Edit/Common/JsonFileLoadContext.cpp deleted file mode 100644 index fb5f7b09a9..0000000000 --- a/Gems/Atom/RPI/Code/Source/RPI.Edit/Common/JsonFileLoadContext.cpp +++ /dev/null @@ -1,39 +0,0 @@ -/* - * 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 - -namespace AZ -{ - namespace RPI - { - // Note, we use string not string_view on purpose because m_thisFilePath.push_back() will make a new string anyway. - void JsonFileLoadContext::PushFilePath(AZStd::string path) - { - m_thisFilePath.push_back(AZStd::move(path)); - } - - AZStd::string_view JsonFileLoadContext::GetFilePath() const - { - if (m_thisFilePath.empty()) - { - return ""; - } - else - { - return m_thisFilePath.back(); - } - } - - void JsonFileLoadContext::PopFilePath() - { - m_thisFilePath.pop_back(); - } - - } // namespace RPI -} // namespace AZ diff --git a/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialFunctorSourceDataSerializer.cpp b/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialFunctorSourceDataSerializer.cpp index a944a35162..ceb7093362 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialFunctorSourceDataSerializer.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialFunctorSourceDataSerializer.cpp @@ -9,7 +9,6 @@ #include #include #include -#include #include #include 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 ae7889aa93..857e518e4e 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialSourceData.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialSourceData.cpp @@ -14,7 +14,6 @@ #include #include -#include #include #include diff --git a/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialUtils.cpp b/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialUtils.cpp index b03771173f..be5feccb80 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialUtils.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialUtils.cpp @@ -14,11 +14,11 @@ #include #include #include -#include #include #include #include #include +#include #include #include @@ -73,21 +73,29 @@ namespace AZ outResolvedValue = enumValue; return true; } - - AZ::Outcome LoadMaterialTypeSourceData(const AZStd::string& filePath, const rapidjson::Value* document) + + AZ::Outcome LoadMaterialTypeSourceData(const AZStd::string& filePath, rapidjson::Document* document) { - AZ::Outcome loadOutcome; + rapidjson::Document localDocument; + if (document == nullptr) { - loadOutcome = AZ::JsonSerializationUtils::ReadJsonFile(filePath, AZ::RPI::JsonUtils::DefaultMaxFileSize); + AZ::Outcome loadOutcome = AZ::JsonSerializationUtils::ReadJsonFile(filePath, AZ::RPI::JsonUtils::DefaultMaxFileSize); if (!loadOutcome.IsSuccess()) { AZ_Error("AZ::RPI::JsonUtils", false, "%s", loadOutcome.GetError().c_str()); return AZ::Failure(); } - document = &loadOutcome.GetValue(); + localDocument = loadOutcome.TakeValue(); + document = &localDocument; } + + AZ::BaseJsonImporter jsonImporter; + AZ::JsonImportSettings importSettings; + importSettings.m_importer = &jsonImporter; + importSettings.m_loadedJsonPath = filePath; + AZ::JsonSerializationResult::ResultCode result = AZ::JsonSerialization::ResolveImports(document->GetObject(), document->GetAllocator(), importSettings); MaterialTypeSourceData materialType; @@ -96,11 +104,6 @@ namespace AZ JsonReportingHelper reportingHelper; reportingHelper.Attach(settings); - // This is required by some custom material serializers to support relative path references. - JsonFileLoadContext fileLoadContext; - fileLoadContext.PushFilePath(filePath); - settings.m_metadata.Add(fileLoadContext); - JsonSerialization::Load(materialType, *document, settings); materialType.ConvertToNewDataFormat(); materialType.ResolveUvEnums(); diff --git a/Gems/Atom/RPI/Code/Tests/Common/JsonTestUtils.h b/Gems/Atom/RPI/Code/Tests/Common/JsonTestUtils.h index 56d537f236..94616b23e9 100644 --- a/Gems/Atom/RPI/Code/Tests/Common/JsonTestUtils.h +++ b/Gems/Atom/RPI/Code/Tests/Common/JsonTestUtils.h @@ -16,7 +16,6 @@ #include #include #include -#include namespace UnitTest { @@ -46,17 +45,12 @@ namespace UnitTest //! Uses JsonSerialization to load JSON data into a reflected object template - JsonTestResult LoadTestDataFromJson(T& object, rapidjson::Value& json, AZ::RPI::JsonFileLoadContext* jsonFileLoadContext = nullptr) + JsonTestResult LoadTestDataFromJson(T& object, rapidjson::Value& json) { JsonTestResult result; AZ::JsonDeserializerSettings settings; - if (jsonFileLoadContext) - { - settings.m_metadata.Add(*jsonFileLoadContext); - } - settings.m_reporting = [&result](AZStd::string_view message, AZ::JsonSerializationResult::ResultCode resultCode, AZStd::string_view path) { JsonTestResult::Report report; @@ -74,14 +68,14 @@ namespace UnitTest //! Uses JsonSerialization to load JSON data from a string into a reflected object template - JsonTestResult LoadTestDataFromJson(T& object, AZStd::string_view jsonText, AZ::RPI::JsonFileLoadContext* jsonFileLoadContext = nullptr) + JsonTestResult LoadTestDataFromJson(T& object, AZStd::string_view jsonText) { auto parseResult = AZ::JsonSerializationUtils::ReadJsonString(jsonText); EXPECT_TRUE(parseResult.IsSuccess()) << parseResult.GetError().c_str(); if (parseResult.IsSuccess()) { - return LoadTestDataFromJson(object, parseResult.GetValue(), jsonFileLoadContext); + return LoadTestDataFromJson(object, parseResult.GetValue()); } else { diff --git a/Gems/Atom/RPI/Code/Tests/Material/MaterialTypeSourceDataTests.cpp b/Gems/Atom/RPI/Code/Tests/Material/MaterialTypeSourceDataTests.cpp index 206073e96a..e0fa16c5f4 100644 --- a/Gems/Atom/RPI/Code/Tests/Material/MaterialTypeSourceDataTests.cpp +++ b/Gems/Atom/RPI/Code/Tests/Material/MaterialTypeSourceDataTests.cpp @@ -16,11 +16,13 @@ #include #include #include +#include #include #include #include #include +#include #include #include #include @@ -35,6 +37,7 @@ namespace UnitTest { protected: + AZ::IO::FixedMaxPath m_tempFolder; RHI::Ptr m_testMaterialSrgLayout; Data::Asset m_testShaderAsset; Data::Asset m_testShaderAsset2; @@ -329,6 +332,9 @@ namespace UnitTest AZStd::string testImageFilepathAbsolute(TestImageFilepathAbsolute); AzFramework::StringFunc::Path::Normalize(testImageFilepathAbsolute); m_assetSystemStub.RegisterSourceInfo(testImageFilepathAbsolute.c_str(), testImageAssetInfo2, ""); + + m_tempFolder = AZ::Utils::GetExecutableDirectory(); + m_tempFolder = m_tempFolder/"temp"/"MaterialTypeSourceDataTest"; } void TearDown() override @@ -1473,6 +1479,8 @@ namespace UnitTest { // Note that serialization of individual fields within material properties is thoroughly tested in // MaterialPropertySerializerTests, so the sample property data used here is cursory. + // We also don't cover fields related to providing name contexts for nested property groups, like + // "shaderInputPrefix" and "shaderOptionPrefix" as those are covered in CreateMaterialTypeAsset_NestedGroups*. const AZStd::string inputJson = R"( { @@ -1701,7 +1709,7 @@ namespace UnitTest JsonTestResult storeResult = StoreTestDataToJson(material, outputJson); ExpectSimilarJson(inputJson, outputJson); } - + TEST_F(MaterialTypeSourceDataTests, LoadAllFieldsUsingOldFormat) { // The content of this test was copied from LoadAndStoreJson_AllFields to prove backward compatibility. @@ -1963,4 +1971,58 @@ namespace UnitTest errorMessageFinder.CheckExpectedErrorsFound(); } + + TEST_F(MaterialTypeSourceDataTests, LoadWithImportedJson) + { + const AZStd::string propertyGroupJson = R"( + { + "name": "myGroup", + "displayName": "My Group", + "description": "This group is defined in a separate JSON file", + "properties": [ + { + "name": "foo", + "type": "Bool" + }, + { + "name": "bar", + "type": "Float" + } + ] + } + )"; + + IO::FixedMaxPath propertyGroupJsonFilePath = m_tempFolder/"MyPropertyGroup.json"; + AZ::Utils::WriteFile(propertyGroupJson, propertyGroupJsonFilePath.c_str()); + + const AZStd::string materialTypeJson = R"( + { + "propertyLayout": { + "propertyGroups": [ + { "$import": "MyPropertyGroup.json" } + ] + } + } + )"; + + IO::FixedMaxPath materialTypeJsonFilePath = m_tempFolder/"TestImport.materialtype"; + AZ::Utils::WriteFile(materialTypeJson, materialTypeJsonFilePath.c_str()); + + auto loadMaterialTypeResult = MaterialUtils::LoadMaterialTypeSourceData(materialTypeJsonFilePath.c_str()); + EXPECT_TRUE(loadMaterialTypeResult); + MaterialTypeSourceData materialType = loadMaterialTypeResult.TakeValue(); + + EXPECT_EQ(materialType.GetPropertyLayout().m_propertyGroups.size(), 1); + EXPECT_TRUE(materialType.FindPropertyGroup("myGroup") != nullptr); + EXPECT_EQ(materialType.FindPropertyGroup("myGroup")->GetDisplayName(), "My Group"); + EXPECT_EQ(materialType.FindPropertyGroup("myGroup")->GetDescription(), "This group is defined in a separate JSON file"); + EXPECT_EQ(materialType.FindPropertyGroup("myGroup")->GetProperties().size(), 2); + EXPECT_NE(materialType.FindProperty("myGroup.foo"), nullptr); + EXPECT_NE(materialType.FindProperty("myGroup.bar"), nullptr); + EXPECT_EQ(materialType.FindProperty("myGroup.foo")->GetName(), "foo"); + EXPECT_EQ(materialType.FindProperty("myGroup.bar")->GetName(), "bar"); + EXPECT_EQ(materialType.FindProperty("myGroup.foo")->m_dataType, MaterialPropertyDataType::Bool); + EXPECT_EQ(materialType.FindProperty("myGroup.bar")->m_dataType, MaterialPropertyDataType::Float); + } + } diff --git a/Gems/Atom/RPI/Code/atom_rpi_edit_files.cmake b/Gems/Atom/RPI/Code/atom_rpi_edit_files.cmake index 3c345cc00b..84d423127a 100644 --- a/Gems/Atom/RPI/Code/atom_rpi_edit_files.cmake +++ b/Gems/Atom/RPI/Code/atom_rpi_edit_files.cmake @@ -11,7 +11,6 @@ set(FILES Include/Atom/RPI.Edit/Common/AssetAliasesSourceData.h Include/Atom/RPI.Edit/Common/ColorUtils.h Include/Atom/RPI.Edit/Common/ConvertibleSource.h - Include/Atom/RPI.Edit/Common/JsonFileLoadContext.h Include/Atom/RPI.Edit/Common/JsonReportingHelper.h Include/Atom/RPI.Edit/Common/JsonUtils.h Include/Atom/RPI.Edit/Material/LuaMaterialFunctorSourceData.h @@ -56,7 +55,6 @@ set(FILES Source/RPI.Edit/Common/AssetAliasesSourceData.cpp Source/RPI.Edit/Common/ColorUtils.cpp Source/RPI.Edit/Common/ConvertibleSource.cpp - Source/RPI.Edit/Common/JsonFileLoadContext.cpp Source/RPI.Edit/Common/JsonReportingHelper.cpp Source/RPI.Edit/Common/JsonUtils.cpp ) From 90e34e86496ff5540fad191da102bd2b8ea3f743 Mon Sep 17 00:00:00 2001 From: santorac <55155825+santorac@users.noreply.github.com> Date: Fri, 28 Jan 2022 13:51:42 -0800 Subject: [PATCH 02/14] Fixed a couple places where we had variables called 'propertyNameContext' instead of 'propertyIdContext' which was inconsistent Signed-off-by: santorac <55155825+santorac@users.noreply.github.com> --- .../Material/MaterialTypeSourceData.h | 4 +-- .../Material/MaterialTypeSourceData.cpp | 32 +++++++++---------- 2 files changed, 18 insertions(+), 18 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 40f82c8ec7..df8b7c4997 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 @@ -320,13 +320,13 @@ namespace AZ //! Recursively populates a material asset with properties from the tree of material property groups. //! @param materialTypeSourceFilePath path to the material type file that is being processed, used to look up relative paths - //! @param propertyNameContext the accumulated prefix that should be applied to any property names encountered in the current @propertyGroup + //! @param propertyIdContext the accumulated prefix that should be applied to any property names encountered in the current @propertyGroup //! @param propertyGroup the current PropertyGroup that is being processed //! @return false if errors are detected and processing should abort bool BuildPropertyList( const AZStd::string& materialTypeSourceFilePath, MaterialTypeAssetCreator& materialTypeAssetCreator, - AZStd::vector& propertyNameContext, + AZStd::vector& propertyIdContext, const MaterialTypeSourceData::PropertyGroup* propertyGroup) const; //! Construct a complete list of group definitions, including implicit groups, arranged in the same order as the source data. 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 b9a5754732..b7808abf17 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialTypeSourceData.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialTypeSourceData.cpp @@ -376,18 +376,18 @@ namespace AZ return parts; } - bool MaterialTypeSourceData::EnumeratePropertyGroups(const EnumeratePropertyGroupsCallback& callback, AZStd::string propertyNameContext, const AZStd::vector>& inPropertyGroupList) const + bool MaterialTypeSourceData::EnumeratePropertyGroups(const EnumeratePropertyGroupsCallback& callback, AZStd::string propertyIdContext, const AZStd::vector>& inPropertyGroupList) const { for (auto& propertyGroup : inPropertyGroupList) { - if (!callback(propertyNameContext, propertyGroup.get())) + if (!callback(propertyIdContext, propertyGroup.get())) { return false; // Stop processing } - const AZStd::string propertyNameContext2 = propertyNameContext + propertyGroup->m_name + "."; + const AZStd::string propertyIdContext2 = propertyIdContext + propertyGroup->m_name + "."; - if (!EnumeratePropertyGroups(callback, propertyNameContext2, propertyGroup->m_propertyGroups)) + if (!EnumeratePropertyGroups(callback, propertyIdContext2, propertyGroup->m_propertyGroups)) { return false; // Stop processing } @@ -406,22 +406,22 @@ namespace AZ return EnumeratePropertyGroups(callback, {}, m_propertyLayout.m_propertyGroups); } - bool MaterialTypeSourceData::EnumerateProperties(const EnumeratePropertiesCallback& callback, AZStd::string propertyNameContext, const AZStd::vector>& inPropertyGroupList) const + bool MaterialTypeSourceData::EnumerateProperties(const EnumeratePropertiesCallback& callback, AZStd::string propertyIdContext, const AZStd::vector>& inPropertyGroupList) const { for (auto& propertyGroup : inPropertyGroupList) { - const AZStd::string propertyNameContext2 = propertyNameContext + propertyGroup->m_name + "."; + const AZStd::string propertyIdContext2 = propertyIdContext + propertyGroup->m_name + "."; for (auto& property : propertyGroup->m_properties) { - if (!callback(propertyNameContext2, property.get())) + if (!callback(propertyIdContext2, property.get())) { return false; // Stop processing } } - if (!EnumerateProperties(callback, propertyNameContext2, propertyGroup->m_propertyGroups)) + if (!EnumerateProperties(callback, propertyIdContext2, propertyGroup->m_propertyGroups)) { return false; // Stop processing } @@ -532,14 +532,14 @@ namespace AZ bool MaterialTypeSourceData::BuildPropertyList( const AZStd::string& materialTypeSourceFilePath, MaterialTypeAssetCreator& materialTypeAssetCreator, - AZStd::vector& propertyNameContext, + AZStd::vector& propertyIdContext, const MaterialTypeSourceData::PropertyGroup* propertyGroup) const { for (const AZStd::unique_ptr& property : propertyGroup->m_properties) { // Register the property... - MaterialPropertyId propertyId{propertyNameContext, property->GetName()}; + MaterialPropertyId propertyId{propertyIdContext, property->GetName()}; if (!propertyId.IsValid()) { @@ -652,15 +652,15 @@ namespace AZ for (const AZStd::unique_ptr& propertySubset : propertyGroup->m_propertyGroups) { - propertyNameContext.push_back(propertySubset->m_name); + propertyIdContext.push_back(propertySubset->m_name); bool success = BuildPropertyList( materialTypeSourceFilePath, materialTypeAssetCreator, - propertyNameContext, + propertyIdContext, propertySubset.get()); - propertyNameContext.pop_back(); + propertyIdContext.pop_back(); if (!success) { @@ -797,9 +797,9 @@ namespace AZ for (const AZStd::unique_ptr& propertyGroup : m_propertyLayout.m_propertyGroups) { - AZStd::vector propertyNameContext; - propertyNameContext.push_back(propertyGroup->m_name); - bool success = BuildPropertyList(materialTypeSourceFilePath, materialTypeAssetCreator, propertyNameContext, propertyGroup.get()); + AZStd::vector propertyIdContext; + propertyIdContext.push_back(propertyGroup->m_name); + bool success = BuildPropertyList(materialTypeSourceFilePath, materialTypeAssetCreator, propertyIdContext, propertyGroup.get()); if (!success) { From a352807c87c891139bb57c396b2613504c1fef3f Mon Sep 17 00:00:00 2001 From: santorac <55155825+santorac@users.noreply.github.com> Date: Tue, 1 Feb 2022 09:56:27 -0800 Subject: [PATCH 03/14] Renamed MaterialFunctor code to use propertyId instead of propertyName, because 'name' is us for the short name of a property and 'ID' is for the full name including groups. Signed-off-by: santorac <55155825+santorac@users.noreply.github.com> --- .../RPI.Reflect/Material/MaterialFunctor.h | 28 ++--- .../RPI.Reflect/Material/MaterialFunctor.cpp | 106 +++++++++--------- 2 files changed, 67 insertions(+), 67 deletions(-) diff --git a/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialFunctor.h b/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialFunctor.h index 09d10bf538..92a9ab789a 100644 --- a/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialFunctor.h +++ b/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialFunctor.h @@ -91,11 +91,11 @@ namespace AZ //! Get the property value. The type must be one of those in MaterialPropertyValue. //! Otherwise, a compile error will be reported. template - const Type& GetMaterialPropertyValue(const Name& propertyName) const; + const Type& GetMaterialPropertyValue(const Name& propertyId) const; template const Type& GetMaterialPropertyValue(const MaterialPropertyIndex& index) const; //! Get the property value. GetMaterialPropertyValue() is equivalent to GetMaterialPropertyValue().GetValue(). - const MaterialPropertyValue& GetMaterialPropertyValue(const Name& propertyName) const; + const MaterialPropertyValue& GetMaterialPropertyValue(const Name& propertyId) const; const MaterialPropertyValue& GetMaterialPropertyValue(const MaterialPropertyIndex& index) const; const MaterialPropertiesLayout* GetMaterialPropertiesLayout() const { return m_materialPropertiesLayout.get(); } @@ -164,19 +164,19 @@ namespace AZ { friend class LuaMaterialFunctorEditorContext; public: - const MaterialPropertyDynamicMetadata* GetMaterialPropertyMetadata(const Name& propertyName) const; + const MaterialPropertyDynamicMetadata* GetMaterialPropertyMetadata(const Name& propertyId) const; const MaterialPropertyDynamicMetadata* GetMaterialPropertyMetadata(const MaterialPropertyIndex& index) const; - const MaterialPropertyGroupDynamicMetadata* GetMaterialPropertyGroupMetadata(const Name& propertyName) const; + const MaterialPropertyGroupDynamicMetadata* GetMaterialPropertyGroupMetadata(const Name& propertyId) const; //! Get the property value. The type must be one of those in MaterialPropertyValue. //! Otherwise, a compile error will be reported. template - const Type& GetMaterialPropertyValue(const Name& propertyName) const; + const Type& GetMaterialPropertyValue(const Name& propertyId) const; template const Type& GetMaterialPropertyValue(const MaterialPropertyIndex& index) const; //! Get the property value. GetMaterialPropertyValue() is equivalent to GetMaterialPropertyValue().GetValue(). - const MaterialPropertyValue& GetMaterialPropertyValue(const Name& propertyName) const; + const MaterialPropertyValue& GetMaterialPropertyValue(const Name& propertyId) const; const MaterialPropertyValue& GetMaterialPropertyValue(const MaterialPropertyIndex& index) const; const MaterialPropertiesLayout* GetMaterialPropertiesLayout() const { return m_materialPropertiesLayout.get(); } @@ -184,22 +184,22 @@ namespace AZ MaterialPropertyPsoHandling GetMaterialPropertyPsoHandling() const { return MaterialPropertyPsoHandling::Allowed; } //! Set the visibility dynamic metadata of a material property. - bool SetMaterialPropertyVisibility(const Name& propertyName, MaterialPropertyVisibility visibility); + bool SetMaterialPropertyVisibility(const Name& propertyId, MaterialPropertyVisibility visibility); bool SetMaterialPropertyVisibility(const MaterialPropertyIndex& index, MaterialPropertyVisibility visibility); - bool SetMaterialPropertyDescription(const Name& propertyName, AZStd::string description); + bool SetMaterialPropertyDescription(const Name& propertyId, AZStd::string description); bool SetMaterialPropertyDescription(const MaterialPropertyIndex& index, AZStd::string description); - bool SetMaterialPropertyMinValue(const Name& propertyName, const MaterialPropertyValue& min); + bool SetMaterialPropertyMinValue(const Name& propertyId, const MaterialPropertyValue& min); bool SetMaterialPropertyMinValue(const MaterialPropertyIndex& index, const MaterialPropertyValue& min); - bool SetMaterialPropertyMaxValue(const Name& propertyName, const MaterialPropertyValue& max); + bool SetMaterialPropertyMaxValue(const Name& propertyId, const MaterialPropertyValue& max); bool SetMaterialPropertyMaxValue(const MaterialPropertyIndex& index, const MaterialPropertyValue& max); - bool SetMaterialPropertySoftMinValue(const Name& propertyName, const MaterialPropertyValue& min); + bool SetMaterialPropertySoftMinValue(const Name& propertyId, const MaterialPropertyValue& min); bool SetMaterialPropertySoftMinValue(const MaterialPropertyIndex& index, const MaterialPropertyValue& min); - bool SetMaterialPropertySoftMaxValue(const Name& propertyName, const MaterialPropertyValue& max); + bool SetMaterialPropertySoftMaxValue(const Name& propertyId, const MaterialPropertyValue& max); bool SetMaterialPropertySoftMaxValue(const MaterialPropertyIndex& index, const MaterialPropertyValue& max); bool SetMaterialPropertyGroupVisibility(const Name& propertyGroupName, MaterialPropertyGroupVisibility visibility); @@ -218,8 +218,8 @@ namespace AZ ); private: - MaterialPropertyDynamicMetadata* QueryMaterialPropertyMetadata(const Name& propertyName) const; - MaterialPropertyGroupDynamicMetadata* QueryMaterialPropertyGroupMetadata(const Name& propertyGroupName) const; + MaterialPropertyDynamicMetadata* QueryMaterialPropertyMetadata(const Name& propertyId) const; + MaterialPropertyGroupDynamicMetadata* QueryMaterialPropertyGroupMetadata(const Name& propertyGroupId) const; const AZStd::vector& m_materialPropertyValues; RHI::ConstPtr m_materialPropertiesLayout; diff --git a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialFunctor.cpp b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialFunctor.cpp index 52b6349ca0..3d280b8d60 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialFunctor.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialFunctor.cpp @@ -155,9 +155,9 @@ namespace AZ , m_materialPropertyDependencies(materialPropertyDependencies) {} - const MaterialPropertyDynamicMetadata* MaterialFunctor::EditorContext::GetMaterialPropertyMetadata(const Name& propertyName) const + const MaterialPropertyDynamicMetadata* MaterialFunctor::EditorContext::GetMaterialPropertyMetadata(const Name& propertyId) const { - return QueryMaterialPropertyMetadata(propertyName); + return QueryMaterialPropertyMetadata(propertyId); } const MaterialPropertyDynamicMetadata* MaterialFunctor::EditorContext::GetMaterialPropertyMetadata(const MaterialPropertyIndex& index) const @@ -166,9 +166,9 @@ namespace AZ return GetMaterialPropertyMetadata(name); } - const MaterialPropertyGroupDynamicMetadata* MaterialFunctor::EditorContext::GetMaterialPropertyGroupMetadata(const Name& propertyName) const + const MaterialPropertyGroupDynamicMetadata* MaterialFunctor::EditorContext::GetMaterialPropertyGroupMetadata(const Name& propertyId) const { - return QueryMaterialPropertyGroupMetadata(propertyName); + return QueryMaterialPropertyGroupMetadata(propertyId); } bool MaterialFunctor::EditorContext::SetMaterialPropertyGroupVisibility(const Name& propertyGroupName, MaterialPropertyGroupVisibility visibility) @@ -188,9 +188,9 @@ namespace AZ return true; } - bool MaterialFunctor::EditorContext::SetMaterialPropertyVisibility(const Name& propertyName, MaterialPropertyVisibility visibility) + bool MaterialFunctor::EditorContext::SetMaterialPropertyVisibility(const Name& propertyId, MaterialPropertyVisibility visibility) { - MaterialPropertyDynamicMetadata* metadata = QueryMaterialPropertyMetadata(propertyName); + MaterialPropertyDynamicMetadata* metadata = QueryMaterialPropertyMetadata(propertyId); if (!metadata) { return false; @@ -199,7 +199,7 @@ namespace AZ if (metadata->m_visibility != visibility) { metadata->m_visibility = visibility; - m_updatedPropertiesOut.insert(propertyName); + m_updatedPropertiesOut.insert(propertyId); } return true; @@ -211,9 +211,9 @@ namespace AZ return SetMaterialPropertyVisibility(name, visibility); } - bool MaterialFunctor::EditorContext::SetMaterialPropertyDescription(const Name& propertyName, AZStd::string description) + bool MaterialFunctor::EditorContext::SetMaterialPropertyDescription(const Name& propertyId, AZStd::string description) { - MaterialPropertyDynamicMetadata* metadata = QueryMaterialPropertyMetadata(propertyName); + MaterialPropertyDynamicMetadata* metadata = QueryMaterialPropertyMetadata(propertyId); if (!metadata) { return false; @@ -222,7 +222,7 @@ namespace AZ if (metadata->m_description != description) { metadata->m_description = description; - m_updatedPropertiesOut.insert(propertyName); + m_updatedPropertiesOut.insert(propertyId); } return true; @@ -234,9 +234,9 @@ namespace AZ return SetMaterialPropertyDescription(name, description); } - bool MaterialFunctor::EditorContext::SetMaterialPropertyMinValue(const Name& propertyName, const MaterialPropertyValue& min) + bool MaterialFunctor::EditorContext::SetMaterialPropertyMinValue(const Name& propertyId, const MaterialPropertyValue& min) { - MaterialPropertyDynamicMetadata* metadata = QueryMaterialPropertyMetadata(propertyName); + MaterialPropertyDynamicMetadata* metadata = QueryMaterialPropertyMetadata(propertyId); if (!metadata) { return false; @@ -245,7 +245,7 @@ namespace AZ if(metadata->m_propertyRange.m_min != min) { metadata->m_propertyRange.m_min = min; - m_updatedPropertiesOut.insert(propertyName); + m_updatedPropertiesOut.insert(propertyId); } return true; @@ -257,9 +257,9 @@ namespace AZ return SetMaterialPropertyMinValue(name, min); } - bool MaterialFunctor::EditorContext::SetMaterialPropertyMaxValue(const Name& propertyName, const MaterialPropertyValue& max) + bool MaterialFunctor::EditorContext::SetMaterialPropertyMaxValue(const Name& propertyId, const MaterialPropertyValue& max) { - MaterialPropertyDynamicMetadata* metadata = QueryMaterialPropertyMetadata(propertyName); + MaterialPropertyDynamicMetadata* metadata = QueryMaterialPropertyMetadata(propertyId); if (!metadata) { return false; @@ -268,7 +268,7 @@ namespace AZ if (metadata->m_propertyRange.m_max != max) { metadata->m_propertyRange.m_max = max; - m_updatedPropertiesOut.insert(propertyName); + m_updatedPropertiesOut.insert(propertyId); } return true; @@ -280,9 +280,9 @@ namespace AZ return SetMaterialPropertyMaxValue(name, max); } - bool MaterialFunctor::EditorContext::SetMaterialPropertySoftMinValue(const Name& propertyName, const MaterialPropertyValue& min) + bool MaterialFunctor::EditorContext::SetMaterialPropertySoftMinValue(const Name& propertyId, const MaterialPropertyValue& min) { - MaterialPropertyDynamicMetadata* metadata = QueryMaterialPropertyMetadata(propertyName); + MaterialPropertyDynamicMetadata* metadata = QueryMaterialPropertyMetadata(propertyId); if (!metadata) { return false; @@ -291,7 +291,7 @@ namespace AZ if (metadata->m_propertyRange.m_softMin != min) { metadata->m_propertyRange.m_softMin = min; - m_updatedPropertiesOut.insert(propertyName); + m_updatedPropertiesOut.insert(propertyId); } return true; @@ -303,9 +303,9 @@ namespace AZ return SetMaterialPropertySoftMinValue(name, min); } - bool MaterialFunctor::EditorContext::SetMaterialPropertySoftMaxValue(const Name& propertyName, const MaterialPropertyValue& max) + bool MaterialFunctor::EditorContext::SetMaterialPropertySoftMaxValue(const Name& propertyId, const MaterialPropertyValue& max) { - MaterialPropertyDynamicMetadata* metadata = QueryMaterialPropertyMetadata(propertyName); + MaterialPropertyDynamicMetadata* metadata = QueryMaterialPropertyMetadata(propertyId); if (!metadata) { return false; @@ -314,7 +314,7 @@ namespace AZ if (metadata->m_propertyRange.m_softMax != max) { metadata->m_propertyRange.m_softMax = max; - m_updatedPropertiesOut.insert(propertyName); + m_updatedPropertiesOut.insert(propertyId); } return true; @@ -326,24 +326,24 @@ namespace AZ return SetMaterialPropertySoftMaxValue(name, max); } - MaterialPropertyDynamicMetadata* MaterialFunctor::EditorContext::QueryMaterialPropertyMetadata(const Name& propertyName) const + MaterialPropertyDynamicMetadata* MaterialFunctor::EditorContext::QueryMaterialPropertyMetadata(const Name& propertyId) const { - auto it = m_propertyMetadata.find(propertyName); + auto it = m_propertyMetadata.find(propertyId); if (it == m_propertyMetadata.end()) { - AZ_Error("MaterialFunctor", false, "Couldn't find metadata for material property: %s.", propertyName.GetCStr()); + AZ_Error("MaterialFunctor", false, "Couldn't find metadata for material property: %s.", propertyId.GetCStr()); return nullptr; } return &it->second; } - MaterialPropertyGroupDynamicMetadata* MaterialFunctor::EditorContext::QueryMaterialPropertyGroupMetadata(const Name& propertyGroupName) const + MaterialPropertyGroupDynamicMetadata* MaterialFunctor::EditorContext::QueryMaterialPropertyGroupMetadata(const Name& propertyGroupId) const { - auto it = m_propertyGroupMetadata.find(propertyGroupName); + auto it = m_propertyGroupMetadata.find(propertyGroupId); if (it == m_propertyGroupMetadata.end()) { - AZ_Error("MaterialFunctor", false, "Couldn't find metadata for material property group: %s.", propertyGroupName.GetCStr()); + AZ_Error("MaterialFunctor", false, "Couldn't find metadata for material property group: %s.", propertyGroupId.GetCStr()); return nullptr; } @@ -357,20 +357,20 @@ namespace AZ } // explicit template instantiation - template const bool& MaterialFunctor::RuntimeContext::GetMaterialPropertyValue (const Name& propertyName) const; - template const int32_t& MaterialFunctor::RuntimeContext::GetMaterialPropertyValue (const Name& propertyName) const; - template const uint32_t& MaterialFunctor::RuntimeContext::GetMaterialPropertyValue (const Name& propertyName) const; - template const float& MaterialFunctor::RuntimeContext::GetMaterialPropertyValue (const Name& propertyName) const; - template const Vector2& MaterialFunctor::RuntimeContext::GetMaterialPropertyValue (const Name& propertyName) const; - template const Vector3& MaterialFunctor::RuntimeContext::GetMaterialPropertyValue (const Name& propertyName) const; - template const Vector4& MaterialFunctor::RuntimeContext::GetMaterialPropertyValue (const Name& propertyName) const; - template const Color& MaterialFunctor::RuntimeContext::GetMaterialPropertyValue (const Name& propertyName) const; - template const Data::Instance& MaterialFunctor::RuntimeContext::GetMaterialPropertyValue>(const Name& propertyName) const; + template const bool& MaterialFunctor::RuntimeContext::GetMaterialPropertyValue (const Name& propertyId) const; + template const int32_t& MaterialFunctor::RuntimeContext::GetMaterialPropertyValue (const Name& propertyId) const; + template const uint32_t& MaterialFunctor::RuntimeContext::GetMaterialPropertyValue (const Name& propertyId) const; + template const float& MaterialFunctor::RuntimeContext::GetMaterialPropertyValue (const Name& propertyId) const; + template const Vector2& MaterialFunctor::RuntimeContext::GetMaterialPropertyValue (const Name& propertyId) const; + template const Vector3& MaterialFunctor::RuntimeContext::GetMaterialPropertyValue (const Name& propertyId) const; + template const Vector4& MaterialFunctor::RuntimeContext::GetMaterialPropertyValue (const Name& propertyId) const; + template const Color& MaterialFunctor::RuntimeContext::GetMaterialPropertyValue (const Name& propertyId) const; + template const Data::Instance& MaterialFunctor::RuntimeContext::GetMaterialPropertyValue>(const Name& propertyId) const; template - const Type& MaterialFunctor::RuntimeContext::GetMaterialPropertyValue(const Name& propertyName) const + const Type& MaterialFunctor::RuntimeContext::GetMaterialPropertyValue(const Name& propertyId) const { - return GetMaterialPropertyValue(propertyName).GetValue(); + return GetMaterialPropertyValue(propertyId).GetValue(); } // explicit template instantiation @@ -391,19 +391,19 @@ namespace AZ } // explicit template instantiation - template const bool& MaterialFunctor::EditorContext::GetMaterialPropertyValue (const Name& propertyName) const; - template const int32_t& MaterialFunctor::EditorContext::GetMaterialPropertyValue (const Name& propertyName) const; - template const uint32_t& MaterialFunctor::EditorContext::GetMaterialPropertyValue (const Name& propertyName) const; - template const float& MaterialFunctor::EditorContext::GetMaterialPropertyValue (const Name& propertyName) const; - template const Vector2& MaterialFunctor::EditorContext::GetMaterialPropertyValue (const Name& propertyName) const; - template const Vector3& MaterialFunctor::EditorContext::GetMaterialPropertyValue (const Name& propertyName) const; - template const Vector4& MaterialFunctor::EditorContext::GetMaterialPropertyValue (const Name& propertyName) const; - template const Color& MaterialFunctor::EditorContext::GetMaterialPropertyValue (const Name& propertyName) const; + template const bool& MaterialFunctor::EditorContext::GetMaterialPropertyValue (const Name& propertyId) const; + template const int32_t& MaterialFunctor::EditorContext::GetMaterialPropertyValue (const Name& propertyId) const; + template const uint32_t& MaterialFunctor::EditorContext::GetMaterialPropertyValue (const Name& propertyId) const; + template const float& MaterialFunctor::EditorContext::GetMaterialPropertyValue (const Name& propertyId) const; + template const Vector2& MaterialFunctor::EditorContext::GetMaterialPropertyValue (const Name& propertyId) const; + template const Vector3& MaterialFunctor::EditorContext::GetMaterialPropertyValue (const Name& propertyId) const; + template const Vector4& MaterialFunctor::EditorContext::GetMaterialPropertyValue (const Name& propertyId) const; + template const Color& MaterialFunctor::EditorContext::GetMaterialPropertyValue (const Name& propertyId) const; template - const Type& MaterialFunctor::EditorContext::GetMaterialPropertyValue(const Name& propertyName) const + const Type& MaterialFunctor::EditorContext::GetMaterialPropertyValue(const Name& propertyId) const { - return GetMaterialPropertyValue(propertyName).GetValue(); + return GetMaterialPropertyValue(propertyId).GetValue(); } // explicit template instantiation @@ -436,9 +436,9 @@ namespace AZ return m_materialPropertyValues[index.GetIndex()]; } - const MaterialPropertyValue& MaterialFunctor::RuntimeContext::GetMaterialPropertyValue(const Name& propertyName) const + const MaterialPropertyValue& MaterialFunctor::RuntimeContext::GetMaterialPropertyValue(const Name& propertyId) const { - MaterialPropertyIndex index = m_materialPropertiesLayout->FindPropertyIndex(propertyName); + MaterialPropertyIndex index = m_materialPropertiesLayout->FindPropertyIndex(propertyId); return GetMaterialPropertyValue(index); } @@ -449,9 +449,9 @@ namespace AZ return m_materialPropertyValues[index.GetIndex()]; } - const MaterialPropertyValue& MaterialFunctor::EditorContext::GetMaterialPropertyValue(const Name& propertyName) const + const MaterialPropertyValue& MaterialFunctor::EditorContext::GetMaterialPropertyValue(const Name& propertyId) const { - MaterialPropertyIndex index = m_materialPropertiesLayout->FindPropertyIndex(propertyName); + MaterialPropertyIndex index = m_materialPropertiesLayout->FindPropertyIndex(propertyId); return GetMaterialPropertyValue(index); } From 2c59f1b8a47c312f9cd69b48023703fe994c8c36 Mon Sep 17 00:00:00 2001 From: santorac <55155825+santorac@users.noreply.github.com> Date: Tue, 1 Feb 2022 16:35:42 -0800 Subject: [PATCH 04/14] Added support for deeply nested material property groups. The main addition here is the MaterialNameContext class which represents the concept of a namespace for properties, shader options, and SRG fields. This concept was already somewhat supported in LuaMaterialFunctor through bespoke "prefix" fields, but I have generalized it be available for all material functors. Note that I have not yet updated the other material functor types to ensure they take advantage of this feature, that will be in another commit. Signed-off-by: santorac <55155825+santorac@users.noreply.github.com> --- .../Material/LuaMaterialFunctorSourceData.h | 5 +- .../Material/MaterialFunctorSourceData.h | 48 +++- .../RPI.Edit/Material/MaterialPropertyId.h | 7 +- .../Material/MaterialTypeSourceData.h | 16 +- .../RPI.Reflect/Material/LuaMaterialFunctor.h | 31 +-- .../RPI.Reflect/Material/MaterialFunctor.h | 4 +- .../Material/MaterialNameContext.h | 59 +++++ .../RPI.Builders/Material/MaterialBuilder.cpp | 4 +- .../Material/LuaMaterialFunctorSourceData.cpp | 24 +- .../Material/MaterialFunctorSourceData.cpp | 36 ++- .../RPI.Edit/Material/MaterialPropertyId.cpp | 24 +- .../Material/MaterialTypeSourceData.cpp | 124 +++++----- .../RPI.Public/Material/MaterialSystem.cpp | 1 + .../Material/LuaMaterialFunctor.cpp | 57 +++-- .../Material/MaterialNameContext.cpp | 89 +++++++ .../Material/LuaMaterialFunctorTests.cpp | 5 +- .../Tests/Material/MaterialFunctorTests.cpp | 172 +++++++++++++- .../MaterialPropertyValueSourceDataTests.cpp | 6 +- .../Material/MaterialTypeSourceDataTests.cpp | 218 +++++++++++++++++- .../RPI/Code/atom_rpi_reflect_files.cmake | 2 + .../Code/Source/Document/MaterialDocument.cpp | 20 +- .../EditorMaterialComponentInspector.cpp | 6 +- .../Material/EditorMaterialComponentUtil.cpp | 7 +- 23 files changed, 801 insertions(+), 164 deletions(-) create mode 100644 Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialNameContext.h create mode 100644 Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialNameContext.cpp diff --git a/Gems/Atom/RPI/Code/Include/Atom/RPI.Edit/Material/LuaMaterialFunctorSourceData.h b/Gems/Atom/RPI/Code/Include/Atom/RPI.Edit/Material/LuaMaterialFunctorSourceData.h index 7deda73c05..e08ee90fda 100644 --- a/Gems/Atom/RPI/Code/Include/Atom/RPI.Edit/Material/LuaMaterialFunctorSourceData.h +++ b/Gems/Atom/RPI/Code/Include/Atom/RPI.Edit/Material/LuaMaterialFunctorSourceData.h @@ -41,7 +41,10 @@ namespace AZ // Calls a lua function that returns a list of strings. Outcome, void> GetNameListFromLuaScript(AZ::ScriptContext& scriptContext, const char* luaFunctionName) const; - FunctorResult CreateFunctor(const AZStd::string& materialTypeSourceFilePath, const MaterialPropertiesLayout* propertiesLayout) const; + FunctorResult CreateFunctor( + const AZStd::string& materialTypeSourceFilePath, + const MaterialPropertiesLayout* propertiesLayout, + const MaterialNameContext* materialNameContext) const; // Only one of these should have data AZStd::string m_luaSourceFile; diff --git a/Gems/Atom/RPI/Code/Include/Atom/RPI.Edit/Material/MaterialFunctorSourceData.h b/Gems/Atom/RPI/Code/Include/Atom/RPI.Edit/Material/MaterialFunctorSourceData.h index a8f4f4c524..8bf88342c3 100644 --- a/Gems/Atom/RPI/Code/Include/Atom/RPI.Edit/Material/MaterialFunctorSourceData.h +++ b/Gems/Atom/RPI/Code/Include/Atom/RPI.Edit/Material/MaterialFunctorSourceData.h @@ -31,6 +31,7 @@ namespace AZ class MaterialPropertiesLayout; class ShaderOptionGroupLayout; class JsonMaterialFunctorSourceDataSerializer; + class MaterialNameContext; //! This is an abstract base class for initializing MaterialFunctor objects. //! Material functors provide custom logic and calculations to configure shaders, render states, and more. @@ -54,11 +55,17 @@ namespace AZ struct RuntimeContext { public: - RuntimeContext(const AZStd::string& materialTypeFilePath, const MaterialPropertiesLayout* materialPropertiesLayout, const RHI::ShaderResourceGroupLayout* shaderResourceGroupLayout, const ShaderCollection* shaderCollection) + RuntimeContext( + const AZStd::string& materialTypeFilePath, + const MaterialPropertiesLayout* materialPropertiesLayout, + const RHI::ShaderResourceGroupLayout* shaderResourceGroupLayout, + const ShaderCollection* shaderCollection, + const MaterialNameContext* materialNameContext) : m_materialTypeFilePath(materialTypeFilePath) , m_materialPropertiesLayout(materialPropertiesLayout) , m_shaderResourceGroupLayout(shaderResourceGroupLayout) , m_shaderCollection(shaderCollection) + , m_materialNameContext(materialNameContext) {} const AZStd::string& GetMaterialTypeSourceFilePath() const { return m_materialTypeFilePath; } @@ -66,6 +73,10 @@ namespace AZ const MaterialPropertiesLayout* GetMaterialPropertiesLayout() const; const RHI::ShaderResourceGroupLayout* GetShaderResourceGroupLayout() const; + //! Find the index of a ShaderResourceGroup input. This will automatically apply the MaterialNameContext. + RHI::ShaderInputConstantIndex FindShaderInputConstantIndex(Name inputName) const; + RHI::ShaderInputImageIndex FindShaderInputImageIndex(Name inputName) const; + //! Returns the number of shaders available in this material type. AZStd::size_t GetShaderCount() const; @@ -77,16 +88,18 @@ namespace AZ AZStd::vector GetShaderTags() const; //! Find a property's index by its name. It will report error and return a Null index if it fails. - MaterialPropertyIndex FindMaterialPropertyIndex(const Name& propertyName) const; + //! This will also automatically apply the MaterialNameContext. + MaterialPropertyIndex FindMaterialPropertyIndex(Name propertyId) const; //! Find a shader option index using an option name from the shader by index or tag. + //! This will also automatically apply the MaterialNameContext. //! @param shaderIndex index into the material type's list of shader options //! @param shaderTag tag name to index into the material type's list of shader options //! @param optionName the name of the option to find //! @param reportErrors if true, report an error message when if the option name was not found. //! @return the found index, or an empty handle if the option could not be found - ShaderOptionIndex FindShaderOptionIndex(AZStd::size_t shaderIndex, const Name& optionName, bool reportErrors = true) const; - ShaderOptionIndex FindShaderOptionIndex(const AZ::Name& shaderTag, const Name& optionName, bool reportErrors = true) const; + ShaderOptionIndex FindShaderOptionIndex(AZStd::size_t shaderIndex, Name optionName, bool reportErrors = true) const; + ShaderOptionIndex FindShaderOptionIndex(const AZ::Name& shaderTag, Name optionName, bool reportErrors = true) const; //! Return true if a shaderIndex is within the range of the number of shaders defined in this material. //! And also report an error message if the index is invalid. @@ -96,19 +109,32 @@ namespace AZ //! And also report an error message if shaderTag is invalid. bool CheckShaderTagValid(const AZ::Name& shaderTag) const; + //! Returns the name context for the functor. + //! It acts like a namespace for any names that the MaterialFunctorSourceData might reference. The namespace + //! is automatically applied by the other relevant functions of this RuntimeContext class. + //! Not that by default the MaterialNameContext is not saved as part of the final MaterialFunctor class + //! (most CreateFunctor() implementations should convert names to indexes anyway) but CreateFunctor() can + //! copy it to the created MaterialFunctor for use at runtime if needed. + const MaterialNameContext* GetNameContext() const { return m_materialNameContext; } + private: const AZStd::string m_materialTypeFilePath; const MaterialPropertiesLayout* m_materialPropertiesLayout; const RHI::ShaderResourceGroupLayout* m_shaderResourceGroupLayout; const ShaderCollection* m_shaderCollection; + const MaterialNameContext* m_materialNameContext; }; struct EditorContext { public: - EditorContext(const AZStd::string& materialTypeFilePath, const MaterialPropertiesLayout* materialPropertiesLayout) + EditorContext( + const AZStd::string& materialTypeFilePath, + const MaterialPropertiesLayout* materialPropertiesLayout, + const MaterialNameContext* materialNameContext) : m_materialTypeFilePath(materialTypeFilePath) , m_materialPropertiesLayout(materialPropertiesLayout) + , m_materialNameContext(materialNameContext) {} const AZStd::string& GetMaterialTypeSourceFilePath() const { return m_materialTypeFilePath; } @@ -116,11 +142,21 @@ namespace AZ const MaterialPropertiesLayout* GetMaterialPropertiesLayout() const; //! Find a property's index by its name. It will report error and return a Null index if it fails. - MaterialPropertyIndex FindMaterialPropertyIndex(const Name& propertyName) const; + //! This will also automatically apply the MaterialNameContext. + MaterialPropertyIndex FindMaterialPropertyIndex(Name propertyId) const; + + //! Returns the name context for the functor. + //! It acts like a namespace for any names that the MaterialFunctorSourceData might reference. The namespace + //! is automatically applied by the other relevant functions of this RuntimeContext class. + //! Not that by default the MaterialNameContext is not saved as part of the final MaterialFunctor class + //! (most CreateFunctor() implementations should convert names to indexes anyway) but CreateFunctor() can + //! copy it to the created MaterialFunctor for use at runtime if needed. + const MaterialNameContext* GetNameContext() const { return m_materialNameContext; } private: const AZStd::string m_materialTypeFilePath; const MaterialPropertiesLayout* m_materialPropertiesLayout; + const MaterialNameContext* m_materialNameContext; }; //! Creates a fully initialized MaterialFunctor object that is ready to be serialized to the cache. 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 8c78d4b491..424a3a187a 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 @@ -22,9 +22,14 @@ namespace AZ //! The groups are optional, in which case the full property ID will just be like "propertyName". class MaterialPropertyId { - public: + public: + //! Returns whether the name is a valid C-style identifier static bool IsValidName(AZStd::string_view name); static bool IsValidName(const AZ::Name& name); + + //! Returns whether the name is a valid C-style identifier, and reports errors if it is not. + static bool CheckIsValidName(AZStd::string_view name); + static bool CheckIsValidName(const AZ::Name& name); //! Creates a MaterialPropertyId from a full name string like "groupA.groupB.[...].propertyName" or just "propertyName". //! Also checks the name for validity. 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 400556bf2f..70d92ffccf 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 @@ -158,6 +158,8 @@ namespace AZ AZStd::string m_name; AZStd::string m_displayName; AZStd::string m_description; + AZStd::string m_shaderInputsPrefix; //!< The name of all SRG inputs under this group will get this prefix. + AZStd::string m_shaderOptionsPrefix; //!< The name of all shader options under this group will get this prefix. PropertyList m_properties; AZStd::vector> m_propertyGroups; AZStd::vector> m_materialFunctorSourceData; @@ -282,7 +284,7 @@ namespace AZ //! Call back function type used with the enumeration functions. //! Return false to terminate the traversal. using EnumeratePropertyGroupsCallback = AZStd::function; @@ -293,7 +295,7 @@ namespace AZ //! Call back function type used with the numeration functions. //! Return false to terminate the traversal. using EnumeratePropertiesCallback = AZStd::function; @@ -316,10 +318,12 @@ namespace AZ PropertyDefinition* FindProperty(AZStd::span parsedPropertyId, AZStd::span> inPropertyGroupList); // Function overloads for recursion, returns false to indicate that recursion should end. - bool EnumeratePropertyGroups(const EnumeratePropertyGroupsCallback& callback, AZStd::string propertyIdContext, const AZStd::vector>& inPropertyGroupList) const; - bool EnumerateProperties(const EnumeratePropertiesCallback& callback, AZStd::string propertyIdContext, const AZStd::vector>& inPropertyGroupList) const; + bool EnumeratePropertyGroups(const EnumeratePropertyGroupsCallback& callback, MaterialNameContext materialNameContext, const AZStd::vector>& inPropertyGroupList) const; + bool EnumerateProperties(const EnumeratePropertiesCallback& callback, MaterialNameContext materialNameContext, const AZStd::vector>& inPropertyGroupList) const; + + static MaterialNameContext ExtendNameContext(MaterialNameContext nameContext, const MaterialTypeSourceData::PropertyGroup& propertyGroup); - //! Recursively populates a material asset with properties from the tree of material property groups. + //! Recursively populates a material type asset with properties from the tree of material property groups. //! @param materialTypeSourceFilePath path to the material type file that is being processed, used to look up relative paths //! @param propertyIdContext the accumulated prefix that should be applied to any property names encountered in the current @propertyGroup //! @param propertyGroup the current PropertyGroup that is being processed @@ -327,7 +331,7 @@ namespace AZ bool BuildPropertyList( const AZStd::string& materialTypeSourceFilePath, MaterialTypeAssetCreator& materialTypeAssetCreator, - AZStd::vector& propertyIdContext, + MaterialNameContext materialNameContext, const MaterialTypeSourceData::PropertyGroup* propertyGroup) const; //! Construct a complete list of group definitions, including implicit groups, arranged in the same order as the source data. diff --git a/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/LuaMaterialFunctor.h b/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/LuaMaterialFunctor.h index 5f4dce40e5..1bce036c73 100644 --- a/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/LuaMaterialFunctor.h +++ b/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/LuaMaterialFunctor.h @@ -10,6 +10,7 @@ #include #include +#include #include namespace UnitTest @@ -60,12 +61,8 @@ namespace AZ AZStd::unique_ptr m_sriptBehaviorContext; AZStd::unique_ptr m_scriptContext; - // These are prefix strings that will be applied to every name lookup in the lua functor. - // This allows the lua script to be reused in different contexts. - AZStd::string m_propertyNamePrefix; - AZStd::string m_srgNamePrefix; - AZStd::string m_optionsNamePrefix; - + MaterialNameContext m_materialNameContext; + enum class ScriptStatus { Uninitialized, @@ -84,15 +81,11 @@ namespace AZ explicit LuaMaterialFunctorCommonContext(MaterialFunctor::RuntimeContext* runtimeContextImpl, const MaterialPropertyFlags* materialPropertyDependencies, - const AZStd::string& propertyNamePrefix, - const AZStd::string& srgNamePrefix, - const AZStd::string& optionsNamePrefix); + const MaterialNameContext &materialNameContext); explicit LuaMaterialFunctorCommonContext(MaterialFunctor::EditorContext* editorContextImpl, const MaterialPropertyFlags* materialPropertyDependencies, - const AZStd::string& propertyNamePrefix, - const AZStd::string& srgNamePrefix, - const AZStd::string& optionsNamePrefix); + const MaterialNameContext &materialNameContext); //! Returns false if PSO changes are not allowed, and may report errors or warnings bool CheckPsoChangesAllowed(); @@ -112,11 +105,7 @@ namespace AZ AZStd::string GetMaterialPropertyDependenciesString() const; - // These are prefix strings that will be applied to every name lookup in the lua functor. - // This allows the lua script to be reused in different contexts. - const AZStd::string& m_propertyNamePrefix; - const AZStd::string& m_srgNamePrefix; - const AZStd::string& m_optionsNamePrefix; + const MaterialNameContext &m_materialNameContext; private: @@ -284,9 +273,7 @@ namespace AZ explicit LuaMaterialFunctorRuntimeContext(MaterialFunctor::RuntimeContext* runtimeContextImpl, const MaterialPropertyFlags* materialPropertyDependencies, - const AZStd::string& propertyNamePrefix, - const AZStd::string& srgNamePrefix, - const AZStd::string& optionsNamePrefix); + const MaterialNameContext &materialNameContext); template Type GetMaterialPropertyValue(const char* name) const; @@ -324,9 +311,7 @@ namespace AZ explicit LuaMaterialFunctorEditorContext(MaterialFunctor::EditorContext* editorContextImpl, const MaterialPropertyFlags* materialPropertyDependencies, - const AZStd::string& propertyNamePrefix, - const AZStd::string& srgNamePrefix, - const AZStd::string& optionsNamePrefix); + const MaterialNameContext &materialNameContext); template Type GetMaterialPropertyValue(const char* name) const; diff --git a/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialFunctor.h b/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialFunctor.h index 92a9ab789a..459dd601f9 100644 --- a/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialFunctor.h +++ b/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialFunctor.h @@ -49,8 +49,8 @@ namespace AZ //! MaterialFunctor objects provide custom logic and calculations to configure shaders, render states, //! editor metadata, and more. - //! Atom will provide an implementation of this class that uses a script to define the custom logic - //! for a convenient workflow. Clients may also provide their own custom hard-coded implementations + //! Atom provides a LuaMaterialFunctor subclass that uses a script to define the custom logic + //! for a convenient workflow. Developers may also provide their own custom hard-coded implementations //! as an optimization rather than taking the scripted approach. //! Any custom subclasses of MaterialFunctor will also need a corresponding MaterialFunctorSourceData subclass //! to create the functor at build-time. Depending on the builder context, clients can choose to create a runtime diff --git a/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialNameContext.h b/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialNameContext.h new file mode 100644 index 0000000000..3ecdfeb449 --- /dev/null +++ b/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialNameContext.h @@ -0,0 +1,59 @@ +/* + * 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 + +namespace AZ +{ + class ReflectContext; + class Name; + + namespace RPI + { + //! This acts like a namespace description for various types of identifiers that appear in .materialtype files. + //! When reusable property groups are nested inside other property groups, they usually need alternate naming + //! to connect to the appropriate shader inputs. For example, a baseColor property group inside a "layer1" group + //! needs to connect to "m_layer1_baseColor_texture" and the same property definition is repeated inside a "layer2" + //! group where it connects to "m_layer2_baseColor_texture". This data structure provides the name context, like + //! "m_layer1_" or "m_layer2_". + class MaterialNameContext + { + public: + AZ_TYPE_INFO(MaterialNameContext, "{AAC9BB28-F463-455D-8467-F877E50E1FA7}") + + static void Reflect(ReflectContext* context); + + MaterialNameContext() = default; + + //! Extends the name context to a deeper property group. + void ExtendPropertyIdContext(AZStd::string_view nameContext, bool insertDelimiter=true); + void ExtendSrgInputContext(AZStd::string_view nameContext); + void ExtendShaderOptionContext(AZStd::string_view nameContext); + + //! Applies the name context to a given leaf name. + bool ContextualizeProperty(Name& propertyName) const; + bool ContextualizeSrgInput(Name& srgInputName) const; + bool ContextualizeShaderOption(Name& shaderOptionName) const; + + //! Returns true if there is some non-default name context. + bool HasContextForProperties() const { return !m_propertyIdContext.empty(); } + bool HasContextForSrgInputs() const { return !m_srgInputNameContext.empty(); } + bool HasContextForShaderOptions() const { return !m_shaderOptionNameContext.empty(); } + + //! Returns true if the name context is empty. + bool IsDefault() const; + + private: + AZStd::string m_propertyIdContext; + AZStd::string m_srgInputNameContext; + AZStd::string m_shaderOptionNameContext; + }; + + } // namespace RPI +} // namespace AZ 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 c81b91d777..bf65a09cef 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Builders/Material/MaterialBuilder.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Builders/Material/MaterialBuilder.cpp @@ -51,7 +51,7 @@ namespace AZ { AssetBuilderSDK::AssetBuilderDesc materialBuilderDescriptor; materialBuilderDescriptor.m_name = JobKey; - materialBuilderDescriptor.m_version = 117; // new material type file format + materialBuilderDescriptor.m_version = 122; // nested property layers 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(); @@ -269,7 +269,7 @@ namespace AZ response.m_result = AssetBuilderSDK::CreateJobsResultCode::Success; } - AZ::Data::Asset CreateMaterialTypeAsset(AZStd::string_view materialTypeSourceFilePath, const rapidjson::Value& json) + AZ::Data::Asset CreateMaterialTypeAsset(AZStd::string_view materialTypeSourceFilePath, rapidjson::Document& json) { auto materialType = MaterialUtils::LoadMaterialTypeSourceData(materialTypeSourceFilePath, &json); diff --git a/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/LuaMaterialFunctorSourceData.cpp b/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/LuaMaterialFunctorSourceData.cpp index 14ef3bb17d..c614222ee6 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/LuaMaterialFunctorSourceData.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/LuaMaterialFunctorSourceData.cpp @@ -115,16 +115,26 @@ namespace AZ RPI::LuaMaterialFunctorSourceData::FunctorResult LuaMaterialFunctorSourceData::CreateFunctor( const AZStd::string& materialTypeSourceFilePath, - const MaterialPropertiesLayout* propertiesLayout + const MaterialPropertiesLayout* propertiesLayout, + const MaterialNameContext* materialNameContext ) const { using namespace RPI; RPI::Ptr functor = aznew LuaMaterialFunctor; - functor->m_propertyNamePrefix = m_propertyNamePrefix; - functor->m_srgNamePrefix = m_srgNamePrefix; - functor->m_optionsNamePrefix = m_optionsNamePrefix; + if (materialNameContext->IsDefault()) + { + // This is a legacy feature that was used for a while to support reusing the same functor for multiple layers in StandardMultilayerPbr.materialtype. + // Now that we have support for nested property groups, this functionality is only supported for functors at the top level, for backward compatibility. + functor->m_materialNameContext.ExtendPropertyIdContext(m_propertyNamePrefix, false); + functor->m_materialNameContext.ExtendSrgInputContext(m_srgNamePrefix); + functor->m_materialNameContext.ExtendShaderOptionContext(m_optionsNamePrefix); + } + else + { + functor->m_materialNameContext = *materialNameContext; + } if (!m_luaScript.empty() && !m_luaSourceFile.empty()) { @@ -205,14 +215,16 @@ namespace AZ { return CreateFunctor( context.GetMaterialTypeSourceFilePath(), - context.GetMaterialPropertiesLayout()); + context.GetMaterialPropertiesLayout(), + context.GetNameContext()); } RPI::LuaMaterialFunctorSourceData::FunctorResult LuaMaterialFunctorSourceData::CreateFunctor(const EditorContext& context) const { return CreateFunctor( context.GetMaterialTypeSourceFilePath(), - context.GetMaterialPropertiesLayout()); + context.GetMaterialPropertiesLayout(), + context.GetNameContext()); } } } diff --git a/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialFunctorSourceData.cpp b/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialFunctorSourceData.cpp index 36217af1b6..cdc75b8c05 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialFunctorSourceData.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialFunctorSourceData.cpp @@ -10,6 +10,7 @@ #include #include #include +#include #include #include @@ -66,17 +67,20 @@ namespace AZ return m_shaderResourceGroupLayout; } - MaterialPropertyIndex MaterialFunctorSourceData::RuntimeContext::FindMaterialPropertyIndex(const Name& propertyName) const + MaterialPropertyIndex MaterialFunctorSourceData::RuntimeContext::FindMaterialPropertyIndex(Name propertyId) const { - MaterialPropertyIndex propertyIndex = m_materialPropertiesLayout->FindPropertyIndex(propertyName); + m_materialNameContext->ContextualizeProperty(propertyId); + MaterialPropertyIndex propertyIndex = m_materialPropertiesLayout->FindPropertyIndex(propertyId); - AZ_Error("MaterialFunctorSourceData", propertyIndex.IsValid(), "Could not find property '%s'.", propertyName.GetCStr()); + AZ_Error("MaterialFunctorSourceData", propertyIndex.IsValid(), "Could not find property '%s'.", propertyId.GetCStr()); return propertyIndex; } - ShaderOptionIndex MaterialFunctorSourceData::RuntimeContext::FindShaderOptionIndex(AZStd::size_t shaderIndex, const Name& optionName, bool reportErrors) const + ShaderOptionIndex MaterialFunctorSourceData::RuntimeContext::FindShaderOptionIndex(AZStd::size_t shaderIndex, Name optionName, bool reportErrors) const { + m_materialNameContext->ContextualizeShaderOption(optionName); + const ShaderOptionGroupLayout* shaderOptionGroupLayout = GetShaderOptionGroupLayout(shaderIndex); if (shaderOptionGroupLayout) @@ -92,8 +96,10 @@ namespace AZ return ShaderOptionIndex(); } - AZ::RPI::ShaderOptionIndex MaterialFunctorSourceData::RuntimeContext::FindShaderOptionIndex(const AZ::Name& shaderTag, const Name& optionName, bool reportErrors /*= true*/) const + AZ::RPI::ShaderOptionIndex MaterialFunctorSourceData::RuntimeContext::FindShaderOptionIndex(const AZ::Name& shaderTag, Name optionName, bool reportErrors /*= true*/) const { + m_materialNameContext->ContextualizeShaderOption(optionName); + const ShaderOptionGroupLayout* shaderOptionGroupLayout = GetShaderOptionGroupLayout(shaderTag); if (shaderOptionGroupLayout) @@ -122,19 +128,33 @@ namespace AZ AZ_Error("MaterialFunctorSourceData", valid, "Shader tag '%s' is invalid", shaderTag.GetCStr()); return valid; } + + RHI::ShaderInputConstantIndex MaterialFunctorSourceData::RuntimeContext::FindShaderInputConstantIndex(Name inputName) const + { + m_materialNameContext->ContextualizeSrgInput(inputName); + return m_shaderResourceGroupLayout->FindShaderInputConstantIndex(inputName); + } + + RHI::ShaderInputImageIndex MaterialFunctorSourceData::RuntimeContext::FindShaderInputImageIndex(Name inputName) const + { + m_materialNameContext->ContextualizeSrgInput(inputName); + return m_shaderResourceGroupLayout->FindShaderInputImageIndex(inputName); + } const MaterialPropertiesLayout* MaterialFunctorSourceData::EditorContext::GetMaterialPropertiesLayout() const { return m_materialPropertiesLayout; } - MaterialPropertyIndex MaterialFunctorSourceData::EditorContext::FindMaterialPropertyIndex(const Name& propertyName) const + MaterialPropertyIndex MaterialFunctorSourceData::EditorContext::FindMaterialPropertyIndex(Name propertyId) const { - MaterialPropertyIndex propertyIndex = m_materialPropertiesLayout->FindPropertyIndex(propertyName); + m_materialNameContext->ContextualizeProperty(propertyId); + MaterialPropertyIndex propertyIndex = m_materialPropertiesLayout->FindPropertyIndex(propertyId); - AZ_Error("MaterialFunctorSourceData", propertyIndex.IsValid(), "Could not find property '%s'", propertyName.GetCStr()); + AZ_Error("MaterialFunctorSourceData", propertyIndex.IsValid(), "Could not find property '%s'", propertyId.GetCStr()); return propertyIndex; } + } // namespace RPI } // namespace AZ 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 1f5581e417..843fa44dd7 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialPropertyId.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialPropertyId.cpp @@ -25,11 +25,29 @@ namespace AZ return IsValidName(name.GetStringView()); } + bool MaterialPropertyId::CheckIsValidName(AZStd::string_view name) + { + if (IsValidName(name)) + { + return true; + } + else + { + AZ_Error("MaterialPropertyId", false, "'%.*s' is not a valid identifier", AZ_STRING_ARG(name)); + return false; + } + } + + bool MaterialPropertyId::CheckIsValidName(const AZ::Name& name) + { + return CheckIsValidName(name.GetStringView()); + } + bool MaterialPropertyId::IsValid() const { return !m_fullName.IsEmpty(); } - + MaterialPropertyId MaterialPropertyId::Parse(AZStd::string_view fullPropertyId) { AZStd::vector tokens; @@ -94,14 +112,14 @@ namespace AZ { if (!IsValidName(name)) { - AZ_Error("MaterialPropertyId", false, "'%s' is not a valid identifier.", name.c_str()); + AZ_Error("MaterialPropertyId", false, "Group name '%s' is not a valid identifier.", name.c_str()); return; } } if (!IsValidName(propertyName)) { - AZ_Error("MaterialPropertyId", false, "'%.*s' is not a valid identifier.", AZ_STRING_ARG(propertyName)); + AZ_Error("MaterialPropertyId", false, "Property name '%.*s' is not a valid identifier.", AZ_STRING_ARG(propertyName)); return; } 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 b7808abf17..9b251213fe 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialTypeSourceData.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialTypeSourceData.cpp @@ -17,6 +17,7 @@ #include #include #include +#include #include #include #include @@ -89,10 +90,12 @@ namespace AZ ; serializeContext->Class() - ->Version(1) + ->Version(2) ->Field("name", &PropertyGroup::m_name) ->Field("displayName", &PropertyGroup::m_displayName) ->Field("description", &PropertyGroup::m_description) + ->Field("shaderInputsPrefix", &PropertyGroup::m_shaderInputsPrefix) + ->Field("shaderOptionsPrefix", &PropertyGroup::m_shaderOptionsPrefix) ->Field("properties", &PropertyGroup::m_properties) ->Field("propertyGroups", &PropertyGroup::m_propertyGroups) ->Field("functors", &PropertyGroup::m_materialFunctorSourceData) @@ -134,22 +137,21 @@ namespace AZ /*static*/ MaterialTypeSourceData::PropertyGroup* MaterialTypeSourceData::PropertyGroup::AddPropertyGroup(AZStd::string_view name, AZStd::vector>& toPropertyGroupList) { + if (!MaterialPropertyId::CheckIsValidName(name)) + { + return nullptr; + } + auto iter = AZStd::find_if(toPropertyGroupList.begin(), toPropertyGroupList.end(), [name](const AZStd::unique_ptr& existingPropertyGroup) { return existingPropertyGroup->m_name == name; }); - + if (iter != toPropertyGroupList.end()) { AZ_Error("Material source data", false, "PropertyGroup named '%.*s' already exists", AZ_STRING_ARG(name)); return nullptr; } - - if (!MaterialPropertyId::IsValidName(name)) - { - AZ_Error("Material source data", false, "'%.*s' is not a valid identifier", AZ_STRING_ARG(name)); - return nullptr; - } toPropertyGroupList.push_back(AZStd::make_unique()); toPropertyGroupList.back()->m_name = name; @@ -158,6 +160,11 @@ namespace AZ MaterialTypeSourceData::PropertyDefinition* MaterialTypeSourceData::PropertyGroup::AddProperty(AZStd::string_view name) { + if (!MaterialPropertyId::CheckIsValidName(name)) + { + return nullptr; + } + auto propertyIter = AZStd::find_if(m_properties.begin(), m_properties.end(), [name](const AZStd::unique_ptr& existingProperty) { return existingProperty->GetName() == name; @@ -180,12 +187,6 @@ namespace AZ return nullptr; } - if (!MaterialPropertyId::IsValidName(name)) - { - AZ_Error("Material source data", false, "'%.*s' is not a valid identifier", AZ_STRING_ARG(name)); - return nullptr; - } - m_properties.emplace_back(AZStd::make_unique(name)); return m_properties.back().get(); } @@ -265,10 +266,10 @@ namespace AZ if (!subPath.empty()) { - const MaterialTypeSourceData::PropertyGroup* propertySubset = FindPropertyGroup(subPath, propertyGroup->m_propertyGroups); - if (propertySubset) + const MaterialTypeSourceData::PropertyGroup* propertySubgroup = FindPropertyGroup(subPath, propertyGroup->m_propertyGroups); + if (propertySubgroup) { - return propertySubset; + return propertySubgroup; } } } @@ -376,18 +377,18 @@ namespace AZ return parts; } - bool MaterialTypeSourceData::EnumeratePropertyGroups(const EnumeratePropertyGroupsCallback& callback, AZStd::string propertyIdContext, const AZStd::vector>& inPropertyGroupList) const + bool MaterialTypeSourceData::EnumeratePropertyGroups(const EnumeratePropertyGroupsCallback& callback, MaterialNameContext materialNameContext, const AZStd::vector>& inPropertyGroupList) const { for (auto& propertyGroup : inPropertyGroupList) { - if (!callback(propertyIdContext, propertyGroup.get())) + if (!callback(materialNameContext, propertyGroup.get())) { return false; // Stop processing } + + MaterialNameContext materialNameContext2 = ExtendNameContext(materialNameContext, *propertyGroup); - const AZStd::string propertyIdContext2 = propertyIdContext + propertyGroup->m_name + "."; - - if (!EnumeratePropertyGroups(callback, propertyIdContext2, propertyGroup->m_propertyGroups)) + if (!EnumeratePropertyGroups(callback, materialNameContext2, propertyGroup->m_propertyGroups)) { return false; // Stop processing } @@ -406,22 +407,21 @@ namespace AZ return EnumeratePropertyGroups(callback, {}, m_propertyLayout.m_propertyGroups); } - bool MaterialTypeSourceData::EnumerateProperties(const EnumeratePropertiesCallback& callback, AZStd::string propertyIdContext, const AZStd::vector>& inPropertyGroupList) const + bool MaterialTypeSourceData::EnumerateProperties(const EnumeratePropertiesCallback& callback, MaterialNameContext materialNameContext, const AZStd::vector>& inPropertyGroupList) const { - for (auto& propertyGroup : inPropertyGroupList) { - const AZStd::string propertyIdContext2 = propertyIdContext + propertyGroup->m_name + "."; + MaterialNameContext materialNameContext2 = ExtendNameContext(materialNameContext, *propertyGroup); for (auto& property : propertyGroup->m_properties) { - if (!callback(propertyIdContext2, property.get())) + if (!callback(materialNameContext2, property.get())) { return false; // Stop processing } } - if (!EnumerateProperties(callback, propertyIdContext2, propertyGroup->m_propertyGroups)) + if (!EnumerateProperties(callback, materialNameContext2, propertyGroup->m_propertyGroups)) { return false; // Stop processing } @@ -483,7 +483,7 @@ namespace AZ enumValues.push_back(uvNamePair.second); } - EnumerateProperties([&enumValues](const AZStd::string&, const MaterialTypeSourceData::PropertyDefinition* property) + EnumerateProperties([&enumValues](const MaterialNameContext&, const MaterialTypeSourceData::PropertyDefinition* property) { if (property->m_dataType == AZ::RPI::MaterialPropertyDataType::Enum && property->m_enumIsUv) { @@ -529,24 +529,40 @@ namespace AZ return groupDefinitions; } + MaterialNameContext MaterialTypeSourceData::ExtendNameContext(MaterialNameContext nameContext, const MaterialTypeSourceData::PropertyGroup& propertyGroup) + { + MaterialNameContext materialNameContext2 = nameContext; + materialNameContext2.ExtendPropertyIdContext(propertyGroup.m_name); + materialNameContext2.ExtendShaderOptionContext(propertyGroup.m_shaderOptionsPrefix); + materialNameContext2.ExtendSrgInputContext(propertyGroup.m_shaderInputsPrefix); + return materialNameContext2; + } + bool MaterialTypeSourceData::BuildPropertyList( const AZStd::string& materialTypeSourceFilePath, MaterialTypeAssetCreator& materialTypeAssetCreator, - AZStd::vector& propertyIdContext, + MaterialNameContext materialNameContext, const MaterialTypeSourceData::PropertyGroup* propertyGroup) const - { + { + if (!MaterialPropertyId::CheckIsValidName(propertyGroup->m_name)) + { + return false; + } + + materialNameContext = ExtendNameContext(materialNameContext, *propertyGroup); + for (const AZStd::unique_ptr& property : propertyGroup->m_properties) { // Register the property... - MaterialPropertyId propertyId{propertyIdContext, property->GetName()}; - - if (!propertyId.IsValid()) + if (!MaterialPropertyId::CheckIsValidName(property->GetName())) { - // MaterialPropertyId reports an error message return false; } + Name propertyId{property->GetName()}; + materialNameContext.ContextualizeProperty(propertyId); + auto propertyGroupIter = AZStd::find_if(propertyGroup->GetPropertyGroups().begin(), propertyGroup->GetPropertyGroups().end(), [&property](const AZStd::unique_ptr& existingPropertyGroup) { @@ -572,18 +588,23 @@ namespace AZ { case MaterialPropertyOutputType::ShaderInput: { - materialTypeAssetCreator.ConnectMaterialPropertyToShaderInput(Name{output.m_fieldName}); + Name fieldName{output.m_fieldName}; + materialNameContext.ContextualizeSrgInput(fieldName); + materialTypeAssetCreator.ConnectMaterialPropertyToShaderInput(fieldName); break; } case MaterialPropertyOutputType::ShaderOption: { + Name fieldName{output.m_fieldName}; + materialNameContext.ContextualizeShaderOption(fieldName); + if (output.m_shaderIndex >= 0) { - materialTypeAssetCreator.ConnectMaterialPropertyToShaderOption(Name{output.m_fieldName}, output.m_shaderIndex); + materialTypeAssetCreator.ConnectMaterialPropertyToShaderOption(fieldName, output.m_shaderIndex); } else { - materialTypeAssetCreator.ConnectMaterialPropertyToShaderOptions(Name{output.m_fieldName}); + materialTypeAssetCreator.ConnectMaterialPropertyToShaderOptions(fieldName); } break; } @@ -650,17 +671,13 @@ namespace AZ } } - for (const AZStd::unique_ptr& propertySubset : propertyGroup->m_propertyGroups) + for (const AZStd::unique_ptr& propertySubgroup : propertyGroup->m_propertyGroups) { - propertyIdContext.push_back(propertySubset->m_name); - bool success = BuildPropertyList( materialTypeSourceFilePath, materialTypeAssetCreator, - propertyIdContext, - propertySubset.get()); - - propertyIdContext.pop_back(); + materialNameContext, + propertySubgroup.get()); if (!success) { @@ -677,7 +694,8 @@ namespace AZ materialTypeSourceFilePath, materialTypeAssetCreator.GetMaterialPropertiesLayout(), materialTypeAssetCreator.GetMaterialShaderResourceGroupLayout(), - materialTypeAssetCreator.GetShaderCollection() + materialTypeAssetCreator.GetShaderCollection(), + &materialNameContext ) ); @@ -688,9 +706,10 @@ namespace AZ { materialTypeAssetCreator.AddMaterialFunctor(functor); - for (const AZ::Name& optionName : functorData->GetActualSourceData()->GetShaderOptionDependencies()) + for (AZ::Name optionName : functorData->GetActualSourceData()->GetShaderOptionDependencies()) { - materialTypeAssetCreator.ClaimShaderOptionOwnership(Name{optionName.GetCStr()}); + materialNameContext.ContextualizeShaderOption(optionName); + materialTypeAssetCreator.ClaimShaderOptionOwnership(optionName); } } } @@ -795,11 +814,9 @@ namespace AZ } } - for (const AZStd::unique_ptr& propertyGroup : m_propertyLayout.m_propertyGroups) + for (const AZStd::unique_ptr& propertyGroup : m_propertyLayout.m_propertyGroups) { - AZStd::vector propertyIdContext; - propertyIdContext.push_back(propertyGroup->m_name); - bool success = BuildPropertyList(materialTypeSourceFilePath, materialTypeAssetCreator, propertyIdContext, propertyGroup.get()); + bool success = BuildPropertyList(materialTypeSourceFilePath, materialTypeAssetCreator, MaterialNameContext{}, propertyGroup.get()); if (!success) { @@ -807,6 +824,8 @@ namespace AZ } } + MaterialNameContext nameContext; + // 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) @@ -816,7 +835,8 @@ namespace AZ materialTypeSourceFilePath, materialTypeAssetCreator.GetMaterialPropertiesLayout(), materialTypeAssetCreator.GetMaterialShaderResourceGroupLayout(), - materialTypeAssetCreator.GetShaderCollection() + materialTypeAssetCreator.GetShaderCollection(), + &nameContext ) ); diff --git a/Gems/Atom/RPI/Code/Source/RPI.Public/Material/MaterialSystem.cpp b/Gems/Atom/RPI/Code/Source/RPI.Public/Material/MaterialSystem.cpp index b76fa9d676..ae7fe6cc60 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Public/Material/MaterialSystem.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Public/Material/MaterialSystem.cpp @@ -27,6 +27,7 @@ namespace AZ MaterialAsset::Reflect(context); MaterialPropertiesLayout::Reflect(context); MaterialFunctor::Reflect(context); + MaterialNameContext::Reflect(context); LuaMaterialFunctor::Reflect(context); ReflectMaterialDynamicMetadata(context); } diff --git a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/LuaMaterialFunctor.cpp b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/LuaMaterialFunctor.cpp index 4212583e60..7eedb82ecc 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/LuaMaterialFunctor.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/LuaMaterialFunctor.cpp @@ -29,9 +29,7 @@ namespace AZ serializeContext->Class() ->Version(1) ->Field("scriptAsset", &LuaMaterialFunctor::m_scriptAsset) - ->Field("propertyNamePrefix", &LuaMaterialFunctor::m_propertyNamePrefix) - ->Field("srgNamePrefix", &LuaMaterialFunctor::m_srgNamePrefix) - ->Field("optionsNamePrefix", &LuaMaterialFunctor::m_optionsNamePrefix) + ->Field("materialNameContext", &LuaMaterialFunctor::m_materialNameContext) ; } } @@ -127,7 +125,7 @@ namespace AZ if (m_scriptStatus == ScriptStatus::Ready) { - LuaMaterialFunctorRuntimeContext luaContext{&context, &GetMaterialPropertyDependencies(), m_propertyNamePrefix, m_srgNamePrefix, m_optionsNamePrefix}; + LuaMaterialFunctorRuntimeContext luaContext{&context, &GetMaterialPropertyDependencies(), m_materialNameContext}; AZ::ScriptDataContext call; if (m_scriptContext->Call("Process", call)) { @@ -145,7 +143,7 @@ namespace AZ if (m_scriptStatus == ScriptStatus::Ready) { - LuaMaterialFunctorEditorContext luaContext{&context, &GetMaterialPropertyDependencies(), m_propertyNamePrefix, m_srgNamePrefix, m_optionsNamePrefix}; + LuaMaterialFunctorEditorContext luaContext{&context, &GetMaterialPropertyDependencies(), m_materialNameContext}; AZ::ScriptDataContext call; if (m_scriptContext->Call("ProcessEditor", call)) { @@ -157,27 +155,19 @@ namespace AZ LuaMaterialFunctorCommonContext::LuaMaterialFunctorCommonContext(MaterialFunctor::RuntimeContext* runtimeContextImpl, const MaterialPropertyFlags* materialPropertyDependencies, - const AZStd::string& propertyNamePrefix, - const AZStd::string& srgNamePrefix, - const AZStd::string& optionsNamePrefix) + const MaterialNameContext& materialNameContext) : m_runtimeContextImpl(runtimeContextImpl) , m_materialPropertyDependencies(materialPropertyDependencies) - , m_propertyNamePrefix(propertyNamePrefix) - , m_srgNamePrefix(srgNamePrefix) - , m_optionsNamePrefix(optionsNamePrefix) + , m_materialNameContext(materialNameContext) { } LuaMaterialFunctorCommonContext::LuaMaterialFunctorCommonContext(MaterialFunctor::EditorContext* editorContextImpl, const MaterialPropertyFlags* materialPropertyDependencies, - const AZStd::string& propertyNamePrefix, - const AZStd::string& srgNamePrefix, - const AZStd::string& optionsNamePrefix) + const MaterialNameContext& materialNameContext) : m_editorContextImpl(editorContextImpl) , m_materialPropertyDependencies(materialPropertyDependencies) - , m_propertyNamePrefix(propertyNamePrefix) - , m_srgNamePrefix(srgNamePrefix) - , m_optionsNamePrefix(optionsNamePrefix) + , m_materialNameContext(materialNameContext) { } @@ -256,7 +246,8 @@ namespace AZ { MaterialPropertyIndex propertyIndex; - Name propertyFullName{m_propertyNamePrefix + name}; + Name propertyFullName{name}; + m_materialNameContext.ContextualizeProperty(propertyFullName); propertyIndex = GetMaterialPropertiesLayout()->FindPropertyIndex(propertyFullName); @@ -361,10 +352,8 @@ namespace AZ LuaMaterialFunctorRuntimeContext::LuaMaterialFunctorRuntimeContext(MaterialFunctor::RuntimeContext* runtimeContextImpl, const MaterialPropertyFlags* materialPropertyDependencies, - const AZStd::string& propertyNamePrefix, - const AZStd::string& srgNamePrefix, - const AZStd::string& optionsNamePrefix) - : LuaMaterialFunctorCommonContext(runtimeContextImpl, materialPropertyDependencies, propertyNamePrefix, srgNamePrefix, optionsNamePrefix) + const MaterialNameContext& materialNameContext) + : LuaMaterialFunctorCommonContext(runtimeContextImpl, materialPropertyDependencies, materialNameContext) , m_runtimeContextImpl(runtimeContextImpl) { } @@ -379,7 +368,8 @@ namespace AZ { bool didSetOne = false; - Name fullOptionName{m_optionsNamePrefix + name}; + Name fullOptionName{name}; + m_materialNameContext.ContextualizeShaderOption(fullOptionName); for (AZStd::size_t i = 0; i < m_runtimeContextImpl->m_shaderCollection->size(); ++i) { @@ -429,7 +419,8 @@ namespace AZ RHI::ShaderInputConstantIndex LuaMaterialFunctorRuntimeContext::GetShaderInputConstantIndex(const char* name, const char* functionName) const { - Name fullInputName{m_srgNamePrefix + name}; + Name fullInputName{name}; + m_materialNameContext.ContextualizeSrgInput(fullInputName); RHI::ShaderInputConstantIndex index = m_runtimeContextImpl->m_shaderResourceGroup->FindShaderInputConstantIndex(fullInputName); @@ -524,10 +515,8 @@ namespace AZ LuaMaterialFunctorEditorContext::LuaMaterialFunctorEditorContext(MaterialFunctor::EditorContext* editorContextImpl, const MaterialPropertyFlags* materialPropertyDependencies, - const AZStd::string& propertyNamePrefix, - const AZStd::string& srgNamePrefix, - const AZStd::string& optionsNamePrefix) - : LuaMaterialFunctorCommonContext(editorContextImpl, materialPropertyDependencies, propertyNamePrefix, srgNamePrefix, optionsNamePrefix) + const MaterialNameContext& materialNameContext) + : LuaMaterialFunctorCommonContext(editorContextImpl, materialPropertyDependencies, materialNameContext) , m_editorContextImpl(editorContextImpl) { } @@ -598,7 +587,9 @@ namespace AZ { if (m_editorContextImpl) { - return m_editorContextImpl->SetMaterialPropertyGroupVisibility(Name{m_propertyNamePrefix + name}, visibility); + Name fullName{name}; + m_materialNameContext.ContextualizeProperty(fullName); + return m_editorContextImpl->SetMaterialPropertyGroupVisibility(fullName, visibility); } return false; } @@ -607,7 +598,9 @@ namespace AZ { if (m_editorContextImpl) { - return m_editorContextImpl->SetMaterialPropertyVisibility(Name{m_propertyNamePrefix + name}, visibility); + Name fullName{name}; + m_materialNameContext.ContextualizeProperty(fullName); + return m_editorContextImpl->SetMaterialPropertyVisibility(fullName, visibility); } return false; } @@ -616,7 +609,9 @@ namespace AZ { if (m_editorContextImpl) { - return m_editorContextImpl->SetMaterialPropertyDescription(Name{m_propertyNamePrefix + name}, description); + Name fullName{name}; + m_materialNameContext.ContextualizeProperty(fullName); + return m_editorContextImpl->SetMaterialPropertyDescription(fullName, description); } return false; } diff --git a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialNameContext.cpp b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialNameContext.cpp new file mode 100644 index 0000000000..bf457a0a93 --- /dev/null +++ b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialNameContext.cpp @@ -0,0 +1,89 @@ +/* + * 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 AZ +{ + namespace RPI + { + void MaterialNameContext::Reflect(ReflectContext* context) + { + if (auto* serializeContext = azrtti_cast(context)) + { + serializeContext->Class() + ->Version(1) + ->Field("propertyIdContext", &MaterialNameContext::m_propertyIdContext) + ->Field("srgInputNameContext", &MaterialNameContext::m_srgInputNameContext) + ->Field("shaderOptionNameContext", &MaterialNameContext::m_shaderOptionNameContext) + ; + } + } + + bool MaterialNameContext::IsDefault() const + { + return m_propertyIdContext.empty() && m_srgInputNameContext.empty() && m_shaderOptionNameContext.empty(); + } + + void MaterialNameContext::ExtendPropertyIdContext(AZStd::string_view nameContext, bool insertDelimiter) + { + m_propertyIdContext += nameContext; + if (insertDelimiter && !nameContext.empty() && !nameContext.ends_with(".")) + { + m_propertyIdContext += "."; + } + } + + void MaterialNameContext::ExtendSrgInputContext(AZStd::string_view nameContext) + { + m_srgInputNameContext += nameContext; + } + + void MaterialNameContext::ExtendShaderOptionContext(AZStd::string_view nameContext) + { + m_shaderOptionNameContext += nameContext; + } + + bool MaterialNameContext::ContextualizeProperty(Name& propertyName) const + { + if (m_propertyIdContext.empty()) + { + return false; + } + + propertyName = m_propertyIdContext + propertyName.GetCStr(); + return true; + } + + bool MaterialNameContext::ContextualizeSrgInput(Name& srgInputName) const + { + if (m_srgInputNameContext.empty()) + { + return false; + } + + srgInputName = m_srgInputNameContext + srgInputName.GetCStr(); + return true; + } + + bool MaterialNameContext::ContextualizeShaderOption(Name& shaderOptionName) const + { + if (m_shaderOptionNameContext.empty()) + { + return false; + } + + shaderOptionName = m_shaderOptionNameContext + shaderOptionName.GetCStr(); + return true; + } + + } // namespace RPI +} // namespace AZ diff --git a/Gems/Atom/RPI/Code/Tests/Material/LuaMaterialFunctorTests.cpp b/Gems/Atom/RPI/Code/Tests/Material/LuaMaterialFunctorTests.cpp index 15f66916a6..1ed920ebc0 100644 --- a/Gems/Atom/RPI/Code/Tests/Material/LuaMaterialFunctorTests.cpp +++ b/Gems/Atom/RPI/Code/Tests/Material/LuaMaterialFunctorTests.cpp @@ -35,11 +35,14 @@ namespace UnitTest LuaMaterialFunctorSourceData functorSourceData; functorSourceData.m_luaScript = script; + MaterialNameContext nameContext; + MaterialFunctorSourceData::RuntimeContext createFunctorContext{ "Dummy.materialtype", materialTypeCreator.GetMaterialPropertiesLayout(), materialTypeCreator.GetMaterialShaderResourceGroupLayout(), - materialTypeCreator.GetShaderCollection() + materialTypeCreator.GetShaderCollection(), + &nameContext }; MaterialFunctorSourceData::FunctorResult result = functorSourceData.CreateFunctor(createFunctorContext); diff --git a/Gems/Atom/RPI/Code/Tests/Material/MaterialFunctorTests.cpp b/Gems/Atom/RPI/Code/Tests/Material/MaterialFunctorTests.cpp index 08049f3393..dbf911b09a 100644 --- a/Gems/Atom/RPI/Code/Tests/Material/MaterialFunctorTests.cpp +++ b/Gems/Atom/RPI/Code/Tests/Material/MaterialFunctorTests.cpp @@ -15,6 +15,8 @@ #include #include #include +#include +#include #include #include #include @@ -256,12 +258,15 @@ namespace UnitTest functorSourceData.m_registedPropertyName = registedPropertyName.GetStringView(); functorSourceData.m_unregistedPropertyName = unregistedPropertyName.GetStringView(); + MaterialNameContext nameContext; + MaterialFunctorSourceData::FunctorResult result = functorSourceData.CreateFunctor( MaterialFunctorSourceData::RuntimeContext( "Dummy.materialtype", materialTypeCreator.GetMaterialPropertiesLayout(), materialTypeCreator.GetMaterialShaderResourceGroupLayout(), - materialTypeCreator.GetShaderCollection() + materialTypeCreator.GetShaderCollection(), + &nameContext ) ); @@ -310,4 +315,169 @@ namespace UnitTest m_testMaterialTypeAsset = {}; m_testMaterialAsset = {}; } + + TEST_F(MaterialFunctorTests, UseNameContextInFunctorSourceData_PropertyLookup) + { + class FindPropertyIndexTestFunctor : public MaterialFunctor + { + public: + MaterialPropertyIndex m_foundIndex; + }; + + class FindPropertyIndexTestFunctorSourceData : public MaterialFunctorSourceData + { + public: + Name m_materialPropertyName; + + using MaterialFunctorSourceData::CreateFunctor; + FunctorResult CreateFunctor(const RuntimeContext& runtimeContext) const override + { + RPI::Ptr functor = aznew FindPropertyIndexTestFunctor; + functor->m_foundIndex = runtimeContext.FindMaterialPropertyIndex(m_materialPropertyName); + return Success(RPI::Ptr(functor)); + } + }; + + Data::Asset materialTypeAsset; + MaterialTypeAssetCreator materialTypeCreator; + materialTypeCreator.Begin(Uuid::CreateRandom()); + materialTypeCreator.BeginMaterialProperty(Name{"layer1.baseColor.factor"}, MaterialPropertyDataType::Float); + materialTypeCreator.EndMaterialProperty(); + materialTypeCreator.End(materialTypeAsset); + + FindPropertyIndexTestFunctorSourceData sourceData; + sourceData.m_materialPropertyName = "factor"; + + MaterialNameContext nameContext; + nameContext.ExtendPropertyIdContext("layer1"); + nameContext.ExtendPropertyIdContext("baseColor"); + + MaterialFunctorSourceData::RuntimeContext createFunctorContext( + "", + materialTypeAsset->GetMaterialPropertiesLayout(), + nullptr, + nullptr, + &nameContext); + + RPI::Ptr functor = sourceData.CreateFunctor(createFunctorContext).TakeValue(); + + EXPECT_TRUE(reinterpret_cast(functor.get())->m_foundIndex.IsValid()); + } + + TEST_F(MaterialFunctorTests, UseNameContextInFunctorSourceData_ShaderOptionLookup) + { + class FindShaderOptionIndexTestFunctor : public MaterialFunctor + { + public: + ShaderOptionIndex m_foundIndex; + }; + + class FindShaderOptionIndexTestFunctorSourceData : public MaterialFunctorSourceData + { + public: + Name m_shaderOptionName; + + using MaterialFunctorSourceData::CreateFunctor; + FunctorResult CreateFunctor(const RuntimeContext& runtimeContext) const override + { + RPI::Ptr functor = aznew FindShaderOptionIndexTestFunctor; + functor->m_foundIndex = runtimeContext.FindShaderOptionIndex(0, m_shaderOptionName); + return Success(RPI::Ptr(functor)); + } + }; + + RPI::Ptr shaderOptionLayout = RPI::ShaderOptionGroupLayout::Create(); + shaderOptionLayout->AddShaderOption( + RPI::ShaderOptionDescriptor{Name("o_layer1_baseColor_useTexture"), RPI::ShaderOptionType::Boolean, 0, 0, CreateBoolShaderOptionValues()}); + shaderOptionLayout->Finalize(); + + Data::Asset shaderAsset = CreateTestShaderAsset(Uuid::CreateRandom(), nullptr, shaderOptionLayout); + + Data::Asset materialTypeAsset; + MaterialTypeAssetCreator materialTypeCreator; + materialTypeCreator.Begin(Uuid::CreateRandom()); + materialTypeCreator.AddShader(shaderAsset); + materialTypeCreator.End(materialTypeAsset); + + FindShaderOptionIndexTestFunctorSourceData sourceData; + sourceData.m_shaderOptionName = "useTexture"; + + MaterialNameContext nameContext; + nameContext.ExtendShaderOptionContext("o_layer1_baseColor_"); + + MaterialFunctorSourceData::RuntimeContext createFunctorContext( + "", + nullptr, + nullptr, + &materialTypeAsset->GetShaderCollection(), + &nameContext); + + RPI::Ptr functor = sourceData.CreateFunctor(createFunctorContext).TakeValue(); + + EXPECT_TRUE(reinterpret_cast(functor.get())->m_foundIndex.IsValid()); + } + + TEST_F(MaterialFunctorTests, UseNameContextInFunctorSourceData_ShaderConstantLookup) + { + class FindShaderInputIndexTestFunctor : public MaterialFunctor + { + public: + RHI::ShaderInputConstantIndex m_foundConstantIndex; + RHI::ShaderInputImageIndex m_foundImageIndex; + }; + + class FindShaderInputIndexTestFunctorSourceData : public MaterialFunctorSourceData + { + public: + Name m_shaderConstantName; + Name m_shaderImageName; + + using MaterialFunctorSourceData::CreateFunctor; + FunctorResult CreateFunctor(const RuntimeContext& runtimeContext) const override + { + RPI::Ptr functor = aznew FindShaderInputIndexTestFunctor; + functor->m_foundConstantIndex = runtimeContext.FindShaderInputConstantIndex(m_shaderConstantName); + functor->m_foundImageIndex = runtimeContext.FindShaderInputImageIndex(m_shaderImageName); + return Success(RPI::Ptr(functor)); + } + }; + + AZ::RHI::Ptr srgLayout = RHI::ShaderResourceGroupLayout::Create(); + srgLayout->SetName(Name("MaterialSrg")); + srgLayout->SetUniqueId(Uuid::CreateRandom().ToString()); // Any random string will suffice. + srgLayout->SetBindingSlot(SrgBindingSlot::Material); + srgLayout->AddShaderInput(RHI::ShaderInputConstantDescriptor{Name{ "m_layer1_baseColor_factor" }, 0, 4, 0}); + srgLayout->AddShaderInput(RHI::ShaderInputImageDescriptor{Name{ "m_layer1_baseColor_texture" }, RHI::ShaderInputImageAccess::Read, RHI::ShaderInputImageType::Image2D, 1, 1}); + srgLayout->Finalize(); + + Data::Asset shaderAsset = CreateTestShaderAsset(Uuid::CreateRandom(), srgLayout); + + Data::Asset materialTypeAsset; + MaterialTypeAssetCreator materialTypeCreator; + materialTypeCreator.Begin(Uuid::CreateRandom()); + materialTypeCreator.AddShader(shaderAsset); + materialTypeCreator.End(materialTypeAsset); + + FindShaderInputIndexTestFunctorSourceData sourceData; + sourceData.m_shaderConstantName = "factor"; + sourceData.m_shaderImageName = "texture"; + + MaterialNameContext nameContext; + nameContext.ExtendSrgInputContext("m_layer1_baseColor_"); + + MaterialFunctorSourceData::RuntimeContext createFunctorContext( + "", + nullptr, + srgLayout.get(), + nullptr, + &nameContext); + + RPI::Ptr functor = sourceData.CreateFunctor(createFunctorContext).TakeValue(); + + EXPECT_TRUE(reinterpret_cast(functor.get())->m_foundConstantIndex.IsValid()); + EXPECT_TRUE(reinterpret_cast(functor.get())->m_foundImageIndex.IsValid()); + } + + + } diff --git a/Gems/Atom/RPI/Code/Tests/Material/MaterialPropertyValueSourceDataTests.cpp b/Gems/Atom/RPI/Code/Tests/Material/MaterialPropertyValueSourceDataTests.cpp index 5dde21ece1..f67a58cf97 100644 --- a/Gems/Atom/RPI/Code/Tests/Material/MaterialPropertyValueSourceDataTests.cpp +++ b/Gems/Atom/RPI/Code/Tests/Material/MaterialPropertyValueSourceDataTests.cpp @@ -19,6 +19,7 @@ #include #include #include +#include namespace JsonSerializationTests { @@ -256,13 +257,16 @@ namespace UnitTest JsonTestResult loadResult = LoadTestDataFromJson(*functorData, inputJson); + MaterialNameContext nameContext; + // Where type resolving happens. MaterialFunctorSourceData::FunctorResult functorResult = functorData->CreateFunctor( MaterialFunctorSourceData::RuntimeContext( "Dummy.materialtype", m_materialTypeCreator.GetMaterialPropertiesLayout(), m_materialTypeCreator.GetMaterialShaderResourceGroupLayout(), - m_materialTypeCreator.GetShaderCollection() + m_materialTypeCreator.GetShaderCollection(), + &nameContext ) ); diff --git a/Gems/Atom/RPI/Code/Tests/Material/MaterialTypeSourceDataTests.cpp b/Gems/Atom/RPI/Code/Tests/Material/MaterialTypeSourceDataTests.cpp index 5be02df935..98a86fe9ce 100644 --- a/Gems/Atom/RPI/Code/Tests/Material/MaterialTypeSourceDataTests.cpp +++ b/Gems/Atom/RPI/Code/Tests/Material/MaterialTypeSourceDataTests.cpp @@ -19,6 +19,7 @@ #include #include #include +#include #include #include @@ -252,6 +253,57 @@ namespace UnitTest AZStd::string m_enablePropertyName; }; + // All this functor does is save the MaterialNameContext + class SaveNameContextTestFunctor final + : public AZ::RPI::MaterialFunctor + { + public: + AZ_RTTI(SaveNameContextTestFunctor, "{FD680069-B430-4278-9E5B-A2B9617627D5}", AZ::RPI::MaterialFunctor); + + static void Reflect(AZ::ReflectContext* context) + { + if (auto* serializeContext = azrtti_cast(context)) + { + serializeContext->Class() + ->Version(1) + ->Field("nameContext", &SaveNameContextTestFunctor::m_nameContext); + } + } + + using AZ::RPI::MaterialFunctor::Process; + void Process(AZ::RPI::MaterialFunctor::RuntimeContext&) override + { + } + + MaterialNameContext m_nameContext; + }; + + // All this functor does is save the MaterialNameContext + class SaveNameContextTestFunctorSourceData final + : public MaterialFunctorSourceData + { + public: + AZ_RTTI(SaveNameContextTestFunctorSourceData, "{4261A2EC-4AB6-420E-884A-18D1A36500BE}", MaterialFunctorSourceData); + + static void Reflect(AZ::ReflectContext* context) + { + if (auto* serializeContext = azrtti_cast(context)) + { + serializeContext->Class() + ->Version(1) + ; + } + } + + using MaterialFunctorSourceData::CreateFunctor; + FunctorResult CreateFunctor([[maybe_unused]] const RuntimeContext& context) const override + { + Ptr functor = aznew SaveNameContextTestFunctor; + functor->m_nameContext = *context.GetNameContext(); + return Success(Ptr(functor)); + } + }; + ///////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////// void Reflect(ReflectContext* context) override @@ -265,6 +317,7 @@ namespace UnitTest Splat3FunctorSourceData::Reflect(context); EnableShaderFunctorSourceData::Reflect(context); SetShaderOptionFunctorSourceData::Reflect(context); + SaveNameContextTestFunctorSourceData::Reflect(context); } void SetUp() override @@ -276,6 +329,7 @@ namespace UnitTest AZ::RPI::MaterialFunctorSourceDataRegistration::Get()->RegisterMaterialFunctor("Splat3", azrtti_typeid()); AZ::RPI::MaterialFunctorSourceDataRegistration::Get()->RegisterMaterialFunctor("EnableShader", azrtti_typeid()); AZ::RPI::MaterialFunctorSourceDataRegistration::Get()->RegisterMaterialFunctor("SetShaderOption", azrtti_typeid()); + AZ::RPI::MaterialFunctorSourceDataRegistration::Get()->RegisterMaterialFunctor("SaveNameContext", azrtti_typeid()); const Name materialSrgId{"MaterialSrg"}; m_testMaterialSrgLayout = RHI::ShaderResourceGroupLayout::Create(); @@ -432,20 +486,25 @@ namespace UnitTest struct EnumeratePropertyGroupsResult { - AZStd::string m_propertyIdContext; + MaterialNameContext m_materialNameContext; const MaterialTypeSourceData::PropertyGroup* m_propertyGroup; void Check(AZStd::string expectedIdContext, const MaterialTypeSourceData::PropertyGroup* expectedPropertyGroup) { - EXPECT_EQ(expectedIdContext, m_propertyIdContext); + Name groupFullId{m_propertyGroup->GetName()}; + m_materialNameContext.ContextualizeProperty(groupFullId); + + AZStd::string expectedPropertyId = expectedIdContext + expectedPropertyGroup->GetName(); + + EXPECT_EQ(expectedPropertyId, groupFullId.GetStringView()); EXPECT_EQ(expectedPropertyGroup, m_propertyGroup); } }; AZStd::vector enumeratePropertyGroupsResults; - sourceData.EnumeratePropertyGroups([&enumeratePropertyGroupsResults](const AZStd::string& propertyIdContext, const MaterialTypeSourceData::PropertyGroup* propertyGroup) + sourceData.EnumeratePropertyGroups([&enumeratePropertyGroupsResults](const MaterialNameContext& materialNameContext, const MaterialTypeSourceData::PropertyGroup* propertyGroup) { - enumeratePropertyGroupsResults.push_back(EnumeratePropertyGroupsResult{propertyIdContext, propertyGroup}); + enumeratePropertyGroupsResults.push_back(EnumeratePropertyGroupsResult{materialNameContext, propertyGroup}); return true; }); @@ -466,20 +525,25 @@ namespace UnitTest struct EnumeratePropertiesResult { - AZStd::string m_propertyIdContext; + MaterialNameContext m_materialNameContext; const MaterialTypeSourceData::PropertyDefinition* m_propertyDefinition; void Check(AZStd::string expectedIdContext, const MaterialTypeSourceData::PropertyDefinition* expectedPropertyDefinition) { - EXPECT_EQ(expectedIdContext, m_propertyIdContext); + Name propertyFullId{m_propertyDefinition->GetName()}; + m_materialNameContext.ContextualizeProperty(propertyFullId); + + AZStd::string expectedPropertyId = expectedIdContext + expectedPropertyDefinition->GetName(); + + EXPECT_EQ(expectedPropertyId, propertyFullId.GetStringView()); EXPECT_EQ(expectedPropertyDefinition, m_propertyDefinition); } }; AZStd::vector enumeratePropertiesResults; - sourceData.EnumerateProperties([&enumeratePropertiesResults](const AZStd::string& propertyIdContext, const MaterialTypeSourceData::PropertyDefinition* propertyDefinition) + sourceData.EnumerateProperties([&enumeratePropertiesResults](const MaterialNameContext& materialNameContext, const MaterialTypeSourceData::PropertyDefinition* propertyDefinition) { - enumeratePropertiesResults.push_back(EnumeratePropertiesResult{propertyIdContext, propertyDefinition}); + enumeratePropertiesResults.push_back(EnumeratePropertiesResult{materialNameContext, propertyDefinition}); return true; }); @@ -1469,7 +1533,7 @@ namespace UnitTest // Note that serialization of individual fields within material properties is thoroughly tested in // MaterialPropertySerializerTests, so the sample property data used here is cursory. // We also don't cover fields related to providing name contexts for nested property groups, like - // "shaderInputPrefix" and "shaderOptionPrefix" as those are covered in CreateMaterialTypeAsset_NestedGroups*. + // "shaderInputsPrefix" and "shaderOptionsPrefix" as those are covered in CreateMaterialTypeAsset_NestedGroups*. const AZStd::string inputJson = R"( { @@ -2014,4 +2078,140 @@ namespace UnitTest EXPECT_EQ(materialType.FindProperty("myGroup.bar")->m_dataType, MaterialPropertyDataType::Float); } + TEST_F(MaterialTypeSourceDataTests, CreateMaterialTypeAsset_NestedGroupNameContext) + { + const Name materialSrgId{"MaterialSrg"}; + RHI::Ptr materialSrgLayout = RHI::ShaderResourceGroupLayout::Create(); + materialSrgLayout->SetName(materialSrgId); + materialSrgLayout->SetBindingSlot(SrgBindingSlot::Material); + materialSrgLayout->AddShaderInput(RHI::ShaderInputImageDescriptor{ Name{ "m_unused1" }, RHI::ShaderInputImageAccess::Read, RHI::ShaderInputImageType::Image2D, 1, 1 }); + materialSrgLayout->AddShaderInput(RHI::ShaderInputImageDescriptor{ Name{ "m_unused2" }, RHI::ShaderInputImageAccess::Read, RHI::ShaderInputImageType::Image2D, 1, 1 }); + materialSrgLayout->AddShaderInput(RHI::ShaderInputImageDescriptor{ Name{ "m_groupA_m_groupB_m_texture" }, RHI::ShaderInputImageAccess::Read, RHI::ShaderInputImageType::Image2D, 1, 1 }); + EXPECT_TRUE(materialSrgLayout->Finalize()); + + Ptr shaderOptions = ShaderOptionGroupLayout::Create(); + shaderOptions->AddShaderOption(ShaderOptionDescriptor{Name{"o_unused"}, ShaderOptionType::Boolean, 0, 0, CreateBoolShaderOptionValues()}); + shaderOptions->AddShaderOption(ShaderOptionDescriptor{Name{"o_groupA_o_groupB_o_useTexture"}, ShaderOptionType::Boolean, 1, 1, CreateBoolShaderOptionValues()}); + shaderOptions->AddShaderOption(ShaderOptionDescriptor{Name{"o_groupA_o_groupB_o_useTextureAlt"}, ShaderOptionType::Boolean, 2, 2, CreateBoolShaderOptionValues()}); + shaderOptions->Finalize(); + + Data::Asset shaderAsset = CreateTestShaderAsset(Uuid::CreateRandom(), materialSrgLayout, shaderOptions); + + Data::AssetInfo testShaderAssetInfo; + testShaderAssetInfo.m_assetId = shaderAsset.GetId(); + m_assetSystemStub.RegisterSourceInfo("NestedGroupNameContext.shader", testShaderAssetInfo, ""); + + const AZStd::string materialTypeJson = R"( + { + "propertyLayout": { + "propertyGroups": [ + { + "name": "groupA", + "shaderInputsPrefix": "m_groupA_", + "shaderOptionsPrefix": "o_groupA_", + "propertyGroups": [ + { + "name": "groupB", + "shaderInputsPrefix": "m_groupB_", + "shaderOptionsPrefix": "o_groupB_", + "propertyGroups": [ + { + "name": "groupC", + "properties": [ + { + "name": "textureMap", + "type": "Image", + "connection": { + "type": "ShaderInput", + "name": "m_texture" + } + }, + { + "name": "useTextureMap", + "type": "Bool", + "connection": [ + { + "type": "ShaderOption", + "name": "o_useTexture" + }, + { + "type": "ShaderOption", + "name": "o_useTextureAlt", + "shaderIndex": 0 // Having a specific shaderIndex traverses a different code path + } + ] + } + ], + "functors": [ + { + "type": "SaveNameContext" + } + ] + } + ] + } + ] + } + ] + }, + "shaders": [ + { + "file": "NestedGroupNameContext.shader" + } + ] + } + )"; + + MaterialTypeSourceData materialTypeSourceData; + JsonTestResult loadResult = LoadTestDataFromJson(materialTypeSourceData, materialTypeJson); + + auto materialTypeAssetOutcome = materialTypeSourceData.CreateMaterialTypeAsset(Uuid::CreateRandom()); + EXPECT_TRUE(materialTypeAssetOutcome.IsSuccess()); + + Data::Asset materialTypeAsset = materialTypeAssetOutcome.TakeValue(); + const MaterialPropertiesLayout* propertiesLayout = materialTypeAsset->GetMaterialPropertiesLayout(); + + EXPECT_EQ(2, propertiesLayout->GetPropertyCount()); + + EXPECT_EQ(0, propertiesLayout->FindPropertyIndex(Name("groupA.groupB.groupC.textureMap")).GetIndex()); + EXPECT_EQ(1, propertiesLayout->FindPropertyIndex(Name("groupA.groupB.groupC.useTextureMap")).GetIndex()); + + // groupA.gropuB.groupC.textureMap has a connection to m_groupA_m_groupB_m_texture + MaterialPropertyIndex texturePropertyIndex{0}; + EXPECT_EQ(1, propertiesLayout->GetPropertyDescriptor(texturePropertyIndex)->GetOutputConnections().size()); + EXPECT_EQ(materialSrgLayout->FindShaderInputImageIndex(Name("m_groupA_m_groupB_m_texture")).GetIndex(), + propertiesLayout->GetPropertyDescriptor(texturePropertyIndex)->GetOutputConnections()[0].m_itemIndex.GetIndex()); + + // groupA.gropuB.groupC.useTextureMap has a connection to o_groupA_o_groupB_o_useTexture and o_groupA_o_groupB_o_useTextureAlt + MaterialPropertyIndex useTexturePropertyIndex{1}; + EXPECT_EQ(2, propertiesLayout->GetPropertyDescriptor(useTexturePropertyIndex)->GetOutputConnections().size()); + EXPECT_EQ(shaderOptions->FindShaderOptionIndex(Name("o_groupA_o_groupB_o_useTexture")).GetIndex(), + propertiesLayout->GetPropertyDescriptor(useTexturePropertyIndex)->GetOutputConnections()[0].m_itemIndex.GetIndex()); + EXPECT_EQ(shaderOptions->FindShaderOptionIndex(Name("o_groupA_o_groupB_o_useTextureAlt")).GetIndex(), + propertiesLayout->GetPropertyDescriptor(useTexturePropertyIndex)->GetOutputConnections()[1].m_itemIndex.GetIndex()); + + // There should be one functor, which processes useTextureMap, and it should have the appropriate name context for constructing the correct full names. + EXPECT_EQ(1, materialTypeAsset->GetMaterialFunctors().size()); + + EXPECT_TRUE(azrtti_istypeof(materialTypeAsset->GetMaterialFunctors()[0].get())); + + auto saveNameContextFunctor = azrtti_cast(materialTypeAsset->GetMaterialFunctors()[0].get()); + const MaterialNameContext& nameContext = saveNameContextFunctor->m_nameContext; + + Name textureMapProperty{"textureMap"}; + Name textureMapShaderInput{"m_texture"}; + Name useTextureMapProperty{"useTextureMap"}; + Name useTextureShaderOption{"o_useTexture"}; + + nameContext.ContextualizeProperty(textureMapProperty); + nameContext.ContextualizeProperty(useTextureMapProperty); + nameContext.ContextualizeSrgInput(textureMapShaderInput); + nameContext.ContextualizeShaderOption(useTextureShaderOption); + + EXPECT_EQ("groupA.groupB.groupC.useTextureMap", useTextureMapProperty.GetStringView()); + EXPECT_EQ("o_groupA_o_groupB_o_useTexture", useTextureShaderOption.GetStringView()); + EXPECT_EQ("groupA.groupB.groupC.textureMap", textureMapProperty.GetStringView()); + EXPECT_EQ("m_groupA_m_groupB_m_texture", textureMapShaderInput.GetStringView()); + } + } diff --git a/Gems/Atom/RPI/Code/atom_rpi_reflect_files.cmake b/Gems/Atom/RPI/Code/atom_rpi_reflect_files.cmake index 6e9cdfeb4e..c28911f4af 100644 --- a/Gems/Atom/RPI/Code/atom_rpi_reflect_files.cmake +++ b/Gems/Atom/RPI/Code/atom_rpi_reflect_files.cmake @@ -53,6 +53,7 @@ set(FILES Include/Atom/RPI.Reflect/Material/MaterialAsset.h Include/Atom/RPI.Reflect/Material/MaterialAssetCreator.h Include/Atom/RPI.Reflect/Material/MaterialDynamicMetadata.h + Include/Atom/RPI.Reflect/Material/MaterialNameContext.h Include/Atom/RPI.Reflect/Material/MaterialPropertyDescriptor.h Include/Atom/RPI.Reflect/Material/MaterialPropertiesLayout.h Include/Atom/RPI.Reflect/Material/MaterialPropertyValue.h @@ -133,6 +134,7 @@ set(FILES Source/RPI.Reflect/Material/MaterialPropertyValue.cpp Source/RPI.Reflect/Material/MaterialAsset.cpp Source/RPI.Reflect/Material/MaterialAssetCreator.cpp + Source/RPI.Reflect/Material/MaterialNameContext.cpp Source/RPI.Reflect/Material/LuaMaterialFunctor.cpp Source/RPI.Reflect/Material/MaterialDynamicMetadata.cpp Source/RPI.Reflect/Material/MaterialPropertyDescriptor.cpp diff --git a/Gems/Atom/Tools/MaterialEditor/Code/Source/Document/MaterialDocument.cpp b/Gems/Atom/Tools/MaterialEditor/Code/Source/Document/MaterialDocument.cpp index c441762710..aaad77266c 100644 --- a/Gems/Atom/Tools/MaterialEditor/Code/Source/Document/MaterialDocument.cpp +++ b/Gems/Atom/Tools/MaterialEditor/Code/Source/Document/MaterialDocument.cpp @@ -17,6 +17,7 @@ #include #include #include +#include #include #include #include @@ -585,9 +586,10 @@ namespace MaterialEditor bool result = true; // populate sourceData with properties that meet the filter - m_materialTypeSourceData.EnumerateProperties([&](const AZStd::string& propertyIdContext, const auto& propertyDefinition) { + m_materialTypeSourceData.EnumerateProperties([&](const MaterialNameContext& materialNameContext, const auto& propertyDefinition) { - Name propertyId{propertyIdContext + propertyDefinition->GetName()}; + Name propertyId{propertyDefinition->GetName()}; + materialNameContext.ContextualizeProperty(propertyId); const auto it = m_properties.find(propertyId); if (it != m_properties.end() && propertyFilter(it->second)) @@ -781,14 +783,17 @@ namespace MaterialEditor // Populate the property map from a combination of source data and assets // Assets must still be used for now because they contain the final accumulated value after all other materials // in the hierarchy are applied - m_materialTypeSourceData.EnumeratePropertyGroups([this, &parentPropertyValues](const AZStd::string& propertyIdContext, const MaterialTypeSourceData::PropertyGroup* propertyGroup) + m_materialTypeSourceData.EnumeratePropertyGroups([this, &parentPropertyValues](const MaterialNameContext& materialNameContext, const MaterialTypeSourceData::PropertyGroup* propertyGroup) { AtomToolsFramework::DynamicPropertyConfig propertyConfig; for (const auto& propertyDefinition : propertyGroup->GetProperties()) { // Assign id before conversion so it can be used in dynamic description - propertyConfig.m_id = propertyIdContext + propertyGroup->GetName() + "." + propertyDefinition->GetName(); + MaterialNameContext groupNameContext = materialNameContext; + groupNameContext.ExtendPropertyIdContext(propertyGroup->GetName()); + propertyConfig.m_id = propertyDefinition->GetName(); + groupNameContext.ContextualizeProperty(propertyConfig.m_id); const auto& propertyIndex = m_materialAsset->GetMaterialPropertiesLayout()->FindPropertyIndex(propertyConfig.m_id); const bool propertyIndexInBounds = propertyIndex.IsValid() && propertyIndex.GetIndex() < m_materialAsset->GetPropertyValues().size(); @@ -878,8 +883,9 @@ namespace MaterialEditor } // Add material functors that are in the top-level functors list. + AZ::RPI::MaterialNameContext materialNameContext; // There is no name context for top-level functors, only functors inside PropertyGroups const MaterialFunctorSourceData::EditorContext editorContext = - MaterialFunctorSourceData::EditorContext(m_materialSourceData.m_materialType, m_materialAsset->GetMaterialPropertiesLayout()); + MaterialFunctorSourceData::EditorContext(m_materialSourceData.m_materialType, m_materialAsset->GetMaterialPropertiesLayout(), &materialNameContext); for (Ptr functorData : m_materialTypeSourceData.m_materialFunctorSourceData) { MaterialFunctorSourceData::FunctorResult result2 = functorData->CreateFunctor(editorContext); @@ -901,10 +907,10 @@ namespace MaterialEditor // Add any material functors that are located inside each property group. bool enumerateResult = m_materialTypeSourceData.EnumeratePropertyGroups( - [this](const AZStd::string&, const MaterialTypeSourceData::PropertyGroup* propertyGroup) + [this](const MaterialNameContext& nameContext, const MaterialTypeSourceData::PropertyGroup* propertyGroup) { const MaterialFunctorSourceData::EditorContext editorContext = MaterialFunctorSourceData::EditorContext( - m_materialSourceData.m_materialType, m_materialAsset->GetMaterialPropertiesLayout()); + m_materialSourceData.m_materialType, m_materialAsset->GetMaterialPropertiesLayout(), &nameContext); for (Ptr functorData : propertyGroup->GetFunctors()) { diff --git a/Gems/AtomLyIntegration/CommonFeatures/Code/Source/Material/EditorMaterialComponentInspector.cpp b/Gems/AtomLyIntegration/CommonFeatures/Code/Source/Material/EditorMaterialComponentInspector.cpp index dde81e77b5..376aba2208 100644 --- a/Gems/AtomLyIntegration/CommonFeatures/Code/Source/Material/EditorMaterialComponentInspector.cpp +++ b/Gems/AtomLyIntegration/CommonFeatures/Code/Source/Material/EditorMaterialComponentInspector.cpp @@ -15,6 +15,7 @@ #include #include #include +#include #include #include #include @@ -80,7 +81,7 @@ namespace AZ m_materialAssignmentId); } - if (!materialAssetId.IsValid()) + if (!materialAssetId.IsValid()) { UnloadMaterial(); return false; @@ -104,8 +105,9 @@ namespace AZ // Get a list of all the editor functors to be used for property editor states auto propertyLayout = m_editData.m_materialAsset->GetMaterialPropertiesLayout(); + AZ::RPI::MaterialNameContext materialNameContext; // There is no name context for top-level functors, only functors inside PropertyGroups const AZ::RPI::MaterialFunctorSourceData::EditorContext editorContext = - AZ::RPI::MaterialFunctorSourceData::EditorContext(m_editData.m_materialTypeSourcePath, propertyLayout); + AZ::RPI::MaterialFunctorSourceData::EditorContext(m_editData.m_materialTypeSourcePath, propertyLayout, &materialNameContext); for (AZ::RPI::Ptr functorData : m_editData.m_materialTypeSourceData.m_materialFunctorSourceData) { AZ::RPI::MaterialFunctorSourceData::FunctorResult createResult = functorData->CreateFunctor(editorContext); diff --git a/Gems/AtomLyIntegration/CommonFeatures/Code/Source/Material/EditorMaterialComponentUtil.cpp b/Gems/AtomLyIntegration/CommonFeatures/Code/Source/Material/EditorMaterialComponentUtil.cpp index dde9644c50..320d5e2e08 100644 --- a/Gems/AtomLyIntegration/CommonFeatures/Code/Source/Material/EditorMaterialComponentUtil.cpp +++ b/Gems/AtomLyIntegration/CommonFeatures/Code/Source/Material/EditorMaterialComponentUtil.cpp @@ -17,6 +17,7 @@ #include #include #include +#include #include #include #include @@ -113,9 +114,11 @@ namespace AZ // Copy all of the properties from the material asset to the source data that will be exported bool result = true; - editData.m_materialTypeSourceData.EnumerateProperties([&](const AZStd::string& propertyIdContext, const AZ::RPI::MaterialTypeSourceData::PropertyDefinition* propertyDefinition) + editData.m_materialTypeSourceData.EnumerateProperties([&](const AZ::RPI::MaterialNameContext& materialNameContext, const AZ::RPI::MaterialTypeSourceData::PropertyDefinition* propertyDefinition) { - AZ::Name propertyId(propertyIdContext + propertyDefinition->GetName()); + AZ::Name propertyId{propertyDefinition->GetName()}; + materialNameContext.ContextualizeProperty(propertyId); + const AZ::RPI::MaterialPropertyIndex propertyIndex = editData.m_materialAsset->GetMaterialPropertiesLayout()->FindPropertyIndex(propertyId); From a7bdb05d661e9f3dca0b353926c0737d144980bc Mon Sep 17 00:00:00 2001 From: santorac <55155825+santorac@users.noreply.github.com> Date: Tue, 1 Feb 2022 22:37:47 -0800 Subject: [PATCH 05/14] Tied up some loose ends with nested property group support. Fixed the MaterialBuilder to report source dependencies on imported JSON files, so the material type will rebuild when you edit an imported file that contains shared property data. Fixed a spot in LuaMaterialFunctorSourceData where it was only applying the old "prefix" feature and not the new name context. Updated a couple built-in material functors to take advantage of the name context (they were using RuntimeContext::GetShaderResourceGroupLayout instead of the RuntimeContext::FindShaderInputConstantIndex wrapper utility function). Fixed an issue with EnumeratePropertyGroups where it wasn't passing the right name context, so Material Editor wasn't able to load some material types. Reordered the parameters in the MaterialTypeSourceData enumerate callback functions, I felt this order was more natural. Signed-off-by: santorac <55155825+santorac@users.noreply.github.com> --- ...surfaceTransmissionParameterFunctorSourceData.cpp | 6 +++--- .../Source/Material/Transform2DFunctorSourceData.cpp | 4 ++-- .../Atom/RPI.Edit/Material/MaterialTypeSourceData.h | 8 ++++---- .../Include/Atom/RPI.Edit/Material/MaterialUtils.h | 5 ++++- .../Source/RPI.Builders/Material/MaterialBuilder.cpp | 10 +++++++++- .../Material/LuaMaterialFunctorSourceData.cpp | 5 ++++- .../RPI.Edit/Material/MaterialTypeSourceData.cpp | 10 +++++----- .../Code/Source/RPI.Edit/Material/MaterialUtils.cpp | 6 +++++- .../Tests/Material/MaterialTypeSourceDataTests.cpp | 12 ++++++------ .../Code/Source/Document/MaterialDocument.cpp | 12 +++++------- .../Source/Material/EditorMaterialComponentUtil.cpp | 4 ++-- 11 files changed, 49 insertions(+), 33 deletions(-) diff --git a/Gems/Atom/Feature/Common/Code/Source/Material/SubsurfaceTransmissionParameterFunctorSourceData.cpp b/Gems/Atom/Feature/Common/Code/Source/Material/SubsurfaceTransmissionParameterFunctorSourceData.cpp index a8ba40fb7f..ea447a422b 100644 --- a/Gems/Atom/Feature/Common/Code/Source/Material/SubsurfaceTransmissionParameterFunctorSourceData.cpp +++ b/Gems/Atom/Feature/Common/Code/Source/Material/SubsurfaceTransmissionParameterFunctorSourceData.cpp @@ -72,9 +72,9 @@ namespace AZ AddMaterialPropertyDependency(functor, functor->m_scatterDistanceColor); AddMaterialPropertyDependency(functor, functor->m_scatterDistanceIntensity); - functor->m_scatterDistance = context.GetShaderResourceGroupLayout()->FindShaderInputConstantIndex(Name{ m_scatterDistance }); - functor->m_transmissionParams = context.GetShaderResourceGroupLayout()->FindShaderInputConstantIndex(Name{ m_transmissionParams }); - functor->m_transmissionTintThickness = context.GetShaderResourceGroupLayout()->FindShaderInputConstantIndex(Name{ m_transmissionTintThickness }); + functor->m_scatterDistance = context.FindShaderInputConstantIndex(Name{ m_scatterDistance }); + functor->m_transmissionParams = context.FindShaderInputConstantIndex(Name{ m_transmissionParams }); + functor->m_transmissionTintThickness = context.FindShaderInputConstantIndex(Name{ m_transmissionTintThickness }); if (functor->m_scatterDistance.IsNull()) { diff --git a/Gems/Atom/Feature/Common/Code/Source/Material/Transform2DFunctorSourceData.cpp b/Gems/Atom/Feature/Common/Code/Source/Material/Transform2DFunctorSourceData.cpp index 26e7983a4b..dd7b9b891a 100644 --- a/Gems/Atom/Feature/Common/Code/Source/Material/Transform2DFunctorSourceData.cpp +++ b/Gems/Atom/Feature/Common/Code/Source/Material/Transform2DFunctorSourceData.cpp @@ -62,7 +62,7 @@ namespace AZ AddMaterialPropertyDependency(functor, functor->m_translateY); AddMaterialPropertyDependency(functor, functor->m_rotateDegrees); - functor->m_transformMatrix = context.GetShaderResourceGroupLayout()->FindShaderInputConstantIndex(Name{m_transformMatrix}); + functor->m_transformMatrix = context.FindShaderInputConstantIndex(Name{m_transformMatrix}); if (functor->m_transformMatrix.IsNull()) { @@ -74,7 +74,7 @@ namespace AZ // In that case, the.materialtype file will not provide the name of an inverse matrix because it doesn't have one. if (!m_transformMatrixInverse.empty()) { - functor->m_transformMatrixInverse = context.GetShaderResourceGroupLayout()->FindShaderInputConstantIndex(Name{m_transformMatrixInverse}); + functor->m_transformMatrixInverse = context.FindShaderInputConstantIndex(Name{m_transformMatrixInverse}); if (functor->m_transformMatrixInverse.IsNull()) { 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 70d92ffccf..9aa31af97a 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 @@ -284,8 +284,8 @@ namespace AZ //! Call back function type used with the enumeration functions. //! Return false to terminate the traversal. using EnumeratePropertyGroupsCallback = AZStd::function; //! Recursively traverses all of the property groups contained in the material type, executing a callback function for each. @@ -295,8 +295,8 @@ namespace AZ //! Call back function type used with the numeration functions. //! Return false to terminate the traversal. using EnumeratePropertiesCallback = AZStd::function; //! Recursively traverses all of the properties contained in the material type, executing a callback function for each. diff --git a/Gems/Atom/RPI/Code/Include/Atom/RPI.Edit/Material/MaterialUtils.h b/Gems/Atom/RPI/Code/Include/Atom/RPI.Edit/Material/MaterialUtils.h index 6ab7132109..4d1c61146d 100644 --- a/Gems/Atom/RPI/Code/Include/Atom/RPI.Edit/Material/MaterialUtils.h +++ b/Gems/Atom/RPI/Code/Include/Atom/RPI.Edit/Material/MaterialUtils.h @@ -28,6 +28,8 @@ namespace AZ namespace MaterialUtils { + using ImportedJsonFiles = AZStd::unordered_set; + enum class GetImageAssetResult { Empty, //! No image was actually requested, the path was empty @@ -52,7 +54,8 @@ namespace AZ //! Otherwise, it will use the passed in document first if not null, or load the json document from the path. //! @param filePath path to the JSON file to load, unless the @document is already provided. In either case, this path will be used to resolve any relative file references. //! @param document an optional already loaded json document. - AZ::Outcome LoadMaterialTypeSourceData(const AZStd::string& filePath, rapidjson::Document* document = nullptr); + //! @param importedFiles receives the list of files that were imported by the JSON serializer + AZ::Outcome LoadMaterialTypeSourceData(const AZStd::string& filePath, rapidjson::Document* document = nullptr, ImportedJsonFiles* importedFiles = nullptr); //! Utility function for custom JSON serializers to report results as "Skipped" when encountering keys that aren't recognized //! as part of the custom format. 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 bf65a09cef..5858c3e675 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Builders/Material/MaterialBuilder.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Builders/Material/MaterialBuilder.cpp @@ -187,13 +187,21 @@ namespace AZ const bool isMaterialTypeFile = AzFramework::StringFunc::Path::IsExtension(request.m_sourceFile.c_str(), MaterialTypeSourceData::Extension); if (isMaterialTypeFile) { - auto materialTypeSourceData = MaterialUtils::LoadMaterialTypeSourceData(fullSourcePath, &document); + MaterialUtils::ImportedJsonFiles importedJsonFiles; + auto materialTypeSourceData = MaterialUtils::LoadMaterialTypeSourceData(fullSourcePath, &document, &importedJsonFiles); if (!materialTypeSourceData.IsSuccess()) { return; } + for (auto& importedJsonFile : importedJsonFiles) + { + AssetBuilderSDK::SourceFileDependency sourceDependency; + sourceDependency.m_sourceFileDependencyPath = importedJsonFile; + response.m_sourceFileDependencyList.push_back(sourceDependency); + } + for (auto& shader : materialTypeSourceData.GetValue().m_shaderCollection) { AddPossibleDependencies(request.m_sourceFile, diff --git a/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/LuaMaterialFunctorSourceData.cpp b/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/LuaMaterialFunctorSourceData.cpp index c614222ee6..2477360b8c 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/LuaMaterialFunctorSourceData.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/LuaMaterialFunctorSourceData.cpp @@ -198,7 +198,10 @@ namespace AZ for (const Name& materialProperty : materialPropertyDependencies.GetValue()) { - MaterialPropertyIndex index = propertiesLayout->FindPropertyIndex(Name{m_propertyNamePrefix + materialProperty.GetCStr()}); + Name propertyName{materialProperty.GetCStr()}; + functor->m_materialNameContext.ContextualizeProperty(propertyName); + + MaterialPropertyIndex index = propertiesLayout->FindPropertyIndex(propertyName); if (!index.IsValid()) { AZ_Error("LuaMaterialFunctorSourceData", false, "Property '%s' is not found in material type.", materialProperty.GetCStr()); 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 9b251213fe..725041eab4 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialTypeSourceData.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialTypeSourceData.cpp @@ -381,13 +381,13 @@ namespace AZ { for (auto& propertyGroup : inPropertyGroupList) { - if (!callback(materialNameContext, propertyGroup.get())) + MaterialNameContext materialNameContext2 = ExtendNameContext(materialNameContext, *propertyGroup); + + if (!callback(propertyGroup.get(), materialNameContext2)) { return false; // Stop processing } - MaterialNameContext materialNameContext2 = ExtendNameContext(materialNameContext, *propertyGroup); - if (!EnumeratePropertyGroups(callback, materialNameContext2, propertyGroup->m_propertyGroups)) { return false; // Stop processing @@ -415,7 +415,7 @@ namespace AZ for (auto& property : propertyGroup->m_properties) { - if (!callback(materialNameContext2, property.get())) + if (!callback(property.get(), materialNameContext2)) { return false; // Stop processing } @@ -483,7 +483,7 @@ namespace AZ enumValues.push_back(uvNamePair.second); } - EnumerateProperties([&enumValues](const MaterialNameContext&, const MaterialTypeSourceData::PropertyDefinition* property) + EnumerateProperties([&enumValues](const MaterialTypeSourceData::PropertyDefinition* property, const MaterialNameContext&) { if (property->m_dataType == AZ::RPI::MaterialPropertyDataType::Enum && property->m_enumIsUv) { diff --git a/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialUtils.cpp b/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialUtils.cpp index be5feccb80..926d9abb74 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialUtils.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialUtils.cpp @@ -74,7 +74,7 @@ namespace AZ return true; } - AZ::Outcome LoadMaterialTypeSourceData(const AZStd::string& filePath, rapidjson::Document* document) + AZ::Outcome LoadMaterialTypeSourceData(const AZStd::string& filePath, rapidjson::Document* document, ImportedJsonFiles* importedFiles) { rapidjson::Document localDocument; @@ -96,6 +96,10 @@ namespace AZ importSettings.m_importer = &jsonImporter; importSettings.m_loadedJsonPath = filePath; AZ::JsonSerializationResult::ResultCode result = AZ::JsonSerialization::ResolveImports(document->GetObject(), document->GetAllocator(), importSettings); + if (importedFiles) + { + *importedFiles = importSettings.m_importer->GetImportedFiles(); + } MaterialTypeSourceData materialType; diff --git a/Gems/Atom/RPI/Code/Tests/Material/MaterialTypeSourceDataTests.cpp b/Gems/Atom/RPI/Code/Tests/Material/MaterialTypeSourceDataTests.cpp index 98a86fe9ce..04b55b3aaf 100644 --- a/Gems/Atom/RPI/Code/Tests/Material/MaterialTypeSourceDataTests.cpp +++ b/Gems/Atom/RPI/Code/Tests/Material/MaterialTypeSourceDataTests.cpp @@ -486,8 +486,8 @@ namespace UnitTest struct EnumeratePropertyGroupsResult { - MaterialNameContext m_materialNameContext; const MaterialTypeSourceData::PropertyGroup* m_propertyGroup; + MaterialNameContext m_materialNameContext; void Check(AZStd::string expectedIdContext, const MaterialTypeSourceData::PropertyGroup* expectedPropertyGroup) { @@ -502,9 +502,9 @@ namespace UnitTest }; AZStd::vector enumeratePropertyGroupsResults; - sourceData.EnumeratePropertyGroups([&enumeratePropertyGroupsResults](const MaterialNameContext& materialNameContext, const MaterialTypeSourceData::PropertyGroup* propertyGroup) + sourceData.EnumeratePropertyGroups([&enumeratePropertyGroupsResults](const MaterialTypeSourceData::PropertyGroup* propertyGroup, const MaterialNameContext& nameContext) { - enumeratePropertyGroupsResults.push_back(EnumeratePropertyGroupsResult{materialNameContext, propertyGroup}); + enumeratePropertyGroupsResults.push_back(EnumeratePropertyGroupsResult{propertyGroup, nameContext}); return true; }); @@ -525,8 +525,8 @@ namespace UnitTest struct EnumeratePropertiesResult { - MaterialNameContext m_materialNameContext; const MaterialTypeSourceData::PropertyDefinition* m_propertyDefinition; + MaterialNameContext m_materialNameContext; void Check(AZStd::string expectedIdContext, const MaterialTypeSourceData::PropertyDefinition* expectedPropertyDefinition) { @@ -541,9 +541,9 @@ namespace UnitTest }; AZStd::vector enumeratePropertiesResults; - sourceData.EnumerateProperties([&enumeratePropertiesResults](const MaterialNameContext& materialNameContext, const MaterialTypeSourceData::PropertyDefinition* propertyDefinition) + sourceData.EnumerateProperties([&enumeratePropertiesResults](const MaterialTypeSourceData::PropertyDefinition* propertyDefinition, const MaterialNameContext& nameContext) { - enumeratePropertiesResults.push_back(EnumeratePropertiesResult{materialNameContext, propertyDefinition}); + enumeratePropertiesResults.push_back(EnumeratePropertiesResult{propertyDefinition, nameContext}); return true; }); diff --git a/Gems/Atom/Tools/MaterialEditor/Code/Source/Document/MaterialDocument.cpp b/Gems/Atom/Tools/MaterialEditor/Code/Source/Document/MaterialDocument.cpp index aaad77266c..9349ad4dc9 100644 --- a/Gems/Atom/Tools/MaterialEditor/Code/Source/Document/MaterialDocument.cpp +++ b/Gems/Atom/Tools/MaterialEditor/Code/Source/Document/MaterialDocument.cpp @@ -586,10 +586,10 @@ namespace MaterialEditor bool result = true; // populate sourceData with properties that meet the filter - m_materialTypeSourceData.EnumerateProperties([&](const MaterialNameContext& materialNameContext, const auto& propertyDefinition) { + m_materialTypeSourceData.EnumerateProperties([&](const auto& propertyDefinition, const MaterialNameContext& nameContext) { Name propertyId{propertyDefinition->GetName()}; - materialNameContext.ContextualizeProperty(propertyId); + nameContext.ContextualizeProperty(propertyId); const auto it = m_properties.find(propertyId); if (it != m_properties.end() && propertyFilter(it->second)) @@ -783,17 +783,15 @@ namespace MaterialEditor // Populate the property map from a combination of source data and assets // Assets must still be used for now because they contain the final accumulated value after all other materials // in the hierarchy are applied - m_materialTypeSourceData.EnumeratePropertyGroups([this, &parentPropertyValues](const MaterialNameContext& materialNameContext, const MaterialTypeSourceData::PropertyGroup* propertyGroup) + m_materialTypeSourceData.EnumeratePropertyGroups([this, &parentPropertyValues](const MaterialTypeSourceData::PropertyGroup* propertyGroup, const MaterialNameContext& nameContext) { AtomToolsFramework::DynamicPropertyConfig propertyConfig; for (const auto& propertyDefinition : propertyGroup->GetProperties()) { // Assign id before conversion so it can be used in dynamic description - MaterialNameContext groupNameContext = materialNameContext; - groupNameContext.ExtendPropertyIdContext(propertyGroup->GetName()); propertyConfig.m_id = propertyDefinition->GetName(); - groupNameContext.ContextualizeProperty(propertyConfig.m_id); + nameContext.ContextualizeProperty(propertyConfig.m_id); const auto& propertyIndex = m_materialAsset->GetMaterialPropertiesLayout()->FindPropertyIndex(propertyConfig.m_id); const bool propertyIndexInBounds = propertyIndex.IsValid() && propertyIndex.GetIndex() < m_materialAsset->GetPropertyValues().size(); @@ -907,7 +905,7 @@ namespace MaterialEditor // Add any material functors that are located inside each property group. bool enumerateResult = m_materialTypeSourceData.EnumeratePropertyGroups( - [this](const MaterialNameContext& nameContext, const MaterialTypeSourceData::PropertyGroup* propertyGroup) + [this](const MaterialTypeSourceData::PropertyGroup* propertyGroup, const MaterialNameContext& nameContext) { const MaterialFunctorSourceData::EditorContext editorContext = MaterialFunctorSourceData::EditorContext( m_materialSourceData.m_materialType, m_materialAsset->GetMaterialPropertiesLayout(), &nameContext); diff --git a/Gems/AtomLyIntegration/CommonFeatures/Code/Source/Material/EditorMaterialComponentUtil.cpp b/Gems/AtomLyIntegration/CommonFeatures/Code/Source/Material/EditorMaterialComponentUtil.cpp index 320d5e2e08..fd66243cdc 100644 --- a/Gems/AtomLyIntegration/CommonFeatures/Code/Source/Material/EditorMaterialComponentUtil.cpp +++ b/Gems/AtomLyIntegration/CommonFeatures/Code/Source/Material/EditorMaterialComponentUtil.cpp @@ -114,10 +114,10 @@ namespace AZ // Copy all of the properties from the material asset to the source data that will be exported bool result = true; - editData.m_materialTypeSourceData.EnumerateProperties([&](const AZ::RPI::MaterialNameContext& materialNameContext, const AZ::RPI::MaterialTypeSourceData::PropertyDefinition* propertyDefinition) + editData.m_materialTypeSourceData.EnumerateProperties([&](const AZ::RPI::MaterialTypeSourceData::PropertyDefinition* propertyDefinition, const AZ::RPI::MaterialNameContext& nameContext) { AZ::Name propertyId{propertyDefinition->GetName()}; - materialNameContext.ContextualizeProperty(propertyId); + nameContext.ContextualizeProperty(propertyId); const AZ::RPI::MaterialPropertyIndex propertyIndex = editData.m_materialAsset->GetMaterialPropertiesLayout()->FindPropertyIndex(propertyId); From 466e388f6000ca93a5911a976d2ebf58eecc3051 Mon Sep 17 00:00:00 2001 From: santorac <55155825+santorac@users.noreply.github.com> Date: Thu, 3 Feb 2022 10:28:43 -0800 Subject: [PATCH 06/14] Fixed a unit test that broke in my last commit. Signed-off-by: santorac <55155825+santorac@users.noreply.github.com> --- .../RPI.Edit/Material/MaterialTypeSourceData.h | 9 +++++---- .../Material/MaterialTypeSourceData.cpp | 18 +++++++++--------- .../Material/MaterialTypeSourceDataTests.cpp | 14 ++++++++++---- .../Code/Source/Document/MaterialDocument.cpp | 9 +++++---- 4 files changed, 29 insertions(+), 21 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 9aa31af97a..ef14555061 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 @@ -285,7 +285,8 @@ namespace AZ //! Return false to terminate the traversal. using EnumeratePropertyGroupsCallback = AZStd::function; //! Recursively traverses all of the property groups contained in the material type, executing a callback function for each. @@ -296,7 +297,7 @@ namespace AZ //! Return false to terminate the traversal. using EnumeratePropertiesCallback = AZStd::function; //! Recursively traverses all of the properties contained in the material type, executing a callback function for each. @@ -318,8 +319,8 @@ namespace AZ PropertyDefinition* FindProperty(AZStd::span parsedPropertyId, AZStd::span> inPropertyGroupList); // Function overloads for recursion, returns false to indicate that recursion should end. - bool EnumeratePropertyGroups(const EnumeratePropertyGroupsCallback& callback, MaterialNameContext materialNameContext, const AZStd::vector>& inPropertyGroupList) const; - bool EnumerateProperties(const EnumeratePropertiesCallback& callback, MaterialNameContext materialNameContext, const AZStd::vector>& inPropertyGroupList) const; + bool EnumeratePropertyGroups(const EnumeratePropertyGroupsCallback& callback, MaterialNameContext nameContext, const AZStd::vector>& inPropertyGroupList) const; + bool EnumerateProperties(const EnumeratePropertiesCallback& callback, MaterialNameContext nameContext, const AZStd::vector>& inPropertyGroupList) const; static MaterialNameContext ExtendNameContext(MaterialNameContext nameContext, const MaterialTypeSourceData::PropertyGroup& propertyGroup); 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 725041eab4..874039d605 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialTypeSourceData.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialTypeSourceData.cpp @@ -377,18 +377,18 @@ namespace AZ return parts; } - bool MaterialTypeSourceData::EnumeratePropertyGroups(const EnumeratePropertyGroupsCallback& callback, MaterialNameContext materialNameContext, const AZStd::vector>& inPropertyGroupList) const + bool MaterialTypeSourceData::EnumeratePropertyGroups(const EnumeratePropertyGroupsCallback& callback, MaterialNameContext nameContext, const AZStd::vector>& inPropertyGroupList) const { for (auto& propertyGroup : inPropertyGroupList) { - MaterialNameContext materialNameContext2 = ExtendNameContext(materialNameContext, *propertyGroup); + MaterialNameContext groupNameContext = ExtendNameContext(nameContext, *propertyGroup); - if (!callback(propertyGroup.get(), materialNameContext2)) + if (!callback(propertyGroup.get(), groupNameContext, nameContext)) { - return false; // Stop processing + return false; // Stop processing } - if (!EnumeratePropertyGroups(callback, materialNameContext2, propertyGroup->m_propertyGroups)) + if (!EnumeratePropertyGroups(callback, groupNameContext, propertyGroup->m_propertyGroups)) { return false; // Stop processing } @@ -407,21 +407,21 @@ namespace AZ return EnumeratePropertyGroups(callback, {}, m_propertyLayout.m_propertyGroups); } - bool MaterialTypeSourceData::EnumerateProperties(const EnumeratePropertiesCallback& callback, MaterialNameContext materialNameContext, const AZStd::vector>& inPropertyGroupList) const + bool MaterialTypeSourceData::EnumerateProperties(const EnumeratePropertiesCallback& callback, MaterialNameContext nameContext, const AZStd::vector>& inPropertyGroupList) const { for (auto& propertyGroup : inPropertyGroupList) { - MaterialNameContext materialNameContext2 = ExtendNameContext(materialNameContext, *propertyGroup); + MaterialNameContext groupNameContext = ExtendNameContext(nameContext, *propertyGroup); for (auto& property : propertyGroup->m_properties) { - if (!callback(property.get(), materialNameContext2)) + if (!callback(property.get(), groupNameContext)) { return false; // Stop processing } } - if (!EnumerateProperties(callback, materialNameContext2, propertyGroup->m_propertyGroups)) + if (!EnumerateProperties(callback, groupNameContext, propertyGroup->m_propertyGroups)) { return false; // Stop processing } diff --git a/Gems/Atom/RPI/Code/Tests/Material/MaterialTypeSourceDataTests.cpp b/Gems/Atom/RPI/Code/Tests/Material/MaterialTypeSourceDataTests.cpp index 04b55b3aaf..46cbb1ac46 100644 --- a/Gems/Atom/RPI/Code/Tests/Material/MaterialTypeSourceDataTests.cpp +++ b/Gems/Atom/RPI/Code/Tests/Material/MaterialTypeSourceDataTests.cpp @@ -487,24 +487,30 @@ namespace UnitTest struct EnumeratePropertyGroupsResult { const MaterialTypeSourceData::PropertyGroup* m_propertyGroup; - MaterialNameContext m_materialNameContext; + MaterialNameContext m_groupNameContext; + MaterialNameContext m_parentNameContext; void Check(AZStd::string expectedIdContext, const MaterialTypeSourceData::PropertyGroup* expectedPropertyGroup) { Name groupFullId{m_propertyGroup->GetName()}; - m_materialNameContext.ContextualizeProperty(groupFullId); + m_parentNameContext.ContextualizeProperty(groupFullId); AZStd::string expectedPropertyId = expectedIdContext + expectedPropertyGroup->GetName(); EXPECT_EQ(expectedPropertyId, groupFullId.GetStringView()); EXPECT_EQ(expectedPropertyGroup, m_propertyGroup); + + Name imaginaryPropertyName{"someChildProperty"}; + m_groupNameContext.ContextualizeProperty(imaginaryPropertyName); + EXPECT_EQ(AZStd::string(groupFullId.GetStringView()) + ".someChildProperty", imaginaryPropertyName.GetStringView()); } }; AZStd::vector enumeratePropertyGroupsResults; - sourceData.EnumeratePropertyGroups([&enumeratePropertyGroupsResults](const MaterialTypeSourceData::PropertyGroup* propertyGroup, const MaterialNameContext& nameContext) + sourceData.EnumeratePropertyGroups([&enumeratePropertyGroupsResults]( + const MaterialTypeSourceData::PropertyGroup* propertyGroup, const MaterialNameContext& groupNameContext, const MaterialNameContext& parentNameContext) { - enumeratePropertyGroupsResults.push_back(EnumeratePropertyGroupsResult{propertyGroup, nameContext}); + enumeratePropertyGroupsResults.push_back(EnumeratePropertyGroupsResult{propertyGroup, groupNameContext, parentNameContext}); return true; }); diff --git a/Gems/Atom/Tools/MaterialEditor/Code/Source/Document/MaterialDocument.cpp b/Gems/Atom/Tools/MaterialEditor/Code/Source/Document/MaterialDocument.cpp index 9349ad4dc9..e4a99b73ed 100644 --- a/Gems/Atom/Tools/MaterialEditor/Code/Source/Document/MaterialDocument.cpp +++ b/Gems/Atom/Tools/MaterialEditor/Code/Source/Document/MaterialDocument.cpp @@ -783,7 +783,8 @@ namespace MaterialEditor // Populate the property map from a combination of source data and assets // Assets must still be used for now because they contain the final accumulated value after all other materials // in the hierarchy are applied - m_materialTypeSourceData.EnumeratePropertyGroups([this, &parentPropertyValues](const MaterialTypeSourceData::PropertyGroup* propertyGroup, const MaterialNameContext& nameContext) + m_materialTypeSourceData.EnumeratePropertyGroups([this, &parentPropertyValues]( + const MaterialTypeSourceData::PropertyGroup* propertyGroup, const MaterialNameContext& groupNameContext, const MaterialNameContext& /*parentNameContext*/) { AtomToolsFramework::DynamicPropertyConfig propertyConfig; @@ -791,7 +792,7 @@ namespace MaterialEditor { // Assign id before conversion so it can be used in dynamic description propertyConfig.m_id = propertyDefinition->GetName(); - nameContext.ContextualizeProperty(propertyConfig.m_id); + groupNameContext.ContextualizeProperty(propertyConfig.m_id); const auto& propertyIndex = m_materialAsset->GetMaterialPropertiesLayout()->FindPropertyIndex(propertyConfig.m_id); const bool propertyIndexInBounds = propertyIndex.IsValid() && propertyIndex.GetIndex() < m_materialAsset->GetPropertyValues().size(); @@ -905,10 +906,10 @@ namespace MaterialEditor // Add any material functors that are located inside each property group. bool enumerateResult = m_materialTypeSourceData.EnumeratePropertyGroups( - [this](const MaterialTypeSourceData::PropertyGroup* propertyGroup, const MaterialNameContext& nameContext) + [this](const MaterialTypeSourceData::PropertyGroup* propertyGroup, const MaterialNameContext& groupNameContext, const MaterialNameContext& /*parentNameContext*/) { const MaterialFunctorSourceData::EditorContext editorContext = MaterialFunctorSourceData::EditorContext( - m_materialSourceData.m_materialType, m_materialAsset->GetMaterialPropertiesLayout(), &nameContext); + m_materialSourceData.m_materialType, m_materialAsset->GetMaterialPropertiesLayout(), &groupNameContext); for (Ptr functorData : propertyGroup->GetFunctors()) { From 74f6d1ec742baf7cad54c64a3100564efc7c9124 Mon Sep 17 00:00:00 2001 From: santorac <55155825+santorac@users.noreply.github.com> Date: Tue, 8 Feb 2022 12:12:40 -0800 Subject: [PATCH 07/14] Cleanup up and resolving conflicts after the latest merge. Signed-off-by: santorac <55155825+santorac@users.noreply.github.com> --- .../Material/MaterialNameContext.h | 3 ++ .../Material/MaterialNameContext.cpp | 33 +++++++++++++++++++ .../Code/Source/Document/MaterialDocument.cpp | 26 +++++++++------ .../Code/Source/Document/MaterialDocument.h | 3 +- 4 files changed, 54 insertions(+), 11 deletions(-) diff --git a/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialNameContext.h b/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialNameContext.h index 3ecdfeb449..124d73ad7a 100644 --- a/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialNameContext.h +++ b/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialNameContext.h @@ -40,6 +40,9 @@ namespace AZ bool ContextualizeProperty(Name& propertyName) const; bool ContextualizeSrgInput(Name& srgInputName) const; bool ContextualizeShaderOption(Name& shaderOptionName) const; + bool ContextualizeProperty(AZStd::string& propertyName) const; + bool ContextualizeSrgInput(AZStd::string& srgInputName) const; + bool ContextualizeShaderOption(AZStd::string& shaderOptionName) const; //! Returns true if there is some non-default name context. bool HasContextForProperties() const { return !m_propertyIdContext.empty(); } diff --git a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialNameContext.cpp b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialNameContext.cpp index bf457a0a93..5846c60e05 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialNameContext.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialNameContext.cpp @@ -84,6 +84,39 @@ namespace AZ shaderOptionName = m_shaderOptionNameContext + shaderOptionName.GetCStr(); return true; } + + bool MaterialNameContext::ContextualizeProperty(AZStd::string& propertyName) const + { + if (m_propertyIdContext.empty()) + { + return false; + } + + propertyName = m_propertyIdContext + propertyName; + return true; + } + + bool MaterialNameContext::ContextualizeSrgInput(AZStd::string& srgInputName) const + { + if (m_srgInputNameContext.empty()) + { + return false; + } + + srgInputName = m_srgInputNameContext + srgInputName; + return true; + } + + bool MaterialNameContext::ContextualizeShaderOption(AZStd::string& shaderOptionName) const + { + if (m_shaderOptionNameContext.empty()) + { + return false; + } + + shaderOptionName = m_shaderOptionNameContext + shaderOptionName; + return true; + } } // namespace RPI } // namespace AZ diff --git a/Gems/Atom/Tools/MaterialEditor/Code/Source/Document/MaterialDocument.cpp b/Gems/Atom/Tools/MaterialEditor/Code/Source/Document/MaterialDocument.cpp index a66aa1dd21..648b0dd4c8 100644 --- a/Gems/Atom/Tools/MaterialEditor/Code/Source/Document/MaterialDocument.cpp +++ b/Gems/Atom/Tools/MaterialEditor/Code/Source/Document/MaterialDocument.cpp @@ -336,9 +336,9 @@ namespace MaterialEditor bool addPropertiesResult = true; // populate sourceData with properties that meet the filter - m_materialTypeSourceData.EnumerateProperties([&](const auto& propertyDefinition, const MaterialNameContext& nameContext) { + m_materialTypeSourceData.EnumerateProperties([&](const auto& propertyDefinition, const AZ::RPI::MaterialNameContext& nameContext) { - Name propertyId{propertyDefinition->GetName()}; + AZ::Name propertyId{propertyDefinition->GetName()}; nameContext.ContextualizeProperty(propertyId); const auto property = FindProperty(propertyId); @@ -551,16 +551,19 @@ namespace MaterialEditor // in the hierarchy are applied bool enumerateResult = m_materialTypeSourceData.EnumeratePropertyGroups( [this, &parentPropertyValues]( - const AZStd::string& propertyIdContext, const AZ::RPI::MaterialTypeSourceData::PropertyGroup* propertyGroup) + const AZ::RPI::MaterialTypeSourceData::PropertyGroup* propertyGroup, + const AZ::RPI::MaterialNameContext& groupNameContext, + const AZ::RPI::MaterialNameContext& parentNameContext) { // Add any material functors that are located inside each property group. - if (!AddEditorMaterialFunctors(propertyGroup->GetFunctors())) + if (!AddEditorMaterialFunctors(propertyGroup->GetFunctors(), groupNameContext)) { return false; } - + m_groups.emplace_back(aznew AtomToolsFramework::DynamicPropertyGroup); - m_groups.back()->m_name = propertyIdContext + propertyGroup->GetName(); + m_groups.back()->m_name = propertyGroup->GetName(); + parentNameContext.ContextualizeProperty(m_groups.back()->m_name); m_groups.back()->m_displayName = propertyGroup->GetDisplayName(); m_groups.back()->m_description = propertyGroup->GetDescription(); @@ -568,7 +571,8 @@ namespace MaterialEditor { // Assign id before conversion so it can be used in dynamic description AtomToolsFramework::DynamicPropertyConfig propertyConfig; - propertyConfig.m_id = m_groups.back()->m_name + "." + propertyDefinition->GetName(); + propertyConfig.m_id = propertyDefinition->GetName(); + groupNameContext.ContextualizeProperty(m_groups.back()->m_name); const auto& propertyIndex = m_materialAsset->GetMaterialPropertiesLayout()->FindPropertyIndex(propertyConfig.m_id); const bool propertyIndexInBounds = @@ -610,7 +614,8 @@ namespace MaterialEditor } // Add material functors that are in the top-level functors list. - if (!AddEditorMaterialFunctors(m_materialTypeSourceData.m_materialFunctorSourceData)) + AZ::RPI::MaterialNameContext materialNameContext; // There is no name context for top-level functors, only functors inside PropertyGroups + if (!AddEditorMaterialFunctors(m_materialTypeSourceData.m_materialFunctorSourceData, materialNameContext)) { return OpenFailed(); } @@ -736,10 +741,11 @@ namespace MaterialEditor } bool MaterialDocument::AddEditorMaterialFunctors( - const AZStd::vector>& functorSourceDataHolders) + const AZStd::vector>& functorSourceDataHolders, + const AZ::RPI::MaterialNameContext& nameContext) { const AZ::RPI::MaterialFunctorSourceData::EditorContext editorContext = AZ::RPI::MaterialFunctorSourceData::EditorContext( - m_materialSourceData.m_materialType, m_materialAsset->GetMaterialPropertiesLayout()); + m_materialSourceData.m_materialType, m_materialAsset->GetMaterialPropertiesLayout(), &nameContext); for (AZ::RPI::Ptr functorData : functorSourceDataHolders) { diff --git a/Gems/Atom/Tools/MaterialEditor/Code/Source/Document/MaterialDocument.h b/Gems/Atom/Tools/MaterialEditor/Code/Source/Document/MaterialDocument.h index 37b3e1922f..42385cfb25 100644 --- a/Gems/Atom/Tools/MaterialEditor/Code/Source/Document/MaterialDocument.h +++ b/Gems/Atom/Tools/MaterialEditor/Code/Source/Document/MaterialDocument.h @@ -82,7 +82,8 @@ namespace MaterialEditor void RestorePropertyValues(const PropertyValueMap& propertyValues); bool AddEditorMaterialFunctors( - const AZStd::vector>& functorSourceDataHolders); + const AZStd::vector>& functorSourceDataHolders, + const AZ::RPI::MaterialNameContext& nameContext); // Run editor material functor to update editor metadata. // @param dirtyFlags indicates which properties have changed, and thus which MaterialFunctors need to be run. From 1af5b28ea367e50b2a6592979684e7777d20fc5e Mon Sep 17 00:00:00 2001 From: santorac <55155825+santorac@users.noreply.github.com> Date: Tue, 8 Feb 2022 12:41:03 -0800 Subject: [PATCH 08/14] Fixed a copy-paste mistake, wrong name for a property. Signed-off-by: santorac <55155825+santorac@users.noreply.github.com> --- .../MaterialEditor/Code/Source/Document/MaterialDocument.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Gems/Atom/Tools/MaterialEditor/Code/Source/Document/MaterialDocument.cpp b/Gems/Atom/Tools/MaterialEditor/Code/Source/Document/MaterialDocument.cpp index 648b0dd4c8..04021f2def 100644 --- a/Gems/Atom/Tools/MaterialEditor/Code/Source/Document/MaterialDocument.cpp +++ b/Gems/Atom/Tools/MaterialEditor/Code/Source/Document/MaterialDocument.cpp @@ -572,7 +572,7 @@ namespace MaterialEditor // Assign id before conversion so it can be used in dynamic description AtomToolsFramework::DynamicPropertyConfig propertyConfig; propertyConfig.m_id = propertyDefinition->GetName(); - groupNameContext.ContextualizeProperty(m_groups.back()->m_name); + groupNameContext.ContextualizeProperty(propertyConfig.m_id); const auto& propertyIndex = m_materialAsset->GetMaterialPropertiesLayout()->FindPropertyIndex(propertyConfig.m_id); const bool propertyIndexInBounds = From 2a3c5b38e54f93dc68f70415e80120c0296e237a Mon Sep 17 00:00:00 2001 From: santorac <55155825+santorac@users.noreply.github.com> Date: Tue, 8 Feb 2022 14:06:53 -0800 Subject: [PATCH 09/14] Added a unit test demonstrating that MaterialSourceData can support properties and subgroups together in the same property group. Signed-off-by: santorac <55155825+santorac@users.noreply.github.com> --- .../Material/MaterialTypeSourceDataTests.cpp | 35 ++++++++++++++----- 1 file changed, 27 insertions(+), 8 deletions(-) diff --git a/Gems/Atom/RPI/Code/Tests/Material/MaterialTypeSourceDataTests.cpp b/Gems/Atom/RPI/Code/Tests/Material/MaterialTypeSourceDataTests.cpp index 46cbb1ac46..c187660377 100644 --- a/Gems/Atom/RPI/Code/Tests/Material/MaterialTypeSourceDataTests.cpp +++ b/Gems/Atom/RPI/Code/Tests/Material/MaterialTypeSourceDataTests.cpp @@ -2093,6 +2093,8 @@ namespace UnitTest materialSrgLayout->AddShaderInput(RHI::ShaderInputImageDescriptor{ Name{ "m_unused1" }, RHI::ShaderInputImageAccess::Read, RHI::ShaderInputImageType::Image2D, 1, 1 }); materialSrgLayout->AddShaderInput(RHI::ShaderInputImageDescriptor{ Name{ "m_unused2" }, RHI::ShaderInputImageAccess::Read, RHI::ShaderInputImageType::Image2D, 1, 1 }); materialSrgLayout->AddShaderInput(RHI::ShaderInputImageDescriptor{ Name{ "m_groupA_m_groupB_m_texture" }, RHI::ShaderInputImageAccess::Read, RHI::ShaderInputImageType::Image2D, 1, 1 }); + materialSrgLayout->AddShaderInput(RHI::ShaderInputConstantDescriptor{ Name{ "m_unused3" }, 0, 4, 0 }); + materialSrgLayout->AddShaderInput(RHI::ShaderInputConstantDescriptor{ Name{ "m_groupA_m_groupB_m_number" }, 4, 4, 0 }); EXPECT_TRUE(materialSrgLayout->Finalize()); Ptr shaderOptions = ShaderOptionGroupLayout::Create(); @@ -2120,6 +2122,16 @@ namespace UnitTest "name": "groupB", "shaderInputsPrefix": "m_groupB_", "shaderOptionsPrefix": "o_groupB_", + "properties": [ + { + "name": "number", + "type": "Float", + "connection": { + "type": "ShaderInput", + "name": "m_number" + } + } + ], "propertyGroups": [ { "name": "groupC", @@ -2177,25 +2189,32 @@ namespace UnitTest Data::Asset materialTypeAsset = materialTypeAssetOutcome.TakeValue(); const MaterialPropertiesLayout* propertiesLayout = materialTypeAsset->GetMaterialPropertiesLayout(); - EXPECT_EQ(2, propertiesLayout->GetPropertyCount()); - - EXPECT_EQ(0, propertiesLayout->FindPropertyIndex(Name("groupA.groupB.groupC.textureMap")).GetIndex()); - EXPECT_EQ(1, propertiesLayout->FindPropertyIndex(Name("groupA.groupB.groupC.useTextureMap")).GetIndex()); + EXPECT_EQ(3, propertiesLayout->GetPropertyCount()); + + EXPECT_EQ(0, propertiesLayout->FindPropertyIndex(Name("groupA.groupB.number")).GetIndex()); + EXPECT_EQ(1, propertiesLayout->FindPropertyIndex(Name("groupA.groupB.groupC.textureMap")).GetIndex()); + EXPECT_EQ(2, propertiesLayout->FindPropertyIndex(Name("groupA.groupB.groupC.useTextureMap")).GetIndex()); + + // groupA.groupB.number has a connection to m_groupA_m_groupB_m_number + MaterialPropertyIndex numberPropertyIndex{0}; + EXPECT_EQ(1, propertiesLayout->GetPropertyDescriptor(numberPropertyIndex)->GetOutputConnections().size()); + EXPECT_EQ(materialSrgLayout->FindShaderInputConstantIndex(Name("m_groupA_m_groupB_m_number")).GetIndex(), + propertiesLayout->GetPropertyDescriptor(numberPropertyIndex)->GetOutputConnections()[0].m_itemIndex.GetIndex()); // groupA.gropuB.groupC.textureMap has a connection to m_groupA_m_groupB_m_texture - MaterialPropertyIndex texturePropertyIndex{0}; + MaterialPropertyIndex texturePropertyIndex{1}; EXPECT_EQ(1, propertiesLayout->GetPropertyDescriptor(texturePropertyIndex)->GetOutputConnections().size()); EXPECT_EQ(materialSrgLayout->FindShaderInputImageIndex(Name("m_groupA_m_groupB_m_texture")).GetIndex(), propertiesLayout->GetPropertyDescriptor(texturePropertyIndex)->GetOutputConnections()[0].m_itemIndex.GetIndex()); - + // groupA.gropuB.groupC.useTextureMap has a connection to o_groupA_o_groupB_o_useTexture and o_groupA_o_groupB_o_useTextureAlt - MaterialPropertyIndex useTexturePropertyIndex{1}; + MaterialPropertyIndex useTexturePropertyIndex{2}; EXPECT_EQ(2, propertiesLayout->GetPropertyDescriptor(useTexturePropertyIndex)->GetOutputConnections().size()); EXPECT_EQ(shaderOptions->FindShaderOptionIndex(Name("o_groupA_o_groupB_o_useTexture")).GetIndex(), propertiesLayout->GetPropertyDescriptor(useTexturePropertyIndex)->GetOutputConnections()[0].m_itemIndex.GetIndex()); EXPECT_EQ(shaderOptions->FindShaderOptionIndex(Name("o_groupA_o_groupB_o_useTextureAlt")).GetIndex(), propertiesLayout->GetPropertyDescriptor(useTexturePropertyIndex)->GetOutputConnections()[1].m_itemIndex.GetIndex()); - + // There should be one functor, which processes useTextureMap, and it should have the appropriate name context for constructing the correct full names. EXPECT_EQ(1, materialTypeAsset->GetMaterialFunctors().size()); From 8fbd2aaaf56573fce07ddb6376c12179137d5ca8 Mon Sep 17 00:00:00 2001 From: santorac <55155825+santorac@users.noreply.github.com> Date: Wed, 9 Feb 2022 13:13:40 -0800 Subject: [PATCH 10/14] Tied up a few loose ends to support deeply nested property groups. Simplified the call back for MaterialTypeSourceData::EnumeratePropertyGroups while also providing more data. Made the Material Inspector join nested property group display names to be like "Layer 1 | Base Color", since the leaf property groups are shown as a flat list in the inspector. Fixed CreateMaterialAssetFromSourceData to include the imported json files in the list of sourceDependencies. This triggers the Material Editor to hot-reload when one of these json files changes. Updated a few places that were still assuming only one level of property group. Updated EditorMaterialComponentInspector to apply the per-property-group material functors, before it was still only applying the top-level onces. Moved some accessor function implementations to the cpp files, per feedback on another already-merged PR. Testing: Made changes to MinimalMultilayerPbr (in AtomSampleViewer) to use nested property groups, and saw the correct behavior in the Material Editor's property inspector. Used MaterialComponent's property inspector to edit a StandardPbr material instance. Confrimed that functors were correctly controlling property visibility by enabling and disabling things like emissive and clear coat. Used MaterialComponent's property inspector to edit a MinimalMultilayerPbr material instance. Saw all the expected groups and properties show up. Confirmed that per-group functors were correctly controlling property visibility. Used MaterialComponent's property inspector to export a material instance and confirmed the .material file included the expected properties. Signed-off-by: santorac <55155825+santorac@users.noreply.github.com> --- .../Material/MaterialTypeSourceData.h | 37 +++-- .../Material/MaterialNameContext.h | 6 +- .../RPI.Edit/Material/MaterialSourceData.cpp | 2 +- .../Material/MaterialTypeSourceData.cpp | 90 ++++++++-- .../Material/MaterialNameContext.cpp | 14 ++ .../Code/Source/Document/MaterialDocument.cpp | 40 +++-- .../EditorMaterialComponentInspector.cpp | 156 +++++++++++------- .../EditorMaterialComponentInspector.h | 4 + .../Material/EditorMaterialComponentUtil.cpp | 4 +- 9 files changed, 241 insertions(+), 112 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 900a57746a..507652b9fa 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 @@ -131,15 +131,17 @@ namespace AZ PropertyGroup() = default; AZ_DISABLE_COPY(PropertyGroup) - const AZStd::string& GetName() const { return m_name; } - const AZStd::string& GetDisplayName() const { return m_displayName; } - const AZStd::string& GetDescription() const { return m_description; } - const PropertyList& GetProperties() const { return m_properties; } - const AZStd::vector>& GetPropertyGroups() const { return m_propertyGroups; } - const AZStd::vector>& GetFunctors() const { return m_materialFunctorSourceData; } + const AZStd::string& GetName() const; + const AZStd::string& GetDisplayName() const; + const AZStd::string& GetDescription() const; + const PropertyList& GetProperties() const; + const AZStd::string& GetShaderInputsPrefix() const; + const AZStd::string& GetShaderOptionsPrefix() const; + const AZStd::vector>& GetPropertyGroups() const; + const AZStd::vector>& GetFunctors() const; - void SetDisplayName(AZStd::string_view displayName) { m_displayName = displayName; } - void SetDescription(AZStd::string_view description) { m_description = description; } + void SetDisplayName(AZStd::string_view displayName); + void SetDescription(AZStd::string_view description); //! Add a new property to this PropertyGroup. //! @param name a unique for the property. Must be a C-style identifier. @@ -281,13 +283,13 @@ namespace AZ //! Splits an ID string like "itemA.itemB.itemC" into a vector like ["itemA.itemB", "itemC"]. static AZStd::vector SplitId(AZStd::string_view id); + //! Describes a path in the hierarchy of property groups, with the top level group at the beginning and a leaf-most group at the end. + using PropertyGroupStack = AZStd::vector; + //! Call back function type used with the enumeration functions. + //! The PropertyGroupStack contains the stack of property groups at the current point in the traversal. //! Return false to terminate the traversal. - using EnumeratePropertyGroupsCallback = AZStd::function; + using EnumeratePropertyGroupsCallback = AZStd::function; //! Recursively traverses all of the property groups contained in the material type, executing a callback function for each. //! @return false if the enumeration was terminated early by the callback returning false. @@ -304,6 +306,9 @@ namespace AZ //! @return false if the enumeration was terminated early by the callback returning false. bool EnumerateProperties(const EnumeratePropertiesCallback& callback) const; + //! Returns a MaterialNameContext for a specific path through the property group hierarchy. + static MaterialNameContext MakeMaterialNameContext(const MaterialTypeSourceData::PropertyGroupStack& propertyGroupStack); + Outcome> CreateMaterialTypeAsset(Data::AssetId assetId, AZStd::string_view materialTypeSourceFilePath = "", bool elevateWarnings = true) const; //! If the data was loaded from an old format file (i.e. where "groups" and "properties" were separate sections), @@ -319,10 +324,10 @@ namespace AZ PropertyDefinition* FindProperty(AZStd::span parsedPropertyId, AZStd::span> inPropertyGroupList); // Function overloads for recursion, returns false to indicate that recursion should end. - bool EnumeratePropertyGroups(const EnumeratePropertyGroupsCallback& callback, MaterialNameContext nameContext, const AZStd::vector>& inPropertyGroupList) const; + bool EnumeratePropertyGroups(const EnumeratePropertyGroupsCallback& callback, PropertyGroupStack* propertyGroupStack, const AZStd::vector>& inPropertyGroupList) const; bool EnumerateProperties(const EnumeratePropertiesCallback& callback, MaterialNameContext nameContext, const AZStd::vector>& inPropertyGroupList) const; - static MaterialNameContext ExtendNameContext(MaterialNameContext nameContext, const MaterialTypeSourceData::PropertyGroup& propertyGroup); + static void ExtendNameContext(MaterialNameContext& nameContext, const MaterialTypeSourceData::PropertyGroup& propertyGroup); //! Recursively populates a material type asset with properties from the tree of material property groups. //! @param materialTypeSourceFilePath path to the material type file that is being processed, used to look up relative paths @@ -342,7 +347,7 @@ namespace AZ PropertyLayout m_propertyLayout; }; - + //! The wrapper class for derived material functors. //! It is used in deserialization so that derived material functors can be deserialized by name. class MaterialFunctorSourceDataHolder final diff --git a/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialNameContext.h b/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialNameContext.h index 124d73ad7a..ac6adffe2b 100644 --- a/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialNameContext.h +++ b/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialNameContext.h @@ -45,9 +45,9 @@ namespace AZ bool ContextualizeShaderOption(AZStd::string& shaderOptionName) const; //! Returns true if there is some non-default name context. - bool HasContextForProperties() const { return !m_propertyIdContext.empty(); } - bool HasContextForSrgInputs() const { return !m_srgInputNameContext.empty(); } - bool HasContextForShaderOptions() const { return !m_shaderOptionNameContext.empty(); } + bool HasContextForProperties() const; + bool HasContextForSrgInputs() const; + bool HasContextForShaderOptions() const; //! Returns true if the name context is empty. bool IsDefault() 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 7f2c13515a..91a7092362 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialSourceData.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialSourceData.cpp @@ -262,7 +262,7 @@ namespace AZ return Failure(); } - auto materialTypeLoadOutcome = MaterialUtils::LoadMaterialTypeSourceData(materialTypeSourcePath); + auto materialTypeLoadOutcome = MaterialUtils::LoadMaterialTypeSourceData(materialTypeSourcePath, nullptr, sourceDependencies); if (!materialTypeLoadOutcome) { AZ_Error("MaterialSourceData", false, "Failed to load MaterialTypeSourceData: '%s'.", materialTypeSourcePath.c_str()); 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 6bcaa5d519..9153120732 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialTypeSourceData.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialTypeSourceData.cpp @@ -157,6 +157,56 @@ namespace AZ toPropertyGroupList.back()->m_name = name; return toPropertyGroupList.back().get(); } + + const AZStd::string& MaterialTypeSourceData::PropertyGroup::GetName() const + { + return m_name; + } + + const AZStd::string& MaterialTypeSourceData::PropertyGroup::GetDisplayName() const + { + return m_displayName; + } + + const AZStd::string& MaterialTypeSourceData::PropertyGroup::GetDescription() const + { + return m_description; + } + + const MaterialTypeSourceData::PropertyList& MaterialTypeSourceData::PropertyGroup::GetProperties() const + { + return m_properties; + } + + const AZStd::string& MaterialTypeSourceData::PropertyGroup::GetShaderInputsPrefix() const + { + return m_shaderInputsPrefix; + } + + const AZStd::string& MaterialTypeSourceData::PropertyGroup::GetShaderOptionsPrefix() const + { + return m_shaderOptionsPrefix; + } + + const AZStd::vector>& MaterialTypeSourceData::PropertyGroup::GetPropertyGroups() const + { + return m_propertyGroups; + } + + const AZStd::vector>& MaterialTypeSourceData::PropertyGroup::GetFunctors() const + { + return m_materialFunctorSourceData; + } + + void MaterialTypeSourceData::PropertyGroup::SetDisplayName(AZStd::string_view displayName) + { + m_displayName = displayName; + } + + void MaterialTypeSourceData::PropertyGroup::SetDescription(AZStd::string_view description) + { + m_description = description; + } MaterialTypeSourceData::PropertyDefinition* MaterialTypeSourceData::PropertyGroup::AddProperty(AZStd::string_view name) { @@ -377,21 +427,23 @@ namespace AZ return parts; } - bool MaterialTypeSourceData::EnumeratePropertyGroups(const EnumeratePropertyGroupsCallback& callback, MaterialNameContext nameContext, const AZStd::vector>& inPropertyGroupList) const + bool MaterialTypeSourceData::EnumeratePropertyGroups(const EnumeratePropertyGroupsCallback& callback, PropertyGroupStack* propertyGroupStack, const AZStd::vector>& inPropertyGroupList) const { for (auto& propertyGroup : inPropertyGroupList) { - MaterialNameContext groupNameContext = ExtendNameContext(nameContext, *propertyGroup); + propertyGroupStack->push_back(propertyGroup.get()); - if (!callback(propertyGroup.get(), groupNameContext, nameContext)) + if (!callback(*propertyGroupStack)) { return false; // Stop processing } - if (!EnumeratePropertyGroups(callback, groupNameContext, propertyGroup->m_propertyGroups)) + if (!EnumeratePropertyGroups(callback, propertyGroupStack, propertyGroup->m_propertyGroups)) { return false; // Stop processing } + + propertyGroupStack->pop_back(); } return true; @@ -404,14 +456,16 @@ namespace AZ return false; } - return EnumeratePropertyGroups(callback, {}, m_propertyLayout.m_propertyGroups); + PropertyGroupStack propertyGroupStack; + return EnumeratePropertyGroups(callback, &propertyGroupStack, m_propertyLayout.m_propertyGroups); } bool MaterialTypeSourceData::EnumerateProperties(const EnumeratePropertiesCallback& callback, MaterialNameContext nameContext, const AZStd::vector>& inPropertyGroupList) const { for (auto& propertyGroup : inPropertyGroupList) { - MaterialNameContext groupNameContext = ExtendNameContext(nameContext, *propertyGroup); + MaterialNameContext groupNameContext = nameContext; + ExtendNameContext(groupNameContext, *propertyGroup); for (auto& property : propertyGroup->m_properties) { @@ -529,13 +583,21 @@ namespace AZ return groupDefinitions; } - MaterialNameContext MaterialTypeSourceData::ExtendNameContext(MaterialNameContext nameContext, const MaterialTypeSourceData::PropertyGroup& propertyGroup) + void MaterialTypeSourceData::ExtendNameContext(MaterialNameContext& nameContext, const MaterialTypeSourceData::PropertyGroup& propertyGroup) { - MaterialNameContext materialNameContext2 = nameContext; - materialNameContext2.ExtendPropertyIdContext(propertyGroup.m_name); - materialNameContext2.ExtendShaderOptionContext(propertyGroup.m_shaderOptionsPrefix); - materialNameContext2.ExtendSrgInputContext(propertyGroup.m_shaderInputsPrefix); - return materialNameContext2; + nameContext.ExtendPropertyIdContext(propertyGroup.m_name); + nameContext.ExtendShaderOptionContext(propertyGroup.m_shaderOptionsPrefix); + nameContext.ExtendSrgInputContext(propertyGroup.m_shaderInputsPrefix); + } + + /*static*/ MaterialNameContext MaterialTypeSourceData::MakeMaterialNameContext(const MaterialTypeSourceData::PropertyGroupStack& propertyGroupStack) + { + MaterialNameContext nameContext; + for (auto& group : propertyGroupStack) + { + ExtendNameContext(nameContext, *group); + } + return nameContext; } bool MaterialTypeSourceData::BuildPropertyList( @@ -549,7 +611,7 @@ namespace AZ return false; } - materialNameContext = ExtendNameContext(materialNameContext, *propertyGroup); + ExtendNameContext(materialNameContext, *propertyGroup); for (const AZStd::unique_ptr& property : propertyGroup->m_properties) { @@ -886,6 +948,6 @@ namespace AZ return Failure(); } } - + } // namespace RPI } // namespace AZ diff --git a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialNameContext.cpp b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialNameContext.cpp index 5846c60e05..9ef97c8785 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialNameContext.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialNameContext.cpp @@ -117,6 +117,20 @@ namespace AZ shaderOptionName = m_shaderOptionNameContext + shaderOptionName; return true; } + + bool MaterialNameContext::HasContextForProperties() const + { + return !m_propertyIdContext.empty(); + } + bool MaterialNameContext::HasContextForSrgInputs() const + { + return !m_srgInputNameContext.empty(); + } + + bool MaterialNameContext::HasContextForShaderOptions() const + { + return !m_shaderOptionNameContext.empty(); + } } // namespace RPI } // namespace AZ diff --git a/Gems/Atom/Tools/MaterialEditor/Code/Source/Document/MaterialDocument.cpp b/Gems/Atom/Tools/MaterialEditor/Code/Source/Document/MaterialDocument.cpp index 5605917e42..d122a7fff4 100644 --- a/Gems/Atom/Tools/MaterialEditor/Code/Source/Document/MaterialDocument.cpp +++ b/Gems/Atom/Tools/MaterialEditor/Code/Source/Document/MaterialDocument.cpp @@ -354,9 +354,7 @@ namespace MaterialEditor return false; } - // TODO: Support populating the Material Editor with nested property groups, not just the top level. - AZStd::string_view groupName = propertyId.GetStringView().substr(0, propertyId.GetStringView().size() - propertyDefinition->GetName().size() - 1); - sourceData.SetPropertyValue(AZ::RPI::MaterialPropertyId{groupName, propertyDefinition->GetName()}, propertyValue); + sourceData.SetPropertyValue(propertyId, propertyValue); } } return true; @@ -553,27 +551,38 @@ namespace MaterialEditor // Assets must still be used for now because they contain the final accumulated value after all other materials // in the hierarchy are applied bool enumerateResult = m_materialTypeSourceData.EnumeratePropertyGroups( - [this, &parentPropertyValues]( - const AZ::RPI::MaterialTypeSourceData::PropertyGroup* propertyGroup, - const AZ::RPI::MaterialNameContext& groupNameContext, - const AZ::RPI::MaterialNameContext& parentNameContext) + [this, &parentPropertyValues](const AZ::RPI::MaterialTypeSourceData::PropertyGroupStack& propertyGroupStack) { - // Add any material functors that are located inside each property group. + using namespace AZ::RPI; + + const MaterialTypeSourceData::PropertyGroup* propertyGroup = propertyGroupStack.back(); + + MaterialNameContext groupNameContext = MaterialTypeSourceData::MakeMaterialNameContext(propertyGroupStack); + if (!AddEditorMaterialFunctors(propertyGroup->GetFunctors(), groupNameContext)) { return false; } - + + AZStd::vector groupNameVector; + AZStd::vector groupDisplayNameVector; + + for (auto& group : propertyGroupStack) + { + groupNameVector.push_back(group->GetName()); + groupDisplayNameVector.push_back(group->GetDisplayName()); + } + m_groups.emplace_back(aznew AtomToolsFramework::DynamicPropertyGroup); - m_groups.back()->m_name = propertyGroup->GetName(); - parentNameContext.ContextualizeProperty(m_groups.back()->m_name); - m_groups.back()->m_displayName = propertyGroup->GetDisplayName(); m_groups.back()->m_description = propertyGroup->GetDescription(); + AzFramework::StringFunc::Join(m_groups.back()->m_name, groupNameVector.begin(), groupNameVector.end(), "."); + AzFramework::StringFunc::Join(m_groups.back()->m_displayName, groupDisplayNameVector.begin(), groupDisplayNameVector.end(), " | "); for (const auto& propertyDefinition : propertyGroup->GetProperties()) { - // Assign id before conversion so it can be used in dynamic description AtomToolsFramework::DynamicPropertyConfig propertyConfig; + + // Assign id before conversion so it can be used in dynamic description propertyConfig.m_id = propertyDefinition->GetName(); groupNameContext.ContextualizeProperty(propertyConfig.m_id); @@ -587,9 +596,8 @@ namespace MaterialEditor if (propertyIndexInBounds) { AtomToolsFramework::ConvertToPropertyConfig(propertyConfig, *propertyDefinition); - - // TODO: Support populating the Material Editor with nested property groups, not just the top level. - // (Does DynamicPropertyConfig really even need m_groupDisplayName?) + + // (Does DynamicPropertyConfig really even need m_groupName? It doesn't seem to be used anywhere) propertyConfig.m_groupName = m_groups.back()->m_name; propertyConfig.m_groupDisplayName = m_groups.back()->m_displayName; propertyConfig.m_showThumbnail = true; diff --git a/Gems/AtomLyIntegration/CommonFeatures/Code/Source/Material/EditorMaterialComponentInspector.cpp b/Gems/AtomLyIntegration/CommonFeatures/Code/Source/Material/EditorMaterialComponentInspector.cpp index 376aba2208..1c6410f8c3 100644 --- a/Gems/AtomLyIntegration/CommonFeatures/Code/Source/Material/EditorMaterialComponentInspector.cpp +++ b/Gems/AtomLyIntegration/CommonFeatures/Code/Source/Material/EditorMaterialComponentInspector.cpp @@ -102,29 +102,10 @@ namespace AZ UnloadMaterial(); return false; } + + // Add material functors that are in the top-level functors list. Other functors are also added per-property-group elsewhere. + AddEditorMaterialFunctors(m_editData.m_materialTypeSourceData.m_materialFunctorSourceData, AZ::RPI::MaterialNameContext{}); - // Get a list of all the editor functors to be used for property editor states - auto propertyLayout = m_editData.m_materialAsset->GetMaterialPropertiesLayout(); - AZ::RPI::MaterialNameContext materialNameContext; // There is no name context for top-level functors, only functors inside PropertyGroups - const AZ::RPI::MaterialFunctorSourceData::EditorContext editorContext = - AZ::RPI::MaterialFunctorSourceData::EditorContext(m_editData.m_materialTypeSourcePath, propertyLayout, &materialNameContext); - for (AZ::RPI::Ptr functorData : m_editData.m_materialTypeSourceData.m_materialFunctorSourceData) - { - AZ::RPI::MaterialFunctorSourceData::FunctorResult createResult = functorData->CreateFunctor(editorContext); - - if (createResult.IsSuccess()) - { - AZ::RPI::Ptr& functor = createResult.GetValue(); - if (functor != nullptr) - { - m_editorFunctors.push_back(functor); - } - } - else - { - AZ_Error("AZ::Render::EditorMaterialComponentInspector", false, "Material functors were not created: '%s'.", m_editData.m_materialTypeSourcePath.c_str()); - } - } Populate(); LoadOverridesFromEntity(); @@ -295,47 +276,73 @@ namespace AZ void MaterialPropertyInspector::AddPropertiesGroup() { // Copy all of the properties from the material asset to the source data that will be exported - // TODO: Support populating the Material Editor with nested property groups, not just the top level. - for (const AZStd::unique_ptr& propertyGroup : m_editData.m_materialTypeSourceData.GetPropertyLayout().m_propertyGroups) - { - const AZStd::string& groupName = propertyGroup->GetName(); - const AZStd::string& groupDisplayName = !propertyGroup->GetDisplayName().empty() ? propertyGroup->GetDisplayName() : groupName; - const AZStd::string& groupDescription = !propertyGroup->GetDescription().empty() ? propertyGroup->GetDescription() : groupDisplayName; - auto& group = m_groups[groupName]; - - group.m_properties.reserve(propertyGroup->GetProperties().size()); - for (const auto& propertyDefinition : propertyGroup->GetProperties()) + m_editData.m_materialTypeSourceData.EnumeratePropertyGroups( + [this](const AZ::RPI::MaterialTypeSourceData::PropertyGroupStack& propertyGroupStack) { - AtomToolsFramework::DynamicPropertyConfig propertyConfig; + using namespace AZ::RPI; - // Assign id before conversion so it can be used in dynamic description - propertyConfig.m_id = AZ::RPI::MaterialPropertyId(groupName, propertyDefinition->GetName()); - - AtomToolsFramework::ConvertToPropertyConfig(propertyConfig, *propertyDefinition.get()); - - const auto& propertyIndex = - m_editData.m_materialAsset->GetMaterialPropertiesLayout()->FindPropertyIndex(propertyConfig.m_id); + const MaterialTypeSourceData::PropertyGroup* propertyGroupDefinition = propertyGroupStack.back(); + + MaterialNameContext groupNameContext = MaterialTypeSourceData::MakeMaterialNameContext(propertyGroupStack); - propertyConfig.m_groupName = groupDisplayName; - propertyConfig.m_showThumbnail = true; - propertyConfig.m_defaultValue = AtomToolsFramework::ConvertToEditableType( - m_editData.m_materialTypeAsset->GetDefaultPropertyValues()[propertyIndex.GetIndex()]); - - // There is no explicit parent material here. Material instance property overrides replace the values from the - // assigned material asset. Its values should be treated as parent, for comparison, in this case. - propertyConfig.m_parentValue = AtomToolsFramework::ConvertToEditableType( - m_editData.m_materialTypeAsset->GetDefaultPropertyValues()[propertyIndex.GetIndex()]); - propertyConfig.m_originalValue = AtomToolsFramework::ConvertToEditableType( - m_editData.m_materialAsset->GetPropertyValues()[propertyIndex.GetIndex()]); - group.m_properties.emplace_back(propertyConfig); - } + AddEditorMaterialFunctors(propertyGroupDefinition->GetFunctors(), groupNameContext); - // Passing in same group as main and comparison instance to enable custom value comparison for highlighting modified properties - auto propertyGroupWidget = new AtomToolsFramework::InspectorPropertyGroupWidget( - &group, &group, group.TYPEINFO_Uuid(), this, this, GetGroupSaveStateKey(groupName), {}, - [this](const auto node) { return GetInstanceNodePropertyIndicator(node); }, 0); - AddGroup(groupName, groupDisplayName, groupDescription, propertyGroupWidget); - } + AZStd::vector groupNameVector; + AZStd::vector groupDisplayNameVector; + + for (auto& group : propertyGroupStack) + { + groupNameVector.push_back(group->GetName()); + groupDisplayNameVector.push_back(!group->GetDisplayName().empty() ? group->GetDisplayName() : group->GetName()); + } + + AZStd::string groupId; + AzFramework::StringFunc::Join(groupId, groupNameVector.begin(), groupNameVector.end(), "."); + + auto& group = m_groups[groupId]; + group.m_name = groupId; + AzFramework::StringFunc::Join(group.m_displayName, groupDisplayNameVector.begin(), groupDisplayNameVector.end(), " | "); + group.m_description = !propertyGroupDefinition->GetDescription().empty() ? propertyGroupDefinition->GetDescription() : group.m_displayName; + + group.m_properties.reserve(propertyGroupDefinition->GetProperties().size()); + for (const auto& propertyDefinition : propertyGroupDefinition->GetProperties()) + { + AtomToolsFramework::DynamicPropertyConfig propertyConfig; + + // Assign id before conversion so it can be used in dynamic description + propertyConfig.m_id = propertyDefinition->GetName(); + groupNameContext.ContextualizeProperty(propertyConfig.m_id); + + AtomToolsFramework::ConvertToPropertyConfig(propertyConfig, *propertyDefinition); + + const auto& propertyIndex = + m_editData.m_materialAsset->GetMaterialPropertiesLayout()->FindPropertyIndex(propertyConfig.m_id); + + // (Does DynamicPropertyConfig really even need m_groupName? It doesn't seem to be used anywhere) + propertyConfig.m_groupName = group.m_name; + propertyConfig.m_groupDisplayName = group.m_displayName; + propertyConfig.m_showThumbnail = true; + + propertyConfig.m_defaultValue = AtomToolsFramework::ConvertToEditableType( + m_editData.m_materialTypeAsset->GetDefaultPropertyValues()[propertyIndex.GetIndex()]); + + // There is no explicit parent material here. Material instance property overrides replace the values from the + // assigned material asset. Its values should be treated as parent, for comparison, in this case. + propertyConfig.m_parentValue = AtomToolsFramework::ConvertToEditableType( + m_editData.m_materialTypeAsset->GetDefaultPropertyValues()[propertyIndex.GetIndex()]); + propertyConfig.m_originalValue = AtomToolsFramework::ConvertToEditableType( + m_editData.m_materialAsset->GetPropertyValues()[propertyIndex.GetIndex()]); + group.m_properties.emplace_back(propertyConfig); + } + + // Passing in same group as main and comparison instance to enable custom value comparison for highlighting modified properties + auto propertyGroupWidget = new AtomToolsFramework::InspectorPropertyGroupWidget( + &group, &group, group.TYPEINFO_Uuid(), this, this, GetGroupSaveStateKey(group.m_name), {}, + [this](const auto node) { return GetInstanceNodePropertyIndicator(node); }, 0); + AddGroup(group.m_name, group.m_displayName, group.m_description, propertyGroupWidget); + + return true; + }); } void MaterialPropertyInspector::Populate() @@ -435,6 +442,37 @@ namespace AZ // m_updatePreview should be set to true here for continuous preview updates as slider/color properties change but needs // throttling } + + bool MaterialPropertyInspector::AddEditorMaterialFunctors( + const AZStd::vector>& functorSourceDataHolders, + const AZ::RPI::MaterialNameContext& nameContext) + { + // Copied from MaterialDocument::AddEditorMaterialFunctors, should be refactored at some point + + const AZ::RPI::MaterialFunctorSourceData::EditorContext editorContext = AZ::RPI::MaterialFunctorSourceData::EditorContext( + m_editData.m_materialTypeSourcePath, m_editData.m_materialAsset->GetMaterialPropertiesLayout(), &nameContext); + + for (AZ::RPI::Ptr functorData : functorSourceDataHolders) + { + AZ::RPI::MaterialFunctorSourceData::FunctorResult result = functorData->CreateFunctor(editorContext); + + if (result.IsSuccess()) + { + AZ::RPI::Ptr& functor = result.GetValue(); + if (functor != nullptr) + { + m_editorFunctors.push_back(functor); + } + } + else + { + AZ_Error("MaterialDocument", false, "Material functors were not created: '%s'.", m_editData.m_materialTypeSourcePath.c_str()); + return false; + } + } + + return true; + } void MaterialPropertyInspector::RunEditorMaterialFunctors() { diff --git a/Gems/AtomLyIntegration/CommonFeatures/Code/Source/Material/EditorMaterialComponentInspector.h b/Gems/AtomLyIntegration/CommonFeatures/Code/Source/Material/EditorMaterialComponentInspector.h index 82f46ebc90..44f1fb3491 100644 --- a/Gems/AtomLyIntegration/CommonFeatures/Code/Source/Material/EditorMaterialComponentInspector.h +++ b/Gems/AtomLyIntegration/CommonFeatures/Code/Source/Material/EditorMaterialComponentInspector.h @@ -108,6 +108,10 @@ namespace AZ void RunEditorMaterialFunctors(); void UpdateMaterialInstanceProperty(const AtomToolsFramework::DynamicProperty& property); + bool AddEditorMaterialFunctors( + const AZStd::vector>& functorSourceDataHolders, + const AZ::RPI::MaterialNameContext& nameContext); + AZ::Crc32 GetGroupSaveStateKey(const AZStd::string& groupName) const; bool IsInstanceNodePropertyModifed(const AzToolsFramework::InstanceDataNode* node) const; const char* GetInstanceNodePropertyIndicator(const AzToolsFramework::InstanceDataNode* node) const; diff --git a/Gems/AtomLyIntegration/CommonFeatures/Code/Source/Material/EditorMaterialComponentUtil.cpp b/Gems/AtomLyIntegration/CommonFeatures/Code/Source/Material/EditorMaterialComponentUtil.cpp index 35633068d5..88cbbba87b 100644 --- a/Gems/AtomLyIntegration/CommonFeatures/Code/Source/Material/EditorMaterialComponentUtil.cpp +++ b/Gems/AtomLyIntegration/CommonFeatures/Code/Source/Material/EditorMaterialComponentUtil.cpp @@ -151,9 +151,7 @@ namespace AZ return true; } - // TODO: Support populating the Material Editor with nested property groups, not just the top level. - AZStd::string_view groupName = propertyId.GetStringView().substr(0, propertyId.GetStringView().size() - propertyDefinition->GetName().size() - 1); - exportData.SetPropertyValue(RPI::MaterialPropertyId{groupName, propertyDefinition->GetName()}, propertyValue); + exportData.SetPropertyValue(propertyId, propertyValue); return true; }); From 214b53de7398cdc113985b25e9c66280f501f2bf Mon Sep 17 00:00:00 2001 From: santorac <55155825+santorac@users.noreply.github.com> Date: Wed, 9 Feb 2022 16:22:44 -0800 Subject: [PATCH 11/14] Fixed a unit test. Signed-off-by: santorac <55155825+santorac@users.noreply.github.com> --- .../Material/MaterialTypeSourceDataTests.cpp | 44 ++++++++----------- 1 file changed, 18 insertions(+), 26 deletions(-) diff --git a/Gems/Atom/RPI/Code/Tests/Material/MaterialTypeSourceDataTests.cpp b/Gems/Atom/RPI/Code/Tests/Material/MaterialTypeSourceDataTests.cpp index c187660377..67b468d5bb 100644 --- a/Gems/Atom/RPI/Code/Tests/Material/MaterialTypeSourceDataTests.cpp +++ b/Gems/Atom/RPI/Code/Tests/Material/MaterialTypeSourceDataTests.cpp @@ -486,45 +486,37 @@ namespace UnitTest struct EnumeratePropertyGroupsResult { - const MaterialTypeSourceData::PropertyGroup* m_propertyGroup; - MaterialNameContext m_groupNameContext; - MaterialNameContext m_parentNameContext; + MaterialNameContext m_nameContext; - void Check(AZStd::string expectedIdContext, const MaterialTypeSourceData::PropertyGroup* expectedPropertyGroup) + void Check(AZStd::string expectedGroupId) { - Name groupFullId{m_propertyGroup->GetName()}; - m_parentNameContext.ContextualizeProperty(groupFullId); + Name imaginaryProperty{"someChildProperty"}; + m_nameContext.ContextualizeProperty(imaginaryProperty); - AZStd::string expectedPropertyId = expectedIdContext + expectedPropertyGroup->GetName(); - - EXPECT_EQ(expectedPropertyId, groupFullId.GetStringView()); - EXPECT_EQ(expectedPropertyGroup, m_propertyGroup); - - Name imaginaryPropertyName{"someChildProperty"}; - m_groupNameContext.ContextualizeProperty(imaginaryPropertyName); - EXPECT_EQ(AZStd::string(groupFullId.GetStringView()) + ".someChildProperty", imaginaryPropertyName.GetStringView()); + EXPECT_EQ(expectedGroupId + ".someChildProperty", imaginaryProperty.GetStringView()); } }; AZStd::vector enumeratePropertyGroupsResults; sourceData.EnumeratePropertyGroups([&enumeratePropertyGroupsResults]( - const MaterialTypeSourceData::PropertyGroup* propertyGroup, const MaterialNameContext& groupNameContext, const MaterialNameContext& parentNameContext) + const MaterialTypeSourceData::PropertyGroupStack& propertyGroupStack) { - enumeratePropertyGroupsResults.push_back(EnumeratePropertyGroupsResult{propertyGroup, groupNameContext, parentNameContext}); + MaterialNameContext nameContext = MaterialTypeSourceData::MakeMaterialNameContext(propertyGroupStack); + enumeratePropertyGroupsResults.push_back(EnumeratePropertyGroupsResult{nameContext}); return true; }); int resultIndex = 0; - enumeratePropertyGroupsResults[resultIndex++].Check("", layer1); - enumeratePropertyGroupsResults[resultIndex++].Check("layer1.", layer1_baseColor); - enumeratePropertyGroupsResults[resultIndex++].Check("layer1.", layer1_roughness); - enumeratePropertyGroupsResults[resultIndex++].Check("", layer2); - enumeratePropertyGroupsResults[resultIndex++].Check("layer2.", layer2_baseColor); - enumeratePropertyGroupsResults[resultIndex++].Check("layer2.", layer2_roughness); - enumeratePropertyGroupsResults[resultIndex++].Check("layer2.", layer2_clearCoat); - enumeratePropertyGroupsResults[resultIndex++].Check("layer2.clearCoat.", layer2_clearCoat_roughness); - enumeratePropertyGroupsResults[resultIndex++].Check("layer2.clearCoat.", layer2_clearCoat_normal); - enumeratePropertyGroupsResults[resultIndex++].Check("", blend); + enumeratePropertyGroupsResults[resultIndex++].Check("layer1"); + enumeratePropertyGroupsResults[resultIndex++].Check("layer1.baseColor"); + enumeratePropertyGroupsResults[resultIndex++].Check("layer1.roughness"); + enumeratePropertyGroupsResults[resultIndex++].Check("layer2"); + enumeratePropertyGroupsResults[resultIndex++].Check("layer2.baseColor"); + enumeratePropertyGroupsResults[resultIndex++].Check("layer2.roughness"); + enumeratePropertyGroupsResults[resultIndex++].Check("layer2.clearCoat"); + enumeratePropertyGroupsResults[resultIndex++].Check("layer2.clearCoat.roughness"); + enumeratePropertyGroupsResults[resultIndex++].Check("layer2.clearCoat.normal"); + enumeratePropertyGroupsResults[resultIndex++].Check("blend"); EXPECT_EQ(resultIndex, enumeratePropertyGroupsResults.size()); // Check EnumerateProperties From 6b047fd89f657513652e689ba34b549d761a0f22 Mon Sep 17 00:00:00 2001 From: santorac <55155825+santorac@users.noreply.github.com> Date: Wed, 9 Feb 2022 19:43:25 -0800 Subject: [PATCH 12/14] Addressed code review feedback, minor updates. Signed-off-by: santorac <55155825+santorac@users.noreply.github.com> --- .../Atom/RPI.Edit/Material/MaterialTypeSourceData.h | 2 +- .../RPI.Edit/Material/MaterialTypeSourceData.cpp | 10 +++++----- .../Material/EditorMaterialComponentInspector.cpp | 8 ++++---- 3 files changed, 10 insertions(+), 10 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 507652b9fa..ee2a464290 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 @@ -324,7 +324,7 @@ namespace AZ PropertyDefinition* FindProperty(AZStd::span parsedPropertyId, AZStd::span> inPropertyGroupList); // Function overloads for recursion, returns false to indicate that recursion should end. - bool EnumeratePropertyGroups(const EnumeratePropertyGroupsCallback& callback, PropertyGroupStack* propertyGroupStack, const AZStd::vector>& inPropertyGroupList) const; + bool EnumeratePropertyGroups(const EnumeratePropertyGroupsCallback& callback, PropertyGroupStack& propertyGroupStack, const AZStd::vector>& inPropertyGroupList) const; bool EnumerateProperties(const EnumeratePropertiesCallback& callback, MaterialNameContext nameContext, const AZStd::vector>& inPropertyGroupList) const; static void ExtendNameContext(MaterialNameContext& nameContext, const MaterialTypeSourceData::PropertyGroup& propertyGroup); 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 9153120732..3d40e5551e 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialTypeSourceData.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialTypeSourceData.cpp @@ -427,13 +427,13 @@ namespace AZ return parts; } - bool MaterialTypeSourceData::EnumeratePropertyGroups(const EnumeratePropertyGroupsCallback& callback, PropertyGroupStack* propertyGroupStack, const AZStd::vector>& inPropertyGroupList) const + bool MaterialTypeSourceData::EnumeratePropertyGroups(const EnumeratePropertyGroupsCallback& callback, PropertyGroupStack& propertyGroupStack, const AZStd::vector>& inPropertyGroupList) const { for (auto& propertyGroup : inPropertyGroupList) { - propertyGroupStack->push_back(propertyGroup.get()); + propertyGroupStack.push_back(propertyGroup.get()); - if (!callback(*propertyGroupStack)) + if (!callback(propertyGroupStack)) { return false; // Stop processing } @@ -443,7 +443,7 @@ namespace AZ return false; // Stop processing } - propertyGroupStack->pop_back(); + propertyGroupStack.pop_back(); } return true; @@ -457,7 +457,7 @@ namespace AZ } PropertyGroupStack propertyGroupStack; - return EnumeratePropertyGroups(callback, &propertyGroupStack, m_propertyLayout.m_propertyGroups); + return EnumeratePropertyGroups(callback, propertyGroupStack, m_propertyLayout.m_propertyGroups); } bool MaterialTypeSourceData::EnumerateProperties(const EnumeratePropertiesCallback& callback, MaterialNameContext nameContext, const AZStd::vector>& inPropertyGroupList) const diff --git a/Gems/AtomLyIntegration/CommonFeatures/Code/Source/Material/EditorMaterialComponentInspector.cpp b/Gems/AtomLyIntegration/CommonFeatures/Code/Source/Material/EditorMaterialComponentInspector.cpp index 1c6410f8c3..37ca44fa43 100644 --- a/Gems/AtomLyIntegration/CommonFeatures/Code/Source/Material/EditorMaterialComponentInspector.cpp +++ b/Gems/AtomLyIntegration/CommonFeatures/Code/Source/Material/EditorMaterialComponentInspector.cpp @@ -275,7 +275,7 @@ namespace AZ void MaterialPropertyInspector::AddPropertiesGroup() { - // Copy all of the properties from the material asset to the source data that will be exported + // Copy all of the properties from the material asset to the populate the inspector m_editData.m_materialTypeSourceData.EnumeratePropertyGroups( [this](const AZ::RPI::MaterialTypeSourceData::PropertyGroupStack& propertyGroupStack) { @@ -290,10 +290,10 @@ namespace AZ AZStd::vector groupNameVector; AZStd::vector groupDisplayNameVector; - for (auto& group : propertyGroupStack) + for (auto& nextGroup : propertyGroupStack) { - groupNameVector.push_back(group->GetName()); - groupDisplayNameVector.push_back(!group->GetDisplayName().empty() ? group->GetDisplayName() : group->GetName()); + groupNameVector.push_back(nextGroup->GetName()); + groupDisplayNameVector.push_back(!nextGroup->GetDisplayName().empty() ? nextGroup->GetDisplayName() : nextGroup->GetName()); } AZStd::string groupId; From b67862a1331b6f31c50ffeb8cd7fadc8ce24ab4f Mon Sep 17 00:00:00 2001 From: santorac <55155825+santorac@users.noreply.github.com> Date: Fri, 11 Feb 2022 09:37:12 -0800 Subject: [PATCH 13/14] Minor updates from code review feedback. Signed-off-by: santorac <55155825+santorac@users.noreply.github.com> --- .../Atom/RPI.Edit/Material/MaterialFunctorSourceData.h | 4 ++-- .../Include/Atom/RPI.Edit/Material/MaterialTypeSourceData.h | 3 ++- .../Source/RPI.Edit/Material/LuaMaterialFunctorSourceData.cpp | 2 +- .../RPI/Code/Tests/Material/MaterialTypeSourceDataTests.cpp | 2 ++ .../MaterialEditor/Code/Source/Document/MaterialDocument.cpp | 3 +++ .../Code/Source/Material/EditorMaterialComponentInspector.cpp | 3 +++ 6 files changed, 13 insertions(+), 4 deletions(-) diff --git a/Gems/Atom/RPI/Code/Include/Atom/RPI.Edit/Material/MaterialFunctorSourceData.h b/Gems/Atom/RPI/Code/Include/Atom/RPI.Edit/Material/MaterialFunctorSourceData.h index 8bf88342c3..d26e70831b 100644 --- a/Gems/Atom/RPI/Code/Include/Atom/RPI.Edit/Material/MaterialFunctorSourceData.h +++ b/Gems/Atom/RPI/Code/Include/Atom/RPI.Edit/Material/MaterialFunctorSourceData.h @@ -112,7 +112,7 @@ namespace AZ //! Returns the name context for the functor. //! It acts like a namespace for any names that the MaterialFunctorSourceData might reference. The namespace //! is automatically applied by the other relevant functions of this RuntimeContext class. - //! Not that by default the MaterialNameContext is not saved as part of the final MaterialFunctor class + //! Note that by default the MaterialNameContext is not saved as part of the final MaterialFunctor class //! (most CreateFunctor() implementations should convert names to indexes anyway) but CreateFunctor() can //! copy it to the created MaterialFunctor for use at runtime if needed. const MaterialNameContext* GetNameContext() const { return m_materialNameContext; } @@ -148,7 +148,7 @@ namespace AZ //! Returns the name context for the functor. //! It acts like a namespace for any names that the MaterialFunctorSourceData might reference. The namespace //! is automatically applied by the other relevant functions of this RuntimeContext class. - //! Not that by default the MaterialNameContext is not saved as part of the final MaterialFunctor class + //! Note that by default the MaterialNameContext is not saved as part of the final MaterialFunctor class //! (most CreateFunctor() implementations should convert names to indexes anyway) but CreateFunctor() can //! copy it to the created MaterialFunctor for use at runtime if needed. const MaterialNameContext* GetNameContext() const { return m_materialNameContext; } 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 ee2a464290..f1e92a70ea 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 @@ -331,7 +331,8 @@ namespace AZ //! Recursively populates a material type asset with properties from the tree of material property groups. //! @param materialTypeSourceFilePath path to the material type file that is being processed, used to look up relative paths - //! @param propertyIdContext the accumulated prefix that should be applied to any property names encountered in the current @propertyGroup + //! @param materialTypeAssetCreator properties will be added to this creator + //! @param materialNameContext the accumulated name context that should be applied to any property names or connection names encountered in the current @propertyGroup //! @param propertyGroup the current PropertyGroup that is being processed //! @return false if errors are detected and processing should abort bool BuildPropertyList( diff --git a/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/LuaMaterialFunctorSourceData.cpp b/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/LuaMaterialFunctorSourceData.cpp index 2477360b8c..790b721fb7 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/LuaMaterialFunctorSourceData.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/LuaMaterialFunctorSourceData.cpp @@ -198,7 +198,7 @@ namespace AZ for (const Name& materialProperty : materialPropertyDependencies.GetValue()) { - Name propertyName{materialProperty.GetCStr()}; + Name propertyName{materialProperty}; functor->m_materialNameContext.ContextualizeProperty(propertyName); MaterialPropertyIndex index = propertiesLayout->FindPropertyIndex(propertyName); diff --git a/Gems/Atom/RPI/Code/Tests/Material/MaterialTypeSourceDataTests.cpp b/Gems/Atom/RPI/Code/Tests/Material/MaterialTypeSourceDataTests.cpp index 67b468d5bb..85d1a4ccd9 100644 --- a/Gems/Atom/RPI/Code/Tests/Material/MaterialTypeSourceDataTests.cpp +++ b/Gems/Atom/RPI/Code/Tests/Material/MaterialTypeSourceDataTests.cpp @@ -273,6 +273,8 @@ namespace UnitTest using AZ::RPI::MaterialFunctor::Process; void Process(AZ::RPI::MaterialFunctor::RuntimeContext&) override { + // Intentionally empty, this is where the functor would do it's normal processing, + // but all this test functor does is store the MaterialNameContext. } MaterialNameContext m_nameContext; diff --git a/Gems/Atom/Tools/MaterialEditor/Code/Source/Document/MaterialDocument.cpp b/Gems/Atom/Tools/MaterialEditor/Code/Source/Document/MaterialDocument.cpp index d122a7fff4..13b2519368 100644 --- a/Gems/Atom/Tools/MaterialEditor/Code/Source/Document/MaterialDocument.cpp +++ b/Gems/Atom/Tools/MaterialEditor/Code/Source/Document/MaterialDocument.cpp @@ -566,6 +566,9 @@ namespace MaterialEditor AZStd::vector groupNameVector; AZStd::vector groupDisplayNameVector; + + groupNameVector.reserve(propertyGroupStack.size()); + groupDisplayNameVector.reserve(propertyGroupStack.size()); for (auto& group : propertyGroupStack) { diff --git a/Gems/AtomLyIntegration/CommonFeatures/Code/Source/Material/EditorMaterialComponentInspector.cpp b/Gems/AtomLyIntegration/CommonFeatures/Code/Source/Material/EditorMaterialComponentInspector.cpp index 37ca44fa43..ff31f3382f 100644 --- a/Gems/AtomLyIntegration/CommonFeatures/Code/Source/Material/EditorMaterialComponentInspector.cpp +++ b/Gems/AtomLyIntegration/CommonFeatures/Code/Source/Material/EditorMaterialComponentInspector.cpp @@ -289,6 +289,9 @@ namespace AZ AZStd::vector groupNameVector; AZStd::vector groupDisplayNameVector; + + groupNameVector.reserve(propertyGroupStack.size()); + groupDisplayNameVector.reserve(propertyGroupStack.size()); for (auto& nextGroup : propertyGroupStack) { From b829c08f758b8132e98f1ede8e81c96a9ff6ddfb Mon Sep 17 00:00:00 2001 From: santorac <55155825+santorac@users.noreply.github.com> Date: Fri, 11 Feb 2022 10:12:57 -0800 Subject: [PATCH 14/14] Fixed build error. Added error message if material type file JSON import fails. Signed-off-by: santorac <55155825+santorac@users.noreply.github.com> --- .../RPI/Code/Source/RPI.Edit/Material/MaterialUtils.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialUtils.cpp b/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialUtils.cpp index 1af62d1247..f4653a076e 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialUtils.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialUtils.cpp @@ -97,6 +97,12 @@ namespace AZ importSettings.m_importer = &jsonImporter; importSettings.m_loadedJsonPath = filePath; AZ::JsonSerializationResult::ResultCode result = AZ::JsonSerialization::ResolveImports(document->GetObject(), document->GetAllocator(), importSettings); + if (result.GetProcessing() != AZ::JsonSerializationResult::Processing::Completed) + { + AZ_Error("MaterialUtils", false, "%s", result.ToString(filePath).c_str()); + return AZ::Failure(); + } + if (importedFiles) { *importedFiles = importSettings.m_importer->GetImportedFiles();