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] - Fixes Timer Precision Error Causing Panic #2362

Closed

Conversation

jacobgardner
Copy link
Contributor

Objective

Fixes #2361

Solution

Uses integer division instead of floating-point which prevents precision errors, I think.

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Jun 19, 2021
@jacobgardner jacobgardner changed the title Bug fix and tests for #2361 Fixes Timer Precision Error Causing Panic Jun 19, 2021
@NathanSWard NathanSWard added C-Bug An unexpected or incorrect behavior A-Core Common functionality for all bevy apps and removed S-Needs-Triage This issue needs to be labelled labels Jun 19, 2021
@NathanSWard
Copy link
Contributor

NathanSWard commented Jun 19, 2021

I'm a little confused because in #2361 the panic error is:

overflow when subtracting durations

which would imply the panic was at

self.set_elapsed(self.elapsed() - self.duration() * self.times_finished);

(Specifically the substraction).

EDIT Ahhh ok, now I see. The value of elapsed is a result of times_finished)

@NathanSWard
Copy link
Contributor

bors try

bors bot added a commit that referenced this pull request Jun 19, 2021
@bors
Copy link
Contributor

bors bot commented Jun 19, 2021

try

Build failed:

@NathanSWard
Copy link
Contributor

NathanSWard commented Jun 19, 2021

@jacobgardner it looks like you need to run cargo fmt --all before this build can pass.

@jacobgardner
Copy link
Contributor Author

@NathanSWard sorry about that. Should be formatted now.

@NathanSWard
Copy link
Contributor

bors try

bors bot added a commit that referenced this pull request Jun 20, 2021
@NathanSWard NathanSWard 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 20, 2021
t.tick(duration);
assert_eq!(t.times_finished(), 33);
t.tick(duration);
assert_eq!(t.times_finished(), 33);
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't the extra tick be on this one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So my logic was roughly this:

0 + 33.3333 = 33.3333 (Rounds down to 33 times finished, leaving the remainder as .3333)
.3333 + 33.3333 = 33.6666 (Rounds down to 33 again, remainder fo .6666)
.6666 + 33.3333 = 33.9999 (Rounds down last time to 33 with remainder of .9999)
.9999 + 33.3333 = 34.3332 (Rounds down to 34 this time)

Copy link
Member

Choose a reason for hiding this comment

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

It will add the extra tick after the fourth iteration, then it will correctly add it every three iterations... I would have liked it to be every three from the start, but that's probably not possible when using those kind of numbers 🤷

Copy link
Contributor

Choose a reason for hiding this comment

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

Gotta love floating point numbers 🙃

@mockersf
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Jun 26, 2021
# Objective

Fixes #2361 

## Solution

Uses integer division instead of floating-point which prevents precision errors, I think.
@bors bors bot changed the title Fixes Timer Precision Error Causing Panic [Merged by Bors] - Fixes Timer Precision Error Causing Panic Jun 26, 2021
@bors bors bot closed this Jun 26, 2021
@jacobgardner jacobgardner deleted the bugfix/timer-precision-bug branch June 26, 2021 23:39
ostwilkens pushed a commit to ostwilkens/bevy that referenced this pull request Jul 27, 2021
# Objective

Fixes bevyengine#2361 

## Solution

Uses integer division instead of floating-point which prevents precision errors, I think.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Core Common functionality for all bevy apps 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.

Timer tick can lead to a rounding error causing an overflow panic
4 participants