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

Update wgpu (and ash, to avoid duplicate deps). #925

Merged
merged 2 commits into from
Mar 22, 2023

Conversation

eddyb
Copy link
Contributor

@eddyb eddyb commented Oct 4, 2022

For updating wgpu, these backwards-incompatible changes required fixes:

The original hope was that with newer wgpu we'll get newer naga which might fix the bug breaking the mouse shader (likely gfx-rs/naga#1977 - at least with SPIR-T) - but alas, no such luck
(however, replacing the for loop with a while loop, and turning off SPIR-T with RUSTGPU_CODEGEN_ARGS=--no-spirt works around naga bugs, and is enough for testing).


I've also took the liberty of tweaking a couple dependency versions and (selectively) running cargo update -p ... to reduce duplicates in Cargo.lock, hence the larger amount of changes than might be expected (and the dependency on EmbarkStudios/ash-molten#71).


One thing I have not figured out yet is why the mouse shader (with the locally applied workaround) doesn't render new frames except on input events only with X11 (Wayland is fine) - there may be a "power saving" setting that has changed default in wgpu/winit?

EDIT: turns out that was us using winit as if we were a GUI application, in "only redraw after events" mode (not sure my workaround here is the best approach, but it works, and enabling VSync makes it acceptable).

@eddyb
Copy link
Contributor Author

eddyb commented Oct 4, 2022

This is incredible, the cargo-deny failure is because of something needed for font rendering.

Why would we need to render text? Well, we don't, except... Look at what winit needs to support Wayland:

So according to that, there's this "SCTK" (smithay-client-toolkit) which among other things handles CSDs on Wayland (where CSD is "Client-Side Decoration", i.e. letting an app have a custom titlebar).

But why would winit default to CSD? Surely custom window decorations are optional, right?
Well, GNOME appears to have forced the custom mode on everyone.

Not just that but some of them even posted to KDE venues proposing KDE also do the same (i.e. remove perfectly good WM-side window decorations and force everyone to use custom "CSD" ones).

My only hope is that I can turn this stuff off and maybe cargo-deny will then be happy but I have doubts.
EDIT: doubts were justified, cargo-deny doesn't ignore disabled optional dependencies.

EDIT2: ah, nice, rust-windowing/winit#2438 replaces the problematically-licensed dependency, and I switched winit to a git dep, but that might block this PR until a new winit version is released.

@eddyb eddyb force-pushed the wgpu-update branch 2 times, most recently from 8313bc1 to 5765a5f Compare October 4, 2022 17:34
@eddyb eddyb force-pushed the wgpu-update branch 3 times, most recently from ec034a9 to 9d875b5 Compare October 19, 2022 12:17
present_mode: wgpu::PresentMode::Mailbox,
present_mode: wgpu::PresentMode::AutoNoVsync,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change was found by @fu5ha because macOS apparently doesn't support Mailbox (did this ever work? I think main might be broken too).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that in the latest version of this PR, I've changed it to AutoVsync because AutoNoVsync was doing excessive redraws (thousands of frames per second).

@ValorZard
Copy link

All the stuff that was blocking this PR got merged, so does that mean that wgpu can be updated now?

@eddyb
Copy link
Contributor Author

eddyb commented Nov 22, 2022

All the stuff that was blocking this PR got merged, so does that mean that wgpu can be updated now?

@ValorZard To be clear, none of my links above have changed status since I posted them. I was linking already merged PRs, some of them just to describe why certain changes where needed.

The only blocker link AFAICT is there's no new release containing e.g. the crossfont->ab_glyph change (rust-windowing/winit@f9b41fd - note how GitHub doesn't list any tags as containing it).

You can also see it's still under the "Unreleased" section in the CHANGELOG right now:

  • On Wayland, wayland-csd-adwaita now uses ab_glyph instead of crossfont to render the title for decorations.

We're basically waiting for winit to release 0.28, which I'm not seeing a timeline for.


For now, this update is not that important, at least not for Naga, until these issues are fixed:

We should just make it possible to use the raw SPIR-V (i.e. bypassing Naga entirely) until Naga improves.

@eddyb eddyb marked this pull request as ready for review March 22, 2023 03:14
@eddyb eddyb requested review from fu5ha, VZout and oisyn as code owners March 22, 2023 03:14
@eddyb eddyb enabled auto-merge (rebase) March 22, 2023 03:15
@eddyb
Copy link
Contributor Author

eddyb commented Mar 22, 2023

winit 0.28 has since been released (and since then some further patch releases too).

I've further updated winit, wgpu (and a few other miscellaneous crates), but also changed how we handle wgpu's naga dependency, so that we don't have to update wgpu to get newer naga.

That was in part motivated by a desire to maybe try fixing naga bugs ourselves, and being on the the latest from git would make that much easier - but also we could try out e.g. wgpu raytracing support, before it lands.

EDIT: the naga part was a bad idea, it creates noise on every Cargo command, took it out for now.

@eddyb

This comment was marked as resolved.

@MarijnS95

This comment was marked as resolved.

@eddyb
Copy link
Contributor Author

eddyb commented Mar 22, 2023

Got Android building (and WASM for that matter, but it doesn't work, I get a lot of errors I haven't looked into).

This is what I can get out of cargo apk run -p example-runner-wgpu --lib --target x86_64-linux-android, after starting the Android emulator with -no-snapshot -feature Vulkan,GLDirectMem:

I'm not sure what's wrong with the texture format, but the na-winit-wgpu example has the same problem.

@eddyb
Copy link
Contributor Author

eddyb commented Mar 22, 2023

Opened a separate PR with the the addition I was using to test the WASM path:

Which will have to wait until the massive problem with wgpu::backend::web is fixed:

So this PR is truly unblocked, just needs a review now!

@eddyb eddyb removed request for oisyn and fu5ha March 22, 2023 18:15
@eddyb eddyb requested review from fu5ha and VZout and removed request for VZout March 22, 2023 18:16
@eddyb eddyb merged commit 9cd0b27 into EmbarkStudios:main Mar 22, 2023
@eddyb eddyb deleted the wgpu-update branch March 22, 2023 19:01
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.

4 participants