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

Use Window.requestIdleCallback() #2880

Merged
merged 1 commit into from
Jun 13, 2023

Conversation

daxpedda
Copy link
Member

This PR replaces the use of Window.requestAnimationFrame with Window.requestIdleCallback, which is in line with what ControlFlow::Poll is supposed to be:

When the current loop iteration finishes, immediately begin a new iteration regardless of whether or not new events are available to process.

This is going to make ControlFlow::Poll very fast, which is much closer to how other backends work, but it is also gonna consume way more power, which may or may not be desired considering this is Web and running in the browser.

I think a lot of people are using RedrawRequested as a VSync source, which it is not as far as I understand. See #2412 for that.

Reverts #1519.
Related #1305.

Copy link
Member

@notgull notgull left a comment

Choose a reason for hiding this comment

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

This kind of thing might want to be configurable. As in, in the event loop constructor for web, add an option for whether or not to use this or the animation frame.

src/platform_impl/web/web_sys/timeout.rs Show resolved Hide resolved
@daxpedda daxpedda mentioned this pull request Jun 13, 2023
25 tasks
@daxpedda
Copy link
Member Author

daxpedda commented Jun 13, 2023

This kind of thing might want to be configurable. As in, in the event loop constructor for web, add an option for whether or not to use this or the animation frame.

I would rather not, that would push users towards using RedrawRequest as a VSync source, which not only is wrong, but it doesn't actually work, because any registered event will wake up the event loop anyway.

@kchibisov
Copy link
Member

I would rather not, that would push users towards using RedrawRequest as a VSync source, which not only is wrong, but it doesn't actually work, because any registered event will wake up the event loop anyway.

Yeah, the vsync source is requestAnimationFrame, which doesn't map to any present event we have in winit yet, but the frame hint mentioned on #2412 should be it.

@daxpedda daxpedda merged commit 7ce86c3 into rust-windowing:master Jun 13, 2023
@cybersoulK
Copy link

cybersoulK commented Jun 17, 2023

@daxpedda

The current master i have error in console (Chrome + firefox on mac):

panicked at 'Error in Surface::configure: Validation Error

Caused by:
    Both `Surface` width and height must be non-zero. Wait to recreate the `Surface` until the window has non-zero area.

Not sure what the error means.

My canvas is spawned by winit itself, and not manually.

might be because of setting up wgpu surface when calling:
self.surface.configure(&self.device, &self.config); either at initialization, or in a resize event

i am not sure how it relates to this PR, but #2878 is working for me (rev = ed14254)

@daxpedda
Copy link
Member Author

You always have to check if the window size isn't zero yourself, Winit doesn't offer any guarantees in this regard, this is an issue with other backends as well.

We are tracking solutions for this here: #2863. But it will require some further design work.

@GloopShlugger
Copy link

I think this change causes the loop to pause due to inactivity (or spuriously) on chrome based browsers, or at least the game loop keeps stopping until there's input / or randomly when embedded in itch.io for example

@kchibisov
Copy link
Member

I think that's expected, you should use request_redraw if you want to wake up. This will happen as well other platforms.

However which control flow mode are you using? Is it Poll? @daxpedda can we have some sort of busy loop with Poll?

@GloopShlugger
Copy link

i am using poll and i tried requesting redraws but that did not change anything -- it is for a typical gameloop

@kchibisov
Copy link
Member

kchibisov commented Aug 1, 2023

Well, you'd have to wait for #2896 merge, so the requestAnimationFrame will be used under the hood for Window::request_redraw.

@GloopShlugger
Copy link

GloopShlugger commented Aug 1, 2023

i see thank you, for now setting controlflow to wait for a zero duration seems to do it (though ignoring vsync on web) until the fix lands

@daxpedda
Copy link
Member Author

daxpedda commented Aug 4, 2023

I think this change causes the loop to pause due to inactivity (or spuriously) on chrome based browsers, or at least the game loop keeps stopping until there's input / or randomly when embedded in itch.io for example

Window.requestIdleCallback() should pretty much be as fast as possible.
FYI there isn't really an alternative, we were using Window.requestAnimationFrame() beforehand which is definitely slower and you can't just have a busy loop on the main thread as that would just block the whole page.

@GloopShlugger how long are these pauses? Do you have to do anything to induce them? I can't seem to reproduce this on Chrome nor Firefox.

@GloopShlugger
Copy link

GloopShlugger commented Aug 11, 2023

i have found it to only happen on chrome-based browsers embedded inside an iframe like itch.io, i have also seen it happen with just the dev console open. The way i detected it is by printing every frame and after a while (which has not been a consistent amount) it just stops updating, with or without input. It also on startup seems to run the eventloop once, and then wait for input events like mouse clicks or just moving the mouse seems to trigger a rerun, at which point it runs the loop again for a bit until it just stops.

