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

WGPU 0.10 #185

Closed
JMS55 opened this issue Aug 18, 2021 · 4 comments · Fixed by #187
Closed

WGPU 0.10 #185

JMS55 opened this issue Aug 18, 2021 · 4 comments · Fixed by #187
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@JMS55
Copy link
Contributor

JMS55 commented Aug 18, 2021

No description provided.

@parasyte parasyte added enhancement New feature or request help wanted Extra attention is needed labels Aug 19, 2021
@parasyte
Copy link
Owner

parasyte commented Aug 19, 2021

I kind of want to replace our SurfaceTexture and SurfaceSize with wgpu::SurfaceConfiguration. But the surface config owns some things that are currently optional in PixelsBuilder. The best course of action right now looks like slimming down the builder and making the render texture format required. E.g. the caller can use surface::get_preferred_format() if they want to.

I'll mull it over. But this was just one of the things I noticed while working on it this morning.

@JMS55
Copy link
Contributor Author

JMS55 commented Aug 20, 2021

Taking a brief look, wgpu::SurfaceConfiguration doesn't have a window handle. Were you thinking or something like, instead of Pixels::new(pixels_size, window, window_size) (which is approximately what we have now), you'd do Pixels::new(pixels_size, window, wgpu::SurfaceConfiguration)? Imo that's worse - it's more work the user has to do, when there's sensible defaults we could choose for them. And then if they want advanced usage, that's what the builder is for.

I'm of the opinion, based on a quick 5m thinking, that our current API is fine/better. Maybe we could flatten Pixels::new() out instead of taking structs the user has to marshal their window and window size into, if we made any changes.

@parasyte
Copy link
Owner

parasyte commented Aug 20, 2021

Sort of. I was thinking of something like this for the builder:

let builder = PixelsBuilder::new(width, height, &window, surface_config);

And to retain some of the niceties of the current API, adding a new util function to create a wgpu::SurfaceConfiguration might be helpful:

pub fn surface_config(width: u32, height: u32, format: Option<wgpu::TextureFormat>) -> wgpu::SurfaceConfiguration

There's still a distinction between the "surface size" and the "pixel buffer size". And the surface_config method can pick sane defaults for usage, format, and present_mode, but of course this util function is entirely optional. The existing builder methods can still be used to override any of the defaults.

In short, pixels::surface_config() replaces pixels::SurfaceTexture::new().

The reason I want to do this is because we need to maintain a SurfaceConfiguration anyway, (this is the new way to recreate the swap chain). This structure owns many of the things that would otherwise be internally duplicated or massaged into a SurfaceConfiguration. The goal is consolidating data management. And if it means removing some structs that serve a similar purpose, that sounds like a win. (Obviously dependent on not making ergonomics objectively worse.)

@parasyte
Copy link
Owner

parasyte commented Sep 1, 2021

@JMS55 The updates I have in #187 so far don't change the builder in the way I was thinking aloud above. I think the only publicly visible change (apart from reexports) is renaming Error::Swapchain -> Error::Surface.

We don't need to make any of the changes described above for this upgrade. But I will create a new ticket for API change ideas.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants