Clean up code to find the current keyframe #11306
Conversation
alice-i-cecile
added
C-Code-Quality
|
refactoring this is a very good opportunity to add tests 🙂 |
|
I've added a solid battery of tests now. Both the original code and the refactor versions pass all 6 of them: I am now confident that there is no change in behavior and that the behavior is correct. See c9f4f9f for the port of the original algorithm: I deliberately committed and then reverted this to make it easier for reviewers to verify. |
alice-i-cecile
added
the
C-Usability
|
Basic perf testing reveals no major regressions: I'm getting 2.2 ms frame times on the I don't think we can get a better answer than that without some actual benchmarks (and better methods in general).
From @mockersf: yellow is this PR, red is main. |
| pub fn len(&self) -> usize { | ||
| match self { | ||
| Keyframes::Weights(vec) => vec.len(), | ||
| Keyframes::Translation(vec) | Keyframes::Scale(vec) => vec.len(), |
There was a problem hiding this comment.
Why are these two on the same line while the others are split up?
| Keyframes::Translation(vec) | Keyframes::Scale(vec) => vec.len(), | |
| Keyframes::Translation(vec) => vec.len(), | |
| Keyframes::Scale(vec) => vec.len(), |
There was a problem hiding this comment.
because vec is the same type for translation and scale, but not for weights and rotation
There was a problem hiding this comment.
Yep I would do all 4 on one line but it didn't compile.
| // PERF: finding the current keyframe can be optimised | ||
| let search_result = self | ||
| .keyframe_timestamps | ||
| .binary_search_by(|probe| probe.partial_cmp(&seek_time).unwrap()); |
There was a problem hiding this comment.
im willing to bet 2 cents and a teaspoon of maple syrup that the extremely minor perf regression is thanks to the unwrap check, and something as smoothbrained and inelegant as
|probe| if probe < &seek_time { Ordering::Less } else if probe == &seek_time { Ordering::Equal } else { Ordering::Greater }
would fix it
There was a problem hiding this comment.
I tried comparing with tracy, the manual compare is a tiny bit worse than the unwrap.
I would love that teaspoon of maple syrup!
There was a problem hiding this comment.
alright come over, im in berkeley. i'll even make you a pancake to put it on
| ); | ||
| } | ||
| if animation.path_cache.len() != animation_clip.paths.len() { | ||
| animation.path_cache = vec![Vec::new(); animation_clip.paths.len()]; |
There was a problem hiding this comment.
do we need to make a whole new vec or can we just add/remove elements until the len is the same?
There was a problem hiding this comment.
It's possible to do a clear() followed by an extend(…). The Vec::new here doesn't cause an allocation until something is pushed to it.
mockersf
added
the
S-Ready-For-Final-Review
Objective
While working on #10832, I found this code very dense and hard to understand.
I was not confident in my fix (or the correctness of the existing code).
Solution
Clean up, test and document the code used in the
apply_animationsystem.I also added a pair of length-related utility methods to
Keyframesfor easier testing. They seemed generically useful, so I made them pub.Changelog
VariableCurve::find_current_keyframemethod.Keyframes::lenandis_emptymethods.