Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RenderGraph Labelization #10644

Merged
merged 22 commits into from
Jan 31, 2024
Merged

Conversation

DasLixou
Copy link
Contributor

@DasLixou DasLixou commented Nov 19, 2023

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 alice-i-cecile 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 M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide D-Complex Quite challenging from either a design or technical perspective. Ask for help! labels Nov 19, 2023
@alice-i-cecile
Copy link
Member

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

@DasLixou
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor

IceSentry commented Nov 19, 2023

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

@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
Copy link
Contributor Author

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


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

@IceSentry
Copy link
Contributor

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
Copy link
Contributor Author

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

Copy link
Contributor

@atlv24 atlv24 left a comment

Choose a reason for hiding this comment

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

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.

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jan 31, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jan 31, 2024
Merged via the queue into bevyengine:main with commit 16d28cc Jan 31, 2024
23 checks passed
@DasLixou DasLixou deleted the render_graph_labels branch January 31, 2024 15:23
@Shute052
Copy link

Shute052 commented Feb 3, 2024

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
Copy link
Contributor Author

DasLixou commented Feb 3, 2024

@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
Copy link

Shute052 commented Feb 3, 2024

@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
Copy link
Contributor Author

DasLixou commented Feb 3, 2024

@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
Copy link
Member

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

@Shute052
Copy link

Shute052 commented Feb 3, 2024

    #[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
Copy link
Contributor Author

DasLixou commented Feb 3, 2024

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

sure, ill also add it to the blog PR

@DasLixou
Copy link
Contributor Author

DasLixou commented Feb 3, 2024

    #[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 👍

tjamaan pushed a commit to tjamaan/bevy that referenced this pull request Feb 6, 2024
# 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 `RenderLabel`s: `Labels2d`, `Labels3d`, `LabelsPbr`,
`LabelsUi`
* Introduced new `RenderSubGraph`s: `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.

```rs
// 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,
        ),
    );
```

### 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 `NodeSlot`s 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](https://discord.com/channels/691052431525675048/743663924229963868/1175197016947699742)
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...
@cart
Copy link
Member

cart commented Feb 13, 2024

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

@DasLixou
Copy link
Contributor Author

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

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

github-merge-queue bot pushed a commit that referenced this pull request Feb 15, 2024
# Objective

#10644 introduced nice "statically typed" labels that replace the old
strings. I would like to propose some changes to the names introduced:

* `SubGraph2d` -> `Core2d` and `SubGraph3d` -> `Core3d`. The names of
these graphs have been / should continue to be the "core 2d" graph not
the "sub graph 2d" graph. The crate is called `bevy_core_pipeline`, the
modules are still `core_2d` and `core_3d`, etc.
* `Labels2d` and `Labels3d`, at the very least, should not be plural to
follow naming conventions. A Label enum is not a "collection of labels",
it is a _specific_ Label. However I think `Label2d` and `Label3d` is
significantly less clear than `Node2d` and `Node3d`, so I propose those
changes here. I've done the same for `LabelsPbr` -> `NodePbr` and
`LabelsUi` -> `NodeUi`

Additionally, #10644 accidentally made one of the Camera2dBundle
constructors use the 3D graph instead of the 2D graph. I've fixed that
here.
 
---

## Changelog

* Renamed `SubGraph2d` -> `Core2d`, `SubGraph3d` -> `Core3d`, `Labels2d`
-> `Node2d`, `Labels3d` -> `Node3d`, `LabelsUi` -> `NodeUi`, `LabelsPbr`
-> `NodePbr`
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-Complex Quite challenging from either a design or technical perspective. Ask for help! M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants