-
Notifications
You must be signed in to change notification settings - Fork 657
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Dynamic state3 multisample rasterization #856
base: main
Are you sure you want to change the base?
Dynamic state3 multisample rasterization #856
Conversation
Signed-off-by: Pawel Jastrzebski <pawel.jastrzebski@mobica.com>
Signed-off-by: pawel-jastrzebski-mobica <pawel.jastrzebski@mobica.com>
Signed-off-by: pawel-jastrzebski-mobica <pawel.jastrzebski@mobica.com>
Signed-off-by: pawel-jastrzebski-mobica <pawel.jastrzebski@mobica.com>
Signed-off-by: pawel-jastrzebski-mobica <pawel.jastrzebski@mobica.com>
Signed-off-by: pawel-jastrzebski-mobica <pawel.jastrzebski@mobica.com>
Signed-off-by: pawel-jastrzebski-mobica <pawel.jastrzebski@mobica.com>
Signed-off-by: pawel-jastrzebski-mobica <pawel.jastrzebski@mobica.com>
Signed-off-by: pawel-jastrzebski-mobica <pawel.jastrzebski@mobica.com>
Signed-off-by: pawel-jastrzebski-mobica <pawel.jastrzebski@mobica.com>
Signed-off-by: pawel-jastrzebski-mobica <pawel.jastrzebski@mobica.com>
Signed-off-by: pawel-jastrzebski-mobica <pawel.jastrzebski@mobica.com>
Signed-off-by: pawel-jastrzebski-mobica <pawel.jastrzebski@mobica.com>
Signed-off-by: pawel-jastrzebski-mobica <pawel.jastrzebski@mobica.com>
Signed-off-by: pawel-jastrzebski-mobica <pawel.jastrzebski@mobica.com>
Signed-off-by: pawel-jastrzebski-mobica <pawel.jastrzebski@mobica.com>
Signed-off-by: pawel-jastrzebski-mobica <pawel.jastrzebski@mobica.com>
Signed-off-by: pawel-jastrzebski-mobica <pawel.jastrzebski@mobica.com>
Signed-off-by: pawel-jastrzebski-mobica <pawel.jastrzebski@mobica.com>
Signed-off-by: pawel-jastrzebski-mobica <pawel.jastrzebski@mobica.com>
Signed-off-by: pawel-jastrzebski-mobica <pawel.jastrzebski@mobica.com>
Signed-off-by: pawel-jastrzebski-mobica <pawel.jastrzebski@mobica.com>
Signed-off-by: pawel-jastrzebski-mobica <pawel.jastrzebski@mobica.com>
Signed-off-by: pawel-jastrzebski-mobica <pawel.jastrzebski@mobica.com>
Signed-off-by: pawel-jastrzebski-mobica <pawel.jastrzebski@mobica.com>
Signed-off-by: pawel-jastrzebski-mobica <pawel.jastrzebski@mobica.com>
Signed-off-by: pawel-jastrzebski-mobica <pawel.jastrzebski@mobica.com>
When I start this sample, I get this error message: That is, you would need to |
Thanks for letting me know about that VL error. On which platform do you test? I didn't see that issue on ubuntu, but I will add that line and please give me feedback if that resolve your VL issue. |
Strange, I can use structs from this extension without add but when I add that line I get error because of not having that extension available on my GPU. I think something is not right, because I obviously can use features of that extension without adding it to application. |
I see that on Win10, using an NVIDIA GPU, Vulkan SDK 1.3.280. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works fine on my setup (Win 11, NV RTX 4070). Validation is clean, and changing MSAA modes has a clear visual effect. 👍🏻
I only noticed a few minor issues not directly related to the rendering part itself.
Those should be easy to fix, and once done I'll approve this :)
samples/extensions/dynamic_multisample_rasterization/README.adoc
Outdated
Show resolved
Hide resolved
samples/extensions/dynamic_multisample_rasterization/dynamic_multisample_rasterization.cpp
Show resolved
Hide resolved
samples/extensions/dynamic_multisample_rasterization/dynamic_multisample_rasterization.h
Outdated
Show resolved
Hide resolved
As stated half a year ago, you nicely sort the triangles into four different buckets, but use just two different pipelines to render them: all opaque and transparent triangles are rendered with blending enabled, even though that's only needed for the transparent ones. The same holds for the flipped triangles. Besides that, I increased the camera's rotation speed by 10. |
Hi @pawel-jastrzebski-mobica, this looks close. Can you please take a look at Andreas' comments and then resolve the merge conflicts here so we can get this merged shortly? Thanks! |
I don't know if anybody is still working on this PR. |
@asuessenbach Hello and sorry for long time not responding. I saw from meeting minutes that this PR can be easy fixed and merged, so I will add your changes and resolve merge conflicts. |
Hi @pawel-jastrzebski-mobica, we were discussing how to move this one forward on the Samples call this week. We're happy to help out here if resources are limited at Mobica - if it makes sense, we could merge this as is and then make any PRs needed to fix remaining issues. Would that work for you? If so, we'd just need you to fix the merge conflicts so we can merge. LMK how you'd like to proceed. Thanks! |
20c15e3
I wanted to fix this PR fast and probably I made a mistake by merge --squash instead of normal merge, so github on my Mobica branch still is seeing 107 commits behind Khronos:main. I will work on that today after regular working hours. |
Is there a neat way to revers that merge commit? - 2d48c2e I know it is a way to use push with --force to overwrite, but this PR is close to end and I don't want to do more mess here by testing different methods for fixing it. |
bb6cd2c
to
1be1bed
Compare
I was cleaned my previous mess. PR is ready to be check :) @asuessenbach @SaschaWillems |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost there :)
Only found a few minor things. For the documentation related minor issues see my comments.
The other minor issue I found is with resizing. If you change the size of the window, the projection matrix is not updated. So in order to get that to properly adjust, you need to click into the sample (which probably updates the camera settings.
Example after resizing:
It's clearly visible, that there is something wrong with projection. If I then click into the window, the projection is corrected:
Other than that I'm happy with the sample. Hope those minor issues are easy to fix :)
|
||
To be able to use this extension in Vulkan API: | ||
`VK_EXT_extended_dynamic_state3` depends on `VK_KHR_get_physical_device_properties2`, which is promoted to Vulkan 1.1. That is, to use this extension, `VK_EXT_extended_dynamic_state3` and either `VK_KHR_get_physical_device_properties2` or Vulkan 1.1 are required. | ||
Additionally this sample uses `VK_KHR_dynamic_rendering" which required Vulkan 1.2. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
VK_KHR_dynamic_rendering should be in single quotation marks, otherwise it won't render properly in asciidoc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
|
||
== Resources | ||
|
||
https://registry.khronos.org/vulkan/specs/1.3-extensions/man/html/VK_EXT_extended_dynamic_state3.html |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Versioned spec is no longer build, so these should be updated to use /latest/ instead of /1.3-extensions/
The latter still works due to a redirect but might stop working at some point in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just two minor issues from my side.
for (auto &node : scene_nodes_transparent_flipped) | ||
{ | ||
draw_node(draw_cmd_buffers[i], node); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before binding a pipeline, one could check if there are nodes for that pipeline at all, like
if (!scene_nodes_opaque.empty())
{
vkCmdBindPipeline(draw_cmd_buffers[i], VK_PIPELINE_BIND_POINT_GRAPHICS, pipeline_opaque);
for (auto &node : scene_nodes_opaque)
{
draw_node(draw_cmd_buffers[i], node);
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes applied.
|
||
bool DynamicMultisampleRasterization::resize(const uint32_t _width, const uint32_t _height) | ||
{ | ||
sample_count = VK_SAMPLE_COUNT_1_BIT; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comment why you need to switch sample_count to 1_BIT here and back to the "actual" value right after resizing would be fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is related to this VL Error
Validation Error: [ VUID-VkFramebufferCreateInfo-pAttachments-00881 ] Object 0: handle = 0xba7514000000002a, type = VK_OBJECT_TYPE_RENDER_PASS; Object 1: handle = 0x2002ea00000002ad, type = VK_OBJECT_TYPE_IMAGE_VIEW; Object 2: handle = 0x46b46400000002ab, type = VK_OBJECT_TYPE_IMAGE; | MessageID = 0x2ff52eec | vkCreateFramebuffer(): pCreateInfo->pAttachments[1] has VK_SAMPLE_COUNT_4_BIT samples that do not match the VK_SAMPLE_COUNT_1_BIT samples used by the corresponding attachment for VkRenderPass 0xba7514000000002a[].
While window is resized recreation of framebuffer is performing. When using other sample count then VK_SAMPLE_COUNT_1_BIT
it is complaining because renderpass sample count is by default set to VK_SAMPLE_COUNT_1_BIT
I can think about more neat solution, but if needed only :)
The resizing issue is probably not specific to this sample. |
I confirm that change resolve that issue. Thanks for pointing it. |
Overview
This sample demonstrates one of the functionalities of VK_EXT_extended_dynamic_state3 related to rasterization samples.
The extension can be used to dynamically change sampling without any need to restart the application.
Enabling the extension
To be able to use this extension in Vulkan API:
VkPhysicalDeviceExtendedDynamicStateFeaturesEXT
,VkPhysicalDeviceExtendedDynamicState2FeaturesEXT
,VkPhysicalDeviceExtendedDynamicState3FeaturesEXT
must be added to the feature chain of
VkPhysicalDeviceProperties2
andVkPhysicalDeviceExtendedDynamicState3PropertiesEXT
must be added toVkPhysicalDeviceProperties2
.Using the extension
To use the extension:
VK_DYNAMIC_STATE_RASTERIZATION_SAMPLES_EXT
must be added toVkPipelineDynamicStateCreateInfo
.void vkCmdSetRasterizationSamplesEXT(VkCommandBuffer commandBuffer, VkSampleCountFlagBits rasterizationSamples)
should be called betweenvkCmdBeginRenderPass
andvkCmdEndRenderPass
.Known issues
The extension always reports the following validation error when enabled:
This implies several other validation errors during runtime:
Resources
https://registry.khronos.org/vulkan/specs/1.3-extensions/man/html/VK_EXT_extended_dynamic_state3.html
https://registry.khronos.org/vulkan/specs/1.3-extensions/man/html/vkCmdSetRasterizationSamplesEXT.html
Please ensure the following points are checked:
Note: The Samples CI runs a number of checks including: