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

Timer tick can lead to a rounding error causing an overflow panic #2361

Closed
jacobgardner opened this issue Jun 19, 2021 · 2 comments
Closed
Labels
C-Bug An unexpected or incorrect behavior

Comments

@jacobgardner
Copy link
Contributor

Bevy version

Confirmed broken on 0.5 and 00d8d5d (HEAD of main at time of bug report)

Operating system & version

Windows 10

What you did

    #[test]
    fn reproduce_bug() {
        let mut t = Timer::from_seconds(0.01, true);
        let duration = Duration::from_secs_f64(1.0 / 3.0);

        t.tick(duration);
        t.tick(duration);
        t.tick(duration);
    }

What you expected to happen

No run-time panic. Roughly the same behavior as seen when using Duration::from_secs_f32

What actually happened

PS E:\bevy\crates\bevy_core> cargo test reproduce_bug
    Blocking waiting for file lock on build directory
   Compiling bevy_core v0.5.0 (E:\bevy\crates\bevy_core)
    Finished test [unoptimized + debuginfo] target(s) in 17.81s
     Running unittests (E:\bevy\target\debug\deps\bevy_core-51c265877747d3ba.exe)

running 1 test
test time::timer::tests::reproduce_bug ... FAILED

failures:

---- time::timer::tests::reproduce_bug stdout ----
thread 'time::timer::tests::reproduce_bug' panicked at 'overflow when subtracting durations', library\core\src\time.rs:890:31
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Additional information

For a fixed timestep, I can work around this by using f32 or changing the offset, but if relying on a non-fixed timestep, I imagine this edge-case will periodically appear.

@jacobgardner jacobgardner added C-Bug An unexpected or incorrect behavior S-Needs-Triage This issue needs to be labelled labels Jun 19, 2021
@jacobgardner jacobgardner changed the title Timer overflow panic when tick leads to rounding error. Timer tick can lead to a rounding error causing an overflow panic Jun 19, 2021
@jacobgardner
Copy link
Contributor Author

jacobgardner commented Jun 19, 2021

It looks like Duration's nanosecond precision can cause Timer::percent() to return 1.0 even when it's just barely not quite there yet.

// From bevy_core/src/time/timer.rs:220 - 222

  self.times_finished = self.percent().floor() as u32;
  // Duration does not have a modulo
  self.set_elapsed(self.elapsed() - self.duration() * self.times_finished);

So times_finished should be at 0 still instead of 1 (or whatever rising edge we're at), but because percent's precision was too low it got rounded up prematurely.

@jacobgardner
Copy link
Contributor Author

All the tests appear to pass when doing integer division directly, i.e.

        if self.finished() {
            if self.repeating() {
                self.times_finished = (self.elapsed().as_nanos() / self.duration.as_nanos()) as u32;
                // Duration does not have a modulo
                self.set_elapsed(self.elapsed() - self.duration() * self.times_finished);
            } else {
                self.times_finished = 1;
                self.set_elapsed(self.duration());
            }
        }

jacobgardner added a commit to jacobgardner/bevy that referenced this issue Jun 19, 2021
@NathanSWard NathanSWard added core and removed S-Needs-Triage This issue needs to be labelled labels Jun 19, 2021
@bors bors bot closed this as completed in 52e8a19 Jun 26, 2021
ostwilkens pushed a commit to ostwilkens/bevy that referenced this issue 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
C-Bug An unexpected or incorrect behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants