A framegraph framework that supports 3 render paths #2090
Conversation
1. Added basic framework of framegraph to manage and organize new rendering passes; 2. Split existing rendering into several core RenderPasses; 3. Added multiple rendering paths (forward, deferred, tile based deferred); 4. Activated Framegraph system by useFramegraph, activated rendering paths by setRenderPath;
Fix flickering issues in tile based deferred shading;
…tDirectionalAndSpotLightShadows.java"
|
How about we mark this PR as a draft until it's ready for final review? |
riccardobl
added
the
hacktoberfest-accepted
I think it's possible, however advancing this might require some extra work:
|
|
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.
Testing and bug reporting can and should continue while the PR is a draft.
Your decision; we can go either way.
I'm fine with that. |
Added two renderPath sample codes. Added terrain unlit shading model test code; Added terrain lighting shading model test code;
|
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. |
| } | ||
| } | ||
|
|
||
| private void prepaLightData(int lightNum){ |
There was a problem hiding this comment.
"prepaLightData" is slightly confusing.
Should this method be named "prepLightData" or "prepareLightData" instead?
| if (l instanceof AmbientLight) { | ||
| ambientLightColor.addLocal(l.getColor()); | ||
| if(removeLights){ | ||
| lightList.remove(l); |
There was a problem hiding this comment.
Faster to remove by index instead of by object.
| if (l instanceof LightProbe) { | ||
| skyLightAndReflectionProbes.add((LightProbe) l); | ||
| if(removeLights){ | ||
| lightList.remove(l); |
There was a problem hiding this comment.
Faster to remove by index instead of by object.
| * <tt>max</tt> (inclusive). | ||
| */ | ||
| public static float nextRandomFloat(float min, float max) { | ||
| return (nextRandomFloat() * (max - min + 1.0f)) + min; |
There was a problem hiding this comment.
Can return outside specified range.
There was a problem hiding this comment.
Thank you for pointing out the issue, it has now been fixed.
| } | ||
| } | ||
|
|
||
| private void prepaLightData(int lightNum){ |
There was a problem hiding this comment.
Confusing name "prepaLightData".
I think it would be better if it were "prepLightData" or "prepareLightData".
There was a problem hiding this comment.
Thank you for pointing out the issue, it has now been fixed.
| Node tank = (Node) assetManager.loadModel("Models/HoverTank/Tank2.mesh.xml"); | ||
| rootNode.attachChild(tank); | ||
|
|
||
| ColorRGBA colors[] = new ColorRGBA[]{ |
There was a problem hiding this comment.
Replica of another array.
There was a problem hiding this comment.
This code is from some early temporary testing I did, so I don't think it impacts any functionality. I won't adjust the TestSimpleDeferredLighting.java code for now.
| key.setTextureTypeHint(Texture.Type.CubeMap); | ||
| final Texture tex = assetManager.loadTexture(key); | ||
|
|
||
| for (Spatial geom : buggy.getChildren()) { |
There was a problem hiding this comment.
I think it would be better to use Spatial#depthFirstTraversal here, so that this test does not depend on the model's internal layout.
| if (nb > 60) { | ||
| n.removeLight(light); | ||
| } else { | ||
| int rand = FastMath.nextRandomInt(0, 3); |
There was a problem hiding this comment.
Add method to return a randomly selected color?
| uniform sampler2D m_SplatEmissiveMap; | ||
| #endif | ||
|
|
||
| uniform int m_AfflictionSplatScale; |
There was a problem hiding this comment.
I suggest removing "Affliction" from uniform names, since that could become confusing in the future.
There was a problem hiding this comment.
I discussed renaming this in the past when I submitted the pr for the terrain shaders, and we ultimately decided to keep the naming because I don't think another better name could be used to describe the functionality since it does more than 1 thing.
The "affliction" feature and all related code could all be removed if really needed, it mostly was just left because it doesn't hurt anything and is a bonus feature if anyone ever wants to use it. It essentially is just an extra alpha map that uses the R channel for a desaturation effect (to simulate things like grass textures turning brown/gray) and in the G channel it does texture splatting with suppor for noise to look natural with a low-res alpha map.
So I uiltimately called this unique alpha-mapping "affliction" since that encompassed the 2 features based on how they are used in my game to "afflict" the terrain with desaturation and slime effects dynamically. (and in the future I plan to add things like dynamic wetness and dynamic snowy-ness to the remaining 2 texture channels of the affliction alpha map.
So I would ultimately advise not removing the word "affliction" from anything since its an optional feature and shouldn't be confused with other normal terrain textures in the shader. Or if the affliction feature causes any issues with this PR, then it would also be okay to just remove all of the affliction related params and gut the features entirely.
Edit: I just looked through the shader and it appears all the changes are related to the lighting calculations, whereas the affliction splatting stuff is all texture-read related. So it shouldn't break anything, but of course feel free to remove it if I'm wrong and it causes any issues or confusion.
There was a problem hiding this comment.
This file looks like a nightmare to maintain. Is there a way to combine all the tasks in a #for, since most of this code looks copy/pasted?
There was a problem hiding this comment.
I meant to refactor to use the #for loops in the original TerrainLighting.frag shader (which TerrainLightingGBufferPack.frag appears to be based on) however I unfortunately forgot after doing it for the PBR shaders, but I still plan to do that.
It would be a good addition to this PR, however I do notice that the #for loops are related to the shader's texture reads (and not the lighting calculations which this pr seems to be focused on) and the texture reading code all appears to be identical to the original shader its derrived from (which is great for maintainability) and the shaders only deviate in the lighting calculation. So if @JohnLKkk doesn't manage to refactor the texture reads in this shader to use the for loops in this PR, I will add it to my to-do list when I refactor the other original TerrainLighting.frag to do the same.
Thank you for your valuable suggestions. I will refer to most of the suggestions and only need to make corresponding modifications and adjustments. |
… java file, fix a few minor issues
I have just supplemented a large number of core comments as well as complete JME license information in each file. |
|
I would prefer a nice, clean, easy to use framegraph api in this PR. That is the foundation of all following, and that should be implemented first. And like John provided, an example how to use it but from user space, not by modifying the core again. This is the point where tests can be made on various scenarios to see if the framegraph api offers enough flexibility without running into configuration hell. |
Perhaps we can decide the usage of frameGraph in the SimpleApplication application layer in this PR? I will reference the user-facing usage interface you provided, but I'm not sure if this PR should add that part (because this PR is mainly about the rendering path). Also, if we decide to add a frameGraph interface in this PR that won't eventually change the usage flow in the SimpleApplication layer, I still need to keep the fallbackFrameGraph member variable in RenderManager in this PR to address some issues I haven't sorted out yet. Would it be okay to have some intermediates in this PR and future versions, but no breaking changes to the SimpleApplication layer interface? |
|
To me it feels logical to have a separate PR for the framegraph changes and then this PR can depend on that one. I don't know if that causes issues, though... because we already suggested it and there was some problem doing it. |
Perhaps I haven't considered all cases. I assumed most jme3 users are using the JME3 engine based on SimpleApplication, so even if I adjust RenderManager and other internal module code in the future, as long as it doesn't invasively impact the interfaces and usage of the upper layer SimpleApplication, it could work (because I assume most JME3 users won't look at and develop based on engine code like RenderManager, so I can have some intermediate code in these codes, and refactor slowly later)? |
My original idea was to improve FG in subsequent multi-pass PRs. In this current PR, I tried to first make the rendering path, FG compatible with existing JME3 code (meaning this PR only first embeds FG in engine code without providing external interfaces). However, if people hope that this PR can provide external usage interfaces for FG and ensure that the usage will not change in the future, I will consider adding external FG interfaces on top of this PR (but I probably won't have time to adjust internal FG interfaces for now). |
The current FG is not flexible enough yet and still requires many features to be added, but can we first discuss the final external usage interfaces and provide implementation details for the minimal code currently available, to ensure SimpleApplication can use "a little bit" of FG functionality in its current state, and allow me to implement FG internal logic in subsequent multi-pass PRs? |
What you mentioned is an approach. To be honest, I scattered the "features" across multiple PRs (PR1->PR2->PR3). Perhaps like what I or others have said, there should be a "development" branch or "lab" branch to implement new technologies. Maybe I misunderstood something, I'm currently using the master branch as the "development" branch, so I scattered features across multiple PRs for implementation. |
You spoke my inner thoughts, thank you. |
|
No one is talking about third party libraries anymore. You guys already shot and killed that horse then buried it, dug it back up, burned it, smashed it into bits and buried it again. We get it. Spending 2 hours to learn maven publish is 2 hours better spent on something else... even when we were ready to make that process part of the engine infrastructure. We give up on that. So we are in "make the branch perfect" mode. Anyway, what we have is a PR. PRs are just fancy branches. They don't have to be based on master. PR to make the engine more flexible can be the basis of other PRs. It's quite common to work this way. But if the existing engine cannot be easily converted to run framegraphs then maybe that hasn't finished cooking yet. That's fine too. Once you get it all perfect then we can look at ways to make it more reviewable. |
|
i think Paul you misunderstand general thought what he meant. Anyway if there is so much problem for single person, it seems like experimental branch in JME would be way to go. (and i guess John is fine with this way too). I still remember as new JME user i were using Nightly builds to see new incoming things. Its still in core, it can be tested by "normal-user" people - so all would seem fine like this too. I would not care about naming here, it can be lab or experimental or nightly, but general idea would be to make it accessible for everyone and integrated with core. Were both of this are required. So for example if there will be time to implement multiple render backends, it would be easy to integrate with core-code + test by normal users. While JME versions could be of non-experimental(default) branch, when experimental one can be used and tested, it could be merged when time comes. |
This is still a good idea, and it would be useful to still do in in case we end up in a similar scenario in the future. Some type of engine infrastructure to easily upload and post new versions of libraries or even just some documentation or tutorial video to show how exactly to do it would probably make contributors much less averse to the idea in the future. |
No, i am worried that you take shortcuts instead of a clean api. Lets take the RenderPath as example. For what do you need that variable? As far i see, it is used only to choose from the different framegraph definitions you choose to hard code inside rendermanager. (ShortCut #1) Now, when implementing the TileBased version you needed some variables. Instead of adjusting the FG api to be able to work on some kind of settings, you choose to hardcode them in rendermanagers. (ShortCut #2) Now, at this stage, in the very very beginning you took two shortcuts that makes it impossible for engine users to customize the Framegraph api. You will see in future iterations that you have limited yourself. Now, to keep yourself of those short cuts, you should make rendermanager/viewport able to render using a framegraph, thats it! I do not have time to write a more detailed explanation where i already see issues. I am still open for a collaboration, but i would prefer a quieter place where we can discuss technical aspects without other topics dropping in. |
I did take shortcuts in this PR, as I have explained several times (I need some intermediate artifacts during the transition period to facilitate my feature development), as you said, you don't recommend having intermediate artifact code exist in master, then, I suggest marking this PR as a "Dev PR"(@stephengold ) until I complete FG (which may take some time). Alternatively, I will remove FG entirely from this PR and implement the three rendering paths in the old pipeline way. These days, I have been thinking about an issue, |
I don't know what you mean by that. Are you suggesting I re-title this PR? Or are referring to some mechanism build into GitHub? Please elaborate. |
I love having technical discussions that anyone can participate in, and I find straw polls cute and amusing. However, I'm opposed to making technical decisions via an open poll. Such decisions should be made by responsible, informed individuals with a deep understanding of the issues. Open polls risk uninformed participants and sockpuppetry. In this instance, I'm not well informed on the subject matter, so I'm waiting for a consensus to emerge. |
|
@stephengold While i would agree about missing docs/one-unused-variable, other things are just like feature requests for first pr. So i still think my earlier idea would be consensus here. I think John meant he just will add next features into this PR(like finished all FG code), thats what he meant probably. |
|
We should also avoid adding things to the public API that will just get ripped out again in the immediate next version. If the code needs time to bake then give it time to bake. There is no reason to rush adding it to core. People can already try it out if they like and will recognize that the API is in flux. Once something goes into a real public release, it's a minimum of two more dot versions before it can be removed again. If it's not ready yet then it's not ready yet. jme3 took a long time, too, and that popped out almost fully baked. |
|
I understand it's not easy to break down a big feature into steps that make sense by themselves. |
|
I was not expecting a full featured UE RDG in this first pull request. I was expecting that the features you use in this first pull request are available to the end user. And as far as this pull request is considered, cleaning up those hard codes is doable. You are probably correct that we need to settle for a public api first. Maybe that should have been the very first step. At least it would be clear to everyone what featureset the api offers, and what parts of the engine have to be changed to support it. |
Currently, I'm looking for a consensus on what this PR should contain. My original plan was as @oxplay2 described, but @zzuegg felt that this PR should not contain these temporary shortcut codes. To that end, I proposed a solution: Could we decide in this PR what the public API for FG should look like? I will later explain the API forms of UE RDG and U3D FG. Then we decide on a final form of the FG public API in this PR, and we won't change these public APIs in subsequent development.
As for whether to create a topic for everyone to vote, you may be right, people unfamiliar with it should not participate in voting to decide the result, but at least let the JME community know the API forms of the new technology, and which FG public API we ultimately choose. |
Ultimately, once we finalize the FG public API, I will continue to develop the code for this PR, so the PR remains open (I don't know if there is a Dev tag or something, if not, just keep this PR open, and I will modify the functional content of the PR afterwards). |
Yes, so we need to reach a consensus here. |
Yes, I think this is feasible, to finalize the FG Public API, then I continue adding these codes to this PR, and won't break the FG public API in the future unless absolutely necessary. |
@stephengold I have modified the content until this PR completes the FG Public API. For now, just keep the PR open. |
|
Do others have anything to add? If not for now, then I will organize some related content a bit later (probably tomorrow), then we can discuss in the community what the JME FG public API should look like. |
|
There's no rush to close this PR, just like there's no rush to integrate it. @JohnLKkk as creator of this PR, you can mark it as a "draft", to indicate it's a work in progress and/or not ready for review. |
I have changed it to "Draft" status until we finalize the public API in this post and complete the public API(https://hub.jmonkeyengine.org/t/on-framegraph-in-the-future-of-jme/47240?u=jhonkkk), then re-enter review. |
|
Hi @JohnLKkk , is this PR complete and integrable for testing in the next alpha version of the engine? We are all excited about these changes that should improve the graphical performance of the engine. Please let us know if you are still working on it. |
see:https://hub.jmonkeyengine.org/t/enhancements-to-the-jme3-rendering-engine/47145/27
Enhance the capabilities of the JME3.7 renderer, mainly including:
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);