Shader include support #6902

Jhonnyg · 2022-08-22T20:59:03Z

Added support for #include pragmas in shader files. This feature can be used to separate shader functions into modules that can be used across several shader files, which should help out with reusing code from your own project, or from asset extensions.

Fixes #6892

PR checklist (remove before merging)

Prepared the PR for release notes
- Proper description of feature or fix (see example x?)
- Added a #fixes ### if it fixes an issue
- Assigned issue to a project
[-] Added documentation for public API changes
Is this a new feature?
- Added engine and/or editor unit tests
- Added or updated manual entry (Added shader include manual entry doc#316)

…/defold into issue-6892-shader-include-support

editor/src/clj/editor/workspace.clj

Jhonnyg · 2023-01-09T13:26:41Z

editor/resources/templates/template.glsl

+// To get access to the functions, you need to put:
+// #include "/my-folder/my-file.glsl"
+// in any script using the functions.
+// Please consult the manual on how to use this feature!


We could include a link to the manual here but the entry doesn't exist yet since this hasn't been merged yet..

Jhonnyg · 2023-01-09T13:27:25Z

com.dynamo.cr/com.dynamo.cr.bob/src/com/dynamo/bob/pipeline/ShaderUtil.java


 public class ShaderUtil {
+    public static class Common {


A lot of the shader transpilation uses these functions so just moved them into a separate namespace

com.dynamo.cr/com.dynamo.cr.bob/src/com/dynamo/bob/pipeline/ShaderProgramBuilder.java

Jhonnyg · 2023-01-09T13:37:15Z

com.dynamo.cr/com.dynamo.cr.bob/src/com/dynamo/bob/pipeline/ShaderProgramBuilder.java

+        // Iterate the source lines to find the first occurance of a 'valid' line
+        // i.e a line that doesn't already have a preprocessor directive, or is empty
+        // This is needed to get a valid line number when shader compilation has failed


This (and the lines after) is used for getting debug output if the shader fails to compile in runtime and won't work 100% with pasting the include source since most includes will use #ifdef header guards and the lines won't correspond to the actual lines from the includes.

But on the other hand.. it already produces wrong #line pragmas right now anyway if you have block comments for instance, so it's already incorrect behaviour anyway and I don't know what the alternative is :(

One really roundabout way of doing it would be to split the source by each include since OpenGL supports uploading shader source by a list of strings (https://registry.khronos.org/OpenGL-Refpages/gl4/html/glShaderSource.xhtml) and then using this information to output which include the error came from. I don't think it should be a showstopper for this feature, but perhaps something that will come up eventually..

com.dynamo.cr/com.dynamo.cr.bob/src/com/dynamo/bob/pipeline/ShaderIncludeCompiler.java

editor/src/clj/editor/workspace.clj

vlaaad

LGTM, but I agree with Mats we should use built-in features for the relativization of paths.

editor/src/clj/editor/code/shader.clj

editor/src/clj/editor/workspace.clj

Jhonnyg · 2023-01-10T16:10:45Z

@matgis @vlaaad fixed yer concerns

vlaaad

LGTM!

It would be nice if we could create .glsl files in the editor using File -> New menu.

vlaaad · 2023-01-11T08:33:58Z

editor/src/clj/editor/code/shader.clj

                  :resource-ext (resource/type-ext resource)}})])

+(def ^:private include-pattern #"^\s*#include \s*\"([^\"]+?)\"\s*$")


\s* (space before slash) -> \s* (no spaces before slash)?

Or rather \s+? + being "1 or more" ?

Also, this doesn't support comments after the include (something I do a lot), since the $matches the end.
Try something like this:
^\s*\#include\s*(<([^"<>|\b]+)>|"([^"<>|\b]+)")

https://regex101.com/r/q99Yz4/1

Hm, that regex will still allow this:
#include "yada" jiasjdsjiasdidas
Maybe:
^\s*\#include\s*(<([^"<>|\b]+)>|"([^"<>|\b]+)")\s*(?:\/\/.*)?$
https://regex101.com/r/xvR1hT/1

It requires the $, but we look at includes line-by-line and it won't work if you use block comments, but not sure we want to fix everything right now?

LGTM!

It would be nice if we could create .glsl files in the editor using File -> New menu.

It does, it's called "shader include"

JCash

Look good.
I think the include regex can be improved upon to support comments at the end of the line.
There's also a question about preserving the order-of-appearance

JCash · 2023-01-11T09:36:17Z

com.dynamo.cr/com.dynamo.cr.bob/src/com/dynamo/bob/pipeline/ShaderProgramBuilder.java

+        for (String path : includes) {
+            taskBuilder.addInput(this.project.getResource(path));
+        }


com.dynamo.cr/com.dynamo.cr.bob/src/com/dynamo/bob/pipeline/ShaderProgramBuilder.java

JCash · 2023-01-11T09:46:54Z

editor/src/clj/editor/code/shader.clj

+                   :view-opts glsl-opts}
+                  {:ext "glsl"
+                   :label "Shader Include"
+                   :icon "icons/64/Icons_29-AT-Unknown.png"


Perhaps we can add a glsl icon?

JCash · 2023-01-11T09:48:03Z

editor/src/clj/editor/code/shader.clj

                  :resource-ext (resource/type-ext resource)}})])

+(def ^:private include-pattern #"^\s*#include \s*\"([^\"]+?)\"\s*$")


Or rather \s+? + being "1 or more" ?

Also, this doesn't support comments after the include (something I do a lot), since the $matches the end.
Try something like this:
^\s*\#include\s*(<([^"<>|\b]+)>|"([^"<>|\b]+)")

https://regex101.com/r/q99Yz4/1

com.dynamo.cr/com.dynamo.cr.bob/src/com/dynamo/bob/pipeline/ShaderIncludeCompiler.java

skip-ci

Jhonnyg and others added 15 commits August 22, 2022 22:58

Moved compile code to ShaderUtil

2b155f7

Use .glsl resource for include directive replacement

ffcd918

Refactor

19cf8a9

Parse includes from cmd line

6575cec

WIP

e7ddce0

Move shader include compiler to separate class

03d47e1

Merge branch 'dev' into issue-6892-shader-include-support

7461a3f

Merge branch 'dev' into issue-6892-shader-include-support

fff8139

Merge branch 'dev' into issue-6892-shader-include-support

13921d7

Merge branch 'dev' into issue-6892-shader-include-support

4b8efab

Editor support for shader includes work-in-progress

a578c66

Merge branch 'issue-6892-shader-include-support' of github.com:defold…

a43c1b2

…/defold into issue-6892-shader-include-support

Refactor include compiler

92f0ab7

Added shader include tests in bob

09836bf

Cleanup

10c4369

Jhonnyg added feature request A suggestion for a new feature engine Issues related to the Defold engine render labels Jan 3, 2023

Jhonnyg added 4 commits January 3, 2023 23:51

Merge branch 'dev' into issue-6892-shader-include-support

19450f5

Fix tests

128f1b0

Cleanup and two more tests

d759c86

Add .glsl template

1bdb0c2

Jhonnyg commented Jan 9, 2023

View reviewed changes

editor/src/clj/editor/workspace.clj Outdated Show resolved Hide resolved

Jhonnyg commented Jan 9, 2023

View reviewed changes

com.dynamo.cr/com.dynamo.cr.bob/src/com/dynamo/bob/pipeline/ShaderProgramBuilder.java Outdated Show resolved Hide resolved

Jhonnyg commented Jan 9, 2023

View reviewed changes

com.dynamo.cr/com.dynamo.cr.bob/src/com/dynamo/bob/pipeline/ShaderIncludeCompiler.java Outdated Show resolved Hide resolved

Jhonnyg requested review from JCash and vlaaad January 9, 2023 13:40

Jhonnyg marked this pull request as ready for review January 9, 2023 13:40

matgis requested changes Jan 9, 2023

View reviewed changes

editor/src/clj/editor/workspace.clj Outdated Show resolved Hide resolved

editor/src/clj/editor/workspace.clj Outdated Show resolved Hide resolved

editor/src/clj/editor/workspace.clj Outdated Show resolved Hide resolved

vlaaad requested changes Jan 10, 2023

View reviewed changes

editor/src/clj/editor/code/shader.clj Outdated Show resolved Hide resolved

editor/src/clj/editor/workspace.clj Outdated Show resolved Hide resolved

Jhonnyg added 4 commits January 10, 2023 14:16

Merge branch 'dev' into issue-6892-shader-include-support

a1a8707

Use canonical path instead for resource resolving

5505111

Minor simplification

9afbc5d

Refactor toProjectRelativePath to use relativize

222c2af

Remove unused

20ce694

vlaaad previously approved these changes Jan 11, 2023

View reviewed changes

JCash requested changes Jan 11, 2023

View reviewed changes

Jhonnyg added 2 commits January 11, 2023 17:02

Merge branch 'dev' into issue-6892-shader-include-support

880aac2

Review fixes, use new regex

5607778

Jhonnyg dismissed vlaaad’s stale review via 5607778 January 11, 2023 20:08

Jhonnyg added 3 commits January 12, 2023 10:13

Small cleanup

8408d69

skip-ci

Fix windows support

75e99be

Merge branch 'dev' into issue-6892-shader-include-support

52a627f

Jhonnyg merged commit 6658043 into dev Jan 17, 2023

Jhonnyg deleted the issue-6892-shader-include-support branch January 17, 2023 00:08

matgis mentioned this pull request Jan 24, 2023

Editor: Ensure the workspace root path is canonical #7313

Merged

Jhonnyg changed the title ~~Issue 6892: Shader include support~~ Shader include support Feb 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Shader include support #6902

Shader include support #6902

Jhonnyg commented Aug 22, 2022 •

edited

Loading

Jhonnyg Jan 9, 2023

Jhonnyg Jan 9, 2023

Jhonnyg Jan 9, 2023

vlaaad left a comment

Jhonnyg commented Jan 10, 2023

vlaaad left a comment

vlaaad Jan 11, 2023

JCash Jan 11, 2023

Jhonnyg Jan 11, 2023

Jhonnyg Jan 11, 2023

JCash left a comment

JCash Jan 11, 2023

JCash Jan 11, 2023

JCash Jan 11, 2023

		:resource-ext (resource/type-ext resource)}})])

		(def ^:private include-pattern #"^\s#include \s\"([^\"]+?)\"\s*$")

Shader include support #6902

Shader include support #6902

Conversation

Jhonnyg commented Aug 22, 2022 • edited Loading

PR checklist (remove before merging)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vlaaad left a comment

Choose a reason for hiding this comment

Jhonnyg commented Jan 10, 2023

vlaaad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JCash left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jhonnyg commented Aug 22, 2022 •

edited

Loading