Skip to content
This repository has been archived by the owner on Oct 2, 2020. It is now read-only.

Check for duplicate lines #286

Merged
merged 1 commit into from
May 5, 2019
Merged

Conversation

cpresser
Copy link
Contributor

Related #278

@cpresser
Copy link
Contributor Author

This checks for duplicate/overlapping lines in silkscreen and fab layers.
Courtyard layers are checked by #284 and not included in this patch.

@poeschlr
Copy link
Collaborator

Merging this would require documenting this also on the website. Would you be up to extending rule 5.1 with this? We can not include any check against undocumented rules ;)

@poeschlr poeschlr self-assigned this Jan 31, 2019
@cpresser
Copy link
Contributor Author

Imho it would require changes in 5.1 and 5.2
And yes, I can provide a patch for KLC.
But I can't seem to find the respository that has the KLC souce.

@evanshultz
Copy link
Collaborator

@cpresser
Copy link
Contributor Author

I just figured that this patch could be considered as incomplete.
It only works for lines, but not arcs and circles.
Circles are simple; arcs could be done, there is similar logic in #284 that I could reuse.

What do you think?

@evanshultz
Copy link
Collaborator

Should this also check courtyard line overlaps? That is the other layer that typically includes lines.

Definitely circles should be included if they're relatively simple. Arcs would be great too but I don't see a reason not to merge lines and circles if they're quick and then do arcs as a second step if you prefer.

@poeschlr
Copy link
Collaborator

poeschlr commented Feb 1, 2019

If we extend this to courtyard (and other layers) then we might want to write a new separate rule that clarifies this instead of adding this to every rule.

This would then mean that this check should also be in a separate file.

@cpresser
Copy link
Contributor Author

cpresser commented Feb 1, 2019

I made another patch to check the courtyard (#284). That's why I did not include the courtyard check in this patch.
I was about to postulate that this is already enough. But there are cases where this is not true. A rectangular courtyard which is fully duplicated (8 lines total) will be considered as closed. Right now I am lacking ideas on how to change #284 to detect that as well.

That means this #286 should also check the courtyard layers. Adding a check for circles is trivial; a update for this PR will be ready soon.
Arcs seem to be more complicated; as proposed, I will look into that in a second step.

Making a F5.4 rule that checks all layers seems like a good idea.
I started a pad to coordinate the text: https://pads.ccc.ac/p/KLC_F5.4

Introduce new rule F5.4
It checks for duplicate/overlapping lines and circles on all graphic layers.
Currently arcs are not supported yet.

Related KiCad#278
@cpresser
Copy link
Contributor Author

A changed patch has been (force) pushed.
It now checks for duplicate/overlapping lines and for duplicate circles.
I have some Ideas for Arcs as well, but would like to do that in a separate PR.

KLC (website) has a PR as well: KiCad/kicad-website#384

@poeschlr poeschlr added the Ready for review Use this to mark pull requests that are updated but you could not review instantly label Feb 21, 2019
@poeschlr
Copy link
Collaborator

poeschlr commented May 5, 2019

I think this is ok now. lets test it on the real world.

@poeschlr poeschlr merged commit d3568c6 into KiCad:master May 5, 2019
@poeschlr poeschlr removed the Ready for review Use this to mark pull requests that are updated but you could not review instantly label May 5, 2019
@poeschlr
Copy link
Collaborator

@cpresser it seems your script has false positives if a two lines meet at a point and are in the same angle (so if a long line is split into two smaller lines.)

@cpresser
Copy link
Contributor Author

@poeschlr Good catch.
This is a case were the check failed.
A patch to fix this is pretty simple. A PR is here: #301

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

Successfully merging this pull request may close these issues.

3 participants