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] - Remaining fn in Timer #5971

Closed
wants to merge 4 commits into from

Conversation

Sergi-Ferrez
Copy link
Contributor

Objective

Fixes #5963

Solution

Add remaining fn in Timer class, this function only minus total duration with elapsed time.

@alice-i-cecile alice-i-cecile added C-Usability A simple quality-of-life change that makes Bevy easier to use A-Utils Utility functions and types labels Sep 12, 2022
@rparrett
Copy link
Contributor

rparrett commented Sep 12, 2022

For symmetry with elapsed and elapsed_secs, I think we should name this remaining_secs and add a remaining function that returns a Duration.

crates/bevy_time/src/timer.rs Outdated Show resolved Hide resolved
/// use std::time::Duration;
/// let mut timer = Timer::from_seconds(2.0, false);
/// timer.tick(Duration::from_secs_f32(0.5));
/// assert_eq!(timer.remaining(), 1.5);
Copy link
Member

Choose a reason for hiding this comment

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

Float equality checking is a bad habit; we should have some error bars here.

@Sergi-Ferrez
Copy link
Contributor Author

I purpose using float-cmp to properly compare floats in an error range.

@harudagondi
Copy link
Member

harudagondi commented Sep 13, 2022

I purpose using float-cmp to properly compare floats in an error range.

We don't have to pull in another crate. I think f32::total_cmp should be fine for doctests.

Another alternative is following the suggestion of this clippy lint: float_cmp.

@alice-i-cecile
Copy link
Member

LGTM once the float equality check is addressed :)

@Sergi-Ferrez
Copy link
Contributor Author

Other parts of timer implementation also use implicit float comparison. Maybe create another issue for fixing that?

@rparrett
Copy link
Contributor

total_cmp doesn't really help with the sort of issue we're trying to avoid here.
https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=95d76d813f1d9c954a4e00323b129f52

@alice-i-cecile
Copy link
Member

(Oops, approved by accident; don't count it)

@mockersf
Copy link
Member

In my opinion, the doc tests you added are not really needed and should be removed. But it's the current documentation style of Timer, so 🤷

For me, having that many code examples that show "obvious" behaviours make docs less readable: https://docs.rs/bevy/latest/bevy/prelude/struct.Timer.html

Approving this PR as is, but I think an issue or a followup PR should be opened to remove most of the doc examples.

@mockersf mockersf added A-Time Involves time keeping and reporting and removed A-Utils Utility functions and types labels Sep 13, 2022
crates/bevy_time/src/timer.rs Outdated Show resolved Hide resolved
@cart
Copy link
Member

cart commented Sep 14, 2022

bors r+

bors bot pushed a commit that referenced this pull request Sep 14, 2022
# Objective

Fixes #5963 

## Solution

Add remaining fn in Timer class, this function only minus total duration with elapsed time.

Co-authored-by: Sergi-Ferrez <61662926+Sergi-Ferrez@users.noreply.github.com>
@bors bors bot changed the title Remaining fn in Timer [Merged by Bors] - Remaining fn in Timer Sep 14, 2022
@bors bors bot closed this Sep 14, 2022
@Sergi-Ferrez Sergi-Ferrez deleted the Add-Remaining-Time branch September 14, 2022 19:54
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 19, 2022
# Objective

Fixes bevyengine#5963 

## Solution

Add remaining fn in Timer class, this function only minus total duration with elapsed time.

Co-authored-by: Sergi-Ferrez <61662926+Sergi-Ferrez@users.noreply.github.com>
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 28, 2022
# Objective

Fixes bevyengine#5963 

## Solution

Add remaining fn in Timer class, this function only minus total duration with elapsed time.

Co-authored-by: Sergi-Ferrez <61662926+Sergi-Ferrez@users.noreply.github.com>
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective

Fixes bevyengine#5963 

## Solution

Add remaining fn in Timer class, this function only minus total duration with elapsed time.

Co-authored-by: Sergi-Ferrez <61662926+Sergi-Ferrez@users.noreply.github.com>
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-Usability A simple quality-of-life change that makes Bevy easier to use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a method on Timer to get the remaining time
6 participants