Closed Bug 1329878 Opened 4 years ago Closed 4 years ago

Stylo: Implement addition on Servo AnimationValue

Categories

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

defect

Tracking

()

RESOLVED FIXED
Tracking Status
firefox55 --- fixed

People

(Reporter: boris, Assigned: birtles)

References

(Blocks 1 open bug)

Details

Attachments

(5 files)

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.
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.
(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.
Priority: -- → P3
Blocks: 1338087
Depends on: 1311257
Blocks: 1353202
Summary: Stylo: Implement accumulate and addition on Servo AnimationValue → Stylo: Implement addition on Servo AnimationValue
Blocks: 1355349
Depends on: 1356941
No longer blocks: 1353202
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.
Assignee: nobody → bbirtles
Status: NEW → ASSIGNED
Priority: P3 → P1
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.)
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
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.
Depends on: 1363573
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).
Attached file Simple addition test
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)
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.
Attachment #8867583 - Flags: review?(hikezoe) → review+
Comment on attachment 8867585 [details]
Bug 1329878 - Implement animation addition in Servo_AnimationCompose;

https://reviewboard.mozilla.org/r/139134/#review142374
Attachment #8867585 - Flags: review?(hikezoe) → review+
Comment on attachment 8867584 [details]
Bug 1329878 - Add add() method to Animatable interface;

https://reviewboard.mozilla.org/r/139132/#review142376
Attachment #8867584 - Flags: review?(hikezoe) → review+
(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.
Attached file Servo PR #16863
https://hg.mozilla.org/mozilla-central/rev/d2d964851925
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.