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

Texture dumping and replacement #4868

Merged
merged 24 commits into from Nov 9, 2019
Merged

Texture dumping and replacement #4868

merged 24 commits into from Nov 9, 2019

Conversation

khang06
Copy link
Contributor

@khang06 khang06 commented Aug 6, 2019

please don't make anything big with this until the file format is finalized

this pull request is now on feature freeze. all changes from now on should be compatible with the texture packs that exist right now. if your texture pack breaks because i updated something, that's on me now, not you

this is something i was working on back in april and decided to pick up recently. i shared a couple of screenshots and discussed the fork in the discord a couple of times. originally i wanted to just have the original implementation done and have someone else clean it up, but i might as well try doing it myself since i was working on it again anyway.

the usage is almost exactly like how dolphin does, with the file format being almost the same. textures are dumped to dump/textures/[title id] and textures are loaded from load/textures/[title id]

3 entries are added to the graphics section of the configuration, each being self-explanatory if you've ever used dolphin's custom texture stuff
image

some things that i'm not so sure about/aren't done are

  • dds support. dolphin does it and it looks like there are a couple of benefits of dds over png
  • mipmap support. i don't have any testcases
  • dolphin-style resource pack format. i'm awful with designing uis, so it would probably be better if someone else did this
  • probably other stuff i missed

none of the games i have seem to break in any way with this, but i only really tested 5-7 games and not very thoroughly.

the way i implemented everything overall feels pretty hacky (uploading a texture to the gpu and then instantly downloading it again doesn't feel right, but i tried doing this in an alternative way on another branch and it doesn't dump as many textures). i really need some feedback and suggestions for this, so that's why i'm pr'ing in this state.


This change is Reviewable

u64 hash;
u32 format; // unused
// TODO: more modern way of doing this
if (std::sscanf(file.virtualName.c_str(), "tex1_%ux%u_%llX_%u.png", &width, &height,
Copy link
Contributor Author

@khang06 khang06 Aug 6, 2019

Choose a reason for hiding this comment

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

the way i did this really sucks, but i can't figure out any better way of doing it (fmt doesn't seem to have a sscanf function). dolphin appears to take the filename and use it in a map, but i don't want to construct a string on every texture upload

@FearlessTobi
Copy link
Contributor

FearlessTobi commented Aug 6, 2019

Please run the clang-format build target on the branch you made these changes on.

@wwylele
Copy link
Member

wwylele commented Aug 6, 2019

About the added png library: can we just use Qt instead? We want the core|video_core to have as few dependencies as possible. You can do a little dependency injection to use Qt: let video_core expose some abstract class / function pointers / function objects and let Qt plug in the PNG store/load implementation. Bonus: Qt supports more image formats.

src/common/texture.cpp Outdated Show resolved Hide resolved
src/common/texture.cpp Outdated Show resolved Hide resolved
src/common/texture.cpp Show resolved Hide resolved
src/common/texture.h Show resolved Hide resolved
src/core/core.cpp Outdated Show resolved Hide resolved
src/core/custom_tex_cache.h Outdated Show resolved Hide resolved
src/core/custom_tex_cache.h Outdated Show resolved Hide resolved
src/core/custom_tex_cache.h Outdated Show resolved Hide resolved
src/core/custom_tex_cache.h Outdated Show resolved Hide resolved
Copy link
Contributor

@B3n30 B3n30 left a comment

Just some thoughts:

  • I agree on wwyleles suggestion about qt for png/format handling.
    But if lodepng is kept it should be included as a subproject.
  • Is it useful to have Use Custom Textures and Dump Textures enabled at the same time? Or should only one option be selectable at a time?
  • Is there any reason not to have preload textures enabled every time one use custom textures? I'm all for having as less as user options s possible. And not having this enabled seems like we could a lot of users stating for stuttering or bad performance
  • IIRC one other dev tested your branch with some custom textures and hit some asserts because their textures had some not compatible sizes (I don't remember the rules but aren't they supposed to have a width/height a multiple of 4). So it could be a good idea to error if a wrong custom texture is used (on preload and/or on upload to gpu). And maybe if the size is wrong don't replace the texture?

Also your clang-format seems to be messed up. Maybe talk to the devs in discord to fix that.

src/common/texture.cpp Outdated Show resolved Hide resolved
@khang06
Copy link
Contributor Author

khang06 commented Aug 6, 2019

wouldn't it be worse to have video_core/core depend on qt instead of depend on lodepng? the reason why i went with adding lodepng is because i don't want to require a library as huge as qt just to add something as small as texture dumping and replacement. if there's already parts of them that require qt to be linked, even if not using citra-qt, i'm totally fine with replacing lodepng.

having use custom textures and dump textures could be useful if a texture pack author needs additional textures dumped without having a lot of overlapping textures. theoretically, they could have users run citra in that configuration to obtain missing textures faster.

i have preload as a separate option because of the chance of really high memory usage if someone has many, many high quality textures in a pack

for the last point, scaling the incompatible texture to a "normal" resolution would probably be a better option unless i can find a good way to notify the user the texture isn't scaled properly.

ping me on discord if you know what's wrong with my clang-format (it's probably just visual studio or something messing up)

src/core/custom_tex_cache.cpp Outdated Show resolved Hide resolved
src/video_core/renderer_opengl/gl_rasterizer_cache.cpp Outdated Show resolved Hide resolved
@khang06
Copy link
Contributor Author

khang06 commented Aug 7, 2019

citra_qt now uses qimage for all png handling, while the sdl frontend still uses lodepng using a generic image interface

i'm not sure what to do with android

@khang06
Copy link
Contributor Author

khang06 commented Aug 7, 2019

it looks like the std::optional stuff broke macos compilation. thanks apple!

/Users/travis/build/citra-emu/citra/src/video_core/renderer_opengl/gl_rasterizer_cache.cpp:960:40: error: call to unavailable member function 'value': introduced in macOS 10.14
            dump_path = temp_dump_path.value();
                        ~~~~~~~~~~~~~~~^~~~~

according to octobanana/peaclock#2, this can be fixed by updating the compiler or updating to macos 10.14. i don't know how viable this is for travis-ci's macos environment, though. what's a possible workaround for this?

@B3n30
Copy link
Contributor

B3n30 commented Aug 7, 2019

first check with .has_value() and then get the value with *temp_dump_path

Copy link
Contributor

@B3n30 B3n30 left a comment

My questions from my previous comment weren't answered yet...

height = image.height();