I feel like this is 10000% a browser implementation choice like some sort of powersaving measure chrome does, i may be able to provide an itch.io build if required

@daxpedda
Copy link
Member Author

That is unfortunate indeed. I always knew Window.requestIdleCallback() isn't ideal, but it's turning out to be more of a problem then I thought it would be.

The proper solution is to use the Prioritized Task Scheduling API, unfortunately this isn't available in Firefox/Safari yet.

I had another idea that might fix this issue:
The problem with setTimeout() is that if you call it from another nested setTimeout() callback, the there will be a minimum delay of 4ms. We could get around this by queuing the next setTimeout() not from inside the callback, but from a queueMicroTask().

I will make a PR and ping you.

@GloopShlugger
Copy link

interesting i'll definitelly try it, i did end up changing the wait to be reasonable like 5ms in the loop. I do think it would likely end up being fine once / if the update and draw split happens. The other easy short term solution is just forking winit and changing it back to requestanimationframe which isn't that big of a change

@daxpedda
Copy link
Member Author

In #2896 we changed Window::request_redraw() to use Window.requestAnimationFrame(), so if you end up forking it you probably want to change it to setTimeout() instead.
But you could just use ControlFlow::WaitUntil(Instant::now()), which would end up being the same.

@GloopShlugger
Copy link

oh my god it landed and i didn't even notice i'll try that first and see if it changes anything

@GloopShlugger
Copy link

GloopShlugger commented Aug 11, 2023

I tried out the newest master and with controlflow::poll and requesting redraws both in AboutToWait and RedrawRequested still get stuck on chrome, but if you use controlflow::wait and RedrawRequested it does vsync correctly now

@daxpedda
Copy link
Member Author

daxpedda commented Aug 24, 2023

I tried out the newest master and with controlflow::poll and requesting redraws both in AboutToWait and RedrawRequested still get stuck on chrome

@GloopShlugger this doesn't sound right, Window::request_redraw() uses Window.requestAnimationFrame(), which shouldn't be throttled. Can you recheck and verify that Window::request_redraw() should never throttle? Because if it does it might be a bug in Winit.

Working on a PR to move away from Window.requestIdleCallback() now.

@GloopShlugger
Copy link

GloopShlugger commented Aug 24, 2023

@daxpedda it does get stuck immediately and the first frame does not get drawn on chrome until you press something. request_redraw too gets stopped after a short amount of time. (easiest to trigger with developer console opened)
this only happens with set_poll, and the issue vanishes completely if you use a wait.

it will be interesting to see if moving away from RequestIdleCallback changes things. I'm very unfamiliar with the winit codebase so i sadly can't help much, but i'd really like to thank you for looking into it so much

@daxpedda
Copy link
Member Author

daxpedda commented Aug 24, 2023

it does get stuck immediately and the first frame does not get drawn on chrome until you press something. request_redraw too gets stopped after a short amount of time.

Are you sure that Window::request_redraw() is actually called? It might be that the event you call it from is being throttled.
Basically if you call Window.request_redraw() you should always get the RequestRedraw event, unless Window.requestAnimationFrame() is being throttled which I don't believe, so there would be some deeper Winit bug in that case.

@GloopShlugger
Copy link

GloopShlugger commented Aug 24, 2023

I'm calling it from inside the RedrawRequested event, it works perfectly fine in all other cases, there was no difference between AboutToWait and that, or both, the event loop just seemingly stops

@daxpedda
Copy link
Member Author

Would you mind running a simple JS script in the background to figure out whether this is really a Winit bug or not?

const fun = () => {
    console.log('rAF callback')
    requestAnimationFrame(fun)
}
requestAnimationFrame(fun)

@GloopShlugger
Copy link

when this runs in the background the issue is fixed :,)

@daxpedda
Copy link
Member Author

daxpedda commented Aug 24, 2023

???
I guess this is a bug in Chrome then?
Or maybe it's throttling algorithm is just very weird.

EDIT: or maybe it's really a bug in Winit, will continue to investigate.

@GloopShlugger
Copy link

i will try the PR the moment you ping me and i see it, I hope there's some solution that works with winits model, thank you again for looking into it -- dancing in the pits of web api hell

@daxpedda
Copy link
Member Author

I spent a lot of time going through the implementation, but I think there is just something weird going with Chrome, I can't find anything wrong with Winit.

If you ever figure out a minimal reproducable example please hit me up, it's hard to debug this without being able to try it.

In any case, see #3044.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

5 participants