implement timers in winit #792
Conversation
richard-uk1
force-pushed
the
progress_bar_animation
branch
from
969392c to
a8f3781
Compare
There was a problem hiding this comment.
Sorry it took me forever to review. You'll probably need to either rewrite this from the ground up or deal with some thorny merge conflicts, given the refactors we've done since.
Overall, I like this PR. The design is pretty elegant and avoids relying on Winit too much. One thing I would like before we can consider merging this is better support deterministic testing in TestHarness.
There's a bunch of issues that need to be fixed that I assume are due to this being a draft: lack of documentation, examples/animation.rs is copy-pasted from the hello example, I think Instant doesn't work with webassembly, etc.
| /// An ordered list of timers set by masonry | ||
| /// | ||
| /// Implemented as a min priority queue | ||
| pub struct TimerQueue { | ||
| queue: BinaryHeap<Timer>, | ||
| } |
There was a problem hiding this comment.
Given that this type just straight up wraps BinaryHeap methods, I think you could remove it and use BinaryHeap in the places that import TimerQueue.
There was a problem hiding this comment.
I did it with the binary heap directly originally but switched to a wrapping struct because I needed a min-first queue and that meant a more complex inner type (something like BinaryHeap<Reverse<Value>> but since I now have a custom Timer type I think I can get rid of the struct wrapper as you say.
| /// Note - if calling this in a lifecycle method, you must also request an anim frame. | ||
| pub fn animate(mut self, animate: bool) -> Self { |
There was a problem hiding this comment.
I think this comment is above the wrong function.
| tracing::debug!("progress_bar: on_anim_frame"); | ||
| // pixel per 1ms | ||
| const NS_PER_PIXEL: f64 = 1_000_000.; | ||
| const DURATION_BETWEEN_ANIMATIONS: Duration = Duration::from_millis(2_500); |
There was a problem hiding this comment.
It's not super clear to me why you want the progress bar to have an animation every 2.5 seconds?
There was a problem hiding this comment.
It's arbitrary - I'm copying the way progress bars work on windows (with periodic movement to indicate progress). My motivation for having timers is triggering events at specific intervals in a simple way without needing to feed in messages from external tasks etc. Happy to go with whatever setup we wanted for animation on the progress bar. In any case it should be optional, with a with_animation on at least the xilem view. If I didn't do this already I should before merging.
| @@ -110,6 +111,7 @@ pub struct TestHarness { | |||
| has_ime_session: bool, | |||
| ime_rect: (LogicalPosition<f64>, LogicalSize<f64>), | |||
| title: String, | |||
| timers: TimerQueue, | |||
There was a problem hiding this comment.
I would expect the TestHarness to also get a method to move timers forward.
It would probably get merged with animate_ms.
There was a problem hiding this comment.
I want to solve testing but didn't have an idea how to do it. Happy to go with your approach.
Co-authored-by: Olivier FAURE <[email protected]>
This PR implements timers using a priority queue and using winit's 'wait for' functionality.
It doesn't yet work. I'm opening a draft PR to see if anyone can see why.