Skip to content

Conversation

@JMS55
Copy link
Contributor

@JMS55 JMS55 commented Oct 19, 2025

Objective

  • Reduce boilerplate for render nodes, building on my experience writing SSAO, TAA, virtual geometry, Solari, etc
  • Provide the missing low-level render API helper:
    • High level - Material and FullscreenMaterial
    • Mid level - Phase stuff
    • Low level - Missing until now

Solution

  • Provide a new RenderTask trait to handle all the boilerplate you would otherwise have to write manually:
    • Checking GPU features/limits when setting up the plugin
    • Setting up a render graph node and edge ordering
    • Preparing and caching textures and buffers
    • Preparing and caching pipelines
    • Preparing and caching bind groups/layouts
    • Setting up wgpu compute/render passes and setting state between dispatches/draws
    • Inserting profiling spans

Testing

  • Did you test these changes? If so, how?
  • Are there any parts that need more testing?
  • How can other people (reviewers) test your changes? Is there anything specific they need to know?
  • If relevant, what platforms did you test these changes on, and are there any important ones you can't test?

Showcase

impl RenderTask for SolariLighting {
    type RenderNodeLabel = node::graph::SolariLightingNode;
    type RenderNodeSubGraph = Core3d;
    fn render_node_ordering() -> impl IntoRenderNodeArray {
        (
            Node3d::EndPrepasses,
            node::graph::SolariLightingNode,
            Node3d::EndMainPass,
        )
    }

    fn encode_commands(&self, mut encoder: RenderTaskEncoder, entity: Entity, world: &World) {
        let Some((
            solari_lighting_resources,
            view_target,
            view_prepass_textures,
            view_uniform_offset,
            previous_view_uniform_offset,
        )) = world.entity(entity).get_components::<(
            &SolariLightingResources,
            &ViewTarget,
            &ViewPrepassTextures,
            &ViewUniformOffset,
            &PreviousViewUniformOffset,
        )>()
        else {
            return;
        };

        let frame_count = world.resource::<FrameCount>();
        let frame_index = frame_count.0.wrapping_mul(5782582);

        let push_constants = &[frame_index, self.reset as u32];

        encoder
            .compute_pass("presample_light_tiles")
            .shader(load_embedded_asset!(world, "presample_light_tiles.wgsl"))
            .push_constants(push_constants)
            .dispatch_1d(LIGHT_TILE_BLOCKS as u32);
    }
}

@JMS55 JMS55 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 D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes labels Oct 19, 2025
@torsteingrindvik
Copy link
Contributor

Is ordering always (before, myself, after)? If so is it enough to specify before and after?

@JMS55
Copy link
Contributor Author

JMS55 commented Oct 19, 2025

Is ordering always (before, myself, after)? If so is it enough to specify before and after?

I suppose yeah, but before/after can be more than 1 thing.

@atlv24
Copy link
Contributor

atlv24 commented Oct 19, 2025

I think @torsteingrindvik was suggesting having

    type AfterNodeLabel = Node3d::EndPrepasses;
    type RenderNodeLabel = node::graph::SolariLightingNode;
    type BeforeNodeLabel = Node3d::EndMainPass;
    
    // this is the same for all RenderTasks
    fn render_node_ordering() -> impl IntoRenderNodeArray {
        (
            Self::AfterNodeLabel,
            Self::RenderNodeLabel,
            Self::BeforeNodeLabel,
        )
    }

@JMS55
Copy link
Contributor Author

JMS55 commented Oct 19, 2025

No yeah I get the idea. I just don't think we can do that because you could want more than 1 thing before/after.

@JMS55 JMS55 marked this pull request as ready for review October 31, 2025 17:53
@JMS55 JMS55 added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Oct 31, 2025
@IceSentry IceSentry self-requested a review October 31, 2025 18:26
@tychedelia
Copy link
Member

Need to review more closely, but I'm a little on the fence about this. On the one hand, I've wanted to implement something exactly this shape before for similar reasons. But I've hesitated in the past because this kind of approach feels a bit like patching over fundamental deficiencies of our render graph architecture rather than a real solution. I worry that these kinds of trait based solutions are brittle and often do not reduce as much boilerplate as we would like. I've often had the feeling when working with ECS that things can be ugly but them being ad-hoc is still preferable to being formalized prematurely into a clunky abstraction, which is what I worry about here.

@cart
Copy link
Member

cart commented Oct 31, 2025

but I'm a little on the fence about this

Same. Adding a new layer of abstraction should be a method of last resort, and I'm not (yet) convinced this is a net win. It would be helpful to port a few Bevy features over in this PR to illustrate the value here (and also show that the abstraction is functional).

Comment on lines +57 to +61
todo!()
// self.bind_groups
// .entry(descriptor.clone())
// .or_insert_with(|| render_device.wgpu_device().create_bind_group(&descriptor))
// .clone()
Copy link
Contributor

Choose a reason for hiding this comment

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

?

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 haven't implemented caching for bind groups yet. Tricky to figure out how to do the hashing.

Comment on lines +16 to +20
textures: HashMap<(Entity, TextureDescriptor<'static>), TextureView>,
buffers: HashMap<(Entity, BufferDescriptor<'static>), Buffer>,
_bind_groups: HashMap<BindGroupDescriptor<'static>, BindGroup>,
compute_pipelines: HashMap<ComputePipelineDescriptor, CachedComputePipelineId>,
render_pipelines: HashMap<RenderPipelineDescriptor, CachedRenderPipelineId>,
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if some of these should be separate / global and not constrained to one render task instance?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They could be, but I don't see much point in it, and it would reduce parallelism unless we used a parallel hashmap.


/// Begin a new render pass.
pub fn render_pass(&mut self) {
todo!()
Copy link
Contributor

Choose a reason for hiding this comment

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

D:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's easy enough to add later, but I wanted to focus on compute first.

pub struct RenderTaskContext<'a> {
camera_entity: Entity,
command_encoder: &'a mut CommandEncoder,
compute_pass: Option<ComputePass<'static>>,
Copy link
Contributor

Choose a reason for hiding this comment

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

That this is 'static looks surprising to me. It feels like it should have its own lifetime that is bound to the lifetime of the CommandEncoder, which is 'a?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a little bit of a hack because I can't express the lifetime properly. Notice that I used forget_lifetime.

Comment on lines 9 to 35
/// Corresponds to `var<storage, read> my_buffer: T` in a WGSL shader.
#[derive(Clone, Deref)]
pub struct StorageBufferReadOnly<'a>(pub &'a Buffer);

/// Corresponds to `var<storage, read_write> my_buffer: T` in a WGSL shader.
#[derive(Clone, Deref)]
pub struct StorageBufferReadWrite<'a>(pub &'a Buffer);

/// Corresponds to `var my_texture: texture_2d<T>` in a WGSL shader.
#[derive(Clone, Deref)]
pub struct SampledTexture<'a>(pub &'a TextureView);

/// Corresponds to `var my_texture: texture_storage_2d<F, read_write>` in a WGSL shader.
#[derive(Clone, Deref)]
pub struct StorageTextureReadWrite<'a>(pub &'a TextureView);

/// Corresponds to `var my_texture: texture_storage_2d<F, write>` in a WGSL shader.
#[derive(Clone, Deref)]
pub struct StorageTextureWriteOnly<'a>(pub &'a TextureView);

/// Corresponds to `var my_texture: texture_storage_2d<F, read>` in a WGSL shader.
#[derive(Clone, Deref)]
pub struct StorageTextureReadOnly<'a>(pub &'a TextureView);

/// Corresponds to `var my_texture: texture_storage_2d<F, atomic>` in a WGSL shader.
#[derive(Clone, Deref)]
pub struct StorageTextureAtomic<'a>(pub &'a TextureView);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not keen on this. Adding separate types for all combinations of buffer and texture binding types feel 'undesirable'?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why so? You need to specify the bindings types somewhere.

Comment on lines 124 to 140
pub fn dispatch_1d(mut self, x: u32) -> Option<Self> {
self.setup_state()?;
self.pass.dispatch_workgroups(x, 1, 1);
Some(self)
}

pub fn dispatch_2d(mut self, x: u32, y: u32) -> Option<Self> {
self.setup_state()?;
self.pass.dispatch_workgroups(x, y, 1);
Some(self)
}

pub fn dispatch_3d(mut self, x: u32, y: u32, z: u32) -> Option<Self> {
self.setup_state()?;
self.pass.dispatch_workgroups(x, y, z);
Some(self)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there much value in these three instead of just dispatch_3d called dispatch?

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 mean sure, but I think it's nice compared to doing dispatch(x, y, 1).

use bevy_camera::Camera;
use bevy_ecs::system::{Commands, Query};

// TODO: Use SyncComponentPlugin or ExtractComponentPlugin?
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm 90% sure that ExtractComponentPlugin handles everything you're trying to do here. Including handling when a Camera is removed.

Maybe not mutating T in the main world, but then again. I'm not quite sure of all the things you're trying to do with RenderTask. If you have more precise questions, I think I can answer them though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I think you're right. And I think I can get away with not needing to handle main world mutations if I move temporal reset tracking to a dedicated component.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Except handling camera.is_active, actually.

@JMS55 JMS55 marked this pull request as draft November 26, 2025 03:30
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 D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes M-Release-Note Work that should be called out in the blog due to impact S-Needs-Review Needs reviewer attention (from anyone!) to move forward

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

9 participants