Skip to content

Commit

Permalink
Rework ViewTarget to better support post processing (bevyengine#6415)
Browse files Browse the repository at this point in the history
# Objective

Post processing effects cannot read and write to the same texture. Currently they must own their own intermediate texture and redundantly copy from that back to the main texture. This is very inefficient.

Additionally, working with ViewTarget is more complicated than it needs to be, especially when working with HDR textures. 

## Solution

`ViewTarget` now stores two copies of the "main texture". It uses an atomic value to track which is currently the "main texture" (this interior mutability is necessary to accommodate read-only RenderGraph execution). 

`ViewTarget` now has a `post_process_write` method, which will return a source and destination texture. Each call to this method will flip between the two copies of the "main texture".

```rust
let post_process = render_target.post_process_write();
let source_texture = post_process.source;
let destination_texture = post_process.destination;
```
The caller _must_ read from the source texture and write to the destination texture, as it is assumed that the destination texture will become the new "main texture".


For simplicity / understandability `ViewTarget` is now a flat type. "hdr-ness" is a property of the `TextureFormat`. The internals are fully private in the interest of providing simple / consistent apis. Developers can now easily access the main texture by calling `view_target.main_texture()`.

HDR ViewTargets no longer have an "ldr texture" with `TextureFormat::bevy_default`. They _only_ have their two "hdr" textures. This simplifies the mental model.  All we have is the "currently active hdr texture" and the "other hdr texture", which we flip between for post processing effects.

The tonemapping node has been rephrased to use this "post processing pattern". The blit pass has been removed, and it now only runs a pass when HDR is enabled. Notably, both the input and output texture are assumed to be HDR. This means that tonemapping behaves just like any other "post processing effect". It could theoretically be moved anywhere in the "effect chain" and continue to work.

In general, I think these changes will make the lives of people making post processing effects much easier. And they better position us to start building higher level / more structured  "post processing effect stacks".

---

## Changelog

- `ViewTarget` now stores two copies of the "main texture". Calling `ViewTarget::post_process_write` will flip between copies of the main texture.
  • Loading branch information
cart authored and ItsDoot committed Feb 1, 2023
1 parent a715e10 commit 8564ff1
Show file tree
Hide file tree
Showing 7 changed files with 181 additions and 199 deletions.
11 changes: 0 additions & 11 deletions crates/bevy_core_pipeline/src/tonemapping/blit.wgsl

This file was deleted.

46 changes: 10 additions & 36 deletions crates/bevy_core_pipeline/src/tonemapping/mod.rs
@@ -1,30 +1,25 @@
mod node;

use bevy_ecs::query::QueryItem;
use bevy_render::camera::Camera;
use bevy_render::extract_component::{ExtractComponent, ExtractComponentPlugin};
pub use node::TonemappingNode;

use crate::fullscreen_vertex_shader::fullscreen_shader_vertex_state;
use bevy_app::prelude::*;
use bevy_asset::{load_internal_asset, HandleUntyped};
use bevy_ecs::prelude::*;
use bevy_ecs::query::QueryItem;
use bevy_reflect::TypeUuid;
use bevy_render::camera::Camera;
use bevy_render::extract_component::{ExtractComponent, ExtractComponentPlugin};
use bevy_render::renderer::RenderDevice;
use bevy_render::texture::BevyDefault;
use bevy_render::view::ViewTarget;
use bevy_render::{render_resource::*, RenderApp};

use bevy_reflect::TypeUuid;

use crate::fullscreen_vertex_shader::fullscreen_shader_vertex_state;

const TONEMAPPING_SHADER_HANDLE: HandleUntyped =
HandleUntyped::weak_from_u64(Shader::TYPE_UUID, 17015368199668024512);

const TONEMAPPING_SHARED_SHADER_HANDLE: HandleUntyped =
HandleUntyped::weak_from_u64(Shader::TYPE_UUID, 2499430578245347910);

const BLIT_SHADER_HANDLE: HandleUntyped =
HandleUntyped::weak_from_u64(Shader::TYPE_UUID, 2982361071241723543);

pub struct TonemappingPlugin;

impl Plugin for TonemappingPlugin {
Expand All @@ -41,7 +36,6 @@ impl Plugin for TonemappingPlugin {
"tonemapping_shared.wgsl",
Shader::from_wgsl
);
load_internal_asset!(app, BLIT_SHADER_HANDLE, "blit.wgsl", Shader::from_wgsl);

app.add_plugin(ExtractComponentPlugin::<Tonemapping>::default());

Expand All @@ -53,9 +47,8 @@ impl Plugin for TonemappingPlugin {

#[derive(Resource)]
pub struct TonemappingPipeline {
hdr_texture_bind_group: BindGroupLayout,
texture_bind_group: BindGroupLayout,
tonemapping_pipeline_id: CachedRenderPipelineId,
blit_pipeline_id: CachedRenderPipelineId,
}

impl FromWorld for TonemappingPipeline {
Expand Down Expand Up @@ -91,9 +84,9 @@ impl FromWorld for TonemappingPipeline {
fragment: Some(FragmentState {
shader: TONEMAPPING_SHADER_HANDLE.typed(),
shader_defs: vec![],
entry_point: "fs_main".into(),
entry_point: "fragment".into(),
targets: vec![Some(ColorTargetState {
format: TextureFormat::bevy_default(),
format: ViewTarget::TEXTURE_FORMAT_HDR,
blend: None,
write_mask: ColorWrites::ALL,
})],
Expand All @@ -103,29 +96,10 @@ impl FromWorld for TonemappingPipeline {
multisample: MultisampleState::default(),
};

let blit_descriptor = RenderPipelineDescriptor {
label: Some("blit pipeline".into()),
layout: Some(vec![tonemap_texture_bind_group.clone()]),
vertex: fullscreen_shader_vertex_state(),
fragment: Some(FragmentState {
shader: BLIT_SHADER_HANDLE.typed(),
shader_defs: vec![],
entry_point: "fs_main".into(),
targets: vec![Some(ColorTargetState {
format: TextureFormat::bevy_default(),
blend: None,
write_mask: ColorWrites::ALL,
})],
}),
primitive: PrimitiveState::default(),
depth_stencil: None,
multisample: MultisampleState::default(),
};
let mut cache = render_world.resource_mut::<PipelineCache>();
TonemappingPipeline {
hdr_texture_bind_group: tonemap_texture_bind_group,
texture_bind_group: tonemap_texture_bind_group,
tonemapping_pipeline_id: cache.queue_render_pipeline(tonemap_descriptor),
blit_pipeline_id: cache.queue_render_pipeline(blit_descriptor),
}
}
}
Expand Down
36 changes: 15 additions & 21 deletions crates/bevy_core_pipeline/src/tonemapping/node.rs
Expand Up @@ -11,7 +11,7 @@ use bevy_render::{
TextureViewId,
},
renderer::RenderContext,
view::{ExtractedView, ViewMainTexture, ViewTarget},
view::{ExtractedView, ViewTarget},
};

pub struct TonemappingNode {
Expand Down Expand Up @@ -54,31 +54,25 @@ impl Node for TonemappingNode {
Err(_) => return Ok(()),
};

let ldr_texture = match &target.main_texture {
ViewMainTexture::Hdr { ldr_texture, .. } => ldr_texture,
ViewMainTexture::Sdr { .. } => {
// non-hdr does tone mapping in the main pass node
return Ok(());
}
};

let tonemapping_enabled = tonemapping.map_or(false, |t| t.is_enabled);
let pipeline_id = if tonemapping_enabled {
tonemapping_pipeline.tonemapping_pipeline_id
} else {
tonemapping_pipeline.blit_pipeline_id
};
if !tonemapping_enabled || !target.is_hdr() {
return Ok(());
}

let pipeline = match pipeline_cache.get_render_pipeline(pipeline_id) {
let pipeline = match pipeline_cache
.get_render_pipeline(tonemapping_pipeline.tonemapping_pipeline_id)
{
Some(pipeline) => pipeline,
None => return Ok(()),
};

let main_texture = target.main_texture.texture();
let post_process = target.post_process_write();
let source = post_process.source;
let destination = post_process.destination;

let mut cached_bind_group = self.cached_texture_bind_group.lock().unwrap();
let bind_group = match &mut *cached_bind_group {
Some((id, bind_group)) if main_texture.id() == *id => bind_group,
Some((id, bind_group)) if source.id() == *id => bind_group,
cached_bind_group => {
let sampler = render_context
.render_device
Expand All @@ -89,11 +83,11 @@ impl Node for TonemappingNode {
.render_device
.create_bind_group(&BindGroupDescriptor {
label: None,
layout: &tonemapping_pipeline.hdr_texture_bind_group,
layout: &tonemapping_pipeline.texture_bind_group,
entries: &[
BindGroupEntry {
binding: 0,
resource: BindingResource::TextureView(main_texture),
resource: BindingResource::TextureView(source),
},
BindGroupEntry {
binding: 1,
Expand All @@ -102,15 +96,15 @@ impl Node for TonemappingNode {
],
});

let (_, bind_group) = cached_bind_group.insert((main_texture.id(), bind_group));
let (_, bind_group) = cached_bind_group.insert((source.id(), bind_group));
bind_group
}
};

let pass_descriptor = RenderPassDescriptor {
label: Some("tonemapping_pass"),
color_attachments: &[Some(RenderPassColorAttachment {
view: ldr_texture,
view: destination,
resolve_target: None,
ops: Operations {
load: LoadOp::Clear(Default::default()), // TODO shouldn't need to be cleared
Expand Down
2 changes: 1 addition & 1 deletion crates/bevy_core_pipeline/src/tonemapping/tonemapping.wgsl
Expand Up @@ -7,7 +7,7 @@ var hdr_texture: texture_2d<f32>;
var hdr_sampler: sampler;

@fragment
fn fs_main(in: FullscreenVertexOutput) -> @location(0) vec4<f32> {
fn fragment(in: FullscreenVertexOutput) -> @location(0) vec4<f32> {
let hdr_color = textureSample(hdr_texture, hdr_sampler, in.uv);

return vec4<f32>(reinhard_luminance(hdr_color.rgb), hdr_color.a);
Expand Down
14 changes: 6 additions & 8 deletions crates/bevy_core_pipeline/src/upscaling/mod.rs
Expand Up @@ -38,16 +38,16 @@ impl Plugin for UpscalingPlugin {

#[derive(Resource)]
pub struct UpscalingPipeline {
ldr_texture_bind_group: BindGroupLayout,
texture_bind_group: BindGroupLayout,
}

impl FromWorld for UpscalingPipeline {
fn from_world(render_world: &mut World) -> Self {
let render_device = render_world.resource::<RenderDevice>();

let ldr_texture_bind_group =
let texture_bind_group =
render_device.create_bind_group_layout(&BindGroupLayoutDescriptor {
label: Some("upscaling_ldr_texture_bind_group_layout"),
label: Some("upscaling_texture_bind_group_layout"),
entries: &[
BindGroupLayoutEntry {
binding: 0,
Expand All @@ -68,9 +68,7 @@ impl FromWorld for UpscalingPipeline {
],
});

UpscalingPipeline {
ldr_texture_bind_group,
}
UpscalingPipeline { texture_bind_group }
}
}

Expand All @@ -92,7 +90,7 @@ impl SpecializedRenderPipeline for UpscalingPipeline {
fn specialize(&self, key: Self::Key) -> RenderPipelineDescriptor {
RenderPipelineDescriptor {
label: Some("upscaling pipeline".into()),
layout: Some(vec![self.ldr_texture_bind_group.clone()]),
layout: Some(vec![self.texture_bind_group.clone()]),
vertex: fullscreen_shader_vertex_state(),
fragment: Some(FragmentState {
shader: UPSCALING_SHADER_HANDLE.typed(),
Expand Down Expand Up @@ -126,7 +124,7 @@ fn queue_upscaling_bind_groups(
for (entity, view_target) in view_targets.iter() {
let key = UpscalingPipelineKey {
upscaling_mode: UpscalingMode::Filtering,
texture_format: view_target.out_texture_format,
texture_format: view_target.out_texture_format(),
};
let pipeline = pipelines.specialize(&mut pipeline_cache, &upscaling_pipeline, key);

Expand Down
11 changes: 4 additions & 7 deletions crates/bevy_core_pipeline/src/upscaling/node.rs
Expand Up @@ -56,10 +56,7 @@ impl Node for UpscalingNode {
Err(_) => return Ok(()),
};

let upscaled_texture = match &target.main_texture {
bevy_render::view::ViewMainTexture::Hdr { ldr_texture, .. } => ldr_texture,
bevy_render::view::ViewMainTexture::Sdr { texture, .. } => texture,
};
let upscaled_texture = target.main_texture();

let mut cached_bind_group = self.cached_texture_bind_group.lock().unwrap();
let bind_group = match &mut *cached_bind_group {
Expand All @@ -74,7 +71,7 @@ impl Node for UpscalingNode {
.render_device
.create_bind_group(&BindGroupDescriptor {
label: None,
layout: &upscaling_pipeline.ldr_texture_bind_group,
layout: &upscaling_pipeline.texture_bind_group,
entries: &[
BindGroupEntry {
binding: 0,
Expand All @@ -100,10 +97,10 @@ impl Node for UpscalingNode {
let pass_descriptor = RenderPassDescriptor {
label: Some("upscaling_pass"),
color_attachments: &[Some(RenderPassColorAttachment {
view: &target.out_texture,
view: target.out_texture(),
resolve_target: None,
ops: Operations {
load: LoadOp::Clear(Default::default()), // TODO dont_care
load: LoadOp::Clear(Default::default()),
store: true,
},
})],
Expand Down

0 comments on commit 8564ff1

Please sign in to comment.