Skip to content

TrackedRenderPass Ergonomics#7227

Closed
kurtkuehnert wants to merge 2 commits intobevyengine:mainfrom
kurtkuehnert:tracked_render_pass_ergonomics
Closed

TrackedRenderPass Ergonomics#7227
kurtkuehnert wants to merge 2 commits intobevyengine:mainfrom
kurtkuehnert:tracked_render_pass_ergonomics

Conversation

@kurtkuehnert
Copy link
Contributor

@kurtkuehnert kurtkuehnert commented Jan 16, 2023

Objective

The current code for creating render passes and executing render phases is quite indentation-heavy and repetitive.

I have experimented with further improving the API, based on the proposal by @cart (posted in #7043 (comment)):

// This would create and set up a TrackedRenderPass, which would set the viewport, if it is
// configured on the camera. "pass" would need to be a wrapper over TrackedRenderPass.
let pass = camera.begin_render_pass(render_context, view_entity);
pass.render_phase(opaque_phase, world);

Example

First, let me show you a comparison between the old and the new API.

// Bevy Main
let mut render_pass = render_context.begin_tracked_render_pass(RenderPassDescriptor {
    label: Some("main_alpha_mask_pass_3d"),
    // NOTE: The alpha_mask pass loads the color buffer as well as overwriting it where appropriate.
    color_attachments: &[Some(target.get_color_attachment(Operations {
        load: LoadOp::Load,
        store: true,
    }))],
    depth_stencil_attachment: Some(RenderPassDepthStencilAttachment {
        view: &depth.view,
        // NOTE: The alpha mask pass loads the depth buffer and possibly overwrites it
        depth_ops: Some(Operations {
            load: LoadOp::Load,
            store: true,
        }),
        stencil_ops: None,
    }),
});

if let Some(viewport) = camera.viewport.as_ref() {
    render_pass.set_camera_viewport(viewport);
}

alpha_mask_phase.render(&mut render_pass, world, view_entity);

// this PR
render_context
    .render_pass(view_entity)
    .set_label("main_alpha_mask_pass_3d")
    .add_view_target(target)
    .set_color_ops(LoadOp::Load, true)
    .set_depth_stencil_attachment(&depth.view)
    .set_depth_ops(LoadOp::Load, true)
    .begin()
    .set_camera_viewport(camera)
    .render_phase(alpha_mask_phase, world);

Solution

Cart's solution is not directly realizable, since we have no central camera object.
Instead, I propose a builder-style API for setting up the TrackedRenderPass.

This PR comprises multiple improvements (which can be split apart and merged separately, if desired. I wanted to showcase them all at once first.)

1. I have added a TrackedRenderPassBuilder, which sets up the render pass's attachments.

  • In TrackedRenderPass driven RenderPhases #7080 I have tried implementing this as a single method similar to the one proposed by @cart. I do not think that this is the right design. Due to the many optional types, a builder-style API seems like the better fit.
  • The views of the attachments, are added separately from their operations, to keep nesting to a minimum and make use of default operations.
  • Let me know if you have got suggestions or differing opinions regarding this API.
  • Contrary to my previous attempts this new version supports all usages through the bevy code base.
  • I believe we should consider merging the ViewTarget and the ViewDepthTexture into a single struct (in a follow up PR).
// Bevy main
let mut render_pass = render_context.begin_tracked_render_pass(RenderPassDescriptor {
    label: Some("main_opaque_pass_3d"),
    // NOTE: The opaque pass loads the color
    // buffer as well as writing to it.
    color_attachments: &[Some(target.get_color_attachment(Operations {
        load: match camera_3d.clear_color {
            ClearColorConfig::Default => {
            LoadOp::Clear(world.resource::<ClearColor>().0.into())
            }
            ClearColorConfig::Custom(color) => LoadOp::Clear(color.into()),
            ClearColorConfig::None => LoadOp::Load,
        },
        store: true,
    }))],
    depth_stencil_attachment: Some(RenderPassDepthStencilAttachment {
        view: &depth.view,
        // NOTE: The opaque main pass loads the depth buffer and possibly overwrites it
        depth_ops: Some(Operations {
            load: if depth_prepass.is_some() || normal_prepass.is_some() {
                // if any prepass runs, it will generate a depth buffer so we should use it,
                // even if only the normal_prepass is used.
                Camera3dDepthLoadOp::Load
            } else {
                // NOTE: 0.0 is the far plane due to bevy's use of reverse-z projections.
                camera_3d.depth_load_op.clone()
            }
            .into(),
            store: true,
        }),
        stencil_ops: None,
    }),
});

// cart
let mut render_pass = camera.begin_render_pass(render_context, view_entity);

// #7080
let mut render_pass = TrackedRenderPass::create_for_camera(
    render_context,
    "main_alpha_mask_pass_3d",
    view_entity,
    target,
    Operations {
    load: LoadOp::Load,
    store: true,
    },
    Some(depth),
    Some(Operations {
    load: LoadOp::Load,
    store: true,
    }),
    &camera.viewport,
);

// this PR
let depth_load_op = if depth_prepass.is_some() || normal_prepass.is_some() {
    // if any prepass runs, it will generate a depth buffer so we should use it,
    // even if only the normal_prepass is used.
    LoadOp::Load
} else {
    camera_3d.depth_load_op.clone().into()
};

render_context
    .render_pass(view_entity)
    .set_label("main_opaque_pass_3d")
    .add_view_target(target)
    .set_color_ops(camera_3d.clear_color.load_op(world), true)
    .set_depth_stencil_attachment(&depth.view)
    .set_depth_ops(depth_load_op, true)
    .begin()
    .set_camera_viewport(camera)
    .render_phase(opaque_phase, world);

2. The RenderPhases are now executed directly using the TrackedRenderPass, with the API described above.

  • Therefore, each TrackedRenderPass is associated with a view_entity.
  • Additionally, this alleviates the need to pass around the view_entitys in the render method of Draw functions. Instead, they can be accessed directly from the TrackedRenderPass if they are needed.
  • I believe that conceptually this is now way more clear and discoverable. You execute zero or multiple render phases on a render pass.
// old
opaque_phase.render(&mut render_pass, world, view_entity);

// new
render_pass.render_phase(opaque_phase, world);

3. Changed TrackedRenderPass::set_camera_viewport to take a &ExtractedCamera instead of a &Viewport

  • this minor change avoids a lot of repetition
// bevy main
if let Some(viewport) = camera.viewport.as_ref() {
    render_pass.set_camera_viewport(viewport);
}
// this PR
render_pass.set_camera_viewport(camera);

4. Chainable TrackedRenderPass methods

  • For convenience, I have made all methods of the TrackedRenderPass return a mutable reference to Self.
  • This allows us to chain multiple of these calls.
// bevy main
downsampling_pass.set_render_pipeline(downsampling_pipeline);
downsampling_pass.set_bind_group(
     0, 
     &bind_groups.downsampling_bind_groups[mip as usize - 1],
     &[uniform_index.index()],
);
if let Some(viewport) = camera.viewport.as_ref() {
    downsampling_pass.set_camera_viewport(viewport);
}
downsampling_pass.draw(0..3, 0..1);

// this PR
downsampling_pass
    .set_camera_viewport(camera)
    .set_render_pipeline(downsampling_pipeline)
    .set_bind_group(
        0,
        &bind_groups.downsampling_bind_groups[mip as usize - 1], 
        &[uniform_index.index()],
     )
     .draw(0..3, 0..1);

5. Added load_op method to ClearColorConfig

  • This tiny change removes a lot of code duplication and indentation as well.
// Bevy Main
match camera_2d.clear_color {
    ClearColorConfig::Default => {
        LoadOp::Clear(world.resource::<ClearColor>().0.into())
    }
    ClearColorConfig::Custom(color) => LoadOp::Clear(color.into()),
    ClearColorConfig::None => LoadOp::Load,
}

// this PR
camera_2d.clear_color.load_op(world)
  • Add Documentation
  • Write Changelog
  • Write Migration Guide

Changelog

Todo

Migration Guide

Todo

@kurtkuehnert kurtkuehnert added A-Rendering Drawing game state to the screen C-Code-Quality A section of code that is hard to understand or change C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Jan 16, 2023
@kurtkuehnert kurtkuehnert force-pushed the tracked_render_pass_ergonomics branch from bcdef6b to bb7b5d4 Compare January 23, 2023 13:01
.set_label("bloom_prefilter_pass")
.add_color_attachment(view)
.begin()
.set_camera_viewport(camera)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I should probably remove this line.


impl ClearColorConfig {
#[inline]
pub fn load_op(&self, world: &World) -> LoadOp<RawColor> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Todo: needs documentation

Comment on lines +336 to +354
pub(crate) fn begin_tracked_render_pass<'a>(
&'a mut self,
descriptor: RenderPassDescriptor<'a, '_>,
view_entity: Entity,
) -> TrackedRenderPass<'a> {
// Cannot use command_encoder() as we need to split the borrow on self
let command_encoder = self.command_encoder.get_or_insert_with(|| {
self.render_device
.create_command_encoder(&wgpu::CommandEncoderDescriptor::default())
});
TrackedRenderPass::new(
&self.render_device,
command_encoder.begin_render_pass(&descriptor),
view_entity,
)
}
Copy link
Contributor Author

@kurtkuehnert kurtkuehnert Jan 23, 2023

Choose a reason for hiding this comment

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

This method is still required, due to the split borrow.

@kurtkuehnert
Copy link
Contributor Author

IMO we should consider merging TrackedRenderPass and RenderPass into a single abstraction.

@kurtkuehnert kurtkuehnert mentioned this pull request Feb 7, 2023
2 tasks
@kurtkuehnert kurtkuehnert force-pushed the tracked_render_pass_ergonomics branch from 26dc71f to 4ff6233 Compare February 14, 2023 08:31
…r API

updated builder API to support arbitrary attachments
TrackedRenderPass methods can be chained
associated TrackedRenderPass with the view Entity
added render_phase method to TrackedRenderPass
added TrackedRenderPassBuilder
changed set_camera_viewport method to take &Camera instead of &Viewport
@kurtkuehnert kurtkuehnert force-pushed the tracked_render_pass_ergonomics branch from 4ff6233 to 23b9e18 Compare March 9, 2023 14:01
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-Code-Quality A section of code that is hard to understand or change C-Usability A targeted quality-of-life change that makes Bevy easier to use

Projects

No open projects
Status: Done

Development

Successfully merging this pull request may close these issues.

1 participant