birkeh/o3de
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

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.

Browse Source 
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>
This commit is contained in:
santorac
2022-01-14 10:49:17 -08:00
parent 8084775d7a
commit 2d6d14abf7
8 changed files with 35 additions and 28 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Gems/Atom/RPI/Code/Include/Atom/RPI.Edit/Material/MaterialSourceData.h
+1 -1
View File
@@ -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
Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialAsset.h
+5 -6
View File
@@ -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<void(const char*)> reportWarning = nullptr, AZStd::function<void(const char*)> 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<void(const char*)> reportWarning = nullptr, AZStd::function<void(const char*)> 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();
Gems/Atom/RPI/Code/Source/RPI.Builders/Material/MaterialBuilder.cpp
+1 -1
View File
@@ -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<MaterialBuilder>();
Gems/Atom/RPI/Code/Source/RPI.Builders/Model/MaterialAssetBuilderComponent.cpp
+3 -2
View File
@@ -45,7 +45,8 @@ namespace AZ
if (auto* serialize = azrtti_cast<SerializeContext*>(context))
{
serialize->Class<MaterialAssetDependenciesComponent, Component>()
->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<Crc32>({ AssetBuilderSDK::ComponentTags::AssetBuilder }));
}
}
@@ -127,7 +128,7 @@ namespace AZ
if (auto* serialize = azrtti_cast<SerializeContext*>(context))
{
serialize->Class<MaterialAssetBuilderComponent, SceneAPI::SceneCore::ExportingComponent>()
->Version(19); // material dependency improvements
->Version(20); // material dependency improvements
}
}
Gems/Atom/RPI/Code/Source/RPI.Public/Material/Material.cpp
+2
View File
@@ -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)
Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialAsset.cpp
+1 -4
View File
@@ -199,10 +199,7 @@ namespace AZ
const AZStd::vector<MaterialPropertyValue>& 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<MaterialAsset*>(this)->Finalize();
AZ_Error(s_debugTraceName, IsFinalized(), "MaterialAsset must be finalized before its property values can be accessed");
return m_propertyValues;
}
Gems/Atom/RPI/Code/Tests/Material/MaterialAssetTests.cpp
+4 -2
View File
@@ -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<int32_t>());
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();
Gems/Atom/RPI/Code/Tests/Material/MaterialSourceDataTests.cpp
+18 -12
View File
@@ -228,8 +228,13 @@ namespace UnitTest
Data::Asset<MaterialAsset> 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.
checkRawPropertyValues();
// The material will automatically finalize itself when the properties are accessed.
// We check that everything is still in the original un-finalized state after going through the serialization process.
EXPECT_FALSE(materialAsset->IsFinalized());
materialAsset->GetPropertyValues();
checkRawPropertyValues();
expectNotFinalizedError.ResetCounts();
EXPECT_TRUE(materialAsset->GetPropertyValues().empty());
expectNotFinalizedError.CheckExpectedErrorsFound();
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<Data::Asset<ImageAsset>>(), m_testImageAsset);
EXPECT_EQ(materialAsset->GetPropertyValues()[9].GetValue<uint32_t>(), 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<MaterialPropertyValue> properties;
@@ -779,10 +789,6 @@ namespace UnitTest
EXPECT_EQ(properties[myFloat.GetIndex()].GetValue<float>(), 3.5f);
EXPECT_EQ(properties[myFloat2.GetIndex()].GetValue<Vector2>(), Vector2(4.1f, 4.2f));
EXPECT_EQ(properties[myColor.GetIndex()].GetValue<Color>(), 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)