egui icon indicating copy to clipboard operation
egui copied to clipboard

Consider making egui::Context !Send+!Sync

Open emilk opened this issue 3 years ago • 3 comments

Prompted by https://github.com/emilk/egui/pull/1398

Currently Context and Shape are both Send+Sync. This means Shape::Callback can only capture things that are Send+Sync, even though almost all users will be running their UI code on the the paint thread.

We could consider making Shape: !Send + !Sync, but that would mean egui::Context could not be Send+Sync either (because the egui context stores shapes). This could actually be fine. egui::Context should only be used from a background thread for calling request_repaint and allocating textures. These could be made separate parts of the egui Context, so that one would do:

let repaint_signal = egui_ctx.repaint_signal();
let tex_mngr = egui_ctx.tex_mngr();
std::thread::spawn(move || {
    // We can use repaint_signal and tex_mngr here,
    // but NOT `egui_ctx`.
}):

Making Context: !Sync would also solve deadlocks discussed in #1379 and #1380.

The downside of this is that it would stop users from running their UI code in a separate thread from their main window/paint thread.

Related:

  • https://github.com/emilk/egui/issues/1379
  • https://github.com/emilk/egui/pull/1380
  • https://github.com/emilk/egui/pull/1389

emilk avatar Mar 22 '22 09:03 emilk

I'm looking into this, removing locks entirely from the egui crate. It's quite a challenge to both keep the API simple and refactor the way multiple (mutable) references are kept to the context, but I think eliminating the deadlocks is worth it since it's a pretty confusing issue.

Pjottos avatar Jun 02 '22 21:06 Pjottos

@Pjottos thank you so much for working on this!

emilk avatar Jun 10 '22 10:06 emilk

I got the demo to compile and run without a lock on the context. Seems to be working as expected ~~but the performance actually regressed which might be because of extra copying? Should be fixable.~~ Can't reproduce this anymore, frame times seem a bit lower in the demo app with all windows open.

Pjottos avatar Jun 11 '22 12:06 Pjottos

With wgpu marking its types !Send + !Sync on wasm this means using egui_wgpu::CallbackTrait is still really, really difficult, even with the API improvments in 0.23.0. Making Context !Send + !Sync (at least on wasm) would go a long way to alleviate that imho.

At the moment, if you want to access any wgpu types in your custom callbacks you can't store them directly in a struct impl'ing egui_wgpu::CallbackTrait, you need to instead manually append stuff to egui_wgpu::Renderer::callback_resources, and getting a hold of an egui_wgpu::Renderer isn't very easy.

By making Context !Send + !Sync egui_wgpu::CallbackTrait would no longer need to be Send + Sync either and it'd be possible to directly store wgpu types in your callback structs. It might even be possible at that point to drop callback_resources entirely?

melody-rs avatar Nov 20 '23 06:11 melody-rs