arc transform fixed

[Vrroom]

Hello, I found that the formula you were using to calculate the semi major and the semi minor axis of an arc after a transformation was wrong. Earlier, you were simply treating the radii as a vector and applying the transformation matrix directly to this vector.
This is somewhat incorrect. (Because for example, the radii shouldn't change if the matrix is a rotation + translation matrix but in your case they do).
I found the correct formula at here and have implemented it in this branch. I would appreciate any feedback and anything else required from my end to merge this with the master. Thanks for this awesome project!!!

[SebKuzminsky]

I tested this arcTransformFix branch and found that it still displays the problem in #111.

[matthijskooijman]

I ran into similar problems working with svg2mod and using this library. I had one particular circular arc that had a matrix transformattion applied, which I think included 45° rotation. With the old code, that would result in an error:

  File "/home/matthijs/.local/lib/python3.8/site-packages/svgpathtools/path.py", line 1376, in __init__
    assert radius.real != 0 and radius.imag != 0
AssertionError

I couldn't easily reproduce this in a reduced example (due to floating errors, the value would just become very small but not 0), but this makes sense: If you rotate a 45° vector by 45°, one of the axis (so radius) values will become 0.

Note that even if no radius becomes 0, the resulting arc would be incorrect. It does not immediately lead to an error, though, since the Arc constructor seems the Arc is incorrect and by default tries to fix it by scaling the ellipse radius to make it fit anyway. For this particular call to Arc(), I think this is not a good idea: The transform should produce a correct arc, or bail out if it fails. I've left an inline comment with a suggestion to set autoscale_radius=False to achieve this.

With that change made, I believe all rotation of arcs will fail with the current master (ValueError: No such elliptic arc exists.).

This PR then neatly seems to fix that problem for rotations of circular arcs (i.e. rx == ry), also when combined with scaling.

However, for rotation of elliptical arcs (rx != ry), this PR does not produce correct transformations yet. The radius of the resulting ellipse seems to be correct, but I believe the rotation value of the Arc should also be updated. With the PR as-is (and my suggested change), I get the ValueError: No such elliptic arc exists. error still for a rotated ellipse. However, when I manually correct the rotation value passed to the Arc constructor (i.e. by hardcoding it in the constructor call inside transform()), the resulting arc is correct. So I suspect that all that's needed is to figure out the rotation from the matrix and apply that. I'm not sure if the current code already calculates this as a side effect maybe, but otherwise it could be a matter of taking an arbitrary vector, transform it using the matrix and then calculate the angle difference between the original and transformed vector maybe?

Note that I just tried very little cases, so this is by no means a comprehensive review. Also, I haven't dug into the math, so can't really comment on the implementation itself either.

I tested this arcTransformFix branch and found that it still displays the problem in #111.

IIUC, that problem is fixed by #112. This PR should be rebased on top of master (with that PR merged) to confirm that the combination still works, though (haven't tested that).

[mathandy]

Thanks @Vrroom @SebKuzminsky @matthijskooijman. @Vrroom, please address the concerns mentioned above.

[Vrroom]

Hello @matthijskooijman @SebKuzminsky @mathandy!

Sorry for such a late reply. Github's notification system really confuses me 😅. @matthijskooijman I commited your suggestion into this PR.

[matthijskooijman]

It's been a while since I looked at this, but re-reading my previous comment, I think you now ensured that incorrect arcs are no longer silently "corrected", but bail out instead. However, I think that elliptical arcs still do not produce correct results due to missing rotation, which was the other concern in my comment, which I think is still not addressed?

[Vrroom]

Can you share a graphic on which this issue arises? It would help me understand better what the issue is

[matthijskooijman]

Not easily, I'm no longer working with this code (and svg2mod which I worked on fixed things differently rather than switching to svgpathtools). But I think that it should be a matter of making an elliptical arc and rotating it.

[Vrroom]

Does this SVG work to illustrate your point @matthijskooijman

<?xml version="1.0" encoding="UTF-8" standalone="no"?>
<svg
   xmlns:dc="http://purl.org/dc/elements/1.1/"
   xmlns:cc="http://creativecommons.org/ns#"
   xmlns:rdf="http://www.w3.org/1999/02/22-rdf-syntax-ns#"
   xmlns:svg="http://www.w3.org/2000/svg"
   xmlns="http://www.w3.org/2000/svg"
   id="svg8"
   version="1.1"
   viewBox="0 0 210 297"
   height="297mm"
   width="210mm">
  <defs
     id="defs2" />
  <metadata
     id="metadata5">
    <rdf:RDF>
      <cc:Work
         rdf:about="">
        <dc:format>image/svg+xml</dc:format>
        <dc:type
           rdf:resource="http://purl.org/dc/dcmitype/StillImage" />
        <dc:title></dc:title>
      </cc:Work>
    </rdf:RDF>
  </metadata>
  <g
     id="layer1">
    <g
       transform="rotate(-45,111.88095,134.60932)"
       id="g4750">
      <path
         style="opacity:1;fill:#00baff;fill-opacity:1;stroke:none;stroke-width:0.26499999;stroke-linecap:round;stroke-miterlimit:4;stroke-dasharray:none;stroke-dashoffset:0;stroke-opacity:1"
         id="path4747"
         d="m 182.94048,133.3363 a 71.059525,34.395832 0 0 1 -57.74432,33.78659 71.059525,34.395832 0 0 1 -79.384695,-21.12465 71.059525,34.395832 0 0 1 27.993926,-41.70331 71.059525,34.395832 0 0 1 89.875769,5.49583 l -51.80021,23.54554 z" />
    </g>
  </g>
</svg>

[matthijskooijman]

Looks like it should. Does svgpathtools crash on this? If not, then maybe I was mistaken or something was fixed in the mean time (you did merge the master branch).

[Vrroom]

It does seem to be working. I saved the above as drawing.svg.

>>> import svgpathtools as svg
>>> svg.Document('drawing.svg').paths()
[Path(Arc(start=(161.22746445634277+83.46248339448498j), radius=(71.05952500000001+34.395832j), rotation=0.0, large_arc=False, sweep=True, end=(144.28679111150643+148.18461054366054j)),
     Arc(start=(144.28679111150643+148.18461054366054j), radius=(71.05952500000001+34.395832j), rotation=0.0, large_arc=False, sweep=True, end=(73.2159516893882+189.38068343539396j)),
     Arc(start=(73.2159516893882+189.38068343539396j), radius=(71.05952500000001+34.395832j), rotation=0.0, large_arc=False, sweep=True, end=(63.52195329709785+140.0972952298348j)),
     Arc(start=(63.52195329709785+140.0972952298348j), radius=(71.05952500000001+34.395832j), rotation=0.0, large_arc=False, sweep=True, end=(130.95985768260203+80.43166816682755j)),
     Line(start=(130.95985768260203+80.43166816682755j), end=(110.9807889254139+133.70915892541385j)),
     Line(start=(110.9807889254139+133.70915892541385j), end=(161.22746445634277+83.46248339448498j)))]
>>> 

[matthijskooijman]

Hm, are you sure you're using the right version of the code? I just tried your branch with the above file:

$ python3 -c 'import svgpathtools as svg;svg.Document("drawing.svg").paths()'
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/home/matthijs/docs/src/upstream/svgpathtools/svgpathtools/document.py", line 261, in paths
    return flattened_paths(self.tree.getroot(), group_filter,
  File "/home/matthijs/docs/src/upstream/svgpathtools/svgpathtools/document.py", line 143, in flattened_paths
    path = transform(parse_path(converter(path_elem)), path_tf)
  File "/home/matthijs/docs/src/upstream/svgpathtools/svgpathtools/path.py", line 304, in transform
    return transform_segments_together(curve, transformation)
  File "/home/matthijs/docs/src/upstream/svgpathtools/svgpathtools/path.py", line 187, in transform_segments_together
    transformed_segs = [transformation(seg) for seg in path]
  File "/home/matthijs/docs/src/upstream/svgpathtools/svgpathtools/path.py", line 187, in <listcomp>
    transformed_segs = [transformation(seg) for seg in path]
  File "/home/matthijs/docs/src/upstream/svgpathtools/svgpathtools/path.py", line 303, in <lambda>
    transformation = lambda seg: transform(seg, tf)
  File "/home/matthijs/docs/src/upstream/svgpathtools/svgpathtools/path.py", line 331, in transform
    return Arc(new_start, radius=new_radius, rotation=curve.rotation,
  File "/home/matthijs/docs/src/upstream/svgpathtools/svgpathtools/path.py", line 1431, in __init__
    self._parameterize()
  File "/home/matthijs/docs/src/upstream/svgpathtools/svgpathtools/path.py", line 1486, in _parameterize
    raise ValueError("No such elliptic arc exists.")
ValueError: No such elliptic arc exists.

[Vrroom]

I checked out the version d3961ea4d0 and re-ran the test and it gave me what I got earlier as well.

[Vrroom]

@matthijskooijman Do you want to hop on a quick call to see what is wrong?

[matthijskooijman]

Ok, I have the same commit checked out, so there must be some other difference. Interestingly, when I create a new virtualenv and run inside there, then it does work for me as well. Looking at dependencies, svgwrite is the same version inside and outside the venv, but numpy is newer. But downgrading numpy (from 1.20.3 to 1.17.4) inside the venv does not break it... Both use python 3.8.5.

With some prints I confirmed that the input of the path.transform() function is equal, but the radius it calculates is off (by factor of 2, 71-ish vs 34-ish). It looks like the numpy eigvals function returns different results. In both cases, the the D input variable is identical, but numpy produces different output. E.g. with this patch:

--- a/svgpathtools/path.py
+++ b/svgpathtools/path.py
@@ -319,11 +319,14 @@ def to_complex(v):
         D = invT.T @ Q @ invT
 
         eigvals = np.linalg.eigvals(D)
+        print(D)
+        print(eigvals)
 
         rx = 1 / np.sqrt(eigvals[0])
         ry = 1 / np.sqrt(eigvals[1])
 
         new_radius = complex(rx, ry)
+        print(new_radius)
 
         if new_radius.real == 0 or new_radius.imag == 0 :
             return Line(new_start, new_end)

I get this output:

$ python3 -c 'import svgpathtools as svg;print(svg.Document("drawing.svg").paths())'
[[0.00052165 0.00032361]
 [0.00032361 0.00052165]]
[0.00084526 0.00019804]
(34.395832+71.059525j)
(...more output and exception snipped...)

$ ./venv/bin/python3 -c 'import svgpathtools as svg;print(svg.Document("drawing.svg").paths())'
[[0.00052165 0.00032361]
 [0.00032361 0.00052165]]
[0.00019804 0.00084526]
(71.05952500000001+34.395832j)
(...more output snipped...)

Note that the D value is equal, but the resulting eigvalues is different. Also, the transform function runs a 4 times, of which the second one fails with an exception, but the values printed are the same all 4 times, so I'm showing only the first one. I also just noted that the eigenvalues, and thus also the radius values are reversed between the working and non-working cases.

But then, the plot thickens... Running just the eigenvalues calculation, shows no difference, and returns the broken (wrong?) value in both cases.

$ python3 -c 'import numpy; print(numpy.linalg.eigvals([[0.00052165,0.00032361],[0.00032361,0.00052165]]))'
[0.00084526 0.00019804]
$ venv/bin/python3 -c 'import numpy; print(numpy.linalg.eigvals([[0.00052165,0.00032361],[0.00032361,0.00052165]]))'
[0.00084526 0.00019804]

Maybe this is due to rounding (I've copied the printed values for D, which might have been rounded?).

I'm not sure what is going on here... Are you familiar enough with the math to check that 1) the value for D shown here is correct and 2) what the correct eigenvalue and radius should be? The input to the transform function that generates the above output is Arc(start=(144.28679111150643+148.18461054366054j), radius=(71.05952500000001+34.395832j), rotation=0.0, large_arc=False, sweep=True, end=(73.21595168938822+189.38068343539396j)).

@matthijskooijman Do you want to hop on a quick call to see what is wrong?

I really should be doing other things, this problem is something that was relevant to me a while back but not really anymore right now. However, since I'm the only one that can reproduce this problem, I'll try to produce enough info to at least figure this out. Let's see if you can find anything from my above comments, if you then think it is useful to do some more interactive debugging in a call, I'm open to that.

[Vrroom]

@matthijskooijman First of all I'm sorry that I didn't read your first comment properly. It pointed out a serious bug with rotation. I only realized this after re-reading my code. Regarding your concern about the D matrix and the eigenvalues, you might have noticed that in both cases, the eigenvalues are the same, just that their order is reversed. In one case you got [0.00084526 0.00019804] and in the other, [0.00019804 0.00084526]. I checked the numpy documentation and it confirms that:

The eigenvalues, each repeated according to its multiplicity. The eigenvalues are not necessarily ordered.

So how is that relevant to us? After a transformation, we are free to decide which is rx and ry among the two eigenvalues. But once we fix them, we have to pick the eigenvector corresponding to the eigenvalue chosen for rx to calculate curve rotation.

In my latest commit e5520b7e0, I maintain this consistency by picking the first eigenvalue for rx and then using the first eigenvector to calculate the rotation of the x-axis. This is then added to curve.rotation.

        rx = 1 / np.sqrt(eigvals[0])
        ry = 1 / np.sqrt(eigvals[1])

        new_radius = complex(rx, ry)

        xeigvec = eigvecs[:, 0]
        rot = np.degrees(np.arccos(xeigvec[0]))

I used the following test file to try out the new code and the path in output.svg matched that in drawing.svg.

import svgpathtools as svg

doc = svg.Document('./drawing.svg')
vbox = doc.tree.getroot().attrib['viewBox']
paths = doc.paths()
svg.wsvg(paths, stroke_widths=[1], viewbox=vbox, filename='./output.svg')

Thanks for this discussion. Please try it out now and see if any new bugs crop up!

[matthijskooijman]

Sounds like a good analysis and fix, even though I do not follow the math completely. I can confirm that the code no longer raises an exception, so that's good. I don't have any readily available files or setup to test with, though.

[Vrroom]

@mathandy @matthijskooijman Do you need me to add automated tests or something? Can we go ahead and merge this?

[matthijskooijman]

I'm just a passer-by that happened to run in a similar issue, so I'm leaving that question for @mathandy.

[mathandy]

So when I first wrote the code to handle arcs I realized there are some discrepancies between how the math would suggest arcs would work, how the SVG docs on W3 suggested they should work, and how they actually rendered in standard browsers (e.g. Chrome).

Can you give me a visual example of what this code fixes?

BTW I tend to think of Chrome as the standard to go by, but really it depends on your application and I'm open to arguments to thinking otherwise.

[Vrroom]

Consider the test function I provided earlier. Copying here for convenience:

import svgpathtools as svg

doc = svg.Document('./drawing.svg')
vbox = doc.tree.getroot().attrib['viewBox']
paths = doc.paths()
svg.wsvg(paths, stroke_widths=[1], viewbox=vbox, filename='./output.svg')

drawing.svg

<?xml version="1.0" encoding="UTF-8" standalone="no"?>
<svg
   xmlns:dc="http://purl.org/dc/elements/1.1/"
   xmlns:cc="http://creativecommons.org/ns#"
   xmlns:rdf="http://www.w3.org/1999/02/22-rdf-syntax-ns#"
   xmlns:svg="http://www.w3.org/2000/svg"
   xmlns="http://www.w3.org/2000/svg"
   id="svg8"
   version="1.1"
   viewBox="0 0 210 297"
   height="297mm"
   width="210mm">
  <g transform="rotate(-45,111.88095,134.60932)" fill="none" stroke="black">
    <path
       d="m 182.94048,133.3363 a 71.059525,34.395832 0 0 1 -57.74432,33.78659 71.059525,34.395832 0 0 1 -79.384695,-21.12465 71.059525,34.395832 0 0 1 27.993926,-41.70331 71.059525,34.395832 0 0 1 89.875769,5.49583" />
  </g>
</svg>

This graphic looks like:

The input drawing.svg and the output output.svg, produced by my test code, should have identical geometries if the transformation handling for arcs is correct. Without this fix, this is not the case. The output with the current master is:

With my contribution, it becomes:

[Vrroom]

@mathandy Are we going to go forward with this?

[mathandy]

Thanks @Vrroom !