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

Builtin vertex input fields appear in generated Rust structure #34

Open
Aern-do opened this issue Jul 19, 2024 · 8 comments
Open

Builtin vertex input fields appear in generated Rust structure #34

Aern-do opened this issue Jul 19, 2024 · 8 comments
Labels
bug Something isn't working

Comments

@Aern-do
Copy link

Aern-do commented Jul 19, 2024

I have this structure in a shader used as vertex shader input

struct VertexInput {
    @location(0) position: vec3<f32>,
    @location(1) texture_id: u32,

    @builtin(vertex_index) vertex_index: u32
}

@vertex
fn vs_main(in: VertexInput) -> VertexOutput { ... }

In the generated Rust code, the structure still contains the builtin field vertex_index

#[repr(C)]
#[derive(Debug, PartialEq, Clone, Copy)]
pub struct VertexInput {
    pub position: [f32; 4],
    pub texture_id: u32,
    pub vertex_index: u32,
}

However, vertex attributes do not have this field, and it appears only in the structure itself

pub const VERTEX_ATTRIBUTES: [wgpu::VertexAttribute; 2] = [
    wgpu::VertexAttribute {
        format: wgpu::VertexFormat::Float32x3,
        offset: std::mem::offset_of!(VertexInput, position) as u64,
        shader_location: 0,
    },
    wgpu::VertexAttribute {
        format: wgpu::VertexFormat::Uint32,
        offset: std::mem::offset_of!(VertexInput, texture_id) as u64,
        shader_location: 1,
    },
];
@Aern-do
Copy link
Author

Aern-do commented Jul 19, 2024

Also, in the generated structure, the position field is [f32; 4] instead of [f32; 3]. I'm not sure if this is correct behavior or not

@Swoorup
Copy link
Owner

Swoorup commented Jul 20, 2024

However, vertex attributes do not have this field, and it appears only in the structure itself

The vertex_index you are specifying is built in and doesn't need to be specified.

Also, in the generated structure, the position field is [f32; 4] instead of [f32; 3]. I'm not sure if this is correct behavior or not

That is the correct behaviour, it is automatically adding the padding bytes in this case. You can provide your own types, it will still ensure (through const assertions that position is 16 bytes in length).

@Swoorup Swoorup closed this as completed Jul 20, 2024
@Swoorup Swoorup reopened this Jul 20, 2024
@Swoorup
Copy link
Owner

Swoorup commented Jul 20, 2024

@Aern-do
Copy link
Author

Aern-do commented Jul 20, 2024

The vertex_index you are specifying is built in and doesn't need to be specified.

I understand that this is the intended behavior. However, the vertex_index should not be included in the generated structure, since it is not a user input.

@Swoorup
Copy link
Owner

Swoorup commented Jul 20, 2024

The vertex_index you are specifying is built in and doesn't need to be specified.

I understand that this is the intended behavior. However, the vertex_index should not be included in the generated structure, since it is not a user input.

Ah you're right. My caffeine is weak today. 😅

@Swoorup Swoorup added the bug Something isn't working label Jul 27, 2024
sytherax added a commit that referenced this issue Jul 27, 2024
@Swoorup
Copy link
Owner

Swoorup commented Jul 27, 2024

On second thought, generating vertex_index isn't necessary a bug. In places where this is used in eg: storage buffer, it will be treated as padding field, and the gpu fills this for you. I have made a change where constructors and Init structs will zero out this field.

The other part about the position being 16 bytes instead of 12 is likely a bug. Alignments are likely handled differently in vertex attrib. I'll need to play around with a working example to test that however.

@jullanggit
Copy link

Disclaimer

I don't know if this is the right place to put this (it does seem to be related), if you want to I can move this into a seperate issue:

Issue

For some reason the binding for this:

struct VertexInput {
    @builtin(vertex_index) vertex_index: u32,
};

is this:

#[repr(C)]
#[derive(Debug, PartialEq, Clone, Copy, encase::ShaderType)]
pub struct VertexInput {
    pub vertex_index: [u8; 0x4],
}

which is a bit weird, but wouldn't really be a problem, but encase doesn't implement ShaderSize for u8's, so this is a compiler error!

@Swoorup
Copy link
Owner

Swoorup commented Dec 22, 2024

Disclaimer

I don't know if this is the right place to put this (it does seem to be related), if you want to I can move this into a seperate issue:

Issue

For some reason the binding for this:

struct VertexInput {
    @builtin(vertex_index) vertex_index: u32,
};

is this:

#[repr(C)]
#[derive(Debug, PartialEq, Clone, Copy, encase::ShaderType)]
pub struct VertexInput {
    pub vertex_index: [u8; 0x4],
}

which is a bit weird, but wouldn't really be a problem, but encase doesn't implement ShaderSize for u8's, so this is a compiler error!

This is a separate issue. I haven't given much attention to encase but can look into it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants