WIP: Implement builder-like setters

[MatusT]

This PR implements builder-like setters in order to reduce the noise created by manual filling of Vulkan's structs. It is supposed to work nicely with the implementation of Default trait and further improve it. The main contribution is in more rustier versions of p_ and pp_ fields, now taking Rust's slices and filling appropriate _count or _size field automatically.

Example of code before:

    let device_create_info = vk::DeviceCreateInfo {
        p_queue_create_infos: &graphics_queue_create_info,
        queue_create_info_count: 1,

        enabled_layer_count: instance_layers_names.len() as u32,
        pp_enabled_layer_names: instance_layers_names_raw.as_ptr(),

        enabled_extension_count: device_extension_names_raw.len() as u32,
        pp_enabled_extension_names: device_extension_names_raw.as_ptr(),

        p_enabled_features: &device_features,
        ..Default::default()
    };

    let pipeline_vertex_input_state_create_info = vk::PipelineVertexInputStateCreateInfo {
        vertex_binding_description_count: Vertex::binding_descriptions().len() as u32,
        p_vertex_binding_descriptions: (&Vertex::binding_descriptions()).as_ptr(),
        vertex_attribute_description_count: Vertex::attribute_descriptions().len() as u32,
        p_vertex_attribute_descriptions: (&Vertex::attribute_descriptions()).as_ptr(),
        ..Default::default()
    };

And example of code after applying builder-like pattern.

    let device_create_info = vk::DeviceCreateInfo::default()
        .p_queue_create_infos(&[graphics_queue_create_info])
        .pp_enabled_layer_names(&instance_layers_names_raw)
        .pp_enabled_extension_names(device_extension_names_raw)
        .p_enabled_features(&device_features);

    let pipeline_vertex_input_state_create_info = vk::PipelineVertexInputStateCreateInfo::default()
        .p_vertex_binding_descriptions(&Vertex::binding_descriptions())
        .p_vertex_attribute_descriptions(&Vertex::attribute_descriptions());

Please let me know your opinion on this in the comments.

EDIT:
Implementation based on the PhantomData and Deref is in the PR. Things to resolve:

• should p_ and pp_ prefixes be removed? yes
• what should be taken instead of *const char? &str, CStr? &CStr for p_ &[*const c_char] for pp_
• does Ash even want this pattern or should this be delegated to higher-level libraries? yes

---

This change is 

[Ralith]

Ooh, great idea! Not being able to ergonomically supply slices to structs was always disappointing.

As written, however, I am worried that this is a bit of a landmine. In your example code, I believe the array literals are short-lived--their lifetime ends after the DeviceCreateInfo is constructed but before it's used, resulting in UB when the vulkan impl dereferences the pointer. I wonder if there's a way we can finagle the lifetime annotations to require that the slices outlive self?

I'd also suggest stripping the p_/pp_ prefixes, since slices aren't pointers, a unified setter doesn't need the disambiguation so much, and rust has strong types.

[MaikKlein]

I assume that something like this should work

struct Foo(*const u32);
impl Foo 
 {
    fn set<'a>(&'a mut self, data: &'a u32) -> &'a mut Foo{
        self.0 = data as _;
        self
    }
}
fn get_value() -> u32 {
    42
}
fn main() {
    /// Doesn't compile
    //let mut foo = Foo(std::ptr::null()).set(&get_value());
    // Note that something like `&3` gets an extended lifetime because
    // it is an rvalue reference of a constant
    let value = get_value();
    let mut foo = Foo(std::ptr::null());
    foo.set(&value);
}

This should then look like

let bindings = Vertex::binding_descriptions();
let desc = Vertex::attribute_descriptions();
let mut pipeline_vertex_input_state_create_info = vk::PipelineVertexInputStateCreateInfo::default();
pipeline_vertex_input_state_create_info
    .vertex_binding_descriptions(&bindings)
    .vertex_attribute_descriptions(&desc);

Thoughts?

[Ralith]

That doesn't help; the compile error you're seeing is unrelated. foo.set(&get_value()) passes the checker as well. The 'a in &'a mut self references the lifetime of a borrow, not of the object itself.

[MaikKlein]

use std::marker::PhantomData;
struct Foo<'a>(*const u32, *const f32, PhantomData<&'a ()>);
impl<'a> Foo<'a> {
    fn set_u32(mut self, data: &'a u32) -> Foo<'a> {
        self.0 = data as _;
        self
    }
    fn set_f32(mut self, data: &'a f32) -> Foo<'a> {
        self.1 = data as _;
        self
    }
}

fn get_value() -> u32 {
    42
}
fn main() {
    let v = 3;
    let f = 3.0;
    let foo = Foo(std::ptr::null(), std::ptr::null(), PhantomData)
        .set_u32(&v)
        .set_f32(&f);
}

https://play.rust-lang.org/?gist=d14a77c92f295ccdebe2b4dd90ec9afa&version=stable&mode=debug&edition=2015

This should work and users can still store ii with Foo<'static> if they wanted to, although then they can't use the builder.
But this requires some significant work to the generator because now the impl requires a lifetime. I also don't want to make the generator too complex.

Alternatively we could just accept it as it is, deref happens inside Vulkan and those functions are already marked as unsafe.

[zakarumych]

deref happens inside Vulkan and those functions are already marked as unsafe.

But this hides raw pointers behind references.
Yet with PhantomData (any ZST in general) the type become not ffi-safe.

[GabrielMajeri]

Yet with PhantomData (any ZST in general) the type become not ffi-safe.

Can't #[repr(transparent)] help?

[MaikKlein]

Can't #[repr(transparent)] help?

This only works for structs with one variant.

Yet with PhantomData (any ZST in general) the type become not ffi-safe.

I am not sure why PhantomData would be unsafe in FFI? Where is this documented? I always assumed that it is just ignored in codegen?

[GabrielMajeri]

This only works for structs with one variant.

I assume you mean only one field. But that's not the case. Example from the RFC:

// Transparent struct wrapper with a marker.
#[repr(transparent)]
struct TransparentWrapper<T> {
    only_non_zero_sized_field: f64,
    marker: PhantomData<T>,
}

The non-zero-sized field could be the actual structure we need to pass to Vulkan. The builder wraps that structure and adds a PhantomData marker.

[MaikKlein]

The non-zero-sized field could be the actual structure we need to pass to Vulkan. The builder wraps that structure and adds a PhantomData marker.

That would be another possibility

use std::marker::PhantomData;
struct Builder<'a, T> {
    build: T,
    _m: PhantomData<&'a ()>,
}

struct Foo(*const u32);
impl Foo {
    fn builder<'a>() -> Builder<'a, Foo> {
        Builder {
            build: Foo(std::ptr::null()), // Use default here or sth
            _m: PhantomData,
        }
    }
}

impl<'a, T> Builder<'a, T> {
    pub fn build(self) -> T {
        self.build
    }
}
impl<'a> Builder<'a, Foo> {
    fn set(mut self, data: &'a u32) -> Builder<'a, Foo> {
        self.build.0 = data as _;
        self
    }
}

fn get_value() -> u32 {
    42
}
fn main() {
    let v = 3;
    let foo = Foo::builder().set(&v).build();
}

https://play.rust-lang.org/?gist=d14a77c92f295ccdebe2b4dd90ec9afa&version=stable&mode=debug&edition=2015

Edit:

I guess we could also just implement Deref<Target=T> for the builder, then we don't even need the build method.

