Boris Chiou [:boris] Reporter
	Description • 8 years ago

We call into Servo code to interpolate RawServoAnimationValues to create new value in Bug 1317209. However, we haven't implement Accumulation and Addition on RawServoAnimationValues, so we should implement them in Servo code and then call from Gecko.

	Hiroyuki Ikezoe (:hiro)
	Comment 1 • 8 years ago

I guess, at that time, we can do a bunch of stuff in current KeyframeEffectReadOnly::ComposeStyle() in servo side and just invoke an FFI function which does the bunch of stuff.  I might be missing something though.

	Boris Chiou [:boris] Reporter
	Comment 2 • 8 years ago

(In reply to Hiroyuki Ikezoe (:hiro) from comment #1)
> I guess, at that time, we can do a bunch of stuff in current
> KeyframeEffectReadOnly::ComposeStyle() in servo side and just invoke an FFI
> function which does the bunch of stuff.  I might be missing something though.

Yes, that's what I think. Bug 1317209 only handles the interpolation, so I'd like to leave this bug number and the TODO comment in ComposeStyle.

	Brian Birtles (:birtles) Assignee
	Comment 3 • 7 years ago

I started looking into this (and accumulate too since it probably makes sense to do these together).

Currently I'm trying to determine if it makes sense use the same "add_weighted" approach we have in StyleAnimationValue or not. I find the "add weighted" code to be hard to read, and doing it that way might make it more difficult to mark selected properties as non-additive (assuming we actually need to do that), but it might mean less code to maintain overall. I'm going to try with separate add / accumulate / get_zero_value methods for now and see how it looks.

	Brian Birtles (:birtles) Assignee
	Comment 4 • 7 years ago

Originally I was thinking of adding something like:

pub trait Addition: Sized {
    /// Returns the [sum][animation-addition] of this value and |other|.
    ///
    /// "For animation types that do not define a specific procedure for addition or which are
    /// defined as not additive, the addition procedure is simply Vres = Vb."
    ///
    /// [animation-addition]: https://w3c.github.io/web-animations/#animation-addition
    fn add(&self, other: &Self) -> Self { other }

    /// Returns the result of combining |other| with this value such that |other| is treated as
    /// a delta from this value. See
    /// [accumulation][https://w3c.github.io/web-animations/#animation-accumulation].
    ///
    /// For animation types that do not define a specific procedure for accumulation, the
    /// accumulation procedure is identical to the addition procedure for that type.
    fn accumulate(&self, other: &Self, count: u64) -> Self { self.add(other) }

    /// For SMIL, addition is performed *after* interpolation so we often require a zero value to
    /// interpolate with (e.g. for "by animation"). For Web Animations, however, addition happens
    /// before interpolation so this is not required.
    fn get_zero_value(&self) -> Option<Self> { None }
}

And, then adding an is_additive properties to the Longhand class so we could do something like:

    % if not property.is_additive:
         impl Addition for computed_value::T {}
    % endif

and pick up the default implementation that way.

The difficulty is that the |accumulate| and |add| methods are often very similar. It would be good if we didn't have to define *both* for all these types. So perhaps the add_weighted approach makes sense, after all.

For example, what if we combine the Interpolate trait with the Addition trait into one "Animatable" trait with default implementations for the addition methods that we can override selectively. Then, perhaps we could add to that an add_weighted method and define the default implementation of interpolate / add / accumulate in terms of that? Perhaps we could even do the implementation of discrete implementation of interpolate there. (And then I wonder if ComputeDistance should be combined too -- it would, I think, be better to have all the animation-related methods for say, animating colors, grouped together like this.)

	Brian Birtles (:birtles) Assignee
	Comment 5 • 7 years ago

I've been trying to switch all the animation code over to the following interface to see if it would work:

  pub fn discrete_add_weighted<T>(a: &T, b: &T, a_portion: f64, b_portion: f64) -> T {
      if a_portion > b_portion {
          a
      } else {
          b
      }
  }

  /// A trait used to implement various procedures used during animation.
  pub trait Animatable: Sized {
      /// Generic helper method to add together a portion of two values and return the result.
      ///
      /// For most animation types, it is sufficient just to implement this function and allow
      /// the default implementation of interpolate, add, and accumulate below to depend on it.
      fn add_weighted(&self, other: &Self, self_portion: f64, other_portion: f64) -> Self {
          discrete_add_weighted(self, other, self_portion, other_portion)
      }

      /// [Interpolates][interpolation] a value with another for a given property.
      ///
      /// [interpolation]: https://w3c.github.io/web-animations/#animation-interpolation
      fn interpolate(&self, other: &Self, progress: f64) -> Self {
          self.add_weighted(other, 1.0 - progress, progress)
      }

      /// Returns the [sum][animation-addition] of this value and |other|.
      ///
      /// "For animation types that do not define a specific procedure for addition or which are
      /// defined as not additive, the addition procedure is simply Vres = Vb."
      ///
      /// [animation-addition]: https://w3c.github.io/web-animations/#animation-addition
      fn add(&self, other: &Self) -> Self {
          self.add_weighted(other, 1.0, 1.0)
      }

      /// Returns the result of combining |other| with this value such that |other| is treated as
      /// a delta from this value. See
      /// [accumulation][https://w3c.github.io/web-animations/#animation-accumulation].
      ///
      /// "For animation types that do not define a specific procedure for accumulation, the
      /// accumulation procedure is identical to the addition procedure for that type."
      fn accumulate(&self, other: &Self, count: u64) -> Self {
          match count {
              0 => other,
              _ => self.add_weighted(other, 1.0, count as f64)
          }
      }

      /// For SMIL, addition is performed *after* interpolation so we often require a zero value to
      /// interpolate with (e.g. for "by animation"). For Web Animations, however, addition happens
      /// before interpolation so this is not required.
      fn get_zero_value(&self) -> Self {
          self.add_weighted(self, 0.0, 0.0)
      }

      /// Compute distance between a value and another for a given property.
      fn compute_distance(&self, other: &Self) -> Result<f64, ()> { Err(()) }

      /// In order to compute the Euclidean distance of a list or property value with multiple
      /// components, we need to compute squared distance for each element, so the vector can sum it
      /// and then get its squared root as the distance.
      fn compute_squared_distance(&self, other: &Self) -> Result<f64, ()> {
          self.compute_distance(other).map(|d| d * d)
      }
  }

So far it seems to fit well and mean we generally need less code. For example, the |Au| type would have the following implementation:

  impl Animatable for Au {
      #[inline]
      fn add_weighted(&self, other: &Self, self_portion: f64, other_portion: f64) -> Self {
          Au((self.0 as f64 * self_portion + other.0 as f64 * other_portion).round() as i32)
      }

      #[inline]
      fn compute_distance(&self, other: &Self) -> Result<f64, ()> {
          self.0.compute_distance(&other.0)
      }
  }

Then we should get interpolate, add, accumulate, get_zero_value for free from their default definitions. For some types we'll need to explicitly implement add() or accumulate() however, to get the particular behavior we need.

Also, by combining these into one trait we can avoid a bunch of impl definitions for ComputeDistance that just returns an Err().

Another difference is that I've made interpolate() and friends infallible. This, I think, it what we want. That is, suppose we have a struct with three members of type LengthOrPercentage. When we interpolate a struct like that, suppose we can successfully interpolate two members but not the third, I *think* we want to just use discrete interpolate for the third member. I think so, but I'm not really sure. If that the case, then I think we want always just fall back to discrete interpolation.

If we do that we'll need to add an extra method to Animatable to determine if a member should be interpolable for transitions or not (e.g. is_transitionable).

At this point I should probably stop refactoring and break it down into a few steps:

* Rename Interpolate to Animatable
* Merge ComputeDistance with Animatable and add default definition
* Rewrite interpolate() in terms of add_weighted
* Add a separate is_transitionable() method
* Make interpolate() and add_weighted() infallible
  (See what breaks at this point)
* Add add() method
* Add Gecko bindings for add() and call
* Add accumulate() method
* Add Gecko bindings for accumulate() and call
* Add get_zero_value() method (probably a separate bug for this)
* Use get_zero_value() in SMIL code and hook up additive SMIL animations

	Brian Birtles (:birtles) Assignee
	Comment 6 • 7 years ago

I looked into making interpolate() infallible and, while I still think this is the right thing to do (and would make working on this code a lot simpler), I'm a bit concerned that it will introduce changes in behavior in some cases. In particular, comments like bug 520488 comment 6 concern me. It's probably best to make that change as a separate bug that we can back out if we discover it's not compatible.

For what it's worth, I looked into how we interpolate clip() since it can take auto values. We currently allow interpolating from 'rect(10px, 20px, 30px, 40px)' to 'rect(50px, 60px, 70px, auto)' using discrete animation for the 'auto' component but we have a check to make sure that only works for interpolation and not addition. Ultimately, I think addition should work for that too (producing 'auto' as in the regular fallback additive behavior, until we implement 'calc(40px + auto)') but we can change that later.

	Brian Birtles (:birtles) Assignee
	Comment 7 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=684b32e812b2801d20ece26ab8758b65a37bbba3

This seems to work for some simple tests. I still need to do type-specific handling of addition however (e.g. filter lists).

	Brian Birtles (:birtles) Assignee
	Comment 12 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=834c6710b18cd3dcb354cb082858ba0963b75d14

Running addition-per-property.html locally, the tests pass except for:

* All the discrete properties we've yet to make animatable (i.e. align-items works but align-content does not, bug 1353966)
* Filter property (bug 1362897)
* Various SVG color properties that are not marked as being animatable: flood-color, lighting-color, stop-color (no bug? Bug 1360133 is close, though)

	Hiroyuki Ikezoe (:hiro)
	Comment 13 • 7 years ago
mozreview-review

Comment on attachment 8867583 [details]
Bug 1329878 - Rewrite interpolate() in terms of a more general add_weighted() function

https://reviewboard.mozilla.org/r/139130/#review142360

Looks pretty good to me.
One thing I am curious about is the order of arguments of add_weighted(), it's different from gecko's order.
I don't have strong opinion which is better, I am just curious.

	Hiroyuki Ikezoe (:hiro)
	Comment 14 • 7 years ago
mozreview-review

Comment on attachment 8867585 [details]
Bug 1329878 - Implement animation addition in Servo_AnimationCompose;

https://reviewboard.mozilla.org/r/139134/#review142374

	Hiroyuki Ikezoe (:hiro)
	Comment 15 • 7 years ago
mozreview-review

Comment on attachment 8867584 [details]
Bug 1329878 - Add add() method to Animatable interface;

https://reviewboard.mozilla.org/r/139132/#review142376

	Brian Birtles (:birtles) Assignee
	Comment 16 • 7 years ago

(In reply to Hiroyuki Ikezoe (:hiro) from comment #13)
> Comment on attachment 8867583 [details]
> Bug 1329878 - Rewrite interpolate() in terms of a more general
> add_weighted() function
> 
> https://reviewboard.mozilla.org/r/139130/#review142360
> 
> Looks pretty good to me.
> One thing I am curious about is the order of arguments of add_weighted(),
> it's different from gecko's order.
> I don't have strong opinion which is better, I am just curious.

Yeah, I don't really know either. I just felt like the two portions belonged together but I don't have a strong preference either.

	Hiroyuki Ikezoe (:hiro)
	Comment 18 • 7 years ago

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