Software: Remove alpha=1 blending special-case #11389
Conversation
This was added in dolphin-emu#10394 for both the hardware and software backends to work around an issue with Mario Kart Wii, Fortune Street, and Baten Kaitos. However, it seems like the software renderer handles blending well enough that we don't need this (and in any case, it's easy to change blending in the software renderer). Some experimentation with dolphin-emu#11387 (not pushed) showed that the software renderer's logic would also produce correct results on the hardware backends with this hack removed, but would require fbfetch (currently); if a better solution is found the hack can also be removed from the hardware backends.
|
FifoCI detected that this change impacts graphical rendering. Here are the behavior differences detected by the system:
automated-fifoci-reporter |
|
Nearly all of those fifoci diffs are just a few individual pixels changing slightly (probably alpha=1 showing up naturally near the edge of transparent objects). The only exception seems to be the clouds in xenoblade-menu / xblade-bloom (but I can confirm (at least for xblade-bloom) that the new result is closer to correct than it was before; the colors seem to match almost perfectly now before the bloom step), and bk-tev where a few things also got slightly brighter. bk-tev, mkw-flags, and fortune-street-white-box did not regress. For reference, here's the software renderer's blend logic: dolphin/Source/Core/VideoBackends/Software/EfbInterface.cpp Lines 311 to 331 in 82e5f43 The key logic here is Then, after multiplying and also adding in the dest factor with the same logic, the code divides by 256, truncating towards zero (written as If the source factor (e.g. alpha value) is 1 and the source color is 255, then the output is Similar logic exists for the tev's |
|
Thanks for the write up @Pokechu22 ! Your explanation makes sense. You mention in your original PR that it fixed https://bugs.dolphin-emu.org/issues/12713#note-6 . Does that still work properly? |
| @@ -544,19 +544,6 @@ void Tev::Draw() | |||
| if (!TevAlphaTest(output[ALP_C])) | |||
| return; | |||
|
|
|||
| // Hardware testing indicates that an alpha of 1 can pass an alpha test, | |||
There was a problem hiding this comment.
I see this blurb was written by you. You said it was hardware tested but then you have the TODO. Does that mean there was some issue with the hardware test you performed or was there another reason you doubted the correctness of this behavior?
I see in the original PR that you mention that the second commit (the one that contains this change) you were less certain on...
There was a problem hiding this comment.
I think I see what you are saying, it was only tested in one scenario, so while there was a single case that was tested, not every case was tested and therefore it needed more investigation.
There was a problem hiding this comment.
Ok, sorry for the comment spam. I was a bit confused initially why this was added (especially if the original logic in Software Renderer was working) but I think I fully understand the history now. In addition to the one scenario bit above, you were also trying to resolve an issue with the hardware renderers. So in order to be consistent, you also added the workaround to the software renderer.
If the plan is to fix the hardware renderers and avoid this hack altogether, then I have no concerns. Even if not, software being the ideal solution is fine too.
There was a problem hiding this comment.
Yes, that's correct. I was trying to remove a previous issue that affected the hardware renderers, and removed an older hack (#4514) that existed in both the software renderer and the hardware renderers. I then added an alternative hack on alpha=1 that was needed to avoid regressions in fortune street caused by removing the original hack, and I added that to both the software renderer and hardware renderers.
I did investigate what happens both when the first hack is removed and the second added in #10394 (comment), and I even noted that there was "No diff" for fortune-street-white-box on the software renderer, but I didn't realize at the time that that meant that the software renderer didn't regress from removing the hack and thus it didn't need the new alpha=1 hack at all. This PR removes that hack.
Incidentally, the old hack wasn't added to the software renderer in #4514, but instead was accidentally added in 1dead05 for both color and alpha, and then fixed for color (but not alpha) in 5e1c6ab. So, instead of being deliberately introduced there, the exact same behavior existed by accident, and thus was there for me to remove in my PR.
I'd like to remove the hack in the hardware renderers too, but I'm not sure how feasible that will be.
|
Yes, micro machines still works properly with this PR. |
This was added in #10394 for both the hardware and software backends to work around an issue with Mario Kart Wii, Fortune Street, and Baten Kaitos. However, it seems like the software renderer handles blending well enough that we don't need this (and in any case, it's easy to change blending in the software renderer).
Some experimentation with #11387 (not pushed) showed that the software renderer's logic would also produce correct results on the hardware backends with this hack removed, but would require fbfetch (currently); if a better solution is found the hack can also be removed from the hardware backends.