Allow depth-writing shaders to work with shadow maps

[viktor-ferenczi]

Solution to Proposal-1942

• Allowed objects with a DEPTH setting fragment shader to receive shadows and cast shadow on themselves.
• The change allows fragment shaders to modify the VERTEX variable, so they can be update it consistently to DEPTH change.
• There is still a FIXME comment in the code change. I need a hint there how to add a shader compilation time condition.

Tested only on Windows with the Vulkan renderer. Please see the test results in my comment on the proposal above.

[clayjohn]

You can add the conditional by adding a usage_define to the shader, here is how it is added for NORMAL in the clustered renderer:

godot/servers/rendering/renderer_rd/forward_clustered/scene_shader_forward_clustered.cpp

Line 717 in b6d102c

actions.usage_defines["NORMAL"] = "#define NORMAL_USED\n";

You would just have to add a similar line for the Vulkan Mobile renderer and the GLES3 renderer as well.

[clayjohn]

I wonder if we could just handle this automatically. For example after the user fragment shader code (in pseduocode):

#if FRAG_DEPTH_USED
vertex.z = in_view_space(-gl_FragDepth);
 #ifdef USE_MULTIVIEW
	view = -normalize(vertex - scene_data.eye_offset[ViewIndex].xyz);
#else
	view = -normalize(vertex);
#endif
#endif

I am worried that if people are able to write to VERTEX they will expect that they can write to it to modify the actual location of the pixel, when instead they are just modifying the position for the purpose of lighting/shadow code.

Additionally, on mobile devices it is very important for shadow code to be able to read directly from input variables rather than local variables as it enables a lot of optimizations. So I want to discourage writing to VERTEX as much as possible.

[viktor-ferenczi]

@clayjohn

vertex.z = in_view_space(-gl_FragDepth);

It would indeed solve the problem in a compatible way. But performance and precision may not be optimal. Such a fragment shader would likely have the vertex.z already available, so calculating it again from gl_FragDepth is not optimal.

Relevant part of such a user fragment shader:

vec3 vertex;
// Comes up with view space vertex position based on the carved out geometry (changes only the depth)
vec4 ndc = PROJECTION_MATRIX * vec4(vertex, 1.0);
DEPTH = ndc.z / ndc.w;

We could introduce a new out float LINEAR_DEPTH variable to allow overriding the Z component of the vertex and also to offload the projected (nonlinear) DEPTH calculation to Godot's shader code. Then the user fragment shader could be simplified to just setting LINEAR_DEPTH and not having to deal with DEPTH at all.

• If the user fragment shader sets DEPTH, we override the vertex.z in Godot's shader (your suggestion) for compatibility.
• If the user fragment shader sets LINEAR_DEPTH, then vertex.z is overridden and DEPTH is calculated by Godot's shader.
• If both are set, then LINEAR_DEPTH takes precedence.

We would encourage the use of LINEAR_DEPTH for better performance and simpler user fragment shader code whenever the updated vertex Z coordinate is available.

It would also reduce any confusion about how to set DEPTH, since the use of PROJECTION_MATRIX is non-trivial for new shader developers.

The value is LINEAR_DEPTH is usually negative, since it is just a replacement Z component of the VERTEX vector which is in the view space. I could have added a few negations to make it positive (as a depth), but that would just complicate the arithmetic involved. Whoever develops such advanced depth writing shaders should easily accommodate to the negative value anyway.

Any problems with this approach?
Any possible performance issues on mobile?

[viktor-ferenczi]

@clayjohn Implement your suggestion:

In scene_shader_forward_clustered.cpp defined:

actions.usage_defines["DEPTH"] = "#define DEPTH_USED\n";

In scene_forward_clustered.glsl at the beginning of LIGHTING section:

#ifdef DEPTH_USED
	vec3 ndc = vec3(screen_uv * 2.0 - 1.0, gl_FragDepth);
	vec4 view_pos = inv_projection_matrix * vec4(ndc, 1.0);
	vertex.z = view_pos.z / view_pos.w;
#ifdef USE_MULTIVIEW
	view = -normalize(vertex - scene_data.eye_offset[ViewIndex].xyz);
#else
	view = -normalize(vertex);
#endif
#endif

