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: Use the copy filter for EFB copies as well as XFB copies #10204

Merged
merged 1 commit into from Nov 15, 2021

Conversation

Pokechu22
Copy link
Contributor

This fixes the pink screens in EA Sports Active. See https://gist.github.com/Pokechu22/49455f9094ed0ff017da64e3f7aa0404 for details.

This will also need to be implemented for the software renderer, which has its own implementation of EFB copies. Unfortunately it doesn't seem like there is a quick fix for it, as the relevant code (in TextureEncoder.cpp, while XFBs are in EfbInterface.cpp) is pretty complicated.

@dolphin-emu-bot
Copy link
Contributor

FifoCI detected that this change impacts graphical rendering. Here are the behavior differences detected by the system:

  • ea-pink on ogl-lin-mesa: diff
  • nfsu-reflections on ogl-lin-mesa: diff
  • spyro-bloom on ogl-lin-mesa: diff
  • spyro-depth on ogl-lin-mesa: diff
  • tla-menu on ogl-lin-mesa: diff
  • ea-pink on ogl-lin-radeon: diff
  • nfsu-reflections on ogl-lin-radeon: diff
  • spyro-bloom on ogl-lin-radeon: diff
  • spyro-depth on ogl-lin-radeon: diff
  • tla-menu on ogl-lin-radeon: diff
  • ea-pink on uberogl-lin-radeon: diff
  • nfsu-reflections on uberogl-lin-radeon: diff
  • spyro-bloom on uberogl-lin-radeon: diff
  • spyro-depth on uberogl-lin-radeon: diff
  • tla-menu on uberogl-lin-radeon: diff

automated-fifoci-reporter

@phire
Copy link
Member

phire commented Nov 15, 2021

I'm super confused as to why Spyro-depth is partially fixed by this PR. I thought I had that bisected to a texture cache issue.

@Pokechu22
Copy link
Contributor Author

Pokechu22 commented Nov 15, 2021

If you look at fifoci itself the issue did first appear in #8350 so your bisect is probably right. For reference, here's a diff for this PR compared to prior to that PR: mesa, radeon.

With spyro-depth, most EFB copies have the copy filter set as (0, 0, 1, 63, 0, 0, 0), but others have it set to (0, 0, 0, 63, 0, 0, 0), which would be slightly darker. A difference in brightness is likely to make a texture's hash change, so probably this resolved a few of the texture cache collisions simply by chance.

I also don't experience that blackness on my machine when using the fifolog (with vulkan and texture cache set to fast). Not sure why.


Oh, also, the reason why this fixes tla-menu is because The Last Airbender is doing almost exactly the same thing as EA Sports Active (a 512 by 16 texture which is used to fake a 3D texture), though there are slight differences (they use a different column in one matrix, and they use different texture units) which makes me think that it's just chance that they came up with similar solutions, rather than the solution being copied between projects.

@JMC47 JMC47 merged commit 26fdc19 into dolphin-emu:master Nov 15, 2021
10 checks passed
@phire
Copy link
Member

phire commented Nov 15, 2021

If you look at fifoci itself the issue did first appear in #8350 so your bisect is probably right.

Yeah, it was a known regression of #8350. But further investigation showed that Minimal TMEM was hiding the actual source of the regression, and I tracked it down to cdbd986. Or at least I thought I did, there were two related regressions and maybe I didn't confirm that both bisected to that commit.

phire added a commit to phire/dolphin that referenced this pull request Nov 16, 2021
This fixes horizontal lines in the bloom effect of Spyro: A Hero's Tail
which is a regression caused by PR dolphin-emu#10204.

Spyro uses an 640x80 pixel sub-buffer within the EFB to calculate
it's bloom effects, which it places below the main 640x448 buffer.
This allows it to do bloom without backing up part of the EFB to
main memory, as most games do.

But, since some of the sub-buffers used in the bloom effect are taller
than 80 pixels, they need to be sliced up into smaller sub, sub buffers
which get combined later when copied to main memory.

At one point, a 320x224 buffer is broken up into 320x80, 320x64 and
320x80 slices. These are copied out with the copy filter set to a
vertical blur.

Because there was an off-by-one errror in the clamping coordiantes,
the bottom line of the color buffer would be blurred into
the top of each slice.

Before dolphin-emu#10204 the copy filter wasn't enabled for efb copies, and most
other games don't do this type of slicing.
@phire phire mentioned this pull request Nov 16, 2021
phire added a commit to phire/dolphin that referenced this pull request Nov 16, 2021
This fixes horizontal lines in the bloom effect of Spyro: A Hero's Tail,
which is a regression caused by PR dolphin-emu#10204

Screenshot of regression:
https://user-images.githubusercontent.com/138484/142030503-90fcd8d5-63d3-4820-874a-72e9be0c4768.png

Fixed:
https://user-images.githubusercontent.com/138484/142031598-b85ff55c-1302-4e4d-bcb2-57848974056b.png

Spyro uses an 640x80 pixel sub-buffer within the EFB to calculate
it's bloom effects, which it places below the main 640x448 buffer.

EFB layout:
https://user-images.githubusercontent.com/138484/142030573-e933b6ae-c37e-4be6-86d4-0bc779b92535.png
Note: Colors are wrong because the main color buffer uses RGBA6,
      while the bloom is calculated in RGB8

This allows it to do bloom without backing up part of the EFB to
main memory, as most games do.

But, since some of the sub-buffers used in the bloom effect are taller
than 80 pixels, they need to be sliced up into smaller sub, sub buffers
which get combined later when copied to main memory.

At one point, a 320x224 buffer is broken up into 320x80, 320x64 and
320x80 slices. These are copied out with the copy filter set to a
vertical blur.

Because there was an off-by-one errror in the clamping coordinates,
the bottom line of the color buffer would be blurred into
the top of each slice.

Final combined EFB copy:
https://user-images.githubusercontent.com/138484/142031360-2c076839-7c96-4b3b-a093-d899d0a2c7ae.png

Fixed version:
https://user-images.githubusercontent.com/138484/142031370-72e41a35-3b3e-4662-a483-79203e357ecc.png

Before dolphin-emu#10204 the copy filter wasn't enabled for efb copies, and most
other games don't do this type of slicing.

FIFO CI shows that a few other games are effected, it's always just a minor difference to the top line where there was previously a slight hint of garbage.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
6 participants