ScanMountGoat / wgsl_to_wgpu

Generate typesafe Rust bindings from WGSL shaders to wgpu
MIT License
42 stars 12 forks source link

Generate entry point constants and VertexState functions #31

Closed AndrewBrownK closed 1 year ago

AndrewBrownK commented 1 year ago

I think this captures what was described in the discussion at https://github.com/ScanMountGoat/wgsl_to_wgpu/pull/30

Code that generates the entire wgpu::VertexState for each vertex entry point in the shader module. This would probably need to be a function since the step modes need to be provided by the user.

AndrewBrownK commented 1 year ago

erk, actually this has problems still.

We can't accept the VertexStepModes as arguments and invoke the vertex_buffer_layout functions on our own, due to the lifetime requirements of the array of VertexBufferLayout in VertexState. However if the user of the functions generating VertexStates has to provide their own array of VertexBufferLayouts, then we are not really saving them any complexity (e.g. ensuring the right layouts are paired with the right entry function name).

I think we can get work around this if we are comfortable adopting pub const INFERRED_BUFFER_LAYOUTS seen in https://github.com/ScanMountGoat/wgsl_to_wgpu/pull/30, but that would presume the VertexStepModes instead of accepting them as parameters. If we don't want to go the inferred route then we might have to scrap this idea for now.

ScanMountGoat commented 1 year ago

We can't accept the VertexStepModes as arguments and invoke the vertex_buffer_layout functions on our own, due to the lifetime requirements of the array of VertexBufferLayout in VertexState.

I'm getting an error about referencing a local variable that gets dropped when the function returns. This should be an easy fix though. Just create a temporary variable that owns the buffers and pass that as a reference to the function instead. The function for each entry point will return VertexEntry instead of the entire VertexState. The usage should look almost identical for the user. Feel free to adjust things slightly as needed.

struct VertexEntry<const N: usize> {
    entry_point: &'static str,
    buffers: [wgpu::VertexBufferLayout<'static>; N],
}

fn vertex_state<'a, const N: usize>(
    module: &'a wgpu::ShaderModule,
    entry: &'a VertexEntry<N>,
) -> wgpu::VertexState<'a> {
    wgpu::VertexState {
        module,
        entry_point: entry.entry_point,
        buffers: &entry.buffers,
    }
}

The user could call it like vertex: vertex_state(&module, &vs_main_vertex_entry(step_mode_params))

ScanMountGoat commented 1 year ago

@AndrewBrownK Just wanting to check if there was still interest in working on this or if you had any questions. I'll probably end up squashing and merging, so I wouldn't worry about creating a clean history when resolving the conflicts.

AndrewBrownK commented 1 year ago

@ScanMountGoat sorry for the absence. Here are some thoughts for disucssion. We could do the vertex_state function and VertexEntry struct (with static lifetimes) like you describe, but it doesn't quite sit right for me.

Some things I like:

ScanMountGoat commented 1 year ago

This is an example of the the code you have to type now.

vertex: wgpu::VertexState {
    module: &module,
    entry_point: "vs_main",
    buffers: &[
        shader::model::VertexInput::vertex_buffer_layout(wgpu::VertexStepMode::Vertex),
        shader::model::InstanceInput::vertex_buffer_layout(wgpu::VertexStepMode::Instance),
    ],
},

This would be after the suggested changes. The user no longer needs to specify the entry point string or the number and types of the vertex input structs.

vertex: shader::vertex_state(
    &module,
    &shader::vs_main_entry(wgpu::VertexStepMode::Vertex, wgpu::VertexStepMode::Instance),
),

wgsl_to_wgpu would just need to generate something like this for each vertex entry point.

fn vs_main_entry(
    vertex_input: wgpu::VertexStepMode,
    instance_input: wgpu::VertexStepMode,
) -> VertexEntry<2> {
    VertexEntry {
        entry_point: "vs_main",
        buffers: [
            shader::model::VertexInput::vertex_buffer_layout(vertex_input),
            shader::model::InstanceInput::vertex_buffer_layout(instance_input),
        ],
    }
}
AndrewBrownK commented 1 year ago

Okay I see now. I can implement this shortly

AndrewBrownK commented 1 year ago

I think we got it! Thanks for the sharp insights and detailed examples

ScanMountGoat commented 1 year ago

Nice! The code changes look good. I'd like at least one test case in lib.rs and then this will be ready to merge. There may also be some other tests cases that need to be updated. I would look at the write_vertex_module tests for inspiration for how to test the output of the vertex_states function.