Extensible structs as shader pre-preprocessor directive

User Code & Projects
1

I am writing a pre-preprocessor directive for our shader loader that would add to the two we have already ( #import and #for ) and i’d like to hear your opinions on it.

The purpose is to create a struct that is based on another struct but adds some fields, you can see it as a very limited class extends.

The implementation is a as follow:

#struct MyStruct1
    float a;
    float b;
#endstruct

#struct MyStruct2 
   float c;
#endstruct

#struct MyStruct3 extends MyStruct1, MyStruct2
   float d;
#endstruct

The jme shader loader will translate it to:

#define STRUCT_MyStruct1 \
    float a;             \
    float b;

struct MyStruct1 {
    STRUCT_MyStruct1
};

#define STRUCT_MyStruct2 \
    float c;              

struct MyStruct2 {
    STRUCT_MyStruct2
};

#define STRUCT_MyStruct3      \
    STRUCT_MyStruct1               \
    STRUCT_MyStruct2               \
    float d;

struct MyStruct3 {
    STRUCT_MyStruct3
};

And then the glsl preprocessor will do the rest.

The end result is that you will have a MyStruct3 that has all the fields of MyStruct1 and MyStruct2.

Usage example

#struct MyStruct1
    float a;
    float b;
#endstruct

#struct MyStruct2 
   float c;
#endstruct

#struct MyStruct3 extends MyStruct1, MyStruct2
   float d;
#endstruct

void main(){
  MyStruct3 s;
  s.a=1.;
  s.b=1.;
  s.c=1.;
  s.d=1.;
}

The goal is to use it for a struct based implementation of #2191 to allow shaders to extend and redefine the default base structs if they need to store more data.

7 Likes
2

This sounds like a good idea to me. And using a struct like this for refactoring things in #2191 should be better than in/outting every parameter the way I was initially doing it.

1 Like
3

There is one potential issue that was pointed out to me after a recent review from @codex.

This new custom pre-processor structs will slightly increase the compile time of shaders.

I have yet to profile this to see how significant it is, although I assume it is very small, likely a negligible amount.

However this amount would also increase with the number of shaders using the new structs, and in my use-case I have many (~8 currently) forks of PBRLighting, each for niche special effects for a single pbr model, and will likely have more in the future, so I do think it is worthwhile for me to profile it and consider if the extra compile time is worth it, especially since this new struct functionality does not improve anything on the player’s end, and is solely a change to the way that shaders are developed in jme. Even a .1 second load time increase feels like it would be hard for me to justify when there are no perceivable benefits from the player’s perspective.

But I am curious to hear more input from the community as to whether or not you like these changes to PBRLighting. The end goal would be to replace the current PBRLighting shader with this modularized version in which case you would need to learn the new struct system in order to fork pbr lighting in the future, so I think it is important to hear more input on this topic.

4

I think modular PBR is still a really good idea. It will enable fixes and features to proliferate much faster and easier to forks of PBRLighting, including all the terrain shaders.

If the increased compile time does turn out to be a problem, we could probably optimize something about the compilation process to account for it. Maybe a tool can be developed that will apply all the defines that are explicitly set in the shader (as many of your defines are) so that developers can optimize their shaders for deployment if they want to. That would completely nullify whatever performance hits that occur from your changes from the player’s perspective.

So yeah, I’m in favor of moving forward with the PR.

1 Like
5

I don’t think the compile time is a concern here, jme caches compiled shaders, so it will be just something that concerns the first frame the material is ever rendered or preloaded.

1 Like
6

Or whenever a define-linked material parameter is changed. (Edit: or is this done before the #define substitution?)

If this is a "makes developer’s life easier’ feature then perhaps it can be an “asset generation time” thing and not a “expand the shader at runtime” thing. But I haven’t looked into it.

Seems a shame not to just fix shader nodes so that it is easier to do in code.

1 Like
7

I’m not against that idea, and hopefully any future attempt to implement shader nodes will be easier after this PR, since it splits up all the different parts of PBRLighting into reusable functions in a way that will be useful to a shader node system.

But I just never caught on to using shader nodes myself, and am already committed to maintaining a set of forked pbr shaders created from copy/pasting and changing the original.

So the main goal of my PR is just to reduce redundant code between the pbr terrain and main pbr shaders and move it all to functions in .glsl libs so they are easier to maintain and reuse, and then any further modularization (like the struct approach in the PR) can be built on top of that.

But a shader node system certainly is nice for an engine to have too, so if anyone else is ever interested in working on shader nodes I’d still encourage them to do so.

3 Likes
8

The last issue to discuss in regards to my PR before I am ready for it to be merged:

(I mentioned this in the PR on github but have received no response yet, so I will re-summarize here)

My changes were originally made to PBRLighting.frag, however the PR was added onto by @RiccardoBlb and the code was moved to a new version of PBRLighting in a “modular/” directory, resulting in 2 functionally identical copies of PBRLighting.frag.

I do not agree with this decision, as it defeats the whole point of reducing redundant code, and will require future changes to be made in more than 2 frag files (possibly 6 different files if I am forced to do the same thing with the terrain shaders…yikes… :sweat_smile:)

I can already see a future where these modular shaders get forgotten about and fall behind the current PBR shader, and then we end up with more cluttery and un-maintained shade code in the engine.

So before I make the final changes to this PR, I neeed to make sure that I have approval for this to replace the current PBRLighting shader.

Otherwise the whole point of the PR is kind of pointless imo. The struct approach and extra modularization are nice and I’m glad to include that in my PR, but I really only made this PR to reduce redundant and identical shader code between files, not to increase it.

2 Likes
9

I guess I will take the lack of response from riccardo (or any other official jme leader) as a “no” in regards to my question:

This means that I unfortunately will no longer be doing bug fixes or adding anything new to the official jme PBR terrain shaders that I had previously planned, because it will just be too much unnecessary work trying to keep the pbr terrains on master up to date with my own modular one after I finally switch over to using glsllib in my own project.

10

I think the lack of response simply means that nobody’s in charge at the moment. If you really want to add a major feature to the Engine, perhaps you should volunteer to be a release manager.

2 Likes
11

I also found this a bit off-putting but the way contributing to JME usually goes is that PRs mostly hang around unmerged until a release period. And then and only then are PRs seriously considered for merge

1 Like
12

I had just assumed someone was already handling the role since there was discussion about the next release in another recent thread, but if the spot is empty and someone is needed I’d be glad to volunteer.

With that said, my intention here hasn’t been to rush anyone into putting this PR into an official release, I mostly just wanted this PR approved and merged to master so the modular structure could be finalized before I started working on 2 other dependent PRs (for a pbr terrain bug fix and optimizations for far terrain rendering)

But I will have more time than usual to work on my JME project for the next few months, so if a release manager is needed then I’d be willing to fill that role for a while. However I will probably need some guidance, at least to get started.

6 Likes
13

I believe a release manager is needed.
I can guide you, just as I guided @adi.barda in v3.7.0-stable.
If I don’t hear any objections or alternatives in a few days, I’ll make the announcement and send you a PM to get the process started.

4 Likes
14

@yaRnMcDonuts, sgold - best guide you can get. Also in the “money time” Riccardo answers your engine related questions if you have doubts regarding any of the PRs. We also have excellent documentation about the release process.
Good luck and thank you!

3 Likes