Fix questionable use of floats and re-work all ticking logic #21
Fix questionable use of floats and re-work all ticking logic #21SUPERCILEX wants to merge 1 commit intodjeedai:mainfrom
Conversation
What's broken? - Float usage inconsistently used f64 or f32. I changed everything to be f32 since precision isn't really the priority in graphics. - Float rounding issues were present in several places. For example, the AnimClock never reset the elapsed time leading to a continual loss of precision as the animation progressed. - Sequences did not handle deltas that crossed animation boundaries. This means that if a one-hour delta was received, only one animation would complete. - I tried to simplify all the other logic in the process while improving performance. Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>
djeedai
left a comment
djeedai
left a comment
There was a problem hiding this comment.
Thanks for the contribution! There's a bunch of good things in there, but also unfortunately a few other ones that are either arguable or conflict with on-going works toward making Sequence (and possibly Tracks) loopable, and therefore would be reverted as soon as that work lands. I'm also not very clear on where are the bugs being fixed.
I'd suggest splitting the changes into smaller chunks, separating the targeted bug fix(es), the optimization(s), and the clean-ups around f32 / f64 mix. We can chat on Discord if you want.
| use std::cmp::min; | ||
| use std::time::Duration; |
There was a problem hiding this comment.
| use std::cmp::min; | |
| use std::time::Duration; | |
| use std::{cmp::min, time::Duration}; |
|
|
||
| fn times_completed(&self) -> u32 { | ||
| self.times_completed | ||
| if self.index == self.tweens.len() { |
There was a problem hiding this comment.
This will get reverted by #16. Keeping self.times_completed seems like a better choice.
| time: Duration, | ||
| times_completed: u32, | ||
| elapsed: Duration, | ||
| completed: bool, |
There was a problem hiding this comment.
This will get reverted by #8. It seems better to keep times_completed.
| let times_completed = progress as u32; | ||
| fn set_progress(&mut self, progress: f32) { | ||
| let progress = if self.is_looping { | ||
| progress.fract() |
There was a problem hiding this comment.
Because of the deleted .max(0.), this may set progress to a negative value since (-3.6_f32).fract() == -0.6_f32, which will make Duration::mul_f32() panic.
In general the interface for set_progress() shouldn't allow negative values, that makes little sense unless the animation is infinitely looping. So it's better to just avoid it completely for the sake of simplicity and clarity of the API.
| return; | ||
| } | ||
|
|
||
| self.elapsed = self.duration.mul_f32(progress.clamp(0., 1.)); |
There was a problem hiding this comment.
The interface for set_progress() should accept values > 1., which allows jumping to iteration N of a repeating animation. Looping sequences are not yet implemented but there's work on-going for them (#16).
| let full_completions = | ||
| (tween.times_completed() - prev_completions - 1) * tween_duration; |
There was a problem hiding this comment.
Note that this seems to attempt to handle a case that is currently not supported, namely having looping tweenables as part of a Sequence. Currently full_completions will always be zero. Also, if a tweenable is looping, it currently never return TweenState::Completed since looping tweenables are of infinite duration by definition.
2 participants
What's broken?