Dynamically creating VertexBufferLayout

[dragly]

I am playing around with the idea of making a library on top of wgpu and naga that can dynamically generate shaders and render pipelines. Currently I am stuck on creating a VertexBufferLayout with a dynamic offset because the attributes in that struct are stored as a reference.

In particular, I am trying to compile the following function:

fn vertex_buffer_layout<'a>(shader_location_offset: u32) -> VertexBufferLayout<'a> {
    VertexBufferLayout {
        attributes: &[
            wgpu::VertexAttribute {
                format: wgpu::VertexFormat::Float32x3,
                offset: 0,
                shader_location: shader_location_offset + 0,
            },
            wgpu::VertexAttribute {
                format: wgpu::VertexFormat::Float32,
                offset: 3 * 4,
                shader_location: shader_location_offset + 1,
            },
        ],
        array_stride: 4 * 4 as wgpu::BufferAddress,
        step_mode: wgpu::VertexStepMode::Instance,
    }
}

However, it fails with the following error message:

error[E0515]: cannot return value referencing temporary value
   --> visula/examples/molecular_dynamics.rs:225:9
    |
225 |  /         VertexBufferLayout {
226 |  |             attributes: &[
    |  |__________________________-
227 | ||                 wgpu::VertexAttribute {
228 | ||                     format: wgpu::VertexFormat::Float32x3,
229 | ||                     offset: 0,
...   ||
236 | ||                 },
237 | ||             ],
    | ||_____________- temporary value created here
238 |  |             array_stride: 4 * 4 as wgpu::BufferAddress,
239 |  |             step_mode: wgpu::VertexStepMode::Instance,
240 |  |         }
    |  |_________^ returns a value referencing data owned by the current function

This is somewhat related to #1790, but the solution suggested there does unfortunately not apply here since the function is non-const now.

I am considering making a hacky workaround by returning the members of VertexBufferLayout separately and then reconstructing the object where the render pipeline is made, but figured I should ask here first in case I am missing someting obvious.

As a side-note: I am also curious to know the background for storing a reference to [VertexAttribute] in VertexBufferLayout as opposed to for instance storing the attributes as a Vec or using an Arena/Handle-design like I saw is used in naga. There are always so many tradeoffs with different patterns that it is a bit hard to know what to apply when. I would love to know more about why the current one is chosen in wgpu.

[kvark]

I faced this as well in Baryon, and there is a solution I found, which works if shader_location_offset is known at compile time: https://github.com/kvark/baryon/blob/8643ecc2712ecd2795cdae03d34b7bf8fe2b5b2f/src/lib.rs#L27

impl Vertex {
    const fn layout<const LOCATION: u32>() -> wgpu::VertexBufferLayout<'static> {
        wgpu::VertexBufferLayout {
            array_stride: mem::size_of::<Self>() as u64,
            step_mode: wgpu::VertexStepMode::Vertex,
            attributes: &[
              wgpu::VertexAttribute {
                format: wgpu::VertexFormat::Float32x3,
                offset: 0,
                shader_location: LOCATION,
              },
              wgpu::VertexAttribute {
                format: wgpu::VertexFormat::Float32x2,
                offset: 0,
                shader_location: LOCATION + 1,
              },
            ],
        }
    }
}

Would this wok for your case as well?

I am also curious to know the background for storing a reference to [VertexAttribute] in VertexBufferLayout as opposed to for instance storing the attributes as a Vec or using an Arena/Handle-design like I saw is used in naga. There are always so many tradeoffs with different patterns that it is a bit hard to know what to apply when. I would love to know more about why the current one is chosen in wgpu.

You are totally right. There are many solutions with trade-offs. We prioritize consistency. If it makes sense for most APIs to work with bare references, then we use them for all the APIs.

But this particular issue has been coming back to us more and more, so we may make an exception here.

[dragly]

Unfortunately, I do not know the shader_location_offset at compile time. I could maybe rewrite my current code to change that, but part of what I want down the line is to modify the shaders based on user input.

Thanks for the explanation. It definitely makes sense to use references in most cases, and it also makes for a nice API where it works - which is almost always :)

[CatCode79]

Personally, as a rust newbie, I struggled with those type of things and in the end I solved with the builder pattern.
Brutal copy and paste from my framework:

`#[derive(Clone, Debug)]
pub struct VertexStateBuilder<'a> {
vertex_state: wgpu::VertexState<'a>
}

impl<'a> VertexStateBuilder<'a> {
pub const DEFAULT_ENTRY_POINT: &'static str = "main";

pub fn new(module: &'a wgpu::ShaderModule) -> Self {
    Self {
        vertex_state: wgpu::VertexState {
            module,
            entry_point: VertexStateBuilder::DEFAULT_ENTRY_POINT,
            buffers: &[],
        },
    }
}

// [...] Others setter methods here

pub fn buffers(mut self, buffers: &'a [wgpu::VertexBufferLayout]) -> Self {
    self.vertex_state.buffers = buffers;
    self
}

pub fn build(self) -> wgpu::VertexState<'a> {
    self.vertex_state
}

}
`

(I would like to know why Github doesn't quote the rust code correcly...)

[dragly]

Thanks for the tip! Unfortunately, I think I end up in the same situation as before as long as the attributes are temporary in a function. At least, I still get a similar error:

error[E0515]: cannot return value referencing temporary value
   --> visula/examples/molecular_dynamics.rs:237:9
    |
225 |           let builder = builder.set_attributes(&[
    |  _______________________________________________-
226 | |             wgpu::VertexAttribute {
227 | |                 format: wgpu::VertexFormat::Float32x3,
228 | |                 offset: 0,
...   |
235 | |             },
236 | |         ]);
    | |_________- temporary value created here
237 |           builder.build()
    |           ^^^^^^^^^^^^^^^ returns a value referencing data owned by the current function

From this code:

pub struct VertexBufferLayoutBuilder<'a> {
    layout: VertexBufferLayout<'a>,
}

impl<'a> VertexBufferLayoutBuilder<'a> {
    pub fn new() -> Self {
        Self {
            layout: VertexBufferLayout {
                attributes: &[],
                array_stride: 0 as wgpu::BufferAddress,
                step_mode: wgpu::VertexStepMode::Vertex,
            }
        }
    }

    pub fn set_array_stride(mut self, array_stride: wgpu::BufferAddress) -> Self {
        self.layout.array_stride = array_stride;
        self
    }

    pub fn set_step_mode(mut self, step_mode: wgpu::VertexStepMode) -> Self {
        self.layout.step_mode = step_mode;
        self
    }

    pub fn set_attributes(mut self, attributes: &'a [VertexAttribute]) -> Self {
        self.layout.attributes = attributes;
        self
    }

    pub fn build(self) -> VertexBufferLayout<'a> {
        self.layout
    }
}

fn vertex_buffer_layout<'a>(shader_location_offset: u32) -> VertexBufferLayout<'a> {
    let builder = VertexBufferLayoutBuilder::new();
    let builder = builder.set_array_stride(4 * 4 as wgpu::BufferAddress);
    let builder = builder.set_step_mode(wgpu::VertexStepMode::Instance);
    let builder = builder.set_attributes(&[
        wgpu::VertexAttribute {
            format: wgpu::VertexFormat::Float32x3,
            offset: 0,
            shader_location: shader_location_offset + 0,
        },
        wgpu::VertexAttribute {
            format: wgpu::VertexFormat::Float32,
            offset: 3 * 4,
            shader_location: shader_location_offset + 1,
        },
    ]);
    builder.build()
}

But your idea got me thinking that I might be able to refactor my code so that the attributes live outside this function. Perhaps by making one function for the attributes, and one for the layout that takes the attributes as a parameter. I will give that a shot.

As for proper formatting in GitHub, it looks like you tried to use single backticks (`), while you need triple backticks (```). Your code got partially formatted because GitHub also looks for indentation and assumes that it means you have a code block. I think you can hit the three dots in the upper right corner of my post and click "Quote reply` to see how I applied the formatting.

[dragly]

Thanks again! The builder pattern works as long as I also

• store the attributes on the builder in a Vec until build is called
• wait with calling build until I am in the same scope as I make the render pipeline, ensuring the lifetime of the attributes.

Concretely, this code appears to work for me;

pub struct VertexBufferLayoutBuilder<'a> {
    layout: VertexBufferLayout<'a>,
    attributes: Vec<VertexAttribute>,
}

impl<'a> VertexBufferLayoutBuilder<'a> {
    pub fn new() -> Self {
        Self {
            attributes: vec![],
            layout: VertexBufferLayout {
                attributes: &[],
                array_stride: 0 as wgpu::BufferAddress,
                step_mode: wgpu::VertexStepMode::Vertex,
            }
        }
    }

    pub fn set_array_stride(mut self, array_stride: wgpu::BufferAddress) -> Self {
        self.layout.array_stride = array_stride;
        self
    }

    pub fn set_step_mode(mut self, step_mode: wgpu::VertexStepMode) -> Self {
        self.layout.step_mode = step_mode;
        self
    }

    pub fn set_attributes(mut self, attributes: Vec<VertexAttribute>) -> Self {
        self.attributes = attributes;
        self
    }

    pub fn build(&'a self) -> VertexBufferLayout<'a> {
        let mut layout = self.layout.clone();
        layout.attributes = &self.attributes;
        layout
    }
}

fn vertex_buffer_layout<'a>(shader_location_offset: u32) -> VertexBufferLayoutBuilder<'a> {
    let builder = VertexBufferLayoutBuilder::new();
    let builder = builder.set_array_stride(4 * 4 as wgpu::BufferAddress);
    let builder = builder.set_step_mode(wgpu::VertexStepMode::Instance);
    let builder = builder.set_attributes(vec![
        wgpu::VertexAttribute {
            format: wgpu::VertexFormat::Float32x3,
            offset: 0,
            shader_location: shader_location_offset + 0,
        },
        wgpu::VertexAttribute {
            format: wgpu::VertexFormat::Float32,
            offset: 3 * 4,
            shader_location: shader_location_offset + 1,
        },
    ]);
    builder
}

// ...
// where the layouts are collected
        let layout = vertex_buffer_layout(offset);
        self.layouts.push(layout); // self.layout is Vec<VertexBufferLayoutBuilder>
// ...

// where the layouts are used in the pipeline
        let layouts = self.layouts.iter().map(|layout| layout.build()).collect();
        let render_pipeline = device.create_render_pipeline(&wgpu::RenderPipelineDescriptor {
            label: Some("Render pipeline"),
            layout: Some(&pipeline_layout),
            vertex: wgpu::VertexState {
                module: &shader_module,
                entry_point: "vs_main",
                buffers: &layouts,
            },
// ...

[CatCode79]

But your idea got me thinking that I might be able to refactor my code so that the attributes live outside this function

Exact! That is the trick.

About your last solution maybe is good but, personally I don't like the lifetime here:
pub fn build(&'a self) -> ...
generally the less lifetime you use in rust the better.

Probably best is to pass the value given to set_attributes to the vertex_buffer_layout function
or
you can create statically this value before the function call (perhaps with lazy_static crate?) in order to have it ready.

Other resources of interest:
Derive Builder Crate
nannou_wgpu crate inside the nannou project

the nannou_wgpu crate is a pretty good layer upon wgpu, is plenty of builders and other code that makes wgpu friendlier and a little higher level: I like it!

PS: Thank for the ``` tip! So """pythonish"""!