more camera output options #7490

robtfm · 2023-02-03T18:15:53Z

Objective

fix some post-processing issues
provide a means to manage advanced camera composition

Solution

added a CameraOutputMode to the Camera struct. defines whether the camera retains it's results or flushes them to the output. CameraOutputMode::Default will retain results unless the camera is last, in which case it will flush them.
changed upscaling to write back to the next pass's input texture when output mode is Retain, which fixes some issues with post-processing steps being dropped
calculated the camera's CameraChainPosition in extract_cameras, which describes whether a camera is first, first after a flush or neither of those, and
changed ClearColorConfig::Default to do the most appropriate thing in the current camera position
- first -> clear with the global clear color
- first after a flush -> clear with transparent
- not first -> don't clear
(fixed bloom alpha as well - this was necessary to make alpha-blending of bloomed output work correctly)

fix #7435
fix #7361 (you would need to add a Flush to the first camera, so that bloom on the second camera doesn't affect the first camera's results) EDIT: and remove the ClearColorConfig::None from the second camera so it defaults to transparent clear
fix #5721

note - if you use multiple overlapping cameras in Retain mode, you should disable FXAA and tonemapping (and any other fullscreen effects) on all but the last one.

todo:

use remote reflection for the blend state when bevy_reflect: Reflect remote types #6042 lands
maybe move the camera chain calcs out of extract? they are not very expensive as there are generally not a lot of cameras, but could do this if needed

Migration Guide

this should maintain all existing behaviour - i think if it worked before it'll still work now. On secondary cameras, adding 'ClearColorConfig::None` is no longer required but is not harmful.

github-actions · 2023-02-03T18:29:39Z

Example load_gltf failed to run, please try running it locally and check the result.

superdump · 2023-02-07T12:23:04Z

crates/bevy_core_pipeline/Cargo.toml

@@ -31,3 +31,4 @@ bevy_utils = { path = "../bevy_utils", version = "0.9.0" }
 serde = { version = "1", features = ["derive"] }
 bitflags = "1.2"
 radsort = "0.1"
+wgpu = "0.15.0"


superdump · 2023-02-07T12:34:56Z

crates/bevy_render/src/camera/camera.rs

+    /// Flush the current render results to the [`RenderTarget`].
+    /// The blend mode is used to combine results with previous contents of the target. Typically the first flushing
+    /// camera writing to a target will use `Flush(None)` to clear the target and write the current render results.
+    /// If multiple flushing cameras are used, later cameras may prefer to use `Flush(Some(BlendState::ALPHA_BLENDING))`
+    /// to combine render results with previously flushed results.
+    /// if a camera uses `Flush`, the next camera on the the same target should clear the internal render results with
+    /// `ClearColorConfig::Custom(Color::NONE)` to avoid double writing of the first results on the next flush.
+    Flush(Option<wgpu::BlendState>),
+    /// Keep the current render results internally. A later camera must output with `Flush` for any results to be output.
+    /// This is useful for combining the results of several cameras before applying post-processing to the full set of results.
+    /// If a camera uses `Retain`, the next camera on the same target should make sure NOT to clear the render results by
+    /// using `ClearColorConfig::None`, else they will be discarded.
+    Retain,


I feel like the names don't make obvious what the modes are. As I was reading various parts of the PR I wasn't sure if Flush was going to mean something like discarding or writing out what is present. Similarly Retain feels like it could mean keep the results (as in write them out) when compared to Flush, if Flush were understood to mean discard.

How about renaming Flush to Blend or perhaps Combine (I prefer Blend)?

hmm... i was trying to draw an analogy with stream flushing (like stdout). my problem with combine and blend is that they don't really make you feel like you need to do it to get results. but its true that flush can also mean "clear" so it isn't great.

maybe Finish and Continue..?

Yeah, the naming isn't intuitive.

Finish and Continue are clearer, but continue still seems relatively unspecific.

Maybe the combination of Finish and Retain (or even Gather?) might be an option.

changed to Finish and Retain

crates/bevy_core_pipeline/src/bloom/mod.rs

superdump · 2023-02-07T12:40:46Z

Adding this to 0.10 as it looks to clear up quite a few camera composition and post-processing issues.

crates/bevy_core_pipeline/src/clear_color.rs

kurtkuehnert · 2023-02-07T18:32:26Z

crates/bevy_render/src/camera/camera.rs

+    /// Flush the current render results to the [`RenderTarget`].
+    /// The blend mode is used to combine results with previous contents of the target. Typically the first flushing
+    /// camera writing to a target will use `Flush(None)` to clear the target and write the current render results.
+    /// If multiple flushing cameras are used, later cameras may prefer to use `Flush(Some(BlendState::ALPHA_BLENDING))`
+    /// to combine render results with previously flushed results.
+    /// if a camera uses `Flush`, the next camera on the the same target should clear the internal render results with
+    /// `ClearColorConfig::Custom(Color::NONE)` to avoid double writing of the first results on the next flush.
+    Flush(Option<wgpu::BlendState>),
+    /// Keep the current render results internally. A later camera must output with `Flush` for any results to be output.
+    /// This is useful for combining the results of several cameras before applying post-processing to the full set of results.
+    /// If a camera uses `Retain`, the next camera on the same target should make sure NOT to clear the render results by
+    /// using `ClearColorConfig::None`, else they will be discarded.
+    Retain,


Yeah, the naming isn't intuitive.

Finish and Continue are clearer, but continue still seems relatively unspecific.

Maybe the combination of Finish and Retain (or even Gather?) might be an option.

kurtkuehnert

I am not super familiar with the camera and postprocessing code, but the code quality is great and the feature is easy enough to understand from my point of view. LGTM.

cart · 2023-02-09T23:20:15Z

This feels a bit weird to me for a number of reasons. Took me awhile to sort out why it felt wrong, but I think I finally consolidated my thoughts:

It introduces the concept of a "base" texture (aka the "first" texture). This makes the mental model more complicated. I think all that should matter is "which texture is the current texture" and "which texture is the next texture". Within the context of a single camera, this is all that matters. Across cameras, which one is the "current texture" should be an implementation detail. Writing back to a "canonical base intermediate texture" should never be a thing. It seems like this is attempting to solve the "we implicitly reset the RenderTarget's main texture to the first texture between cameras, because each camera has its own index" problem. It seems like the real solution here is to share that index across cameras. That is a "render target property" not a "camera ViewTarget property". Additionally, overloading the "upscaling node" to do this "writeback to base" also feels wrong / unclear. But that problem is solved if we just stop doing writeback.
The new default "clear color is dependent on CameraChainPosition" behavior is complicated / implicit / hard to follow. Ditto for the default CameraOutputMode.

This also doesn't seem to fix #7361. I think I did the recommended config correctly?
(Finish(None) on the first camera, ClearColorConfig::None on the second)
https://gist.github.com/cart/4bdae302232d67ad53cdb3293875c5c2. This feels like a bloom-specific implementation issue? It seems like the "cameras are a direct path from logical views to pixels in RenderTargets" mental model (the one on main) on its own should "sandbox" cameras, provided the second bloom camera does a clear in the main pass.

I think this is my preferred solution (starting from the current state on main):

Make the "current active intermediate texture index" a shared RenderTarget property (across cameras with the same render target). An Arc<AtomicUsize> assigned in prepare_view_targets should do the trick.
Add a simplified CameraOutputMode that just has CameraOutputMode::Skip (skips the upscaling pass entirely) and CameraOutputMode::Write(Option<BlendMode>), which writes the output in the upscaling pass using the given blend mode. This should probably default to CameraOutputMode::Write(Some(BlendMode::ALPHA_BLENDING)), but we can discuss that.
Leave ClearColorConfig untouched. No implicit default stuff.

This way, whenever you access the main_texture for a RenderTarget (at any point in the pipeline across cameras), it will always be valid. If your first camera draws something and your second camera doesn't clear, the second camera will have access to that state, enabling "aggregate camera output" post processing. This removes the need for weird cross-camera statefulness, "automatic implicit defaults", or unnecessary texture writebacks to make things work as expected.

For example:

// draw the scene, skip upscaling, default clear color
CameraX: order = 0, CameraOutputMode::Skip, clearcolorconfig::default
// draw another part of the scene, skip upscaling, retain outputs from X because we dont clear,
CameraY: order = 1, CameraOutputMode::Skip, clearcolorconfig::None 
// draw nothing, but apply post processing effects to retained outputs from X and Y because we dont clear,
// writes to actual render target because we use the default CameraOutputMode::Write mode 
CameraZ: order = 2, CameraOutputMode::default(), clearcolorconfig:None

robtfm · 2023-02-10T01:30:21Z

"which texture is the current texture" and "which texture is the next texture". Within the context of a single camera, this is all that matters

this is made trickier with msaa. after post-processing we are on a non-msaa texture and the next pass wants to start from the multisampled texture so a copy-back is really required. i thought doing the simple thing and always copying back was the easiest to follow (and this will need reworking if/when we do msaa-per-camera in future anyway).

This also doesn't seem to fix #7361. I think I did the recommended config correctly?

in this case you don't want to set ClearColorConfig::None as that means "don't clear" so bloom still affects the first shape. without it it'll default to Custom(None) i.e. clear to transparent and will work -- but you'll need to turn up the bloom power as well since the second camera now doesn't have the 0.1 background colour to bounce the bloom off.

Make the "current active intermediate texture index" a shared RenderTarget property (across cameras with the same render target). An Arc<AtomicUsize> assigned in prepare_view_targets should do the trick.

with msaa i think copy back is required

Add a simplified CameraOutputMode that just has CameraOutputMode::Skip (skips the upscaling pass entirely) and CameraOutputMode::Write(Option<BlendMode>), which writes the output in the upscaling pass using the given blend mode. This should probably default to CameraOutputMode::Write(Some(BlendMode::ALPHA_BLENDING)), but we can discuss that.

i also changed the upscaling to Load instead of Clear so that multiple passes can be composed, so you need the first flushing cam to be Finish(None) or OVERWRITE or thereabouts to clear the previous frame data.

Leave ClearColorConfig untouched. No implicit default stuff.

happy to remove that behaviour if you prefer. i added it since the correct choice isn't always obvious (maybe we should rename ClearColorConfig::None so it's not so easy to mix up with ClearColorConfig::Custom(None)), and wanted to make things work as well as possible without extra config.

cart · 2023-02-10T02:26:01Z

this is made trickier with msaa. after post-processing we are on a non-msaa texture and the next pass wants to start from the multisampled texture so a copy-back is really required. i thought doing the simple thing and always copying back was the easiest to follow (and this will need reworking if/when we do msaa-per-camera in future anyway).

Arg yeah I completely left that out of my mental model. My bad! Definitely complicates things. I'll ponder this for a bit and get back to you.

in this case you don't want to set ClearColorConfig::None as that means "don't clear" so bloom still affects the first shape. without it it'll default to Custom(None) i.e. clear to transparent and will work -- but you'll need to turn up the bloom power as well since the second camera now doesn't have the 0.1 background colour to bounce the bloom off.

That does indeed work!

i also changed the upscaling to Load instead of Clear so that multiple passes can be composed, so you need the first flushing cam to be Finish(None) or OVERWRITE or thereabouts to clear the previous frame data.

Hmm that makes sense. I think making this explicit (and i guess defaulting to overwrite to make the normal case work) might be the move. I'll add it to the list of constraints that I missed and get back to you :)

cart · 2023-02-10T02:28:35Z

My non-serious suggestion is that we remove MSAA in the interest of pipeline purity :)

cart · 2023-02-11T02:34:04Z

Ok so I've been experimenting / poking / prodding / thinking today and it definitely does seem like writeback is required (for MSAA). Here is my "next" proposal:

Add "msaa writeback" (actual name TBD) 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. Note that this moves writeback from the "previous" camera to the "camera that needs MSAA textures to contain the last camera's results". This makes this behavior explicit and scopes it specifically to MSAA.
Use this CameraOutputMode, which makes write behaviors explicit and directly configurable.

#[derive(Debug, Clone, Copy)]
pub enum CameraOutputMode {
    /// Writes the camera output to configured render target.
    Write {
        /// The blend state that will be used by the pipeline that writes the intermediate render textures to the final render target texture.
        blend_state: Option<BlendState>,
        /// The color attachment load operation that will be used by the pipeline that writes the intermediate render textures to the final render
        /// target texture.
        color_attachment_load_op: wgpu::LoadOp<wgpu::Color>,
    },
    /// Skips writing the camera output to the configured render target. The output will remain in the
    /// Render Target's "intermediate" textures, which a camera with a higher order should write to the render target
    /// using [`CameraOutputMode::Write`]. The "skip" mode can easily prevent render results from being displayed, or cause
    /// them to be lost. Only use this if you know what you are doing!
    Skip,
}

Default to Write { blend_state: None, color_attachment_load_op: LoadOp::Clear(Default::default())}, which should produce the correct outputs for most multi-camera situations. Anything more complicated has direct access to the knobs required to configure behavior.
Apply the shared Arc<AtomicUsize> "main texture RenderTarget tracker" changes described in my last comment.

I have everything but (1) implemented, but I'm leaving for the rest of the night (and all of tomorrow) starting now, so if anyone beats me to the final impl I wont mind. If we go with my branch I'll add @robtfm as a co-author as this is largely just a reframing of the work done here / theres no way I would have come up with this on my own without way more time invested.

cart · 2023-02-11T02:41:46Z

Also this is just a proposal. If I've (once again) missed something important please let me know. Very open to feedback!

cart · 2023-02-15T23:36:25Z

Closing in favor of #7671

@robtfm

# Objective Alternative to #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 #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 #7435 Fixes #7361 Fixes #5721 ## Solution This implements the solution I [outlined here](#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.

@robtfm

# 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.

robtfm added 4 commits February 3, 2023 16:14

all the output modes

34106d1

fix webgl

d2b70a4

docs and ci

eeae519

docs again

600d626

robtfm mentioned this pull request Feb 3, 2023

fix some post-processing issues #7474

Closed

3 tasks

alice-i-cecile added C-Bug An unexpected or incorrect behavior A-Rendering Drawing game state to the screen C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Feb 3, 2023

IceSentry self-requested a review February 7, 2023 06:42

superdump self-requested a review February 7, 2023 12:22

superdump reviewed Feb 7, 2023

View reviewed changes

superdump requested a review from cart February 7, 2023 12:40

superdump added this to the 0.10 milestone Feb 7, 2023

kurtkuehnert reviewed Feb 7, 2023

View reviewed changes

robtfm added 2 commits February 8, 2023 13:23

review feedback

281904c

Merge branch 'main' into camera_output

7c1a869

kurtkuehnert approved these changes Feb 9, 2023

View reviewed changes

cart mentioned this pull request Feb 14, 2023

[Merged by Bors] - Camera Output Modes, MSAA Writeback, and BlitPipeline #7671

Closed

cart closed this Feb 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

more camera output options #7490

more camera output options #7490

robtfm commented Feb 3, 2023 •

edited

Loading

github-actions bot commented Feb 3, 2023

superdump Feb 7, 2023

superdump Feb 7, 2023

robtfm Feb 7, 2023

kurtkuehnert Feb 7, 2023

robtfm Feb 8, 2023

superdump commented Feb 7, 2023

kurtkuehnert Feb 7, 2023

kurtkuehnert left a comment

cart commented Feb 9, 2023 •

edited

Loading

robtfm commented Feb 10, 2023

cart commented Feb 10, 2023

cart commented Feb 10, 2023

cart commented Feb 11, 2023 •

edited

Loading

cart commented Feb 11, 2023

cart commented Feb 15, 2023

more camera output options #7490

more camera output options #7490

Conversation

robtfm commented Feb 3, 2023 • edited Loading

Objective

Solution

Migration Guide

github-actions bot commented Feb 3, 2023

superdump Feb 7, 2023

Choose a reason for hiding this comment

superdump Feb 7, 2023

Choose a reason for hiding this comment

robtfm Feb 7, 2023

Choose a reason for hiding this comment

kurtkuehnert Feb 7, 2023

Choose a reason for hiding this comment

robtfm Feb 8, 2023

Choose a reason for hiding this comment

superdump commented Feb 7, 2023

kurtkuehnert Feb 7, 2023

Choose a reason for hiding this comment

kurtkuehnert left a comment

Choose a reason for hiding this comment

cart commented Feb 9, 2023 • edited Loading

robtfm commented Feb 10, 2023

cart commented Feb 10, 2023

cart commented Feb 10, 2023

cart commented Feb 11, 2023 • edited Loading

cart commented Feb 11, 2023

cart commented Feb 15, 2023

robtfm commented Feb 3, 2023 •

edited

Loading

cart commented Feb 9, 2023 •

edited

Loading

cart commented Feb 11, 2023 •

edited

Loading