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)