fonttools / skia-pathops

Python bindings for the Skia library's Path Ops
https://skia.org/docs/dev/present/pathops/
BSD 3-Clause "New" or "Revised" License
47 stars 14 forks source link

rename top-level package simply 'pathops' #5

Closed anthrotype closed 6 years ago

anthrotype commented 6 years ago

Initially I thought of making this only the bindings for the skia pathops module, and have a separate higher-level package like booleanOperations that would use that and provide a UFO PointPen interface for UFO editors. That's why I called the top-level module "skia", with submodules like core and pathops following skia's API.

However, after discussing with Adrien, we decided to keep this simpler and have a single "pathops" python package which contains a cython extension module calling the skia C++ library, and which also provides a python interface similar to booleanOperations: one function per each boolean operation, taking a list of UFO contours (with drawPoints method) and an output pointPen to draw the result on. This way, we can call this skia-pathops directly from fontmake or TruFont.

BooleanOperations (the python module) can still use skia-pathops internally, as it provides other useful classes like the BooleanGlyph which we don't currently need in our pipeline. But that's a separate concern.

The pathops/init.py is where the python api will be exported. Then we have a pathops._pathops.so extension module (built from the pathops/_pathops.pyx cython file) that translates between Python and C++.

Inside pathops/_skia/ folder are the *.pxd files with the external declarations for the skia library that are called from Cython code.

I added the _ to the _skia subfolder to mean this it is private. i.e. this is not a full-blown python bindings to skia, just the headers we care about here.

I hope this makes sense.

anthrotype commented 6 years ago

oh look, the Appveyor webhook has been triggered by the pull request.. but it's still in the infinite queue :disappointed:

4

anthrotype commented 6 years ago

so for example, the way you would use this module would be something like this

from pathops import union

pen = glyph.getPointPen()
contours = list(glyph)
glyph.clearContours()
union(contours, pen)
anthrotype commented 6 years ago

@khaledhosny @adrientetar shall I keep the distribution name "skia-pathops", even though the module is now called pathops? Do you think this may confuse users expecting the python package name and the distribution package name to be the same?

I believe we need to mention "skia" somewhere, as prominently as in the name itself.

anthrotype commented 6 years ago

as things stand right now, one would pip install skia-pathops, but import pathops.

adrientetar commented 6 years ago

yes I think that's fine.

it provides other useful classes like the BooleanGlyph

I never used BooleanGlyph since I noticed the pen API which I think is better.

typemytype commented 6 years ago

I guess this will be more difficult as it when you perform everything in the cpython thingy:

how to maintain the identifiers, point names and extra data a point pen can handle when it goes through skia penlike api. And you dont want to lose that data when applying a boolean operation. As skia pathops only knows about performing tasks on path structures.

fe keeping identifiers is crucial in some hinting implementation, even after a remove overlap... the identifier needs to be there

I dont mind if this become the responsibility of BooleanOperations and this pathops is just a simple skin on top of skia... without such features and functionalities

BooleanGlyph is super useful as it provides math solution for glyph paths similar to what a mathGlyph does for interpolation...

@anthrotype when pathops is working, I would need some help to implement pathops in BooleanOperations so will install nicely, like it does now

anthrotype commented 6 years ago

I guess this will be more difficult as it when you perform everything in the cpython thingy

not necessarily. we can decide how much to push down into C and how much to keep at the pure-python level depending on our needs. That's the beauty of Cython.

how to maintain the identifiers, point names and extra data a point pen can handle... skia pathops only knows about performing tasks on path structures.

it's a good point, haven't thought about that yet. We can probably keep them aside and re-insert them back somehow.

when pathops is working, I would need some help so BooleanOperations will install nicely

no problem, i'll help with that

adrientetar commented 6 years ago

how to maintain the identifiers, point names

Oh, I never used these thus far. But if contours get merged you have to drop them/regenerate anyway, no? Do you try to match the points that are not removed by the boolean op to their respective extra data?

and extra data

This extra data support has to be written specially for the custom classes you are using, yes?

I dont mind if this become the responsibility of BooleanOperations and this pathops is just a simple skin on top of skia...

Yes in any case I think we definitely want the generic API to be exposed, and we can probably fold extra functionality to this package as well if it's "generic" enough, i.e. not too tied to a specific use-case.

BooleanGlyph is super useful as it provides math solution for glyph paths

Do you mean the operator overload like glyph1 & glyph2? I think that if desired it could be implemented in the custom defcon subclass directly, I mean I personally don't get why the MyGlyph subclass from defcon AND the BooleanGlyph are both needed.

adrientetar commented 6 years ago

I mean I personally don't get why the MyGlyph subclass from defcon AND the BooleanGlyph are both needed.

Or you could use multiple inheritance and derive from a BooleanGlyph class ("mixin") that has these operations implemented, I guess. If the concern is not repeating the code/splitting it into the booleanOps package.

anthrotype commented 6 years ago

not the arithmetic (+ - * /) operators, but the set operators (& | - ^)

typemytype commented 6 years ago

point pens accepts **kwargs and it would be nice to preserve them, like BooleanOperation does