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

Shaders changes require two or more change cycles before updating (#5142)

Browse Source 
* Shaders changes require two or more change cycles before updating

This fixes the problem described in the title.

Consolidated the responsibility to update the root shader variant
asset into the Shader() class. It was unnecessarily spread across
Shader(), ShaderVariant() and ShaderAsset().
In particular OnAssetReloaded now makes a temporary copy of the root
ShaderVariantAsset and updates the ShaderAsset with such reference
only when OnAssetReloaded() is called on behalf of the ShaderAsset.

Signed-off-by: galibzon <66021303+galibzon@users.noreply.github.com>
This commit is contained in:
galibzon
2021-11-02 16:21:26 -05:00
committed by GitHub
parent f8aa265253
commit 6763e2a3ac
13 changed files with 80 additions and 125 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Gems/Atom/Feature/Common/Code/Source/Decals/DecalTextureArray.cpp
+1 -1
View File
@@ -78,7 +78,7 @@ namespace AZ
AZ_Warning("DecalTextureArray", false, "Material property: %s does not have a valid asset Id", propertyName.GetCStr());
return {};
}
return { imageAsset.GetAs< AZ::RPI::StreamingImageAsset>(), AZ::Data::AssetLoadBehavior::PreLoad };
return Data::static_pointer_cast<AZ::RPI::StreamingImageAsset>(imageAsset);
}
static AZ::Data::Asset<AZ::RPI::StreamingImageAsset> GetStreamingImageAsset(const AZ::Data::Asset<Data::AssetData> materialAssetData, const AZ::Name& propertyName)
Gems/Atom/RPI/Code/Include/Atom/RPI.Public/Shader/Shader.h
+7 -11
View File
@@ -52,9 +52,8 @@ namespace AZ
*/
class Shader final
: public Data::InstanceData
, public Data::AssetBus::Handler
, public Data::AssetBus::MultiHandler
, public ShaderVariantFinderNotificationBus::Handler
, public ShaderReloadNotificationBus::Handler
{
friend class ShaderSystem;
public:
@@ -165,15 +164,6 @@ namespace AZ
void OnShaderVariantTreeAssetReady(Data::Asset<ShaderVariantTreeAsset> /*shaderVariantTreeAsset*/, bool /*isError*/) override {};
void OnShaderVariantAssetReady(Data::Asset<ShaderVariantAsset> shaderVariantAsset, bool IsError) override;
///////////////////////////////////////////////////////////////////
///////////////////////////////////////////////////////////////////
// ShaderReloadNotificationBus overrides...
void OnShaderAssetReinitialized(const Data::Asset<ShaderAsset>& 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.
///////////////////////////////////////////////////////////////////
//! A strong reference to the shader asset.
Data::Asset<ShaderAsset> m_asset;
@@ -206,6 +196,12 @@ namespace AZ
//! PipelineLibrary file name
char m_pipelineLibraryPath[AZ_MAX_PATH_LEN] = { 0 };
//! During OnAssetReloaded, the internal references to ShaderVariantAsset inside
//! ShaderAsset are not updated correctly. We store here a reference to the root ShaderVariantAsset
//! when it got reloaded, later when We get OnAssetReloaded for the ShaderAsset We update its internal
//! reference to the root variant asset.
Data::Asset<ShaderVariantAsset> m_reloadedRootShaderVariantAsset;
};
}
}
Gems/Atom/RPI/Code/Include/Atom/RPI.Public/Shader/ShaderVariant.h
-4
View File
@@ -19,7 +19,6 @@ namespace AZ
//! the RHI::PipelineStateType of the parent Shader instance. For shaders on the raster
//! pipeline, the RHI::DrawFilterTag is also provided.
class ShaderVariant final
: public Data::AssetBus::MultiHandler
{
friend class Shader;
public:
@@ -58,9 +57,6 @@ namespace AZ
const Data::Asset<ShaderVariantAsset>& shaderVariantAsset,
SupervariantIndex supervariantIndex);
// AssetBus overrides...
void OnAssetReloaded(Data::Asset<Data::AssetData> asset) override;
//! A reference to the shader asset that this is a variant of.
Data::Asset<ShaderAsset> m_shaderAsset;
Gems/Atom/RPI/Code/Include/Atom/RPI.Reflect/Shader/ShaderAsset.h
+7 -10
View File
@@ -53,12 +53,12 @@ namespace AZ
class ShaderAsset final
: public Data::AssetData
, public ShaderVariantFinderNotificationBus::Handler
, public Data::AssetBus::Handler
, public AssetInitBus::Handler
{
friend class ShaderAssetCreator;
friend class ShaderAssetHandler;
friend class ShaderAssetTester;
friend class Shader;
public:
AZ_RTTI(ShaderAsset, "{823395A3-D570-49F4-99A9-D820CD1DEF98}", Data::AssetData);
static void Reflect(ReflectContext* context);
@@ -212,22 +212,19 @@ namespace AZ
return GetAttribute(shaderStage, attributeName, DefaultSupervariantIndex);
}
private:
///////////////////////////////////////////////////////////////////
/// AssetBus overrides
void OnAssetReloaded(Data::Asset<Data::AssetData> asset) override;
void OnAssetReady(Data::Asset<Data::AssetData> asset) override;
///////////////////////////////////////////////////////////////////
void ReinitializeRootShaderVariant(Data::Asset<Data::AssetData> asset);
///////////////////////////////////////////////////////////////////
/// ShaderVariantFinderNotificationBus overrides
void OnShaderVariantTreeAssetReady(Data::Asset<ShaderVariantTreeAsset> shaderVariantTreeAsset, bool isError) override;
void OnShaderVariantAssetReady(Data::Asset<ShaderVariantAsset> /*shaderVariantAsset*/, bool /*isError*/) override {};
///////////////////////////////////////////////////////////////////
// Only Shader::OnAssetReloaded() should call this function, because it is pointless for an Asset to
// to refresh its own "serialized references" to other assets during OnAssetReloaded().
// The problem is that OnAssetReloaded() doesn't do a good job at updating "serialized references" to other assets,
// So some other class must update the reference and that's why Shader() is the best class to do it.
void UpdateRootShaderVariantAsset(SupervariantIndex SupervariantIndex, Data::Asset<ShaderVariantAsset> newRootVariant);
//! A Supervariant represents a set of static shader compilation parameters.
//! Those parameters can be predefined c-preprocessor macros or specific arguments
//! for AZSLc.
Gems/Atom/RPI/Code/Source/RPI.Public/Material/Material.cpp
+2 -2
View File
@@ -234,7 +234,7 @@ namespace AZ
{
ShaderReloadDebugTracker::ScopedSection reloadSection("{%p}->Material::OnAssetReloaded %s", this, asset.GetHint().c_str());
Data::Asset<MaterialAsset> newMaterialAsset = { asset.GetAs<MaterialAsset>(), AZ::Data::AssetLoadBehavior::PreLoad };
Data::Asset<MaterialAsset> newMaterialAsset = Data::static_pointer_cast<MaterialAsset>(asset);
if (newMaterialAsset)
{
@@ -610,7 +610,7 @@ namespace AZ
}
}
if (Data::Asset<StreamingImageAsset> streamingImageAsset = { imageAsset.GetAs<StreamingImageAsset>(), AZ::Data::AssetLoadBehavior::PreLoad })
if (Data::Asset<StreamingImageAsset> streamingImageAsset = Data::static_pointer_cast<StreamingImageAsset>(imageAsset))
{
Data::Instance<Image> image = StreamingImage::FindOrCreate(streamingImageAsset);
if (!image)
Gems/Atom/RPI/Code/Source/RPI.Public/Pass/PassLibrary.cpp
+1 -1
View File
@@ -281,7 +281,7 @@ namespace AZ
void PassLibrary::OnAssetReloaded(Data::Asset<Data::AssetData> asset)
{
// Handle pass asset reload
Data::Asset<PassAsset> passAsset = { asset.GetAs<PassAsset>(), AZ::Data::AssetLoadBehavior::PreLoad };
Data::Asset<PassAsset> passAsset = Data::static_pointer_cast<PassAsset>(asset);
if (passAsset && passAsset->GetPassTemplate())
{
LoadPassAsset(passAsset->GetPassTemplate()->m_name, passAsset, true);
Gems/Atom/RPI/Code/Source/RPI.Public/Pass/Specific/ImageAttachmentPreviewPass.cpp
+1 -1
View File
@@ -231,7 +231,7 @@ namespace AZ
void ImageAttachmentPreviewPass::OnAssetReloaded(Data::Asset<Data::AssetData> asset)
{
Data::Asset<ShaderAsset> shaderAsset = { asset.GetAs<ShaderAsset>(), AZ::Data::AssetLoadBehavior::PreLoad };
Data::Asset<ShaderAsset> shaderAsset = Data::static_pointer_cast<ShaderAsset>(asset);
if (shaderAsset)
{
m_needsShaderLoad = true;
Gems/Atom/RPI/Code/Source/RPI.Public/Shader/Shader.cpp
+50 -28
View File
@@ -15,6 +15,8 @@
#include <Atom/RPI.Public/Shader/ShaderSystemInterface.h>
#include <AzCore/Interface/Interface.h>
#include <AzCore/Component/TickBus.h>
namespace AZ
{
@@ -96,8 +98,7 @@ namespace AZ
RHI::ResultCode Shader::Init(ShaderAsset& shaderAsset)
{
Data::AssetBus::Handler::BusDisconnect();
ShaderReloadNotificationBus::Handler::BusDisconnect();
Data::AssetBus::MultiHandler::BusDisconnect();
ShaderVariantFinderNotificationBus::Handler::BusDisconnect();
RHI::RHISystemInterface* rhiSystem = RHI::RHISystemInterface::Get();
@@ -112,7 +113,8 @@ namespace AZ
AZStd::unique_lock<decltype(m_variantCacheMutex)> lock(m_variantCacheMutex);
m_shaderVariants.clear();
}
m_rootVariant.Init(Data::Asset<ShaderAsset>{&shaderAsset, AZ::Data::AssetLoadBehavior::PreLoad}, shaderAsset.GetRootVariant(m_supervariantIndex), m_supervariantIndex);
auto rootShaderVariantAsset = shaderAsset.GetRootVariant(m_supervariantIndex);
m_rootVariant.Init(m_asset, rootShaderVariantAsset, m_supervariantIndex);
if (m_pipelineLibraryHandle.IsNull())
{
@@ -146,8 +148,8 @@ namespace AZ
}
ShaderVariantFinderNotificationBus::Handler::BusConnect(m_asset.GetId());
Data::AssetBus::Handler::BusConnect(m_asset.GetId());
ShaderReloadNotificationBus::Handler::BusConnect(m_asset.GetId());
Data::AssetBus::MultiHandler::BusConnect(rootShaderVariantAsset.GetId());
Data::AssetBus::MultiHandler::BusConnect(m_asset.GetId());
return RHI::ResultCode::Success;
}
@@ -155,8 +157,7 @@ namespace AZ
void Shader::Shutdown()
{
ShaderVariantFinderNotificationBus::Handler::BusDisconnect();
Data::AssetBus::Handler::BusDisconnect();
ShaderReloadNotificationBus::Handler::BusDisconnect();
Data::AssetBus::MultiHandler::BusDisconnect();
if (m_pipelineLibraryHandle.IsValid())
{
@@ -181,14 +182,52 @@ namespace AZ
{
ShaderReloadDebugTracker::ScopedSection reloadSection("{%p}->Shader::OnAssetReloaded %s", this, asset.GetHint().c_str());
if (asset->GetId() == m_asset->GetId())
if (asset.GetAs<ShaderVariantAsset>())
{
Data::Asset<ShaderAsset> newAsset = { asset.GetAs<ShaderAsset>(), AZ::Data::AssetLoadBehavior::PreLoad };
AZ_Assert(newAsset, "Reloaded ShaderAsset is null");
m_reloadedRootShaderVariantAsset = Data::static_pointer_cast<ShaderVariantAsset>(asset);
if (m_asset->m_shaderAssetBuildTimestamp == m_reloadedRootShaderVariantAsset->GetBuildTimestamp())
{
Init(*m_asset.Get());
ShaderReloadNotificationBus::Event(asset.GetId(), &ShaderReloadNotificationBus::Events::OnShaderReinitialized, *this);
}
return;
}
Init(*newAsset.Get());
if (asset.GetAs<ShaderAsset>())
{
m_asset = Data::static_pointer_cast<ShaderAsset>(asset);
if (!m_reloadedRootShaderVariantAsset.IsReady())
{
// Do nothing, as We should not re-initilize until the root shader variant asset has been reloaded.
return;
}
AZ_Assert(m_asset->m_shaderAssetBuildTimestamp == m_reloadedRootShaderVariantAsset->GetBuildTimestamp(),
"shaderAsset timeStamp=%lld, but Root ShaderVariantAsset timeStamp=%lld",
m_asset->m_shaderAssetBuildTimestamp, m_reloadedRootShaderVariantAsset->GetBuildTimestamp());
m_asset->UpdateRootShaderVariantAsset(m_supervariantIndex, m_reloadedRootShaderVariantAsset);
m_reloadedRootShaderVariantAsset = {}; // Clear the temporary reference.
if (ShaderReloadDebugTracker::IsEnabled())
{
auto makeTimeString = [](AZStd::sys_time_t timestamp, AZStd::sys_time_t now)
{
AZStd::sys_time_t elapsedMicroseconds = now - timestamp;
double elapsedSeconds = aznumeric_cast<double>(elapsedMicroseconds / 1'000'000);
AZStd::string timeString = AZStd::string::format("%lld (%f seconds ago)", timestamp, elapsedSeconds);
return timeString;
};
AZStd::sys_time_t now = AZStd::GetTimeNowMicroSecond();
const auto shaderVariantAsset = m_asset->GetRootVariant();
ShaderReloadDebugTracker::Printf("{%p}->Shader::OnAssetReloaded for shader '%s' [build time %s] found variant '%s' [build time %s]", this,
m_asset.GetHint().c_str(), makeTimeString(m_asset->m_shaderAssetBuildTimestamp, now).c_str(),
shaderVariantAsset.GetHint().c_str(), makeTimeString(shaderVariantAsset->GetBuildTimestamp(), now).c_str());
}
Init(*m_asset.Get());
ShaderReloadNotificationBus::Event(asset.GetId(), &ShaderReloadNotificationBus::Events::OnShaderReinitialized, *this);
}
}
///////////////////////////////////////////////////////////////////////
@@ -253,23 +292,6 @@ namespace AZ
ShaderReloadNotificationBus::Event(m_asset.GetId(), &ShaderReloadNotificationBus::Events::OnShaderVariantReinitialized, updatedVariant);
}
///////////////////////////////////////////////////////////////////
///////////////////////////////////////////////////////////////////
// ShaderReloadNotificationBus overrides...
void Shader::OnShaderAssetReinitialized(const Data::Asset<ShaderAsset>& shaderAsset)
{
// 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);
}
}
///////////////////////////////////////////////////////////////////
ConstPtr<RHI::PipelineLibraryData> Shader::LoadPipelineLibrary() const
{
Gems/Atom/RPI/Code/Source/RPI.Public/Shader/ShaderVariant.cpp
+5 -29
View File
@@ -22,24 +22,20 @@ namespace AZ
const Data::Asset<ShaderAsset>& shaderAsset,
const Data::Asset<ShaderVariantAsset>& shaderVariantAsset,
SupervariantIndex supervariantIndex)
{
{
m_shaderAsset = shaderAsset;
m_shaderVariantAsset = shaderVariantAsset;
m_supervariantIndex = supervariantIndex;
m_pipelineStateType = shaderAsset->GetPipelineStateType();
m_pipelineLayoutDescriptor = shaderAsset->GetPipelineLayoutDescriptor(supervariantIndex);
m_shaderVariantAsset = shaderVariantAsset;
m_renderStates = &shaderAsset->GetRenderStates(supervariantIndex);
m_supervariantIndex = supervariantIndex;
Data::AssetBus::MultiHandler::BusDisconnect();
Data::AssetBus::MultiHandler::BusConnect(shaderAsset.GetId());
Data::AssetBus::MultiHandler::BusConnect(shaderVariantAsset.GetId());
m_shaderAsset = shaderAsset;
return true;
}
ShaderVariant::~ShaderVariant()
{
Data::AssetBus::MultiHandler::BusDisconnect();
}
void ShaderVariant::ConfigurePipelineState(RHI::PipelineStateDescriptor& descriptor) const
@@ -82,25 +78,5 @@ namespace AZ
}
}
void ShaderVariant::OnAssetReloaded(Data::Asset<Data::AssetData> asset)
{
ShaderReloadDebugTracker::ScopedSection reloadSection("{%p}->ShaderVariant::OnAssetReloaded %s", this, asset.GetHint().c_str());
if (asset.GetAs<ShaderVariantAsset>())
{
Data::Asset<ShaderVariantAsset> shaderVariantAsset = { asset.GetAs<ShaderVariantAsset>(), AZ::Data::AssetLoadBehavior::PreLoad };
Init(m_shaderAsset, shaderVariantAsset, m_supervariantIndex);
ShaderReloadNotificationBus::Event(m_shaderAsset.GetId(), &ShaderReloadNotificationBus::Events::OnShaderVariantReinitialized, *this);
}
if (asset.GetAs<ShaderAsset>())
{
Data::Asset<ShaderAsset> shaderAsset = { asset.GetAs<ShaderAsset>(), AZ::Data::AssetLoadBehavior::PreLoad };
Init(shaderAsset, m_shaderVariantAsset, m_supervariantIndex);
ShaderReloadNotificationBus::Event(m_shaderAsset.GetId(), &ShaderReloadNotificationBus::Events::OnShaderVariantReinitialized, *this);
}
}
} // namespace RPI
} // namespace AZ
Gems/Atom/RPI/Code/Source/RPI.Reflect/Material/MaterialAsset.cpp
+1 -1
View File
@@ -237,7 +237,7 @@ namespace AZ
void MaterialAsset::ReinitializeMaterialTypeAsset(Data::Asset<Data::AssetData> asset)
{
Data::Asset<MaterialTypeAsset> newMaterialTypeAsset = { asset.GetAs<MaterialTypeAsset>(), AZ::Data::AssetLoadBehavior::PreLoad };
Data::Asset<MaterialTypeAsset> newMaterialTypeAsset = Data::static_pointer_cast<MaterialTypeAsset>(asset);
if (newMaterialTypeAsset)
{
Gems/Atom/RPI/Code/Source/RPI.Reflect/Shader/ShaderAsset.cpp
+3 -35
View File
@@ -108,7 +108,6 @@ namespace AZ
ShaderAsset::~ShaderAsset()
{
Data::AssetBus::Handler::BusDisconnect();
ShaderVariantFinderNotificationBus::Handler::BusDisconnect();
AssetInitBus::Handler::BusDisconnect();
}
@@ -570,46 +569,16 @@ namespace AZ
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());
AssetInitBus::Handler::BusDisconnect();
return true;
}
void ShaderAsset::ReinitializeRootShaderVariant(Data::Asset<Data::AssetData> asset)
{
Data::Asset<ShaderVariantAsset> shaderVariantAsset = { asset.GetAs<ShaderVariantAsset>(), AZ::Data::AssetLoadBehavior::PreLoad };
AZ_Assert(shaderVariantAsset->GetStableId() == RootShaderVariantStableId, "Was expecting to update the root variant");
SupervariantIndex supervariantIndex = GetSupervariantIndexFromAssetId(asset.GetId());
GetCurrentShaderApiData().m_supervariants[supervariantIndex.GetIndex()].m_rootShaderVariantAsset = asset;
ShaderReloadNotificationBus::Event(GetId(), &ShaderReloadNotificationBus::Events::OnShaderAssetReinitialized, Data::Asset<ShaderAsset>{ this, AZ::Data::AssetLoadBehavior::PreLoad } );
}
///////////////////////////////////////////////////////////////////////
// AssetBus overrides...
void ShaderAsset::OnAssetReloaded(Data::Asset<Data::AssetData> asset)
void ShaderAsset::UpdateRootShaderVariantAsset(SupervariantIndex supervariantIndex, Data::Asset<ShaderVariantAsset> newRootVariant)
{
ShaderReloadDebugTracker::ScopedSection reloadSection("{%p}->ShaderAsset::OnAssetReloaded %s", this, asset.GetHint().c_str());
ReinitializeRootShaderVariant(asset);
GetCurrentShaderApiData().m_supervariants[supervariantIndex.GetIndex()].m_rootShaderVariantAsset = newRootVariant;
}
void ShaderAsset::OnAssetReady(Data::Asset<Data::AssetData> 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.
ShaderReloadDebugTracker::ScopedSection reloadSection("{%p}->ShaderAsset::OnAssetReady %s", this, asset.GetHint().c_str());
ReinitializeRootShaderVariant(asset);
}
///////////////////////////////////////////////////////////////////////
///////////////////////////////////////////////////////////////////
/// ShaderVariantFinderNotificationBus overrides
@@ -628,7 +597,6 @@ namespace AZ
m_shaderVariantTree = shaderVariantTreeAsset;
}
lock.unlock();
ShaderReloadNotificationBus::Event(GetId(), &ShaderReloadNotificationBus::Events::OnShaderAssetReinitialized, Data::Asset<ShaderAsset>{ this, AZ::Data::AssetLoadBehavior::PreLoad });
}
///////////////////////////////////////////////////////////////////
Gems/AtomLyIntegration/CommonFeatures/Code/Source/DiffuseGlobalIllumination/EditorDiffuseProbeGridComponent.cpp
+1 -1
View File
@@ -196,7 +196,7 @@ namespace AZ
{
// bake is complete, update configuration with the new baked texture asset
AzToolsFramework::ScopedUndoBatch undoBatch("DiffuseProbeGrid Texture Bake");
configurationAsset = { textureAsset.GetAs<RPI::StreamingImageAsset>(), AZ::Data::AssetLoadBehavior::PreLoad };
configurationAsset = textureAsset;
SetDirty();
if (m_controller.m_configuration.m_bakedIrradianceTextureAsset.IsReady() &&
Gems/AtomLyIntegration/CommonFeatures/Code/Source/ReflectionProbe/EditorReflectionProbeComponent.cpp
+1 -1
View File
@@ -178,7 +178,7 @@ namespace AZ
if (notificationType == CubeMapAssetNotificationType::Ready)
{
// bake is complete, update configuration with the new baked cubemap asset
m_controller.m_configuration.m_bakedCubeMapAsset = { cubeMapAsset.GetAs<RPI::StreamingImageAsset>(), AZ::Data::AssetLoadBehavior::PreLoad };
m_controller.m_configuration.m_bakedCubeMapAsset = cubeMapAsset;
// refresh the currently rendered cubemap
m_controller.UpdateCubeMap();