Modularize PBRLighting.frag

[yaRnMcDonuts]

Addresses this issue: https://github.com/jMonkeyEngine/jmonkeyengine/issues/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]

The concern of code duplication is valid, but this change makes the shaders even more spaghetti coded. I think a better approach would be to keep the uniforms in the main shader and make all the functions work with well defined structs (eg. Light, Surface ...) , but generally speaking i would be reluctant to change well tested core code like this one, instead this could be a glsllib available for new shaders. Also, there are additions that seem unrelated or maybe have very opinionated names eg. indoorSunLightExposure , afaik there is no concept of indoor in this shader.

[stephengold]

@scenemax3d I have limited experience writing shaders and don't fully understand what this PR is doing, so I'd rather not make judgments regarding it.

[yaRnMcDonuts]

but this change makes the shaders even more spaghetti coded.

I would have to adamantly disagree with this in almost every way possible.

Of course there are things I can still do to improve the organization of my PR, however I cannot believe that my PR is in any way more spagheti-coded than the previous PBRLighting.frag implementation, as that was extremely disorganized and needlessly difficult to make and maintain forks of.

The previous .frag shader was a jumbled mess with lighting-related code and paramter reads all mixed together, making it extremely annoying to write a fork that alters basic values that were previously spread accross the whole shader with some lighting-related calculations spread in between.

Now it is organized so that any forks will 99% of the time be putting their code between the parmReads() method and the lightingCalculation() method I've added, so now no one will ever have to go and untangle the jumbled mess when they try ti make a fork of PBRLighting.

Want to do simple splatting by blending to another albedo/normal/roughness/etc value in the old PBR shader? Well then you'd have to go down 100 lines to do the normal mapping after all the code putting the normal map in tangent space, then another 150 lines to do the roughness/metallic blending, then you had to go alll the way to the bottom of the shader (past the spec-gloss params and some lighting related stuff) to do the ao blending because for some reason AO got placed really far down, and at that point you now have to scroll up/down a huge file to keep track of code that should've been all in the same place. And with this PR, all the code that someone would add when forking this shader will 99% of the time fit simply between the paramReads and the lightincCalculation functions I've reorganized everything into.

Also, there are additions that seem unrelated or maybe have very opinionated names eg. indoorSunLightExposure , afaik there is no concept of indoor in this shader.

I added some code to do indoor-lighting using the red channel of the vertex colors as an AO channel for only the DirectionalLight (I have videos to show it works if that is a concern). I did this to keep PBRLighting consistent with the official PBR terrain shader I added, since they will both use the same lighting glsllib. And this indoor lighting has been in my pbr terrain since I first contributed that shader. Also @JohnLKkk added this feature to his since-discontinued rendering pipeline and had plans to integrate it there, so I was also trying to stay consistent with the shaders being used for his PR.

So maybe that would be better renamed from InddorSunlightExposure to instead be something like DirecionalLightOcclusion?

[yaRnMcDonuts]

I think a better approach would be to keep the uniforms in the main shader

I could do this, but the general idea of this PR was to move as much solidified-code as possible out of the PBRLighting.frag file and into a glsllib, so that next time someone adds a PR changing the texture reads or lighting calculatons for PBR, anyone using that glsllib will only have to add the matParams to the .jmd and will not have to go and make a ton of changes in different places in the fragment sahder too.

For example, I have 6 forks of PBRLighting that all do something unique. So if I want to include the latest PRs from master that add AoStrength and SpecularAA, I have to go and put them in the right spot in every jumbled fragment shader, add al the uniforms to every fragment shader, and then also add all the params and defines to the .j3md.

With this PR, I would only have to add the params and defines for AoStrength and specularAA to the .j3md files, and would not have to change any of my forked .frag shaders since all the uniforms and important code for AOStrength and SpecularAA are in the .glsllib that every fork (and master) all use.

[yaRnMcDonuts]

but generally speaking i would be reluctant to change well tested core code like this one, instead this could be a glsllib available for new shaders.

Having 2 identically functioning PBRLighting shaders in master sounds like a recipe for disaster and will just make things even more convoluted and confusing as far as keeping forks up to date with master.

This would mean that any new PRs to PBRLighting.frag on mater (like AoStrength or SpecualrAA, to conitnue with the examples I've been using) would also need implemented into the 2 glsllibs I've added (and vice versa), which would defeat the purpose of this PR that aims to make maintaining multiple forks of PBRLighting easier.

[riccardobl]

I would have to adamantly disagree with this in almost every way possible.

Maybe i should elaborate on my previous point, i have nothing against the PBRLightingParamsReader that is closely related to PBRLighting, as you said, this can simplify the creation of "forks" of PBRLighting.

My argument is against PBRLighting.glsllib , since it declares some uniforms closely related to PBRLighting.frag implementation (or ibl technique) but also some more generic PBR code that could be used by shaders not necessarily forking PBRLighting.

IMO all the uniforms and macro declaration should be relegated to PBRLightingParamsReader and the generic code in PBRLighting.glsllib should be extracted and refactored into self contained functions that do not access uniforms directly from the function body, the resulting library should be self contained and usable also from new shaders.

I am opening a review to point exactly to the changes i am talking about

---

Maybe outside the scope of this pr, since it is a big refactoring of the pbr logic, but this was my solution: reimplementing everything as functions that receive structs as input and outputs new structs If you are interested, you can see the code here: https://gist.github.com/riccardobl/373028cd15c6c2ccca83889baeccce61 (this is partially based/inspired by jme pbrs).

[yaRnMcDonuts]

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.

[scenemax3d]

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?

[riccardobl]

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.

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 see in your example that implementations are supposed to freely include readPBRParams or calculateFinalLightingValue function, or reimplement them if needed. This will work only if we mandate that the behavior of readPBRParams and calculateFinalLightingValue will never ever be changed, otherwise a non backward compatible change in readPBRParams might break unexpectedly all the shaders depending on it, i think this might end up adding a lot of confusion and require multiple implementation of readPBRParams and calculateFinalLightingValue going forward.

[riccardobl]

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.

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. That said, i am not opposed to this PR in principle

[yaRnMcDonuts]

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?

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've found that debugging shaders is very very hard and it is better to keep them as straightforward as possible,

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.

This will work only if we mandate that the behavior of readPBRParams and calculateFinalLightingValue will never ever be changed,

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.

[yaRnMcDonuts]

otherwise a non backward compatible change in readPBRParams might break unexpectedly all the shaders depending on it,

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.

[yaRnMcDonuts]

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.

[scenemax3d]

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

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.

[scenemax3d]

Hi @yaRnMcDonuts, Following the discussion in the forum, are you going to update this PR with the improvement suggested by @riccardobl ? Can you share your plans for this PR?

Thanks :)

[yaRnMcDonuts]

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.

[scenemax3d]

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?

[riccardobl]

The draft is here: https://github.com/riccardobl/jmonkeyengine/tree/modularpbr If @yaRnMcDonuts is ok with it, i can push to this pr branch

[yaRnMcDonuts]

@riccardobl Yeah that sounds good to me

[scenemax3d]

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

[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.