The depth calculation is based on the advanced post-processing documentation.

Tested with the same good result, but without having to set VERTEX from the user fragment shader.

Also reverted the VERTEX variable to be read-only.

As you can see it takes a lot of work to calculate the vertex.z from the depth. Better performance could be achieved if the user fragment shader already have the Z component available.

[viktor-ferenczi]

Now the PR has both a compatible shadow fix (requires writing the DEPTH only) and LINEAR_DEPTH as an alternative (see my comment above).

It has not been ported to the mobile Vulkan renderer and the GLES3 driver yet. I will do the porting and the documentation update once we agree about this solution in general.

Thank you for reviewing it.

Preliminary fragment shader documentation update:

+----------------------------------------+--------------------------------------------------------------------------------------------------+
| out float **DEPTH**                    | Custom depth value (0..1). Consider setting LINEAR_DEPTH instead if that is easier to produce.   |
+----------------------------------------+--------------------------------------------------------------------------------------------------+
| out float **LINEAR_DEPTH**             | Custom linear depth value. Replaces the Z coordinate in view space for lighting calculations.    |
|                                        | Sets DEPTH as well by projecting the updated surface coordinate. Usually has a negative value.   |
+----------------------------------------+--------------------------------------------------------------------------------------------------+

[viktor-ferenczi]

Do you agree with the above solution? (Compatible fix if only DEPTH is set. Providing LINEAR_DEPTH as an alternative.)

If you do agree, then I would go ahead and complete the PR by porting the change to GLES3 and mobile Vulkan.

[clayjohn]

I'm not sure I agree, exposing both DEPTH and LINEAR_DEPTH seems like it will be confusing to users (especially since both can be set, but then DEPTH will silently be ignored.

Ultimately, the only benefit of using LINEAR_DEPTH is saving a few instructions in the case where the user manipulates VERTEX in view space. In my opinion its not worth adding a new built-in just to save a single matrix multiplication.

Also, there is no point in doing a full matrix multiplication and then immediately throwing out the xy and w components. It is likely more efficient to only do the calculations necessary to calculate the z-channel. That way you can reduce the overhead to as few instructions as possible and maintain a simple API for users.

github's ui sucks

[lyuma]

I'm a bit unsure if clayjohn's approach might cause floating point precision errors. I'd only be worried about z-fighting between two shaders which had different view matrices but tried to write the same depth value.

But I'd rather try @clayjohn's approach of only using DEPTH, until a need for the more complicated LINEAR_DEPTH or other system is deemed necessary. Godot strives to keep systems as simple as possible until a need for a more complex system is demonstrated. A performance bottleneck has not been demonstrated here. Depth-writing shader performance in my experience will be dominated by overdraw and loss of early depth test optimization.

(If we're going to bring up performance, the first thing to implement would be this: https://registry.khronos.org/OpenGL/extensions/ARB/ARB_conservative_depth.txt with a render_mode depth_less or DEPTH_LESS = variable - but honestly IMHO we have bigger fish to fry in 4.0, so I wouldn't really push for conservative depth until a later 4 version once people have had time to try things enough to demonstrate a bottleneck.)

[paddy-exe]

I'm not sure I agree, exposing both DEPTH and LINEAR_DEPTH seems like it will be confusing to users (especially since both can be set, but then DEPTH will silently be ignored.

Ultimately, the only benefit of using LINEAR_DEPTH is saving a few instructions in the case where the user manipulates VERTEX in view space. In my opinion its not worth adding a new built-in just to save a single matrix multiplication.

Also, there is no point in doing a full matrix multiplication and then immediately throwing out the xy and w components. It is likely more efficient to only do the calculations necessary to calculate the z-channel. That way you can reduce the overhead to as few instructions as possible and maintain a simple API for users.

I have to disagree here. My last VisualShader PR added the Linear Depth node anyway. (I would have added it in my other PR before as well in as a built-in if I knew this workaround.) So currently we have Depth Texture and Linear Depth in Visual Shaders. I do not see why having both DEPTH and Linear Depth can't be there (as long as both are useful). I asked myself before already why this wasn't simplified since I have never encountered a usecase for using a different depth value scale (except for internal uses perhaps).

[viktor-ferenczi]

Do you agree that not receiving shadow (correctly) in case of depth writing shaders is actually a bug?

I think fixing this at least for the DEPTH case should go into Godot 4.0.

That would not need the LINEAR_DEPTH, although I would really like to have it for my case to avoid the extra calculations.

As a compromise, I suggest restricting this PR to fix the shadow with no change to the shader variables.

ARB_conservative_depth

Later we can have another PR to rework the depth as mentioned by @lyuma above. That could include a linear depth option.

Also, there is no point in doing a full matrix multiplication and then immediately throwing out the xy and w components.

The compiler's optimizer is expected to take care of such cases, so we can have more readable code closer to the math involved. Sorry if I have been naïve in this regard. I need to read-up on how good GLSL optimizers are these days.

[clayjohn]

Do you agree that not receiving shadow (correctly) in case of depth writing shaders is actually a bug?
I think fixing this at least for the DEPTH case should go into Godot 4.0.

Yes, I do agree this is a bug and should be fixed for the DEPTH case for 4.0 without a change to the API.

After that, we can certainly discuss adding LINEAR_DEPTH in a proposal. With things like this it is always a tradeoff between complexity/usability/performance. So having input from more users can definitely sway the decision one way or the other. My gut tells me that the tradeoff is not worth it, but I am happy to hear from other users and contributors who would benefit from the change.

[Calinou]

We discussed this in today's PR review meeting, we did not reach a consensus on what to do. Reduz feels like this addition is getting to be too much of a corner case that would be better solved by exposing the ability for users to write a fully custom base shader rather than making an addition to the core shader.

On the other hand, it should be kept in mind that this PR allows improving the quality of some built-in effects such as parallax mapping. This will have a performance cost, so it needs to be disableable in the project settings though (or on a per-material basis with a render mode). The feature could also be disabled by default if we're worried about lowering performance out of the box. I'd recommend running benchmarks to compare the before/after performance with this PR.

I suppose SDF rendering shaders will also benefit a lot from this change. The +116/-33 diff from this pull request doesn't seem too large compared to the number of use cases this opens (and the possible quality increase for parallax mapping).

[lyuma]

@viktor-ferenczi I agree with your on your second and third points. The compile times from editing the base shader GLSL are horrendous, the error reporting is broken on windows (I usually have to hack the BUFFER_SIZE constant in platform/windows/windows_terminal_logger.cpp to see the generated code so I can track down the error), and very unpleasant in general. I assume other core devs have a method to this madness but I've never been able to figure out how to stay sane while editing scene*.glsl.

Changes to the shader system are planned in the 4.x timeline, and some basic improvements may make it in at the end of the 4.0 beta cycle, but serious refactoring of the type you ask will not be in 4.0

With that out of the way, regarding your first point, you are making the claim that this would not work if "this PR would be stripped down". I respectfully disagree.

#define geometric_normal normal does not match the old behavior. This is a substantial change to the old behavior, and it clearly has no reason to change if not DEPTH_USED. Furthermore there is a lot of code added, which creates a maintainability burden.

I believe there is a way to structure this change, so that it can resolve your usecase when #ifdef DEPTH_USED without adding too much duplicate code (tangent = normalize(tangent); is repeated at least two times, hence duplicate code), and without changing the performance characteristics when not DEPTH_USED.

I think making a serious effort to try and make this review concise and not affect performance when not DEPTH_USED would go a long way towards making this change more palatable... and if you are unable to do so, knowing what you attempted and what went wrong would be useful justification for those changes being as they are.

[viktor-ferenczi]

Released the first version of the Voxel plugin today, it works only on Godot 4.

Voxel plugin on the AssetLib

Certainly the received shadows and lighting calculations are wrong without this PR being merged. Users are advised by the documentation to make a custom build of Godot to include the changes from the PR branch, but this is far from ideal.

Could you please reconsider this PR or an equivalent solution for the 4.1 release?

Since voxel rendering is resource intensive, most likely it will only be used on desktop platforms in the short term. Maybe applying this fix only to the Forward+ desktop renderer would work as a temporary solution, accepting that the lighting would be incorrect on mobile if the official Godot build is used.

[viktor-ferenczi]

Any chance getting this PR reconsidered? (Sorry for the bump.)

