removeOverlaps: work around pathops.simplify error #2288
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sometimes skia-pathops simplify may fail (for unknown reasons which I'm still trying to debug). It's a good idea to know the name of the offending glyph google/fonts#3365
3e6a095 to
d7b3b9f
Compare
d7b3b9f to
84b8513
Compare
a9dab1a to
5dd1877
Compare
5dd1877 to
80598d3
Compare
rsheeter
reviewed
May 7, 2021
rsheeter
approved these changes
May 7, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Sometimes skia-pathops simplify may fail (for unknown reasons which I'm still trying to debug).
It's a good idea to know the name of the offending glyph
google/fonts#3365
EDIT: It's actually even better to not fail at all. It turns out that if we round the path's coordinates to integers before passing it on to pathops.simplify, then it works -- at least for the two glyphs in the Noto Ethiopic and Telugu fonts which triggered this issue.
The effect of rounding before removing overlaps is minimal, since we are going to round afterwards anyway once we encode the glyf table; besides, we only try rounding when simplify fails with the unrounded float coordinates (the ones that we get from the instancer).
All in all, I believe the workaround is better than blocking those projects while the Skia bug is fixed upstream (https://bugs.chromium.org/p/skia/issues/detail?id=11958), which may take a while since we have no idea what's going on under the hood.