A framegraph framework that supports 3 render paths

[JohnLKkk]

see:https://hub.jmonkeyengine.org/t/enhancements-to-the-jme3-rendering-engine/47145/27

Enhance the capabilities of the JME3.7 renderer, mainly including:

1. Introduce a basic framegraph framework for managing renderPasses, while modularizing the existing rendering modules into several core Passes for reuse;
2. Added support for render paths, including Forward (default render path), Deferred (a standard DeferredShading), and TileBasedDeferred (Tile-optimized DeferredShading). They should be compatible with most existing modules.
3. Added some code demos for RenderPath.

Regarding render paths, please refer to: https://docs.unity3d.com/Manual/RenderingPaths.html

Performance considerations

The rendering overhead of real-time lights in deferred shading is proportional to the number of pixels illuminated by the light and not dependent on Scene complexity. So small Point Lights or Spot Lights are very cheap to render and if they are fully or partially occluded by Scene GameObjects then they are even cheaper. Of course, lights with shadows are much more expensive than lights without shadows. In deferred shading, shadow-casting GameObjects still need to be rendered once or more for each shadow-casting light. Furthermore, the lighting shader that applies shadows has a higher rendering overhead than the one used when shadows are disabled.

GBuffer format:

RT0, RGBA16F: Pack DiffuseColor, Ao, Alpha RT1, RGBA16F: Pack SpecularColor, FZero, Roughness RT2, RGBA16F: Pack ShadingModelId, emissiveColor RT3, RGBA32F: Pack Normal (contains Normal with TBN transform and modelNormal, using cubemap mapping) RT4, depth: Pack depth

LightPack:

There are two ways to package lightData, one of which is to use a uniform array, which should be faster, but its quantity is limited, and a large number of light sources will be drawn and accumulated through several passes to complete all light sources; Another way is to package all light source information through three texture1D, which may have some performance overhead, but supports a large amount of light source data;

Tile-Based DeferredShading:

TileLightDecode: Packs tile information, x indicates the first light source offset used by the tile (for lookup in TileLightIndex), y indicates how many lights are associated with the tile (currently no limit, i.e. unlimited number of lights), z indicates the uv coordinate offset when sampling TileLightIndex for the tile; TileLightIndex: Packs light source ids for each tile

UseRenderPath:

Enable the corresponding renderPath in the following ways： renderManager.setCurMaxDeferredShadingLightNum(1000);// Pre-allocate a maximum value for light sources to ensure the maximum number of light sources in the scene does not exceed this value. renderManager.setRenderPath(RenderManager.RenderPath.Deferred);

UseFramegraph(Still in development...):

Includes a set of FG public APIs (current internal implementation details may not be perfect, but usable interfaces will be finalized in this PR) Enable the corresponding framegraph in the following ways： renderManager.enableFramegraph(true);

[stephengold]

How about we mark this PR as a draft until it's ready for final review?

[JohnLKkk]

How about we mark this PR as a draft until it's ready for final review?

I think it's possible, however advancing this might require some extra work:

1. Should testing be done and related bugs reported and fixed (although I personally tested my local examples code, others' situations might differ)?
2. Should this PR be a "1.0 version to enhance renderer capabilities" instead of a one-step perfect version? (I think this PR is not perfect now because the render path is not thoroughly optimized, however, theoretically when adding a new feature, it should first be made compatible with existing architecture and run stably, then do 2.0 or 3.0 iteration and optimization work afterwards)
3. I may not include global illumination content in this PR, because I think the subject deserves its own new PR created specifically for it.

[stephengold]

I think this PR should be marked as a draft. Marking in-progress PRs as a drafts reduces the mental load on integrators and prevents premature integration.

Should testing be done and related bugs reported and fixed (although I personally tested my local examples code, others' situations might differ)?

Testing and bug reporting can and should continue while the PR is a draft.

Should this PR be a "1.0 version to enhance renderer capabilities" instead of a one-step perfect version?

Your decision; we can go either way.

I may not include global illumination content in this PR, because I think the subject deserves its own new PR created specifically for it.

I'm fine with that.

[stephengold]

Each Java sourcefile you add to the repo should have the full JME license at the top of the file, with the current year for the copyright date.

[JohnLKkk]

Each Java sourcefile you add to the repo should have the full JME license at the top of the file, with the current year for the copyright date.

I think I will make time to modify each Java file according to the engine source code conventions.

[JohnLKkk]

I don't understand a lot about rendering, so I probably won't catch any major issues. Hopefully this review will be helpful anyway. 😉 I've refrained from mentioning javadoc and basic style issues, since it sounds like you'll be working on that soon.

Thank you for your valuable suggestions. I will refer to most of the suggestions and only need to make corresponding modifications and adjustments.

[JohnLKkk]

Each Java sourcefile you add to the repo should have the full JME license at the top of the file, with the current year for the copyright date.

I have just supplemented a large number of core comments as well as complete JME license information in each file.

[zzuegg]

I am not happy with the material handling. I know there are some upsides when using a separate technique, but having all the shader code twice is going to be a nightmare to maintain/extend.

My proposal would be adding a define "GBUFFER_WRITE" to the default techniques. you can set the define to 1 using MatParamOverrrides

Inside the shaders then a simple #ifdef #else can be used to write to data to gbuffer, or continue with the default lighting code.

But i would like to discuss this, since i am facing the same issue with the OrderIndepenededTranslucency and this is the implementation i opted for.

[capdevon]

Hi @JohnLKkk, just my two cents to this amazing work: according to the engine's default java programming style, package names should not contain capital letters. Could you please rename the package com.jme3.renderer.renderPass to com.jme3.renderer.renderpass ?

[pspeed42]

Maybe even just com.jme3.renderer.pass. "render" might be redundant when already under the renderer package.

[capdevon]

I agree with @pspeed42

[JohnLKkk]

Maybe even just com.jme3.renderer.pass. "render" might be redundant when already under the renderer package.

I will adjust the java package name to com.jme3.renderer.pass later.

[JohnLKkk]

My proposal would be adding a define "GBUFFER_WRITE" to the default techniques. you can set the define to 1 using MatParamOverrrides

Inside the shaders then a simple #ifdef #else can be used to write to data to gbuffer, or continue with the default lighting code.

I am not happy with the material handling. I know there are some upsides when using a separate technique, but having all the shader code twice is going to be a nightmare to maintain/extend.

My proposal would be adding a define "GBUFFER_WRITE" to the default techniques. you can set the define to 1 using MatParamOverrrides

Inside the shaders then a simple #ifdef #else can be used to write to data to gbuffer, or continue with the default lighting code.

But i would like to discuss this, since i am facing the same issue with the OrderIndepenededTranslucency and this is the implementation i opted for.

You raised a good suggestion. In fact, my original implementation was the approach you mentioned, which is to include multiple logic branches in a .frag file. However, I finally chose to use independent GBufferPass Technique blocks for the following reasons:

1. A primary goal: How to ensure a Mesh should be rendered in the Deferred stage or Forward stage of the hybrid rendering path? The answer is determined by its currently bound material. So I need a way to judge whether to dispatch the MeshDrawcommands to the DeferredRenderPass when distributing MeshDrawcommands. If I handle the default lighting or GBUFFER_WRITE through macro definitions or MaterialParameters in the default Technique, then when distributing MeshDrawcommands in the renderPass, I can only judge whether the Mesh should enter the DeferredRenderPass through the following ugly code: if(Mesh.getMaterial().getParam("HasGBufferWriteParam") != null){
   // can dispatch to DeferredRenderPass Mesh.getMaterial().setParam("HasGBufferWriteParam", true); } Or judge if the macro definition exists: HasGBufferWriteDefine = techniqueDef.addShaderUnmappedDefine("GBUFFER_WRITE", VarType.Int|VarType.Boolean); if(HasGBufferWriteDefine != null){ // inject macro definition, recreate shader variant, disable macro definition next frame... } There is another issue when considering switching the rendering path at runtime. If I set the Material through HasGBufferWriteDefine or HasGBufferWriteParam, then when I switch from DeferredPath to ForwardPath, I need additional tracking lists to record which Meshes' Materials have been modified with HasGBufferWriteParam or HasGBufferWriteDefine, and in switching rendering paths, loop through all Meshes' Materials and reset their HasGBufferWriteDefine or HasGBufferWriteParam to false. Such logic is not good... As you can see, this code is ugly. I finally chose to judge whether the Mesh can be dispatched to the specified rendering path by: if(Mesh.getMaterial().HasTechnique("GBufferPass")){ // can dispatch to DeferredRenderPass } Then I don't need to switch the Technique for each Mesh here, because JME already encapsulates an interface: renderManager.setForcedTechnique(XXXX); So I just need to collect objects and execute the GBufferPass through the encapsulated JME interface.
2. For safety, if I change to a single Technique block, I may not choose to use macro definitions to implement shader branches, because that would create shader variants (only one branch executes at a time, so no gpu warp dispatch overhead here, material parameters can be used safely), so I would use material parameters to control the branch (so the default Technique block would only create one shader variant). But in this way, JME3 users may edit the value of this "HasGBufferWriteParam" in the MaterialEditor...
3. Clarity of materials. If I include branches to handle Forward and Deferred logic in a default Technique block, then the user's custom MaterialDef would need to add a material parameter "HasGBufferWriteParam", or add a macro definition HasGBufferWriteParam. Also, users may add the HasGBufferWriteParam material parameter but forget to write the corresponding branch code. However, in the DispathMeshDrawcommands stage, because I judge that Mesh.getMaterial().getParam("HasGBufferWriteParam") exists, I think it can be rendered in the DeferredRenderPass, and the final result is blank, and JME3 users may not find the reason. On the contrary, with a separate GBufferPass Technique block, users must explicitly define this block. If this block does not exist, it will not be dispatched to the DeferredRenderPass, but will execute in the ForwardPass. Then I can detect whether the GBufferPass Technique block is valid when parsing the MaterialDef, and report an error to remind JME3 users to write their custom GBufferPass.
4. I need extra processing of the GBufferPass in subsequent stages, and you may have seen the following Pipeline tag: Technique GBufferPass{ Pipeline Deferred } It doesn't work now, but it will work for future features. If I integrate the GBufferWrite code into the default Technique block, I won't be able to distinguish the extra work of handling Pipeline Deferred...
5. Regarding your point about duplicate shader code, this is indeed a big problem. I discussed this issue with yaRnMcDonuts. Our idea is to extract the common code into glsllib so that only one copy needs to be maintained. This work has been started in this PR (https://github.com/jMonkeyEngine/jmonkeyengine/issues/2122，but not yet done)... In summary, I'm not saying this approach is most reasonable, it's just that currently I don't have a better encapsulation idea. After repeatedly considering points 1, 2, 3, and 4, my compromise was to delete the early code (which was the approach you mentioned, integrating all logic into the default Technique block)... If there are better architectures or encapsulation ideas in the future, it can be refactored here. If you have a better solution, please feel free to discuss with me anytime. Thanks!

[JohnLKkk]

Hi @JohnLKkk, just my two cents to this amazing work: according to the engine's default java programming style, package names should not contain capital letters. Could you please rename the package com.jme3.renderer.renderPass to com.jme3.renderer.renderpass ?

Thank you for pointing that out. I will adjust the java package path naming here.

[zzuegg]

Yeah, i am also not sure whats the best approach here. I am thinking about it, but thanks for the full explanation. Something more to consider. So the proposed layout of any material should be: Mat.j3md Mat.glsllib Mat-forward.frag Mat-deferred.frag

Tbh, i do not like this solution, and i do see the issues of the branching solution, but at this moment i also do not know of anything there could be done. (As adding a feature to the material system that would eliminate this problems). I am providing a more in depth response but i am in a hurry.

[JohnLKkk]

Maybe even just com.jme3.renderer.pass. "render" might be redundant when already under the renderer package.

The naming has been fixed.

[JohnLKkk]

Hi @JohnLKkk, just my two cents to this amazing work: according to the engine's default java programming style, package names should not contain capital letters. Could you please rename the package com.jme3.renderer.renderPass to com.jme3.renderer.renderpass ?

The naming has been fixed.

[JohnLKkk]

Yeah, i am also not sure whats the best approach here. I am thinking about it, but thanks for the full explanation. Something more to consider. So the proposed layout of any material should be: Mat.j3md Mat.glsllib Mat-forward.frag Mat-deferred.frag

Tbh, i do not like this solution, and i do see the issues of the branching solution, but at this moment i also do not know of anything there could be done. (As adding a feature to the material system that would eliminate this problems). I am providing a more in depth response but i am in a hurry.

Yeah, i am also not sure whats the best approach here. I am thinking about it, but thanks for the full explanation. Something more to consider. So the proposed layout of any material should be: Mat.j3md Mat.glsllib Mat-forward.frag Mat-deferred.frag

Tbh, i do not like this solution, and i do see the issues of the branching solution, but at this moment i also do not know of anything there could be done. (As adding a feature to the material system that would eliminate this problems). I am providing a more in depth response but i am in a hurry.

In fact, unless we refactor the MaterialSystem and Render to reasonably consolidate all the code into one Technique, and avoid the 4 concerns I worried about, it would be very unpleasant to resolve the 4 concerns I mentioned earlier (among which the most important ones are point 1 and 3) (not that the current architecture cannot do it, but the code would be very ugly...), so the current approach is somewhat a compromise I made for the existing JME3 architecture. However, unless a significant number of JME3 users reject this approach, I currently do not have the bandwidth to adjust the MaterialSystem and Render parts...because I have more important PRs (global illumination and FSR, VRS) to work on.

[JohnLKkk]

@stephengold @riccardobl I noticed there is currently "1 workflow awaiting approval" on the PR, should I wait or? Sorry I hope someone can let me know...

[oxplay2]

i guess it just need code rewiew people to mark "approve" not just "comment" but might be wrong.

Im not sure how it will work now, since there was HUB topic to improve PR. From Riccardo post earlier:

Anyone within the community who possesses knowledge of the code changes is encouraged to review the Pull Request If a reviewer is confident that the PR requires no further alterations, they are encouraged to provide their approval from the github interface

I guess zzuegg might be one i would hope, since he had knowledge

Also:

For feature PRs, they need approval from at least one core developer (two if it’s from a core developer). Everyone is encouraged to review, and if someone lacks the expertise (or time), they can still express approval or disapproval by reacting with :+1: or :-1: to indicate their stance on the feature’s general concept.

Im not sure if this new ideas are existing now, but this PR already have some :+1: so should be fine i guess.

Anyway yes, i think we should wait for core-devs confirmation

[riccardobl]

@stephengold @riccardobl I noticed there is currently "1 workflow awaiting approval" on the PR, should I wait or? Sorry I hope someone can let me know...

It seems it is your first contribution, or the first contribution since when forks need approval to run on github actions and so it needs to be manually approved. I approved the workflow :+1:

[zzuegg]

I think i have stated my opinion regarding the jme integration part here and on the forum. So i will let others argue about that. I have reviewed most of the files changed, but i have not yet dig into the new one. Don't know whats expected from a reviewer..

[oxplay2]

zzuegg i seen forum replies should answer about first case.

Second case idk, tho last commit were only about loggers. I guess some changes might be also made in 2.0.

[zzuegg]

So, after reviewing most of the code, and having spent too much time arguing with various people here is my summary:

This PR introduces a possible future rendering style that is up to date. It should offer a simple and flexible way to adapt the rendering to the needs of the game) In the current state, this is not the case, because the two available options are hardcoded into RenderManager.java with no option to customize it. As discussed with JohnLKKK a user configurable render graph is the final goal and he is working towards the same goal. As far i understand, his ultimate solution is quite close to what i had in mind.

So here is the bullet list why i vote against a merge into master:

• The integration is not done at all. See all stuff hardcoded into RenderManager.java
• This PR add methods/types that are not used in any code // i might be wrong on this one, they are used to switch between the hardcoded once
• Everything in the FrameGraph package is poorly documented
• Naming inside the shaders (If i have to look up a java file and a project desription to see whats inside a texture it is bad to maintain)
• It offers no more flexibility than the current backend. (Only moar lights)
• The api is far from stable and v2 in his repository is quite different than this
• The public api added trough this PR it will break code once it hopefully gets integrated cleanly.
• The current implementations skips still important topics (shadows/post processing)
• The usual formatting issues

All in all, i have the feeling this is a proof of concept, and an early test if the underlying FrameGraph api works as expected. I would probably do the same in my private repository, but it should not go on master.

JohnLKKK seems to know this, and he also mentioned that he has no hurry for merging and would continue even if this PR stays open for a long time. Since my efforts to look for a way if it could be done externally failed because of public rejection i see no option to make the feature available for the public (Beside a gradle install from his repository of course)

I personally did choose a bad PR to start my reviewing career, so i will still comment if things are unclear, but i am not going to argue with various "i want this in core!!!now!!!" people.

If you all come to a different conclusion then the rules for merging to master have changed drastically, but go ahead

[adibarda]

• The integration is not done at all. See all stuff hardcoded into RenderManager.java
• This PR add methods/types that are not used in any code
• Everything in the FrameGraph package is poorly documented
• Naming inside the shaders (If i have to look up a java file and a project desription to see whats inside a texture it is bad to maintain)
• It offers no more flexibility than the current backend. (Only moar lights)
• The api is far from stable and v2 in his repository is quite different than this
• The public api added trough this PR it will break code once it hopefully gets integrated cleanly.
• The current implementations skips still important topics (shadows/post processing)
• The usual formatting issues

Hi zuuegg, Thanks for the summary. If I understand correctly your points are: we have some hard-coded stuff in RenderManager we have new methods that are not used anywhere else - is it an issue? it doesn't break anything. it's a new foundation and likely to add new stuff that will be used later on FrameGraph is poorly documented - fixable Naming in shaders can be better We only get more lights - I guess more features will add much more code to this already huge PR The API is not stable - johnLkkk is handling it in V2 The public API added will introduce breaking changes to the current version - is it avoidable? Shandows / post processing is not improved in this PR - again, more features will add much more code to this already huge PR general formatting - I see several reviewers commented and johnLkkk fixed in all concrete comments

so, in general I think we have 2 major points here: hard-coded stuff in RenderManager new API introduces breaking changes

regarding the hard-coded stuff I need to go back and see johnLkkk's response and how sever it is regarding the breaking changes - @JohnLKkk - can you elaborate what us (the end users) need to change in current code?

Thanks again!

[zzuegg]

"Handeling/Fixing/Needing it in V2" talk is an early sign for me that it is currently not ready to ship. And the fact that he is already way into V2 is even more a strong sign that V1 is not ready.

I would be more open minded if it would not be the central part of the engine. This should be done right from the beginning. Or at least try

[yaRnMcDonuts]

This PR introduces a possible future rendering style that is up to date. It should offer a simple and flexible way to adapt the rendering to the needs of the game) In the current state, this is not the case, because the two available options are hardcoded into RenderManager.java with no option to customize it.

Sure it would be great to refactor it the way you described to let other renderers be swapped out, but right now we have 0 modular renderers to swap around so why make it the author of this PR's job to refactor our old rendering system and his new rendering system so they can be swapped in and out with any other new renderers that also don't exist yet? We can refactor that all in the future, but it shouldn't be a reason to slow down the implementation of a better renderer. I'd say it's on the fault of jme that our engine wasn't written to work with swappable renderers in the first place, but we're only just realizing it now apparently. So why make this the burden of the guy who's just trying to write us a cool new renderer?

Since my efforts to look for a way if it could be done externally failed because of public rejection

I would say this is actually because of author decision. I (and others) agreed that 3rd party libs have objective benefits, but not everyone likes making them and the author said he doesn't want to go that route. So we either let him make the PR, or we tell him to stop if he's not going to do it the way that we insist is easier for him.

I also stated how I believe we are being overly protective of the master branch and have yet to hear a reason to prove why we should be acting this way, especially about such groundbreaking features that many devs expect of a modern engine, and many choose not to use JME as a result of.

If this PR does end up abandoned and unfinished, there's nothing stopping us from reverting all the PRs and cleaning out the master branch. We have versions and beta/stable releases for a reason, and the documentation clearly advises that jme devs should not use master in production. So if master is more unstable for a while during this time then I don't think that's a bad price to pay, especially if the alternative is axing this feature altogether because the author is forced to do it in a way that is non-conducive to his preferred workflow and ultimately leads him to get frustrated and give up.

If you all come to a different conclusion then the rules for merging to master have changed drastically, but go ahead

We aren't changing the rules on master branch to say that any and every feature/idea can now be workshopped and tested directly on master like this. We are making a very specific choice to allow this to happen because the scope of the feature and the benefits it can bring.

[zzuegg]

So whats the benefit of adding it to master if it does not get shipped with next version?

Then, i fortunately have not be the guy to make the final call, this are my impressions and the result of my review. Thats it, not more not less.

[yaRnMcDonuts]

So whats the benefit of adding it to master if it does not get shipped with next version?

I think the benefit is that he gets to work the way he wants and is less likely to get discouraged by the hurdles you have to jump through to learn how to post and manage your first 3rd party lib.

I did it once for my PBR terrains before they got merged to core, and it was not fun and took a lot of my time trying to overcome minor bugs and issues learning and using the interface for the place I was instructed to upload my library to. Time I'd have rather spent working on the actual shaders but instead was getting frustrated trying to learn a new websites interface. I'd say it was a valuable learning experience, but I completely forget the process because it was so long ago and the Jfrog place I was uploading to was apparently getting shut down shortly after I started using it, so that was discouraging lol. Needless to say I do not look forward to relearning this all again when I make another 3rd party library.

So even if it is objectively better way to make everything a 3rd party lib, that is only true for devs who are experienced in that process and know the process like the back of their hand. I commend those of you who are in that position, but that is not the majority position, and expecting absolutely everyone who wants to commit a big feature to jme to go through that process with no exceptions seems like a great way to discourage contributions.

[oxplay2]

Based on HUB topic referred to zzuegg comment, currently i only see 4 real topics based on it:

• currently i see mainly lack of docks in FrameGraph package files. (on experimental branch it might not be important but docs should be added before merge in current way)

• Formatting, not that important(worth to reformat ofc), but not to me to decide here. In general like already discussed on HUB, would be good to add github formatting verification step or auto-formatting later anyway.

• one unused variable(with setters) in ViewPort file. "setRenderPath/getRenderPath" seems unused. Might be added later when needed, just not now.

• Also related to HUB zzuegg comment: Everything added in the RenderManager.java again will probably get removed again. All setters of the various techniques have to go to some VarSinks at some point related to: The public api added trough this PR it will break code once it hopefully gets integrated cleanly. Someone would need refer to this.

So Imo just this 4 topics here. One of zzuegg points were in fact new feature, since current code do not provide this.