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

Post processing doesn't work correctly when using multiple viewports #7435

Closed
IceSentry opened this issue Jan 31, 2023 · 9 comments
Closed
Labels
A-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior

Comments

@IceSentry
Copy link
Contributor

Bevy version

0.9 and main

What you did

In the split_screen example:

  • Enable hdr on both cameras (enabling hdr only on one also breaks bevy, but this is a separate issue).
  • Add BloomSettings::default() to the left camera
  • Use an emissive color for the plane

image

I would have expected the left viewport to have bloom applied to it.

What went wrong

The bloom effect does not appear in the left viewport.

As you can see here, the bloom effect is indeed computed and generated correctly, but once it gets to the final upscaling pass, the texture modified by the post process is not used as an input.
image

image

I haven't investigated more than that, but it seems like the upscaling node is using the wrong texture when using multiple viewports combined with post processing.

@IceSentry IceSentry added C-Bug An unexpected or incorrect behavior S-Needs-Triage This issue needs to be labelled A-Rendering Drawing game state to the screen and removed S-Needs-Triage This issue needs to be labelled labels Jan 31, 2023
@rparrett
Copy link
Contributor

rparrett commented Jan 31, 2023

related to #7361

@IceSentry
Copy link
Contributor Author

IceSentry commented Jan 31, 2023

It might be related, but this happens with any post processing effect, not just bloom. Bloom is simply the easiest to test. For example, if I enable bloom on the right camera instead, it works as expected where only one camera has bloom applied.

image

@Elabajaba
Copy link
Contributor

Elabajaba commented Jan 31, 2023

Multiple cameras with multiple windows work fine (where each window has its own camera), but multiple cameras that composite to the same window don't.

@Elabajaba
Copy link
Contributor

It might be related, but this happens with any post processing effect, not just bloom. Bloom is simply the easiest to test. For example, if I enable bloom on the right camera instead, it works as expected where only one camera has bloom applied.

From what I can tell, it only works for bloom because bloom works on a separate set of textures that are limited to the view.

Fxaa (and my CAS pr) use the main_texture and end up applying to everything on screen instead of only the right camera's viewport.

@Elabajaba
Copy link
Contributor

Elabajaba commented Feb 1, 2023

Debugging it on discord with @robtfm, this is a bug with Msaa. In the 2nd pass it's reading the main_texture_sampled from the previous pass, which doesn't have any post processing applied.

Disabling Msaa also revealed another bug, where because it now relies on the previous main_texture_a the output is reliant on what the last pass that wrote to main_texture_a was. With only bloom, it works fine (by accident), but add Fxaa and you end up with a double tonemapped final output because main_texture_a's was tonemapped before it started rendering the 2nd viewport.

@IceSentry
Copy link
Contributor Author

From what I can tell, it only works for bloom because bloom works on a separate set of textures that are limited to the view.

Right, the other issue I had was for a post process effect that uses multiple textures like bloom does so I mistakenly assumed it was an issue with all post processing.

@robtfm
Copy link
Contributor

robtfm commented Feb 1, 2023

there's a few post-processing issues compounding one another here. i think if we do the below, then almost everything works as intended:

  • fxaa and tonemapping need to set viewport
    • currently they read/write full-screen and ignore viewports
  • cameras that are not LAST need to blit their final result back to the base target (either the multisampled texture or main_texture_a depending on MSAA). this can be skipped if the final output is already to that target (i.e. if last write was to main_texture_a and MSAA is disabled)
    • currently we do not copy back, so the last postprocessing step may be ignored (if msaa is off and the last write was to main_texture_b), or all postprocessing may be ignored (if msaa is on)
  • cameras that are not FIRST need to reverse-tonemap within their viewports
    • currently this is not done at all, and leads to darkening from multiple tonemap applications for overlapping viewports

the above still doesn't solve problems with overlapping non-destructive post-processing cameras (i.e. cameras that write to previously written pixels and don't overwrite every pixel in their viewport, then apply postprocessing). bloom (e.g.) applied to these cameras will affect previous camera's pixels as well because they are unavoidably there in the bloom input texture. you can avoid that in this particular case by running the bloom cameras first, but there's no general solution while rendering to the same texture.

the full solution for that is rendering to multiple texture targets and composing, which is supported already but requires a bit of extra work (see the render-to-texture example).

@robtfm
Copy link
Contributor

robtfm commented Feb 1, 2023

one more, though it doesn't matter in the core pipeline and i believe is already addressed by the bloom rework:

  • bloom needs to source from the current post_processing source texture
    • currently it builds the bindgroup in prepare and assumes that the source will be main_texture_a. if other post-processing steps are added before bloom it would potentially discard the last step prior to it

@Keelar
Copy link

Keelar commented Feb 1, 2023

Could this also be related to #5721? Seems like there are some similarities, as they are both influenced by MSAA and camera rendering order. Apologies if they're unrelated. I'm not too familiar how the engine works at a low level yet but some of the discussion in here reminded me of that issue so I thought it was worth bringing up.

@bors bors bot closed this as completed in abcb066 Mar 1, 2023
Shfty pushed a commit to shfty-rust/bevy that referenced this issue Mar 19, 2023
# Objective

Alternative to bevyengine#7490. I wrote all of the code in this PR, but I have added @robtfm as co-author on commits that build on ideas from bevyengine#7490. I would not have been able to solve these problems on my own without much more time investment and I'm largely just rephrasing the ideas from that PR.

Fixes bevyengine#7435
Fixes bevyengine#7361
Fixes bevyengine#5721

## Solution

This implements the solution I [outlined here](bevyengine#7490 (comment)).


 * Adds "msaa writeback" as an explicit "msaa camera feature" and default to msaa_writeback: true for each camera. If this is true, a camera has MSAA enabled, and it isn't the first camera for the target, add a writeback before the main pass for that camera.
 * Adds a CameraOutputMode, which can be used to configure if (and how) the results of a camera's rendering will be written to the final RenderTarget output texture (via the upscaling node). The `blend_state` and `color_attachment_load_op` are now configurable, giving much more control over how a camera will write to the output texture.
 * Made cameras with the same target share the same main_texture tracker by using `Arc<AtomicUsize>`, which ensures continuity across cameras. This was previously broken / could produce weird results in some cases. `ViewTarget::main_texture()` is now correct in every context.
 * Added a new generic / specializable BlitPipeline, which the new MsaaWritebackNode uses internally. The UpscalingPipelineNode now uses BlitPipeline instead of its own pipeline. We might ultimately need to fork this back out if we choose to add more configurability to the upscaling, but for now this will save on binary size by not embedding the same shader twice.
 * Moved the "camera sorting" logic from the camera driver node to its own system. The results are now stored in the `SortedCameras` resource, which can be used anywhere in the renderer. MSAA writeback makes use of this.

---

## Changelog

- Added `Camera::msaa_writeback` which can enable and disable msaa writeback.
- Added specializable `BlitPipeline` and ported the upscaling node to use this.
- Added SortedCameras, exposing information that was previously internal to the camera driver node.
- Made cameras with the same target share the same main_texture tracker, which ensures continuity across cameras.
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-Bug An unexpected or incorrect behavior
Projects
None yet
5 participants