From c2e51dc603118eb025aa95b4fcf64f44cc5cfca9 Mon Sep 17 00:00:00 2001 From: Kah Wei Date: Tue, 25 Oct 2022 14:31:28 +0800 Subject: [PATCH] Fixed deletion crash caused by Renderables --- .../Graphics/MiddleEnd/Batching/SHBatch.cpp | 74 ++++++++++++------- .../src/Graphics/MiddleEnd/Batching/SHBatch.h | 2 +- .../MiddleEnd/Interface/SHGraphicsSystem.cpp | 4 +- .../MiddleEnd/Interface/SHRenderable.cpp | 24 +++--- .../MiddleEnd/Interface/SHRenderable.h | 4 +- 5 files changed, 67 insertions(+), 41 deletions(-) diff --git a/SHADE_Engine/src/Graphics/MiddleEnd/Batching/SHBatch.cpp b/SHADE_Engine/src/Graphics/MiddleEnd/Batching/SHBatch.cpp index 40826047..24dd76c5 100644 --- a/SHADE_Engine/src/Graphics/MiddleEnd/Batching/SHBatch.cpp +++ b/SHADE_Engine/src/Graphics/MiddleEnd/Batching/SHBatch.cpp @@ -59,7 +59,7 @@ namespace SHADE } // Add renderable in - subBatch->Renderables.insert(renderable); + subBatch->Renderables.insert(renderable->GetEID()); // Also add material instance in referencedMatInstances.insert(renderable->GetMaterial()); @@ -80,27 +80,34 @@ namespace SHADE if (subBatch == subBatches.end()) return; - subBatch->Renderables.erase(renderable); + subBatch->Renderables.erase(renderable->GetEID()); // Check if other renderables in subBatches contain the same material instance bool matUnused = true; for (const auto& sb : subBatches) - for (const auto& rend : sb.Renderables) + for (const auto& rendId : sb.Renderables) { - if (rend->GetMaterial() == renderable->GetMaterial()) + auto rend = SHComponentManager::GetComponent(rendId); + if (rend) { - matUnused = false; - break; + if (rend->GetMaterial() == renderable->GetMaterial()) + { + matUnused = false; + break; + } + } + else + { + SHLOG_WARNING("[SHBatch] Entity with a missing SHRenderable found!"); } } // Material is no longer in this library, so we remove it if (matUnused) - referencedMatInstances.erase(renderable->GetMaterial()); + referencedMatInstances.erase(renderable->WasMaterialChanged() ? renderable->GetPrevMaterial() : renderable->GetMaterial()); // Mark all as dirty - for (bool& dirt : isDirty) - dirt = true; + setAllDirtyFlags(); } void SHBatch::Clear() @@ -162,9 +169,17 @@ namespace SHADE // Build CPU Buffer char* propsCurrPtr = matPropsData.get(); for (auto& subBatch : subBatches) - for (const SHRenderable* renderable : subBatch.Renderables) + for (auto rendId : subBatch.Renderables) { - renderable->GetMaterial()->ExportProperties(propsCurrPtr); + const SHRenderable* renderable = SHComponentManager::GetComponent(rendId); + if (renderable) + { + renderable->GetMaterial()->ExportProperties(propsCurrPtr); + } + else + { + SHLOG_WARNING("[SHBatch] Entity with a missing SHRenderable found!"); + } propsCurrPtr += singleMatPropAlignedSize; } @@ -188,18 +203,18 @@ namespace SHADE // Populate on the CPU for (auto& subBatch : subBatches) - for (const SHRenderable* renderable : subBatch.Renderables) + for (auto rendId : subBatch.Renderables) { // Transform - auto transform = SHComponentManager::GetComponent(renderable->GetEID()); - if (!transform) + auto transform = SHComponentManager::GetComponent(rendId); + if (transform) { - SHLOG_WARNING("[SHBatch] Entity contianing a SHRenderable with no SHTransformComponent found!"); - transformData.emplace_back(); + transformData.emplace_back(transform->GetTRS()); } else { - transformData.emplace_back(transform->GetTRS()); + SHLOG_WARNING("[SHBatch] Entity contianing a SHRenderable with no SHTransformComponent found!"); + transformData.emplace_back(); } } @@ -221,10 +236,10 @@ namespace SHADE // Populate on the CPU for (auto& subBatch : subBatches) - for (const SHRenderable* renderable : subBatch.Renderables) - { - eidData.emplace_back(renderable->GetEID()); - } + for (auto rendId : subBatch.Renderables) + { + eidData.emplace_back(rendId); + } // Transfer to GPU if (eidBuffer[frameIndex]) @@ -305,11 +320,10 @@ namespace SHADE }); // Fill in buffers (CPU) - for (const SHRenderable* renderable : subBatch.Renderables) + for (auto rendId : subBatch.Renderables) { // Transform - EntityID eid = renderable->GetEID(); - auto transform = SHComponentManager::GetComponent_s(eid); + auto transform = SHComponentManager::GetComponent_s(rendId); if (!transform) { SHLOG_WARNING("[SHBatch] Entity contianing a SHRenderable with no SHTransformComponent found!"); @@ -320,12 +334,20 @@ namespace SHADE transformData.emplace_back(transform->GetTRS()); } - eidData.emplace_back(eid); + eidData.emplace_back(rendId); // Material Properties if (!EMPTY_MAT_PROPS) { - renderable->GetMaterial()->ExportProperties(propsCurrPtr); + const SHRenderable* renderable = SHComponentManager::GetComponent(rendId); + if (renderable) + { + renderable->GetMaterial()->ExportProperties(propsCurrPtr); + } + else + { + SHLOG_WARNING("[SHBatch] Entity with a missing SHRenderable found!"); + } propsCurrPtr += singleMatPropAlignedSize; } } diff --git a/SHADE_Engine/src/Graphics/MiddleEnd/Batching/SHBatch.h b/SHADE_Engine/src/Graphics/MiddleEnd/Batching/SHBatch.h index 193fff0d..fa18fad5 100644 --- a/SHADE_Engine/src/Graphics/MiddleEnd/Batching/SHBatch.h +++ b/SHADE_Engine/src/Graphics/MiddleEnd/Batching/SHBatch.h @@ -55,7 +55,7 @@ namespace SHADE /* Data Members */ /*-----------------------------------------------------------------------------*/ Handle Mesh; - std::unordered_set Renderables; + std::unordered_set Renderables; }; /***********************************************************************************/ /*! diff --git a/SHADE_Engine/src/Graphics/MiddleEnd/Interface/SHGraphicsSystem.cpp b/SHADE_Engine/src/Graphics/MiddleEnd/Interface/SHGraphicsSystem.cpp index 7ecd92d2..4280cafd 100644 --- a/SHADE_Engine/src/Graphics/MiddleEnd/Interface/SHGraphicsSystem.cpp +++ b/SHADE_Engine/src/Graphics/MiddleEnd/Interface/SHGraphicsSystem.cpp @@ -677,10 +677,10 @@ namespace SHADE continue; // Remove from old material's SuperBatch - Handle prevMaterial = renderable.GetPrevMaterial(); + Handle prevMaterial = renderable.GetPrevMaterial(); if (prevMaterial) { - Handle oldSuperBatch = prevMaterial->GetPipeline()->GetPipelineState().GetSubpass()->GetSuperBatch(); + Handle oldSuperBatch = prevMaterial->GetBaseMaterial()->GetPipeline()->GetPipelineState().GetSubpass()->GetSuperBatch(); oldSuperBatch->Remove(&renderable); } diff --git a/SHADE_Engine/src/Graphics/MiddleEnd/Interface/SHRenderable.cpp b/SHADE_Engine/src/Graphics/MiddleEnd/Interface/SHRenderable.cpp index 0b1c1b66..9dcd391d 100644 --- a/SHADE_Engine/src/Graphics/MiddleEnd/Interface/SHRenderable.cpp +++ b/SHADE_Engine/src/Graphics/MiddleEnd/Interface/SHRenderable.cpp @@ -31,15 +31,16 @@ namespace SHADE void SHRenderable::OnDestroy() { + // Remove from SuperBatch + Handle superBatch = sharedMaterial->GetBaseMaterial()->GetPipeline()->GetPipelineState().GetSubpass()->GetSuperBatch(); + superBatch->Remove(this); + + // Free resources if (material) { material.Free(); material = {}; } - - // Remove from SuperBatch - Handle superBatch = sharedMaterial->GetBaseMaterial()->GetPipeline()->GetPipelineState().GetSubpass()->GetSuperBatch(); - superBatch->Remove(this); } /*-----------------------------------------------------------------------------------*/ @@ -51,20 +52,23 @@ namespace SHADE if (!material && sharedMaterial == materialInstance) return; + // Flag that material was changed + materialChanged = true; + // Free copies of materials if any if (material) { + oldMaterial = material; material.Free(); material = {}; } - - // Flag that material was changed - materialChanged = true; - if (sharedMaterial) - oldMaterial = sharedMaterial->GetBaseMaterial(); + else if (sharedMaterial) + { + oldMaterial = sharedMaterial; + } // Update the material - sharedMaterial = materialInstance; + sharedMaterial = materialInstance; } Handle SHRenderable::GetMaterial() const diff --git a/SHADE_Engine/src/Graphics/MiddleEnd/Interface/SHRenderable.h b/SHADE_Engine/src/Graphics/MiddleEnd/Interface/SHRenderable.h index bc885ca2..5312cf82 100644 --- a/SHADE_Engine/src/Graphics/MiddleEnd/Interface/SHRenderable.h +++ b/SHADE_Engine/src/Graphics/MiddleEnd/Interface/SHRenderable.h @@ -55,7 +55,7 @@ namespace SHADE /* Getter Functions */ /*-------------------------------------------------------------------------------*/ bool WasMaterialChanged() const noexcept { return materialChanged; } - Handle GetPrevMaterial() const noexcept { return oldMaterial; } + Handle GetPrevMaterial() const noexcept { return oldMaterial; } /*-------------------------------------------------------------------------------*/ /* Batcher Dispatcher Functions */ @@ -74,7 +74,7 @@ namespace SHADE Handle sharedMaterial; Handle material; bool materialChanged = true; - Handle oldMaterial; + Handle oldMaterial; }; }