Fix depth_bias and build errors on less capable platforms #17079
Conversation
|
Welcome, new contributor! Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨ |
alice-i-cecile
added
C-Bug
|
Out of curiosity, which platform + OS were you compiling for? |
|
ESP32 microcontroller. It runs FreeRTOS and have a compatibility layer for std. The compiler target_os config is the following: #[cfg(target_os = "espidf")]I don't think bevy should have any traces of this platform thats why I thought its a better idea to just specify the OSes that actually have support for ctrlc. |
| @@ -1238,7 +1238,7 @@ impl From<&StandardMaterial> for StandardMaterialKey { | |||
| } | |||
|
|
|||
| key.insert(StandardMaterialKey::from_bits_retain( | |||
| (material.depth_bias as u64) << STANDARD_MATERIAL_KEY_DEPTH_BIAS_SHIFT, | |||
| (material.depth_bias as i32 as u64) << STANDARD_MATERIAL_KEY_DEPTH_BIAS_SHIFT, | |||
There was a problem hiding this comment.
Why is this correct?
There was a problem hiding this comment.
The depth_bias is actually stored as an i32 past this step, the i32 value will go to wgpu. Casting from f32 to an unsigned will cause it to lose its value when its negative and will not work well with i32 out of range values(e.g. f32::INFINITY). If we cast to i32 first, it will then cast to u64 it will retain the full range of i32 and will be passed to wgpu correctly.
I also made you a small demo here to better understand the casting (left os old behaviour, right is new):
https://godbolt.org/z/d94h7nfYT
There was a problem hiding this comment.
Can you add a comment describing this? I'm fine if it's even just copy pasted from what you just wrote. It's not immediately obvious otherwise.
There was a problem hiding this comment.
I gave it some thought. I wouldn't copy my previous comment as is since it was made in the context of the current issue which may cause more confusion to a future contributor.
Instead I explained why the double casting is necessary to make sure its not removed accidentally by someone else:
// Casting to i32 first to ensure the full i32 range is preserved.
// (wgpu expects the depth_bias as an i32 when this is extracted in a later step)
crates/bevy_app/Cargo.toml
Outdated
| @@ -93,7 +93,7 @@ portable-atomic-util = { version = "0.2.4", features = [ | |||
| "alloc", | |||
| ], optional = true } | |||
|
|
|||
| [target.'cfg(not(target_arch = "wasm32"))'.dependencies] | |||
| [target.'cfg(any(target_os = "macos", target_os = "linux", target_os = "windows"))'.dependencies] | |||
There was a problem hiding this comment.
Will this work for all platforms supported by ctrlc? Looks like they use #[cfg(unix)] and #[cfg(windows)]:
#[cfg(unix)]
mod unix;
#[cfg(windows)]
mod windows;There was a problem hiding this comment.
Specifically, are there platforms that are #[cfg(unix)] that are not Linux or MacOS that we should worry about?
There was a problem hiding this comment.
We support FreeBSD and there are others like TvOS we could support.
There was a problem hiding this comment.
Changed all occurrences from
cfg(any(target_os = "macos", target_os = "linux", target_os = "windows"))to
cfg(any(unix, windows))Re-tested on Windows and Linux and CtrlC handler still works.
BenjaminBrienen
added
S-Ready-For-Final-Review
Objective
depth_biasis ignored onStandardMaterial#14169)Solutions
Testing
Also ran a modified version of 3d_scene example with the following results:
RED cube depth_bias: -1000.0
BLUE cube depth_bias: 0.0
RED cube depth_bias: -INF
BLUE cube depth_bias: 0.0
RED cube depth_bias: INF (case reported in #14169)
BLUE cube depth_bias: 0.0
(Im not completely sure whats going on with the shadows here, it seems like depth_bias has some affect to those aswell, if this is unintentional this issue was not introduced by this PR)