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

menu example panics with can not convert float seconds to Duration: value is negative #41

Closed
djeedai opened this issue Aug 3, 2022 · 13 comments
Labels
bug Something isn't working
Milestone

Comments

@djeedai
Copy link
Owner

djeedai commented Aug 3, 2022

@Gyrobifastigium FYI this is probably a bug in #39

This is blocking #34

@djeedai djeedai added the bug Something isn't working label Aug 3, 2022
@djeedai djeedai added this to the Bevy 0.8 milestone Aug 3, 2022
@djeedai djeedai pinned this issue Aug 3, 2022
@Gyrobifastigium
Copy link
Contributor

Gyrobifastigium commented Aug 4, 2022

The panic happens because:

  • The Delay tween in the example has a 0-duration self.timer, so self.timer.percent() returns NaN.
  • As a result, tween.progress() returns NaN.
  • As a result, tween.duration().mul_f32(1.0 - tween.progress()) panics.

I feel like Delay::progress() should yield 1.0 (or 0.0) for a 0-duration timer, but that would require special-casing.

EDIT: I guess any return value between 0.0 and 1.0 would be "technically true" and would make the math work in Sequence::tick because it gets multiplied by tween.duration() (aka 0) anyways. An elapsed-based API would circumvent the issue.

@SUPERCILEX
Copy link
Contributor

If that's the case, then this will almost certainly be fixed by #38.

Sitenote: #39 is probably going to get reverted since we're rewriting tracks and sequences.

@SUPERCILEX
Copy link
Contributor

Nevermind, not fixed by #38 because progress is still implemented the same way.

Though as mentioned, tick needs to be rewritten anyway to not use progress so this will go away.

@SUPERCILEX
Copy link
Contributor

Note to self: write a test for this when rewriting sequences.

@Gyrobifastigium
Copy link
Contributor

I don't know if this should go in a separate issue, but somehow in my game where I'm using a fork of bevy_tweening v0.4 with #39 applied, I just got a panic on the line delta -= tween_remaining. That shouldn't be possible unless some invariant in the tween API (between duration(), progress(), and tick()) is broken -- maybe as a result of floating point / rounding error.

As a workaround, I'm changing the line to delta = delta.saturating_sub(tween_remaining) in my fork for now.

@djeedai
Copy link
Owner Author

djeedai commented Aug 9, 2022

Interesting. What's the panic about exactly? Is delta becoming negative?

@Gyrobifastigium
Copy link
Contributor

Gyrobifastigium commented Aug 9, 2022

Yes, it is. I've only seen it happen once so it seems to be pretty rare.

@shuoli84
Copy link
Contributor

In my case, it is not rare, I hit by the panic each time run

RUST_BACKTRACE=1 cargo run --example menu --features="bevy/bevy_winit"

@shuoli84
Copy link
Contributor

A work around, set start_time_ms to a small value, e.g

    let mut start_time_ms = 10;

@benfrankel
Copy link

benfrankel commented Sep 27, 2022

In my case, it is not rare, I hit by the panic each time run

RUST_BACKTRACE=1 cargo run --example menu --features="bevy/bevy_winit"

I assume you're referring to the "can not convert float seconds to Duration: value is negative" panic (original panic mentioned in this issue, guaranteed to occur in the menu example), and not the "overflow when subtracting durations" panic (mentioned in #41 (comment), seen rarely in other code).

@shuoli84
Copy link
Contributor

@benfrankel ah, yeah, different panic, "'can not convert float seconds to Duration: value is either too big or NaN'"

@djeedai
Copy link
Owner Author

djeedai commented Sep 27, 2022

The Delay tween in the example has a 0-duration self.timer, so self.timer.percent() returns NaN.

Actually I think a Delay with a 0 duration makes no sense. I'll fix that to panic because creating such an object is most likely never what a user wants to do.

djeedai added a commit that referenced this issue Sep 27, 2022
Make `Delay::new()` panic if a zero duration is passed as argument. Fix
the `menu` example to skip inserting a `Sequence<Transform>` containing
a zero-duration `Delay`.

Bug: #41
djeedai added a commit that referenced this issue Sep 27, 2022
Make `Delay::new()` panic if a zero duration is passed as argument. Fix
the `menu` example to skip inserting a `Sequence<Transform>` containing
a zero-duration `Delay`.

Bug: #41
@djeedai
Copy link
Owner Author

djeedai commented Sep 27, 2022

I'm closing this issue because the menu example one is fixed. I believe the rare issue of @Gyrobifastigium might also be fixed, but since it's in a fork we can't really officially support it anyway. Please reopen if there's still an issue with the menu example, or open a different issue if something else. Thanks!

@djeedai djeedai closed this as completed Sep 27, 2022
@djeedai djeedai unpinned this issue Oct 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

5 participants