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

Add get_default_configure to simplify user creation of SurfaceConfiguration #3034

Merged
merged 1 commit into from
Oct 13, 2022

Conversation

jinleili
Copy link
Contributor

@jinleili jinleili commented Sep 18, 2022

Checklist

  • Run cargo clippy.
  • Run RUSTFLAGS=--cfg=web_sys_unstable_apis cargo clippy --target wasm32-unknown-unknown if applicable.
  • Add change to CHANGELOG.md. See simple instructions inside file.

Description
Most users don't really need to care about the values of usage, format, present_mode, and possibly alpha_mode, color_space, etc. when configuring SurfaceConfiguration. It is only necessary to understand and specify these fields if the user has a clear need for them. impl Default simplifies the use of users.

Bgra8UnormSrgb is set as the default format value because it is the only format supported by all platforms. Although Bgra8Unorm and Rgba8Unorm are defined in the WebGPU spec, metal actually only supports Bgra8Unorm as the texture format for the SwapChain

Testing
Tested examples locally

@jinleili jinleili force-pushed the default_surface_config branch from dd32cb8 to 7024df7 Compare September 18, 2022 13:24
@kpreid
Copy link
Contributor

kpreid commented Sep 18, 2022

Adding Default seems like it might be a future compatibility risk. What if some new backend results in there being no single always-supported format? How about instead making it a method on Surface (or Device, or a function in wgpu::util) that accepts a width and height, and returns a configuration?

@jinleili
Copy link
Contributor Author

jinleili commented Sep 18, 2022

All field values within SurfaceConfiguration should have a valid default value (exclude width and height), it's just that this value may be reinterpreted in different backends, as in the case of Auto for alpha_mode.

If used as an util function, additional surface and adapter are also required to obtain values that can actually be supported

@cwfitzgerald
Copy link
Member

I think the biggest problem here is that FIFO isn't actually available everywhere - this is a problem I discovered after starting to tell people that FIFO works everywhere 😬 - wayland doesn't actually have any presentation mode other than MAILBOX.

@jinleili
Copy link
Contributor Author

@cwfitzgerald Is it acceptable to use AutoNoVsync instead of Fifo here?

@jinleili jinleili force-pushed the default_surface_config branch from 7024df7 to 1d5ee0a Compare September 19, 2022 04:45
wgpu-types/src/lib.rs Outdated Show resolved Hide resolved
wgpu-types/src/lib.rs Outdated Show resolved Hide resolved
@jinleili jinleili force-pushed the default_surface_config branch from 1d5ee0a to 627fdf8 Compare September 19, 2022 07:22
@jinleili jinleili force-pushed the default_surface_config branch 2 times, most recently from 5de3ffe to 21ac3ca Compare September 20, 2022 09:04
@jinleili jinleili changed the title impl Default for SurfaceConfiguration, simplify the use of users. Add get_default_configure to simplify user creation of SurfaceConfiguration Sep 20, 2022
@jinleili jinleili force-pushed the default_surface_config branch from 21ac3ca to fa44e57 Compare September 20, 2022 09:11
@cwfitzgerald
Copy link
Member

Sorry, this one has had a lot of revisions

@jinleili jinleili force-pushed the default_surface_config branch from fa44e57 to 9cba89f Compare September 22, 2022 06:19
Copy link
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

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

LGTM after one nit

wgpu/examples/framework.rs Outdated Show resolved Hide resolved
@jinleili jinleili force-pushed the default_surface_config branch from 9cba89f to caca9a3 Compare October 13, 2022 05:52
@cwfitzgerald cwfitzgerald enabled auto-merge (squash) October 13, 2022 06:09
@cwfitzgerald cwfitzgerald merged commit caee97e into gfx-rs:master Oct 13, 2022
@jinleili jinleili deleted the default_surface_config branch October 13, 2022 06:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants