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

Fix shader language float literal precision truncation #78972

Merged
merged 1 commit into from
Jul 7, 2023

Conversation

bitsawer
Copy link
Member

@bitsawer bitsawer commented Jul 3, 2023

Fixes #77325
Fixes #78896

Related issue, but not affected by this PR: #78204

This PR simply replaces rtoss() with rtos(), which behaves much better with float/double precision and is also much more used in the Godot codebase. After this PR, rtoss() will only be used in one place in variant_parser.cpp. This should most likely also replaced with a better behaving version. However, the remaining issue is the underlying String::num_scientific(), which can sometimes drop too many decimals from float or double values, as documented in #78204.

Currently, the float literal conversion between Godot shader language and the final GLSL version can drop the float decimal part accuracy dramatically, which breaks many otherwise valid GLSL tutorials, demos and libraries.

Original Godot (or GLSL) shader code:

float rand(vec2 x) {
    return fract(cos(mod(dot(x, vec2(13.9898, 8.141)), 3.14)) * 43758.5453);
}

Raw GLSL code generated from Godot Shader Language (notice the dropped decimals in the 43758.5):

float m_rand(vec2 m_x) {
    return fract((cos(mod(dot(m_x, vec2(13.9898,8.141)), 3.14)) * 43758.5));
}

Raw GLSL after this PR (notice the improved precision, for example 43758.546875 instead of just 43758.5):

float m_rand(vec2 m_x) {
    return fract((cos(mod(dot(m_x, vec2(13.989800453186,8.14099979400635)), 3.14000010490417)) * 43758.546875));
}

Precision is very important for many use cases, for example that pretty famous (if not very portable) code snippet uses fract() to extract the float fractional part and preserving as much precision as possible is important here. Many existing GLSL / HLSL demos and tutorials currently suffer from this float literal precision loss when porting them to Godot, sometimes drastically, sometimes more subtly.

In many cases this PR slightly increases the length of each float literal in the final shader code, but the added size and parsing performance impact should still be very minor. If we want to be real resource misers with this, we could maybe try to limit the decimals to a smaller float precision (for example calling String::num(x, 10) etc. directly), but I don't think it's necessary. Also assuming that we don't want to support GLSL 4.0 style lf/LF double literals in the future.

This PR should not directly break backwards compatibility. However, this increased precision can change existing user shader code behavior, especially if people have already tweaked their code to work around this precision loss.

Generated noise values before this PR:

noise_before

After this PR:

noise_after

@bitsawer bitsawer requested a review from a team as a code owner July 3, 2023 10:03
@Calinou Calinou added this to the 4.2 milestone Jul 3, 2023
@Calinou Calinou added the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Jul 3, 2023
Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

Looks good. For most numbers the generated string shouldn't be much longer anyway. And in those few cases where it is, we should definitely err on the side of having more decimals rather than fewer

@akien-mga akien-mga merged commit b2ada1b into godotengine:master Jul 7, 2023
13 checks passed
@akien-mga
Copy link
Member

Thanks!

@bitsawer bitsawer deleted the fix_shader_float_precision branch July 7, 2023 13:24
@YuriSizov YuriSizov removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Jul 10, 2023
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.1.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants