Fix rendering issue with depth in WebXR

[dsnopek]

Currently on master, in WebXR there is a rendering issue with depth where it draws some triangles in front, that really should be behind.

Here's a screenshot showing the problem (most obvious on the fingers):

And here's a screenshot with this PR:

It looks like this issue was introduced in PR #87386

[clayjohn]

It sounds like the depth just wasn't being written as we suspected. But the fix seems odd to me. The result is that we will define a fragment out value even when rendering to depth which could have a negative performance impact on some devices.

I'll take a deeper look on Monday and see how those defines are set

[dsnopek]

I've been looking at this some more. It seems like the glDrawBuffers(0, nullptr) before doing the depth-only pass should make it so we don't need to have the fragment color out.

However, I wonder if there are some differences here between OpenGL and GLES? Is it possible that some GLES implementations require having a fragment color out if the framebuffer has a color attachment?

Looking at the scene.glsl for GLES3 in Godot 3, it appears to always be declaring the fragment color out, even when RENDER_DEPTH is defined (which I think is the equivalent to MODE_RENDER_DEPTH, but I'm less familiar with the Godot 3 renderer). Perhaps Godot 3 is doing that for a reason?

[dsnopek]

I had the idea that maybe switching all the glDrawBuffers(0, nullptr) to this might work:

{
  GLuint db = GL_NONE;
  glDrawBuffers(1, &db);
}

Since I saw some WebGL code like gl.drawBuffers([ gl.NONE ]) (which is the Javascript equivalent) when trying to Google this problem.

Unfortunately, it didn't work. :-(

@clayjohn Do you have any other ideas? So far this PR is the only thing that has worked for me to fix this issue.

[clayjohn]

Looking more closely, I can see that the only different in behaviour is in the cases where MODE_RENDER_DEPTH is defined.

The more I look into this, the more it doesn't make sense. Here is the WebGL 2.0 spec talking about the differences from GL ES 3.0:

5.4 Draw Buffers
The value of the MAX_COLOR_ATTACHMENTS parameter must be equal to that of the MAX_DRAW_BUFFERS parameter.

There is no use case for these parameters being different.
If an ESSL1 fragment shader writes to neither gl_FragColor nor gl_FragData, the values of the fragment colors following shader execution are untouched. If corresponding output variables are not defined in an ESSL3 fragment shader, the values of the fragment colors following shader execution are untouched.

All user-defined output variables default to zero if they are not written during a shader execution.

For optimal performance, an output array should not include any elements that are not accessed.

If the values written by the fragment shader do not match the format(s) of the corresponding color buffer(s) (for example, the output variable is an integer, but the corresponding color buffer has a floating-point format, or vice versa), and the color buffer states are not set to NONE by DrawBuffers, draws generate an INVALID_OPERATION error; if the color buffer states are set to NONE by DrawBuffers, they remain untouched.

If any draw buffer with an attachment does not have a defined fragment shader output, draws generate INVALID_OPERATION, unless all 4 channels of colorMask are set to false.

The last comment is particularly interesting here. It implies that we can write only depth to a framebuffer with a bound color attachment , but we need to have a proper colorMask.

In Godot we write 0 instead of false

godot/drivers/gles3/rasterizer_scene_gles3.cpp

Line 2560 in 7d2ca2d

glColorMask(0, 0, 0, 0);

Ultimately, I can't see anything that would cause this issue. So let's go ahead with this PR. It reintroduces the state we were in prior to #87386 with respect to defining frag_color. Plus, frag_color is never written and glDrawBuffers should ensure that the driver knows it won't be written.

[clayjohn]

Let's go ahead with this. We can remove the check for MODE_RENDER_DEPTH since we don't actually care about it if we are always using some color output

[dsnopek]

@clayjohn Thanks! I've made your suggested changes in my latest push :-)

[akien-mga]

Thanks!