    let v = 3;
    let foo = Foo::builder().set(&v);
    some_vk_function(&foo);

[Ralith]

That also avoids needing to deal with impact on other traits, not to mention complicating every vulkan function with the addition of lifetime arguments; I like it.

I guess we could also just implement Deref<Target=T> for the builder, then we don't even need the build method.

I think this is preferable, because it doesn't interfere with lifetime checking.

[zakarumych]

It would be great if builder could check that all fields are setup with valid data.
But codegen would be required for that.

EDIT. Maybe this is not the job for ash but for higher level code.

[MaikKlein]

@omni-viral I think this is best suited for layers / validation layers.

[Ralith]

should p_ and pp_ prefixes be removed?

I'd say yes; after all, these methods may set multiple fields (e.g. a length field too), and it leaves the door open for other higher-level settlers in the future.

what should be taken instead of *const char? &str, CStr?

&CStr is the correct type for "borrowed C string".

does Ash even want this pattern or should this be delegated to higher-level libraries?

I think this is exactly the sort of zero-cost helper Ash should have. There's discussion of exposing a bindings-only sys crate at #93 for when that's not desired.

[MaikKlein]

should p_ and pp_ prefixes be removed?

Yes I think that would make sense.

what should be taken instead of *const char? &str, CStr?

CStr

does Ash even want this pattern or should this be delegated to higher-level libraries?

Yes this fits Ash perfectly.

[kocsis1david]

It doesn't seem useful to me to have enabled_layer_count after the enabled_layer_names setter.

[MaikKlein]

I only do a partial review for the generated code for now. I'll look at it more closely tomorrow.

Some thoughts:

All the count setters are kind of useless. You might be able to filter them out. I am not sure if there are any edge cases. For example a field with the count name that is not used for an array. I think you can check for the substring in the names

For example

            enabled_layer_count: u32::default(),
            pp_enabled_layer_names: ::std::ptr::null(),

enabled_layer appears as a substring more than one time, and then we should remove the count method.

    pub fn sample_mask(
        mut self,
        sample_mask: *const SampleMask,
    ) -> PipelineMultisampleStateCreateInfoBuilder<'a> {
        self.inner.p_sample_mask = sample_mask;
        self
    }

I think this could become

    pub fn sample_mask(
        mut self,
        sample_mask: &'a SampleMask,
    ) -> PipelineMultisampleStateCreateInfoBuilder<'a> {
        self.inner.p_sample_mask = sample_mask;
        self
    }

I haven't yet thought about all the implications yet. I guess it could protect you from very stupid things but the lifetime is only tied to the builder and so it might appear more safe than it actually is. But the builder also using borrows for slices so this would make it more consistent.

[kocsis1david]

While using this branch, I've noticed that inside SubmitInfoBuilder, the wait_semaphore_count is set by both wait_semaphores and wait_dst_stage_mask functions, but maybe it's not that important. It's nice to use the setters.

[MaikKlein]

While using this branch, I've noticed that inside SubmitInfoBuilder, the wait_semaphore_count is set by both wait_semaphores and wait_dst_stage_mask functions, but maybe it's not that important. It's nice to use the setters.

Hm that does seem quite tricky to properly implement. I guess we could add escape hatches for specific types where we could manually implement some of these setters.

[Ralith]

All the count setters are kind of useless. You might be able to filter them out.

It should be straightforward to filter these out by never generating setters for anything referenced by a len attribute in the same struct.

I guess we could add escape hatches for specific types where we could manually implement some of these setters.

This isn't absolutely necessary. For example, you could detect when a single length field is used by multiple pointers, and generate whatever preferred implementation in that case. I'm not sure what the preferred implementation for this case would be, though...

[kocsis1david]

Maybe just an assert! or debug_assert! is enough to check if the slice length matches the previous count value. But currently there's no harm by assigning the count twice, it will be optimized away. It's not safe, but I'm not sure if the builder is supposed to be super safe.

[Ralith]

A debug_assert! is a nice thought, but is redundant to the validation layers, and wouldn't be able to catch all the cases anyway (since you can't distinguish uninitialized from assigned-to-zero, or easily ensure that other related slices get assigned at all). I think the practical approach is just to accept the unsafety.

[Ralith]

What invalid cases are excluded by the ends_with("Count") constraint?

[MatusT]

That's a failure in my brain. Thanks for catching that.

[MatusT]

It seems in C99 type punning is fine if done through unions[1].
Rust (thanks to LLVM IR?) doesn't seem to have strict aliasing restrictions[2] and it is only necessary to keep alignment in order.

In any case, is there any user-friendly way to create shader module with SPIR-V obtained from include_bytes aside from converting it to vector of proper alignment?

[1] https://stackoverflow.com/questions/25664848/unions-and-type-punning
[2] https://stackoverflow.com/questions/26163272/strict-aliasing-in-rust

[MaikKlein]

&[u8], its the most natural. &[u32] enforces alignment but that doesn't really help if the data is invalid in the first place. Just converting it to &[u32] doesn't really make it more safe. But the vulkan api uses &[u32] so that is what we probably should use.

In any case, is there any user-friendly way to create shader module with SPIR-V obtained from include_bytes aside from converting it to vector of proper alignment?

You don't need to worry about alignment, either it is valid in the first place or it is not. You can use from_raw_parts to convert it. If it is a ptr (which you can get with as_ptr() on a slice) you can just cast *const u8 to *const u32. The length (code_size) for the shader module is still in bytes, so be careful about that.

I am not sure what the best API for the builder would be.

[zakarumych]

You don't need to worry about alignment, either it is valid in the first place or it is not

That's why you should worry.

you can just cast *const u8 to *const u32

No, you can't.

[MaikKlein]

That's why you should worry.

That is not the job of ash. As a user you need to check this yourself and make sure the SPIR-V that you load is valid.

No, you can't.

That is not really a helpful comment at all. Why not?

A SPIR-V module is a single linear stream of words.

Surely you can read it as bytes and cast it to u32? Why do you think that would be illegal?

[kocsis1david]

&[u32] is not convenient to use, because if you read it from a file or use include_bytes!, it will be &[u8]. Correct me if I'm wrong, but to use it safely you have to copy it to a different buffer to make it &[u32]. People don't bother with this and just cast the pointer which is not safer than taking &[u8]. But I understand that you want show with the API that it needs 4 byte alignment.

I think Rust doesn't have a perfect way of describing a type that is a binary blob with a certain alignment. It doesn't have to consist of only u32 integers, it may also contain floats and anything else mixed. A common convention is to always use byte arrays in other languages, but yes, that doesn't represent alignment requirements.

[Ralith]

People don't bother with this and just cast the pointer which is not safer than taking &[u8].

It's better to make the unsafety obvious than to silently perform such a conversion internally.

I think Rust doesn't have a perfect way of describing a type that is a binary blob with a certain alignment. It doesn't have to consist of only u32 integers, it may also contain floats and anything else mixed. A common convention is to always use byte arrays in other languages, but yes, that doesn't represent alignment requirements.

The convention established by Vulkan here is to treat it as a buffer of u32, per the wrapped structure. If any convention takes precedence I imagine that's the one that should.

[kocsis1david]

The convention established by Vulkan here is to treat it as a buffer of u32, per the wrapped structure. If any convention takes precedence I imagine that's the one that should.

But even in Vulkan, codeSize is in bytes, only the pointer points to uint32_t, so it's an exception for the generator.

Maybe the correct way is to use &[u32].
But then the problem is: One does not simply load a SPIR-V module.

[Ralith]

It's definitely weird and awkward, and if there was authoritative information that the alignment implications of the pointer type could be ignored then I'd go for that in a heartbeat.

[MaikKlein]

I think &[u8] would be better but because Vulkan uses &[u32]I think that is what we should use. Honestly it doesn't really matter that much.

The PR looks fine to me, if there are no objections I am going to merge it tomorrow. And @MatusT thank you for all the hard work.

[MaikKlein]

length_type seems unnecessary. I think you can just cast with self.foo = foo.len() as _, I'll address it myself

[kocsis1david]

VkPipelineMultisampleStateCreateInfo.pSampleMask is an array of rasterizationSamples bits, so it shouldn't be a reference I guess, but I don't understand why Vulkan has rasterizationSamples as an enum rather than integer.
Also it's nullable, so just a simple slice is not very good.

[kocsis1david]

