Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upImprovements to animation keyframe computation #26562
Merged
Conversation
highfive
commented
May 18, 2020
|
Heads up! This PR modifies the following files:
|
highfive
commented
May 18, 2020
|
|
|
@bors-servo try=wpt |
bors-servo
added a commit
that referenced
this pull request
May 18, 2020
Cache animation computed values when animations change Instead of recalculating the animation style every tick of an animation, cache the computed values when animations change. In addition to being more efficient, this will allow us to return animation rules as property declarations because we don't need to consult the final style to produce them. --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: --> - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes do not require tests because they should not change behavior. <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
|
|
|
|
mrobinson
changed the title
|
I've added a closely related change to this PR which improves the computation of keyframe data and ditches Servo's use of |
mrobinson
changed the title
|
This needs a little bit more work. |
|
I read through 9173503 and it looks sensible. |
|
@jdm Thanks for looking at the first commit. That should be unchanged in the newest version of the branch. The second commit is unfortunately a bit large, which means it's probably harder to review. I wasn't sure how to split it into simpler chunks. |
|
@bors-servo try=wpt |
bors-servo
added a commit
that referenced
this pull request
May 30, 2020
[WIP] Improvements to animation keyframe computation This pull request contains two changes: **Caching computed keyframes between animation changes**: Instead of recomputing keyframe data for every tick of an animation, cache the computed values when animations change. In addition to being more efficient, this will allow us to return animation rules as property declarations because we don't need to consult the final style to produce them. **Better computation of keyframe data**: Instead of naively using `apply_declarations` to compute the style of each keyframe, use an approach more like Gecko's where we carefully walk through the animation keyframes and try to extract `AnimationValue`s for each computed keyframe. In this approach we respect the order with which the properties are set in the keyframe source. --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: --> - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes do not require tests because they should not change behavior. <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
mrobinson
changed the title
|
|
|
@bors-servo try=wpt |
|
@bors-servo try=wpt-2020 |
|
@bors-servo retry |
bors-servo
added a commit
that referenced
this pull request
Jun 1, 2020
Improvements to animation keyframe computation This pull request contains two changes: **Caching computed keyframes between animation changes**: Instead of recomputing keyframe data for every tick of an animation, cache the computed values when animations change. In addition to being more efficient, this will allow us to return animation rules as property declarations because we don't need to consult the final style to produce them. **Better computation of keyframe data**: Instead of naively using `apply_declarations` to compute the style of each keyframe, use an approach more like Gecko's where we carefully walk through the animation keyframes and try to extract `AnimationValue`s for each computed keyframe. In this approach we respect the order with which the properties are set in the keyframe source and try to deal with CSS custom properties. --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: --> - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] There are tests for these changes. <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
|
|
|
These changes look sensible to me. |
components/style/animation.rs
Outdated
| for (index, from) in prev_keyframe.values.iter().enumerate() { | ||
| PropertyAnimation { | ||
| from, | ||
| to, | ||
| from: from.clone(), | ||
| to: next_keyframe.values[index].clone(), |
Comment on lines
712
to
715
This comment has been minimized.
This comment has been minimized.
jdm
Jun 4, 2020
Member
Is there any benefit to writing this as:
for (from, to) in prev_keyframe.values.iter().zip(next_keyframe.values.iter()) {
PropertyAnimation {
from: from.clone(),
to: to.clone(),
...
}
...
}
This comment has been minimized.
This comment has been minimized.
jdm
added
S-needs-code-changes
S-needs-rebase
and removed
S-awaiting-review
S-needs-rebase
labels
|
Yeah, nice progression :) |
components/style/animation.rs
Outdated
| self.timing_function = Some(timing_function.to_computed_value_without_context()); | ||
| } | ||
|
|
||
| match step.value { |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
components/style/animation.rs
Outdated
| &mut self, | ||
| step: &KeyframesStep, | ||
| context: &SharedStyleContext, | ||
| base_style: &Arc<ComputedValues>, |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Instead of recalculating the animation style every tick of an animation, cache the computed values when animations change. In addition to being more efficient, this will allow us to return animation rules as property declarations because we don't need to consult the final style to produce them.
This begins to address #26625 by properly applying CSS variables during keyframe computation and no longer using `apply_declarations`. Instead, walk the declarations, combining them into IntermediateComputedKeyframe, maintaining declarations that modify CSS custom properties. Then compute a set of AnimationValues for each keyframe and use those to produce interpolated animation values.
|
@bors-servo r=jdm,emilio Thank you both for the reviews! |
|
|
|
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
mrobinson commentedMay 18, 2020
•
edited
This pull request contains two changes:
Caching computed keyframes between animation changes:
Instead of recomputing keyframe data for every tick of an animation,
cache the computed values when animations change. In addition to being
more efficient, this will allow us to return animation rules as property
declarations because we don't need to consult the final style to produce
them.
Better computation of keyframe data:
Instead of naively using
apply_declarationsto compute the style ofeach keyframe, use an approach more like Gecko's where we carefully
walk through the animation keyframes and try to extract
AnimationValuesfor each computed keyframe. In this approach we respect the order with
which the properties are set in the keyframe source and try to deal with
CSS custom properties.
./mach build -ddoes not report any errors./mach test-tidydoes not report any errors