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

Add Context::request_repaint_after #1694

Merged
merged 18 commits into from
Jun 22, 2022
Merged

Add Context::request_repaint_after #1694

merged 18 commits into from
Jun 22, 2022

Conversation

coderedart
Copy link
Contributor

Closes #1691.

kinda my first PR.

I am wondering whether i should store this idle timeout interval in platform output our full output. I did not implement this for wgpu yet, but should be pretty similar.

by storing it in platform output, it affects the take function and the Default derive to be manually implemented.

@emilk
Copy link
Owner

emilk commented May 29, 2022

Thanks for starting to work on this!

I think it is worth thinking through what the most common use cases for this feature would be.

If this is a setting ("minimal refresh interval") I think it would make most sense to have it as a member of eframe::App (similar to auto_save_interval, for instance).

However, there could be cases were an egui widget want to schedule a deferred repaint. For instance, a clock widget may want an update every second to show move the second hand. In this case it makes more sense to have it as a member of egui::Context (like is currently the case of this PR). In that case, we also want to make sure that if several widgets calls the function then we do a repaint after the shortest requested time. So if one widget needs updating every two seconds, and one every 500ms, the latter time is used no matter what order the widgets are called. Let's call this something like request_repaint_in with a Duration member. In this case we probably also want request_repaint and request_repaint_in(Duration::ZERO) to mean the exact same thing.

Thoughts?

@coderedart
Copy link
Contributor Author

I thought it would be best to implement this in eframe too, but then i realized other non-eframe implementations might want this feature too and decided to use Context instead.

The primary purpose of this issue was to just have some sort of timeout interval for the next repaint callback.

I had two use cases in mind:

  1. a simple stopwatch or timer app. if it is only showing seconds, then they only need to refresh every second. but request_repaint doesn't allow them to delay the repaint signal. and every time the app repaints, they can set the interval for the next frame by checking the current time and calculating how long before the seconds text displayed in gui needs to change. then, set that as the delay for the next frame. here, you want to make sure that the gui refreshes at this particular instant in future, regardless of whether it had input or not. if there's input, causing an earlier refresh, app will recalculate the timeout and set it again this frame.
  2. a normal application like weather app or something, where you only want the gui to not be completely stale / dead due to idling in the background. here, you don't care about how long till a particular moment in time, but rather just a simple, maximum timeout like 5 seconds or something from the frame where it received the last input event. you can just provide the same duration every frame with no calculation.

I did not think about widgets explicitly wanting to set a timeout. but, I don't think that changes anything here.

my plan was to only just store the shortest duration too. as long as the repaint happens at or before the Instant desired by widget / user, they can recalculate their next duration according to their repaint needs just like in stop watch usecase.

I guess, that settles most questions.
request_repaint_in will take a duration, and will keep track of the smallest duration inside context. will provide that duration to backend in FullOutput along with the needs_repaint bool. winit/glfw/sdl will need to make sure to not idle longer than that duration waiting for events.

the name request_repaint_in is slightly misleading, but it should be fine. duration starts at the next frame. lets say for example that its a very inefficient app and takes 500 milliseconds per frame at 2 fps. the widget / user might want a repaint in next 500 milliseconds. now, app takes 1000 ms per frame (1 fps) because the backend event timeout takes 500 milli seconds AFTER the vsync swap buffer.

so, its not that we are requesting repaint within X duration. we are rather timing out during app idle time without any input events.
just need to mention this in the docs, so that they understand what is happening in the background.

@emilk
Copy link
Owner

emilk commented May 29, 2022

Good points! Perhaps request_repaint_after is a better name to communicate that the time starts ticking after the current frame finishes?

You can also use Duration::MAX to avoid using an Option (will make .min(…) easier to call)

@coderedart
Copy link
Contributor Author

should i just replace needs_repaint bool with the repaint_after duration? then, checking for needs_repaint is simply repaint_after == Duration::ZERO

it will be a breaking change

@emilk
Copy link
Owner

emilk commented May 30, 2022

should i just replace needs_repaint bool with the repaint_after duration? then, checking for needs_repaint is simply repaint_after == Duration::ZERO

it will be a breaking change

Yeah, I think that sounds good!

@coderedart coderedart marked this pull request as ready for review May 30, 2022 15:34
egui/src/context.rs Outdated Show resolved Hide resolved
@coderedart
Copy link
Contributor Author

coderedart commented May 30, 2022

I have no idea how to deal with web, so i left it alone except changing the type signature in the logic function.
todo:

  • add to changelog about breaking change

eframe/src/web/backend.rs Outdated Show resolved Hide resolved
@coderedart
Copy link
Contributor Author

Unfortunately libspeechd is failing to link on my arch. So, I could not check for failures offline with --all-features. Had to commit and wait for ci workflow to run and show me errors to fix.

I hope this works now.

Copy link
Owner

@emilk emilk left a comment

Choose a reason for hiding this comment

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

Excellent job - sorry for the slow review, but almost there now!

CHANGELOG.md Outdated Show resolved Hide resolved
eframe/src/native/run.rs Show resolved Hide resolved
eframe/src/web/events.rs Show resolved Hide resolved
egui-winit/src/lib.rs Show resolved Hide resolved
egui/src/context.rs Show resolved Hide resolved
egui_glium/examples/native_texture.rs Show resolved Hide resolved
egui_glium/examples/pure_glium.rs Show resolved Hide resolved
egui_glow/examples/pure_glow.rs Show resolved Hide resolved
egui_glow/src/winit.rs Show resolved Hide resolved
egui/src/context.rs Outdated Show resolved Hide resolved
@@ -194,7 +202,12 @@ pub fn run_glow(
painter.destroy();
}
winit::event::Event::UserEvent(RequestRepaintEvent) => window.request_redraw(),
_ => (),
winit::event::Event::NewEvents(winit::event::StartCause::ResumeTimeReached {
Copy link
Owner

Choose a reason for hiding this comment

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

You should be able to combine these with winit::event::Event::UserEvent(RequestRepaintEvent | winit::event::Event::NewEvents…

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have a feeling that we might see some sort of issue with repaint_after feature once people start using it.

so, i thought it would be better for debugging or adding special case handling for ResumeTimeReached event if both are treated as separate events.

@coderedart
Copy link
Contributor Author

apologies for the delay. I had sudden irl stuff to take care of until yesterday and only now got to finally open my PC to work on this.

@coderedart
Copy link
Contributor Author

think all CI should pass now. its been so long since i cared about grammar, that i forgot you capitalize first letter in a sentence :)

@emilk emilk changed the title start working on idle timeout repaint interval Add Context::request_repaint_after Jun 22, 2022
@emilk emilk merged commit 935913b into emilk:master Jun 22, 2022
@emilk
Copy link
Owner

emilk commented Jun 22, 2022

Great job!

emilk added a commit that referenced this pull request Jun 22, 2022
emilk added a commit that referenced this pull request Jun 22, 2022
* Implement repaint_after for eframe web

Follow-up to #1694

* cargo fmt

* Simplify demo UI for "repaint_after"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

provide manual timeout to egui in reactive mode to force refresh even if there's no new input
2 participants