gfx-rs / portability

Vulkan Portability Implementation
Mozilla Public License 2.0
383 stars 25 forks source link

Dynamic state line width setting sometimes discarded #206

Open krolli opened 4 years ago

krolli commented 4 years ago

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:

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 commented 4 years ago

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 commented 4 years ago

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:

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 commented 4 years ago

Thank you, that sounds about right, and I was missing these details. So what do the examples do? Bind a non-line pipeline with this dynamic state, setting the width, and only after switching to the line pipeline. That seems rather tricky on their part...

On Jan 27, 2020, at 05:39, Martin Krošlák notifications@github.com wrote:

 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 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.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

krolli commented 4 years ago

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 commented 4 years ago

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