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

Duration passed to request_repaint_after is ignored if called each frame #3109

Closed
Sedeniono opened this issue Jun 25, 2023 · 20 comments · Fixed by #3978 · May be fixed by #3275
Closed

Duration passed to request_repaint_after is ignored if called each frame #3109

Sedeniono opened this issue Jun 25, 2023 · 20 comments · Fixed by #3978 · May be fixed by #3275
Labels
bug Something is broken

Comments

@Sedeniono
Copy link

Describe the bug
Using egui 0.22.0, calling request_repaint_after() causes the application to redraw the UI with a high refresh rate, regardless of the duration passed into request_repaint_after(). It behaves (mostly) as if calling request_repaint().
The bug was not there in version 0.21.0.

Steps to reproduce the behavior:
The following code was adapted from the hello world example. I replaced the controls with a fps (frames per second) and counter label. Rust file:

use eframe::egui;
use std::time::{Duration, Instant};

fn main() -> Result<(), eframe::Error> {
    let options = eframe::NativeOptions {
        initial_window_size: Some(egui::vec2(320.0, 240.0)),
        ..Default::default()
    };
    eframe::run_native(
        "My egui App",
        options,
        Box::new(|_cc| Box::<MyApp>::default()),
    )
}

struct MyApp {
    previous_instant: Instant,
    previous_count: u64,
}

impl Default for MyApp {
    fn default() -> Self {
        Self {
            previous_instant: Instant::now(),
            previous_count: 0,
        }
    }
}

impl eframe::App for MyApp {
    fn update(&mut self, ctx: &egui::Context, _frame: &mut eframe::Frame) {
        egui::CentralPanel::default().show(ctx, |ui| {
            ui.heading("My egui Application");
            let fps = 1.0 / self.previous_instant.elapsed().as_secs_f64();
            self.previous_instant = Instant::now();
            self.previous_count += 1;
            ui.label(format!("fps: {:.2}, counter: {}", fps, self.previous_count));
        });
        ctx.request_repaint_after(Duration::from_secs(1));
    }
}

toml:

[package]
name = "egui_request_repaint_bug"
version = "0.1.0"
edition = "2021"

[dependencies]
egui = "0.22.0"
eframe = { version = "0.22.0", default-features = false, features = [
    "default_fonts", # Embed the default egui fonts.
    "glow",          # Use the glow rendering backend. Alternative: "wgpu".
] }

[patch.crates-io]
egui = { git = "https://github.com/emilk/egui", branch = "master" }
eframe = { git = "https://github.com/emilk/egui", branch = "master" }

Build and run on Windows 10.

When the application has focus, the "fps" labels shows approximately a value of 144 on my monitor (which has a refresh rate of 144 Hz), regardless of where the mouse is. When the application does not have focus, the "fps" label continues updating with a high frequency and shows approximately 90. (I have no idea where the 90 is coming from.)

If I remove the call to request_repaint_after(), the application no longer updates with a high rate.
Keeping the request_repaint_after() call and going back to egui 0.21.0, I get the expected behavior. Considering the release notes, I guess this is related to #2939.

Expected behavior
When the application does NOT have focus, the "fps" label should show a value of approximately 1. In other words, the GUI should update only once a second because I passed 1s as duration to request_repaint_after().
When the application does have focus, the "fps" label should show a value of 144 only while moving around the mouse.

Desktop:

  • OS: Windows 10
  • Browser: Not applicable; running on native
  • Version of egui: 0.22.0 (version 0.21.0 is not affected)
@Sedeniono Sedeniono added the bug Something is broken label Jun 25, 2023
@chrisheib
Copy link

have the same problem in my app https://github.com/chrisheib/STStat

I'll downgrade for the time being, can't let my users waste their ressources like this.

@chrisheib
Copy link

First time I got to run git bisect in the wild :D

The bad commit is 834e2e9.

Which makes sense since it touched the repaint logic.

@emilk any immediate thoughts on this?

@emilk
Copy link
Owner

emilk commented Jun 30, 2023

If you run with RUST_LOG=eframe=trace you should get some log output that explains why eframe is repainting; maybe that can help you find the bug. I would appreciate help here since I'm way too busy right now to take a proper look at it.

Most of the logic can be found in crates/eframe/src/native/run.rs (in the run_and_return function) and in struct Repaint in crates/egui/src/context.rs

@emilk emilk added this to the 0.23.0 milestone Aug 10, 2023
@emilk
Copy link
Owner

emilk commented Aug 11, 2023

I tried your code on my Mac on latest master, and I fail to reproduce.

The results is as expected: High FPS when moving the mouse over the window, and 1 fps when the mouse is still or outside the egui window. The behavior is the same if the windows is in focus or in the background.

Can you please try again on latest master? Perhaps this is only a Windows bug. If so, perhaps it will be resolved when we update to winit 0.29

@OmegaJak
Copy link
Contributor

I'm seeing this bug on Windows as well with latest master. I'll take a look and see if I can figure anything out.

@OmegaJak
Copy link
Contributor

OmegaJak commented Aug 23, 2023

Alright - I understand why this is happening in the current code and have a possible (though dubious) fix in mind. I didn't run the code before the commit that introduced this (834e2e9) to figure out why it wasn't happening previously.

The workaround on this line is at least partially to blame.

When there's a RedrawEventsCleared event on Windows, we redraw. This event is triggered when request_repaint_after is called, because that leads to a winit RepaintAt or Wait event being triggered, which is immediately followed by RedrawEventsCleared. This then triggers another run of the main ui. So if the main ui is calling request_repaint_after, then the cycle continues and we continuously repaint. This only happens on Windows because that workaround is only applied for Windows - on other platforms, it uses the RedrawRequested event to trigger a repaint, which isn't triggered by Wait or RepaintAt immediately.

This bug only happens when the request_repaint_after is called inside the main ui paint loop. The button inside the demo app that calls it from another thread works fine, even on Windows.

There's another bug confusing things here - the frame_nr inside UserEvent::RequestRepaint is always 1 less than the frame number when checked inside the winit event loop if request_repaint_after was called during the main ui update. This prevents us from properly scheduling repaints much of the time because of this line.

I have two very dubious fixes and some unpolished tweaks to the demo app for testing this in #3275. I removed the workaround and allowed off-by-one for the frame number. Neither seems like a real fix. With these changes, calling request_repaint_after every time inside the main ui loop works as expected, and so does calling it from another thread - but a one-off call of request_repaint_after inside the ui loop does not trigger a repaint later.

I'm looking for suggestions of what the proper fix(es) might be - I don't think what I've done in my PR are adequate or correct.

@emilk
Copy link
Owner

emilk commented Aug 24, 2023

Thanks for the investigation!

I’m hoping we can remove the windows-hack when the new winit version drops, which should be any week now 🤞 The new winit has a substantially new event-loop

@emilk emilk removed this from the 0.23.0 milestone Sep 20, 2023
@sdasda7777
Copy link

sdasda7777 commented Oct 15, 2023

Hi, I stumbled upon this on accident, when I noticed adding ctx.request_repaint_after(Duration::new(1,0)); to the code increased the CPU usage drastically compared to when not using it, and that the same behaviour occurs even with ctx.request_repaint_after(Duration::new(1000000,0)); (1000000 seconds = 11.5 day refresh interval, which obviously should not cause any refreshes). Are there any good workarounds?

@trsh
Copy link

trsh commented Oct 26, 2023

I am also dealing with this problem. @OmegaJak , @emilk any temporal workarounds?

@OmegaJak
Copy link
Contributor

I am also dealing with this problem. @OmegaJak , @emilk any temporal workarounds?

Here's my clunky temporary solution - just spin up another thread that sleeps and occasionally calls request_repaint:
https://github.com/OmegaJak/gwaihir/blob/f132180306bfeca47321f14d5d881d8742d920d5/crates/gwaihir-client/src/periodic_repaint_thread.rs

@dimtpap
Copy link
Contributor

dimtpap commented Dec 13, 2023

This is also happening on Linux on Wayland after I upgraded eframe from 0.23.0 to 0.24.1

@bglw
Copy link

bglw commented Dec 18, 2023

Likewise here, macOS. @OmegaJak's fix is a great tip, though 👍

@bglw
Copy link

bglw commented Dec 18, 2023

Actually for those also running on the web, you'll need a non-std::thread implementation.

