Use Reverse Z for the depth buffer

[Khasehemwy]

Add support for reverse-z depth buffer resolve godotengine/godot-proposals#3539
@BastiaanOlij @clayjohn

These changes break shader compatibility!

I finished part of it.

Have finished:

1. operations related to Projection class (set_depth_correction() function).
2. most depth clear values in cpp files are inverted (e.g. 1.0 changed to 0.0) (canvas not use reversed-z).
3. most depth test function inverted (e.g. LESS changed to GREATER) (canvas not use reversed-z).
4. glsl for skybox.
5. light shadow.
   (directional shadow seems to be some problem, mentioned in the conversation below)
   (update: clayjohn fixed directional shadow problem)
6. FSR2 (mentioned in the conversation below)
7. SSR, SSAO, SSIL, Dof, SDFGI, VoxelGI, VolumetricFog (Thanks clayjohn for finishing them)
8. XR rendering (Thanks BastiaanOlij for finishing them)

Unfinished and known issues:

1. some problems with the xr part when using OpenGL API (mentioned in the conversation below)
2. SSAO is incorrect (most post processing effects will have problems)

[AThousandShips]

Some of these are exposed, and unless they are unchanged by this they break compatibility

Was discussed

[Calinou]

Tested locally, it mostly works as expected.

Testing project: https://github.com/godotengine/godot-proposals/files/10502399/test_zbuffer_precision.zip

Directional shadows for large objects break, even if the object is subdivided (each plane here is subdivided into 21×21 parts of equal size). I've also tried adjusting Pancake Size in DirectionalLight3D to no avail.

View videos in fullscreen to make the difference more noticeable. Focus on the area between the orange and white planes.

Forward+

Before

no_reverse_z_forward_plus.mp4

After (this PR)

Notice the phantom shadow appearing at the beginning of the video, which isn't visible in master.

reverse_z_forward_plus.mp4

Mobile

The shadow issues on the cyan spheres also appear in master without this PR.

reverse_z_mobile.mp4

Compatibility

This PR doesn't seem to affect precision in Compatibility so far.

no_reverse_z_compatibility.mp4

[BastiaanOlij]

This needs more testing by myself and clay and there are a few change we likely want. But it is important to know that there is consensus within the rendering team that the compatibility breakage is acceptable as this will effect really limited edge cases.
Anyone who does unprojection in shaders is not effected as the unprojection deals with this. Only when working with the z values as is could result in breakage. The benefits far outweigh the cons here.
We do have a few suggestions for limiting breakage, those will be forthcoming soon.

[BastiaanOlij]

@Khasehemwy one important change you might be able to do, we don't want the Z-flip to be part of the code for populating the the projection matrix, but only apply this when populating the GPU buffers. So we leave all the setters etc. alone.

The Projection class needs a single reverse_z function that applies the flip. This way we can keep using the projection matrix as is, we can keep the culling logic as is, we simply call the reverse_z early on in the render_scene logic, maybe as part of render_scene_data so only the rendering happens with the matrix with Z reversed.

That has far less impact than doing this early on.

[Khasehemwy]

@Khasehemwy one important change you might be able to do, we don't want the Z-flip to be part of the code for populating the the projection matrix, but only apply this when populating the GPU buffers. So we leave all the setters etc. alone.

The Projection class needs a single reverse_z function that applies the flip. This way we can keep using the projection matrix as is, we can keep the culling logic as is, we simply call the reverse_z early on in the render_scene logic, maybe as part of render_scene_data so only the rendering happens with the matrix with Z reversed.

That has far less impact than doing this early on.

I changed the projection matrix back to normal z, then added reverse z-related operations, and added reverse-related operations where I thought they needed to be added.

In addition, I found that there is a problem with the Dual Paraboloid shadow mode of Omni Light in compatibility mode. It seems that the official version also has problems. It may not be caused by my changes.

[Khasehemwy]

The case of far<near is not considered (if far<near, columns[2][2] will be <0 when reversed-z is used). I don't think we need to consider this situation.

[BastiaanOlij]

I don't think so either, I think we protect that on Camera3D anyway but if not, it's an acceptable assumption.

[roalyr]

Will this not be an issue (asking just in case): roalyr#15 ?

[dsnopek]

@BastiaanOlij Thanks so much!

I've retested the latest version of this PR (which includes Bastiaan's changes) using both OpenGL and the Vulkan mobile renderer on a Meta Quest Pro, and everything (including sky rendering) seems to be working correctly now. :-)

[Khasehemwy]

Looks like this PR is in good shape now. Is there anything else I can do to assist with the merge?

[clayjohn]

@Khasehemwy I think it looks good now! We have all been away for the last couple of weeks due to GDC and vacations.

The final step before merging is to squash all the commits together so that the whole PR only contains 1 big commit with all your changes. We like to merge one commit at a time to keep the git history clean and navigable.

If you don't know how to do that, we have a helpful tutorial in the official documentation https://docs.godotengine.org/en/latest/community/contributing/pr_workflow.html#the-interactive-rebase

[clayjohn]

Looks good to me now (pending squash and rebase). I have tested extensively on desktop devices and I trust the testing of Bastiaan and David on VR devices.

Once this is squashed, we can go ahead and merge it for 4.3

[lyuma]

Sorry if this is an obvious question but this is a pretty big breaking change (as noted in the OP) that will impact user shaders.
How can I as a user modify my shaders to be compatible with both the 4.2 and 4.3 rendering design?

I am surprised that a change like this is better in all cases and would have no downsides. If this is true, do all other major engines also use this approach on dx/vulkan? (Should this perhaps be a project setting?)

I saw XR mentioned. Are there any interoperability concerns related to submitting reversed depth buffer to OpenXR?

[Ansraer]

Sorry if this is an obvious question but this is a pretty big breaking change (as noted in the OP) that will impact user shaders.
How can I as a user modify my shaders to be compatible with both the 4.2 and 4.3 rendering design?

Most of your shaders should be completely fine, this only has an impact on you if you either manually write to depth or read it. That's why we really want to get this merged before the compositor PR makes writing custom post processing shaders much easier.

Should you run into any problems all you need to know is that the depth buffer now goes from 1.0-0.0 instead of 0.0-1.0. This means that you might have to invert any compare ops you do and maybe switch additions and subtraction operations.

I am surprised that a change like this is better in all cases and would have no downsides. If this is true, do all other major engines also use this approach on dx/vulkan? (Should this perhaps be a project setting?)

Yes, this is the approach that is used by any recent-ish engine. It has major advantages when it comes to precision and accuracy with 0 downsides, so there is no point in adding a project setting to turn it off (as a matter of fact that would actually be counterproductive, since custom post-processing assets could break depending on the state of that project setting).
The only reason why this wasn't a thing in godot already is because this optimization has no visible advantage in opengl, and people forgot to add it when the engine moved to vulkan.

I saw XR mentioned. Are there any interoperability concerns related to submitting reversed depth buffer to OpenXR?

Not that I am aware of. As said before, rev depth is the current state of the art and recommended by every hardware vendor out there. I have little experience with developing for VR, but I would be seriously surprised if rev depth was a problem.

[darksylinc]

I am surprised that a change like this is better in all cases and would have no downsides.

Reverse Z has the following conditions:

Much Better in all scenarios if:

1. Depth Buffer is set to D32_FLOAT. Only very old GPUs (more than 15+ years old) do not support it.
   • However on Linux + X11 + OpenGL (i.e. Compatibility), Mesa does not support D32_FLOAT for the render window. Fortunately Godot does not render directly to the render window so this not an issue but it can be for other engines.
2. In OpenGL (i.e. compatibility) glClipControl is supported and is set to range [0; 1]. This is not a given for OpenGL ES (i.e. mobile).

Slightly worse if:

1. Depth Buffer is set to D24_UNORM (it should not be an issue in Godot) or
2. glClipControl is not supported (i.e. Compatibility in Mobile).

If this is true, do all other major engines also use this approach on dx/vulkan? (Should this perhaps be a project setting?)

Other engines chose to either move to reverse Z as well, or allow for optionally support both.

Supporting both options requires a lot of testing and maintenance which is why the Godot team chose not to support the old regular Z at all. Often engines that support both options end up having bugs when using regular Z because very few people actually use that code path (due to the massive superiority of reverse Z).

How can I as a user modify my shaders to be compatible with both the 4.2 and 4.3 rendering design?

Godot devs will have to answer you this one. You do make a point that perhaps Godot 4.3 should define "something" to signal user shaders that reverse Z is being used; in case a user wants to support both versions (<= 4.2 + >= 4.3).

[lyuma]

Ok your answers all make sense. I am pretty sold.

the main thing then is having some docs on porting shaders. For example one shader here is hardcoded in the godot engine source and probably needs to be edited:
https://github.com/godotengine/godot/blob/master/editor/plugins/skeleton_3d_editor_plugin.cpp#L919..L920

In terms of user shaders, godot vrm does something with the projection space z coordinate for outlines:
https://github.com/V-Sekai/godot-vrm/blob/d88d2cc5a629d21a33196ef33a71f4bcb2736916/addons/Godot-MToon-Shader/mtoon_common.gdshaderinc#L103
People download this code and expect it to work in both 4.2 and 4.3 so I might need to come up with some heuristic to check for the inverted z buffer... I assume it should be possible to determine from the sign of some element of the projection matrix?

[BastiaanOlij]

I have little experience with developing for VR, but I would be seriously surprised if rev depth was a problem.

I did a little research on this and sadly, non of the APIs in OpenXR that allow us to submit a depth buffer, provide the ability to tell the XR runtime we're storing reverse-Z, meaning they will be incorrectly interpreted.

Right now we only use the composition layer depth extension, and that is generally turned off. This discussion actually made me find a bug in that logic that might explain some issues we've had in the past.

Metas Application Spacewarp will also be effected by this.

That said, seeing many game engines are switching over to this, it's something that I have raised within the working group, hopefully that will lead to discussions to eventually add support for this.

[Khasehemwy]

@clayjohn Awesome! Thank you all! All the submissions have been squashed now.

[Khasehemwy]

I assume it should be possible to determine from the sign of some element of the projection matrix?

@lyuma Yes, you can determine whether it's reversed-z in the shader by checking the value of the PROJECTION_MATRIX. When reversed-z is applied, columns[2][2] > 0 ( columns[2][2] = (p_z_far + p_z_near) / (p_z_far - p_z_near) ). The same principle applies to orthogonal as well ( columns[2][2] > 0 ( columns[2][2] = 2.0 / (p_zfar - p_znear); ) ). Of course, we should assume that p_z_far > p_z_near by default.

[clayjohn]

Ok your answers all make sense. I am pretty sold.

the main thing then is having some docs on porting shaders. For example one shader here is hardcoded in the godot engine source and probably needs to be edited: https://github.com/godotengine/godot/blob/master/editor/plugins/skeleton_3d_editor_plugin.cpp#L919..L920

In terms of user shaders, godot vrm does something with the projection space z coordinate for outlines: https://github.com/V-Sekai/godot-vrm/blob/d88d2cc5a629d21a33196ef33a71f4bcb2736916/addons/Godot-MToon-Shader/mtoon_common.gdshaderinc#L103 People download this code and expect it to work in both 4.2 and 4.3 so I might need to come up with some heuristic to check for the inverted z buffer... I assume it should be possible to determine from the sign of some element of the projection matrix?

@lyuma Neither of the shaders you linked will need to be modified. The only user shaders that will need to be modified are those that adjust DEPTH or read from the depth_texture without using PROJECTION_MATRIX or INV_PROJECTION_MATRIX. In practice, it almost never makes sense to manipulate depth in clip space, so I don't think we will run into this issue often.

In other words:

// Compatibility is broken (need to flip the sign)
float depth = texture(depth_tex, SCREEN_UV).r;
vec4 clip = PROJECTION_MATRIX * vec4(VERTEX, 1.0);
clip.xyz /= clip.w;
if (depth < clip.z) {
    ...
}

// This is fine
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) {
    ...
}

[akien-mga]

I amended the commit message to clean the description which still included past fixup commit titles, and retitled it to be clearer, especially regarding the fact that it's changing the convention, not adding a new option.

[akien-mga]

Thanks! And congrats for your first – but massive! – merged Godot contribution 🎉

[clayjohn]

Adding a note for the dev blog post:

This dev release switches our depth buffer to using a "reverse z" projection. This means that instead of the depth going from 0 (close to viewer) to 1 (far from view), it goes from 1 to 0. This is a enhancement found in most modern engines and it significantly improves depth precision by taking advantage of how floating point numbers are stored. This is a breaking change, however, we think the compatibility breakage is minimal and well worth the benefits.

In particular, any operation that uses clip space values directly from the depth buffer are now reversed. That includes uses of the depth buffer in custom render hook callbacks and uses of the depth_texture hint in shaders. That being said, we are confident that most users are reading depth to reconstruct view space, or world space depth which still works the same as before. 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) {
    ...
}

As long as you are using the INV_PROJECTION_MATRIX to convert the depth into view space, no modifications are necessary. If you are working in clip space directly, you will need to reverse the sign of depth comparisons manually. For example take the following code:

float depth = texture(depth_tex, SCREEN_UV).r;
vec4 clip = PROJECTION_MATRIX * vec4(VERTEX, 1.0);
clip.xyz /= clip.w;
if (depth < clip.z) {
    ...
}

To maintain compatibility, you will have to change the comparison from depth < clip.z to depth > clip.z. The same is true for any code that is directly working with a depth value, or a clip space value returned by using PROJECTION_MATRIX.

Overall, we regret to break compatibility in this way, but we felt strongly that the tradeoff was worthwhile and the alternative was to wait until Godot 5 which will be many years away.