[Merged by Bors] - Avoid creating SurfaceConfiguration in prepare_windows#6255
[Merged by Bors] - Avoid creating SurfaceConfiguration in prepare_windows#6255MDeiml wants to merge 2 commits intobevyengine:mainfrom
SurfaceConfiguration in prepare_windows#6255Conversation
This changes `prepare_windows` to only create a `SurfaceConfiguration` if it is acutally needed. Without this change, `SurfaceConfiguration` is built for every window in every frame. This also includes calling `get_supported_formats` for every window.
nicopap
added
A-Windowing
There was a problem hiding this comment.
So the basic change is to replace the:
let swap_chain_descriptor = wgpu…with
let create_swap_chain_descriptor = || wgpu…And call the closure when relevant. I've no idea on the perf profile of get_supported_formats (defined here https://docs.rs/wgpu/latest/src/wgpu/backend/direct.rs.html#963-975) So this might be an improvement.
The other change is to split the render_device.configure_surface(…) into two separate branches. The only benefit is to have a distinct error message ("error configuring" vs "error reconfiguring"), but I find it makes it much more difficult to read the code and I don't see how having a slightly different error message in the case of initial configuration helps, IMO this should be reverted, and only the "lazification" of swap_chain_descriptor should be kept.
Obviously if you have a good reason for this I'm missing I'd be glad to hear it out.
|
Without changing the control flow the I think that this case should never happen though, as it does not make sense for the surface to be outdated immediately after being configured. So calling the closure twice should be ok. I just think that the control flow I used represents the actual logic better, since the intent is to call |
|
I also had a shallow look at how |
|
Of course! Yeah naturally, the let force_update_surface = window_surfaces.configured_windows.insert(window.id)
|| window.size_changed
|| window.present_mode_changed;
let frame = match (surface.get_current_texture(), force_update_surface) {
(_, true) | (Err(Outdated), _) => {
render_device.configure_surface(surface, &create_swap_chain_descriptor());
surface
.get_current_texture()
.expect("Error reconfiguring surface")
}
(Ok(swap…), false) => …
…
};
window.swap_chain_texture = Some(TextureView::from(frame));A more "imperative" alternative would make judicious use of an early return and inline the Regardless, you made a good point, and the code is acceptable as is. |
alice-i-cecile
left a comment
alice-i-cecile
left a comment
There was a problem hiding this comment.
Makes sense, and I've checked to ensure that the logic is equivalent.
Added a small comment to make this logic a bit easier to skim; 0 parameter closures can be hard to read.
alice-i-cecile
added
the
S-Ready-For-Final-Review
|
Great work, this is a nice little optimization. bors r+ |
# Objective - Avoids creating a `SurfaceConfiguration` for every window in every frame for the `prepare_windows` system - As such also avoid calling `get_supported_formats` for every window in every frame ## Solution - Construct `SurfaceConfiguration` lazyly in `prepare_windows` --- This also changes the error message for failed initial surface configuration from "Failed to acquire next swapchain texture" to "Error configuring surface".
|
Build failed (retrying...): |
|
bors r- |
|
Canceled. |
MDeiml
force-pushed
the
avoid-creating-surface-config
branch
from
a82c3fb to
1a7a539
Compare
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
MDeiml
force-pushed
the
avoid-creating-surface-config
branch
from
1a7a539 to
fb287c0
Compare
|
Fixed it |
|
bors r+ |
# Objective - Avoids creating a `SurfaceConfiguration` for every window in every frame for the `prepare_windows` system - As such also avoid calling `get_supported_formats` for every window in every frame ## Solution - Construct `SurfaceConfiguration` lazyly in `prepare_windows` --- This also changes the error message for failed initial surface configuration from "Failed to acquire next swapchain texture" to "Error configuring surface".
bors
bot
changed the title
SurfaceConfiguration in prepare_windowsSurfaceConfiguration in prepare_windows
…e#6255) # Objective - Avoids creating a `SurfaceConfiguration` for every window in every frame for the `prepare_windows` system - As such also avoid calling `get_supported_formats` for every window in every frame ## Solution - Construct `SurfaceConfiguration` lazyly in `prepare_windows` --- This also changes the error message for failed initial surface configuration from "Failed to acquire next swapchain texture" to "Error configuring surface".
…e#6255) # Objective - Avoids creating a `SurfaceConfiguration` for every window in every frame for the `prepare_windows` system - As such also avoid calling `get_supported_formats` for every window in every frame ## Solution - Construct `SurfaceConfiguration` lazyly in `prepare_windows` --- This also changes the error message for failed initial surface configuration from "Failed to acquire next swapchain texture" to "Error configuring surface".
…e#6255) # Objective - Avoids creating a `SurfaceConfiguration` for every window in every frame for the `prepare_windows` system - As such also avoid calling `get_supported_formats` for every window in every frame ## Solution - Construct `SurfaceConfiguration` lazyly in `prepare_windows` --- This also changes the error message for failed initial surface configuration from "Failed to acquire next swapchain texture" to "Error configuring surface".
4 participants
Objective
SurfaceConfigurationfor every window in every frame for theprepare_windowssystemget_supported_formatsfor every window in every frameSolution
SurfaceConfigurationlazyly inprepare_windowsThis also changes the error message for failed initial surface configuration from "Failed to acquire next swapchain texture" to "Error configuring surface".