Skip to content

Conversation

IceSentry
Copy link
Contributor

@IceSentry IceSentry commented Mar 16, 2023

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:

// 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.

// 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.

@IceSentry IceSentry added A-Rendering Drawing game state to the screen M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide C-Feature A new feature, making something new possible labels Mar 16, 2023
@IceSentry IceSentry force-pushed the render-graph-view-entity branch from 642b6bd to 4fa9f82 Compare March 16, 2023 22:16
@github-actions
Copy link
Contributor

Example breakout failed to run, please try running it locally and check the result.

1 similar comment
@github-actions
Copy link
Contributor

Example breakout failed to run, please try running it locally and check the result.

@IceSentry IceSentry force-pushed the render-graph-view-entity branch from e278541 to 06567fd Compare March 17, 2023 05:21
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.

Very nice usability win. Very on board for this!

@cart cart added this pull request to the merge queue Mar 21, 2023
@cart cart merged commit 2c21d42 into bevyengine:main Mar 21, 2023
Elabajaba added a commit to Elabajaba/bevy that referenced this pull request Mar 22, 2023
@IceSentry IceSentry deleted the render-graph-view-entity branch March 27, 2023 20:21
Elabajaba added a commit to Elabajaba/bevy that referenced this pull request Mar 29, 2023
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 M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants