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

GSdx: Texture Dumping and Replacing #4199

Closed
wants to merge 29 commits into from
Closed

GSdx: Texture Dumping and Replacing #4199

wants to merge 29 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Jan 31, 2021

What is this?
GSdx, edited to be able to load DDS files from the local storage, and then replace them accordingly. It also has the ability to dump textures with a specific hash to use with the replacement function (The hash in question is generated with CRC32)

Primarily made because I wanted to, but it also closes the issue: #4038.

How does it work?
In PCSX2's executable directory, you have 2 folders. One of them is txtconfig, the other one being textures.
The txtconfig folder contains the texture loading configuration, a kind of hashlist, to tell the plugin what file should be loaded from where and what texture should it replace. The file should have the name of [GAME_CRC].yaml. In case of KH2FM, it would be txtconfig/F266B00B.yaml

This file would only have 1 table in it. ProcessTEX. In that table, the texture replacements would be declared in this format:
0x[TEXTURE_CRC]: [FILE_PATH]

[TEXTURE_CRC] being the hash that is generated for the texture on dump. [FILE_PATH] being the path of the file, relevant to the textures folder in PCSX2's executable directory. For example, 0x0CF2C584: "@COM/field2d/zz0field.dds" would load the file from say <pcsx2Install>/textures/@COM/field2d/zz0field.dds

Below, you can see such a file as an example:

ProcessTEX: 
  0x0CF2C584: "@COM/field2d/zz0field.dds"
  0x1971E6D5: "@COM/field2d/zz1field.dds"
  0x0F92318E: "@COM/obj/P_EX100/texture00.dds"
  0xB901C8DB: "@COM/obj/P_EX100/texture01.dds"

Dumping textures dumps every texture that is caught (and once again if the texture gets "completed") to <pcsx2Install>/textures/@DUMP/[GAME_CRC]/[TEXTURE_CRC].dds, the [TEXTURE_CRC] being the hash generated for the texture, and ""[GAME_CRC]" is the CRC of the loaded game.
To mitigate issues in speed, textures are dumped once a second.

The feature to allow dumps and replacements can be enabled from either the GUI, or by pressing the F10 key.
Pressing F11 will switch between dumping and replacing modes.

Why DDS?
Speed. Loading in, parsing, and handling DDS files, although are costly on space, are much faster and overall easier and more efficient. The poor performance of loading in big PNG files was a big complaint on the testing phases, and the switch to DDS was requested. PNG support can be added to co-exist with DDS later on if necessary.

Advantages:

  • ROM Modding is not required for texture stuffs.
  • Extremely fast replacement, precise HASH generation.
  • Texture replacements are not done on EE/GS memory. PS2 RAM is completely unaffected. Allowing HDification.

Disadvantages:

  • Hefty texture sizes because of DDS.

Tested Games:

  • Soulcalibur 3
  • Kingdom Hearts II: Final Mix
  • Kingdom Hearts I: Final Mix
  • Kingdom Hearts - Re: Chain of Memories

Known Issues:

  • AMD Cards require Anti-Aliasing to be turned on for replaced textures, otherwise they will look blocky.

Details:

  • Time took to develop: 15 days, no sleep.
  • Time took to perfect: ~1 month.
  • Time took to test, bugchase and fix caught bugs: 20 days.
  • Amount of testers: 4 people and Topaz.

Important Details:

  • The DDS classes present only support RGBA/BGRA - Uncompressed DDS files. This is fine for what we want to do. However, this support will be extended and will be implemented in a better fashion.

Screenshots:
Here is a screenshot of the new GUI, packing these features:
image

