-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
VideoBackends: Use proper floating point depth precision. #2369
Conversation
Can you explain in more detail what this does? |
Hardware tests showed that our depth values are off-by-one in the hardware video backends: https://gist.github.com/Armada651/bab3e023b5b813e18cb7#comment-1446986 Using a graphics analyzer I could see that we are accurate to the point where we actually store the depth value. According to Fiora this inaccuracy was introduced by the divisor so I changed that after which the D3D backend passes the hardware test. The OGL backend still fails the test though, but now in the other direction with a value of 13884591.
|
It's a problem of "correct range, correct precision, but not both". The only way to losslessly store a 24-bit integer into a 32-bit float is to multiply it by a power of 2 before conversion (or no multiply at all). 0xFFFFFF isn't a power of 2, so error will occur. This unfortunately also means "max depth" (0xFFFFFF) won't actually map to 1.0. I don't know enough about the graphics stack to know if this matters. |
Confirmed to fix pokemon snap in D3D, this PR currently breaks OGL because it's now off-by-one in the other direction. |
@degasus Since the OGL backend reads the depth buffer as integers this is a bit difficult to fix. It may be fixed if we just use I could also switch the OGL backend to use floating points, but that's a bit of a shame since integers more accurately reflect the console. Another option is to switch back to the old divisor and add or subtract one where needed like we do with the epsilon hack, but I don't really like adding more epsilons to our code. |
@@ -146,7 +146,7 @@ const char depth_matrix_program[] = { | |||
" in float4 pos : SV_Position,\n" | |||
" in float3 uv0 : TEXCOORD0){\n" | |||
" float4 texcol = Tex0.Sample(samp0,uv0);\n" | |||
" int depth = int(round(texcol.x * float(0xFFFFFF)));\n" | |||
" int depth = int(round(texcol.x * 16777216.0));\n" |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
@@ -89,7 +89,7 @@ FramebufferManager::FramebufferManager(int targetWidth, int targetHeight, int ms | |||
glTexParameteri(m_textureType, GL_TEXTURE_MAX_LEVEL, 0); | |||
glTexParameteri(m_textureType, GL_TEXTURE_WRAP_S, GL_CLAMP_TO_EDGE); | |||
glTexParameteri(m_textureType, GL_TEXTURE_WRAP_T, GL_CLAMP_TO_EDGE); | |||
glTexImage3D(m_textureType, 0, GL_DEPTH_COMPONENT24, m_targetWidth, m_targetHeight, m_EFBLayers, 0, GL_DEPTH_COMPONENT, GL_UNSIGNED_INT, nullptr); | |||
glTexImage3D(m_textureType, 0, GL_DEPTH_COMPONENT24, m_targetWidth, m_targetHeight, m_EFBLayers, 0, GL_DEPTH_COMPONENT, GL_FLOAT, nullptr); |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
@degasus Ready for review. Do you still want to switch the D3D depth buffer from |
@@ -1224,11 +1224,11 @@ static inline void WritePerPixelDepth(T& out, pixel_shader_uid_data* uid_data, A | |||
if (ApiType == API_OPENGL) | |||
out.Write("\tscreenpos.y = %i - screenpos.y;\n", EFB_HEIGHT); | |||
|
|||
out.Write("\tdepth = float(" I_ZSLOPE".z + " I_ZSLOPE".x * screenpos.x + " I_ZSLOPE".y * screenpos.y) / float(0xFFFFFF);\n"); | |||
out.Write("\tdepth = float(" I_ZSLOPE".z + " I_ZSLOPE".x * screenpos.x + " I_ZSLOPE".y * screenpos.y) / 16777216.0;\n"); |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
@Armada651 If there are no issues, we should use the same format on both backends. |
96a466b
to
de265c7
Compare
The sky is black in the simpsons-tev above. |
@ZephyrSurfer We're aware of the regression, but we're probably going to merge it without fixing that bug. Since the sky in simpsons-tev is also black on the Software backend and that game has never worked correctly in Dolphin anyway we don't see a need to fix it right now. |
…t instead or rounding.
LGTM, but please wait for fifoci ;) |
@Armada651: This comment grants you the permission to merge this pull request whenever you think it is ready. After addressing the remaining comments, click this link to merge. @dolphin-emu-bot allowmerge |
VideoBackends: Use proper floating point depth precision.
FifoCI detected that this change impacts graphical rendering. Here are the behavior differences detected by the system:
automated-fifoci-reporter |
Some good and some bad. This looks nice. Some broken ones. |
Too bad Windows/DX builder was stuck, so we didn't get this one earlier. Simpsons however was more or less accepted as broken, since it never worked correctly. Def Jam however is odd. |
Still doing some accuracy tests, but it seems promising that this will fix our off-by-one inaccuracies.