svgdotjs / svgdom

Straightforward DOM implementation to make SVG.js run headless on Node.js
MIT License
269 stars 53 forks source link

Obtaining length of some Arc can exceed maximum call stack #91

Open mryellow opened 2 years ago

mryellow commented 2 years ago

M 216.1027854225359 271.1209756706295 A 46.283096266347606 28.725390201836586 2.166914683186652 0 1 287.49723659993464 261.4021001840497

M 164.16806031012334 277.88997477304815 A 112.78408575681235 76.52010425131027 -175.93248043341987 1 1 158.42630883977398 299.49871866124164

RangeError: Maximum call stack size exceeded

I believe the issue may be related to early return of an estimate being delayed too deeply into the recursive length call stack.

Perhaps this 0.00001 constant is the wrong size. Perhaps a count of call stack depth is required. Or perhaps simply a try/catch where the result is returned on RangeError.

https://github.com/svgdotjs/svgdom/blob/0a2a9c95bf5cb756675860916cd140d777e7f1da/src/utils/pathUtils.js#L351

mryellow commented 2 years ago

Succeeds:

Fails:

mryellow commented 2 years ago
/* global describe, it */

import { createSVGDocument } from '../main-module.js'
import assert from 'assert'

describe('arc length', () => {
  it('obtaining arc length should not result in RangeError', () => {
    const svgDoc = createSVGDocument()

    const el = svgDoc.createElement('path');
    // (rx ry x-axis-rotation large-arc-flag sweep-flag x y)+
    //el.setAttribute('d', 'M 216.1027854225359 271.1209756706295 A 46.283096266347606 28.725390201836586 2.166914683186652 0 1 287.49723659993464 261.4021001840497');
    //el.setAttribute('d', 'M 216 271 A 46 28 2 0 1 287 261');
    //el.setAttribute('d', 'A 46 28 2 0 1 287 261');
    el.setAttribute('d', 'A 10 10 0 0 0 100 251');
    svgDoc.documentElement.appendChild(el);

    const length = svgDoc.querySelectorAll('svg path')[0].getTotalLength();
    assert(length > 0);
  })
})
mryellow commented 2 years ago

Rather than bisecting over and over again svg-path-properties decides ahead of time how many segments to limit itself to.

https://github.com/rveciana/svg-path-properties/blob/40d42d50729b2560dcfda2cf36e5d78b049a4f73/src/arc.ts#L253-L254

Fuzzyma commented 2 years ago

Deciding ahead of time how many segments to use can lead to unprecise approximations for really large arcs. But yes, this is a problem and a bug. Just not sure how to solve it properly

mryellow commented 2 years ago

Could always decouple with setTimeout and return a promise... Though it seems a little out-of-place here.

Fuzzyma commented 2 years ago

That wouldnt work since this function is expected to return directly. Cant break user expectation here. I am not following dom standards to the max but this lib still remains a drop in replacement for a real dom :D

mryellow commented 2 years ago

I'd think return what you have on a RangeError though the way it's dividing and recusing down there isn't really a "what you have".

Haven't gone back and reviewed what scopes are available but could maybe keep a count of recursions somewhere and return before the error.

Fuzzyma commented 2 years ago

or maybe rewrite the recursion as a while loop (which should be possible if it is tail call optimizable).