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

Panel optimize #169

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Panel optimize #169

wants to merge 2 commits into from

Conversation

set-soft
Copy link
Contributor

When processing a panel with tabs and similar stuff the PCB contour is complex.
PcbDraw time needed to process these cases increases exponentially.
This patch solves a couple of issues related to extracting the PCB contour:

  1. The algorithm used to find the closest element to a point.
  2. Compute the contour just once (not twice)

- Try to find a perfect match
- If not found look for the smaller distance
- Avoid using power operation for the pseudo-distance
This is a very slow process for a panel with tabs and similar stuff
Comment on lines +139 to +140
# def distance(a: Point, b: Point) -> Numeric:
# return math.sqrt((a[0] - b[0])**2 + (a[1] - b[1])**2)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the point of commenting out this function? At the moment, it is not used. However, it makes sense to have it ready to complement pseudo_distance. And if we want to get rid of it, we should delete it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I commented it because it isn't used, you can uncomment it when needed. Or just as a reference of what a real distance is.

I usually uncomment unused functions to avoid confusion (people might think this is used), misleading coverage results (code not covered just because is dead), avoid wasting resources, etc.

@yaqwsx
Copy link
Owner

yaqwsx commented Apr 24, 2024

Hi! Thanks for the optimization. I have a suggestion not to use commented out code.

Nevertheless, the true cost is in the following routine:

PcbDraw/pcbdraw/plot.py

Lines 429 to 469 in d2af9d4

while len(elements) > 0:
# Initiate seed for the outline
outline = [elements[0]]
elements = elements[1:]
size = 0
# Append new segments to the ends of outline until there is none to append.
while size != len(outline) and len(elements) > 0:
size = len(outline)
i = get_closest(outline[0].start, [x.end for x in elements])
if SvgPathItem.is_same(outline[0].start, elements[i].end):
outline.insert(0, elements[i])
del elements[i]
continue
i = get_closest(outline[0].start, [x.start for x in elements])
if SvgPathItem.is_same(outline[0].start, elements[i].start):
e = elements[i]
e.flip()
outline.insert(0, e)
del elements[i]
continue
i = get_closest(outline[-1].end, [x.start for x in elements])
if SvgPathItem.is_same(outline[-1].end, elements[i].start):
outline.insert(0, elements[i])
del elements[i]
continue
i = get_closest(outline[-1].end, [x.end for x in elements])
if SvgPathItem.is_same(outline[-1].end, elements[i].end):
e = elements[i]
e.flip()
outline.insert(0, e)
del elements[i]
continue
# ...then, append it to path.
first = True
for x in outline:
path += x.format(first)
first = False

It is cubic to the number of input segments. It could be done in n log(n). However, that would require the use of some space segmentation trees. I wanted to preserve the simplicity of the code, and I was counting on the outline being quite simple. This is, however, not the case with panels without reconstructed arcs.

@set-soft
Copy link
Contributor Author

Nevertheless, the true cost is in the following routine:

Yes, this is exactly why this patch optimizes get_closest, the bottle neck of the code you mention.

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.

2 participants