Upgrade to wgpu v29. Reduce MSRV to 1.88.#1534
Conversation
| ## Minimum supported Rust Version (MSRV) | ||
|
|
||
| This version of Vello has been verified to compile with **Rust 1.86** and later. | ||
| This version of Vello has been verified to compile with **Rust 1.88** and later. |
There was a problem hiding this comment.
This should have been updated to 1.92 with the wgpu v28 bump but got missed
| CurrentSurfaceTexture::Occluded | ||
| | CurrentSurfaceTexture::Timeout | ||
| | CurrentSurfaceTexture::Outdated | ||
| | CurrentSurfaceTexture::Suboptimal(_) => { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Should we really silently return in case those happened? Seems to me like, especially for the example, it's fine to just panic here
There was a problem hiding this comment.
People will copy the example.
There was a problem hiding this comment.
But then is the best thing to do really to just return? I don't know, I'm not super familiar with wgpu API. 😄
There was a problem hiding this comment.
It's important not to panic here (which we were previously doing!). These errors can and do happen relatively often (we've gotten bug reports for Blitz), but they are transient/recoverable errors so the recommended thing to do is skip the current frame and immediately retry by rendering another frame.
See the WGPU v29 release notes, where they've intentionally changed this API not to use Result to push people towards not just calling unwrap/expect on these errors. https://github.com/gfx-rs/wgpu/releases/tag/v29.0.0
For some of those cases, I'm supposed to "reconfigure the surface", which I have done in the native cases, but I wasn't sure how to do that on the webgl backend. And for the Lost case you're apparently supposed to recreate the device in some cases (but I think this happens much less often so I'm happy to just continue panicking here for now).
| .surface | ||
| .get_current_texture() | ||
| .expect("failed to get surface texture"); | ||
| let surface_texture = match surface.surface.get_current_texture() { |
| let backend_options = wgpu::BackendOptions::from_env_or_default(); | ||
| let instance = Instance::new(&wgpu::InstanceDescriptor { | ||
| let instance = Instance::new(wgpu::InstanceDescriptor { | ||
| // TODO: consider allowing users to pass display handle when constructing a RenderContext |
There was a problem hiding this comment.
Will this cause regressions for existing users if we don't allow this now?
There was a problem hiding this comment.
My understanding is that this primarily impacts GL backends. Which I think is not relevant at all for Vello Classic. It could potentially affect Vello Hybrid (but the WebGL backend has a separate setup where we do already pass the "web display handle").
So I believe it would be "people running Vello Hybrid with OpenGL on native platforms" that would need this.
PR for this is gfx-rs/wgpu#8782
| .surface | ||
| .get_current_texture() | ||
| .expect("failed to get surface texture"); | ||
| let surface_texture = match surface.surface.get_current_texture() { |
|
Very confused what's going on with CI here: the errors seem to reference code which doesn't exist (either locally or in the Github view of the file). And the commands are passing locally :/ And I've also tried busting the cache (by changing the cache key), which seems to be working because it says "no cache found", but that doesn't seem to help. @xStrom @DJMcNab Any idea what might be going on here? Am I being an idiot? |
|
On mobile, so hard to check. Best guess is that it merges clean, so CI is running on the combined merge. That behaviour sort of makes sense, but GitHub explains it exceedingly poorly; you have to infer what's going on. So to rule that out, I'd try a rebase (or a fast forward merge? Idk, I always rebase) |
|
I think you probably just haven't rebased onto newest main, which contains the new opaque/alpha strips path. |
DJMcNab
left a comment
There was a problem hiding this comment.
All seems reasonable to me, although this is a mobile only review, and I've not yet read the upstream changelog, so won't review it.
If I've not reviewed by 16:30ish Sydney time tomorrow, feel free to ping again here and/or on Zulip.
| .expect("failed to get surface texture"); | ||
| let surface_texture = match surface.surface.get_current_texture() { | ||
| CurrentSurfaceTexture::Success(surface_texture) => surface_texture, | ||
| CurrentSurfaceTexture::Outdated | CurrentSurfaceTexture::Suboptimal(_) => { |
There was a problem hiding this comment.
There are reasonable arguments for still drawing to a suboptimal surface. I'm worried about the case where suboptimal means "use a different device", which would mean we would loop here (this is unvalidated, to be clear, as I'm on mobile)
Given that this is an example, I don't really care about it.
There was a problem hiding this comment.
Yeah, not sure. Either way, this seems better than panicking (which is what we were doing before). Handling the "should use a different device case" seems like it would be good to do in future (also for CurrentSurfaceTexture::Lost), but is probably quite rare in practice?
| let instance = wgpu::Instance::new(wgpu::InstanceDescriptor { | ||
| backends: wgpu::Backends::GL, | ||
| ..Default::default() | ||
| ..wgpu::InstanceDescriptor::new_with_display_handle(Box::new(OurDisplayHandle)) |
There was a problem hiding this comment.
Why not Box::new(DisplayHandle::web())? I don't think this example is quite so precious about memory usage. We could give a hint comment if we really wanted to.
There was a problem hiding this comment.
Because that wasn't valid until wgpu v29.0.1! But we should be able to change that now :)
There was a problem hiding this comment.
Hmm... actually this still isn't working, even with v29.0.1. I've reinstated this code. Issue is that DisplayHandle::web() isn't Send/Sync. I did have discussions with wgpu maintainers about removing this bound for WASM (which wgpu already does elsewhere), but I don't think this has actually happened yet.
There was a problem hiding this comment.
Ah, apologies. Thanks for checking! If there's an upstream issue, we should link to it in a comment (not checked if you're already done this).
| } | ||
|
|
||
| fn configure_surface(&self, surface: &RenderSurface<'_>) { | ||
| pub fn configure_surface(&self, surface: &RenderSurface<'_>) { |
There was a problem hiding this comment.
Is this finally my impetus for killing this file entirely?
Either way, that shouldn't happen in this PR...
There was a problem hiding this comment.
I do now maintain https://docs.rs/wgpu_context as part of AnyRender which can potentially serve as a replacement (it would be annoying if vello depended on this, but a dev-dependency would be fine).
Signed-off-by: Nico Burns <nico@nicoburns.com>
b9d22ce to
2854169
Compare
Signed-off-by: Nico Burns <nico@nicoburns.com>
(this was it. I had rebased on my local |
@DJMcNab ping! |
DJMcNab
left a comment
There was a problem hiding this comment.
I've not re-reviewed this - I'll aim to post-review next week. I've got an extra long weekend, so that won't be before Tuesday. But happy to land to let dependant PRs happen.
Cool. In that case I will merge now to avoid conflicts. |
Upgrade Vello and Vello Hybrid to wgpu v29. Also reduces MSRV to 1.88 as wgpu lowered theirs.
https://github.com/gfx-rs/wgpu/releases/tag/v29.0.0