Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Procedural Macros to create VertexBufferLayout #5621

Closed
tgross35 opened this issue Apr 28, 2024 · 5 comments
Closed

Procedural Macros to create VertexBufferLayout #5621

tgross35 opened this issue Apr 28, 2024 · 5 comments
Labels
area: api Issues related to API surface type: enhancement New feature or request

Comments

@tgross35
Copy link

Is your feature request related to a problem? Please describe.

I think it is fairly common to have code like this:

#[repr(C)]
#[derive(Copy, Clone, Debug, Pod, Zeroable)]
struct TessVertex {
    position: [f32; 2],
    normal: [f32; 2],
    prim_id: u32,
}

impl TessVertex {
    fn desc() -> wgpu::VertexBufferLayout<'static> {
        wgpu::VertexBufferLayout {
            array_stride: mem::size_of::<Self>() as wgpu::BufferAddress,
            step_mode: wgpu::VertexStepMode::Vertex,
            attributes: &[
                wgpu::VertexAttribute {
                    offset: 0,
                    shader_location: 0,
                    format: wgpu::VertexFormat::Float32x2,
                },
                wgpu::VertexAttribute {
                    offset: 8,
                    shader_location: 1,
                    format: wgpu::VertexFormat::Float32x2,
                },
                wgpu::VertexAttribute {
                    offset: 16,
                    shader_location: 2,
                    format: wgpu::VertexFormat::Uint32,
                },
            ],
        }
    }
}

This is a bit tedious to maintain but is fairly boilerplate, so a macro could help usability here.

Describe the solution you'd like

Probably extract the vertex format to a trait:

trait GpuFormat {
    const VERTEX_FORMAT: VertexFormat;
}

// implement this for relevant primitives
impl GpuFormat for f32 { const VERTEX_FORMAT: VertexFormat = VertexFormat::Float32; }
impl GpuFormat for [f32; 2] { const VERTEX_FORMAT: VertexFormat = VertexFormat::Float32x2; }
// ...

And then add a derive macro that leveragesGpuFormat and the recently-stabilized offset_of:

// trait to be implemented
trait BufferLayout {
    fn vertex_buffer_layout() -> VertexBufferLayout<'static>;
}

// usage

#[derive(BufferLayout)]
struct TessVertex {
    #[wgpu(shader_location = 0)]
    position: [f32; 2],
    #[wgpu(shader_location = 10)]
    normal: [f32; 2],
    #[wgpu(shader_location = 4)]
    prim_id: u32,
}

// code generated by the macro

impl BufferLayout for TessVertex {
    fn vertex_buffer_layout() -> VertexBufferLayout<'static> {
        VertexBufferLayout {
            array_stride: mem::size_of::<Self>() as wgpu::BufferAddress,
            step_mode: wgpu::VertexStepMode::Vertex,
            attributes: &[
                wgpu::VertexAttribute {
                    offset: std::mem::offset_of!(Self, position),
                    shader_location: 0,
                    format: [f32; 2]::VERTEX_FORMAT,
                },
                wgpu::VertexAttribute {
                    offset: std::mem::offset_of!(Self, normal),
                    shader_location: 10,
                    format: [f32; 2]::VERTEX_FORMAT,
                },
                wgpu::VertexAttribute {
                    offset: std::mem::offset_of!(Self, prim_id),
                    shader_location: 4,
                    format: u32::VERTEX_FORMAT,
                },
            ],
        }
    }
}

Note that this exact code above currently hits rust-lang/rust#124478, but that should be fixed soon in rust-lang/rust#124484.

Describe alternatives you've considered

It would be nice if more information could be reflected from the shader, rather than needing to keep the shader and the Rust code in sync (pretty big source of my errors when just getting started). Maybe an alternative to include_wgsl! could create a rust module with automatically created types for everything in WGSL shader?

I am sure this has been talked about somewhere :)

@cwfitzgerald cwfitzgerald added type: enhancement New feature or request area: api Issues related to API surface labels Apr 29, 2024
@villuna
Copy link
Contributor

villuna commented Apr 29, 2024

I also think this would be pretty cool (and also dunno if its been talked about before). It feels like something that could also be done by a separate crate, though it may be a good edition to the util module? I'd like to give it a crack

@teoxoy
Copy link
Member

teoxoy commented Apr 29, 2024

Do note that we have a vertex_attr_array! macro.

@tgross35
Copy link
Author

Agreed that this could probably be developed in a separate crate. I actually kind of like the wgsl reflection proc macro idea even more than just the derive macro. It could be invoked like this:

wgpu::wgsl_module! {
    file: "my_shader.wgsl", // file to load
    module: my_shader, // Rust module name to create
}

And the macro would:

  1. Use naga to load the file into a Module
  2. Create a Rust representation of all Types in the module
  3. For each of these types, implement a trait like the BufferLayout trait I suggested in the top post
  4. Make a function for all StructMembers with a binding to produce the bind specification
  5. Create a State struct that holds buffer objects
  6. Add methods on State that allow you to pass a queue and a slice of vertex/index objects to write_buffer

Of course not all of that would be necessary, a MVP would just be creating a Rust representation for any structs in the shader. But it would really be convenient to only define data structures once in the shader to get a strongly typed API on the Rust side. Out of sync layouts or mistakes sending data through untyped byte buffers is probably the biggest source of bugs and beginner unfriendliness.

@cwfitzgerald
Copy link
Member

Second that this is a very interesting idea, but definitely not in scope for the wgpu crate itself.

@Wumpf
Copy link
Member

Wumpf commented Apr 30, 2024

Thirding this and closing as won't fix.
There's a myriad of things that would be nice sugar on top of wgpu, but it's a slippery slope. vertex_attr_array goes a long way without hiding too much of the underlying functionality, this proposal here sounds a lot more complex and I really don't want to start adding derive macros to wgpu if we can avoid it :)

@Wumpf Wumpf closed this as not planned Won't fix, can't repro, duplicate, stale Apr 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: api Issues related to API surface type: enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants