-
Notifications
You must be signed in to change notification settings - Fork 935
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
[glsl-out] Incorrect rounding for max-float constant expression #4568
Comments
The difference between let f32max = 3.4028235e38_f32;
println!("f64: {:?}", f32max as f64); // What glsl-out effectively did before gfx-rs/naga#2266, outputs "3.4028234663852886e38"
println!("f32: {:?}", f32max); // What glsl-out does now, outputs "3.4028235e38"
println!("Display: {}.0", f32max); // What metal-out does, outputs "340282350000000000000000000000000000000.0" But while different in string format, parsed as 32-bit floating point numbers by rustc, the three numbers are identical. Check https://evanw.github.io/float-toy/ or the below snippet: let a = 3.4028234663852886e38_f32;
let b = 3.4028235e38_f32;
let c = 340282350000000000000000000000000000000.0_f32;
println!("a: {:0>32b}", a.to_bits()); // "01111111011111111111111111111111"
println!("b: {:0>32b}", b.to_bits()); // "01111111011111111111111111111111"
println!("c: {:0>32b}", c.to_bits()); // "01111111011111111111111111111111"
println!("{}, {}", a == b, b == c); // "true, true" Here the bits
Since we've found an observable change in behaviour, we still need to understand what is happening. @Wumpf To determine if the format makes any difference, could you try |
thanks for the detailed analysis and providing a test case @fornwall! I didn't build a simple repro case on my side, so the instructions to see the thing are somewhat.. involved. For reference, this is the commit I tried against rerun-io/rerun@a395e5a |
### What * Depends on emilk/egui#3253 * allows for a bit nicer callback handling on our side. Still not amazing but quite a bit better I reckon (no more locks and additional ref counting on our side!). I was hoping to use the new, more flexible `shared_paint_callback_resources` to store `ViewBuilder`, but it can't be accessed without a handle to the eframe renderer. The end goal would be to get rid of re_renderer's `per_frame_data_helper`. This will likely be enabled with wgpu's "Arcanization" effort as this will eliminate `wgpu::RenderPass`'s lifetime dependency. In that case we could store a `Mutex<ViewBuilder>` on `ReRendererCallback` 🤔 * For the forseeable future we'll have to enable `fragile-send-sync-non-atomic-wasm` on wgpu (luckily egui itself no longer has this restriction). The problem is that a lot of our infrastructure is built around assuming `Send`/`Sync` whereas in actuality webgpu rendering resources can't be shared accross threads easily. * Had to (discover and) work around https://github.com/gfx-rs/naga/issues/2436 ### Checklist * [x] I have read and agree to [Contributor Guide](https://github.com/rerun-io/rerun/blob/main/CONTRIBUTING.md) and the [Code of Conduct](https://github.com/rerun-io/rerun/blob/main/CODE_OF_CONDUCT.md) * [x] I've included a screenshot or gif (if applicable) * [x] I have tested [demo.rerun.io](https://demo.rerun.io/pr/2980) (if applicable) * [x] Test WebGPU build - [PR Build Summary](https://build.rerun.io/pr/2980) - [Docs preview](https://rerun.io/preview/pr%3Aandreas%2Fupdate-wgpu/docs) - [Examples preview](https://rerun.io/preview/pr%3Aandreas%2Fupdate-wgpu/examples)
A stackoverflow question shows that there a WebGL2 implementation computed a value as the equivalent 64-bit floating point code in C++ would have, which seems to be allowed behaviour. Related: gpuweb/gpuweb#3431, As I understand it the previous |
If the problem is that webgl uses 64bit floats that would imply in this case that...
So that would mean that if we always set the 64bit representation of floats for constants in Glsl as it does on the branch you provided we should be at least better off? I mean the problem boils down to Agreed that having limit constants in wgsl would be preferable in any case, but arguably this is an orthogonal problem to the one of "constant gets the wrong value assigned" |
In a webgl2 shader, it seems like indeed As said above, this is in contrast to treating the numbers are 32-bit floats, for instance in rust: So the effective behaviour is like the shader uses f64 in computations: let value_from_texture = 3.4028235e38_f32 as f64;
let f32max: f64 = 3.4028235e38_f32; which causes Using f64 literals would indeed solve this problem. Are there any potential drawbacks? Let's create a PR to raise visibility and perhaps get more eyes on it (I can do it later today). |
I think we are running into rust-lang/rust#63171 whereas previously we were passing an f64 for formatting which was causing the fmt function to give us more digits. The full value of |
In my case Also found https://float.exposed/0x7f7fffff which is a nice tool for looking at representations of floats. Not sure what a proper solution for this is, maybe we should just cast to |
Created gfx-rs/naga#2443, adding the |
@teoxoy Interesting! So having |
No, I'm also getting the blue triangle. The result of If there is a syntax error in the shader (set |
I'm leaning towards merging gfx-rs/naga#2443, as it
We can always revisit if there is a related issue or more information in the future. |
Is it only an issue in the GLSL backend? Potentially, all other backends (besides SPV) have the same issue. Or does their rounding match rust's? f64 also most likely has the same issue. I was trying to figure out yesterday if we can somehow tell rust's formatting to give us one more digit than it thinks its needed since I think that would fix the issue but couldn't find how to do it. There is a way to specify the total nr of digits and also the digits after the period but we can't get the estimate rust is using in the fmt function. |
The IEEE 754-2019 spec defines H as being the minimum nr of significant digits needed to perform the conversion. For Maybe we can instead use the exponential formatting rust provides and pass 11 & 19 for the nr of digits after the period. The downside seems to be that insignficant 0's are still written out. |
The output of fn compute() -> f32 { return 3.40282346639e38; } chrome fails (test here) with the below error:
It seems like chrome effectively does something like: let num = "3.40282346639e38".parse::<f64>().unwrap();
if num > (f32::MAX as f64) { // Fail with "{num} cannot be represented as 'f32'". Is chrome doing the correct thing here? Looking at 6.2.1. Abstract Numeric Types, I guess so?
I guess this is something that will be fixed in naga with (or after) const expressions, to propagate Abstract{Float,Int} instead of resolving them during parsing? Anyway, for wgsl output, |
I'm starting to think the most correct way to fix this is to output the float as its binary representation (via |
@fornwall would you be up for updating our text backends (glsl, hlsl, msl & wgsl) with the solution above; as opposed to going with gfx-rs/naga#2443? |
@teoxoy Do you mean a change like this for glsl: -crate::Literal::F32(value) => write!(self.out, "{:?}", value)?,
+crate::Literal::F32(value) => write!(self.out, "uintBitsToFloat({}u)", value.to_bits())?, causing changes like this: -const vec3 const2_ = vec3(0.0, 1.0, 2.0);
+const vec3 const2_ = vec3(uintBitsToFloat(0u), uintBitsToFloat(1065353216u), uintBitsToFloat(1073741824u)); ? That does not pass
|
What tint ends up doing for at least glsl template <typename T>
StringStream& EmitFloat(const T& value) {
// Try printing the float in fixed point, with a smallish limit on the precision
std::stringstream fixed;
fixed.flags(fixed.flags() | std::ios_base::showpoint | std::ios_base::fixed);
fixed.imbue(std::locale::classic());
fixed.precision(20);
fixed << value;
std::string str = fixed.str();
// If this string can be parsed without loss of information, use it.
// (Use double here to dodge a bug in older libc++ versions which would incorrectly read
// back FLT_MAX as INF.)
double roundtripped;
fixed >> roundtripped;
// Strip trailing zeros from the number.
auto float_equal_no_warning = std::equal_to<T>();
if (float_equal_no_warning(value, static_cast<T>(roundtripped))) {
while (str.length() >= 2 && str[str.size() - 1] == '0' && str[str.size() - 2] != '.') {
str.pop_back();
}
sstream_ << str;
return *this;
}
// Resort to scientific, with the minimum precision needed to preserve the whole float
std::stringstream sci;
sci.imbue(std::locale::classic());
sci.precision(std::numeric_limits<T>::max_digits10);
sci << value;
sstream_ << sci.str();
return *this;
} That is, after having special cased infinity and NaN: void PrintF32(StringStream& out, float value) {
if (std::isinf(value)) {
out << "0.0f " << (value >= 0 ? "/* inf */" : "/* -inf */");
} else if (std::isnan(value)) {
out << "0.0f /* nan */";
} else {
out << tint::writer::FloatToString(value) << "f"; // Ends up calling EmitFloat() above
}
} |
This is surprising to me. The GLSL spec (3.20.6) says that a constant expression can be:
This seems to date all the way back to GLSL ES 1.0. |
In v0.12 (prior to gfx-rs/naga#2266), this wgsl constant
const f32max = 0x1.fffffep+127;
would inline in glsl as
3.4028234663852886e38
.However, in v0.13 this results into a glsl constant
const float f32max = 3.4028235e38;
When testing a provided max-float value (read from a texture in my case) for
>=f32max
this will now yieldfalse
in a WebGL setup.Interestingly, this scenario works fine on Metal where this yields
constant float f32max = 340282350000000000000000000000000000000.0;
. I suspect that glsl applies different rounding here.More backends and situations might be affected by this, but I was able to verify it only so far for glsl-out and rule it out for msl-out. It would be worth exploring the effect on other backends as well and find out if this has any other accuracy problems.
The text was updated successfully, but these errors were encountered: