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

Fixed a recently introduced Material initialization bug.

Browse Source 
An early-return in Material::SetPropertyValue was breaking initialization because Init() was called multiple times, and on subsequent initializations the property values weren't getting reset.
Added a unit test to ensure this kind of thing doesn't happen again.

Signed-off-by: santorac <55155825+santorac@users.noreply.github.com>
This commit is contained in:
santorac
2021-09-17 18:03:33 -07:00
parent 453dfd6f34
commit bd0032a6d1
3 changed files with 85 additions and 1 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialAsset.h
+6
View File
@@ -19,6 +19,11 @@
#include <AzCore/EBus/Event.h>
namespace UnitTest
{
class MaterialTests;
}
namespace AZ
{
class ReflectContext;
@@ -40,6 +45,7 @@ namespace AZ
friend class MaterialAssetCreator;
friend class MaterialAssetHandler;
friend class MaterialAssetCreatorCommon;
friend class UnitTest::MaterialTests;
public:
AZ_RTTI(MaterialAsset, "{522C7BE0-501D-463E-92C6-15184A2B7AD8}", AZ::Data::AssetData);
Gems/Atom/RPI/Code/Source/RPI.Public/Material/Material.cpp
+7 -1
View File
@@ -100,9 +100,15 @@ namespace AZ
ShaderReloadNotificationBus::MultiHandler::BusConnect(shaderItem.GetShaderAsset().GetId());
}
// If this Init() is actually a re-initialize, we need to re-apply any overridden property values
// after loading the property values from the asset, so we save that data here.
MaterialPropertyFlags prevOverrideFlags = m_propertyOverrideFlags;
AZStd::vector<MaterialPropertyValue> prevPropertyValues = m_propertyValues;
// The property values are cleared to their default state to ensure that SetPropertyValue() does not early-return
// when called below. This is important when Init() is actually a re-initialize.
m_propertyValues.clear();
// Initialize the shader runtime data like shader constant buffers and shader variants by applying the
// material's property values. This will feed through the normal runtime material value-change data flow, which may
// include custom property change handlers provided by the material type.
@@ -504,7 +510,7 @@ namespace AZ
MaterialPropertyValue& savedPropertyValue = m_propertyValues[index.GetIndex()];
// If the property value didn't actually change, don't waste time running functors and compiling the changes
// If the property value didn't actually change, don't waste time running functors and compiling the changes.
if (savedPropertyValue == value)
{
return false;
Gems/Atom/RPI/Code/Tests/Material/MaterialTests.cpp
+72
View File
@@ -182,6 +182,13 @@ namespace UnitTest
EXPECT_EQ(srgData.GetImageView(srgData.FindShaderInputImageIndex(Name{ "m_image" }), 0), m_testImage->GetImageView());
EXPECT_EQ(srgData.GetConstant<uint32_t>(srgData.FindShaderInputConstantIndex(Name{ "m_enum" })), 2u);
}
//! Provides write access to private material asset property values, primarily for simulating
//! MaterialAsset hot reload.
MaterialPropertyValue& AccessMaterialAssetPropertyValue(Data::Asset<MaterialAsset> materialAsset, Name propertyName)
{
return materialAsset->m_propertyValues[materialAsset->GetMaterialPropertiesLayout()->FindPropertyIndex(propertyName).GetIndex()];
}
};
TEST_F(MaterialTests, TestCreateVsFindOrCreate)
@@ -313,6 +320,30 @@ namespace UnitTest
EXPECT_EQ(srgData.GetConstant<uint32_t>(srgData.FindShaderInputConstantIndex(Name{ "m_uint" })), 42u);
}
TEST_F(MaterialTests, TestSetPropertyValueWhenValueIsUnchanged)
{
Data::Instance<Material> material = Material::FindOrCreate(m_testMaterialAsset);
EXPECT_TRUE(material->SetPropertyValue<float>(material->FindPropertyIndex(Name{ "MyFloat" }), 2.5f));
ProcessQueuedSrgCompilations(m_testMaterialShaderAsset, m_testMaterialSrgLayout->GetName());
EXPECT_TRUE(material->Compile());
// Taint the SRG so we can check whether it was set by the SetPropertyValue() calls below.
const RHI::ShaderResourceGroup* srg = material->GetRHIShaderResourceGroup();
const RHI::ShaderResourceGroupData& srgData = srg->GetData();
const_cast<RHI::ShaderResourceGroupData*>(&srgData)->SetConstant(m_testMaterialSrgLayout->FindShaderInputConstantIndex(Name{"m_float"}), 0.0f);
// Set the properties to the same values as before
EXPECT_FALSE(material->SetPropertyValue<float>(material->FindPropertyIndex(Name{ "MyFloat" }), 2.5f));
ProcessQueuedSrgCompilations(m_testMaterialShaderAsset, m_testMaterialSrgLayout->GetName());
EXPECT_FALSE(material->Compile());
// Make sure the SRG is still tainted, because the SetPropertyValue() functions weren't processed
EXPECT_EQ(srgData.GetConstant<float>(srgData.FindShaderInputConstantIndex(Name{ "m_float" })), 0.0f);
}
TEST_F(MaterialTests, TestImageNotProvided)
{
Data::Asset<MaterialAsset> materialAssetWithEmptyImage;
@@ -785,4 +816,45 @@ namespace UnitTest
EXPECT_EQ((float)inputColor.GetElement(i), (float)colorFromMaterial.GetElement(i));
}
}
TEST_F(MaterialTests, TestReinitializeForHotReload)
{
Data::Instance<Material> material = Material::FindOrCreate(m_testMaterialAsset);
const RHI::ShaderResourceGroupData* srgData = &material->GetRHIShaderResourceGroup()->GetData();
ProcessQueuedSrgCompilations(m_testMaterialShaderAsset, m_testMaterialSrgLayout->GetName());
// Check the default property value
EXPECT_EQ(material->GetPropertyValue<float>(material->FindPropertyIndex(Name{ "MyFloat" })), 1.5f);
EXPECT_EQ(srgData->GetConstant<float>(srgData->FindShaderInputConstantIndex(Name{ "m_float" })), 1.5f);
EXPECT_EQ(material->GetPropertyValue<int32_t>(material->FindPropertyIndex(Name{ "MyInt" })), -2);
EXPECT_EQ(srgData->GetConstant<int32_t>(srgData->FindShaderInputConstantIndex(Name{ "m_int" })), -2);
// Override a property value
EXPECT_TRUE(material->SetPropertyValue<float>(material->FindPropertyIndex(Name{ "MyFloat" }), 5.5f));
// Apply the changes
EXPECT_TRUE(material->Compile());
ProcessQueuedSrgCompilations(m_testMaterialShaderAsset, m_testMaterialSrgLayout->GetName());
// Check the updated values with one overridden
EXPECT_EQ(material->GetPropertyValue<float>(material->FindPropertyIndex(Name{ "MyFloat" })), 5.5f);
EXPECT_EQ(srgData->GetConstant<float>(srgData->FindShaderInputConstantIndex(Name{ "m_float" })), 5.5f);
EXPECT_EQ(material->GetPropertyValue<int32_t>(material->FindPropertyIndex(Name{ "MyInt" })), -2);
EXPECT_EQ(srgData->GetConstant<int32_t>(srgData->FindShaderInputConstantIndex(Name{ "m_int" })), -2);
// Pretend there was a hot-reload with new default values
AccessMaterialAssetPropertyValue(m_testMaterialAsset, Name{"MyFloat"}) = 0.5f;
AccessMaterialAssetPropertyValue(m_testMaterialAsset, Name{"MyInt"}) = -7;
AZ::Data::AssetBus::Event(m_testMaterialAsset.GetId(), &AZ::Data::AssetBus::Handler::OnAssetReloaded, m_testMaterialAsset);
srgData = &material->GetRHIShaderResourceGroup()->GetData();
ProcessQueuedSrgCompilations(m_testMaterialShaderAsset, m_testMaterialSrgLayout->GetName());
// Make sure the override values are still there
EXPECT_EQ(srgData->GetConstant<float>(srgData->FindShaderInputConstantIndex(Name{ "m_float" })), 5.5f);
EXPECT_EQ(material->GetPropertyValue<float>(material->FindPropertyIndex(Name{ "MyFloat" })), 5.5f);
// Make sure the new default value is applied where it was not overridden
EXPECT_EQ(material->GetPropertyValue<int32_t>(material->FindPropertyIndex(Name{ "MyInt" })), -7);
EXPECT_EQ(srgData->GetConstant<int32_t>(srgData->FindShaderInputConstantIndex(Name{ "m_int" })), -7);
}
}