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.

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 proximity fade in Compatibility renderer #100220

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tetrapod00
Copy link
Contributor

Fixes #89942.
Supersedes #89966.

Uses the correct NDC for the Compatibility renderer.

Uses the preprocessor defines for the current renderer introduced in 4.4 in #98549. We also document a nearly identical reconstruction of world space using NDC in this tutorial, which now uses the preprocessor defines to be renderer-independent.

@@ -1716,7 +1716,11 @@ void fragment() {)";
code += R"(
// Proximity Fade: Enabled
float proximity_depth_tex = textureLod(depth_texture, SCREEN_UV, 0.0).r;
#if CURRENT_RENDERER == RENDERER_COMPATIBILITY
vec4 proximity_view_pos = INV_PROJECTION_MATRIX * vec4(SCREEN_UV * 2.0 - 1.0, proximity_depth_tex * 2.0 - 1.0, 1.0);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alternately:

Suggested change
vec4 proximity_view_pos = INV_PROJECTION_MATRIX * vec4(SCREEN_UV * 2.0 - 1.0, proximity_depth_tex * 2.0 - 1.0, 1.0);
vec4 proximity_view_pos = INV_PROJECTION_MATRIX * vec4(vec3(SCREEN_UV, proximity_depth_tex) * 2.0 - 1.0, 1.0);

We could also do something more like this, leaving the "default" case of Forward+/Mobile implicit:

float proximity_depth_tex = textureLod(depth_texture, SCREEN_UV, 0.0).r;
#if CURRENT_RENDERER == RENDERER_COMPATIBILITY
proximity_depth_tex = proximity_depth_tex * 2.0 - 1.0;
#endif
vec4 proximity_view_pos = INV_PROJECTION_MATRIX * vec4(SCREEN_UV * 2.0 - 1.0, proximity_depth_tex, 1.0);

But I think the current form is the most explicit and clear about exactly what changes between the renderers.

@tetrapod00
Copy link
Contributor Author

tetrapod00 commented Dec 10, 2024

Looks like this runs into the same problem as #97646 (comment) - the preprocessor is not available in BaseMaterial3D. Either that's an engine bug that can be fixed, or this PR is not actually the right approach, and something more like #89966 is right.

Set proximity_fade_enabled
  :46 - Tokenizer: Unknown character #35: '#'
  Shader compilation failed.
--Main Shader--
    1 | // NOTE: Shader automatically converted from Godot Engine 4.4.dev's StandardMaterial3D.
...
   44 | 	// Proximity Fade: Enabled
   45 | 	float proximity_depth_tex = textureLod(depth_texture, SCREEN_UV, 0.0).r;
E  46-> 	#if CURRENT_RENDERER == RENDERER_COMPATIBILITY
   47 | 	vec4 proximity_view_pos = INV_PROJECTION_MATRIX * vec4(SCREEN_UV * 2.0 - 1.0, proximity_depth_tex * 2.0 - 1.0, 1.0);
   48 | 	#else
   49 | 	vec4 proximity_view_pos = INV_PROJECTION_MATRIX * vec4(SCREEN_UV * 2.0 - 1.0, proximity_depth_tex, 1.0);
   50 | 	#endif
   51 | 	proximity_view_pos.xyz /= proximity_view_pos.w;
   52 | 	ALPHA *= clamp(1.0 - smoothstep(proximity_view_pos.z + proximity_fade_distance, proximity_view_pos.z, VERTEX.z), 0.0, 1.0);
   53 | }
   54 | 

@tetrapod00 tetrapod00 marked this pull request as draft December 10, 2024 04:44
@akien-mga akien-mga requested review from a team and removed request for a team December 10, 2024 12:03
@akien-mga
Copy link
Member

Instead of using the preprocessor, you can do the if/else in C++ and add code to the generated string conditionally.

@tetrapod00
Copy link
Contributor Author

Instead of using the preprocessor, you can do the if/else in C++ and add code to the generated string conditionally.

Yeah, the hope was to avoid that, so that the material is correct in multiple renderers even after being converted to a ShaderMaterial. The superseded PR already takes a similar approach with conditionally added C++, and was apparently deferred for a while in the hope that we could use the preprocessor somehow instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proximity Fade broken in Compatibility (OpenGL)
2 participants