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

Consider making egui::Context !Send+!Sync #1399

Open
emilk opened this issue Mar 22, 2022 · 4 comments
Open

Consider making egui::Context !Send+!Sync #1399

emilk opened this issue Mar 22, 2022 · 4 comments
Labels
feature New feature or request

Comments

@emilk
Copy link
Owner

emilk commented Mar 22, 2022

Prompted by #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:

@emilk emilk added the feature New feature or request label Mar 22, 2022
@emilk emilk mentioned this issue Mar 22, 2022
4 tasks
emilk added a commit that referenced this issue Mar 22, 2022
https://github.com/asny/three-d recently merged a PR adding
`glow` support: asny/three-d#210
This means it is a prime candidate for embedding 3D painting inside
an eframe app.

There are currently a few kinks that need to be figured out:

When reusing the same three_d context over time (as one should),
we only get one frame of egui together with three_d, and then after that
a black screen with just the three_d painting on top.

I need to fix that before merging this PR.

`Shape` is `Send + Sync` and `three_d::Context` is not. This means
we cannot store a three_d context and send it to the `Shape::Callback`.

So we either need to recreate the three_d context each frame (obviously
a bad idea), or access it through a `thread_local` hack.
This PR adds both as examples, with a checkbox to switch.

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 is discussed in #1399
emilk added a commit that referenced this issue Mar 22, 2022
https://github.com/asny/three-d recently merged a PR adding
`glow` support: asny/three-d#210
This means it is a prime candidate for embedding 3D painting inside
an eframe app.

There are currently a few kinks that need to be figured out:

When reusing the same three_d context over time (as one should),
we only get one frame of egui together with three_d, and then after that
a black screen with just the three_d painting on top.

I need to fix that before merging this PR.

`Shape` is `Send + Sync` and `three_d::Context` is not. This means
we cannot store a three_d context and send it to the `Shape::Callback`.

So we either need to recreate the three_d context each frame (obviously
a bad idea), or access it through a `thread_local` hack.
This PR adds both as examples, with a checkbox to switch.

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 is discussed in #1399
@Pjottos
Copy link

Pjottos commented Jun 2, 2022

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.

@emilk
Copy link
Owner Author

emilk commented Jun 10, 2022

@Pjottos thank you so much for working on this!

@Pjottos
Copy link

Pjottos commented Jun 11, 2022

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.

@Speak2Erase
Copy link
Contributor

Speak2Erase commented Nov 20, 2023

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?

Speak2Erase added a commit to Speak2Erase/Luminol that referenced this issue Nov 20, 2023
In wgpu 0.17 wgpu types are not Send + Sync, because they reference things in the JS heap (meaning they cannot leave the thread they were created on)

egui_wgpu's CallbackTrait requires Send + Sync even on wasm and we need to store wgpu types in callbacks. The callback_resources typemap passed to callbacks doesn't need to be Send + Sync.. but I can't find a simple way to add anything to the typemap.

emilk/egui#1399 feels relevant
Speak2Erase added a commit to Astrabit-ST/Luminol that referenced this issue Nov 22, 2023
* refactor: 🚧 Split luminol into separate crates

* refactor: 🚧 Start working out dependencies

* Sorta figure out dependencies

* Sorta start getting somewhere

* Move Luminol into crates folder

* Move the filesystem trait out of luminol-core

* refactor: 🚧 refactor luminol-graphics and it's a mess

* refactor:

* refactor: add UpdateState to luminol-core and move various traits

* refactor(filesystem): ♻️ require FileSystem::File to be 'static

* refactor: ♻️ don't take an &'static reference to graphics state

* refactor: ♻️ refactor modals

* refactor: ♻️ unify tabs and windows

* refactor: ♻️ start to fix up windows

* refactor: ♻️ hack together things so that they compile

* refactor: ♻️ partially resolve async code issues

* refactor: ♻️ it compiles (with a LOT of unfinished things)

* refactor: ♻️ NOW it compiles

* fix(graphics): 🐛 fix sprite shader compilation

* fix(data cache): ♻️ get data cache semi-working

* refactor(data cache): ♻️ actually load data cache

* refactor(config): ♻️ store code theme in global state again

* fix(ui): 🐛 fix windows and tabs not being added

* refactor: ♻️ use workspace metadata for package metadata

* refactor: ♻️ remove generic parameters on update state by using dynamic dispatch

I couldn't get generics to allow adding tabs or windows inside tabs or windows

* refactor(tabs): ♻️ pass update state when requesting tab name

* fix(tabs): 🐛 fix tabs not adding properly

* Update window.rs

* fix: 🐛 get top bar loading projects

* fix: 🐛 fix new project creation

because reqwest requires tokio we need to spawn a tokio runtime. i do not think this scales well to web

* refactor: 🚧 try splitting up the map tab

* Enable -Zthreads compiler flag

* Update nightly in workflows

* revert: ⏪ Undo making cursor state an enum (will tackle this later)

* Sorta resolve dependencies on wasm

Still a mess (and is especially complicated by the fact that you can't specify workspace target specific dependencies)

* Fix backing web filesystem implementation

Still need to work on the project filesystem though!
I'll be honest, I'm not sure how this will work, especially with the way I have the native filesystem set up

* Fix native build

* Remove jobs flag

* Fix audio on wasm

* Temporarily impl Send + Sync for wgpu callbacks

In wgpu 0.17 wgpu types are not Send + Sync, because they reference things in the JS heap (meaning they cannot leave the thread they were created on)

egui_wgpu's CallbackTrait requires Send + Sync even on wasm and we need to store wgpu types in callbacks. The callback_resources typemap passed to callbacks doesn't need to be Send + Sync.. but I can't find a simple way to add anything to the typemap.

emilk/egui#1399 feels relevant

* Fix winit not compiling in CI

emilk/egui#3228

* Get wasm filesystem compiling

* Get luminol compiling on wasm!

* Fix missed rename

* Dumb typo

* Move luminol crate to be root package

Trunk doesn't like virtual workspaces unfortunately
:(

* Run workspace tests

* fix(audio): 🐛 Completely read audio file on wasm

* perf(wasm): ⚡ Use oneshot crate for oneshot channels

* fix(filesystem): 🐛 Pass idb key instead of path to Filesystem::from_idb_key

* refactor(tilemap): ♻️ Use naga oil instead of const_format

* fix(ui): 🐛 Fix top bar opening & closing projects

* Bump nightly in trunk build

* Remove luminol- in folder prefixes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants