Fixes to the Glium example - #117
Conversation
MarijnS95
left a comment
MarijnS95
left a comment
There was a problem hiding this comment.
Really happy to see the new glutin and winit being used via a glium update! There are a few design choices we might want to address though, and I don't think the CI will succeed on this yet.
| print!( | ||
| "\rms: {}\t (buffer) + {}\t (UI)", | ||
| println!( | ||
| "ms: {}\t (buffer) + {}\t (UI)", |
There was a problem hiding this comment.
I think \r without ln was here to continuously overwrite the same line.
There was a problem hiding this comment.
Oops, I forgot about this. I found it a lot more useful for debugging to print every frame on its own line, though.
| event_loop | ||
| .run(move |event, elwt| { | ||
| fn user_event(&mut self, event_loop: &ActiveEventLoop, event: UserEvent) { | ||
| (self.user_event)(event) |
There was a problem hiding this comment.
Is there a reason to not inline the original run code directly here?
There was a problem hiding this comment.
The closure captures the environment. The struct would have to turn every captured variable into a field, so I just took the easiest path.
Should I turn it into a struct?
| let _ = event_loop_proxy.send_event(UserEvent::WakeUp); | ||
| tx.send(data).unwrap(); |
There was a problem hiding this comment.
Could we send data though the proxy instead of needing a separate channel?
There was a problem hiding this comment.
Kinda, but I think that would make control flow even more complicated. Not every buffer will result in a rendered frame, some buffers will be dropped. Waking up on every buffer from a stream where we can't skip events means that every buffer will have to render, even if there's another one enqueued.
(I'll add the frame skipping next.)
|
|
||
| event_loop | ||
| .run(move |event, elwt| { | ||
| fn user_event(&mut self, event_loop: &ActiveEventLoop, event: UserEvent) { |
There was a problem hiding this comment.
Note that if you know a value can only have one variant, you can pattern-match on it in the function signature:
| fn user_event(&mut self, event_loop: &ActiveEventLoop, event: UserEvent) { | |
| fn user_event(&mut self, event_loop: &ActiveEventLoop, UserEvent::WakeUp: UserEvent) { |
This even works to unpack a tuple or struct field if you pass data as part of UserEvent. Then, since there's only one event, you can also change enum to struct.
There was a problem hiding this comment.
I think using the signature as the trait defines it is more readable.
About keeping this as an enum, I think most real-world applications are going to have some form of an enum, so this should make it easier to integrate.
I'm willing to change both if it's required for merging, though.
| event_loop.run_app(&mut LoopHandler { | ||
| user_event: move |_event| { | ||
| let t0 = Instant::now(); | ||
| let data = rx.recv().unwrap(); |
There was a problem hiding this comment.
Glad to see that this blocking code now only runs on fn user_event(), rather than on every event that used to be delivered in the past!
Fwiw I'd argue that this won't fly on tiling window managers, where the size of the window cannot be dictated by the input source (unless the window is made floating). Here the easiest to expect is scaling/stretching? |
If a window manager wants to use a different size, then setting the size hint isn't going to stop it. Meanwhile, window managers respecting the size will have the right size. The right solution for wrong size would be letterboxing, but I'm not ready to tackle that in this moment. |
|
I added frame skipping here. Tested by adding an artificial delay in the rendering code and observing that frame latencies don't keep growing forever. |
2 participants
The example had several problems: freezing, wrong window size, and old glium version.
This fixes the problems.