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

Fix timer with zero duration #8467

Merged
merged 1 commit into from Apr 24, 2023

Conversation

CrazyRoka
Copy link
Contributor

Objective

Timer with zero Duration panics at tick() because of division by zero. This PR Fixes #8463 .

Solution

  • Handle division by zero separately with checked_div and checked_rem.

Changelog

  • Replace division with checked_div. Set times_finished_this_tick to u32::MAX when duration is zero.
  • Set elapsed to Duration::ZERO when timer duration is zero.
  • Set percent to 1.0 when duration is zero.
  • times_finished_this_tick is not used anywhere, that's why this change will not affect other parts of the project.
  • times_finished_this_tick is set to 0 after reset() and before first tick() call.

@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-Time Involves time keeping and reporting labels Apr 22, 2023
@alice-i-cecile
Copy link
Member

These fixes look correct: nice work handling this robustly.

The other alternative would be to simply make it impossible to construct a timer with zero duration. It's not entirely clear what behavior would be best there though: panicking on construction simply moves the problem. We could enforce this with NonZero types, but again, constructing those usually involves an unwrap or other surprising failure.

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.

Ultimately I think this is the least problematic way to fix this panic.

@alice-i-cecile alice-i-cecile added this to the 0.11 milestone Apr 23, 2023
@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 Apr 23, 2023
Copy link
Member

@mockersf mockersf left a comment

Choose a reason for hiding this comment

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

I would prefer to make the issue impossible by types, as this means the check will happen several times. Someone doing lots of timers and calling percent on every frame may notice an impact... but we can revisit that then

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Apr 24, 2023
Merged via the queue into bevyengine:main with commit e2531b2 Apr 24, 2023
24 checks passed
Connor-McMillin added a commit to Connor-McMillin/bevy that referenced this pull request May 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Time Involves time keeping and reporting 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.

Setting the timer duration to zero panicks
4 participants