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

lib/path.js

@@ -1,6 +1,6 @@
 'use strict';
 
-const { removeLeadingZero } = require('./svgo/tools');
+const { removeLeadingZero, toFixed } = require('./svgo/tools');
 
 /**
  * @typedef {import('./types').PathDataItem} PathDataItem
@@ -250,8 +250,7 @@ exports.parsePathData = parsePathData;
  */
 const roundAndStringify = (number, precision) => {
   if (precision != null) {
-    const ratio = 10 ** precision;
-    number = Math.round(number * ratio) / ratio;
+    number = toFixed(number, precision);
   }
 
   return {

lib/svgo/tools.js

@@ -229,3 +229,17 @@ const findReferences = (attribute, value) => {
   return results.map((body) => decodeURI(body));
 };
 exports.findReferences = findReferences;
+
+/**
+ * Does the same as {@link Number.toFixed} but without casting
+ * the return value to a string.
+ *
+ * @param {number} num
+ * @param {number} precision
+ * @returns {number}
+ */
+const toFixed = (num, precision) => {
+  const pow = 10 ** precision;
+  return Math.round(num * pow) / pow;
+};
+exports.toFixed = toFixed;

plugins/_transforms.js

@@ -1,175 +1,182 @@
 'use strict';
 
-const regTransformTypes = /matrix|translate|scale|rotate|skewX|skewY/;
-const regTransformSplit =
-  /\s*(matrix|translate|scale|rotate|skewX|skewY)\s*\(\s*(.+?)\s*\)[\s,]*/;
-const regNumericValues = /[-+]?(?:\d*\.\d+|\d+\.?)(?:[eE][-+]?\d+)?/g;
+const { toFixed } = require('../lib/svgo/tools');
 
 /**
  * @typedef {{ name: string, data: number[] }} TransformItem
+ * @typedef {{
+ *   convertToShorts: boolean,
+ *   floatPrecision: number,
+ *   transformPrecision: number,
+ *   matrixToTransform: boolean,
+ *   shortTranslate: boolean,
+ *   shortScale: boolean,
+ *   shortRotate: boolean,
+ *   removeUseless: boolean,
+ *   collapseIntoOne: boolean,
+ *   leadingZero: boolean,
+ *   negativeExtraSpace: boolean,
+ * }} TransformParams
  */
 
+const transformTypes = new Set([
+  'matrix',
+  'rotate',
+  'scale',
+  'skewX',
+  'skewY',
+  'translate',
+]);
+
+const regTransformSplit =
+  /\s*(matrix|translate|scale|rotate|skewX|skewY)\s*\(\s*(.+?)\s*\)[\s,]*/;
+const regNumericValues = /[-+]?(?:\d*\.\d+|\d+\.?)(?:[eE][-+]?\d+)?/g;
+
 /**
  * Convert transform string to JS representation.
  *
- * @type {(transformString: string) => TransformItem[]}
+ * @param {string} transformString
+ * @returns {TransformItem[]} Object representation of transform, or an empty array if it was malformed.
  */
 exports.transform2js = (transformString) => {
-  // JS representation of the transform data
-  /**
-   * @type {TransformItem[]}
-   */
+  /** @type {TransformItem[]} */
   const transforms = [];
-  // current transform context
-  /**
-   * @type {?TransformItem}
-   */
-  let current = null;
+  /** @type {?TransformItem} */
+  let currentTransform = null;
+
   // split value into ['', 'translate', '10 50', '', 'scale', '2', '', 'rotate', '-45', '']
   for (const item of transformString.split(regTransformSplit)) {
-    var num;
-    if (item) {
-      // if item is a translate function
-      if (regTransformTypes.test(item)) {
-        // then collect it and change current context
-        current = { name: item, data: [] };
-        transforms.push(current);
-        // else if item is data
-      } else {
-        // then split it into [10, 50] and collect as context.data
-        while ((num = regNumericValues.exec(item))) {
-          num = Number(num);
-          if (current != null) {
-            current.data.push(num);
-          }
+    if (!item) {
+      continue;
+    }
+
+    if (transformTypes.has(item)) {
+      currentTransform = { name: item, data: [] };
+      transforms.push(currentTransform);
+    } else {
+      let num;
+      // then split it into [10, 50] and collect as context.data
+      while ((num = regNumericValues.exec(item))) {
+        num = Number(num);
+        if (currentTransform != null) {
+          currentTransform.data.push(num);
         }
       }
     }
   }
-  // return empty array if broken transform (no data)
-  return current == null || current.data.length == 0 ? [] : transforms;
+
+  return currentTransform == null || currentTransform.data.length == 0
+    ? []
+    : transforms;
 };
 
 /**
  * Multiply transforms into one.
  *
- * @type {(transforms: TransformItem[]) => TransformItem}
+ * @param {TransformItem[]} transforms
+ * @returns {TransformItem}
  */
 exports.transformsMultiply = (transforms) => {
-  // convert transforms objects to the matrices
   const matrixData = transforms.map((transform) => {
     if (transform.name === 'matrix') {
       return transform.data;
     }
     return transformToMatrix(transform);
   });
-  // multiply all matrices into one
+
   const matrixTransform = {
     name: 'matrix',
     data:
       matrixData.length > 0 ? matrixData.reduce(multiplyTransformMatrices) : [],
   };
+
   return matrixTransform;
 };
 
 /**
- * math utilities in radians.
+ * Math utilities in radians.
  */
 const mth = {
   /**
-   * @type {(deg: number) => number}
+   * @param {number} deg
+   * @returns {number}
    */
   rad: (deg) => {
     return (deg * Math.PI) / 180;
   },
 
   /**
-   * @type {(rad: number) => number}
+   * @param {number} rad
+   * @returns {number}
    */
   deg: (rad) => {
     return (rad * 180) / Math.PI;
   },
 
   /**
-   * @type {(deg: number) => number}
+   * @param {number} deg
+   * @returns {number}
    */
   cos: (deg) => {
     return Math.cos(mth.rad(deg));
   },
 
   /**
-   * @type {(val: number, floatPrecision: number) => number}
+   * @param {number} val
+   * @param {number} floatPrecision
+   * @returns {number}
    */
   acos: (val, floatPrecision) => {
-    return Number(mth.deg(Math.acos(val)).toFixed(floatPrecision));
+    return toFixed(mth.deg(Math.acos(val)), floatPrecision);
   },
 
   /**
-   * @type {(deg: number) => number}
+   * @param {number} deg
+   * @returns {number}
    */
   sin: (deg) => {
     return Math.sin(mth.rad(deg));
   },
 
   /**
-   * @type {(val: number, floatPrecision: number) => number}
+   * @param {number} val
+   * @param {number} floatPrecision
+   * @returns {number}
    */
   asin: (val, floatPrecision) => {
-    return Number(mth.deg(Math.asin(val)).toFixed(floatPrecision));
+    return toFixed(mth.deg(Math.asin(val)), floatPrecision);
   },
 
   /**
-   * @type {(deg: number) => number}
+   * @param {number} deg
+   * @returns {number}
    */
   tan: (deg) => {
     return Math.tan(mth.rad(deg));
   },
 
   /**
-   * @type {(val: number, floatPrecision: number) => number}
+   * @param {number} val
+   * @param {number} floatPrecision
+   * @returns {number}
    */
   atan: (val, floatPrecision) => {
-    return Number(mth.deg(Math.atan(val)).toFixed(floatPrecision));
+    return toFixed(mth.deg(Math.atan(val)), floatPrecision);
   },
 };
 
 /**
- * @typedef {{
- *   convertToShorts: boolean,
- *   floatPrecision: number,
- *   transformPrecision: number,
- *   matrixToTransform: boolean,
- *   shortTranslate: boolean,
- *   shortScale: boolean,
- *   shortRotate: boolean,
- *   removeUseless: boolean,
- *   collapseIntoOne: boolean,
- *   leadingZero: boolean,
- *   negativeExtraSpace: boolean,
- * }} TransformParams
- */
-
-/**
- * Decompose matrix into simple transforms. See
- * https://frederic-wang.fr/decomposition-of-2d-transform-matrices.html
+ * Decompose matrix into simple transforms.
  *
- * @type {(transform: TransformItem, params: TransformParams) => TransformItem[]}
+ * @param {TransformItem} transform
+ * @param {TransformParams} params
+ * @returns {TransformItem[]}
+ * @see https://frederic-wang.fr/decomposition-of-2d-transform-matrices.html
  */
 exports.matrixToTransform = (transform, params) => {
-  let floatPrecision = params.floatPrecision;
-  let data = transform.data;
-  let transforms = [];
-  let sx = Number(
-    Math.hypot(data[0], data[1]).toFixed(params.transformPrecision),
-  );
-  let sy = Number(
-    ((data[0] * data[3] - data[1] * data[2]) / sx).toFixed(
-      params.transformPrecision,
-    ),
-  );
-  let colsSum = data[0] * data[2] + data[1] * data[3];
-  let rowsSum = data[0] * data[1] + data[2] * data[3];
-  let scaleBefore = rowsSum != 0 || sx == sy;
+  const floatPrecision = params.floatPrecision;
+  const data = transform.data;
+  const transforms = [];
 
   // [..., ..., ..., ..., tx, ty] → translate(tx, ty)
   if (data[4] || data[5]) {
@@ -179,6 +186,15 @@ exports.matrixToTransform = (transform, params) => {
     });
   }
 
+  let sx = toFixed(Math.hypot(data[0], data[1]), params.transformPrecision);
+  let sy = toFixed(
+    (data[0] * data[3] - data[1] * data[2]) / sx,
+    params.transformPrecision,
+  );
+  const colsSum = data[0] * data[2] + data[1] * data[3];
+  const rowsSum = data[0] * data[1] + data[2] * data[3];
+  const scaleBefore = rowsSum !== 0 || sx === sy;
+
   // [sx, 0, tan(a)·sy, sy, 0, 0] → skewX(a)·scale(sx, sy)
   if (!data[1] && data[2]) {
     transforms.push({
@@ -197,19 +213,34 @@ exports.matrixToTransform = (transform, params) => {
 
     // [sx·cos(a), sx·sin(a), sy·-sin(a), sy·cos(a), x, y] → rotate(a[, cx, cy])·(scale or skewX) or
     // [sx·cos(a), sy·sin(a), sx·-sin(a), sy·cos(a), x, y] → scale(sx, sy)·rotate(a[, cx, cy]) (if !scaleBefore)
-  } else if (!colsSum || (sx == 1 && sy == 1) || !scaleBefore) {
+  } else if (!colsSum || (sx === 1 && sy === 1) || !scaleBefore) {
     if (!scaleBefore) {
-      sx = (data[0] < 0 ? -1 : 1) * Math.hypot(data[0], data[2]);
-      sy = (data[3] < 0 ? -1 : 1) * Math.hypot(data[1], data[3]);
+      sx = Math.hypot(data[0], data[2]);
+      sy = Math.hypot(data[1], data[3]);
+
+      if (toFixed(data[0], params.transformPrecision) < 0) {
+        sx = -sx;
+      }
+
+      if (
+        data[3] < 0 ||
+        (Math.sign(data[1]) === Math.sign(data[2]) &&
+          toFixed(data[3], params.transformPrecision) === 0)
+      ) {
+        sy = -sy;
+      }
+
       transforms.push({ name: 'scale', data: [sx, sy] });
     }
-    var angle = Math.min(Math.max(-1, data[0] / sx), 1),
-      rotate = [
-        mth.acos(angle, floatPrecision) *
-          ((scaleBefore ? 1 : sy) * data[1] < 0 ? -1 : 1),
-      ];
+    const angle = Math.min(Math.max(-1, data[0] / sx), 1);
+    const rotate = [
+      mth.acos(angle, floatPrecision) *
+        ((scaleBefore ? 1 : sy) * data[1] < 0 ? -1 : 1),
+    ];
 
-    if (rotate[0]) transforms.push({ name: 'rotate', data: rotate });
+    if (rotate[0]) {
+      transforms.push({ name: 'rotate', data: rotate });
+    }
 
     if (rowsSum && colsSum)
       transforms.push({
@@ -220,27 +251,28 @@ exports.matrixToTransform = (transform, params) => {
     // rotate(a, cx, cy) can consume translate() within optional arguments cx, cy (rotation point)
     if (rotate[0] && (data[4] || data[5])) {
       transforms.shift();
-      var cos = data[0] / sx,
-        sin = data[1] / (scaleBefore ? sx : sy),
-        x = data[4] * (scaleBefore ? 1 : sy),
-        y = data[5] * (scaleBefore ? 1 : sx),
-        denom =
-          (Math.pow(1 - cos, 2) + Math.pow(sin, 2)) *
-          (scaleBefore ? 1 : sx * sy);
-      rotate.push(((1 - cos) * x - sin * y) / denom);
-      rotate.push(((1 - cos) * y + sin * x) / denom);
+      const oneOverCos = 1 - data[0] / sx;
+      const sin = data[1] / (scaleBefore ? sx : sy);
+      const x = data[4] * (scaleBefore ? 1 : sy);
+      const y = data[5] * (scaleBefore ? 1 : sx);
+      const denom = (oneOverCos ** 2 + sin ** 2) * (scaleBefore ? 1 : sx * sy);
+      rotate.push(
+        (oneOverCos * x - sin * y) / denom,
+        (oneOverCos * y + sin * x) / denom,
+      );
     }
 
     // Too many transformations, return original matrix if it isn't just a scale/translate
   } else if (data[1] || data[2]) {
     return [transform];
   }
 
-  if ((scaleBefore && (sx != 1 || sy != 1)) || !transforms.length)
+  if ((scaleBefore && (sx != 1 || sy != 1)) || !transforms.length) {
     transforms.push({
       name: 'scale',
       data: sx == sy ? [sx] : [sx, sy],
     });
+  }
 
   return transforms;
 };