Modularize PBRLighting.frag

[yaRnMcDonuts]

Addresses this issue: #2122

To re-summarize:

This PR extracts all of the texture reads and base PBR matParam assignment into a .glslib file (currently named PBRLightingParamReads.glsllib)

And then it also extracts all of the lighting calculation into a .glslib file (currently named PBRLighting.glsllib)

I also fixed alot of formatting and indentation mistakes, and overall reorganized the shader.

This should now make it as easy as possible for jme devs to fork PBRLighting to make changes, and they will no longer have to update the lighting or texReads to stay on par with master, as all future changes will all be in the glsllibs. This also reduces the chances that a lesser-skilled shader dev (or even skilled shader devs on a bad day) mistakenly mess up the lighting calculations when forking the shader.

Any feedback and review is greatly appreciated.

[yaRnMcDonuts]

Once approved, I will also update the PBRTerrain shaders to create 2 new glsllibs titled "AdvancedPBRTerrainParamReads.glsllibs" and "PBRTerrainParamReads.glsllib" so that those will be modularized as well.

Both of those shaders will also be able to use PBRLighting.glslib for the final lighting calculation, ensuring that PBRLighting and PBRTerrain shaders always share the same lighting code and this will greatly reduce code redundancy.

It will also make shader development waaay easier for devs like myself who have 10 forks of PBRLighting all with minor differences.

[yaRnMcDonuts]

I have also placed both of the glslllibs in Common/ShaderLib/ for now...

However I am not sure if that is the right place for these, as there is also a PBRLighting.glslib that contains the deeper pbr lighting equations (which are referenced and used in the new PBRLighting.glsllib) and the two could be easily confused.

So maybe we should make a new shaderLib directory for modular PBR glsllibs? I'm open to hearing others input on this (for now I don't have the correct reference to the glsllib in PbrLighting.frag, but I will update the import to be correct once a final location has been determined for these new glsllibs)

I have also removed some ifDefs aroungs things like tbnMat's calculation, so that it will be calculated even if there is no normal map (I also still need to change PBRLighting.vert to remove ifDefs around the passing of the tbnMat and vViewDir varyings) . This will be important so that forks of the shader that do things like splatting normal maps will still be easy to make even if the original model does not have a normal map, without having to go in and change any of these .glsllibs

[yaRnMcDonuts]

Just made one more set of changes because I forgot about the spec-gloss pipeilne.

I typically only use the metallic pipeline so I overlooked spec gloss yesterday, but I fixed it so both will work now.

I've extensively tested my changes with models that use the metallic pipeline, however I have very few spec-gloss models so I didn't get to test that as thoroughly.

I've also updated the vert and j3md files, so now this PR should be ready for a full-scale review.

(The only reamining error in the code that I am aware of is the incorrect reference to importing PBRLighting.glsllib and PBRLightingParamReads.glsllib, since I am still unsure of where the best place to place these 2 new glslibs is)

[scenemax3d]

Hi @yaRnMcDonuts ,
Thank you for this PR 👍
This split that you have made , does it breaks compatibility in any way for devs who forked the current shaders? do they need to change any of their custom shaders because of this change?

10X :)

[yaRnMcDonuts]

There shouldn't be any compatibility issues for any existent forks that are already out there. I made sure to not remove any params or defines from the shader, and only added 3 defines that will not have any effect if left undefined. So all files (the .j3md, .frag, and .vert) on master should still be compatible with forked versions of each other.

do they need to change any of their custom shaders because of this change?

If any jme devs out there don't want to adapt these changes to their own existing forks of PBRLighting.frag, then they should still be able to keep all of the logic in one big file and manually keep it up to date with master/stable without any newly added trouble.

However, the next time someone makes a PR to add a new feature to the new PBRLighting .glsllibs on master, (such as the recent SpecularAA or AOStrength PRs) then, at that point, I'd highly recommend everyone with forks of PBRLighting adapt the changes and have their forks use the new glsllibs, so then they will never have to worry about updating their forks to stay on par with master/stable ever again in the future.

But if anyone still chooses not to adapt these changes and doesn't mind manually adding new features to their own custom version of PBRLighting.frag everytime master/stable has an update, then they are still free to keep on doing things the old way without any issues.

[capdevon]

How about renaming the file PBRLightingParamReads.glsllib -> PBRLightingParamsReader.glsllib since it contains a method named readMatParamsAndTextures ?

[yaRnMcDonuts]

That sounds like a good name to me, I updated the PR to rename it to PBRLightingParamsReader.glsllib and also updated the imports in PBRLighting.frag to reflect the change.

I also fixed the import path to point to Common/ShaderLib/ in jme-core, so unless anyone objects to having these 2 new .glsllibs in the Common/ShaderLib/ directory in favor of a better place, then I will leave them there.

[yaRnMcDonuts]

I updated the comments to fix the typos you pointed out, thanks for the proofread.

I also appeared to have misspelled glsllib as glslib multiple times (missing the second L) in the comments, so I fixed that as well.

[yaRnMcDonuts]

The build failed on mac due to a time-out error, however I resubmitted the same file and it worked again... Strange, but at least everything is okay and there were no real problems.

[scenemax3d]

@yaRnMcDonuts , Since this is a change to the way dev. will use the engine and indicates some kind of official best practice approach for using shaders, I would like engine core devs to comment on it before integration.
So, @pspeed42 , @riccardobl , @stephengold, @MeFisto94 - can you please review & comment on this PR?

Thanks!

[riccardobl]

Hi @riccardobl , when do you plan to push your changes to this PR?

I don't have the push permissions, i've asked @yaRnMcDonuts for them, maybe worth pinging him here too

[yaRnMcDonuts]

Sorry I didn't see your message on discord, I don't have discord launch by default on my computer so sometimes I miss notifications on there until I manually open it up again.

I went into my repo in the "collaborators" section of my repo's settings, and added you as a collaborator. Does that allow you to push to that branch, or is there any other setting/permission I need to change in the repo settings?

[riccardobl]

That worked, thanks @yaRnMcDonuts

[riccardobl]

I've pushed the changes, the code needs to be fully tested.
This is a documentation dump of everything in this PR

Struct based modularity

The idea behind this design is to use mutable structs to create data containers that can be passed to functions.
Functions should mainly use and manipulate these structs to implement the shader logic.
See the following simple illustrative example:

uniform sampler2D m_Tex;
uniform vec2 texCoord;

struct Surface {
    vec4 baseColor;
    vec4 litColor;
} 

struct Light {
    vec4 lightColor;
}

Surface getSurface(){
    Surface s;
    s.baseColor=texture2D(m_Tex,texCoord);
    return s;
}

Light getLight(){
    Light l;
    l.lightColor=vec4(1);
    return l;
}

void applyLight(inout Surface surface, in Light light){
    surface.litColor=surface.baseColor+light.lightColor;
}

void main(){
    Surface surface=getSurface();
    Light light=getLight();
    applyLight(surface,light);
    gl_FragColor=surface.litColor;
}

From there by strategically splitting the code into multiple libraries and using macros we can create reusable modular shader code.

The #struct directive and extends struct

This PR adds a #struct directive that works exactly like the struct keywords but makes structs extensible (like a class extends ) so that they can be reused, together with all their related functions, by shaders that need to store more data fields
See example below:

#struct MyStruct
    float a;
    float b;
#endstruct

#struct MyStruct2 extends MyStruct
   float c;
#endstruct

void main(){
    MyStruct s;
    s.a=1.0;
    s.b=1.0;

    MyStruct2 s2;
    s2.a=1.0;
    s2.b=1.0;
    s2.c=1.0;
}

It can also extend more than one struct:

#struct MyStructMulti extends StructA,StructB,StructC 
   float z;
#endstruct

Using defines to swap structs

#struct is pretty much a preprocessor hack, there isn't real inheritance in glsl, so if, for example, we have a function with this signature doSomething(inout MyStruct s) we can't just pass MyStruct2 to it, even if it "extends" MyStruct.
To overcome this limitation we can use a simple trick.
First, let's create a struct in MyStruct.glsl (nb .glsllib and .glsl are loaded in the same way, i like to use glsllib for libraries comprised of functions and .glsl for fragments)
MyStruct.glsl

#struct StrMyStruct
    float a;
    float b;
#endstruct

#define MyStruct StrMyStruct

Note that we are prefixing the struct with "Str" and then creating a macro that maps MyStruct to StrMyStruct.
This is the key of our little trick.

Now let's write a library that uses this struct
MyLibrary.glsllib

void doSomething(inout MyStruct s){
  s.a=1.0;
}

Note that we used "MyStruct" here, the compiler will replace it with StrMyStruct as per our macro.

Now let's implement the library in a shader
Fragment.frag

#import "MyStruct.glsl"
#import "MyLibrary.glsllib"

void main(){
     MyStruct s;
     doSomething(s); 
}

Nothing out of the ordinary here, but what if we need to extend MyStruct for our shader logic, but we still want to use MyLibrary.glsllib ?
Very simple
FragmentEXTEND.frag

#import "MyStruct.glsl"

// the magic is here
#struct StrMyStruct2 extends StrMyStruct
   float c;
#endstruct
#define MyStruct StrMyStruct2
// .....

#import "MyLibrary.glsllib"
void main(){
     MyStruct s;
     doSomething(s);
     s.c=1.0;    
}

That's it, by redefining the macro to map to our extended struct
#define MyStruct StrMyStruct2

We are telling the compiler to replace MyStruct with StrMyStruct2 instead of StrMyStruct everywhere in the code that is included below the definition, this makes MyLibrary use StrMyStruct2 internally, StrMyStruct2 has the same fields of StrMyStruct plus one more, so the code will compile and work just fine and we will have the extra c parameter for our shader logic.

Standard Structs

This PR comes with two standard structs

• StdLight -> mapped to Light : a jmonkeyengine light
• StdPBRSurface -> mapped to PBRSurface : a surface of a pbr material

These are used as Light and PBRSurface in the code and can be extended and swapped as explained in the previous points.
They are found in
Common/ShaderLib/module/Light.glsl
Common/ShaderLib/module/PBRSurface.glsl

PBRLightingUtils.glslib

This is a GLSLLib (Common/ShaderLib/module/pbrlighting/PBRLightingUtils.glsllib) implementing most of the PBR Lighting logic (both param reading and computing).
It is built around the requirements of PBRLighting.frag, so some of its code might not be needed by different shaders, for this reason it employs several macros that allow to easily and surgically include only the apis needed by each implementation.
This is done by defining ENABLE_functionName to 1 before importing the library

#define ENABLE_PBRLightingUtils_getWorldPosition 1
#define ENABLE_PBRLightingUtils_getWorldNormal 1
#define ENABLE_PBRLightingUtils_getWorldTangent 1
#define ENABLE_PBRLightingUtils_getTexCoord 1
#define ENABLE_PBRLightingUtils_newLight 1
#define ENABLE_PBRLightingUtils_computeLightInWorldSpace 1
#define ENABLE_PBRLightingUtils_readPBRSurface 1
#define ENABLE_PBRLightingUtils_computeDirectLight 1
#define ENABLE_PBRLightingUtils_computeDirectLightContribution 1
#define ENABLE_PBRLightingUtils_computeProbesContribution 1

#import "Common/ShaderLib/module/pbrlighting/PBRLightingUtils.glsllib"

It is just a matter of omitting the defines for the apis that are not needed or need to be reimplemented with custom logic.

Modular PBRLighting.j3md

This PR doesn't change the default PBRLighting but instead creates a new material in
"Common/MatDefs/Light/modular/PBRLighting.j3md"
This is the material that should be used going forward while the old one should be kept only as legacy code and should be deprecated once the new one is tested enough.
In a similar manner other materials can be converted to "modular materials" and made available with a similar conventions under a "modular" subdirectory next to their monolithic variant.

[scenemax3d]

Hi @riccardobl ,
Is this PR with all the latest modification you committed to it is mature enough for integrating to master branch?

WDYT?

[riccardobl]

Hi @riccardobl , Is this PR with all the latest modification you committed to it is mature enough for integrating to master branch?

WDYT?

We need to test it and see if the implementation is good enough or if things need to be abstracted further.
@yaRnMcDonuts confirmed to me that he is in the process of adding it to a demo, so we should leave this open for a while in case we need to make adjustments

[yaRnMcDonuts]

I unfortunately haven't gotten around to testing your changes in my own project yet, although they do look good so far from what I have reviewed.

I haven't had as much time as to work on my jme game this year as I have had in the past, and I got pulled into some other non shader related things around the time I last commented about testing this PR, so I haven't had time to shift back into shader-brain to work on this again (and likely won't be able to anytime soon).

It does look like the latest updates you made are all okay, and the approach you took is cleaner than my initial proposal for this PR, while still retaining the important benefits from my initial PR.

However the functionality between your updated code and my original code is still pretty much the same, so even though your implementation is an improvement on my original code and is cleaner and more extensible, I haven't been able to find the time/motivation to re-refactor all my PBR forks to match your latest update to this PR, and I'm also somewhat burnt out on shader refactoring and this PR since I already refactored multiple forks of PBR lighting and the terrains multiple times while working on this.

I do plan to tackle this task and refactor all my shaders to match your changes some time in the future, but, as of right now, doing so unfortunately offers no benefits to my main JME project since it is solely a refactoring task and doesn't change any features/functionality in my game, so I have to prioritize some other tasks first.

In the meantime I would say it is probably okay to pass this PR and have the community test it in an alpha/beta release rather than waiting for me to test it on my pbr forks, especially since shaders tend to produce different errors/issues on different devices.

[yaRnMcDonuts]

Hi @riccardobl

I finally have time to work on this again, and I just built this fork to test it and implement everything into my main project.

The TestPBR case with the tank ran fine, however now that I've built and imported this fork's jme-core jar to my own game, I am getting the following crash related to my usage of the Overaly.j3md filter:

`
Oct 09, 2024 3:11:55 PM com.jme3.renderer.opengl.GLRenderer updateShaderSourceData
WARNING: Bad compile of:
1	#version 110
2	#define SRGB 1
3	#define FRAGMENT_SHADER 1
4	#extension GL_ARB_texture_multisample : enable
5	// -- begin import Common/ShaderLib/GLSLCompat.glsllib --
6	#ifdef FRAGMENT_SHADER
7	      precision highp float;
8	      precision highp int;
9	      precision highp sampler2DArray;
10	      precision highp sampler2DShadow;
11	      precision highp samplerCube;
12	      precision highp sampler3D;
13	      precision highp sampler2D;
14	      #if __VERSION__ >= 310
15	        precision highp sampler2DMS;
16	      #endif
17	
18	#endif
19	
20	#if defined GL_ES
21	#  define hfloat highp float
22	#  define hvec2  highp vec2
23	#  define hvec3  highp vec3
24	#  define hvec4  highp vec4
25	#  define lfloat lowp float
26	#  define lvec2 lowp vec2
27	#  define lvec3 lowp vec3
28	#  define lvec4 lowp vec4
29	#else
30	#  define hfloat float
31	#  define hvec2  vec2
32	#  define hvec3  vec3
33	#  define hvec4  vec4
34	#  define lfloat float
35	#  define lvec2  vec2
36	#  define lvec3  vec3
37	#  define lvec4  vec4
38	#endif
39	
40	#if __VERSION__ >= 130
41	
42	#ifdef FRAGMENT_SHADER
43	  #ifdef GL_ES
44	    #ifdef BOUND_DRAW_BUFFER
45	
46	 #if 0<=BOUND_DRAW_BUFFER  
47	        #if BOUND_DRAW_BUFFER == 0
48	          layout( location = 0 ) out highp vec4 outFragColor;
49	        #else
50	          layout( location = 0 ) out highp vec4 outNOP0;
51	        #endif
52	 #endif 
53	
54	 #if 1<=BOUND_DRAW_BUFFER  
55	        #if BOUND_DRAW_BUFFER == 1
56	          layout( location = 1 ) out highp vec4 outFragColor;
57	        #else
58	          layout( location = 1 ) out highp vec4 outNOP1;
59	        #endif
60	 #endif 
61	
62	 #if 2<=BOUND_DRAW_BUFFER  
63	        #if BOUND_DRAW_BUFFER == 2
64	          layout( location = 2 ) out highp vec4 outFragColor;
65	        #else
66	          layout( location = 2 ) out highp vec4 outNOP2;
67	        #endif
68	 #endif 
69	
70	 #if 3<=BOUND_DRAW_BUFFER  
71	        #if BOUND_DRAW_BUFFER == 3
72	          layout( location = 3 ) out highp vec4 outFragColor;
73	        #else
74	          layout( location = 3 ) out highp vec4 outNOP3;
75	        #endif
76	 #endif 
77	
78	 #if 4<=BOUND_DRAW_BUFFER  
79	        #if BOUND_DRAW_BUFFER == 4
80	          layout( location = 4 ) out highp vec4 outFragColor;
81	        #else
82	          layout( location = 4 ) out highp vec4 outNOP4;
83	        #endif
84	 #endif 
85	
86	 #if 5<=BOUND_DRAW_BUFFER  
87	        #if BOUND_DRAW_BUFFER == 5
88	          layout( location = 5 ) out highp vec4 outFragColor;
89	        #else
90	          layout( location = 5 ) out highp vec4 outNOP5;
91	        #endif
92	 #endif 
93	
94	 #if 6<=BOUND_DRAW_BUFFER  
95	        #if BOUND_DRAW_BUFFER == 6
96	          layout( location = 6 ) out highp vec4 outFragColor;
97	        #else
98	          layout( location = 6 ) out highp vec4 outNOP6;
99	        #endif
100	 #endif 
101	
102	 #if 7<=BOUND_DRAW_BUFFER  
103	        #if BOUND_DRAW_BUFFER == 7
104	          layout( location = 7 ) out highp vec4 outFragColor;
105	        #else
106	          layout( location = 7 ) out highp vec4 outNOP7;
107	        #endif
108	 #endif 
109	
110	 #if 8<=BOUND_DRAW_BUFFER  
111	        #if BOUND_DRAW_BUFFER == 8
112	          layout( location = 8 ) out highp vec4 outFragColor;
113	        #else
114	          layout( location = 8 ) out highp vec4 outNOP8;
115	        #endif
116	 #endif 
117	
118	 #if 9<=BOUND_DRAW_BUFFER  
119	        #if BOUND_DRAW_BUFFER == 9
120	          layout( location = 9 ) out highp vec4 outFragColor;
121	        #else
122	          layout( location = 9 ) out highp vec4 outNOP9;
123	        #endif
124	 #endif 
125	
126	 #if 10<=BOUND_DRAW_BUFFER  
127	        #if BOUND_DRAW_BUFFER == 10
128	          layout( location = 10 ) out highp vec4 outFragColor;
129	        #else
130	          layout( location = 10 ) out highp vec4 outNOP10;
131	        #endif
132	 #endif 
133	
134	 #if 11<=BOUND_DRAW_BUFFER  
135	        #if BOUND_DRAW_BUFFER == 11
136	          layout( location = 11 ) out highp vec4 outFragColor;
137	        #else
138	          layout( location = 11 ) out highp vec4 outNOP11;
139	        #endif
140	 #endif 
141	
142	 #if 12<=BOUND_DRAW_BUFFER  
143	        #if BOUND_DRAW_BUFFER == 12
144	          layout( location = 12 ) out highp vec4 outFragColor;
145	        #else
146	          layout( location = 12 ) out highp vec4 outNOP12;
147	        #endif
148	 #endif 
149	
150	 #if 13<=BOUND_DRAW_BUFFER  
151	        #if BOUND_DRAW_BUFFER == 13
152	          layout( location = 13 ) out highp vec4 outFragColor;
153	        #else
154	          layout( location = 13 ) out highp vec4 outNOP13;
155	        #endif
156	 #endif 
157	
158	 #if 14<=BOUND_DRAW_BUFFER  
159	        #if BOUND_DRAW_BUFFER == 14
160	          layout( location = 14 ) out highp vec4 outFragColor;
161	        #else
162	          layout( location = 14 ) out highp vec4 outNOP14;
163	        #endif
164	 #endif 
165	    #else
166	      out highp vec4 outFragColor;
167	    #endif
168	  #else
169	    out vec4 outFragColor;
170	  #endif
171	#endif
172	
173	#  define texture1D texture
174	#  define texture2D texture
175	#  define texture3D texture
176	#  define textureCube texture
177	#  define texture2DLod textureLod
178	#  define textureCubeLod textureLod
179	#  define texture2DArray texture
180	#  if defined VERTEX_SHADER
181	#    define varying out
182	#    define attribute in
183	#  elif defined FRAGMENT_SHADER
184	#    define varying in
185	#    define gl_FragColor outFragColor
186	#  endif
187	#else
188	#  define isnan(val) !(val<0.0||val>0.0||val==0.0)
189	#endif
190	
191	#if __VERSION__ == 110
192	mat3 mat3_sub(mat4 m) {
193	  return mat3(m[0].xyz, m[1].xyz, m[2].xyz);
194	}
195	#else
196	 #define mat3_sub mat3
197	#endif
198	
199	#if __VERSION__ <= 140
200	float determinant(mat2 m) {
201	  return m[0][0] * m[1][1] - m[1][0] * m[0][1];
202	}
203	
204	float determinant(mat3 m) {
205	  return  + m[0][0] * (m[1][1] * m[2][2] - m[1][2] * m[2][1])
206	          - m[0][1] * (m[1][0] * m[2][2] - m[1][2] * m[2][0])
207	          + m[0][2] * (m[1][0] * m[2][1] - m[1][1] * m[2][0]);
208	}
209	#endif
210	
211	#if __VERSION__ <= 130
212	mat2 inverse(mat2 m) {
213	  return mat2(m[1][1], -m[0][1], -m[1][0], m[0][0]) / determinant(m);
214	}
215	
216	mat3 inverse(mat3 m) {
217	  return mat3(
218	    + (m[1][1] * m[2][2] - m[2][1] * m[1][2]),
219	    - (m[1][0] * m[2][2] - m[2][0] * m[1][2]),
220	    + (m[1][0] * m[2][1] - m[2][0] * m[1][1]),
221	    - (m[0][1] * m[2][2] - m[2][1] * m[0][2]),
222	    + (m[0][0] * m[2][2] - m[2][0] * m[0][2]),
223	    - (m[0][0] * m[2][1] - m[2][0] * m[0][1]),
224	    + (m[0][1] * m[1][2] - m[1][1] * m[0][2]),
225	    - (m[0][0] * m[1][2] - m[1][0] * m[0][2]),
226	    + (m[0][0] * m[1][1] - m[1][0] * m[0][1])) / determinant(m);
227	}
228	#endif
229	// -- end import Common/ShaderLib/GLSLCompat.glsllib --
230	// -- begin import Common/ShaderLib/MultiSample.glsllib --
231	
232	uniform int m_NumSamples;
233	uniform int m_NumSamplesDepth;
234	
235	#ifdef RESOLVE_MS
236	    #define COLORTEXTURE sampler2DMS
237	#else
238	    #define COLORTEXTURE sampler2D
239	#endif
240	
241	#ifdef RESOLVE_DEPTH_MS
242	    #define DEPTHTEXTURE sampler2DMS
243	#else
244	    #define DEPTHTEXTURE sampler2D
245	#endif
246	
247	// NOTE: Only define multisample functions if multisample is available
248	#if defined(GL_ARB_texture_multisample) || (defined GL_ES && __VERSION__>=310)
249	vec4 textureFetch(in sampler2DMS tex,in vec2 texC, in int numSamples){
250	      ivec2 iTexC = ivec2(texC * vec2(textureSize(tex)));
251	      vec4 color = vec4(0.0);
252	      for (int i = 0; i < numSamples; i++){
253	         color += texelFetch(tex, iTexC, i);
254	      }
255	      return color / float(numSamples);
256	}
257	
258	vec4 fetchTextureSample(in sampler2DMS tex,in vec2 texC,in int sampleId){
259	    ivec2 iTexC = ivec2(texC * vec2(textureSize(tex)));
260	    return texelFetch(tex, iTexC, sampleId);
261	}
262	
263	vec4 getColor(in sampler2DMS tex, in vec2 texC){
264	      return textureFetch(tex, texC, m_NumSamples);
265	}
266	
267	vec4 getColorSingle(in sampler2DMS tex, in vec2 texC){
268	    ivec2 iTexC = ivec2(texC * vec2(textureSize(tex)));
269	    return texelFetch(tex, iTexC, 0);
270	}
271	
272	vec4 getDepth(in sampler2DMS tex,in vec2 texC){
273	      return textureFetch(tex,texC,m_NumSamplesDepth);
274	}
275	
276	#endif
277	
278	vec4 fetchTextureSample(in sampler2D tex,in vec2 texC,in int sampleId){
279	    return texture2D(tex,texC);
280	}
281	
282	vec4 getColor(in sampler2D tex, in vec2 texC){
283	    return texture2D(tex,texC);
284	}
285	
286	vec4 getColorSingle(in sampler2D tex, in vec2 texC){
287	    return texture2D(tex, texC);
288	}
289	
290	vec4 getDepth(in sampler2D tex,in vec2 texC){
291	    return texture2D(tex,texC);
292	}
293	// -- end import Common/ShaderLib/MultiSample.glsllib --
294	
295	uniform COLORTEXTURE m_Texture;
296	uniform vec4 m_Color;
297	varying vec2 texCoord;
298	
299	void main() {
300	      vec4 texVal = getColor(m_Texture, texCoord);
301	      gl_FragColor = texVal * m_Color;
302	}

Oct 09, 2024 3:11:55 PM com.jme3.app.LegacyApplication handleError
SEVERE: Uncaught exception thrown in Thread[jME3 Main,5,main]
com.jme3.renderer.RendererException: compile error in: ShaderSource[name=Common/MatDefs/Post/Overlay.frag, defines, type=Fragment, language=GLSL100]
0(7) : warning C7022: unrecognized profile specifier "highp"
0(7) : warning C7022: unrecognized profile specifier "precision"
0(8) : warning C7022: unrecognized profile specifier "highp"
0(8) : warning C7022: unrecognized profile specifier "precision"
0(9) : error C7532: global type sampler2DArray requires "#version 130" or later
0(9) : error C0000: ... or #extension GL_EXT_gpu_shader4 : enable
0(9) : error C0000: ... or #extension GL_EXT_texture_array : enable
0(9) : error C0000: ... or #extension GL_NV_texture_array : enable
0(9) : warning C7022: unrecognized profile specifier "highp"
0(9) : warning C7022: unrecognized profile specifier "precision"
0(10) : warning C7022: unrecognized profile specifier "highp"
0(10) : warning C7022: unrecognized profile specifier "precision"
0(11) : warning C7022: unrecognized profile specifier "highp"
0(11) : warning C7022: unrecognized profile specifier "precision"
0(12) : warning C7022: unrecognized profile specifier "highp"
0(12) : warning C7022: unrecognized profile specifier "precision"
0(13) : warning C7022: unrecognized profile specifier "highp"
0(13) : warning C7022: unrecognized profile specifier "precision"


  at com.jme3.renderer.opengl.GLRenderer.updateShaderSourceData(GLRenderer.java:1658)
  at com.jme3.renderer.opengl.GLRenderer.updateShaderData(GLRenderer.java:1685)
  at com.jme3.renderer.opengl.GLRenderer.setShader(GLRenderer.java:1750)
  at com.jme3.material.logic.DefaultTechniqueDefLogic.render(DefaultTechniqueDefLogic.java:97)
  at com.jme3.material.Technique.render(Technique.java:168)
  at com.jme3.material.Material.render(Material.java:1090)
  at com.jme3.renderer.RenderManager.renderGeometry(RenderManager.java:695)
  at com.jme3.post.FilterPostProcessor.renderProcessing(FilterPostProcessor.java:235)
  at com.jme3.post.FilterPostProcessor.renderFilterChain(FilterPostProcessor.java:321)
  at com.jme3.post.FilterPostProcessor.postFrame(FilterPostProcessor.java:342)
  at com.jme3.renderer.RenderManager.renderViewPort(RenderManager.java:1238)
  at com.jme3.renderer.RenderManager.render(RenderManager.java:1292)
  at com.jme3.app.SimpleApplication.update(SimpleApplication.java:283)
  at com.jme3.system.lwjgl.LwjglAbstractDisplay.runLoop(LwjglAbstractDisplay.java:160)
  at com.jme3.system.lwjgl.LwjglDisplay.runLoop(LwjglDisplay.java:225)
  at com.jme3.system.lwjgl.LwjglAbstractDisplay.run(LwjglAbstractDisplay.java:242)
  at java.base/java.lang.Thread.run(Thread.java:840)
`

If I remove the filter using Overlay.frag, then my game runs with no errors, and in that case it looks like the new changes are not breaking anything else with my own shaders so far.

But some changes we've made appear to have broken that specific filter, I am not sure why though since it looks like a very basic color overlay filter.

[riccardobl]

0(9) : error C7532: global type sampler2DArray requires "#version 130" or later
0(9) : error C0000: ... or #extension GL_EXT_gpu_shader4 : enable
0(9) : error C0000: ... or #extension GL_EXT_texture_array : enable
0(9) : error C0000: ... or #extension GL_NV_texture_array : enable

i think we need to exclude

      precision highp sampler2DArray;
      precision highp sampler2DShadow;

on GLSL < 130 in glslcompat

[yaRnMcDonuts]

I updated it to check for the version before precision highp sampler2DArray; and the error is gone now, so that appears to have fixed it.

It says there was a conflict with this branch though, it appears a #ifdef GL_ES check was added to master since this PR opened, so I updated GLSLCompat on this branch to reflect those changes. Let me know if it looks like that was the correct way to resolve the branch conflict.

[stephengold]

Let me know if it looks like that was the correct way to resolve the branch conflict.

Not a correct solution, @yaRnMcDonuts . Try using the web editor.

[yaRnMcDonuts]

I don't quite understand what is incorrect in my 3rd (and most recent) update to GLSLCompat.glsllib

I did use the web editor to add a line of code from the current master branch (based on the differences I saw when I clicked "resolve conflict" after my first edit to GLSLCompat.glsllib) but now it still says there is a conflict and i don't understand what the conflict is.

I must be misunderstanding something about what github means when it says this branch has conflicts, hopefully someone can let me know what I'm doing wrong.

[yaRnMcDonuts]

nevermind my last post, it looks like I just had to remove the conflict marker characters <<<<<<<, =======, >>>>>>> that it had inserted into the file after the initial conflict.

Not sure why github actually added those lines into the file, I thought that was just display-only text in the web editor so I didn't realize I needed to delete it manually. It should be resolved now.

So this PR should finally be ready to merge( as long as it looks okay to everyone else). I built this branch and ran my game with these changes and everything appears to be okay.

Once everything is approved and this PR is merged, then I'll start working on modularizing PBRTerrain and AdvancedPBRTerrain with the same struct funtionality and shared pbrLighting glsllib, and after that the engine's PBR shaders will be much easier to maintain and update in the future.

[yaRnMcDonuts]

So this PR should finally be ready to merge( as long as it looks okay to everyone else)

Can any core devs/engine leaders take a look at this PR for a final review?

[yaRnMcDonuts]

@riccardobl do you think this PR is ready to be merged now?

I'm hoping that we could get this in for the next beta release for more widespread testing, and then (as long as there are no further issues found) have this modularized version replace the current PBR shader by the next stable release.

[yaRnMcDonuts]

Are there any core devs or engine leaders that could please take a look at this PR and merge it? I am starting to get the feeling that this PR is not wanted in core, and am starting to doubt that it will ever make it to the point that this replace PBRLighting due to the lack of interest any other jme dev has. I guess no one else forks PBRLighting as heavily as I do, so no one else sees the insane benefit and time-saving potential that this PR can bring.

This PR is also holding me up in regards to doing important bug-fixes with PBRTerrain and AdvancedPBRTerrain, and it is also holding up my own personal refactoring of PBR forks in my own project. (I already did work refactoring my own shaders and deleted it twice due to changes I was told to make to this PR, so I want to avoid that again and am holding off on working on many things until this PR passes)

So if this PR is not acceptable and is no longer something that engine leaders want, then I would apprecaite if someone could tell me and I will close the PR and go back to implementing things in my own projects. It would be a shame if that is the case, but I get the feeling no one else sees the huge benefit in this PR that I see, and feel like I've mostly been commenting into the void in this PR.

[codex128]

I'll try taking a look at it. I should be able to do it within this week.

[codex128]

I've mostly suggested readability changes. The other PBRLighting files are a mess, so it'd be great if this were a bit neater.

Also, I think it'd be a good idea to profile the modular PBRLighting versus the current PBRLighting. There are so many defines added that it may have impacted shader compilation performance.

[codex128]

Add spaces around =

[codex128]

Add spacing: for (int i = 0; i < NB_LIGHTS; i += 3) {

[codex128]

Add spaces: if (m_DebugValuesMode == n) { for each if statement.

[codex128]

See about removing redundant comments to make the parameters more neat and easier to read.

[codex128]

Is this file any different from Common/MatDefs/Light/PBRLighting.vert?

[yaRnMcDonuts]

I think it is identical, all of the changes in this PR are related to modularizing the Fragment shader, so a new .vert shader should not be necessary. I think riccardo added this when he decided to make this into a new shader, rather than modifying the existing PBRLighting shader (although I still disagree with that decision, as creating a new shader it defeats the whole purpose of modularizing things in the first place)

[codex128]

One line function declaration.

[codex128]

Fix indentation in added test methods.

[codex128]

String comparison with == is dangerous. Replace with structBodyLine.isEmpty().

[codex128]

Revert this. It would be better to create a new j3m file specifically for testing the modular PBR matdef.

[codex128]

Could this debug block be moved to a glsllib?

[yaRnMcDonuts]

Unfortunately I do not think this code can be moved. I recall trying moving that code in the past, but it appeared that any references to "gl_FragColor" need to be in the main .frag file

[codex128]

Ok, I'll take your word for it. It isn't important anyway.

[codex128]

A few more problems I noticed.

[codex128]

If you wanted to further modularize this, perhaps move each texture read type to a seperate function?

[yaRnMcDonuts]

Yes that would increase the modularity, although I didn't do this at first because I figured iit would be unlikely that a shader dev would need to write their shader to do an albedo texture read without having code to do the rest of the standard pbr texture reads, since typically you'd just leave the un-wanted textures un-defined to achieve the same thing.

But from a code standpoint, it would certainly look more organized if each texture read were in its own function, so I will update the .glsllibs to put each texture read in its own function.

[codex128]

Insert spaces around =

[codex128]

Fix indentation (lines 458 through 480)

[codex128]

Insert spaces around =

[yaRnMcDonuts]

@codex128 thanks for the review!

You are right that extra compilation time is a potential issue with the new struct design riccardo added. I suspect the extra time should be negligible, but I will profile the shader to make sure (do you know if there is a simple jme shader profiler tool I can use to do this?).

I also read all of your formatting suggestions, and I will edit the PR to fix all of the formatting mistakes you pointed out once I've checked the compilation time to make sure that the new struct system is not slowing anything down. (I just don't want to start making minor formatting changes again until I am sure that the file being reviewed is indeed going to remain, as I already did some formatting work that got deleted after riccardo's changes)

In the event that the struct system is taking a significant amount of extra time to compile, the first half of this PR (the .glsllibs) can still be used and the struct functionality can be put on hold.

In hindsight, I also should have had riccardo put the struct functionality in a separate PR and helped him work on that PR after this one gets merged. This PR has become somewhat of a mess trying to combine 2 new loosely related features (the .glsllibs and the struct functionality) into one mega PR.

[yaRnMcDonuts]

Another important thing to note before I do anymore work on this:

After more consideration, I am still very firm in my original belief that these changes should be immediately applied to PBRLighting.frag, and an additional PBR shader in a new directory should NOT be created (as is currently the plan after riccardo's last changes to this PR).

JME does not need an extra modularized PBR shader that requires extra maintaining. It needs 1 modularized PBR shader that is easy to test and maintain.

I specifically made this PR to so that it would be less work to update 10 forks of PBRLighting anytime PBRLighting.frag gets a new feature or bug-fix on core.

By making this into a new shader, that just adds one more extra place to make changes, and defeats the whole purpose of a modular system that only requires updates in 1 place.

So if this is a deal breaker and others still think this should be a whole new shader, then I will close the PR and make these changes to the main PBRLighting shader the right way in my own project, the way I originally envisioned in order to solve the original issue. I feel that my efforts to share this PR have ended up causing the core point of this PR to be derailed and overlooked.

[yaRnMcDonuts]

I'll wait a few more days for anyone to comment with additional review, and then I will proceed making the final minor changes before I merge this PR.

I aim to have this PR merged and ready for the second alpha release of 3.8 (3.8.0-alpha2)