Closed
Bug 1272549
Opened 4 years ago
Closed 3 years ago
Support paced spacing for transform
Categories
(Core :: DOM: Animation, defect)
Core
DOM: Animation
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/xreviewboardrequest

birtles
:
review+

Details 
58 bytes,
text/xreviewboardrequest

birtles
:
review+

Details 
58 bytes,
text/xreviewboardrequest

derf
:
review+

Details 
58 bytes,
text/xreviewboardrequest

birtles
:
review+

Details 
58 bytes,
text/xreviewboardrequest

birtles
:
review+

Details 
58 bytes,
text/xreviewboardrequest

birtles
:
review+

Details 
58 bytes,
text/xreviewboardrequest

birtles
:
review+

Details 
58 bytes,
text/xreviewboardrequest

birtles
:
review+

Details 
58 bytes,
text/xreviewboardrequest

birtles
:
review+

Details 
58 bytes,
text/xreviewboardrequest

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/mozillacentral/rev/3461f3cae78495f100a0f7d3d2e0b89292d3ec02/layout/style/StyleAnimationValue.cpp#864872
Assignee  
Updated•4 years ago

Assignee  
Comment 1•4 years ago


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/cssshapes/#basicshapeinterpolation [2] https://drafts.fxtf.org/filters/#animationoffilters 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?
Comment 2•4 years ago


(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/webanimations/#transformlistanimationtypesection > 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/cssshapes/#basicshapeinterpolation > [2] https://drafts.fxtf.org/filters/#animationoffilters I think it's quite fine to split this into separate bugs. However, noone'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 reuse 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/csstransforms/#interpolationoftransforms
Assignee  
Updated•4 years ago

Summary: Support paced spacing on transform, shape, and filter function → Support paced spacing for transform
Assignee  
Updated•3 years ago

Assignee: nobody → boris.chiou
Assignee  
Updated•3 years ago

Status: NEW → ASSIGNED
Assignee  
Comment 3•3 years ago


(In reply to Brian Birtles (:birtles) from comment #2) > I haven't really thought about these properly but we can possibly reuse > 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/csstransforms/#interpolationoftransforms 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/webanimations/#issue789f9fd1
Comment 4•3 years ago


(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 asis? 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 resultin many cases the developer can't freely reorder the list without affecting the result so I don't think they can use the order of functions to choose how spacing should work.
Comment hidden (typo) 
Assignee  
Comment 6•3 years ago


(In reply to Brian Birtles (:birtles, busy 1923 Sep) from comment #4) > Why do we need to normalize? Can't we just use the result asis? 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 nonsmoothing, 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/csstransforms/#decomposinga3dmatrix 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((200100)^2 + (5545)^2 + (100200)^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/mozillacentral/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.
Assignee  
Comment 7•3 years ago


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/csstransforms/#transformprimitives) 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/csstransforms/#decomposinga2dmatrix) 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/csstransforms/#decomposinga3dmatrix) 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.
Assignee  
Updated•3 years ago

Assignee  
Comment 8•3 years ago


(In reply to Boris Chiou [:boris] from comment #7) > 4. None and None > Nothing happens This means the distance is 0.0.
Comment hidden (typo) 
Comment hidden (obsolete) 
Assignee  
Comment 11•3 years ago


(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 xaxis component by TransformReferenceBox::Width(), yaxis component by TransformReferenceBox::Height() [1], but I don't have any idea now for zaxis. (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 xaxis component, TransformReferenceBox::Height() for yaxis. But I still have no idea for zaxis 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/mozillacentral/rev/8910ca900f826a9b714607fd23bfa1b37a191eca/layout/style/nsStyleTransformMatrix.h#102,106
Comment 12•3 years ago


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 suboptimal 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.
Assignee  
Comment 13•3 years ago


(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 suboptimal 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).
Comment hidden (mozreviewrequest) 
Comment hidden (mozreviewrequest) 
Comment hidden (mozreviewrequest) 
Comment hidden (mozreviewrequest) 
Comment hidden (mozreviewrequest) 
Comment hidden (mozreviewrequest) 
Comment hidden (mozreviewrequest) 
Comment hidden (mozreviewrequest) 
Comment hidden (mozreviewrequest) 
Comment hidden (mozreviewrequest) 
Comment hidden (mozreviewrequest) 
Assignee  
Comment 25•3 years ago


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 17components 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.
Assignee  
Updated•3 years ago

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 26•3 years ago


mozreviewreview 
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 hidden (obsolete) 
Assignee  
Comment 28•3 years ago


mozreviewreview 
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 };
Assignee  
Comment 29•3 years ago


mozreviewreviewreply 
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 30•3 years ago


mozreviewreview 
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 31•3 years ago


mozreviewreview 
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 belowI 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/csstransforms/#rotatethreefunction 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 nonnull (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 nonnull before dereferencing it in the next iteration of the loop) Also, I think we expect aListX>mValue.GetArrayValue() to be nonnull 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+
Comment 32•3 years ago


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 reassign the reviews to hiro or someone else if you want to land this soon.
Assignee  
Comment 33•3 years ago


(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 reassign 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.
Comment 34•3 years ago


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)
Updated•3 years ago

Attachment #8800120 
Flags: review?(bbirtles) → review?(matt.woodrow)
Comment 35•3 years ago


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 36•3 years ago


mozreviewreview 
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 37•3 years ago


mozreviewreview 
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 reuse the code we have for interpolating 'none'. If that's not possible, please rerequest 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)
Assignee  
Comment 38•3 years ago


(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)
Assignee  
Updated•3 years ago

Attachment #8800119 
Flags: review?(bbirtles)
Assignee  
Updated•3 years ago

Attachment #8800126 
Flags: review?(bbirtles)
Assignee  
Comment 39•3 years ago


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 mc.)
Assignee  
Comment 40•3 years ago


mozreviewreviewreply 
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( <lengthpercentage> , <lengthpercentage> , <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/csstransforms1/#funcdeftranslate3d > Do we ever encounter the 3 argument form of rotate? > > rotate( <angle> [, <length>, <length>]? )[1] > > [1] https://drafts.csswg.org/csstransforms/#rotatethreefunction > > 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/mozillacentral/rev/d96317a351af8aa78ab9847e7feed964bbaac7d7/layout/style/StyleAnimationValue.cpp#22602271 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/mozillacentral/rev/d96317a351af8aa78ab9847e7feed964bbaac7d7/layout/style/nsStyleTransformMatrix.cpp#662665 > Should we assert that aList1 and aList2 are nonnull (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 nonnull before dereferencing it in the next iteration of the loop) > > Also, I think we expect aListX>mValue.GetArrayValue() to be nonnull 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.
Assignee  
Comment 41•3 years ago


mozreviewreviewreply 
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.
Assignee  
Comment 42•3 years ago


mozreviewreviewreply 
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 43•3 years ago


mozreviewreview 
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 44•3 years ago


mozreviewreview 
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/enUS/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 45•3 years ago


mozreviewreview 
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 (fixedlength?) array then use a loop here to build up an array?
Attachment #8800125 
Flags: review?(bbirtles)
Assignee  
Comment 46•3 years ago


mozreviewreviewreply 
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.
Assignee  
Comment 47•3 years ago


mozreviewreviewreply 
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/enUS/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{2D3D}Matrix" after #includes in the previous patch.
Assignee  
Comment 48•3 years ago


mozreviewreviewreply 
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/mozillacentral/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((1t) * 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/csstransforms/#interpolationofdecomposed3dmatrixvalues [2] http://searchfox.org/mozillacentral/rev/d96317a351af8aa78ab9847e7feed964bbaac7d7/gfx/thebes/gfxQuaternion.h#3540 [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 (fixedlength?) array then use a loop here to build up an array? OK. I will try to add a constructor for Matrix4x4.
Assignee  
Comment 49•3 years ago


mozreviewreviewreply 
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((1t) * 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/csstransforms/#interpolationofdecomposed3dmatrixvalues > [2] http://searchfox.org/mozillacentral/rev/d96317a351af8aa78ab9847e7feed964bbaac7d7/gfx/thebes/gfxQuaternion.h#3540 > [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 angleaxis to unit quaternion vector: http://www.euclideanspace.com/maths/geometry/rotations/conversions/angleToQuaternion/
Comment 50•3 years ago


mozreviewreview 
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.
Updated•3 years ago

Attachment #8800120 
Flags: review?(tterribe)
Comment 51•3 years ago


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 52•3 years ago


mozreviewreview 
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+
Assignee  
Comment 53•3 years ago


mozreviewreviewreply 
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/mozillacentral/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/WDcss32dtransforms20111215/ [2] https://drafts.csswg.org/csstransforms/#decomposinga3dmatrix
Assignee  
Comment 54•3 years ago


mozreviewreviewreply 
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");
Comment hidden (mozreviewrequest) 
Comment hidden (mozreviewrequest) 
Comment hidden (mozreviewrequest) 
Comment hidden (mozreviewrequest) 
Comment hidden (mozreviewrequest) 
Comment hidden (mozreviewrequest) 
Comment hidden (mozreviewrequest) 
Comment hidden (mozreviewrequest) 
Comment hidden (mozreviewrequest) 
Assignee  
Updated•3 years ago

Attachment #8800119 
Attachment is obsolete: true
Assignee  
Updated•3 years ago

Attachment #8800126 
Flags: review?(bbirtles)
Comment 64•3 years ago


mozreviewreview 
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 nonnull here. My preference is (a) since compiletime checking is always better than runtime 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 65•3 years ago


mozreviewreview 
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 66•3 years ago


mozreviewreview 
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 nullcheck 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+
Assignee  
Comment 67•3 years ago


mozreviewreviewreply 
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/csstransforms/#PerspectiveDefined [2] http://searchfox.org/mozillacentral/rev/2e7511ed5af5497cbd7e2fa5c043efdc1751b34a/layout/style/nsStyleTransformMatrix.cpp#557560 > I think we should either: > > (a) pass aMatrix by reference, or > (b) just assert aMatrix is nonnull here. > > My preference is (a) since compiletime checking is always better than runtime 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 nonnull here. > Should we assert that TransformFunctionOf(aMatrix) is either eCSSKeyword_matrix or eCSSKeyword_matrix3d? OK. using assertion might be better.
Assignee  
Comment 68•3 years ago


(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/csstransforms/#PerspectiveDefined > [2] > http://searchfox.org/mozillacentral/rev/ > 2e7511ed5af5497cbd7e2fa5c043efdc1751b34a/layout/style/nsStyleTransformMatrix. > cpp#557560 Hi Brian, need your feedback for this comment. Thanks.
Flags: needinfo?(bbirtles)
Assignee  
Comment 69•3 years ago


mozreviewreviewreply 
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 boundschecking assertions for operator[], but finally I use memcpy, so I think there is no difference between them. I can change back to Float[16].
Assignee  
Updated•3 years ago

Attachment #8800127 
Flags: review?(hiikezoe)
Assignee  
Updated•3 years ago

Attachment #8800127 
Flags: review?(hiikezoe)
Comment 70•3 years ago


(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/csstransforms/#PerspectiveDefined > > [2] > > http://searchfox.org/mozillacentral/rev/ > > 2e7511ed5af5497cbd7e2fa5c043efdc1751b34a/layout/style/nsStyleTransformMatrix. > > cpp#557560 > > Hi Brian, need your feedback for this comment. Thanks. Sounds fine to me. Thanks Boris!
Flags: needinfo?(bbirtles)
Comment 71•3 years ago


mozreviewreview 
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 72•3 years ago


Comment on attachment 8800127 [details] Bug 1272549  Part 10: Test. I did not notice that Brois cleared r?.
Attachment #8800127 
Flags: review?(hiikezoe)
Assignee  
Comment 73•3 years ago


(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
Assignee  
Comment 74•3 years ago


mozreviewreviewreply 
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 nullcheck element. We don't seem to on the lines just above this.) I checked it and we don't do nullcheck 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 nullcheck. I will skip it. Thanks. [1] http://searchfox.org/mozillacentral/rev/2e7511ed5af5497cbd7e2fa5c043efdc1751b34a/dom/base/Element.h#14601470
Comment hidden (typo) 
Comment hidden (typo) 
Assignee  
Comment 77•3 years ago


mozreviewreviewreply 
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 hidden (mozreviewrequest) 
Comment hidden (mozreviewrequest) 
Comment hidden (mozreviewrequest) 
Comment hidden (mozreviewrequest) 
Comment hidden (mozreviewrequest) 
Comment hidden (mozreviewrequest) 
Comment hidden (mozreviewrequest) 
Assignee  
Comment 85•3 years ago


Comment on attachment 8800121 [details] Bug 1272549  Part 8: Compute distance for perspective transform function. Hi Brian, This is needed to be rereviewed 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 86•3 years ago


mozreviewreview 
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 87•3 years ago


mozreviewreview 
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+
Assignee  
Comment 88•3 years ago


mozreviewreviewreply 
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.
Comment hidden (mozreviewrequest) 
Comment hidden (mozreviewrequest) 
Comment hidden (mozreviewrequest) 
Comment hidden (mozreviewrequest) 
Comment hidden (mozreviewrequest) 
Comment hidden (mozreviewrequest) 
Comment hidden (mozreviewrequest) 
Comment hidden (mozreviewrequest) 
Comment 97•3 years ago


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
Comment 98•3 years ago


bugherder 
https://hg.mozilla.org/mozillacentral/rev/1479897f7a86 https://hg.mozilla.org/mozillacentral/rev/3eddc9ff6f4b https://hg.mozilla.org/mozillacentral/rev/191bd8c2c0ff https://hg.mozilla.org/mozillacentral/rev/da8910563fad https://hg.mozilla.org/mozillacentral/rev/92a5d32ed2da https://hg.mozilla.org/mozillacentral/rev/be30e778b634 https://hg.mozilla.org/mozillacentral/rev/00e8df3987f9 https://hg.mozilla.org/mozillacentral/rev/6f417fc0c45d https://hg.mozilla.org/mozillacentral/rev/89e37cf072fe https://hg.mozilla.org/mozillacentral/rev/8a0851bef539
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
statusfirefox52:
 → fixed
Resolution:  → FIXED
Target Milestone:  → mozilla52
You need to log in
before you can comment on or make changes to this bug.
Description
•