WIP: 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: Validation layers for the new extension aren't public yet. I've made sure that the sample doesn't show any validation errors with an internal version of the layers. Unless you have access to that, please disable validation for this sample.

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

tomadamatkinson · 2024-03-21T22:00:19Z

Hey @SaschaWillems i tried locally with pre-commit run clang-format --files $(git diff main --name-only)

Did you install pre-commit to validate your commits? It will pull and run the correct version of clang-format for you

SaschaWillems · 2024-03-23T17:18:57Z

Five of the six files in your screenshot aren't even touched by this PR.

And I actually did disable pre-commit due to another problem, will try and check what happens with pre-commit enabled.

asuessenbach · 2024-03-25T11:02:50Z

samples/README.adoc

@@ -44,494 +44,3 @@ include::./extensions/README.adoc[]

 [[tooling-samples]]
 include::./tooling/README.adoc[]
-


You removed more or less the complete readme?!?

…ocal_read

SaschaWillems · 2024-03-25T19:10:54Z

@tomadamatkinson: Okay, so I need to enable pre-commit explicitly for each folder. I work on branches in separate folders. Doing that and running your command I see it changes at least one of the files from the failure.

BUT, the changes make no sense to me, e.g. this. Here it wants to remove the whole alignment of the assignments that is almost 100% similar to the other path in the define

This doesn't look correct either:

(Compare above code with the one below it wants to change)

It seems like it has troubles with code in defines.

That doesn't feel right :/

SaschaWillems · 2024-03-25T19:19:12Z

The main issue I have with pre-commit: It won't let me push my manual changes anymore with adding it's own changes. That's probably the reason I had to disable it for this branch :/

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

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 4 commits March 21, 2024 20:54

Fix documentation

5967c05

Minor cleanup

be1c22c

Desperately trying to fix clang format

29edcdf

Trying to fix CI

05509bc

asuessenbach reviewed Mar 25, 2024

View reviewed changes

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

3a86d5d

…ocal_read

SaschaWillems added 2 commits March 25, 2024 20:20

Trying to fix CI

d5e2a4b

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

6ee9bed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Add sample for dynamic rendering local read #887

WIP: Add sample for dynamic rendering local read #887

SaschaWillems commented Jan 27, 2024 •

edited

SaschaWillems commented Jan 27, 2024 •

edited

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

SaschaWillems commented Mar 19, 2024

SaschaWillems commented Mar 21, 2024 •

edited

tomadamatkinson commented Mar 21, 2024

SaschaWillems commented Mar 23, 2024 •

edited

asuessenbach Mar 25, 2024

SaschaWillems commented Mar 25, 2024 •

edited

SaschaWillems commented Mar 25, 2024

jeroenbakker-atmind commented May 7, 2024

SaschaWillems commented May 7, 2024

jeroenbakker-atmind commented May 10, 2024 •

edited

		@@ -44,494 +44,3 @@ include::./extensions/README.adoc[]

		[[tooling-samples]]
		include::./tooling/README.adoc[]

WIP: Add sample for dynamic rendering local read #887

Are you sure you want to change the base?

WIP: Add sample for dynamic rendering local read #887

Conversation

SaschaWillems commented Jan 27, 2024 • edited

Description

General Checklist:

Sample Checklist

SaschaWillems commented Jan 27, 2024 • edited

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

SaschaWillems commented Mar 19, 2024

SaschaWillems commented Mar 21, 2024 • edited

tomadamatkinson commented Mar 21, 2024

SaschaWillems commented Mar 23, 2024 • edited

asuessenbach Mar 25, 2024

Choose a reason for hiding this comment

SaschaWillems commented Mar 25, 2024 • edited

SaschaWillems commented Mar 25, 2024

jeroenbakker-atmind commented May 7, 2024

SaschaWillems commented May 7, 2024

jeroenbakker-atmind commented May 10, 2024 • edited

SaschaWillems commented Jan 27, 2024 •

edited

SaschaWillems commented Jan 27, 2024 •

edited

SaschaWillems commented Mar 19, 2024 •

edited

SaschaWillems commented Mar 21, 2024 •

edited

SaschaWillems commented Mar 23, 2024 •

edited

SaschaWillems commented Mar 25, 2024 •

edited

jeroenbakker-atmind commented May 10, 2024 •

edited