Skip to content

Always use repr(C) for vertices. #79

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

PieroV
Copy link

@PieroV PieroV commented Apr 6, 2025

I think it'd be useful to always give the guarantee that vertices are repr(C), not only with vulkano.

@simnalamburt
Copy link
Owner

🤔 I wonder if it's breaking change or not

@PieroV
Copy link
Author

PieroV commented Apr 6, 2025

FWIW, tests passes, and when using vulkano you already have this representation.

@PieroV
Copy link
Author

PieroV commented Apr 6, 2025

TBH, I think also bytemuck without Volcano would be great 🙂.

@simnalamburt
Copy link
Owner

bytemuck without Volcano would be great

I think this should be possible.

always give the guarantee that vertices are repr(C)

Would you mind if I asked where you're using the repr(C) attribute? Does GLSL require repr(C) condition for some reason?

@PieroV
Copy link
Author

PieroV commented Apr 7, 2025

always give the guarantee that vertices are repr(C)

Would you mind if I asked where you're using the repr(C) attribute? Does GLSL require repr(C) condition for some reason?

No, it doesn't.
However, the Rust representation on my machine for the various vertices is the same as repr(C) (e.g., TexturedVertex is 36 bytes, the same as 9 f32).
However, it seems to me that with repr(C) you're more sure you can pass Vec<V> (with V a vertex type) slices to glBufferData.
From repr(C) in the Rustonomicon:

This [repr(C)] is also necessary to soundly do more elaborate tricks with data layout such as reinterpreting values as a different type.

Hence the request.

Also, you are also sure you can send these types through FFI in this way.
For example, nalgebra also uses #[repr(C)], even though not strictly needed.

For my case, using bytemuck would also work, but for now I had to use reinterpret, instead.

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 this pull request may close these issues.

2 participants