Closed Bug 531344 Opened 10 years ago Closed 9 years ago

implement animation of CSS transforms in nsStyleAnimation

Categories

(Core :: CSS Parsing and Computation, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla2.0b2
Tracking Status
blocking2.0 --- beta2+

People

(Reporter: dbaron, Assigned: dbaron)

References

(Blocks 1 open bug)

Details

Attachments

(8 files, 1 obsolete file)

9.20 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
7.98 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
4.84 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
4.93 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
3.51 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
12.08 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
7.80 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
57.48 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
This bug is for implementing animation of CSS transforms in
nsStyleAnimation (i.e., support CSS transitions of CSS 2D transforms).

The relevant section of the transitions specification:
http://dev.w3.org/csswg/css3-transitions/#animation-of-property-types-
defers all of the details to the 2-D and 3-D transforms specifications.
For the 2-D transforms specification (all that's relevant for us, right
now), the relevant section is:
http://dev.w3.org/csswg/css3-2d-transforms/#animation
This, in turn, refers to the unmatrix program in Graphics Gems,
available from http://tog.acm.org/resources/GraphicsGems/ , and in
particular as the file GraphicsGems/gemsii/unmatrix.c
in http://tog.acm.org/resources/GraphicsGems/AllGems.tar.gz

The unmatrix reference is for general 3-D transform matrices (any of the
16 components can have any value).

For CSS 2-D transforms, we have a 2-D matrix with the bottom row constant:

[ A C E ]
[ B D F ]
[ 0 0 1 ]

For that case, I believe the algorithm in unmatrix reduces to:

 (1) If A * D - B * C == 0, the matrix is singular.  Fail.

 (2) Set translation components (Tx and Ty) to the translation parts of
     the matrix (E and F) and then ignore them for the rest of the time.
     (For us, E and F each actually consist of three constants:  a
     length, a multiplier for the width, and a multiplier for the
     height.  This actually requires its own decomposition, but I'll
     keep that separate.)

 (3) Let the X scale factor (Sx) be sqrt(A^2 + B^2).  Then divide both A
     and B by it.

 (4) Let the XY shear (K) be A * C + B * D.  From C, subtract A times
     the XY shear.  From D, subtract B times the XY shear.

 (5) Let the Y scale (Sy) be sqrt(C^2 + D^2).  Divide C, D, and the XY
     shear (K) by it.

 (6) At this point, A * D - B * C is either 1 or -1.  If it is -1,
     negate all of A, B, C, D, the X scale (Sx), and the Y scale (Sy).

 (7) Let the rotation be R = atan2(B, A).

Then the resulting decomposed transformation is:

  translate(Tx, Ty) rotate(R) skewX(atan(K)) scale(Sx, Sy)

An interesting result of this is that all of the simple transform
functions (i.e., all functions other than matrix()), in isolation,
decompose back to themselves except for:
  'skewY(φ)', which is 'matrix(1, tan(φ), 0, 1, 0, 0)', which decomposes
  to 'rotate(φ) skewX(φ) scale(sec(φ), cos(φ))' since (ignoring the
  alternate sign possibilities that would get fixed in step 6):
    In step 3, the X scale factor is sqrt(1+tan²(φ)) = sqrt(sec²(φ)) = sec(φ).
    Thus, after step 3, A = 1/sec(φ) = cos(φ) and B = tan(φ) / sec(φ) = sin(φ).
    In step 4, the XY shear is sin(φ).
    Thus, after step 4, C = -cos(φ)sin(φ) and D = 1 - sin²(φ) = cos²(φ).
    Thus, in step 5, the Y scale is sqrt(cos²(φ)(sin²(φ) + cos²(φ)) = cos(φ).
    Thus, after step 5, C = -sin(φ), D = cos(φ), and the XY shear is tan(φ).
    Thus, in step 6, A * D - B * C = cos²(φ) + sin²(φ) = 1.
    In step 7, the rotation is thus φ.

  skew(θ, φ), which is matrix(1, tan(φ), tan(θ), 1, 0, 0), which decomposes
  to 'rotate(φ) skewX(θ + φ) scale(sec(φ), cos(φ))' since (ignoring
  the alternate sign possibilities that would get fixed in step 6):
    In step 3, the X scale factor is sqrt(1+tan²(φ)) = sqrt(sec²(φ)) = sec(φ).
    Thus, after step 3, A = 1/sec(φ) = cos(φ) and B = tan(φ) / sec(φ) = sin(φ).
    In step 4, the XY shear is cos(φ)tan(θ) + sin(φ).
    Thus, after step 4,
    C = tan(θ) - cos(φ)(cos(φ)tan(θ) + sin(φ)) = tan(θ)sin²(φ) - cos(φ)sin(φ)
    D = 1 - sin(φ)(cos(φ)tan(θ) + sin(φ)) = cos²(φ) - sin(φ)cos(φ)tan(θ)
    Thus, in step 5, the Y scale is sqrt(C² + D²) = 
    sqrt(tan²(θ)(sin⁴(φ) + sin²(φ)cos²(φ)) -
         2 tan(θ)(sin³(φ)cos(φ) + sin(φ)cos³(φ)) +
         (sin²(φ)cos²(φ) + cos⁴(φ))) =
    sqrt(tan²(θ)sin²(φ) - 2 tan(θ)sin(φ)cos(φ) + cos²(φ)) =
    cos(φ) - tan(θ)sin(φ) (taking the negative of the obvious solution so
    we avoid flipping in step 6).
    After step 5, C = -sin(φ) and D = cos(φ), and the XY shear is
    (cos(φ)tan(θ) + sin(φ)) / (cos(φ) - tan(θ)sin(φ)) =
    (dividing both numerator and denominator by cos(φ))
    (tan(θ) + tan(φ)) / (1 - tan(θ)tan(φ)) = tan(θ + φ).
    (See http://en.wikipedia.org/wiki/List_of_trigonometric_identities .)
    Thus, in step 6, A * D - B * C = cos²(φ) + sin²(φ) = 1.
    In step 7, the rotation is thus φ.

    To check this result, we can multiply things back together:

    [ cos(φ) -sin(φ) ] [ 1 tan(θ + φ) ] [ sec(φ)    0   ]
    [ sin(φ)  cos(φ) ] [ 0      1     ] [   0    cos(φ) ]

    [ cos(φ)      cos(φ)tan(θ + φ) - sin(φ) ] [ sec(φ)    0   ]
    [ sin(φ)      sin(φ)tan(θ + φ) + cos(φ) ] [   0    cos(φ) ]

    but since tan(θ + φ) = (tan(θ) + tan(φ)) / (1 - tan(θ)tan(φ)),
    cos(φ)tan(θ + φ) - sin(φ)
     = cos(φ)(tan(θ) + tan(φ)) - sin(φ) + sin(φ)tan(θ)tan(φ)
     = cos(φ)tan(θ) + sin(φ) - sin(φ) + sin(φ)tan(θ)tan(φ)
     = cos(φ)tan(θ) + sin(φ)tan(θ)tan(φ)
     = tan(θ) (cos(φ) + sin(φ)tan(φ))
     = tan(θ) sec(φ) (cos²(φ) + sin²(φ))
     = tan(θ) sec(φ)
    and
    sin(φ)tan(θ + φ) + cos(φ)
     = sin(φ)(tan(θ) + tan(φ)) + cos(φ) - cos(φ)tan(θ)tan(φ)
     = tan(θ) (sin(φ) - sin(φ)) + sin(φ)tan(φ) + cos(φ)
     = sec(φ) (sin²(φ) + cos²(φ))
     = sec(φ)
    so the above is:
    [ cos(φ)  tan(θ) sec(φ) ] [ sec(φ)    0   ]
    [ sin(φ)     sec(φ)     ] [   0    cos(φ) ]

    [    1   tan(θ) ]
    [ tan(φ)    1   ]



So the algorithms in the spec seem largely reasonable to me.

I think it is valuable to avoid the matrix decomposition for transform
lists that match, since it will produce better results in a number of
cases, I think.

One question I wonder, though, is whether animation of skew should
animate over angle or over number (i.e., the animation function
transitions between two tangents rather than between two angles); I tend
to think it should actually be the latter.
Duplicate of this bug: 554602
This patch series applies on top of the patches in bug 363249 up through patch 22 (though it doesn't need patch 19 or 20 there).
Attachment #454191 - Flags: review?(dholbert)
For transforms, I'm going to implement calc() without min() and max(); that makes it straightforward to convert to the matrix form that we use; without that restriction we'd have to do the full transform computation after we learned the width and height.
Attachment #454194 - Flags: review?(bzbarsky)
Comment on attachment 454195 [details] [diff] [review]
patch 5: fix an odd function signature in nsStyleTransformMatrix

Glancing at patch 5 first, since it looks smallest. :)

>+static nscoord CalcLength(const nsCSSValue &aValue,
>+                          nsStyleContext* aContext,
>+                          nsPresContext* aPresContext,
>+                          PRBool &aCanStoreInRuleTree)
> {
>-  aOut = nsRuleNode::CalcLength(aValue, aContext, aPresContext,
>+  return nsRuleNode::CalcLength(aValue, aContext, aPresContext,
>                                 aCanStoreInRuleTree);
> }

Why do we need this method at all?  Wouldn't it be simpler for the clients of this method to just call nsRuleNode::CalcLength directly?
We don't, really, though it saves an "nsRuleNode::".  I should probably make it inline if we keep it, though.
Here's a revised version of the main patch.  The revision is to allow a small amount of floating point error to accumulate in two of the tests (one, which was previously commented out, is needed on my machine, and for the other test it's needed for Mac try).

With that change, the patches in this bug and the calc() patches they depend on are green on try server.
Attachment #454199 - Attachment is obsolete: true
Attachment #454333 - Flags: review?(dholbert)
Attachment #454199 - Flags: review?(dholbert)
Comment on attachment 454191 [details] [diff] [review]
patch 1: store the specified transform list in nsStyleDisplay

Notes for patch 1:

>     /* If we need to inherit, do so by making a full deep-copy. */
>     else if (head->mValue.GetUnit() == eCSSUnit_Inherit)  {
>-      display->mTransformPresent = parentDisplay->mTransformPresent;
>-      if (parentDisplay->mTransformPresent)
>+      display->mSpecifiedTransform = parentDisplay->mSpecifiedTransform;
>+      if (parentDisplay->mSpecifiedTransform)

As we just spoke about, I think this comment needs updating (sine the code doesn't actually do a deep-copy).  Otherwise, looks good; r=dholbert
Attachment #454191 - Flags: review?(dholbert) → review+
Comment on attachment 454192 [details] [diff] [review]
patch 2: add helper functions for some common nsCSSValue interpolations

r=dholbert on patch 2
Attachment #454192 - Flags: review?(dholbert) → review+
Comment on attachment 454193 [details] [diff] [review]
patch 3: add common function for setting pieces of transform matrix to a translation

r=dholbert on patch 3
Attachment #454193 - Flags: review?(dholbert) → review+
Attachment #454195 - Flags: review?(dholbert) → review+
Comment on attachment 454195 [details] [diff] [review]
patch 5: fix an odd function signature in nsStyleTransformMatrix

(In reply to comment #11)
> We don't, really, though it saves an "nsRuleNode::".  I should probably make it
> inline if we keep it, though.

r=dholbert if you make the CalcLength helper-function inline.  You also should probably update the comment above it to mention something like "Convenience wrapper for RuleNode::CalcLength".

(or if you prefer, this patch could just use direct calls to nsRuleNode::CalcLength everywhere -- r=dholbert either way)
(In reply to comment #16)
> r=dholbert if you make the CalcLength helper-function inline.  You also should
> probably update the comment above it to mention something like "Convenience
> wrapper for RuleNode::CalcLength".
> 
> (or if you prefer, this patch could just use direct calls to
> nsRuleNode::CalcLength everywhere -- r=dholbert either way)

Actually, the reason it's not inline is that it becomes more complicated in the main patch in this bug.  (I just explained that to dholbert verbally, and he was ok with it.)
Comment on attachment 454198 [details] [diff] [review]
patch 7: move ReadTransforms from nsRuleNode to nsStyleTransformMatrix

r=dholbert on patch 7
Attachment #454198 - Flags: review?(dholbert) → review+
Comment on attachment 454333 [details] [diff] [review]
patch 8 (main patch): implement animation of -moz-transform property

>diff --git a/layout/style/nsStyleAnimation.cpp b/layout/style/nsStyleAnimation.cpp
>--- a/layout/style/nsStyleAnimation.cpp
>+++ b/layout/style/nsStyleAnimation.cpp
>+      double squareDistance = 0.0f;

Nit: I don't think we need/want the "f" suffix there, since it's getting stored in a double. (applies to a few other places in this file, too)

>+AppendTransformFunction(nsCSSKeyword aTransformFunction,

Could you add an assertion to check that aTransformFunction is a valid transform function? (and not some arbitrary nsCSSKeyword)

>+  nsRefPtr<nsCSSValue::Array> arr = nsCSSValue::Array::Create(nargs + 1);
>+  if (!arr) {
>+    return nsnull;
>+  }

No need to null-check 'arr' here -- nsCSSValue::Array::Create uses infallible-new under the hood. (or, if you'd prefer to keep the null-check to allow for ::Create to be fallible in the future, then there are a few not-null-checked ::Create calls elsewhere in this patch that would need checking.)

>+  nsCSSValueList *item = new nsCSSValueList;
>+  if (!item) {
>+    return nsnull;
>+  }

No need to null-check 'item'.

>+ *  (3) Let the X scale factor (Sx) be sqrt(A^2 + B^2).  Then divide both A
>+ *      and B by it.
[...]
>+ *  (5) Let the Y scale (Sy) be sqrt(C^2 + D^2).  Divide C, D, and the XY
>+ *      shear (K) by it.

Nit: s/Y scale/Y scale factor/, for consistency with "X scale factor" in (3). (or, remove "factor" from (3))

>+ *  (6) At this point, A * D - B * C is either 1 or -1.  If it is -1,
>+ *      negate the XY shear (K) and the Y scale (Sy).
[...]
>+  if (A * D < B * C) { // A*D - B*C should be 1 or -1
>+    A = -A;
>+    B = -B;
>+    C = -C;
>+    D = -D;
>+    XYshear = -XYshear;
>+    scaleX = -scaleX;
>+  }

The code here doesn't match the documentation (quoted above). In particular:
 - The documentation says to negate the *Y* scale, but the code negate the *X* scale instead.
 - The documentation doesn't mention negating A/B/C/D, and yet the code negates them (which I think has no effect in the case of C & D, since those variables aren't used after this block).

Can you fix the code & documentation so they're in sync on the above points?

Also, could you add an assertion or an NS_ABORT_IF_FALSE just above this block to verify that your "A*D - B*C should be 1 or -1" claim holds? (within some epsilon of float error)

>+      arr = AppendTransformFunction(tfunc, resultTail);
>+      if (!arr) {
>+        return nsnull;
>+      }

Given the null-check-removals that I suggested above, I think AppendTransformFunction as a whole is infallible.  So I don't think you need to null-check its return value here.  (There are other places in the patch where you already don't null-check its return value.)

>+AddTransformLists(const nsCSSValueList* aList1, double aCoeff1,
[...]
>+        nsCSSValue zero(0.0f, eCSSUnit_Pixel);
[...]
>+        nsCSSValue zero(0.0f, eCSSUnit_Radian);

Looks like these |zero| values are never modified -- can we be declare them as |static const| so that we don't have to re-initialize them every time we traverse this code?


>+        AddTransformTranslate(a1->Count() == 3 ? a1->Item(2) : zero,
>+                              aCoeff1,
>+                              a2->Count() == 3 ? a2->Item(2) : zero,
>+                              aCoeff2,
>+                              arr->Item(2));
>+        AddTransformTranslate(a1->Item(1), aCoeff1, a2->Item(1), aCoeff2,
>+                              arr->Item(1));

The ordering here (handling 'y' before 'x') seems a bit counterintuitive -- seems to me like it would make more sense to handle 'x' before 'y'.

Regardless of the ordering, though, could you add comments above each of the AddTransformTranslate calls to say:
  // add x component of translation
and
  // add y component of translation
(With the 1/2/3/zero/a1/a2/etc values and no mention of 'x' or 'y', it takes a bit of work to parse what's going on right now, at first glance.)


Both of these points apply to the "scale" and "skew" cases in the same switch statement.


>@@ -872,16 +1447,86 @@ nsStyleAnimation::AddWeighted(nsCSSPrope
[...]
>+          result = new nsCSSValueList;
>+          if (result) {
>+            result->mValue.SetNoneValue();
>+          }

No need to null-check |result| here.

>+      if (!result) {
>+        return PR_FALSE;
>+      }

(This is at the end of the Transform clause inside of "AddWeighted")
Do we actually need this check? That is, can |result| actually be null here?  It looks to me like it gets set to something non-null in every single case.  But I might be missing something.

>+SubstitutePixelValues(nsStyleContext* aStyleContext,
[...]
>+    PRBool canStoreInRuleTree = PR_TRUE;
>+    nscoord len = nsRuleNode::CalcLength(aInput, aStyleContext,
>+                                         aStyleContext->PresContext(),
>+                                         canStoreInRuleTree);

I'd get rid of the "PR_TRUE" initialization there.  |canStoreInRuleTree| is an outparam (which you're ignoring), so it's a bit misleading to initialize it to something. (Mostly for consistency -- elsewhere in this patch, you pass a boolean named "dummy" as an |aCanStoreInRuleTree| outparam, and you don't initialize |dummy| to anything.)

>@@ -1666,16 +2342,51 @@ nsStyleAnimation::ExtractComputedValue(n
>+              nsCSSValueList *clone = new nsCSSValueList;
>+              if (!clone) {
>+                result = nsnull;
>+                break;
>+              }

No need to null-check |clone| here.

>+            result = new nsCSSValueList();
>+            if (result) {
>+              result->mValue.SetNoneValue();

No need to null-check |result| here.

>+          if (!result) {
>+            NS_WARNING("out of memory");
>+            return PR_FALSE;
>+          }

I don't think we could ever hit this OOM clause (right?), so it could probably be removed or converted into an NS_ABORT_IF_FALSE.

>+++ b/layout/style/nsStyleTransformMatrix.cpp
[...]
> /* Helper function to fill in an nscoord with the specified nsCSSValue. */
> static nscoord CalcLength(const nsCSSValue &aValue,
[...]
>+    // fine) so that callers aare allowed to pass a null style context

s/aare/are/

>+  /**
>+   * Return the transform function, as an nsCSSKeyword, for the given
>+   * nsCSSValue::Array from a transform list.
>+   */
>+/* static */ nsCSSKeyword
>+nsStyleTransformMatrix::TransformFunctionOf(const nsCSSValue::Array* aData)
>+{

That header comment is indented 2 spaces here (probably from being copied from .h-->.cpp file) -- nix the indentation here.

>+++ b/layout/style/nsStyleTransformMatrix.h
>+   * aContext and aPresContext may be null if all of the length values in
>+   * aData are already known to have been converted to eCSSUnit_Pixel (as
>+   * they are in an nsStyleAnimation::Value)
>    */
>   void SetToTransformFunction(const nsCSSValue::Array* aData,

To avoid ambiguity, it's probably worth indicating in the comment above that percent values don't need to have been converted. Perhaps replace "length values" with "non-percent length values", or with "length values (not percents)"?

You do have comments elsewhere about percent-values being OK, but those comments are both inside the .cpp file (in ExtractComputedValue and in your static CalcLength() function, to be specific), and it seems like this is something worth documenting in the header file.

r=dholbert with the above fixed/addressed.
Attachment #454333 - Flags: review?(dholbert) → review+
Comment on attachment 454194 [details] [diff] [review]
patch 4: allow CSS parser to forbid min()/max() within certain calc() expressions

r=bzbarsky
Attachment #454194 - Flags: review?(bzbarsky) → review+
Comment on attachment 454197 [details] [diff] [review]
patch 6: add support for calc() expressions in translate functions of -moz-transform

r=bzbarsky
Attachment #454197 - Flags: review?(bzbarsky) → review+
(In reply to comment #19)
> >+AddTransformLists(const nsCSSValueList* aList1, double aCoeff1,
> [...]
> >+        nsCSSValue zero(0.0f, eCSSUnit_Pixel);
> [...]
> >+        nsCSSValue zero(0.0f, eCSSUnit_Radian);
> 
> Looks like these |zero| values are never modified -- can we be declare them as
> |static const| so that we don't have to re-initialize them every time we
> traverse this code?

I'd actually prefer not to prematurely optimize in this case, especially for an optimization that has an even odds chance of being slower rather than faster.

> >+        AddTransformTranslate(a1->Count() == 3 ? a1->Item(2) : zero,
> >+                              aCoeff1,
> >+                              a2->Count() == 3 ? a2->Item(2) : zero,
> >+                              aCoeff2,
> >+                              arr->Item(2));
> >+        AddTransformTranslate(a1->Item(1), aCoeff1, a2->Item(1), aCoeff2,
> >+                              arr->Item(1));
> 
> The ordering here (handling 'y' before 'x') seems a bit counterintuitive --
> seems to me like it would make more sense to handle 'x' before 'y'.

I did it because the compiler is probably more likely to merge with the case below (which can be done non-DEBUG only) if the code is in that order, I think.

> Regardless of the ordering, though, could you add comments above each of the
> AddTransformTranslate calls to say:
>   // add x component of translation
> and
>   // add y component of translation
> (With the 1/2/3/zero/a1/a2/etc values and no mention of 'x' or 'y', it takes a
> bit of work to parse what's going on right now, at first glance.)

Yeah, I'll add comments, and mention the reason for the order.

> I'd get rid of the "PR_TRUE" initialization there.  |canStoreInRuleTree| is an
> outparam (which you're ignoring), so it's a bit misleading to initialize it to
> something. (Mostly for consistency -- elsewhere in this patch, you pass a
> boolean named "dummy" as an |aCanStoreInRuleTree| outparam, and you don't
> initialize |dummy| to anything.)

It's not a true out-param; it's a param that gets false written to it but never gets true written to it.  I'd sort of prefer to leave the initialization.

I have everything else addressed locally.
Thanks, that sounds good.

RE the pseudo-out-param -- maybe consider whether that logic applies to |dummy|, too (which currently isn't initialized to anything, and gets passed into nsStyleTransformMatrix::ReadTransforms). Maybe that one's more of a 'true' outparam, though, and that's the difference?  Anyway, I trust your judgement on that, of course. :)
I think the difference is that I don't care what value a variable called |dummy| has, but if it has a sensible name it ought to have a sensible value, and the code is a little clearer when it has a sensible name.
And if you notice it crashing, that's probably bug 576761; I'll try to get that fix in for tomorrow's nightly.
Depends on: 576761
(In reply to comment #19)
> >+ *  (6) At this point, A * D - B * C is either 1 or -1.  If it is -1,
> >+ *      negate the XY shear (K) and the Y scale (Sy).
> [...]
> >+  if (A * D < B * C) { // A*D - B*C should be 1 or -1
> >+    A = -A;
> >+    B = -B;
> >+    C = -C;
> >+    D = -D;
> >+    XYshear = -XYshear;
> >+    scaleX = -scaleX;
> >+  }
> 
> The code here doesn't match the documentation (quoted above). In particular:
>  - The documentation says to negate the *Y* scale, but the code negate the *X*
> scale instead.
>  - The documentation doesn't mention negating A/B/C/D, and yet the code negates
> them (which I think has no effect in the case of C & D, since those variables
> aren't used after this block).
> 
> Can you fix the code & documentation so they're in sync on the above points?

Looks like this point wasn't fixed in the final push [1] (the first half seeming more important, about Sx vs Sy) -- mind investigating & fixing whichever of the code vs. documentation is incorrect?

[1] http://hg.mozilla.org/mozilla-central/rev/f2b02ba56bdd
The final push had:

   2.247 + *  (6) At this point, A * D - B * C is either 1 or -1.  If it is -1,
   2.248 + *      negate the XY shear (K), the X scale (Sy), and A, B, C, and D.
   2.249 + *      (Alternatively, we could negate the XY shear (K) and the Y scale
   2.250 + *      (Sy).)

In other words, we have a choice between:
 (1) negating K, Sx, A, B, (C, D)
 (2) negating K, Sy
Since the spec isn't clear (due to the problems described in http://lists.w3.org/Archives/Public/www-style/2010Jun/0602.html ) I just picked one.

The final push does have a single typo, though:  "X scale (Sy)" instead of "X scale (Sx)".
Ah, sorry for overstating it -- I'd just glanced at the comment and saw the Sy, and then saw that the code part hadn't changed & still had scaleX. (but I missed that the rest of the comment had changed).

So, yeah, just a s/Sy/Sx/ is all that's needed, looks like.  Thanks!
Depends on: 578530
Depends on: 582111
Depends on: 582564
Depends on: 576980
Depends on: 584569
You need to log in before you can comment on or make changes to this bug.