PistonDevelopers / conrod

An easy-to-use, 2D GUI library written entirely in Rust.
Other
3.35k stars 296 forks source link

The threaded glium+winit example seems to be broken #1051

Open pedrocr opened 7 years ago

pedrocr commented 7 years ago

I've been adapting the glium+winit threaded example to chimper and input/refresh handling seems to have some brokenness to it. First the needs_update logic seems wrong:

https://github.com/PistonDevelopers/conrod/blob/347fb99515d51a6182bb112b664a336cebf1a816/examples/all_winit_glium_threaded.rs#L101

This should probably be if events.is_empty() && !needs_update to not block on events when we've decided a new update is needed. And then there seems to be a race condition between the conrod and main threads. Here's the sequence of events.

MAIN: Received an event and passed it on to conrod CONROD: Processed the event and issued a new draw and an event loop wakeup() MAIN: works on the event handling some more MAIN: blocks on event loop because the wakeup didn't come while it was blocked

Replacing event_loop.run_forever() with event_loop.poll_events() in the main thread works fine. I've tried moving the event loop into it's own thread that all it does is process events while the blocking in the main thread happens only on the render channel but that has it's own set of issues (conrod::backend::winit::convert_event() blocks, seems slow).

Any ideas on how to make this more robust?

mitchmindtree commented 7 years ago

Hmm the example seems to be working fine for me. Do you get this strange behaviour when running the all_winit_glium_threaded example itself? Or do you only get it in your own code? Could you be more specific in terms off what bugs you are seeing?

Also I'm not quite sure where the race condition is that you are suggesting? The conrod thread only calls wakeup on the EventsLoopProxy after it send the primitives for drawing, so there should always be an Awakened event waiting following each submission of graphics primitives.

Replacing event_loop.run_forever() with event_loop.poll_events() in the main thread works fine.

Yes, but this also means that the main thread will continually loop at ~60fps and will still consume energy when the app is idle (this is probably fine for realtime games, but probably not OK for simple GUI apps).

I've tried moving the event loop into it's own thread

Note that this won't work on macOS as for "historic reasons" the event loop can only be polled on the main thread, so it's best to leave the main event loop polling on the main thread if you're hoping to make your app multi-platform (super frustrating, I know!).

pedrocr commented 7 years ago

Do you get this strange behaviour when running the all_winit_glium_threaded example itself? Or do you only get it in your own code? Could you be more specific in terms off what bugs you are seeing?

I get it in my code because it does more stuff in the main loop. But I believe the issue is there in the example as well. What happens for me is that I run this in my main loop:

https://github.com/pedrocr/chimper/blob/a088385116cff5eb857cea0669b30d6752edfaa8/src/window.rs#L188-L189

to swap images in the map and then issue a redraw. The problem is that it takes long enough that the wakeup is lost.

The conrod thread only calls wakeup on the EventsLoopProxy after it send the primitives for drawing, so there should always be an Awakened event waiting following each submission of graphics primitives.

The problem is that Awakened is not an event. If you issue 100 wakeup() calls in quick succession you won't get 100 Awakened events because when the wakeup() is called when the event loop is not blocked it does nothing.

Yes, but this also means that the main thread will continually loop at ~60fps and will still consume energy when the app is idle (this is probably fine for realtime games, but probably not OK for simple GUI apps).

Yep, which is what I'm very actively trying to avoid.

for "historic reasons" the event loop can only be polled on the main thread

That plus the other issues makes me abandon this option as well.

mitchmindtree commented 7 years ago

The problem is that it takes long enough that the wakeup is lost

The problem is that Awakened is not an event

Ahh I see, my understanding was that there should always be at least one Awakened event emitted if an EventLoopProxy called wakeup whether or not the loop is currently blocking or not. I believe this is the case for the macos backend, I thought it was also the case for x11 and wayland but perhaps I'm wrong. I think I'll open an issue at winit to clarify this behaviour.

For now, I guess one way to avoid this would be to only enter run_forever if there are no primitives already waiting to be rendered. E.g. something like

pending_primitives.extend(render_rx.try_iter().last());
if pending_primitives.is_empty() {
    events_loop.run_forever(|event| {
        ...
    });
}
if let Some(primitives) = pending_primitives.drain(..).chain(render_rx.try_iter()).last() {
    draw(...)
}
pedrocr commented 7 years ago

Ahh I see, my understanding was that there should always be at least one Awakened event emitted if an EventLoopProxy called wakeup whether or not the loop is currently blocking or not.

In my testing it doesn't seem to be the case but I may be looking at things wrong.

For now, I guess one way to avoid this would be to only enter run_forever if there are no primitives already waiting to be rendered.

I considered something like that but in reality it's still a race, just less of a window to hit the issue. Between is_empty() and run_forever() you may have received a wakeup() that is ignored and you're still blocked. It's just much harder to trigger as right now you have a 16ms+ window to do it in.

mitchmindtree commented 7 years ago

In my testing it doesn't seem to be the case but I may be looking at things wrong.

What platform are you on? I might be able to look at the winit src to check for your backend as I'm familiar with a fair bit of it.

In the meantime perhaps you could setup a minimal test case for this behaviour by spawning a secondary thread with the proxy and blocking on main until a message is received saying that the wakeup has been called before entering run_forever and seeing if Awakened is still received.

It's a bit tricky for me to help much further as I still don't know exactly what the strange behaviour that you're noticing is.

pedrocr commented 7 years ago

I'm on X11 (Ubuntu 16.04). If you want to replicate this yourself you can compile this chimper version:

https://github.com/pedrocr/chimper/tree/20ee2f70e31fba1fdd861aa44d7de17b70a4dff6

and then run it in a directory that has images in it. Then move the mouse to the file list and select an image and stop moving the mouse (or generating any other event). You can then wait there indefinitely without the image showing up. Once you move the mouse (or generate another event) the window refreshes and you have the image.

Your test for Awakened also seems interesting.

pedrocr commented 7 years ago

Here's an attempt at a minimal test case:

https://gist.github.com/pedrocr/027af24858d6bc984b2c8f01ad4ee3ab

I may be missing something but I believe this should loop forever getting Awakened events. On my machine this wakes twice and then blocks forever.

Edit: and calling the wakeup() from another thread makes no difference.

https://gist.github.com/pedrocr/63d719fac523ffe5d4d1c085312e9f6d

pedrocr commented 7 years ago

I've added a simplified example to chimper:

https://github.com/pedrocr/chimper/blob/3c8d39e536703ae6b8579f8379721df163efcdfd/src/bin/test_awakened.rs

Here's what I get (on Ubuntu 16.04.3):

$ cargo build --release
# (...) all the compiling goes on
$ ./target/release/test_awakened 
Awakening num 1
Draw num 1

At this point the program hangs forever when the expected behavior was for it to keep looping forever.