bevyengine / bevy

A refreshingly simple data-driven game engine built in Rust
https://bevyengine.org
Apache License 2.0
36.04k stars 3.56k forks source link

Ensure Bevy's rendering byte usage is little-endian #15701

Open alice-i-cecile opened 3 weeks ago

alice-i-cecile commented 3 weeks ago

I got the following response from a wgpu maintainer on Discord:

fwiw wasm is defined to be little endian, so if you're in the browser it's the browsers job to ensure everything looks like the world is little endian. Furthermore wgsl requires everything to be little-endian. But for all your native host code all bets are off, if you're on BE system it is indeed your job to handle that

The way I interpret that is that I was originally right and it is indeed our responsibility to provide data to wgpu as little endian, not native endian. So le_bytes it is, then.

In this case, it might be nice to make a follow-up PR to fix any remaining uses of ne_bytes in Bevy.

Originally posted by @inodentry in https://github.com/bevyengine/bevy/issues/10392#issuecomment-2396819418

clarfonthey commented 3 weeks ago

Also adding in my comment as well:

Also, this file is the one that has ne_bytes instead of le_bytes like I was mentioning: https://github.com/bevyengine/bevy/blob/main/crates/bevy_render/src/texture/image_texture_conversion.rs

Can probably be a later change, but it should be fixed to always cast to little endian instead of the native endian.

Should be pretty easy to just add a config-gated block on each of these that swaps the bytes if you're on a big-endian target.

BD103 commented 3 weeks ago

It looks like ne_bytes() methods are used in 4 different files, using this search:

https://github.com/bevyengine/bevy/blob/90c6f243717b27b6577423c596fa5687b3d8b2dc/crates/bevy_math/src/float_ord.rs#L76-L84

https://github.com/bevyengine/bevy/blob/90c6f243717b27b6577423c596fa5687b3d8b2dc/crates/bevy_image/src/hdr_texture_loader.rs#L57-L60

https://github.com/bevyengine/bevy/blob/90c6f243717b27b6577423c596fa5687b3d8b2dc/crates/bevy_image/src/image_texture_conversion.rs#L122-L125

https://github.com/bevyengine/bevy/blob/90c6f243717b27b6577423c596fa5687b3d8b2dc/crates/bevy_render/src/diagnostic/internal.rs#L477

https://github.com/bevyengine/bevy/blob/90c6f243717b27b6577423c596fa5687b3d8b2dc/crates/bevy_render/src/diagnostic/internal.rs#L484

clarfonthey commented 3 weeks ago

That one in bevy_math is fine since it's computing a hash, but it does look like the other ones also need to be changed.

BD103 commented 3 weeks ago

Quoting @clarfonthey on #15750:

So, this is a strict improvement, but it doesn't fix the issue completely.

There are plenty of places where bytemuck is used to cast slices directly without going through these methods and those need to be fixed too.

So, I'm fine approving, since this is an improvement, but under the condition the original issue stays open. (Right now it would get auto closed by this PR.)

This issue is still open because there is some bytemuck usage that still uses native endian-ness.