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

Add BlueToAlpha compat.ini workaround, fixes Split/Second graphics #15500

Merged
merged 8 commits into from
May 1, 2022

Conversation

hrydgard
Copy link
Owner

@hrydgard hrydgard commented Apr 24, 2022

Fixes #13957 , with a much faster solution than #15501.

(I have some ideas for how to make #15501 faster on Vulkan especially, but those will have to wait for later).

Adds a compatibility option to redirect the blue channel to the destination alpha channel if 565 mode is rendered and the mask is specifically 0x0FFFFF, which converts to 0xF000 in 565 mode*. Very technical explanation below!

This trick is used by several games (for example, Outrun) to render to the alpha channel of RGBA4444 images, which cannot normally be done directly on the PSP.

This new workaround for the trick can also be enabled for Colin McRae's DiRT 2 and Outrun and appears to work fine in both (and improves color precision), although for now I'm just enabling it for Split/Second to avoid additional testing for now. For Outrun I previously did the ReinterpretFramebuffers/ShaderColorBitmask options which emulate this a bit more accurately, but at a pretty substantial performance penalty and complexity. A separate fix that make those work in Split/Second as an alternative is in #15501 .

What the games are doing can be seen like this:

First render to the ABGR4444 texture normally, initializing the RGB channels:

aaaa BBBB GGGG RRRR
0000 1111 1111 1111

Then switch to RGB565 mode which looks like this, and render to blue:

BBBB bggg ggrr rrrr
1111 0000 0000 0000

Which thus end up rendering your blue contents into the alpha channel of the 4444 texture.

This change detects this situation, and sets up blending and shaders to do this directly, instead of trying to convert the render target back and forth (since we can't directly alias the bits, due to us always using 32-bit targets plus APIs not directly supporting that, and we also can't directly support bit-level color masks cheaply).

This turns out to be enough to completely fix the remaining graphics issues in Split/Second.

image

*The color mask is converted as follows: First invert into F0 00 00, then each byte is mapped to the corresponding N bits of the channel. So in BGR565 mode, the mask turns into 0xF000, which means that the top four blue bits match the alpha bits of a ABGR4444 texture.

@hrydgard hrydgard added the GE emulation Backend-independent GPU issues label Apr 24, 2022
@hrydgard hrydgard added this to the v1.13.0 milestone Apr 24, 2022
@hrydgard
Copy link
Owner Author

hrydgard commented Apr 24, 2022

Yeah I just realized that the old problem was that the old ReinterpretFramebuffers/ShaderColorBitmask doesn't properly support blending. So could possibly fix that as an alternative, but would be slower than this and looks slightly worse (and this one avoids reducing color precision).

hrydgard added a commit that referenced this pull request Apr 24, 2022
…k for Split/Second

It took writing and debugging #15500 for me to understand what the issue with the old path was..

Much simpler alternative to #15500, or we could merge both but disable Split/Second
for this one. Needs some benchmarks I guess...
hrydgard added a commit that referenced this pull request Apr 24, 2022
…k for Split/Second

It took writing and debugging #15500 for me to understand what the issue with the old path was..

Much simpler alternative to #15500, or we could merge both but disable Split/Second
for this one. Needs some benchmarks I guess...
draw_->BindFramebufferAsRenderTarget(vfb->fbo, { Draw::RPAction::DONT_CARE, Draw::RPAction::DONT_CARE, Draw::RPAction::DONT_CARE }, reinterpretStrings[(int)oldFormat][(int)newFormat]);
draw_->BindFramebufferAsRenderTarget(vfb->fbo, { Draw::RPAction::DONT_CARE, Draw::RPAction::CLEAR, Draw::RPAction::CLEAR }, reinterpretStrings[(int)oldFormat][(int)newFormat]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are we sure that clearing depth makes sense here? There's no real reason that this operation should delete depth data - I could see that breaking something like Jeanne d'Arc's rendering unexpectedly.

-[Unknown]

Copy link
Owner Author

Choose a reason for hiding this comment

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

Right, should be KEEP probably..

Copy link
Owner Author

Choose a reason for hiding this comment

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

Switched to KEEP.

Many of these render targets are really "depth-less", no operations on them touch any depth buffer. In the future I plan to add support for "depth-less" render passes which should reduce the memory bandwidth usage a bit in cases like this while being as safe as KEEP.

@unknownbrackets
Copy link
Collaborator

I'm not a huge fan of compat flags, although I realize this is a problem of "looking into the future" to know how the framebuffer will be later used (potentially, and worst case, in a future frame.)

I wonder if it makes sense to track bit flags of used formats on a vfb though (we already have format and drawnFormat), which might better inform of something like this is safe. I could imagine having at most a few flags:

  • drawnFormats
  • displayFormats
  • texturedFormats
  • downloadFormats
  • clutFormats

I imagine that displayFormats might be nil for this sort of framebuffer, and texturedFormats would only be 4444. But drawn formats would include 4444 | 565. That would probably indicate pretty clearly (on the second frame) that doing this is safe, without the need of a compat flag. Unfortunately, it'd still be wrong on the first frame.

Probably could even be packed into usageFlags if it were 32 bits.

-[Unknown]

@hrydgard hrydgard force-pushed the split-second-work branch from a96e384 to 5868cf0 Compare April 30, 2022 16:18
Copy link
Owner Author

@hrydgard hrydgard left a comment

Choose a reason for hiding this comment

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

Addressed the feedback.

Yeah I know it's not ideal with compat.ini flags. This will however perform a lot better than reinterpreting the buffers over and over, which Split/Second ends up doing like 40x per frame otherwise.

There is a faster way to do the reinterprets on Vulkan though using input attachments, avoids the copy. But that's a quite involved change which I don't think I'll get in before the next release.

Oh yeah, and tracking of all the formats would indeed make this a bit safer and having all that information you suggest would be pretty neat indeed. Wrong on first frame does not matter that much for Split/Second at least I think..

draw_->BindFramebufferAsRenderTarget(vfb->fbo, { Draw::RPAction::DONT_CARE, Draw::RPAction::DONT_CARE, Draw::RPAction::DONT_CARE }, reinterpretStrings[(int)oldFormat][(int)newFormat]);
draw_->BindFramebufferAsRenderTarget(vfb->fbo, { Draw::RPAction::DONT_CARE, Draw::RPAction::CLEAR, Draw::RPAction::CLEAR }, reinterpretStrings[(int)oldFormat][(int)newFormat]);
Copy link
Owner Author

Choose a reason for hiding this comment

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

Switched to KEEP.

Many of these render targets are really "depth-less", no operations on them touch any depth buffer. In the future I plan to add support for "depth-less" render passes which should reduce the memory bandwidth usage a bit in cases like this while being as safe as KEEP.

@hrydgard
Copy link
Owner Author

hrydgard commented May 1, 2022

I'm gonna go ahead and get this in. It's reasonably isolated and easy to remove if we want to, and it actually improves the image quality of reflections quite a lot thanks to not needing to crush color precision when reinterpreting the buffer, in all the affected games (Split/Second, DiRT, Outrun) (although haven't enabled it in the two latter ones).

@hrydgard hrydgard merged commit a987261 into master May 1, 2022
@hrydgard hrydgard deleted the split-second-work branch May 1, 2022 10:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GE emulation Backend-independent GPU issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Split-Second Missing HUD for 3rd person view
2 participants