fix(convertTransform): fix scale and rotate on skew + refactors

[SethFalco]

I have divided this pull request into two commits, if you want to review it, please look at one commit at a time:

1. Functional changes relevant to the bug being fixed
2. Various refactors

When we encounter a transform matrix with both shears (a B C d e f) set or unset, we conditionally invert sx and sy. This is needed most of the time to preserve the original sign (+/-) of scales (A b c D e f) since this gets lost after squaring in Math.hypot.

The previous implementation lead us to make conversions like the following, which are both wrong:

Matrix	Expected	Actual	Notes
matrix(0 -1 -1 0 0 0)	scale(1 -1)rotate(90)	rotate(-90)
matrix(0 1 1 0 0 0)	matrix(0 1 1 0 0 0)	rotate(90)	The decomposed matrix is scale(1 -1)rotate(-90), but that's long, so we scrap it.

While looking into matrix decomposition, my understanding is that if b and c have the same sign (positive / negative), this effectively rotates and inverts sy (negative vertical scaling), even if a and d weren't explicitly set. We ignore positive d values because in this block, if b and c are non-zero and have the same sign, and a and d are non-zero, they should have opposite signs, so we can imply that we've already inverted sx and will update the rotation. That's done so we can define a shorter transform and keep rotations to 2 digits only.

i.e. rotate(-135)scale(1.4 -1.4) vs scale(-1.4 1.4)rotate(-45)

Anyone with more experience in math and applying or decomposing transformation matrices would be very welcome to chime in.

… Frederic Wang's website puts the scale for matrix(0 -1 -1 0 0 0) inside/after, like rotate(-90) scale(1,-1), but we set scaleBefore to false and put the scale outside/first.
* The fact that "before" is vague makes things more confusing. Is "before" defined as coming first in the string, or as applying first to the object as it's innermost?

— #1892 (comment)

Answering this here, as I now have more context. We do an optimization on top of the algorithm used on Frederic Wang's website to try to keep rotations to 2 digits. For example, where they would return rotation(-135)scale(…) we'll make it scale(…)rotation(-45) and invert the scales appropriately, which saves 1 byte. We just weren't doing the inversions correctly in some cases.

Refactors

Use toFixed when comparing scales (A b c D e f). This makes the output more consistent. For example, all the following transforms should yield the same output, but if the calculated matrix has a value like 1e-6 then it failed to meet the condition before:

• matrix(0, -1, -1, 0, 0, 0)
• scale(1 -1)rotate(90)
• scale(-1 1)rotate(-90)

Since our stringless toFixed method has been migrated to a utility anyway, I've updated some instances of Number(string.toFixed(num)) to use it to avoid all the casting.

Renames significantDigits to numberOfDigits because that's what it is, not that number of significant digits.

Uses a Set to match transformation names instead of a regular expression for performance.

---

This takes us down from 255 failing tests in the SVGO Test Suite (proposed regression tests) down to 172 and finishes in 19.3 minutes. I've also confirmed it only resolves tests and does not break others. 🎉

Related

Discussed here:

• feat(convertPathData): convert c to q #1892 (comment)

Background on sign issues in the past:

• 9b84827
• eeeb67d