Closed Bug 1272549 Opened 4 years ago Closed 3 years ago

Support paced spacing for transform

Categories

(Core :: DOM: Animation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: boris, Assigned: boris)

References

(Depends on 1 open bug, )

Details

Attachments

(10 files, 1 obsolete file)

58 bytes, text/x-review-board-request
birtles
: review+
Details
58 bytes, text/x-review-board-request
birtles
: review+
Details
58 bytes, text/x-review-board-request
derf
: review+
Details
58 bytes, text/x-review-board-request
birtles
: review+
Details
58 bytes, text/x-review-board-request
birtles
: review+
Details
58 bytes, text/x-review-board-request
birtles
: review+
Details
58 bytes, text/x-review-board-request
birtles
: review+
Details
58 bytes, text/x-review-board-request
birtles
: review+
Details
58 bytes, text/x-review-board-request
birtles
: review+
Details
58 bytes, text/x-review-board-request
hiro
: review+
Details
As Bug 1244590 comment 6 said, we haven't implemented StyleAnimationValue::ComputeDistance() for Transform, Shape, and Filter function yet [1]. We use StyleAnimationValue::ComputeDistance() to calculate the cumulative distances on paced spacing mode, so we fall them back to evenly distribute spacing now if someone want to use these CSS properties in paced function. e.g. "spacing: paced(transform)".


[1] https://dxr.mozilla.org/mozilla-central/rev/3461f3cae78495f100a0f7d3d2e0b89292d3ec02/layout/style/StyleAnimationValue.cpp#864-872
I'm thinking how to compute the distance between a pair of transforms/shapes/filters
1. Transform
 * We can follow the formulas in the SVG spec:
   https://www.w3.org/TR/SVG11/animate.html#complexDistances
2. Shape [1] & Filter [2]
 * The spec doesn't give us any formula for calculating the distance between two shape/filter functions.
   There are two options:
   a) File two extra bugs, one for Shape and the other one for Filter, and wait for the update of specs.
   b) We had implemented interpolations for both Basic shape and Filter, so we can follow the same rule and
      may use the square root of the sum of the squares of distances of each components
      between two interpolatable basic shapes/filters. I'm not sure whether there are other challenges for
      calculating the distances for Shape and Filter, so I prefer a) now.

[1] https://drafts.csswg.org/css-shapes/#basic-shape-interpolation
[2] https://drafts.fxtf.org/filters/#animation-of-filters


BTW. we have to handle these use cases (for Transform):
1. Changing the transform functions
var anim = div.animate([ { transform: translate(1, 2) },   // keyframe 0
                         { transform: translate(3, 4) },   // keyframe 1
                         { transform: rotate(5deg) },      // keyframe 2
                         { transform: rotate(10deg) } ],   // keyframe 3
                       { duration: 1000, spacing: 'paced(transform)' });

In the case, I think the distance between keyframe 1 and keyframe 2 is
dist = sqrt( dist(translate(3,4), translate(0, 0))^2 + dist(rotate(0deg), rotate(5deg))^2 )

2. Multiple function values
var anim = div.animate([ { transform: translate(1, 2), rotate(5deg) },   // keyframe 0
                         { transform: translate(3, 4), rotate(5deg) },   // keyframe 1
                         { transform: rotate(5deg) },                    // keyframe 2
                         { transform: rotate(10deg) } ],                 // keyframe 3
                       { duration: 1000, spacing: 'paced(transform)' });
For keyframe 0 and keyframe 1, we still use the square root of the sum of squares. For keyframe 1 and keyframe 2, just like above example. We fill a dummy transform function, translate(0, 0), in keyframe 2, so we can calculate the distance.

Do these make sense?
(In reply to Boris Chiou [:boris] from comment #1)
> I'm thinking how to compute the distance between a pair of
> transforms/shapes/filters
> 1. Transform
>  * We can follow the formulas in the SVG spec:
>    https://www.w3.org/TR/SVG11/animate.html#complexDistances

Yes, we'll need to update [1] for this, but I think that's probably the right idea. We'll probably need to extend it to handle a few additional cases too.

[1] https://w3c.github.io/web-animations/#transform-list-animation-type-section

> 2. Shape [1] & Filter [2]
>  * The spec doesn't give us any formula for calculating the distance between
> two shape/filter functions.
>    There are two options:
>    a) File two extra bugs, one for Shape and the other one for Filter, and
> wait for the update of specs.
>    b) We had implemented interpolations for both Basic shape and Filter, so
> we can follow the same rule and
>       may use the square root of the sum of the squares of distances of each
> components
>       between two interpolatable basic shapes/filters. I'm not sure whether
> there are other challenges for
>       calculating the distances for Shape and Filter, so I prefer a) now.
> 
> [1] https://drafts.csswg.org/css-shapes/#basic-shape-interpolation
> [2] https://drafts.fxtf.org/filters/#animation-of-filters

I think it's quite fine to split this into separate bugs. However, no-one's likely to go and update those specs for us. So I think your proposal above seems reasonable. We should just implement that and then spec it.

> BTW. we have to handle these use cases (for Transform):
> 1. Changing the transform functions
> var anim = div.animate([ { transform: translate(1, 2) },   // keyframe 0
>                          { transform: translate(3, 4) },   // keyframe 1
>                          { transform: rotate(5deg) },      // keyframe 2
>                          { transform: rotate(10deg) } ],   // keyframe 3
>                        { duration: 1000, spacing: 'paced(transform)' });
> 
> In the case, I think the distance between keyframe 1 and keyframe 2 is
> dist = sqrt( dist(translate(3,4), translate(0, 0))^2 + dist(rotate(0deg),
> rotate(5deg))^2 )
> 
> 2. Multiple function values
> var anim = div.animate([ { transform: translate(1, 2), rotate(5deg) },   //
> keyframe 0
>                          { transform: translate(3, 4), rotate(5deg) },   //
> keyframe 1
>                          { transform: rotate(5deg) },                    //
> keyframe 2
>                          { transform: rotate(10deg) } ],                 //
> keyframe 3
>                        { duration: 1000, spacing: 'paced(transform)' });
> For keyframe 0 and keyframe 1, we still use the square root of the sum of
> squares. For keyframe 1 and keyframe 2, just like above example. We fill a
> dummy transform function, translate(0, 0), in keyframe 2, so we can
> calculate the distance.
> 
> Do these make sense?

I haven't really thought about these properly but we can possibly re-use some of the logic for interpolating transform lists described beginning at [2].

I think that logic tries to line up the transform lists as best as possible. We should try to use that if possible.

[2] https://drafts.csswg.org/css-transforms/#interpolation-of-transforms
See Also: → 1286150
Summary: Support paced spacing on transform, shape, and filter function → Support paced spacing for transform
See Also: → 1286151
Assignee: nobody → boris.chiou
Status: NEW → ASSIGNED
(In reply to Brian Birtles (:birtles) from comment #2)
> I haven't really thought about these properly but we can possibly re-use
> some of the logic for interpolating transform lists described beginning at
> [2].
> 
> I think that logic tries to line up the transform lists as best as possible.
> We should try to use that if possible.
> 
> [2] https://drafts.csswg.org/css-transforms/#interpolation-of-transforms

In Issue 10 [1], the spec says "Look only at the first component of the two lists", but then it mentions "looking at only the first component seems odd. Going through each component, working out the distance and then getting the square of the distance also seems much more consistent with what we do elsewhere." IMO, I prefer "look only at the first component of the two lists" because going through each component and then doing summation of the square of distances of each component need to normalize them first; otherwise, the summation may not have any meaning. However, normalizing these transform parameters is not a easy job because the value range might be unlimited.
Also, using the first component makes developer choose what they want for the spacing in a transform list, so it makes things simpler, I think.

[1] https://w3c.github.io/web-animations/#issue-789f9fd1
(In reply to Boris Chiou [:boris] from comment #3)
> In Issue 10 [1], the spec says "Look only at the first component of the two
> lists", but then it mentions "looking at only the first component seems odd.
> Going through each component, working out the distance and then getting the
> square of the distance also seems much more consistent with what we do
> elsewhere." IMO, I prefer "look only at the first component of the two
> lists" because going through each component and then doing summation of the
> square of distances of each component need to normalize them first;
> otherwise, the summation may not have any meaning. However, normalizing
> these transform parameters is not a easy job because the value range might
> be unlimited.

Why do we need to normalize? Can't we just use the result as-is? It's true that it doesn't make sense to compare the distance between "rotate(0deg)" and "rotate(180deg)" with the distance between "translate(0px)" and "translate(1000px)" but I think it's likely that we'll need to compute the distance between "rotate(180deg) translate(0px)" and "rotate(180deg) translate(1000px)" and wouldn't it be odd if that had a distance of zero?

Intuitively, if we have:

  [ "rotate(180deg) translate(0px)",
    "rotate(180deg) translate(1000px)",
    "rotate(360deg) translate(1000px)" ]

perhaps the most reasonable behavior is that it translates for a long time then rotates briefly? What do you think?

Then again, that might not work so well for scale. Perhaps there is some other concept of distance we should consider.

Alternatively, we could try to normalize the components by finding the minimum and maximum values used for each type of function. That sounds a bit fiddly but perhaps produces the more natural result?

> Also, using the first component makes developer choose what they want for
> the spacing in a transform list, so it makes things simpler, I think.

I'm not sure about this part. The order in which functions appear in the list often affects the visual result--in many cases the developer can't freely re-order the list without affecting the result so I don't think they can use the order of functions to choose how spacing should work.
(In reply to Brian Birtles (:birtles, busy 19-23 Sep) from comment #4)
> Why do we need to normalize? Can't we just use the result as-is? It's true
> that it doesn't make sense to compare the distance between "rotate(0deg)"
> and "rotate(180deg)" with the distance between "translate(0px)" and
> "translate(1000px)" but I think it's likely that we'll need to compute the
> distance between "rotate(180deg) translate(0px)" and "rotate(180deg)
> translate(1000px)" and wouldn't it be odd if that had a distance of zero?
> 
> Intuitively, if we have:
> 
>   [ "rotate(180deg) translate(0px)",
>     "rotate(180deg) translate(1000px)",
>     "rotate(360deg) translate(1000px)" ]
> 
> perhaps the most reasonable behavior is that it translates for a long time
> then rotates briefly? What do you think?


If we do paced animation only on one transform function, we can perfectly have a smoothing effect for that function, e.g. rotate, but bad effects on other types, e.g. translate, skew. If we consider two different transform functions together, e.g. rotate and translate, both transform functions would run non-smoothing, but the problem you mentioned disappears. I think this is depending on which is what we really want.

If we prefer to consider all the components:
for example,

   [ "rotate(180deg) translate(0px)",                    // keyframe 0
     "rotate(180deg) translate(1000px) scale(0.5)",      // keyframe 1
     "rotate(360deg) translate(1000px) scale(1.2)",      // keyframe 2
     "rotate(360deg) translate(10px)" ]                  // keyframe 3

While calculating the distance between keyframe 0 and 1, the transform function lists are not matched, so we decompose the matrix and calculate the Euclidean distance by all the components according to the result [1] (if the matrix is 3D). However, while calculating the distance between keyframe 1 and 2, they are matched, so the distance may looks like: 
sqrt((360 - 180)^2 + (1000 - 100)^2 + ((1.2 - 0.5) * |transform block width|)^2).

[1] https://drafts.csswg.org/css-transforms/#decomposing-a-3d-matrix
    Input:  matrix      ; a 4x4 matrix 
    Output: translation ; a 3 component vector
            scale       ; a 3 component vector
            skew        ; skew factors XY,XZ,YZ represented as a 3 component vector
            perspective ; a 4 component vector
            quaternion  ; a 4 component vector


However, I'm not sure how to handle this case properly:
   [ "translate(300px)",                                  // keyframe 0
     "translate(100px) rotate(45deg) translate(200px)",   // keyframe 1
     "translate(200px) rotate(55deg) translate(100px)",   // keyframe 2
     "translate(300px)" ]                                 // keyframe 3

keyframe 0 & 1 are not matched, so we need to decompose the matrices of keyframe 0 and kayframe 1, and calculate the distance by their translate parts.
keyframe 1 & 2 are matched, but there are two translate functions in each list. I think we should not merge two translate functions because "translate(100px) rotate(45deg) translate(200px)" != "translate(200px) translate(100px) rotate(45deg)"
There are two methods I think:
1) Decompose the matrix and calculate only the translate part
2) Just calculate the distance by sqrt((200-100)^2 + (55-45)^2 + (100-200)^2)?
For simplicity's sake, the second one may be OK but not accurate enough for the paced animation. What do you think about this? Thanks.

> 
> Then again, that might not work so well for scale. Perhaps there is some
> other concept of distance we should consider.

This is one of my reasons to calculate the distance by using only the first transform function. It's better not to put scale and other types together. Maybe we can convert the scale into a possible length, e.g. Use the size of reference box [2], width. so if scale is from 0.5 -> 1.2, the distance of scale would be something like: (1.2 - 0.5) * TransformReferenceBox::Width(), and so we can use this value as one component while calculating the Euclidean distance.

[2] http://searchfox.org/mozilla-central/rev/dbbbafc979a8362a1c8e3e99dbea188fc832b1c5/layout/style/nsStyleTransformMatrix.h#102

> 
> Alternatively, we could try to normalize the components by finding the
> minimum and maximum values used for each type of function. That sounds a bit
> fiddly but perhaps produces the more natural result?

Yes, This might be a good solution. The only problem is finding the minimum and maximum values used for each type of function and I think it's hard to find them.
In conclusion:
We may try to fix issue 10:

Look through all the components of the two lists
1. If transform function lists are matched (i.e. same transform function or a derivative of the same primitive)
  We may have these components:
    a) translate, scale, rotate => use primitives (2d with 2 components, 3d with 3 components, rotate3d has 4 components)
       (https://drafts.csswg.org/css-transforms/#transform-primitives)
    b) skew => 2 components (2d skew),
    c) scale => 2 components, but we need to multiply a factor, e.g transform block width (Not sure for this part)
    d) perspective => 1 component
    e) matrix2d => decompose it into: (https://drafts.csswg.org/css-transforms/#decomposing-a-2d-matrix)
                     translation ; a 2 component vector
                     scale       ; a 2 component vector
                     angle       ; rotation
                     m11         ; 1,1 coordinate of 2x2 matrix (ignored here)
                     m12         ; 1,2 coordinate of 2x2 matrix (ignored here)
                     m21         ; 2,1 coordinate of 2x2 matrix (ignored here)
                     m22         ; 2,2 coordinate of 2x2 matrix (ignored here)
                   and only use translation, scale, angle parts => 5 components
    f) matrix3d => decompose it into: (https://drafts.csswg.org/css-transforms/#decomposing-a-3d-matrix)
                     translation ; a 3 component vector
                     scale       ; a 3 component vector
                     skew        ; skew factors XY,XZ,YZ represented as a 3 component vector
                     perspective ; a 4 component vector
                     quaternion  ; a 4 component vector
                   totally 17 components. (so complicated...)
  Finally, calculate Euclidean distance for those components.

2. If transform function lists are not matched
  step 1: Read transforms from list 1 and list 2 by StyleTransformMatrix::ReadTransform().
  step 2: Decompose them, use the same procedure for matrix2d or matrix3d.
          (If both are 2d, use 2d. If one of them are 3d, use 3d.)
  step 3: Calculate Euclidean distance for those components.

3. One of the transform list is None
  It's a special case for "matched" transform lists. Append transform lists for None.
  e.g. translate3d(0px, 0px, 0px), rotate3d(0, 0, 0, 0rad), scale3d(1, 1, 1), skew(0, 0), identity matrix, perspective(inf)...

4. None and None
  Nothing happens

I haven't implemented it yet, and only check the implementation of interpolation of transform. Maybe we could simplify the rule.
(In reply to Boris Chiou [:boris] from comment #7)
> 4. None and None
>   Nothing happens
This means the distance is 0.0.
(In reply to Boris Chiou [:boris] from comment #6)
> > Alternatively, we could try to normalize the components by finding the
> > minimum and maximum values used for each type of function. That sounds a bit
> > fiddly but perhaps produces the more natural result?
> 
> Yes, This might be a good solution. The only problem is finding the minimum
> and maximum values used for each type of function and I think it's hard to
> find them.

Maybe we can try to normalize/convert units by:
Method 1: 
a) Length (e.g. translate):  Normalized by transform reference box size, e.g. sqrt(refBox.Width()^2 + refBox.Height()^2).
                                  or normalized x-axis component by TransformReferenceBox::Width(),
                                  y-axis component by TransformReferenceBox::Height() [1],
                                  but I don't have any idea now for z-axis. (translate3d, translateZ and perspective need this.)
b) Angle (e.g. rotate/skew): Use Radian unit.
c) Number (e.g. scale):      Use it directly.

Method 2:
a) Length: Use user pixel value directly.
b) Angle:  Use Degree unit.
c) Number: Multiplied by length of transform reference box size, e.g. sqrt(refBox.Width()^2 + refBox.Height()^2),
           or by TransformReferenceBox::Width() for x-axis component, TransformReferenceBox::Height() for y-axis.
           But I still have no idea for z-axis now. 

They are not perfect, but I think either way can make the ranges of different types of values closer. What do you think about this? Thanks.

[1] http://searchfox.org/mozilla-central/rev/8910ca900f826a9b714607fd23bfa1b37a191eca/layout/style/nsStyleTransformMatrix.h#102,106
I pinged the Google folks to see if they have any comments on this but they don't appear to at this time.

One thing to bear in mind is that we might end up using these distance calculations in DevTools to visualize the amount of change between keyframes. In that situation, it seems like only considering the first component would be sub-optimal since in cases where you're forced to animate between 'translate(45px) rotate(0deg)' and 'translate(45px) rotate(360deg)' it would be odd if the visualization suggested that nothing changed. So although the current approach is complex, it seems better for that particular use case.

It might not be worth normalizing the values but I think at least reflecting each of the items in the list in the result somehow.

I haven't gone through your proposal in detail but at a glance it seems reasonable. If you're able to find further simplifications as you implement, even better.

Another thought I had was that for transform we allow syntax like 'paced(transform:translate)' (to tell us to just look at the translation components, or even just the translate functions) but maybe that's something to add later if people actually start using this.
(In reply to Brian Birtles (:birtles) from comment #12)
> I pinged the Google folks to see if they have any comments on this but they
> don't appear to at this time.
> 
> One thing to bear in mind is that we might end up using these distance
> calculations in DevTools to visualize the amount of change between
> keyframes. In that situation, it seems like only considering the first
> component would be sub-optimal since in cases where you're forced to animate
> between 'translate(45px) rotate(0deg)' and 'translate(45px) rotate(360deg)'
> it would be odd if the visualization suggested that nothing changed. So
> although the current approach is complex, it seems better for that
> particular use case.
> 
> It might not be worth normalizing the values but I think at least reflecting
> each of the items in the list in the result somehow.
> 
> I haven't gone through your proposal in detail but at a glance it seems
> reasonable. If you're able to find further simplifications as you implement,
> even better.
> 

Thanks, Brian. I will try to implement a beta version by my proposal which goes through all components and try to use suitable units for each component (and maybe ignore some decomposed components).
Hi, Brian,

I would like to explain the calculation details in these patches.

We have two transform lists, and there are four possible cases for them:
1) none v.s. none:
The distance is 0.0, apparently.

2) two matched transform lists:
e.g.
[ { transform: "translate(1000px) rotate3d(1,0,0, 100deg) scale(1.2) skew(60deg)" },
  { transform: "translate(300%)   rotate3d(0,0,1, -50deg) scale(0.7) skew(30deg)" } ]

3) none v.s. a transform list:
We append an equivalent transform list and treat them as case (2).

e.g.
[ { transform: "none" },
  { transform: "translate(300%) rotate3d(0,0,1, -50deg) scale(0.7) skew(30deg)" } ]
=>
[ { transform: "translate(0)    rotate3d(0,0,1, 0deg)   scale(1.0) skew(0deg)" },
  { transform: "translate(300%) rotate3d(0,0,1, -50deg) scale(0.7) skew(30deg)" } ]

4) two mismatched lists:
e.g.
[ { transform: "translate(1000px) scale(1.2) skew(60deg)" },
  { transform: "translate(300%) rotate3d(0,0,1, -50deg)" } ]
For each transform list, we convert it into an equivalent matrix4x4, and then do 3d decomposition on it. Therefore, we will get two decomposed value lists, and each decomposed value list is like this:

transform: "translation3d(x,y,z) scale(x,y,z) skew(x,y,z) perspective(x,y,z,w) quaternion(x,y,z,w)"

We have 17 components in this decomposed value list, so we calculate the sum of squares, and then get the square root of it. (Euclidean distance with 17-components vector).


-----
For the matched transform lists, I use the following rules to get the distance:
  a) translate: (Unit: CSS pixel)
     * Get primitive first, so we have two translate3d functions and
       then compute the sum of squares of the difference.

  b) scale: (Unit: number)
     * The same as translate.

  c) skewX, skewY, skewXY, skew: (Unit: Radian)
     * The sum of squares of the difference.

  d) rotateX, rotateY, rotate: (Unit: Radian)
     * The sum of squares of the difference.

  e) rotate3d: (Unit: Radian)
     * If two rotate functions have the same direction axis, just get the sum of squares of the difference.
       If the direction axes are different, we convert them into two quaternion unit vectors, and
               get the distance by this formula:
               cos(theta/2) = (q1 dot q2) / (|q1| x |q2|), |theta| is their distance angle.
               Finally, get the squares of the angle.

  f) perspective: (Unit: CSS pixel)
     * The squares of the difference.

  g) matrix2d:
     * decompose it into translate, scale, skew, rotate angle. so there are 6 components, and then
       get the sum of squares of the difference.

  h) matrix3d:
     * decompose it into: translate, scale, skew, perspective, and quaternion.
       We get the distance angle of two quaternion vector by the same formula for rotate3d.
       Therefore, there are 3+3+3+4+1 = 14 components. Finally, get the sum of squares of the difference.

After get all the sum of squares, we calculate its square root which is the final answer.
e.g.
element.style.width = 200px;
[ { transform: "translate(1000px) rotate3d(0,0,1, 120deg) scale(1.2) skew(60deg)" },
  { transform: "translate(300%)   rotate3d(0,0,1, -60deg) scale(0.7) skew(30deg)" } ]
The distance is: sqrt( 400^2 + pi^2 + 0.5^2 + (pi/6)^2 ).

I didn't do any normalization in these patches (and so translate and perspective have significant impact on the final distance). It you have any good suggestion, we can fix it. Thanks.
Attachment #8800117 - Flags: review?(bbirtles)
Attachment #8800118 - Flags: review?(bbirtles)
Attachment #8800119 - Flags: review?(bbirtles)
Attachment #8800120 - Flags: review?(bbirtles)
Attachment #8800121 - Flags: review?(bbirtles)
Attachment #8800122 - Flags: review?(bbirtles)
Attachment #8800123 - Flags: review?(bbirtles)
Attachment #8800124 - Flags: review?(bbirtles)
Attachment #8800125 - Flags: review?(bbirtles)
Attachment #8800126 - Flags: review?(bbirtles)
Comment on attachment 8800122 [details]
Bug 1272549 - Part 4: Compute distance for none and a valid transform list.

https://reviewboard.mozilla.org/r/83552/#review83710

::: layout/style/nsStyleTransformMatrix.cpp:689
(Diff revision 1)
>    MOZ_ASSERT(aData->Item(0).GetUnit() == eCSSUnit_Enumerated);
>    return aData->Item(0).GetKeywordValue();
>  }
>  
> +void
> +SetIdentityMatrix(nsCSSValue::Array* aMatrix)

Nice!
I'd like to use this function in bug 1305325.
By the way, I wonder SetFloatValue is more efficient than operator=.
Comment on attachment 8800124 [details]
Bug 1272549 - Part 6: Use enum class for shear in decomposition functions.

https://reviewboard.mozilla.org/r/83556/#review83712

::: layout/style/StyleAnimationValue.cpp:1729
(Diff revision 1)
> -  float shear1[3] = { 0.0f, 0.0f, 0.0f};
> +  ShearArray shear1;
> +  for (auto&& s : shear1) {
> +    s = 0.0f;
> +  }

After we land Bug 1309466, this can be revised as:
ShearArray shear1 = { 0.0f, 0.0f, 0.0f };
Comment on attachment 8800122 [details]
Bug 1272549 - Part 4: Compute distance for none and a valid transform list.

https://reviewboard.mozilla.org/r/83552/#review83710

> Nice!
> I'd like to use this function in bug 1305325.
> By the way, I wonder SetFloatValue is more efficient than operator=.

Sure. I can use SetFloatValue after Brian's review. Thanks.
Comment on attachment 8800117 [details]
Bug 1272549 - Part 1: Reorder some static functions.

https://reviewboard.mozilla.org/r/83542/#review84340

We could just add appropriate declarations to the top of the file to avoid moving code around so much but I guess this is fine.
Attachment #8800117 - Flags: review?(bbirtles) → review+
Comment on attachment 8800118 [details]
Bug 1272549 - Part 2: Compute distance for basic 2d & 3d transform functions for matched transform lists.

https://reviewboard.mozilla.org/r/83544/#review84344

r=me with the following comments addressed

::: layout/style/StyleAnimationValue.cpp:725
(Diff revision 1)
> +  if (!aArray1 || !aArray2) {
> +    return 0.0;
> +  }

See comments below--I think we could replace this with an assertion.

::: layout/style/StyleAnimationValue.cpp:729
(Diff revision 1)
> +  RefPtr<nsCSSValue::Array> a1 = ToPrimitive(aArray1),
> +                            a2 = ToPrimitive(aArray2);
> +  nsCSSKeyword tfunc = nsStyleTransformMatrix::TransformFunctionOf(a1);
> +  MOZ_ASSERT(nsStyleTransformMatrix::TransformFunctionOf(a2) == tfunc);

This could do with a comment. e.g. "Normalize translate and scale functions to equivalent "translate3d" and "scale3d" functions."

::: layout/style/StyleAnimationValue.cpp:746
(Diff revision 1)
> +      double c1 = ExtractCalcValue(x).mLength;
> +      double c2 = ExtractCalcValue(y).mLength;
> +      double c3 = z.GetFloatValue();

Why is z special? Does the spec say it can never have calc() values?

::: layout/style/StyleAnimationValue.cpp:785
(Diff revision 1)
> +                 y.GetAngleValueInRadians() * y.GetAngleValueInRadians();
> +      break;
> +    }
> +    case eCSSKeyword_skewx:
> +    case eCSSKeyword_skewy:
> +    case eCSSKeyword_rotate:

Do we ever encounter the 3 argument form of rotate?

  rotate( <angle> [, <length>, <length>]? )[1]

[1] https://drafts.csswg.org/css-transforms/#rotate-three-function

Or does that get normalized somewhere?

::: layout/style/StyleAnimationValue.cpp:833
(Diff revision 1)
> +    case eCSSKeyword_matrix3d:
> +      // TODO: decompose matrix and calculate distance. This will be fixed in
> +      // the later patch.
> +      distance = 0.0;
> +      break;
> +    case eCSSKeyword_interpolatematrix:

What is interpolatematrix?

::: layout/style/StyleAnimationValue.cpp:841
(Diff revision 1)
> +static double
> +ComputeTransformListDistance(const nsCSSValueList* aList1,
> +                             const nsCSSValueList* aList2)
> +{

Should we assert that aList1 and aList2 are non-null (since we will dereference in the first iteration of the loop without checking).

::: layout/style/StyleAnimationValue.cpp:846
(Diff revision 1)
> +  do {
> +    distance += ComputeTransformDistance(aList1->mValue.GetArrayValue(),
> +                                         aList2->mValue.GetArrayValue());
> +    aList1 = aList1->mNext;
> +    aList2 = aList2->mNext;
> +  } while (aList1);

Should we assert that !aList1 == !aList2 since we only expect lists of matching lengths? (and since we don't check aList2 is non-null before dereferencing it in the next iteration of the loop)

Also, I think we expect aListX->mValue.GetArrayValue() to be non-null so I think we could just assert in ComputeTransformDistance that aArray1 and aArray2 are not null and drop the early return?

::: layout/style/StyleAnimationValue.cpp:1272
(Diff revision 1)
> +          item1 = item1->mNext;
> +          item2 = item2->mNext;
> +        } while (item1 && item2);
> +
> +        if (item1 || item2) {
> +          // Either |break| above or length mismatch.

"Either the transform function types don't match or the lengths don't match."

::: layout/style/StyleAnimationValue.cpp:1278
(Diff revision 1)
> +        } else {
> +          aDistance = ComputeTransformListDistance(list1, list2);
> +        }

If you have an early return you don't need an else.
Attachment #8800118 - Flags: review?(bbirtles) → review+
Thanks for all your work on this. It looks like it was a lot of work and it looks like you've done a very good job.

Unfortunately it's going to take me a while to get through these reviews since I have a lot of other work at the moment. Feel free to re-assign the reviews to hiro or someone else if you want to land this soon.
(In reply to Brian Birtles (:birtles) from comment #32)
> Thanks for all your work on this. It looks like it was a lot of work and it
> looks like you've done a very good job.
> 
> Unfortunately it's going to take me a while to get through these reviews
> since I have a lot of other work at the moment. Feel free to re-assign the
> reviews to hiro or someone else if you want to land this soon.

It's OK. I will start to read other specs (e.g. CSS Shapes, for computing the distance of basic shapes.) and trace stylo code. This bug is related to the spec and so I think you are the most suitable person to review this. Thank you very much.
Hi Boris, I'm looking at part 3 and I'm a bit confused. Didn't we decide in bug 1244590 comment 61 not to depend on layout for distance calculations?
Flags: needinfo?(boris.chiou)
Attachment #8800120 - Flags: review?(bbirtles) → review?(matt.woodrow)
Matt, do you mind looking at part 4 in this patch series. I don't really know 3D math and I think you're the original author of gfxQuaternion?
Comment on attachment 8800121 [details]
Bug 1272549 - Part 8: Compute distance for perspective transform function.

https://reviewboard.mozilla.org/r/83550/#review84928
Attachment #8800121 - Flags: review?(bbirtles) → review+
Comment on attachment 8800122 [details]
Bug 1272549 - Part 4: Compute distance for none and a valid transform list.

https://reviewboard.mozilla.org/r/83552/#review84342

See comments below, I wonder if we can re-use the code we have for interpolating 'none'.

If that's not possible, please re-request review.

::: layout/style/StyleAnimationValue.cpp:721
(Diff revision 1)
> +static nsCSSValueList*
> +CreatEquivalentNoneTransformList(const nsCSSValueList* aList)
> +{

Instead of doing this, could we just call AddTransformLists(0, list2, 0, list2) ?

Perhaps that might not cover matrices but might cover other cases?

::: layout/style/StyleAnimationValue.cpp:722
(Diff revision 1)
>    double diffB = startB - endB;
>    return sqrt(diffA * diffA + diffR * diffR + diffG * diffG + diffB * diffB);
>  }
>  
> +static nsCSSValueList*
> +CreatEquivalentNoneTransformList(const nsCSSValueList* aList)

s/Creat/Create/
Attachment #8800122 - Flags: review?(bbirtles)
(In reply to Brian Birtles (:birtles) from comment #34)
> Hi Boris, I'm looking at part 3 and I'm a bit confused. Didn't we decide in
> bug 1244590 comment 61 not to depend on layout for distance calculations?

Yes. We should skip percent part for translation. But in part 10, I notice that we also need nsIFrame (the layout information) to ReadTransform correctly from two mismatched transform list. If we want to skip the percent part, the 3d transform matrix we read might not be correct. So I added this patch to make these two cases similar. Fortunately, I think only translation part has this problem. I can remove this patch and skip the calculation of calc value in nsStyleTransformMatrix::ProcessTranslatePart, so we can follow the same rule: using the computed value, instead of used value, for calculating paced spacing. Thank you.
Flags: needinfo?(boris.chiou)
Attachment #8800119 - Flags: review?(bbirtles)
Attachment #8800126 - Flags: review?(bbirtles)
Remove the review request of part 10 because I would like to merge "adding nsIFrame parameter" from part 3 into it and we need to revise the interface of nsStyleTransformMatrix::ReadTransforms, so we can handle an empty TransformReferenceBox object correctly. (It will cause some problems if we give an empty TransformReferenceBox object to ReadTransform in the current m-c.)
Comment on attachment 8800118 [details]
Bug 1272549 - Part 2: Compute distance for basic 2d & 3d transform functions for matched transform lists.

https://reviewboard.mozilla.org/r/83544/#review84344

> Why is z special? Does the spec say it can never have calc() values?

Yes. The definition of translate3d is "translate3d( <length-percentage> , <length-percentage> , <length> )" [1]. The z component is must be length, so we don't need to treat it as calc now.

[1] https://www.w3.org/TR/css-transforms-1/#funcdef-translate3d

> Do we ever encounter the 3 argument form of rotate?
> 
>   rotate( <angle> [, <length>, <length>]? )[1]
> 
> [1] https://drafts.csswg.org/css-transforms/#rotate-three-function
> 
> Or does that get normalized somewhere?

I just wrote a simple example:

// e is a SVG rect element or a div element
var anim = e.animate([ { transform: "rotate(0deg, 50px, 50px)" },
  					   { transform: "rotate(90deg, 50px, 50px)" }],
					 { duration: 2000, fill: "forwards"});
The element is not animated, so I guess we didn't support this format by Web Animations API now. (Only in SMIL now? "rotate(90, 50, 50)" works in SMIL SVG transform.)

And I just checked the interpolation part for eCSSKeyword_rotate, and looks like we don't handle this case now in StyleAnimationValue::AddWeighted().
[1] http://searchfox.org/mozilla-central/rev/d96317a351af8aa78ab9847e7feed964bbaac7d7/layout/style/StyleAnimationValue.cpp#2260-2271

Therefore, I think we don't need to handle this case here.

> What is interpolatematrix?

It is used by interpolating two mismatch transform lists.
e.g.
var anim = e.animate([ { transform: "translate(100px)" },
                       { transform: "rotate(90deg)" }],
                     { duration: 2000, fill: "forwards"});
                     
They will be interpolated as "interpolatematrix(translate(100px), rotate(90deg), progress)". If we see an interpolatematrix type [1], we will call ReadTransform() on them to get their equivalent 3d matries, and then do decomposion to get the interpolation result. However, we don't need to handle this case for computing distance, so skip it.

[1] http://searchfox.org/mozilla-central/rev/d96317a351af8aa78ab9847e7feed964bbaac7d7/layout/style/nsStyleTransformMatrix.cpp#662-665

> Should we assert that aList1 and aList2 are non-null (since we will dereference in the first iteration of the loop without checking).

I didn't add assertions here because I assert aList1 and aList2 before calling ComputeTransformListDistance() (in ComputeDistance()). However, I can add assertions here. Thanks.

> Should we assert that !aList1 == !aList2 since we only expect lists of matching lengths? (and since we don't check aList2 is non-null before dereferencing it in the next iteration of the loop)
> 
> Also, I think we expect aListX->mValue.GetArrayValue() to be non-null so I think we could just assert in ComputeTransformDistance that aArray1 and aArray2 are not null and drop the early return?

Sure. I can assert it and do early return.
Comment on attachment 8800118 [details]
Bug 1272549 - Part 2: Compute distance for basic 2d & 3d transform functions for matched transform lists.

https://reviewboard.mozilla.org/r/83544/#review84344

> Sure. I can assert it and do early return.

sorry, I mean dropping the early return.
Comment on attachment 8800122 [details]
Bug 1272549 - Part 4: Compute distance for none and a valid transform list.

https://reviewboard.mozilla.org/r/83552/#review84342

> Instead of doing this, could we just call AddTransformLists(0, list2, 0, list2) ?
> 
> Perhaps that might not cover matrices but might cover other cases?

Yes. translate, rotate, skew, and scale are covered, but perspective, matrix, matrix3d will produce "interpolatematrix" type. I will try to revise AddTransformLists(), so we can get the result we want for these three special types. Thanks, it's a good idea.
Comment on attachment 8800123 [details]
Bug 1272549 - Part 5: Move decompose matrix to nsStyleTransformMatrix.

https://reviewboard.mozilla.org/r/83554/#review85322

This seems fine, but can I ask why? In future, it would be good to put the reason for the changes like this in the commit message.
Attachment #8800123 - Flags: review?(bbirtles) → review+
Comment on attachment 8800124 [details]
Bug 1272549 - Part 6: Use enum class for shear in decomposition functions.

https://reviewboard.mozilla.org/r/83556/#review85326

::: layout/style/StyleAnimationValue.cpp:1725
(Diff revision 1)
>                                                  double aProgress)
>  {
>    // Decompose both matrices
>  
>    // TODO: What do we do if one of these returns false (singular matrix)
> -
> +  using namespace nsStyleTransformMatrix;

Looking at our style guide for this we have:

"using namespace ...; is only allowed in .cpp files after all #includes. Prefer to wrap code in namespace ... { ... }; instead if possible.  using namespace ...; should always specify the fully qualified namespace.  That is, to use Foo::Bar do not write using namespace Foo; using namespace Bar;, write using namespace Foo::Bar;"

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Namespaces

So, I'd suggest you just put "using nsStyleTransformMatrix::Decompose2DMatrix" after the #includes.

And if possible it would be better to do that in the previous patch and avoid the churn below.
Attachment #8800124 - Flags: review?(bbirtles) → review+
Comment on attachment 8800125 [details]
Bug 1272549 - Part 7: Compute distance for matrix and matrix3d.

https://reviewboard.mozilla.org/r/84846/#review85332

I don't quite understand some of the calculations here and I suspect we can do something a little neater for converting matrix types so I'd like to have another look at this.

::: layout/style/StyleAnimationValue.cpp:806
(Diff revision 1)
>  
>  static double
> +ComputeTransform2DMatrixDistance(const Matrix& aMatrix1,
> +                                 const Matrix& aMatrix2)
> +{
> +  using namespace nsStyleTransformMatrix;

As with the previous patch, I think we should be using "using nsStyleTransformMatrix::Decompose2DMatrix" (and likewise for the 3D version) after all the #includes.

::: layout/style/StyleAnimationValue.cpp:807
(Diff revision 1)
>  static double
> +ComputeTransform2DMatrixDistance(const Matrix& aMatrix1,
> +                                 const Matrix& aMatrix2)
> +{
> +  using namespace nsStyleTransformMatrix;
> +  Point3D scale1(1, 1, 1), translate1;

I think this would be more clear as two separate declarations on separate lines.

::: layout/style/StyleAnimationValue.cpp:825
(Diff revision 1)
> +  const double diffShear = atan(shear2[ShearType::XYSHEAR]) -
> +                           atan(shear1[ShearType::XYSHEAR]);
> +  const double diffRotate = 2.0 * (atan2f(rotate2.z, rotate2.w) -
> +                                   atan2f(rotate1.z, rotate1.w));

I trust this is right, but would you mind explaining these calculations? Thanks.

::: layout/style/StyleAnimationValue.cpp:829
(Diff revision 1)
> +  return diffTranslate.DotProduct(diffTranslate) +
> +         diffScale.DotProduct(diffScale) +
> +         diffRotate * diffRotate +
> +         diffShear * diffShear;

I don't understand this. We're adding up the square of all these differences. Don't we need to take the square root somewhere?

::: layout/style/StyleAnimationValue.cpp:839
(Diff revision 1)
> +  using namespace nsStyleTransformMatrix;
> +  Point3D scale1(1, 1, 1), translate1;

As above.

::: layout/style/StyleAnimationValue.cpp:869
(Diff revision 1)
> +  const double dot = clamped(rotate1.DotProduct(rotate2), -1.0, 1.0);
> +  const double diffRotate = 2.0 * acos(dot);

Likewise, would you mind explaining this calculation?

::: layout/style/nsStyleTransformMatrix.cpp:1068
(Diff revision 1)
> +  Matrix4x4 m(aArray->Item(1).GetFloatValue(),
> +              aArray->Item(2).GetFloatValue(),
> +              aArray->Item(3).GetFloatValue(),
> +              aArray->Item(4).GetFloatValue(),
> +              aArray->Item(5).GetFloatValue(),
> +              aArray->Item(6).GetFloatValue(),
> +              aArray->Item(7).GetFloatValue(),
> +              aArray->Item(8).GetFloatValue(),
> +              aArray->Item(9).GetFloatValue(),
> +              aArray->Item(10).GetFloatValue(),
> +              aArray->Item(11).GetFloatValue(),
> +              aArray->Item(12).GetFloatValue(),
> +              aArray->Item(13).GetFloatValue(),
> +              aArray->Item(14).GetFloatValue(),
> +              aArray->Item(15).GetFloatValue(),
> +              aArray->Item(16).GetFloatValue());

Isn't there a better way of doing this? Can we, for example, add a ctor to Matrix4x4 that takes a (fixed-length?) array then use a loop here to build up an array?
Attachment #8800125 - Flags: review?(bbirtles)
Comment on attachment 8800123 [details]
Bug 1272549 - Part 5: Move decompose matrix to nsStyleTransformMatrix.

https://reviewboard.mozilla.org/r/83554/#review85322

My reason is simple. Both ComputeDistance and AddWeighted need to decompose matrix/matrix3d, and StyleAnimationValue.cpp becomes larger and larger (i.e. There are too many static functions and they are mixed with class methods), so I think we can move these functions which related to TransfromMatrix into nsStyleTransformMatrix. I will put this reason in the commit message. Thanks.
Comment on attachment 8800124 [details]
Bug 1272549 - Part 6: Use enum class for shear in decomposition functions.

https://reviewboard.mozilla.org/r/83556/#review85326

> Looking at our style guide for this we have:
> 
> "using namespace ...; is only allowed in .cpp files after all #includes. Prefer to wrap code in namespace ... { ... }; instead if possible.  using namespace ...; should always specify the fully qualified namespace.  That is, to use Foo::Bar do not write using namespace Foo; using namespace Bar;, write using namespace Foo::Bar;"
> 
> [1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Namespaces
> 
> So, I'd suggest you just put "using nsStyleTransformMatrix::Decompose2DMatrix" after the #includes.
> 
> And if possible it would be better to do that in the previous patch and avoid the churn below.

Got it. Thank you. I will put "using xxxxx::Decompose{2D|3D}Matrix" after #includes in the previous patch.
Comment on attachment 8800125 [details]
Bug 1272549 - Part 7: Compute distance for matrix and matrix3d.

https://reviewboard.mozilla.org/r/84846/#review85332

> I trust this is right, but would you mind explaining these calculations? Thanks.

(1) For shear:
I notice that what we get from Decompose2DMatrix() is only XYSHEAR component, and the value is the tan() value of the shear degree, so I can use atan to get the shear angle, and then calcaulte the angle distance.

(2) For rotate:
It is a little bit tricky. Please check the Decompose2DMatrix() again [1]. After getting the rotate angle, we convert it into quaternion vector by this:

aRotate = gfxQuaternion(0, 0, sin(rotate/2), cos(rotate/2));
                              ^^^^^^^^^^^^   ^^^^^^^^^^^^^
                                   z              w

So I can convert it back to the angle distance by atan2f(z, w).

[1] http://searchfox.org/mozilla-central/rev/d96317a351af8aa78ab9847e7feed964bbaac7d7/layout/style/StyleAnimationValue.cpp#1557


Actually, we can also get the angle distance by the inner product of two quaternion vectors, just as what I do in eCSSKeyword_rotate3d.
e.g.
rotate3d(0,0,1, 60deg)  =>  rotate3d(0,0,1, 120deg);

quaternion vector 1: (0, 0, sin(30deg), cos(30deg)) = (0, 0, 1/2, sqrt(3)/2)
quaternion vector 2: (0, 0, sin(60deg), cos(60deg)) = (0, 0, sqrt(3)/2, 1/2)
inner product:  sqrt(3)/4 + sqrt(3)/4 = sqrt(3)/2
Finally, 2 * acos(sqrt(3)/2) = 60deg

However, I think doing atan(...) may be faster than doing "inner product" + acos(...).

> I don't understand this. We're adding up the square of all these differences. Don't we need to take the square root somewhere?

We take the square root in ComputeTransformListDistance() (Part 2). After adding up all the sqaure values from the transform functions in these two matched transform lists, we take the square root there.

Here, we only add up the square of all these component differences for the matrix2d/matrix3d transform function.

> Likewise, would you mind explaining this calculation?

Yes. Just as what I explain above.

I can explain these by our interpolation algorithm: spherical linear interpolation (Slerp).
From our spec [1] and our code [2] and wiki [3]:

Slerp(q1, q2, t) = sin((1-t) * theta)/sin(theta) * q1 + sin(t * theta)/sin(theta) * q2

The |theta| is kind of the angle distance of two vectors (i.e. Subtended angle, the difference of the first and the last points of the arc), and we can get it by "cos(theta) = q1 dot q2" because q1 and q2 are unit vectors. But you will notice that the |theta| is multiplied by 2 in my code because quaternion vector is got by this formular: (x * sin(angle/2), y * sin(angle/2), z * sin(angle/2), cos(angle/2)).
So the final formular is:

angle/2 = theta = acos(q1 dot q2).

[1] https://drafts.csswg.org/css-transforms/#interpolation-of-decomposed-3d-matrix-values
[2] http://searchfox.org/mozilla-central/rev/d96317a351af8aa78ab9847e7feed964bbaac7d7/gfx/thebes/gfxQuaternion.h#35-40
[3] https://en.wikipedia.org/wiki/Slerp

I didn't dig into it much, but I think the subtended angle from two quaternion vectors is what we want.

> Isn't there a better way of doing this? Can we, for example, add a ctor to Matrix4x4 that takes a (fixed-length?) array then use a loop here to build up an array?

OK. I will try to add a constructor for Matrix4x4.
Comment on attachment 8800125 [details]
Bug 1272549 - Part 7: Compute distance for matrix and matrix3d.

https://reviewboard.mozilla.org/r/84846/#review85332

> Yes. Just as what I explain above.
> 
> I can explain these by our interpolation algorithm: spherical linear interpolation (Slerp).
> From our spec [1] and our code [2] and wiki [3]:
> 
> Slerp(q1, q2, t) = sin((1-t) * theta)/sin(theta) * q1 + sin(t * theta)/sin(theta) * q2
> 
> The |theta| is kind of the angle distance of two vectors (i.e. Subtended angle, the difference of the first and the last points of the arc), and we can get it by "cos(theta) = q1 dot q2" because q1 and q2 are unit vectors. But you will notice that the |theta| is multiplied by 2 in my code because quaternion vector is got by this formular: (x * sin(angle/2), y * sin(angle/2), z * sin(angle/2), cos(angle/2)).
> So the final formular is:
> 
> angle/2 = theta = acos(q1 dot q2).
> 
> [1] https://drafts.csswg.org/css-transforms/#interpolation-of-decomposed-3d-matrix-values
> [2] http://searchfox.org/mozilla-central/rev/d96317a351af8aa78ab9847e7feed964bbaac7d7/gfx/thebes/gfxQuaternion.h#35-40
> [3] https://en.wikipedia.org/wiki/Slerp
> 
> I didn't dig into it much, but I think the subtended angle from two quaternion vectors is what we want.

The formular of converting angle-axis to unit quaternion vector:
http://www.euclideanspace.com/maths/geometry/rotations/conversions/angleToQuaternion/
Comment on attachment 8800120 [details]
Bug 1272549 - Part 3: Implement rotate3d with different direction axis.

https://reviewboard.mozilla.org/r/83548/#review85604

::: gfx/thebes/gfxQuaternion.h:45
(Diff revision 1)
> +    // and the angle is |theta|, this formula can be done using
> +    // an extension of Euler's formula:
> +    //   q = cos(theta/2) + (xi + yj + zk)(sin(theta/2))
> +    //     = cos(theta/2) +
> +    //       x*sin(theta/2)i + y*sin(theta/2)j + z*sin(theta/2)k
> +    // Note: aDirection should be an unit vector and

Can we assert that the length of aDirection is 1?

It might be nice if we had strongly typed unit vectors, but that's outside the scope of this bug.
Attachment #8800120 - Flags: review?(tterribe)
Sorry, I'm struggling to get my head around measuring the distance between two unrelated quaternions.

This code was written based on a spec proposal by Tim, so trying him.

Tim, are you still available for reviews?

If not I can try find someone else who understands this math deeply.
Comment on attachment 8800120 [details]
Bug 1272549 - Part 3: Implement rotate3d with different direction axis.

https://reviewboard.mozilla.org/r/83548/#review85638

The math here looks correct to me.
Attachment #8800120 - Flags: review?(tterribe) → review+
Comment on attachment 8800125 [details]
Bug 1272549 - Part 7: Compute distance for matrix and matrix3d.

https://reviewboard.mozilla.org/r/84846/#review85332

> (1) For shear:
> I notice that what we get from Decompose2DMatrix() is only XYSHEAR component, and the value is the tan() value of the shear degree, so I can use atan to get the shear angle, and then calcaulte the angle distance.
> 
> (2) For rotate:
> It is a little bit tricky. Please check the Decompose2DMatrix() again [1]. After getting the rotate angle, we convert it into quaternion vector by this:
> 
> aRotate = gfxQuaternion(0, 0, sin(rotate/2), cos(rotate/2));
>                               ^^^^^^^^^^^^   ^^^^^^^^^^^^^
>                                    z              w
> 
> So I can convert it back to the angle distance by atan2f(z, w).
> 
> [1] http://searchfox.org/mozilla-central/rev/d96317a351af8aa78ab9847e7feed964bbaac7d7/layout/style/StyleAnimationValue.cpp#1557
> 
> 
> Actually, we can also get the angle distance by the inner product of two quaternion vectors, just as what I do in eCSSKeyword_rotate3d.
> e.g.
> rotate3d(0,0,1, 60deg)  =>  rotate3d(0,0,1, 120deg);
> 
> quaternion vector 1: (0, 0, sin(30deg), cos(30deg)) = (0, 0, 1/2, sqrt(3)/2)
> quaternion vector 2: (0, 0, sin(60deg), cos(60deg)) = (0, 0, sqrt(3)/2, 1/2)
> inner product:  sqrt(3)/4 + sqrt(3)/4 = sqrt(3)/2
> Finally, 2 * acos(sqrt(3)/2) = 60deg
> 
> However, I think doing atan(...) may be faster than doing "inner product" + acos(...).

Further explanation for shear part:
The definition of shear factor is "The distance a point moves due to shear divided by the perpendicular distance of a point from the invariant line.", which is also the definition of the tan(shear angle) value.
The spec says the 2d decomposion will return the skew/shear factors of XY [1], and 3d decomposition returns skew/shear factors of XY, YZ, XZ.

[1] The 7.1 unmatrix part of https://www.w3.org/TR/2011/WD-css3-2d-transforms-20111215/
[2] https://drafts.csswg.org/css-transforms/#decomposing-a-3d-matrix
Comment on attachment 8800120 [details]
Bug 1272549 - Part 3: Implement rotate3d with different direction axis.

https://reviewboard.mozilla.org/r/83548/#review85604

> Can we assert that the length of aDirection is 1?
> 
> It might be nice if we had strongly typed unit vectors, but that's outside the scope of this bug.

Sure. I can add an assertion here and reuse the FuzzyEqual() from Matrix.h, like this:

MOZ_ASSERT(mozilla::gfx::FuzzyEqual(aDirection.Length(), 1.0f), "aDirection should be an unit vector");
Attachment #8800119 - Attachment is obsolete: true
Attachment #8800126 - Flags: review?(bbirtles)
Comment on attachment 8800122 [details]
Bug 1272549 - Part 4: Compute distance for none and a valid transform list.

https://reviewboard.mozilla.org/r/83552/#review86446

This seems mostly good to me although I wonder what we should do for perspective()

::: layout/style/StyleAnimationValue.cpp:2536
(Diff revision 2)
> +            arr->Item(1).SetFloatValue(0.0, eCSSUnit_Number);
> +            arr->Item(2).SetFloatValue(0.0, eCSSUnit_Number);
> +            arr->Item(3).SetFloatValue(1.0, eCSSUnit_Number);
> +            arr->Item(4).SetFloatValue(0.0, eCSSUnit_Radian);
> +          } else if (tfunc == eCSSKeyword_perspective) {
> +            arr->Item(1).SetFloatValue(1.0, eCSSUnit_Pixel);

I'm curious. Why is the identity perspective function perspective(1px) ?

Should interpolating from none -> perspective(500px) at progress = 0.0 produce perspective(1px) ? It looks like we currently get the identity matrix for that. Is that because we can't have 'perspective(0px)'.

What's the right thing to do here?

::: layout/style/nsStyleTransformMatrix.cpp:691
(Diff revision 2)
> +  if (!aMatrix) {
> +    return;
> +  }
> +

I think we should either:

(a) pass aMatrix by reference, or
(b) just assert aMatrix is non-null here.

My preference is (a) since compile-time checking is always better than run-time checking, but feel free to do (b) if that's more consistent with the rest of the file or how we pass around |aMatrix| in general.

::: layout/style/nsStyleTransformMatrix.cpp:695
(Diff revision 2)
> +{
> +  if (!aMatrix) {
> +    return;
> +  }
> +
> +  nsCSSKeyword tfunc = TransformFunctionOf(aMatrix);

Should we assert that TransformFunctionOf(aMatrix) is either eCSSKeyword_matrix or eCSSKeyword_matrix3d?
Attachment #8800122 - Flags: review?(bbirtles) → review+
Comment on attachment 8800125 [details]
Bug 1272549 - Part 7: Compute distance for matrix and matrix3d.

https://reviewboard.mozilla.org/r/84846/#review86450

Thanks for the excellent explanation in comment 48. Would you mind including some of that here in this code?

r=me with that and the following comments addressed.

::: gfx/2d/Matrix.h:467
(Diff revision 2)
> +  explicit Matrix4x4Typed(const Array<Float, 16>& aArray)
> +  {
> +    memcpy(components, &aArray[0], sizeof(components));
> +  }

I'm curious. Any reason to prefer Array<Float, 16> over Float[16] ?

::: layout/style/nsStyleTransformMatrix.cpp:1041
(Diff revision 2)
> +  MOZ_ASSERT(TransformFunctionOf(aArray) == eCSSKeyword_matrix &&
> +             aArray->Count() == 7);

Probably should assert aArray is not null too I guess. Likewise below.
Attachment #8800125 - Flags: review?(bbirtles) → review+
Comment on attachment 8800126 [details]
Bug 1272549 - Part 9: Compute distance for mismatched transform lists.

https://reviewboard.mozilla.org/r/84848/#review86456

::: dom/animation/KeyframeUtils.cpp:1569
(Diff revision 2)
>  /**
>   * Get cumulative distances for the paced property.
>   *
>   * @param aValues The computed values returned by GetComputedKeyframeValues.
>   * @param aPacedProperty The paced property.
> + * @param aStyleContext The style context for compute distance on transform.

s/compute/computing/

::: dom/base/nsDOMWindowUtils.cpp:2712
(Diff revision 2)
> -  if (!StyleAnimationValue::ComputeDistance(property, v1, v2, *aResult)) {
> +  Element* element = content->AsElement();
> +  nsIPresShell* shell = element
> +                        ? element->GetUncomposedDoc()->GetShell()
> +                        : nullptr;

(I wonder if we need to null-check |element|. We don't seem to on the lines just above this.)

::: layout/style/StyleAnimationValue.h:90
(Diff revision 2)
> +   * @param aStyleContext The style context used for process translate part on
> +   *                      transform.

'The style context to use for processing the translate part of transforms.'

::: layout/style/StyleAnimationValue.cpp:983
(Diff revision 2)
> +  // We need nsStyleConext and nsPresContext to compute calc() value while
> +  // processing translate part.

s/nsStyleConext/nsStyleContext/
s/calc() value/calc() values/
s/translate part/the translate part of transforms/
Attachment #8800126 - Flags: review?(bbirtles) → review+
Comment on attachment 8800122 [details]
Bug 1272549 - Part 4: Compute distance for none and a valid transform list.

https://reviewboard.mozilla.org/r/83552/#review86446

> I'm curious. Why is the identity perspective function perspective(1px) ?
> 
> Should interpolating from none -> perspective(500px) at progress = 0.0 produce perspective(1px) ? It looks like we currently get the identity matrix for that. Is that because we can't have 'perspective(0px)'.
> 
> What's the right thing to do here?

When doing interpolation for perspective, we convert it into a 3d matrix. And the equivalent matrix is like this [1]:
[ 1, 0,    0, 0 ],
[ 0, 1,    0, 0 ],
[ 0, 0,    1, 0 ],
[ 0, 0, -1/d, 1 ];

I think the correct answer might be an infinite value, which make -1/d ~= 0. But I cannot compute the distance by the infinite value, so use 1px instead to avoid "divided by zero". However, using 1px is not correct, so an alternate way is assigning it a max float value (std::numeric_limits<float>::max), and we normalize the absolute difference (part 4) by the max float value while computing the distance of perspective. If so, I would like to swap the order of part 4 and this patch, and do the revision on that.

[1] https://drafts.csswg.org/css-transforms/#PerspectiveDefined
[2] http://searchfox.org/mozilla-central/rev/2e7511ed5af5497cbd7e2fa5c043efdc1751b34a/layout/style/nsStyleTransformMatrix.cpp#557-560

> I think we should either:
> 
> (a) pass aMatrix by reference, or
> (b) just assert aMatrix is non-null here.
> 
> My preference is (a) since compile-time checking is always better than run-time checking, but feel free to do (b) if that's more consistent with the rest of the file or how we pass around |aMatrix| in general.

OK. I will add an assertion to check the non-null here.

> Should we assert that TransformFunctionOf(aMatrix) is either eCSSKeyword_matrix or eCSSKeyword_matrix3d?

OK. using assertion might be better.
(In reply to Boris Chiou [:boris] from comment #67)
> > I'm curious. Why is the identity perspective function perspective(1px) ?
> > 
> > Should interpolating from none -> perspective(500px) at progress = 0.0 produce perspective(1px) ? It looks like we currently get the identity matrix for that. Is that because we can't have 'perspective(0px)'.
> > 
> > What's the right thing to do here?
> 
> When doing interpolation for perspective, we convert it into a 3d matrix.
> And the equivalent matrix is like this [1]:
> [ 1, 0,    0, 0 ],
> [ 0, 1,    0, 0 ],
> [ 0, 0,    1, 0 ],
> [ 0, 0, -1/d, 1 ];
> 
> I think the correct answer might be an infinite value, which make -1/d ~= 0.
> But I cannot compute the distance by the infinite value, so use 1px instead
> to avoid "divided by zero". However, using 1px is not correct, so an
> alternate way is assigning it a max float value
> (std::numeric_limits<float>::max), and we normalize the absolute difference
> (part 4) by the max float value while computing the distance of perspective.
> If so, I would like to swap the order of part 4 and this patch, and do the
> revision on that.
> 
> [1] https://drafts.csswg.org/css-transforms/#PerspectiveDefined
> [2]
> http://searchfox.org/mozilla-central/rev/
> 2e7511ed5af5497cbd7e2fa5c043efdc1751b34a/layout/style/nsStyleTransformMatrix.
> cpp#557-560

Hi Brian, need your feedback for this comment. Thanks.
Flags: needinfo?(bbirtles)
Comment on attachment 8800125 [details]
Bug 1272549 - Part 7: Compute distance for matrix and matrix3d.

https://reviewboard.mozilla.org/r/84846/#review86450

> I'm curious. Any reason to prefer Array<Float, 16> over Float[16] ?

At first, I think using Array<Float, 16> can do static bounds-checking assertions for operator[], but finally I use memcpy, so I think there is no difference between them. I can change back to Float[16].
Attachment #8800127 - Flags: review?(hiikezoe)
Attachment #8800127 - Flags: review?(hiikezoe)
(In reply to Boris Chiou [:boris] from comment #68)
> (In reply to Boris Chiou [:boris] from comment #67)
> > > I'm curious. Why is the identity perspective function perspective(1px) ?
> > > 
> > > Should interpolating from none -> perspective(500px) at progress = 0.0 produce perspective(1px) ? It looks like we currently get the identity matrix for that. Is that because we can't have 'perspective(0px)'.
> > > 
> > > What's the right thing to do here?
> > 
> > When doing interpolation for perspective, we convert it into a 3d matrix.
> > And the equivalent matrix is like this [1]:
> > [ 1, 0,    0, 0 ],
> > [ 0, 1,    0, 0 ],
> > [ 0, 0,    1, 0 ],
> > [ 0, 0, -1/d, 1 ];
> > 
> > I think the correct answer might be an infinite value, which make -1/d ~= 0.
> > But I cannot compute the distance by the infinite value, so use 1px instead
> > to avoid "divided by zero". However, using 1px is not correct, so an
> > alternate way is assigning it a max float value
> > (std::numeric_limits<float>::max), and we normalize the absolute difference
> > (part 4) by the max float value while computing the distance of perspective.
> > If so, I would like to swap the order of part 4 and this patch, and do the
> > revision on that.
> > 
> > [1] https://drafts.csswg.org/css-transforms/#PerspectiveDefined
> > [2]
> > http://searchfox.org/mozilla-central/rev/
> > 2e7511ed5af5497cbd7e2fa5c043efdc1751b34a/layout/style/nsStyleTransformMatrix.
> > cpp#557-560
> 
> Hi Brian, need your feedback for this comment. Thanks.

Sounds fine to me. Thanks Boris!
Flags: needinfo?(bbirtles)
Comment on attachment 8800127 [details]
Bug 1272549 - Part 10: Test.

https://reviewboard.mozilla.org/r/85134/#review86476

I am happy to review your patch.  I think this patch can be r+ but there is one thing I don't understand.
It's an expected value in the test named 'Test spacing on matched transform lists'.  Please explain it!

::: dom/animation/test/mozilla/file_spacing_transform.html:9
(Diff revision 2)
> +<body>
> +<script>
> +'use strict';
> +
> +// Help function for testing the computed offsets by the distance array.
> +function testOffsets(anim, dist) {

This function should be named assert_animation_offsets() or some sort of?

::: dom/animation/test/mozilla/file_spacing_transform.html:40
(Diff revision 2)
> +  var result = elements.reduce( (prev, curr) => {
> +    prev += (prev.length == 0 ? prefix + "(" : ", ") + curr;
> +    return prev;
> +  }, "" );

Can't we use join() here?

::: dom/animation/test/mozilla/file_spacing_transform.html:109
(Diff revision 2)
> +                                 { transform: "rotate3d(0,0,1,-110deg)" },
> +                                 { transform: "rotate3d(1,0,0,219deg)"} ],
> +                               { spacing: "paced(transform)" });
> +  testOffsets(anim,
> +              [ 0,
> +                10 * Math.PI / 180,

It would be nice to use getAngleDist() here for consistency.

::: dom/animation/test/mozilla/file_spacing_transform.html:139
(Diff revision 2)
> +  var anim = addDiv(t).animate([ { transform: "perspective(1000px)" },
> +                                 { transform: "none" },
> +                                 { transform: "perspective(100px)" },
> +                                 { transform: "perspective(200px)"} ],
> +                               { spacing: "paced(transform)" });
> +  testOffsets(anim, [ 0, 1000 - 1, 100 - 1, 100 ]);

Please add a comment here that 'none' means 'perspective(1px)'.

::: dom/animation/test/mozilla/file_spacing_transform.html:154
(Diff revision 2)
> +  var dist = [ 0,
> +               Math.sqrt(Math.PI * Math.PI + 0),
> +               Math.sqrt(Math.PI * Math.PI + 1000 * 1000),

Hmm I can't understand here. Should this be sqrt(0 * 0 + 1000 * 1000)?

::: dom/animation/test/mozilla/file_spacing_transform.html:168
(Diff revision 2)
> +  const matrix2 = createMatrix([ 1 - 2 * sin(pi/2.0) * sin(pi/2.0),
> +                                 2 * sin(pi/2.0) * cos(pi/2.0),
> +                                 -2 * sin(pi/2.0) * cos(pi/2.0),
> +                                 1 - 2 * sin(pi/2.0) * sin(pi/2.0),

I think this matrix can be written like this.
[ cos(pi), -sin(pi) ]
[ sin(pi),  cos(pi) ]

::: dom/animation/test/mozilla/file_spacing_transform.html:198
(Diff revision 2)
> +  const matrix2 = createMatrix(
> +    [ 1, 0, 0, 0,
> +      0, 1 - 2 * sin(pi/2) * sin(pi/2), 2 * sin(pi/2) * cos(pi/2), 0,
> +      0, -2 * sin(pi/2) * cos(pi/2), 1 - 2 * sin(pi/2) * sin(pi/2), 0,
> +      1000, 0, 0, 1 ],
> +    true);

Likewise here.
Attachment #8800127 - Flags: review?(hiikezoe)
Comment on attachment 8800127 [details]
Bug 1272549 - Part 10: Test.

I did not notice that Brois cleared r?.
Attachment #8800127 - Flags: review?(hiikezoe)
(In reply to Hiroyuki Ikezoe (:hiro) from comment #72)
> Comment on attachment 8800127 [details]
> Bug 1272549 - Part 10: Test.
> 
> I did not notice that Brois cleared r?.

Sorry for that, I just notice I have to double check the formula of perspective so I cleared the r?. I will address your feedback and then resend the review request. Thanks
Comment on attachment 8800126 [details]
Bug 1272549 - Part 9: Compute distance for mismatched transform lists.

https://reviewboard.mozilla.org/r/84848/#review86456

> (I wonder if we need to null-check |element|. We don't seem to on the lines just above this.)

I checked it and we don't do null-check |element| in both ComputeAnimationDistance and ComputeAnimationValue, and we have an assertion to check if IsElement() is true[1]. So looks like we don't need the null-check. I will skip it. Thanks.

[1] http://searchfox.org/mozilla-central/rev/2e7511ed5af5497cbd7e2fa5c043efdc1751b34a/dom/base/Element.h#1460-1470
Comment on attachment 8800127 [details]
Bug 1272549 - Part 10: Test.

https://reviewboard.mozilla.org/r/85134/#review86476

> Can't we use join() here?

Yes, we can use join. I will fix this.

> Please add a comment here that 'none' means 'perspective(1px)'.

I will update this formula, and add some comment to explain it. Thanks.

> Hmm I can't understand here. Should this be sqrt(0 * 0 + 1000 * 1000)?

Yes. I use assert_approx_equals and the epsilon is too large, so I pass thie error locally. I will make epsilon smaller and fix this. Thanks.

> I think this matrix can be written like this.
> [ cos(pi), -sin(pi) ]
> [ sin(pi),  cos(pi) ]

Yes. I should convert them by double angle formula.
Comment on attachment 8800121 [details]
Bug 1272549 - Part 8: Compute distance for perspective transform function.

Hi Brian,

This is needed to be re-reviewed because I change the formula:
1. Using the idea from comment 67 is not good enough. If we try to normalize it by the maximal float, most normalized value are close to zero, so the computed offsets may not be precise enough.

2. The interpolation of perspective is by decomposing the matrix and find its perspective vector, and then do linear interpolation on it. I think it's better to use the same logic as interpolation, so the animation could be really paced. Therefore, my proposal is applying "perspective(infinity)" for none transform, and use matrix decomposition to find the perspective vectors to calculate the distance.
Attachment #8800121 - Flags: review+ → review?(bbirtles)
Comment on attachment 8800127 [details]
Bug 1272549 - Part 10: Test.

https://reviewboard.mozilla.org/r/85134/#review86766

Great!

::: dom/animation/test/mozilla/file_spacing_transform.html:8
(Diff revisions 2 - 3)
> +const pi  = Math.PI;
> +const cos = Math.cos;
> +const sin = Math.sin;
> +const tan = Math.tan;
> +const sqrt = Math.sqrt;
> +

Nice!

::: dom/animation/test/mozilla/file_spacing_transform.html:144
(Diff revisions 2 - 3)
> +  // perspective 1: (0, 0, -1/128, 1);
> +  // perspective 2: (0, 0, -1/256, 1);
> +  // perspective 3: (0, 0, -1/1024, 1);
> +  // perspective 4: (0, 0, -1/32, 1);

IIUC your comment 85, perspective 2 is (0, 0, -1/∞ = 0, 1)?
Attachment #8800127 - Flags: review?(hiikezoe) → review+
Comment on attachment 8800121 [details]
Bug 1272549 - Part 8: Compute distance for perspective transform function.

https://reviewboard.mozilla.org/r/83550/#review86826
Attachment #8800121 - Flags: review?(bbirtles) → review+
Comment on attachment 8800127 [details]
Bug 1272549 - Part 10: Test.

https://reviewboard.mozilla.org/r/85134/#review86766

> IIUC your comment 85, perspective 2 is (0, 0, -1/∞ = 0, 1)?

Yes! I'm so sorry. I will fix this in the new patch.
Pushed by bchiou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1479897f7a86
Part 1: Reorder some static functions. r=birtles
https://hg.mozilla.org/integration/autoland/rev/3eddc9ff6f4b
Part 2: Compute distance for basic 2d & 3d transform functions for matched transform lists. r=birtles
https://hg.mozilla.org/integration/autoland/rev/191bd8c2c0ff
Part 3: Implement rotate3d with different direction axis. r=derf
https://hg.mozilla.org/integration/autoland/rev/da8910563fad
Part 4: Compute distance for none and a valid transform list. r=birtles
https://hg.mozilla.org/integration/autoland/rev/92a5d32ed2da
Part 5: Move decompose matrix to nsStyleTransformMatrix. r=birtles
https://hg.mozilla.org/integration/autoland/rev/be30e778b634
Part 6: Use enum class for shear in decomposition functions. r=birtles
https://hg.mozilla.org/integration/autoland/rev/00e8df3987f9
Part 7: Compute distance for matrix and matrix3d. r=birtles
https://hg.mozilla.org/integration/autoland/rev/6f417fc0c45d
Part 8: Compute distance for perspective transform function. r=birtles
https://hg.mozilla.org/integration/autoland/rev/89e37cf072fe
Part 9: Compute distance for mismatched transform lists. r=birtles
https://hg.mozilla.org/integration/autoland/rev/8a0851bef539
Part 10: Test. r=hiro
Blocks: 1313554
Blocks: 1317914
See Also: → 1362896
See Also: 1362896
Depends on: 1371501
You need to log in before you can comment on or make changes to this bug.