-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
[Merged by Bors] - Do not create nor execute render passes which have no phase items to draw #4643
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks good, just one note about diagnostics.
@@ -55,7 +55,7 @@ impl Node for MainPass3dNode { | |||
Err(_) => return Ok(()), // No window | |||
}; | |||
|
|||
{ | |||
if !opaque_phase.items.is_empty() { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. :)
There was a problem hiding this comment.
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.
@@ -92,7 +92,7 @@ impl Node for MainPass3dNode { | |||
} | |||
} | |||
|
|||
{ | |||
if !alpha_mask_phase.items.is_empty() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto here.
@@ -128,7 +128,7 @@ impl Node for MainPass3dNode { | |||
} | |||
} | |||
|
|||
{ | |||
if !transparent_phase.items.is_empty() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto here.
Just to check will this still run the clear pass if there are no things to draw? |
Yes, I explicitly tested the clear_color example with 2d and 3d orthographic cameras. |
bors r+ |
…draw (#4643) # Objective - Creating and executing render passes has GPU overhead. If there are no phase items in the render phase to draw, then this overhead should not be incurred as it has no benefit. ## Solution - Check if there are no phase items to draw, and if not, do not construct not execute the render pass --- ## Changelog - Changed: Do not create nor execute empty render passes
…draw (bevyengine#4643) # Objective - Creating and executing render passes has GPU overhead. If there are no phase items in the render phase to draw, then this overhead should not be incurred as it has no benefit. ## Solution - Check if there are no phase items to draw, and if not, do not construct not execute the render pass --- ## Changelog - Changed: Do not create nor execute empty render passes
…draw (bevyengine#4643) # Objective - Creating and executing render passes has GPU overhead. If there are no phase items in the render phase to draw, then this overhead should not be incurred as it has no benefit. ## Solution - Check if there are no phase items to draw, and if not, do not construct not execute the render pass --- ## Changelog - Changed: Do not create nor execute empty render passes
Objective
Solution
Changelog