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

VideoCommon: update TextureCache logic for finding oversized XFBs #9694

Merged
merged 1 commit into from May 9, 2021

Conversation

iwubcode
Copy link
Contributor

@iwubcode iwubcode commented May 7, 2021

Previously, when looking for XFBs in the texture cache, if their dimensions didn't match VI we were recomputing the hash. However, when we did so we weren't using the same hash logic as we did when actually hashing the entry.

Additionally, when dealing with oversized XFBs (which are resized on creation) the hashing algorithm is actually different than the one used to hash the VI registers that we compare against. This causes these copies to be invalidated, meaning we will fall back to stitching from guest memory (if it exists).

This change fixes both of those issues. This seems to resolve 12392

@iwubcode iwubcode marked this pull request as draft May 7, 2021 01:57
@iwubcode iwubcode changed the title TextureCacheBase: update oversized texture hashing function to mimic the one used by texture entries VideoCommon: update TextureCache logic for finding oversized XFBs May 7, 2021
@iwubcode iwubcode marked this pull request as ready for review May 7, 2021 03:19
@iwubcode
Copy link
Contributor Author

iwubcode commented May 7, 2021

@stenzek - curious if you have any thoughts on this one!

@phire
Copy link
Member

phire commented May 7, 2021

Also, looking though texture cache code. I find it a little suspicious that we have separate stitching functions for xfb and efb textures (StitchXFBCopy and DoPartialTextureUpdates). It's a bad sign that neither function is generic enough.

The original intention of merging the xfb cache into texture cache was so that only one function to stitch copies would be needed. That it would be generic and able to handle every single usecase.

There is no fundamental difference between efb and xfb copies. They just have a different texture format.

@phire
Copy link
Member

phire commented May 7, 2021

Looks like I left a long comment on this topic back when the separate path was originally PRed: #7412 (comment)

Fixing the fundamental issue (that dolphin's texture cache was implemented at the level of whole textures and any kind of handling for non-matching transformations has been hacked on) probably requires major refactors.

@iwubcode
Copy link
Contributor Author

iwubcode commented May 7, 2021

Also, looking though texture cache code. I find it a little suspicious that we have separate stitching functions for xfb and efb textures (StitchXFBCopy and DoPartialTextureUpdates). It's a bad sign that neither function is generic enough.

Yeah, when originally doing HybridXFB, there was a reason I couldn't consolidate the two completely. Well and DoPartialTextureUpdates is a horrible name :P.

I believe the difference was in the NES zelda in Zelda Collector's Edition. Something about there being some bounds difference? Maybe there have been enough changes to warrant another look.

Looks like I left a long comment on this topic back when the separate path was originally PRed

I remember that. I agree, that would get rid of all the weird EFB specific checks we do right now. All the mess in GetTexture. Though, I think we'd have more work to do to handle hires textures.

I'd be interested in looking into that. Maybe once all my enhancement features are finished.

@phire
Copy link
Member

phire commented May 7, 2021

and DoPartialTextureUpdates is a horrible name :P.

Because when it was originally created, it did one thing only. Handle partial texture updates to the main texture atlas in NSMB Wii.

It's been expanded and has grown into a monster since then.

@iwubcode
Copy link
Contributor Author

iwubcode commented May 7, 2021

@JMC47 - please retest when you have time!

@iwubcode iwubcode marked this pull request as draft May 7, 2021 18:36
@iwubcode iwubcode marked this pull request as ready for review May 7, 2021 18:45
@iwubcode iwubcode marked this pull request as draft May 7, 2021 18:47
@JMC47
Copy link
Contributor

JMC47 commented May 8, 2021

This no longer fixes the games in the latest revision.

@iwubcode iwubcode force-pushed the xfb-tcache-hash branch 2 times, most recently from e1e35a9 to face3c0 Compare May 8, 2021 05:21
@iwubcode iwubcode marked this pull request as ready for review May 8, 2021 05:22
@iwubcode iwubcode requested a review from phire May 8, 2021 05:22
@JMC47
Copy link
Contributor

JMC47 commented May 8, 2021

It is now fixing the games again.

…nst a newly calculated one. This fixes games like Teenage Mutant Ninja Turtles (Wii) which use oversized textures where the stride doesn't match the BytesPerRow and that resulted in a different hash algorithm being used. By not hashing the texture before, we improve performance by hashing at most once in all direct XFB lookup scenarios.
@JMC47
Copy link
Contributor

JMC47 commented May 9, 2021

Let's get this merged early in the month in case there are some unknown edge cases beyond what I'm aware of that crop up.

@JMC47 JMC47 merged commit eb5cd9b into dolphin-emu:master May 9, 2021
@iwubcode iwubcode deleted the xfb-tcache-hash branch May 9, 2021 21:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants