From b176697ce9b18d009a2a06fe0199c82ba8c54e0b Mon Sep 17 00:00:00 2001 From: Chris Santora Date: Wed, 16 Jun 2021 17:04:49 -0700 Subject: [PATCH] Fixed ATOM-14613 Baseviewer MatertialHotReloadTest fails to change the color after turning blending on and off 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. --- .../Material/MaterialReloadNotificationBus.h | 5 +++++ .../Shader/ShaderReloadNotificationBus.h | 5 +++++ .../Source/RPI.Public/Material/Material.cpp | 14 ++++++++++---- .../Code/Source/RPI.Public/Shader/Shader.cpp | 11 ++++++++--- .../RPI.Reflect/Material/MaterialAsset.cpp | 17 ++++++++++++----- 5 files changed, 40 insertions(+), 12 deletions(-) diff --git a/Gems/Atom/RPI/Code/Include/Atom/RPI.Public/Material/MaterialReloadNotificationBus.h b/Gems/Atom/RPI/Code/Include/Atom/RPI.Public/Material/MaterialReloadNotificationBus.h index c28f6a3233..881b40b785 100644 --- a/Gems/Atom/RPI/Code/Include/Atom/RPI.Public/Material/MaterialReloadNotificationBus.h +++ b/Gems/Atom/RPI/Code/Include/Atom/RPI.Public/Material/MaterialReloadNotificationBus.h @@ -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 { diff --git a/Gems/Atom/RPI/Code/Include/Atom/RPI.Public/Shader/ShaderReloadNotificationBus.h b/Gems/Atom/RPI/Code/Include/Atom/RPI.Public/Shader/ShaderReloadNotificationBus.h index c63ba8f5b5..8ea34187ff 100644 --- a/Gems/Atom/RPI/Code/Include/Atom/RPI.Public/Shader/ShaderReloadNotificationBus.h +++ b/Gems/Atom/RPI/Code/Include/Atom/RPI.Public/Shader/ShaderReloadNotificationBus.h @@ -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 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 e134b701ac..3302190156 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Public/Material/Material.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Public/Material/Material.cpp @@ -242,8 +242,16 @@ namespace AZ // MaterialReloadNotificationBus overrides... void Material::OnMaterialAssetReinitialized(const Data::Asset& 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) { - // 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 diff --git a/Gems/Atom/RPI/Code/Source/RPI.Public/Shader/Shader.cpp b/Gems/Atom/RPI/Code/Source/RPI.Public/Shader/Shader.cpp index 6f65bd75c9..f451064450 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Public/Shader/Shader.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Public/Shader/Shader.cpp @@ -213,10 +213,15 @@ namespace AZ // ShaderReloadNotificationBus overrides... void Shader::OnShaderAssetReinitialized(const Data::Asset& 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); + } } /////////////////////////////////////////////////////////////////// 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 b8567b12c5..7446c498cd 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialAsset.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialAsset.cpp @@ -115,12 +115,19 @@ namespace AZ } } - void MaterialAsset::OnMaterialTypeAssetReinitialized(const Data::Asset&) + void MaterialAsset::OnMaterialTypeAssetReinitialized(const Data::Asset& 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{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{this, AZ::Data::AssetLoadBehavior::PreLoad}); + } } void MaterialAsset::ReinitializeMaterialTypeAsset(Data::Asset asset)