Add sample for dynamic rendering local read #887

SaschaWillems · 2024-01-27T11:47:25Z

Description

This PR adds a new sample that shows how to replace render passes + multiple sub passes with dynamic rendering and the new VK_KHR_dynamic_rendering_local_read extension. The sample contains code paths for both those setups which can be toggled via a pre-processor define.

The sample also comes with a small tutorial that explains some of the differences.

Note: This is a copy of the sample from the internal gitlab. With the extension now public, we can continue on this over here.

Note: Requires assets from KhronosGroup/Vulkan-Samples-Assets#23, so that PR needs to be merged before or right after this PR has been merged.

Tested on Windows 11 with an NVIDIA RTX 4070 and the latest public Vulkan developer driver.

General Checklist:

Please ensure the following points are checked:

Note: The Samples CI runs a number of checks including:

I have updated the header Copyright to reflect the current year (CI build will fail if Copyright is out of date)
My changes build on Windows, Linux, macOS and Android. Otherwise I have documented any exceptions

Sample Checklist

If your PR contains a new or modified sample, these further checks must be carried out in addition to the General Checklist:

I have tested the sample on at least one compliant Vulkan implementation
If the sample is vendor-specific, I have tagged it appropriately
I have stated on what implementation the sample has been tested so that others can test on different implementations and platforms
Any dependent assets have been merged and published in downstream modules (see Added new models and texture Vulkan-Samples-Assets#23)
For new samples, I have added a paragraph with a summary to the appropriate chapter in the samples readme
For new samples, I have added a tutorial README.md file to guide users through what they need to know to implement code using this feature. For example, see conditional_rendering
For new samples, I have added a link to the Antora navigation so that the sample will be listed at the Vulkan documentation site

Required for samples that don't use render passes

Initialize base color Removed assert that would trigger on some basic glTFs Refs KhronosGroup#848

Copy from internal git repository

Fix clang format

SaschaWillems · 2024-01-27T12:16:06Z

I'm having trouble with getting the clang format right. I'm on VS 2022 (latest version) and I don't know why our clangformat does fail.

It fails on this:

	VkPipelineRenderingCreateInfoKHR pipeline_rendering_create_info{VK_STRUCTURE_TYPE_PIPELINE_RENDERING_CREATE_INFO_KHR};
	pipeline_create_info.pNext = &pipeline_rendering_create_info;

Which looks totally fine to me and is the same as the already existing dynamic rendering sample. I can only imagine that our script can't cope with code inside of pre-processor blocks.

clang_format.py only works for me in WSL but it tells me that everything is fine and does not change any of the files...

I noticed that our documentation still says "clang-format 8", my WSL setup is already at 14. I'd rather not downgrade.

Any ideas @tomadamatkinson?

Maybe we can discuss this at our next meeting. Tbh. the clang format stuff is driving me mad for pretty much every sample I write :(

samples/extensions/dynamic_rendering_local_read/dynamic_rendering_local_read.cpp

asuessenbach

Some first comments...

asuessenbach · 2024-02-19T14:28:05Z

framework/api_vulkan_sample.cpp

@@ -566,7 +566,10 @@ ApiVulkanSample::~ApiVulkanSample()
 			vkDestroyDescriptorPool(device->get_handle(), descriptor_pool, nullptr);
 		}
 		destroy_command_buffers();
-		vkDestroyRenderPass(device->get_handle(), render_pass, nullptr);
+		if (render_pass != VK_NULL_HANDLE)


No need to check for (render_pass != VK_NULL_HANDLE) here, as vkDestroyRenderPass can handle that.

samples/README.adoc

samples/extensions/dynamic_rendering_local_read/README.adoc

samples/extensions/dynamic_rendering_local_read/dynamic_rendering_local_read.cpp

SaschaWillems · 2024-02-22T07:33:19Z

Just a quick note, that this code isn't 100% matching the internal version of the sample. Thanks for the feedback though, I'll take a look at it once the internal issues are solved.

asuessenbach

Any reason, you made the dynamic rendering a compile-time switch instead of a run-time switch?

samples/extensions/dynamic_rendering_local_read/dynamic_rendering_local_read.cpp

SaschaWillems · 2024-02-23T13:52:10Z

Any reason, you made the dynamic rendering a compile-time switch instead of a run-time switch?

Makes it easier to maintain the sample. The render pass path is just there so people can easily see the differences in code.

This reverts commit 7504395.

…ocal_read

Would require fixes to the framework

SaschaWillems · 2024-03-19T19:56:27Z

@tomadamatkinson : I'm struggling to get this to pass all checks. The pre-commit clang format step did something but it still fails, and I don't have a clue how to fix it. Running the clang format script manually shows some files, but nothing is actually changed.

I also don't know why the doxgen checks are failing now :(

Also no idea why Android is failing all of a sudden :(

SaschaWillems · 2024-03-19T20:05:45Z

And it looks like I made the mistake of committing and pushing files with merge conflict info still present :/

Maybe I should refrain from adding new samples until we are done with the framework rework.

framework/api_vulkan_sample.cpp

SaschaWillems · 2024-03-21T16:37:13Z

Still fails with clang format and I have no clue why. I guess it's because I'm making heavy use of compiler directives? Running the script locally lists some files, but doesn't help with the problem sadly:

This list of files doesn't match with the ones from the CI failure.

It looks like it did something, but no local changes or commit are generated.

…ocal_read

jeroenbakker-atmind · 2024-05-07T09:43:04Z

Have tried this on linux, but there is only support for this example on NVIDIA platforms at this moment. Will try to test on windows later this week.

When building I had to turn off the opencl interop example due to out of sync changes.

SaschaWillems · 2024-05-07T15:57:48Z

Sorry, totally forgot to merge main into this. I've merged and pushed the branch, so it should compile fine now.

And thanks for testing :)

jeroenbakker-atmind · 2024-05-10T12:00:51Z

Works without any issue on:

Operating system: Windows-10-10.0.19045-SP0 64 Bits
Graphics card: AMD Radeon RX 5700 ATI Technologies Inc. 24.1.1.240110

[info] Using dynamic rendering with local read
[info] Initializing Vulkan sample
[info] Vulkan debug utils enabled (VK_EXT_debug_utils)
[info] Extension VK_KHR_get_physical_device_properties2 found, enabling it
[info] Extension VK_KHR_win32_surface found, enabling it
[info] Extension VK_EXT_debug_utils found, enabling it
[info] Enabled Validation Layers:
[info] Found GPU: AMD Radeon RX 5700
[info] Selected GPU: AMD Radeon RX 5700
[info] Dedicated Allocation enabled
[info] Device supports the following requested extensions:
[info]          VK_KHR_get_memory_requirements2
[info]          VK_KHR_dedicated_allocation
[info]          VK_KHR_dynamic_rendering
[info]          VK_KHR_synchronization2
[info]          VK_KHR_dynamic_rendering_local_read       
[info]          VK_KHR_swapchain
[info] Surface supports the following surface formats:
[info]          R8G8B8A8Unorm, SrgbNonlinear
[info]          B8G8R8A8Unorm, SrgbNonlinear
[info]          R8G8B8A8Srgb, SrgbNonlinear
[info]          B8G8R8A8Srgb, SrgbNonlinear
[info] Surface supports the following present modes:
[info]          Immediate
[info]          Fifo
[info]          FifoRelaxed
[warning] (HPPSwapchain) Image extent (0, 0) not supported. Selecting (1280, 720).
[warning] (HPPSwapchain) Surface format (Undefined, SrgbNonlinear) not supported. Selecting (B8G8R8A8Srgb, SrgbNonlinear).
[info] (HPPSwapchain) Image usage flags: TransferSrc ColorAttachment
[warning] (HPPSwapchain) Composite alpha 'Inherit' not supported. Selecting 'Opaque.
[warning] (HPPSwapchain) Present mode 'Mailbox' not supported. Selecting 'Fifo'.
[info] (HPPSwapchain) Surface format selected: B8G8R8A8Srgb, SrgbNonlinear
[info] Depth format selected: VK_FORMAT_D32_SFLOAT
[info] Depth format selected: VK_FORMAT_D32_SFLOAT
[info] Depth format selected: VK_FORMAT_D32_SFLOAT
[info] Depth format selected: VK_FORMAT_D32_SFLOAT
[info] Time spent loading images: 0.000756 seconds across 16 threads.
[info] Time spent loading images: 0.000330 seconds across 16 threads.

If you need more tests on other platforms/architectures, please let me know

asuessenbach · 2024-06-13T13:28:56Z

Silly question... where do I find the assets used with this sample (scenes/subpass_scene_opaque.gltf, scenes/subpass_scene_transparent.gltf, and textures/transparent_glass_rgba.ktx)?

jeroenbakker-atmind · 2024-06-13T13:44:36Z

@asuessenbach you can checkout the next PR KhronosGroup/Vulkan-Samples-Assets#23

asuessenbach · 2024-06-13T14:35:05Z

@jeroenbakker-atmind Thanks, now it runs fine here as well: Win10, NVIDIA RTX A3000 Laptop

asuessenbach · 2024-06-13T14:54:09Z

The gui is only available when USE_DYNAMIC_RENDERING is not defined. Is that for a specific reason?

When I just prepare the gui even if USE_DYNAMIC_RENDERING is defined, I get a VL error:
Validation Error: [ VUID-VkGraphicsPipelineCreateInfo-renderPass-06055 ] | MessageID = 0xcfde2b97 | vkCreateGraphicsPipelines(): pCreateInfos[0].pColorBlendState->attachmentCount is 1, but VkPipelineRenderingCreateInfo::colorAttachmentCount is zero because the pNext chain was not included. The Vulkan spec states: If renderPass is VK_NULL_HANDLE, pColorBlendState is not dynamic, and the pipeline is being created with fragment output interface state, pColorBlendState->attachmentCount must be equal to VkPipelineRenderingCreateInfo::colorAttachmentCount (https://vulkan.lunarg.com/doc/view/1.3.283.0/windows/1.3-extensions/vkspec.html#VUID-VkGraphicsPipelineCreateInfo-renderPass-06055)
Will that be resolved and the GUI could be available with some newer validation layer?

SaschaWillems · 2024-06-13T15:04:27Z

Yes, that layer message is the reason the GUI is disabled by default. For the GUI to work with that sample I would have to do some extensive changes to the GUI framework class. I may do that once this sample has been merged in a separate PR.

IMO not correct, but the only way to get that build step running

jeroenbakker-atmind · 2024-07-01T15:48:21Z

I can still test on NVIDIA GTX 980 + 760 IIRC only the 980 will support this.

pdaniell-nv

I noticed the following running this with Vulkan SDK 1.3.290:

[error] 269944751 - VUID-RuntimeSpirv-NonWritable-06340: Validation Error: [ VUID-RuntimeSpirv-NonWritable-06340 ] Object 0: handle = 0xc25f26000000009c, type = VK_OBJECT_TYPE_SHADER_MODULE; | MessageID = 0x101707af | vkCreateGraphicsPipelines(): pCreateInfos[0].pStages[1] SPIR-V (VK_SHADER_STAGE_FRAGMENT_BIT) uses descriptor [Set 0, Binding 3, variable "LightsBuffer"] (type VK_DESCRIPTOR_TYPE_STORAGE_BUFFER or VK_DESCRIPTOR_TYPE_STORAGE_BUFFER_DYNAMIC) which is not marked with NonWritable, but fragmentStoresAndAtomics was not enabled. The Vulkan spec states: If fragmentStoresAndAtomics is not enabled, then all storage image, storage texel buffer, and storage buffer variables in the fragment stage must be decorated with the NonWritable decoration (https://vulkan.lunarg.com/doc/view/1.3.290.0/windows/1.3-extensions/vkspec.html#VUID-RuntimeSpirv-NonWritable-06340)

SaschaWillems · 2024-08-18T14:06:55Z

Thank you very much for testing. That validation message seems to have been introduced by a recent SDK. I added the readonly attribute to the buffer mentioned in that message.

marty-johnson59 · 2024-09-06T16:04:12Z

Merging - 3 approvals

PR KhronosGroup#887 predated vkb::core::CppBuffer merge, change type used to BufferC

SaschaWillems added 6 commits January 27, 2024 11:27

Updated Vulkan headers tp 1.3.276

e7e9b2d

Only destroy render pass if != null

4bad03b

Required for samples that don't use render passes

Added support for loading colors from glTF files

036fd41

Initialize base color Removed assert that would trigger on some basic glTFs Refs KhronosGroup#848

Added new sample for dynamic rendering local read

7277926

Copy from internal git repository

Added dynamic rendering local read sample to readme and docs navigation

691f845

Update copyright

286c4fb

Fix clang format

cforfang reviewed Jan 27, 2024

View reviewed changes

samples/extensions/dynamic_rendering_local_read/dynamic_rendering_local_read.cpp Outdated Show resolved Hide resolved

SaschaWillems marked this pull request as draft January 27, 2024 17:16

SaschaWillems changed the title ~~Add sample for dynamic rendering local read~~ WIP: Add sample for dynamic rendering local read Jan 27, 2024

Adjusted memory barrier

3573958

SaschaWillems added the sample This is relevant to a sample label Feb 12, 2024

asuessenbach reviewed Feb 21, 2024

View reviewed changes

asuessenbach reviewed Feb 22, 2024

View reviewed changes

samples/extensions/dynamic_rendering_local_read/dynamic_rendering_local_read.cpp Show resolved Hide resolved

SaschaWillems added 6 commits March 17, 2024 13:18

Clean up api sample base class interfaces

7504395

Revert "Clean up api sample base class interfaces"

9214e1e

This reverts commit 7504395.

Merge remote-tracking branch 'upstream/main' into dynamic_rendering_l…

ed8a8d1

…ocal_read

Adjust to recent framework changes

860036c

Clang format

efdf627

Disable GUI for dynamic rendering

227fab4

Would require fixes to the framework

Tring to fix things

8dad7eb

asuessenbach reviewed Mar 20, 2024

View reviewed changes

framework/api_vulkan_sample.cpp Outdated Show resolved Hide resolved

Correct merge error

cec4458

SaschaWillems added 2 commits March 21, 2024 17:50

Added missing break

3ffad83

Merge remote-tracking branch 'upstream/main' into dynamic_rendering_l…

a9b1523

…ocal_read

SaschaWillems added 2 commits May 7, 2024 17:43

Merge branch 'main' of https://github.com/SaschaWillems/Vulkan-Samples

fdc6969

Merge branch 'main' into dynamic_rendering_local_read

0c64397

SaschaWillems added 2 commits June 16, 2024 11:36

Merge branch 'main' into dynamic_rendering_local_read

4230301

Clang format

df6f17b

IMO not correct, but the only way to get that build step running

SaschaWillems marked this pull request as ready for review June 16, 2024 10:21

SaschaWillems changed the title ~~WIP: Add sample for dynamic rendering local read~~ Add sample for dynamic rendering local read Jun 16, 2024

gpx1000 self-requested a review July 1, 2024 15:43

asuessenbach previously approved these changes Jul 1, 2024

View reviewed changes

pdaniell-nv reviewed Aug 12, 2024

View reviewed changes

Added read-only flag to lights buffer

74a5cea

SaschaWillems dismissed asuessenbach’s stale review via 74a5cea August 18, 2024 14:06

asuessenbach approved these changes Aug 19, 2024

View reviewed changes

pdaniell-nv approved these changes Aug 26, 2024

View reviewed changes

gpx1000 approved these changes Sep 6, 2024

View reviewed changes

marty-johnson59 merged commit d0fda1b into KhronosGroup:main Sep 6, 2024
23 checks passed

brimston3 added a commit to brimston3/Vulkan-Samples that referenced this pull request Sep 6, 2024

Fix build of samples dynamic rendering local read and host image copy

2f81e3a

PR KhronosGroup#887 predated vkb::core::CppBuffer merge, change type used to BufferC

This was referenced Sep 6, 2024

Samples dynamic rendering local read and host image copy fail to compile (vkb::core::Buffer merge) #1154

Closed

Fix build of samples dynamic rendering local read and host image copy #1155

Merged

SaschaWillems deleted the dynamic_rendering_local_read branch December 18, 2024 21:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add sample for dynamic rendering local read #887

Add sample for dynamic rendering local read #887

SaschaWillems commented Jan 27, 2024 •

edited

Loading

SaschaWillems commented Jan 27, 2024 •

edited

Loading

asuessenbach left a comment

asuessenbach Feb 19, 2024

SaschaWillems commented Feb 22, 2024

asuessenbach left a comment

SaschaWillems commented Feb 23, 2024

SaschaWillems commented Mar 19, 2024 •

edited

Loading

SaschaWillems commented Mar 19, 2024

SaschaWillems commented Mar 21, 2024 •

edited

Loading

jeroenbakker-atmind commented May 7, 2024

SaschaWillems commented May 7, 2024

jeroenbakker-atmind commented May 10, 2024 •

edited

Loading

asuessenbach commented Jun 13, 2024

jeroenbakker-atmind commented Jun 13, 2024

asuessenbach commented Jun 13, 2024

asuessenbach commented Jun 13, 2024

SaschaWillems commented Jun 13, 2024

jeroenbakker-atmind commented Jul 1, 2024

pdaniell-nv left a comment

SaschaWillems commented Aug 18, 2024

marty-johnson59 commented Sep 6, 2024

Add sample for dynamic rendering local read #887

Add sample for dynamic rendering local read #887

Conversation

SaschaWillems commented Jan 27, 2024 • edited Loading

Description

General Checklist:

Sample Checklist

SaschaWillems commented Jan 27, 2024 • edited Loading

asuessenbach left a comment

Choose a reason for hiding this comment

asuessenbach Feb 19, 2024

Choose a reason for hiding this comment

SaschaWillems commented Feb 22, 2024

asuessenbach left a comment

Choose a reason for hiding this comment

SaschaWillems commented Feb 23, 2024

SaschaWillems commented Mar 19, 2024 • edited Loading

SaschaWillems commented Mar 19, 2024

SaschaWillems commented Mar 21, 2024 • edited Loading

jeroenbakker-atmind commented May 7, 2024

SaschaWillems commented May 7, 2024

jeroenbakker-atmind commented May 10, 2024 • edited Loading

asuessenbach commented Jun 13, 2024

jeroenbakker-atmind commented Jun 13, 2024

asuessenbach commented Jun 13, 2024

asuessenbach commented Jun 13, 2024

SaschaWillems commented Jun 13, 2024

jeroenbakker-atmind commented Jul 1, 2024

pdaniell-nv left a comment

Choose a reason for hiding this comment

SaschaWillems commented Aug 18, 2024

marty-johnson59 commented Sep 6, 2024

SaschaWillems commented Jan 27, 2024 •

edited

Loading

SaschaWillems commented Jan 27, 2024 •

edited

Loading

SaschaWillems commented Mar 19, 2024 •

edited

Loading

SaschaWillems commented Mar 21, 2024 •

edited

Loading

jeroenbakker-atmind commented May 10, 2024 •

edited

Loading