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

Remove NaN uniform #9174

Merged
merged 3 commits into from
Sep 23, 2024
Merged

Remove NaN uniform #9174

merged 3 commits into from
Sep 23, 2024

Conversation

Pessimistress
Copy link
Collaborator

Background

Avoid sending NaN as uniform as it is believed to cause issues in some browsers.

Int value -1 (all ones) is NaN when interpreted as float32: https://en.wikipedia.org/wiki/Single-precision_floating-point_format#Exponent_encoding

According intBitsToFloat this solution may subject to GPU-specific behavior:

If the encoding of a NaN is passed in x, it will not signal and the resulting value will be undefined.

Change List

  • Avoid sending NaN as uniform.

Copy link
Collaborator

@felixpalmer felixpalmer left a comment

Choose a reason for hiding this comment

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

Tested on M3 and working well (with this fix)

@coveralls
Copy link

coveralls commented Sep 23, 2024

Coverage Status

coverage: 91.758% (-0.001%) from 91.759%
when pulling 93dc2c5 on x/nan
into 03145fb on master.

Copy link
Collaborator

@ibgreen ibgreen left a comment

Choose a reason for hiding this comment

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

Strongly approve. The less we rely on NaN's the better. They are notorious for having surprising behavior and causing problems. (I'd personally try to find ways to avoid them inside the shaders too).

@Pessimistress Pessimistress changed the title For discussion: remove NaN uniform Remove NaN uniform Sep 23, 2024
@Pessimistress Pessimistress merged commit 9067f45 into master Sep 23, 2024
4 checks passed
@Pessimistress Pessimistress deleted the x/nan branch September 23, 2024 17:22
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.

4 participants