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

nv2a: Make multiplication by 0 match HW behavior. #1045

Merged

Conversation

abaire
Copy link
Contributor

@abaire abaire commented Jun 8, 2022

Fixes #1008
Fixes #1044

The nv2a returns 0 for anything multiplied by zero, including exceptional
values such as Inf and NaN. Desktop GPUs do not enforce this, leading to
conditions where NaNs wipe out calculations and lead to erroneous behavior.

Test
HW Results

@Shoegzer
Copy link

Shoegzer commented Jun 9, 2022

@abaire Based on your comment in #55 about the broken floor texture in Halo 2 being related to depth buffer precision, I assumed this PR would not fix that issue. I did test it to be sure, to no avail, in case it matters at all.

@abaire
Copy link
Contributor Author

abaire commented Jun 9, 2022

@abaire Based on your comment in #55 about the broken floor texture in Halo 2 being related to depth buffer precision, I assumed this PR would not fix that issue. I did test it to be sure, to no avail, in case it matters at all.

Thanks for checking, I would've been very surprised if this fixed the issue in Halo 2, it should really only affect calculations that end up dealing with multiplication of exceptional values (Inf/NaN/etc..) which did not appear to be the case when I briefly looked at #55.

@Triticum0
Copy link

This fixes the menu issues in american chopper 1 and 2

@Triticum0
Copy link

Triticum0 commented Jun 24, 2022

American Chopper

Before
Black screen
xemu-2022-06-24-13-53-50
xemu-2022-06-24-13-53-54
xemu-2022-06-24-13-53-56
After
xemu-2022-06-24-13-54-21
xemu-2022-06-24-13-54-23
xemu-2022-06-24-13-54-30
xemu-2022-06-24-13-54-32

@Triticum0
Copy link

American Chopper 2
Before
xemu-2022-06-24-13-52-20
xemu-2022-06-24-13-52-30
After
xemu-2022-06-24-13-52-03
xemu-2022-06-24-13-49-37

@Triticum0
Copy link

This pr fixes azurik - rise of perathia late game
Before Screenshot_40

After
Screenshot_42-1

// Unfortunately mix() falls victim to the same handling of exceptional
// (inf/NaN) handling as a multiply, so per-component comparisons are used
// to guarantee HW behavior (anything * 0 must == 0).
" vec4 zero_components = abs(sign(src0) * sign(src1));\n"
Copy link

@PatrickvL PatrickvL Jul 23, 2022

Choose a reason for hiding this comment

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

Why is there an abs() here if the only value checked for is 0.0 - is negative zero a case to cater for as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the intent was to cover the potential for a -0, in practice I believe sign would only return 0.0 so this is probably unnecessarily conservative.

Copy link

@PatrickvL PatrickvL left a comment

Choose a reason for hiding this comment

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

A genuine question

Fixes xemu-project#1008

The nv2a returns 0 for anything multiplied by zero, including exceptional
values such as Inf and NaN. Desktop GPUs do not enforce this, leading to
conditions where NaNs wipe out calculations and lead to erroneous behavior.

[Test](https://github.com/abaire/nxdk_vsh_tests/blob/main/src/tests/americasarmyshader.cpp)
[HW Results](https://github.com/abaire/nxdk_vsh_tests_golden_results/wiki/Results-AmericasArmyShader)
@abaire abaire force-pushed the fix/1008/fix_multiplication_by_zero branch from a342e47 to c6883ff Compare July 24, 2022 00:22
@Triticum0
Copy link

This also affects #1044 using this pr fixes the issue

@user18081972

This comment was marked as spam.

@Triticum0
Copy link

Also seems to fix Breakdown lighting issues https://www.youtube.com/watch?v=LQ5y4sdaA5s

@user18081972

This comment was marked as spam.

@Fabxx
Copy link
Contributor

Fabxx commented Mar 22, 2023

Maybe we should merge it

this is still incomplete yet since it works only at 1x resolution scale, and we don't know if there might be edge cases where graphics might change behavior like it happened before with a previous "version" of this PR. There's no need to rush if we have to grant stability.

@mborgerson mborgerson merged commit 9723b43 into xemu-project:master May 1, 2023
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.

Terminator Redemption Shadows Bug America's Army: Rise of a Soldier: Floor texture Broken
7 participants