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.

Sign up for GitHub

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

Software: Remove alpha=1 blending special-case #11389

Merged
merged 1 commit into from
Dec 29, 2022
+0 −13
Merged

Software: Remove alpha=1 blending special-case #11389

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 0 additions & 13 deletions Source/Core/VideoBackends/Software/Tev.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Contributor

Choose a reason for hiding this comment

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

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...

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@iwubcode iwubcode Dec 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

// but doesn't do anything in blending
// This situation is important for Mario Kart Wii's menus (they will render incorrectly if the
// alpha test for the FMV in the background fails, since they depend on depth for drawing a yellow
// border) and Fortune Street's gameplay (where a rectangle with an alpha value of 1 is drawn over
// the center of the screen several times, but those rectangles shouldn't be visible).
// Blending seems to result in no changes to the output with an alpha of 1, even if the input
// color is white.
// TODO: Investigate this further: we might be handling blending incorrectly in general (though
// there might not be any good way of changing blending behavior)
if (output[ALP_C] == 1)
output[ALP_C] = 0;

// z texture
if (bpmem.ztex2.op != ZTexOp::Disabled)
{
Expand Down