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

PixelShaderGen: Cleanups and fixes for tev combiners. #193

Merged
merged 5 commits into from Mar 26, 2014

Conversation

neobrain
Copy link
Member

This was meant to be merely a cleanup, but it turned out to also fix the shadows issue in Luigi's Mansion.

It also fixes a small issue in the tev compare function, where

"(((%s.r&255) > %s.r) ? (%s&255): int3(0,0,0))", // TEVCMP_R8_GT
"(((%s.r&255) == %s.r) ? (%s&255): int3(0,0,0))", // TEVCMP_R8_EQ

was incorrectly used when it should have actually been

"(((%s.r&255) > (%s.r&255)) ? (%s&255): int3(0,0,0))", // TEVCMP_R8_GT
"(((%s.r&255) == (%s.r&255)) ? (%s&255): int3(0,0,0))", // TEVCMP_R8_EQ

However, this is not what fixed the shadows in Luigi's Mansion. There seems to be another functional chance with this patch, but I wasn't able to figure out where. If you find what else it could be, I'm curious to hear it :p

UPDATE: Turned out two very critical issues was fixed by these patches: The alpha combiner was using incorrect input values in compare mode. But when fixing that issue, the code also needed to be rearchitectured a bit since otherwise the color combiner would overwrite the inputs for the alpha combiner.

"(((%s.a&255) == (%s.a&255)) ? (%s.a&255) : 0)" // TEVCMP_A8_EQ
"((tevin_a.r > tevin_b.r) ? tevin_c.a : 0)", // TEVCMP_R8_GT
"((tevin_a.r == tevin_b.r) ? tevin_c.a : 0)", // TEVCMP_R8_EQ
"((idot(tevin_a.rgb, comp16) > idot(tevin_b.rgb, comp16)) ? tevin_c.a : 0)", // TEVCMP_GR16_GT

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

@neobrain
Copy link
Member Author

Thanks for @degasus for pointing out the "other" functional change. I'll verify that this is indeed correct hw behavior before we merge this change.

@neobrain
Copy link
Member Author

Additionally, this branch now implements the bit-perfect lerping algorithm now, which I found via hardware tests. I still have to verify the first change, however.

// combine the color channel
if (cc.bias != TevBias_COMPARE) // if not compare
{
out.Write("(((tevin_d.rgb%s)%s) %s ((((tevin_a.rgb*256 + (tevin_b.rgb-tevin_a.rgb)*(tevin_c.rgb+(tevin_c.rgb>>7)))%s)%s)>>8))%s", tevBiasTable[cc.bias], tevScaleTableLeft[cc.shift], tevOpTable[cc.op], tevScaleTableLeft[cc.shift], tevLerpBias[2*cc.op+(cc.shift==3)], tevScaleTableRight[cc.shift]);

This comment was marked as off-topic.

This comment was marked as off-topic.

@neobrain
Copy link
Member Author

Verified against hw that the new behavior indeed is correct. Will clean up the lerper, then it should be good for merging.

@degasus
Copy link
Member

degasus commented Mar 23, 2014

Do you also want to fix videosw in the same PR?

@neobrain
Copy link
Member Author

Probably, because it's the same fix after all.

@neobrain
Copy link
Member Author

Should be good to merge. Tell me what you think :)

@Sonicadvance1
Copy link
Contributor

LGTM

@degasus
Copy link
Member

degasus commented Mar 25, 2014

LTGM


out.Write("(((tevin_d.%s%s)%s)", components, tevBiasTable[bias], tevScaleTableLeft[shift]);
out.Write(" %s ", tevOpTable[op]);
out.Write("((((tevin_a.%s*256 + (tevin_b.%s-tevin_a.%s)*(tevin_c.%s+(tevin_c.%s>>7)))%s)%s)>>8)",

This comment was marked as off-topic.

…also reproduces hardware behavior perfectly.

The new behavior has been verified to be correct by hardware tests. This is an improvement over the old code, which was just a guess.
@neobrain
Copy link
Member Author

Added comments explaining wtf is going on.

delroth added a commit that referenced this pull request Mar 26, 2014
PixelShaderGen: Cleanups and fixes for tev combiners.
@delroth delroth merged commit ea6b37c into dolphin-emu:master Mar 26, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
5 participants