Fix space transformations in WorldPositionFromDepth visual shader node generation

[Namey5]

WorldPositionFromDepth Visual Shader node was only normalizing xyz components after transforming from NDC -> view-space, then transforming view-space -> world-space using the still homogenous w component, giving incorrect results.

Fix would be to simply divide __depth_view.xyzw / __depth_view.w before view->world transformation, however at the recommendation of @tetrapod00 I have opted to align the node's generated code with the example given in Advanced post-processing.

Fixes #100345

[Flarkk]

Is z still supposed to be not reversed in low end ?
I thought reverse-z was introduced in all 3 renderers.

[Namey5]

I believe this isn't reverse-z, but rather the difference in how z is represented in NDC-space between OpenGL [-1, 1] and everything else [0, 1]
https://gamedev.stackexchange.com/questions/29018/why-does-opengl-require-all-coordinates-in-1-1-ndc

I'm not sure specifically what Godot does with depth in the OpenGL renderer (without a floating point depth buffer in [0,1] range reverse-z wouldn't really help, but it also wouldn't hurt and would keep things consistent) but these changes do appear to work in all 3 renderers.

[Flarkk]

It seems like is_low_end() returns false in all cases :

godot/servers/rendering/renderer_compositor.cpp

Lines 39 to 40 in ba2c5c1

	bool RendererCompositor::low_end = false;

I'm wondering if we shouldn't completely remove this branch.
Anybody please step in.

[Namey5]

It does appear to be overridden in the GLES3 backend:

godot/drivers/gles3/rasterizer_gles3.h

Line 124 in ba2c5c1

low_end = true;

[Flarkk]

Ah yeah good catch.
But isn't GLES supposed to have [0,1] Zs too ? Sorry if this brings confusion, but I'm confused myself now 😄

[Namey5]

Would it be worth adding a comment for this branch directly in generate_code() just to make it clearer? The same NDC branch is used in a couple of other nodes in this file.

[tetrapod00]

Do you mean a comment in the C++ code or a comment in the generated shader (that will be visible to end users)? Either way I'm not opposed, but I'm also not sure if that's idiomatic for these visual shader nodes. So let's also get a second opinion.

If you add a comment for it, I think you should only add it to this specific shader node, not the others, to keep the changes localized.

[Namey5]

Yeah just a comment in the C++ code, the branch wouldn't exist in the generated output. I think most of the confusion is the use of RenderingServer::is_low_end() - would be nice if there was an equivalent to the above mentioned CLIP_SPACE_FAR in C++ (or even better-er to use CLIP_SPACE_FAR directly in the generated output itself so people copying it get correct outputs regardless of renderer).

I'll leave it as-is for now though as it does match the behaviour in the existing nodes.

[Flarkk]

This will make it a bit clearer already #100384

[tetrapod00]

I think it's fine to defer the potential comments or other code changes for later.

[michaelharmonart]

any changes that need to be made here, or are we just waiting for review and merge at this point?

[tetrapod00]

This is waiting on a review from the shaders team, at least, and whatever other changes they will ask for, if any.

[maxpiepenbrink]

Just pulled, built & tested this branch with a simple distance field alpha glow effect and it worked as expected for Forward+, Mobile and Compatibility. Windows 10, GeForce 3090 32.0.15.6614

godot.windows.editor.x86_64_NYqUcyNxQQ.mp4

[paddy-exe]

Looks good. Not sure how that slipped through my PR tests when I added that node. Thanks for making the PR and fixing it!

[tetrapod00]

While I'm not a member of the shaders team, this looks good to me too, as far as my user's understanding of shaders goes. The new formula matches the one in the documentation, and the variable names are more accurate.

I also tested locally with multiple renderers and it works as expected.

[Repiteo]

Thanks!