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

Add FullscreenPassNode for postprocessing #1988

Closed
wants to merge 10 commits into from
Closed

Conversation

mtsr
Copy link
Contributor

@mtsr mtsr commented Apr 23, 2021

I've just committed a first (mostly) working version of a FullscreenPassNode that can be used to do postprocessing.

I've also done some work to (optionally) integrate it into the base rendergraph. Because of how bevy_render, bevy_pbr and bevy_ui create the render graph, there's still some issues there:

  • I can't currently set up the fullscreen pass only for pbr pipelines. This means there's still some work to at least disable tonemapping for non-pbr pipelines;
  • add_post_pass cannot be enabled only for bevy_pbr without referencing that feature within bevy_render, which seems bad;
  • UiPass doesn't currently run after the standard fullscreen pass like it should, because bevy_ui can not access the add_post_pass config to determine whether it should create an edge from that instead of from MainPass. Similarly, this cannot be set up in bevy_render, because it can't know if there is a UiPass. This could be solved by having plugin config live in Resources, but that would create these weird dependencies between plugins;
  • There's an ordering problem where TextureNode creates the textures in update(), but FullscreenPassNode needs the textures in prepare() that causes a full frame delay before FullscreenPassNode has access to the textures.

Other things I'm working on:

  • Doesn't currently work with MSAA, this should be relatively minor to fix Ended up adding a separate Resolve pass;
  • Checks whether all bind groups are bound in a pretty bad way;
  • Setting bind_groups from Res<RenderResourceBindings>;
  • Properly handling window resolution;
  • Draw a single triangle instead of a quad;

While I'm iterating on this, I'd love to get some feedback on the general approach. Some open questions:

  • How do we want pluggable functionality like other fullscreen passes to work? Currently I foresee something like creating the rendergraph node with the right inputs and setting a node_edge to force it to run before the final gamma correction and msaa resolve pass. But it's going to be very hard to get the correct ordering of fullscreen passes from plugins with this method;
  • How should FullscreenPassNode take textures? Currently you pass it a Vec of textures + names to pass to a shader. Alternatively these could all go through Res<RenderResourceBindings>; RenderGraph textures+samplers are connected as slot_edges to FullscreenPassNode. Other textures or resources should go through Res<RenderResourceBindings>.

Depends on #1998. Rebase with git rebase --onto main <last commit of #1998>.

@alice-i-cecile alice-i-cecile added A-Rendering Drawing game state to the screen C-Feature A new feature, making something new possible help wanted labels Apr 23, 2021
Copy link
Contributor

@StarArawn StarArawn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple of comments on the code, but my first thought is that the tonemapping should live in its own plugin outside of the render code.

attachment: TextureAttachment::Input("color_attachment".to_string()),
resolve_target: None,
ops: Operations {
load: LoadOp::Clear(Color::rgb(0.1, 0.2, 0.3)),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't there a resource for clear color we could use here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Good point.

mip_level_count: 1,
sample_count: msaa.samples,
dimension: TextureDimension::D2,
format: TextureFormat::Bgra8Unorm,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens when a user wants a different buffer format?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair question, that's one for the how do we compose a rendergraph discussion.

crates/bevy_render/src/render_graph/base.rs Outdated Show resolved Hide resolved
@mtsr
Copy link
Contributor Author

mtsr commented Apr 27, 2021

I'm planning on splitting this PR in three. Where the separate PRs have been created, I'll add the reference:

  • One that simply adds the FullscreenPassNode Add FullscreenPassNode for post-processing #2027;
  • One that splits off the BaseRenderGraph out of bevy_render and into a separate plugin, or maybe even multiple for different use cases;
  • One that adds such a separate plugin that builds a standard PBR rendergraph using FullscreenPassNode.

@mtsr mtsr closed this Apr 27, 2021
@mtsr
Copy link
Contributor Author

mtsr commented Apr 27, 2021

In order to preserve useful comments, this is quoted from Akorian on Discord (I don't know your github handle).

  1. A small thing I noticed is Bgra8Unorm being used for the intermediate PBR target (bevy_pbr/src/render_graph/pbr_pipeline/mod.rs:35). Not sure if that's on purpose but this way the hdr content would be clipped before tonemapping (also accuracy will be an issue). Rgba16Float would be a better fit in my opinion.

  2. Is the focus of this only on "fullscreen" post processing with a single node or is it meant to be a general approach for all types post processing? (i.e. using mutliple post process nodes in a larger graph with render targets, including ones that aren't necesserily tied to the window size at all).
    I might be a bit biased by my.. err "special needs" but a general purpose solution that can be used in custom render graphs would be great. Though, I understand if this is out of scope for this specific PR.

  3. The PR mentions MSAA being an issue (and I can say from experience that it's often a huge pain in the butt when it interacts with post processing).
    From what I can tell, if enabled, the post process step will read from a multisampled target and write into one aswell, before being resolved at the very end.
    I don't think this approach (post processing writing into an MSAA target) is a good solution here:

  • writing from the post process step into an MSAA target doesn't really have any benefits (no geometric information here)
  • increases memory consumption & throughput a lot
  • the final resolve step would be unecessary (resolving needs to be done by the person who's writing the post process shader)
  • it also makes writing shaders more difficult for less experienced users (MSAA textures come with their own quirks)

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-Feature A new feature, making something new possible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants