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

x86/FPU: Always preserve sign in neg.s (Fix GT4 text) #10464

Merged
merged 2 commits into from
Dec 28, 2023
Merged

x86/FPU: Always preserve sign in neg.s (Fix GT4 text) #10464

merged 2 commits into from
Dec 28, 2023

Conversation

stenzek
Copy link
Contributor

@stenzek stenzek commented Dec 24, 2023

Description of Changes

Finally had the chance to look into this. The "Full Throttle" text getting stuck on screen seems to be due to the following block of instructions (NTSC-U, Version 1.01):

003A9820 sw   zero, 0x  14(a0)
003A9824 neg.s        f03, f00
003A9828 mul.s        f03, f03, f12
003A982C lwc1 f01, 0x   0(a0)
003A9830 mtc1 zero, f00
003A9834 lui  at, 0x3F80
003A9838 mtc1 at, f02
003A983C add.s        f01, f01, f03
003A9840 max.s        f00, f01, f00
003A9844 min.s        f00, f00, f02
003A9848 jr   ->ra
003A984C swc1      f00, 0x   0(a0)

Forcing full EE FPU clamping for this block fixes the text. But that's a janky solution, because it would mean finding the offsets for every release/revision. Instead, I noticed that we're using floating-point min/max, which means that negative NaNs get punted to positive FLT_MAX.

Notice the block starts with neg.s. With float clamping, +nan becomes +FLT_MAX, instead of actually changing the sign. So the result of the next multiplication would also have its sign inverted. This is what appears to cause the "Full Throttle" text to get stuck on the screen.

So, instead of using a higher clamp mode, we can just use integer clamping for neg.s. I can't think of why you'd actually want the sign of an input to not change, this behaviour seems very wrong, but it is a global change affecting the default clamping level, so there is some risk. But I'm hoping it should be fine.

Rationale behind Changes

Fixes "Full Throttle" text in Gran Turismo 4 without making license tests crash, which happens with full/double FPU.

Fixes split time display.

Also fixes the text in Gran Bikeismo (Tourist Trophy), which isn't surprising, since it's the same engine.

Suggested Testing Steps

Test games known to be sensitive to floating point accuracy.
Make sure GT4 text is fixed, and license tests aren't broken.

To check for save data getting broken:

  1. Boot your existing Gran Turismo 4 save and go to the GT mode.
  2. Enter Home -> Garage and attempt to select every car you own, one by one. If anything goes wrong, the game will throw an error message and refuse to pick that car.

@Nenkai
Copy link

Nenkai commented Dec 24, 2023

Something to keep in mind: This PR changes the default clamping mode for GT4 and has a possibility to break users's save data.
GT4's saving uses encryption with floats and clamping/rounding directly changes the decrypted output which is then checksum'ed and will lead to fail.

There should be a notice for that somewhere (and in general when changing modes specifically with this game). Apologies if this is not the proper place for this.

@refractionpcsx2
Copy link
Member

refractionpcsx2 commented Dec 24, 2023

if it breaks saves, then folks are gonna have to make a savestate on an older version when this merges or something, then load that and save on the new version. We can't not fix bug because a games behaviour will change, that's making everything suffer for 1 game for a temporary problem.

@stenzek
Copy link
Contributor Author

stenzek commented Dec 24, 2023

Silent did mention that. Unless the save routine specifically negates infinities or NaNs, this change won't break anything.

The clamping mode is unchanged. Just the broken behaviour for neg.s with normal clamping are changed.

For what it's worth, my own save seemed fine. But it's definitely something we should test before merging.

@CookiePLMonster
Copy link
Contributor

Also fixes the split times not showing - without this PR, Normal EE Clamping would not display + 0'04.707.
image

@stenzek stenzek merged commit 90a09eb into PCSX2:master Dec 28, 2023
13 checks passed
@stenzek stenzek deleted the gt4-text branch December 28, 2023 12:15
@qwertymaster617
Copy link

Thank you so very much for this fix! IIRC, this issue has been present for at least 10 years, if not closer to 15, and has long been one of the most common and annoying in the whole game. Finally having working split times and having that silly Get ready to drive/Full Throttle text finally disappear without hacks is such a fantastic Christmas gift for someone who still plays this game a lot after all these years.

Fantastic work! Thanks again!

Apologies for PR bumping.

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

Successfully merging this pull request may close these issues.

7 participants