-
Notifications
You must be signed in to change notification settings - Fork 14
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
WIP Keep track of on-curve points in the original path #47
base: master
Are you sure you want to change the base?
Conversation
|
||
def qCurveTo(self, *points): | ||
for pt1, pt2 in _decompose_quadratic_segment(points): | ||
self._qCurveToOne(pt1, pt2) | ||
self.path.originalOnCurvePoints.add(points[-1]) |
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 realize now that I could probably only keep track of the original on-curves at the end of qCurves, instead of all types of original on-curves?
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.
yeah, if we have to remember oncurve points, it's probably better to only do it for when we actually need it, i.e. for qCurve segments only where they can be "implied", the other segment types in pen protocol are all explicit
# TODO: figure out how to transform the original oncurve points using the matrix | ||
# result.originalOnCurvePoints = set( | ||
# transform_tuple_point(matrix, pt) | ||
# for pt in self.originalOnCurvePoints |
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.
SkMatrix
has mapPoint
and mapPoints
(for array of SkPoints) methods:
https://api.skia.org/classSkMatrix.html#a545fc5d678f0c07c40636bc4cb699017
one problem with keeping a set of 'originalOnCurvePoints' is that Path is a mutable object (new paths can be added, or the container can be cleared with reset/rewind methods) so they may get out of sync. |
assert items[2][0] == PathVerb.QUAD | ||
assert items[2][1] == ((2.0, 2.0), (3.0, 3.0)) | ||
|
||
# when drawn back onto a SegmentPen, the implicit on-curves are omitted |
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.
# when drawn back onto a SegmentPen, the implicit on-curves are omitted | |
# when drawn back onto a SegmentPen, the explicit on-curves are preserved |
Hello @anthrotype ! This is both an issue and a pull request that starts to fix the issue. Here is the problem: a designer drew a "registered" sign that is so symmetrical that the code in skia-pathops to reconstruct long quadratic curves with implicit points from the small quadratic curves that come out of Skia thought that the top extrema of the curve, which was a real on-curve point initially, was actually an implicit point and made it disappear. That then caused confusion when the designer was trying to hint the glyph in VTT. See screenshots: on the left, the glyph as it comes out of current skia-pathops, on the right as it comes out with this patch.
I added a test and I'm proposing a way to fix this by remembering the coordinates of the intended on-curve points in the Path object, and then making sure that they're still on-curve after reconstructing the long qCurveTo commands.
However I didn't finish implementing this properly because I'm not sure how thorough I should be with this, e.g. right now it only works if you draw on the Path with its pen, but doesn't if you add path bits via the Path methods...
Also I haven't figured out how to use the Skia matrix transformation to transform single points (needed to keep track of "original on-curves" while decomposing components with transformations).
And also it's the first time I touch Cython code and I don't know whether I'm trashing performance with my changes.