523193 - combine implementation of nsStyleAnimation::Interpolate and Add

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

nsStyleAnimation::Interpolate and nsStyleAnimation::Add can really both be implemented in terms of a method that does a*X + b*Y.  This will reduce code duplication, which gets significantly worse once we start adding code for more complicated value types.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Note that this changes the behavior of Add for colors with alpha, but I think my previous design decision there is probably more likely than not to have been a mistake.

Comment 2

[Daniel Holbert [:dholbert]]

Comment on attachment 407088 [details] [diff] [review]
patch

>+nsStyleAnimation::AddWeighted(double aCoeff1, const Value& aValue1,
>+                              double aCoeff2, const Value& aValue2,
>+                              Value& aResultValue);
> {

Nix the ";" before the opening brace :)

Also, we can (and should) assert that aCoeff1 and aCoeff2 are both >= 0.  In the Add() case, "aCount" is unsigned, so we know that both aCoeff's it passes will be positive. And in the Interpolate() case, we should be passing in two non-negative fractions that sum to 1.  So we should never expect to receive negative aCoeff values.

>+      double Aresf = (A1 * aCoeff1 + A2 * aCoeff2);
>       nscolor resultColor;
>-      if (resAf == 0.0) {
>+      if (Aresf <= 0.0) {

Given the assertion I suggested above, we should be OK to leave this as "== 0", rather than <=0.  Aresf is a weighted sum of non-negative numbers, with non-negative weights, so it can't be negative.

r=dholbert with that.

Comment 3

[Daniel Holbert [:dholbert]]

(In reply to comment #2)
> Also, we can (and should) assert that aCoeff1 and aCoeff2 are both >= 0.

I guess AddWeighted is a public method -- we should state this assumption in its documentation, too. (Unless you believe potential clients of the method might want to use negative coefficients, in which case you can ignore comment 2 aside from the stray semicolon.)

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #2)
> Nix the ";" before the opening brace :)

Hmmm.  I fixed that (it's an error), but must have qrefreshed it into the wrong patch.

> Also, we can (and should) assert that aCoeff1 and aCoeff2 are both >= 0.

Why should we?  Is there a reason that assumption is useful to us?

Comment 5

[Daniel Holbert [:dholbert]]

(In reply to comment #4)
> > Also, we can (and should) assert that aCoeff1 and aCoeff2 are both >= 0.
> 
> Why should we?  Is there a reason that assumption is useful to us?

Because we expect it to be true, given our current callers.

For more complex types, potentially-negative coefficients will mean more special-cases for negative outputs.  (like the lower-bound clamping you've added in that Aresf check and in ClampColor)

Consider e.g. the "stroke-dasharray" property, which SVG defines as is interpolatable but not additive. (So it will support Interpolate() but not Add())  It would simplify our implementation for this type if we could assume that the coefficients are non-negative -- then, we don't need to worry about clamping each length in the final list to be non-negative.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #5)
> Consider e.g. the "stroke-dasharray" property, which SVG defines as is
> interpolatable but not additive. (So it will support Interpolate() but not
> Add())  It would simplify our implementation for this type if we could assume
> that the coefficients are non-negative -- then, we don't need to worry about
> clamping each length in the final list to be non-negative.

Given that, we might just want to implement handling of that type in Interpolate rather than in AddWeighted.  (Does SVG say somewhere *how* one interpolates between two values of 'stroke-dasharray'?)

Comment 7

[Daniel Holbert [:dholbert]]

(In reply to comment #6)
> (Does SVG say somewhere *how* one
> interpolates between two values of 'stroke-dasharray'?)

No, it doesn't. I think Opera only does smooth interpolation when the two lists have the same length, and otherwise it doesn't interpolate at all.  It's been a little while since I tested that property, though, so I could be wrong.

Anyway, I don't care about the negative-coefficient point too much -- if you'd like to keep the method more generalized so that it *could* receive negative coefficients and still work, that's fine by me.

I think it's worth at least stating their allowed range in the AddWeighted documentation, though.  Otherwise, it's a superficially unclear what those parameters can be, since we currently only ever pass in non-negative values.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

How about this:

I'll document that currently negative numbers for the coefficients are allowed but we don't depend on them anywhere, and if, in the future, we decide it's difficult to support them, we might change them to being disallowed.

Comment 9

[Daniel Holbert [:dholbert]]

Sure, that sounds good.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #7)
> (In reply to comment #6)
> > (Does SVG say somewhere *how* one
> > interpolates between two values of 'stroke-dasharray'?)
> 
> No, it doesn't. I think Opera only does smooth interpolation when the two lists
> have the same length, and otherwise it doesn't interpolate at all.  It's been a
> little while since I tested that property, though, so I could be wrong.

Also, I got started thinking about this:  it seems like the best way to do stroke-dasharray animation would be:
 * compute the least common multiple of the input list lengths, and effectively repeat each list so that they both end up being that least common multiple (though no need to produce such a representation in memory).  (Computing LCMs is easy enough, combining:  http://en.wikipedia.org/wiki/Least_common_multiple#Reduction_by_the_greatest_common_divisor http://en.wikipedia.org/wiki/Greatest_common_factor#Using_Euclid.27s_algorithm .)
 * interpolate each element piecewise, just like lengths
which extends just fine to additive animation or any other variant of AddWeighted.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

http://hg.mozilla.org/mozilla-central/rev/c51d02c50812

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I filed bug 523355 on implementing animation of stroke-dasharray.