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

GS: Fix huge ST coordinates in input vertices. #12201

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

TJnotJT
Copy link

@TJnotJT TJnotJT commented Jan 16, 2025

Description of Changes

In the vertex trace, clamp floating point UV coordinates to valid UV ranges [-2047, 2047]. Add a pass to GSState::FlushPrims() that iterates through the primitivies about to be drawn and culls any that may be impossible to draw correctly (e.g., NaN values in ST coordinates) or replaces primitives with huge ST coordinates with a new primitive with clamped coordinates.

Rationale behind Changes

The input texture ST coordinates can sometimes be huge or NaN (for reasons not totally clear, it might be emulation or game bugs) so they cause issues with calculating the vertex trace values, determining texture vounding boxes, etc. This can cause graphical glitches downstream. Also, based on hardware tests it was determined that for truly large floating points values in ST (after divided by Q), they appeart to be clamped to +/-2047 when converting to UV (before the clamping/wrapping determined by the CLAMP register operates).

Suggested Testing Steps

Testing on GS dumps/games that are known to have huge ST coordinates as described above. I found a few examples of graphical issues that are caused in the SW renderer because of how the huge ST coords get clamped (first example below) . They were previously being masked because of the way the SW renderer it partially loads textures or cast floating point to integers in the rasterizer, which was being thrown off by the huge ST coordinates (so two problems were canceling each other out). Any help testing on more games/dumps or feedback on the changes would be greatly appreciated.

Currently these have been tested thouroughly on a large pack of GS dumps (provided by LightningTerror), with ta few of the more visible differences shown below. Some basic profiling was done on some of the GS dumps and the performance impact is similar to the GSVertexTraceFMM::FindMinMax(), since this is called once ever draw. It will have a larger impact on draws that have a large number of huge/infinite ST coordinates, which hopefully are not much.

In the example, before is left and after is right.

counter-terror
Should be a slight blurring effect. Unfortunately, fixing this issue causes another issue with the black patches. This should be "accurate" on the GS side but the input coordinates from the EE may not be in this dump. (Counter_Terrorist_Special_Forces_-_Fire_for_Effect_SLES-53046_20230830195729.gs.xz).

bakugan
Should not have double eyebrows (Bakugan_Battle_Brawlers_shadows.gs.xz).

psychonauts
Should have a faint star-like patterns (Psychonauts_SLUS-21120_20231013005852.gs.xz).

tomb_raider
Should have character's shadow (Tomb_Raider_-_Legend_SLUS-21203_20230905130255.gs.xz).

testdrives
Should not have dark road (testdriveswminmax.gs.xz).

@TJnotJT TJnotJT changed the title GS: Clamp huge ST coordinates before casting in GetTextureMinMax GS: Clamp ST coordinates before casting in GetTextureMinMax Jan 16, 2025
@github-actions github-actions bot added the GS label Jan 16, 2025
@TJnotJT TJnotJT force-pushed the clamp-huge-st branch 2 times, most recently from 28bd2ef to 9a9d41c Compare January 16, 2025 02:09
@TJnotJT TJnotJT changed the title GS: Clamp ST coordinates before casting in GetTextureMinMax GS: Clamp ST coordinates before casting in GSVertexTraceFMM::FindMinMax() Jan 16, 2025
@TJnotJT TJnotJT changed the title GS: Clamp ST coordinates before casting in GSVertexTraceFMM::FindMinMax() GS: Clamp ST coordinates to valid range in GSVertexTraceFMM::FindMinMax() Jan 16, 2025
@lightningterror
Copy link
Contributor

TODO: On the dumps that I saw a difference should be checked on a ps2 to compare how they should look like.

@TJnotJT
Copy link
Author

TJnotJT commented Jan 20, 2025

TODO: On the dumps that I saw a difference should be checked on a ps2 to compare how they should look like.

Lightning, would you be able to provide the dump name (if it is in gspack-10-july-2024)? I will try to work on this further given the new info from the hardware tests Ziemas ran.

@lightningterror
Copy link
Contributor

007 Agent Under Fire - door blend
187 - Ride or Die_SLUS-21116_In_Game
2002 FIFA World Cup
50 Cent - Bulletproof_SLUS-21315_20230329231035
Ace Combat 04 - Shattered Skies_SLUS-20152_20230511225232
Ace_Combat_04_-_Shattered_Skies_SLUS-20152_20230502010138
AFL Premiership 2007_SCES-54639
Alfa Romeo
Ape Escape 2_SLUS-20685_20220914200448
Dave_Mirra_Freestyle_BMX_2_SLES-50217_20230213220044

