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

Misc: Always enable full mipmap with ps2 trilinear by default #11241

Merged
merged 9 commits into from
May 21, 2024

Conversation

lightningterror
Copy link
Contributor

@lightningterror lightningterror commented May 12, 2024

Description of Changes

Always enable full mipmap with ps2 trilinear by default.
Change mipmap option from a drop down to a bool with on off option only, remove automatic and basic options.

TODO for the future: remove support of trilinear and mipmap from db.

Rationale behind Changes

Accuracy, less hassle to keep adding gamefixes, instead we will add gamefixes to force disable mipmap on some games that break.

Suggested Testing Steps

Test variety of games that use mipmap + triliear, make sure to disable gamefixes .
DB list needs to be updated.

@stenzek
Copy link
Contributor

stenzek commented May 13, 2024

Have you done thorough testing with this? There will be a performance impact, texture pack breakage (hey, at least they can't blame me this time), and potentially broken games.

Also, rather than testing for auto all the time, it's probably better to just remove the auto enum value completely, and default to full. Basic can probably be purged too, I don't think there's anything that actually needs it, and it breaks miptricks. So really, it could become a boolean instead ;)

@RedDevilus
Copy link
Contributor

Have you done thorough testing with this? There will be a performance impact, texture pack breakage (hey, at least they can't blame me this time), and potentially broken games.

Also, rather than testing for auto all the time, it's probably better to just remove the auto enum value completely, and default to full. Basic can probably be purged too, I don't think there's anything that actually needs it, and it breaks miptricks. So really, it could become a boolean instead ;)

Basic is still needed for some games, namely FIFA and some other games like ICO and Harry Potter games. But yeah for 98% of the time full mipmapping should be fine. Though not sure when Full mipmapping was fixed, well probably in multiple waves and iteratively built upon I'm assuming.

image

@stenzek
Copy link
Contributor

stenzek commented May 13, 2024

Yeah. Worth checking those games, it might just be legacy holdover, rather than actually being broken with full.

If they are busted with full, upload dumps and I can take a look.

@CookiePLMonster
Copy link
Contributor

Also, rather than testing for auto all the time, it's probably better to just remove the auto enum value completely, and default to full.

What implications does this have for the existing configs? Can the currently saved option be just safely deprecated?

@lightningterror
Copy link
Contributor Author

full. Basic can probably be purged too, I don't think there's anything that actually needs it, and it breaks miptricks. So really, it could become a boolean instead ;)

I was thinking about that too actually.

@lightningterror lightningterror force-pushed the misc_always_mipmap branch 2 times, most recently from 14bba71 to 83eaef6 Compare May 13, 2024 22:59
@TheTechnician27
Copy link
Contributor

Have you done thorough testing with this? There will be a performance impact, texture pack breakage (hey, at least they can't blame me this time), and potentially broken games.

Also, rather than testing for auto all the time, it's probably better to just remove the auto enum value completely, and default to full. Basic can probably be purged too, I don't think there's anything that actually needs it, and it breaks miptricks. So really, it could become a boolean instead ;)

My personal experience going through both every single dump we've collated as well as playing the games I own (a combined total of ~4500 dumps examined by hand) is that mip+tri is useful for accuracy in a majority or a near-majority of games and unused but without any negative impact in a majority or near-majority of games. The games which break with mips by default, again in my anecdotal experience, amount to a rounding error within the PS2's library. The amount of games we have not yet had the resources to test which will become more accurate with this change almost assuredly vastly outweigh the amount that we haven't tested and which do break under full mips.

Additionally, with Jordan's recent 2000-line PR, we've enabled mips by default in most of the major games that have texture packs (I was keeping track of this bracing for helping texture pack users with complaints on the Discord, although we've received very few). We were eventually going to enable mips by default in those games no matter what once we found out they used mips, so this was simply expediting an inevitibility in that respect. So I don't think it should be taken heavily into consideration. Finally, you would know more about the objective performance impact than I would from simple anecdotal experience, so I won't remark on that.

@lightningterror
Copy link
Contributor Author

lightningterror commented May 14, 2024

@stenzek Okay so dumps that break from the archive:

rugby08pitch.gs.xz
jurassicpark.gs.xz
FIFA Football 2005 uniform bug with mipmapping on.gs.xz
Spider-Man_3_Mipsindextex.gs.xz

pcsx2/Pcsx2Config.cpp Outdated Show resolved Hide resolved
@lightningterror lightningterror merged commit 06efa93 into master May 21, 2024
24 checks passed
@lightningterror lightningterror deleted the misc_always_mipmap branch May 21, 2024 08:45
@CarissaIsWierd
Copy link

Can you add back an option somewhere (even if it's a setting only enabled in an .ini file) to use Basic Generated Mipmaps? I made a texture pack for Ape Escape 2 that shows the same texture quality regardless of distance and it requires the Basic setting. It's also just better to have various options and not be locked to off or on.

@refractionpcsx2
Copy link
Member

basic was a hack, we don't really want that in there any longer, what you need to do is dump the mipmaps and update them too.

We're not going to reverse improvements because you did half a job.

@stenzek
Copy link
Contributor

stenzek commented May 21, 2024

I'm (eventually) going to add an option to reverse-bias the texture LOD, instead of using the GS LOD, which will result in higher detail in some games, similar to how basic worked. But it can't be the default, because that would break Jak/Ratchet/etc. It also won't help with the hash changes, since it's just adjusting the sampler, not the actual texture source itself.

Basic was terrible and a giant hack to begin with (breaking miptricks), that's why I suggested removing it in the first place.

@CarissaIsWierd
Copy link

Dumping with Basic on was dumping lower quality versions of all the textures. I manually identified and removed the low quality ones, made copies of the high quality ones, and renamed those to what I removed. I wouldn't call that half a job, even though it was ridiculous to do.

Ape Escape 2 is a unique situation where it's broken with mipmapping off and looks like a terrible blurry mess with it on. Having it set to Basic with a texture pack fixed that. I get that the developers intended it to look that way, but a texture pack barely does anything with mipmapping set to Full.

@refractionpcsx2
Copy link
Member

okay but basic is a hack, you will need mipmaps compatible with full mode, I would expect those to be lower quality textures, since basic just takes the base level and makes the rest up, full has the full chain of mips, but you should modify your texture pack to use them.

@stenzek
Copy link
Contributor

stenzek commented May 21, 2024

Biasing the LODs as I said above should force it to use the higher detail levels, similar to how basic behaved before, just without the hackyness refraction is referring to. We need this for upscaling in general, not just texture packs.

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.

10 participants