Sanitize and use increased precision when normalizing light directions #10477
+12
−4
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I've looked over the fifoci differences and they all seem correct. shadow-eyes still shows an issue, but the problem that that case is looking at is that the eyes have As one more clarifying note, the issue being targeted here is for Mesa's llvmpipe, which is rendering on the CPU. I don't think most end-users are affected by this problem, but it still is annoying to not have correct rendering on FifoCI, and thus worth working around like this. |
Pokechu22
force-pushed
the
light-dir-double-normalize
branch
from
ec6a6ca
to
4669eaa
Compare
Pokechu22
force-pushed
the
light-dir-double-normalize
branch
from
4669eaa
to
48bb8a2
Compare
Pokechu22
force-pushed
the
light-dir-double-normalize
branch
from
48bb8a2
to
fa398a3
Compare
iwubcode
reviewed
Jun 5, 2022
Pokechu22
force-pushed
the
light-dir-double-normalize
branch
from
fa398a3
to
784c9cd
Compare
Pokechu22
force-pushed
the
light-dir-double-normalize
branch
2 times, most recently
from
71dea3c
to
a8f8c15
Compare
Pokechu22
force-pushed
the
light-dir-double-normalize
branch
from
a8f8c15
to
5108283
Compare
This normalization was added in 02ac5e9, and changed to use floats in 4bf031c. The conversion to floats means that sometimes there is insufficient precision for the normalization process, which results in values of NaN or infinity. Performing the whole process with doubles prevents that, but games also sometimes set the values to NaN or infinity directly (possibly accidentally due to the values not being initialized due to them not being used in the current configuration?). The version of Mesa currently in use on FifoCI (20.3.5) has issues with NaN. Although this bug has been fixed (https://gitlab.freedesktop.org/mesa/mesa/-/commit/b3f3287eac066eae16dce0e47aad3229dcff8257 in 21.2.0), FifoCI is stuck with the older version. This change may or may not be incorrect, but it should result in the same behavior as already present in Dolphin, while working around the Mesa bug.
Pokechu22
force-pushed
the
light-dir-double-normalize
branch
from
5108283
to
8129874
Compare
|
FifoCI detected that this change impacts graphical rendering. Here are the behavior differences detected by the system:
automated-fifoci-reporter |
This was referenced Aug 4, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
This normalization was added in 02ac5e9, and changed to use floats in 4bf031c. The conversion to floats means that sometimes there is insufficient precision for the normalization process, which results in values of NaN or infinity. Performing the whole process with doubles prevents that, but games also sometimes set the values to NaN or infinity directly (possibly accidentally due to the values not being initialized due to them not being used in the current configuration?).
The version of Mesa currently in use on FifoCI (20.3.5) has issues with NaN. Although this bug has been fixed (https://gitlab.freedesktop.org/mesa/mesa/-/commit/b3f3287eac066eae16dce0e47aad3229dcff8257 in 21.2.0), FifoCI is stuck with the older version.
This change may or may not be incorrect, but it should result in the same behavior as already present in Dolphin, while working around the Mesa bug.
This PR is an alternative to #10475 that does not introduce regressions of things fixed by #394, but also does not cause
mp3-bloomto be hardware-accurate. It would make sense to merge this as a stopgap to work around the Mesa issue while I research this behavior further.