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

Change WinitPlugin defaults to limit game update rate when window is not visible #7611

Merged
merged 2 commits into from
Aug 3, 2023

Conversation

maniwani
Copy link
Contributor

@maniwani maniwani commented Feb 10, 2023

Fixes #5856. Fixes #8080. Fixes #9040.

Objective

We need to limit the update rate of games whose windows are not visible (minimized or completely occluded). Compositors typically ignore the VSync settings of windows that aren't visible. That, combined with the lack of rendering work, results in a scenario where an app becomes completely CPU-bound and starts updating without limit.

There are currently three update modes.

  • Continuous updates an app as often as possible.
  • Reactive updates when new window or device events have appeared, a timer expires, or a redraw is requested.
  • ReactiveLowPower is the same as Reactive except it ignores device events (e.g. general mouse movement).

The problem is that the default "game" settings are set to Contiuous even when no windows are visible.

More Context

Solution

Change the default "unfocused" UpdateMode for games to ReactiveLowPower just like desktop apps. This way, even when the window is occluded, the app still updates at a sensible rate or if something about the window changes. I chose 20Hz arbitrarily.

@maniwani maniwani added C-Feature A new feature, making something new possible A-Windowing Platform-agnostic interface layer to run your app in C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Feb 10, 2023
@B-head B-head mentioned this pull request Jul 4, 2023
8 tasks
@maniwani maniwani force-pushed the bevy-winit-rate-limiting branch 3 times, most recently from 0b73439 to 3892867 Compare July 25, 2023 19:36
@github-actions
Copy link
Contributor

Example alien_cake_addict failed to run, please try running it locally and check the result.

@maniwani maniwani force-pushed the bevy-winit-rate-limiting branch from 3892867 to 2beffb8 Compare July 25, 2023 19:44
@github-actions
Copy link
Contributor

Example alien_cake_addict failed to run, please try running it locally and check the result.

1 similar comment
@github-actions
Copy link
Contributor

Example alien_cake_addict failed to run, please try running it locally and check the result.

@maniwani maniwani force-pushed the bevy-winit-rate-limiting branch from 2beffb8 to 7acf9a0 Compare July 25, 2023 20:03
@maniwani maniwani changed the title Add rate-limiting update mode to bevy_winit Change defaults to limit game update rate when window is unfocused Jul 25, 2023
@maniwani maniwani requested a review from alice-i-cecile July 25, 2023 20:37
@maniwani
Copy link
Contributor Author

maniwani commented Jul 25, 2023

@NiseVoid suggested that I could use ReactiveLowPower instead of introducing a new variant, and I think that actually works better, so I've done that instead. I also separated this from #7609. I can just rebase the one that isn't merged first.

@maniwani maniwani changed the title Change defaults to limit game update rate when window is unfocused Change WinitPlugin defaults to limit game update rate when window is occluded Jul 25, 2023
@maniwani maniwani added C-Bug An unexpected or incorrect behavior and removed C-Feature A new feature, making something new possible labels Jul 25, 2023
@maniwani maniwani changed the title Change WinitPlugin defaults to limit game update rate when window is occluded Change WinitPlugin defaults to limit game update rate when window is not visible Jul 25, 2023
@mockersf mockersf added the O-MacOS Specific to the MacOS (Apple) desktop operating system label Jul 28, 2023
@mockersf
Copy link
Member

mockersf commented Jul 28, 2023

This seems to be only on macOS, at least all the bugs reported are on macOS.

Do we want to change the default behaviour for everyone just for macOS? Should this be gated based on the OS? Would keeping the background state as 60fps be closer to what most people would expect?

@maniwani you need to repeat the word "fix" before every issue that your PR will fixed otherwise they are not linked

@maniwani
Copy link
Contributor Author

maniwani commented Jul 28, 2023

Do we want to change the default behaviour for everyone just for macOS? Should this be gated based on the OS?

No. (edit: I don't think so.) When researching this issue, I saw reports of it happening in other programs on Windows, macOS, and Linux. Like I wrote in the description, I believe this "hidden windows ignore vsync" behavior is common to most if not all compositors. It's a power-saving feature. So I see no reason to not have this safeguard on all platforms. (edit: I don't see a downside to ReactiveLowPower that would motivate limiting where it's applied either.)

Would keeping the background state as 60fps be closer to what most people would expect?

To enter this "unfocused" mode, the window has to be completely covered or minimized, so you're not interacting with it. Why continue to update at a low-latency rate by default if the window is not visible and there is no input?

I did choose 20Hz arbitrarily as a "slower than normal but still fast" value (to save power), but users can set a higher value if they want.

@mockersf
Copy link
Member

To enter this "unfocused" mode, the window has to be completely covered or minimized, so you're not interacting with it. Why continue to update at a low-latency rate by default if the window is not visible and there is no input?

That's not the case for me, unfocused and visible trigger that mode

Copy link
Member

@aevyrie aevyrie left a comment

Choose a reason for hiding this comment

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

This is a sane default IMO, and I've noticed other games tend to do this when not focused. However, @cart had some opposing feedback when I tried to set this as default behavior in the initial implementation: #3974 (comment)

@mockersf mockersf added the X-Controversial There is active debate or serious implications around merging this PR label Jul 31, 2023
@mockersf
Copy link
Member

controversial because of Cart's comment

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jul 31, 2023
@alice-i-cecile alice-i-cecile requested a review from cart July 31, 2023 22:05
@maniwani
Copy link
Contributor Author

maniwani commented Jul 31, 2023

unfocused and visible trigger that mode

Hmm, I see. Can a window be focused but not visible? And are both of those reliable/truthful?

@cart had some opposing feedback when I tried to set this as default behavior in the initial implementation.

IMO these concerns are off-topic, but I can attempt address them.

Games build logic based on (approximately) fixed timesteps. For things like physics, ticking at 10 FPS increases the likelihood of "tunneling" and things of that nature. Obviously games should try to protect against tick-time-based discrepancies in behavior because perf issues do happen, but completely insulating against sustained low-tick-rates will be challenging for some types of games.

I believe we can consider this resolved now that we have an official FixedUpdate? That is completely insulated from the framerate. Holding this stance for framerate-dependent logic as well is IMO not justifiable.

For online games that send and receive network events from inside a game loop, these should still be processed at "reasonable" intervals to encourage proper state syncs. What constitutes "reasonable" is game-specific. I think the only safe assumption to make by default is "developer configured fps limits for active gameplay".

As someone with a current occupation in this area, realtime multiplayer games generally add their logic to FixedUpdate (or an equivalent). In that setup, low FPS is only an issue if your socket is being (1) read by the main thread and (2) it happens to be essential to minimize delay between a packet arriving and it being read. If that is essential though, the game should consider using a background thread to listen to the socket instead.

Developers can also just change this setting if they want different behavior. We shouldn't leave this bug around just because the default isn't the best for a particular circumstance.

A common case for "unfocusing games" will be "playing a windowed game and alt-tabbing to another window". In this case, an unfocused game might still be visible and the "perceived quality" of the game will go down if it starts "lagging" when alt-tabbed.

Maybe I'm lost on the distinction between unfocused and minimized? I thought they were the same thing until now. #3974 had "reduce power when minimized" in its objectives. The goal here is just to fix CPU utilization blowing up when minimized. With respect to VSync, major OS platforms seem to treat a window being "minimized" and being "completely occluded" the same.

Why does UpdateMode currently change with focus? Is there an issue detecting when all windows are in a minimized/occluded state, or do we just need another argument to UpdateMode? Fixing CPU blowup needs to limit the update rate when minimized, as that's when both VSync and the rendering workload can disappear.

@maniwani
Copy link
Contributor Author

maniwani commented Jul 31, 2023

I mistakenly thought the unfocused mode was specifically for this minimized/occluded case. It seems like we actually need three modes?

  • visible and focused
  • visible and unfocused
  • not visible

Probably with these default settings?

pub fn game() -> Self {
    WinitSettings {
        visible_and_focused: UpdateMode::Continuous,
        visible_and_unfocused: UpdateMode::Continuous,
        not_visible: UpdateMode::ReactiveLowPower {
                max_wait: Duration::from_millis(50), // 20Hz
        },
        ..Default::default()
    }
}

(edit: Apparently, this isn't as easy as it sounds. Not all platforms support window.is_visible() or window.is_minimized(). 🙁)

@cart
Copy link
Member

cart commented Jul 31, 2023

IMO these concerns are off-topic, but I can attempt address them.

I don't see this as being off topic at all and it relates directly to the change you have made.

I believe we can consider this resolved now that we have an official FixedUpdate? That is completely insulated from the framerate. Holding this stance for framerate-dependent logic as well is IMO not justifiable.

This is still coupled to the Main schedule run (which is what you are limiting here). If you only run the App at 20fps, FixedUpdate will still be bounded by that, it will just run more FixedUpdates in the next update.

In that setup, low FPS is only an issue if your socket is being (1) read by the main thread and (2) it happens to be essential to minimize delay between a packet arriving and it being read. If that is essential though, the game should consider using a background thread to listen to the socket instead.

This point wasn't about waiting for network events on the socket. It was about the game itself processing the received messages. Clients processing server messages later than they need to can definitely create problems (ex: lag compensation or client side prediction logic might change based on how delayed the receipt is). Games that aren't resilient to the difference in delay from 10fps to 60fps are probably going to have problems anyway, but latency of that size does matter in some cases (especially when sustained / when the game is real time).

Imo a minimized game indicates that you can stop rendering things (as an optimization), not that you should start changing how the game behaves (such as how often it updates). I think what we want is a configurable FPS cap.

@cart
Copy link
Member

cart commented Jul 31, 2023

I think what we want is a configurable FPS cap.

As in "run my game at no more than 100fps". Functionally this is just the difference between setting the limit to 20 vs 60, but imo that is a huge distinction.

If we're only going to enable the cap for minimized games, we should pick a number we expect to be higher than most refresh rates, not lower.

@maniwani
Copy link
Contributor Author

maniwani commented Jul 31, 2023

This is still coupled to the Main schedule run (which is what you are limiting here). If you only run the App at 20fps, FixedUpdate will still be bounded by that, it will just run more FixedUpdates in the next update.

Right. Is there still an issue? Game logic will behave consistently. It won't suddenly develop a tunneling problem.

This point wasn't about waiting for network events on the socket. It was about the game itself processing the received messages. Clients processing server messages later than they need to can definitely create problems (ex: lag compensation or client side prediction logic might change based on how delayed the receipt is).

If by lag compensation, you're referring to the hit detection kind, the client reading messages late should not have an effect on it. You can do lag-compensated hit detection with perfect accuracy without relying on estimates. There's no reason a server needs to estimate the "when" a client saw something, clients can just send that with their inputs.

Being sensitive to delayed message reads is not an intrinsic issue, it's a weakness of an implementation. In my experience, the only thing this specific source of latency has an unavoidable impact on is a client's efforts to maintain a target "clock offset" from the server (how far ahead of the server in time). Delay between messages arriving and being read results in less precise estimates of what that offset is.

But even then, I haven't observed much sensitivity to those estimation errors at the small delays we're talking about here.

(edit 2: Clients can compensate for any delay on their side by just sending their messages earlier. To begin with, clients run ahead of the server so their inputs are likely to arrive on time. If they're less sure about when their inputs arrive, their "clock offset" just needs to grow proportionally bigger to maintain the same odds.

If you implement that, then it's OK even if a client that normally runs FixedUpdate at 60Hz suddenly drops from 120FPS to 10FPS. The delay from their lower FPS will add to their measured RTT and to the variance in their "clock offset" estimates, so their target "clock offset" will grow bigger, and they'll run further ahead. It's a simple feedback loop and very robust.)

Games that aren't resilient to the difference in delay from 10fps to 60fps are probably going to have problems anyway, but latency of that size does matter in some cases (especially when sustained / when the game is real time).

Can you name those cases? I disagree with your presentation of networking cases so far. (edit: I admit I'm being a bit uncharitable here, but I know from experience that networked gameplay can be made insensitive to this specific delay with relative ease.)

If we're only going to enable the cap for minimized games, we should pick a number we expect to be higher than most refresh rates, not lower.

Having any limit would avoid the bug, but isn't it still important to conserve power compared to when the window is visible?
(edit 3: In the end, users can always just raise the limit if needed. Aren't users much more likely to take issue with a backgrounded app wasting power than with a low cap breaking their netcode? The default being a low cap is the safer move.)

@cart
Copy link
Member

cart commented Aug 1, 2023

Right. Is there still an issue? Game logic will behave consistently. It won't suddenly develop a tunneling problem.

Yeah I was largely thinking of the latency issues when I wrote that. Tunneling would not be a concern (provided physics-like logic is in FixedUpdate, which it should be).

If you implement that, then it's OK even if a client that normally runs FixedUpdate at 60Hz suddenly drops from 120FPS to 10FPS. The delay from their lower FPS will add to their measured RTT and to the variance in their "clock offset" estimates, so their target "clock offset" will grow bigger, and they'll run further ahead. It's a simple feedback loop and very robust.)

Yup I've built systems that do this sort of thing.

Can you name those cases? I disagree with your presentation of networking cases so far.

In client side prediction implementations (both mine and others I've seen), you play user inputs on top of last-known-good server states to estimate the new state that the server will report (ex: how much the player will move when you press right on your controller). In cases where it has been a long time since the last received server state, you need to be careful about what outcomes your report to the user based solely on their local inputs. And in cases where there are interactions between networked objects (which can happen quickly), its possible that client-side-predicted outcome will become out-of-sync with the server. Resolving these outcomes in a way that "feels good" to the user is challenging (ex: people hate rubber-banding).

Whenever you delay processing of a message on the client, you increase the risk of encountering diverging states. Do I think running a networked game (implemented properly) at 20fps would cause problems in the vast majority of cases ... no. But its increasing the odds of creating desync problems that the networking logic needs to resolve. When weighed against the power savings of running at 20fps, I guess saving energy / battery is pretty important. And if the game is fully minimized / obstructed, its likely that the user is no longer providing inputs, so desync is less likely to happen. And even if it does, the user won't see the weirdness.

Provided we are only talking about the minimized / not visible case (and it looks like your second-to-last message is saying that), then I'm more on board for the change.

@maniwani
Copy link
Contributor Author

maniwani commented Aug 1, 2023

When weighed against the power savings of running at 20fps, I guess saving energy / battery is pretty important. And if the game is fully minimized / obstructed, its likely that the user is no longer providing inputs, so desync is less likely to happen. And even if it does, the user won't see the weirdness.

Provided we are only talking about the minimized / not visible case (and it looks like your second-to-last message is saying that), then I'm more on board for the change.

Yes, it'd be ideal if we could have separate update modes for "visible and focused", "visible and not focused", and "not visible". However, I don't think that can work. Window::is_visible() and Window::is_minimized() don't work on several OS platforms.

I've taken that to mean that those platforms do not expose a reliable means of knowing when a window is minimized/obstructed. If that's the case, then we can't really separate "visible and not focused" and "not visible" and we'll have to compromise.

So my argument is that choosing a default that favors general power savings (i.e. 20Hz) over specific usecases like networking is the choice that will benefit the most users. It's also the more unsurprising default in my opinion.

Whenever you delay processing of a message on the client, you increase the risk of encountering diverging states [between client and server]. Do I think running a networked game (implemented properly) at 20fps would cause problems in the vast majority of cases ... no. But its increasing the odds of creating desync problems that the networking logic needs to resolve.

Right, as a client predicts further and further ahead of the server, its odds of being wrong (and seeing jarring corrections) go up. But just having high ping will put you in the same state. So this is standard fare for a networked game in my opinion.

So even with a "low" number like 20Hz, users who write networked games the "right way" will be fine at an artificially lowered FPS because it has nearly the same effect as having a higher ping. And updating once every 50-100ms instead of once every 5-10ms is not going to trigger a (reasonable) connection timeout.

And even if a user doesn't do it the "right way", they can just set a higher cap. This setting is easy to change (even at runtime), and I can't imagine a user having a hard time tracing an issue back to this cap. I don't understand the strong resistance.

(edited)
If a game is played in "exclusive fullscreen" mode, players wouldn't see anything after they tab out. I'd also bet apps most often enter unfocused_mode for being "not visible" and not "visible but not focused".

@cart
Copy link
Member

cart commented Aug 1, 2023

I've taken that to mean that those platforms do not expose a reliable means of knowing when a window is minimized/obstructed. If that's the case, then we can't really separate "visible and not focused" and "not visible" and we'll have to compromise.
So my argument is that choosing a default that favors general power savings (i.e. 20Hz) over specific usecases like networking is the choice that will benefit the most users. It's also the more unsurprising default in my opinion.

If we're capping framerate when unfocused, I think the reduction in perceived quality (dropping from 60 to 20 fps) when you click away is pretty gnarly. If someone is playing in windowed mode and they click to another window, the dropped framerate is going to be perceptible. Windowed mode play isn't super uncommon, especially for "casual" games. And for non-game apps, if a video is playing in the app, dropping the framerate to 20 is going to be nasty.

I think 60 fps is the right compromise / default if we're going to feed off of "focused-ness". But capping framerate on "focused-ness" already seems pretty hackey to me. "Focused-ness" is a measure of "did the user select the window in their window manager", not "does the user care about the contents of the window".

@maniwani
Copy link
Contributor Author

maniwani commented Aug 1, 2023

But capping framerate on "focused-ness" already seems pretty hackey to me. "Focused-ness" is a measure of "did the user select the window in their window manager", not "does the user care about the contents of the window".

I agree, but as far as I know winit (and by extension bevy) is just inheriting the limits of the platforms. Focus seems to be the only universally available indicator of a window's state.

If someone is playing in windowed mode and they click to another window, the dropped framerate is going to be perceptible. Windowed mode play isn't super uncommon, especially for "casual" games. And for non-game apps, if a video is playing in the app, dropping the framerate to 20 is going to be nasty.

Could we recommend that games playing a video clip temporarily raise the cap? We've made this setting as accessible as can be, so it doesn't feel like these cases outweigh more aggressive power savings. In your perspective, is an app wasting power when you click out of it a better default problem than it looking worse?

I'm not sure, and I'll take 60 FPS over no limit at all, but it still doesn't feel super well-justified. Maybe that's just me.

Also, by default, "non-game" apps already run at an even lower FPS (once per minute).

/// Default settings for desktop applications.
///
/// [`Reactive`](UpdateMode::Reactive) if windows have focus,
/// [`ReactiveLowPower`](UpdateMode::ReactiveLowPower) otherwise.
pub fn desktop_app() -> Self {
WinitSettings {
focused_mode: UpdateMode::Reactive {
wait: Duration::from_secs(5),
},
unfocused_mode: UpdateMode::ReactiveLowPower {
wait: Duration::from_secs(60),
},
..Default::default()
}
}

@mockersf
Copy link
Member

mockersf commented Aug 2, 2023

Not as small as this PR, but there is also #8976 / #9034 that could fix this issue and bring more improvements

@maniwani
Copy link
Contributor Author

maniwani commented Aug 2, 2023

To give a bit more context.

I remember the very first "incident" I saw related to this issue. This Discord discussion. A user had left their app running in the background and it began updating so quickly that it reached (what has become known as) MAX_CHANGE_AGE overnight.

(IIRC the math worked out to something like 60,000+ system executions a second. You'd need to average ~38,000 system executions per second to observe that warning within 24 hours.)

Granted, their app was said to be very simple, but bevy is already pretty efficient and is still being improved. It's likely many user apps will easily max out whatever cap we set when you take rendering away, so I'm just trying to be cautious. Phones are the most common device, but players won't be able to play for long if the game is a significant drain even in the background. (edit: Mobile OS platforms may avoid this "visible but unfocused" scenario, so it might not be an issue. Not sure.)


Godot, Unity, and Unreal by default cap their editor FPS to 10-30 when its window loses focus. I don't know if a similar option exists for their exported game binaries or what their defaults are.

@maniwani
Copy link
Contributor Author

maniwani commented Aug 2, 2023

Not as small as this PR, but there is also #8976 / #9034 that could fix this issue

I don't see where these fix this issue. There's still an inherent lack of reliable "window is not visible" detection. I also have a lot of concerns about completely reworking how the runner works.

@mockersf
Copy link
Member

mockersf commented Aug 2, 2023

Phones are the most common device

This will be handled in a completely different way on phones. iOS / Android sends app lifecycle events that informs the app if it should stop rendering and/or stop all execution. Currently all those events are not made available, there are pr to add them at least on iOS side. The events are not "please stop running", they are "you will stop running, prepare yourself for that".

on Android, Bevy currently exit when it detect it's not the main app running. on iOS, the system doesn't let Bevy have CPU time by default in background.

@cart
Copy link
Member

cart commented Aug 2, 2023

Could we recommend that games playing a video clip temporarily raise the cap? We've made this setting as accessible as can be, so it doesn't feel like these cases outweigh more aggressive power savings. In your perspective, is an app wasting power when you click out of it a better default problem than it looking worse?

I think "perceived app quality" should come first in this case (by default). People can use apps when they are unfocused (ex: playing games with controllers, consuming visual content like videos + cutscenes, etc).

I think it should be on the app developer to make the choice to degrade that experience in the interest of power consumption (or select a non-default profile).

I am however convinced that it makes sense to optimize the non-universal obstructed states (minimized + fully obstructed).

I'm not worried about the mobile case. As @mockersf mentioned it handles these things differently.

@maniwani maniwani force-pushed the bevy-winit-rate-limiting branch from 7acf9a0 to ad6daeb Compare August 2, 2023 22:14
@cart cart added this pull request to the merge queue Aug 2, 2023
Merged via the queue into bevyengine:main with commit bbf1c23 Aug 3, 2023
@maniwani maniwani deleted the bevy-winit-rate-limiting branch August 5, 2023 17:38
@cart cart mentioned this pull request Oct 13, 2023
43 tasks
github-merge-queue bot pushed a commit that referenced this pull request Jan 15, 2024
…is not visible (for real this time) (#11305)

# Objective

I goofed. #7611 forgot to change the default update modes set by the
`WinitPlugin`.


<https://github.com/bevyengine/bevy/blob/ce5bae55f64bb095e1516427a706a2622ccf2d23/crates/bevy_winit/src/winit_config.rs#L53-L60>


<https://github.com/bevyengine/bevy/blob/ce5bae55f64bb095e1516427a706a2622ccf2d23/crates/bevy_winit/src/lib.rs#L127>

## Solution

Change `Default` impl for `WinitSettings` to return the `game` settings
that limit FPS when the app runs in the background.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Windowing Platform-agnostic interface layer to run your app in C-Bug An unexpected or incorrect behavior C-Usability A targeted quality-of-life change that makes Bevy easier to use O-MacOS Specific to the MacOS (Apple) desktop operating system S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
6 participants