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

Fix continuous RunMode in glium backend #21

Merged
merged 1 commit into from
Sep 14, 2020

Conversation

parasyte
Copy link
Contributor

The primary change here is that the winit control flow is set according to RunMode, and not assumed to be Wait. I also had to change the event handler to RedrawEventsCleared.

Prior to this patch, the Continuous run mode didn't work correctly in the glium backend. But it does work as expected in the web backend.

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.

Thanks for the PR!

Prior to this patch, the Continuous run mode didn't work correctly in the glium backend

Can you elaborate what was incorrect? For me master works as it should, i.e. repainting at 60 FPS on my 60 Hz display..

I ask, because trying this PR the Reactive mode is now definitely broken on my Mac: it repaints the GUI even when the window is in the background (the point of the Reactive mode is to avoid unnecessary repaints).

@parasyte
Copy link
Contributor Author

parasyte commented Sep 13, 2020

On Windows 10, both modes act completely identically. No updates occur unless an event is delivered (especially moving the mouse cursor). I believe this is caused by a bug in winit. See rust-windowing/winit#987 and rust-windowing/winit#1619

Continuous mode does work correctly on macOS. And this PR also breaks Reactive mode on macOS. So I'll have to go back to the drawing board on this.

@parasyte
Copy link
Contributor Author

parasyte commented Sep 13, 2020

@emilk I force-pushed a new patch for this. Tested on Windows and macOS. I haven't tested on Linux (X11 or Wayland).

Neither event handler RedrawRequested or RedrawEventsCleared worked on both platforms, so I decided to use the latter for Windows and the former (original event) for all other platforms. I wasn't able to come up with a good pattern for platform-specific event handling, so I moved all of the code into a closure. It hurts diff readability, but I don't know of another way to solve that. (The "Hide whitespace changes" feature in github helps a bit.)

I also had to add another request_redraw() call for Continuous mode to work on macOS.

@parasyte
Copy link
Contributor Author

And rebased on master.

@parasyte parasyte requested a review from emilk September 13, 2020 22:43
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.

Great, nice work!

@emilk emilk merged commit 0ea80ae into emilk:master Sep 14, 2020
@parasyte parasyte deleted the fix/glium-continuous-runmode branch September 14, 2020 20:05
@bel8z
Copy link

bel8z commented Sep 29, 2021

A bit late here, but this fix doesn't work as expected for me.

RedrawEventsCleared is raised at the vsync frequency, causing the Reactive mode to behave as Continuous. Removing the platform-dependent handling and responding to RedrawRequested only does the work on my machine. Is this fix still needed?

Using master on Windows 10 btw.

@parasyte
Copy link
Contributor Author

There are two winit issues linked in a comment above. Until those are addressed, I think egui_glium needs to handle this edge case specifically.

I also see that Reactive mode is behaving the same as Continuous mode on Windows 10. This is opposite of what the PR fixed. It's possible that the change to event_loop.run_return() is the cause. But that's a guess just from looking at the code. This should be reported and tracked in a new ticket, IMHO.

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.

3 participants