Add #insert directive for glsl shaders

[VReaperV]

Picked from #1105.

Adds the ability to insert text from a specified glsl shader file into an arbitrary place in another shader.

[slipher]

Please use the feature at least once in some shader. That way we can see how it is supposed to be used and verify that it really works.

[slipher]

This shouldn't match anywhere in the line, only at the beginning (maybe preceded by whitespace). It would be annoying if commenting it out like //#insert blah didn't work.

[VReaperV]

I agree on both. The extensions thing is something that's used throught gl_shaders.cpp IIRC, so changing all of those should probably be its own pr.

[VReaperV]

Now it will only match if #insert is directly following whitespace characters or at the start of the line, no more fixed suffix as well. I've left the extension there because that's what we use everywhere, so having it suddenly different in one place would be confusing, plus I don't really see the benefit of dropping the extension if we're gonna put all shader code into .glsl files anyway.

[slipher]

The existing "include" method (vertexInlines/fragmentInlines) uses the #line directive so that compiler errors have the right line number. It would be nice to have that here too (and also #file I guess, if that's supported).

[illwieckz]

Please use the feature at least once in some shader. That way we can see how it is supposed to be used and verify that it really works.

Usage is expected to be done when merging the material branch:

• Implement material system #1105

This is simply a process of splitting that branch in smaller chunks to review and to merge separately when possible.

I will not oppose at merging this as a structural foundation for what's coming.

[illwieckz]

I agree with the idea of just writing the file name with the suffix _fp/_vp. Having the ability to include the same file in both vertex and fragment shaders is useful. When working on the sRGB code in some of my various attempts I had to include my sRGB colorspace conversion function in both fragment shaders (for lightmaps) and in vertex shaders (for vertex lighting), and using the vertexInline/fragmentInline thing I had to duplicate the file that was annoying…

I'm not sure for the requirement about also writing .glsl extension, it should always be that anyway, and I like that in C++ it is possible to do #include <cstdio> without requiring the extension like in C.

I also agree with the remark about being able to comment out an #insert instruction.

[VReaperV]

Please use the feature at least once in some shader. That way we can see how it is supposed to be used and verify that it really works.

Like illwieckz said, it is just a smaller part of #1105. I don't support adding useless things just for the sake of an "example", this on the other hand is just something that has a proper use in another pr (that pr also shows that this works).

[VReaperV]

The existing "include" method (vertexInlines/fragmentInlines) uses the #line directive so that compiler errors have the right line number. It would be nice to have that here too (and also #file I guess, if that's supported).

I can add that, though personally I found it annoying rather than useful, because some errors didn't make sense with that directive (I had to comment it out at some points while working on #1105).

[VReaperV]

I agree with the idea of just writing the file name with the suffix _fp/_vp. Having the ability to include the same file in both vertex and fragment shaders is useful. When working on the sRGB code in some of my various attempts I had to include my sRGB colorspace conversion function in both fragment shaders (for lightmaps) and in vertex shaders (for vertex lighting), and using the vertexInline/fragmentInline thing I had to duplicate the file that was annoying…

Agreed on that, I thought the same when writing the changes in this pr, I only went with the suffixes because that's what the rest of the code that was already there was doing.

[DolceTriade]

I think it's helpful to split up a large PR even if all the work isn't used in each PR.

[slipher]

OK, I thought it would be easy to add an example since once of the fragmentInlines/vertexInlines could be trivially replaced with a #insert but maybe I'm wrong.

[VReaperV]

The existing "include" method (vertexInlines/fragmentInlines) uses the #line directive so that compiler errors have the right line number. It would be nice to have that here too (and also #file I guess, if that's supported).

Actually, this seems like a bad idea to me now. If there's some error we'll suddenly have a line count restart in the middle of main() or similar.

[VReaperV]

I agree with the idea of just writing the file name with the suffix _fp/_vp. Having the ability to include the same file in both vertex and fragment shaders is useful. When working on the sRGB code in some of my various attempts I had to include my sRGB colorspace conversion function in both fragment shaders (for lightmaps) and in vertex shaders (for vertex lighting), and using the vertexInline/fragmentInline thing I had to duplicate the file that was annoying…

I also agree with the remark about being able to comment out an #insert instruction.

Done now.

[VReaperV]

OK, I thought it would be easy to add an example since once of the fragmentInlines/vertexInlines could be trivially replaced with a #insert but maybe I'm wrong.

It would be trivial to do so, but I just don't see the point of it. The shaders we had worked fine with the inlines thing, so why change them just for the sake of changing? #1105 works as the example IMO.

[slipher]

The existing "include" method (vertexInlines/fragmentInlines) uses the #line directive so that compiler errors have the right line number. It would be nice to have that here too (and also #file I guess, if that's supported).

Actually, this seems like a bad idea to me now. If there's some error we'll suddenly have a line count restart in the middle of main() or similar.

What do you mean?

[VReaperV]

What do you mean?

For example if there's:

void main()
{
	#insert material_fp

	// Compute view direction in world space.
	vec3 viewDir = normalize(u_ViewOrigin - var_Position);

in a shader, then you're gonna see something like this if there's an error:

59: void main()
60: {
0:  	  // A bunch of inserted stuff
..
132:
133:  	// Compute view direction in world space.
134:  	vec3 viewDir = normalize(u_ViewOrigin - var_Position);

I think that'd be annoying.

(or whatever other order of resetting line count there)

[slipher]

I was thinking mainly to have the line directive after the insertion, to maintain correct line numbers for the main file. So #line 61 in your example.

Additionally adding line directives at the beginning of the insertion is also possible, but it seems there is no way to set the file name to that of the included shader, so maybe there is no point.

[slipher]

Actually if you have an insert on line 60, maybe the best thing would be to put #line 59 before every single line so the user will know any error came from the insert on line 60 😂

[illwieckz]

Just an idea that crosses my mind, but I wonder if that's doable, even if ugly, to assume we will never have GLSL files of 10k lines, and then we may prefix insert with #line 10000, with 1 being the first insert.

Let's imagine we have this:

void main()
{
	#insert material_fp
	#insert blabla_fp
	vec3 viewDir = normalize(u_ViewOrigin - var_Position);

We would do:

#line 0
void main()
{
#line 10000
	#insert material_fp
#line 20000
	#insert blabla_fp
#line 4
	vec3 viewDir = normalize(u_ViewOrigin - var_Position);

So we would get:

0: void main()
1: {
10000:	code_from_material_fp;
10001:	code_from_material_fp
10002:	code_from_material_fp
10003:	code_from_material_fp
10004:	code_from_material_fp
20000:	code_from_blabla_fp;
20001:	code_from_blabla_fp;
20002:	code_from_blabla_fp;
4: 	vec3 viewDir = normalize(u_ViewOrigin - var_Position);

[illwieckz]

The code looks good enough for now, we can improve over it later.

LGTM.

[VReaperV]

Actually if you have an insert on line 60, maybe the best thing would be to put #line 59 before every single line so the user will know any error came from the insert on line 60 😂

Lol, that doesn't sound too bad. Something better in general than just doing #line 0 everywhere would be great, e. g. keep a table that matches the non-reset line count to filename + line, then if an error occurs try to get the reported line and lookup in that table... The problem is I don't think shader compiler errors are standardized in any way.

[VReaperV]

Just an idea that crosses my mind, but I wonder if that's doable, even if ugly, to assume we will never have GLSL files of 10k lines, and then we may prefix insert with #line 10000, with 1 being the first insert.

Let's imagine we have this:

void main()
{
	#insert material_fp
	#insert blabla_fp
	vec3 viewDir = normalize(u_ViewOrigin - var_Position);

We would do:

#line 0
void main()
{
#line 10000
	#insert material_fp
#line 20000
	#insert blabla_fp
#line 4
	vec3 viewDir = normalize(u_ViewOrigin - var_Position);

So we would get:

0: void main()
1: {
10000:	code_from_material_fp;
10001:	code_from_material_fp
10002:	code_from_material_fp
10003:	code_from_material_fp
10004:	code_from_material_fp
20000:	code_from_blabla_fp;
20001:	code_from_blabla_fp;
20002:	code_from_blabla_fp;
4: 	vec3 viewDir = normalize(u_ViewOrigin - var_Position);

This looks like a good idea to me.

[VReaperV]

I was thinking mainly to have the line directive after the insertion, to maintain correct line numbers for the main file. So #line 61 in your example.

Ah, I see. To me personally either of those would look annoying though :p

The other problem we have with reporting shader compiler errors is that the log gets spammed with the license text for every included file. I suppose at least it serves as a way to separate each file there lol.

[VReaperV]

Squashed the small fix, no actual changes.