snoyer / ocpsvg

Apache License 2.0
1 stars 0 forks source link

Should ocpsvg have a tolerance parameter? #20

Open jdegenstein opened 5 months ago

jdegenstein commented 5 months ago

I don't know the right answer to this problem, but wanted to bring to your attention in case it is best to fix here.

<?xml version="1.0" encoding="UTF-8" standalone="no"?>
<svg
   width="187.80397mm"
   height="190.72836mm"
   viewBox="0 0 187.80397 190.72836"
   version="1.1"
   id="svg1"
   xml:space="preserve"
   xmlns="http://www.w3.org/2000/svg"
   xmlns:svg="http://www.w3.org/2000/svg"><defs
     id="defs1" /><g
     id="layer1"
     transform="translate(-10.064523,-56.933011)"><g
       id="g1"><ns0:path xmlns:ns0="http://www.w3.org/2000/svg" id="path5" style="fill:#f7f8f8" d="M 104.11876,56.934021 C 63.736597,56.737648 25.41552,85.182615 13.677255,126.80417 c -16.0876352,57.04356 23.474457,116.30107 80.550639,120.65207 5.762765,0.4393 20.717906,0.12691 23.944796,-0.50023 29.53134,-5.73943 50.78323,-20.45287 66.04713,-45.72692 4.31274,-7.14105 7.72319,-15.32478 10.39678,-24.94783 13.17482,-47.42016 -14.87779,-98.88564 -62.53934,-114.734601 -3.61363,-1.201644 -4.26535,-1.369129 -10.59728,-2.729549 -5.78154,-1.242168 -11.59234,-1.855036 -17.36122,-1.883089 z" /></g></g></svg>

The above SVG results in the following behavior in build123d:

from build123d import *
b = import_svg('circle.svg')
len(b.vertices()) # returns 9

Looking at b.vertices() there are two nearly identical duplicate vertices:

[Vertex: (94.05428778934001, 190.72745299296872, 0.0),
...
 Vertex: (94.05428778938001, 190.7274529931684, 0.0)]

Should ocpsvg have a tolerance parameter to merge these vertices, or should this be done at the build123d level somehow?

snoyer commented 5 months ago

I'd feel more comfortable if that was handled on either side of ocpsvg (preprocessing to "fix" the SVG or post-processing to "fix" the OCC curves).

We have to use a tolerance parameter when converting from OCC to SVG because some curves types need to be approximated; but when going from SVG to OCC I think it's fair that ocpsvg gives you the curves as the SVG actually describes them.

I'm assuming in this case the first and last vertices don't line up because of the drift from the relative coordinates, in which case I'm going to claim "not a bug; won't fix"

jdegenstein commented 5 months ago

According to inkscape there are only 8 nodes, yet I think ocpsvg is returning a shape with 9 vertices.

snoyer commented 5 months ago

looks like the path does have 9 vertices though

M 104.11876,56.934021 
C 63.736597,56.737648 25.41552,85.182615 13.677255,126.80417 
c -16.0876352,57.04356 23.474457,116.30107 80.550639,120.65207
   5.762765,0.4393 20.717906,0.12691 23.944796,-0.50023
   29.53134,-5.73943 50.78323,-20.45287 66.04713,-45.72692
   4.31274,-7.14105 7.72319,-15.32478 10.39678,-24.94783
   13.17482,-47.42016 -14.87779,-98.88564 -62.53934,-114.734601
   -3.61363,-1.201644 -4.26535,-1.369129 -10.59728,-2.729549
   -5.78154,-1.242168 -11.59234,-1.855036 -17.36122,-1.883089
   z

I guess the question is how the z command is handled and whether it should merge the first and last vertices.

I do have a tolerance for that internally (https://github.com/snoyer/ocpsvg/blob/main/ocpsvg/svg.py#L353) but if I remember correctly I hardcoded just small enough to avoid issues with OCC (ie. not create excessively short closing segments that would upset OCC).

In this case it looks like the accumulated error is higher that my empirically derived 1e-10 so the z command will add a segment to connect to the start and the last vertex will not be the starting vertex

>>> a = complex(94.05428778934001, 190.72745299296872)
>>> b = complex(94.05428778938001, 190.7274529931684)
>>> abs(a-b)
2.0365842040460295e-10

I'll double check by actually running some code later. Regardless, technically I could expose that parameter all the way up the call chain to the user entry-point but I still think that could and should be done after the fact on the OCC wires instead.