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

Proposals for new defaults for texture replacement #17138

Closed
hrydgard opened this issue Mar 17, 2023 · 5 comments
Closed

Proposals for new defaults for texture replacement #17138

hrydgard opened this issue Mar 17, 2023 · 5 comments

Comments

@hrydgard
Copy link
Owner

We can't change the default settings in the case of no ini file due to backwards compatibility, but if we always generate an ini file when dumping, we can write any new defaults we want to it (and we'll flip to these defaults when dumping starts).

For example, maybe defaulting to reduceHash = true would make sense - hashing only half the texture is pretty much necessary for stuff like title screen replacement.

So, action items:

  • We should always create an ini file if it doesn't exist, when you enable texture dumping.
  • In this ini file, here's the new default content:
# This file is optional and describes your textures.
# Some information on syntax available here:
# https://github.com/hrydgard/ppsspp/wiki/Texture-replacement-ini-syntax
[options]
version = 1
hash = quick
ignoreMipmap = true   # Usually, can just generate them with basisu, no need to dump.
reduceHash = true

[games]
# Used to make it easier to install, and override settings for other regions.
# Files still have to be copied to each TEXTURES folder.
BREA00829 = textures.ini

[hashes]
# Use / for folders not \\, avoid special characters, and stick to lowercase.
# See documentation for more info.
# Example: 08d3961000000909ba70b2af = title.png

[hashranges]
# Lets you set the hashing size of textures explicitly
# See documentation for more info.
# Example: 0x08d39610,512,512 = 480,272

[filtering]
# Lets you override the texture filtering mode for specific textures.

[reducehashranges]
# Lets you specify memory ranges that should hash a specific percentage of textures.
# Advanced, see documentation.

Opinions?

@LunaMoo
Copy link
Collaborator

LunaMoo commented Mar 17, 2023

Isn't it a bit scary to default to reduceHash while generating textures.ini on texture dump?
(Edit: there's at least 1 game where it breaks fonts until the texture pack replaces partialy hashed textures with full texture, but to even dump full one, it needs to be done before using that setting.)
Even if textures will not be replaced this will increase chances of a duplicate leading to potential bugs. Then again I think it only works with stronger hash so if that wasn't changed it will do nothing with "quick" hash?

I'd personally change the default hash for texture dumping with xxh64 through even if reduceHash wouldn't be default simply because it's stronger and very fast on any reasonable modern hardware and it's primary choice of most texture packs anyway, also adding "ignoreAddress" might be great default since that reduces a lot of duplicates while still being very safe.

@unknownbrackets
Copy link
Collaborator

I think someone needs to actually understand how textures work on the PSP before enabling reduceHash, and I'm not really a fan of it being enabled by default. There are a lot of cases it can cause confusion. Yes, title screens are nice and simple. But sometimes there's a face in the bottom half of the texture - I think Crisis Core even does this - and blindly ignoring half the texture does Bad Things.

People will just report confused, incomplete bugs (not really clarifying that this is even the cause) otherwise. But I guess you're the one signing up for the emails and to respond to all the issues, so as long as you want that.

-[Unknown]

@hrydgard
Copy link
Owner Author

Thanks for the feedback both of you. I retract my proposal to enable reduceHash by default, and I've removed the comment that recommends it, too.

The only remaining default change I propose now is ignoreMipmaps, which I think should default to true. Not many people will manually draw in multiple replacement mipmaps anyway, might as well generate them directly with basisu when converting from png, for example, as the new texture replacement documentation explains.

@hrydgard hrydgard added this to the v1.15.0 milestone Mar 18, 2023
@LunaMoo
Copy link
Collaborator

LunaMoo commented Mar 18, 2023

Maybe all non default options should be written as well, but set to false / disabled and with at least short description what are they doing, it's much easier to learn about things you know exist from just looking at template than looking for documentation or guides.

for example:

hash = quick    # options available: "quick", xxh32 - more accurate, but much slower, xxh64 - more accurate and quite fast, but slower than xxh32 on 32 bit cpu's
ignoreMipmap = true   # Usually, can just generate them with basisu, no need to dump.
reduceHash = false    # Unsafe and can cause glitches in some cases, but allows to skip garbage data in some textures reducing endless duplicates as a side effect speeds up hashing as well, requires stronger hash like xxh32 or xxh64
ignoreAddress = false    # Reduces duplicates at the cost of making hash less reliable, requires stronger hash like xxh32 or xxh64

etc.

also some more examples for additional functions like hashranges, filtering and reducehashranges would be cool. Pre-generated textures.ini could become a mini documentation of all the options it has as those things are easily forgotten when dealing with them rarely and best source of information about them is currently just looking through PR's that implemented those.

@hrydgard
Copy link
Owner Author

Yes, I agree that it should be as self documenting as possible, though we'll of course also keep the link to the proper documentation.

I'll steal those lines :)

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

No branches or pull requests

3 participants