Refactor cc/animation code to track existence of elements for each needed property node type. |
|||||
Issue description
Consider an animation on a single element:
<div id="target" style="animation: transform-opacity-filter"></div>
Before BlinkGenPropertyTrees, a composited animation of one element would create a composited layer for that element which would create a property tree node in the effect tree and transform tree with that element's corresponding CompositorElementId so that the composited transform, filter, and opacity could be mutated for that element. e.g.
#target gets UniqueId 9, shifted left 4 and combined with namespace kPrimary (0) to make CompositorElementId 144:
TransformTree
- TransformNode #144 (9 << 4 | kPrimary)
EffectTree
- EffectNode #144 (9 << 4 | kPrimary)
The ElementAnimations code then sets a single ElementId to look for to know when we can start this animation on the compositor. It assumes that once the layer with this ElementId shows up that we must have also created the transform and effect nodes for that id.
After BlinkGenPropertyTrees, we have the ability to only create the nodes we need, and actually create separate effect nodes for the filter from the opacity for the same element. Each of these nodes get a different CompositorElementId using a combination of the unique id and a CompositorElementIdNamespace.
#target gets UniqueId 9, and creates a node for each animated property:
TransformTree
- TransformNode #148 == (9 << 4 | kPrimaryTransform)
EffectTree
- EffectNode #149 == (9 << 4 | kPrimaryEffect)
- EffectNode #150 == (9 << 4 | kEffectFilter)
We have a 1:1 relation of cc::KeyframeEffect to cc::ElementAnimations, but each KeyframeModel on that KeyframeEffect targets a different ElementId - there is no longer a single element id to run the animation on.
In issue 896549 we added separate tracking of the ElementId to use for each KeyframeModel, however, we generate every possible node (Primary transform, primary effect, effect filter) and mark that the animation can start when the primary effect node shows up.
We should refactor the cc/animation code to actually track the existence of elements for each of the KeyframeModels and base starting / stopping the animation on whether all of the required KeyframeModels have been created. Once this is done, we can modify PaintPropertyTreeBuilder to only create nodes currently being animated.
,
Dec 14
While I was trying to make ElementIds mandatory on KeyframeModels I discovered that it seems like VR animations might not use ElementIds. This got me to thinking that we could keep animations classes purely concerned with animations by using a map / array from target property to elementid on the ElementAnimations class instead. I also suspect this would be a more natural transition path for the code, keeping ElementAnimations actually concerned with a single animation target (a blink Element)
,
Dec 14
Since this would essentially reverse map the Element Id namespacing, perhaps we should reconsider letting cc::ElementAnimations know the namespacing scheme for ElementIds so that it could directly convert them without needing to store a list of them per element. +chrishtr, pdr, thoughts?
,
Dec 18
> I also suspect this would be a more natural transition path for the code, keeping ElementAnimations actually concerned with a single animation target (a blink Element) Does cc::Animations actually have to care about the blink Element as a target? It seems natural that the CSS Animation in #1 actually has three targets: one TransformNode and two EffectNodes. There is obviously the grouping concern, but there appears to already be KeyframeModel::group_ as a grouping mechanism? I.e. what is the actual use of ElementAnimations? +yigu, majidvp because I think their knowledge of cc Animations and their insights will be useful here.
,
Dec 19
> While I was trying to make ElementIds mandatory on KeyframeModels I discovered that it seems like VR animations might not use ElementIds. This got me to thinking that we could keep animations classes purely concerned with animations by using a map / array from target property to elementid on the ElementAnimations class instead. Note (since I just learned this): VR doesn't use ElementAnimations either. They only use KeyframeModels, really. They have their own AnimationTarget subclass. So we can essentially disregard them entirely when considering ElementAnimation changes. This is likely what Rob meant by the above comment, but it took this discovery for me to understand the ramifications.
,
Jan 9
Post https://chromium-review.googlesource.com/c/chromium/src/+/1372173 ( https://crrev.com/b0eab4a3e ) ElementAnimations now (virtually) has a map from PropertyId -> ElementId
,
Jan 15
Updating the bug name to more accurately reflect the problem that needs to be solved rather than imply a solution. I am strongly leaning towards putting the complexity of target -> bundle of element ids into the ElementAnimations class (whether that is by it knowing the namespacing scheme or tracking all the element ids) as it keeps the cc/ animation ecosystem free from tracking blink-specific things and preserves the mapping of animations to targets, and VR animations can happily ignore Elements. I think we should let ElementAnimations know about the ElementId namespacing scheme since it corresponds to web elements and would essentially be replicating it anyways at a greater hit to memory otherwise. |
|||||
►
Sign in to add a comment |
|||||
Comment 1 by petermayo@chromium.org
, Dec 12