Screenspace quad shaders are broken

[ultsi]

Tested versions

Reproducible in v4.3.dev.custom_build [a7b860250]

Not reproducible in v4.3.dev.custom_build [02488108a] Not reproducible in v4.3.dev5

System information

Godot v4.3.dev (d0c3dfe91) - Ubuntu 20.04.6 LTS (Focal Fossa) - X11 - Vulkan (Forward+) - dedicated AMD Radeon RX 6600 (RADV NAVI23) () - AMD Ryzen 7 5800X 8-Core Processor (16 Threads)

Issue description

Working view on 02488108a (it's my custom commit, it's somewhere after dev5). I lost the original working branch, just have the binary left :shrug:

Broken view on master a7b860250 :

Steps to reproduce

I uploaded a test project with a simple screenspace quad depth shader. In master it only renders on skybox, in dev5 and somewhere after it it correctly renders on top of the objects.

Minimal reproduction project (MRP)

depth-test.zip

[akien-mga]

That's an expected compat breakage from switching to Reverse Z for depth, see https://github.com/godotengine/godot/pull/88328#issuecomment-2040568888.

[ultsi]

Interesting!

I read the note for dev blog post and I'm baffled:

For example the below is a very common pattern that will continue to work without modification:

float depth = texture(depth_tex, SCREEN_UV).r;
vec3 ndc = vec3(SCREEN_UV * 2.0 - 1.0, depth);
vec4 view = INV_PROJECTION_MATRIX * vec4(ndc, 1.0);
view.xyz /= view.w;
float linear_depth = -view.z;
if (view < VERTEX.z) {
...
}

I'm using exactly the same code, except if clause switched to ALBEDO = vec3(linear_depth / 100.0);. I would imagine that I would get black/white colors on the objects, but instead I don't get anything and only the skybox changes if I toggle the shader on and off.

I think it's not about reverse Z for depth, it's about the quad mesh rendering behind the objects or something like that.

Please try the MRP to see the issue.

[ultsi]

Simplified the test shader code to just

void fragment() {
    float depth = textureLod(DEPTH_TEXTURE, SCREEN_UV, 0.0).r;
    ALBEDO = vec3(depth);
}

I would imagine a black or white screen, instead only the skybox is affected like in the screenshot.

[ultsi]

FWIW I backtracked 2 commits and checked if they work:

Doesn't work: v4.3.dev.custom_build [c928273c6] Works: v4.3.dev.custom_build [282d772f1]

[clayjohn]

It indeed looks like a regression from the Reverse-Z PR. But I'm 99% certain the problem is the vertex shader since its clear that the mesh isn't showing up at all.

edit: Yep, the problem is that the quad shader trick relies on treating the quad coordinates as clip space coordinates, so the z needs to be reversed. I.e. POSITION = vec4(VERTEX.xy, 1.0-VERTEX.z, 1.0); works

We will have to discuss this further with the rendering contributors. I forgot that POSITION allows users to write vertex positions directly in clip space. We knew there would be some compatibility breakage from the reverse-z change, but we figured it would be limited as there are few places where working in clip space is useful in user shaders. However, this shows that there is going to be more widespread breakage than we thought.

One option is to automatically reverse the z-value for users. We could then document that POSITION is in a modified clip space where 0 is the near plane and 1 is the far plane. That would avoid compatibility breakage, but it would be pretty annoying in the long run

[ultsi]

Thanks, this workaround indeed works for now. :v:

[clayjohn]

We discussed this at the rendering meeting tonight. Unfortunately we realized there is no good solution here. We can't simply reverse the z or the user as the user might be using proper z. For example, the following code currently works in both 4.2 and 4.3:

POSITION = PROJECTION_MATRIX * VIEW_MATRIX * vec4(VERTEX, 1.0);

If we reverse the z behind the scenes, then this perfectly valid code will break instead.

Accordingly, we agreed to do the following:

1. Update the documentation ASAP (done: https://github.com/godotengine/godot-docs/pull/9218)
2. Write a blog post highlighting the reverse z feature and explaining how to fix broken shaders (Done: https://github.com/godotengine/godot-website/pull/834)
3. Add a warning in the editor if a user writes POSITION = vec4(VERTEX, 1.0); (this will miss a lot of other broken cases, but it will catch the majority) (done: https://github.com/godotengine/godot/pull/90587)

[Khasehemwy]

I encountered the same issue while modifying the sky shader (where POSITION is directly written without using PROJECTION_MATRIX). I think this issue should indeed be handled by developers themselves. If POSITION is being written, changes should be made according to specific circumstances. It would be helpful to include a prompt when using POSITION (more than just POSITION = vec4(VERTEX, 1.0);).

The key point is, if the vertex coordinate are not using the PROJECTION_MATRIX, we (user) need to pay special attention to it.

[clayjohn]

The key point is, if the vertex coordinate are not using the PROJECTION_MATRIX, we need to pay special attention to it.

Exactly. The trouble is, we have no way of reliably knowing if any particular usage is correct or not.

For example POSITION = vec4(clip_space_pos, 1.0); may be totally fine if clip_space_pos is the result of transforming a view space position into a clip space position with PROJECTION_MATRIX. But it may also be totally wrong if it's something that has been hand tweaked.

We discussed adding a temporary warning when using any of POSITION, DEPTH or hint_depth_texture, but we decided that such a warning would be way too annoying for users that are doing things correctly.

[Khasehemwy]

so the z needs to be reversed. I.e. POSITION = vec4(VERTEX.xy, 1.0-VERTEX.z, 1.0); works

I've thought of another situation that users need to handle themselves:

In Vulkan and DirectX, reversed_z = 1.0 - normal_z, but in OpenGL, reversed_z = -normal_z. This issue also arises in sky shaders. The sky appears in the backmost. When using normal-z, both OpenGL and Vulkan can use Position = vec4(uv_interp, 1.0, 1.0); for sky. However, when using reversed-z, OpenGL uses Position = vec4(uv_interp, -1.0, 1.0);, while Vulkan uses Position = vec4(uv_interp, 0.0, 1.0);.

So we need a method in the shader to determine if the z-value of the NDC Space coordinates is in [-1,1] or [0,1], or to determine if we are currently using OpenGL or another API. For example, adding a macro definition? I'm not sure if such a macro exists now.

[clayjohn]

so the z needs to be reversed. I.e. POSITION = vec4(VERTEX.xy, 1.0-VERTEX.z, 1.0); works

I've thought of another situation that users need to handle themselves:

In Vulkan and DirectX, reversed_z = 1.0 - normal_z, but in OpenGL, reversed_z = -normal_z. This issue also arises in sky shaders. The sky appears in the backmost. When using normal-z, both OpenGL and Vulkan can use Position = vec4(uv_interp, 1.0, 1.0); for sky. However, when using reversed-z, OpenGL uses Position = vec4(uv_interp, -1.0, 1.0);, while Vulkan uses Position = vec4(uv_interp, 0.0, 1.0);.

So we need a method in the shader to determine if the z-value of the NDC Space coordinates is in [-1,1] or [0,1], or to determine if we are currently using OpenGL or another API. For example, adding a macro definition? I'm not sure if such a macro exists now.

We don't have such a macro. But remember, in sky shaders, users don't have access to the vertex shader or to the clip space position. Sky shaders are only fragment shaders as the vertex projection is totally handled for users.

[Khasehemwy]

No,no,no, I actually refer to sky.glsl in the source code. For users, If they want to use a spatial shader to write something that is always rendered at the backmost, they will encounter the same problem. At this time they need a macro to handle both situations.

[clayjohn]

No,no,no, I actually refer to sky.glsl in the source code. For users, If they want to use a spatial shader to write something that is always rendered at the backmost, they will encounter the same problem. At this time they need a macro to handle both situations.

I think I understand you now. We do have a macro to tell when the user is using the compatibility renderer, but it's not obvious. OUTPUT_IS_SRGB is true in the compatibility renderer and false otherwise. we could add a new macro CLIP_SPACE_FAR which is 0 for the Vulkan backend and -1 for OpenGL.

[ttencate]

Wouldn't you typically want to use render_mode depth_test_disabled on a screen-space quad anyway? Then the quad's z coordinate doesn't matter.

[clayjohn]

Wouldn't you typically want to use render_mode depth_test_disabled on a screen-space quad anyway? Then the quad's z coordinate doesn't matter.

That would certainly help in the future. The problem we are grappling with now is the number of existing shaders that are impacted.