Permit builders to be passed in slices

[farnoy]

@MaikKlein @Ralith First of all thanks for your work on builders, they make it so much easier to define only what is required.

Since the Builder objects are transparent, I tried this code, which I think is safe to do:

let color_attachments = &[color_attachment];
let subpass_descs = [
    vk::SubpassDescription::builder()
        .depth_stencil_attachment(&depth_attachment)
        .pipeline_bind_point(vk::PipelineBindPoint::GRAPHICS),
    vk::SubpassDescription::builder()
        .color_attachments(color_attachments)
        .depth_stencil_attachment(&depth_attachment)
        .pipeline_bind_point(vk::PipelineBindPoint::GRAPHICS),
];
let subpass_dependencies = [
    vk::SubpassDependency::builder()
        .src_subpass(vk::SUBPASS_EXTERNAL)
        .dst_subpass(0)
        .src_stage_mask(vk::PipelineStageFlags::BOTTOM_OF_PIPE)
        .dst_stage_mask(vk::PipelineStageFlags::EARLY_FRAGMENT_TESTS)
        .dst_access_mask(
            vk::AccessFlags::DEPTH_STENCIL_ATTACHMENT_READ
                | vk::AccessFlags::DEPTH_STENCIL_ATTACHMENT_WRITE,
        )
        .build(),
    vk::SubpassDependency::builder()
        .src_subpass(0)
        .dst_subpass(1)
        .src_stage_mask(vk::PipelineStageFlags::LATE_FRAGMENT_TESTS)
        .src_access_mask(vk::AccessFlags::DEPTH_STENCIL_ATTACHMENT_WRITE)
        .dst_stage_mask(vk::PipelineStageFlags::EARLY_FRAGMENT_TESTS)
        .dst_access_mask(vk::AccessFlags::DEPTH_STENCIL_ATTACHMENT_READ)
        .build(),
];

let renderpass_create_info = vk::RenderPassCreateInfo::builder()
    .attachments(&attachment_descriptions)
    .subpasses(unsafe {
        std::mem::transmute::<_, &[vk::SubpassDescription; 2]>(&subpass_descs)
    })
    .dependencies(&subpass_dependencies);

And it seems to be working just fine. It has the obvious benefit for tracking lifetimes, so the compiler will error out if I try this:

let subpass_descs = [
    vk::SubpassDescription::builder()
        .depth_stencil_attachment(&depth_attachment)
        .pipeline_bind_point(vk::PipelineBindPoint::GRAPHICS),
    vk::SubpassDescription::builder()
        .color_attachments(&[color_attachment]) // note the temporary value created here
        .depth_stencil_attachment(&depth_attachment)
        .pipeline_bind_point(vk::PipelineBindPoint::GRAPHICS),
];

And this is something I run into periodically when I need to call build().

Could ash somehow generate functions&structs that are compatible with, in the case of RenderPassCreateInfo,&[vk::SubpassDescription] and &[vk::SubpassDescriptionBuilder] at the same time?

[farnoy]

Quick (possibly dumb) idea: can we make all the Builder functions accept only other builders or slices of them? This would be a breaking change, but with positive safety implications and it's not unprecedented.

[MaikKlein]

Yes this is something that is a bit annoying. I think what we can do is this

unsafe trait Cast {
    type Target;
}

    pub fn subpasses(
        mut self,
        subpasses: &'a [impl Cast<Target = SubpassDescription>] {
        ...
    }

where Cast would be implemented for SubpassDescription and SubpassDescriptionBuilder

Just a few quick thoughts, I'll have a look at how we can improve the API. Thanks for opening the issue :).

[zakarumych]

It can be done the other way around - add lifetime parameter and return built struct with same lifetime from build method of builders.

It would be impossible to accidentally use reference to too short-lived temporary value in builder and then use built structure in API calls when value is already dropped.

[colinmarc]

.color_attachments(&[color_attachment]) // note the temporary value created here

I don't actually get a compiler error here - although weirdly, it only manifests as an issue with --release. Did something change?

Edit: Ah, I now understand that the build method is throwing away the lifetime information.

[farnoy]

@MaikKlein Is the whole idea undesirable, or just the implementation with unsafe trait in #253?

[MaikKlein]

@MaikKlein Is the whole idea undesirable, or just the implementation with unsafe trait in #253?

I am actually thinking of bringing this PR back. I just didn't like the additional complexity and it is hard to get feedback. But I think in this case it might be worth it.

[MarijnS95]

This will now be possible thanks to #602: the builder structs are gone and their functions have been implemented directly on the Vulkan struct, which now carries a pub _marker: PhantomData<&'a ()>, when the struct contains one or more pointers. In addition, calling .build() and losing lifetime info is an issue of the past.