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

Merge pull request #1385 from aws-lumberyard-dev/santorac/stabilization/2106/HotReloadFixes

Browse Source 
Fixed ATOM-14613 MatertialHotReloadTest fails to reload the material after reloading the shader

The problem was...
After a MaterialAsset reload, there could be two different versions of the MaterialAsset in memory: the old one and the reloaded one. The old one is still connected to buses and can send reinitialization messages when other things reload or reinitialize. So when the shader asset reloaded, both the old and new MaterialAsset were sending reinitialization messages. Material::OnMaterialAssetReinitialized was using the materialAsset parameter to initialize the Material, and the latest call to OnMaterialAssetReinitialized was for the old MaterialAsset.

The solution is to use the m_materialAsset member when reinitializing the Material. I also added checks in a couple places to skip unnecessary reinitialization, and added comments in the bus headers to warn developers about this issue.

Testing: Added a new step to ASV's MaterialHotReloadTest.bv.lua script for the error scenario, and this now passes. Ran ASV full test suite, both dx12 and vulkan, only known issues occurred. Tested .shader and .material hot reload in Editor and Material Editor.

See also aws-lumberyard/o3de-atom-sampleviewer#121
This commit is contained in:
Chris Santora
2021-06-16 18:47:32 -07:00
committed by GitHub
parent 7b0d2aac03 688c5d1052
commit 6cd1c44b7e
5 changed files with 40 additions and 12 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Gems/Atom/RPI/Code/Include/Atom/RPI.Public/Material/MaterialReloadNotificationBus.h
+5
View File
@@ -24,6 +24,11 @@ namespace AZ
//! Connect to this EBus to get notifications whenever material objects reload.
//! The bus address is the AssetId of the MaterialAsset or MaterialTypeAsset.
//!
//! Be careful when using the parameters provided by these functions. The bus ID is an AssetId, and it's possible for the system to have
//! both *old* versions and *new reloaded* versions of the asset in memory at the same time, and they will have the same AssetId. Therefore
//! your bus Handlers could receive Reinitialized messages from multiple sources. It may be necessary to check the memory addresses of these
//! parameters against local members before using this data.
class MaterialReloadNotifications
: public EBusTraits
{
Gems/Atom/RPI/Code/Include/Atom/RPI.Public/Shader/ShaderReloadNotificationBus.h
+5
View File
@@ -27,6 +27,11 @@ namespace AZ
/**
* Connect to this EBus to get notifications whenever a shader system class reinitializes itself.
* The bus address is the AssetId of the ShaderAsset, even when the thing being reinitialized is a ShaderVariant or other shader related class.
*
* Be careful when using the parameters provided by these functions. The bus ID is an AssetId, and it's possible for the system to have
* both *old* versions and *new reloaded* versions of the asset in memory at the same time, and they will have the same AssetId. Therefore
* your bus Handlers could receive Reinitialized messages from multiple sources. It may be necessary to check the memory addresses of these
* parameters against local members before using this data.
*/
class ShaderReloadNotifications
: public EBusTraits
Gems/Atom/RPI/Code/Source/RPI.Public/Material/Material.cpp
+10 -4
View File
@@ -242,8 +242,16 @@ namespace AZ
// MaterialReloadNotificationBus overrides...
void Material::OnMaterialAssetReinitialized(const Data::Asset<MaterialAsset>& materialAsset)
{
ShaderReloadDebugTracker::ScopedSection reloadSection("{%p}->Material::OnMaterialAssetReinitialized %s", this, materialAsset.GetHint().c_str());
OnAssetReloaded(materialAsset);
// It's important that we don't just pass materialAsset to Init() because when reloads occur,
// it's possible for old Asset objects to hang around and report reinitialization, so materialAsset
// might be stale data.
if (materialAsset.Get() == m_materialAsset.Get())
{
ShaderReloadDebugTracker::ScopedSection reloadSection("{%p}->Material::OnMaterialAssetReinitialized %s", this, materialAsset.GetHint().c_str());
OnAssetReloaded(m_materialAsset);
}
}
///////////////////////////////////////////////////////////////////
@@ -259,8 +267,6 @@ namespace AZ
void Material::OnShaderAssetReinitialized(const Data::Asset<ShaderAsset>& shaderAsset)
{
// TODO: I think we should make Shader handle OnShaderAssetReinitialized and treat it just like the shader reloaded.
ShaderReloadDebugTracker::ScopedSection reloadSection("{%p}->Material::OnShaderAssetReinitialized %s", this, shaderAsset.GetHint().c_str());
// Note that it might not be strictly necessary to reinitialize the entire material, we might be able to get away with
// just bumping the m_currentChangeId or some other minor updates. But it's pretty hard to know what exactly needs to be
Gems/Atom/RPI/Code/Source/RPI.Public/Shader/Shader.cpp
+8 -3
View File
@@ -213,10 +213,15 @@ namespace AZ
// ShaderReloadNotificationBus overrides...
void Shader::OnShaderAssetReinitialized(const Data::Asset<ShaderAsset>& shaderAsset)
{
ShaderReloadDebugTracker::ScopedSection reloadSection("{%p}->Shader::OnShaderAssetReinitialized %s", this, shaderAsset.GetHint().c_str());
// When reloads occur, it's possible for old Asset objects to hang around and report reinitialization,
// so we can reduce unnecessary reinitialization in that case.
if (shaderAsset.Get() == m_asset.Get())
{
ShaderReloadDebugTracker::ScopedSection reloadSection("{%p}->Shader::OnShaderAssetReinitialized %s", this, shaderAsset.GetHint().c_str());
Init(*m_asset.Get());
ShaderReloadNotificationBus::Event(shaderAsset.GetId(), &ShaderReloadNotificationBus::Events::OnShaderReinitialized, *this);
Init(*m_asset.Get());
ShaderReloadNotificationBus::Event(shaderAsset.GetId(), &ShaderReloadNotificationBus::Events::OnShaderReinitialized, *this);
}
}
///////////////////////////////////////////////////////////////////
Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialAsset.cpp
+12 -5
View File
@@ -115,12 +115,19 @@ namespace AZ
}
}
void MaterialAsset::OnMaterialTypeAssetReinitialized(const Data::Asset<MaterialTypeAsset>&)
void MaterialAsset::OnMaterialTypeAssetReinitialized(const Data::Asset<MaterialTypeAsset>& materialTypeAsset)
{
// MaterialAsset doesn't need to reinitialize any of its own data when MaterialTypeAsset reinitializes,
// because all it depends on is the MaterialTypeAsset reference, rather than the data inside it.
// Ultimately it's the Material that cares about these changes, so we just forward any signal we get.
MaterialReloadNotificationBus::Event(GetId(), &MaterialReloadNotifications::OnMaterialAssetReinitialized, Data::Asset<MaterialAsset>{this, AZ::Data::AssetLoadBehavior::PreLoad});
// When reloads occur, it's possible for old Asset objects to hang around and report reinitialization,
// so we can reduce unnecessary reinitialization in that case.
if (materialTypeAsset.Get() == m_materialTypeAsset.Get())
{
ShaderReloadDebugTracker::ScopedSection reloadSection("{%p}->MaterialAsset::OnMaterialTypeAssetReinitialized %s", this, materialTypeAsset.GetHint().c_str());
// MaterialAsset doesn't need to reinitialize any of its own data when MaterialTypeAsset reinitializes,
// because all it depends on is the MaterialTypeAsset reference, rather than the data inside it.
// Ultimately it's the Material that cares about these changes, so we just forward any signal we get.
MaterialReloadNotificationBus::Event(GetId(), &MaterialReloadNotifications::OnMaterialAssetReinitialized, Data::Asset<MaterialAsset>{this, AZ::Data::AssetLoadBehavior::PreLoad});
}
}
void MaterialAsset::ReinitializeMaterialTypeAsset(Data::Asset<Data::AssetData> asset)