Here's an analog to the above using weird wasm/async stuff (I already had these specific crates hanging around, there might be better options here 🤷):

Loop (using gloo-timers):

// TODO: Once https://github.com/emilk/egui/issues/3109 is resolved, use request_repaint_after in main loop instead
#[cfg(target_arch = "wasm32")]
pub async fn create_periodic_repaint_async(
    egui_ctx: egui::Context,
    millis_between_repaints: u32,
) {
    loop {
        egui_ctx.request_repaint();
        gloo_timers::future::TimeoutFuture::new(millis_between_repaints).await;
    }
}

Setup inside the creation context (using wasm-bindgen-futures):

// TODO: Once https://github.com/emilk/egui/issues/3109 is resolved, use request_repaint_after in main loop instead
#[cfg(target_arch = "wasm32")]
wasm_bindgen_futures::spawn_local(create_periodic_repaint_async(
    cc.egui_ctx.clone(),
));

@emilk
Copy link
Owner

emilk commented Dec 19, 2023

Is this still a problem in egui/eframe 0.18 0.24? The repaint logic has been significantly rewritten

(EDIT: version snafu)

@sdasda7777
Copy link

@emilk Could you please elaborate what you mean by egui/eframe 0.18? I believe I am still able to replicate the issue on egui/eframe 0.24.1.

@bglw
Copy link

bglw commented Dec 19, 2023

Yes I only encountered this issue this week after updating from .21.x to .24.x

@emilk
Copy link
Owner

emilk commented Dec 20, 2023

I don't know how I miss-typed "0.24" as "0.18", but I did 🤦

@jayzhudev
Copy link
Contributor

jayzhudev commented Dec 28, 2023

I don't know how I miss-typed "0.24" as "0.18", but I did 🤦

Adding some more data points. I didn't encounter this issue until after upgrading to 0.24.1 from 0.23 (macOS). I don't have a thorough understanding of the implementation yet but it seems like this is where the request_repaint duration gets ignored:

https://github.com/emilk/egui/blob/master/crates/egui/src/context.rs#L82

If I add a dummy callback to the egui context then the request_repaintduration has effect again is also ignored but the reactive behavior is back (instead of running full FPS always):

cc.egui_ctx.set_request_repaint_callback(|_| {});

@dimtpap
Copy link
Contributor

dimtpap commented Dec 28, 2023

I don't have a thorough understanding of the implementation yet but it seems like this is where the request_repaint duration gets ignored:

https://github.com/emilk/egui/blob/master/crates/egui/src/context.rs#L82

Both glow and wgpu integrations call set_request_repaint_callback on init

If I add a dummy callback to the egui context then the request_repaintduration has effect again is also ignored but the reactive behavior is back

I think that's just because you're overriding the integration's callback

@jayzhudev
Copy link
Contributor

I don't have a thorough understanding of the implementation yet but it seems like this is where the request_repaint duration gets ignored:
https://github.com/emilk/egui/blob/master/crates/egui/src/context.rs#L82

Both glow and wgpu integrations call set_request_repaint_callback on init

If I add a dummy callback to the egui context then the request_repaintduration has effect again is also ignored but the reactive behavior is back

I think that's just because you're overriding the integration's callback

That's right, I read a bit more after my previous comment. Based on the limited time I got to wonder around. It seems like it's somewhere around

https://github.com/emilk/egui/blob/master/crates/egui/src/context.rs#L80 and
https://github.com/emilk/egui/blob/master/crates/egui/src/context.rs#L261

that's causing this issue. I don't know what's behind https://github.com/emilk/egui/blob/master/crates/egui/src/context.rs#L80 so I'm not sure what's the correct and simplest fix that won't change other expected behaviors. But I hope this observation helps.

@emilk emilk added this to the Next Major Release milestone Dec 29, 2023
@emilk emilk changed the title Duration passed to request_repaint_after is ignored Duration passed to request_repaint_after is ignored on Windows Jan 4, 2024
dimtpap added a commit to dimtpap/coppwr that referenced this issue Jan 16, 2024
On a new branch because of emilk/egui#3109
@emilk emilk changed the title Duration passed to request_repaint_after is ignored on Windows Duration passed to request_repaint_after is ignored if called each frame Feb 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is broken
Projects
None yet
9 participants