Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cleanup core pipeline core_3d render nodes/order/options/etc #7981

Closed
JMS55 opened this issue Mar 8, 2023 · 5 comments
Closed

Cleanup core pipeline core_3d render nodes/order/options/etc #7981

JMS55 opened this issue Mar 8, 2023 · 5 comments
Labels
A-Rendering Drawing game state to the screen C-Usability A targeted quality-of-life change that makes Bevy easier to use X-Controversial There is active debate or serious implications around merging this PR

Comments

@JMS55
Copy link
Contributor

JMS55 commented Mar 8, 2023

After working on bevy for a while, I've noticed some papercuts with the core_3d passes/components. Let's discuss how to improve it :)

What problem does this solve or what need does it fill?

  1. It's easy to accidentally get the order of post process nodes wrong. You must order it after the main pass, and before tonemapping, but also order the pass before/after other specific post processing passes. Otherwise, you end up with TAA running before bloom by accident. It's too easy to underconstrain the node orders
  2. The "tonemap in shader" (main pass) default is kinda a bad idea. Tonemapping should always go last, after/during post processing. Currently this only happens if you enable hdr.
  3. The "upscale" pipeline (blit main texture to swapchain) is pointless. I'm not sure what the original intention behind naming it upscaling is, but for TSR (temporal upscaling) techniques like FSR/DLSS, they should run where TAA goes - right after the main pass, before any other post processing.
  4. Most people will probably want to enable hdr, but it's off by default.
  5. After bevy 0.10, we have better tonemapping options. Lets change the default tonemapper.

What solution would you like?

  • Define a set of 3 pass labels (PREPASS, MAIN_PASS, POST_PROCESS), that run in that order.
  • Within POST_PROCESS, define a strict ordering (setup as part of Core3dPlugin, rather than each individual plugin needing to set their order properly) as follows: (TAA, BLOOM, TONEMAPPING).
    • Tonemapping will write directly to the swapchain, and combines/replaces the previous tonemap/upscale nodes
    • I'm unsure where FXAA should go. FXAA wants to go after tonemapping, but we want to setup tonemapping to write to the swapchain. Perhaps we have an abstraction in ViewTarget where the last pass always writes directly to the swapchain. That, or special case tonemapping to write to either the swapchain or ViewTarget depending on if FXAA exists.
  • Remove the tonemap in shader option, or at least make it off by default unless you enable a setting. The default should be a separate tonemap node for better compatibility with post processing.
    • A hypothetical future LowEndMobileCamera3dBundle could add this component to enable the setting and cut down on render passes
  • Camera { hdr } should be on by default, and renamed to something better. It has nothing to do with hdr monitors / output - it's just what texture format bevy uses internally during render passes.
    • On wasm/webgl2, hdr has issues, I think in combination with MSAA. We can either turn it off by default on the web until webgpu releases, replace MSAA with FXAA as our default AA method, or something else.
@JMS55 JMS55 added C-Feature A new feature, making something new possible A-Rendering Drawing game state to the screen X-Controversial There is active debate or serious implications around merging this PR labels Mar 8, 2023
@JMS55 JMS55 added this to the 0.11 milestone Mar 8, 2023
@alice-i-cecile alice-i-cecile added C-Usability A targeted quality-of-life change that makes Bevy easier to use and removed C-Feature A new feature, making something new possible labels Mar 8, 2023
@DGriffin91
Copy link
Contributor

Ideally I think FXAA is supposed to be after tonemapping. I think it's also probably ok to have FXAA before tonemapping if that ends up being easier. Mostly the user settings just have to be slightly tweaked. iirc we did some testing previously and it work ok in either position. Though we could test more. We could also do reversible tonemapping, which we probably should be doing to make MSAA work correctly too.

