From e69b0ff59add62a248d973dd42a6d97bc69b929b Mon Sep 17 00:00:00 2001 From: MatejSakmary Date: Sun, 10 Mar 2024 19:26:34 +0100 Subject: [PATCH] Add fixes to issues raised in pr review: - removed dynamic_rendering_attachment - added declare_dynamic(_for) functions to avk::attachment which create dynamic rendering attachments - changed graphics_pipeline_t::renderpass_pointer() back to renderpass_reference() which now returns a reference wrapper --- README.md | 2 +- include/avk/attachment.hpp | 57 +++++++--------- include/avk/avk.hpp | 20 ++++-- include/avk/graphics_pipeline.hpp | 6 +- include/avk/graphics_pipeline_config.hpp | 15 +---- src/avk.cpp | 84 +++++++++++++----------- 6 files changed, 88 insertions(+), 96 deletions(-) diff --git a/README.md b/README.md index 0a35544..7cc489a 100644 --- a/README.md +++ b/README.md @@ -79,7 +79,7 @@ avk::framebuffer framebuffer = myRoot.create_framebuffer(...); avk::semaphore imageAvailableSemaphore = ...; mRoot.record({ - avk::command::render_pass(*graphicsPipeline->renderpass_pointer().value(), framebuffer.as_reference(), { + avk::command::render_pass(graphicsPipeline->renderpass_reference().value(), framebuffer.as_reference(), { avk::command::bind_pipeline(graphicsPipeline.as_reference()), avk::command::draw(3u, 1u, 0u, 0u) }) diff --git a/include/avk/attachment.hpp b/include/avk/attachment.hpp index 74fbf93..71e9d3e 100644 --- a/include/avk/attachment.hpp +++ b/include/avk/attachment.hpp @@ -3,39 +3,6 @@ namespace avk { - - /** Describes a dynamic rendering attachment. It can only be used with pipeline that has dynamic rendering enabled. - * It has fewever parameters than regular attachment since some of its values (load/store ops etc...) are set - * when starting the dynamic render pass as opposed to being declared beforehand. - * It can describe color attachments as well as depth/stencil attachments - * and holds some additional config parameters for these attachments. - */ - struct dynamic_rendering_attachment - { - /** Declare multisampled format of an attachment for a dynamic rendering pipeline - * @param aFormatAndSamples Multisampled format definition: A tuple with the format of the attachment in its first element, and with the number of samples in its second element. - */ - static dynamic_rendering_attachment declare(std::tuple aFormatAndSamples); - /** Declare format of an attachment for a dynamic rendering pipeline - * @param aFormat The format of the attachment - */ - static dynamic_rendering_attachment declare(vk::Format aFormat); - - /** Declare format of an attachment for a dynamic rendering pipeline - * @param aImageView The format of the attachment is copied from the given image view. - */ - static dynamic_rendering_attachment declare_for(const image_view_t& aImageView); - - /** The color/depth/stencil format of the attachment */ - auto format() const { return mFormat; } - /** True if the sample count is greater than 1 */ - bool is_multisampled() const { return mSampleCount != vk::SampleCountFlagBits::e1; } - /** The sample count for this attachment. */ - auto sample_count() const { return mSampleCount; } - - vk::Format mFormat; - vk::SampleCountFlagBits mSampleCount; - }; /** Describes an attachment to a framebuffer or a renderpass. * It can describe color attachments as well as depth/stencil attachments * and holds some additional config parameters for these attachments. @@ -93,6 +60,27 @@ namespace avk */ static attachment declare_for(const image_view_t& aImageView, attachment_load_config aLoadOp, subpass_usages aUsageInSubpasses, attachment_store_config aStoreOp); + /** Declare multisampled format of an attachment for a dynamic rendering pipeline. This attachment can only be used with pipeline that has dynamic rendering enabled. + * It has fewever parameters than regular attachment since some of its values (load/store ops etc...) are set when starting the dynamic render pass + * as opposed to being declared beforehand.. + * @param aFormatAndSamples Multisampled format definition: A tuple with the format of the attachment in its first element, and with the number of samples in its second element. + */ + static attachment declare_dynamic(std::tuple aFormatAndSamples); + + /** Declare multisampled format of an attachment for a dynamic rendering pipeline. This attachment can only be used with pipeline that has dynamic rendering enabled. + * It has fewever parameters than regular attachment since some of its values (load/store ops etc...) are set when starting the dynamic render pass + * as opposed to being declared beforehand.. + * @param aFormat The format of the attachment + */ + static attachment declare_dynamic(vk::Format aFormat); + + /** Declare multisampled format of an attachment for a dynamic rendering pipeline. This attachment can only be used with pipeline that has dynamic rendering enabled. + * It has fewever parameters than regular attachment since some of its values (load/store ops etc...) are set when starting the dynamic render pass + * as opposed to being declared beforehand.. + * @param aImageView The format of the attachment is copied from the given image view. + */ + static attachment declare_dynamic_for(const image_view_t& aImageView); + attachment& set_clear_color(std::array aColor) { mColorClearValue = aColor; return *this; } attachment& set_depth_clear_value(float aDepthClear) { mDepthClearValue = aDepthClear; return *this; } attachment& set_stencil_clear_value(uint32_t aStencilClear) { mStencilClearValue = aStencilClear; return *this; } @@ -121,6 +109,8 @@ namespace avk auto sample_count() const { return mSampleCount; } /** True if a multisample resolve pass shall be set up. */ auto is_to_be_resolved() const { return mSubpassUsages.contains_resolve(); } + /** True if this attachment is declared for dynamic rendering pipelines ie. using one of the dynamic declare functions*/ + bool is_for_dynamic_rendering() const { return mDynamicRenderingAttachment; } /** Returns the stencil load operation */ auto get_stencil_load_op() const { return mStencilLoadOperation.value_or(mLoadOperation); } @@ -141,5 +131,6 @@ namespace avk std::array mColorClearValue; float mDepthClearValue; uint32_t mStencilClearValue; + bool mDynamicRenderingAttachment; }; } diff --git a/include/avk/avk.hpp b/include/avk/avk.hpp index 2300aca..f334344 100644 --- a/include/avk/avk.hpp +++ b/include/avk/avk.hpp @@ -762,18 +762,26 @@ namespace avk std::function alterConfigFunction; graphics_pipeline_config config; add_config(config, renderPassAttachments, alterConfigFunction, std::move(args)...); + bool hasRenderPassAttachments = false; + bool hasDynamicRenderingAttachments = false; + for(const auto & attachment : renderPassAttachments) + { + if(attachment.is_for_dynamic_rendering()) + { + hasDynamicRenderingAttachments = true; + } else { + hasRenderPassAttachments = true; + } + } const bool hasValidRenderPass = config.mRenderPassSubpass.has_value() && static_cast(std::get(*config.mRenderPassSubpass)->handle()); - const bool hasRenderPassAttachments = renderPassAttachments.size() > 0; const bool isDynamicRenderingSet = config.mDynamicRendering == avk::cfg::dynamic_rendering::enabled; - // .has_value() should be enough since I don't fill in the optional unless there was dynamic_rendering_attachment provided - const bool hasDynamicRenderingAttachments = config.mDynamicRenderingAttachments.has_value(); // Check all invalid configurations when dynamic rendering is set if (isDynamicRenderingSet ) { if(hasValidRenderPass) { throw avk::runtime_error("Dynamic rendering does not accept renderpasses! They are set dynamically during rendering!"); } - if(hasRenderPassAttachments) { throw avk::runtime_error("Usage of avk::attachment is not allowed when dynamic rendering is enabled! Use avk::dynamic_rendering_attachment instead!"); } - if(!hasDynamicRenderingAttachments) { throw avk::runtime_error("Dynamic rendering enabled but no avk::dynamic_rendering_attachments provided! Please provide at least one attachment!"); } + if(hasRenderPassAttachments) { throw avk::runtime_error("Only avk::attachments created by declare_dynamic(_for) functions are allowed when dynamic rendering is enabled!"); } + if(!hasDynamicRenderingAttachments) { throw avk::runtime_error("Dynamic rendering enabled but no avk::attachmenst created by declare_dynamic(_for) functions provided! Please provide at least one attachment!"); } } // Check all invalid configurations when normal rendering (with renderpasses) is used else @@ -785,6 +793,8 @@ namespace avk // ^ that was the sanity check. See if we have to build the renderpass from the attachments: if (hasRenderPassAttachments) { add_config(config, renderPassAttachments, alterConfigFunction, create_renderpass(std::move(renderPassAttachments))); + } else { + config.mDynamicRenderingAttachments = std::move(renderPassAttachments); } // 2. CREATE PIPELINE according to the config diff --git a/include/avk/graphics_pipeline.hpp b/include/avk/graphics_pipeline.hpp index c1f91c4..d9a97d7 100644 --- a/include/avk/graphics_pipeline.hpp +++ b/include/avk/graphics_pipeline.hpp @@ -1,5 +1,6 @@ #pragma once #include "avk/avk.hpp" +#include namespace avk { @@ -17,10 +18,9 @@ namespace avk ~graphics_pipeline_t() = default; [[nodiscard]] auto renderpass() const { return mRenderPass; } - // TODO(msakmary) I can also keep the consistent naming aka renderpass_reference() return std::optional> I feel like this is way cleaner? - [[nodiscard]] auto renderpass_pointer() const -> std::optional + [[nodiscard]] auto renderpass_reference() const -> std::optional> { - if(mRenderPass.has_value()) { return &(mRenderPass.value().get()); } + if(mRenderPass.has_value()) { return std::cref(mRenderPass.value().get()); } else { return std::nullopt; } } // TODO(msakmary) Perhaps I just return std::optional here? It would probably be more readable then declval diff --git a/include/avk/graphics_pipeline_config.hpp b/include/avk/graphics_pipeline_config.hpp index 94bb9dd..6e90519 100644 --- a/include/avk/graphics_pipeline_config.hpp +++ b/include/avk/graphics_pipeline_config.hpp @@ -615,8 +615,8 @@ namespace avk ~graphics_pipeline_config() = default; cfg::pipeline_settings mPipelineSettings; // TODO: Handle settings! - std::optional> mDynamicRenderingAttachments; std::optional> mRenderPassSubpass; + std::optional> mDynamicRenderingAttachments; std::vector mInputBindingLocations; cfg::primitive_topology mPrimitiveTopology; std::vector mShaderInfos; @@ -681,19 +681,6 @@ namespace avk add_config(aConfig, aAttachments, aFunc, std::move(args)...); } - // Add a dynamic rendering attachment which are used when dynamic_rendering is enabled for this pipeline - template - void add_config(graphics_pipeline_config& aConfig, std::vector& aAttachments, std::function& aFunc, avk::dynamic_rendering_attachment aDynAttachment, Ts... args) - { - if(aConfig.mDynamicRenderingAttachments.has_value()) - { - aConfig.mDynamicRenderingAttachments.value().push_back(aDynAttachment); - } else { - aConfig.mDynamicRenderingAttachments = std::vector{aDynAttachment}; - } - add_config(aConfig, aAttachments, aFunc, std::move(args)...); - } - // Add a renderpass attachment to the (temporary) attachments vector and later build renderpass from them template void add_config(graphics_pipeline_config& aConfig, std::vector& aAttachments, std::function& aFunc, avk::attachment aAttachment, Ts... args) diff --git a/src/avk.cpp b/src/avk.cpp index ee5dc65..fa1dd91 100644 --- a/src/avk.cpp +++ b/src/avk.cpp @@ -1418,25 +1418,6 @@ namespace avk #pragma endregion #pragma region dynamic rendering attachment definitions - dynamic_rendering_attachment dynamic_rendering_attachment::declare(std::tuple aFormatAndSamples) - { - return dynamic_rendering_attachment{ - std::get(aFormatAndSamples), - std::get(aFormatAndSamples), - }; - } - - dynamic_rendering_attachment dynamic_rendering_attachment::declare(vk::Format aFormat) - { - return declare({aFormat, vk::SampleCountFlagBits::e1}); - } - - dynamic_rendering_attachment dynamic_rendering_attachment::declare_for(const image_view_t& aImageView) - { - const auto& imageConfig = aImageView.get_image().create_info(); - const auto format = imageConfig.format; - return declare(format); - } #pragma endregion #pragma region attachment definitions @@ -1449,7 +1430,8 @@ namespace avk {}, {}, std::move(aUsageInSubpasses), { 0.0, 0.0, 0.0, 0.0 }, - 1.0f, 0u + 1.0f, 0u, + false }; } @@ -1465,6 +1447,28 @@ namespace avk auto result = declare({format, imageConfig.samples}, aLoadOp, std::move(aUsageInSubpasses), aStoreOp); return result; } + + attachment attachment::declare_dynamic(std::tuple aFormatAndSamples) + { + return attachment{ + .mFormat = std::get(aFormatAndSamples), + .mSampleCount = std::get(aFormatAndSamples), + .mSubpassUsages = subpass_usages(subpass_usage_type::create_unused()), + .mDynamicRenderingAttachment = true + }; + } + + attachment attachment::declare_dynamic(vk::Format aFormat) + { + return declare_dynamic({aFormat, vk::SampleCountFlagBits::e1}); + } + + attachment attachment::declare_dynamic_for(const image_view_t& aImageView) + { + const auto& imageConfig = aImageView.get_image().create_info(); + const auto format = imageConfig.format; + return declare_dynamic(format); + } #pragma endregion #pragma region acceleration structure definitions @@ -4378,7 +4382,7 @@ namespace avk { aPreparedPipeline.mMultisampleStateCreateInfo .setRasterizationSamples( - aPreparedPipeline.renderpass_pointer().value()->num_samples_for_subpass(aPreparedPipeline.subpass_id().value())) + aPreparedPipeline.renderpass_reference().value().get().num_samples_for_subpass(aPreparedPipeline.subpass_id().value())) .setPSampleMask(nullptr); } @@ -4393,7 +4397,7 @@ namespace avk assert(static_cast(aPreparedPipeline.layout_handle())); const void * pNext = dynamicRenderingEnabled ? &(aPreparedPipeline.mRenderingCreateInfo.value()) : nullptr; - VkRenderPass render_pass{}; + VkRenderPass render_pass = VK_NULL_HANDLE; if(!dynamicRenderingEnabled) { render_pass = (aPreparedPipeline.mRenderPass.value())->handle(); @@ -4488,7 +4492,7 @@ namespace avk const bool dynamicRenderingEnabled = aConfig.mDynamicRendering == avk::cfg::dynamic_rendering::enabled; { - if(!dynamic_rendering_enabled) + if(!dynamicRenderingEnabled) { assert(aConfig.mRenderPassSubpass.has_value()); auto [rp, sp] = std::move(aConfig.mRenderPassSubpass.value()); @@ -4678,8 +4682,8 @@ namespace avk } // Iterate over all color target attachments and set a color blending config - size_t blending_config_num; - if (!dynamic_rendering_enabled) + size_t blendingConfigNum; + if (!dynamicRenderingEnabled) { const auto & renderPassVal = result.mRenderPass.value(); if (result.subpass_id() >= renderPassVal->attachment_descriptions().size()) { @@ -4690,7 +4694,7 @@ namespace avk + std::to_string(result.subpass_id().value()) + ") indicates. I.e. the subpass index is out of bounds."); } - blending_config_num = renderPassVal->color_attachments_for_subpass(result.subpass_id().value()).size(); /////////////////// TODO: (doublecheck or) FIX this section (after renderpass refactoring) + blendingConfigNum = renderPassVal->color_attachments_for_subpass(result.subpass_id().value()).size(); /////////////////// TODO: (doublecheck or) FIX this section (after renderpass refactoring) } // Renderpasses and Subpasses are not supported when dynamic rendering is enabled // Instead we read size of the dynamic_rendering_attachments provided @@ -4698,16 +4702,16 @@ namespace avk { blendingConfigNum = 0; - for(const auto & dyn_rendering_attachment : aConfig.mDynamicRenderingAttachments.value()) + for(const auto & dynRenderingAttachment : aConfig.mDynamicRenderingAttachments.value()) { - if(!is_depth_format(dyn_rendering_attachment.format())) + if(!is_depth_format(dynRenderingAttachment.format())) { - blending_config_num++; + blendingConfigNum++; } } } - result.mBlendingConfigsForColorAttachments.reserve(blending_config_num); // Important! Otherwise the vector might realloc and .data() will become invalid! - for (size_t i = 0; i < blending_config_num; ++i) { + result.mBlendingConfigsForColorAttachments.reserve(blendingConfigNum); // Important! Otherwise the vector might realloc and .data() will become invalid! + for (size_t i = 0; i < blendingConfigNum; ++i) { // Do we have a specific blending config for color attachment i? #if defined(_MSC_VER) && _MSC_VER < 1930 auto configForI = aConfig.mColorBlendingPerAttachment @@ -4755,7 +4759,7 @@ namespace avk // TODO: Can the settings be inferred from the renderpass' color attachments (as they are right now)? If they can't, how to handle this situation? { /////////////////// TODO: FIX this section (after renderpass refactoring) vk::SampleCountFlagBits numSamples = vk::SampleCountFlagBits::e1; - if(!dynamic_rendering_enabled) + if(!dynamicRenderingEnabled) { numSamples = result.mRenderPass.value()->num_samples_for_subpass(result.subpass_id().value()); } else { @@ -4865,19 +4869,19 @@ namespace avk .setPPushConstantRanges(result.mPushConstantRanges.data()); // 15. Set Rendering info if dynamic rendering is enabled - if(dynamic_rendering_enabled) + if(dynamicRenderingEnabled) { std::vector depth_attachments; std::vector stencil_attachments; - for(const auto & dynamic_rendering_attachment : aConfig.mDynamicRenderingAttachments.value()) + for(const auto & dynamicRenderingAttachment : aConfig.mDynamicRenderingAttachments.value()) { - if(is_depth_format(dynamic_rendering_attachment.format())) + if(is_depth_format(dynamicRenderingAttachment.format())) { - depth_attachments.push_back(dynamic_rendering_attachment.format()); - } else if (is_stencil_format(dynamic_rendering_attachment.format())) { - stencil_attachments.push_back(dynamic_rendering_attachment.format()); + depth_attachments.push_back(dynamicRenderingAttachment.format()); + } else if (is_stencil_format(dynamicRenderingAttachment.format())) { + stencil_attachments.push_back(dynamicRenderingAttachment.format()); } else { - result.mDynamicRenderingColorFormats.push_back(dynamic_rendering_attachment.format()); + result.mDynamicRenderingColorFormats.push_back(dynamicRenderingAttachment.format()); } } if(depth_attachments.size() > 1) { throw avk::runtime_error("Provided multiple depth attachments! Only one is supported!"); } @@ -4895,7 +4899,7 @@ namespace avk aAlterConfigBeforeCreation(result); } - assert (aConfig.mRenderPassSubpass.has_value() || dynamic_rendering_enabled); + assert (aConfig.mRenderPassSubpass.has_value() || dynamicRenderingEnabled); rewire_config_and_create_graphics_pipeline(result); return result; }