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

Shader preprocessor remake #62513

Merged
merged 2 commits into from
Jul 22, 2022

Conversation

reduz
Copy link
Member

@reduz reduz commented Jun 29, 2022

Based on #61462.

  • Moved preprocessor to Shader and ShaderInclude
  • Clean up RenderingServer side
  • Preprocessor is separate from parser now, but it emits tokens with include location hints.
  • Improved ShaderEditor validation code
  • Added include file code completion
  • Added notification for all files affected by a broken include.

Images:
shader_edit
Error reporting on the text side:
image
Code completion for includes:
image

Bugsquad edit: Closes godotengine/godot-proposals#944

scene/resources/shader.h Outdated Show resolved Hide resolved
@reduz reduz force-pushed the shader_preprocessor_remake branch 3 times, most recently from 63a914e to af2e20a Compare June 29, 2022 15:10
@clayjohn
Copy link
Member

clayjohn commented Jun 29, 2022

Its working well so far from my testing. I am noticing a few issues though.

  1. When editing a processor function (vertex, fragment etc.) in a ShaderInclude file, there is no highlighting or autocompletion for built-ins.
  2. Stemming from "1." the shader editor reports a compile error if you edit a processor function from a ShaderInclude, yet it actually compiles and runs the shader just fine.
    Screenshot from 2022-06-29 09-02-28
    Screenshot from 2022-06-29 09-04-48

@reduz
Copy link
Member Author

reduz commented Jun 30, 2022

@clayjohn Did you try using a shader_type in the include? that should work I think.

@clayjohn
Copy link
Member

@clayjohn Did you try using a shader_type in the include? that should work I think.

@reduz Just tried now. It results in an error

Screenshot from 2022-06-30 08-40-58
Screenshot from 2022-06-30 08-40-53

Comment on lines +883 to +889
bool ShaderPreprocessor::check_directive_before_type(Tokenizer *p_tokenizer, const String &p_directive) {
if (p_tokenizer->get_index() < state->shader_type_pos) {
set_error(vformat(RTR("`#%s` may not be defined before `shader_type`."), p_directive), p_tokenizer->get_line());
return false;
}
return true;
}
Copy link
Member

@Chaosus Chaosus Jul 3, 2022

Choose a reason for hiding this comment

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

This function does not needed now, since you've removed the handling of shader_type_pos (which prevent declaration of directives before shader type).

int include_depth = 0;
String current_include;
String current_shader_type;
int shader_type_pos = -1;
Copy link
Member

Choose a reason for hiding this comment

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

Remove shader_type_pos - if we don't need handling of it (to prevent preprocessor directive declaration before shader type.

@Chaosus
Copy link
Member

Chaosus commented Jul 3, 2022

@reduz Error handling messages seems broken:

image

@warriormaster12
Copy link
Contributor

  1. Is it possible to backport this feature Godot 3.x?
  2. will you implement same functionality for visual shaders?

@Chaosus
Copy link
Member

Chaosus commented Jul 20, 2022

  1. Is it possible to backport this feature Godot 3.x?
  2. will you implement same functionality for visual shaders?
  1. This may be possible
  2. How do you imagine it? Which nodes should be made?

@warriormaster12
Copy link
Contributor

@Chaosus
Copy link
Member

Chaosus commented Jul 20, 2022

@Chaosus

docs.unrealengine.com/4.27/en-US/RenderingAndGraphics/Materials/Functions

What exactly do you mean? - there are a lot of functions inside UE. Also, create a proposal for this please.

@warriormaster12
Copy link
Contributor

I mean that a developer could make custom nodes by writing first a visual shader and then including that visual shader in a bigger visual shader as function node (with user defined inputs and outputs). The expression node kinda works the way I described but currently you can only write inside it in text and you can't seperately save it to the disk for use.

I just wanted to clarify what I meant, if I still need to create a proposal then I'll do it but for sake of consistency it would be nice to see something similar to this pull request but for visual shaders.

@Chaosus
Copy link
Member

Chaosus commented Jul 20, 2022

I mean that a developer could make custom nodes by writing first a visual shader and then including that visual shader in a bigger visual shader as function node (with user defined inputs and outputs). The expression node kinda works the way I described but currently you can only write inside it in text and you can't seperately save it to the disk for use.

You can already use VisualShaderNodeCustom (https://docs.godotengine.org/en/latest/tutorials/plugins/editor/visual_shader_plugins.html) and create custom visual shader nodes.

@warriormaster12
Copy link
Contributor

Feels a bit less intuitive to create that way in my opinion .

Chaosus and others added 2 commits July 22, 2022 22:51
Co-authored-by: TheOrangeDay <6472143+TheOrangeDay@users.noreply.github.com>
* Moved preprocessor to Shader and ShaderInclude
* Clean up RenderingServer side
* Preprocessor is separate from parser now, but it emits tokens with include location hints.
* Improved ShaderEditor validation code
* Added include file code completion
* Added notification for all files affected by a broken include.
@reduz reduz force-pushed the shader_preprocessor_remake branch from af2e20a to f649678 Compare July 22, 2022 21:01
@reduz
Copy link
Member Author

reduz commented Jul 22, 2022

@akien-mga We discussed with @Chaosus that the purpose of this PR, which was redoing the preprocess design, to solve some of the fundamental problems in integrating the original PR, have been achieved and it majorly works. It may be better to just merge it as-is (its fully usable) and then @Chaosus can work on fixing the small issues in the shader compiler/preprocessor which is more his area than mine nowadays.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Agreed, let's merge this and improve further in follow up PRs.

Testers who found bugs are welcome to open dedicated issues to track them further.

@akien-mga akien-mga merged commit fe929d4 into godotengine:master Jul 22, 2022
@akien-mga
Copy link
Member

akien-mga commented Jul 22, 2022

Thanks to everyone involved in making this happen!

🥇 @bitsawer @fire @MagdielM @TheOrangeDay @Chaosus @clayjohn @reduz

@AlfishSoftware
Copy link

Oops I never noticed this update. I guess I have to update the vscode plugin's syntax for .gdshader and add .gdshaderinc (presumably the exact same syntax?).
Would be nice if you guys ping me on GDShader syntax updates; or is there somewhere I can subscribe to be notified on just those? Because I'm not actively monitoring Godot, and just happened to know of this by accident.

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.

Add support for sharing and preprocessing shader code (through #include, #if, #ifdef, …)
6 participants