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] 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 )