There's more but the list is huge, ace combat 4, ape escape 2 dave mirra seem broken.

@TJnotJT
Copy link
Author

TJnotJT commented Jan 20, 2025

Thanks much, this should keep me busy for some time

@TJnotJT
Copy link
Author

TJnotJT commented Jan 24, 2025

I've fixed a few issues and the dump run seems to look good now on my end. Edit: Actually, there was a big error in the last force push that I didn't test. Should be fixed now hopefully.

@TJnotJT TJnotJT changed the title GS: Clamp ST coordinates to valid range in GSVertexTraceFMM::FindMinMax() GS: Clamp ST coordinates to valid range in GS vertex trace. Jan 24, 2025
@TJnotJT TJnotJT force-pushed the clamp-huge-st branch 2 times, most recently from 25ee06b to 7f89851 Compare January 25, 2025 01:25
@TJnotJT TJnotJT marked this pull request as ready for review January 25, 2025 03:49
@lightningterror
Copy link
Contributor

What's the status on this, is it still worked on or ready for merge?

@TJnotJT
Copy link
Author

TJnotJT commented Jan 28, 2025

What's the status on this, is it still worked on or ready for merge?

@lightningterror Thanks for checking, I actually noticed a glitch with Counter_Terrorist_Special_Forces_-_Fire_for_Effect_SLES-53046_20230830195729.gs.xz. This is because we currently don't have handling of infinite STs in the rasterizer (the incorrect texture load was masking the issue at the expense of the missing haze effect; see image below). Unfortunately, even if we emulate this accurately, I think we would end up with the same or similar image. The issue may be further upstream with incorrect floating point calculations in VU or EE.

00099_f00003_rt0_00800_C_32_1
The area outlined has a flat patch from sampling the corner of the texture.

So the current status is I need to add infinite ST handling to the rasterizer and see if it fixes the above glitch.

Also, I thought you might want to run a dump run on your end to double check me since you may have additional dumps in your collection. If you have the time that would be super helpful, or if you think the testing I'm doing is sufficient, that's ok also.

@TJnotJT TJnotJT force-pushed the clamp-huge-st branch 5 times, most recently from 2a2b880 to e26760a Compare February 1, 2025 16:36
@TJnotJT TJnotJT changed the title GS: Clamp ST coordinates to valid range in GS vertex trace. GS: Fix huge ST coordinates in input vertices. Feb 1, 2025
@TJnotJT TJnotJT force-pushed the clamp-huge-st branch 8 times, most recently from 560e889 to 339de89 Compare February 3, 2025 08:06
@TJnotJT
Copy link
Author

TJnotJT commented Feb 3, 2025

@lightningterror This may be ready for review and/or merging. I updated the original post to some of the drawbacks of the fix, since it causes a graphical bug in at least a couple GS dumps in SW, but I believe these are problems with the input that are (hopefully) being emulated accurately on the GS. I also did a bit of profiling and found that the performance impact was not bad.

@@ -1675,50 +1668,19 @@ void GSState::FlushPrim()
}
#endif


Copy link
Contributor

Choose a reason for hiding this comment

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

Extra row.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks, fixed it

@lightningterror
Copy link
Contributor

This breaks Oni
Oni.gs.zip

Fixes Galerian 2
image

@TJnotJT
Copy link
Author

TJnotJT commented Feb 5, 2025

Thanks for the reviews lightningterror. The Oni dump had NaNs in the ST coordinates that were causing the triangle to be culled. I changed the handling to instead replace NaN ST coordinates with 0 (I think this was TellowKrinkle's suggestion at some point). I will run a full dump run to make sure nothing got broken and get back to you.

@lightningterror
Copy link
Contributor

No longer fixes Galerian 2, dunno if that's intended or not
Galerian_2_-_Text_And_2D_Sceens.gs.zip

@TJnotJT
Copy link
Author

TJnotJT commented Feb 5, 2025

Thanks for catching this. That Galerian dump is strange; it has some NaN ST coordinates but I haven't found yet where the lines come from. I will need to investigate this further and maybe figure out how to do a hardware test.

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.

2 participants