Skip to content

fix: tools/toFixed precision #1950

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

fix: tools/toFixed precision #1950

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
840ef2d
Prevent toFixed from adding more decimals than was given
carlosjeurissen Feb 2, 2024
5b48095
Use toFixed in more plugins
carlosjeurissen Feb 2, 2024
90927e2
Simplify logic by just skipping toFixed with any precision higher tha…
carlosjeurissen Feb 3, 2024
1d389d2
Add toFixedStr as string alternative for toFixed
carlosjeurissen Feb 4, 2024
48e3d1c
Start using new toFixedStr method in cleanupListOfValues and cleanupN…
carlosjeurissen Feb 4, 2024
89ae4bb
Correct return type for toFixedStr
carlosjeurissen Feb 4, 2024
6d1a6b2
Start using toFixedStr in path.js
carlosjeurissen Feb 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 28 additions & 37 deletions lib/path.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { removeLeadingZero, toFixed } from './svgo/tools.js';
import { removeLeadingZero, toFixedStr } from './svgo/tools.js';

/**
* @typedef {import('./types.js').PathDataItem} PathDataItem
Expand Down Expand Up @@ -239,23 +239,6 @@ export const parsePathData = (string) => {
return pathData;
};

/**
* @type {(number: number, precision?: number) => {
* roundedStr: string,
* rounded: number
* }}
*/
const roundAndStringify = (number, precision) => {
if (precision != null) {
number = toFixed(number, precision);
}

return {
roundedStr: removeLeadingZero(number),
rounded: number,
};
};

/**
* Elliptical arc large-arc and sweep flags are rendered with spaces
* because many non-browser environments are not able to parse such paths
Expand All @@ -272,30 +255,38 @@ const stringifyArgs = (command, args, precision, disableSpaceAfterFlags) => {
let previous;

for (let i = 0; i < args.length; i++) {
const { roundedStr, rounded } = roundAndStringify(args[i], precision);
if (
let number = args[i];
if (precision != null) {
number = toFixedStr(number.toString(), precision);
}
const roundedStr = removeLeadingZero(number);

const skipSpace = (
// avoid space before first
i === 0
) || (
// avoid space before negative numbers
number < 0
) || (
// consider combined arcs
disableSpaceAfterFlags &&
(command === 'A' || command === 'a') &&
// consider combined arcs
(i % 7 === 4 || i % 7 === 5)
) {
result += roundedStr;
} else if (i === 0 || rounded < 0) {
// avoid space before first and negative numbers
result += roundedStr;
} else if (
!Number.isInteger(previous) &&
rounded != 0 &&
rounded < 1 &&
rounded > -1
) {
) || (
// remove space before decimal with zero whole
// only when previous number is also decimal
result += roundedStr;
} else {
result += ` ${roundedStr}`;
previous % 1 && // only when previous number is also decimal
number != 0 &&
number < 1 &&
number > -1
);

if (!skipSpace) {
result += ' ';
}
previous = rounded;

result += roundedStr;

previous = number;
}

return result;
Expand Down
23 changes: 22 additions & 1 deletion lib/svgo/tools.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,13 +226,34 @@ export const findReferences = (attribute, value) => {

/**
* Does the same as {@link Number.toFixed} but without casting
* the return value to a string.
* the return value to a string and correctly fix numbers
* like 21.0565 to 21.057 when precision is 3.
*
* @param {number} num
* @param {number} precision
* @returns {number}
*/
export const toFixed = (num, precision) => {
if (precision > 17 || num % 1 == 0) return num;

const pow = 10 ** precision;
return Math.round(num * pow) / pow;
};

/**
* Does the same as {@link Number.toFixed} and correctly
* fix numbers like 2.5845 to 2.585 when precision is 3.
*
* @param {string} numStr
* @param {number} precision
* @returns {string}
*/
export const toFixedStr = (numStr, precision) => {
const pow = 10 ** precision;
const fixed = Math.round(numStr * pow) / pow;
const result = fixed.toString();

// prevent returning more digits than originally given
const isRegression = result.length > numStr.length;
Copy link
Member

@GreLI GreLI Jun 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

toFixed(2.5845, 3) correctly gives me 2.585. Are there real examples where the check is needed? I iterated over every number few years ago and didn't found any.

Copy link
Contributor

@KTibow KTibow Jun 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

toFixed(1.99999999999, 18) -> 1.9999999999899998

Copy link
Author

@carlosjeurissen carlosjeurissen Jun 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@GreLI Here is an example of before and after which adds additional digits as the result of floating point arithmetic.

Before:

<?xml version="1.0" encoding="UTF-8"?>
<svg width="48px" height="48px" viewBox="0 0 48 48" version="1.1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
    <defs>
        <linearGradient x1="31.7538776%" y1="65.3789342%" x2="76.9899438%" y2="27.2412226%" id="linearGradient-1">
            <stop stop-color="#217BFE" offset="0%"></stop>
            <stop stop-color="#078EFB" offset="27%"></stop>
            <stop stop-color="#A190FF" offset="78%"></stop>
            <stop stop-color="#BD99FE" offset="100%"></stop>
        </linearGradient>
    </defs>
    <g stroke="none" stroke-width="1" fill="none" fill-rule="evenodd">
        <path d="M45.912505,23.9125099 C42.8951764,23.9125099 40.1078348,23.340075 37.4554855,22.2152028 C34.8006363,21.0528349 32.4607693,19.4655153 30.4983808,17.5032383 C28.5359923,15.5409613 26.9510823,13.1987274 25.7861485,10.5465288 C24.6587126,7.89433019 24.088745,5.1046472 24.088745,2.08749006 C24.088745,2.03999546 24.0512471,2 24.0012499,2 C23.9512528,2 23.9137549,2.03999546 23.9137549,2.08749006 C23.9137549,5.1046472 23.3237884,7.89183047 22.1613545,10.5465288 C21.0339185,13.2012271 19.4665076,15.5409613 17.5041191,17.5032383 C15.5417306,19.4655153 13.1993637,21.0503352 10.5470144,22.2152028 C7.89466508,23.3425747 5.10482359,23.9125099 2.08749503,23.9125099 C2.03999773,23.9125099 2,23.9525054 2,24 C2,24.0474946 2.03999773,24.0874901 2.08749503,24.0874901 C5.10482359,24.0874901 7.89216522,24.677423 10.5470144,25.8397909 C13.2018635,26.9671628 15.5417306,28.5344847 17.5041191,30.4967617 C19.4665076,32.4590387 21.0339185,34.8012726 22.1613545,37.4559709 C23.3237884,40.1081695 23.9137549,42.8953528 23.9137549,45.9125099 C23.9137549,45.9600045 23.9537526,46 24.0012499,46 C24.0487472,46 24.088745,45.9625043 24.088745,45.9125099 C24.088745,42.8953528 24.6587126,40.1081695 25.7861485,37.4559709 C26.9485825,34.8012726 28.5334924,32.4615385 30.4983808,30.4967617 C32.4607693,28.5344847 34.8006363,26.9671628 37.4554855,25.8397909 C40.1078348,24.677423 42.8951764,24.0874901 45.912505,24.0874901 C45.9600023,24.0874901 46,24.0499943 46,24 C46,23.9500057 45.9600023,23.9125099 45.912505,23.9125099 Z" fill="url(#linearGradient-1)" fill-rule="nonzero"></path>
    </g>
</svg>

after:

<?xml version="1.0" encoding="utf-8"?>
<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 48 48">
  <defs>
    <linearGradient id="a" x1="31.753877599999996%" x2="76.9899438%" y1="65.3789342%" y2="27.2412226%">
      <stop offset="0%" stop-color="#217BFE"/>
      <stop offset="27%" stop-color="#078EFB"/>
      <stop offset="78%" stop-color="#A190FF"/>
      <stop offset="100%" stop-color="#BD99FE"/>
    </linearGradient>
  </defs>
  <path fill="url(#a)" d="M45.912505 23.9125099C42.8951764 23.9125099 40.1078348 23.340075 37.4554855 22.2152028 34.8006363 21.0528349 32.4607693 19.4655153 30.4983808 17.5032383 28.5359923 15.5409613 26.9510823 13.1987274 25.7861485 10.5465288 24.6587126 7.89433019 24.088745 5.1046472 24.088745 2.0874900600000004 24.088745 2.03999546 24.0512471 2 24.0012499 2 23.9512528 2 23.9137549 2.03999546 23.9137549 2.08749006 23.9137549 5.1046472 23.3237884 7.89183047 22.1613545 10.5465288 21.0339185 13.2012271 19.4665076 15.5409613 17.5041191 17.5032383 15.5417306 19.4655153 13.1993637 21.0503352 10.5470144 22.2152028 7.89466508 23.3425747 5.10482359 23.9125099 2.0874950299999995 23.9125099 2.03999773 23.9125099 2 23.9525054 2 24S2.03999773 24.0874901 2.08749503 24.0874901C5.10482359 24.0874901 7.89216522 24.677423 10.5470144 25.8397909 13.2018635 26.9671628 15.5417306 28.5344847 17.5041191 30.4967617 19.4665076 32.4590387 21.0339185 34.8012726 22.1613545 37.4559709 23.3237884 40.1081695 23.9137549 42.8953528 23.9137549 45.9125099 23.9137549 45.9600045 23.9537526 46 24.0012499 46S24.088745 45.9625043 24.088745 45.9125099C24.088745 42.8953528 24.6587126 40.1081695 25.7861485 37.4559709 26.9485825 34.8012726 28.5334924 32.4615385 30.4983808 30.4967617 32.4607693 28.5344847 34.8006363 26.9671628 37.4554855 25.8397909 40.1078348 24.677423 42.8951764 24.0874901 45.912505 24.0874901 45.9600023 24.0874901 46 24.0499943 46 24S45.9600023 23.9125099 45.912505 23.9125099"/>
</svg>

with the following svgo config:

const prettyprint = true;
const floatPrecision = 19;

const js2svg = {
  eol: 'lf',
  finalNewline: true,
  indent: 2,
  pretty: prettyprint,
};

const config = {
  multipass: true,

  js2svg: js2svg,

  floatPrecision: floatPrecision,

  plugins: [
    {
      name: 'preset-default',
      params: {
        overrides: {
          // customize default plugin options
          inlineStyles: {
            onlyMatchedOnce: false,
          },

          convertPathData: {
            makeArcs: {
              threshold: 2.5,
              tolerance: 0.5,
            },
            transformPrecision: floatPrecision,

            forceAbsolutePath: false,
            noSpaceAfterFlags: false,
          },

          cleanupNumericValues: {
          },

          convertShapeToPath: {
            convertArcs: false,
          },

          removeUnknownsAndDefaults: {
            keepAriaAttrs: false,
            keepDataAttrs: false,
          },

          mergePaths: {
            force: true,
            noSpaceAfterFlags: true,
          },

          sortAttrs: {
            order: [
              'id',
              'fill-rule',
              'fill',
              'stroke',
              'stroke-width',
              'width',
              'height',
              'x',
              'x1',
              'x2',
              'y',
              'y1',
              'y2',
              'cx',
              'cy',
              'r',
              'marker',
              'd',
              'points',
            ],
            xmlnsOrder: 'front',
          },
          removeDoctype: false,
          removeViewBox: false,
          removeXMLProcInst: false,
        },
      },
    },

    'convertStyleToAttrs',
    'removeDimensions',
    'removeOffCanvasPaths',
  ],
};

export default config;

As you can see it happens with the x1 attribute in the linear-gradient. And there is also the 2.0874900600000004 in a d attribute further down.

Copy link
Member

@GreLI GreLI Jun 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@KTibow Ah, I see. Without if (precision > 17 || num % 1 == 0) return num; on line 237 it really have such issues.

Though I must say, that are quite extreme precisions, far from practical ones. E.g. you lose precision by multiplication numbers with more than 9 digits in the fractional part due to how floating point calculations work. I wouldn't recommend to use floatPrecision = 19 to anyone. Editors usually don't write more than 5 fractional digits.

Speaking of pixels there wouldn't be difference more than 1/256 of pixel (ok, 1/512 with high dpi display). So, given the usual display has no more than thousands of pixels, 7 digits in total is more than enough. Thus there is no sense in more than 5 fractional digits in percentages. Practically, one wouldn't see difference for something like SVGO default 3 digits. For some images it can be even 2 or 1 (without curves—I have some ideas about that).

Copy link
Member

@SethFalco SethFalco Jun 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The most egregious example I've seen in the wild is:

<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 673 205">
  <path d="M0 0L6235704.138785 2260595.537351" transform="matrix(0.0000431728,0,0,0.0000431865,60.47463,20.8221)" stroke-width="12700" stroke="black"/>
</svg>

— #1858 (comment)

Without any kind of path normalization to bring the scale of the numbers closer to 0 (which may be worth looking at in future) reducing the precision can be problematic. But even then, a float precision like 19 does seem odd.

So, given the usual display has no more than thousands of pixels, 7 digits in total is more than enough

Generally this is true, but depend on the scale that transforms operate on, that can still cause problems. (Not denying, that SVGs like the example given seem bizarre, but it does happen.)

I haven't reviewed this PR yet as I've been focused on other things in SVGO, so can't say if I'm in favor of the change or not yet. Just noting that it still may be worth having better support for large precision values if it's otherwise causing problems.

@carlosjeurissen Sorry for putting you off! I've been busy with client work, and now that I have time for SVGO again, my priority is with issues related to the v4 release candidates. (Namely imports/interface related issues.)

Once those are sorted, we can take a deeper look at this.

Copy link
Member

@GreLI GreLI Jun 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, transformations are often sick and overused. That's why it's better to apply transformations at first—that makes thinks easier.

return isRegression ? numStr : result;
};
9 changes: 3 additions & 6 deletions plugins/applyTransforms.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import {
import { referencesProps, attrsGroupsDefaults } from './_collections.js';
import { collectStylesheet, computeStyle } from '../lib/style.js';

import { removeLeadingZero, includesUrlReference } from '../lib/svgo/tools.js';
import { removeLeadingZero, includesUrlReference, toFixed } from '../lib/svgo/tools.js';

/**
* @typedef {PathDataItem[]} PathData
Expand Down Expand Up @@ -92,11 +92,8 @@ export const applyTransforms = (root, params) => {
return;
}

const scale = Number(
Math.hypot(matrix.data[0], matrix.data[1]).toFixed(
transformPrecision,
),
);
const scale = toFixed(Math.hypot(matrix.data[0], matrix.data[1]),
transformPrecision);

if (stroke && stroke != 'none') {
if (!params.applyTransformsStroked) {
Expand Down
20 changes: 8 additions & 12 deletions plugins/cleanupListOfValues.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { removeLeadingZero } from '../lib/svgo/tools.js';
import { removeLeadingZero, toFixedStr } from '../lib/svgo/tools.js';

export const name = 'cleanupListOfValues';
export const description = 'rounds list of values to the fixed precision';
Expand Down Expand Up @@ -52,8 +52,7 @@ export const fn = (_root, params) => {

// if attribute value matches regNumericValues
if (match) {
// round it to the fixed precision
let num = Number(Number(match[1]).toFixed(floatPrecision));
let strNum = match[1];
/**
* @type {any}
*/
Expand All @@ -65,22 +64,19 @@ export const fn = (_root, params) => {

// convert absolute values to pixels
if (convertToPx && units && units in absoluteLengths) {
const pxNum = Number(
(absoluteLengths[units] * Number(match[1])).toFixed(floatPrecision),
);
const pxNum = (absoluteLengths[units] * strNum).toString();

if (pxNum.toString().length < match[0].length) {
num = pxNum;
if (pxNum.length < match[0].length) {
strNum = pxNum;
units = 'px';
}
}

// round it to the fixed precision
let str = toFixedStr(strNum, floatPrecision);
// and remove leading zero
let str;
if (leadingZero) {
str = removeLeadingZero(num);
} else {
str = num.toString();
str = removeLeadingZero(str);
}

// remove default 'px' units
Expand Down
23 changes: 9 additions & 14 deletions plugins/cleanupNumericValues.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { removeLeadingZero } from '../lib/svgo/tools.js';
import { removeLeadingZero, toFixedStr } from '../lib/svgo/tools.js';

export const name = 'cleanupNumericValues';
export const description =
Expand Down Expand Up @@ -43,7 +43,7 @@ export const fn = (_root, params) => {
const num = Number(value);
return Number.isNaN(num)
? value
: Number(num.toFixed(floatPrecision));
: toFixedStr(value, floatPrecision);
})
.join(' ');
}
Expand All @@ -59,7 +59,7 @@ export const fn = (_root, params) => {
// if attribute value matches regNumericValues
if (match) {
// round it to the fixed precision
let num = Number(Number(match[1]).toFixed(floatPrecision));
let strNum = match[1];
/**
* @type {any}
*/
Expand All @@ -71,23 +71,18 @@ export const fn = (_root, params) => {

// convert absolute values to pixels
if (convertToPx && units !== '' && units in absoluteLengths) {
const pxNum = Number(
(absoluteLengths[units] * Number(match[1])).toFixed(
floatPrecision,
),
);
if (pxNum.toString().length < match[0].length) {
num = pxNum;
const pxNum = (absoluteLengths[units] * strNum).toString();
if (pxNum.length < match[0].length) {
strNum = pxNum;
units = 'px';
}
}

// round it to the fixed precision
let str = toFixedStr(strNum, floatPrecision);
// and remove leading zero
let str;
if (leadingZero) {
str = removeLeadingZero(num);
} else {
str = num.toString();
str = removeLeadingZero(str);
}

// remove default 'px' units
Expand Down