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

Add ViewNode to simplify render node management #8118

Merged
merged 6 commits into from
May 8, 2023

Conversation

IceSentry
Copy link
Contributor

@IceSentry IceSentry commented Mar 17, 2023

Objective

  • When writing render nodes that need a view, you always need to define a Query on the associated view and make sure to update it manually and query it manually. This is verbose and error prone.

Solution

  • Introduce a new ViewNode trait and ViewNodeRunner Node that will take care of managing the associated view query automatically.
  • The trait is currently a passthrough of the Node trait. So it still has the update/run with all the same data passed in.
  • The ViewNodeRunner is the actual node that is added to the render graph and it contains the custom node. This is necessary because it's the one that takes care of updating the node.

Changelog

  • Add ViewNode
  • Add ViewNodeRunner

Notes

Currently, this only handles the view query, but it could probably have a ReadOnlySystemState that would also simplify querying all the readonly resources that most render nodes currently query manually. The issue is that I don't know how to do that without a &mut self.

At first, I tried making this a default feature of all Node, but I kept hitting errors related to traits and generics and stuff I'm not super comfortable with. This implementations is much simpler and keeps the default Node behaviour so isn't a breaking change

Reviewer Notes

The PR looks quite big, but the core of the PR is the changes in render_graph/node.rs. Every other change is simply updating existing nodes to use this new feature.

Open questions

- Naming is not final, I'm opened to anything. I named it ViewQueryNode because it's a node with a managed Query on a View.
- What to do when the query fails? All nodes using this pattern currently just return Ok(()) when it fails, so I chose that, but should it be more flexible?
- Is the ViewQueryFilter actually necessary? All view queries run on the entity that is already guaranteed to be a view. Filtering won't do much, but maybe someone wants to control an effect with the presence of a component instead of a flag.
- What to do with Nodes that are empty struct? Implementing FromWorld is pretty verbose but not implementing it means there's 2 ways to create a ViewNodeRunner which seems less ideal. This is an issue now because most node simply existed to hold the query, but now that they don't hold the query state we are left with a bunch of empty structs.

  • Should we have a RenderGraphApp::add_render_graph_view_node(), this isn't necessary, but it could make the code a bit shorter.

@IceSentry IceSentry added A-Rendering Drawing game state to the screen C-Feature A new feature, making something new possible labels Mar 17, 2023
@IceSentry IceSentry requested a review from JMS55 March 17, 2023 21:16
@JMS55
Copy link
Contributor

JMS55 commented Mar 18, 2023

Naming is not final, I'm opened to anything. I named it ViewQueryNode because it's a node with a managed Query on a View.

I'd go with just ViewNode, or ViewNodeWrapper or something

What to do when the query fails? All nodes using this pattern currently just return Ok(()) when it fails, so I chose that, but should it be more flexible?

Ok(()) is fine. The query failing isn't actually an error. It just means no views are relevant to that node, e.g. if the FXAA node exists but no views are using FXAA, then the node just has nothing to do, but that's not an error.

Is the ViewQueryFilter actually necessary? All view queries run on the entity that is already guaranteed to be a view. Filtering won't do much, but maybe someone wants to control an effect with the presence of a component instead of a flag.

I'd remove it. There's no need to filter by the time it gets to the node, usually you filter earlier on in extract or prepare and stuff. Adding this just adds boilerplate and complexity for no benefit.

@JMS55
Copy link
Contributor

JMS55 commented Mar 18, 2023

Any reason you're not using ViewQueryNode for the main pass nodes?

@IceSentry
Copy link
Contributor Author

Any reason you're not using ViewQueryNode for the main pass nodes?

I should probably have made this a draft PR, but I wanted to play with the idea a bit first before updating everything. That's also the advantage of this approach, it doesn't force it on every node, but there's nothing blocking it for the main pass node.

Copy link
Contributor

@kurtkuehnert kurtkuehnert left a comment

Choose a reason for hiding this comment

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

I really like the direction of this PR.
Doing the same for the resources in a followup PR would be great as well.

crates/bevy_core_pipeline/src/core_2d/mod.rs Outdated Show resolved Hide resolved
@kurtkuehnert
Copy link
Contributor

I second @JMS55's suggestion on naming: ViewNode for ViewQueryNode and ViewNodeWrapper for the ViewQueryNodeRunner.

@IceSentry
Copy link
Contributor Author

Renamed to ViewNode and ViewNodeRunner. I think I prefer Runner instead of Wrapper since it's a bit more explicit in my head.

I also removed the filter since we agree that it seems unnecessary.

@IceSentry IceSentry force-pushed the render-graph-view-query branch 2 times, most recently from 3cbfa5e to b6f82e0 Compare March 31, 2023 17:49
@IceSentry IceSentry changed the title Add ViewQueryNode to simplify render node management Add ViewNode to simplify render node management Mar 31, 2023
@IceSentry IceSentry force-pushed the render-graph-view-query branch 4 times, most recently from da15a35 to c769440 Compare March 31, 2023 18:16
@IceSentry
Copy link
Contributor Author

I implemented a bunch more nodes. I added a new question to the description.

Essentially, after this PR, most Nodes are just empty structs because they only held the view query state and a couple of them create an empty mutex that is initialized on the first run. I'm trying to impl FromWorld on the ViewNodeRunner, but now most nodes don't need the World to be created, which means that if we want to use ViewNodeRunner::<Node>::from_world(&mut world) to create all view nodes we also need a bunch of empty impl FromWorld. I'm not sure how to tackle this issue.

@IceSentry IceSentry force-pushed the render-graph-view-query branch 3 times, most recently from 3fb1285 to eb988fd Compare April 1, 2023 18:49
@JMS55 JMS55 added this to the 0.11 milestone Apr 4, 2023
@IceSentry IceSentry force-pushed the render-graph-view-query branch 3 times, most recently from 554802e to 806ba7b Compare April 9, 2023 18:49
examples/shader/post_process_pass.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@nicopap nicopap left a comment

Choose a reason for hiding this comment

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

Excellent job. Useful abstraction, also helps document the patterns used for render graph nodes in bevy.

crates/bevy_core_pipeline/src/tonemapping/node.rs Outdated Show resolved Hide resolved
crates/bevy_core_pipeline/src/upscaling/node.rs Outdated Show resolved Hide resolved
@nicopap nicopap 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 24, 2023
Copy link
Member

@cart cart left a comment

Choose a reason for hiding this comment

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

This looks great!
Just pushed some (very) minor tweaks:

  • In the ViewNode interface, ROQueryItem can just be QueryItem for simplicity. The "read-onlyness" is implied by the ReadOnlyWorldQuery constraint
  • I took this as a chance to use &'static everywhere (for consistency), rather than mixing and matching with Read<>
  • Renamed ViewWorldQuery to ViewQuery to match the variable name and reduce "concept noise". I think "world-query-ness" can be implied here.

@cart cart enabled auto-merge May 8, 2023 19:31
@cart cart added this pull request to the merge queue May 8, 2023
Merged via the queue into bevyengine:main with commit 613b5a6 May 8, 2023
@IceSentry IceSentry deleted the render-graph-view-query branch May 9, 2023 23:20
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-Feature A new feature, making something new possible 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.

5 participants