RenderGraph Labelization

[DasLixou]

Objective

The whole Cow<'static, str> naming for nodes and subgraphs in RenderGraph is a mess.

Solution

Replaces hardcoded and potentially overlapping strings for nodes and subgraphs inside RenderGraph with bevy's labelsystem.

---

Changelog

• Two new labels: RenderLabel and RenderSubGraph.
• Replaced all uses for hardcoded strings with those labels
• Moved Taa label from its own mod to all the other Labels3d
• add_render_graph_edges now needs a tuple of labels
• Moved ScreenSpaceAmbientOcclusion label from its own mod with the ShadowPass label to LabelsPbr
• Removed NodeId
• Renamed Edges.id() to Edges.label()
• Removed NodeLabel
• Changed examples according to the new label system
• Introduced new RenderLabels: Labels2d, Labels3d, LabelsPbr, LabelsUi
• Introduced new RenderSubGraphs: SubGraph2d, SubGraph3d, SubGraphUi
• Removed Reflect and Default derive from CameraRenderGraph component struct
• Improved some error messages

Migration Guide

For Nodes and SubGraphs, instead of using hardcoded strings, you now pass labels, which can be derived with structs and enums.

// old
#[derive(Default)]
struct MyRenderNode;
impl MyRenderNode {
    pub const NAME: &'static str = "my_render_node"
}

render_app
    .add_render_graph_node::<ViewNodeRunner<MyRenderNode>>(
        core_3d::graph::NAME,
        MyRenderNode::NAME,
    )
    .add_render_graph_edges(
        core_3d::graph::NAME,
        &[
            core_3d::graph::node::TONEMAPPING,
            MyRenderNode::NAME,
            core_3d::graph::node::END_MAIN_PASS_POST_PROCESSING,
        ],
    );

// new
use bevy::core_pipeline::core_3d::graph::{Labels3d, SubGraph3d};

#[derive(Debug, Hash, PartialEq, Eq, Clone, RenderLabel)]
pub struct MyRenderLabel;

#[derive(Default)]
struct MyRenderNode;

render_app
    .add_render_graph_node::<ViewNodeRunner<MyRenderNode>>(
        SubGraph3d,
        MyRenderLabel,
    )
    .add_render_graph_edges(
        SubGraph3d,
        (
            Labels3d::Tonemapping,
            MyRenderLabel,
            Labels3d::EndMainPassPostProcessing,
        ),
    );

If you still want to use dynamic labels, you can easily create those with tuple structs:

#[derive(Debug, Hash, PartialEq, Eq, Clone, RenderLabel)]
pub struct MyDynamicLabel(&'static str);

SubGraphs

in bevy_core_pipeline::core_2d::graph

old string-based path	new label
NAME	SubGraph2d

in bevy_core_pipeline::core_3d::graph

old string-based path	new label
NAME	SubGraph3d

in bevy_ui::render

old string-based path	new label
draw_ui_graph::NAME	graph::SubGraphUi

Nodes

in bevy_core_pipeline::core_2d::graph

old string-based path	new label
node::MSAA_WRITEBACK	Labels2d::MsaaWriteback
node::MAIN_PASS	Labels2d::MainPass
node::BLOOM	Labels2d::Bloom
node::TONEMAPPING	Labels2d::Tonemapping
node::FXAA	Labels2d::Fxaa
node::UPSCALING	Labels2d::Upscaling
node::CONTRAST_ADAPTIVE_SHARPENING	Labels2d::ConstrastAdaptiveSharpening
node::END_MAIN_PASS_POST_PROCESSING	Labels2d::EndMainPassPostProcessing

in bevy_core_pipeline::core_3d::graph

old string-based path	new label
node::MSAA_WRITEBACK	Labels3d::MsaaWriteback
node::PREPASS	Labels3d::Prepass
node::DEFERRED_PREPASS	Labels3d::DeferredPrepass
node::COPY_DEFERRED_LIGHTING_ID	Labels3d::CopyDeferredLightingId
node::END_PREPASSES	Labels3d::EndPrepasses
node::START_MAIN_PASS	Labels3d::StartMainPass
node::MAIN_OPAQUE_PASS	Labels3d::MainOpaquePass
node::MAIN_TRANSMISSIVE_PASS	Labels3d::MainTransmissivePass
node::MAIN_TRANSPARENT_PASS	Labels3d::MainTransparentPass
node::END_MAIN_PASS	Labels3d::EndMainPass
node::BLOOM	Labels3d::Bloom
node::TONEMAPPING	Labels3d::Tonemapping
node::FXAA	Labels3d::Fxaa
node::UPSCALING	Labels3d::Upscaling
node::CONTRAST_ADAPTIVE_SHARPENING	Labels3d::ContrastAdaptiveSharpening
node::END_MAIN_PASS_POST_PROCESSING	Labels3d::EndMainPassPostProcessing

in bevy_core_pipeline

old string-based path	new label
taa::draw_3d_graph::node::TAA	Labels3d::Taa

in bevy_pbr

old string-based path	new label
draw_3d_graph::node::SHADOW_PASS	LabelsPbr::ShadowPass
ssao::draw_3d_graph::node::SCREEN_SPACE_AMBIENT_OCCLUSION	LabelsPbr::ScreenSpaceAmbientOcclusion
deferred::DEFFERED_LIGHTING_PASS	LabelsPbr::DeferredLightingPass

in bevy_render

old string-based path	new label
main_graph::node::CAMERA_DRIVER	graph::CameraDriverLabel

in bevy_ui::render

old string-based path	new label
draw_ui_graph::node::UI_PASS	graph::LabelsUi::UiPass

---

Future work

• Make NodeSlots also use types. Ideally, we have an enum with unit variants where every variant resembles one slot. Then to make sure you are using the right slot enum and make rust-analyzer play nicely with it, we should make an associated type in the Node trait. With today's system, we can introduce 3rd party slots to a node, and i wasnt sure if this was used, so I didn't do this in this PR.

Unresolved Questions

When looking at the post_processing example, we have a struct for the label and a struct for the node, this seems like boilerplate and on discord, @IceSentry (sowy for the ping) asked if a node could automatically introduce a label (or i completely misunderstood that). The problem with that is, that nodes like EmptyNode exist multiple times inside the same (sub)graph, so there we need extern labels to distinguish between those. Hopefully we can find a way to reduce boilerplate and still have everything unique. For EmptyNode, we could maybe make a macro which implements an "empty node" for a type, but for nodes which contain code and need to be present multiple times, this could get nasty...

[alice-i-cecile]

In favor of this! Stringly typed labels are bad, and we shouldn't use them here either :)

[DasLixou]

I also want to make benchmarks, im highly interested if this is a small regression or small improvement, since we now no longer hash strings. Is there a simple tutorial on how to benchmark bevy? also some people are using tracy, how can i get to that neat view where the two... mountains... overlap? 😅

[IceSentry]

Yeah, I hit the same EmptyNode issue last time I looked into it. This PR is already a big improvement as it is so we can figure out that issue in the future. Doesn't have to be in this PR.

As for benchmarks, while it wouldn't hurt, I would be highly surprised if it made any difference but here's the docs on profiling. It explains how to use tracy https://github.com/bevyengine/bevy/blob/main/docs/profiling.md

[IceSentry]

Please add a code snippet in the migration guide.

Something like

// 0.12
#[derive(Default)]
struct PostProcessNode;
impl PostProcessNode {
    pub const NAME: &'static str = "post_process";
}

// 0.13
#[derive(Default)]
struct PostProcessNode;
#[derive(Debug, Hash, PartialEq, Eq, Clone, RenderLabel)]
pub struct PostProcessLabel;

[DasLixou]

Yeah, I hit the same EmptyNode issue last time I looked into it. This PR is already a big improvement as it is so we can figure out that issue in the future. Doesn't have to be in this PR.

As for benchmarks, while it wouldn't hurt, I would be highly surprised if it made any difference but here's the docs on profiling. It explains how to use tracy https://github.com/bevyengine/bevy/blob/main/docs/profiling.md

That markdown file is exactly what i was searching for

[DasLixou]

Please add a code snippet in the migration guide.

Something like

// 0.12
#[derive(Default)]
struct PostProcessNode;
impl PostProcessNode {
    pub const NAME: &'static str = "post_process";
}

// 0.13
#[derive(Default)]
struct PostProcessNode;
#[derive(Debug, Hash, PartialEq, Eq, Clone, RenderLabel)]
pub struct PostProcessLabel;

done :>

[alice-i-cecile]

If you're feeling particularly nice, it would be good to have a markdown table showing how to migrate all of the existing labels to their type equivalent.

[DasLixou]

@alice-i-cecile Done! :D (also funny, there was a UI_PASS_DRIVER which was never used)

• I'm a bit unsure about the naming of CameraDriverLabel. I named all the enums Labels<topic> because I couldn't put the 3 of 3d at the front, so I am asking if i should name it LabelCameraDriver?
• Then there was draw_ui_graph::node::UI_PASS, which was the only "label" for Ui, but since it was in a module, I put it in a enum LabelsUi with only one variant. Im unsure, should I also convert it to LabelUiPass?
• Last but not least, there are many inconsistent namings, like in Labels3d: Prepass, EndPrepasses, StartMainPass, is it with or without start and end and is it plural or singular? Now is the best chance to clean those up. An additional Breaking change later after a release would be annoying.

[DasLixou]

This = PR, External = main
Running main_graph seems to have a bigger spike. (what does the spike mean?)

---

This = PR, External = main
Running SubGraphUi (formerly draw_ui) however, it seems faster for whatever reason

[IceSentry]

Last but not least, there are many inconsistent namings, like in Labels3d: Prepass, EndPrepasses, StartMainPass, is it with or without start and end and is it plural or singular? Now is the best chance to clean those up. An additional Breaking change later after a release would be annoying.

Prepass was singular because at the time of creation only 1 prepass existed. EndPrepasses is plural because it waits for the prepass and the deferred_prepass to finish. The main pass has a start and an end label because there are many nodes used in the main pass. So my opinion would be to keep it as is.

[DasLixou]

Yay @alice-i-cecile looks like CI is passing.

[atlv24]

Looks good, no perf regressions i can see vs main. Sometimes its slower by less than a hundredth of a millisecond on average. Sometimes its faster by a tenth of a ms on average.

[Shute052]

I've encountered some challenges. Could you please advise on how to convert dynamic hardcoded strings into RenderLabels? For example, in the bevy_egui parts for updating them to be compatible with Bevy 0.13.

/// Sets up the pipeline for newly created windows.
pub fn setup_new_windows_render_system(
    windows: Extract<Query<Entity, Added<Window>>>,
    mut render_graph: ResMut<RenderGraph>,
) {
    for window in windows.iter() {
        let egui_pass = format!("egui-{}-{}", window.index(), window.generation());

        let new_node = EguiNode::new(window);

        render_graph.add_node(egui_pass.clone(), new_node);

        render_graph.add_node_edge(
            bevy::render::graph::CameraDriverLabel,
            egui_pass.to_string(),
        );
    }
}

[DasLixou]

@Shute052 im not 100% sure but I think it should be fine to make a tuple struct like

#[derive(..., RenderLabel)]
struct EguiPass(&'static str);

[Shute052]

@Shute052 im not 100% sure but I think it should be fine to make a tuple struct like

#[derive(..., RenderLabel)]
struct EguiPass(&'static str);

Thanks! It works and successfully passed all of four tests in bevy_egui, which have limited coverage. I'm not entirely certain that all behaviors are error-free.

[DasLixou]

@Shute052 im not 100% sure but I think it should be fine to make a tuple struct like

#[derive(..., RenderLabel)]
struct EguiPass(&'static str);

Thanks! It works and successfully passed all of four tests in bevy_egui, which have limited coverage. I'm not entirely certain that all behaviors are error-free.

Well it shouldn't differ from the old one except when you have two different structs, because now wouldn't collide anymore, means you could also strip out the egui- from the format! macro

[alice-i-cecile]

Could you update the migration guide to include advice on dynamic labels? :)

[Shute052]

    #[derive(Debug, Hash, PartialEq, Eq, Clone, RenderLabel)]
    struct EguiPass(u32, u32);
    let egui_pass = EguiPass(window.index(), window.generation());

This passed the compilation, would it cause any problems? Stores two u32 might be better than a String

[DasLixou]

Could you update the migration guide to include advice on dynamic labels? :)

sure, ill also add it to the blog PR

[DasLixou]

    #[derive(Debug, Hash, PartialEq, Eq, Clone, RenderLabel)]
    struct EguiPass(u32, u32);
    let egui_pass = EguiPass(window.index(), window.generation());

This passed the compilation, would it cause any problems? Stores two u32 might be better than a String

Already proposed that in the bevy_egui pr 👍

[cart]

A note: removing Reflect / Default from CameraRenderGraph has broken spawning scenes with cameras.

[DasLixou]

A note: removing Reflect / Default from CameraRenderGraph has broken spawning scenes with cameras.

yeah uhm.. but how do we handle such stuff then?