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

Fixed a recently introduced Material initialization bug.

Browse Source 
Merge pull request #4203 from aws-lumberyard-dev/Atom/santorac/FixMaterialInitializationBug-ATOM-16476

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.

Testing:
Atom Sample Viewer full test suite on vulkan and dx12. Test either passed or failed in exactly the same way before the bug was introduced (using local baseline to verify).
RPI unit tests passed.
Ran the editor and used material component to apply metallic materials. This was showing the black material before, and shows correctly now.
This commit is contained in:
santorac
2021-09-17 21:00:01 -07:00
committed by GitHub
parent df9b0714a4 f2a4746b8b
commit e456b36722
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);
}
}