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

[Merged by Bors] - Fix crash when using Duration::MAX #4900

Closed
wants to merge 3 commits into from
Closed

[Merged by Bors] - Fix crash when using Duration::MAX #4900

wants to merge 3 commits into from

Conversation

SUPERCILEX
Copy link
Contributor

Objective

If you set the ReactiveLowPower max wait to Duration::MAX, stuff panics. Fix that.

Solution

Wait forever if addition failed.

Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>
@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-Windowing Platform-agnostic interface layer to run your app in labels Jun 2, 2022
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Good choice. Can we document this behavior under ControlFlow?

@SUPERCILEX
Copy link
Contributor Author

Do you mean add a comment near the if statement?

@alice-i-cecile
Copy link
Member

Ah my bad. No, I meant as a comment on the variants in UpdateMode.

Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>
@SUPERCILEX
Copy link
Contributor Author

Gotya, done.

@james7132 james7132 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 Jun 2, 2022
@@ -68,7 +68,11 @@ pub enum UpdateMode {
/// Once the app has executed all bevy systems and reaches the end of the event loop, there is
/// no way to force the app to wake and update again, unless a `winit` event (such as user
/// input, or the window being resized) is received or the time limit is reached.
Reactive { max_wait: Duration },
Reactive {
/// The maximum time to wait before the event loop runs again. Note that if the duration is
Copy link
Member

Choose a reason for hiding this comment

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

These should be line split, and the advice should be more precise. What do we need to set the value to in order to get an indefinite wait?

In many ways, this is actually a feature, so we should demonstrate how to use it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well the problem is that it's platform dependent. Some platforms store Instants as u32s, others store them as u64s so the answer is "dunno, you figure it out." :) That said, using Duration::MAX is effectively the same as waiting for ever so I made a note of that.

Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Perfect, thanks!

@alice-i-cecile
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Jun 2, 2022
# Objective

If you set the `ReactiveLowPower` max wait to `Duration::MAX`, stuff panics. Fix that.

## Solution

Wait forever if addition failed.
@bors bors bot changed the title Fix crash when using Duration::MAX [Merged by Bors] - Fix crash when using Duration::MAX Jun 2, 2022
@bors bors bot closed this Jun 2, 2022
james7132 pushed a commit to james7132/bevy that referenced this pull request Jun 7, 2022
# Objective

If you set the `ReactiveLowPower` max wait to `Duration::MAX`, stuff panics. Fix that.

## Solution

Wait forever if addition failed.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective

If you set the `ReactiveLowPower` max wait to `Duration::MAX`, stuff panics. Fix that.

## Solution

Wait forever if addition failed.
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 S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants