Skip to content
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

Update to latest skia library #20

Merged
merged 17 commits into from
Oct 22, 2019
Merged

Update to latest skia library #20

merged 17 commits into from
Oct 22, 2019

Conversation

anthrotype
Copy link
Member

This updates the embedded skia library to the latest revision from git mirror's master branch.
Since currently skia requires a C++14 compiler (and it's 2019), I had to remove support for the 32-bit builds and also for the Windows 2.7 build. I'm still building python 2.7 for Linux/Mac, we can keep that until cython drops supports.

To build for the manylinux1 platform, I needed to use a more recent gcc compiler than the one in the default Docker image from PyPA. I used one that has gcc-9.1.0, and also passed -static-libstdc++ flag.

For Windows, I need to use Visual Studio 2017, which is the miniumum one that supports C++14.

For macOS, specifically only for the Python 3.6 build (not sure why that, and not also for 2.7 or 3.7...), I needed to patch one of skia headers to rename a symbol that was clashing with another macro defined in sys/termios.h system header.
This is the patch:
https://github.com/google/skia/compare/master...fonttools:fix-B0-macro?expand=1
The src/cpp/skia git submodule is configured to point to our skia fork at fonttools/skia.
I will file a bug report upstream sometime later.

There's also one test case that changed output. I haven't had the chance to look at it visually to confirm that it's doing the right thing. I was lazy and simply updated the expected test results.

@anthrotype anthrotype requested a review from madig October 22, 2019 12:44

# install tox to run test suite in a virtual environment
- pip install --upgrade tox

# build wheel
- tox -e wheel
- pip wheel --no-deps --wheel-dir dist .
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed appveyor to build directly using pip instead of indirectly calling tox, because some environment variable was not getting through the tox sandboxing. I know I can use passenv but I didn't know exactly the name of the required env vars (doing passenv=* would have been even lazier). It's not really necessary to build the wheel inside a tox virtual environment. This works just fine.
The tests using the pre-built wheel are still being run inside tox.

- os: linux
env:
- MB_PYTHON_VERSION=2.7
- UNICODE_WIDTH=16
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"narrow" unicode linux py27 builds are also gone, nobody uses these nowadays

@madig
Copy link
Contributor

madig commented Oct 22, 2019

The overwritten test case is suspicious, need to visualize.

@madig
Copy link
Contributor

madig commented Oct 22, 2019

Old:
image

New:
image

@anthrotype
Copy link
Member Author

Thanks for visualizing it!
What do the original paths before overlap-removal look like?

@madig
Copy link
Contributor

madig commented Oct 22, 2019

image
The top stroke goes off into the far distance.

@anthrotype
Copy link
Member Author

Ok. Well, very weird test contours we have. I'm inclined to merge this anyway.

@anthrotype anthrotype merged commit 2872710 into master Oct 22, 2019
@anthrotype anthrotype deleted the update-skia branch October 22, 2019 15:58
@typemytype
Copy link

Does this skia update fix any of the issues where additional inflection points are added or other issues reported on fonttools path ops?

@anthrotype
Copy link
Member Author

unfortunately no..

@anthrotype
Copy link
Member Author

btw it looks like Cary Clark has left Google since the time we got in touch about these issues.
The major issue I see currently is #11.
We should probably send a feature request to https://bugs.chromium.org/p/skia/issues asking if it would be possible to re-join these complex curves that had been split for the sake of finding intersections so that we don't end up with unnecessary additional oncurve points.
@typemytype fancy doing that?

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

Successfully merging this pull request may close these issues.

3 participants