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

Fixed depth matrix shaders in OpenGL and Direct3D to be more precise. #823

Merged
merged 1 commit into from Aug 17, 2014
Merged

Fixed depth matrix shaders in OpenGL and Direct3D to be more precise. #823

merged 1 commit into from Aug 17, 2014

Conversation

KScorp
Copy link
Contributor

@KScorp KScorp commented Aug 17, 2014

The depth matrix shaders were experiencing loss of precision because the most significant bits were not being subtracted out when shifting the least significant bits left. This change separates the calculations into several statements in order to subtract out the bits that have already been decoded before shifting left, minimizing precision loss. This fixes some graphical glitches when using Copy EFB to Texture. (EDIT: It seems to fix some issues that appear on both copy to tex and copy to RAM as well.)

Code inspired by the old ARBfp1.0 shader that existed before GLSL-master was merged.

One obvious graphic glitch example (from Skyward Sword):
OpenGL without fix, Copy EFB to Texture:
opengl-nofix
OpenGL with fix, Copy EFB to Texture:
opengl-fix
Similar results with D3D.

@lioncash
Copy link
Member

Don't use tabs for indenting the comments. The coding style says to use spaces for vertical alignment.

@Sonicadvance1
Copy link
Contributor

You're also doing implicit integer to float conversions in those shaders which will fail on GL ES targets.

@lioncash
Copy link
Member

Can you squash these changes back into the original commit?

@KScorp
Copy link
Contributor Author

KScorp commented Aug 17, 2014

Sorry I'm not familiar with git yet, seems that what I did instead is created a new commit with the changes combined. :/

@lioncash
Copy link
Member

@KScorp Alright do the following git rebase -i 66a30d0fc6770fa18fd08f3d305534ec3f459d84

Then put 's' in the place of 'edit' for all of the commits except the original.

@shuffle2
Copy link
Contributor

What do you think of the version which galop1n came up with?
https://github.com/galop1n/dolphin/blob/wip/Source/Core/VideoBackends/D3D/PixelShaderCache.cpp#L130
I remember seeing pics from him saying he fixed this (z:tp) depth bug.

@Parlane
Copy link
Member

Parlane commented Aug 17, 2014

@dolphin-emu-bot rebuild

@KScorp
Copy link
Contributor Author

KScorp commented Aug 17, 2014

Sorry, I seem to be incompetent when it comes to interfacing with Git...

… Fixes some graphical glitches in some games.
@KScorp
Copy link
Contributor Author

KScorp commented Aug 17, 2014

Ok, finally did it! One commit, with the corrections needed so far. I'll look over galop1n's changes tomorrow.

@JMC47
Copy link
Contributor

JMC47 commented Aug 17, 2014

This appears to fix the remaining issues where Mii faces are broken. Good job!

Edit: Okay, I have to take that back. Good job just isn't right for this. This fixes a whole BUNCH of weird issues that have plagued the emulator.

Luigi's Mansion Black Lines!
Metroid Prime Magmoor Cavern Defects and fog issues!
Super Mario Galaxy 2 fog issues!

And likely other things that were only working in D3D9. So, excellent job

Edit2: Found more!

Mario Golf Shadow clipping bugs!

@Parlane
Copy link
Member

Parlane commented Aug 17, 2014

@dolphin-emu-bot rebuild

@degasus
Copy link
Member

degasus commented Aug 17, 2014

Oh, I see how the old code is broken... Nice fix, so LGTM with round().

Out of scope, but I think we should get rid of this colmat variable and just compile a different shader per color format :/

@degasus
Copy link
Member

degasus commented Aug 17, 2014

nvm, I'm wrong with the round() vs floor().

LGTM

@KScorp
Copy link
Contributor Author

KScorp commented Aug 17, 2014

When using galop1n's changes with D3D + MSAA, everything in Skyward Sword is blurrier and the glitch is much worse:
d3d-galop1n

Their method might work with some fixes, I assume the only problem with their code is line 138/179, converting the depth into an integer.

I'm not sure if it would be faster though. Compiling HLSL with fxc, theirs compiles into 30 instructions, while mine compiles to 29. I'm not familiar with how quickly each instruction is executed though, so it would require more analysis.

@JMC47
Copy link
Contributor

JMC47 commented Aug 17, 2014

If your fixes work better, then so be it. I think the point was to get a look at another implementation that "fixed" the issues.

If your version produces better results (which seems to be apparent) I'd prefer your version regardless of tiny speedups.

@shuffle2
Copy link
Contributor

Actually it was just trying to find if that fix was in that branch or not :P ...guess not.

(Sorry if this appears twice, github apparently ignores mails from my phone)

@KScorp
Copy link
Contributor Author

KScorp commented Aug 17, 2014

FYI: Original D3D-MSAA shader compiles into 22 instructions. Again, unsure if this means there will be any performance reduction or not as a result of this change.

(I'm not suggesting to continue using the broken code, this is just a heads up.)

@JMC47
Copy link
Contributor

JMC47 commented Aug 17, 2014

Well, I tested a bunch more games, I don't really see any regressions. LGTM.

delroth added a commit that referenced this pull request Aug 17, 2014
Fixed depth matrix shaders in OpenGL and Direct3D to be more precise.
@delroth delroth merged commit 9e2fbaf into dolphin-emu:master Aug 17, 2014
@purpasmart96
Copy link

What about Mario Tennis, Does this fix any of those issues?

@JMC47
Copy link
Contributor

JMC47 commented Aug 18, 2014

That's zfreeze, different issue.

@purpasmart96
Copy link

In OpenGL the effect is not as effective as it is in D3D, there is still shadow lines in Luigi's mansion under OpenGL, but not as much as there used to be, in D3D it is entirely gone.

@JMC47
Copy link
Contributor

JMC47 commented Aug 18, 2014

That's correct though, there were some there on console. It depends on video card, apparently.

@KScorp
Copy link
Contributor Author

KScorp commented Aug 18, 2014

There are still some inaccuracies that can be fixed. I only fixed a minor coding issue which improved things quite a bit, but the implementation itself is probably flawed. Encoding three 8 bit integers into a float is a (potentially) awful and inaccurate implementation.

I'm still familiarizing myself with the code. I will likely try to redesign these shaders if possible. galop1n's code probably changed both the decoding and encoding, which is why when I tested it it failed miserably.

@KScorp KScorp deleted the depthmatrixshaders branch August 19, 2014 00:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
9 participants