[clayjohn]

We really need to discuss this once we start PR review meetings again. Previously we were unable to reach consensus on whether this was better to include as-is, or if the engine should provide better tools to write custom shaders so that the built in shader doesn't need to support this use-case. Since there was no consensus we can't really move forward until we have a chance to re-discuss it

[viktor-ferenczi]

Has this been discussed on a PR review meeting?

The Voxel plugin is a good use case and test case for this issue.

I will rebase the changes to resolve the conflicts.

[Scoppio]

Is there still interest on this PR? the changes here provide non-trivial improvements on final render and also allow for alot of flexibility on implementations of new shaders.

[jitspoe]

Is there a reason to do this vs. just allowing VERTEX to be writable in the fragment shader? I needed that for doing per texel lighting. That can be done by simply changing shader_modes[RS::SHADER_SPATIAL].functions["fragment"].built_ins["VERTEX"] = constt(ShaderLanguage::TYPE_VEC3); to shader_modes[RS::SHADER_SPATIAL].functions["fragment"].built_ins["VERTEX"] = ShaderLanguage::TYPE_VEC3; in shader_types.cpp

[celyk]

If the user knows how to set DEPTH correcty, surely they can handle VERTEX, right? Let's not forget that DEPTH is actually gl_FragDepth being exposed to the user, and you SHOULD assume that they know how it works. When the user sets POSITION (gl_Position), you don't automatically adjust VERTEX just so the lighting looks right... Fact is, those variables have nothing to do with lighting!

Allow setting of VERTEX and I'll be happy. :)

[viktor-ferenczi]

There is interest, but the core devs don't seem to care anymore. I gave up TBH, but it would still be nice to see my Voxel plugin working properly with the default Godot build.

[Lunatix89]

I really hope that this gets approved someday as I currently port my voxel based game, which uses raymarching, from Unity to Godot and need this to get shadows and lighting to work correctly.
In the meantime I will use a fork with some customizations but it's brilliant to just be able to write to DEPTH and have the core shader take care of using the correct position so there is actually no need to do it on my own like I did in my version of this (see PR #84213).

@viktor-ferenczi really nice work you did there :)

[viktor-ferenczi]

@viktor-ferenczi really nice work you did there :)

Thank you.

You may find some reusable shader code here for voxel rendering:
https://github.com/viktor-ferenczi/godot-voxel

I got the Magica Voxel import and rendering working during the Godot 4 Beta as a GDExtension. But then the extension logic changed and this PR was not approved on a timely manner, so I ended up not maintaining and publishing that code, unfortunately. If anybody wants it being finished please reach out directly. Thanks.

[lyuma]

My concerns from one year ago have not been addressed: This PR is changing the functioning of shadow biases even in the non-DEPTH_USED case, and it's not yet clear to me why that must be.

I can only think of two possibilities for this:

1. If the current usage of normal_interp is wrong, that would imply a bug in Godot that is unrelated to the DEPTH variable. If so, that should be solved as its own isolated PR with a detailed testcase and explanation why this should be changed. In this case, it would be a bugfix (which implies an issue should be filed with a minimal reproduction project which shows the issue when DEPTH is not used) rather than a feature.

2. However, If the current usage of normal_interp is correct, it stands to reason that this change shall only affect the DEPTH_USED case. If this is done, it should be easier to justify the correctness. In particular, changing the NORMAL value in a conventional non-depth writing shader should not affect shadow bias, but with the PR as written, it seems like it could be wrong.
   In particular, not all usages of normal maps use the fixed-function NORMALMAP variable, so special-casing the NORMALMAP math seems incorrect.

Finally, if this change is better justified or simplified / put behind preprocessor flags, it will still need to go through a review meeting as clayjohn noted.

[Firepal]

Ughhh, this is such bullshit.
There is a clear bug and still no sign of the "custom base shader"-editing. After over a year!

What is it in your PR meetings that made you say "adding this functionality long before the scene shader editing is a bad idea"?!

[AThousandShips]

@Firepal please keep your feedback constructive, consider the code of conduct

The concerns raised haven't been resolved, as far as I can tell, so there's no merging it until that's done, and OP haven't responded or done any changes

[akien-mga]

Superseded by #91136. Thanks for the contribution!