Vulkan-Samples icon indicating copy to clipboard operation
Vulkan-Samples copied to clipboard

Add sample for dynamic rendering local read

Open SaschaWillems opened this issue 1 year ago • 20 comments

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:

  • [x] My code follows the coding style
  • [x] I have reviewed file licenses
  • [x] I have commented any added functions (in line with Doxygen)
  • [x] I have commented any code that could be hard to understand
  • [x] My changes do not add any new compiler warnings
  • [x] My changes do not add any new validation layer errors or warnings
  • [x] I have used existing framework/helper functions where possible
  • [x] My changes do not add any regressions
  • [x] I have tested every sample to ensure everything runs correctly
  • [x] This PR describes the scope and expected impact of the changes I am making

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

  • [x] I have updated the header Copyright to reflect the current year (CI build will fail if Copyright is out of date)
  • [x] 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:

  • [x] I have tested the sample on at least one compliant Vulkan implementation
  • [x] If the sample is vendor-specific, I have tagged it appropriately
  • [x] 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 https://github.com/KhronosGroup/Vulkan-Samples-Assets/pull/23)
  • [x] For new samples, I have added a paragraph with a summary to the appropriate chapter in the samples readme
  • [x] 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
  • [x] For new samples, I have added a link to the Antora navigation so that the sample will be listed at the Vulkan documentation site

SaschaWillems avatar Jan 27 '24 11:01 SaschaWillems

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 avatar Jan 27 '24 12:01 SaschaWillems

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 avatar Feb 22 '24 07:02 SaschaWillems

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 avatar Feb 23 '24 13:02 SaschaWillems

@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 avatar Mar 19 '24 19:03 SaschaWillems

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 avatar Mar 19 '24 20:03 SaschaWillems

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.

SaschaWillems avatar Mar 21 '24 16:03 SaschaWillems

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

tomadamatkinson avatar Mar 21 '24 22:03 tomadamatkinson

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 avatar Mar 23 '24 17:03 SaschaWillems

@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 avatar Mar 25 '24 19:03 SaschaWillems

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

SaschaWillems avatar Mar 25 '24 19:03 SaschaWillems

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.

jeroenbakker-atmind avatar May 07 '24 09:05 jeroenbakker-atmind

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

SaschaWillems avatar May 07 '24 15:05 SaschaWillems

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

jeroenbakker-atmind avatar May 10 '24 12:05 jeroenbakker-atmind

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)?

asuessenbach avatar Jun 13 '24 13:06 asuessenbach

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

jeroenbakker-atmind avatar Jun 13 '24 13:06 jeroenbakker-atmind

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

asuessenbach avatar Jun 13 '24 14:06 asuessenbach

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?

asuessenbach avatar Jun 13 '24 14:06 asuessenbach

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.

SaschaWillems avatar Jun 13 '24 15:06 SaschaWillems

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

jeroenbakker-atmind avatar Jul 01 '24 15:07 jeroenbakker-atmind

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.

SaschaWillems avatar Aug 18 '24 14:08 SaschaWillems