Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix space transformations in WorldPositionFromDepth visual shader node generation #100350

Merged
merged 1 commit into from
Jan 22, 2025
Merged

Fix space transformations in WorldPositionFromDepth visual shader node generation #100350

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions scene/resources/visual_shader_nodes.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1747,12 +1747,13 @@ String VisualShaderNodeWorldPositionFromDepth::generate_code(Shader::Mode p_mode

code += " float __log_depth = textureLod(" + make_unique_id(p_type, p_id, "depth_tex") + ", " + uv + ", 0.0).x;\n";
if (!RenderingServer::get_singleton()->is_low_end()) {
code += " vec4 __depth_view = INV_PROJECTION_MATRIX * vec4(" + uv + " * 2.0 - 1.0, __log_depth, 1.0);\n";
code += " vec4 __ndc = vec4(" + uv + " * 2.0 - 1.0, __log_depth, 1.0);\n";
} else {
code += " vec4 __depth_view = INV_PROJECTION_MATRIX * vec4(vec3(" + uv + ", __log_depth) * 2.0 - 1.0, 1.0);\n";
code += " vec4 __ndc = vec4(vec3(" + uv + ", __log_depth) * 2.0 - 1.0, 1.0);\n";
}
Comment on lines 1751 to 1753
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@Namey5 Namey5 Dec 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@Flarkk Flarkk Dec 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does appear to be overridden in the GLES3 backend:

godot/drivers/gles3/rasterizer_gles3.h

Line 124 in ba2c5c1

low_end = true;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@tetrapod00 tetrapod00 Dec 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@Namey5 Namey5 Dec 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will make it a bit clearer already #100384

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

code += " __depth_view.xyz /= __depth_view.w;\n";
code += vformat(" %s = (INV_VIEW_MATRIX * __depth_view).xyz;\n", p_output_vars[0]);
code += " vec4 __position_world = INV_VIEW_MATRIX * INV_PROJECTION_MATRIX * __ndc;\n";
code += " __position_world.xyz /= __position_world.w;\n";
code += vformat(" %s = __position_world.xyz;\n", p_output_vars[0]);

code += " }\n";
return code;
Expand Down