Proposal: use non_exhaustive (instead of builders) for all extendable Vulkan structs

[Rua]

Vulkan has a neat, if somewhat cumbersome mechanism for adding new functionality to existing APIs, by means of the pNext pointers. The goal behind this design is to allow extending the API without breaking existing code. Preventing code breakage is also a goal for Vulkano, and one idiomatic way to do that is by using builders. You can always add new functions to a builder without breaking existing code, as long as the builder behaves the same as before the new function was added.

I therefore propose that, in principle, whenever Vulkan allows for extension using pNext, Vulkano maps this to a builder. Existing object types not currently using a builder would be converted to use one, such as Instance, Device, UnsafeBuffer, ComputePipeline and others.

[Eliah-Lakhin]

Can you show an example of how e.g. ComputePipeline's builder will look like?

[Rua]

Something like this:

ComputePipeline::start(device)
    .shader(cs.entry_point("main").unwrap())
    .layout(layout) // would be optional, defaults to a layout generated from the shader
    .build().unwrap()

So it would be similar to how the GraphicsPipeline builder works, just with a lot less options.

[Eliah-Lakhin]

ok, looks good!

[gurchetansingh]

Yes, that sounds good! I think DeviceMemory already uses one. Feel free to change the API if you find a cleaner way to do it generically!

[Rua]

An alternative design I thought of, that is a bit further from the current typical builder design but closer to Vulkan, is this version:

ComputePipeline::new(
    device,
    ComputePipelineCreateInfo::default()
        .shader(cs.entry_point("main").unwrap())
        .layout(layout)
    ).unwrap()

In this version, the "builder" only helps to gather the creation info, and is then passed to new, where the construction itself happens. There's not necessarily an advantage or disadvantage to this style, it's just different, and closer to the C Vulkan style.

Not all extendable structures are found in object creation APIs either. Note VkPhysicalDeviceImageFormatInfo2 for example, which is used to retrieve information from the device and is exposed from Vulkano 0.28 as PhysicalDevice::image_format_properties. A function can obviously not be builderfied, but a parameter of that function can be. So using the more "Vulkany" style above, such a call would become:

physical_device.image_format_properties(ImageFormatInfo::default()
    .format(format)
    .ty(ty)
    .tiling(tiling)
    .usage(usage)
    .flags(flags)
    .external_memory_handle_type(handle_type) // from extension struct
    .image_view_type(view_type)               // from extension struct
)

Please let me know what you think of this style, whether you think it's better than Vulkano's current builder style or if you have alternative ideas.

Lastly, there's the few APIs, including the one above, where the output of a Vulkan function is an extendable struct. Builderfying a struct that's meant to be returned to the user seems awkward, it would essentially be a struct with only getter methods. We may have to look for alternative ways to make them more robust against breaking changes.

[Rua]

I just discovered (perhaps way too late) that structs can be given a non_exhaustive attribute. For some reason I thought it was only usable for enums. When you give a struct this attribute, users outside the crate cannot construct the struct using the normal struct construction syntax. They must use struct update syntax, using something like Default::default() or one of the methods of the struct to create a new instance of the struct.

This seems useful for extendable structs. Constructing a device would look like this (using #1814 as a base):

Device::new(
    physical_device,
    DeviceCreateInfo {
        queues: [
            QueueCreateInfo {
                queues: [1.0].into(),
                ..QueueCreate::family(queue_family_graphics)
            },
            QueueCreateInfo {
                queues: [0.5].into(),
                ..QueueCreate::family(queue_family_compute),
        ].into(),
        enabled_extensions: physical.required_extensions().union(&device_extensions),
        enabled_features: features,
        ..Default::default()
    },
)

This is not only backwards compatible thanks to the struct update syntax, but should also be more familiar to C Vulkan users. Note that thanks to the struct update syntax, you can still use sensible defaults just like you can with builders. For example, an ImageView could be as simple as this, using all the default values:

ImageView::new(
    image,
    Default::default(),
)

Personally I really like this style. It is rather different from current Vulkano, but I think it has some usability advantages, and uses a core Rust feature specifically introduced to avoid breaking compatibility. A big advantage is that structures can't just be written this way, but the fields from the structs can also be read, something not possible with a builder unless you give it both getters and setters (ew!).

[Eliah-Lakhin]

Interesting. This is a new thing for me too :)

[Amjad50]

I think it would be great. Even now, we are using many getters in vulkano to access internal fields.

[Rua]

One detail to note is that values with no sensible default would have to be wrapped in an Option. This is the case for example when constructing an image and providing the format: I can't think of any format that would make sense as a default here, so it would have to be Option<Format> and would be provided when constructing as format: Some(Format::R8G8B8A8_UNORM).

[Rua]

Well, never mind that, because non_exhaustive prevents struct update syntax apparently. That's rather frustrating and unintuitive, I hope they add something to allow it.

[Rua]

I found a workaround, and made a draft PR for #1815 using the struct update syntax instead of a builder.