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

Structs generated by vulkano-shaders can't be used in CpuAccessibleBuffers by default #1925

Closed
zrneely opened this issue Jul 16, 2022 · 2 comments · Fixed by #2117
Closed

Comments

@zrneely
Copy link

zrneely commented Jul 16, 2022

  • Version of vulkano: 0.29
  • OS: Windows 10
  • GPU (the selected PhysicalDevice): N/A
  • GPU Driver: N/A
  • A reasonably minimal complete main.rs file that demonstrates the issue:
// shaders/motion.glsl
struct Agent {
    vec2 position;
    float angle;
    float age;
};

layout(set = 1, binding = 0) buffer Data {
    Agent agents[];
} agents;

// ...

void main() {
  // ...
}
// main.rs
mod motion_shader {
  vulkano_shaders::shader! {
    ty: "compute",
    path: "shaders/motion.glsl",
  }
}

fn main() {
  // ... init
  let agents = vec![motion_shader::ty::Agent { position: [ 0f32, 0f32 ], angle: 0f32, age: 0f32 }];
  let agent_buffer = CpuAccessibleBuffer::from_iter(device.clone(), BufferUsage::all(), false, agents.into_iter()).unwrap();
}

Issue

I ran into this while upgrading an old project to vulkano 0.29. This code fails to compile because the generated motion_shader::ty::Agent doesn't implement bytemuck::Pod. Here's the types generated by the shader macro:

        #[repr(C)]
        #[allow(non_snake_case)]
        pub struct Agent {
            pub position: [f32; 2usize],
            pub angle: f32,
            pub age: f32,
        }
        #[automatically_derived]
        #[allow(unused_qualifications)]
        #[allow(non_snake_case)]
        impl ::core::marker::Copy for Agent {}
        impl Clone for Agent {
            fn clone(&self) -> Self {
                Agent {
                    position: self.position,
                    angle: self.angle,
                    age: self.age,
                }
            }
        }
        #[repr(C)]
        #[allow(non_snake_case)]
        pub struct Data {
            pub agents: [Agent],
        }

In this case, it seems logical that Agent should implement Pod (and Zeroable), and it's initially very surprising that shader-defined types can't be used in a CpuAccessibleBuffer. Of course the types are actually defined by macro-generated rust code, but the intuition is that they're defined in the shader itself.

Manually adding the impls is a good workaround and works:

    unsafe impl bytemuck::Zeroable for motion_shader::ty::Agent {}
    unsafe impl bytemuck::Pod for motion_shader::ty::Agent {}

but requires adding a dependency on bytemuck myself, and it's the weird sort of dependency that has to match the version depended on by a different dependency. Unfortunately the types_meta option of the shader macro doesn't work either since these are unsafe impls, which aren't recognized by the macro.

Manually implementing BufferContents for Data would work as well, but that requires writing a bunch of error-prone code and shouldn't be necessary.

A few possible improvements I can think of:

  • Re-export Pod and Zeroable from vulkano to avoid needing to take another dependency
  • Adding the ability to impl unsafe traits (or maybe just Pod and Zeroable in particular) via the types_meta option of the shader macro
  • Document this unexpected behavior, and how to get it to work, on either vulkano-shaders or the CpuAccessibleBuffer type in vulkano (or both) - or maybe even provide an example demonstrating this scenario.
@JMicheli
Copy link
Contributor

JMicheli commented Jul 16, 2022

I agree that modifying the shader! macro would be ideal, but I note that I have been able to use #[derive(Zeroable, Pod)] in the types_meta for my shaders without issue. I think that is the current best approach to solving this problem.

@Rua
Copy link
Contributor

Rua commented Jul 27, 2022

I agree that re-exporting the Bytemuck macros would be ideal, but unfortunately I ran into Lokathor/bytemuck#93 when I tried. I can see about automatically adding these traits though.

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

Successfully merging a pull request may close this issue.

3 participants