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 to wgpu 0.17.0 #3170

Merged
merged 7 commits into from
Aug 11, 2023
Merged

Update to wgpu 0.17.0 #3170

merged 7 commits into from
Aug 11, 2023

Conversation

Aaron1011
Copy link
Contributor

This required bumping wasm-bindgen to 0.2.87

This required bumping wasm-bindgen to 0.2.87
@Chickenkeeper
Copy link

Would it be possible to remove the minor part of the version number, i.e. "0.17", to match most of the other dependencies? The minor versions are only used for non-breaking updates, which get missed unless egui can be updated every time wgpu does

@Mingun
Copy link
Contributor

Mingun commented Jul 26, 2023

Cargo specification already means ^0.17.0, which is shorthand for >=0.17.0,<0.18.0

@Wumpf Wumpf self-requested a review July 26, 2023 16:47
@Wumpf Wumpf added the dependencies Pull requests that update a dependency file label Jul 26, 2023
Copy link
Collaborator

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

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

👍

@Wumpf
Copy link
Collaborator

Wumpf commented Jul 26, 2023

This actually breaks the web build because of wgpu's Send/Sync changes. @Aaron1011 do you have time to look into that?

@Chickenkeeper
Copy link

Cargo specification already means ^0.17.0, which is shorthand for >=0.17.0,<0.18.0

Oh that's good to know, I was under the impression that it was equal to =0.17.0 instead. Thanks for the links, I'll read up on it properly

@Aaron1011
Copy link
Contributor Author

This is tricky - the type_map::concurrent::TypeMap instance that egui exposes requires Send + Sync tpyes, which means that you can no longer store wgpu types in it on wasm.

I think our two options are:

  • Use a non-concurrent TypeMap in the exposed field. This will complicate multithreaded non-wasm use cases, as references to that TypeMap Wil lno longer be able to be shared between threads.
  • Provide two TypeMap instances - one non-concurrent, and one type_map::concurrent::TypeMap. This will require adding a parameter to some user closures, which might break lots of user code. However, it will provide more flexibility, and make it easier for users to write code targeting wasm.

@lucasmerlin
Copy link
Collaborator

I solved the typemap problem in my project by choosing the concurrent version based on the target_arch:

#[cfg(not(target_arch = "wasm32"))]
use type_map::concurrent::TypeMap;
#[cfg(target_arch = "wasm32")]
use type_map::TypeMap;

Maybe this would work for egui as well. Since most wasm code is single threaded anyways, I don't think it'd be necessary to have the concurrent typemap on wasm.

@Aaron1011
Copy link
Contributor Author

@lucasmerlin: My concern is that doing that would make it unnecessarily difficult for downstream crates to compile against wasm - using TypeMap in a function/closure signature (e.g. for PrepareCallback) would require a similar #[cfg] in downstream crates.

@Wumpf
Copy link
Collaborator

Wumpf commented Jul 30, 2023

Huh, tricky. Agreed that the "infectious" cfg isn't great either.
Tbh I'm quite unhappy with the interface having this typemap in there in the first place. Maybe we should bite the bullet and come up with something different 🤔
Need to update wgpu over on https://github.com/rerun-io/rerun/ soon as well, so I might be spending some time on this as well in the next couple of days, but I'd be very happy for more input.
Pondering if we should go with the stop-gap-solution of enabling fragile-send-sync-non-atomic-wasm for the time being (should then ofc referencing new bug ticket explaining the situation)

@Aaron1011
Copy link
Contributor Author

I believe that gfx-rs/wgpu#3626 will unblock some lifetime constraint relaxtions (see gfx-rs/wgpu#1453). This might remove the need for egui to provide a TypeMap at all.

@Wumpf
Copy link
Collaborator

Wumpf commented Aug 1, 2023

That might very well be, but gfx-rs/wgpu#3626 has been in the works for very long and may not land all that soon. Also will definitely be a major release of wgpu, not a patch; so we're still stack with pre-arcanization.
Also depends a bit how the typemap is used there in the first place, that design is not that tied to lifetime issues I'd argue

@Speak2Erase
Copy link
Contributor

While getting rid of the typemap is nice, even after gfx-rs/wgpu#3626 lands it could take a while for them to fix gfx-rs/wgpu#1453

I'm really hoping that comes sooner rather than later though. The typemap setup is very annoying to work with when doing custom rendering.

@cwfitzgerald
Copy link
Contributor

If everything goes to plan, expect to see both changes in 0.18. No hard promises though.

@fornwall
Copy link
Contributor

fornwall commented Aug 9, 2023

As said above, perhaps features = ["fragile-send-sync-non-atomic-wasm"] is the best way forward to unblock the update for now?

We are no worse off than with wgpu 0.16.0, and a proper fix can hopefully be included in a future wgpu 0.18.0/0.19.0 update.

@emilk
Copy link
Owner

emilk commented Aug 9, 2023

Sounds good to me @fornwall

@Wumpf Wumpf added this to the 0.23.0 milestone Aug 10, 2023
@fornwall
Copy link
Contributor

@Aaron1011 Is setting features = ["fragile-send-sync-non-atomic-wasm"] in this PR something you have time to do?

@emilk emilk added egui-wgpu eframe Relates to epi and eframe labels Aug 11, 2023
crates/eframe/Cargo.toml Outdated Show resolved Hide resolved
@emilk emilk changed the title Bump wgpu to 0.17.0 Update to wgpu 0.17.0 Aug 11, 2023
@emilk
Copy link
Owner

emilk commented Aug 11, 2023

Tested natively on Mac, as well as on Web.

@Wumpf Wumpf merged commit 9808702 into emilk:master Aug 11, 2023
18 checks passed
@Aaron1011
Copy link
Contributor Author

Sorry for the delay in getting to this - thank you for taking care of this!

@daxpedda
Copy link
Contributor

As said above, perhaps features = ["fragile-send-sync-non-atomic-wasm"] is the best way forward to unblock the update for now?

We are no worse off than with wgpu 0.16.0, and a proper fix can hopefully be included in a future wgpu 0.18.0/0.19.0 update.

FWIW egui used to work fine with Wasm Atomics, after this PR it won't compile with it.

In any case, great work here!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file eframe Relates to epi and eframe egui-wgpu
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants