Add a new sample - subgroups operations #715
Conversation
Patryk-Jastrzebski-Mobica
changed the title
| VK_CHECK(vkAllocateDescriptorSets(get_device().get_handle(), &alloc_info, &compute.descriptor_set)); | ||
| } | ||
|
|
||
| void SubgroupsOperations::preapre_compute_pipeline_layout() |
There was a problem hiding this comment.
typo. Should be "prepare_compute_pipeline_layout"
| VK_DESCRIPTOR_TYPE_COMBINED_IMAGE_SAMPLER, | ||
| 1u, | ||
| &image_descriptor)}; | ||
| vkUpdateDescriptorSets(get_device().get_handle(), static_cast<uint32_t>(write_descriptor_sets.size()), write_descriptor_sets.data(), 0u, NULL); |
There was a problem hiding this comment.
How about using nullptr instead of NULL? The last argument of vkUpdateDescriptorSets is a pointer to an array of VkCopyDescriptorSet so nullptr better suggest that is the pointer
There was a problem hiding this comment.
out to date
| VkViewport viewport = vkb::initializers::viewport(static_cast<float>(width), static_cast<float>(height), 0.0f, 1.0f); | ||
| vkCmdSetViewport(draw_cmd_buffers[i], 0u, 1u, &viewport); | ||
|
|
||
| VkRect2D scissor = vkb::initializers::rect2D(width, height, 0, 0); |
There was a problem hiding this comment.
Perhaps it can be explicitly cast by static_cast<int32_t>(width) and static_cast<int32_t>(height). This is not necessary, just a small suggestion
|
|
||
| // draw texture | ||
| { | ||
| vkCmdBindDescriptorSets(draw_cmd_buffers[i], VK_PIPELINE_BIND_POINT_GRAPHICS, texture_object.pipeline.pipeline_layout, 0, 1, &texture_object.descriptor_set, 0, nullptr); |
There was a problem hiding this comment.
For code style consistency I suggest to add "u" to numbers
vkCmdBindDescriptorSets(draw_cmd_buffers[i], VK_PIPELINE_BIND_POINT_GRAPHICS, texture_object.pipeline.pipeline_layout, 0u, 1u, &texture_object.descriptor_set, 0u, nullptr);
There was a problem hiding this comment.
out to date
| vkCmdBindPipeline(draw_cmd_buffers[i], VK_PIPELINE_BIND_POINT_GRAPHICS, texture_object.pipeline.pipeline); | ||
|
|
||
| VkDeviceSize offset[] = {0}; | ||
| vkCmdBindVertexBuffers(draw_cmd_buffers[i], 0, 1, texture_object.buffers.vertex->get(), offset); |
There was a problem hiding this comment.
For code style consistency I suggest to add "u" to unsigned int arguments
There was a problem hiding this comment.
out to date
| vkCmdBindVertexBuffers(draw_cmd_buffers[i], 0, 1, texture_object.buffers.vertex->get(), offset); | ||
| vkCmdBindIndexBuffer(draw_cmd_buffers[i], texture_object.buffers.index->get_handle(), 0, VK_INDEX_TYPE_UINT32); | ||
|
|
||
| vkCmdDrawIndexed(draw_cmd_buffers[i], texture_object.buffers.index_count, 1u, 0u, 0u, 0u); |
There was a problem hiding this comment.
vertexOffset argument is int32_t so it should be
vkCmdDrawIndexed(draw_cmd_buffers[i], texture_object.buffers.index_count, 1u, 0u, 0, 0u);
There was a problem hiding this comment.
out to date
|
|
||
| struct GuiSettings | ||
| { | ||
| int32_t selected_filtr = 0; |
There was a problem hiding this comment.
typo. Should be selected_filter
There was a problem hiding this comment.
out to date
| int32_t selected_filtr = 0; | ||
| static std::vector<std::string> init_filters_name() | ||
| { | ||
| std::vector<std::string> filtrs = { |
There was a problem hiding this comment.
typo. Should be std::vectorstd::string filters
There was a problem hiding this comment.
out to date
|
That's a nice little sample! |
|
Hi @asuessenbach, |
|
Any update on this? Having a subgroups sample would be very much appreciated :) |
|
Any updates on this sample? Having a subgroup sample would be a great addition :) |
Patryk-Jastrzebski-Mobica
force-pushed
the
subgroups_operations
branch
from
6a2eef8
to
a67ff3e
Compare
|
Hi @SaschaWillems, I apologise for not updating for such a long time. Unfortunately the implementation of the sample is not yet finished. I still have some work left to do:
|
|
No need to apologize. Your work is very much appreciated and I'm happy to hear that this sample is coming along :) |
|
Any update on this? |
|
|
|
Hi @Patryk-Jastrzebski-Mobica, we noticed there's an issue with the CLA on this one - looks like the test got confused with the login alias (kdmitruk). Can you look into this? Not quite sure what the fix is - perhaps sign the CLA again using the alias? Thanks |
|
Hi @marty-johnson59, I will to do something about it this week. I hope to find some time in the near future to finish the sample :). |
|
Hi @Patryk-Jastrzebski-Mobica, quick reminder on this. We briefly discussed this on the Samples call this week and there's definitely interest in seeing this sample released. Let us know if/how we can help. Thanks! |
|
Hi @marty-johnson59, I am very sorry for such a long delay. I got back to work on this example this week. For now, I am pushing the fixes onto my own branch https://github.com/Mobica/Vulkan-Samples/tree/subgroups_operations_WIP3 . As soon as I have fixed the reflections on the model, I will immediately start to rebase this branch and deliver the code. |
|
Awesome, thank you! (and NP on the delay - just following up and letting you know there is still strong interest in this sample :) ) |
Patryk-Jastrzebski-Mobica
force-pushed
the
subgroups_operations
branch
from
a67ff3e
to
f1936a7
Compare
… remove useless shaders
…ilde_h0 shader and removed commented lines
|
Hi, |
Patryk-Jastrzebski-Mobica
changed the title
|
I don't know why, but this little patch makes the subgroups enabled case look correct again. |
|
Hey, I wanted to check -- does this sample work for all possible legal subgroup sizes? I didn't notice any checks for the exposed subgroupSize, nor any logic to handle different ones. It wasn't clear to me how/if the algorithm is such that it 'just works' whatever size is exposed or not. EDIT: |
|
@cforfang : The sample is still work-in-progress |
Understood, just to highlight already now in case incompatibility might come up as a problem later for example :) |
Description
Please include a summary of the change, new sample or fixed issue. Please also include relevant motivation and context.
Please read the contribution guidelines
Fixes #
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: