PadLex / SvgToGcode

MIT License
46 stars 27 forks source link

Add support for viewBox and rotate transforms around a non-0,0 centre point #19

Closed amcewen closed 3 years ago

amcewen commented 3 years ago

Code to fix both #12 and #13.

The tests pass apart from hiking.svg, which exposes a separate bug which has been raised as #18.

PadLex commented 3 years ago

This looks wonderful, thank you! I'm on vacation right now, but I'll review and merge your changes as soon as I'm back.

amcewen commented 3 years ago

@PadLex No worries, enjoy the rest of your holiday!

Seems Github has automatically added the two new commits I've just pushed to my fork. So it now has the fix for https://github.com/PadLex/SvgToGcode/issues/18 too.

PadLex commented 3 years ago

I'm encountering a scaling issue and a deprecation warning. You can reproduce the scaling issue by having a look at svg_to_gcode/testing/linear_approximation/viewBox.svg.

image

The red line should trace the black one exactly. Unfortunately, after running the automated tests with your changes, the traces are scaled down. If your prefer, you can also reproduce it by updating the inkscape extension's svg_to_gcode folder with your changes.

The deprecation warnings state:

:28: DeprecationWarning: invalid escape sequence \d
 m = re.search('(\d+)[\.]*(\d*)(\w*)', length)
:83: DeprecationWarning: invalid escape sequence \d
  p = re.compile("([\d\.]+),?\s+([\d\.]+),?\s+([\d\.]+),?\s+([\d\.]+)")

But don't worry, we can deal with the warnings later. I like your coding style btw, the comments are quite handy.

amcewen commented 3 years ago

Weird that I didn't get the warnings here, but as you say, they'll be pretty easy to fix.

I wasn't sure what the linear approximation files were for, so had mostly ignored that bit, sorry. Looking at it now, I think the problem is that the testing/other_tests/linear_approximation/viewBox.svg file still has the viewBox element from the original file in it, so that gets applied to the red path too.

I guess the test code will need to work out the correct reverse transforms to apply to the red path, unless we could do something like embed the two SVGs into an enclosing one? (thinking out loud with that last suggestion)

<svg>
    <svg...>
        <!-- original SVG in here, with its viewBox, etc. -->
    </svg>
    <svg...>
        <!-- resulting GCode turned back to an SVG here -->
    </svg>
</svg>

Not sure which is the better approach.

Btw, spotted that testing/ther_tests/linear_approximation/test.py has the same code that'll hit the #18 bug - I'd only grepped through the svg_to_gcode folder looking for instances.

PadLex commented 3 years ago

As far as I can tell your implementation works correctly. I'm just having trouble connecting it to the extension. I'll merge immediately and then update the pypi as soon as I get everything ironed-out. Thank you for your contributions!