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

ShaderGen: Toggle value of uninitialized color. #3315

Merged
merged 2 commits into from Dec 18, 2015

Conversation

degasus
Copy link
Member

@degasus degasus commented Dec 5, 2015

SMS seems to need 0, no regressions either.

@degasus degasus changed the title VertexShaderGen: Toggle value of uninitialized color ShaderGen: Toggle value of uninitialized color. Dec 5, 2015
@Sonicadvance1
Copy link
Contributor

I support this PR

@@ -149,8 +149,8 @@ static void GenerateLightingShader(T& object, LightingUidData& uid_data, int com
else
// TODO: this isn't verified. Here we want to read the ambient from the vertex,
// but the vertex itself has no color. So we don't know which value to read.
// Returing 1.0 is the same as disabled lightning, so this could be fine
object.Write("lacc = int4(255, 255, 255, 255);\n");
// Those boxes in SMS seems to need 0.

This comment was marked as off-topic.

@delroth
Copy link
Member

delroth commented Dec 5, 2015

I support this PR if it gets an hwtest.

@JMC47
Copy link
Contributor

JMC47 commented Dec 5, 2015

Fixes a super interesting issue. All good for me. How do we test it on hardware?

@bb010g
Copy link
Contributor

bb010g commented Dec 6, 2015

What's happening with the inverted-depth-range tests?

@degasus
Copy link
Member Author

degasus commented Dec 6, 2015

@bb010g I'm not sure which one is correct.

@dolphin-emu-bot
Copy link
Contributor

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

  • inverted-depth-range on ogl-lin-intel: diff
  • sms-gc on ogl-lin-intel: diff
  • inverted-depth-range on ogl-lin-mesa: diff
  • sms-gc on ogl-lin-mesa: diff
  • inverted-depth-range on ogl-lin-nv: diff
  • sms-gc on ogl-lin-nv: diff
  • inverted-depth-range on sw-lin-mesa: diff
  • sms-gc on sw-lin-mesa: diff

automated-fifoci-reporter

@phire
Copy link
Member

phire commented Dec 18, 2015

I'm not sure we really need hardware tests for this. FifoCI results are very positive.

delroth added a commit that referenced this pull request Dec 18, 2015
ShaderGen: Toggle value of uninitialized color.
@delroth delroth merged commit de21da5 into dolphin-emu:master Dec 18, 2015
@roflcopter777
Copy link

Slight issue with 4.0-8388, this change completely breaks THPS3, as in causing invisible characters in-game. I reported this to the Dolphin Team but they claim to only "be my issue on my end" and yet, I tested 4.0-8385 and the issue is completely nonexistent.

https://www.youtube.com/watch?v=r-7P9SWaWSM This was tested with 4.0-8443, why this is happening, I don't know. I don't even think this will reach anyone or prove vital. I don't even know why I'm reporting this issue as it will only fall on deaf ears anyway. Again, because "it must be your end".

@JosJuice
Copy link
Member

@roflcopter777 Please create an issue report at the issue tracker. Most developers don't monitor GitHub comments on closed PRs. https://bugs.dolphin-emu.org/projects/emulator

@JosJuice
Copy link
Member

@roflcopter777 JMC made a report now, so you don't have to make a new one. You can comment on it if you want to: https://bugs.dolphin-emu.org/issues/9210

@roflcopter777
Copy link

Sigh Dammit, I'm so sorry for being a douche -_- I should've been the one to make the report.

How do I comment on that page? There's no field for entering text.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
9 participants