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

Fix fragment_shading_rate_dynamic sample

Open jherico opened this issue 1 year ago • 3 comments

Description

Fix the declaration of the compute_buffer in a loop as an r-value reference to a simple reference.

Fixes #913

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

jherico avatar Feb 16 '24 14:02 jherico

I'm actually not sure why that sample works at all. It allocates the compute command buffers from the base class command pool, which may be limited to the graphics queue family index.

Imo it should be adjusted to create it's own compute command pool, like we do in the other compute related samples.

SaschaWillems avatar Feb 19 '24 19:02 SaschaWillems

I'm actually a bit confused, because I recently did a batch test against my allocated-refactor branch which does NOT have this fix and it passed without crashing. And the error you're seeing is what I was getting before I made this change on the sample, so I think the sample is more subtle than I suspected at first...

Imo it should be adjusted to create it's own compute command pool, like we do in the other compute related samples.

I initially tried that approach because I noticed that FragmentShadingRateDynamic::build_command_buffers resets this command pool, which invalidates any of the compute buffers allocated from it, but that didn't seem to help. That's also where my investigation with git bisect led me.

I'm going to give that another shot and test it again checking both release and debug modes this time.

jherico avatar Feb 19 '24 20:02 jherico

OK, so using a dedicated compute queue for the compute operations would require significant refactoring of the example. While the acquire and release barriers in the compute command buffers are pretty straight-forward, it's not immediately clear to me where they'd go on the graphics side. It looks like the acquire would have to go in the draw_cmd_buffers but the release would have to go in the small_command_buffers, I think. At any rate, I'm going to just try to resolve this with a unified queue for now.

jherico avatar Feb 20 '24 03:02 jherico