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

Split opaque and transparent phases #8090

Merged
merged 4 commits into from
Mar 28, 2023
Merged

Conversation

james-j-obrien
Copy link
Contributor

@james-j-obrien james-j-obrien commented Mar 14, 2023

Objective

Fixes #8089.

Solution

Splits the MainPass3dNode into 2 nodes, one for the opaque + alpha passes and one for the transparent pass.


Changelog

  • Split MainPass3dNode into MainOpaquePass3dNode and MainTransparentPass3dNode
  • Combine opaque and alpha phases in MainOpaquePass3dNode into one pass
  • Create START_MAIN_PASS and END_MAIN_PASS empty nodes as labels
  • Main pass becomes START_MAIN_PASS -> MAIN_OPAQUE_PASS -> MAIN_TRANSPARENT_PASS -> END_MAIN_PASS

Migration Guide

Nodes that previously added edges involving MAIN_PASS should now add edges to or from START_MAIN_PASS or END_MAIN_PASS respectively.

@IceSentry
Copy link
Contributor

I'd like to keep the ability of scheduling things relative to a MAIN_PASS that isn't concerned about transparency. Unfortunately, that might mean adding a START_MAIN_PASS/END_MAIN_PASS empty node.

@james-j-obrien
Copy link
Contributor Author

I'd like to keep the ability of scheduling things relative to a MAIN_PASS that isn't concerned about transparency. Unfortunately, that might mean adding a START_MAIN_PASS/END_MAIN_PASS empty node.

I had the same thought, it did seem odd going into the ui and pbr node and having to just know which pass to order relative to, happy to also add those if we think it would help with usability.

@JMS55
Copy link
Contributor

JMS55 commented Mar 14, 2023

I'd like to keep the ability of scheduling things relative to a MAIN_PASS that isn't concerned about transparency. Unfortunately, that might mean adding a START_MAIN_PASS/END_MAIN_PASS empty node.

Sounds like a good solution for now. More indication that we should split up "node graph markers/labels" and actual running nodes though.

Copy link
Contributor

@JMS55 JMS55 left a comment

Choose a reason for hiding this comment

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

Looks good to me so far, thanks for the PR :)

Here's my requested changes:

  • Use empty nodes for start/end main pass, and use labels relative to those for things like bloom (update the migration guide)
  • Combine the opaque/alpha render passes for better performance
  • Add doc comments to the two node types, indicating that they do opaque+alpha and transparency, respectively (use the actual RenderPhase names within the docs).
  • Add a changelog noting the split

@james7132 james7132 added C-Enhancement A new feature A-Rendering Drawing game state to the screen labels Mar 14, 2023
Copy link
Contributor

@IceSentry IceSentry left a comment

Choose a reason for hiding this comment

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

LGTM, I didn't try running it, but the change seem good

Copy link
Contributor

@JMS55 JMS55 left a comment

Choose a reason for hiding this comment

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

Tested it, lgtm!

@coreh
Copy link
Contributor

coreh commented Mar 15, 2023

Ooo this is really nice, since it allows a more “pluggable” approach with the (potential) transmissive phase and might allow in the future to have it also optionally go after the transparent phase (for scenes where you also want to refract transparent meshes)

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.

Other than that I would split the transparent node and the reset viewport node this LGTM.

Comment on lines +104 to +111
#[cfg(feature = "webgl")]
if camera.viewport.is_some() {
#[cfg(feature = "trace")]
let _reset_viewport_pass_3d = info_span!("reset_viewport_pass_3d").entered();
let pass_descriptor = RenderPassDescriptor {
label: Some("reset_viewport_pass_3d"),
color_attachments: &[Some(target.get_color_attachment(Operations {
load: LoadOp::Load,
store: true,
}))],
depth_stencil_attachment: None,
};

render_context
.command_encoder()
.begin_render_pass(&pass_descriptor);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be a separate node as well. We want this even when no transparent node is added. Additionally, we could only insert it on WebGL builds.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is fair. It does seem to be a mix of concerns and could be problematic for those who want to build their own render graph without a transparent pass. However it does also mean those users would have to know to add the reset viewport node. Conditionally adding the node for web builds could be a good solution as I wouldn't want to add another empty node if we could avoid it.

@james7132 james7132 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 Mar 16, 2023
Copy link
Contributor

@robtfm robtfm left a comment

Choose a reason for hiding this comment

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

lgtm

i do agree that the viewport reset pass should not be here, but i don't think it's necessary to change it in this PR since it's a bit of a fudge already.

specifically, this breaks currently if people don't set viewport state immediately after any prior pass that sets a viewport ... the generally webgl-safe approach seems to be setting your own viewport explicitly every time or resetting the viewport with an empty pass every time. perhaps we could have the TrackedRenderPass set viewport automatically in webgl, but maybe it needs the viewport size which isn't available? so maybe the TrackedRenderPass could just run an empty pass for you if you don't explicitly set a viewport. should that be at the end of a viewport-setting pass or the beginning of a non-viewport-setting pass? can we make it conditional to when the viewport was previously set if the latter? is there anything we can do to make sure people are using tracked render passes instead of native ones? can we just punt the whole issue until webgpu?

@superdump
Copy link
Contributor

Please address the merge conflicts and then we can get this merged.

@superdump superdump added this pull request to the merge queue Mar 28, 2023
Merged via the queue into bevyengine:main with commit ae31361 Mar 28, 2023
@Selene-Amanita Selene-Amanita added the C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide label Jul 9, 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-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide C-Enhancement A new feature 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.

Split up MainPass3dNode
9 participants