Closed Bug 1336668 Opened 3 years ago Closed 3 years ago

stylo: implement discrete type animation

Categories

(Core :: DOM: Animation, defect, P2)

defect

Tracking

()

RESOLVED FIXED
Tracking Status
firefox54 --- affected

People

(Reporter: hiro, Assigned: hiro)

References

Details

Attachments

(3 files, 2 obsolete files)

No description provided.
Priority: -- → P2
No longer blocks: 1324696
See Also: → 1326131
See Also: 1326131
For stylo, to make a property animatable we need to implement clone_xx() and Intepolate trait for the property.  But with this patch, we don't need to implement Interpolate trait respectively.

So rest of work should be easy.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #1)
> Created attachment 8854308 [details] [diff] [review]
> Implement discrete type animations (just implemented align-items)
> 
> For stylo, to make a property animatable we need to implement clone_xx() and
> Intepolate trait for the property.  But with this patch, we don't need to
> implement Interpolate trait respectively.

To be precise, we don't need to implement Interpolate trait for discrete type animation properties.
I did forget to set animatable flag True, and the previous patch could not be compiled.
Attachment #8854308 - Attachment is obsolete: true
Blocks: 1353918
In attachment 8854366 [details] [diff] [review], I added a new flag to represent that the property is animatable as discrete, but after some thought 
I think it would be better to make current 'animatable' flag tri-state. E.g.

animatable=None
animatable="Normal"
animatable="Discrete"

But "Normal" is somewhat blurry. Brian, any ideas?

Note for logical properties, there is already a flag for it, so we can distinguish it.

https://hg.mozilla.org/mozilla-central/file/867df9483d5a/servo/components/style/properties/data.py#l99
Why tri-state? Why isn't discrete just another type of animation?
(In reply to Brian Birtles (:birtles) from comment #5)
> Why tri-state? Why isn't discrete just another type of animation?

In Servo, we don't have animation-type flag for each property just like Gecko, Servo just implements Interpolate trait for each *type*, e.g. 'impl Interpolate for LengthOrPercentage', etc.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #6)
> (In reply to Brian Birtles (:birtles) from comment #5)
> > Why tri-state? Why isn't discrete just another type of animation?
> 
> In Servo, we don't have animation-type flag for each property just like
> Gecko, Servo just implements Interpolate trait for each *type*, e.g. 'impl
> Interpolate for LengthOrPercentage', etc.

But for discrete type properties, we don't need to implement Interpolate at all, i.e skip calling Interpolate::interpolate() method respectively.  This is my intention in attachment 8854366 [details] [diff] [review].
(In reply to Hiroyuki Ikezoe (:hiro) from comment #6)
> (In reply to Brian Birtles (:birtles) from comment #5)
> > Why tri-state? Why isn't discrete just another type of animation?
> 
> In Servo, we don't have animation-type flag for each property just like
> Gecko, Servo just implements Interpolate trait for each *type*, e.g. 'impl
> Interpolate for LengthOrPercentage', etc.

So Servo conflates the CSS type with animation type. I think we'll need to fix that at some point (in effect, that's what you're proposing, just on a limited scale) but for now it's probably ok.
(In reply to Brian Birtles (:birtles) from comment #8)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #6)
> > (In reply to Brian Birtles (:birtles) from comment #5)
> > > Why tri-state? Why isn't discrete just another type of animation?
> > 
> > In Servo, we don't have animation-type flag for each property just like
> > Gecko, Servo just implements Interpolate trait for each *type*, e.g. 'impl
> > Interpolate for LengthOrPercentage', etc.
> 
> So Servo conflates the CSS type with animation type. I think we'll need to
> fix that at some point (in effect, that's what you're proposing, just on a
> limited scale) but for now it's probably ok.

Thanks, then I will rename 'animatable' variable to 'animation-type' in this bug, and just make it represent None, 'Normal' or 'Discrete' for now.  Later we will add more types there.
Blocks: 1353966
Attachment #8854366 - Attachment is obsolete: true
Comment on attachment 8855177 [details]
Bug 1336668 - Rename animatable to animation_type.

https://reviewboard.mozilla.org/r/127062/#review129864

::: servo/components/style/properties/data.py:138
(Diff revision 1)
>          self.allowed_in_keyframe_block = allowed_in_keyframe_block \
>              and allowed_in_keyframe_block != "False"
>  
>          # This is done like this since just a plain bool argument seemed like
>          # really random.
> -        if animatable is None:
> +        if animation_type is None:

Let's ensure here that animation_type is one of the expected values.
Attachment #8855177 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8855178 [details]
Bug 1336668 - Add a tweak to avoid calling interpolate() if animation_type is discrete.

https://reviewboard.mozilla.org/r/127064/#review129866

::: commit-message-c04df:1
(Diff revision 1)
> +Bug 1336668 - Add a tweak to avoid calling interpolte() if animation_type is discrete. r?emilio

nit: Typo in the commit message (`interpolate` instead of `interpolte`).

::: commit-message-c04df:6
(Diff revision 1)
> +Bug 1336668 - Add a tweak to avoid calling interpolte() if animation_type is discrete. r?emilio
> +
> +For discrete type of animations, we just need to return 'from' value if
> +progress is less than 0.5 and otherwise return 'to' value.
> +
> +https://w3c.github.io/web-animations/#discrete-animation-type

Let's add a link to the spec also in the code (as a comment near the `if prop.animation_type == "discrete"`.

::: servo/components/style/properties/helpers/animated_properties.mako.rs:222
(Diff revision 1)
>      /// animation at `progress`.
>      pub fn update(&self, style: &mut ComputedValues, progress: f64) {
>          match *self {
>              % for prop in data.longhands:
>                  % if prop.animatable:
> +                    % if prop.animation_type == "discrete":

nit: Let's move the `if prop.animation_type` inside the `AnimatedProperty::..` branch:

```
AnimatedProperty::${prop.camel_case}(ref from, ref to) => {
    % if prop.animation_type == "discrete":
         let value = ...;
    % else:
         if let Ok(value) = ...
    % endif
}
```

Also, we could share the mutate call, like so:

```
% if prop.animation_type == "discrete":
    let value = ...;
% else:
    let value = match from.interpolate(to, progress) {
        Ok(value) => value,
        Err(()) => return,
    };
% endif
    style.mutate_${prop...}.set_${prop.ident}(value);
```

wdyt?
Attachment #8855178 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8855178 [details]
Bug 1336668 - Add a tweak to avoid calling interpolate() if animation_type is discrete.

https://reviewboard.mozilla.org/r/127064/#review129868

::: servo/components/style/properties/helpers/animated_properties.mako.rs:404
(Diff revision 1)
>      fn interpolate(&self, other: &Self, progress: f64) -> Result<Self, ()> {
>          match (self, other) {
>              % for prop in data.longhands:
>                  % if prop.animatable:
> +                    % if prop.animation_type == "discrete":
> +                        (&AnimationValue::${prop.camel_case}(ref from),

Oh, also, same recommendation about moving the if inside the branches here.
Comment on attachment 8855179 [details]
Bug 1336668 - Make align-items animatable as discrete type.

https://reviewboard.mozilla.org/r/127066/#review129870

Looks good, is this tested?
Attachment #8855179 - Flags: review?(emilio+bugs) → review+
(In reply to Emilio Cobos Álvarez [:emilio] from comment #14)
> ::: servo/components/style/properties/helpers/animated_properties.mako.rs:222
> (Diff revision 1)
> >      /// animation at `progress`.
> >      pub fn update(&self, style: &mut ComputedValues, progress: f64) {
> >          match *self {
> >              % for prop in data.longhands:
> >                  % if prop.animatable:
> > +                    % if prop.animation_type == "discrete":
> 
> nit: Let's move the `if prop.animation_type` inside the
> `AnimatedProperty::..` branch:
> 
> ```
> AnimatedProperty::${prop.camel_case}(ref from, ref to) => {
>     % if prop.animation_type == "discrete":
>          let value = ...;
>     % else:
>          if let Ok(value) = ...
>     % endif
> }
> ```
> 
> Also, we could share the mutate call, like so:
> 
> ```
> % if prop.animation_type == "discrete":
>     let value = ...;
> % else:
>     let value = match from.interpolate(to, progress) {
>         Ok(value) => value,
>         Err(()) => return,
>     };
> % endif
>     style.mutate_${prop...}.set_${prop.ident}(value);
> ```


Neat! Thanks! I will do it.
(In reply to Emilio Cobos Álvarez [:emilio] from comment #16)
> Comment on attachment 8855179 [details]
> Bug 1336668 - Make align-items animatable as discrete type.
> 
> https://reviewboard.mozilla.org/r/127066/#review129870
> 
> Looks good, is this tested?

We have a test case in web-platform-tests.
https://hg.mozilla.org/mozilla-central/file/3c68d659c2b7/testing/web-platform/tests/web-animations/animation-model/animation-types/property-list.js#l10
Comment on attachment 8855177 [details]
Bug 1336668 - Rename animatable to animation_type.

https://reviewboard.mozilla.org/r/127062/#review130100

Looks good.

("normal" seems not to explain what is happening, however. But "interpolable" doesn't seem right either since, for example, "visibility" is neither "discrete" nor "interpolable". So let's just stick with "normal" for now since I don't have any better suggestions.)
Attachment #8855177 - Flags: review?(bbirtles) → review+
Thanks for the review!

https://github.com/servo/servo/pull/16292
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
https://hg.mozilla.org/integration/autoland/rev/3315299058de
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Duplicate of this bug: 1356071
You need to log in before you can comment on or make changes to this bug.