gumyr / build123d

A python CAD programming library
Apache License 2.0
539 stars 90 forks source link

SVG import - generate a set of BuildLine instances, one for each path. #56

Closed gumyr closed 1 year ago

gumyr commented 1 year ago

To preserve more of the data, I think it is worth considering generating a list of list of edges, where each top-level list represents a separate path in the SVG file.

snoyer commented 1 year ago

I'd be interested in contributing to the SVG import functionality. First, a few observations:

  1. the current dependency on svgpathtools would be enough to create wires form path strings (M ... L ... Z ... M ....)
  2. however to parse whole svg documents reliably we would need to use something like svgelements in order to also handle inherited transforms and css properties.

@gumyr, What's your take on the way forward?

gumyr commented 1 year ago

There are quite a few aspects of how SVGs are handled that should be improved. I'll list them here and we can discuss further:

  1. When writing SVG files, all curves are converted to straight line segments. I've had laser cut parts made from the results of this algorithm but it seems very problematic in that any post processing that's looking for holes as circles (ellipses I guess) would not work. The DXF exporter translates curves into appropriate curves for ezdxf - the same should be done for SVG. One extra complication is that as far as I know all splines need to be converted to cubic or quadratic bezier curves - there are tools in OpenCascade that will help with the conversion but there would likely be effort required convert bezier with up to 26 knots (the OpenCascade limit IIRC) to something compatible with SVG.
  2. All SVG exports are projections while DXF exports are just edges or wires. It seems as though the projection functionality should be extracted such that it's independent and able to be used for both types of exports. The current export APIs would have to change but I think this could be made to work without too much churn from the current API.
  3. SVG (and DXF IIRC) really only work with plain edges, wires & paths not faces other than extracting wires. Being able to export/import edges/faces with fill and stroke(color & patterns) would be a big step forward. build123d already has a color attribute for all objects, adding stroke for edges and wires might be useful.
  4. As you mention, there very well could be fundamental parts of SVGs that are not being handled (other than what I've already described). I'm certainly not a SVG expert so your expertise is appreciated here.
  5. Being able to do perspective projections seems desirable - this is a new feature for CadQuery and is already supported by OpenCascade so shouldn't be much work to implement.

If svgelements should replace svgpathtools then maybe it makes sense to start there but I don't know enough about either to really appreciate the difference.

Give all of the above - next steps?

snoyer commented 1 year ago

I have not looked into any of the exporting to be honest. I guess my first question would be what is the scope of the SVG import and export? What are the use cases?

For example, my personal use-case for import is to be able to use shapes defined as svg paths (maybe they're drawn in Inkscape, or maybe they come from some program that can only export svg, maybe I just prefer generating d strings rather than imperatively calling move_to(), line_to(), ...) and then do CAD stuff from there. For that purpose having a function that creates Wires for open paths and Faces for closed/filled paths (using svgpathtools) is enough. Importing an svg document by running every path/shape in it through that same function isn't much of a stretch (adding svgpathelements in the mix) but the semantics may get a bit blurry: is there a use-case for sorting through a big pile of Face/Wire generated from all the paths/shape in a whole svg file? Do we need to tag the the faces/wires with ids from the SVG? Do we need to consider what we would export so it can be re-imported? I'm not sure

For the export I can see it used for graphical oriented export (basically vector-based render/screenshot/blueprint) and for slice export for 2d CNC (laser-cutting, v-carve, ...), but I'm not sure how perspective projections and the like fit in. Again I've not looked into it.

One thing to keep in mind is that SVG is a complex format, it sits in between data/geometric and visual/graphical, and has lots of feature irrelevant to CAD so creating a general/complete importer/exporter in a vacuum (starting from the format spec) is unwise if not straight impossible. We need to start from the use-cases (and I won't claim I know any apart from mine so far).

gumyr commented 1 year ago

I guess my first question would be what is the scope of the SVG import and export? What are the use cases?

My use case for import is exactly as you describe - draw something relatively simple in Inkscape (etc.) import as wires to build a 2D sketch - with an addition, import_svg_as_buildline_code provides source code from the SVG source which I then can edit. This is how I created the playing card suits.

Do we need to tag the the faces/wires with ids from the SVG?

It would be cool to assign labels to Edges/Wires from any id's in the SVG but I don't think that's high on the list of things that need to be done.

Do we need to consider what we would export so it can be re-imported?

I implemented some testing this way which was really handy. Not sure why the average user would do this though.

I'm not sure how perspective projections and the like fit in

@Jern and I have use SVG extensively in the documentation. I've recently seen an amazing clock mechanism as a isometric/projected SVG. It seems as though visualization and planar export for laser cutting etc. are the two primary uses. Perspective projections are just a nice option.

SVG is a complex format

It hadn't even crossed my mind to try to import the build123d logo - I was thinking much less ambitiously, just basic 2D shapes that would become part of a sketch. I'm a big advocate of incremental development.

snoyer commented 1 year ago

My use case for import is exactly as you describe - draw something relatively simple in Inkscape (etc.) import as wires to build a 2D sketch

Alright I'll clean-up my code an push it in a branch so you can see how it would fit. As a quick note wrt Wires, it's possible (and possibly easier) to sort faces out (that is figuring out outer and inner curves) at the svg level rather than emitting a bunch of wires for the user to reconstruct. Should be clearer when I show you the code...

with an addition, import_svg_as_buildline_code provides source code from the SVG source which I then can edit. This is how I created the playing card suits.

Not familiar with the BuildLine stuff but one possibility would be to have SVG paths as a drawable type/object (maybe as a string M 0,0 L 10,12 ..., or tokenized list [('M, (0,0)), ('L', (10,12)), ...]) that would be a more compact syntax that lines of "move_to() line_to() ...`

Generating code and using exec is a big no-no though, that needs to go.

It hadn't even crossed my mind to try to import the build123d logo

That was just for me to see if the code would handle a file from "in the wild" (that would have a bit more than my lame 3 curves I tested on), and then throw some random extrusions just because

So, to recap, next step is I come back with some code that can import faces and wires from svg strings and files. Should be a good start.

gumyr commented 1 year ago

The BuildLine support is important to me (check out this for a description of BuildLine). Being able to edit the SVG is important to me as it allows the user to start parameterizing and tweaking that which was drawn with the GUI tool and to ultimately drop the SVG file which otherwise can be maintenance headache.

Here is an example of code that started as an SVG file:

class Spade(BaseSketchObject):
    def __init__(
        self,
        height: float,
        rotation: float = 0,
        align: tuple[Align, Align] = (Align.CENTER, Align.CENTER),
        mode: Mode = Mode.ADD,
    ):
        with BuildSketch() as spade:
            with BuildLine():
                b0 = Bezier((0, 198), (6, 190), (41, 127), (112, 61))
                b1 = Bezier(b0 @ 1, (242, -72), (114, -168), (11, -105))
                b2 = Bezier(b1 @ 1, (31, -174), (42, -179), (53, -198))
                l0 = Line(b2 @ 1, (0, -198))
                Mirror(about=Plane.YZ)
            MakeFace()
            Scale(by=height / spade.sketch.bounding_box().size.Y)
        super().__init__(obj=spade.sketch, rotation=rotation, align=align, mode=mode)

Creating Bezier curves with code is going to be difficult but here I've extracted the hard part from the SVG and then made a new BuildSketch object out of it (this Spade object is a full peer to Circle or Rectangle).

Use of exec is generally a no-no but this isn't just some random code being executed, it's entirely under the control of the importer. As the SVG file is pulled apart down to point values there is no way in inject malicious code into this system.

snoyer commented 1 year ago

I see 3 overlapping but distinct cases here:

  1. going from svg (either whole document from disk or isolated M0,0 L ... strings) straight to Wires and Faces.
  2. going from svg to BuildLine calls, I haven't looked into that but there should be better ways than concatenating a whole code block as text and execing it. For example generating call arguments that could be streamed from a BulidLine context and called on the fly (no exec, no buffering of the whole sequence)
  3. Generating build123d code to be reviewed/edited and used in place of regular code in a .py file. If I understand correctly that sounds like it could be a tool separate from the core build123d API

Respective plans for these 3 tracks:

  1. I'm on it
  2. pretty sure it can be achieved reusing most of the machinery from 1. Unless the implicit context magic gets in the way somehow, but it shouldn't. I'll look into that next
  3. You already made it. Could be repackaged as a command line tool depending on the details of your workflow?
snoyer commented 1 year ago

The BuildLine support is important to me (check out this for a description of BuildLine).

I've had a look and I'll allow myself on a small tangent: incrementally building paths is easier using commands than using segments. The pattern of having to recall the end of the previous segment to build the next one:

a = Line(xy0, xy1)
b = Line(a @ 1, xy2)
c = Bezier(b @ 1, xy3, xy4, xy5)

can be avoided by using move_to(start) and segment_to(..., end) commands on a builder that keeps track of the current point. For example, the Cairo syntax (cairocffi for python) for the same path would be:

ctx.move_to(x0,y0)
ctx.line_to(x1,y1)
ctx.line_to(x2,y2)
ctx.curve_to(x3,y3, x4,y4, x5,y5)
snoyer commented 1 year ago

Self contained code can be found here: https://github.com/snoyer/build123d-experiments If you could have a look, possibly try it, and give some feedback before I try to fit it in the lib and make a PR that'd be great.

gumyr commented 1 year ago

Thanks for doing this, you're taking svg import far beyond where it was. Importing the logo back as a 2D version of the 3D object broke my brain a little bit :-). Also, I learned something new today *, forces all following parameters to be keyword only - cool. Here's my feedback:

I like how you've already got tests! Usually I get to do all this fun work.

I'm certainly ready for a PR if you are. Thanks again.

snoyer commented 1 year ago

Also, I learned something new today *, forces all following parameters to be keyword only - cool

I learned about it recently too, it's a matter of taste when to use it but for non-obvious optional parameters I think forcing the increased the verbosity is worth it, both for code readability but also make sure we don't break user code if the keyword arguments are ever reordered in the API.

Currently b3d supports Python 3.9 and 3.10. The use of | instead of Union in typing would remove 3.9 support.

Good point, my bad, got used to the prettier new syntax on my own stuff where I don't have to worry too much about compatibility

There already are TOLERANCE = 1e-6 and TOL = 1e-2 constants, I need to clear this up some time.

I think I should be able to do away with the tolerances altogether. Right now they are needed for Wire.combine but as stated in a comment we know that subpath is already continuous and ordered at that point so combine is overkill.

Personally I avoid try blocks so the following seems a little foreign but it's your call:

May feel a bit weird coming from other languages but it's got advantages (see other example) and is actually the prescribed way: https://docs.python.org/3/glossary.html?highlight=eafp#term-EAFP

There must be alternatives to id for labels, starting to appreciate the complexity of it all.

Can already provide the name of the attribute to grab for the label with the label_by param. The thing is for advanced enough usages it's better to let the user preprocess the document their own way. There's only so much you can anticipate and take responsibility for.

svg_path_to_edges could easily be modified to create BuildLine code. For every Edge.make_... there is a corresponding BuildLine object. Maybe in addition to generating the actual Edges, it could be adding to a code string? The original goal of this issue was to generate a BuildLine object per path add a bit more overhead although it would help with Face generation as faces_from_svg_path could insert a MakeFace() in there.

Indeed, but I think generating BuildLine code should really be a separate functionality. No need to couple-in higher level stuff in here.

Alternatively, wouldn't it be even better have a function to generate this code directly from Edge/Wire/Face objects, regardless of whether they came from SVG or anywhere else?

There are a few things pylint identified that could be addressed.

I'll look into that

I'm certainly ready for a PR if you are. Thanks again.

You're welcome :) Let me try and ditch the Wire.combine call and then it's a go.

jrmobley commented 1 year ago

I'm willing to work on the export side. I am specifically interested in exporting edges or wires to SVG the same way they are currently exported to DXF, without conversion to line segments.

My use case is to import board outlines and component location marks into KiCad (PCB design). I've had some failures with DXF that are difficult to diagnose, so I want to try SVG. It's easier to read as a human, and there are more tools available that work with SVG. There is also the possibly that the KiCad SVG importer is more robust (or at least has different bugs, so a choice of vector formats increases the chance of successful data transfer).

gumyr commented 1 year ago

@jrmobley thanks for the offer.

If you look into SVG.get_svg you'll see "Setup the projector", "Create the visible edges", "Create the hidden edges", and "convert to native shape objects" sections. The first three are related to projection and could be used SVG, DXF or possibly other types of export and therefore should be made independent of SVG.

All of the SVG paths are created as just short line segments; however, DXF works differently,DXF._dxf_line, DXF._dxf_circle, DXF._dxf_ellipse, and DXF._dxf_spline are mapped to the b3d Edges and the appropriately generated - this is what we want for SVG. The importer is doing the reverse mapping.

I don't think SVG supports a spline so splines will have to be converted to cubic and/or quadratic bezier curves with https://dev.opencascade.org/doc/refman/html/class_geom_convert___b_spline_curve_to_bezier_curve.html (let me know if you need a hand with raw opencascade). I think the bezier curve conversion could be the most tricky as OCCT supports 26 knots while SVG support 3 or 4. I was planning on recursively sub-dividing the spline until the OCCT conversion was okay but maybe there is a better way.

svgpathtools and/or svgelements does the actual writing of the SVG file mostly it just mapping to (svgpathtools example):

Thanks for giving this a try.

gumyr commented 1 year ago

@snoyer I'd really like to get your excellent work as part of build123d - any way you could do a PR as is? If there's more work and you're not able to I'm happy to finish it off. Thanks again.

gumyr commented 1 year ago

The ocpsvg package was incorporated into svg import as of this commit: dd4f923f6cd62d25c45dc4188bf223778047e619 Not only are multiple paths supported but so are Faces, labels and colors.