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
VideoCommon: migrate texture packs to use the asset loader system #11681
Conversation
That is a significant behavior change. I assume this can still be avoided by preloading? |
Yes, preloading is still supported. |
49830d8
to
24906f4
Compare
24906f4
to
e0ceb11
Compare
e0ceb11
to
9361792
Compare
This comment was marked as outdated.
This comment was marked as outdated.
9361792
to
014904c
Compare
014904c
to
133ddfa
Compare
133ddfa
to
ab1937d
Compare
ab1937d
to
583f3a4
Compare
Okay, this is a pretty unfamiliar corner of the code for me, so let me know if I got the logic right.
Both of these cases don't actually load the texture into memory yet, but only create an unfilled Asset object that will then be asynchonously filled by a background thread. Okay, I think that's right so far. But then I'm not entirely sure what happens next. Whenever a texture is then rendered by the game,
But anyway, that aside, assuming the reloading did work correctly, that still means:
One other thing I noticed: Does the wildcard feature actually work properly when not prefetching here? I don't think it does; not only are cached wildcard textures preferred to uncached non-wildcard textures, the You also never clear |
fwiw I think if you just make |
aaa5cfe
to
8ebc832
Compare
First of all, thank you so much Admiral for reviewing this! Especially when it's not your usual stomping grounds :)
Yes, you are!
If the texture doesn't exist in cache, it will create a texture. As part of this creation process, if high resolution textures are enabled, we try and load an asset as a "linked_asset", which is both the asset and a cached load time.
Next time the game tries to access that asset, we check to see if our cached time is older than our asset's load time and if so we reload the asset. There was a bug here...which I'll talk about after all this.
Yes, this was a great catch. You're right I could have used a
This is actually an "issue" in the current logic too. From my understanding, most people will just check and uncheck the "Load Textures" box to force a refresh. It is klunky but users are used to it. I was planning on reworking this with a mythical editor.
Yes.
That is a good point, hmm.
Wildcard worked with prefetch it was broken without it. I fixed that. Thank you for catching that!
Thank you for that as well! Updated. |
Ok, now to the bug. I noticed that the original code wrapped the cache calls with this:
I was only doing that in the initial load. This Regardless, once I put that in, I noticed none of my textures were loading with prefetch off. Why? Well first was that I was destroying the last reference to the asset (because the asset wasn't stored in the cache anymore!) before loading the new texture which caused things to reset. But even after fixing that, I still saw the issue. Turns out the game I was testing was leveraging the tmem caching! I now provide a |
8ebc832
to
533caf7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks sane to me now. Someone should probably still test a few edge cases, maybe I'll do that later.
…ets to be reloaded
… cache, this is no longer needed, assets reload automatically!
533caf7
to
e831d7b
Compare
Okay, tested this a bit. Seems to work fine but I found a crash when a texture is renamed/removed while the game is running -- you're using the throwing |
Oh, that piece of code was in another PR... well whatever, I'll just merge this and then fixup in a different PR. |
Specifically it's the two calls in https://en.cppreference.com/w/cpp/filesystem/last_write_time |
Thanks @AdmiralCurtiss ! I didn't even think of that. I can open a review in a few hours. |
This moves high resolution textures to be loaded as assets by the asset loader system. This makes the high resolution textures load in an asynchronous manner and also allows for them to be reloaded automatically:
High resolution textures did have some particular behaviors that didn't fit very well in the asset loader system, I've listed them below.
Change: The OSD messages about all textures being loaded was removed.
Response: I know that this message is useful for debugging. This doesn't make sense for the asset loader though because an asset can be loaded at any time (not just on start up). My idea is to add an imgui window that lists a count of every asset type loaded. I haven't done this yet. If this is a blocker, I can implement it in a separate review before merging this.
Change: High resolution textures no longer block the game when they need to load. This eliminates stutter at the cost of textures "popping in".
Response: On a higher fidelity machine, this doesn't seem distracting and all textures seem to be loaded just as quickly as on master. At least one or two users I've spoken with said they'd prefer pop-in over stutter if given the choice. And at least one other emulator offers this option (while also mentioning this is the preferred approach) so moving to this wouldn't surprise users.
Change: Previously, if the high resolution textures exceeded the memory limit Dolphin calculated, high resolution textures would be turned off completely and aborted. Now the system will just reject future assets, logging that the memory cap has been reached.
Response: I think the previous solution was a poor decision and think that users which are memory capped would rather see some of the textures then none at all. In the future a smarter memory management system would possibly alleviate this.
Change: Previously, mip textures were more flexible and could be anywhere in the texture pack.
Response: This made sense given the solution but is now difficult to do. I could introduce a separate library system just for high-resolution textures but in practice most packs I've seen keep their mip textures next to the main textures or are using DDS textures with the mips included. Most packs seem to use the same file type for the mip as they do the main texture too. The odd packs that don't should be updated to one of the aforementioned solutions.
TODO: