PadLex / SvgToGcode

MIT License
46 stars 27 forks source link

Incorrect parsing of floating point values #18

Open amcewen opened 3 years ago

amcewen commented 3 years ago

While running the tests for #12 (Yay test code!) the new viewBox support was producing slightly different gcode for the hiking.svg test file.

Digging into it a bit I found that it was because the SVG height and width attributes were being truncated - I was getting a height of 791.716 for example, rather than the full 791.71631 in the SVG file. The viewBox code ended up scaling things a tiny bit, which threw the test code off.

The problem seems to be with the lines like this: canvas_height = float(height_str) if height_str.isnumeric() else float(height_str[:-2])

Seems that isnumeric() returns False for strings containing decimals, and the code assumes they've got a mm or px, etc. at the end and chops off the last two characters - in the case of hiking.svg that's the last two decimal places of the number.

amcewen commented 3 years ago

Checking the SVG spec, the width and height elements could be specified in values other than mm. From https://www.w3.org/TR/CSS21/syndata.html#value-def-length:

Absolute length units are fixed in relation to each other. They are mainly useful when the output environment is known. The absolute units consist of the physical units (in, cm, mm, pt, pc) and the px unit:

  • in: inches — 1in is equal to 2.54cm.
  • cm: centimeters
  • mm: millimeters
  • pt: points — the points used by CSS are equal to 1/72nd of 1in.
  • pc: picas — 1pc is equal to 12pt.
  • px: pixel units — 1px is equal to 0.75pt.

So ideally we'd parse them to handle any of these and convert them all to mm

amcewen commented 3 years ago

I've added a new function to _parser_methods.py which can parse any of the valid lengths (and doesn't truncate the decimal part), and included some simple test cases for each of them. I doubt anyone will be specifying lengths in points or picas, but now they can :grinning:

That hasn't stopped the hiking.svg test case from failing. However, I've randomly sampled a couple of dozen points, and they all varied from the previous test case by less than 0.001mm, all in the Y dimension, so I'm assuming it's down to minor variations in the floating point arithmetic with the additional transformation.

The update to the hiking.svg test case is in a separate commit, in case you want to leave it out for further investigation.

Github seems to have added the commits for this to the existing PR automatically, so accepting that will fix this issue too.