KhronosGroup / Vulkan-Samples

One stop solution for all Vulkan samples
Apache License 2.0
4.33k stars 648 forks source link

Add sample for dynamic rendering local read #887

Closed SaschaWillems closed 2 months ago

SaschaWillems commented 9 months ago

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 https://github.com/KhronosGroup/Vulkan-Samples-Assets/pull/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:

Sample Checklist

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

SaschaWillems commented 9 months ago

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 :(

SaschaWillems commented 8 months ago

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.

SaschaWillems commented 8 months ago

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.

SaschaWillems commented 8 months ago

@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 commented 8 months ago

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.

SaschaWillems commented 8 months ago

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:

image

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.

tomadamatkinson commented 8 months ago

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

image

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

SaschaWillems commented 7 months ago

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.

SaschaWillems commented 7 months ago

@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

image

This doesn't look correct either:

image

(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 commented 7 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 5 months ago

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 commented 5 months ago

@asuessenbach you can checkout the next PR https://github.com/KhronosGroup/Vulkan-Samples-Assets/pull/23

asuessenbach commented 5 months ago

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

asuessenbach commented 5 months ago

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 commented 5 months ago

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.

jeroenbakker-atmind commented 4 months ago

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

SaschaWillems commented 3 months ago

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 commented 2 months ago

Merging - 3 approvals