// Write RGBA8 to vector
for (u32 y = 1; y < image.height() + 1; y++) {
Copy link
Contributor

@B3n30 B3n30 Aug 7, 2019

Choose a reason for hiding this comment

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

is QImage.convertTo or QImage.convertToFormat usable here?

Copy link
Contributor Author

@khang06 khang06 Aug 7, 2019

Choose a reason for hiding this comment

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

i had that there in 2504528, but removed it in da53f4e because converting it wouldn't do anything since i'm reading out the pixel colors manually instead of copying the buffer


namespace Common {
void FlipRGBA8Texture(std::vector<u8>& tex, u64 width, u64 height) {
ASSERT(tex.size() == width * height * 4);
Copy link
Contributor

@B3n30 B3n30 Aug 7, 2019

Choose a reason for hiding this comment

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

Is this assert really guranteed? It seems like users could add bad sized textures and then citra will crash...

Copy link
Contributor Author

@khang06 khang06 Aug 7, 2019

Choose a reason for hiding this comment

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

since it should always be rgba8, it should be guaranteed

return dumped_textures.find(hash) != dumped_textures.end();
}

void CustomTexCache::SetTextureDumped(const u64 hash) {
Copy link
Contributor

@B3n30 B3n30 Aug 7, 2019

Choose a reason for hiding this comment

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

no const

src/core/custom_tex_cache.cpp Outdated Show resolved Hide resolved
src/core/custom_tex_cache.cpp Outdated Show resolved Hide resolved
src/core/custom_tex_cache.h Outdated Show resolved Hide resolved
src/core/frontend/image_interface.h Show resolved Hide resolved
src/video_core/renderer_opengl/gl_rasterizer_cache.cpp Outdated Show resolved Hide resolved
@RocketRobz
Copy link

RocketRobz commented Aug 7, 2019

@B3n30 I can answer No. 2 and 3.
2. Let's say someone doesn't want an incomplete texture pack to used. One could disable the custom textures, and just keep dumping the original textures, so that they can be edited later on.
(EDIT: nvm. Didn't know there was a dump folder as well.)
3. One reason to not preload custom textures into memory, is because memory might be running low on space.

src/core/frontend/image_interface.h Outdated Show resolved Hide resolved
src/core/frontend/image_interface.h Outdated Show resolved Hide resolved
src/core/frontend/image_interface.h Outdated Show resolved Hide resolved
src/core/frontend/image_interface.h Show resolved Hide resolved
@tywald
Copy link
Contributor

tywald commented Aug 8, 2019

Hm, would it make sense to auto-generate load\[Title ID] folder if it doesn't exist upon starting a game when Use Custom Textures is enabled? Would be convenient for users to not having to manually create.

@B3n30
Copy link
Contributor

B3n30 commented Aug 9, 2019

It seems the mingw build failure is legit :-/

@khang06
Copy link
Contributor Author

khang06 commented Aug 9, 2019

huh? it's been failing with that missing libuv dll for every commit i pushed on mingw...

@B3n30
Copy link
Contributor

B3n30 commented Aug 9, 2019

oh. you are right other PR are failing with that msg too. So is appveyor mingw broken?

In that case I'll add that PR to canary to get some user testing going

@khang06
Copy link
Contributor Author

khang06 commented Aug 10, 2019

reorganized graphics settings following discord discussion and moved speed stuff to general
image
image
image

B3n30
B3n30 approved these changes Aug 10, 2019
@B3n30
Copy link
Contributor

B3n30 commented Aug 10, 2019

Can't add it to canary since it conflicts with #4602.
This will also conflict with #4864.

khang06 added 21 commits Nov 9, 2019
…e comments, fix comments

remove unnecessary conversion
hotfix 2: check if the texture is custom before dumping

hotfix 4: fix custom texture conflict detection
address more comments
@jroweboy
Copy link
Contributor

jroweboy commented Nov 9, 2019

Rebased it for you @khang06 and merging now. Thanks a ton for the contribution, awesome work!

@jroweboy jroweboy merged commit 4f19d38 into citra-emu:master Nov 9, 2019
0 of 4 checks passed
@fuhuan416
Copy link

fuhuan416 commented Dec 3, 2020

Tekken 3D Prime Edition dumped PNGs are inverted 180°
tex1_256x256_D285853CEEDA1EA3_4

@fuhuan416
Copy link

fuhuan416 commented Dec 3, 2020

Tekken 3D Prime Edition dumped PNGs are inverted 180°
tex1_256x256_D285853CEEDA1EA3_4

It's just vertical iverted, isn't by some directions turns.

@fuhuan416
Copy link

fuhuan416 commented Dec 10, 2020

Tekken 3D Prime Edition dumped PNGs are inverted 180°
tex1_256x256_D285853CEEDA1EA3_4

It's just vertical iverted, isn't by some directions turns.

It seems that this is just a small problem, I can solve it with Photoshop. After testing, I found that the map can only be 2x pixels in size. If it exceeds 2x pixels, the replacement is invalid. We hope to increase the pixel size to 4K or higher, because in some games, the pixel itself is not too low. If it is limited to 2x pixels, there will be no Ultra HD visual effect.

@schmurtzm
Copy link

schmurtzm commented Mar 31, 2022

Sadly this awesome feature is impacted by a bug from this PR since one year : #5710
The bug is detailed in this issue : #5728
Retroarch core is also impacted. May be @khang06, @lioncash or @B3n30 you could help on this ?

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.

None yet