Skip to content

Commit

Permalink
Context::request_repaint will wake up the UI thread (#1366)
Browse files Browse the repository at this point in the history
This adds a callback (set by `Context::set_request_repaint_callback`)
which integration can use to wake up the UI thread.

eframe (egui_web and egui_glow) will use this, replacing
`epi::Frame::request_repaint`.

Existing code calling `epi::Frame::request_repaint` should be changed
to instead call `egui::Context::request_repaint`.

This is the first callback added to the egui API, which otherwise is
completely driven by data.

The purpose of this is to remove the confusion between the two
`request_repaint` methods (by removing one). Furthermore, it makes
`epi::Frame` a lot simpler, allowing future simplifications to it
(perhaps no longer having it be `Send+Sync+Clone`).
  • Loading branch information
emilk committed Mar 15, 2022
1 parent 6aee499 commit c768d1d
Show file tree
Hide file tree
Showing 12 changed files with 46 additions and 48 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ NOTE: [`epaint`](epaint/CHANGELOG.md), [`eframe`](eframe/CHANGELOG.md), [`egui_w
### Added ⭐
* Add `Shape::Callback` for backend-specific painting ([#1351](https://github.com/emilk/egui/pull/1351)).
* Added `Frame::canvas` ([#1362](https://github.com/emilk/egui/pull/1362)).
* `Context::request_repaint` will wake up UI thread, if integrations has called `Context::set_request_repaint_callback` ([#1366](https://github.com/emilk/egui/pull/1366)).

### Changed 🔧
* `ClippedMesh` has been replaced with `ClippedPrimitive` ([#1351](https://github.com/emilk/egui/pull/1351)).
Expand Down
2 changes: 2 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions eframe/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ NOTE: [`egui_web`](../egui_web/CHANGELOG.md), [`egui-winit`](../egui-winit/CHANG
## Unreleased
* Remove the `egui_glium` feature. `eframe` will now always use `egui_glow` as the native backend ([#1357](https://github.com/emilk/egui/pull/1357)).
* Change default for `NativeOptions::drag_and_drop_support` to `true` ([#1329](https://github.com/emilk/egui/pull/1329)).
* Removed `Frame::request_repaint` - just call `egui::Context::request_repaint` for the same effect ([#1366](https://github.com/emilk/egui/pull/1366)).


## 0.17.0 - 2022-02-22
Expand Down
6 changes: 3 additions & 3 deletions eframe/examples/download_image.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,18 +20,18 @@ impl epi::App for MyApp {
"Download and show an image with eframe/egui"
}

fn update(&mut self, ctx: &egui::Context, frame: &epi::Frame) {
fn update(&mut self, ctx: &egui::Context, _frame: &epi::Frame) {
let promise = self.promise.get_or_insert_with(|| {
// Begin download.
// We download the image using `ehttp`, a library that works both in WASM and on native.
// We use the `poll-promise` library to communicate with the UI thread.
let frame = frame.clone();
let ctx = ctx.clone();
let (sender, promise) = Promise::new();
let request = ehttp::Request::get("https://picsum.photos/seed/1.759706314/1024");
ehttp::fetch(request, move |response| {
let image = response.and_then(parse_response);
sender.send(image); // send the results back to the UI thread.
frame.request_repaint(); // wake up UI thread
ctx.request_repaint(); // wake up UI thread
});
promise
});
Expand Down
2 changes: 0 additions & 2 deletions egui-winit/src/epi.rs
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,6 @@ impl EpiIntegration {
max_texture_side: usize,
window: &winit::window::Window,
gl: &std::rc::Rc<glow::Context>,
repaint_signal: std::sync::Arc<dyn epi::backend::RepaintSignal>,
persistence: crate::epi::Persistence,
app: Box<dyn epi::App>,
) -> Self {
Expand All @@ -252,7 +251,6 @@ impl EpiIntegration {
native_pixels_per_point: Some(crate::native_pixels_per_point(window)),
},
output: Default::default(),
repaint_signal,
});

if prefer_dark_mode == Some(true) {
Expand Down
20 changes: 19 additions & 1 deletion egui/src/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ struct ContextImpl {

/// While positive, keep requesting repaints. Decrement at the end of each frame.
repaint_requests: u32,
request_repaint_callbacks: Option<Box<dyn Fn() + Send + Sync>>,
}

impl ContextImpl {
Expand Down Expand Up @@ -533,11 +534,28 @@ impl Context {

impl Context {
/// Call this if there is need to repaint the UI, i.e. if you are showing an animation.
///
/// If this is called at least once in a frame, then there will be another frame right after this.
/// Call as many times as you wish, only one repaint will be issued.
///
/// If called from outside the UI thread, the UI thread will wake up and run,
/// provided the egui integration has set that up via [`Self::set_request_repaint_callback`]
/// (this will work on `eframe`).
pub fn request_repaint(&self) {
// request two frames of repaint, just to cover some corner cases (frame delays):
self.write().repaint_requests = 2;
let mut ctx = self.write();
ctx.repaint_requests = 2;
if let Some(callback) = &ctx.request_repaint_callbacks {
(callback)();
}
}

/// For integrations: this callback will be called when an egui user calls [`Self::request_repaint`].
///
/// This lets you wake up a sleeping UI thread.
pub fn set_request_repaint_callback(&self, callback: impl Fn() + Send + Sync + 'static) {
let callback = Box::new(callback);
self.write().request_repaint_callbacks = Some(callback);
}

/// Tell `egui` which fonts to use.
Expand Down
1 change: 1 addition & 0 deletions egui_demo_lib/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ epi = { version = "0.17.0", path = "../epi" }

chrono = { version = "0.4", optional = true, features = ["js-sys", "wasmbind"] }
enum-map = { version = "2", features = ["serde"] }
tracing = "0.1"
unicode_names2 = { version = "0.5.0", default-features = false }

# feature "http":
Expand Down
3 changes: 1 addition & 2 deletions egui_demo_lib/src/apps/http_app.rs
Original file line number Diff line number Diff line change
Expand Up @@ -78,11 +78,10 @@ impl epi::App for HttpApp {

if trigger_fetch {
let ctx = ctx.clone();
let frame = frame.clone();
let (sender, promise) = Promise::new();
let request = ehttp::Request::get(&self.url);
ehttp::fetch(request, move |response| {
frame.request_repaint(); // wake up UI thread
ctx.request_repaint(); // wake up UI thread
let resource = response.map(|response| Resource::from_response(&ctx, response));
sender.send(resource);
});
Expand Down
1 change: 1 addition & 0 deletions egui_glow/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ epi = { version = "0.17.0", path = "../epi", optional = true }
bytemuck = "1.7"
glow = "0.11"
memoffset = "0.6"
parking_lot = "0.12"
tracing = "0.1"

[target.'cfg(not(target_arch = "wasm32"))'.dependencies]
Expand Down
20 changes: 7 additions & 13 deletions egui_glow/src/epi_backend.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,6 @@ use egui_winit::winit;

struct RequestRepaintEvent;

struct GlowRepaintSignal(std::sync::Mutex<winit::event_loop::EventLoopProxy<RequestRepaintEvent>>);

impl epi::backend::RepaintSignal for GlowRepaintSignal {
fn request_repaint(&self) {
self.0.lock().unwrap().send_event(RequestRepaintEvent).ok();
}
}

#[allow(unsafe_code)]
fn create_display(
window_builder: winit::window::WindowBuilder,
Expand Down Expand Up @@ -56,22 +48,24 @@ pub fn run(app: Box<dyn epi::App>, native_options: &epi::NativeOptions) -> ! {
let (gl_window, gl) = create_display(window_builder, &event_loop);
let gl = std::rc::Rc::new(gl);

let repaint_signal = std::sync::Arc::new(GlowRepaintSignal(std::sync::Mutex::new(
event_loop.create_proxy(),
)));

let mut painter = crate::Painter::new(gl.clone(), None, "")
.unwrap_or_else(|error| panic!("some OpenGL error occurred {}\n", error));
let mut integration = egui_winit::epi::EpiIntegration::new(
"egui_glow",
painter.max_texture_side(),
gl_window.window(),
&gl,
repaint_signal,
persistence,
app,
);

{
let event_loop_proxy = parking_lot::Mutex::new(event_loop.create_proxy());
integration.egui_ctx.set_request_repaint_callback(move || {
event_loop_proxy.lock().send_event(RequestRepaintEvent).ok();
});
}

let mut is_focused = true;

event_loop.run(move |event, _, control_flow| {
Expand Down
19 changes: 10 additions & 9 deletions egui_web/src/backend.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,12 +50,6 @@ impl NeedRepaint {
}
}

impl epi::backend::RepaintSignal for NeedRepaint {
fn request_repaint(&self) {
self.0.store(true, SeqCst);
}
}

// ----------------------------------------------------------------------------

fn web_location() -> epi::Location {
Expand Down Expand Up @@ -150,8 +144,6 @@ impl AppRunner {

let prefer_dark_mode = crate::prefer_dark_mode();

let needs_repaint: std::sync::Arc<NeedRepaint> = Default::default();

let frame = epi::Frame::new(epi::backend::FrameData {
info: epi::IntegrationInfo {
name: "egui_web",
Expand All @@ -163,10 +155,19 @@ impl AppRunner {
native_pixels_per_point: Some(native_pixels_per_point()),
},
output: Default::default(),
repaint_signal: needs_repaint.clone(),
});

let needs_repaint: std::sync::Arc<NeedRepaint> = Default::default();

let egui_ctx = egui::Context::default();

{
let needs_repaint = needs_repaint.clone();
egui_ctx.set_request_repaint_callback(move || {
needs_repaint.0.store(true, SeqCst);
});
}

load_memory(&egui_ctx);
if prefer_dark_mode == Some(true) {
egui_ctx.set_visuals(egui::Visuals::dark());
Expand Down
18 changes: 0 additions & 18 deletions epi/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -359,13 +359,6 @@ impl Frame {
self.lock().output.drag_window = true;
}

/// This signals the [`egui`] integration that a repaint is required.
///
/// Call this e.g. when a background process finishes in an async context and/or background thread.
pub fn request_repaint(&self) {
self.lock().repaint_signal.request_repaint();

This comment has been minimized.

Copy link
@DusterTheFirst

DusterTheFirst Mar 17, 2022

Contributor

This change causes problems with calling ctx.request_repaint on another thread that did not exist when calling on frame.

Frame always uses a std::sync::Mutex to lock its state which makes this code fine to call from another thread as it will just block if the mutex is locked. Context on the other hand switches between using an AtomicRefCell and a parking_lot::Mutex depending on if epaint/multi_threaded is enabled (which it normally is not when using eframe. So when ctx.request_repaint is called from another thread and the ui thread has a lock on the Context the application just panics with the following message:

The application panicked (crashed).
Message:  already mutably borrowed
Location: C:\Users\ME\.cargo\registry\src\github.com-1ecc6299db9ec823\atomic_refcell-0.1.8\src\lib.rs:126

The solution to this would be to just have a way to enable or enable by default the multi_threaded feature on epaint. But it would leave this discrepency between different parts of code using std::sync::Mutex, parking_lot::Mutex and AtomicRefCell for depending on features or just what part of the code they are in. Standardizing all of them to use parking_lot's synchronization would be my suggestion, but again the Cargo.toml for epaint does note a minor performance impact when not using AtomicRefCell in a single threaded context:

egui/epaint/Cargo.toml

Lines 52 to 54 in c8f6cae

# Only needed if you plan to use the same fonts from multiple threads.
# It comes with a minor performance impact.
multi_threaded = ["parking_lot"]

This comment has been minimized.

Copy link
@quietvoid

quietvoid Mar 19, 2022

Contributor

I think I'm encountering this problem, an issue probably needs to be created.

}

/// for integrations only: call once per frame
pub fn take_app_output(&self) -> crate::backend::AppOutput {
std::mem::take(&mut self.lock().output)
Expand Down Expand Up @@ -524,24 +517,13 @@ pub const APP_KEY: &str = "app";
pub mod backend {
use super::*;

/// How to signal the [`egui`] integration that a repaint is required.
pub trait RepaintSignal: Send + Sync {
/// This signals the [`egui`] integration that a repaint is required.
///
/// Call this e.g. when a background process finishes in an async context and/or background thread.
fn request_repaint(&self);
}

/// The data required by [`Frame`] each frame.
pub struct FrameData {
/// Information about the integration.
pub info: IntegrationInfo,

/// Where the app can issue commands back to the integration.
pub output: AppOutput,

/// If you need to request a repaint from another thread, clone this and send it to that other thread.
pub repaint_signal: std::sync::Arc<dyn RepaintSignal>,
}

/// Action that can be taken by the user app.
Expand Down

0 comments on commit c768d1d

Please sign in to comment.