-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Modularize PBRLighting.frag #2191
base: master
Are you sure you want to change the base?
Conversation
Addresses this issue: jMonkeyEngine#2122 To re-summarize: This PR extracts all of the texture reads and basematParam 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.
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. |
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 |
Forgot about spec-gloss pipeline
Added spec-gloss vars
add inouts for specularColor and glossiness to account for specGloss pipeline
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) |
removed ifDef checks around some things (like tangents) that could still be used by other glslibs even if the shader doesn't have a normal map.
add matDefs for indoor sunlight exposure and debugging final output. These features were previously not in PBRLighting.frag, but they are in PBRTerrain.frag which will eventually share the same glslib for lighting (after doing texture reads), so they both should be consistent.
remove unnecessary assignment of norm, as normal will already = norm if a normalMap is not assigned, and norm isn't used anywhere else in this glslib so no need to pass it in
Hi @yaRnMcDonuts , 10X :) |
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.
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. |
How about renaming the file |
…eader.glsllib renamed to PBRLightingParamsReader.glsllib
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. |
jme3-core/src/main/resources/Common/MatDefs/Light/PBRLighting.frag
Outdated
Show resolved
Hide resolved
jme3-core/src/main/resources/Common/MatDefs/Light/PBRLighting.frag
Outdated
Show resolved
Hide resolved
jme3-core/src/main/resources/Common/MatDefs/Light/PBRLighting.frag
Outdated
Show resolved
Hide resolved
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. |
not sure why build failed after last commit that was only changing comments. resubmitting to try again...
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. |
jme3-core/src/main/resources/Common/MatDefs/Light/PBRLighting.j3md
Outdated
Show resolved
Hide resolved
added missing in/out to readMatParamsAndTextures
added missing uniform to .frag shader
@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. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you said , jme's pbr is a jumbled mess, so the only way i managed to give it a sense was by extensively using structs, if you want to give it a shot and try to implement the same solution or something entirely different , feel free.
If you deem it not worth or too complex/out-of-scope, i am fine with this pr as it is, i want to give it a closer look before approving it, though.
#ifdef SPECULAR_AA_THRESHOLD | ||
uniform float m_SpecularAAKappa; | ||
#endif | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These uniforms should be relegated to PBRLightingParamsReader since they are specific to PBRLighting material params
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I must've made a mistake leaving the SpecularAA params there.
As for these other lighting variables:
uniform vec4 g_LightData[NB_LIGHTS];
uniform vec3 g_CameraPosition;
uniform vec4 g_AmbientLightColor;
#if NB_PROBES >= 1
uniform samplerCube g_PrefEnvMap;
uniform vec3 g_ShCoeffs[9];
uniform mat4 g_LightProbeData;
#endif
#if NB_PROBES >= 2
uniform samplerCube g_PrefEnvMap2;
uniform vec3 g_ShCoeffs2[9];
uniform mat4 g_LightProbeData2;
#endif
#if NB_PROBES == 3
uniform samplerCube g_PrefEnvMap3;
uniform vec3 g_ShCoeffs3[9];
uniform mat4 g_LightProbeData3;
#endif
Would these just be better off placed in the .frag file then (rather than either .glsllib), since they can be used by both a single pass and multi-pass implementation? Or are these lighting uniform variables exclusive to the current multi-pass lighting?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This goes together with the other comment, unless the logic is abstracted, there isn't a good place to declare uniforms.
When i said spaghetti code i was referring to this: let's say i want to fork PBRLighting.frag, i need to know that this glsllib declares these uniforms, but also the other one declares some others, so if my fork needs the g_CameraPosition, and i have this glsllib included, i need to know that i can't declare the g_CameraPosition uniform again, because it is already in the glsllib.
Imagine this applied to all the shader code, now you have libraries that they look as if they provided useful reusable code, but they actually can conflict unless used into a specific way.
This can be at best a temporary fix for simplify PBRLighting forks, as you said, but it is not the right way to make it modular.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had planned to declare all potentially reusable uniforms in the .frag shader to avoid any potential conflicting uniforms in a glsllib that needs that variable.
There may be some that I missed, but I did,(for example) keep varyings and uniforms like wPosition and tbnMat declared in the .frag shader specifically because I thought those are likelky to be re-used in other .glsllibs.
However I just wasn't certain if these lighting params were the type that would ever need declared again, since I don't have a ton of experience with other lighting code aside from whats in PBRLighting.frag curently.
But I can move all of those uniforms you mentioned back to the .frag shader.
And yes this PR is probably more of a temporary fix, or morseo only a partial modularization, mostly aimed at modularizing the param & texture reads more than the lighting, since I don't have as much experience with the lighting side. And as I mentioned in the other comment, I don't see as big of a need to modularize the lighting on core when we only have one current lighting pipeline on core at the moment anyways. But that certainly is the correct solution at some point in the future.
uniform float m_SpecularAAKappa; | ||
#endif | ||
|
||
vec3 calculatePBRLighting(in vec4 albedo, in float Metallic, in float Roughness, in vec4 specularColor, in float glossiness, in vec3 lightMapColor, in vec3 ao, in float indoorSunLightExposure, in vec3 normal, in vec3 norm, in vec3 viewDir){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code can be refactored into generic functions in PBR.glsllib.
To achieve this, all the code that relies on preprocessor directives (eg. #ifdef LIGHTMAP , for( int i = 0;i < NB_LIGHTS; i+=3){ ) should be split into different functions that are then called by the material shader depending on its logic (eg. a single pass lighting would use the for-loop NB_LIGHTS, but a multipass shader would not, so having this logic baked calculatePBRLighting limits its flexibility).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Making a single-pass version of the PBRLighting shader may be out of the scope of my capabilities currently, since (unless I'm mistaken) the current PBRLighting implementation only handles multi-pass and I'm mostly learning from working with that code, so I was mostly writing the PBRLighting.glsllib to be just be a multi-pass implementation
So maybe I should rename PBRLighting.glsllib to instead be MultiPassPbrLighting.glsllib ?
(and then I can still try split the non-multi pass lighting code into more modular functions that could also be used by a future single pass implementation that would be called SinglePassPBRLighting.glsllib)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current PBRLighting is single pass. The point i was making is that in PBRLighting.glsllib there are parts that are generic PBR stuff, parts that are for light calculation in world space and parts that are just specific to the PBRLighting implementation.
Even if you split the code as you did, it solves only some of the issue, eg. if you want to implement a shader that uses UBOs (now that they are core) you would need to rewrite the PBRLighting.glsllib entirely, that's why a further abstraction would be preferable.
Eg. see this code that uses structs and UBOs for dynamic lighting in tiled deferred shading
vec3 dlightSum=vec3(0);
for(int i=0;i<MAX_LIGHTS_PER_GEOM;i++){
// this part is specific to the implementation and collects the light data passed as UBO that you normally have in g_LightData in jme
if(i>=bo_LightsData.nLights||i>=m_NLightsPerGeo)break;
int lightIndex=m_LightsIndices[i];
Light light=bo_LightsData.lights[lightIndex];
// This is the generic world space light computation that you have in PBRLighting.glsllib
ComputedJmeLight clight=Light_computeFor(surface.position, surface.normal, surface.viewDir, light);
// This is the PBR lighting
vec3 lightResult = PBR_computeDirectLight(surface,clight);
dlightSum += lightResult;
}
outColor.rgb+=dlightSum;
This would go into the fragment shader and as you can see it can reuse most of the code and abstracts away all the complex logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could have swore that I was told before that (even though the technique is called SinglePassAndImageBased) that PBRLighting is actually multi-pass and has to do another pass to re-draw the scene once per light (and I thought that was the reason why my scenes lag with many point lights, and why a deferred rendering pipeline is needed to instead do the passes more efficiently).
Is this assumption incorrect?
I could also try my best to refactor the contents of PBRLighting.glsllib to be reusable to support more lighting techniques, but since there isn't really an official deferred rendering pipeline in jme-core (and I'm far from an expert in the subject) I really don't think I will be able to do a good job, since I've only barely messed with the lighting code in the current pipeline and can't say I fully understand it currently.
I had planned to wait til jhonkkk was done with his deferred rendering pipeline before doing this this PR (briefly discussed in the issue that spurred this PR: #2122) and I was going to use his pipeline as a reference to compare the current pipeline to so I could pull out and refactor duplicate code, but now that he disappeared I figured I'd atleast go ahead and modularize it as best as I can for JME's one current rendering pipeline.
So maybe I should focus on modularizing the ParamReads glsllib and leave the PBRLighting.glsllib as it is for now, atleast until we finally get a second rendering pipeline in the engine. Then once I have gained experience with a different type of pipeline and its lighting code I could probably come back and properly modularize the lighting side of things. But for right now that feels beyond my capabilities.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could have swore that I was told before that (even though the technique is called SinglePassAndImageBased) that PBRLighting is actually multi-pass and has to do another pass to re-draw the scene once per light (and I thought that was the reason why my scenes lag with many point lights, and why a deferred rendering pipeline is needed to instead do the passes more efficiently).
Is this assumption incorrect?
Yes, it is incorrect, PBRLighting is single pass, but there is a catch: you can't efficiently render unlimited lights with a fragment shaders, so jme does batch them in an array up to renderManager.getSinglePassLightBatchSize()
in size (nb. you can set any value for it but bigger values will consume more vram and bandwidth) and then renders each batch with one pass each.
Eg.
- SinglePassLightBatchSize = 64, you have 32 visible lights in your scene - > scene is rendered in 1 pass
- SinglePassLightBatchSize = 64, you have 65 visible lights in your scene - > scene is rendered in 2 passes
- SinglePassLightBatchSize = 64, you have 80 visible lights in your scene - > scene is rendered in 2 passes (still)
- SinglePassLightBatchSize = 64, you have 129 visible lights - > scene is rendered in 3 passes
You see what i mean, pure multipass would need 64 full passes to render 64 lights.
There are better way to do it, such as tiled deferred rendering, where lights are rendered in one or more passes for each tile of the screen and then shading is done in one single pass after.
So maybe I should focus on modularizing the ParamReads glsllib and leave the PBRLighting.glsllib as it is for now, atleast until we finally get a second rendering pipeline in the engine. Then once I have gained experience with a different type of pipeline and its lighting code I could probably come back and properly modularize the lighting side of things. But for right now that feels beyond my capabilities.
Mhh i'd like to hear your opinion on this: could we tackle this issue the other way around?
Eg. keep PBRLighting.frag as a single file (maybe reorganized a bit as you did) and have a way to inject/hook custom code into it?
I am thinking for example: i want to add a code in PBRLighting to do some computation after the parameters are read, the PBRLighting.frag will have something like
// just the normal shader code
... something something...
vec3 normal=texture(......
... something something...
// THE HOOK
#ifdef _HOOK_POST_PARAM_READ_
_HOOK_POST_PARAM_READ_(param1, normal, param3...)
#endif
and in MyForkPBRLighting.frag i will have something like
// create transform function
void postParamReadHook(inout param1, inout normal, inout param3....){
normal*=2;
}
// hook into PBRLighting
#define _HOOK_POST_PARAM_READ_ postParamReadHook
#import "PBRLighting.frag"
in this way i can really extend PBRLighting.frag from my shader in a more "standard" way, by hooking into specific parts of the code, we can have many hooks for each section with no performance impact whatsoever and with very minimal refactoring.
But we would still need the uniforms and varying to be into a separate glsllib, that can be imported multiple times without breaking the shader, so since we don't have pragma once;
in glsl, it should use macros like if we were C programmers in the 70s:
#ifndef __PBR_PARAMS__
#define __PBR_PARAMS__ 1
uniform sampler2D m_.....
......
......
#endif
So PBRLighting.frag could import it internally but if needed the fork could also import it to use uniform and varying into postParamReadHook by just importing the file at the top, first import will win.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- SinglePassLightBatchSize = 64, you have 32 visible lights in your scene - > scene is rendered in 1 pass
Hmm this still confuses me based on what I thought I'd been told in the past and especially in regards to my own findings when trying to use even a few point lights. It sounds like (based on what you're saying) that I should be able to use PBRLighting with up to 64 lights without having framerate issues since its all in one pass for up to 64 lights, but I can barely even use 2 point lights with in a moderate sized scene without the framerate dropping drastically.
Pretty much all my scenes (even ones with just a few triplanar terrains and no other models) can't render even 1-3 PointLights without destroying the framerate because (I thought) that every additional light was causing it to render the terrain and do all the triplanar texture reads again for each light.
Is that not correct? And if so then why am I getting such bad lag with only 1-3 point lights if it is <64 and supposedly all done in one pass? Is it something else causing the lights to lag then?
I also am not quite sure if I understand the hooking concept or the benefit it would add. It seems more confusing to me than if I were to just call a method from a glsllib between the param reads and lighting calculation and (unless I'm misunderstanding) it seems like the hooking could end up in a situation where there is conflicting variables since PBRLighting.frag is imported after the custom code. I would expect the hooking example code you pasted to have more issues with conflicting variables since the PBRLighting.frag import is declared after defining the hook (so the forked shader won't know about any of the variables in PBRLighting.frag, so then PBRLighting would need all the not-defined checks and would just send up with a bunch of extra unnecessary code checking if certain uniforms were already defined or not.
So I don't quite understand how the hooking would be any different than the way I currently did it, both just seem like different way to put code between the param reads and lighting, but hooking seems like it would just require more code to get it to work by checking the #ifndef before everyuniform in PBRLighting.frag, and would also add more defines for each hook (which could be problematic currently since the current version of jme limits each shader to 64 defines until another PR fixing that issue is merged to stable, so I always typically avoid creating new defines as much as possible)
I still don't see the issue with just defining every uniform and variable that has potential to be re-used in the .frag shader. Then as long as the user puts the import for their glsllib after the variable declarations in PBRLighting.frag (which already is required in some cases even without this PR), then they will never have to worry about double declarations.
It seems like this would be a lot of work just to protect a potentially careless shader dev from accidentally declaring a variable twice, which IMO isn't a big worry becuase in the case that a glsllib does try to declare an already existent uniform, the app will just crash and tell them which variable is already declared, then it should only take a minute to go back into their glsllib and remove the uniform that is already declared in pbrlighting.frag, or add the #nifdef to their glsllib (rather than putting those not-defined checks all in PBRLighting.frag). So it sort of feels like this would a lot of extra code in PBRLighting.frag just to add guard-rails to prevent a mistake that is easy to fix.
So unless I'm still overlooking something, it feels like the way I have it set up is just simpler and will do the same thing with less code. I also don't know if there would be any benefit to putting more hooks in more places (aside from between the lighting and param reads), since there should almost never be any reason to change the way that the base PBR textures and params are read, otherwise you'd be breaking PBRLighting and it really wouldn't be a fork of PBRLighting anymore.
My thoughts were just to put the param reads and lighting into 2 big glsllibs since the code in those libs should almost never change for a true fork of PBRLighting, and the lighting code would only change if you switch to a different lighting method at which point you can just use a different glsllib for the lighting part. I think anything further than that needlessly overcomplicates it and will juts make existing forks require even more refactoring to get up to date with the changes to use the hooking method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eg. keep PBRLighting.frag as a single file (maybe reorganized a bit as you did)
This would also defeat the purpose of this PR since I also plan to reuse the PBRLigthing.glsllib for the lighting calculations in PBRTerrain.frag and AdvancedPBRTerrain.frag in the jme-terrain library. So I will also be refactoring both pbr terrain shaders in jme-terrain to work with this PR and share a lighting .glsllib.
And that was where the idea originally came idea too because jhonkkk was adapting PBRLighting + both terrain shaders to his deferred rendering, and he noticed they all share the same lighting code and wanted to be able to make changes in one single glsllib (rather than changing all 3 .frag shaders) to develop all 3 easier and ensure they were consistent.
There would also be nothing stopping a jme dev from making a fork where they put the code from the 2 glsllibs back into one mega .frag shader if that is necessary for their fork somehow or if it feels easier for them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- SinglePassLightBatchSize = 64, you have 32 visible lights in your scene - > scene is rendered in 1 pass
Hmm this still confuses me based on what I thought I'd been told in the past and especially in regards to my own findings when trying to use even a few point lights. It sounds like (based on what you're saying) that I should be able to use PBRLighting with up to 64 lights without having framerate issues since its all in one pass for up to 64 lights, but I can barely even use 2 point lights with in a moderate sized scene without the framerate dropping drastically.
Pretty much all my scenes (even ones with just a few triplanar terrains and no other models) can't render even 1-3 PointLights without destroying the framerate because (I thought) that every additional light was causing it to render the terrain and do all the triplanar texture reads again for each light.
Is that not correct? And if so then why am I getting such bad lag with only 1-3 point lights if it is <64 and supposedly all done in one pass? Is it something else causing the lights to lag then?
I said 64 as an example, singlePassLightBatchSize is actually 1 by default, you should set it with renderManager.setSinglePassLightBatchSize(int)
to a value that make sense for your scene.
I also am not quite sure if I understand the hooking concept or the benefit it would add. It seems more confusing to me than if I were to just call a method from a glsllib between the param reads and lighting calculation and (unless I'm misunderstanding) it seems like the hooking could end up in a situation where there is conflicting variables since PBRLighting.frag is imported after the custom code. I would expect the hooking example code you pasted to have more issues with conflicting variables since the PBRLighting.frag import is declared after defining the hook (so the forked shader won't know about any of the variables in PBRLighting.frag, so then PBRLighting would need all the not-defined checks and would just send up with a bunch of extra unnecessary code checking if certain uniforms were already defined or not.
So I don't quite understand how the hooking would be any different than the way I currently did it, both just seem like different way to put code between the param reads and lighting, but hooking seems like it would just require more code to get it to work by checking the #ifndef before everyuniform in PBRLighting.frag, and would also add more defines for each hook (which could be problematic currently since the current version of jme limits each shader to 64 defines until another PR fixing that issue is merged to stable, so I always typically avoid creating new defines as much as possible)
I still don't see the issue with just defining every uniform and variable that has potential to be re-used in the .frag shader. Then as long as the user puts the import for their glsllib after the variable declarations in PBRLighting.frag (which already is required in some cases even without this PR), then they will never have to worry about double declarations.
It seems like this would be a lot of work just to protect a potentially careless shader dev from accidentally declaring a variable twice, which IMO isn't a big worry becuase in the case that a glsllib does try to declare an already existent uniform, the app will just crash and tell them which variable is already declared, then it should only take a minute to go back into their glsllib and remove the uniform that is already declared in pbrlighting.frag, or add the #nifdef to their glsllib (rather than putting those not-defined checks all in PBRLighting.frag). So it sort of feels like this would a lot of extra code in PBRLighting.frag just to add guard-rails to prevent a mistake that is easy to fix.
So unless I'm still overlooking something, it feels like the way I have it set up is just simpler and will do the same thing with less code. I also don't know if there would be any benefit to putting more hooks in more places (aside from between the lighting and param reads), since there should almost never be any reason to change the way that the base PBR textures and params are read, otherwise you'd be breaking PBRLighting and it really wouldn't be a fork of PBRLighting anymore.
My thoughts were just to put the param reads and lighting into 2 big glsllibs since the code in those libs should almost never change for a true fork of PBRLighting, and the lighting code would only change if you switch to a different lighting method at which point you can just use a different glsllib for the lighting part. I think anything further than that needlessly overcomplicates it and will juts make existing forks require even more refactoring to get up to date with the changes to use the hooking method.
Eg. keep PBRLighting.frag as a single file (maybe reorganized a bit as you did)
This would also defeat the purpose of this PR since I also plan to reuse the PBRLigthing.glsllib for the lighting calculations in PBRTerrain.frag and AdvancedPBRTerrain.frag in the jme-terrain library. So I will also be refactoring both pbr terrain shaders in jme-terrain to work with this PR and share a lighting .glsllib.
And that was where the idea originally came idea too because jhonkkk was adapting PBRLighting + both terrain shaders to his deferred rendering, and he noticed they all share the same lighting code and wanted to be able to make changes in one single glsllib (rather than changing all 3 .frag shaders) to develop all 3 easier and ensure they were consistent.
There would also be nothing stopping a jme dev from making a fork where they put the code from the 2 glsllibs back into one mega .frag shader if that is necessary for their fork somehow or if it feels easier for them.
Ok, i'll think more about this use case, can you show (even in pseudocode) how PBRTerrain can be refactored with this PR? Do you need to include the two glsllib before and after the terrain code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PBRTerrain would have its texture reads moved to another .glsllib called something like PBRTerrainParmReader, and that would also be reusable by another lighting implementation such as the one that jhonkkk made based on PBRTerrain:
So once all the refactoring is done, his PBRTerrain shader's main function would look like:
#import "Common/ShaderLib/PBRTerrainParamsReader.glsllib"
#import "Common/ShaderLib/GBufferPackLighting.glsllib"
main(){
readPBRTerrainParams():
calculateGBufferPackLightingStuff();
}
and the original PBRTerrain shader currently in jme-terrain would use the same texture-reading .glsllib, but would use the calculateFinaLightingValue() method that is also in PBRLighting.frag. So PBRterrain's main funciton would look like:
#import "Common/ShaderLib/PBRTerrainParamsReader.glsllib"
#import "Common/ShaderLib/PBRLighting.glsllib"
...
readPBRTerrainParams();
vec4 finalLightingValue = calculateFinalLightingValue();
and PBRLighting.frag would look like:
#import "Common/ShaderLib/PBRLightingParamsReader.glsllib"
#import "Common/ShaderLib/PBRLighting.glsllib"
...
readPBRParams();
vec4 finalLightingValue = calculateFinalLightingValue();
and jhonkkk's gbufferpack implementation (PBRLightingGBufferPack.frag) would look like:
#import "Common/ShaderLib/PBRLightingParamsReader.glsllib"
#import "Common/ShaderLib/GBufferPackLighting.glsllib"
...
readPBRParams();
calculateGBufferPackLightingStuff();
So PBRligthing, PBRTerrain, and AdvacnedPBRTerrain will all use the lighting code from PBRLighting.glsllib.
And then another lighting implementation (such as the gbufferpack-based one from jhonkkk that I linked) would re-use the glsllibs for reading params, but would all share a different GBufferPackLighting.glslib file.
So if I had done this PR earlier, then this would have drastically reduced the work that someone such as jhonkkk would've had to do when he had to copy/paste all the param-reading code from PBRLighting. PBRTerrain, and AdvancedPBRTerrain ino his own GBufferpack implementations and update all the lighting code in each of the 3 different shaders everytime he made a change. It also soudned like the cluttered param-reading code may have caused him some unecessary confusion early on, before he realized it was all identical and mostly irrelevent to the lighting code (and was also intermingled in a way that made it hard to work with), which is when he suggested splitting it into .glsllibs like this.
Also here is a link to a .frag shader that I forked from PBRLighting.frag that already adapted the changes in this PR:
https://github.com/yaRnMcDonuts/jme-shader-pack/blob/372e4545d649b961b804c6104df290887cdec4c2/src/main/resources/Shaders/PBRCharacters.frag#L72
It just calls the method for reading params, then the method for lighting. And the forked code goes in a method between the two. And all the reusable uniforms and variables get declared in the .frag file so I don't end up having issues re-declare anything in any of the glsllibs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I said 64 as an example, singlePassLightBatchSize is actually 1 by default, you should set it with
renderManager.setSinglePassLightBatchSize(int)
to a value that make sense for your scene.
I never knew that was something that could be increased with the current pipeline, thanks! I'll have to try that and hopefully I can start using some point lights again in my game.
Well I still have a few minor changes to make to this PR, however I am going to wait to do any more work on this PR until I am sure it is going to be accepted... Based on the discussion in the issue that spurred this PR (as well as some discussion in a jme thread where with jhonkkk where he suggested I do this) I thought that there would be no issue passing a PR like this. I understand there are ways to modularize things even more, and this isn't the only way. Anyone else who wants to further modularize these shaders is welcome to do so in addition to this PR. But I personally think these first steps I've taken to refactor PBRLighting are very basic and it seem like it should be common sense to implement such changes to reduce all of the duplicate code across the PBR and Terrain shaders.... this will make JME's stock shaders so much easier to maintain on core, and will also make them easier to extend for other rendering pipeline that want to use JME's current PBR implementation as a foundation (like how jhonkkk did). Also @riccardobl I understand you are very skilled with shaders, but it also seems like you have your own versions of a PBR shader and a PBR terrain that do not match the ones on core and your shaders have deviated quite far from the ones on core, so I worry that you will not see the benefits of this PR as much as someone like myself who bases all of their shaders off of the ones on core. I have used and forked jme's stock PBRLighting and PBRTerrain shaders more than anyone else in this community probably, so I find it discouraging that I am encountering resistance on a PR like this. |
Does it make sense to suggest 2 side-by-side versions of PBRLighting? your way and the current way and let people decide which technique fits them better? or it will make everything more complicated? |
I see the issue you are raising and i think this solves it to some extent, my worry is if this can be future proof. |
I am sorry you feel discouraged, that's not my intention, i've also did a lot of experimentation with jme shaders and shaders in general and i've found that debugging shaders is very very hard and it is better to keep them as straightforward as possible, even if they look ugly and are monolithic. |
The only issue with having 2 versions is that the code/functionality would be identical so anytime someone updates PBRLighting they'd have to make the same changes in two placed. So we'd be maintaining 2 different shaders that are funciontallly identical which seems like unnecessary extra work, and wouldn't be a good permanent solution in my opinion. This would potentially an acceptable short-term solution, that way we could ensure there are no issues with the refactored version of PBRLighting, PBRTerrain, and AdvancedPBRTerrain prior to replacing them. But having 2 different versions of an identical shader in the long term seems like it would defeat part of the purpose of this PR, since the goal is to make it easier to maintain those. For example, both PBRTerrain shaders on master are currently a few PRs behind PBRLighting (missing things like SpecularAA) so right now I will have to go and copy/paste those changes to both shaders. And if we create two versions of each, then that would actually increase the work and I'd have to copy those changes to 4 places now. This is the issue I currently have with my 8-9 forks of PBRLighting that all do the same param-reads and lighting, but have just a few changes between those 2 functions. My shader dev has been 100x less head-ache inducing since I no longer have to keep track of identical code in 8 different places.
I think the new version in this PR is much more straightforward, considering i de-coupled some lighting and param-reading code that was previously intermingled in PBRLighting, and separated the shader into 2 logical pieces (param reads and lighting). As long as a shader dev knows how .glsllibs work then it should be fairly easy to know which of the 2 new glsllibs you need to open up when an error points to code in those glsllibs. And this issue already exists considering PBR.glsllib already outsources some important lighting code to a glsllib, so I don't understand what is the fear in organizing some more reusable code into glsllibs like this.
Yes this was my intention to make these 2 .glsllibs un-changing, and they are always meant to be used with PBRLighting.frag or a fork of it. There are tons of things in PBRLighting.frag currently that should never be changed, but could easily be changed by mistake when someone forks it. For example there is only 1 right way to read an albedo/normal/metallic roughness map for a PBR material. No one should be trying to fork a shader to change the base param reads susbtantially, and if they do so then they are no longer workign with a true fork of PBRLighting, and they would likely be working with a broken shader and probably don't know what they are doing and would have issues whether the code is in .glsllibs or in one big mega shader. The only time anyone would ever change either of these .glsllibs is when someone makes a new PR that would've previously been added to PBRLighting.frag and PBRTerrain (such as the recent SpecularAA PR). I do not expect anyone to ever change these files when they try to make a fork of the current PBRLighting shaderr on master. If someone wants to make a new PBR shader with a new lighting technique, then they would be able to re-use PBRParamReader but would write their own code for the lighting, and they could even pack that into a glsllib so it can be reused by other shaders (like terrains) that share the new lighting technique. I keep mentioning JhonKKK's abandoned branch because that was the best example and is why I did this PR in the first place. He wanted to make his new lighting system work so that jme devs could re-use all of their existent PBR materials with his new system simply by changing the .j3md used by your existing materials. This meant that his new shader had to have all of the same MatParams as PBRLighting, thus why I packed those all into a glsllib so that they can be reused and won't have to continually be copy/pasted into new shaders, which would result in a ton of identical code across multiple forks which makes a ton of extra work trying to maintain them all. |
This would only happen if we mistakenly let someone submit a bad PR to the glsllib for reading PBRParamaters and that would also break PBRLighting.frag on master, so I don't expect this to ever happen as long as we treat the new glsllibs the same way we treat the current mega shader. That glsllib is exclusivley intended to work with PBRLighting.frag and true-forks of PBRLighting.frag. The only other reason PBRParamReader.glsllib would break an existing fork is if that fork is not a true-fork of PBRLighting, in which case they should not have tried reusing the PBRParamReader for their shader in the first place. That would be like if a jme dev was trying to use the Param reads from Lighting.frag with the lighting equation from PBRLighting.frag and then wonder why they don't have a roughness/metallic value.... at that point there's nothing we can do to help someone if they are going to try and mis-match shader code in such an incorrect way. I understand the desire to make things fool-proof, but I also am confused because typically jme core devs say the exact opposite, and I've often been told that "jme is a developer's engine" and that we should not sacrifice functionality and modularity just to make things more bug-proof and easier for an unskilled developer. If this makes things a tiny bit more confusing for shader devs that don't like working with .glsllibs and ensuring that their .frag is set up parallell to PBRLighting.frag, well than I think that's an acceptable sacrifice to make. But if we decide to not implement my changes, then we are making it harder for an experienced shader dev to maintain PBRLighting, PBRTerrain, and AdvancedPBRTerrain (as well as the 3 other GBufferPack version of these shaders that share param-reading but use a different lighting .glsllib) just so that other less skilled shader devs don't get as confused having to work with glsllibs instead of working in one mega shader. And I'd argue my solution is actually more fool-proof because an unskilled shader dev should not be trying to change things in the lighting or paramater reading equation anyways. 99% of forks of PBRLighting do not change the param reads or the lighting equation, they just alter the values like albedo/normal/rouhngess after the param reads but before the lighting. So by putting that static param reading and lighting code into glsllibs, the chance of a dev breaking something important is actually drastically reduced and the shader becomes way easier to debug when the forker's code is not intermingled with the code they should never be changing. |
I also think I need to mention one more thing for context, to ensure we are on the same page: When I say "true fork of PBRLighting" I am talking about a fork that uses the same param reads and same lighting code as the version of PBRLighting on master. I do not consider the "PBRLightingGBufferPack" shader by jhonkkk or your personal versions of a deferred PBR shaderr to be a true-fork of PBRLighting.j3md. Those are their own shaders with their own lighting techniques. They might be able to share the same param reads (as is the case with JhonKKK's GBufferPack shaders), but since it does lighting in a drastically different way it is not really a fork of PBRLighting anymore (it is a new vesion of PBRLighting with new lighting code), and I do not expect these .glsllibs to be usable by anyone who deviates too far from the official version of PBRLighting.j3md. This PR is specifically aimed at making the current PBR shaders on master (PBRLighting, PBRTerrain, AdvanceDPBRTerrain) easier to maintain and easier to fork for people who want to use our current rendering pipeline. And the fact that the param reads are able to be reused by other pipelines in the future (which JhonKKK's implementation appears to have proven is possible) will make the job easier for him or the next person to pick up where he left off. He could just plug the params returned from PBRParamReader into his own lighting code and save himself the trouble of copy/pasting identical code to multiple frag files. And if he (or anyone else) really needed to make changes to the paramReader file in order for it to work with the new shader, then they could simply fork PBRLightingParamReader.glsllib and create a slighlty altered version called GBufferPackPBRLightingParamReader and then all his GBufferPack shaders could use that while all the old PBRLighting shaders would use the one I made. But that would be unlikely because PBR shaders will almost always all use the same base textures and params, even for different lighting techniques. The code in PBRLighting.glsllib is only the code that is likely to deviate and require new .glsllibs for each new rendering pieplline. And then at that point we could still go back and modularize the lighting code that is shared by multiple pipelines too, as you suggested in an earlier post. But since we only have 1 pipeline as of now, that is not a worry for me currently. I'll cross that bridge when/if we ever get another rendering pipeline. |
My intention was to suggest some kind of deprecation process for the current shaders so we can safely remove them in future releases. not to keep updating both of them. Also, since this is actually an interface change I guess we should update the documentation with explanation about using those shaders with the new lib, extending it etc. I see that @riccardobl is not opposing the PR and personally I think it's a step in the correct direction (engineering wise) so I tend to integrate it but we need to make sure we have the updated docs. |
Hi @yaRnMcDonuts, Thanks :) |
Yes I was planning to continue working on this and was going to try to have it ready in time for the next release if possible. But @riccardobl messaged me to discuss the PR, and he had said he would work on a new struct based implementation of this PR that includes the important aspects of my PR with his struct approach. So hopefully he can comment and let you know the current status. |
OK so if I understand correctly, @riccardobl will provide a new PR based on this PR and his structs approach? If so, can we close this PR as soon as he provides the new one? |
The draft is here: https://github.com/riccardobl/jmonkeyengine/tree/modularpbr |
@riccardobl Yeah that sounds good to me |
Co-authored-by: Ryan McDonough <[email protected]>
Hi @riccardobl , |
I don't have the push permissions, i've asked @yaRnMcDonuts for them, maybe worth pinging him here too |
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? |
That worked, thanks @yaRnMcDonuts |
I've pushed the changes, the code needs to be fully tested. Struct based modularityThe idea behind this design is to use mutable structs to create data containers that can be passed to functions. 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
|
Hi @riccardobl , WDYT? |
We need to test it and see if the implementation is good enough or if things need to be abstracted further. |
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. |
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:
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. |
i think we need to exclude
on GLSL < 130 in glslcompat |
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.