Here are a few comparison screenshots, taken from Kingdom Hearts - Re: Chain of Memories (If any GUI Elements are missing, it's because they are not present in the DDS file. Did not have time to recreate them for this demonstration):
image
image
image
image

Great thanks to:
@Xeeynamo - Technical assistance, code checking.
@FerocityVine - General assistance, code testing, texture testing.
@Vladabdf - Texture packs for testing this code, advices given.
@Akita23 - A lot of testing.

Conclusion:
I would say this would be an awesome feature to have in PCSX2. It has been wildly requested, and it works great! No issues, overflows, slowdowns, etc. have been found in the testing period. If anything needs changing, altering or fixing PLEASE let me know.

Sincerely:

  • TopazTK.

EDIT: Typo fixes.
EDIT: Changed content to be consistent with the latest additions and changes (3 times).

@refractionpcsx2
Copy link
Member

refractionpcsx2 commented Jan 31, 2021

Okay there's a few things wrong with this.

  1. It doesn't build for linux of mac, please rectify this
  2. You included a CRC project when there are easier ways to get a CRC, PCSX2 already does this without the need for a 3rd party library
  3. You included an ini reading library when GSDX can already read ini files, please just use that.
  4. There's a lot of glaring formatting errors in the code with extra whitespace, too many tabs etc
  5. You spelt "Manipulation" wrong in the UI, correct that.

This is just a few things I've noticed from looking at it for 5 minutes.

@ghost
Copy link
Author

ghost commented Jan 31, 2021

It doesn't build for linux of mac, please rectify this

I do realize this. I, however, do not know CMake to make this possible. However, a few people reached out to me, offering help.
So will be taking care of this ASAP.

You included a CRC project when there are easier ways to get a CRC, PCSX2 already does this without the need for a 3rd party library
You included an ini reading library when GSDX can already read ini files, please just use that.

Oh, I did not realize. My deepest apologies. Will do.

There's a lot of glaring formatting errors in the code with extra whitespace, too many tabs etc

*glares at Visual Studio* Will fix.

You spelt "Manipulation" wrong in the UI, correct that.

*glares at myself* Will fix.

Thanks for your feedback! I will get to it right away!

@lightningterror
Copy link
Contributor

Regarding the UI, it would fit better in Advanced Settings.
Also you undid some recent commits.

@ghost
Copy link
Author

ghost commented Jan 31, 2021

Regarding the UI, it would fit better in Advanced Settings.
Also you undid some recent commits.

Ah, I was afraid this would happen because of simply how late I got. I rebased this and ported all the code about it around 5 days or so ago, too.
I will work to reverse this. Thanks for bringing this to my attention.

@TellowKrinkle
Copy link
Member

TellowKrinkle commented Jan 31, 2021

Note: If you separate pieces out into separate, nicely labeled commits it makes things way easier to review, and much more obvious when you accidentally undo existing commits so that you can fix it before you even open the PR. Things that could have been separate in the current PR: Addition of new libraries, addition of texture dumping, addition of ProcessSTD, addition of ProcessSPC

Having both SaveDDS and Save as virtual (separately-implemented) methods seems like a lot of repeated code, maybe factor out the shared part into a virtual download texture method and put the dds and png parts in non-virtual methods so there's only one copy of them

@ghost
Copy link
Author

ghost commented Jan 31, 2021

Note: If you separate pieces out into separate, nicely labeled commits it makes things way easier to review, and much more obvious when you accidentally undo existing commits so that you can fix it before you even open the PR. Things that could have been separate in the current PR: Addition of new libraries, addition of texture dumping, addition of ProcessSTD, addition of ProcessSPC

Yea, I realized I messed up a bit in that part. However, I have a good reason for this, which I will keep private. I can ensure this won't happen again.

Having both SaveDDS and Save as virtual (separately-implemented) methods seems like a lot of repeated code, maybe factor out the shared part into a virtual download texture method and put the dds and png parts in non-virtual methods so there's only one copy of them

Noted. Will do. Thanks for the feedback ^^

@GovanifY
Copy link
Member

GovanifY commented Jan 31, 2021

Hey, that's a neat feature, glad to see interest on this!

Unfortunately, you're chiming in at a very bad timing, the project codebase is going to heavily change in the upcoming months and will probably take a bunch of work to rebase. I am not going to tell you to stop fixing it now because of this though, but you'll have to keep this in mind.
Your PR also needs a lot of changes before reviewing, some of which have been outlined by refraction, especially the Linux and MacOS build. The formatting and overall coherency of your PR needs to be taken care of too before we will do any preliminary review.

Feature wise there are probably concerns regarding cache coherency and other things, but I'll let others talk about this more in detail.

@ghost
Copy link
Author

ghost commented Jan 31, 2021

Heya Govan! Long time no see!

Unfortunately, you're chiming in at a very bad timing, the project codebase is going to heavily change in the upcoming months and will probably take a bunch of work to rebase. I am not going to tell you to stop fixing it now because of this though, but you'll have to keep this in mind.

If this has a chance to get merged, I am willing to rebase this until the grand overhaul. If not, I am willing to do a rewrite at the worst possible case scenario. I know it has some problems I need to address but hey, it's a start, isn't it? This feature actually existing and working in PCSX2 is remarkable enough in my opinion. And I am willing to keep it going.

Your PR also needs a lot of changes before reviewing, some of which have been outlined by refraction, especially the Linux and MacOS build. The formatting and overall coherency of your PR needs to be taken care of too before we will do any preliminary review.

You know that I was never a Linux nor a macOS guy myself. However, I have been talking with some people over this, and I think it will be possible to fix this (To be honest, I do believe they will be fixed once I use the internally existing classes and libraries). The formatting I will also try to fix to the best of my ability.

Feature wise there are probably concerns regarding cache coherency and other things, but I'll let others talk about this more in detail.

This is the main concern I did have, and why I and Vlad were insisting on a LOT of testing. This was finished in around January 1st and was being tested and fixed until today. We were able to spot some problems with memory, but I was able to address them. I do believe, anyways.

More testing is necessary, of course it is. But I was not able to get it to behave weirdly or badly even with monumental effort to do so. However, I did hit a hard limit of 4096x4096 textures. That may be cache or memory, but that is only for a single texture it may seem. As loading several 2048x2048 textures worked fine.

I do appreciate your feedback. Really, I do. And I will take everything you said into consideration whilst I am fixing all of the error and problems with this feature. And who knows, perhaps I will be able to make it all before the great overhaul.

@GovanifY
Copy link
Member

A thing I noticed on the PR: You are using mIni for loading settings. Don't.
We already have an internal settings class for that, which is currently being ported to yaml too.
I'd encourage you to wait for the YAML merge to be done (GameDB is already in YAML if you want to take a quick snip at it) and make your work based off this instead of adding another unneeded dependency.

And obviously this has a chance to be merged, PRs are always welcome, but this will definitely require a lot more work than what you are proposing!

@ghost
Copy link
Author

ghost commented Jan 31, 2021

A thing I noticed on the PR: You are using mIni for loading settings. Don't.

I was told. I think I will wait on the YAML implementation for porting the settings and what not.

And obviously this has a chance to be merged, PRs are always welcome, but this will definitely require a lot more work than what you are proposing!

Thanks for your kind words. However, if you or any of the other devs may or may not think otherwise, please do let me know! I want to fix any and all problems that I can. And I do realize this needs work. I will do my best ^^

@Gunlaus
Copy link

Gunlaus commented Feb 1, 2021

Hey, so how do I give this a shot? I'm willing to be a tester.

@GovanifY
Copy link
Member

GovanifY commented Feb 1, 2021

@Gunlaus There isn't really anything to test for now, this PR isn't ready for testing nor for review.
For people who nevertheless wants to download the build just go to details on the windows build, then top right, artifacts and download.

The 64 bit artifact isn't ready either, so only windows 32 bit will work.

@ghost
Copy link
Author

ghost commented Feb 1, 2021

A problem I have encountered. I generate a CRC-checksum from the CLUT data to use with replacements. I do this to generate 1 hash per texture. The other things I tried to hash resulted in 5-10-20-etc. hashes per texture.

I use CRCpp to do this, which I was told to use the crc32 function within PCSX2 instead (Which is on zlib). However, I can no longer get a single hash to generate. Anyone has a clue on what is causing it?

I try to get the hash like so:

// Capture the palette.
auto _palette = m_src->m_palette_obj.get();
auto _len = _palette->m_pal * 4;

// Capture the CLUT data as Bytef for zlib/crc32
std::vector<Bytef> _clut(_palette->m_clut, _palette->m_clut + _len);

// Calculate a CRC32 value from the palette CLUT data.
_currentChecksum = crc32(0, _clut.data(), _len);

But it causes a lot of hashes per texture, a problem I had when I was first developing this.

EDIT: Nevermind, I was a doofus. I fixed this issue.

@Gunlaus
Copy link

Gunlaus commented Feb 1, 2021

Brilliant. First try and it's dumping textures as perfectly as when I used to manually rip them with Texmod.

@ghost
Copy link
Author

ghost commented Feb 2, 2021

All of the latest changes should address the following:

  • Typos in the GUI
  • Formatting (Needs to be checked, can run through an auto-formatter if requested)
  • Usage of 3rd party libraries for CRC and Ini (Switched to zlib/crc32 and yaml-cpp instead)
  • Reversal of a previous PR (added in the changes made, which this PR reversed)
  • Build fail on Linux (Edited CMake files accordingly)
  • Build fail on Windows x64 (Edited additional dependencies accordingly)

@iMineLink
Copy link
Contributor

I think you can add the xxHash GitHub repo as submodule.

@da2ce7
Copy link

da2ce7 commented Feb 21, 2021

@GovanifY , and @Topaz-Reality

How about considering: https://github.com/google/highwayhash/blob/master/highwayhash/highwayhash.h
It is a variant of SipHash; but 4x faster.

I think that this might fit the bill; 128bit should be enough to never worry about a collision again.

@ghost
Copy link
Author

ghost commented Feb 22, 2021

@GovanifY , and @Topaz-Reality

How about considering: https://github.com/google/highwayhash/blob/master/highwayhash/highwayhash.h
It is a variant of SipHash; but 4x faster.

I think that this might fit the bill; 128bit should be enough to never worry about a collision again.

The collisions are not because of CRC32, but rather, the method I use to hash texture in a speedy manner.
However, since collisions are occurring: I need another method to parse them. The issue is CRC32 is a bit too slow to parse the hashes using the new method. Although the new method may be point blank accurate.

I did not test with xHash or SipHash or HighwayHash. I am still waiting for #4005 to merge before moving further with anything. However, thanks for the suggestion! If the devs are okay with it, and if it's license is GPLv2 compliant, I will be sure to test it.

@ghost
Copy link
Author

ghost commented Feb 22, 2021

Nevermind, I think we cannot use HighwayHash. Turns out it's license (Apache v2) is incompatible with PCSX2's license (GPL v2).

@FVitor7
Copy link

FVitor7 commented Mar 5, 2021

Incredible your work! I would like to know how to find out the values ​​of the textures without the hash. I edit a PS2 game and this is essential.

@ghost
Copy link
Author

ghost commented Mar 5, 2021

Incredible your work! I would like to know how to find out the values ​​of the textures without the hash. I edit a PS2 game and this is essential.

You cannot.

@Tellilum
Copy link

Tellilum commented Mar 5, 2021

so I've been trying to test this out on .hack//IMOQF and it doesn't seem to be working, attached is my yaml file (changed to txt for GitHub upload), am i doing something wrong?
273933B4.txt

@playmer
Copy link

playmer commented Mar 7, 2021

Likely you missed a key detail in the description. The DDS files have to be in an RGBA (uncompressed) format. I spent some time writing a little pipeline to take the dumped textures, run them through waifu2X then spit them out into the required DDS format to test to make sure IMOQ was working. I did experience some issues on the main menu, but I can't be certain that wasn't something with the pipeline screwing up a texture or two. If you'd like I can share it out.

To write to the appropriate format I adapted the writing code from the PR into an exe: https://gist.github.com/playmer/9ec5cd78f8a599a8481548edbfb18637

So for further testing you can use that.

But yeah, all that said, seems to work:
image

While reviewing the code to understand some of what was going wrong here I think I saw at least one nit, so when I wake up I'll take another look to make sure I'm not missing anything.

I will say, I do think compressed DDS is worth looking into. These files can get quite big. For laughs I had upscaled them to 16x and some were in the 150MB or so range.

@ghost
Copy link
Author

ghost commented Mar 7, 2021

Likely you missed a key detail in the description. The DDS files have to be in an RGBA (uncompressed) format.

If you had actually taken the time to read the PR, you would see the following:

image

@playmer
Copy link

playmer commented Mar 7, 2021

To be honest, until I sat down and read the code in more detail that line was unclear and insufficient, and somewhat of a buried lede despite me looking through to find the specific import format. Does it mean Exporting? Importing? Both? Until I read the code I didn't know. As it is, the class handles importing via CatchDDS. I'd recommend changing that to something like ReadDDS. The export all happens in the individual renderers, probably should unify that into the class. As for the format itself, I've never even heard of such a format of DDS. None of my converters I'm familiar with supported such a format.

@ghost
Copy link
Author

ghost commented Mar 7, 2021

To be honest, until I sat down and read the code in more detail that line was unclear and insufficient, and somewhat of a buried lede despite me looking through to find the specific import format. Does it mean Exporting? Importing? Both? Until I read the code I didn't know. As it is, the class handles importing via CatchDDS. I'd recommend changing that to something like ReadDDS. The export all happens in the individual renderers, probably should unify that into the class. As for the format itself, I've never even heard of such a format of DDS. None of my converters I'm familiar with supported such a format.

Of course you have not. It's not like you work with DX or anything.
DDS stands for DirectDraw Surface. It holds the raw image data in a form factor that can be read fast by the GPU to improve performance significantly.

The code may be a little unclear on the DDS part, but literally EVERYTHING else is commented in a way that even a non-programmer can understand. I am not sure how you managed to have problems with it.

As for the DDS class itself, it is going to be changed anyways. But if you think you can do a better job than me than please do push to this PR in one way or another (I do not know if Github allows it), so I can use it with my work.

Also, 'the class only supports X' means both importing and exporting. It is sufficient and clear, I am sorry you could not understand this very simple to understand line for one reason or another.

@ghost
Copy link
Author

ghost commented Mar 7, 2021

On that note; I would like to make it clear that any and all help is appreciated and I do appreciate your feedback.
Sorry if I sound rude sometimes, that is unfortunately just my way of speech.
However, I am baffled by you not understanding, what is to everyone who read it so far, a very simple to understand PR and commenting. Please tell me what I did wrong, and if the majority agree, I will mend my ways.

@playmer
Copy link

playmer commented Mar 8, 2021

To clarify, I'm generally familiar with DDS, I didn't know it supported uncompressed RGBA inside of it. Then couldn't find a tool that would create a DDS with that internal format, but I probably just missed it. I typically use Universal formats on disk such as crunch or basis and transcode to things like BC7 or BC3, which are the internal formats I'd generally expect to find in a DDS file.

I'm interested in extending this PR with such functionality, along with addressing what I can to help it along. I'd also like to put PNG support back in as an option, as the workflow for dealing with the files mean that a texture pack creator is going to have to interchange their texture to something else anyway. You mentioned this in the description as a possibility so I'm happy to do that. I'll open a PR on your repo when I'm further along and we can discuss it there.

@Tellilum
Copy link

Tellilum commented Mar 8, 2021

Likely you missed a key detail in the description. The DDS files have to be in an RGBA (uncompressed) format. I spent some time writing a little pipeline to take the dumped textures, run them through waifu2X then spit them out into the required DDS format to test to make sure IMOQ was working. I did experience some issues on the main menu, but I can't be certain that wasn't something with the pipeline screwing up a texture or two. If you'd like I can share it out.

To write to the appropriate format I adapted the writing code from the PR into an exe: https://gist.github.com/playmer/9ec5cd78f8a599a8481548edbfb18637

So for further testing you can use that.

But yeah, all that said, seems to work:

image

While reviewing the code to understand some of what was going wrong here I think I saw at least one nit, so when I wake up I'll take another look to make sure I'm not missing anything.

I will say, I do think compressed DDS is worth looking into. These files can get quite big. For laughs I had upscaled them to 16x and some were in the 150MB or so range.

Thank you sooo much! This will help me alot!

@ghost ghost mentioned this pull request Mar 27, 2021
@WitheredGryphon
Copy link

Likely you missed a key detail in the description. The DDS files have to be in an RGBA (uncompressed) format. I spent some time writing a little pipeline to take the dumped textures, run them through waifu2X then spit them out into the required DDS format to test to make sure IMOQ was working. I did experience some issues on the main menu, but I can't be certain that wasn't something with the pipeline screwing up a texture or two. If you'd like I can share it out.

To write to the appropriate format I adapted the writing code from the PR into an exe: https://gist.github.com/playmer/9ec5cd78f8a599a8481548edbfb18637

So for further testing you can use that.

But yeah, all that said, seems to work:
image

While reviewing the code to understand some of what was going wrong here I think I saw at least one nit, so when I wake up I'll take another look to make sure I'm not missing anything.

I will say, I do think compressed DDS is worth looking into. These files can get quite big. For laughs I had upscaled them to 16x and some were in the 150MB or so range.

This is really helpful for non-Photoshop users, but it doesn't seem to support partial transparency. For Photoshop users, you can use NVIDIA's Photoshop plugin which includes partial transparency support (https://developer.nvidia.com/nvidia-texture-tools-exporter)

I save with BGRA and it works perfectly. Filesize is, of course, still a problem unfortunately.

@playmer
Copy link

playmer commented Mar 28, 2021

This is really helpful for non-Photoshop users, but it doesn't seem to support partial transparency. For Photoshop users, you can use NVIDIA's Photoshop plugin which includes partial transparency support (https://developer.nvidia.com/nvidia-texture-tools-exporter)

Yeah, that was a naïve early attempt at figuring out what to use although it's a little odd you're seeing partial transparency issues. I've started to use texconv for this as it does support the needed formats (DXGI_FORMAT_R8G8B8A8_UNORM or DXGI_FORMAT_B8G8R8A8_UNORM). Although I can't speak to the transparency support.

Something like texconv.exe myImageToUpscale.png -ft dds -f R8G8B8A8_UNORM should work I think.

@Anuskuss
Copy link

Anuskuss commented Apr 9, 2021

A little late to the party, but I'm wondering why you chose DDS particulary? I don't know how the loading works (AFAICT DDS is merely a container) but won't this be DirectX-exclusive then? And why not use more advanced tech like BC7 (same size compressed but better quality)?
Also this is probably not in the scope of this PR but you could definitely just put all uncompressed textures in a ZIP and then just decompress them on game start.

@SirFancyBacon
Copy link

A little late to the party, but I'm wondering why you chose DDS particulary? I don't know how the loading works (AFAICT DDS is merely a container) but won't this be DirectX-exclusive then? And why not use more advanced tech like BC7 (same size compressed but better quality)?
Also this is probably not in the scope of this PR but you could definitely just put all uncompressed textures in a ZIP and then just decompress them on game start.

I use OpenGL without issues.

@BoukObelisk
Copy link

Amazing work, Topaz! Seriously what I have been dreaming of for a very long time.

To make the YAML generation easier for people, I would recommend putting this texturesmapper tool by delltalal into your guide: https://github.com/delltalal/TexturesMapper/

Just FYI. Thanks for your excellent work!

@ghost ghost closed this Apr 11, 2021
@PCSX2 PCSX2 locked as resolved and limited conversation to collaborators Apr 11, 2021
@PCSX2 PCSX2 deleted a comment Apr 11, 2021
@refractionpcsx2
Copy link
Member

refractionpcsx2 commented Apr 11, 2021

@Topaz-Reality has closed this due to a disagreement with another contributor and will be continuing it in his fork of PCSX2. His original closing message has been deleted for targeted harassment

Edit: For those viewing this for Reddit, this PR was closed due to comments by a contributor (not PCSX2 member) on a project completely unrelated to this one. This is all just drama sturred up by them to try and cause problems for us, it's kinda sad, a lot of people were looking forward to this.

This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.