fix(gerber-plotter): Board outline regression#64
Conversation
kasbah
changed the title
kasbah
force-pushed
the
fix-board-outline-regression
branch
from
39e4609 to
f14a447
Compare
mcous
left a comment
mcous
left a comment
There was a problem hiding this comment.
Great catch! Could you do me a favor and split this up into two PRs for the sake of the changelog?
- Add new boards for fixtures (with a few changes)
- See comment about
skipSnapshot - See comment about filename extensions
- See comment about
- Fix board outline regression
| }) | ||
|
|
||
| if (existing != null) { | ||
| return |
| @@ -0,0 +1,48 @@ | |||
| { | |||
| "skipSnapshot": true, | |||
There was a problem hiding this comment.
Feel free to remove this line from any of the boards! It's in 8-bit mixtape because the complicated silkscreens makes it take forever to snapshot. I remember bus-pirate may be a little long, too, but feel free to use your best judgement
| "skipSnapshot": true, | ||
| "layers": [ | ||
| { | ||
| "name": "PIC18F14K50.GBL", |
There was a problem hiding this comment.
Can these files be renamed to have lower-case extensions for consistency? Just wanna avoid any weird case-sensitive file-system issues in any refactors down the road
There was a problem hiding this comment.
But shouldn't we have some test cases with uppercase extensions to be sure not to break that behaviour?
There was a problem hiding this comment.
Yup! And we do in the normal whats-that-gerber unit tests (packages/fixtures/gerber-filenames.json). Better to test that functionality there than in integration tests
| @@ -0,0 +1,29 @@ | |||
| #!/usr/bin/env node | |||
| //utility for creating manifest.json files for the test boards | |||
There was a problem hiding this comment.
Can we leave this out for now? Requires a little extra discussion. The layer type and file type in the manifest is functioning as inputs to tests of whats-that-gerber and gerber-parser, so I'm a little uncomfortable with a manifest generation script that uses whats-that-gerber to generate the expectation
There was a problem hiding this comment.
Sure, I'll remove that commit for now. The idea is to check over and modify the manifest after it's created and just use the generated one as a base. Maybe I could rename it to init-manifest? It's a bit like making a snapshot as well and it should be fairly easy to spot any errors introduced.
There was a problem hiding this comment.
Wanna put it in its own PR and we can figure it out there?
kasbah
force-pushed
the
fix-board-outline-regression
branch
from
f14a447 to
a104e9a
Compare
kasbah
force-pushed
the
fix-board-outline-regression
branch
from
a104e9a to
553215f
Compare
2 participants
I am looking at the regression observed in the outlines drawn from the bus-pirate
and maulwurfexample boards. This is most likely something to do with trying to use the closest point for outline gap filling which was added to the gerber-plotter recently and seemed to be working well for the usbvil board.