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.

Sign up for GitHub

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

Jump to bottom

[Merged by Bors] - Do not create nor execute render passes which have no phase items to draw #4643

Closed
wants to merge 1 commit into from
Closed

[Merged by Bors] - Do not create nor execute render passes which have no phase items to draw #4643

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions crates/bevy_core_pipeline/src/main_pass_2d.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,10 @@ impl Node for MainPass2dNode {
.get_manual(world, view_entity)
.expect("view entity should exist");

if transparent_phase.items.is_empty() {
return Ok(());
}

let pass_descriptor = RenderPassDescriptor {
label: Some("main_pass_2d"),
color_attachments: &[target.get_color_attachment(Operations {
Expand Down
6 changes: 3 additions & 3 deletions crates/bevy_core_pipeline/src/main_pass_3d.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ impl Node for MainPass3dNode {
Err(_) => return Ok(()), // No window
};

{
if !opaque_phase.items.is_empty() {
Copy link
Member

Choose a reason for hiding this comment

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

Should we move this check inside the tracing span? This could impact profile output and averages across multiple frames if applied inconsistently.

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 feel like it would mess up mean and median execution times if we count the zeros, so to speak. If the pass runs once per 10 frames for whatever reason, would we want to see a mean that is 1/10th the single-frame execution time? I feel like not.

Copy link
Member

Choose a reason for hiding this comment

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

It may be a matter of expectations.

If it's viewed from the perspective of "how much frame time does this section take on average?", I'd say counting the zeroes here is accurate, and properly reflects the performance changes that do affect the phase. If the phase is only in use 10% of the time, any optimization made within each will only impact 10% of the frames.

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 see your point. I just disagree. :)

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’ll merge it like this and if it turns out to be the wrong call, it is easy to change. I’m thinking that frame time consistency is more important than throughout. If you do something once every 10 frames and it makes for a significant frame stutter, that’s bad. So averaging over only three instances where the thing actually happens seems like the preference. Maybe I’m wrong. Or maybe we need both for different situations.

// Run the opaque pass, sorted front-to-back
// NOTE: Scoped to drop the mutable borrow of render_context
#[cfg(feature = "trace")]
Expand Down Expand Up @@ -92,7 +92,7 @@ impl Node for MainPass3dNode {
}
}

{
if !alpha_mask_phase.items.is_empty() {
Copy link
Member

Choose a reason for hiding this comment

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

Ditto here.

// Run the alpha mask pass, sorted front-to-back
// NOTE: Scoped to drop the mutable borrow of render_context
#[cfg(feature = "trace")]
Expand Down Expand Up @@ -128,7 +128,7 @@ impl Node for MainPass3dNode {
}
}

{
if !transparent_phase.items.is_empty() {
Copy link
Member

Choose a reason for hiding this comment

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

Ditto here.

// Run the transparent pass, sorted back-to-front
// NOTE: Scoped to drop the mutable borrow of render_context
#[cfg(feature = "trace")]
Expand Down
5 changes: 5 additions & 0 deletions crates/bevy_pbr/src/render/light.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1439,6 +1439,11 @@ impl Node for ShadowPassNode {
.view_light_query
.get_manual(world, view_light_entity)
.unwrap();

if shadow_phase.items.is_empty() {
continue;
}

let pass_descriptor = RenderPassDescriptor {
label: Some(&view_light.pass_name),
color_attachments: &[],
Expand Down
5 changes: 5 additions & 0 deletions crates/bevy_ui/src/render/render_pass.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,11 @@ impl Node for UiPassNode {
.query
.get_manual(world, view_entity)
.expect("view entity should exist");

if transparent_phase.items.is_empty() {
return Ok(());
}

let pass_descriptor = RenderPassDescriptor {
label: Some("ui_pass"),
color_attachments: &[RenderPassColorAttachment {
Expand Down