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

Add a shader preprocessor #define for the renderer (Compatibility, Forward+, or Mobile) #10764

Closed
tetrapod00 opened this issue Sep 17, 2024 · 4 comments · Fixed by godotengine/godot#98549

Comments

@tetrapod00
Copy link

tetrapod00 commented Sep 17, 2024

Describe the project you are working on

Writing shaders, including snippets that will be copied around.

Describe the problem or limitation you are having in your project

There is a common code snippet to reconstruct NDC from SCREEN_UV and depth, used to reconstruct world position or get linear depth:
vec3 ndc = vec3(SCREEN_UV * 2.0 - 1.0, depth);
It's documented in Advanced Postprocessing and used in the VisualShaderNodeLinearSceneDepth shader node.

NDC space varies between Forward+/Mobile and Compatibility. If you are writing a shader which uses this snippet, you must account for both NDC spaces for the shader to be usable in all renderers.

Currently this is possible with a snippet like this:

float depth = texture(DEPTH_TEXTURE, SCREEN_UV).x;
vec3 ndc;
if (OUTPUT_IS_SRGB){
  // Compatibility. Uses OpenGL NDC space: range of [-1,1] for NDC.xy, range of [-1,1] for NDC.z
  ndc = vec3(screen_uv, depth) * 2.0 - 1.0;
} else {
  // Forward+ or Mobile. Uses Vulkan NDC space: range of [-1,1] for NDC.xy, range of [0,1] for NDC.z
  ndc = vec3(screen_uv * 2.0 - 1.0, depth); 
}

There are two problems with this:

  • It uses runtime branching with an if() statement.
  • It uses a shader built-in, OUTPUT_IS_SRGB, which is a proxy for the relevant information but whose name does not actually guarantee that behavior.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

Add a predefined value representing the renderer to the shader preprocessor. I want to write this code instead:

float depth = texture(DEPTH_TEXTURE, SCREEN_UV).x;
#ifdef RENDERER_IS_COMPATIBILITY
  vec3 ndc = vec3(screen_uv, depth) * 2.0 - 1.0;
#else
  vec3 ndc = vec3(screen_uv * 2.0 - 1.0, depth); 
#endif

The minimum solution that would work for my use case is a single define:
RENDERER_IS_COMPATIBILITY. true if the renderer is Compatibility, false otherwise.
Additionally, we probably want:
RENDERER_IS_FORWARD_PLUS. true if the renderer is Forward+, false otherwise.
RENDERER_IS_MOBILE. true if the renderer is Mobile, false otherwise.
RENDERER_IS_MOBILE_OR_FORWARD. true if the renderer is Forward+ or Mobile, false otherwise. I don't know what the right name for this one is - RENDERER_IS_VULKAN isn't right. We may also want to consider the possibility of other renderers, like a Deferred renderer.

Alternately, we may want to explicitly define macros for the relevant feature, rather than making assumptions:
USE_OPEN_GL_NDC. true if the renderer is Compatibility, false if Forward+ or Mobile.
I feel that a define for the renderer is more general purpose though.

There are several related proposals, which could solve this problem and more. I see that some of them had PRs and were rejected earlier in the 4.x cycle. In keeping with the godot philosophy, I suggest this instead, which solves a specific problem with a specific solution.
Related: #6207, #5062, #8076

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

I don't know the implementation details for this one, sorry!

If this enhancement will not be used often, can it be worked around with a few lines of script?

Yes, there is an existing workaround. See above.

Is there a reason why this should be core and not an add-on in the asset library?

Requires changes to the shader preprocessor.

@sockeye-d
Copy link

Correct me if I'm wrong but runtime branching isn't a huge performance penalty if all of the threads branch the same way, right? I do think it's a good idea to have some preprocessor defines though

@sockeye-d
Copy link

It may also be worth having something like this:

#define CURRENT_RENDERER // 0, 1, 2
#define RENDERER_COMPATIBILITY 0
#define RENDERER_MOBILE 1
#define RENDERER_FORWARD_PLUS 2

// used like this
#if CURRENT_RENDERER == RENDERER_MOBILE

I don't think the preprocessor supports bitflags but that could be a good way to handle the different features

@tetrapod00
Copy link
Author

Yeah, the shader preprocessor docs links to this article by Jason Booth which describes how branching is not so bad. As is, you do need to use uniform bools and runtime branching for a configurable shader, since godot doesn't support setting #defines using the shader inspector.

In this case, there really is no need to be runtime branching though! The renderer is known from when you first open godot.

@clayjohn
Copy link
Member

It may also be worth having something like this:

#define CURRENT_RENDERER // 0, 1, 2
#define RENDERER_COMPATIBILITY 0
#define RENDERER_MOBILE 1
#define RENDERER_FORWARD_PLUS 2

// used like this
#if CURRENT_RENDERER == RENDERER_MOBILE

I don't think the preprocessor supports bitflags but that could be a good way to handle the different features

I would go with this approach so that it allows people to write shaders that are forward-compatible when we add a new renderer (we plan on adding one more renderer BTW).

For example I could write a plugin that says:

 #if CURRENT_RENDERER == RENDERER_MOBILE
// ...
#elif CURRENT_RENDERER == 3
// ...
#endif

While this is a magic number (which isn't great) this shader would work in version 4.4 or 4.5 or whatever. So I could ship it to users of all versions, not just users of a version after we add the new renderer

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

Successfully merging a pull request may close this issue.

5 participants