One problem with VkPipelineViewportStateCreateInfo: If you want to use dynamic state, the viewportCount has to be filled, but the pViewports should be NULL.

[MatusT]

@kocsis1david
VkPipelineMultisampleStateCreateInfo.pSampleMask: good catch. It seems that this is because I was filtering out members which I shouldn't have done. When you look at documentation and/or vk.xml there is: <member optional="true" len="latexmath:[\lceil{\mathit{rasterizationSamples} \over 32}\rceil]" altlen="(rasterizationSamples + 31) / 32">. It is one of two members using latexmath, the other being codeSize in ShaderModule, and I don't think there is any general solution except ignoring the latexmath and special casing some members as needed.

VkPipelineViewportStateCreateInfo.viewportCount: now this is quite a problem. Because it was agreed that we filter out count members that are associated with array members and as such can be removed. If there are no more of these cases except pViewports(and pScissors) a special case could be added.

TL;DR: if there are no more of these cases => treat them as special cases and let's add manual overrides similar to pCode and pCodeSize. @MaikKlein @omni-viral @Ralith Sounds ok?

[kocsis1david]

One more thing is dynamic_state inside the pipeline state is also nullable, a simple reference setter doesn't allows setting it to null. But as a workaround, you can leave it out from the builder calls and its default will be null.

[MaikKlein]

if there are no more of these cases => treat them as special cases and let's add manual overrides similar to pCode and pCodeSize.

I think you meant no more cases? Yes definitively, I kind of expect a few more special cases to pop up. I think it would probably make more sense to catch those setters by name, don't generate any code for them, and then implement everything manually.

[Ralith]

Automatically generating everything according to general rules is never quite going to work out right; a small proportion of special cases is expected and absolutely fine.

[kocsis1david]

DescriptorSetLayoutBinding has no setter for descriptor_count

BTW, I hope you don't mind if I write my findings here or is there a better place?

[MaikKlein]

@kocsis1david It is okay, at least everything is still in one place.

[MatusT]

So far I fixed:

• counts: viewportCount, scissorCount, descriptorCount (there is an array in the code where you can just add any additional counts you find)
• sampleMask as an additional special case

Should I open additional PRs for builder fixes?

[MatusT]

The Bool32 -> bool is implemented as well.

[zakarumych]

Builders for types with pNext chain should has a method that takes reference to any of suitable structures.
How it can look from API standpoint:

• In form of fn next(&mut self, &T) where T: X. Where X is an unsafe trait specific to each builder type.

  impl SubmitInfoBuilder {
    fn next<T>(&mut self, next: &T) -> &mut Self where T: SubmitInfoNext {
      self.inner.p_next = next;
      self.
    }
  }
  

• In form of multiple fn next_*(&mut self, &NextType) methods.

[Ralith]

That's a pretty cool idea, but I don't think it should block getting the badly needed next release out the door.

[MaikKlein]

@omni-viral Seems doable

I looked at the vk.xml

        <type category="struct" name="VkDebugReportCallbackCreateInfoEXT" structextends="VkInstanceCreateInfo">
            <member values="VK_STRUCTURE_TYPE_DEBUG_REPORT_CALLBACK_CREATE_INFO_EXT"><type>VkStructureType</type> <name>sType</name></member>
            <member>const <type>void</type>*                      <name>pNext</name></member>
            <member optional="true"><type>VkDebugReportFlagsEXT</type>            <name>flags</name><comment>Indicates which events call this callback</comment></member>
            <member><type>PFN_vkDebugReportCallbackEXT</type>     <name>pfnCallback</name><comment>Function pointer of a callback function</comment></member>
            <member optional="true"><type>void</type>*            <name>pUserData</name><comment>User data provided to callback function</comment></member>
        </type>

I think we can rely on structextends to indicate the p_next.

[zakarumych]

@Ralith thanks. It's just an idea. It isn't blocking. I just figured it is good place to write it down 😄

[MaikKlein]

@MatusT Yes PRs would be appreciated.

Lets open separate issues, Github starts to get slow.