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

(Premul)Rgba8::to_u32 now returns in little endian format. #77

Merged

Conversation

waywardmonkeys
Copy link
Collaborator

@waywardmonkeys waywardmonkeys commented Dec 1, 2024

This matches the WebGPU spec which states that the internal layout of values is such that values in host-shared buffers are stored in little-endian format: https://www.w3.org/TR/WGSL/#internal-value-layout

This matches the WebGPU spec which states that the internal layout
of values is such that values in host-shared buffers are stored in
little-endian format:

https://www.w3.org/TR/WGSL/#internal-value-layout
@waywardmonkeys
Copy link
Collaborator Author

I will have to update the Vello shaders when this winds its way through Peniko and into Vello.

This also makes it the same as the as_u32 function in bevy_color which also returns a little endian value.

Copy link
Contributor

@raphlinus raphlinus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, I think it makes sense to standardize on this ordering. Thanks!

I am wondering about conversions into Rgba8, but I think that's less urgent. I'm also wondering (out loud) whether the From trait might be appropriate here.

@waywardmonkeys waywardmonkeys added this pull request to the merge queue Dec 1, 2024
Merged via the queue into linebender:main with commit a4fa61a Dec 1, 2024
15 checks passed
@waywardmonkeys waywardmonkeys deleted the to_u32_little_endian branch December 1, 2024 21:49
Copy link
Member

@DJMcNab DJMcNab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This raises questions about the bytemuck implementations. Endianness is like affinity, in that it's really hard to reason about quickly, but I think the bytemuck implementation is big endian? Should we re-order the fields to make that also be little endian?

We should have a test that they result in the same value, at least.

@waywardmonkeys
Copy link
Collaborator Author

I would be more inclined to remove that particular bytemuck impl in favor of using the new to_u8__array and the other stdlib features.

@DJMcNab
Copy link
Member

DJMcNab commented Dec 2, 2024

Maybe? Certainly that would be reasonable for Rgb8, but I think for Rgba8 being able to cast slices would be useful (maybe we should give it an align of 4 as well?)

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.

3 participants