rhi: Rework the way we handle RenderTargets

Instead of creating a RenderTarget when we create a pipeline, we now
create RenderTargets any time a frontend defines one.

This allows to align with the other type of resources that we create
before pipelines.

Add a dirty flag on QRenderTarget so that backend can easily check if a
frontend RenderTarget has changed.

Also remove the handling of QRenderTargetSelector::outputs which allows
to specify a subset of draw buffers for a given FBO. This cannot be
handled with RHI and in a more general way, removing that altogether
might simplify the API. If you share the attachments, nothing stops
you from creating several FBO to handle that case.

Fix coding style and update the unit tests.

Change-Id: I9f3623e9d7ab9d508db207f2c43cc6c1eedb4f01
Reviewed-by: Mike Krus <mike.krus@kdab.com>

8 files changed, 81 insertions, 118 deletions

diff --git a/src/plugins/renderers/rhi/managers/rhiresourcemanagers_p.h b/src/plugins/renderers/rhi/managers/rhiresourcemanagers_p.h
index 044204e21..7269bae8f 100644
--- a/src/plugins/renderers/rhi/managers/rhiresourcemanagers_p.h
+++ b/src/plugins/renderers/rhi/managers/rhiresourcemanagers_p.h
@@ -85,8 +85,6 @@ public:
 class Q_AUTOTEST_EXPORT RHIRenderTargetManager
         : public Qt3DCore::QResourceManager<RHIRenderTarget, Qt3DCore::QNodeId, Qt3DCore::NonLockingPolicy>
 {
-public:
-    QHash<RHIRenderTarget *, Qt3DCore::QNodeId> nodeIdForRHIRenderTarget;
 };
 
 class Q_AUTOTEST_EXPORT RHIShaderManager : public APIShaderManager<RHIShader>
diff --git a/src/plugins/renderers/rhi/renderer/renderer.cpp b/src/plugins/renderers/rhi/renderer/renderer.cpp
index 480c1d5fc..d1c5110fa 100644
--- a/src/plugins/renderers/rhi/renderer/renderer.cpp
+++ b/src/plugins/renderers/rhi/renderer/renderer.cpp
@@ -1113,19 +1113,29 @@ void Renderer::buildComputePipelines(RHIComputePipeline *computePipeline,
         return onFailure();
 }
 
-void Renderer::createRenderTarget(RenderView *rv, RHIRenderTarget *target)
+void Renderer::createRenderTarget(RenderTarget *target)
 {
+    const Qt3DCore::QNodeId &renderTargetId = target->peerId();
+    RHIRenderTargetManager *rhiRenderTargetManager = m_RHIResourceManagers->rhiRenderTargetManager();
+
+    Q_ASSERT(!m_RHIResourceManagers->rhiRenderTargetManager()->contains(renderTargetId));
+    RHIRenderTarget *rhiTarget = rhiRenderTargetManager->getOrCreateResource(renderTargetId);
+
     // Used in case of failure
     QVarLengthArray<QRhiResource*> resourcesToClean;
     auto cleanAllocatedResources = [&] {
-        for (auto res : resourcesToClean)
-        {
-            res->destroy(); delete res;
+        for (auto res : resourcesToClean) {
+            res->destroy();
+            delete res;
         }
     };
 
-    auto &texman = *rhiResourceManagers()->rhiTextureManager();
-    auto &pack = rv->attachmentPack();
+    RHITextureManager *texman = rhiResourceManagers()->rhiTextureManager();
+    // TO DO: We use all render targets and ignore the fact that
+    // QRenderTargetSelector can specify a subset of outputs
+    // -> We should propably remove that from the frontend API
+    // as it's hard to handle for us and very unlikely anyone uses that
+    const AttachmentPack pack = AttachmentPack(target, m_nodesManager->attachmentManager());
     QRhiTextureRenderTargetDescription desc;
 
     QSize targetSize{};
@@ -1135,51 +1145,46 @@ void Renderer::createRenderTarget(RenderView *rv, RHIRenderTarget *target)
     bool hasDepthTexture = false;
 
     // Look up attachments to populate the RT description
-    for (const Attachment &attachment : pack.attachments())
-    {
-        RHITexture *tex = texman.lookupResource(attachment.m_textureUuid);
-        if (tex && tex->getRhiTexture())
-        {
+    // Attachments are sorted by attachment point (Color0 is first)
+    for (const Attachment &attachment : pack.attachments()) {
+        RHITexture *tex = texman->lookupResource(attachment.m_textureUuid);
+        if (tex && tex->getRhiTexture()) {
+
             auto rhiTex = tex->getRhiTexture();
             if (!rhiTex->flags().testFlag(QRhiTexture::RenderTarget)) {
                 rhiTex->setFlags(rhiTex->flags() | QRhiTexture::RenderTarget);
                 rhiTex->create();
             }
-            switch (rhiTex->format())
-            {
+            switch (rhiTex->format()) {
             case QRhiTexture::Format::D16:
             case QRhiTexture::Format::D24:
             case QRhiTexture::Format::D24S8:
-            case QRhiTexture::Format::D32F:
-            {
+            case QRhiTexture::Format::D32F: {
                 desc.setDepthTexture(rhiTex);
                 targetSize = tex->size();
                 hasDepthTexture = true;
                 break;
             }
-            default:
-            {
+            default: {
                 QRhiColorAttachment rhiAtt{rhiTex};
                 // TODO handle cubemap face
+                targetSize = tex->size();
+                targetSamples = tex->properties().samples;
+
                 rhiAtt.setLayer(attachment.m_layer);
                 rhiAtt.setLevel(attachment.m_mipLevel);
                 rhiAttachments.push_back(rhiAtt);
 
-                targetSize = tex->size();
-                targetSamples = tex->properties().samples;
                 break;
             }
             }
-        }
-        else
-        {
+        } else {
             cleanAllocatedResources();
             return;
         }
     }
 
-    if (targetSize.width() <= 0 || targetSize.height() <= 0)
-    {
+    if (targetSize.width() <= 0 || targetSize.height() <= 0) {
         cleanAllocatedResources();
         return;
     }
@@ -1188,8 +1193,7 @@ void Renderer::createRenderTarget(RenderView *rv, RHIRenderTarget *target)
 
     // Potentially create a depth & stencil renderbuffer
     QRhiRenderBuffer *ds{};
-    if (!hasDepthTexture)
-    {
+    if (!hasDepthTexture) {
       ds = m_submissionContext->rhi()->newRenderBuffer(QRhiRenderBuffer::DepthStencil, targetSize, targetSamples);
       resourcesToClean << ds;
 
@@ -1211,12 +1215,13 @@ void Renderer::createRenderTarget(RenderView *rv, RHIRenderTarget *target)
 
     if (!rt->create()) {
         cleanAllocatedResources();
+        rhiRenderTargetManager->releaseResource(renderTargetId);
         return;
     }
 
-    target->renderTarget = rt;
-    target->renderPassDescriptor = rp;
-    target->depthStencilBuffer = ds;
+    rhiTarget->renderTarget = rt;
+    rhiTarget->renderPassDescriptor = rp;
+    rhiTarget->depthStencilBuffer = ds;
 }
 
 bool Renderer::setupRenderTarget(RenderView *rv,
@@ -1231,26 +1236,10 @@ bool Renderer::setupRenderTarget(RenderView *rv,
     auto *renderTarget = renderTargetManager.lookupResource(rv->renderTargetId());
     if (renderTarget) {
         // Render to texture
-        RHIRenderTarget *rhiTarget{};
-        {
-            const auto &renderTargetId = renderTarget->peerId();
-            auto rhiRenderTargetManager = m_RHIResourceManagers->rhiRenderTargetManager();
-            rhiTarget = rhiRenderTargetManager->lookupResource(renderTargetId);
-
-            // No RHIRenderTarget associated yet -> create it
-            if (rhiTarget == nullptr) {
-                rhiTarget = rhiRenderTargetManager->getOrCreateResource(renderTargetId);
-
-                createRenderTarget(rv, rhiTarget);
-
-                // In case of error during creation renderTarget won't be set.
-                if (!rhiTarget->renderTarget) {
-                    rhiRenderTargetManager->releaseResource(renderTargetId);
-                    return false;
-                }
-                rhiRenderTargetManager->nodeIdForRHIRenderTarget.insert(rhiTarget, renderTargetId);
-            }
-        }
+        const Qt3DCore::QNodeId &renderTargetId = renderTarget->peerId();
+        RHIRenderTargetManager *rhiRenderTargetManager = m_RHIResourceManagers->rhiRenderTargetManager();
+        RHIRenderTarget *rhiTarget = rhiRenderTargetManager->lookupResource(renderTargetId);
+        Q_ASSERT(rhiTarget);
         rhiPipeline->setRenderPassDescriptor(rhiTarget->renderPassDescriptor);
         rhiPipeline->setSampleCount(rhiTarget->renderTarget->sampleCount());
         return true;
@@ -1268,6 +1257,7 @@ bool Renderer::setupRenderTarget(RenderView *rv,
         return true;
     }
 }
+
 // When this function is called, we must not be processing the commands for frame n+1
 std::vector<Renderer::RHIPassInfo>
 Renderer::prepareCommandsSubmission(const std::vector<RenderView *> &renderViews)
@@ -1324,13 +1314,17 @@ Renderer::prepareCommandsSubmission(const std::vector<RenderView *> &renderViews
         bucket.rvs = std::move(sameRenderTargetRVs);
         bucket.surface = refRV->surface();
         bucket.renderTargetId = refRV->renderTargetId();
-        bucket.attachmentPack = refRV->attachmentPack();
         rhiPassesInfo.push_back(bucket);
     }
 
     // Assign a Graphics Pipeline to each RenderCommand
     for (size_t i = 0; i < renderViewCount; ++i) {
         RenderView *rv = renderViews[i];
+
+        // Handle BlitFrameBufferCase
+        if (rv->hasBlitFramebufferInfo())
+            qWarning(Backend) << "The RHI backend doesn't support Blit operations. Instead, we recommend drawing a full screen quad with a custom shader and resolving manually.";
+
         rv->forEachCommand([&] (RenderCommand &command) {
             // Update/Create GraphicsPipelines
             if (command.m_type == RenderCommand::Draw) {
@@ -1656,6 +1650,7 @@ bool Renderer::prepareGeometryInputBindings(const Geometry *geometry, const RHIS
             case QAttribute::Double:
                 return 8;
             }
+            return 0;
         };
 
         uint byteStride = attrib->byteStride();
@@ -1844,27 +1839,42 @@ void Renderer::updateResources()
         const std::vector<HTarget> &activeHandles = renderTargetManager->activeHandles();
         for (const HTarget &hTarget : activeHandles) {
             const Qt3DCore::QNodeId renderTargetId = hTarget->peerId();
-            const Qt3DCore::QNodeIdVector &attachmentIds = hTarget->renderOutputs();
-            for (const Qt3DCore::QNodeId &attachmentId : attachmentIds) {
-                RenderTargetOutput *output = attachmentManager->lookupResource(attachmentId);
-
-                auto it = std::find_if(m_updatedTextureProperties.begin(),
-                                       m_updatedTextureProperties.end(),
-                                       [&output] (const QPair<Texture::TextureUpdateInfo, Qt3DCore::QNodeIdVector> &updateData) {
-                    const Qt3DCore::QNodeIdVector &referencedTextureIds = updateData.second;
-                    return referencedTextureIds.contains(output->textureUuid());
-                });
-                // Attachment references a texture which was update
+            bool isDirty = hTarget->isDirty() || !rhiRenderTargetManager->contains(renderTargetId);
+
+            // Check dirtiness of attachments if RenderTarget is not dirty
+            if (!isDirty) {
+                const Qt3DCore::QNodeIdVector &attachmentIds = hTarget->renderOutputs();
+                for (const Qt3DCore::QNodeId &attachmentId : attachmentIds) {
+                    RenderTargetOutput *output = attachmentManager->lookupResource(attachmentId);
+
+                    auto it = std::find_if(m_updatedTextureProperties.begin(),
+                                           m_updatedTextureProperties.end(),
+                                           [&output] (const QPair<Texture::TextureUpdateInfo, Qt3DCore::QNodeIdVector> &updateData) {
+                        const Qt3DCore::QNodeIdVector &referencedTextureIds = updateData.second;
+                        return referencedTextureIds.contains(output->textureUuid());
+                    });
+                    // Attachment references a texture which was updated
+                    isDirty = (it != m_updatedTextureProperties.end());
+                }
+            }
+
+            if (isDirty) {
+                hTarget->unsetDirty();
                 // We need to destroy the render target and the pipelines associated with it
                 // so that they can be recreated
-                if (it != m_updatedTextureProperties.end()) {
-                    graphicsPipelineManager->releasePipelinesReferencingRenderTarget(renderTargetId);
-                    rhiRenderTargetManager->releaseResource(renderTargetId);
-                }
+                // If the RT was never created, the 2 lines below are noop
+                graphicsPipelineManager->releasePipelinesReferencingRenderTarget(renderTargetId);
+                rhiRenderTargetManager->releaseResource(renderTargetId);
+
+                // Create RenderTarget
+                createRenderTarget(hTarget.data());
             }
         }
     }
 
+
+
+
     // Record list of buffer that might need uploading
     lookForDownloadableBuffers();
 }
diff --git a/src/plugins/renderers/rhi/renderer/renderer_p.h b/src/plugins/renderers/rhi/renderer/renderer_p.h
index 5f5c3ea12..d0d06c7b4 100644
--- a/src/plugins/renderers/rhi/renderer/renderer_p.h
+++ b/src/plugins/renderers/rhi/renderer/renderer_p.h
@@ -255,16 +255,12 @@ public:
     void cleanupTexture(Qt3DCore::QNodeId cleanedUpTextureId);
     void cleanupShader(const Shader *shader);
     void downloadGLBuffers();
-    void blitFramebuffer(Qt3DCore::QNodeId inputRenderTargetId,
-                         Qt3DCore::QNodeId outputRenderTargetId, QRect inputRect, QRect outputRect,
-                         GLuint defaultFramebuffer);
 
     struct RHIPassInfo
     {
         std::vector<RenderView *> rvs;
         QSurface *surface = nullptr;
         Qt3DCore::QNodeId renderTargetId;
-        AttachmentPack attachmentPack;
     };
 
     std::vector<RHIPassInfo> prepareCommandsSubmission(const std::vector<RenderView *> &renderViews);
@@ -440,7 +436,7 @@ private:
 
     void cleanupRenderTarget(const Qt3DCore::QNodeId &renderTarget);
 
-    void createRenderTarget(RenderView* rv, RHIRenderTarget *);
+    void createRenderTarget(RenderTarget *);
     bool setupRenderTarget(RenderView* rv, RHIGraphicsPipeline* graphicsPipeline, QRhiSwapChain* swapchain);
 
     bool uploadBuffersForCommand(QRhiCommandBuffer *cb, const RenderView *rv,
diff --git a/src/plugins/renderers/rhi/renderer/renderview.cpp b/src/plugins/renderers/rhi/renderer/renderview.cpp
index 9742ebbdd..d9adcc9c4 100644
--- a/src/plugins/renderers/rhi/renderer/renderview.cpp
+++ b/src/plugins/renderers/rhi/renderer/renderview.cpp
@@ -221,19 +221,13 @@ void RenderView::setRenderViewConfigFromFrameGraphLeafNode(RenderView *rv, const
                 const RenderTargetSelector *targetSelector =
                         static_cast<const RenderTargetSelector *>(node);
                 Qt3DCore::QNodeId renderTargetUid = targetSelector->renderTargetUuid();
-                HTarget renderTargetHandle =
-                        manager->renderTargetManager()->lookupHandle(renderTargetUid);
 
-                // Add renderTarget Handle and build renderCommand AttachmentPack
+                // Note: we ignore the render target outputs the RenderTargetSelector
+                // might specify as we can't handle that with RHI
+
+                // Add renderTarget Handle
                 if (!rv->renderTargetId()) {
                     rv->setRenderTargetId(renderTargetUid);
-
-                    RenderTarget *renderTarget =
-                            manager->renderTargetManager()->data(renderTargetHandle);
-                    if (renderTarget)
-                        rv->setAttachmentPack(AttachmentPack(renderTarget,
-                                                             manager->attachmentManager(),
-                                                             targetSelector->outputs()));
                 }
                 break;
             }
diff --git a/src/plugins/renderers/rhi/renderer/renderview_p.h b/src/plugins/renderers/rhi/renderer/renderview_p.h
index 29831e51e..7afbcbeec 100644
--- a/src/plugins/renderers/rhi/renderer/renderview_p.h
+++ b/src/plugins/renderers/rhi/renderer/renderview_p.h
@@ -250,9 +250,6 @@ public:
 
     void updateRenderCommand(const EntityRenderCommandDataSubView &subView);
 
-    void setAttachmentPack(const AttachmentPack &pack) { m_attachmentPack = pack; }
-    const AttachmentPack &attachmentPack() const { return m_attachmentPack; }
-
     void setRenderTargetId(Qt3DCore::QNodeId renderTargetId) Q_DECL_NOTHROW { m_renderTarget = renderTargetId; }
     Qt3DCore::QNodeId renderTargetId() const Q_DECL_NOTHROW { return m_renderTarget; }
 
diff --git a/src/plugins/renderers/rhi/renderer/renderviewbuilder.cpp b/src/plugins/renderers/rhi/renderer/renderviewbuilder.cpp
index 74b19b069..9b78c3136 100644
--- a/src/plugins/renderers/rhi/renderer/renderviewbuilder.cpp
+++ b/src/plugins/renderers/rhi/renderer/renderviewbuilder.cpp
@@ -439,27 +439,6 @@ private:
     RebuildFlagSet m_rebuildFlags;
 };
 
-class SetClearDrawBufferIndex
-{
-public:
-    explicit SetClearDrawBufferIndex(const RenderViewInitializerJobPtr &renderViewJob)
-        : m_renderViewJob(renderViewJob)
-    {}
-
-    void operator()()
-    {
-        RenderView *rv = m_renderViewJob->renderView();
-        std::vector<ClearBufferInfo> &clearBuffersInfo = rv->specificClearColorBufferInfo();
-        const AttachmentPack &attachmentPack = rv->attachmentPack();
-        for (ClearBufferInfo &clearBufferInfo : clearBuffersInfo)
-            clearBufferInfo.drawBufferIndex = attachmentPack.getDrawBufferIndex(clearBufferInfo.attchmentPoint);
-
-    }
-
-private:
-    RenderViewInitializerJobPtr m_renderViewJob;
-};
-
 class SyncFilterEntityByLayer
 {
 public:
@@ -533,7 +512,6 @@ RenderViewBuilder::RenderViewBuilder(Render::FrameGraphNode *leafNode, int rende
     , m_filterEntityByLayerJob()
     , m_frustumCullingJob(new Render::FrustumCullingJob())
     , m_syncPreFrustumCullingJob(CreateSynchronizerJobPtr(SyncPreFrustumCulling(m_renderViewJob, m_frustumCullingJob), JobTypes::SyncFrustumCulling))
-    , m_setClearDrawBufferIndexJob(CreateSynchronizerJobPtr(SetClearDrawBufferIndex(m_renderViewJob), JobTypes::ClearBufferDrawIndex))
     , m_syncFilterEntityByLayerJob()
     , m_filterProximityJob(Render::FilterProximityDistanceJobPtr::create())
 {
@@ -598,11 +576,6 @@ SynchronizerJobPtr RenderViewBuilder::syncRenderViewPostCommandUpdateJob() const
     return m_syncRenderViewPostCommandUpdateJob;
 }
 
-SynchronizerJobPtr RenderViewBuilder::setClearDrawBufferIndexJob() const
-{
-    return m_setClearDrawBufferIndexJob;
-}
-
 SynchronizerJobPtr RenderViewBuilder::syncFilterEntityByLayerJob() const
 {
     return m_syncFilterEntityByLayerJob;
@@ -737,8 +710,6 @@ std::vector<Qt3DCore::QAspectJobPtr> RenderViewBuilder::buildJobHierachy() const
     m_frustumCullingJob->addDependency(expandBVJob);
     m_frustumCullingJob->addDependency(m_syncPreFrustumCullingJob);
 
-    m_setClearDrawBufferIndexJob->addDependency(m_syncRenderViewPostInitializationJob);
-
     m_syncRenderViewPostInitializationJob->addDependency(m_renderViewJob);
 
     m_filterProximityJob->addDependency(expandBVJob);
@@ -761,7 +732,6 @@ std::vector<Qt3DCore::QAspectJobPtr> RenderViewBuilder::buildJobHierachy() const
     }
 
     m_renderer->frameCleanupJob()->addDependency(m_syncRenderViewPostCommandUpdateJob);
-    m_renderer->frameCleanupJob()->addDependency(m_setClearDrawBufferIndexJob);
 
     // Add jobs
     jobs.push_back(m_renderViewJob); // Step 1
@@ -802,7 +772,6 @@ std::vector<Qt3DCore::QAspectJobPtr> RenderViewBuilder::buildJobHierachy() const
     }
     jobs.push_back(m_syncPreFrustumCullingJob); // Step 3
     jobs.push_back(m_filterProximityJob); // Step 3
-    jobs.push_back(m_setClearDrawBufferIndexJob); // Step 3
 
     if (materialCacheNeedsRebuild) {
         for (const auto &materialGatherer : m_materialGathererJobs)  {
diff --git a/src/plugins/renderers/rhi/renderer/renderviewbuilder_p.h b/src/plugins/renderers/rhi/renderer/renderviewbuilder_p.h
index db1d1b897..6883a10e0 100644
--- a/src/plugins/renderers/rhi/renderer/renderviewbuilder_p.h
+++ b/src/plugins/renderers/rhi/renderer/renderviewbuilder_p.h
@@ -97,7 +97,6 @@ public:
     SynchronizerJobPtr syncRenderViewPreCommandBuildingJob() const;
     SynchronizerJobPtr syncRenderViewPreCommandUpdateJob() const;
     SynchronizerJobPtr syncRenderViewPostCommandUpdateJob() const;
-    SynchronizerJobPtr setClearDrawBufferIndexJob() const;
     SynchronizerJobPtr syncFilterEntityByLayerJob() const;
     FilterProximityDistanceJobPtr filterProximityJob() const;
     SynchronizerJobPtr syncMaterialGathererJob() const;
@@ -142,7 +141,6 @@ private:
     SynchronizerJobPtr m_syncRenderViewPreCommandBuildingJob;
     SynchronizerJobPtr m_syncRenderViewPreCommandUpdateJob;
     SynchronizerJobPtr m_syncRenderViewPostCommandUpdateJob;
-    SynchronizerJobPtr m_setClearDrawBufferIndexJob;
     SynchronizerJobPtr m_syncFilterEntityByLayerJob;
     SynchronizerJobPtr m_syncMaterialGathererJob;
     FilterProximityDistanceJobPtr m_filterProximityJob;
diff --git a/src/plugins/renderers/rhi/renderer/rhirendertarget_p.h b/src/plugins/renderers/rhi/renderer/rhirendertarget_p.h
index ba6528231..74a85196d 100644
--- a/src/plugins/renderers/rhi/renderer/rhirendertarget_p.h
+++ b/src/plugins/renderers/rhi/renderer/rhirendertarget_p.h
@@ -53,6 +53,7 @@
 
 #include <private/qrhi_p.h>
 #include <rhihandle_types_p.h>
+#include <private/attachmentpack_p.h>
 
 QT_BEGIN_NAMESPACE