[vk] Migrate ash structs to builders #3409
Conversation
![monocodus[bot]](https://avatars.githubusercontent.com/in/28526?s=60&v=4){kind=small}
Excludes vk::WriteDescriptorSet due to the more complex way it is constructed.
There was a problem hiding this comment.
That's an interesting pull request. Thank you for making it!
As usual with non-trivial PRs not linked to any issues, there comes a question of - what is the motivation for doing this?
One thing that's clearly a win is that we don't need to write vk::StructureType:: things in all the structs. This is an improvement.
On the other hand, builders hide the fields that we'd potentially want to set but not doing right now. Seeing let builder = ... builder.something(); in a few places is also technically worse than just modifying the structs.
In general, I think builders work best when used by a higher level logic. For example, by the user directly, or by an engine. In this case, they know what they need, and they leave everything else to be defaults. However, in gfx-rs case we pretty much want everything, so the builder makes less sense to use.
I just wanted to sync up on what you expect the outcomes to be from merging this.
| depth_clamp_enable: if desc.rasterizer.depth_clamping { | ||
| this.rasterization_state = vk::PipelineRasterizationStateCreateInfo::builder() | ||
| .flags(vk::PipelineRasterizationStateCreateFlags::empty()) | ||
| .depth_clamp_enable(if desc.rasterizer.depth_clamping { | ||
| if device.shared.features.contains(Features::DEPTH_CLAMP) { |
There was a problem hiding this comment.
we don't really need to check for this, can just call depth_clamp_enable(desc.rasterizer.depth_clamping)
I'm attempting to add OpenXR support to the Vulkan backend (#3219), and I'm currently running into a problem at the top of I figured that the changes might be independently of interest, since I saw some existing places that |
|
Ok, I want to hear out if other members have opinions on this. Otherwise, we'll proceed (let's say, tomorrow) |
|
Ooh, interesting development: my OpenXR diff crashes when based on master (specifically eeccf94, the parent of this branch), but does not crash when rebased on this branch. So there's maybe an actual bug being fixed by this PR... |
| p_application_info: &app_info, | ||
| enabled_layer_count: layers.len() as _, | ||
| pp_enabled_layer_names: str_pointers.as_ptr(), | ||
| enabled_extension_count: extensions.len() as _, |
There was a problem hiding this comment.
False alarm. The bug was here: I was combining extensions with the set of extensions required by the OpenXR runtime, and didn't spot that the wrong length was being used here. The builder API "fixed" the bug because it takes a slice instead of a length+pointer, avoiding this kind of problem entirely.
|
@kvark using the builders sounds alright to me 👍 I don't have much of a preference either way since the trade-offs seem pretty minor |
I didn't call builder methods for values that were clearly being set to their defaults in the struct. I did leave e.g. calls to
*Flags::empty()and other non-obvious defaults, as well as any values that were default but marked asTODO.PR checklist:
makesucceeds (on *nix)make reftestssucceedsmakeon Windows, so I manually rancd src/warden && cargo run --bin reftest --features vulkan -- localvulkanmesh-shading(I didn't have that feature).