Could be most efficient to put FXAA in the "upscale" (as it's currently called) pass with reversible tonemapping, then output into color grading/actual tonemapping, that way the extra pass is avoided.

@IceSentry
Copy link
Contributor

It was recently asked if it would be possible to insert a node between the opaque and transparent phase of the main pass. I'm not sure if this fits here because it might be a bit more involved, but I felt it was worth mentioning.

@JMS55
Copy link
Contributor Author

JMS55 commented Mar 14, 2023

So, here's an update on how discussion has progressed, in order from easiest/most fleshed out to hardest/needs more discussion. The first ~5 are good first issues if anyone is looking to help out!:

  • (WIP) Split MainPass3dNode to allow custom nodes in between: Split opaque and transparent phases #8090
  • (Needs PR) We should have a START_MAIN_PASS_POST_PROCESSING label in addition to the end label. Maybe we should also rename it to not include "main_pass".
  • (Needs PR) We should replace the current ReinhardLuminance tonemapper with something better
  • (Needs PR) All post processing nodes should be inserted and ordered into a chain in bevy_core_pipeline/src/core_3d/mod.rs or a separate postprocessing submodule or something. Currently, FXAA, bloom, and upcoming features do the ordering themselves in their respective plugins, which is very easy to break from my experience.
  • (Needs PR) Combine the tonemap and upscale nodes into one "tonemap and blit to swapchain/display" node. We can call it DisplayOutputNode or something.
    • FXAA can go before tonemapping, and use an invertible tonemap option like TAA does.
  • (Needs discussion) Render Node improvements are being worked on by @IceSentry, but we don't yet have a way of defining labels without using EmptyNodes.
    • Empty nodes for labels are currently being used to mark the start and end of the mainpass/postprocessing. Maybe we should use subgraphs instead? A subgraph for the main pass, one for post processing, one for AA within postprocessing, etc.
    • Eventually, I think the end goal will be nodes as systems. That way we can just put all post processing in a post process system set for example and then order the set relative to the main pass set, as well as all the other associated ergonomic benefits
  • (Needs lots of discussion) Remove the Camera::hdr option + tonemap in shader implementation, and always use floating point textures internally and the tonemap node. @cart confirmed this is a good idea, so long as we can figure out how to handle MSAA.
    • Floating point textures interact poorly with MSAA, and have even further issues on WebGL2. We'd have to figure out how to support MSAA still. I don't have enough knowledge to propose anything concrete for this.
    • Mobile support also needs thought. Cutting down on the number of passes we do is a must. That means we don't want a separate tonemapping node and post processing stack and want to fold it into the main pass, and want to have the main pass write directly to the swapchain. Whatever the solution is, ideally it does not get in the way of the other rendering features, and reuses code where possible. The PBR shaders are somewhat entangled atm.

@JMS55 JMS55 added this to Rendering Mar 16, 2023
@JMS55 JMS55 moved this to Todo in Rendering Mar 16, 2023
@cart
Copy link
Member

cart commented Mar 20, 2023

That means we don't want a separate tonemapping node and post processing stack and want to fold it into the main pass,

Not necessarily. Notably the HypeHype mobile renderer does one "separate" post processing pass that reads from the intermediate "main texture" and does a single unified post processing pass that writes to the swapchain.

Imo we should build a high level "post processing effect" abstraction that generates shader snippets that can get combined into a single post processing shader. We would limit mobile to those effects (at least by default), but it could still be composed with more advanced / unconstrained multi-pass effect chains on desktop.

@JMS55 JMS55 removed this from the 0.11 milestone Jun 11, 2023
@JMS55
Copy link
Contributor Author

JMS55 commented Jun 11, 2023

Closing this for now. Some of these ideas have gotten implemented, others we've since come up with better or different ideas.

@JMS55 JMS55 closed this as completed Jun 11, 2023
@github-project-automation github-project-automation bot moved this from Todo to Done in Rendering Jun 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Usability A targeted quality-of-life change that makes Bevy easier to use X-Controversial There is active debate or serious implications around merging this PR
Projects
Status: Done
Development

No branches or pull requests

5 participants