Add RenderGraphApp to simplify adding render nodes #8007

IceSentry · 2023-03-09T20:55:50Z

Objective

Adding a node to the render_graph can be quite verbose and error prone because there's a lot of moving parts to it.

Solution

Encapsulate this in a simple utility method
- Mostly intended for optional nodes that have specific ordering
- Requires that the Node impl FromWorld, but every internal node is built using a new function taking a &mut World so it was essentially already FromWorld
Use it for the bloom, fxaa and taa, nodes.
- The main nodes don't use it because they rely more on the order of many nodes being added

Changelog

Impl FromWorld for BloomNode, FxaaNode and TaaNode
Added RenderGraph::add_node_edges()
Added RenderGraph::sub_graph()
Added RenderGraph::sub_graph_mut()
Added RenderGraphApp, RenderGraphApp::add_render_graph_node, RenderGraphApp::add_render_graph_edges, RenderGraphApp::add_render_graph_edge

Notes

~~This was taken out of #7995 because it works on it's own. Once the linked PR is done, the new add_node() will be simplified a bit since the input/output params won't be necessary.~~

This feature will be useful in most of the upcoming render nodes so it's impact will be more relevant at that point.

Partially fixes #7985

Future work

Add a way to automatically label nodes or at least make it part of the trait. This would remove one more field from the functions added in this PR
Use it in the main pass 2d/3d

JMS55

A solid step towards more ergonomic render nodes :)

IceSentry · 2023-03-16T22:19:09Z

crates/bevy_render/src/render_graph/graph.rs

@@ -676,6 +714,22 @@ mod tests {
        }
    }

+    fn input_nodes(name: &'static str, graph: &RenderGraph) -> HashSet<NodeId> {


This code was simply moved out of test_graph_edges because I used it in the new test.

# Objective - Currently, the render graph slots are only used to pass the view_entity around. This introduces significant boilerplate for very little value. Instead of using slots for this, make the view_entity part of the `RenderGraphContext`. This also means we won't need to have `IN_VIEW` on every node and and we'll be able to use the default impl of `Node::input()`. ## Solution - Add `view_entity: Option<Entity>` to the `RenderGraphContext` - Update all nodes to use this instead of entity slot input --- ## Changelog - Add optional `view_entity` to `RenderGraphContext` ## Migration Guide You can now get the view_entity directly from the `RenderGraphContext`. When implementing the Node: ```rust // 0.10 struct FooNode; impl FooNode { const IN_VIEW: &'static str = "view"; } impl Node for FooNode { fn input(&self) -> Vec<SlotInfo> { vec![SlotInfo::new(Self::IN_VIEW, SlotType::Entity)] } fn run( &self, graph: &mut RenderGraphContext, // ... ) -> Result<(), NodeRunError> { let view_entity = graph.get_input_entity(Self::IN_VIEW)?; // ... Ok(()) } } // 0.11 struct FooNode; impl Node for FooNode { fn run( &self, graph: &mut RenderGraphContext, // ... ) -> Result<(), NodeRunError> { let view_entity = graph.view_entity(); // ... Ok(()) } } ``` When adding the node to the graph, you don't need to specify a slot_edge for the view_entity. ```rust // 0.10 let mut graph = RenderGraph::default(); graph.add_node(FooNode::NAME, node); let input_node_id = draw_2d_graph.set_input(vec![SlotInfo::new( graph::input::VIEW_ENTITY, SlotType::Entity, )]); graph.add_slot_edge( input_node_id, graph::input::VIEW_ENTITY, FooNode::NAME, FooNode::IN_VIEW, ); // add_node_edge ... // 0.11 let mut graph = RenderGraph::default(); graph.add_node(FooNode::NAME, node); // add_node_edge ... ``` ## Notes This PR paired with #8007 will help reduce a lot of annoying boilerplate with the render nodes. Depending on which one gets merged first. It will require a bit of clean up work to make both compatible. I tagged this as a breaking change, because using the old system to get the view_entity will break things because it's not a node input slot anymore. ## Notes for reviewers A lot of the diffs are just removing the slots in every nodes and graph creation. The important part is mostly in the graph_runner/CameraDriverNode.

kurtkuehnert

A nice change.
Why don't we store the name of each node on the node itself?

struct MyNode: Node {}

impl Node for MyNode {
  fn name() -> &'static str {
    "MyNode"
  }
}

IceSentry · 2023-03-25T17:28:51Z

I've been considering doing that because it would make a few of those patterns even simpler. The issue is that it wouldn't work with EmptyNode that is used in a few places like post processing, but having a way to automatically label a node would be nice a bit like how you can use systems as labels.

So for now, I'd prefer keeping this PR simpler and consider better labels in a future PR

IceSentry · 2023-03-27T23:30:32Z

While updating this for TAA, I noticed that adding the same edge multiple time would cause a panic so I updated the code to not panic in that case since it doesn't seem like something that is inherently wrong. You can just not add it again if it already exist

JMS55 · 2023-03-28T16:31:46Z

I've been considering doing that because it would make a few of those patterns even simpler. The issue is that it wouldn't work with EmptyNode that is used in a few places like post processing, but having a way to automatically label a node would be nice a bit like how you can use systems as labels.

Would something like this work?

struct EmptyNode { name: &'static str }

impl Node for EmptyNode {
  fn name(&self) -> &'static str {
      self.name
  }
}

We could also maybe replace empty nodes with subgraphs? Idk, haven't looked at how subgraphs work.

IceSentry · 2023-03-28T18:52:21Z

Would something like this work?

Yeah, that's exactly what I thought of after my original comment.

I still think I'd prefer doing this in a separate PR just because big PRs tend to take a long time to merge.

cart

I'm very on board for this (love the improved legibility and ergonomics), but I have a few suggestions:

I think this helper should be an "app builder" method. It is already effectively written as one (by taking a mutable app as the first param). It makes this more "standard" (we use this pattern for every other app operation), and it allows us to add it to other app ops.
I think we should split out adding edges and nodes. They are technically orthogonal operations. I know we basically always call them back to back, but I think calling separate named methods helps make it clearer what is going on (rather than having a big list of variables as input that result in a variety of outputs). Combine that with (1) and this actually ends up taking up fewer lines anyway.
Because (1) makes this an App extension method, add_node isn't descriptive enough in a global context. So these should be add_render_graph_X not add_X. (I probably would have pushed for this anyway for the original helper method).

I've pushed a commit that does all of this (as a proposal to make the conversation easier ... we can always revert or change things). I personally think it reads quite well. Check out the diff:

Also as a side note: not a fan of "helper" naming generally and I try to avoid it in Bevy. "helper" is a grab bag term and there is almost always a more descriptive and useful name.

IceSentry · 2023-04-03T21:55:43Z

I think this helper should be an "app builder" method

I didn't even consider this, I was just trying to encapsulate this common pattern but that diff looks really good.

I think we should split out adding edges and nodes.

That's fair, I liked the idea of bundling everything so it's hard to forget to set a node edge, but I do agree that they are separate concepts. My approach also made it impossible to use add_node in the main_pass_2d/3d which is why I ended up adding the add_node_edges function to the render_graph.

not a fan of "helper" naming generally

Completely agree here, I just really didn't know how to name it/where to put this.

In general, I'm on board with the app extension. My only concern here is that people might end up calling it on the App and not RenderApp. I'm not sure how to handle that case. To be fair, it was also an issue with the original approach.

IceSentry · 2023-04-03T22:05:18Z

Since I'm fully on board with this, I updated the description to match this new api

JMS55 · 2023-04-03T22:27:42Z

In general, I'm on board with the app extension. My only concern here is that people might end up calling it on the App and not RenderApp. I'm not sure how to handle that case. To be fair, it was also an issue with the original approach.

I feel that we should have a RenderAppExt trait implemented on App. Users won't be able to access the methods unless they import the trait, which should hopefully be less error prone.

IceSentry · 2023-04-03T22:52:53Z

RenderAppExt trait

I don't think this is possible because RenderApp is just a marker for a sub app. Maybe Apps could have a concept of labels to identify you are using the correct App. Definitely out of scope here though.

I reread your comment, the new feature is implemented like that. It's an extension trait implemented on App. It's just that if it's in scope people could still end up using it on the wrong app.

IceSentry · 2023-04-03T23:00:38Z

I added a check in the function to warn the user if it doesn't find the RenderGraph and to remind them to run it on the RenderApp.

JMS55 · 2023-04-03T23:05:23Z

Alternative: Make RenderApp a type wrapping the render App, and implement the methods there. Then, instead of get_sub_app(), add get_render_app() which wraps the app in RenderApp.

Users could still call get_render_app() on the wrong app, if they had a custom subapp, but it's much more unlikely.

IceSentry · 2023-04-03T23:07:43Z

I think for now, with the improved panic message and considering this api is most likely only used by more advanced user we'll be fine for a bit. But I'll add it to my list of potential improvements.

IceSentry requested a review from JMS55 March 9, 2023 20:55

IceSentry added A-Rendering Drawing game state to the screen C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Mar 9, 2023

alice-i-cecile added this to the 0.11 milestone Mar 9, 2023

JMS55 approved these changes Mar 9, 2023

View reviewed changes

IceSentry force-pushed the render-graph-utils branch 3 times, most recently from 9bed368 to 362d417 Compare March 16, 2023 05:29

IceSentry mentioned this pull request Mar 16, 2023

Make render graph slots optional for most cases #8109

Merged

IceSentry force-pushed the render-graph-utils branch from 94bcf89 to ba346a6 Compare March 16, 2023 22:16

IceSentry commented Mar 16, 2023

View reviewed changes

IceSentry force-pushed the render-graph-utils branch from ba346a6 to 22d741f Compare March 24, 2023 18:57

kurtkuehnert reviewed Mar 25, 2023

View reviewed changes

kurtkuehnert approved these changes Mar 26, 2023

View reviewed changes

IceSentry force-pushed the render-graph-utils branch 2 times, most recently from 18f9a3d to 5849c15 Compare March 27, 2023 23:03

IceSentry force-pushed the render-graph-utils branch 3 times, most recently from bed44df to 00c857f Compare March 31, 2023 18:34

IceSentry changed the title ~~Add utility function to add render nodes~~ Add utility functions to simplify adding render nodes Mar 31, 2023

IceSentry force-pushed the render-graph-utils branch 2 times, most recently from 8e14d99 to 959e7f2 Compare April 1, 2023 19:27

IceSentry 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 Apr 1, 2023

cart reviewed Apr 3, 2023

View reviewed changes

IceSentry changed the title ~~Add utility functions to simplify adding render nodes~~ Add RenderGraphApp to simplify adding render nodes Apr 3, 2023

IceSentry requested review from kurtkuehnert and JMS55 April 3, 2023 22:06

IceSentry and others added 5 commits April 3, 2023 20:20

render graph utils

4f14f3f

fix core_3d

9cf2e86

App builder methods

684cb18

improve panic message if not running on render app

7bc7a48

add single edge variant

9a36d58

IceSentry force-pushed the render-graph-utils branch from a7681d2 to 9a36d58 Compare April 4, 2023 00:34

impl for CASNode

d3e5a27

cart approved these changes Apr 4, 2023

View reviewed changes

cart enabled auto-merge April 4, 2023 00:48

cart added this pull request to the merge queue Apr 4, 2023

Merged via the queue into bevyengine:main with commit 614de30 Apr 4, 2023

This was referenced Jun 28, 2023

News: Release 0.11 (wrong branch ... see #657) bevyengine/bevy-website#656

Closed

News: Release 0.11 bevyengine/bevy-website#657

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add RenderGraphApp to simplify adding render nodes #8007

Add RenderGraphApp to simplify adding render nodes #8007

IceSentry commented Mar 9, 2023 •

edited by cart

Loading

JMS55 left a comment

IceSentry Mar 16, 2023

kurtkuehnert left a comment

IceSentry commented Mar 25, 2023 •

edited

Loading

IceSentry commented Mar 27, 2023

JMS55 commented Mar 28, 2023

IceSentry commented Mar 28, 2023

cart left a comment •

edited

Loading

IceSentry commented Apr 3, 2023 •

edited

Loading

IceSentry commented Apr 3, 2023

JMS55 commented Apr 3, 2023

IceSentry commented Apr 3, 2023 •

edited

Loading

IceSentry commented Apr 3, 2023 •

edited

Loading

JMS55 commented Apr 3, 2023

IceSentry commented Apr 3, 2023

Add RenderGraphApp to simplify adding render nodes #8007

Add RenderGraphApp to simplify adding render nodes #8007

Conversation

IceSentry commented Mar 9, 2023 • edited by cart Loading

Objective

Solution

Changelog

Notes

Future work

JMS55 left a comment

Choose a reason for hiding this comment

IceSentry Mar 16, 2023

Choose a reason for hiding this comment

kurtkuehnert left a comment

Choose a reason for hiding this comment

IceSentry commented Mar 25, 2023 • edited Loading

IceSentry commented Mar 27, 2023

JMS55 commented Mar 28, 2023

IceSentry commented Mar 28, 2023

cart left a comment • edited Loading

Choose a reason for hiding this comment

IceSentry commented Apr 3, 2023 • edited Loading

IceSentry commented Apr 3, 2023

JMS55 commented Apr 3, 2023

IceSentry commented Apr 3, 2023 • edited Loading

IceSentry commented Apr 3, 2023 • edited Loading

JMS55 commented Apr 3, 2023

IceSentry commented Apr 3, 2023

IceSentry commented Mar 9, 2023 •

edited by cart

Loading

IceSentry commented Mar 25, 2023 •

edited

Loading

cart left a comment •

edited

Loading

IceSentry commented Apr 3, 2023 •

edited

Loading

IceSentry commented Apr 3, 2023 •

edited

Loading

IceSentry commented Apr 3, 2023 •

edited

Loading