Fix CSS step timing functions jump-both, jump-start, start at timestamp 0; Early return when possible#342
Fix CSS step timing functions jump-both, jump-start, start at timestamp 0; Early return when possible#342
jump-both, jump-start, start at timestamp 0; Early return when possible#342Conversation
style/servo/animation.rs
Outdated
| .iter() | ||
| .rev() | ||
| .position(|step| total_progress as f32 <= 1. - step.start_percentage) | ||
| .position(|step| (total_progress as f32) <= 1. - step.start_percentage) |
There was a problem hiding this comment.
Actually we shouldn't. Otherwise for reversed direction at timestamp 0, we jump while we shouldn't for jump-start, jump-both, start.
But there's more severe problem I've been trying to fix based on horrible test results..
72f666e to
e9c20d2
Compare
| // At progress 0, we need to handle step functions specially | ||
| // for "jump-both, jump-start, start" step functions. | ||
| if total_progress == 0.0 { | ||
| if let TimingFunction::Steps(_steps, pos) = &prev_keyframe.timing_function { |
There was a problem hiding this comment.
I tend to think this seems the wrong place to handle timing functions. Shouldn't it be handled in calculate_step_output()?
There was a problem hiding this comment.
It is handled there. But previously never get the chance to reach there (L723) due to early return.
The test result looks decent.
There's only one failure case that I hope to check tomorrow.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Agreed to Oriol. Btw I also find PropertyAnimation below weird, as it's resolving the timing function for every property. I imagine a better process would be:
- Use
timing_function.calculate_output()to resolve the progress to aresolved_progress, which is number in [0, 1] - Use
prev_keyframeifresolved_progressis 0; Usenext_keyframeisresolved_progressis 1 - Otherwise, calculate a linear interpolation between the two keyframes at
resolved_progress
In this way we don't need handle special handling of any timing function here.
There was a problem hiding this comment.
Shouldn't it be handled in calculate_step_output()?
fn calculate_step_output() is shared with Firefox. But Firefox doesn't have the problem we have here :/
This PR only changes Servo specific code.
e9c20d2 to
82c89aa
Compare
9a19a15 to
61e8a21
Compare
61e8a21 to
ecb0a95
Compare
jump-both, jump-start, start at timestamp 0
jump-both, jump-start, start at timestamp 0jump-both, jump-start, start at timestamp 0; Early return when possible
yezhizhen
left a comment
There was a problem hiding this comment.
We also made early return happen earlier in various cases, instead of proceed to interpolation.
See "Make early return happen earlier" in description.
Signed-off-by: Euclid Ye <yezhizhenjiakang@gmail.com>
| Some(index) => &self.computed_steps[index], | ||
| None => return, | ||
| None => { | ||
| debug_assert!(false, "next_keyframe_index should always be Some"); |
There was a problem hiding this comment.
This is because:
- We always have steps at 0% and 100%
stylo/style/servo/animation.rs
Lines 174 to 175 in a4f1d69
- This always exists for
total_progressin [0,1), no matter reverse or normal.
style/servo/animation.rs
Outdated
| // Detect zero interval. Prevent division by zero from percentage_between_keyframes. | ||
| if Some(prev_keyframe_index) == next_keyframe_index { |
There was a problem hiding this comment.
The % between two keyframes should never be zero, because if it was then IntermediateComputedKeyframe::generate_from_keyframes should have coalesced those two frames.
If you're seeing two keyframes with the same % then I suspect the bug is in that function and not here.
There was a problem hiding this comment.
It is not a bug of that function, but the result of index computation.
For example, reverse direction at total_progress == 0.0 can trigger the equality.
Signed-off-by: Euclid Ye <euclid.ye@huawei.com>
08c6e3f to
1a41cbe
Compare
jump-both,jump-start,startat timestamp 0; Early return when possible #342 (comment)) and added direction‑aware step‑function handling.Make early return happen earlier:
percentage_between_keyframesTry
Servo PR: servo/servo#43594