Skip to content
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 state line width setting sometimes discarded #206

Open
krolli opened this issue Jan 26, 2020 · 5 comments
Open

Dynamic state line width setting sometimes discarded #206

krolli opened this issue Jan 26, 2020 · 5 comments

Comments

@krolli
Copy link
Contributor

krolli commented Jan 26, 2020

When user attempts to create graphics pipeline with VK_DYNAMIC_STATE_LINE_WIDTH and polygon mode other than VK_POLYGON_MODE_LINE, Vulkan back-end will create graphics pipeline without this dynamic state setting. This can potentially lead to several issues, such as:

  • any calls to vkCmdSetLineWidth while this pipeline is bound are invalid, causing correct code to break (triggering validation errors as well)
  • certain sequences of pipeline binding that relied on dynamic state remaining undisturbed will be broken

This currently affects https://github.com/SaschaWillems/Vulkan (specifically https://github.com/SaschaWillems/Vulkan/blob/master/examples/pipelines/pipelines.cpp#L152) where line width is set while pipeline with fill mode is bound. Even though all created pipelines use line width as dynamic state, only wireframe pipeline actually has this setting.

My understanding is that this is due to gfx-hal storing dynamic state information as part of line polygon mode enum variant. Other dynamic states may be affected by this problem as well, depending on how they are represented in gfx-hal structures.

@kvark
Copy link
Member

kvark commented Jan 26, 2020

Thank you for tiling this bug!
Something doesn't match up to me. Documentation for vkCmdSetLineWidth doesn't list the requirement (of VK_DYNAMIC_STATE_LINE_WIDTH being set on the current pipeline) in valid usage. Therefore, it shouldn't error out if the state is not dynamic. Perhaps, this is a validation layer bug?

certain sequences of pipeline binding that relied on dynamic state remaining undisturbed will be broken

I also don't quite understand why these cases would be broken. vkCmdSetLineWidth is documented as "Set the dynamic line width state", and the user doesn't specify the specific pipeline here, which means to me that, basically, a command buffer has this state and this command just sets it, regardless of what is going to use it later on.

@krolli
Copy link
Contributor Author

krolli commented Jan 27, 2020

You're right that validation error doesn't quite match what is in spec. For completeness, this is the error:

VUID-vkCmdSetLineWidth-None-00787(ERROR / SPEC): msgNum: 0 - vkCmdSetLineWidth called but pipeline was created without VK_DYNAMIC_STATE_LINE_WIDTH flag. The Vulkan spec states: The bound graphics pipeline must have been created with the VK_DYNAMIC_STATE_LINE_WIDTH dynamic state enabled (https://www.khronos.org/registry/vulkan/specs/1.1-extensions/html/vkspec.html#VUID-vkCmdSetLineWidth-None-00787)
Objects: 1
[0] 0x237507cd590, type: 6, name: NULL

I didn't find the quoted section in specification, so at least that seems to be a mistake in validation.

However, section about Dynamic State is being rather explicit about a few things:

  • there seems to be only one state in command buffer
  • when binding pipeline with static state, this command buffer state is modified
  • when binding pipeline with dynamic state, this command buffer state is not disturbed
    • but it seems that, when switching from static to dynamic, it must be specified again using dynamic state setting command before it is used; value from pipeline where this state is static does not persist
  • after pipeline with static state is bound, there must be no calls to corresponding dynamic state setting commands before any draw or dispatch

At least, this is my understanding of that section. I am not sure whether someone just wanted to avoid repetition when writing valid usage for vkCmdSetLineWidth, or whether there is some deeper meaning there.

@kvark
Copy link
Member

kvark commented Jan 27, 2020 via email

@krolli
Copy link
Contributor Author

krolli commented Jan 27, 2020

Not necessarily. In order to keep cost of pipeline changes low, it makes sense for engine to keep settings consistent across pipelines, even if this setting might not play a role in them. Pipeline derivatives further encourage unifying pipeline descriptions where possible.

A simple (and a bit contrived) example would be vector UI rendering, where one might switch between line and triangle rendering (triangles for filling out shapes, lines for vector outlines on these shapes). Obviously, higher-level rendering engine might decide it wants to use static line width when not rendering lines, but it should be decision done by the engine as it changes behavior of binding pipeline in a way that may require recording extra commands.

Also, I believe this setting affects any line rasterization, including line primitive topology. With current structure, this means that line width will be ignored for line primitive topology when polygon fill mode is not set to line as well.

@kvark
Copy link
Member

kvark commented Jan 28, 2020

Good points! Let's move the line width state out of the PolygonMode::Line then in gfx-rs.

bors bot added a commit to gfx-rs/gfx that referenced this issue Feb 5, 2020
3139: Move line width out of line polygon mode r=kvark a=krolli

Line width state is not tied only to line polygon mode and storing it
there has correctness implications on Vulkan back-end. This is
problematic for gfx-portability when targeting Vulkan back-end, since it
can cause correct Vulkan application to trigger validation errors.

Prepares for fixing gfx-rs/portability#206
PR checklist:
- [x] `make` succeeds (on *nix)
- [x] `make reftests` succeeds
- [x] tested examples with the following backends: dx12, vulkan
- [x] `rustfmt` run on changed code


Co-authored-by: Martin Krošlák <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants