From c5b128bec422a9ac716ac4f9164b204daabd29c3 Mon Sep 17 00:00:00 2001 From: santorac <55155825+santorac@users.noreply.github.com> Date: Fri, 17 Dec 2021 00:46:40 -0800 Subject: [PATCH 01/10] First pass at reworking and formalizing the way deferred material asset baking works. The feature basically works but needs more testing. Before, the material builder was loading the MaterialTypeAsset and doing some processing with it, but was avoiding declaring job dependencies that would cause reprocessing lots of assets when a shader or .materialtype file changes. Reading the asset data isn't safe when not declaring a job dependency (or when declaring a weak job dependency like OrderOnce which is the case here). This caused to several known bugs. The main change here is it no longer loads the MaterialTypeAsset at all; all other changes flow from there. The biggest changes (when deferred material processing is enabled) are ... 1) MaterialSourceData no longer loads MaterialTypeAsset. All it really needs is to determine whether a string is an image file reference or an enum value, which is easy to do by just looking for the "." for the extension. 2) MaterialAssetCreator no longer produces a finalized material asset. It no longer uses MaterialAssetCreatorCommon because that only produces a non-finalized MaterialAsset, which has very different needs for the SetPropertyValue function. (We could consider merging MaterialAssetCreatorCommon into MaterialTypeAssetCreator since that's the only subclass at this point). And it doesn't do any validation against the properties layout since that can be done at runtime. 3) Moved processing of enum property values from MaterialSourceData to MaterialAsset::Finalize (this was the only thing being done in the builder that actually needed to read the material type asset data). Also... - Updated the MaterialAsset class mostly to clarify and formalize the two different modes it can be in: whether it is finalized or not. - Merged the separate "IncludeMaterialPropertyNames" registry settings from MaterialConverterSystemComponent and MaterialBuilder into one "FinalizeMaterialAssets" setting used for both. - Removed MaterialSourceData::ApplyVersionUpdates. Now the flow of data is the same regardless of whether the materials are finalized by the AP or at runtime. Version updates are always applied on the MaterialAsset. - Added a validation check to MaterialTypeAssetCreator ensuring that once a property is renamed, the old name can never be used again for a new property. This assumption was already made previously, but not formalized, in that Material::FindPropertyIndex does not expect every caller to provide a version number for the material property name, also the material asset's list of raw property names was never versioned. The only way for this to be a safe assumption is to prevent reuse of old names. Signed-off-by: santorac <55155825+santorac@users.noreply.github.com> --- .../MaterialConverterSystemComponent.cpp | 8 +- .../MaterialConverterSystemComponent.h | 6 - .../RPI.Edit/Material/MaterialConverterBus.h | 4 - .../RPI.Edit/Material/MaterialSourceData.h | 21 +- .../Atom/RPI.Edit/Material/MaterialUtils.h | 5 + .../Atom/RPI.Reflect/Material/MaterialAsset.h | 53 +++-- .../Material/MaterialAssetCreator.h | 19 +- .../Material/MaterialPropertyValue.h | 4 + .../RPI.Builders/Material/MaterialBuilder.cpp | 62 ++--- .../Model/MaterialAssetBuilderComponent.cpp | 25 ++- .../RPI.Edit/Material/MaterialSourceData.cpp | 212 ++++++++---------- .../RPI.Edit/Material/MaterialUtils.cpp | 15 ++ .../RPI.Reflect/Material/MaterialAsset.cpp | 135 +++++++---- .../Material/MaterialAssetCreator.cpp | 115 +++------- .../Material/MaterialTypeAssetCreator.cpp | 15 ++ .../Material/MaterialVersionUpdate.cpp | 12 +- .../Code/Source/Document/MaterialDocument.cpp | 6 - 17 files changed, 362 insertions(+), 355 deletions(-) diff --git a/Gems/Atom/Feature/Common/Code/Source/Material/MaterialConverterSystemComponent.cpp b/Gems/Atom/Feature/Common/Code/Source/Material/MaterialConverterSystemComponent.cpp index fd1c836b96..d5096d7a0b 100644 --- a/Gems/Atom/Feature/Common/Code/Source/Material/MaterialConverterSystemComponent.cpp +++ b/Gems/Atom/Feature/Common/Code/Source/Material/MaterialConverterSystemComponent.cpp @@ -28,8 +28,7 @@ namespace AZ serializeContext->Class() ->Version(2) ->Field("Enable", &MaterialConverterSettings::m_enable) - ->Field("DefaultMaterial", &MaterialConverterSettings::m_defaultMaterial) - ->Field("IncludeMaterialPropertyNames", &MaterialConverterSettings::m_includeMaterialPropertyNames); + ->Field("DefaultMaterial", &MaterialConverterSettings::m_defaultMaterial); } } @@ -70,11 +69,6 @@ namespace AZ return m_settings.m_enable; } - bool MaterialConverterSystemComponent::ShouldIncludeMaterialPropertyNames() const - { - return m_settings.m_includeMaterialPropertyNames; - } - bool MaterialConverterSystemComponent::ConvertMaterial( const AZ::SceneAPI::DataTypes::IMaterialData& materialData, RPI::MaterialSourceData& sourceData) { diff --git a/Gems/Atom/Feature/Common/Code/Source/Material/MaterialConverterSystemComponent.h b/Gems/Atom/Feature/Common/Code/Source/Material/MaterialConverterSystemComponent.h index 150529b474..7d95024759 100644 --- a/Gems/Atom/Feature/Common/Code/Source/Material/MaterialConverterSystemComponent.h +++ b/Gems/Atom/Feature/Common/Code/Source/Material/MaterialConverterSystemComponent.h @@ -26,11 +26,6 @@ namespace AZ bool m_enable = true; AZStd::string m_defaultMaterial; - //! Sets whether to include material property names when generating material assets. If this - //! setting is true, material property name resolution and validation is deferred into load - //! time rather than at build time, allowing to break some dependencies (e.g. fbx files will no - //! longer need to be dependent on materialtype files). - bool m_includeMaterialPropertyNames = true; }; //! Atom's implementation of converting SceneAPI data into Atom's default material: StandardPBR @@ -50,7 +45,6 @@ namespace AZ // MaterialConverterBus overrides ... bool IsEnabled() const override; - bool ShouldIncludeMaterialPropertyNames() const override; bool ConvertMaterial(const AZ::SceneAPI::DataTypes::IMaterialData& materialData, RPI::MaterialSourceData& out) override; AZStd::string GetMaterialTypePath() const override; AZStd::string GetDefaultMaterialPath() const override; diff --git a/Gems/Atom/RPI/Code/Include/Atom/RPI.Edit/Material/MaterialConverterBus.h b/Gems/Atom/RPI/Code/Include/Atom/RPI.Edit/Material/MaterialConverterBus.h index 1807fca15e..8052fc4feb 100644 --- a/Gems/Atom/RPI/Code/Include/Atom/RPI.Edit/Material/MaterialConverterBus.h +++ b/Gems/Atom/RPI/Code/Include/Atom/RPI.Edit/Material/MaterialConverterBus.h @@ -32,10 +32,6 @@ namespace AZ virtual bool IsEnabled() const = 0; - //! Returns true if material property names should be included in azmaterials. This allows unlinking of dependencies for some - //! file types to materialtype files (e.g. fbx). - virtual bool ShouldIncludeMaterialPropertyNames() const = 0; - //! Converts data from a IMaterialData object to an Atom MaterialSourceData. //! Only works when IsEnabled() is true. //! @return true if the MaterialSourceData output was populated with converted material data. diff --git a/Gems/Atom/RPI/Code/Include/Atom/RPI.Edit/Material/MaterialSourceData.h b/Gems/Atom/RPI/Code/Include/Atom/RPI.Edit/Material/MaterialSourceData.h index 53d3072370..a5fb0214e6 100644 --- a/Gems/Atom/RPI/Code/Include/Atom/RPI.Edit/Material/MaterialSourceData.h +++ b/Gems/Atom/RPI/Code/Include/Atom/RPI.Edit/Material/MaterialSourceData.h @@ -33,6 +33,12 @@ namespace AZ class MaterialAsset; class MaterialAssetCreator; + enum MaterialAssetProcessingMode + { + PreBake, //!< all material asset processing is done in the Asset Processor, producing a finalized material asset + DeferredBake //!< some material asset processing is deferred, and the material asset is finalized at runtime after loading + }; + //! This is a simple data structure for serializing in/out material source files. class MaterialSourceData final { @@ -72,35 +78,28 @@ namespace AZ UpdatesApplied }; - //! Checks the material type version and potentially applies a series of property changes (most common are simple property renames) - //! based on the MaterialTypeAsset's version update procedure. - //! @param materialSourceFilePath Indicates the path of the .material file that the MaterialSourceData represents. Used for resolving file-relative paths. - ApplyVersionUpdatesResult ApplyVersionUpdates(AZStd::string_view materialSourceFilePath = ""); - //! Creates a MaterialAsset from the MaterialSourceData content. //! @param assetId ID for the MaterialAsset //! @param materialSourceFilePath Indicates the path of the .material file that the MaterialSourceData represents. Used for //! resolving file-relative paths. + //! @param processingMode Indicates whether to finalize the material asset using data from the MaterialTypeAsset. //! @param elevateWarnings Indicates whether to treat warnings as errors - //! @param includeMaterialPropertyNames Indicates whether to save material property names into the material asset file Outcome> CreateMaterialAsset( Data::AssetId assetId, - AZStd::string_view materialSourceFilePath = "", - bool elevateWarnings = true, - bool includeMaterialPropertyNames = true) const; + AZStd::string_view materialSourceFilePath, + MaterialAssetProcessingMode processingMode, + bool elevateWarnings = true) const; //! Creates a MaterialAsset from the MaterialSourceData content. //! @param assetId ID for the MaterialAsset //! @param materialSourceFilePath Indicates the path of the .material file that the MaterialSourceData represents. Used for //! resolving file-relative paths. //! @param elevateWarnings Indicates whether to treat warnings as errors - //! @param includeMaterialPropertyNames Indicates whether to save material property names into the material asset file //! @param sourceDependencies if not null, will be populated with a set of all of the loaded material and material type paths Outcome> CreateMaterialAssetFromSourceData( Data::AssetId assetId, AZStd::string_view materialSourceFilePath = "", bool elevateWarnings = true, - bool includeMaterialPropertyNames = true, AZStd::unordered_set* sourceDependencies = nullptr) const; private: 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 c1183c7aa1..2fb55e5f40 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 @@ -64,6 +64,11 @@ namespace AZ void CheckForUnrecognizedJsonFields( const AZStd::string_view* acceptedFieldNames, uint32_t acceptedFieldNameCount, const rapidjson::Value& object, JsonDeserializerContext& context, JsonSerializationResult::ResultCode& result); + + //! Materials assets can either be finalized during asset-processing time or when materials are loaded at runtime. + //! Finalizing during asset processing reduces load times and obfuscates the material data. + //! Waiting to finalize at load time reduces dependencies on the material type data, resulting in fewer asset rebuilds and less time spent processing assets. + bool BuildersShouldFinalizeMaterialAssets(); } } } diff --git a/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialAsset.h b/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialAsset.h index 2a1de6debd..34e51b40af 100644 --- a/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialAsset.h +++ b/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialAsset.h @@ -105,6 +105,16 @@ namespace AZ //! Returns a layout that includes a list of MaterialPropertyDescriptors for each material property. const MaterialPropertiesLayout* GetMaterialPropertiesLayout() const; + //! Returns whether the material's properties are fully processed or not. + //! If true, property values can be accessed through GetPropertyValues(). + //! If false, property values can be accessed through GetRawPropertyValues(). + bool IsFinalized() const; + + //! If the material asset is not finalized yet, this does the final processing of m_rawPropertyValues to + //! get the material asset ready to be used. + //! Note m_materialTypeAsset must be valid before this is called. + void Finalize(); + //! Returns the list of values for all properties in this material. //! The entries in this list align with the entries in the MaterialPropertiesLayout. Each AZStd::any is guaranteed //! to have a value of type that matches the corresponding MaterialPropertyDescriptor. @@ -112,16 +122,13 @@ namespace AZ //! //! Note that even though material source data files contain only override values and inherit the rest from //! their parent material, they all get flattened at build time so every MaterialAsset has the full set of values. - AZStd::array_view GetPropertyValues() const; + const AZStd::vector& GetPropertyValues() const; + + const AZStd::vector>& GetRawPropertyValues() const; private: bool PostLoadInit() override; - //! Realigns property value and name indices with MaterialProperiesLayout by using m_propertyNames. Property names not found in the - //! MaterialPropertiesLayout are discarded, while property names not included in m_propertyNames will use the default value - //! from m_materialTypeAsset. - void RealignPropertyValuesAndNames(); - //! Checks the material type version and potentially applies a series of property changes (most common are simple property renames) //! based on the MaterialTypeAsset's version update procedure. void ApplyVersionUpdates(); @@ -146,17 +153,33 @@ namespace AZ //! Holds values for each material property, used to initialize Material instances. //! This is indexed by MaterialPropertyIndex and aligns with entries in m_materialPropertiesLayout. AZStd::vector m_propertyValues; - //! This is used to realign m_propertyValues as well as itself with MaterialPropertiesLayout when not empty. - //! If empty, this implies that m_propertyValues is aligned with the entries in m_materialPropertiesLayout. - AZStd::vector m_propertyNames; - //! The materialTypeVersion this materialAsset was based of. If the versions do not match at runtime when a - //! materialTypeAsset is loaded, an update will be performed on m_propertyNames if populated. + //! The MaterialAsset can be created in a "half-baked" state where minimal processing has been done because it does + //! not yet have access to the MaterialTypeAsset. In that case, this list will be populated with values copied from + //! the source .material file with little or no validation or other processing, and the m_propertyValues list will be empty. + //! Once a MaterialTypeAsset is available, Finalize() must be called to finish processing these values into the + //! final m_propertyValues list. + //! Note that the content of this list will remain after finalizing in order to support hot-reload of the MaterialTypeAsset. + //! The reason we use a vector instead of a map is to ensure inherited property values are applied in the right order; + //! if the material has a parent, and that parent uses an older material type version with renamed properties, then + //! m_rawPropertyValues could be holding two values for the same property under different names. The auto-rename process + //! can't be applied until the MaterialTypeAsset is available, so we have to keep the properties in the same order they + //! were originally encountered. + AZStd::vector> m_rawPropertyValues; + + //! Tracks whether Finalize() has been called, meaning m_propertyValues is populated with data matching the material type's property layout. + bool m_isFinalized = false; + + //! Tracks whether the MaterialAsset was already in a finalized state when it was loaded. + //! (This value is intentionally not serialized) + bool m_wasPreFinalized = false; + + //! The materialTypeVersion this materialAsset was based off. If the versions do not match at runtime when a + //! materialTypeAsset is loaded, automatic updates will be attempted at runtime. Note this is not needed to + //! determine which updates to apply, but simply as an optimization to ignore the update procedure when the + //! version numbers match. (We determine which updates to apply by simply checking the property name, and not + //! allowing the same name to ever be used for two different properties, see MaterialTypeAssetCreator::ValidateMaterialVersion) uint32_t m_materialTypeVersion = 1; - - //! A flag to determine if m_propertyValues needs to be aligned with MaterialPropertiesLayout. Set to true whenever - //! m_materialTypeAsset is reinitializing. - bool m_isDirty = true; }; diff --git a/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialAssetCreator.h b/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialAssetCreator.h index 862d98ce0c..b7b1f9705f 100644 --- a/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialAssetCreator.h +++ b/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialAssetCreator.h @@ -9,7 +9,6 @@ #include #include -#include namespace AZ { @@ -19,21 +18,21 @@ namespace AZ //! The MaterialAsset will be based on a MaterialTypeAsset or another MaterialAsset. //! Either way, the base provides the necessary data to define the layout //! and behavior of the material. The MaterialAsset only provides property value overrides. - class MaterialAssetCreator + //! Note however that the MaterialTypeAsset does not have to be loaded and available yet; + //! only the AssetId is required. The resulting MaterialAsset will be in a non-finalized state; + //! it must be finalized afterwards when the MaterialTypeAsset is available before it can be used. + class MaterialAssetCreator : public AssetCreator - , public MaterialAssetCreatorCommon { public: friend class MaterialSourceData; - - void Begin(const Data::AssetId& assetId, MaterialAsset& parentMaterial, bool includeMaterialPropertyNames = true); - void Begin(const Data::AssetId& assetId, MaterialTypeAsset& materialType, bool includeMaterialPropertyNames = true); + + void Begin(const Data::AssetId& assetId, const Data::Asset& materialType); bool End(Data::Asset& result); - private: - void PopulatePropertyNameList(); - - const MaterialPropertiesLayout* m_materialPropertiesLayout = nullptr; + void SetMaterialTypeVersion(uint32_t version); + + void SetPropertyValue(const Name& name, const MaterialPropertyValue& value); }; } // namespace RPI } // namespace AZ diff --git a/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialPropertyValue.h b/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialPropertyValue.h index 718615eb9f..79dda1d80a 100644 --- a/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialPropertyValue.h +++ b/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialPropertyValue.h @@ -30,6 +30,10 @@ namespace AZ { namespace RPI { + //! This is a variant data type that represents the value of a material property. + //! For convenience, it supports all the types necessary for *both* the runtime data (MaterialAsset) as well as .material file data (MaterialSourceData). + //! For example, Instance is exclusive to the runtime data and AZStd::string is primarily for image file paths in .material files. Most other + //! data types are relevant in both contexts. class MaterialPropertyValue final { public: 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 f11b2f94ac..1b5635a1c1 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Builders/Material/MaterialBuilder.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Builders/Material/MaterialBuilder.cpp @@ -43,17 +43,24 @@ namespace AZ const char* MaterialBuilder::JobKey = "Atom Material Builder"; + AZStd::string GetBuilderSettingsFingerprint() + { + return AZStd::string::format("[BuildersShouldFinalizeMaterialAssets=%d]", MaterialUtils::BuildersShouldFinalizeMaterialAssets()); + } + void MaterialBuilder::RegisterBuilder() { AssetBuilderSDK::AssetBuilderDesc materialBuilderDescriptor; materialBuilderDescriptor.m_name = JobKey; - materialBuilderDescriptor.m_version = 110; // Material version auto update feature + materialBuilderDescriptor.m_version = 111; // material dependency improvements 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(); materialBuilderDescriptor.m_createJobFunction = AZStd::bind(&MaterialBuilder::CreateJobs, this, AZStd::placeholders::_1, AZStd::placeholders::_2); materialBuilderDescriptor.m_processJobFunction = AZStd::bind(&MaterialBuilder::ProcessJob, this, AZStd::placeholders::_1, AZStd::placeholders::_2); + materialBuilderDescriptor.m_analysisFingerprint = GetBuilderSettingsFingerprint(); + BusConnect(materialBuilderDescriptor.m_busId); AssetBuilderSDK::AssetBuilderBus::Broadcast(&AssetBuilderSDK::AssetBuilderBus::Handler::RegisterBuilderInformation, materialBuilderDescriptor); @@ -63,7 +70,7 @@ namespace AZ { BusDisconnect(); } - + bool MaterialBuilder::ReportMaterialAssetWarningsAsErrors() const { bool warningsAsErrors = false; @@ -77,15 +84,18 @@ namespace AZ //! Adds all relevant dependencies for a referenced source file, considering that the path might be relative to the original file location or a full asset path. //! This will usually include multiple source dependencies and a single job dependency, but will include only source dependencies if the file is not found. //! Note the AssetBuilderSDK::JobDependency::m_platformIdentifier will not be set by this function. The calling code must set this value before passing back - //! to the AssetBuilderSDK::CreateJobsResponse. If isOrderedOnceForMaterialTypes is true and the dependency is a .materialtype file, the job dependency type - //! will be set to JobDependencyType::OrderOnce. - void AddPossibleDependencies(AZStd::string_view currentFilePath, - AZStd::string_view referencedParentPath, + //! to the AssetBuilderSDK::CreateJobsResponse. + void AddPossibleDependencies( + const AZStd::string& currentFilePath, + const AZStd::string& referencedParentPath, const char* jobKey, - AZStd::vector& jobDependencies, - bool isOrderedOnceForMaterialTypes = false) + AZStd::vector& jobDependencies) { bool dependencyFileFound = false; + + const bool currentFileIsMaterial = AzFramework::StringFunc::Path::IsExtension(currentFilePath.c_str(), MaterialSourceData::Extension); + const bool referencedFileIsMaterialType = AzFramework::StringFunc::Path::IsExtension(referencedParentPath.c_str(), MaterialTypeSourceData::Extension); + const bool ShouldFinalizeMaterialAssets = MaterialUtils::BuildersShouldFinalizeMaterialAssets(); AZStd::vector possibleDependencies = RPI::AssetUtils::GetPossibleDepenencyPaths(currentFilePath, referencedParentPath); for (auto& file : possibleDependencies) @@ -103,9 +113,15 @@ namespace AZ AssetBuilderSDK::JobDependency jobDependency; jobDependency.m_jobKey = jobKey; jobDependency.m_sourceFile.m_sourceFileDependencyPath = file; - - const bool isMaterialTypeFile = AzFramework::StringFunc::Path::IsExtension(file.c_str(), MaterialTypeSourceData::Extension); - jobDependency.m_type = (isMaterialTypeFile && isOrderedOnceForMaterialTypes) ? AssetBuilderSDK::JobDependencyType::OrderOnce : AssetBuilderSDK::JobDependencyType::Order; + jobDependency.m_type = AssetBuilderSDK::JobDependencyType::Order; + + // If we aren't finalizing material assets, then a normal job dependency isn't needed because the MaterialTypeAsset data won't be used. + // However, we do still need at least an OrderOnce dependency to ensure the Asset Processor knows about the material type asset so the builder can get it's AssetId. + // This can significantly reduce AP processing time when a material type or its shaders are edited. + if (currentFileIsMaterial && referencedFileIsMaterialType && !ShouldFinalizeMaterialAssets) + { + jobDependency.m_type = AssetBuilderSDK::JobDependencyType::OrderOnce; + } jobDependencies.push_back(jobDependency); } @@ -156,7 +172,8 @@ namespace AZ // We'll build up this one JobDescriptor and reuse it to register each of the platforms AssetBuilderSDK::JobDescriptor outputJobDescriptor; outputJobDescriptor.m_jobKey = JobKey; - + outputJobDescriptor.m_additionalFingerprintInfo = GetBuilderSettingsFingerprint(); + // Load the file so we can detect and report dependencies. // If the file is a .materialtype, report dependencies on the .shader files. // If the file is a .material, report a dependency on the .materialtype and parent .material file @@ -233,24 +250,12 @@ namespace AZ parentMaterialPath = materialTypePath; } - // If includeMaterialPropertyNames is false, then a job dependency is needed so the material builder can validate MaterialAsset properties - // against the MaterialTypeAsset at asset build time. - // If includeMaterialPropertyNames is true, the material properties will be validated at runtime when the material is loaded, so the job dependency - // is needed only for first-time processing to set up the initial MaterialAsset. This speeds up AP processing time when a materialtype file - // is edited (e.g. 10s when editing StandardPBR.materialtype on AtomTest project from 45s). - bool includeMaterialPropertyNames = true; - if (auto settingsRegistry = AZ::SettingsRegistry::Get(); settingsRegistry != nullptr) - { - settingsRegistry->Get(includeMaterialPropertyNames, "/O3DE/Atom/RPI/MaterialBuilder/IncludeMaterialPropertyNames"); - } - // Register dependency on the parent material source file so we can load it and use it's data to build this variant material. // Note, we don't need a direct dependency on the material type because the parent material will depend on it. AddPossibleDependencies(request.m_sourceFile, parentMaterialPath, JobKey, - outputJobDescriptor.m_jobDependencyList, - includeMaterialPropertyNames); + outputJobDescriptor.m_jobDependencyList); } } @@ -297,12 +302,9 @@ namespace AZ return {}; } - if (MaterialSourceData::ApplyVersionUpdatesResult::Failed == material.GetValue().ApplyVersionUpdates(materialSourceFilePath)) - { - return {}; - } + MaterialAssetProcessingMode processingMode = MaterialUtils::BuildersShouldFinalizeMaterialAssets() ? MaterialAssetProcessingMode::PreBake : MaterialAssetProcessingMode::DeferredBake; - auto materialAssetOutcome = material.GetValue().CreateMaterialAsset(Uuid::CreateRandom(), materialSourceFilePath, ReportMaterialAssetWarningsAsErrors()); + auto materialAssetOutcome = material.GetValue().CreateMaterialAsset(Uuid::CreateRandom(), materialSourceFilePath, processingMode, ReportMaterialAssetWarningsAsErrors()); if (!materialAssetOutcome.IsSuccess()) { return {}; diff --git a/Gems/Atom/RPI/Code/Source/RPI.Builders/Model/MaterialAssetBuilderComponent.cpp b/Gems/Atom/RPI/Code/Source/RPI.Builders/Model/MaterialAssetBuilderComponent.cpp index c83761cf8e..ef251b6848 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Builders/Model/MaterialAssetBuilderComponent.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Builders/Model/MaterialAssetBuilderComponent.cpp @@ -28,6 +28,7 @@ #include #include +#include #include #include @@ -44,7 +45,7 @@ namespace AZ if (auto* serialize = azrtti_cast(context)) { serialize->Class() - ->Version(5) // Set materialtype dependency to OrderOnce + ->Version(5) // <<<<< This probably is NOT the version number you want to bump. What you're looking for is MaterialAssetBuilderComponent::Reflect below ->Attribute(Edit::Attributes::SystemComponentTags, AZStd::vector({ AssetBuilderSDK::ComponentTags::AssetBuilder })); } } @@ -93,9 +94,11 @@ namespace AZ // material properties will be validated at runtime when the material is loaded, so the job dependency is needed only for // first-time processing to set up the initial MaterialAsset. This speeds up AP processing time when a materialtype file is // edited (e.g. 10s when editing StandardPBR.materialtype on AtomTest project from 45s). - bool includeMaterialPropertyNames = true; - RPI::MaterialConverterBus::BroadcastResult(includeMaterialPropertyNames, &RPI::MaterialConverterBus::Events::ShouldIncludeMaterialPropertyNames); - jobDependency.m_type = includeMaterialPropertyNames ? AssetBuilderSDK::JobDependencyType::OrderOnce : AssetBuilderSDK::JobDependencyType::Order; + + // If we aren't finalizing material assets, then a normal job dependency isn't needed because the MaterialTypeAsset data won't be used. + // However, we do still need at least an OrderOnce dependency to ensure the Asset Processor knows about the material type asset so the builder can get it's AssetId. + // This can significantly reduce AP processing time when a material type or its shaders are edited. + jobDependency.m_type = MaterialUtils::BuildersShouldFinalizeMaterialAssets() ? AssetBuilderSDK::JobDependencyType::OrderOnce : AssetBuilderSDK::JobDependencyType::Order; jobDependencyList.push_back(jobDependency); } @@ -108,10 +111,8 @@ namespace AZ bool conversionEnabled = false; RPI::MaterialConverterBus::BroadcastResult(conversionEnabled, &RPI::MaterialConverterBus::Events::IsEnabled); fingerprintInfo.insert(AZStd::string::format("[MaterialConverter enabled=%d]", conversionEnabled)); - - bool includeMaterialPropertyNames = true; - RPI::MaterialConverterBus::BroadcastResult(includeMaterialPropertyNames, &RPI::MaterialConverterBus::Events::ShouldIncludeMaterialPropertyNames); - fingerprintInfo.insert(AZStd::string::format("[MaterialConverter includeMaterialPropertyNames=%d]", includeMaterialPropertyNames)); + + fingerprintInfo.insert(AZStd::string::format("[BuildersShouldFinalizeMaterialAssets=%d]", MaterialUtils::BuildersShouldFinalizeMaterialAssets())); if (!conversionEnabled) { @@ -126,7 +127,7 @@ namespace AZ if (auto* serialize = azrtti_cast(context)) { serialize->Class() - ->Version(16); // Optional material conversion + ->Version(17); // Optional material conversion } } @@ -230,9 +231,9 @@ namespace AZ } } } + + MaterialAssetProcessingMode processingMode = MaterialUtils::BuildersShouldFinalizeMaterialAssets() ? MaterialAssetProcessingMode::PreBake : MaterialAssetProcessingMode::DeferredBake; - bool includeMaterialPropertyNames = true; - RPI::MaterialConverterBus::BroadcastResult(includeMaterialPropertyNames, &RPI::MaterialConverterBus::Events::ShouldIncludeMaterialPropertyNames); // Build material assets. for (auto& itr : materialSourceDataByUid) { @@ -240,7 +241,7 @@ namespace AZ Data::AssetId assetId(sourceSceneUuid, GetMaterialAssetSubId(materialUid)); auto materialSourceData = itr.second; - Outcome> result = materialSourceData.m_data.CreateMaterialAsset(assetId, "", false, includeMaterialPropertyNames); + Outcome> result = materialSourceData.m_data.CreateMaterialAsset(assetId, "", processingMode, false); if (result.IsSuccess()) { context.m_outputMaterialsByUid[materialUid] = { result.GetValue(), materialSourceData.m_name }; 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 4351213c22..cd01544bfa 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialSourceData.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialSourceData.cpp @@ -74,77 +74,49 @@ namespace AZ } } - MaterialSourceData::ApplyVersionUpdatesResult MaterialSourceData::ApplyVersionUpdates(AZStd::string_view materialSourceFilePath) + Outcome> MaterialSourceData::CreateMaterialAsset( + Data::AssetId assetId, AZStd::string_view materialSourceFilePath, MaterialAssetProcessingMode processingMode, bool elevateWarnings) const { - AZStd::string materialTypeFullPath = AssetUtils::ResolvePathReference(materialSourceFilePath, m_materialType); - auto materialTypeSourceDataOutcome = MaterialUtils::LoadMaterialTypeSourceData(materialTypeFullPath); - if (!materialTypeSourceDataOutcome.IsSuccess()) - { - return ApplyVersionUpdatesResult::Failed; - } - - MaterialTypeSourceData materialTypeSourceData = materialTypeSourceDataOutcome.TakeValue(); + MaterialAssetCreator materialAssetCreator; + materialAssetCreator.SetElevateWarnings(elevateWarnings); - if (m_materialTypeVersion == materialTypeSourceData.m_version) + Outcome materialTypeAssetId = AssetUtils::MakeAssetId(materialSourceFilePath, m_materialType, 0); + if (!materialTypeAssetId) { - return ApplyVersionUpdatesResult::NoUpdates; + return Failure(); } - bool changesWereApplied = false; - - // Note that the only kind of property update currently supported is rename... - - PropertyGroupMap newPropertyGroups; - for (auto& groupPair : m_properties) + Data::Asset materialTypeAsset; + + switch (processingMode) { - PropertyMap& propertyMap = groupPair.second; - - for (auto& propertyPair : propertyMap) + case MaterialAssetProcessingMode::DeferredBake: { - MaterialPropertyId propertyId{groupPair.first, propertyPair.first}; - - if (materialTypeSourceData.ApplyPropertyRenames(propertyId, m_materialTypeVersion)) + // Don't load the material type data, just create a reference to it + materialTypeAsset = Data::Asset{ materialTypeAssetId.GetValue(), azrtti_typeid(), m_materialType }; + break; + } + case MaterialAssetProcessingMode::PreBake: + { + // In this case we need to load the material type data in preparation for the material->Finalize() step below. + auto materialTypeAssetOutcome = AssetUtils::LoadAsset(materialTypeAssetId.GetValue()); + if (!materialTypeAssetOutcome) { - changesWereApplied = true; + return Failure(); } - - newPropertyGroups[propertyId.GetGroupName().GetStringView()][propertyId.GetPropertyName().GetStringView()] = propertyPair.second; + materialTypeAsset = materialTypeAssetOutcome.GetValue(); + break; } - } - - if (changesWereApplied) - { - m_properties = AZStd::move(newPropertyGroups); - - AZ_Warning( - "MaterialSourceData", false, - "This material is based on version '%u' of '%s', but the material type is now at version '%u'. " - "Automatic updates are available. Consider updating the .material source file: '%s'.", - m_materialTypeVersion, materialTypeFullPath.c_str(), materialTypeSourceData.m_version, materialSourceFilePath.data()); - } - - m_materialTypeVersion = materialTypeSourceData.m_version; - - return changesWereApplied ? ApplyVersionUpdatesResult::UpdatesApplied : ApplyVersionUpdatesResult::NoUpdates; - } - - Outcome> MaterialSourceData::CreateMaterialAsset( - Data::AssetId assetId, AZStd::string_view materialSourceFilePath, bool elevateWarnings, bool includeMaterialPropertyNames) const - { - MaterialAssetCreator materialAssetCreator; - materialAssetCreator.SetElevateWarnings(elevateWarnings); - - if (m_parentMaterial.empty()) - { - auto materialTypeAsset = AssetUtils::LoadAsset(materialSourceFilePath, m_materialType); - if (!materialTypeAsset.IsSuccess()) + default: { + AZ_Assert(false, "Unhandled MaterialAssetProcessingMode"); return Failure(); } - - materialAssetCreator.Begin(assetId, *materialTypeAsset.GetValue().Get(), includeMaterialPropertyNames); } - else + + materialAssetCreator.Begin(assetId, materialTypeAsset); + + if (!m_parentMaterial.empty()) { auto parentMaterialAsset = AssetUtils::LoadAsset(materialSourceFilePath, m_parentMaterial); if (!parentMaterialAsset.IsSuccess()) @@ -154,25 +126,53 @@ namespace AZ // Make sure the parent material has the same material type { - auto materialTypeIdOutcome = AssetUtils::MakeAssetId(materialSourceFilePath, m_materialType, 0); - if (!materialTypeIdOutcome.IsSuccess()) + Data::AssetId parentsMaterialTypeId = parentMaterialAsset.GetValue()->GetMaterialTypeAsset().GetId(); + + if (materialTypeAssetId.GetValue() != parentsMaterialTypeId) { + AZ_Error("MaterialSourceData", false, "This material and its parent material do not share the same material type."); return Failure(); } + } - Data::AssetId expectedMaterialTypeId = materialTypeIdOutcome.GetValue(); - Data::AssetId parentMaterialId = parentMaterialAsset.GetValue().GetId(); - // This will only be valid if the parent material is not a material type - Data::AssetId parentsMaterialTypeId = parentMaterialAsset.GetValue()->GetMaterialTypeAsset().GetId(); + // Inherit the parent's property values... + switch (processingMode) + { + case MaterialAssetProcessingMode::DeferredBake: + { + for (auto& property : parentMaterialAsset.GetValue()->GetRawPropertyValues()) + { + materialAssetCreator.SetPropertyValue(property.first, property.second); + } - if (expectedMaterialTypeId != parentMaterialId && expectedMaterialTypeId != parentsMaterialTypeId) + break; + } + case MaterialAssetProcessingMode::PreBake: { - AZ_Error("MaterialSourceData", false, "This material and its parent material do not share the same material type."); + const MaterialPropertiesLayout* propertiesLayout = parentMaterialAsset.GetValue()->GetMaterialPropertiesLayout(); + + if (parentMaterialAsset.GetValue()->GetPropertyValues().size() != propertiesLayout->GetPropertyCount()) + { + AZ_Assert(false, "The parent material should have been finalized with %zu properties but it has %zu. Something is out of sync.", + propertiesLayout->GetPropertyCount(), parentMaterialAsset.GetValue()->GetPropertyValues().size()); + return Failure(); + } + + for (size_t propertyIndex = 0; propertyIndex < propertiesLayout->GetPropertyCount(); ++propertyIndex) + { + materialAssetCreator.SetPropertyValue( + propertiesLayout->GetPropertyDescriptor(MaterialPropertyIndex{propertyIndex})->GetName(), + parentMaterialAsset.GetValue()->GetPropertyValues()[propertyIndex]); + } + + break; + } + default: + { + AZ_Assert(false, "Unhandled MaterialAssetProcessingMode"); return Failure(); } } - - materialAssetCreator.Begin(assetId, *parentMaterialAsset.GetValue().Get(), includeMaterialPropertyNames); } ApplyPropertiesToAssetCreator(materialAssetCreator, materialSourceFilePath); @@ -180,6 +180,11 @@ namespace AZ Data::Asset material; if (materialAssetCreator.End(material)) { + if (processingMode == MaterialAssetProcessingMode::PreBake) + { + material->Finalize(); + } + return Success(material); } else @@ -192,7 +197,6 @@ namespace AZ Data::AssetId assetId, AZStd::string_view materialSourceFilePath, bool elevateWarnings, - bool includeMaterialPropertyNames, AZStd::unordered_set* sourceDependencies) const { const auto materialTypeSourcePath = AssetUtils::ResolvePathReference(materialSourceFilePath, m_materialType); @@ -270,7 +274,7 @@ namespace AZ // Create the material asset from all the previously loaded source data MaterialAssetCreator materialAssetCreator; materialAssetCreator.SetElevateWarnings(elevateWarnings); - materialAssetCreator.Begin(assetId, *materialTypeAsset.GetValue().Get(), includeMaterialPropertyNames); + materialAssetCreator.Begin(assetId, materialTypeAsset.GetValue()); while (!parentSourceDataStack.empty()) { @@ -283,6 +287,13 @@ namespace AZ Data::Asset material; if (materialAssetCreator.End(material)) { + // Unlike CreateMaterialAsset(), we can always finalize the material here because we loaded created the MaterialTypeAsset from + // the source .materialtype file, so the necessary data is always available. + // (In case you are wondering why we don't use CreateMaterialAssetFromSourceData in MaterialBuilder: that would require a + // source dependency between the .materialtype and .material file, which would cause all .material files to rebuild when you + // edit the .materialtype; it's faster to not read the material type data at all ... until it's needed at runtime) + material->Finalize(); + if (sourceDependencies) { sourceDependencies->insert(dependencies.begin(), dependencies.end()); @@ -306,59 +317,26 @@ namespace AZ { materialAssetCreator.ReportWarning("Source data for material property value is invalid."); } - else + else if (property.second.m_value.Is() && AzFramework::StringFunc::Contains(property.second.m_value.GetValue(), ".")) { - MaterialPropertyIndex propertyIndex = - materialAssetCreator.m_materialPropertiesLayout->FindPropertyIndex(propertyId.GetFullName()); - if (propertyIndex.IsValid()) - { - const MaterialPropertyDescriptor* propertyDescriptor = - materialAssetCreator.m_materialPropertiesLayout->GetPropertyDescriptor(propertyIndex); - switch (propertyDescriptor->GetDataType()) - { - case MaterialPropertyDataType::Image: - { - Data::Asset imageAsset; - - MaterialUtils::GetImageAssetResult result = MaterialUtils::GetImageAssetReference( - imageAsset, materialSourceFilePath, property.second.m_value.GetValue()); - - if (result == MaterialUtils::GetImageAssetResult::Missing) - { - materialAssetCreator.ReportWarning( - "Material property '%s': Could not find the image '%s'", propertyId.GetFullName().GetCStr(), - property.second.m_value.GetValue().data()); - } + Data::Asset imageAsset; + + MaterialUtils::GetImageAssetResult result = MaterialUtils::GetImageAssetReference( + imageAsset, materialSourceFilePath, property.second.m_value.GetValue()); - imageAsset.SetAutoLoadBehavior(Data::AssetLoadBehavior::PreLoad); - materialAssetCreator.SetPropertyValue(propertyId.GetFullName(), imageAsset); - } - break; - case MaterialPropertyDataType::Enum: - { - AZ::Name enumName = AZ::Name(property.second.m_value.GetValue()); - uint32_t enumValue = propertyDescriptor->GetEnumValue(enumName); - if (enumValue == MaterialPropertyDescriptor::InvalidEnumValue) - { - materialAssetCreator.ReportError( - "Enum value '%s' couldn't be found in the 'enumValues' list", enumName.GetCStr()); - } - else - { - materialAssetCreator.SetPropertyValue(propertyId.GetFullName(), enumValue); - } - } - break; - default: - materialAssetCreator.SetPropertyValue(propertyId.GetFullName(), property.second.m_value); - break; - } - } - else + if (result == MaterialUtils::GetImageAssetResult::Missing) { materialAssetCreator.ReportWarning( - "Can not find property id '%s' in MaterialPropertyLayout", propertyId.GetFullName().GetStringView().data()); + "Material property '%s': Could not find the image '%s'", propertyId.GetFullName().GetCStr(), + property.second.m_value.GetValue().data()); } + + imageAsset.SetAutoLoadBehavior(Data::AssetLoadBehavior::PreLoad); + materialAssetCreator.SetPropertyValue(propertyId.GetFullName(), imageAsset); + } + else + { + materialAssetCreator.SetPropertyValue(propertyId.GetFullName(), property.second.m_value); } } } 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 7fff8d81bc..475c6ca219 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialUtils.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialUtils.cpp @@ -19,6 +19,7 @@ #include #include #include +#include #include @@ -135,6 +136,20 @@ namespace AZ } } } + + bool BuildersShouldFinalizeMaterialAssets() + { + // We default to the faster workflow for developers. Enable this registry setting when releasing the + // game for faster load times and obfuscation of material assets. + bool shouldFinalize = false; + + if (auto settingsRegistry = AZ::SettingsRegistry::Get(); settingsRegistry != nullptr) + { + settingsRegistry->Get(shouldFinalize, "/O3DE/Atom/RPI/MaterialBuilder/FinalizeMaterialAssets"); + } + + return shouldFinalize; + } } } } diff --git a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialAsset.cpp b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialAsset.cpp index 3c6947b83d..7962844736 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialAsset.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialAsset.cpp @@ -33,11 +33,12 @@ namespace AZ if (auto* serializeContext = azrtti_cast(context)) { serializeContext->Class() - ->Version(11) // Material version update + ->Version(13) // added m_rawPropertyValues ->Field("materialTypeAsset", &MaterialAsset::m_materialTypeAsset) ->Field("materialTypeVersion", &MaterialAsset::m_materialTypeVersion) ->Field("propertyValues", &MaterialAsset::m_propertyValues) - ->Field("propertyNames", &MaterialAsset::m_propertyNames) + ->Field("rawPropertyValues", &MaterialAsset::m_rawPropertyValues) + ->Field("isFinalized", &MaterialAsset::m_isFinalized) ; } } @@ -102,32 +103,95 @@ namespace AZ { return m_materialTypeAsset->GetMaterialPropertiesLayout(); } + + bool MaterialAsset::IsFinalized() const + { + if (m_isFinalized) + { + AZ_Assert(GetMaterialPropertiesLayout() && m_propertyValues.size() == GetMaterialPropertiesLayout()->GetPropertyCount(), "MaterialAsset is marked as Finalized but does not have the right number of property values."); + } - AZStd::array_view MaterialAsset::GetPropertyValues() const + return m_isFinalized; + } + + void MaterialAsset::Finalize() { - // If property names are included, they are used to re-arrange the property value list to align with the - // MaterialPropertiesLayout. This realignment would be necessary if the material type is updated with - // a new property layout, and a corresponding material is not reprocessed by the AP and continues using the - // old property layout. - if (!m_propertyNames.empty()) + if (IsFinalized()) + { + return; + } + + const uint32_t materialTypeVersion = m_materialTypeAsset->GetVersion(); + if (m_materialTypeVersion < materialTypeVersion) + { + // It is possible that the material type has had some properties renamed or otherwise updated. If that's the case, + // and this material is still referencing the old property layout, we need to apply any auto updates to rename those + // properties before using them to realign the property values. + ApplyVersionUpdates(); + } + + const MaterialPropertiesLayout* propertyLayout = GetMaterialPropertiesLayout(); + + AZStd::vector finalizedPropertyValues(m_materialTypeAsset->GetDefaultPropertyValues().begin(), m_materialTypeAsset->GetDefaultPropertyValues().end()); + + for (const auto& [name, value] : m_rawPropertyValues) { - const uint32_t materialTypeVersion = m_materialTypeAsset->GetVersion(); - if (m_materialTypeVersion < materialTypeVersion) + const MaterialPropertyIndex propertyIndex = propertyLayout->FindPropertyIndex(name); + if (propertyIndex.IsValid()) { - // It is possible that the material type has had some properties renamed. If that's the case, and this material - // is still referencing the old property layout, we need to apply any auto updates to rename those properties - // before using them to realign the property values. - const_cast(this)->ApplyVersionUpdates(); - } + const MaterialPropertyDescriptor* propertyDescriptor = propertyLayout->GetPropertyDescriptor(propertyIndex); - if (m_isDirty) + if (value.Is() && propertyDescriptor->GetDataType() == MaterialPropertyDataType::Enum) + { + AZ::Name enumName = AZ::Name(value.GetValue()); + uint32_t enumValue = propertyDescriptor->GetEnumValue(enumName); + if (enumValue == MaterialPropertyDescriptor::InvalidEnumValue) + { + AZ_Error(s_debugTraceName, false, "Material property name \"%s\" has invalid enum value \"%s\".", name.GetCStr(), enumName.GetCStr()); + } + else + { + finalizedPropertyValues[propertyIndex.GetIndex()] = enumValue; + } + } + else if (value.Is() && propertyDescriptor->GetDataType() == MaterialPropertyDataType::Image) + { + // Here we assume that the material asset builder resolved any image source file paths to an ImageAsset reference. + // So the only way a string could be present is if it's an empty image path reference, meaning no image should be bound. + AZ_Assert(value.GetValue().empty(), "Material property '%s' references in image '%s'. Image file paths must be resolved by the material asset builder."); + + finalizedPropertyValues[propertyIndex.GetIndex()] = Data::Asset{}; + } + else + { + finalizedPropertyValues[propertyIndex.GetIndex()] = value; + } + } + else { - const_cast(this)->RealignPropertyValuesAndNames(); + AZ_Warning(s_debugTraceName, false, "Material property name \"%s\" is not found in the material properties layout and will not be used.", name.GetCStr()); } } + m_propertyValues.swap(finalizedPropertyValues); + + m_isFinalized = true; + } + + const AZStd::vector& MaterialAsset::GetPropertyValues() const + { + // This can't be done in MaterialAssetHandler::LoadAssetData because the MaterialTypeAsset isn't necessarily loaded at that point. + // And it can't be done in PostLoadInit() because that happens on the next frame which might be too late. So we finalize just-in-time + // when properties are accessed. + const_cast(this)->Finalize(); + return m_propertyValues; } + + const AZStd::vector>& MaterialAsset::GetRawPropertyValues() const + { + return m_rawPropertyValues; + } void MaterialAsset::SetReady() { @@ -173,34 +237,6 @@ namespace AZ } } - void MaterialAsset::RealignPropertyValuesAndNames() - { - const MaterialPropertiesLayout* propertyLayout = GetMaterialPropertiesLayout(); - AZStd::vector alignedPropertyValues(m_materialTypeAsset->GetDefaultPropertyValues().begin(), m_materialTypeAsset->GetDefaultPropertyValues().end()); - for (size_t i = 0; i < m_propertyNames.size(); ++i) - { - const MaterialPropertyIndex propertyIndex = propertyLayout->FindPropertyIndex(m_propertyNames[i]); - if (propertyIndex.IsValid()) - { - alignedPropertyValues[propertyIndex.GetIndex()] = m_propertyValues[i]; - } - else - { - AZ_Warning(s_debugTraceName, false, "Material property name \"%s\" is not found in the material properties layout and will not be used.", m_propertyNames[i].GetCStr()); - } - } - m_propertyValues.swap(alignedPropertyValues); - - const size_t propertyCount = propertyLayout->GetPropertyCount(); - m_propertyNames.resize(propertyCount); - for (size_t i = 0; i < propertyCount; ++i) - { - m_propertyNames[i] = propertyLayout->GetPropertyDescriptor(MaterialPropertyIndex{ i })->GetName(); - } - - m_isDirty = false; - } - void MaterialAsset::ApplyVersionUpdates() { if (m_materialTypeVersion == m_materialTypeAsset->GetVersion()) @@ -248,7 +284,13 @@ namespace AZ // This also covers the case where just the MaterialTypeAsset is reloaded and not the MaterialAsset. m_materialTypeAsset = newMaterialTypeAsset; - m_isDirty = true; + // If the material asset was not finalized on disk, then we clear the previously finalized property values to force re-finalize. + // This + if (!m_wasPreFinalized) + { + m_isFinalized = false; + m_propertyValues.clear(); + } // Notify interested parties that this MaterialAsset is changed and may require other data to reinitialize as well MaterialReloadNotificationBus::Event(GetId(), &MaterialReloadNotifications::OnMaterialAssetReinitialized, Data::Asset{this, AZ::Data::AssetLoadBehavior::PreLoad}); @@ -276,6 +318,7 @@ namespace AZ if (Base::LoadAssetData(asset, stream, assetLoadFilterCB) == Data::AssetHandler::LoadResult::LoadComplete) { asset.GetAs()->AssetInitBus::Handler::BusConnect(); + asset.GetAs()->m_wasPreFinalized = asset.GetAs()->m_isFinalized; return Data::AssetHandler::LoadResult::LoadComplete; } diff --git a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialAssetCreator.cpp b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialAssetCreator.cpp index b62a91a98d..4bee663791 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialAssetCreator.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialAssetCreator.cpp @@ -16,89 +16,19 @@ namespace AZ { namespace RPI { - void MaterialAssetCreator::Begin(const Data::AssetId& assetId, MaterialAsset& parentMaterial, bool includeMaterialPropertyNames) - { - BeginCommon(assetId); - - if (ValidateIsReady()) - { - m_asset->m_materialTypeAsset = parentMaterial.m_materialTypeAsset; - m_asset->m_materialTypeVersion = m_asset->m_materialTypeAsset->GetVersion(); - - if (!m_asset->m_materialTypeAsset) - { - ReportError("MaterialTypeAsset is null"); - return; - } - - m_materialPropertiesLayout = m_asset->GetMaterialPropertiesLayout(); - if (!m_materialPropertiesLayout) - { - ReportError("MaterialPropertiesLayout is null"); - return; - } - if (includeMaterialPropertyNames) - { - PopulatePropertyNameList(); - } - - // Note we don't have to check the validity of these property values because the parent material's AssetCreator already did that. - m_asset->m_propertyValues.assign(parentMaterial.GetPropertyValues().begin(), parentMaterial.GetPropertyValues().end()); - - auto warningFunc = [this](const char* message) - { - ReportWarning("%s", message); - }; - auto errorFunc = [this](const char* message) - { - ReportError("%s", message); - }; - MaterialAssetCreatorCommon::OnBegin(m_materialPropertiesLayout, &(m_asset->m_propertyValues), warningFunc, errorFunc); - } - } - - void MaterialAssetCreator::Begin(const Data::AssetId& assetId, MaterialTypeAsset& materialType, bool includeMaterialPropertyNames) + void MaterialAssetCreator::Begin(const Data::AssetId& assetId, const Data::Asset& materialType) { BeginCommon(assetId); if (ValidateIsReady()) { - m_asset->m_materialTypeAsset = { &materialType, AZ::Data::AssetLoadBehavior::PreLoad }; + m_asset->m_materialTypeAsset = materialType; - if (!m_asset->m_materialTypeAsset) - { - ReportError("MaterialTypeAsset is null"); - return; - } - m_asset->m_materialTypeVersion = m_asset->m_materialTypeAsset->GetVersion(); + m_asset->m_materialTypeAsset.SetAutoLoadBehavior(AZ::Data::AssetLoadBehavior::PreLoad); - m_materialPropertiesLayout = m_asset->GetMaterialPropertiesLayout(); - if (includeMaterialPropertyNames) - { - PopulatePropertyNameList(); - } - - if (!m_materialPropertiesLayout) - { - ReportError("MaterialPropertiesLayout is null"); - return; - } - - // Note we don't have to check the validity of these property values because the parent material's AssetCreator already did that. - m_asset->m_propertyValues.assign(materialType.GetDefaultPropertyValues().begin(), materialType.GetDefaultPropertyValues().end()); - - auto warningFunc = [this](const char* message) - { - ReportWarning("%s", message); - }; - auto errorFunc = [this](const char* message) - { - ReportError("%s", message); - }; - MaterialAssetCreatorCommon::OnBegin(m_materialPropertiesLayout, &(m_asset->m_propertyValues), warningFunc, errorFunc); } } - + bool MaterialAssetCreator::End(Data::Asset& result) { if (!ValidateIsReady()) @@ -106,20 +36,39 @@ namespace AZ return false; } - m_materialPropertiesLayout = nullptr; - MaterialAssetCreatorCommon::OnEnd(); - m_asset->SetReady(); return EndCommon(result); } - - void MaterialAssetCreator::PopulatePropertyNameList() + + void MaterialAssetCreator::SetMaterialTypeVersion(uint32_t version) + { + if (ValidateIsReady()) + { + m_asset->m_materialTypeVersion = version; + } + } + + void MaterialAssetCreator::SetPropertyValue(const Name& name, const MaterialPropertyValue& value) { - for (int i = 0; i < m_materialPropertiesLayout->GetPropertyCount(); ++i) + if (ValidateIsReady()) { - MaterialPropertyIndex propertyIndex{ i }; - auto& propertyName = m_materialPropertiesLayout->GetPropertyDescriptor(propertyIndex)->GetName(); - m_asset->m_propertyNames.emplace_back(propertyName); + // Here we are careful to keep the properties in the same order they were encountered. When the MaterialAsset + // is later finalized with a MaterialTypeAsset, there could be a version update procedure that includes renamed + // properties. So it's possible that the same property could be encountered twice but with two different names. + // Preserving the original order will ensure that the later properties still overwrite the earlier ones even after + // renames have been applied. + + auto iter = AZStd::find_if(m_asset->m_rawPropertyValues.begin(), m_asset->m_rawPropertyValues.end(), [&name](const AZStd::pair& pair) + { + return pair.first == name; + }); + + if (iter != m_asset->m_rawPropertyValues.end()) + { + m_asset->m_rawPropertyValues.erase(iter); + } + + m_asset->m_rawPropertyValues.emplace_back(name, value); } } diff --git a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialTypeAssetCreator.cpp b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialTypeAssetCreator.cpp index 46086dfecc..dd023c56b8 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialTypeAssetCreator.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialTypeAssetCreator.cpp @@ -122,6 +122,21 @@ namespace AZ return false; } + // We don't allow previously renamed property names to be reused for new properties. This would just complicate too many things, + // as every use of every property name (like in Material Component, or in scripts, for example) would have to have a version number + // associated with it, in order to know whether or which rename to apply. + for (size_t propertyIndex = 0; propertyIndex < m_asset->m_materialPropertiesLayout->GetPropertyCount(); ++propertyIndex) + { + Name originalPropertyName = m_asset->m_materialPropertiesLayout->GetPropertyDescriptor(MaterialPropertyIndex{propertyIndex})->GetName(); + Name newPropertyName = originalPropertyName; + if (versionUpdate.ApplyPropertyRenames(newPropertyName)) + { + ReportError("There was a material property named '%s' at material type version %d. This name cannot be reused for another property.", + originalPropertyName.GetCStr(), versionUpdate.GetVersion()); + return false; + } + } + prevVersion = versionUpdate.GetVersion(); } diff --git a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialVersionUpdate.cpp b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialVersionUpdate.cpp index f5dfe9e80c..f12c865eee 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialVersionUpdate.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialVersionUpdate.cpp @@ -77,18 +77,14 @@ namespace AZ { bool changesWereApplied = false; - for (auto& propertyName : materialAsset.m_propertyNames) + for (auto& [name, value] : materialAsset.m_rawPropertyValues) { - for (const auto& action : m_actions) + if (ApplyPropertyRenames(name)) { - if (propertyName == action.m_fromPropertyId) - { - propertyName = action.m_toPropertyId; - changesWereApplied = true; - } + changesWereApplied = true; } } - + return changesWereApplied; } diff --git a/Gems/Atom/Tools/MaterialEditor/Code/Source/Document/MaterialDocument.cpp b/Gems/Atom/Tools/MaterialEditor/Code/Source/Document/MaterialDocument.cpp index 97d8d5e354..93da118b5b 100644 --- a/Gems/Atom/Tools/MaterialEditor/Code/Source/Document/MaterialDocument.cpp +++ b/Gems/Atom/Tools/MaterialEditor/Code/Source/Document/MaterialDocument.cpp @@ -680,12 +680,6 @@ namespace MaterialEditor return false; } m_materialTypeSourceData = materialTypeOutcome.GetValue(); - - if (MaterialSourceData::ApplyVersionUpdatesResult::Failed == m_materialSourceData.ApplyVersionUpdates(m_absolutePath)) - { - AZ_Error("MaterialDocument", false, "Material source data could not be auto updated to the latest version of the material type: '%s'.", m_materialSourceData.m_materialType.c_str()); - return false; - } } else if (AzFramework::StringFunc::Path::IsExtension(m_absolutePath.c_str(), MaterialTypeSourceData::Extension)) { From a627cda5aeee1c7d2714045f5b58b51b57440393 Mon Sep 17 00:00:00 2001 From: santorac <55155825+santorac@users.noreply.github.com> Date: Fri, 17 Dec 2021 17:05:27 -0800 Subject: [PATCH 02/10] Got the unit tests working again. I made MaterialAsset::Finalize private so I could add some parameters specifically for MaterialAssetCreator to use. Now MaterialAssetCreator::Begin has an option to finalize the material or not. Moved MaterialAssetCreatorCommon::ValidateDataType to MaterialPropertyDescriptor as "ValidateMaterialPropertyDataType" so that MaterialAsset::Finalize could use it too Signed-off-by: santorac <55155825+santorac@users.noreply.github.com> --- .../Atom/RPI.Edit/Material/MaterialUtils.h | 1 + .../Include/Atom/RPI.Reflect/AssetCreator.h | 1 + .../Atom/RPI.Reflect/Material/MaterialAsset.h | 12 +- .../Material/MaterialAssetCreator.h | 24 +- .../Material/MaterialAssetCreatorCommon.h | 6 - .../Material/MaterialPropertyDescriptor.h | 5 + .../RPI.Builders/Material/MaterialBuilder.cpp | 2 +- .../Model/MaterialAssetBuilderComponent.cpp | 2 +- .../RPI.Edit/Material/MaterialSourceData.cpp | 23 +- .../RPI.Edit/Material/MaterialUtils.cpp | 1 + .../RPI.Reflect/Material/MaterialAsset.cpp | 27 +- .../Material/MaterialAssetCreator.cpp | 34 ++- .../Material/MaterialAssetCreatorCommon.cpp | 54 +--- .../Material/MaterialPropertyDescriptor.cpp | 51 ++++ .../Material/LuaMaterialFunctorTests.cpp | 8 +- .../Tests/Material/MaterialAssetTests.cpp | 166 +++++++---- .../Tests/Material/MaterialFunctorTests.cpp | 2 +- .../Material/MaterialSourceDataTests.cpp | 282 ++++-------------- .../RPI/Code/Tests/Material/MaterialTests.cpp | 22 +- 19 files changed, 333 insertions(+), 390 deletions(-) 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..2a992159b3 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 @@ -69,6 +69,7 @@ namespace AZ //! Finalizing during asset processing reduces load times and obfuscates the material data. //! Waiting to finalize at load time reduces dependencies on the material type data, resulting in fewer asset rebuilds and less time spent processing assets. bool BuildersShouldFinalizeMaterialAssets(); + } } } diff --git a/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/AssetCreator.h b/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/AssetCreator.h index 79d43d0b8d..257abcc785 100644 --- a/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/AssetCreator.h +++ b/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/AssetCreator.h @@ -28,6 +28,7 @@ namespace AZ // [GFX TODO] We need to iterate on this concept at some point. We may want to expose it through cvars or something // like that, or we may not need this at all. For now it's helpful for testing. void SetElevateWarnings(bool elevated); + bool GetElevateWarnings() const { return m_warningsElevated; } int GetErrorCount() const { return m_errorCount; } int GetWarningCount() const { return m_warningCount; } diff --git a/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialAsset.h b/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialAsset.h index 34e51b40af..736d7c5b53 100644 --- a/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialAsset.h +++ b/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialAsset.h @@ -15,6 +15,7 @@ #include #include #include +#include #include #include @@ -110,11 +111,6 @@ namespace AZ //! If false, property values can be accessed through GetRawPropertyValues(). bool IsFinalized() const; - //! If the material asset is not finalized yet, this does the final processing of m_rawPropertyValues to - //! get the material asset ready to be used. - //! Note m_materialTypeAsset must be valid before this is called. - void Finalize(); - //! Returns the list of values for all properties in this material. //! The entries in this list align with the entries in the MaterialPropertiesLayout. Each AZStd::any is guaranteed //! to have a value of type that matches the corresponding MaterialPropertyDescriptor. @@ -129,6 +125,12 @@ namespace AZ private: bool PostLoadInit() override; + //! If the material asset is not finalized yet, this does the final processing of m_rawPropertyValues to + //! get the material asset ready to be used. + //! Note m_materialTypeAsset must be valid before this is called. + //! @param elevateWarnings Indicates whether to treat warnings as errors + void Finalize(AZStd::function reportWarning = nullptr, AZStd::function reportError = nullptr); + //! Checks the material type version and potentially applies a series of property changes (most common are simple property renames) //! based on the MaterialTypeAsset's version update procedure. void ApplyVersionUpdates(); diff --git a/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialAssetCreator.h b/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialAssetCreator.h index b7b1f9705f..74bd909e08 100644 --- a/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialAssetCreator.h +++ b/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialAssetCreator.h @@ -9,30 +9,38 @@ #include #include +#include +#include namespace AZ { namespace RPI { //! Use a MaterialAssetCreator to create and configure a new MaterialAsset. - //! The MaterialAsset will be based on a MaterialTypeAsset or another MaterialAsset. - //! Either way, the base provides the necessary data to define the layout - //! and behavior of the material. The MaterialAsset only provides property value overrides. - //! Note however that the MaterialTypeAsset does not have to be loaded and available yet; - //! only the AssetId is required. The resulting MaterialAsset will be in a non-finalized state; - //! it must be finalized afterwards when the MaterialTypeAsset is available before it can be used. + //! + //! There are two options for how to create the MaterialAsset, whether it should be finalized now or deferred. + //! - Finalized now: This requires the MaterialTypeAsset to be fully populated so it can read the property layout. + //! - Deferred finalize: This only requires the MaterialTypeAsset to have a valid AssetId; the data inside will not be used. MaterialAsset::Finalize() + //! will need to be called later when the final MaterialTypeAsset is available, presumably after loading the MaterialAsset at runtime. class MaterialAssetCreator : public AssetCreator { public: friend class MaterialSourceData; - - void Begin(const Data::AssetId& assetId, const Data::Asset& materialType); + + void Begin(const Data::AssetId& assetId, const Data::Asset& materialType, bool shouldFinalize); bool End(Data::Asset& result); void SetMaterialTypeVersion(uint32_t version); void SetPropertyValue(const Name& name, const MaterialPropertyValue& value); + + void SetPropertyValue(const Name& name, const Data::Asset& imageAsset); + void SetPropertyValue(const Name& name, const Data::Asset& imageAsset); + void SetPropertyValue(const Name& name, const Data::Asset& imageAsset); + + private: + bool m_shouldFinalize = false; }; } // namespace RPI } // namespace AZ diff --git a/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialAssetCreatorCommon.h b/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialAssetCreatorCommon.h index 312739a0f0..c7fa9c2a4c 100644 --- a/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialAssetCreatorCommon.h +++ b/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialAssetCreatorCommon.h @@ -52,12 +52,6 @@ namespace AZ private: bool PropertyCheck(TypeId typeId, const Name& name); - //! Returns the MaterialPropertyDataType value that corresponds to typeId - MaterialPropertyDataType GetMaterialPropertyDataType(TypeId typeId) const; - - //! Checks that the TypeId typeId matches the type expected by materialPropertyDescriptor - bool ValidateDataType(TypeId typeId, const Name& propertyName, const MaterialPropertyDescriptor* materialPropertyDescriptor); - const MaterialPropertiesLayout* m_propertyLayout = nullptr; //! Points to the m_propertyValues list in a MaterialAsset or MaterialTypeAsset AZStd::vector* m_propertyValues = nullptr; diff --git a/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialPropertyDescriptor.h b/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialPropertyDescriptor.h index 76bb2a6113..bd18c98780 100644 --- a/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialPropertyDescriptor.h +++ b/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialPropertyDescriptor.h @@ -17,6 +17,8 @@ namespace AZ { namespace RPI { + class MaterialPropertyDescriptor; + struct MaterialPropertyIndexType { AZ_TYPE_INFO(MaterialPropertyIndexType, "{cfc09268-f3f1-4474-bd8f-f2c8de27c5f1}"); }; @@ -76,6 +78,9 @@ namespace AZ const char* ToString(MaterialPropertyDataType materialPropertyDataType); AZStd::string GetMaterialPropertyDataTypeString(AZ::TypeId typeId); + + //! Checks that the TypeId matches the type expected by materialPropertyDescriptor + bool ValidateMaterialPropertyDataType(TypeId typeId, const Name& propertyName, const MaterialPropertyDescriptor* materialPropertyDescriptor, AZStd::function onError); //! A material property is any data input to a material, like a bool, float, Vector, Image, Buffer, etc. //! This descriptor defines a single input property, including it's name ID, and how it maps 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 1b5635a1c1..3aa8728e08 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Builders/Material/MaterialBuilder.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Builders/Material/MaterialBuilder.cpp @@ -52,7 +52,7 @@ namespace AZ { AssetBuilderSDK::AssetBuilderDesc materialBuilderDescriptor; materialBuilderDescriptor.m_name = JobKey; - materialBuilderDescriptor.m_version = 111; // material dependency improvements + materialBuilderDescriptor.m_version = 112; // material dependency improvements 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(); diff --git a/Gems/Atom/RPI/Code/Source/RPI.Builders/Model/MaterialAssetBuilderComponent.cpp b/Gems/Atom/RPI/Code/Source/RPI.Builders/Model/MaterialAssetBuilderComponent.cpp index ef251b6848..6f71fb70d4 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Builders/Model/MaterialAssetBuilderComponent.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Builders/Model/MaterialAssetBuilderComponent.cpp @@ -127,7 +127,7 @@ namespace AZ if (auto* serialize = azrtti_cast(context)) { serialize->Class() - ->Version(17); // Optional material conversion + ->Version(18); // material dependency improvements } } 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 cd01544bfa..23657f4871 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialSourceData.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialSourceData.cpp @@ -114,7 +114,7 @@ namespace AZ } } - materialAssetCreator.Begin(assetId, materialTypeAsset); + materialAssetCreator.Begin(assetId, materialTypeAsset, processingMode == MaterialAssetProcessingMode::PreBake); if (!m_parentMaterial.empty()) { @@ -180,11 +180,6 @@ namespace AZ Data::Asset material; if (materialAssetCreator.End(material)) { - if (processingMode == MaterialAssetProcessingMode::PreBake) - { - material->Finalize(); - } - return Success(material); } else @@ -270,11 +265,18 @@ namespace AZ parentSourceAbsPath = AssetUtils::ResolvePathReference(parentSourceAbsPath, parentSourceRelPath); parentSourceDataStack.emplace_back(AZStd::move(parentSourceData)); } + + // Unlike CreateMaterialAsset(), we can always finalize the material here because we loaded created the MaterialTypeAsset from + // the source .materialtype file, so the necessary data is always available. + // (In case you are wondering why we don't use CreateMaterialAssetFromSourceData in MaterialBuilder: that would require a + // source dependency between the .materialtype and .material file, which would cause all .material files to rebuild when you + // edit the .materialtype; it's faster to not read the material type data at all ... until it's needed at runtime) + const bool finalize = true; // Create the material asset from all the previously loaded source data MaterialAssetCreator materialAssetCreator; materialAssetCreator.SetElevateWarnings(elevateWarnings); - materialAssetCreator.Begin(assetId, materialTypeAsset.GetValue()); + materialAssetCreator.Begin(assetId, materialTypeAsset.GetValue(), finalize); while (!parentSourceDataStack.empty()) { @@ -287,13 +289,6 @@ namespace AZ Data::Asset material; if (materialAssetCreator.End(material)) { - // Unlike CreateMaterialAsset(), we can always finalize the material here because we loaded created the MaterialTypeAsset from - // the source .materialtype file, so the necessary data is always available. - // (In case you are wondering why we don't use CreateMaterialAssetFromSourceData in MaterialBuilder: that would require a - // source dependency between the .materialtype and .material file, which would cause all .material files to rebuild when you - // edit the .materialtype; it's faster to not read the material type data at all ... until it's needed at runtime) - material->Finalize(); - if (sourceDependencies) { sourceDependencies->insert(dependencies.begin(), dependencies.end()); 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 475c6ca219..2fe30f632f 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialUtils.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialUtils.cpp @@ -150,6 +150,7 @@ namespace AZ return shouldFinalize; } + } } } diff --git a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialAsset.cpp b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialAsset.cpp index 7962844736..ce5847d12f 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialAsset.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialAsset.cpp @@ -114,13 +114,29 @@ namespace AZ return m_isFinalized; } - void MaterialAsset::Finalize() + void MaterialAsset::Finalize(AZStd::function reportWarning, AZStd::function reportError) { if (IsFinalized()) { return; } + if (!reportWarning) + { + reportWarning = [](const char* message) + { + AZ_Warning(s_debugTraceName, false, "%s", message); + }; + } + + if (!reportError) + { + reportError = [](const char* message) + { + AZ_Error(s_debugTraceName, false, "%s", message); + }; + } + const uint32_t materialTypeVersion = m_materialTypeAsset->GetVersion(); if (m_materialTypeVersion < materialTypeVersion) { @@ -147,7 +163,7 @@ namespace AZ uint32_t enumValue = propertyDescriptor->GetEnumValue(enumName); if (enumValue == MaterialPropertyDescriptor::InvalidEnumValue) { - AZ_Error(s_debugTraceName, false, "Material property name \"%s\" has invalid enum value \"%s\".", name.GetCStr(), enumName.GetCStr()); + reportWarning(AZStd::string::format("Material property name \"%s\" has invalid enum value \"%s\".", name.GetCStr(), enumName.GetCStr()).c_str()); } else { @@ -164,12 +180,15 @@ namespace AZ } else { - finalizedPropertyValues[propertyIndex.GetIndex()] = value; + if (ValidateMaterialPropertyDataType(value.GetTypeId(), name, propertyDescriptor, reportError)) + { + finalizedPropertyValues[propertyIndex.GetIndex()] = value; + } } } else { - AZ_Warning(s_debugTraceName, false, "Material property name \"%s\" is not found in the material properties layout and will not be used.", name.GetCStr()); + reportWarning(AZStd::string::format("Material property name \"%s\" is not found in the material properties layout and will not be used.", name.GetCStr()).c_str()); } } diff --git a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialAssetCreator.cpp b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialAssetCreator.cpp index 4bee663791..f05fb8d848 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialAssetCreator.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialAssetCreator.cpp @@ -16,16 +16,21 @@ namespace AZ { namespace RPI { - void MaterialAssetCreator::Begin(const Data::AssetId& assetId, const Data::Asset& materialType) + void MaterialAssetCreator::Begin(const Data::AssetId& assetId, const Data::Asset& materialType, bool shouldFinalize) { BeginCommon(assetId); if (ValidateIsReady()) { - m_asset->m_materialTypeAsset = materialType; + m_shouldFinalize = shouldFinalize; + m_asset->m_materialTypeAsset = materialType; m_asset->m_materialTypeAsset.SetAutoLoadBehavior(AZ::Data::AssetLoadBehavior::PreLoad); - + + if (shouldFinalize && !m_asset->m_materialTypeAsset) + { + ReportError("MaterialTypeAsset is null, the MaterialAsset cannot be finalized"); + } } } @@ -37,6 +42,14 @@ namespace AZ } m_asset->SetReady(); + + if (m_shouldFinalize) + { + m_asset->Finalize( + [this](const char* message) { ReportWarning("%s", message); }, + [this](const char* message) { ReportError("%s", message); }); + } + return EndCommon(result); } @@ -71,6 +84,21 @@ namespace AZ m_asset->m_rawPropertyValues.emplace_back(name, value); } } + + void MaterialAssetCreator::SetPropertyValue(const Name& name, const Data::Asset& imageAsset) + { + SetPropertyValue(name, MaterialPropertyValue{imageAsset}); + } + + void MaterialAssetCreator::SetPropertyValue(const Name& name, const Data::Asset& imageAsset) + { + SetPropertyValue(name, Data::Asset(imageAsset)); + } + + void MaterialAssetCreator::SetPropertyValue(const Name& name, const Data::Asset& imageAsset) + { + SetPropertyValue(name, Data::Asset(imageAsset)); + } } // namespace RPI } // namespace AZ diff --git a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialAssetCreatorCommon.cpp b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialAssetCreatorCommon.cpp index 1ba74ce0b9..4b2f163f7c 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialAssetCreatorCommon.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialAssetCreatorCommon.cpp @@ -7,6 +7,7 @@ */ #include +#include namespace AZ { @@ -32,57 +33,6 @@ namespace AZ m_reportError = nullptr; } - MaterialPropertyDataType MaterialAssetCreatorCommon::GetMaterialPropertyDataType(TypeId typeId) const - { - if (typeId == azrtti_typeid()) { return MaterialPropertyDataType::Bool; } - if (typeId == azrtti_typeid()) { return MaterialPropertyDataType::Int; } - if (typeId == azrtti_typeid()) { return MaterialPropertyDataType::UInt; } - if (typeId == azrtti_typeid()) { return MaterialPropertyDataType::Float; } - if (typeId == azrtti_typeid()) { return MaterialPropertyDataType::Vector2; } - if (typeId == azrtti_typeid()) { return MaterialPropertyDataType::Vector3; } - if (typeId == azrtti_typeid()) { return MaterialPropertyDataType::Vector4; } - if (typeId == azrtti_typeid()) { return MaterialPropertyDataType::Color; } - if (typeId == azrtti_typeid>()) { return MaterialPropertyDataType::Image; } - else - { - return MaterialPropertyDataType::Invalid; - } - } - - bool MaterialAssetCreatorCommon::ValidateDataType(TypeId typeId, const Name& propertyName, const MaterialPropertyDescriptor* materialPropertyDescriptor) - { - auto expectedDataType = materialPropertyDescriptor->GetDataType(); - auto actualDataType = GetMaterialPropertyDataType(typeId); - - if (expectedDataType == MaterialPropertyDataType::Enum) - { - if (actualDataType != MaterialPropertyDataType::UInt) - { - m_reportError( - AZStd::string::format("Material property '%s' is a Enum type, can only accept UInt value, input value is %s", - propertyName.GetCStr(), - ToString(actualDataType) - ).data()); - return false; - } - } - else - { - if (expectedDataType != actualDataType) - { - m_reportError( - AZStd::string::format("Material property '%s': Type mismatch. Expected %s but was %s", - propertyName.GetCStr(), - ToString(expectedDataType), - ToString(actualDataType) - ).data()); - return false; - } - } - - return true; - } - bool MaterialAssetCreatorCommon::PropertyCheck(TypeId typeId, const Name& name) { if (!m_reportWarning || !m_reportError) @@ -108,7 +58,7 @@ namespace AZ return false; } - if (!ValidateDataType(typeId, name, materialPropertyDescriptor)) + if (!ValidateMaterialPropertyDataType(typeId, name, materialPropertyDescriptor, m_reportError)) { return false; } diff --git a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialPropertyDescriptor.cpp b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialPropertyDescriptor.cpp index 5d0a88a6d3..ddbb902761 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialPropertyDescriptor.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialPropertyDescriptor.cpp @@ -97,6 +97,57 @@ namespace AZ return AZStd::string::format("", typeId.ToString().c_str()); } } + + bool ValidateMaterialPropertyDataType(TypeId typeId, const Name& propertyName, const MaterialPropertyDescriptor* materialPropertyDescriptor, AZStd::function onError) + { + auto toMaterialPropertyDataType = [](TypeId typeId) + { + if (typeId == azrtti_typeid()) { return MaterialPropertyDataType::Bool; } + if (typeId == azrtti_typeid()) { return MaterialPropertyDataType::Int; } + if (typeId == azrtti_typeid()) { return MaterialPropertyDataType::UInt; } + if (typeId == azrtti_typeid()) { return MaterialPropertyDataType::Float; } + if (typeId == azrtti_typeid()) { return MaterialPropertyDataType::Vector2; } + if (typeId == azrtti_typeid()) { return MaterialPropertyDataType::Vector3; } + if (typeId == azrtti_typeid()) { return MaterialPropertyDataType::Vector4; } + if (typeId == azrtti_typeid()) { return MaterialPropertyDataType::Color; } + if (typeId == azrtti_typeid>()) { return MaterialPropertyDataType::Image; } + else + { + return MaterialPropertyDataType::Invalid; + } + }; + + auto expectedDataType = materialPropertyDescriptor->GetDataType(); + auto actualDataType = toMaterialPropertyDataType(typeId); + + if (expectedDataType == MaterialPropertyDataType::Enum) + { + if (actualDataType != MaterialPropertyDataType::UInt) + { + onError( + AZStd::string::format("Material property '%s' is a Enum type, can only accept UInt value, input value is %s", + propertyName.GetCStr(), + ToString(actualDataType) + ).data()); + return false; + } + } + else + { + if (expectedDataType != actualDataType) + { + onError( + AZStd::string::format("Material property '%s': Type mismatch. Expected %s but was %s", + propertyName.GetCStr(), + ToString(expectedDataType), + ToString(actualDataType) + ).data()); + return false; + } + } + + return true; + } void MaterialPropertyOutputId::Reflect(ReflectContext* context) { diff --git a/Gems/Atom/RPI/Code/Tests/Material/LuaMaterialFunctorTests.cpp b/Gems/Atom/RPI/Code/Tests/Material/LuaMaterialFunctorTests.cpp index 2608ac3a9b..5016f8303e 100644 --- a/Gems/Atom/RPI/Code/Tests/Material/LuaMaterialFunctorTests.cpp +++ b/Gems/Atom/RPI/Code/Tests/Material/LuaMaterialFunctorTests.cpp @@ -112,7 +112,7 @@ namespace UnitTest Data::Asset materialAsset; MaterialAssetCreator materialCreator; - materialCreator.Begin(Uuid::CreateRandom(), *m_materialTypeAsset); + materialCreator.Begin(Uuid::CreateRandom(), m_materialTypeAsset, true); EXPECT_TRUE(materialCreator.End(materialAsset)); m_material = Material::Create(materialAsset); @@ -138,7 +138,7 @@ namespace UnitTest Data::Asset materialAsset; MaterialAssetCreator materialCreator; - materialCreator.Begin(Uuid::CreateRandom(), *m_materialTypeAsset); + materialCreator.Begin(Uuid::CreateRandom(),m_materialTypeAsset, true); EXPECT_TRUE(materialCreator.End(materialAsset)); m_material = Material::Create(materialAsset); @@ -165,7 +165,7 @@ namespace UnitTest Data::Asset materialAsset; MaterialAssetCreator materialCreator; - materialCreator.Begin(Uuid::CreateRandom(), *m_materialTypeAsset); + materialCreator.Begin(Uuid::CreateRandom(), m_materialTypeAsset, true); EXPECT_TRUE(materialCreator.End(materialAsset)); m_material = Material::Create(materialAsset); @@ -194,7 +194,7 @@ namespace UnitTest Data::Asset materialAsset; MaterialAssetCreator materialCreator; - materialCreator.Begin(Uuid::CreateRandom(), *m_materialTypeAsset); + materialCreator.Begin(Uuid::CreateRandom(), m_materialTypeAsset, true); EXPECT_TRUE(materialCreator.End(materialAsset)); m_material = Material::Create(materialAsset); diff --git a/Gems/Atom/RPI/Code/Tests/Material/MaterialAssetTests.cpp b/Gems/Atom/RPI/Code/Tests/Material/MaterialAssetTests.cpp index 58a852f176..633953cecc 100644 --- a/Gems/Atom/RPI/Code/Tests/Material/MaterialAssetTests.cpp +++ b/Gems/Atom/RPI/Code/Tests/Material/MaterialAssetTests.cpp @@ -94,7 +94,7 @@ namespace UnitTest Data::AssetId assetId(Uuid::CreateRandom()); MaterialAssetCreator creator; - creator.Begin(assetId, *m_testMaterialTypeAsset); + creator.Begin(assetId, m_testMaterialTypeAsset, true); creator.SetPropertyValue(Name{ "MyFloat2" }, Vector2{ 0.1f, 0.2f }); creator.SetPropertyValue(Name{ "MyFloat3" }, Vector3{ 1.1f, 1.2f, 1.3f }); creator.SetPropertyValue(Name{ "MyFloat4" }, Vector4{ 2.1f, 2.2f, 2.3f, 2.4f }); @@ -129,7 +129,7 @@ namespace UnitTest Data::AssetId assetId(Uuid::CreateRandom()); MaterialAssetCreator creator; - creator.Begin(assetId, *m_testMaterialTypeAsset); + creator.Begin(assetId, m_testMaterialTypeAsset, true); creator.SetPropertyValue(Name{ "MyFloat" }, 3.14f); Data::Asset materialAsset; @@ -171,7 +171,7 @@ namespace UnitTest Data::Asset materialAsset; MaterialAssetCreator materialCreator; - materialCreator.Begin(Uuid::CreateRandom(), *emptyMaterialTypeAsset); + materialCreator.Begin(Uuid::CreateRandom(), emptyMaterialTypeAsset, true); EXPECT_TRUE(materialCreator.End(materialAsset)); EXPECT_EQ(emptyMaterialTypeAsset, materialAsset->GetMaterialTypeAsset()); EXPECT_EQ(materialAsset->GetPropertyValues().size(), 0); @@ -189,7 +189,7 @@ namespace UnitTest Data::AssetId assetId(Uuid::CreateRandom()); MaterialAssetCreator creator; - creator.Begin(assetId, *m_testMaterialTypeAsset); + creator.Begin(assetId, m_testMaterialTypeAsset, true); creator.SetPropertyValue(Name{ "MyImage" }, streamingImageAsset); Data::Asset materialAsset; @@ -231,8 +231,8 @@ namespace UnitTest Data::AssetId assetId(Uuid::CreateRandom()); MaterialAssetCreator creator; - const bool includePropertyNames = true; - creator.Begin(assetId, *testMaterialTypeAssetV1, includePropertyNames); + const bool shouldFinalize = false; + creator.Begin(assetId, testMaterialTypeAssetV1, shouldFinalize); creator.SetPropertyValue(Name{ "MyInt" }, 7); creator.SetPropertyValue(Name{ "MyUInt" }, 8u); creator.SetPropertyValue(Name{ "MyFloat" }, 9.0f); @@ -307,26 +307,68 @@ namespace UnitTest // We use local functions to easily start a new MaterialAssetCreator for each test case because // the AssetCreator would just skip subsequent operations after the first failure is detected. - auto expectCreatorError = [this](AZStd::function passBadInput) + auto expectCreatorError = [this](const char* expectedErrorMessage, AZStd::function passBadInput) { - MaterialAssetCreator creator; - creator.Begin(Uuid::CreateRandom(), *m_testMaterialTypeAsset); + // Test with finalizing enabled + { + MaterialAssetCreator creator; + creator.Begin(Uuid::CreateRandom(), m_testMaterialTypeAsset, true); + + ErrorMessageFinder errorMessageFinder; + errorMessageFinder.AddExpectedErrorMessage(expectedErrorMessage); + errorMessageFinder.AddIgnoredErrorMessage("Failed to build", true); + + passBadInput(creator); - AZ_TEST_START_ASSERTTEST; - passBadInput(creator); - AZ_TEST_STOP_ASSERTTEST(1); + Data::Asset materialAsset; + EXPECT_FALSE(creator.End(materialAsset)); + + errorMessageFinder.CheckExpectedErrorsFound(); + + EXPECT_TRUE(creator.GetErrorCount() > 0); + } + + // Test with finalizing disabled, so no validation occurs because the MaterialTypeAsset data is not used. + { + MaterialAssetCreator creator; + creator.Begin(Uuid::CreateRandom(), m_testMaterialTypeAsset, false); - EXPECT_EQ(1, creator.GetErrorCount()); + passBadInput(creator); + + Data::Asset materialAsset; + EXPECT_TRUE(creator.End(materialAsset)); + + EXPECT_EQ(creator.GetErrorCount(), 0); + } }; auto expectCreatorWarning = [this](AZStd::function passBadInput) { - MaterialAssetCreator creator; - creator.Begin(Uuid::CreateRandom(), *m_testMaterialTypeAsset); + // Test with finalizing enabled + { + MaterialAssetCreator creator; + creator.Begin(Uuid::CreateRandom(), m_testMaterialTypeAsset, true); + + passBadInput(creator); + + Data::Asset material; + creator.End(material); - passBadInput(creator); + EXPECT_EQ(1, creator.GetWarningCount()); + } + + // Test with finalizing disabled, so no validation occurs because the MaterialTypeAsset data is not used. + { + MaterialAssetCreator creator; + creator.Begin(Uuid::CreateRandom(), m_testMaterialTypeAsset, false); + + passBadInput(creator); + + Data::Asset material; + creator.End(material); - EXPECT_EQ(1, creator.GetWarningCount()); + EXPECT_EQ(0, creator.GetWarningCount()); + } }; // Invalid input ID @@ -343,55 +385,65 @@ namespace UnitTest // Test data type mismatches... - expectCreatorError([this](MaterialAssetCreator& creator) - { - creator.SetPropertyValue(Name{ "MyBool" }, m_testImageAsset); - }); + expectCreatorError("Type mismatch", + [this](MaterialAssetCreator& creator) + { + creator.SetPropertyValue(Name{ "MyBool" }, m_testImageAsset); + }); - expectCreatorError([](MaterialAssetCreator& creator) - { - creator.SetPropertyValue(Name{ "MyInt" }, 0.0f); - }); + expectCreatorError("Type mismatch", + [](MaterialAssetCreator& creator) + { + creator.SetPropertyValue(Name{ "MyInt" }, 0.0f); + }); - expectCreatorError([](MaterialAssetCreator& creator) - { - creator.SetPropertyValue(Name{ "MyUInt" }, -1); - }); + expectCreatorError("Type mismatch", + [](MaterialAssetCreator& creator) + { + creator.SetPropertyValue(Name{ "MyUInt" }, -1); + }); - expectCreatorError([](MaterialAssetCreator& creator) - { - creator.SetPropertyValue(Name{ "MyFloat" }, 10u); - }); + expectCreatorError("Type mismatch", + [](MaterialAssetCreator& creator) + { + creator.SetPropertyValue(Name{ "MyFloat" }, 10u); + }); - expectCreatorError([](MaterialAssetCreator& creator) - { - creator.SetPropertyValue(Name{ "MyFloat2" }, 1.0f); - }); + expectCreatorError("Type mismatch", + [](MaterialAssetCreator& creator) + { + creator.SetPropertyValue(Name{ "MyFloat2" }, 1.0f); + }); - expectCreatorError([](MaterialAssetCreator& creator) - { - creator.SetPropertyValue(Name{ "MyFloat3" }, AZ::Vector4{}); - }); + expectCreatorError("Type mismatch", + [](MaterialAssetCreator& creator) + { + creator.SetPropertyValue(Name{ "MyFloat3" }, AZ::Vector4{}); + }); - expectCreatorError([](MaterialAssetCreator& creator) - { - creator.SetPropertyValue(Name{ "MyFloat4" }, AZ::Vector3{}); - }); + expectCreatorError("Type mismatch", + [](MaterialAssetCreator& creator) + { + creator.SetPropertyValue(Name{ "MyFloat4" }, AZ::Vector3{}); + }); - expectCreatorError([](MaterialAssetCreator& creator) - { - creator.SetPropertyValue(Name{ "MyColor" }, MaterialPropertyValue(false)); - }); + expectCreatorError("Type mismatch", + [](MaterialAssetCreator& creator) + { + creator.SetPropertyValue(Name{ "MyColor" }, MaterialPropertyValue(false)); + }); - expectCreatorError([](MaterialAssetCreator& creator) - { - creator.SetPropertyValue(Name{ "MyImage" }, true); - }); + expectCreatorError("Type mismatch", + [](MaterialAssetCreator& creator) + { + creator.SetPropertyValue(Name{ "MyImage" }, true); + }); - expectCreatorError([](MaterialAssetCreator& creator) - { - creator.SetPropertyValue(Name{ "MyEnum" }, -1); - }); + expectCreatorError("can only accept UInt value", + [](MaterialAssetCreator& creator) + { + creator.SetPropertyValue(Name{ "MyEnum" }, -1); + }); } } diff --git a/Gems/Atom/RPI/Code/Tests/Material/MaterialFunctorTests.cpp b/Gems/Atom/RPI/Code/Tests/Material/MaterialFunctorTests.cpp index ff5d24ff9a..3373646c6e 100644 --- a/Gems/Atom/RPI/Code/Tests/Material/MaterialFunctorTests.cpp +++ b/Gems/Atom/RPI/Code/Tests/Material/MaterialFunctorTests.cpp @@ -275,7 +275,7 @@ namespace UnitTest materialTypeCreator.End(m_testMaterialTypeAsset); MaterialAssetCreator materialCreator; - materialCreator.Begin(Uuid::CreateRandom(), *m_testMaterialTypeAsset); + materialCreator.Begin(Uuid::CreateRandom(), m_testMaterialTypeAsset, true); materialCreator.SetPropertyValue(registedPropertyName, 42); materialCreator.SetPropertyValue(unregistedPropertyName, 42); materialCreator.SetPropertyValue(unrelatedPropertyName, 42); diff --git a/Gems/Atom/RPI/Code/Tests/Material/MaterialSourceDataTests.cpp b/Gems/Atom/RPI/Code/Tests/Material/MaterialSourceDataTests.cpp index d4bf3e5eaa..73aeacf818 100644 --- a/Gems/Atom/RPI/Code/Tests/Material/MaterialSourceDataTests.cpp +++ b/Gems/Atom/RPI/Code/Tests/Material/MaterialSourceDataTests.cpp @@ -175,7 +175,7 @@ namespace UnitTest AddProperty(sourceData, "general", "MyImage", AZStd::string("@exefolder@/Temp/test.streamingimage")); AddProperty(sourceData, "general", "MyEnum", AZStd::string("Enum1")); - auto materialAssetOutcome = sourceData.CreateMaterialAsset(Uuid::CreateRandom(), "", true); + auto materialAssetOutcome = sourceData.CreateMaterialAsset(Uuid::CreateRandom(), "", MaterialAssetProcessingMode::PreBake, true); EXPECT_TRUE(materialAssetOutcome.IsSuccess()); Data::Asset materialAsset = materialAssetOutcome.GetValue(); @@ -544,17 +544,17 @@ namespace UnitTest AddPropertyGroup(sourceDataLevel3, "general"); AddProperty(sourceDataLevel3, "general", "MyFloat", 3.5f); - auto materialAssetLevel1 = sourceDataLevel1.CreateMaterialAsset(Uuid::CreateRandom(), "", true); + auto materialAssetLevel1 = sourceDataLevel1.CreateMaterialAsset(Uuid::CreateRandom(), "", MaterialAssetProcessingMode::PreBake, true); EXPECT_TRUE(materialAssetLevel1.IsSuccess()); m_assetSystemStub.RegisterSourceInfo("level1.material", materialAssetLevel1.GetValue().GetId()); - auto materialAssetLevel2 = sourceDataLevel2.CreateMaterialAsset(Uuid::CreateRandom(), "", true); + auto materialAssetLevel2 = sourceDataLevel2.CreateMaterialAsset(Uuid::CreateRandom(), "", MaterialAssetProcessingMode::PreBake, true); EXPECT_TRUE(materialAssetLevel2.IsSuccess()); m_assetSystemStub.RegisterSourceInfo("level2.material", materialAssetLevel2.GetValue().GetId()); - auto materialAssetLevel3 = sourceDataLevel3.CreateMaterialAsset(Uuid::CreateRandom(), "", true); + auto materialAssetLevel3 = sourceDataLevel3.CreateMaterialAsset(Uuid::CreateRandom(), "", MaterialAssetProcessingMode::PreBake, true); EXPECT_TRUE(materialAssetLevel3.IsSuccess()); auto layout = m_testMaterialTypeAsset->GetMaterialPropertiesLayout(); @@ -604,18 +604,18 @@ namespace UnitTest sourceDataLevel3.m_materialType = "@exefolder@/Temp/otherBase.materialtype"; sourceDataLevel3.m_parentMaterial = "level2.material"; - auto materialAssetLevel1 = sourceDataLevel1.CreateMaterialAsset(Uuid::CreateRandom(), "", true); + auto materialAssetLevel1 = sourceDataLevel1.CreateMaterialAsset(Uuid::CreateRandom(), "", MaterialAssetProcessingMode::PreBake, true); EXPECT_TRUE(materialAssetLevel1.IsSuccess()); m_assetSystemStub.RegisterSourceInfo("level1.material", materialAssetLevel1.GetValue().GetId()); - auto materialAssetLevel2 = sourceDataLevel2.CreateMaterialAsset(Uuid::CreateRandom(), "", true); + auto materialAssetLevel2 = sourceDataLevel2.CreateMaterialAsset(Uuid::CreateRandom(), "", MaterialAssetProcessingMode::PreBake, true); EXPECT_TRUE(materialAssetLevel2.IsSuccess()); m_assetSystemStub.RegisterSourceInfo("level2.material", materialAssetLevel2.GetValue().GetId()); AZ_TEST_START_ASSERTTEST; - auto materialAssetLevel3 = sourceDataLevel3.CreateMaterialAsset(Uuid::CreateRandom(), "", true); + auto materialAssetLevel3 = sourceDataLevel3.CreateMaterialAsset(Uuid::CreateRandom(), "", MaterialAssetProcessingMode::PreBake, true); AZ_TEST_STOP_ASSERTTEST(1); EXPECT_FALSE(materialAssetLevel3.IsSuccess()); } @@ -625,7 +625,7 @@ namespace UnitTest // We use local functions to easily start a new MaterialAssetCreator for each test case because // the AssetCreator would just skip subsequent operations after the first failure is detected. - auto expectWarning = [](AZStd::function setOneBadInput, [[maybe_unused]] uint32_t expectedAsserts = 1) + auto expectWarning = [](const char* expectedErrorMessage, AZStd::function setOneBadInput, bool warningOccursBeforeFinalize = false) { MaterialSourceData sourceData; @@ -635,233 +635,69 @@ namespace UnitTest setOneBadInput(sourceData); - AZ_TEST_START_ASSERTTEST; - auto materialAssetOutcome = sourceData.CreateMaterialAsset(Uuid::CreateRandom(), "", true); - AZ_TEST_STOP_ASSERTTEST(expectedAsserts); // Usually just one for when End() is called + // Check with MaterialAssetProcessingMode::PreBake + { + ErrorMessageFinder errorFinder; + errorFinder.AddExpectedErrorMessage(expectedErrorMessage); + errorFinder.AddIgnoredErrorMessage("Failed to build", true); + auto materialAssetOutcome = sourceData.CreateMaterialAsset(Uuid::CreateRandom(), "", MaterialAssetProcessingMode::PreBake, true); + errorFinder.CheckExpectedErrorsFound(); - EXPECT_FALSE(materialAssetOutcome.IsSuccess()); + EXPECT_FALSE(materialAssetOutcome.IsSuccess()); + } + + // Check with MaterialAssetProcessingMode::DeferredBake, no validation occurs because the MaterialTypeAsset cannot be used and so the MaterialAsset is not finalized + if(!warningOccursBeforeFinalize) + { + auto materialAssetOutcome = sourceData.CreateMaterialAsset(Uuid::CreateRandom(), "", MaterialAssetProcessingMode::DeferredBake, true); + EXPECT_TRUE(materialAssetOutcome.IsSuccess()); + } }; // Test property does not exist... - expectWarning([](MaterialSourceData& materialSourceData) - { - AddProperty(materialSourceData, "general", "DoesNotExist", true); - }); + expectWarning("\"general.DoesNotExist\" is not found in the material properties layout", + [](MaterialSourceData& materialSourceData) + { + AddProperty(materialSourceData, "general", "DoesNotExist", true); + }); - expectWarning([](MaterialSourceData& materialSourceData) - { - AddProperty(materialSourceData, "general", "DoesNotExist", -10); - }); + expectWarning("\"general.DoesNotExist\" is not found in the material properties layout", + [](MaterialSourceData& materialSourceData) + { + AddProperty(materialSourceData, "general", "DoesNotExist", -10); + }); - expectWarning([](MaterialSourceData& materialSourceData) - { - AddProperty(materialSourceData, "general", "DoesNotExist", 25u); - }); + expectWarning("\"general.DoesNotExist\" is not found in the material properties layout", + [](MaterialSourceData& materialSourceData) + { + AddProperty(materialSourceData, "general", "DoesNotExist", 25u); + }); - expectWarning([](MaterialSourceData& materialSourceData) - { - AddProperty(materialSourceData, "general", "DoesNotExist", 1.5f); - }); + expectWarning("\"general.DoesNotExist\" is not found in the material properties layout", + [](MaterialSourceData& materialSourceData) + { + AddProperty(materialSourceData, "general", "DoesNotExist", 1.5f); + }); - expectWarning([](MaterialSourceData& materialSourceData) - { - AddProperty(materialSourceData, "general", "DoesNotExist", AZ::Color{ 0.1f, 0.2f, 0.3f, 0.4f }); - }); + expectWarning("\"general.DoesNotExist\" is not found in the material properties layout", + [](MaterialSourceData& materialSourceData) + { + AddProperty(materialSourceData, "general", "DoesNotExist", AZ::Color{ 0.1f, 0.2f, 0.3f, 0.4f }); + }); - expectWarning([](MaterialSourceData& materialSourceData) - { - AddProperty(materialSourceData, "general", "DoesNotExist", AZStd::string("@exefolder@/Temp/test.streamingimage")); - }); + expectWarning("\"general.DoesNotExist\" is not found in the material properties layout", + [](MaterialSourceData& materialSourceData) + { + AddProperty(materialSourceData, "general", "DoesNotExist", AZStd::string("@exefolder@/Temp/test.streamingimage")); + }); // Missing image reference - expectWarning([](MaterialSourceData& materialSourceData) - { - AddProperty(materialSourceData, "general", "MyImage", AZStd::string("doesNotExist.streamingimage")); - }); - } - - - TEST_F(MaterialSourceDataTests, Load_MaterialTypeVersionUpdate) - { - const AZStd::string inputJson = R"( - { - "materialType": "@exefolder@/Temp/test.materialtype", - "materialTypeVersion": 1, - "properties": { - "general": { - "testColorNameA": [0.1, 0.2, 0.3] - } - } - } - )"; - - MaterialSourceData material; - JsonTestResult loadResult = LoadTestDataFromJson(material, inputJson); - - EXPECT_EQ(AZ::JsonSerializationResult::Tasks::ReadField, loadResult.m_jsonResultCode.GetTask()); - EXPECT_EQ(AZ::JsonSerializationResult::Processing::Completed, loadResult.m_jsonResultCode.GetProcessing()); - - // Initially, the loaded material data will match the .material file exactly. This gives us the accurate representation of - // what's actually saved on disk. - - EXPECT_NE(material.m_properties["general"].find("testColorNameA"), material.m_properties["general"].end()); - EXPECT_EQ(material.m_properties["general"].find("testColorNameB"), material.m_properties["general"].end()); - EXPECT_EQ(material.m_properties["general"].find("testColorNameC"), material.m_properties["general"].end()); - EXPECT_EQ(material.m_properties["general"].find("MyColor"), material.m_properties["general"].end()); - - AZ::Color testColor = material.m_properties["general"]["testColorNameA"].m_value.GetValue(); - EXPECT_TRUE(AZ::Color(0.1f, 0.2f, 0.3f, 1.0f).IsClose(testColor, 0.01)); - - EXPECT_EQ(1, material.m_materialTypeVersion); - - // Then we force the material data to update to the latest material type version specification - ErrorMessageFinder warningFinder; // Note this finds errors and warnings, and we're looking for a warning. - warningFinder.AddExpectedErrorMessage("Automatic updates are available. Consider updating the .material source file"); - warningFinder.AddExpectedErrorMessage("This material is based on version '1'"); - warningFinder.AddExpectedErrorMessage("material type is now at version '10'"); - material.ApplyVersionUpdates(); - warningFinder.CheckExpectedErrorsFound(); - - // Now the material data should match the latest material type. - // Look for the property under the latest name in the material type, not the name used in the .material file. - - EXPECT_EQ(material.m_properties["general"].find("testColorNameA"), material.m_properties["general"].end()); - EXPECT_EQ(material.m_properties["general"].find("testColorNameB"), material.m_properties["general"].end()); - EXPECT_EQ(material.m_properties["general"].find("testColorNameC"), material.m_properties["general"].end()); - EXPECT_NE(material.m_properties["general"].find("MyColor"), material.m_properties["general"].end()); - - testColor = material.m_properties["general"]["MyColor"].m_value.GetValue(); - EXPECT_TRUE(AZ::Color(0.1f, 0.2f, 0.3f, 1.0f).IsClose(testColor, 0.01)); - - EXPECT_EQ(10, material.m_materialTypeVersion); - - // Calling ApplyVersionUpdates() again should not report the warning again, since the material has already been updated. - warningFinder.Reset(); - material.ApplyVersionUpdates(); - } - - TEST_F(MaterialSourceDataTests, Load_MaterialTypeVersionUpdate_MovePropertiesToAnotherGroup) - { - const AZStd::string inputJson = R"( - { - "materialType": "@exefolder@/Temp/test.materialtype", - "materialTypeVersion": 3, - "properties": { - "oldGroup": { - "MyFloat": 1.2, - "MyIntOldName": 5 - } - } - } - )"; - - MaterialSourceData material; - JsonTestResult loadResult = LoadTestDataFromJson(material, inputJson); - - EXPECT_EQ(AZ::JsonSerializationResult::Tasks::ReadField, loadResult.m_jsonResultCode.GetTask()); - EXPECT_EQ(AZ::JsonSerializationResult::Processing::Completed, loadResult.m_jsonResultCode.GetProcessing()); - - // Initially, the loaded material data will match the .material file exactly. This gives us the accurate representation of - // what's actually saved on disk. - - EXPECT_NE(material.m_properties["oldGroup"].find("MyFloat"), material.m_properties["oldGroup"].end()); - EXPECT_NE(material.m_properties["oldGroup"].find("MyIntOldName"), material.m_properties["oldGroup"].end()); - EXPECT_EQ(material.m_properties["general"].find("MyFloat"), material.m_properties["general"].end()); - EXPECT_EQ(material.m_properties["general"].find("MyInt"), material.m_properties["general"].end()); - - float myFloat = material.m_properties["oldGroup"]["MyFloat"].m_value.GetValue(); - EXPECT_EQ(myFloat, 1.2f); - - int32_t myInt = material.m_properties["oldGroup"]["MyIntOldName"].m_value.GetValue(); - EXPECT_EQ(myInt, 5); - - EXPECT_EQ(3, material.m_materialTypeVersion); - - // Then we force the material data to update to the latest material type version specification - ErrorMessageFinder warningFinder; // Note this finds errors and warnings, and we're looking for a warning. - warningFinder.AddExpectedErrorMessage("Automatic updates are available. Consider updating the .material source file"); - warningFinder.AddExpectedErrorMessage("This material is based on version '3'"); - warningFinder.AddExpectedErrorMessage("material type is now at version '10'"); - material.ApplyVersionUpdates(); - warningFinder.CheckExpectedErrorsFound(); - - // Now the material data should match the latest material type. - // Look for the property under the latest name in the material type, not the name used in the .material file. - - EXPECT_EQ(material.m_properties["oldGroup"].find("MyFloat"), material.m_properties["oldGroup"].end()); - EXPECT_EQ(material.m_properties["oldGroup"].find("MyIntOldName"), material.m_properties["oldGroup"].end()); - EXPECT_NE(material.m_properties["general"].find("MyFloat"), material.m_properties["general"].end()); - EXPECT_NE(material.m_properties["general"].find("MyInt"), material.m_properties["general"].end()); - - myFloat = material.m_properties["general"]["MyFloat"].m_value.GetValue(); - EXPECT_EQ(myFloat, 1.2f); - - myInt = material.m_properties["general"]["MyInt"].m_value.GetValue(); - EXPECT_EQ(myInt, 5); - - EXPECT_EQ(10, material.m_materialTypeVersion); - - // Calling ApplyVersionUpdates() again should not report the warning again, since the material has already been updated. - warningFinder.Reset(); - material.ApplyVersionUpdates(); - } - - TEST_F(MaterialSourceDataTests, Load_MaterialTypeVersionPartialUpdate) - { - // This case is similar to Load_MaterialTypeVersionUpdate but we start at a later - // version so only some of the version updates are applied. - - const AZStd::string inputJson = R"( - { - "materialType": "@exefolder@/Temp/test.materialtype", - "materialTypeVersion": 3, - "properties": { - "general": { - "testColorNameB": [0.1, 0.2, 0.3] - } - } - } - )"; - - MaterialSourceData material; - JsonTestResult loadResult = LoadTestDataFromJson(material, inputJson); - - EXPECT_EQ(AZ::JsonSerializationResult::Tasks::ReadField, loadResult.m_jsonResultCode.GetTask()); - EXPECT_EQ(AZ::JsonSerializationResult::Processing::Completed, loadResult.m_jsonResultCode.GetProcessing()); - - material.ApplyVersionUpdates(); - - AZ::Color testColor = material.m_properties["general"]["MyColor"].m_value.GetValue(); - EXPECT_TRUE(AZ::Color(0.1f, 0.2f, 0.3f, 1.0f).IsClose(testColor, 0.01)); - - EXPECT_EQ(10, material.m_materialTypeVersion); - } - - TEST_F(MaterialSourceDataTests, Load_Error_MaterialTypeVersionUpdateWithMismatchedVersion) - { - const AZStd::string inputJson = R"( - { - "materialType": "@exefolder@/Temp/test.materialtype", - "materialTypeVersion": 3, // At this version, the property should be testColorNameB not testColorNameA - "properties": { - "general": { - "testColorNameA": [0.1, 0.2, 0.3] - } - } - } - )"; - - MaterialSourceData material; - JsonTestResult loadResult = LoadTestDataFromJson(material, inputJson); - - loadResult.ContainsMessage("/properties/general/testColorNameA", "Property 'general.testColorNameA' not found in material type."); - - EXPECT_FALSE(material.m_properties["general"]["testColorNameA"].m_value.IsValid()); - - material.ApplyVersionUpdates(); - - EXPECT_FALSE(material.m_properties["general"]["MyColor"].m_value.IsValid()); + expectWarning("Could not find the image 'doesNotExist.streamingimage'", + [](MaterialSourceData& materialSourceData) + { + AddProperty(materialSourceData, "general", "MyImage", AZStd::string("doesNotExist.streamingimage")); + }, true); // In this case, the warning does happen even when the asset is not finalized, because the image path is checked earlier than that } } diff --git a/Gems/Atom/RPI/Code/Tests/Material/MaterialTests.cpp b/Gems/Atom/RPI/Code/Tests/Material/MaterialTests.cpp index 477978875b..569f8f6df0 100644 --- a/Gems/Atom/RPI/Code/Tests/Material/MaterialTests.cpp +++ b/Gems/Atom/RPI/Code/Tests/Material/MaterialTests.cpp @@ -85,7 +85,7 @@ namespace UnitTest m_testImage = StreamingImage::FindOrCreate(m_testImageAsset); MaterialAssetCreator materialCreator; - materialCreator.Begin(Uuid::CreateRandom(), *m_testMaterialTypeAsset); + materialCreator.Begin(Uuid::CreateRandom(), m_testMaterialTypeAsset, true); materialCreator.SetPropertyValue(Name{ "MyFloat2" }, Vector2{ 0.1f, 0.2f }); materialCreator.SetPropertyValue(Name{ "MyFloat3" }, Vector3{ 1.1f, 1.2f, 1.3f }); materialCreator.SetPropertyValue(Name{ "MyFloat4" }, Vector4{ 2.1f, 2.2f, 2.3f, 2.4f }); @@ -289,7 +289,7 @@ namespace UnitTest materialTypeCreator.End(materialTypeAsset); MaterialAssetCreator materialAssetCreator; - materialAssetCreator.Begin(Uuid::CreateRandom(), *materialTypeAsset); + materialAssetCreator.Begin(Uuid::CreateRandom(), materialTypeAsset, true); materialAssetCreator.End(materialAsset); Data::Instance material = Material::FindOrCreate(materialAsset); @@ -341,7 +341,7 @@ namespace UnitTest Data::Asset materialAssetWithEmptyImage; MaterialAssetCreator materialCreator; - materialCreator.Begin(Uuid::CreateRandom(), *m_testMaterialTypeAsset); + materialCreator.Begin(Uuid::CreateRandom(), m_testMaterialTypeAsset, true); materialCreator.SetPropertyValue(Name{"MyFloat2"}, Vector2{0.1f, 0.2f}); materialCreator.SetPropertyValue(Name{"MyFloat3"}, Vector3{1.1f, 1.2f, 1.3f}); materialCreator.SetPropertyValue(Name{"MyFloat4"}, Vector4{2.1f, 2.2f, 2.3f, 2.4f}); @@ -379,7 +379,7 @@ namespace UnitTest Data::Asset emptyMaterialAsset; MaterialAssetCreator materialCreator; - materialCreator.Begin(Uuid::CreateRandom(), *emptyMaterialTypeAsset); + materialCreator.Begin(Uuid::CreateRandom(), emptyMaterialTypeAsset, true); EXPECT_TRUE(materialCreator.End(emptyMaterialAsset)); Data::Instance material = Material::FindOrCreate(emptyMaterialAsset); @@ -450,7 +450,7 @@ namespace UnitTest materialTypeCreator.End(m_testMaterialTypeAsset); MaterialAssetCreator materialAssetCreator; - materialAssetCreator.Begin(Uuid::CreateRandom(), *m_testMaterialTypeAsset); + materialAssetCreator.Begin(Uuid::CreateRandom(), m_testMaterialTypeAsset, true); materialAssetCreator.End(m_testMaterialAsset); Data::Instance material = Material::FindOrCreate(m_testMaterialAsset); @@ -521,7 +521,7 @@ namespace UnitTest materialTypeCreator.End(m_testMaterialTypeAsset); MaterialAssetCreator materialAssetCreator; - materialAssetCreator.Begin(Uuid::CreateRandom(), *m_testMaterialTypeAsset); + materialAssetCreator.Begin(Uuid::CreateRandom(), m_testMaterialTypeAsset, true); materialAssetCreator.End(m_testMaterialAsset); Data::Instance material = Material::FindOrCreate(m_testMaterialAsset); @@ -591,7 +591,7 @@ namespace UnitTest materialTypeCreator.End(m_testMaterialTypeAsset); MaterialAssetCreator materialAssetCreator; - materialAssetCreator.Begin(Uuid::CreateRandom(), *m_testMaterialTypeAsset); + materialAssetCreator.Begin(Uuid::CreateRandom(), m_testMaterialTypeAsset, true); materialAssetCreator.End(m_testMaterialAsset); Data::Instance material = Material::FindOrCreate(m_testMaterialAsset); @@ -655,7 +655,7 @@ namespace UnitTest materialTypeCreator.End(m_testMaterialTypeAsset); MaterialAssetCreator materialAssetCreator; - materialAssetCreator.Begin(Uuid::CreateRandom(), *m_testMaterialTypeAsset); + materialAssetCreator.Begin(Uuid::CreateRandom(), m_testMaterialTypeAsset, true); materialAssetCreator.End(m_testMaterialAsset); Data::Instance material = Material::FindOrCreate(m_testMaterialAsset); @@ -669,7 +669,7 @@ namespace UnitTest { Data::Asset materialAsset; MaterialAssetCreator materialCreator; - materialCreator.Begin(Uuid::CreateRandom(), *m_testMaterialTypeAsset); + materialCreator.Begin(Uuid::CreateRandom(), m_testMaterialTypeAsset, true); materialCreator.SetPropertyValue(Name{ "MyFloat2" }, Vector2{ 0.1f, 0.2f }); materialCreator.SetPropertyValue(Name{ "MyFloat3" }, Vector3{ 1.1f, 1.2f, 1.3f }); materialCreator.SetPropertyValue(Name{ "MyFloat4" }, Vector4{ 2.1f, 2.2f, 2.3f, 2.4f }); @@ -778,7 +778,7 @@ namespace UnitTest materialTypeCreator.End(materialTypeAsset); MaterialAssetCreator materialAssetCreator; - materialAssetCreator.Begin(Uuid::CreateRandom(), *materialTypeAsset); + materialAssetCreator.Begin(Uuid::CreateRandom(), materialTypeAsset, true); materialAssetCreator.End(materialAsset); Data::Instance material = Material::FindOrCreate(materialAsset); @@ -859,7 +859,7 @@ namespace UnitTest materialTypeCreator.End(m_testMaterialTypeAsset); MaterialAssetCreator materialCreator; - materialCreator.Begin(Uuid::CreateRandom(), *m_testMaterialTypeAsset); + materialCreator.Begin(Uuid::CreateRandom(), m_testMaterialTypeAsset, true); materialCreator.End(m_testMaterialAsset); Data::Instance material = Material::FindOrCreate(m_testMaterialAsset); From 1fa1eaad158fae98e0339411e3fead04a6b120c9 Mon Sep 17 00:00:00 2001 From: santorac <55155825+santorac@users.noreply.github.com> Date: Tue, 28 Dec 2021 17:51:43 -0800 Subject: [PATCH 03/10] Added unit tests for the new functionality. I found a mistake where MaterialAssetCreator needs to clear the raw data when configured to finalize the material asset. Since MaterialSourceData no longer relies on the material type source file at all, I was able to change MaterialSourceDataTest to avoid saving the source data to disk. Signed-off-by: santorac <55155825+santorac@users.noreply.github.com> --- .../Atom/RPI.Edit/Material/MaterialUtils.h | 1 - .../Atom/RPI.Reflect/Material/MaterialAsset.h | 1 - .../RPI.Edit/Material/MaterialUtils.cpp | 1 - .../Material/MaterialAssetCreator.cpp | 5 + .../Material/MaterialSourceDataTests.cpp | 248 ++++++++++++++++-- 5 files changed, 230 insertions(+), 26 deletions(-) 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 2a992159b3..2fb55e5f40 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 @@ -69,7 +69,6 @@ namespace AZ //! Finalizing during asset processing reduces load times and obfuscates the material data. //! Waiting to finalize at load time reduces dependencies on the material type data, resulting in fewer asset rebuilds and less time spent processing assets. bool BuildersShouldFinalizeMaterialAssets(); - } } } diff --git a/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialAsset.h b/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialAsset.h index 736d7c5b53..4cc6608225 100644 --- a/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialAsset.h +++ b/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialAsset.h @@ -15,7 +15,6 @@ #include #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 2fe30f632f..475c6ca219 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialUtils.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialUtils.cpp @@ -150,7 +150,6 @@ namespace AZ return shouldFinalize; } - } } } diff --git a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialAssetCreator.cpp b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialAssetCreator.cpp index f05fb8d848..7d4ecf0b18 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialAssetCreator.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialAssetCreator.cpp @@ -48,6 +48,11 @@ namespace AZ m_asset->Finalize( [this](const char* message) { ReportWarning("%s", message); }, [this](const char* message) { ReportError("%s", message); }); + + // Finalize() doesn't clear the raw property data because that's the same function used at runtime, which does need to maintain the raw data + // to support hot reload. But here we are pre-baking with the assumption that AP build dependencies will keep the material type + // and material asset in sync, so we can discard the raw property data and just rely on the data in the material type asset. + m_asset->m_rawPropertyValues.clear(); } return EndCommon(result); diff --git a/Gems/Atom/RPI/Code/Tests/Material/MaterialSourceDataTests.cpp b/Gems/Atom/RPI/Code/Tests/Material/MaterialSourceDataTests.cpp index 73aeacf818..e4d3cc9768 100644 --- a/Gems/Atom/RPI/Code/Tests/Material/MaterialSourceDataTests.cpp +++ b/Gems/Atom/RPI/Code/Tests/Material/MaterialSourceDataTests.cpp @@ -11,6 +11,7 @@ #include #include #include +#include #include #include @@ -65,9 +66,29 @@ namespace UnitTest m_testShaderAsset = CreateTestShaderAsset(Uuid::CreateRandom(), m_testMaterialSrgLayout); m_assetSystemStub.RegisterSourceInfo("@exefolder@/Temp/test.shader", m_testShaderAsset.GetId()); - // The MaterialSourceData relies on both MaterialTypeSourceData and MaterialTypeAsset. We have to make sure the - // .materialtype file is present on disk, and that the MaterialTypeAsset is available through the asset database stub... + m_testMaterialTypeAsset = CreateTestMaterialTypeAsset(Uuid::CreateRandom()); + // Since this test doesn't actually instantiate a Material, it won't need to instantiate this ImageAsset, so all we + // need is an asset reference with a valid ID. + m_testImageAsset = Data::Asset{ Data::AssetId{Uuid::CreateRandom(), StreamingImageAsset::GetImageAssetSubId()}, azrtti_typeid() }; + + // Register the test assets with the AssetSystemStub so CreateMaterialAsset() can use AssetUtils. + m_assetSystemStub.RegisterSourceInfo("@exefolder@/Temp/test.materialtype", m_testMaterialTypeAsset.GetId()); + m_assetSystemStub.RegisterSourceInfo("@exefolder@/Temp/test.streamingimage", m_testImageAsset.GetId()); + } + + void TearDown() override + { + m_testMaterialTypeAsset.Reset(); + m_testMaterialSrgLayout = nullptr; + m_testShaderAsset.Reset(); + m_testImageAsset.Reset(); + + RPITestFixture::TearDown(); + } + + Data::Asset CreateTestMaterialTypeAsset(Data::AssetId assetId) + { const char* materialTypeJson = R"( { "version": 10, @@ -122,29 +143,10 @@ namespace UnitTest } )"; - AZ::Utils::WriteFile(materialTypeJson, "@exefolder@/Temp/test.materialtype"); MaterialTypeSourceData materialTypeSourceData; LoadTestDataFromJson(materialTypeSourceData, materialTypeJson); - m_testMaterialTypeAsset = materialTypeSourceData.CreateMaterialTypeAsset(Uuid::CreateRandom()).TakeValue(); - - // Since this test doesn't actually instantiate a Material, it won't need to instantiate this ImageAsset, so all we - // need is an asset reference with a valid ID. - m_testImageAsset = Data::Asset{ Data::AssetId{Uuid::CreateRandom(), StreamingImageAsset::GetImageAssetSubId()}, azrtti_typeid() }; - - // Register the test assets with the AssetSystemStub so CreateMaterialAsset() can use AssetUtils. - m_assetSystemStub.RegisterSourceInfo("@exefolder@/Temp/test.materialtype", m_testMaterialTypeAsset.GetId()); - m_assetSystemStub.RegisterSourceInfo("@exefolder@/Temp/test.streamingimage", m_testImageAsset.GetId()); - } - - void TearDown() override - { - m_testMaterialTypeAsset.Reset(); - m_testMaterialSrgLayout = nullptr; - m_testShaderAsset.Reset(); - m_testImageAsset.Reset(); - - RPITestFixture::TearDown(); + return materialTypeSourceData.CreateMaterialTypeAsset(assetId).TakeValue(); } }; @@ -180,7 +182,10 @@ namespace UnitTest Data::Asset materialAsset = materialAssetOutcome.GetValue(); - // The order here is based on the order in the MaterialTypeSourceData, as added to the the MaterialTypeAssetCreator. + EXPECT_TRUE(materialAsset->IsFinalized()); + EXPECT_EQ(0, materialAsset->GetRawPropertyValues().size()); // A pre-baked material has no need for the original raw property names and values + + // The order here is based on the order in the MaterialTypeSourceData, as added to the MaterialTypeAssetCreator. EXPECT_EQ(materialAsset->GetPropertyValues()[0].GetValue(), true); EXPECT_EQ(materialAsset->GetPropertyValues()[1].GetValue(), -10); EXPECT_EQ(materialAsset->GetPropertyValues()[2].GetValue(), 25u); @@ -192,6 +197,105 @@ namespace UnitTest EXPECT_EQ(materialAsset->GetPropertyValues()[8].GetValue>(), m_testImageAsset); EXPECT_EQ(materialAsset->GetPropertyValues()[9].GetValue(), 1u); } + + TEST_F(MaterialSourceDataTests, CreateMaterialAsset_DeferredBake) + { + // This test is similar to CreateMaterialAsset_BasicProperties but uses MaterialAssetProcessingMode::DeferredBake instead of PreBake. + + Data::AssetId materialTypeAssetId = Uuid::CreateRandom(); + + // This material type asset will be known by the asset system (stub) but doesn't exist in the AssetManager. + // This demonstrates that the CreateMaterialAsset does not attempt to access the MaterialTypeAsset data in MaterialAssetProcessingMode::DeferredBake. + m_assetSystemStub.RegisterSourceInfo("testDeferredBake.materialtype", materialTypeAssetId); + + MaterialSourceData sourceData; + + sourceData.m_materialType = "testDeferredBake.materialtype"; + AddPropertyGroup(sourceData, "general"); + AddProperty(sourceData, "general", "MyBool" , true); + AddProperty(sourceData, "general", "MyInt" , -10); + AddProperty(sourceData, "general", "MyUInt" , 25u); + AddProperty(sourceData, "general", "MyFloat" , 1.5f); + AddProperty(sourceData, "general", "MyColor" , AZ::Color{0.1f, 0.2f, 0.3f, 0.4f}); + AddProperty(sourceData, "general", "MyFloat2", AZ::Vector2(2.1f, 2.2f)); + AddProperty(sourceData, "general", "MyFloat3", AZ::Vector3(3.1f, 3.2f, 3.3f)); + AddProperty(sourceData, "general", "MyFloat4", AZ::Vector4(4.1f, 4.2f, 4.3f, 4.4f)); + AddProperty(sourceData, "general", "MyImage" , AZStd::string("@exefolder@/Temp/test.streamingimage")); + AddProperty(sourceData, "general", "MyEnum" , AZStd::string("Enum1")); + + auto materialAssetOutcome = sourceData.CreateMaterialAsset(Uuid::CreateRandom(), "", MaterialAssetProcessingMode::DeferredBake, true); + EXPECT_TRUE(materialAssetOutcome.IsSuccess()); + + Data::Asset materialAsset = materialAssetOutcome.GetValue(); + + EXPECT_FALSE(materialAsset->IsFinalized()); + // Note we avoid calling GetPropertyValues() because that will auto-finalize the material. We want to check its raw property values first. + + auto findRawPropertyValue = [materialAsset](const char* propertyId) + { + auto iter = AZStd::find_if(materialAsset->GetRawPropertyValues().begin(), materialAsset->GetRawPropertyValues().end(), [propertyId](const AZStd::pair& pair) + { + return pair.first == AZ::Name{propertyId}; + }); + + if (iter == materialAsset->GetRawPropertyValues().end()) + { + return MaterialPropertyValue{}; + } + else + { + return iter->second; + } + }; + + auto checkRawPropertyValues = [findRawPropertyValue, this]() + { + EXPECT_EQ(findRawPropertyValue("general.MyBool" ).GetValue(), true); + EXPECT_EQ(findRawPropertyValue("general.MyInt" ).GetValue(), -10); + EXPECT_EQ(findRawPropertyValue("general.MyUInt" ).GetValue(), 25u); + EXPECT_EQ(findRawPropertyValue("general.MyFloat" ).GetValue(), 1.5f); + EXPECT_EQ(findRawPropertyValue("general.MyFloat2").GetValue(), Vector2(2.1f, 2.2f)); + EXPECT_EQ(findRawPropertyValue("general.MyFloat3").GetValue(), Vector3(3.1f, 3.2f, 3.3f)); + EXPECT_EQ(findRawPropertyValue("general.MyFloat4").GetValue(), Vector4(4.1f, 4.2f, 4.3f, 4.4f)); + EXPECT_EQ(findRawPropertyValue("general.MyColor" ).GetValue(), Color(0.1f, 0.2f, 0.3f, 0.4f)); + EXPECT_EQ(findRawPropertyValue("general.MyImage" ).GetValue>(), m_testImageAsset); + // The raw value for an enum is the original string, not the numerical value, because the material type holds the necessary metadata to match the name to the value. + EXPECT_EQ(findRawPropertyValue("general.MyEnum" ).GetValue(), AZStd::string("Enum1")); + }; + + // We check the raw property values before the material type asset is even available + checkRawPropertyValues(); + + // Now we'll create the material type asset in memory so the material will have what it needs to finalize itself. + Data::Asset testMaterialTypeAsset = CreateTestMaterialTypeAsset(materialTypeAssetId); + + // The MaterialAsset is still holding an reference to an unloaded asset, so we run it through the serializer which causes the loaded MaterialAsset + // to have access to the testMaterialTypeAsset. This is similar to how the AP would save the MaterialAsset to the cache and the runtime would load it. + SerializeTester tester(GetSerializeContext()); + tester.SerializeOut(materialAsset.Get()); + materialAsset = tester.SerializeIn(Uuid::CreateRandom(), ObjectStream::FilterDescriptor{AZ::Data::AssetFilterNoAssetLoading}); + + // We check the raw property values again on the loaded data, showing that the same data is available in the original un-finalized state. + checkRawPropertyValues(); + + // The material will automatically finalize itself when the properties are accessed. + EXPECT_FALSE(materialAsset->IsFinalized()); + materialAsset->GetPropertyValues(); + EXPECT_TRUE(materialAsset->IsFinalized()); + + // Now all the property values should be available through the main GetPropertyValues() API. + EXPECT_EQ(materialAsset->GetPropertyValues()[0].GetValue(), true); + EXPECT_EQ(materialAsset->GetPropertyValues()[1].GetValue(), -10); + EXPECT_EQ(materialAsset->GetPropertyValues()[2].GetValue(), 25u); + EXPECT_EQ(materialAsset->GetPropertyValues()[3].GetValue(), 1.5f); + EXPECT_EQ(materialAsset->GetPropertyValues()[4].GetValue(), Vector2(2.1f, 2.2f)); + EXPECT_EQ(materialAsset->GetPropertyValues()[5].GetValue(), Vector3(3.1f, 3.2f, 3.3f)); + EXPECT_EQ(materialAsset->GetPropertyValues()[6].GetValue(), Vector4(4.1f, 4.2f, 4.3f, 4.4f)); + EXPECT_EQ(materialAsset->GetPropertyValues()[7].GetValue(), Color(0.1f, 0.2f, 0.3f, 0.4f)); + EXPECT_EQ(materialAsset->GetPropertyValues()[8].GetValue>(), m_testImageAsset); + EXPECT_EQ(materialAsset->GetPropertyValues()[9].GetValue(), 1u); + + } void CheckEqual(MaterialSourceData& a, MaterialSourceData& b) { @@ -546,16 +650,19 @@ namespace UnitTest auto materialAssetLevel1 = sourceDataLevel1.CreateMaterialAsset(Uuid::CreateRandom(), "", MaterialAssetProcessingMode::PreBake, true); EXPECT_TRUE(materialAssetLevel1.IsSuccess()); + EXPECT_TRUE(materialAssetLevel1.GetValue()->IsFinalized()); m_assetSystemStub.RegisterSourceInfo("level1.material", materialAssetLevel1.GetValue().GetId()); auto materialAssetLevel2 = sourceDataLevel2.CreateMaterialAsset(Uuid::CreateRandom(), "", MaterialAssetProcessingMode::PreBake, true); EXPECT_TRUE(materialAssetLevel2.IsSuccess()); + EXPECT_TRUE(materialAssetLevel2.GetValue()->IsFinalized()); m_assetSystemStub.RegisterSourceInfo("level2.material", materialAssetLevel2.GetValue().GetId()); auto materialAssetLevel3 = sourceDataLevel3.CreateMaterialAsset(Uuid::CreateRandom(), "", MaterialAssetProcessingMode::PreBake, true); EXPECT_TRUE(materialAssetLevel3.IsSuccess()); + EXPECT_TRUE(materialAssetLevel3.GetValue()->IsFinalized()); auto layout = m_testMaterialTypeAsset->GetMaterialPropertiesLayout(); MaterialPropertyIndex myFloat = layout->FindPropertyIndex(Name("general.MyFloat")); @@ -582,6 +689,101 @@ namespace UnitTest EXPECT_EQ(properties[myFloat2.GetIndex()].GetValue(), Vector2(4.1f, 4.2f)); EXPECT_EQ(properties[myColor.GetIndex()].GetValue(), Color(0.15f, 0.25f, 0.35f, 0.45f)); } + + TEST_F(MaterialSourceDataTests, CreateMaterialAsset_MultiLevelDataInheritance_DeferredBake) + { + // This test is similar to CreateMaterialAsset_MultiLevelDataInheritance but uses MaterialAssetProcessingMode::DeferredBake instead of PreBake. + + Data::AssetId materialTypeAssetId = Uuid::CreateRandom(); + + // This material type asset will be known by the asset system (stub) but doesn't exist in the AssetManager. + // This demonstrates that the CreateMaterialAsset does not attempt to access the MaterialTypeAsset data in MaterialAssetProcessingMode::DeferredBake. + m_assetSystemStub.RegisterSourceInfo("testDeferredBake.materialtype", materialTypeAssetId); + + MaterialSourceData sourceDataLevel1; + sourceDataLevel1.m_materialType = "testDeferredBake.materialtype"; + AddPropertyGroup(sourceDataLevel1, "general"); + AddProperty(sourceDataLevel1, "general", "MyFloat", 1.5f); + AddProperty(sourceDataLevel1, "general", "MyColor", AZ::Color{0.1f, 0.2f, 0.3f, 0.4f}); + + MaterialSourceData sourceDataLevel2; + sourceDataLevel2.m_materialType = "testDeferredBake.materialtype"; + sourceDataLevel2.m_parentMaterial = "level1.material"; + AddPropertyGroup(sourceDataLevel2, "general"); + AddProperty(sourceDataLevel2, "general", "MyColor", AZ::Color{0.15f, 0.25f, 0.35f, 0.45f}); + AddProperty(sourceDataLevel2, "general", "MyFloat2", AZ::Vector2{4.1f, 4.2f}); + + MaterialSourceData sourceDataLevel3; + sourceDataLevel3.m_materialType = "testDeferredBake.materialtype"; + sourceDataLevel3.m_parentMaterial = "level2.material"; + AddPropertyGroup(sourceDataLevel3, "general"); + AddProperty(sourceDataLevel3, "general", "MyFloat", 3.5f); + + auto materialAssetLevel1Result = sourceDataLevel1.CreateMaterialAsset(Uuid::CreateRandom(), "", MaterialAssetProcessingMode::DeferredBake, true); + EXPECT_TRUE(materialAssetLevel1Result.IsSuccess()); + Data::Asset materialAssetLevel1 = materialAssetLevel1Result.TakeValue(); + EXPECT_FALSE(materialAssetLevel1->IsFinalized()); + + m_assetSystemStub.RegisterSourceInfo("level1.material", materialAssetLevel1.GetId()); + + auto materialAssetLevel2Result = sourceDataLevel2.CreateMaterialAsset(Uuid::CreateRandom(), "", MaterialAssetProcessingMode::DeferredBake, true); + EXPECT_TRUE(materialAssetLevel2Result.IsSuccess()); + Data::Asset materialAssetLevel2 = materialAssetLevel2Result.TakeValue(); + EXPECT_FALSE(materialAssetLevel2->IsFinalized()); + + m_assetSystemStub.RegisterSourceInfo("level2.material", materialAssetLevel2.GetId()); + + auto materialAssetLevel3Result = sourceDataLevel3.CreateMaterialAsset(Uuid::CreateRandom(), "", MaterialAssetProcessingMode::DeferredBake, true); + EXPECT_TRUE(materialAssetLevel3Result.IsSuccess()); + Data::Asset materialAssetLevel3 = materialAssetLevel3Result.TakeValue(); + EXPECT_FALSE(materialAssetLevel3->IsFinalized()); + + // Now we'll create the material type asset in memory so the materials will have what they need to finalize. + Data::Asset testMaterialTypeAsset = CreateTestMaterialTypeAsset(materialTypeAssetId); + + auto layout = testMaterialTypeAsset->GetMaterialPropertiesLayout(); + MaterialPropertyIndex myFloat = layout->FindPropertyIndex(Name("general.MyFloat")); + MaterialPropertyIndex myFloat2 = layout->FindPropertyIndex(Name("general.MyFloat2")); + MaterialPropertyIndex myColor = layout->FindPropertyIndex(Name("general.MyColor")); + + + // The MaterialAsset is still holding an reference to an unloaded asset, so we run it through the serializer which causes the loaded MaterialAsset + // to have access to the testMaterialTypeAsset. This is similar to how the AP would save the MaterialAsset to the cache and the runtime would load it. + SerializeTester tester(GetSerializeContext()); + tester.SerializeOut(materialAssetLevel1.Get()); + materialAssetLevel1 = tester.SerializeIn(Uuid::CreateRandom(), ObjectStream::FilterDescriptor{AZ::Data::AssetFilterNoAssetLoading}); + tester.SerializeOut(materialAssetLevel2.Get()); + materialAssetLevel2 = tester.SerializeIn(Uuid::CreateRandom(), ObjectStream::FilterDescriptor{AZ::Data::AssetFilterNoAssetLoading}); + tester.SerializeOut(materialAssetLevel3.Get()); + materialAssetLevel3 = tester.SerializeIn(Uuid::CreateRandom(), ObjectStream::FilterDescriptor{AZ::Data::AssetFilterNoAssetLoading}); + + + // The properties will finalize automatically when we call GetPropertyValues()... + + AZStd::array_view properties; + + // Check level 1 properties + properties = materialAssetLevel1->GetPropertyValues(); + EXPECT_EQ(properties[myFloat.GetIndex()].GetValue(), 1.5f); + EXPECT_EQ(properties[myFloat2.GetIndex()].GetValue(), Vector2(0.0f, 0.0f)); + EXPECT_EQ(properties[myColor.GetIndex()].GetValue(), Color(0.1f, 0.2f, 0.3f, 0.4f)); + + // Check level 2 properties + properties = materialAssetLevel2->GetPropertyValues(); + EXPECT_EQ(properties[myFloat.GetIndex()].GetValue(), 1.5f); + EXPECT_EQ(properties[myFloat2.GetIndex()].GetValue(), Vector2(4.1f, 4.2f)); + EXPECT_EQ(properties[myColor.GetIndex()].GetValue(), Color(0.15f, 0.25f, 0.35f, 0.45f)); + + // Check level 3 properties + properties = materialAssetLevel3->GetPropertyValues(); + EXPECT_EQ(properties[myFloat.GetIndex()].GetValue(), 3.5f); + EXPECT_EQ(properties[myFloat2.GetIndex()].GetValue(), Vector2(4.1f, 4.2f)); + EXPECT_EQ(properties[myColor.GetIndex()].GetValue(), Color(0.15f, 0.25f, 0.35f, 0.45f)); + + EXPECT_TRUE(materialAssetLevel1->IsFinalized()); + EXPECT_TRUE(materialAssetLevel2->IsFinalized()); + EXPECT_TRUE(materialAssetLevel3->IsFinalized()); + } TEST_F(MaterialSourceDataTests, CreateMaterialAsset_MultiLevelDataInheritance_Error_MaterialTypesDontMatch) { From 4ade6bc88a2432a0073c2edf521d6c8ab5cf0716 Mon Sep 17 00:00:00 2001 From: santorac <55155825+santorac@users.noreply.github.com> Date: Tue, 28 Dec 2021 17:52:13 -0800 Subject: [PATCH 04/10] Fixed compile errors in Material Editor. Signed-off-by: santorac <55155825+santorac@users.noreply.github.com> --- .../MaterialEditor/Code/Source/Document/MaterialDocument.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Gems/Atom/Tools/MaterialEditor/Code/Source/Document/MaterialDocument.cpp b/Gems/Atom/Tools/MaterialEditor/Code/Source/Document/MaterialDocument.cpp index 93da118b5b..641d586ea7 100644 --- a/Gems/Atom/Tools/MaterialEditor/Code/Source/Document/MaterialDocument.cpp +++ b/Gems/Atom/Tools/MaterialEditor/Code/Source/Document/MaterialDocument.cpp @@ -713,7 +713,7 @@ namespace MaterialEditor // Long term, the material document should not be concerned with assets at all. The viewport window should be the // only thing concerned with assets or instances. auto materialAssetResult = - m_materialSourceData.CreateMaterialAssetFromSourceData(Uuid::CreateRandom(), m_absolutePath, elevateWarnings, true, &m_sourceDependencies); + m_materialSourceData.CreateMaterialAssetFromSourceData(Uuid::CreateRandom(), m_absolutePath, elevateWarnings, &m_sourceDependencies); if (!materialAssetResult) { AZ_Error("MaterialDocument", false, "Material asset could not be created from source data: '%s'.", m_absolutePath.c_str()); @@ -753,7 +753,7 @@ namespace MaterialEditor } auto parentMaterialAssetResult = parentMaterialSourceData.CreateMaterialAssetFromSourceData( - parentMaterialAssetIdResult.GetValue(), m_materialSourceData.m_parentMaterial, true, true); + parentMaterialAssetIdResult.GetValue(), m_materialSourceData.m_parentMaterial, true); if (!parentMaterialAssetResult) { AZ_Error("MaterialDocument", false, "Material parent asset could not be created from source data: '%s'.", m_materialSourceData.m_parentMaterial.c_str()); From aafd34679af77484c949744964de6eb3fc6a4fb5 Mon Sep 17 00:00:00 2001 From: santorac <55155825+santorac@users.noreply.github.com> Date: Thu, 13 Jan 2022 12:48:32 -0800 Subject: [PATCH 05/10] Merged MaterialAssetCreatorCommon class into MaterialTypeAssetCreator because it is no longer needed for MaterialAssetCreator. Signed-off-by: santorac <55155825+santorac@users.noreply.github.com> --- .../Material/MaterialAssetCreatorCommon.h | 64 ------------- .../RPI.Reflect/Material/MaterialTypeAsset.h | 1 - .../Material/MaterialTypeAssetCreator.h | 14 ++- .../RPI.Builders/Material/MaterialBuilder.cpp | 6 +- .../Model/MaterialAssetBuilderComponent.cpp | 2 +- .../Material/MaterialAssetCreatorCommon.cpp | 93 ------------------- .../Material/MaterialTypeAssetCreator.cpp | 61 +++++++++--- .../RPI/Code/atom_rpi_reflect_files.cmake | 2 - 8 files changed, 63 insertions(+), 180 deletions(-) delete mode 100644 Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialAssetCreatorCommon.h delete mode 100644 Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialAssetCreatorCommon.cpp diff --git a/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialAssetCreatorCommon.h b/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialAssetCreatorCommon.h deleted file mode 100644 index c7fa9c2a4c..0000000000 --- a/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialAssetCreatorCommon.h +++ /dev/null @@ -1,64 +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 -#include - -// These classes are not directly referenced in this header only because the SetPropertyValue() -// function is templatized. But the API is still specific to these data types so we include them here. -#include -#include -#include -#include - -namespace AZ -{ - namespace RPI - { - class StreamingImageAsset; - class AttachmentImageAsset; - - //! Provides common functionality to both MaterialTypeAssetCreator and MaterialAssetCreator. - class MaterialAssetCreatorCommon - { - public: - void SetPropertyValue(const Name& name, const Data::Asset& imageAsset); - void SetPropertyValue(const Name& name, const Data::Asset& imageAsset); - void SetPropertyValue(const Name& name, const Data::Asset& imageAsset); - - //! Sets a property value using data in AZStd::variant-based MaterialPropertyValue. The contained data must match - //! the data type of the property. For type Image, the value must be a Data::Asset. - void SetPropertyValue(const Name& name, const MaterialPropertyValue& value); - - protected: - MaterialAssetCreatorCommon() = default; - - void OnBegin( - const MaterialPropertiesLayout* propertyLayout, - AZStd::vector* propertyValues, - const AZStd::function& warningFunc, - const AZStd::function& errorFunc); - void OnEnd(); - - private: - bool PropertyCheck(TypeId typeId, const Name& name); - - const MaterialPropertiesLayout* m_propertyLayout = nullptr; - //! Points to the m_propertyValues list in a MaterialAsset or MaterialTypeAsset - AZStd::vector* m_propertyValues = nullptr; - - AZStd::function m_reportWarning = nullptr; - AZStd::function m_reportError = nullptr; - }; - - } // namespace RPI -} // namespace AZ diff --git a/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialTypeAsset.h b/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialTypeAsset.h index f194d84263..9065b17254 100644 --- a/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialTypeAsset.h +++ b/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialTypeAsset.h @@ -54,7 +54,6 @@ namespace AZ { friend class MaterialTypeAssetCreator; friend class MaterialTypeAssetHandler; - friend class MaterialAssetCreatorCommon; public: AZ_RTTI(MaterialTypeAsset, "{CD7803AB-9C4C-4A33-9A14-7412F1665464}", AZ::Data::AssetData); diff --git a/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialTypeAssetCreator.h b/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialTypeAssetCreator.h index 5e5f94da6d..6bd84b5546 100644 --- a/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialTypeAssetCreator.h +++ b/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialTypeAssetCreator.h @@ -8,7 +8,6 @@ #pragma once #include -#include #include #include @@ -27,7 +26,6 @@ namespace AZ //! which provides the MaterialTypeAsset and default property values. class MaterialTypeAssetCreator : public AssetCreator - , public MaterialAssetCreatorCommon { public: //! Begin creating a MaterialTypeAsset @@ -71,6 +69,14 @@ namespace AZ //! Finishes creating a material property. void EndMaterialProperty(); + + void SetPropertyValue(const Name& name, const Data::Asset& imageAsset); + void SetPropertyValue(const Name& name, const Data::Asset& imageAsset); + void SetPropertyValue(const Name& name, const Data::Asset& imageAsset); + + //! Sets a property value using data in AZStd::variant-based MaterialPropertyValue. The contained data must match + //! the data type of the property. For type Image, the value must be a Data::Asset. + void SetPropertyValue(const Name& name, const MaterialPropertyValue& value); //! Adds a MaterialFunctor. //! Material functors provide custom logic and calculations to configure shaders, render states, and more.See MaterialFunctor.h for details. @@ -101,7 +107,9 @@ namespace AZ private: void AddMaterialProperty(MaterialPropertyDescriptor&& materialProperty); - + + bool PropertyCheck(TypeId typeId, const Name& name); + //! The material type holds references to shader assets that contain SRGs that are supposed to be the same across all passes in the material. //! This function searches for an SRG given a @bindingSlot. If a valid one is found it makes sure it is the same across all shaders //! and records in srgShaderIndexToUpdate the index of the ShaderAsset in the ShaderCollection where it was found. 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 3aa8728e08..6e50e58dd3 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Builders/Material/MaterialBuilder.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Builders/Material/MaterialBuilder.cpp @@ -52,7 +52,7 @@ namespace AZ { AssetBuilderSDK::AssetBuilderDesc materialBuilderDescriptor; materialBuilderDescriptor.m_name = JobKey; - materialBuilderDescriptor.m_version = 112; // material dependency improvements + materialBuilderDescriptor.m_version = 113; // material dependency improvements 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(); @@ -95,7 +95,7 @@ namespace AZ const bool currentFileIsMaterial = AzFramework::StringFunc::Path::IsExtension(currentFilePath.c_str(), MaterialSourceData::Extension); const bool referencedFileIsMaterialType = AzFramework::StringFunc::Path::IsExtension(referencedParentPath.c_str(), MaterialTypeSourceData::Extension); - const bool ShouldFinalizeMaterialAssets = MaterialUtils::BuildersShouldFinalizeMaterialAssets(); + const bool shouldFinalizeMaterialAssets = MaterialUtils::BuildersShouldFinalizeMaterialAssets(); AZStd::vector possibleDependencies = RPI::AssetUtils::GetPossibleDepenencyPaths(currentFilePath, referencedParentPath); for (auto& file : possibleDependencies) @@ -118,7 +118,7 @@ namespace AZ // If we aren't finalizing material assets, then a normal job dependency isn't needed because the MaterialTypeAsset data won't be used. // However, we do still need at least an OrderOnce dependency to ensure the Asset Processor knows about the material type asset so the builder can get it's AssetId. // This can significantly reduce AP processing time when a material type or its shaders are edited. - if (currentFileIsMaterial && referencedFileIsMaterialType && !ShouldFinalizeMaterialAssets) + if (currentFileIsMaterial && referencedFileIsMaterialType && !shouldFinalizeMaterialAssets) { jobDependency.m_type = AssetBuilderSDK::JobDependencyType::OrderOnce; } diff --git a/Gems/Atom/RPI/Code/Source/RPI.Builders/Model/MaterialAssetBuilderComponent.cpp b/Gems/Atom/RPI/Code/Source/RPI.Builders/Model/MaterialAssetBuilderComponent.cpp index 6f71fb70d4..1cc1925564 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Builders/Model/MaterialAssetBuilderComponent.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Builders/Model/MaterialAssetBuilderComponent.cpp @@ -127,7 +127,7 @@ namespace AZ if (auto* serialize = azrtti_cast(context)) { serialize->Class() - ->Version(18); // material dependency improvements + ->Version(19); // material dependency improvements } } diff --git a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialAssetCreatorCommon.cpp b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialAssetCreatorCommon.cpp deleted file mode 100644 index 4b2f163f7c..0000000000 --- a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialAssetCreatorCommon.cpp +++ /dev/null @@ -1,93 +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 -#include - -namespace AZ -{ - namespace RPI - { - void MaterialAssetCreatorCommon::OnBegin( - const MaterialPropertiesLayout* propertyLayout, - AZStd::vector* propertyValues, - const AZStd::function& warningFunc, - const AZStd::function& errorFunc) - { - m_propertyLayout = propertyLayout; - m_propertyValues = propertyValues; - m_reportWarning = warningFunc; - m_reportError = errorFunc; - } - - void MaterialAssetCreatorCommon::OnEnd() - { - m_propertyLayout = nullptr; - m_propertyValues = nullptr; - m_reportWarning = nullptr; - m_reportError = nullptr; - } - - bool MaterialAssetCreatorCommon::PropertyCheck(TypeId typeId, const Name& name) - { - if (!m_reportWarning || !m_reportError) - { - AZ_Assert(false, "Call Begin() on the AssetCreator before using it."); - return false; - } - - MaterialPropertyIndex propertyIndex = m_propertyLayout->FindPropertyIndex(name); - if (!propertyIndex.IsValid()) - { - m_reportWarning( - AZStd::string::format("Material property '%s' not found", - name.GetCStr() - ).data()); - return false; - } - - const MaterialPropertyDescriptor* materialPropertyDescriptor = m_propertyLayout->GetPropertyDescriptor(propertyIndex); - if (!materialPropertyDescriptor) - { - m_reportError("A material property index was found but the property descriptor was null"); - return false; - } - - if (!ValidateMaterialPropertyDataType(typeId, name, materialPropertyDescriptor, m_reportError)) - { - return false; - } - - return true; - } - - void MaterialAssetCreatorCommon::SetPropertyValue(const Name& name, const Data::Asset& imageAsset) - { - return SetPropertyValue(name, MaterialPropertyValue(imageAsset)); - } - - void MaterialAssetCreatorCommon::SetPropertyValue(const Name& name, const MaterialPropertyValue& value) - { - if (PropertyCheck(value.GetTypeId(), name)) - { - MaterialPropertyIndex propertyIndex = m_propertyLayout->FindPropertyIndex(name); - (*m_propertyValues)[propertyIndex.GetIndex()] = value; - } - } - - void MaterialAssetCreatorCommon::SetPropertyValue(const Name& name, const Data::Asset& imageAsset) - { - SetPropertyValue(name, Data::Asset(imageAsset)); - } - - void MaterialAssetCreatorCommon::SetPropertyValue(const Name& name, const Data::Asset& imageAsset) - { - SetPropertyValue(name, Data::Asset(imageAsset)); - } - } // namespace RPI -} // namespace AZ diff --git a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialTypeAssetCreator.cpp b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialTypeAssetCreator.cpp index dd023c56b8..6746b57740 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialTypeAssetCreator.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialTypeAssetCreator.cpp @@ -22,17 +22,6 @@ namespace AZ { m_materialPropertiesLayout = aznew MaterialPropertiesLayout; m_asset->m_materialPropertiesLayout = m_materialPropertiesLayout; - - auto warningFunc = [this](const char* message) - { - ReportWarning("%s", message); - }; - auto errorFunc = [this](const char* message) - { - ReportError("%s", message); - }; - // Set empty for UV names as material type asset doesn't have overrides. - MaterialAssetCreatorCommon::OnBegin(m_materialPropertiesLayout, &(m_asset->m_propertyValues), warningFunc, errorFunc); } } @@ -48,8 +37,6 @@ namespace AZ m_materialShaderResourceGroupLayout = nullptr; m_materialPropertiesLayout = nullptr; - MaterialAssetCreatorCommon::OnEnd(); - return EndCommon(result); } @@ -499,6 +486,54 @@ namespace AZ m_wipMaterialProperty = MaterialPropertyDescriptor{}; } + + bool MaterialTypeAssetCreator::PropertyCheck(TypeId typeId, const Name& name) + { + MaterialPropertyIndex propertyIndex = m_materialPropertiesLayout->FindPropertyIndex(name); + if (!propertyIndex.IsValid()) + { + ReportWarning("Material property '%s' not found", name.GetCStr()); + return false; + } + + const MaterialPropertyDescriptor* materialPropertyDescriptor = m_materialPropertiesLayout->GetPropertyDescriptor(propertyIndex); + if (!materialPropertyDescriptor) + { + ReportError("A material property index was found but the property descriptor was null"); + return false; + } + + if (!ValidateMaterialPropertyDataType(typeId, name, materialPropertyDescriptor, [this](const char* message){ReportError("%s", message);})) + { + return false; + } + + return true; + } + + void MaterialTypeAssetCreator::SetPropertyValue(const Name& name, const Data::Asset& imageAsset) + { + return SetPropertyValue(name, MaterialPropertyValue(imageAsset)); + } + + void MaterialTypeAssetCreator::SetPropertyValue(const Name& name, const MaterialPropertyValue& value) + { + if (PropertyCheck(value.GetTypeId(), name)) + { + MaterialPropertyIndex propertyIndex = m_materialPropertiesLayout->FindPropertyIndex(name); + m_asset->m_propertyValues[propertyIndex.GetIndex()] = value; + } + } + + void MaterialTypeAssetCreator::SetPropertyValue(const Name& name, const Data::Asset& imageAsset) + { + SetPropertyValue(name, Data::Asset(imageAsset)); + } + + void MaterialTypeAssetCreator::SetPropertyValue(const Name& name, const Data::Asset& imageAsset) + { + SetPropertyValue(name, Data::Asset(imageAsset)); + } void MaterialTypeAssetCreator::AddMaterialFunctor(const Ptr& functor) { diff --git a/Gems/Atom/RPI/Code/atom_rpi_reflect_files.cmake b/Gems/Atom/RPI/Code/atom_rpi_reflect_files.cmake index df8f389c37..6e9cdfeb4e 100644 --- a/Gems/Atom/RPI/Code/atom_rpi_reflect_files.cmake +++ b/Gems/Atom/RPI/Code/atom_rpi_reflect_files.cmake @@ -51,7 +51,6 @@ set(FILES Include/Atom/RPI.Reflect/Image/StreamingImagePoolAssetCreator.h Include/Atom/RPI.Reflect/Material/LuaMaterialFunctor.h Include/Atom/RPI.Reflect/Material/MaterialAsset.h - Include/Atom/RPI.Reflect/Material/MaterialAssetCreatorCommon.h Include/Atom/RPI.Reflect/Material/MaterialAssetCreator.h Include/Atom/RPI.Reflect/Material/MaterialDynamicMetadata.h Include/Atom/RPI.Reflect/Material/MaterialPropertyDescriptor.h @@ -133,7 +132,6 @@ set(FILES Source/RPI.Reflect/Image/StreamingImagePoolAssetCreator.cpp Source/RPI.Reflect/Material/MaterialPropertyValue.cpp Source/RPI.Reflect/Material/MaterialAsset.cpp - Source/RPI.Reflect/Material/MaterialAssetCreatorCommon.cpp Source/RPI.Reflect/Material/MaterialAssetCreator.cpp Source/RPI.Reflect/Material/LuaMaterialFunctor.cpp Source/RPI.Reflect/Material/MaterialDynamicMetadata.cpp From 8084775d7adf384f96672e69fa227c81156e9262 Mon Sep 17 00:00:00 2001 From: santorac <55155825+santorac@users.noreply.github.com> Date: Thu, 13 Jan 2022 12:49:35 -0800 Subject: [PATCH 06/10] Updating code comments. Signed-off-by: santorac <55155825+santorac@users.noreply.github.com> --- .../Atom/RPI.Reflect/Material/MaterialAsset.h | 13 ++++++++++--- .../Source/RPI.Edit/Material/MaterialSourceData.cpp | 3 +++ .../Source/RPI.Reflect/Material/MaterialAsset.cpp | 2 +- 3 files changed, 14 insertions(+), 4 deletions(-) diff --git a/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialAsset.h b/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialAsset.h index 4cc6608225..2a6c89a8fa 100644 --- a/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialAsset.h +++ b/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialAsset.h @@ -34,8 +34,6 @@ namespace AZ class MaterialAssetHandler; //! MaterialAsset defines a single material, which can be used to create a Material instance for rendering at runtime. - //! It fetches MaterialTypeSourceData from the MaterialTypeAsset it owned. - //! //! Use a MaterialAssetCreator to create a MaterialAsset. class MaterialAsset : public AZ::Data::AssetData @@ -46,7 +44,6 @@ namespace AZ friend class MaterialVersionUpdate; friend class MaterialAssetCreator; friend class MaterialAssetHandler; - friend class MaterialAssetCreatorCommon; friend class UnitTest::MaterialTests; friend class UnitTest::MaterialAssetTests; @@ -117,8 +114,18 @@ namespace AZ //! //! Note that even though material source data files contain only override values and inherit the rest from //! their parent material, they all get flattened at build time so every MaterialAsset has the full set of values. + //! + //! Calling GetPropertyValues() will automatically finalize the material asset if it isn't finalized already. The + //! MaterialTypeAsset must be loaded and ready. const AZStd::vector& GetPropertyValues() const; + //! Returns the list of raw values for all properties in this material, as listed in the source .material file(s), before the material asset was Finalized. + //! + //! The MaterialAsset can be created in a "half-baked" state (see MaterialUtils::BuildersShouldFinalizeMaterialAssets) where + //! minimal processing has been done because it did not yet have access to the MaterialTypeAsset. In that case, the list will + //! be populated with values copied from the source .material file with little or no validation or other processing. It includes + //! all parent .material files, with properties listed in low-to-high priority order. + //! This list will be empty however if the asset was finalized at build-time. const AZStd::vector>& GetRawPropertyValues() const; private: 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 23657f4871..bc764ec7fb 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialSourceData.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Edit/Material/MaterialSourceData.cpp @@ -312,6 +312,9 @@ namespace AZ { materialAssetCreator.ReportWarning("Source data for material property value is invalid."); } + // If the source value type is a string, there are two possible property types: Image and Enum. If there is a "." in + // the string (for the extension) we assume it's an Image and look up the referenced Asset. Otherwise, we can assume + // it's an Enum value and just preserve the original string. else if (property.second.m_value.Is() && AzFramework::StringFunc::Contains(property.second.m_value.GetValue(), ".")) { Data::Asset imageAsset; diff --git a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialAsset.cpp b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialAsset.cpp index ce5847d12f..a40fa77019 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialAsset.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialAsset.cpp @@ -304,7 +304,7 @@ namespace AZ m_materialTypeAsset = newMaterialTypeAsset; // If the material asset was not finalized on disk, then we clear the previously finalized property values to force re-finalize. - // This + // This is necessary in case the property layout changed in some way. if (!m_wasPreFinalized) { m_isFinalized = false; From 2d6d14abf72d3db6fba0ab225e83bc23fb7e34ba Mon Sep 17 00:00:00 2001 From: santorac <55155825+santorac@users.noreply.github.com> Date: Fri, 14 Jan 2022 10:49:17 -0800 Subject: [PATCH 07/10] Changed MaterialAsset::GetPropertyValues to not auto-finalize. Client code must call Finalize manually. It's better to avoid unexpected side-effects from a const getter function. In some cases it may be acceptable to do non-const things in a const function as long as it is only manipulating internal data, and the public facing API returns the same values as before. But in this case, the IsFinalized function is a public facing API that would have a different result after GetPropertyValues was called. I also updated a couple other minor things from code review feedback. Signed-off-by: santorac <55155825+santorac@users.noreply.github.com> --- .../RPI.Edit/Material/MaterialSourceData.h | 2 +- .../Atom/RPI.Reflect/Material/MaterialAsset.h | 11 ++++---- .../RPI.Builders/Material/MaterialBuilder.cpp | 2 +- .../Model/MaterialAssetBuilderComponent.cpp | 5 ++-- .../Source/RPI.Public/Material/Material.cpp | 2 ++ .../RPI.Reflect/Material/MaterialAsset.cpp | 5 +--- .../Tests/Material/MaterialAssetTests.cpp | 6 ++-- .../Material/MaterialSourceDataTests.cpp | 28 +++++++++++-------- 8 files changed, 34 insertions(+), 27 deletions(-) diff --git a/Gems/Atom/RPI/Code/Include/Atom/RPI.Edit/Material/MaterialSourceData.h b/Gems/Atom/RPI/Code/Include/Atom/RPI.Edit/Material/MaterialSourceData.h index a5fb0214e6..0a9371f722 100644 --- a/Gems/Atom/RPI/Code/Include/Atom/RPI.Edit/Material/MaterialSourceData.h +++ b/Gems/Atom/RPI/Code/Include/Atom/RPI.Edit/Material/MaterialSourceData.h @@ -33,7 +33,7 @@ namespace AZ class MaterialAsset; class MaterialAssetCreator; - enum MaterialAssetProcessingMode + enum class MaterialAssetProcessingMode { PreBake, //!< all material asset processing is done in the Asset Processor, producing a finalized material asset DeferredBake //!< some material asset processing is deferred, and the material asset is finalized at runtime after loading diff --git a/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialAsset.h b/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialAsset.h index 2a6c89a8fa..941fcd0fe9 100644 --- a/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialAsset.h +++ b/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialAsset.h @@ -107,6 +107,11 @@ namespace AZ //! If false, property values can be accessed through GetRawPropertyValues(). bool IsFinalized() const; + //! If the material asset is not finalized yet, this does the final processing of the raw property values to + //! get the material asset ready to be used. + //! Note the MaterialTypeAsset must be valid before this is called. + void Finalize(AZStd::function reportWarning = nullptr, AZStd::function reportError = nullptr); + //! Returns the list of values for all properties in this material. //! The entries in this list align with the entries in the MaterialPropertiesLayout. Each AZStd::any is guaranteed //! to have a value of type that matches the corresponding MaterialPropertyDescriptor. @@ -131,12 +136,6 @@ namespace AZ private: bool PostLoadInit() override; - //! If the material asset is not finalized yet, this does the final processing of m_rawPropertyValues to - //! get the material asset ready to be used. - //! Note m_materialTypeAsset must be valid before this is called. - //! @param elevateWarnings Indicates whether to treat warnings as errors - void Finalize(AZStd::function reportWarning = nullptr, AZStd::function reportError = nullptr); - //! Checks the material type version and potentially applies a series of property changes (most common are simple property renames) //! based on the MaterialTypeAsset's version update procedure. void ApplyVersionUpdates(); 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 6e50e58dd3..cedbb1df6e 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Builders/Material/MaterialBuilder.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Builders/Material/MaterialBuilder.cpp @@ -52,7 +52,7 @@ namespace AZ { AssetBuilderSDK::AssetBuilderDesc materialBuilderDescriptor; materialBuilderDescriptor.m_name = JobKey; - materialBuilderDescriptor.m_version = 113; // material dependency improvements + materialBuilderDescriptor.m_version = 114; // material dependency improvements 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(); diff --git a/Gems/Atom/RPI/Code/Source/RPI.Builders/Model/MaterialAssetBuilderComponent.cpp b/Gems/Atom/RPI/Code/Source/RPI.Builders/Model/MaterialAssetBuilderComponent.cpp index 1cc1925564..a44b47ee6c 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Builders/Model/MaterialAssetBuilderComponent.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Builders/Model/MaterialAssetBuilderComponent.cpp @@ -45,7 +45,8 @@ namespace AZ if (auto* serialize = azrtti_cast(context)) { serialize->Class() - ->Version(5) // <<<<< This probably is NOT the version number you want to bump. What you're looking for is MaterialAssetBuilderComponent::Reflect below + ->Version(5) // <<<<< If you have made changes to material code and need to force scene files to be reprocessed, this probably is + // NOT the version number you want to bump . What you're looking for is MaterialAssetBuilderComponent::Reflect below. ->Attribute(Edit::Attributes::SystemComponentTags, AZStd::vector({ AssetBuilderSDK::ComponentTags::AssetBuilder })); } } @@ -127,7 +128,7 @@ namespace AZ if (auto* serialize = azrtti_cast(context)) { serialize->Class() - ->Version(19); // material dependency improvements + ->Version(20); // material dependency improvements } } diff --git a/Gems/Atom/RPI/Code/Source/RPI.Public/Material/Material.cpp b/Gems/Atom/RPI/Code/Source/RPI.Public/Material/Material.cpp index 1f739c24f1..fe9dbef278 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Public/Material/Material.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Public/Material/Material.cpp @@ -62,6 +62,8 @@ namespace AZ m_materialAsset = { &materialAsset, AZ::Data::AssetLoadBehavior::PreLoad }; + m_materialAsset->Finalize(); + // Cache off pointers to some key data structures from the material type... auto srgLayout = m_materialAsset->GetMaterialSrgLayout(); if (srgLayout) diff --git a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialAsset.cpp b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialAsset.cpp index a40fa77019..569e2de81a 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialAsset.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialAsset.cpp @@ -199,10 +199,7 @@ namespace AZ const AZStd::vector& MaterialAsset::GetPropertyValues() const { - // This can't be done in MaterialAssetHandler::LoadAssetData because the MaterialTypeAsset isn't necessarily loaded at that point. - // And it can't be done in PostLoadInit() because that happens on the next frame which might be too late. So we finalize just-in-time - // when properties are accessed. - const_cast(this)->Finalize(); + AZ_Error(s_debugTraceName, IsFinalized(), "MaterialAsset must be finalized before its property values can be accessed"); return m_propertyValues; } diff --git a/Gems/Atom/RPI/Code/Tests/Material/MaterialAssetTests.cpp b/Gems/Atom/RPI/Code/Tests/Material/MaterialAssetTests.cpp index 633953cecc..fdb131d449 100644 --- a/Gems/Atom/RPI/Code/Tests/Material/MaterialAssetTests.cpp +++ b/Gems/Atom/RPI/Code/Tests/Material/MaterialAssetTests.cpp @@ -266,6 +266,10 @@ namespace UnitTest warningFinder.AddExpectedErrorMessage("Automatic updates are available. Consider updating the .material source file"); warningFinder.AddExpectedErrorMessage("This material is based on version '1'"); warningFinder.AddExpectedErrorMessage("material type is now at version '2'"); + + materialAsset->Finalize(); + + warningFinder.CheckExpectedErrorsFound(); // Even though this material was created using the old version of the material type, it's property values should get automatically // updated to align with the new property layout in the latest MaterialTypeAsset. @@ -273,8 +277,6 @@ namespace UnitTest EXPECT_EQ(2, myIntIndex.GetIndex()); EXPECT_EQ(7, materialAsset->GetPropertyValues()[myIntIndex.GetIndex()].GetValue()); - warningFinder.CheckExpectedErrorsFound(); - // Since the MaterialAsset has already been updated, and the warning reported once, we should not see the "consider updating" // warning reported again on subsequent property accesses. warningFinder.Reset(); diff --git a/Gems/Atom/RPI/Code/Tests/Material/MaterialSourceDataTests.cpp b/Gems/Atom/RPI/Code/Tests/Material/MaterialSourceDataTests.cpp index e4d3cc9768..29f0e7d101 100644 --- a/Gems/Atom/RPI/Code/Tests/Material/MaterialSourceDataTests.cpp +++ b/Gems/Atom/RPI/Code/Tests/Material/MaterialSourceDataTests.cpp @@ -228,8 +228,13 @@ namespace UnitTest Data::Asset materialAsset = materialAssetOutcome.GetValue(); + ErrorMessageFinder expectNotFinalizedError("MaterialAsset must be finalized"); + EXPECT_FALSE(materialAsset->IsFinalized()); - // Note we avoid calling GetPropertyValues() because that will auto-finalize the material. We want to check its raw property values first. + + expectNotFinalizedError.ResetCounts(); + EXPECT_TRUE(materialAsset->GetPropertyValues().empty()); + expectNotFinalizedError.CheckExpectedErrorsFound(); auto findRawPropertyValue = [materialAsset](const char* propertyId) { @@ -275,12 +280,14 @@ namespace UnitTest tester.SerializeOut(materialAsset.Get()); materialAsset = tester.SerializeIn(Uuid::CreateRandom(), ObjectStream::FilterDescriptor{AZ::Data::AssetFilterNoAssetLoading}); - // We check the raw property values again on the loaded data, showing that the same data is available in the original un-finalized state. + // We check that everything is still in the original un-finalized state after going through the serialization process. + EXPECT_FALSE(materialAsset->IsFinalized()); checkRawPropertyValues(); + expectNotFinalizedError.ResetCounts(); + EXPECT_TRUE(materialAsset->GetPropertyValues().empty()); + expectNotFinalizedError.CheckExpectedErrorsFound(); - // The material will automatically finalize itself when the properties are accessed. - EXPECT_FALSE(materialAsset->IsFinalized()); - materialAsset->GetPropertyValues(); + materialAsset->Finalize(); EXPECT_TRUE(materialAsset->IsFinalized()); // Now all the property values should be available through the main GetPropertyValues() API. @@ -295,6 +302,8 @@ namespace UnitTest EXPECT_EQ(materialAsset->GetPropertyValues()[8].GetValue>(), m_testImageAsset); EXPECT_EQ(materialAsset->GetPropertyValues()[9].GetValue(), 1u); + // The raw property values are still available (because they are needed if a hot-reload of the MaterialTypeAsset occurs) + checkRawPropertyValues(); } void CheckEqual(MaterialSourceData& a, MaterialSourceData& b) @@ -757,8 +766,9 @@ namespace UnitTest tester.SerializeOut(materialAssetLevel3.Get()); materialAssetLevel3 = tester.SerializeIn(Uuid::CreateRandom(), ObjectStream::FilterDescriptor{AZ::Data::AssetFilterNoAssetLoading}); - - // The properties will finalize automatically when we call GetPropertyValues()... + materialAssetLevel1->Finalize(); + materialAssetLevel2->Finalize(); + materialAssetLevel3->Finalize(); AZStd::array_view properties; @@ -779,10 +789,6 @@ namespace UnitTest EXPECT_EQ(properties[myFloat.GetIndex()].GetValue(), 3.5f); EXPECT_EQ(properties[myFloat2.GetIndex()].GetValue(), Vector2(4.1f, 4.2f)); EXPECT_EQ(properties[myColor.GetIndex()].GetValue(), Color(0.15f, 0.25f, 0.35f, 0.45f)); - - EXPECT_TRUE(materialAssetLevel1->IsFinalized()); - EXPECT_TRUE(materialAssetLevel2->IsFinalized()); - EXPECT_TRUE(materialAssetLevel3->IsFinalized()); } TEST_F(MaterialSourceDataTests, CreateMaterialAsset_MultiLevelDataInheritance_Error_MaterialTypesDontMatch) From c24546a85d1ba0d190fac9e424e9e9b87a9fe6a5 Mon Sep 17 00:00:00 2001 From: santorac <55155825+santorac@users.noreply.github.com> Date: Fri, 14 Jan 2022 10:57:03 -0800 Subject: [PATCH 08/10] Fixed unused variable warning. Signed-off-by: santorac <55155825+santorac@users.noreply.github.com> --- .../Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialAsset.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialAsset.cpp b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialAsset.cpp index 569e2de81a..310c10c39b 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialAsset.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialAsset.cpp @@ -123,7 +123,7 @@ namespace AZ if (!reportWarning) { - reportWarning = [](const char* message) + reportWarning = []([[maybe_unused]] const char* message) { AZ_Warning(s_debugTraceName, false, "%s", message); }; From 2627b507d3081bf38248389b475e89e4192f0fd4 Mon Sep 17 00:00:00 2001 From: santorac <55155825+santorac@users.noreply.github.com> Date: Tue, 18 Jan 2022 12:01:19 -0800 Subject: [PATCH 09/10] Fixed unused variable warning Signed-off-by: santorac <55155825+santorac@users.noreply.github.com> --- .../Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialAsset.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialAsset.cpp b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialAsset.cpp index 310c10c39b..0325c3ac38 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialAsset.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialAsset.cpp @@ -131,7 +131,7 @@ namespace AZ if (!reportError) { - reportError = [](const char* message) + reportError = []([[maybe_unused]] const char* message) { AZ_Error(s_debugTraceName, false, "%s", message); }; From 45429872d60d79c1daa8b7957e967cc2526e642a Mon Sep 17 00:00:00 2001 From: santorac <55155825+santorac@users.noreply.github.com> Date: Tue, 18 Jan 2022 17:39:44 -0800 Subject: [PATCH 10/10] Switched back to making MaterialAsset::GetPropertyValues automatically finalize the material asset. I realized that it's too burdensome to expect client code to call Finalize on the MaterialAsset; every code that calls GetPropertyValues would have to call Finalize(). Instead of using const_cast in GetPropertyValues like I was doing before, I just changed GetPropertyValues to be a non-const function. There were a few places in Decal code I had to update to pass non-const MaterialAsset pointers. This isn't ideal, but I think it's better than the alternatives. Signed-off-by: santorac <55155825+santorac@users.noreply.github.com> --- .../Code/Source/Decals/DecalTextureArray.cpp | 6 +- .../Code/Source/Decals/DecalTextureArray.h | 2 +- .../DecalTextureArrayFeatureProcessor.cpp | 6 +- .../DecalTextureArrayFeatureProcessor.h | 2 +- .../Atom/RPI.Reflect/Material/MaterialAsset.h | 29 ++++----- .../RPI.Builders/Material/MaterialBuilder.cpp | 2 +- .../Model/MaterialAssetBuilderComponent.cpp | 2 +- .../Source/RPI.Public/Material/Material.cpp | 2 - .../RPI.Reflect/Material/MaterialAsset.cpp | 35 ++++++----- .../Material/MaterialAssetCreator.cpp | 2 + .../Tests/Material/MaterialAssetTests.cpp | 61 +++++++++++++++++-- .../Material/MaterialSourceDataTests.cpp | 41 +++++-------- 12 files changed, 118 insertions(+), 72 deletions(-) diff --git a/Gems/Atom/Feature/Common/Code/Source/Decals/DecalTextureArray.cpp b/Gems/Atom/Feature/Common/Code/Source/Decals/DecalTextureArray.cpp index 36a59bd07f..56ff1648d4 100644 --- a/Gems/Atom/Feature/Common/Code/Source/Decals/DecalTextureArray.cpp +++ b/Gems/Atom/Feature/Common/Code/Source/Decals/DecalTextureArray.cpp @@ -49,7 +49,7 @@ namespace AZ } // Extract exactly which texture asset we need to load from the given material and map type (diffuse, normal, etc). - static AZ::Data::Asset GetStreamingImageAsset(const AZ::RPI::MaterialAsset& materialAsset, const AZ::Name& propertyName) + static AZ::Data::Asset GetStreamingImageAsset(AZ::RPI::MaterialAsset& materialAsset, const AZ::Name& propertyName) { if (!materialAsset.IsReady()) { @@ -84,7 +84,7 @@ namespace AZ static AZ::Data::Asset GetStreamingImageAsset(const AZ::Data::Asset materialAssetData, const AZ::Name& propertyName) { AZ_Assert(materialAssetData->IsReady(), "GetStreamingImageAsset() called with AssetData that is not ready."); - const AZ::RPI::MaterialAsset* materialAsset = materialAssetData.GetAs(); + AZ::RPI::MaterialAsset* materialAsset = materialAssetData.GetAs(); return GetStreamingImageAsset(*materialAsset, propertyName); } } @@ -141,7 +141,7 @@ namespace AZ return m_textureArrayPacked[mapType]; } - bool DecalTextureArray::IsValidDecalMaterial(const AZ::RPI::MaterialAsset& materialAsset) + bool DecalTextureArray::IsValidDecalMaterial(AZ::RPI::MaterialAsset& materialAsset) { return GetStreamingImageAsset(materialAsset, GetMapName(DecalMapType_Diffuse)).IsReady(); } diff --git a/Gems/Atom/Feature/Common/Code/Source/Decals/DecalTextureArray.h b/Gems/Atom/Feature/Common/Code/Source/Decals/DecalTextureArray.h index 97bd8b9cbe..39e66176fd 100644 --- a/Gems/Atom/Feature/Common/Code/Source/Decals/DecalTextureArray.h +++ b/Gems/Atom/Feature/Common/Code/Source/Decals/DecalTextureArray.h @@ -57,7 +57,7 @@ namespace AZ // often different (BC5 for normals, BC7 for diffuse, etc) const Data::Instance& GetPackedTexture(const DecalMapType mapType) const; - static bool IsValidDecalMaterial(const RPI::MaterialAsset& materialAsset); + static bool IsValidDecalMaterial(RPI::MaterialAsset& materialAsset); private: diff --git a/Gems/Atom/Feature/Common/Code/Source/Decals/DecalTextureArrayFeatureProcessor.cpp b/Gems/Atom/Feature/Common/Code/Source/Decals/DecalTextureArrayFeatureProcessor.cpp index 393104907a..c6b4d4e754 100644 --- a/Gems/Atom/Feature/Common/Code/Source/Decals/DecalTextureArrayFeatureProcessor.cpp +++ b/Gems/Atom/Feature/Common/Code/Source/Decals/DecalTextureArrayFeatureProcessor.cpp @@ -25,7 +25,7 @@ namespace AZ { namespace { - static AZ::RHI::Size GetTextureSizeFromMaterialAsset(const AZ::RPI::MaterialAsset* materialAsset) + static AZ::RHI::Size GetTextureSizeFromMaterialAsset(AZ::RPI::MaterialAsset* materialAsset) { for (const auto& elem : materialAsset->GetPropertyValues()) { @@ -375,7 +375,7 @@ namespace AZ } } - AZStd::optional DecalTextureArrayFeatureProcessor::AddMaterialToTextureArrays(const AZ::RPI::MaterialAsset* materialAsset) + AZStd::optional DecalTextureArrayFeatureProcessor::AddMaterialToTextureArrays(AZ::RPI::MaterialAsset* materialAsset) { const RHI::Size textureSize = GetTextureSizeFromMaterialAsset(materialAsset); @@ -410,7 +410,7 @@ namespace AZ AZ_PROFILE_SCOPE(AzRender, "DecalTextureArrayFeatureProcessor: OnAssetReady"); const Data::AssetId& assetId = asset->GetId(); - const RPI::MaterialAsset* materialAsset = asset.GetAs(); + RPI::MaterialAsset* materialAsset = asset.GetAs(); const bool validDecalMaterial = materialAsset && DecalTextureArray::IsValidDecalMaterial(*materialAsset); if (validDecalMaterial) { diff --git a/Gems/Atom/Feature/Common/Code/Source/Decals/DecalTextureArrayFeatureProcessor.h b/Gems/Atom/Feature/Common/Code/Source/Decals/DecalTextureArrayFeatureProcessor.h index fd535bbe64..bdd1739ebb 100644 --- a/Gems/Atom/Feature/Common/Code/Source/Decals/DecalTextureArrayFeatureProcessor.h +++ b/Gems/Atom/Feature/Common/Code/Source/Decals/DecalTextureArrayFeatureProcessor.h @@ -111,7 +111,7 @@ namespace AZ void CacheShaderIndices(); // This call could fail (returning nullopt) if we run out of texture arrays - AZStd::optional AddMaterialToTextureArrays(const AZ::RPI::MaterialAsset* materialAsset); + AZStd::optional AddMaterialToTextureArrays(AZ::RPI::MaterialAsset* materialAsset); int FindTextureArrayWithSize(const RHI::Size& size) const; void RemoveMaterialFromDecal(const uint16_t decalIndex); diff --git a/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialAsset.h b/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialAsset.h index 941fcd0fe9..b7cc51dcc4 100644 --- a/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialAsset.h +++ b/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialAsset.h @@ -102,16 +102,6 @@ namespace AZ //! Returns a layout that includes a list of MaterialPropertyDescriptors for each material property. const MaterialPropertiesLayout* GetMaterialPropertiesLayout() const; - //! Returns whether the material's properties are fully processed or not. - //! If true, property values can be accessed through GetPropertyValues(). - //! If false, property values can be accessed through GetRawPropertyValues(). - bool IsFinalized() const; - - //! If the material asset is not finalized yet, this does the final processing of the raw property values to - //! get the material asset ready to be used. - //! Note the MaterialTypeAsset must be valid before this is called. - void Finalize(AZStd::function reportWarning = nullptr, AZStd::function reportError = nullptr); - //! Returns the list of values for all properties in this material. //! The entries in this list align with the entries in the MaterialPropertiesLayout. Each AZStd::any is guaranteed //! to have a value of type that matches the corresponding MaterialPropertyDescriptor. @@ -122,19 +112,26 @@ namespace AZ //! //! Calling GetPropertyValues() will automatically finalize the material asset if it isn't finalized already. The //! MaterialTypeAsset must be loaded and ready. - const AZStd::vector& GetPropertyValues() const; - + const AZStd::vector& GetPropertyValues(); + + //! Returns true if material was created in a finalize state, as opposed to being finalized after loading from disk. + bool WasPreFinalized() const; + //! Returns the list of raw values for all properties in this material, as listed in the source .material file(s), before the material asset was Finalized. //! //! The MaterialAsset can be created in a "half-baked" state (see MaterialUtils::BuildersShouldFinalizeMaterialAssets) where //! minimal processing has been done because it did not yet have access to the MaterialTypeAsset. In that case, the list will //! be populated with values copied from the source .material file with little or no validation or other processing. It includes //! all parent .material files, with properties listed in low-to-high priority order. - //! This list will be empty however if the asset was finalized at build-time. + //! This list will be empty however if the asset was finalized at build-time (i.e. WasPreFinalized() returns true). const AZStd::vector>& GetRawPropertyValues() const; private: bool PostLoadInit() override; + + //! If the material asset is not finalized yet, this does the final processing of the raw property values to get the material asset ready to be used. + //! MaterialTypeAsset must be valid before this is called. + void Finalize(AZStd::function reportWarning = nullptr, AZStd::function reportError = nullptr); //! Checks the material type version and potentially applies a series of property changes (most common are simple property renames) //! based on the MaterialTypeAsset's version update procedure. @@ -159,7 +156,7 @@ namespace AZ //! Holds values for each material property, used to initialize Material instances. //! This is indexed by MaterialPropertyIndex and aligns with entries in m_materialPropertiesLayout. - AZStd::vector m_propertyValues; + mutable AZStd::vector m_propertyValues; //! The MaterialAsset can be created in a "half-baked" state where minimal processing has been done because it does //! not yet have access to the MaterialTypeAsset. In that case, this list will be populated with values copied from @@ -175,10 +172,10 @@ namespace AZ AZStd::vector> m_rawPropertyValues; //! Tracks whether Finalize() has been called, meaning m_propertyValues is populated with data matching the material type's property layout. - bool m_isFinalized = false; + //! (This value is intentionally not serialized, it is set by the Finalize() function) + mutable bool m_isFinalized = false; //! Tracks whether the MaterialAsset was already in a finalized state when it was loaded. - //! (This value is intentionally not serialized) bool m_wasPreFinalized = false; //! The materialTypeVersion this materialAsset was based off. If the versions do not match at runtime when a 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 cedbb1df6e..768890a29b 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Builders/Material/MaterialBuilder.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Builders/Material/MaterialBuilder.cpp @@ -52,7 +52,7 @@ namespace AZ { AssetBuilderSDK::AssetBuilderDesc materialBuilderDescriptor; materialBuilderDescriptor.m_name = JobKey; - materialBuilderDescriptor.m_version = 114; // material dependency improvements + materialBuilderDescriptor.m_version = 115; // material dependency improvements updated 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(); diff --git a/Gems/Atom/RPI/Code/Source/RPI.Builders/Model/MaterialAssetBuilderComponent.cpp b/Gems/Atom/RPI/Code/Source/RPI.Builders/Model/MaterialAssetBuilderComponent.cpp index a44b47ee6c..9a92e7b762 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Builders/Model/MaterialAssetBuilderComponent.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Builders/Model/MaterialAssetBuilderComponent.cpp @@ -128,7 +128,7 @@ namespace AZ if (auto* serialize = azrtti_cast(context)) { serialize->Class() - ->Version(20); // material dependency improvements + ->Version(21); // material dependency improvements updated } } diff --git a/Gems/Atom/RPI/Code/Source/RPI.Public/Material/Material.cpp b/Gems/Atom/RPI/Code/Source/RPI.Public/Material/Material.cpp index fe9dbef278..1f739c24f1 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Public/Material/Material.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Public/Material/Material.cpp @@ -62,8 +62,6 @@ namespace AZ m_materialAsset = { &materialAsset, AZ::Data::AssetLoadBehavior::PreLoad }; - m_materialAsset->Finalize(); - // Cache off pointers to some key data structures from the material type... auto srgLayout = m_materialAsset->GetMaterialSrgLayout(); if (srgLayout) diff --git a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialAsset.cpp b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialAsset.cpp index 0325c3ac38..309daf8b62 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialAsset.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialAsset.cpp @@ -33,12 +33,12 @@ namespace AZ if (auto* serializeContext = azrtti_cast(context)) { serializeContext->Class() - ->Version(13) // added m_rawPropertyValues + ->Version(14) // added m_rawPropertyValues ->Field("materialTypeAsset", &MaterialAsset::m_materialTypeAsset) ->Field("materialTypeVersion", &MaterialAsset::m_materialTypeVersion) ->Field("propertyValues", &MaterialAsset::m_propertyValues) ->Field("rawPropertyValues", &MaterialAsset::m_rawPropertyValues) - ->Field("isFinalized", &MaterialAsset::m_isFinalized) + ->Field("finalized", &MaterialAsset::m_wasPreFinalized) ; } } @@ -104,19 +104,19 @@ namespace AZ return m_materialTypeAsset->GetMaterialPropertiesLayout(); } - bool MaterialAsset::IsFinalized() const + bool MaterialAsset::WasPreFinalized() const { - if (m_isFinalized) - { - AZ_Assert(GetMaterialPropertiesLayout() && m_propertyValues.size() == GetMaterialPropertiesLayout()->GetPropertyCount(), "MaterialAsset is marked as Finalized but does not have the right number of property values."); - } - - return m_isFinalized; + return m_wasPreFinalized; } void MaterialAsset::Finalize(AZStd::function reportWarning, AZStd::function reportError) { - if (IsFinalized()) + if (m_wasPreFinalized) + { + m_isFinalized = true; + } + + if (m_isFinalized) { return; } @@ -197,10 +197,18 @@ namespace AZ m_isFinalized = true; } - const AZStd::vector& MaterialAsset::GetPropertyValues() const + const AZStd::vector& MaterialAsset::GetPropertyValues() { - AZ_Error(s_debugTraceName, IsFinalized(), "MaterialAsset must be finalized before its property values can be accessed"); - + // This can't be done in MaterialAssetHandler::LoadAssetData because the MaterialTypeAsset isn't necessarily loaded at that point. + // And it can't be done in PostLoadInit() because that happens on the next frame which might be too late. + // And overriding AssetHandler::InitAsset in MaterialAssetHandler didn't work, because there seems to be non-determinism on the order + // of InitAsset calls when a ModelAsset references a MaterialAsset, the model gets initialized first and then fails to use the material. + // So we finalize just-in-time when properties are accessed. + // If we could solve the problem with InitAsset, that would be the ideal place to call Finalize() and we could make GetPropertyValues() const again. + Finalize(); + + AZ_Assert(GetMaterialPropertiesLayout() && m_propertyValues.size() == GetMaterialPropertiesLayout()->GetPropertyCount(), "MaterialAsset should be finalized but does not have the right number of property values."); + return m_propertyValues; } @@ -334,7 +342,6 @@ namespace AZ if (Base::LoadAssetData(asset, stream, assetLoadFilterCB) == Data::AssetHandler::LoadResult::LoadComplete) { asset.GetAs()->AssetInitBus::Handler::BusConnect(); - asset.GetAs()->m_wasPreFinalized = asset.GetAs()->m_isFinalized; return Data::AssetHandler::LoadResult::LoadComplete; } diff --git a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialAssetCreator.cpp b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialAssetCreator.cpp index 7d4ecf0b18..872e4ad754 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialAssetCreator.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialAssetCreator.cpp @@ -49,6 +49,8 @@ namespace AZ [this](const char* message) { ReportWarning("%s", message); }, [this](const char* message) { ReportError("%s", message); }); + m_asset->m_wasPreFinalized = true; + // Finalize() doesn't clear the raw property data because that's the same function used at runtime, which does need to maintain the raw data // to support hot reload. But here we are pre-baking with the assumption that AP build dependencies will keep the material type // and material asset in sync, so we can discard the raw property data and just rely on the data in the material type asset. diff --git a/Gems/Atom/RPI/Code/Tests/Material/MaterialAssetTests.cpp b/Gems/Atom/RPI/Code/Tests/Material/MaterialAssetTests.cpp index fdb131d449..61e1283f52 100644 --- a/Gems/Atom/RPI/Code/Tests/Material/MaterialAssetTests.cpp +++ b/Gems/Atom/RPI/Code/Tests/Material/MaterialAssetTests.cpp @@ -111,6 +111,10 @@ namespace UnitTest EXPECT_EQ(assetId, materialAsset->GetId()); EXPECT_EQ(Data::AssetData::AssetStatus::Ready, materialAsset->GetStatus()); + + EXPECT_TRUE(materialAsset->WasPreFinalized()); + EXPECT_EQ(0, materialAsset->GetRawPropertyValues().size()); + validate(materialAsset); // Also test serialization... @@ -123,6 +127,57 @@ namespace UnitTest Data::Asset serializedAsset = tester.SerializeIn(Data::AssetId(Uuid::CreateRandom()), noAssets); validate(serializedAsset); } + + TEST_F(MaterialAssetTests, DeferredFinalize) + { + Data::AssetId assetId(Uuid::CreateRandom()); + + MaterialAssetCreator creator; + bool shouldFinalize = false; + creator.Begin(assetId, m_testMaterialTypeAsset, shouldFinalize); + + creator.SetPropertyValue(Name{ "MyFloat2" }, Vector2{ 0.1f, 0.2f }); + creator.SetPropertyValue(Name{ "MyFloat3" }, Vector3{ 1.1f, 1.2f, 1.3f }); + creator.SetPropertyValue(Name{ "MyFloat4" }, Vector4{ 2.1f, 2.2f, 2.3f, 2.4f }); + creator.SetPropertyValue(Name{ "MyColor" }, Color{ 1.0f, 1.0f, 1.0f, 1.0f }); + creator.SetPropertyValue(Name{ "MyInt" }, -2); + creator.SetPropertyValue(Name{ "MyUInt" }, 12u); + creator.SetPropertyValue(Name{ "MyFloat" }, 1.5f); + creator.SetPropertyValue(Name{ "MyBool" }, true); + creator.SetPropertyValue(Name{ "MyImage" }, m_testImageAsset); + creator.SetPropertyValue(Name{ "MyEnum" }, 1u); + + Data::Asset materialAsset; + EXPECT_TRUE(creator.End(materialAsset)); + + EXPECT_FALSE(materialAsset->WasPreFinalized()); + EXPECT_EQ(10, materialAsset->GetRawPropertyValues().size()); + + // Also test serialization... + + SerializeTester tester(GetSerializeContext()); + tester.SerializeOut(materialAsset.Get()); + + // Using a filter that skips loading assets because we are using a dummy image asset + ObjectStream::FilterDescriptor noAssets{ AZ::Data::AssetFilterNoAssetLoading }; + Data::Asset serializedAsset = tester.SerializeIn(Data::AssetId(Uuid::CreateRandom()), noAssets); + + EXPECT_FALSE(materialAsset->WasPreFinalized()); + EXPECT_EQ(10, materialAsset->GetRawPropertyValues().size()); + + // GetPropertyValues() will automatically finalize the material asset, so we can go ahead and check the property values. + EXPECT_EQ(materialAsset->GetPropertyValues().size(), 10); + EXPECT_EQ(materialAsset->GetPropertyValues()[0].GetValue(), true); + EXPECT_EQ(materialAsset->GetPropertyValues()[1].GetValue(), -2); + EXPECT_EQ(materialAsset->GetPropertyValues()[2].GetValue(), 12); + EXPECT_EQ(materialAsset->GetPropertyValues()[3].GetValue(), 1.5f); + EXPECT_EQ(materialAsset->GetPropertyValues()[4].GetValue(), Vector2(0.1f, 0.2f)); + EXPECT_EQ(materialAsset->GetPropertyValues()[5].GetValue(), Vector3(1.1f, 1.2f, 1.3f)); + EXPECT_EQ(materialAsset->GetPropertyValues()[6].GetValue(), Vector4(2.1f, 2.2f, 2.3f, 2.4f)); + EXPECT_EQ(materialAsset->GetPropertyValues()[7].GetValue(), Color(1.0f, 1.0f, 1.0f, 1.0f)); + EXPECT_EQ(materialAsset->GetPropertyValues()[8].GetValue>(), m_testImageAsset); + EXPECT_EQ(materialAsset->GetPropertyValues()[9].GetValue(), 1u); + } TEST_F(MaterialAssetTests, PropertyDefaultValuesComeFromParentMaterial) { @@ -267,15 +322,13 @@ namespace UnitTest warningFinder.AddExpectedErrorMessage("This material is based on version '1'"); warningFinder.AddExpectedErrorMessage("material type is now at version '2'"); - materialAsset->Finalize(); - - warningFinder.CheckExpectedErrorsFound(); - // Even though this material was created using the old version of the material type, it's property values should get automatically // updated to align with the new property layout in the latest MaterialTypeAsset. MaterialPropertyIndex myIntIndex = materialAsset->GetMaterialPropertiesLayout()->FindPropertyIndex(Name{"MyIntRenamed"}); EXPECT_EQ(2, myIntIndex.GetIndex()); EXPECT_EQ(7, materialAsset->GetPropertyValues()[myIntIndex.GetIndex()].GetValue()); + + warningFinder.CheckExpectedErrorsFound(); // Since the MaterialAsset has already been updated, and the warning reported once, we should not see the "consider updating" // warning reported again on subsequent property accesses. diff --git a/Gems/Atom/RPI/Code/Tests/Material/MaterialSourceDataTests.cpp b/Gems/Atom/RPI/Code/Tests/Material/MaterialSourceDataTests.cpp index 29f0e7d101..ef89e0138c 100644 --- a/Gems/Atom/RPI/Code/Tests/Material/MaterialSourceDataTests.cpp +++ b/Gems/Atom/RPI/Code/Tests/Material/MaterialSourceDataTests.cpp @@ -182,7 +182,7 @@ namespace UnitTest Data::Asset materialAsset = materialAssetOutcome.GetValue(); - EXPECT_TRUE(materialAsset->IsFinalized()); + EXPECT_TRUE(materialAsset->WasPreFinalized()); EXPECT_EQ(0, materialAsset->GetRawPropertyValues().size()); // A pre-baked material has no need for the original raw property names and values // The order here is based on the order in the MaterialTypeSourceData, as added to the MaterialTypeAssetCreator. @@ -227,14 +227,10 @@ namespace UnitTest EXPECT_TRUE(materialAssetOutcome.IsSuccess()); Data::Asset materialAsset = materialAssetOutcome.GetValue(); - - ErrorMessageFinder expectNotFinalizedError("MaterialAsset must be finalized"); - EXPECT_FALSE(materialAsset->IsFinalized()); + EXPECT_FALSE(materialAsset->WasPreFinalized()); - expectNotFinalizedError.ResetCounts(); - EXPECT_TRUE(materialAsset->GetPropertyValues().empty()); - expectNotFinalizedError.CheckExpectedErrorsFound(); + // Note we avoid calling GetPropertyValues() because that will auto-finalize the material. We want to check its raw property values first. auto findRawPropertyValue = [materialAsset](const char* propertyId) { @@ -279,16 +275,10 @@ namespace UnitTest SerializeTester tester(GetSerializeContext()); tester.SerializeOut(materialAsset.Get()); materialAsset = tester.SerializeIn(Uuid::CreateRandom(), ObjectStream::FilterDescriptor{AZ::Data::AssetFilterNoAssetLoading}); - - // We check that everything is still in the original un-finalized state after going through the serialization process. - EXPECT_FALSE(materialAsset->IsFinalized()); - checkRawPropertyValues(); - expectNotFinalizedError.ResetCounts(); - EXPECT_TRUE(materialAsset->GetPropertyValues().empty()); - expectNotFinalizedError.CheckExpectedErrorsFound(); - materialAsset->Finalize(); - EXPECT_TRUE(materialAsset->IsFinalized()); + // We check that the asset is still in the original un-finalized state after going through the serialization process. + EXPECT_FALSE(materialAsset->WasPreFinalized()); + checkRawPropertyValues(); // Now all the property values should be available through the main GetPropertyValues() API. EXPECT_EQ(materialAsset->GetPropertyValues()[0].GetValue(), true); @@ -301,8 +291,9 @@ namespace UnitTest EXPECT_EQ(materialAsset->GetPropertyValues()[7].GetValue(), Color(0.1f, 0.2f, 0.3f, 0.4f)); EXPECT_EQ(materialAsset->GetPropertyValues()[8].GetValue>(), m_testImageAsset); EXPECT_EQ(materialAsset->GetPropertyValues()[9].GetValue(), 1u); - + // The raw property values are still available (because they are needed if a hot-reload of the MaterialTypeAsset occurs) + EXPECT_FALSE(materialAsset->WasPreFinalized()); checkRawPropertyValues(); } @@ -659,19 +650,19 @@ namespace UnitTest auto materialAssetLevel1 = sourceDataLevel1.CreateMaterialAsset(Uuid::CreateRandom(), "", MaterialAssetProcessingMode::PreBake, true); EXPECT_TRUE(materialAssetLevel1.IsSuccess()); - EXPECT_TRUE(materialAssetLevel1.GetValue()->IsFinalized()); + EXPECT_TRUE(materialAssetLevel1.GetValue()->WasPreFinalized()); m_assetSystemStub.RegisterSourceInfo("level1.material", materialAssetLevel1.GetValue().GetId()); auto materialAssetLevel2 = sourceDataLevel2.CreateMaterialAsset(Uuid::CreateRandom(), "", MaterialAssetProcessingMode::PreBake, true); EXPECT_TRUE(materialAssetLevel2.IsSuccess()); - EXPECT_TRUE(materialAssetLevel2.GetValue()->IsFinalized()); + EXPECT_TRUE(materialAssetLevel2.GetValue()->WasPreFinalized()); m_assetSystemStub.RegisterSourceInfo("level2.material", materialAssetLevel2.GetValue().GetId()); auto materialAssetLevel3 = sourceDataLevel3.CreateMaterialAsset(Uuid::CreateRandom(), "", MaterialAssetProcessingMode::PreBake, true); EXPECT_TRUE(materialAssetLevel3.IsSuccess()); - EXPECT_TRUE(materialAssetLevel3.GetValue()->IsFinalized()); + EXPECT_TRUE(materialAssetLevel3.GetValue()->WasPreFinalized()); auto layout = m_testMaterialTypeAsset->GetMaterialPropertiesLayout(); MaterialPropertyIndex myFloat = layout->FindPropertyIndex(Name("general.MyFloat")); @@ -731,21 +722,21 @@ namespace UnitTest auto materialAssetLevel1Result = sourceDataLevel1.CreateMaterialAsset(Uuid::CreateRandom(), "", MaterialAssetProcessingMode::DeferredBake, true); EXPECT_TRUE(materialAssetLevel1Result.IsSuccess()); Data::Asset materialAssetLevel1 = materialAssetLevel1Result.TakeValue(); - EXPECT_FALSE(materialAssetLevel1->IsFinalized()); + EXPECT_FALSE(materialAssetLevel1->WasPreFinalized()); m_assetSystemStub.RegisterSourceInfo("level1.material", materialAssetLevel1.GetId()); auto materialAssetLevel2Result = sourceDataLevel2.CreateMaterialAsset(Uuid::CreateRandom(), "", MaterialAssetProcessingMode::DeferredBake, true); EXPECT_TRUE(materialAssetLevel2Result.IsSuccess()); Data::Asset materialAssetLevel2 = materialAssetLevel2Result.TakeValue(); - EXPECT_FALSE(materialAssetLevel2->IsFinalized()); + EXPECT_FALSE(materialAssetLevel2->WasPreFinalized()); m_assetSystemStub.RegisterSourceInfo("level2.material", materialAssetLevel2.GetId()); auto materialAssetLevel3Result = sourceDataLevel3.CreateMaterialAsset(Uuid::CreateRandom(), "", MaterialAssetProcessingMode::DeferredBake, true); EXPECT_TRUE(materialAssetLevel3Result.IsSuccess()); Data::Asset materialAssetLevel3 = materialAssetLevel3Result.TakeValue(); - EXPECT_FALSE(materialAssetLevel3->IsFinalized()); + EXPECT_FALSE(materialAssetLevel3->WasPreFinalized()); // Now we'll create the material type asset in memory so the materials will have what they need to finalize. Data::Asset testMaterialTypeAsset = CreateTestMaterialTypeAsset(materialTypeAssetId); @@ -766,9 +757,7 @@ namespace UnitTest tester.SerializeOut(materialAssetLevel3.Get()); materialAssetLevel3 = tester.SerializeIn(Uuid::CreateRandom(), ObjectStream::FilterDescriptor{AZ::Data::AssetFilterNoAssetLoading}); - materialAssetLevel1->Finalize(); - materialAssetLevel2->Finalize(); - materialAssetLevel3->Finalize(); + // The properties will finalize automatically when we call GetPropertyValues()... AZStd::array_view properties;