Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add import_svg_as_forced_outline function #718

Draft
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

BlueDrink9
Copy link

Make importing unclean outlines easier. Works really well on the KiCad exports of two unrelated PCBs that I tested it on.

@gumyr
Copy link
Owner

gumyr commented Sep 29, 2024

Here are a few high level things to consider:

  • I wonder if this functionality wouldn't be better suited to the ocpsvg project (https://github.com/snoyer/ocpsvg) which provides almost all of the SVG import capabilities of build123d.
  • The contribution rules are here: https://github.com/gumyr/build123d/blob/dev/CONTRIBUTING.md which covers some of the following.
  • Unittests will need to be provided with coverage at (or very close to) 100%
  • pylint needs to be happy with the code
  • The content following line 617 shouldn't be included here (looks like the start of some tests)
  • There shouldn't be any print statements (ValueError takes a string)
  • Tests aren't running this morning so I can't check, but this code needs to run on Windows, Linux, Mac (both Intel and Arm architectures)

@BlueDrink9 BlueDrink9 marked this pull request as draft September 30, 2024 03:27
@BlueDrink9
Copy link
Author

Sure, might not get around to fulfilling all of those requests in the near future, but I'll aim to following the guidelines. Apologies for missing those the first time.

Regarding ocpsvg, currently I'm using b123d features to connect the wires cleanly, so I think that would be more work than I'm willing and able to put in

@snoyer
Copy link
Contributor

snoyer commented Oct 4, 2024

I've not studied the whole thing but it looks like what this PR accomplishes is pretty close to, or at least overlaps with, existing functionality in build123d like make_wire() that makes wire from unorganized edges.
If my understanding is correct I don't think that's in ocpsvg's scope and it would be more appropriate (and more convenient/easier) to implemented it after the SVG import at the build123d level

@BlueDrink9
Copy link
Author

I'll have to read the source for make wire. All I know is that it didn't work for the faces I imported, and this makes some notable changes to the imported that I wouldn't want made by default for all shapes

@gumyr
Copy link
Owner

gumyr commented Oct 4, 2024

I've not studied the whole thing but it looks like what this PR accomplishes is pretty close to, or at least overlaps with, existing functionality in build123d like make_wire() that makes wire from unorganized edges. If my understanding is correct I don't think that's in ocpsvg's scope and it would be more appropriate (and more convenient/easier) to implemented it after the SVG import at the build123d level

IIRC the SVG import crashes so there is no opportunity to fix the broken wires.

@gumyr
Copy link
Owner

gumyr commented Oct 4, 2024

Another way to approach this problem might be to enhance the SVG importer to enable a request for just Edges with no attempt to build Wires or Faces. With just the Edges, the Wire(random_edges, sequenced=True) could used (or possible enhanced) to build the perimeter of a Face for these poorly formed SVG files.

@snoyer
Copy link
Contributor

snoyer commented Oct 4, 2024

IIRC the SVG import crashes so there is no opportunity to fix the broken wires.

Is there any data to reproduce this crash?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants