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

Implement custom non-trivial Visual Shader nodes #64248

Merged
merged 1 commit into from
Aug 28, 2022

Conversation

paddy-exe
Copy link
Contributor

@paddy-exe paddy-exe commented Aug 10, 2022

Implement (partly) these proposals:

(RotationMatrix node and other DistanceFade modes in future PR) godotengine/godot-proposals#5106
(only as VS node): godotengine/godot-proposals#5112

  • Proximity fade -> Category: Fade
  • Distance fade -> Category: Fade
  • Rand between two values -> Category: Random
  • Polar coordinates conversion -> Category: Textures / Common
  • Remap -> Category: Utility
  • Linear depth -> Category: Textures / Depth

@paddy-exe paddy-exe force-pushed the built-in-shader-functions branch 2 times, most recently from 0e56ea3 to b89b75d Compare August 10, 2022 23:18
@Chaosus Chaosus added this to the 4.0 milestone Aug 11, 2022
@nathanfranke
Copy link
Contributor

Is M_PI exposed to the user, if so, it should be something like built_ins.glsl (not with _functions).

Also, looks like files in Godot's repository favor no spaces with builtin, so maybe builtins.glsl instead, although I admit that's harder to read.

$ find -iname '*builtin*' | wc -l
104
$ find -iname '*built_in*' | wc -l
0

@paddy-exe
Copy link
Contributor Author

paddy-exe commented Aug 11, 2022

Is M_PI exposed to the user, if so, it should be something like built_ins.glsl (not with _functions).

The user can call PI in shader code since it's a global built-in now. I wasn't sure if I need to include it in the glsl file or not but I saw it in other shader files and I needed it for the polar_coordinates function.

Also, looks like files in Godot's repository favor no spaces with builtin, so maybe builtins.glsl instead, although I admit that's harder to read.

Thanks for going through the log! I will have to try it out. Since the plan is that it is automatically included anyway, the user doesn't need to include the file. (At least that's how I understood it when Clay John explained it to me)

@paddy-exe paddy-exe force-pushed the built-in-shader-functions branch 3 times, most recently from 7298d40 to 1309b23 Compare August 12, 2022 20:20
@reduz
Copy link
Member

reduz commented Aug 13, 2022

I don't think adding functions to the core shaders to be used with the high level shaders is a good idea. This will increase compilation time for those shaders (and there are a lot to compile) without need.

IMO if you want to add these things for the core shaders as something to help users, this should be downloaded from the asset library, not be provided built-in.

If this is needed just for visual shaders, feel free to put those there directly, so they are used only on demand. Don't make them always available for the rest of the shaders.

@nathanfranke
Copy link
Contributor

nathanfranke commented Aug 14, 2022

Is it possible to compile a snippet once, but still have it available everywhere? Otherwise, I agree that this should be in AssetLib, not core (knowing how many core shaders there are and how noticeable the shader compilation already is).

@paddy-exe
Copy link
Contributor Author

Is it possible to compile a snippet once, but still have it available everywhere? Otherwise, I agree that this should be in AssetLib, not core (knowing how many core shaders there are and how noticeable the shader compilation already is).

@Chaosus had the idea for text-based shader add-ons but right now I guess we already have the shader preprocessor with include files that should do the trick.
I will be updating the PR according to Juan's feedback.👍🏻

@paddy-exe paddy-exe force-pushed the built-in-shader-functions branch 3 times, most recently from ed91178 to f25c1fd Compare August 14, 2022 20:06
@paddy-exe
Copy link
Contributor Author

Polar Coordinates done for now:
image
image

@paddy-exe paddy-exe changed the title Implement custom non-trivial shader functions Implement custom non-trivial VS nodes Aug 14, 2022
@paddy-exe paddy-exe force-pushed the built-in-shader-functions branch 4 times, most recently from d801df8 to fab9240 Compare August 18, 2022 11:49
@paddy-exe
Copy link
Contributor Author

Random Range works now as well:
image

@paddy-exe
Copy link
Contributor Author

Remap node also works
image

@paddy-exe paddy-exe force-pushed the built-in-shader-functions branch 5 times, most recently from c4d759f to 27cfd59 Compare August 21, 2022 15:13
@paddy-exe paddy-exe force-pushed the built-in-shader-functions branch from 27cfd59 to 7605b70 Compare August 21, 2022 15:14
@paddy-exe paddy-exe marked this pull request as ready for review August 21, 2022 15:14
@paddy-exe paddy-exe requested a review from a team as a code owner August 21, 2022 15:14
Copy link
Member

@Chaosus Chaosus left a comment

Choose a reason for hiding this comment

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

I think no need to overcomplicate port names so much.

@paddy-exe paddy-exe force-pushed the built-in-shader-functions branch 2 times, most recently from df6baf6 to fc8bdfe Compare August 23, 2022 17:28
@Chaosus Chaosus self-requested a review August 23, 2022 19:39
@paddy-exe paddy-exe force-pushed the built-in-shader-functions branch from fc8bdfe to 6760a74 Compare August 23, 2022 21:23
@Chaosus
Copy link
Member

Chaosus commented Aug 24, 2022

Need to fix the static checks and generate docs. You can generate docs by executing the following command
in the Godot's exe directory: ./godot.windows.opt.tools.64.exe.exe --doctool .. - This is for Windows, on Linux it might be a bit different.

@paddy-exe paddy-exe force-pushed the built-in-shader-functions branch from 6760a74 to c5ccc73 Compare August 26, 2022 18:15
@paddy-exe paddy-exe requested a review from a team as a code owner August 26, 2022 18:15
@mhilbrunner mhilbrunner changed the title Implement custom non-trivial VS nodes Implement custom non-trivial Visual Shader nodes Aug 26, 2022
@paddy-exe paddy-exe force-pushed the built-in-shader-functions branch from c5ccc73 to 0450bfc Compare August 27, 2022 18:27
@paddy-exe paddy-exe force-pushed the built-in-shader-functions branch from 0450bfc to 55bbcc5 Compare August 27, 2022 20:59
@paddy-exe
Copy link
Contributor Author

Rebased and should be ready to merge

@Chaosus Chaosus merged commit 1f9b992 into godotengine:master Aug 28, 2022
@Chaosus
Copy link
Member

Chaosus commented Aug 28, 2022

Thanks!

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.

None yet

5 participants