KhronosGroup / MoltenVK

MoltenVK is a Vulkan Portability implementation. It layers a subset of the high-performance, industry-standard Vulkan graphics and compute API over Apple's Metal graphics framework, enabling Vulkan applications to run on macOS, iOS and tvOS.
Apache License 2.0
4.76k stars 419 forks source link

Issues with vertex layout when using VK_LAYER_KHRONOS_shader_object #2051

Open maksym-pasichnyk opened 11 months ago

maksym-pasichnyk commented 11 months ago

When I try use VK_LAYER_KHRONOS_shader_object geomeometry is not visible. MoltenVK set MTLBufferLayoutStrideDynamic into MTLVertexBufferLayoutDescriptor, and geometry is not drawing. When I change line below (first screenshot) and set pVKVB->stride into vbDesc.stride everything working fine and geometry is visible (second screenshot).

Screenshot 2023-10-28 at 02 37 58 Screenshot 2023-10-28 at 02 42 45
cdavis5e commented 11 months ago

That would indicate that you (or the layer you're using, since VK_EXT_shader_object also supports this) set vertex input strides to be a dynamic state, but you're not calling vkCmdBindVertexBuffers2(). According to the Vulkan spec:

In other words, you have to call vkCmdBindVertexBuffers2() or the behavior is undefined--it is allowed to crash, corrupt your hard disk, make demons fly out of your nose, or even work just fine if you don't call vkCmdBindVertexBuffers2() in this case. But note that it doesn't have to "work just fine" everywhere. The thing about undefined behavior is that it varies from implementation to implementation.

What happens when you use the validation layer? Are you calling vkCmdBindVertexBuffers2() like you're supposed to? Are you calling vkCmdSetVertexInput() instead? The latter won't work with MoltenVK because it doesn't implement VK_EXT_vertex_input_dynamic_state.

maksym-pasichnyk commented 11 months ago

@cdavis5e Yes, I'm calling vkCmdBindVertexBuffers2 and specify strides, but they are ignored in MoltenVK because IsVtxStrideStatic is false. Nothing happens when I use validation layer, no errors, no warnings

richard-lunarg commented 11 months ago

Which version of macOS are you running? You need Metal 3.1 for VK_DYNAMIC_STATE_VERTEX_INPUT_BINDING_STRIDE. I'm not sure the extension should actually be exposed unless Metal 3.1 is available and the full functionality is available.

cdavis5e commented 11 months ago

@cdavis5e Yes, I'm calling vkCmdBindVertexBuffers2 and specify strides, but they are ignored in MoltenVK because IsVtxStrideStatic is false. Nothing happens when I use validation layer, no errors, no warnings

??

You're not making sense. The code you cited sets up the Metal vertex input descriptor for the pipeline object. It sets the STATIC vertex input state--you know, what would be used if you were using pipeline objects without VK_EXT_extended_dynamic_state or VK_EXT_shader_object. MTLBufferLayoutStrideDynamic is used in Metal pipeline vertex input descriptors to mark dynamic vertex input strides.

...

Are you calling vkCmdBindVertexBuffers2()... or vkCmdBindVertexBuffers2EXT() (with suffix)? You actually need to call the latter on MoltenVK, because MoltenVK doesn't support Vulkan 1.3.

JagexDark commented 1 month ago

Also ran into this and found that force disabling VK_EXT_extended_dynamic_state allows VK_LAYER_KHRONOS_shader_object to function correctly with vkCmdSetVertexInputEXT and vkCmdBindVertexBuffers.

I'm inclined to agree that VK_EXT_extended_dynamic_state should not be exposed without full support, or alternatively logic could be added to the shader object layer to detect this situation (and also still allow the other parts of VK_EXT_extended_dynamic_state to be used)

billhollings commented 1 month ago

Also ran into this and found that force disabling VK_EXT_extended_dynamic_state allows VK_LAYER_KHRONOS_shader_object to function correctly with vkCmdSetVertexInputEXT and vkCmdBindVertexBuffers.

I'm inclined to agree that VK_EXT_extended_dynamic_state should not be exposed without full support, or alternatively logic could be added to the shader object layer to detect this situation (and also still allow the other parts of VK_EXT_extended_dynamic_state to be used)

Thanks for the feedback and input. You make a valid point.

What is missing from MoltenVK's implementation of VK_EXT_extended_dynamic_state that is causing the issue that you are encountering?

Is it that VK_DYNAMIC_STATE_VERTEX_INPUT_BINDING_STRIDE is not available before Metal 3.1 (macOS 14/iOS 17), or something else?

If VK_DYNAMIC_STATE_VERTEX_INPUT_BINDING_STRIDE is the concern, compound extensions like VK_EXT_extended_dynamic_state, are a challenge when one of many features can't be handled.

With MoltenVK, our general approach is to provide as much functionality as possible, even if it means slight deviations from strict Vulkan conformance. With that in mind, we would generally not shut down an extension with lots of independent features, because one of them can't be handled in certain platform configurations. Instead, we try to highlight to the user when we can't adhere to strict conformance, and in this case, to help the app adapt, or advise their own users, if the user's platform does not have the necessary features, we log and report:

This device and platform does not support VK_DYNAMIC_STATE_VERTEX_INPUT_BINDING_STRIDE (macOS 14.0 or iOS/tvOS 17.0, plus either Apple4 or Mac2 GPU)."

In this particular case, that approach is complicated by the fact that VK_LAYER_KHRONOS_shader_object can't perform other very useful work because it depends on this particular feature. It is fortunate that you are able to disable the entire VK_EXT_extended_dynamic_state extension, without missing any of its other features.