-
-
Notifications
You must be signed in to change notification settings - Fork 21
feat: outline generation for given path #93
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
base: main
Are you sure you want to change the base?
feat: outline generation for given path #93
Conversation
|
draft review and some feedback would be appreciated |
| ], | ||
| } satisfies Record<string, Partial<OffsetOptions>[]>; | ||
|
|
||
| const TEST_SHAPES = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No arcs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see generateOutlinePath handles that by doing .aToC, but it's not being validated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes current implementation supports only lines and beziers, this def should/could be extended but as POC, i didn't bother
| ): boolean { | ||
| if (!p1 || !p2) return false; | ||
|
|
||
| return Math.sqrt((p2.x - p1.x) ** 2 + (p2.y - p1.y) ** 2) <= tolerance; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Often this is implemented as (...) <= tolerance ** 2 to avoid the sqrt. Just an aside.
| ...TEST_SHAPES.withCorners, | ||
| ...TEST_SHAPES.special, | ||
| }; | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest writing a custom function which generate a baseline/snapshot in the form of a viewable SVG. Eg a big table with rows = test_shapes and columns = TEST_CONFIGS (or at least a subset). That way anyone can easily verify that the results make sense. Currently I can't really validate the snapshots without additional effort
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i pushed my testing file that you can add in generateOutlinePath, thats a reason why i have been making those functions :)
writeSvg(createOutlineSvg(path, pathData, options, {
showControlPoints: true,
showHandleLines: true,
showSegmentPoints: true,
showDirectionArrows: false,
showBounds: true,
}));if you think its a good idea, i can push instead of that my website, tho i used vite to serve it locally into something like docs or play folder as separate PR
Looks good to me, you have comments and tests. Added a couple of comments. Wrt bezier.js, I'm fine to depend on it. I think you're mainly using it for the bezier/bezier and bezier/line intersections. Been a while, but iirc those are fairly easy to reimplement. |
|
bezier rn is used for
splitting (
|

Fixes #89
unblocks nfroidure/svgicons2svgfont#60 nfroidure/svgicons2svgfont#113
this is still an early draft that need a lot of cleanup, i have been working on this for quite some time and optimization and other changes are required, but outline itself is producing now correct results for most path's (~99%)
i'm also not keen on using bezier in here, but in this first draft that was easiest way to verify if this is feasible
this is a revised and improved version of outline generation that i did in nfroidure/svgicons2svgfont#178
Proposed changes
Code quality
License
To get your contribution merged, you must check the following.