1332633 - stylo: Implement ComputeDistance for AnimationValues

Status: RESOLVED FIXED

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

Description

[Boris Chiou [:boris]]

We use StyleAnimationValue::ComputeDistance() for a Web Animation API, spacing [1], so we can do something like Interpolate in animated_properties.mako.rs: implement a ComputedDistance trait for AnimationValue, and add a FFI, so we can use it from Gecko.

However, I think we might not need to implement ComputeDistance for Shape, Filter, and Transform because we don't have the official spec for them now.
* CSS Shape: https://github.com/w3c/csswg-drafts/issues/662
* CSS Filter: https://github.com/w3c/fxtf-drafts/issues/91
* CSS Transform: Bug 1318591

Besides, nsDOMWindowUtils::ComputeAniamtionDistance() also uses it [3].

[1] http://searchfox.org/mozilla-central/rev/30fcf167af036aeddf322de44a2fadd370acfd2f/dom/animation/KeyframeUtils.cpp#1778,1793
[2] http://searchfox.org/mozilla-central/rev/30fcf167af036aeddf322de44a2fadd370acfd2f/layout/style/StyleAnimationValue.cpp#1808,1814,1819
[3] http://searchfox.org/mozilla-central/rev/30fcf167af036aeddf322de44a2fadd370acfd2f/dom/base/nsDOMWindowUtils.cpp#2725

Comment 1

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Making this P3 for now, even though we have discussed possibly dropping paced animation. Even if we do that, we'll still need this for SMIL animation I think.

Comment 2

[Boris Chiou [:boris]]

This blocks a P2 bug (i.e. SMIL animation), so promote to P2.

Comment 3

[Boris Chiou [:boris]]

Attachment: Part 1: Implement ComputeDistance trait.

MozReview-Commit-ID: LmmrUXYlDb6

Comment 4

[Boris Chiou [:boris]]

Attachment: Part 2: Implement FFI for compute distance.

MozReview-Commit-ID: G3aYICDnCLA

Comment 5

[Boris Chiou [:boris]]

Attachment: Part 3: ComputeAnimationDistance for stylo.

MozReview-Commit-ID: CX8wairpnfN

Comment 6

[Boris Chiou [:boris]]

Work on this now.

Comment 7

[Boris Chiou [:boris]]

Attachment: Bug 1332633 - Part 1: Implement ComputeDistance trait.

Introduce ComputeDistance trait and ComputeSquaredDistance trait.

For vector, ComputeSquaredDistance is necessary because we use Euclidean
distance as the distance between two values. The easier way to implement
ComputeSquaredDistance is to square the result from ComputeDistance, but
for some property values, they may have many components, e.g. (v1, v2, v3).
If we just square the result from ComputeDistance, the computation is
(sqrt(v1^2 + v2^2 + v3^2))^2. There are two redundant operators:
"square-root" and then "square". In order to avoid this, we implement
ComputeSquaredDistance by computing the sum of squares directly.

Review commit: https://reviewboard.mozilla.org/r/123418/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/123418/

Comment 8

[Boris Chiou [:boris]]

Attachment: Bug 1332633 - Add Servo_AnimationValues_ComputeDistance.

This FFI will be used by nsDOMWindowUtils::ComputeAnimationDistance,
which will be implemented in another bug.

Review commit: https://reviewboard.mozilla.org/r/123420/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/123420/

Comment 9

[Boris Chiou [:boris]]

Comment on attachment 8859634 [details]
Bug 1332633 - Part 1: Implement ComputeDistance trait.

https://reviewboard.mozilla.org/r/123418/#review134396

::: servo/components/style/properties/helpers/animated_properties.mako.rs:1979
(Diff revision 1)
>          }
>      }
>  }
> +
> +
> +% if product == "gecko":

I'm not sure whether for gecko only or for both servo and gecko. If you think this is ok for servo, I will remove this.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8859634 [details]
Bug 1332633 - Part 1: Implement ComputeDistance trait.

https://reviewboard.mozilla.org/r/123418/#review134396

> I'm not sure whether for gecko only or for both servo and gecko. If you think this is ok for servo, I will remove this.

Yeah, seems harmless for servo, so let's compile it for both, if only because people working on servo won't accidentally break it and then realizing they need to fix this when CI runs.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8859634 [details]
Bug 1332633 - Part 1: Implement ComputeDistance trait.

https://reviewboard.mozilla.org/r/123418/#review134480

::: servo/components/style/properties/helpers/animated_properties.mako.rs:2017
(Diff revision 1)
> +/// In order to compute the Euclidean distance of a list, we need to compute squared distance
> +/// for each element, so the vector can sum it and then get its squared root as the distance.
> +pub trait ComputeSquaredDistance: Sized {
> +    /// Compute squared distance between a value and another for a given property.
> +    /// This is used for list or if there are many components in a property value.
> +    fn compute_squared_distance(&self, other: &Self) -> Result<f64, ()>;

One thought, without digging on the patch yet since I have to run now. Why not doing:

```
pub trait ComputeDistance: Sized {
    fn compute_distance(&self, other: &Self) -> Result<f64, ()>;
    
    fn compute_squared_distance(&self, other: &self) -> Result<f64, ()> {
        self.compute_distance(other).map(|d| d * d)
    }
}
```

Or are we doing something smarter than that? (as I said, still haven't dug on the patch)

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8859634 [details]
Bug 1332633 - Part 1: Implement ComputeDistance trait.

https://reviewboard.mozilla.org/r/123418/#review134548

::: servo/components/style/properties/helpers/animated_properties.mako.rs:1984
(Diff revision 1)
> +            % for prop in data.longhands:
> +                % if prop.animation_type == "normal":
> +                    (&AnimationValue::${prop.camel_case}(ref from),

As we talked on IRC, would you mind checking animatable instead of animation_type?

Comment 13

[Boris Chiou [:boris]]

Comment on attachment 8859634 [details]
Bug 1332633 - Part 1: Implement ComputeDistance trait.

https://reviewboard.mozilla.org/r/123418/#review134548

> As we talked on IRC, would you mind checking animatable instead of animation_type?

Sure. This is my earlier version, so I haven't revise this part.

Comment 14

[Boris Chiou [:boris]]

Comment on attachment 8859634 [details]
Bug 1332633 - Part 1: Implement ComputeDistance trait.

https://reviewboard.mozilla.org/r/123418/#review134480

> One thought, without digging on the patch yet since I have to run now. Why not doing:
> 
> ```
> pub trait ComputeDistance: Sized {
>     fn compute_distance(&self, other: &Self) -> Result<f64, ()>;
>     
>     fn compute_squared_distance(&self, other: &self) -> Result<f64, ()> {
>         self.compute_distance(other).map(|d| d * d)
>     }
> }
> ```
> 
> Or are we doing something smarter than that? (as I said, still haven't dug on the patch)

For compute_squared_distance, there are a case I want to avoid:

For example, transform-origin type: (horizonal + vertical + depth)
The distance is:         ```sqrt(horizonal_diff^2 + vertical_diff^2 + depth_diff^2)```
The squared distance is: ```horizonal_diff^2 + vertical_diff^2 + depth_diff^2```

I don't want to get square-root and then sqaure it for squared distance, so we should implement compute_squared_distance by different ways for some types. However, put them together is fine to me, I will combine these two traits.

Comment 15

[Boris Chiou [:boris]]

Comment on attachment 8859634 [details]
Bug 1332633 - Part 1: Implement ComputeDistance trait.

https://reviewboard.mozilla.org/r/123418/#review134396

> Yeah, seems harmless for servo, so let's compile it for both, if only because people working on servo won't accidentally break it and then realizing they need to fix this when CI runs.

OK. I will remove this

Comment 16

[Boris Chiou [:boris]]

Comment on attachment 8859634 [details]
Bug 1332633 - Part 1: Implement ComputeDistance trait.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123418/diff/1-2/

Comment 17

[Boris Chiou [:boris]]

Comment on attachment 8859635 [details]
Bug 1332633 - Add Servo_AnimationValues_ComputeDistance.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123420/diff/1-2/

Comment 18

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8859634 [details]
Bug 1332633 - Part 1: Implement ComputeDistance trait.

https://reviewboard.mozilla.org/r/123418/#review134824

Looks great!

I reviewed high-level top-to-bottom, and then looked into the details while I went up, so the comments are probably in the wrong order, but r=me with those addressed.

::: servo/components/style/properties/helpers.mako.rs:831
(Diff revision 2)
> +            match (self, other) {
> +                (&T(Some(ref this)), &T(Some(ref other))) => {
> +                    this.compute_distance(other)
> +                },
> +                (&T(Some(ref this)), &T(None)) => {
> +                    this.compute_distance(&${value_for_none})

And here.

::: servo/components/style/properties/helpers.mako.rs:848
(Diff revision 2)
> +        fn compute_squared_distance(&self, other: &Self) -> Result<f64, ()> {
> +            match (self, other) {
> +                (&T(Some(ref this)), &T(Some(ref other))) => {
> +                    this.compute_squared_distance(other)
> +                },
> +                (&T(Some(ref this)), &T(None)) => {

You could also merge branches here.

::: servo/components/style/properties/helpers/animated_properties.mako.rs:2483
(Diff revision 2)
> +        for i in 0..max_len {
> +            diff_squared += match (self.0.get(i), other.0.get(i)) {
> +                (Some(shadow), Some(other)) => {
> +                    try!(shadow.compute_squared_distance(other))
> +                },
> +                (Some(shadow), None) => {

ditto

::: servo/components/style/properties/helpers/animated_properties.mako.rs:2541
(Diff revision 2)
> +        for i in 0..max_len {
> +            diff_squared += match (self.0.get(i), other.0.get(i)) {
> +                (Some(shadow), Some(other)) => {
> +                    try!(shadow.compute_squared_distance(other))
> +                },
> +                (Some(shadow), None) => {

Since the squared distance should always be positive, I believe you could merge this and the following branch:

```
(Some(shadow), None) |
(None, Some(shadow)) => {
    zero.inset = shadow.inset;
    try!(zero.compute_squared_distance(shadow))
}
```

Comment 19

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8859635 [details]
Bug 1332633 - Add Servo_AnimationValues_ComputeDistance.

https://reviewboard.mozilla.org/r/123420/#review134826

::: servo/ports/geckolib/glue.rs:275
(Diff revision 2)
> +pub extern "C" fn Servo_AnimationValues_ComputeDistance(from: RawServoAnimationValueBorrowed,
> +                                                        to: RawServoAnimationValueBorrowed)
> +                                                        -> f64 {
> +    let from_value = AnimationValue::as_arc(&from);
> +    let to_value = AnimationValue::as_arc(&to);
> +    match from_value.compute_distance(to_value) {

nit: you can use `from_value.compute_distance(to_value).unwrap_or(0.0)`.

Comment 20

[Boris Chiou [:boris]]

Comment on attachment 8859635 [details]
Bug 1332633 - Add Servo_AnimationValues_ComputeDistance.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123420/diff/2-3/

Comment 21

[Boris Chiou [:boris]]

Comment on attachment 8859635 [details]
Bug 1332633 - Add Servo_AnimationValues_ComputeDistance.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123420/diff/3-4/

Comment 22

[Boris Chiou [:boris]]

Attachment: Servo PR, #16554

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f902d870c642
Add Servo_AnimationValues_ComputeDistance. r=emilio

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f902d870c642