Replace RenderGraph with systems

[tychedelia]

render-graph-as-systems

Note

Remember to check hide whitespace in diff view options when reviewing this PR

This PR removes the RenderGraph in favor of using systems.

Motivation

The RenderGraph API was originally created when the ECS was significantly more immature. It was also created with the intention of supporting an input/output based slot system for managing resources that has never been used. While resource management is an important potential use of a render graph, current rendering code doesn't make use of any patterns relating to it.

Since the ECS has improved, the functionality of Schedule has basically become co-extensive with what the RenderGraph API is doing, i.e. ordering bits of system-like logic relative to one another and executing them in a big chunk. Additionally, while there's still desire for more advanced techniques like resource management in the graph, it's desirable to implement those in ECS terms rather than creating more RenderGraph specific abstraction.

In short, this sets us up to iterate on a more ECS based approach, while deleting ~3k lines of mostly unused code.

Implementation

At a high level: We use Schedule as our "sub-graph." Rather than running the graph, we run a schedule. Systems can be ordered relative to one another.

The render system uses a RenderGraph schedule to define the "root" of the graph. core_pipeline adds a camera_driver system that runs the per-camera schedules. This top level schedule provides an extension point for apps that may want to do custom rendering, or non-camera rendering.

CurrentView / ViewQuery

When running schedules per-camera in the camera_driver system, we insert a CurrentView resource that's used to mark the currently iterating view. We also add a new param ViewQuery that internally uses this resource to execute the query and skip the system if it doesn't match as a convenience.

RenderContext

The RenderContext is now a system param that wraps a Deferred for tracking the state of the current command encoder and queued buffers.

SystemBuffer

We use an system buffer impl to track command encoders in the render context and rely on apply deferred in order to encode them all. Currently, this encodes them in series. There are likely opportunities here to make this more efficient.

Benchmarks

Bistro

Caldera

Future steps

There are a number of exciting potential changes that could follow here:

• We can explore adding something like a read-only schedule to pick up some more potential parallelism in graph execution.
• We can use more things like run conditions in order to prevent systems from running at all in the first place.
• We can explore things like automating resource creation via system params.

TODO:

• Make sure 100% of everything still works.
• Benchmark to make sure we don't regress performance
• Re-add docs

[github-actions]

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Please review the instructions for writing migration guides, then expand or revise the content in the migration guides directory to reflect your changes.

[github-actions]

It looks like your PR has been selected for a highlight in the next release blog post, but you didn't provide a release note.

Please review the instructions for writing release notes, then expand or revise the content in the release notes directory to showcase your changes.

[cart]

One of the "big" reasons I've pushed back against RenderGraph -> Systems in the past is that it prevents us from parallelizing command encoding in cases like this where we need to run specific, potentially very expensive "subgraphs" in a loop.

The ownership and access patterns in the current system allow for that sort of thing (and were explicitly chosen to enable it).

[tychedelia]

A few things that I'd like to note:

• With the exception of one caveat I'll note in the PR description, this isn't worse than the existing solution in terms of overall parallelism.
• In the context of cameras, we effectively currently need to be serial because we re-use ViewTarget textures. Without a higher level resource tracking system, we have to pessimistically assume that cameras need to run in terms of the order they are given due to implicit dependencies.
• Something like a "read only" schedule marker has been proposed before to allow running schedules on &World. ECS devs have suggested to me this is feasible.
• We do do mutation in the current graph in many places, but have to resort to hack-y atomics usage. It's not crazy that you want to mutate world sometimes (i.e. to insert some gpu resource to be used by the next frame).
• I'd like to solve issues around parallelism in the context of more generalized ECS patterns. I have a number of more speculative ideas here, but I think there are a few different directions we could go.

But yeah, we obviously need to make sure we don't regress current state performance.

[IceSentry]

I think we also need to keep in mind maintenance. This PR manages to remove a lot of code and makes the render graph much more approachable for people that already know bevy's ECS. It's 3.8k less lines of code to maintain. Of course, line of code isn't a super useful metric for many things but I think it's significant enough that it can't just be ignored. With the current render graph, we almost never see PRs to it and I wouldn't be surprised if a big reason for that is that it looks so foreign in bevy and because it's also a lot of very complex code.

[dcvz]

Besides the points made above about approachability, this also means dogfooding systems for a very performance critical part of Bevy. Working towards meeting the demands of that via systems and working on optimizations will then turn into benefits for everything else

[atlv24]

reviewer note: fxaa is ordered after tonemapping, so the .after tonemapping here is redundant thus removed

[atlv24]

some ordering constraints were lost here

[atlv24]

Remember to check Hide whitespace in diff view options!

Some notes:

• should we be using fallible systems to more closely match the Result behavior of the original nodes?
• some ordering constraints were removed but i think many were redundant. i noted these
• tracing infra probably is redundant with system traces now (followup)
• can probably use run conditions in a bunch of places too now (followup)
• render_context -> ctx could have been a different pr (and is a matter of taste but i agree with it anyways) its just noise here
• one of the solari files is jumbled and unreviewable

This is an absolutely colossal amount of work with incredible attention to detail
fantastic job, excited to see this merge. Lots of small fixes bundled here and its pretty much all good. There's some comments that were removed but i dont feel very strongly about them.

Very in favor of this merging, approval will come after comments addressed and pr exits draft status.

I'm going to dig into the CI failures soon

[atlv24]

yayyyyyy

solari and meshlet examples both work, checked a few other examples and all looks good

[atlv24]

this makes clean compile a bit worse cus linearizes compilation of these two crates, but its probably fine. could use system sets to avoid it though

[atlv24]

i think we should use them whenever they help compilation parallelism i guess

[andriyDev]

Overall LGTM, though I'm no rendering guru, just a regular ECS enjoyer :)

[andriyDev]

Do we need two of this here? Maybe this should just be MainPass and then users should just say .before(MainPass) or .after(MainPass)?

If we never say in_set for these, then I think these are the wrong system set names - most of our rendering systems IMO should be inside these sets, not scheduled around them.

Same reasoning for Core2dSystems.

[tychedelia]

I need to think about this more. In the old render graph, they were empty place-holder nodes used only for relative ordering. But that kind of usage might be weird with system sets. I think you're right that we may just want to have a MainPass, although that does make us lose the ability to order relative to the start/end. I've wondered before whether it would be possible to have a system config that's like "as late as possible within set" which would function similarly.

[IceSentry]

Yeah, I think I'd be in favour of a MainPass system set here. I'm pretty sure the only reason it's not like that right now is because the render graph didn't support that kind of api.

[tychedelia]

Okay, I've made that change! We now just have Prepass, MainPass, and PostProcess.

[andriyDev]

I'm not excited about how many of these things we're making public. We might consider adding SystemSets for these systems so we don't have to make these internal types public.

[IceSentry]

Arguably, many more things in the renderer should be public. It can often be useful to access those things as users.

[tychedelia]

Yeah, I think there's a valid conversation to have here about whether we want to use system sets preferentially over systems but I do think making things pub is nbd in general and we should typically favor it unless there's really good reason for encapsulation.

[ickshonpe]

I've always hated working with the render graph API and this makes so much more sense to me. The UI rendering changes all seem fine.

I ran some naive benchmarks, and everything is great, except for one major regression with the many_buttons example with many-cameras enabled:

cargo run --example many_buttons --release -- --many-cameras --buttons 25

It's a very silly unrealistic benchmark though, so might not be an issue.

[tychedelia]

It's a very silly unrealistic benchmark though, so might not be an issue.

Let me at least take a peak to see if I can identify where the overhead is coming from. Thanks for checking these.

[JMS55]

Why are there two separate functions? Use a type alias for the ViewQuery contents behind a cfg instead, like how Solari works on mainline.

[tychedelia]

Okay yeah, I have cleaned this up as well as fixed a number of other divergences from main.

[tychedelia]

@ickshonpe Okay, that was a real change in behavior that was mostly unintentional. I was accidentally submitting after every camera rather than at the end of the graph. With that fixed in e29562b, it's now only showing a ~10% regression on that benchmark.

[ickshonpe]

@ickshonpe Okay, that was a real change in behavior that was mostly unintentional. I was accidentally submitting after every camera rather than at the end of the graph. With that fixed in e29562b, it's now only showing a ~10% regression on that benchmark.

Still seems like there might be some other problem. Even with e29562b many_cameras_lights really chokes with more cameras and less lights:

const CAMERA_ROWS: usize = 25;
const CAMERA_COLS: usize = 25;
const NUM_LIGHTS: usize = 1;

(Red=main)