Skip to content

Divide MainPass3dNode sequential passes into separate nodes - #6097

Closed
jngls wants to merge 2 commits into
bevyengine:mainfrom
jngls:separate-render-phases
Closed

jngls wants to merge 2 commits into
bevyengine:mainfrom
jngls:separate-render-phases

Conversation

@jngls

@jngls jngls commented Sep 25, 2022

Copy link
Copy Markdown

Objective

The objective of this PR is to give the user the ability to insert their own logic between render passes, and to further modularize Bevy's code.

There's currently no mechanism for inserting custom render logic between render passes in the core pipeline. E.g. between drawing alpha masked entities and transparent entities. All three passes, opaque, alpha mask, and transparent, execute back-to-back inside MainPass3dNode.

Solution

  • Divide MainPass3dNode into three separate nodes: OpaquePass3dNode, AlphaMaskPass3dNode, and TransparentPass3dNode
  • Update references to core_3d::graph::node::MAIN_PASS to core_3d::graph::node::TRANSPARENT_PASS
  • (optional extra) There's a hack in both MainPass3dNode and MainPass2dNode (search "WebGL2 quirk"); this could be made into something like DummyPassNode and only included in the graph when feature webgl is active - for now, I've moved it to TransparentPass3dNode

Changelog

Changed

  • MainPass3dNode has been divided into three separate nodes: OpaquePass3dNode, AlphaMaskPass3dNode, and TransparentPass3dNode

Migration Guide

  • core_3d::graph::node::MAIN_PASS has been removed and replaced with core_3d::graph::node::OPAQUE_PASS, core_3d::graph::node::ALPHA_MASK_PASS, and core_3d::graph::node::TRANSPARENT_PASS, which represent nodes for each individual pass in the core 3d pipeline

@jngls

jngls commented Sep 25, 2022

Copy link
Copy Markdown
Author

FYI This is my first PR, just in case you need to poke the CI server.

Also I'm not a huge fan of external nodes referencing core_3d::graph::node::TRANSPARENT_PASS (e.g. bevy_ui), since their intention was "to run after the core 3d graph in it's entirety" and if a new pass node is appended to the core 3d graph, that intention will be violated. Not sure on solution for that yet.

@IceSentry IceSentry added the A-Rendering Drawing game state to the screen label Sep 25, 2022
@IceSentry

Copy link
Copy Markdown
Contributor

Would it be possible to add an empty node that's called FINAL_PASS that runs after the TRANSPARENT_PASS? That way other nodes that depend on the main pass won't need to know the transparent pass is the last one.

@IceSentry IceSentry added the M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Sep 25, 2022
@jngls

jngls commented Sep 25, 2022

Copy link
Copy Markdown
Author

Would it be possible to add an empty node that's called FINAL_PASS that runs after the TRANSPARENT_PASS? That way other nodes that depend on the main pass won't need to know the transparent pass is the last one.

Potentially, and that could be where the hack I mentioned earlier lives, however I'm not yet sure if empty nodes adds unnecessary overhead.

Another potential would be to add the FINAL_PASS label, but have it point to the last node in the core 3d graph.

@MDeiml

MDeiml commented Oct 13, 2022

Copy link
Copy Markdown
Contributor

Also I'm not a huge fan of external nodes referencing core_3d::graph::node::TRANSPARENT_PASS (e.g. bevy_ui), since their intention was "to run after the core 3d graph in it's entirety" and if a new pass node is appended to the core 3d graph, that intention will be violated. Not sure on solution for that yet.

Couldn't you make the 3 nodes into a subgraph? I think this would even make these changes non-breaking.

@jngls

jngls commented Oct 14, 2022

Copy link
Copy Markdown
Author

Couldn't you make the 3 nodes into a subgraph? I think this would even make these changes non-breaking.

Nice idea. Yes, I'll reconfigure and update this PR today.

@torsteingrindvik

Copy link
Copy Markdown
Contributor

I ran the transparency_3d example locally on this PR:

transparency_3d_pr_6097.mp4

On main (cb5e2d84b):

transparency_3d_main.mp4

As you can see the behaviour is different.
Could you try rebasing your PR?

@jngls

jngls commented Oct 22, 2022

Copy link
Copy Markdown
Author

Could you try rebasing your PR?

Sure thing.

alexmadeathing added 2 commits November 13, 2022 13:09
Current issues:
- I'm not yet sure where to define the node names
- I'm not yet sure whether to implement the inner opaque, apha_mask, and transparent nodes as a separate plugin, or whether to do all the setup in Core3dPlugin
- There is an assertion failure in the current code in this commit because the render graph is not set up correctly. I'm sure this should be an easy fix
@jngls
jngls force-pushed the separate-render-phases branch from f376dc5 to c2d7902 Compare December 9, 2022 20:25
@jngls

jngls commented Dec 9, 2022

Copy link
Copy Markdown
Author

Hi folks, I need to admit that I'm struggling with some ongoing personal issues right now and I can't commit to finishing this right now.

If anyone wants to take over, be my guest.

I did start the rebase and implementing the render graph as a sub-graph of the main render pass, but I'm afraid I haven't been able to finish it right now. You can find my current progress at: https://github.com/alexmadeathing/bevy/tree/separate-render-phases

Here's a copy of the latest commit message, which includes info about the issues I was running into:

WIP attempting to rebase render graph work after recent changes in main

Current issues:
- I'm not yet sure where to define the node names
- I'm not yet sure whether to implement the inner opaque, apha_mask, and transparent nodes as a separate plugin, or whether to do all the setup in Core3dPlugin
- There is an assertion failure in the current code in this commit because the render graph is not set up correctly. I'm sure this should be an easy fix

In addition, since I was working on this, main has moved on a bit. So this branch will need to be rebased again.

Good luck! If I'm better soon, I will of course come back to this. I'll check in here when that time comes up.

@james7132 james7132 added the S-Adopt-Me The original PR author has no intent to complete this work. Pick me up! label Dec 9, 2022
@tygyh

tygyh commented Feb 8, 2024

Copy link
Copy Markdown
Contributor

I think all the changes from this PR have already been done in #8090. Should this be closed?

@JMS55

JMS55 commented Aug 16, 2024

Copy link
Copy Markdown
Contributor

Opaque and alpha are still combined, but transparent was split. Given the age of this PR, I'm closing it. If there's a demand for splitting alpha and opaque up, please open a new PR/issue.

@JMS55 JMS55 closed this Aug 16, 2024
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 M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Adopt-Me The original PR author has no intent to complete this work. Pick me up!

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants