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

Automatically add padding fields? #50

Closed
Swoorup opened this issue Feb 2, 2024 · 6 comments
Closed

Automatically add padding fields? #50

Swoorup opened this issue Feb 2, 2024 · 6 comments

Comments

@Swoorup
Copy link

Swoorup commented Feb 2, 2024

First of all great crate, it makes working with wgpu much easier. A question I have is generating unaligned struct fields

struct QuadInstanceData {
    points: array<vec2<f32>, 4>,
    color: vec3<f32>,
}

generates

...
pub struct QuadInstanceData {
    pub points: [[f32; 2]; 4],
    pub color: [f32; 3],
}
const _: () = assert!(
    std::mem::size_of:: < QuadInstanceData > () == 48,
    "size of QuadInstanceData does not match WGSL"
);
const _: () = assert!(
    memoffset::offset_of!(QuadInstanceData, points) == 0,
    "offset of QuadInstanceData.points does not match WGSL"
);
const _: () = assert!(
    memoffset::offset_of!(QuadInstanceData, color) == 32,
    "offset of QuadInstanceData.color does not match WGSL"
);

which fails with

const _: () = assert!(
   |  _______________^
18 | |     std::mem::size_of:: < QuadInstanceData > () == 48,
19 | |     "size of QuadInstanceData does not match WGSL"
20 | | );
   | |_^ the evaluated program panicked at 'size of QuadInstanceData does not match WGSL', test-wgpu/quad_shader.rs:17:15

Isn't it possible to automatically add the padding fields to the generated code instead such that it is always correct? And it is upto the user to either use MaybeUnit or zero out the padding fields. In this case

pub struct QuadInstanceData {
    pub points: [[f32; 2]; 4],
    pub color: [f32; 3],
    pub _pad: f32,
}

would fix the issue

@ScanMountGoat
Copy link
Owner

ScanMountGoat commented Feb 2, 2024

While I could automatically add the padding field on the Rust side, I feel adding fields not part of the WGSL definition that are never read would be confusing. Using types like vec3 instead of vec4 doesn't save any space due to alignment requirements. You can either add padding fields to the WGSL struct, use vec4 for the WGSL fields, or use the encase derive instead of bytemuck to perform the padding at runtime.

WGSL defines the memory layout requirements for types that are shared between the CPU and GPU for storage and uniform buffers. wgsl_to_wgpu only requires Rust structs to match the layout in WGSL when using bytemuck. Vertex input structs don't generate those safety checks since the memory layout is defined by the vertex attributes and the requirements are different. I'll try and add an explanation to the readme, since this isn't obvious.

@Swoorup
Copy link
Author

Swoorup commented Feb 2, 2024

Hmm, I understand. Perhaps the feature can be enabled through another WriteOptions field. And padding field initialisation can be hidden behind a constructor 😄

The way I am working around is adding to wgsl as well so the build generates the padding field in rust as well. But sometimes I'll have to work out the offsets required myself.

@ScanMountGoat
Copy link
Owner

Vectors and scalars typically have alignment equal to their size. The only exception is vec3, which has the same alignment as vec4. As long as you stick to vec4 instead of vec3, you shouldn't have too many alignment issues. You can also use encase instead of bytemuck if you still want to use types like f32 or vec3 without worrying about padding.

For your specific example, I would just use vec4 since it's technically the same size after padding.

struct QuadInstanceData {
    points: array<vec2<f32>, 4>,
    color: vec4<f32>,
}

You could even do everything with vec4 and unpack in the shader.

struct QuadInstanceData {
    point01: vec4<f32>,
    point23: vec4<f32>,
    color: vec4<f32>,
}

https://crates.io/crates/encase
https://www.w3.org/TR/WGSL/#alignment-and-size

@ScanMountGoat ScanMountGoat closed this as not planned Won't fix, can't repro, duplicate, stale Feb 2, 2024
@Swoorup
Copy link
Author

Swoorup commented Feb 2, 2024

@ScanMountGoat if I want to use encase, and not bytemuck shouldn't the const assertions that complains about sizes/offsets not be generated then?

@ScanMountGoat
Copy link
Owner

Correct. There would be no assertions in that case.

@Swoorup
Copy link
Author

Swoorup commented Feb 2, 2024

I know this is closed, but in case you'd decide to revisit this later, I've added a working draft, to ensure it adds padding fields which makes all the const assertions pass.
#52

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants