From 982406d4d5c5da77abb26c96d93add96438e0f56 Mon Sep 17 00:00:00 2001 From: Chris Santora Date: Mon, 14 Jun 2021 20:36:12 -0700 Subject: [PATCH 1/4] Fixed race conditions that could prevent successful hot-reload of shaders. ATOM-15728 Shader Hot Reload Fails in Debug Build The main change was to add OnAssetReady handlers to each of the asset classes. See comments in ShaderAsset::OnAssetReady for a detailed explanation. In short, OnAssetReloaded gets missed while assets are being reloaded at the same time on multiple threads, but OnAssetReady is always called whenever connecting to the AssetBus because of its AssetConnectionPolicy. The above change required the addition of a new AssetInitBus to call the PostLoadInit() functions. Because OnAssetReady connects to buses that are not mutex-protected, they have to be connected on the main thread. AssetInitBus::PostLoadInit is called every frame in RPISystem::SimulationTick. All Atom's asset handlers that need to do post-load initialization must connect to the AssetInitBus, and the asset will disconnect itself after initialization is complete. We also need the Shader class to handle OnShaderAssetReinitialized to properly handle the shader reload. With these changes I can click back and forth between "Blending On" and "Blending Off" many times (like 20 times) without issue. --- .../Include/Atom/RPI.Public/AssetInitBus.h | 41 ++++++++++++++ .../Include/Atom/RPI.Public/Shader/Shader.h | 11 ++++ .../Atom/RPI.Reflect/Material/MaterialAsset.h | 8 ++- .../RPI.Reflect/Material/MaterialTypeAsset.h | 8 ++- .../Atom/RPI.Reflect/Shader/ShaderAsset.h | 8 ++- .../RPI/Code/Source/RPI.Public/RPISystem.cpp | 3 ++ .../Code/Source/RPI.Public/Shader/Shader.cpp | 23 ++++++-- .../RPI.Reflect/Material/MaterialAsset.cpp | 37 +++++++++---- .../Material/MaterialTypeAsset.cpp | 34 ++++++++---- .../Source/RPI.Reflect/Shader/ShaderAsset.cpp | 53 +++++++++++-------- .../RPI.Reflect/Shader/ShaderAssetCreator.cpp | 2 +- .../Atom/RPI/Code/atom_rpi_public_files.cmake | 1 + 12 files changed, 179 insertions(+), 50 deletions(-) create mode 100644 Gems/Atom/RPI/Code/Include/Atom/RPI.Public/AssetInitBus.h diff --git a/Gems/Atom/RPI/Code/Include/Atom/RPI.Public/AssetInitBus.h b/Gems/Atom/RPI/Code/Include/Atom/RPI.Public/AssetInitBus.h new file mode 100644 index 0000000000..8f49aec929 --- /dev/null +++ b/Gems/Atom/RPI/Code/Include/Atom/RPI.Public/AssetInitBus.h @@ -0,0 +1,41 @@ +/* +* All or portions of this file Copyright (c) Amazon.com, Inc. or its affiliates or +* its licensors. +* +* For complete copyright and license terms please see the LICENSE at the root of this +* distribution (the "License"). All use of this software is governed by the License, +* or, if provided, by the license below or the license accompanying this file. Do not +* remove or modify any license notices. This file is distributed on an "AS IS" BASIS, +* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +* +*/ + +#pragma once + +#include + +namespace AZ +{ + namespace RPI + { + //! Bus for post-load initialization of assets. + //! Assets that need to do post-load initialization should connect to this bus in their asset handler's LoadAssetData() function. + //! Be sure to disconnect from this bus as soon as initialization is complete, as it will be called every frame. + class AssetInitEvents + : public EBusTraits + { + public: + // EBus Configuration + static const AZ::EBusHandlerPolicy HandlerPolicy = AZ::EBusHandlerPolicy::Multiple; + static const AZ::EBusAddressPolicy AddressPolicy = AZ::EBusAddressPolicy::Single; + typedef AZStd::recursive_mutex MutexType; + + //! This function is called every frame on the main thread to perform any necessary post-load initialization. + //! Connect to the bus after loading the asset data, and disconnect when initialization is complete. + //! @return whether initialization was successful + virtual bool PostLoadInit() = 0; + }; + + using AssetInitBus = AZ::EBus; + } // namespace RPI +} // namespace AZ diff --git a/Gems/Atom/RPI/Code/Include/Atom/RPI.Public/Shader/Shader.h b/Gems/Atom/RPI/Code/Include/Atom/RPI.Public/Shader/Shader.h index a81a3a99eb..a8e65e68f4 100644 --- a/Gems/Atom/RPI/Code/Include/Atom/RPI.Public/Shader/Shader.h +++ b/Gems/Atom/RPI/Code/Include/Atom/RPI.Public/Shader/Shader.h @@ -12,6 +12,7 @@ #pragma once #include +#include #include #include @@ -57,6 +58,7 @@ namespace AZ : public Data::InstanceData , public Data::AssetBus::Handler , public ShaderVariantFinderNotificationBus::Handler + , public ShaderReloadNotificationBus::Handler { friend class ShaderSystem; public: @@ -149,6 +151,15 @@ namespace AZ void OnShaderVariantTreeAssetReady(Data::Asset /*shaderVariantTreeAsset*/, bool /*isError*/) override {}; void OnShaderVariantAssetReady(Data::Asset shaderVariantAsset, bool IsError) override; /////////////////////////////////////////////////////////////////// + + /////////////////////////////////////////////////////////////////// + // ShaderReloadNotificationBus overrides... + void OnShaderAssetReinitialized(const Data::Asset& shaderAsset) override; + // Note we don't need OnShaderVariantReinitialized because the Shader class doesn't do anything with the data inside + // the ShaderVariant object. The only thing we might want to do is propagate the message upward, but that's unnecessary + // because the ShaderReloadNotificationBus uses the Shader's AssetId as the ID for all messages including those from the variants. + // And of course we don't need to handle OnShaderReinitialized because this *is* this Shader. + /////////////////////////////////////////////////////////////////// /// Returns the path to the pipeline library cache file. AZStd::string GetPipelineLibraryPath() const; diff --git a/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialAsset.h b/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialAsset.h index c941f1f587..d59f8a5c1c 100644 --- a/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialAsset.h +++ b/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialAsset.h @@ -15,6 +15,7 @@ #include #include +#include #include #include #include @@ -38,6 +39,7 @@ namespace AZ : public AZ::Data::AssetData , public Data::AssetBus::Handler , public MaterialReloadNotificationBus::Handler + , public AssetInitBus::Handler { friend class MaterialAssetCreator; friend class MaterialAssetHandler; @@ -90,13 +92,17 @@ namespace AZ AZStd::array_view GetPropertyValues() const; private: - bool PostLoadInit(); + bool PostLoadInit() override; //! Called by asset creators to assign the asset to a ready state. void SetReady(); // AssetBus overrides... void OnAssetReloaded(Data::Asset asset) override; + void OnAssetReady(Data::Asset asset) override; + + //! Replaces the MaterialTypeAsset when a reload occurs + void ReinitializeMaterialTypeAsset(Data::Asset asset); // MaterialReloadNotificationBus overrides... void OnMaterialTypeAssetReinitialized(const Data::Asset& materialTypeAsset) override; diff --git a/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialTypeAsset.h b/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialTypeAsset.h index 6b260b5ae3..f046a1afe2 100644 --- a/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialTypeAsset.h +++ b/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Material/MaterialTypeAsset.h @@ -17,6 +17,7 @@ #include #include +#include #include #include #include @@ -52,6 +53,7 @@ namespace AZ class MaterialTypeAsset : public AZ::Data::AssetData , public Data::AssetBus::MultiHandler + , public AssetInitBus::Handler { friend class MaterialTypeAssetCreator; friend class MaterialTypeAssetHandler; @@ -100,13 +102,17 @@ namespace AZ MaterialUvNameMap GetUvNameMap() const; private: - bool PostLoadInit(); + bool PostLoadInit() override; //! Called by asset creators to assign the asset to a ready state. void SetReady(); // AssetBus overrides... void OnAssetReloaded(Data::Asset asset) override; + void OnAssetReady(Data::Asset asset) override; + + //! Replaces the appropriate asset members when a reload occurs + void ReinitializeAsset(Data::Asset asset); //! Holds values for each material property, used to initialize Material instances. //! This is indexed by MaterialPropertyIndex and aligns with entries in m_materialPropertiesLayout. diff --git a/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Shader/ShaderAsset.h b/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Shader/ShaderAsset.h index cc31b0735a..55d10717a7 100644 --- a/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Shader/ShaderAsset.h +++ b/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Shader/ShaderAsset.h @@ -15,6 +15,7 @@ #include #include +#include #include #include #include @@ -40,6 +41,7 @@ namespace AZ : public Data::AssetData , public ShaderVariantFinderNotificationBus::Handler , public Data::AssetBus::Handler + , public AssetInitBus::Handler { friend class ShaderAssetCreator; friend class ShaderAssetHandler; @@ -134,8 +136,11 @@ namespace AZ /////////////////////////////////////////////////////////////////// /// AssetBus overrides void OnAssetReloaded(Data::Asset asset) override; + void OnAssetReady(Data::Asset asset) override; /////////////////////////////////////////////////////////////////// + void ReinitializeRootShaderVariant(Data::Asset asset); + /////////////////////////////////////////////////////////////////// /// ShaderVariantFinderNotificationBus overrides void OnShaderVariantTreeAssetReady(Data::Asset shaderVariantTreeAsset, bool isError) override; @@ -165,7 +170,7 @@ namespace AZ RHI::ShaderStageAttributeMapList m_attributeMaps; }; - bool FinalizeAfterLoad(); + bool PostLoadInit() override; void SetReady(); ShaderApiDataContainer& GetCurrentShaderApiData(); const ShaderApiDataContainer& GetCurrentShaderApiData() const; @@ -216,7 +221,6 @@ namespace AZ const Data::Asset& asset, AZStd::shared_ptr stream, const Data::AssetFilterCB& assetLoadFilterCB) override; - Data::AssetHandler::LoadResult PostLoadInit(const Data::Asset& asset); }; ////////////////////////////////////////////////////////////////////////// diff --git a/Gems/Atom/RPI/Code/Source/RPI.Public/RPISystem.cpp b/Gems/Atom/RPI/Code/Source/RPI.Public/RPISystem.cpp index 44be2dabeb..87b6f5d92e 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Public/RPISystem.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Public/RPISystem.cpp @@ -23,6 +23,7 @@ #include #include +#include #include #include #include @@ -240,6 +241,8 @@ namespace AZ } AZ_ATOM_PROFILE_FUNCTION("RPI", "RPISystem: SimulationTick"); + AssetInitBus::Broadcast(&AssetInitBus::Events::PostLoadInit); + // Update tick time info FillTickTimeInfo(); 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 147f24d4f8..3a79f32b05 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Public/Shader/Shader.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Public/Shader/Shader.cpp @@ -21,7 +21,6 @@ #include #include -#include #include namespace AZ @@ -55,8 +54,9 @@ namespace AZ RHI::ResultCode Shader::Init(ShaderAsset& shaderAsset) { + Data::AssetBus::Handler::BusDisconnect(); + ShaderReloadNotificationBus::Handler::BusDisconnect(); ShaderVariantFinderNotificationBus::Handler::BusDisconnect(); - ShaderVariantFinderNotificationBus::Handler::BusConnect(shaderAsset.GetId()); RHI::RHISystemInterface* rhiSystem = RHI::RHISystemInterface::Get(); RHI::DrawListTagRegistry* drawListTagRegistry = rhiSystem->GetDrawListTagRegistry(); @@ -100,8 +100,10 @@ namespace AZ AZ_Error("Shader", false, "Failed to acquire a DrawListTag. Entries are full."); } } - + + ShaderVariantFinderNotificationBus::Handler::BusConnect(m_asset.GetId()); Data::AssetBus::Handler::BusConnect(m_asset.GetId()); + ShaderReloadNotificationBus::Handler::BusConnect(m_asset.GetId()); return RHI::ResultCode::Success; } @@ -110,6 +112,7 @@ namespace AZ { ShaderVariantFinderNotificationBus::Handler::BusDisconnect(); Data::AssetBus::Handler::BusDisconnect(); + ShaderReloadNotificationBus::Handler::BusDisconnect(); if (m_pipelineLibraryHandle.IsValid()) { @@ -139,7 +142,6 @@ namespace AZ Data::Asset newAsset = { asset.GetAs(), AZ::Data::AssetLoadBehavior::PreLoad }; AZ_Assert(newAsset, "Reloaded ShaderAsset is null"); - Data::AssetBus::Handler::BusDisconnect(); Init(*newAsset.Get()); ShaderReloadNotificationBus::Event(asset.GetId(), &ShaderReloadNotificationBus::Events::OnShaderReinitialized, *this); } @@ -196,6 +198,19 @@ namespace AZ } /////////////////////////////////////////////////////////////////// + + /////////////////////////////////////////////////////////////////// + // ShaderReloadNotificationBus overrides... + void Shader::OnShaderAssetReinitialized(const Data::Asset& shaderAsset) + { + ShaderReloadDebugTracker::ScopedSection reloadSection("{%p}->Shader::OnShaderAssetReinitialized %s", this, shaderAsset.GetHint().c_str()); + + Init(*m_asset.Get()); + ShaderReloadNotificationBus::Event(shaderAsset.GetId(), &ShaderReloadNotificationBus::Events::OnShaderReinitialized, *this); + } + /////////////////////////////////////////////////////////////////// + + ConstPtr Shader::LoadPipelineLibrary() const { if (IO::FileIOBase::GetInstance()) 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 2e56c30652..b8567b12c5 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialAsset.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialAsset.cpp @@ -18,6 +18,7 @@ #include #include +#include namespace AZ { @@ -47,6 +48,7 @@ namespace AZ { MaterialReloadNotificationBus::Handler::BusDisconnect(); Data::AssetBus::Handler::BusDisconnect(); + AssetInitBus::Handler::BusDisconnect(); } const Data::Asset& MaterialAsset::GetMaterialTypeAsset() const @@ -97,6 +99,8 @@ namespace AZ { if (!m_materialTypeAsset.Get()) { + AssetInitBus::Handler::BusDisconnect(); + // Any MaterialAsset with invalid MaterialTypeAsset is not a successfully-loaded asset. return false; } @@ -104,6 +108,8 @@ namespace AZ { Data::AssetBus::Handler::BusConnect(m_materialTypeAsset.GetId()); MaterialReloadNotificationBus::Handler::BusConnect(m_materialTypeAsset.GetId()); + + AssetInitBus::Handler::BusDisconnect(); return true; } @@ -116,11 +122,9 @@ namespace AZ // 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::OnAssetReloaded(Data::Asset asset) + + void MaterialAsset::ReinitializeMaterialTypeAsset(Data::Asset asset) { - ShaderReloadDebugTracker::ScopedSection reloadSection("{%p}->MaterialAsset::OnAssetReloaded %s", this, asset.GetHint().c_str()); - Data::Asset newMaterialTypeAsset = { asset.GetAs(), AZ::Data::AssetLoadBehavior::PreLoad }; if (newMaterialTypeAsset) @@ -135,16 +139,31 @@ namespace AZ } } + void MaterialAsset::OnAssetReloaded(Data::Asset asset) + { + ShaderReloadDebugTracker::ScopedSection reloadSection("{%p}->MaterialAsset::OnAssetReloaded %s", this, asset.GetHint().c_str()); + ReinitializeMaterialTypeAsset(asset); + } + + void MaterialAsset::OnAssetReady(Data::Asset asset) + { + // Regarding why we listen to both OnAssetReloaded and OnAssetReady, see explanation in ShaderAsset::OnAssetReady. + ShaderReloadDebugTracker::ScopedSection reloadSection("{%p}->MaterialAsset::OnAssetReady %s", this, asset.GetHint().c_str()); + ReinitializeMaterialTypeAsset(asset); + } + Data::AssetHandler::LoadResult MaterialAssetHandler::LoadAssetData( const AZ::Data::Asset& asset, AZStd::shared_ptr stream, const AZ::Data::AssetFilterCB& assetLoadFilterCB) { - Data::AssetHandler::LoadResult baseResult = Base::LoadAssetData(asset, stream, assetLoadFilterCB); - bool postLoadResult = asset.GetAs()->PostLoadInit(); - return ((baseResult == Data::AssetHandler::LoadResult::LoadComplete) && postLoadResult) ? - Data::AssetHandler::LoadResult::LoadComplete : - Data::AssetHandler::LoadResult::Error; + if (Base::LoadAssetData(asset, stream, assetLoadFilterCB) == Data::AssetHandler::LoadResult::LoadComplete) + { + asset.GetAs()->AssetInitBus::Handler::BusConnect(); + return Data::AssetHandler::LoadResult::LoadComplete; + } + + return Data::AssetHandler::LoadResult::Error; } } // namespace RPI diff --git a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialTypeAsset.cpp b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialTypeAsset.cpp index f7d0a83ac9..eaf6f0fcde 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialTypeAsset.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialTypeAsset.cpp @@ -65,6 +65,7 @@ namespace AZ MaterialTypeAsset::~MaterialTypeAsset() { Data::AssetBus::MultiHandler::BusDisconnect(); + AssetInitBus::Handler::BusDisconnect(); } const ShaderCollection& MaterialTypeAsset::GetShaderCollection() const @@ -116,6 +117,8 @@ namespace AZ Data::AssetBus::MultiHandler::BusConnect(shaderItem.GetShaderAsset().GetId()); } + AssetInitBus::Handler::BusDisconnect(); + return true; } @@ -127,11 +130,9 @@ namespace AZ assetToReplace = newAsset; } } - - void MaterialTypeAsset::OnAssetReloaded(Data::Asset asset) + + void MaterialTypeAsset::ReinitializeAsset(Data::Asset asset) { - ShaderReloadDebugTracker::ScopedSection reloadSection("{%p}->MaterialTypeAsset::OnAssetReloaded %s", this, asset.GetHint().c_str()); - // The order of asset reloads is non-deterministic. If the MaterialTypeAsset reloads before these // dependency assets, this will make sure the MaterialTypeAsset gets the latest ones when they reload. // Or in some cases a these assets could get updated and reloaded without reloading the MaterialTypeAsset at all. @@ -146,16 +147,31 @@ namespace AZ MaterialReloadNotificationBus::Event(GetId(), &MaterialReloadNotifications::OnMaterialTypeAssetReinitialized, Data::Asset{this, AZ::Data::AssetLoadBehavior::PreLoad}); } + void MaterialTypeAsset::OnAssetReloaded(Data::Asset asset) + { + ShaderReloadDebugTracker::ScopedSection reloadSection("{%p}->MaterialTypeAsset::OnAssetReloaded %s", this, asset.GetHint().c_str()); + ReinitializeAsset(asset); + } + + void MaterialTypeAsset::OnAssetReady(Data::Asset asset) + { + // Regarding why we listen to both OnAssetReloaded and OnAssetReady, see explanation in ShaderAsset::OnAssetReady. + ShaderReloadDebugTracker::ScopedSection reloadSection("{%p}->MaterialTypeAsset::OnAssetReady %s", this, asset.GetHint().c_str()); + ReinitializeAsset(asset); + } + AZ::Data::AssetHandler::LoadResult MaterialTypeAssetHandler::LoadAssetData( const AZ::Data::Asset& asset, AZStd::shared_ptr stream, const AZ::Data::AssetFilterCB& assetLoadFilterCB) { - Data::AssetHandler::LoadResult baseResult = Base::LoadAssetData(asset, stream, assetLoadFilterCB); - bool postLoadResult = asset.GetAs()->PostLoadInit(); - return ((baseResult == Data::AssetHandler::LoadResult::LoadComplete) && postLoadResult) ? - Data::AssetHandler::LoadResult::LoadComplete : - Data::AssetHandler::LoadResult::Error; + if (Base::LoadAssetData(asset, stream, assetLoadFilterCB) == Data::AssetHandler::LoadResult::LoadComplete) + { + asset.GetAs()->AssetInitBus::Handler::BusConnect(); + return Data::AssetHandler::LoadResult::LoadComplete; + } + + return Data::AssetHandler::LoadResult::Error; } } diff --git a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Shader/ShaderAsset.cpp b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Shader/ShaderAsset.cpp index 75d4a47bc0..e08cf425d7 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Shader/ShaderAsset.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Shader/ShaderAsset.cpp @@ -85,6 +85,7 @@ namespace AZ { Data::AssetBus::Handler::BusDisconnect(); ShaderVariantFinderNotificationBus::Handler::BusDisconnect(); + AssetInitBus::Handler::BusDisconnect(); } const Name& ShaderAsset::GetName() const @@ -332,7 +333,7 @@ namespace AZ return m_perAPIShaderData[0]; } - bool ShaderAsset::FinalizeAfterLoad() + bool ShaderAsset::PostLoadInit() { // Use the current RHI that is active to select which shader data to use. // We don't assert if the Factory is not available because this method could be called during build time, @@ -373,25 +374,43 @@ namespace AZ // Once the ShaderAsset is loaded, it is necessary to listen for changes in the Root Variant Asset. Data::AssetBus::Handler::BusConnect(GetRootVariant().GetId()); ShaderVariantFinderNotificationBus::Handler::BusConnect(GetId()); + + AssetInitBus::Handler::BusDisconnect(); return true; } + + void ShaderAsset::ReinitializeRootShaderVariant(Data::Asset asset) + { + Data::Asset shaderVariantAsset = { asset.GetAs(), AZ::Data::AssetLoadBehavior::PreLoad }; + AZ_Assert(shaderVariantAsset->GetStableId() == RootShaderVariantStableId, "Was expecting to update the root variant"); + GetCurrentShaderApiData().m_rootShaderVariantAsset = asset; + ShaderReloadNotificationBus::Event(GetId(), &ShaderReloadNotificationBus::Events::OnShaderAssetReinitialized, Data::Asset{ this, AZ::Data::AssetLoadBehavior::PreLoad } ); + } /////////////////////////////////////////////////////////////////////// // AssetBus overrides... void ShaderAsset::OnAssetReloaded(Data::Asset asset) { ShaderReloadDebugTracker::ScopedSection reloadSection("{%p}->ShaderAsset::OnAssetReloaded %s", this, asset.GetHint().c_str()); + ReinitializeRootShaderVariant(asset); + } + void ShaderAsset::OnAssetReady(Data::Asset asset) + { + // We have to listen to OnAssetReady, OnAssetReloaded isn't enough, because of the following scenario: + // The user changes a .shader file, which causes the AP to rebuild the ShaderAsset and root ShaderVariantAsset. + // 1) Thread A creates the new ShaderAsset, loads it, and gets the old ShaderVariantAsset. + // 2) Thread B creates the new ShaderVariantAsset, loads it, and calls OnAssetReloaded. + // 3) Main thread calls ShaderAsset::PostLoadInit which connects to the AssetBus but it's too late to receive OnAssetReloaded, + // so it continues using the old ShaderVariantAsset instead of the new one. + // The OnAssetReady bus function is called automatically whenever a connection to AssetBus is made, so listening to this gives + // us the opportunity to assign the appropriate ShaderVariantAsset. - Data::Asset shaderVariantAsset = { asset.GetAs(), AZ::Data::AssetLoadBehavior::PreLoad }; - AZ_Assert(shaderVariantAsset->GetStableId() == RootShaderVariantStableId, - "Was expecting to update the root variant"); - GetCurrentShaderApiData().m_rootShaderVariantAsset = asset; - - ShaderReloadNotificationBus::Event(GetId(), &ShaderReloadNotificationBus::Events::OnShaderAssetReinitialized, Data::Asset{ this, AZ::Data::AssetLoadBehavior::PreLoad } ); + ShaderReloadDebugTracker::ScopedSection reloadSection("{%p}->ShaderAsset::OnAssetReady %s", this, asset.GetHint().c_str()); + ReinitializeRootShaderVariant(asset); } /////////////////////////////////////////////////////////////////////// - + /////////////////////////////////////////////////////////////////// /// ShaderVariantFinderNotificationBus overrides void ShaderAsset::OnShaderVariantTreeAssetReady(Data::Asset shaderVariantTreeAsset, bool isError) @@ -425,25 +444,13 @@ namespace AZ { if (Base::LoadAssetData(asset, stream, assetLoadFilterCB) == Data::AssetHandler::LoadResult::LoadComplete) { - return PostLoadInit(asset); - } - return Data::AssetHandler::LoadResult::Error; - } - - Data::AssetHandler::LoadResult ShaderAssetHandler::PostLoadInit(const Data::Asset& asset) - { - if (ShaderAsset* shaderAsset = asset.GetAs()) - { - if (!shaderAsset->FinalizeAfterLoad()) - { - AZ_Error("ShaderAssetHandler", false, "Shader asset failed to finalize."); - return Data::AssetHandler::LoadResult::Error; - } + asset.GetAs()->AssetInitBus::Handler::BusConnect(); return Data::AssetHandler::LoadResult::LoadComplete; } + return Data::AssetHandler::LoadResult::Error; } - + /////////////////////////////////////////////////////////////////////// } // namespace RPI diff --git a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Shader/ShaderAssetCreator.cpp b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Shader/ShaderAssetCreator.cpp index 2e2be18e0a..3f698854df 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Shader/ShaderAssetCreator.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Shader/ShaderAssetCreator.cpp @@ -218,7 +218,7 @@ namespace AZ return false; } - if (!m_asset->FinalizeAfterLoad()) + if (!m_asset->PostLoadInit()) { ReportError("Failed to finalize the ShaderAsset."); return false; diff --git a/Gems/Atom/RPI/Code/atom_rpi_public_files.cmake b/Gems/Atom/RPI/Code/atom_rpi_public_files.cmake index a1d98bbd38..c70e0bfaa3 100644 --- a/Gems/Atom/RPI/Code/atom_rpi_public_files.cmake +++ b/Gems/Atom/RPI/Code/atom_rpi_public_files.cmake @@ -10,6 +10,7 @@ # set(FILES + Include/Atom/RPI.Public/AssetInitBus.h Include/Atom/RPI.Public/Base.h Include/Atom/RPI.Public/Culling.h Include/Atom/RPI.Public/FeatureProcessor.h From 56cf345dba9328e4319c6b7367ac72a8f4339e74 Mon Sep 17 00:00:00 2001 From: Chris Santora Date: Tue, 15 Jun 2021 00:11:43 -0700 Subject: [PATCH 2/4] Fixed an issue where vulkan shaders didn't work in AtomSampleViewer because the correct API data wasn't being selected. --- .../Atom/RPI.Reflect/Shader/ShaderAsset.h | 5 ++++ .../Source/RPI.Reflect/Shader/ShaderAsset.cpp | 23 ++++++++++++++++--- .../RPI.Reflect/Shader/ShaderAssetCreator.cpp | 2 +- 3 files changed, 26 insertions(+), 4 deletions(-) diff --git a/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Shader/ShaderAsset.h b/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Shader/ShaderAsset.h index 55d10717a7..e2926c2eab 100644 --- a/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Shader/ShaderAsset.h +++ b/Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Shader/ShaderAsset.h @@ -172,6 +172,11 @@ namespace AZ bool PostLoadInit() override; void SetReady(); + + //! SelectShaderApiData() must be called before most other ShaderAsset functions. + bool SelectShaderApiData(); + + //! Returns the active ShaderApiDataContainer which was selected in SelectShaderApiData(). ShaderApiDataContainer& GetCurrentShaderApiData(); const ShaderApiDataContainer& GetCurrentShaderApiData() const; diff --git a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Shader/ShaderAsset.cpp b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Shader/ShaderAsset.cpp index e08cf425d7..b4c041b763 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Shader/ShaderAsset.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Shader/ShaderAsset.cpp @@ -332,8 +332,8 @@ namespace AZ // We may only endup here when running in a Builder context. return m_perAPIShaderData[0]; } - - bool ShaderAsset::PostLoadInit() + + bool ShaderAsset::SelectShaderApiData() { // Use the current RHI that is active to select which shader data to use. // We don't assert if the Factory is not available because this method could be called during build time, @@ -371,6 +371,11 @@ namespace AZ } } + return true; + } + + bool ShaderAsset::PostLoadInit() + { // Once the ShaderAsset is loaded, it is necessary to listen for changes in the Root Variant Asset. Data::AssetBus::Handler::BusConnect(GetRootVariant().GetId()); ShaderVariantFinderNotificationBus::Handler::BusConnect(GetId()); @@ -444,7 +449,19 @@ namespace AZ { if (Base::LoadAssetData(asset, stream, assetLoadFilterCB) == Data::AssetHandler::LoadResult::LoadComplete) { - asset.GetAs()->AssetInitBus::Handler::BusConnect(); + ShaderAsset* shaderAsset = asset.GetAs(); + + // The shader API selection must occur immediately ofter loading, on the same thread, rather than + // deferring to AssetInitBus::PostLoadInit. Many functions in the ShaderAsset class are invalid + // until after SelectShaderApiData() is called and some client code may need to access data in + // the ShaderAsset before then. + if (!shaderAsset->SelectShaderApiData()) + { + return Data::AssetHandler::LoadResult::Error; + } + + shaderAsset->AssetInitBus::Handler::BusConnect(); + return Data::AssetHandler::LoadResult::LoadComplete; } diff --git a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Shader/ShaderAssetCreator.cpp b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Shader/ShaderAssetCreator.cpp index 3f698854df..ce912c2a10 100644 --- a/Gems/Atom/RPI/Code/Source/RPI.Reflect/Shader/ShaderAssetCreator.cpp +++ b/Gems/Atom/RPI/Code/Source/RPI.Reflect/Shader/ShaderAssetCreator.cpp @@ -218,7 +218,7 @@ namespace AZ return false; } - if (!m_asset->PostLoadInit()) + if (!m_asset->SelectShaderApiData()) { ReportError("Failed to finalize the ShaderAsset."); return false; From 03171fee54b5e7f71160ba60c6c05d03d8c88246 Mon Sep 17 00:00:00 2001 From: Chris Santora Date: Tue, 15 Jun 2021 12:17:44 -0700 Subject: [PATCH 3/4] Updated AssetInitBus comments. --- Gems/Atom/RPI/Code/Include/Atom/RPI.Public/AssetInitBus.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Gems/Atom/RPI/Code/Include/Atom/RPI.Public/AssetInitBus.h b/Gems/Atom/RPI/Code/Include/Atom/RPI.Public/AssetInitBus.h index 8f49aec929..e8a1d69ece 100644 --- a/Gems/Atom/RPI/Code/Include/Atom/RPI.Public/AssetInitBus.h +++ b/Gems/Atom/RPI/Code/Include/Atom/RPI.Public/AssetInitBus.h @@ -21,6 +21,8 @@ namespace AZ //! Bus for post-load initialization of assets. //! Assets that need to do post-load initialization should connect to this bus in their asset handler's LoadAssetData() function. //! Be sure to disconnect from this bus as soon as initialization is complete, as it will be called every frame. + //! (Note this bus is needed rather than utilizing TickBus because TickBus is not protected by a mutex which means it can't be + //! connected on an asset load job thread). class AssetInitEvents : public EBusTraits { From 570f4f5f360be4e4691aab354ffea028a6dd6d08 Mon Sep 17 00:00:00 2001 From: Chris Santora Date: Tue, 15 Jun 2021 17:00:30 -0700 Subject: [PATCH 4/4] Fixed unit test compile issue --- Gems/Atom/RPI/Code/Tests/Shader/ShaderTests.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Gems/Atom/RPI/Code/Tests/Shader/ShaderTests.cpp b/Gems/Atom/RPI/Code/Tests/Shader/ShaderTests.cpp index 7a3fe2454e..f3c68ac7f6 100644 --- a/Gems/Atom/RPI/Code/Tests/Shader/ShaderTests.cpp +++ b/Gems/Atom/RPI/Code/Tests/Shader/ShaderTests.cpp @@ -56,7 +56,7 @@ namespace AZ AZ::Data::Asset SerializeInHelper(const AZ::Data::AssetId& assetId) { AZ::Data::Asset asset = Base::SerializeIn(assetId); - asset->FinalizeAfterLoad(); + asset->SelectShaderApiData(); asset->SetReady(); return asset; }