booleanGlyph: use ufoLib as fallback

[adrientetar]

r? @typemytype

[typemytype]

Im not going to merge this request as I guess it is best to create a UFO3 branch and not combine UFO2 and UFO3 in a single branch

@anthrotype how does branching work best with all the automatic compiling things?

[typemytype]

This difference could be very dramatic: (not only for BooleanOperations)

from robofab.pens.boundsPen import BoundsPen as RFBoundsPen
from fontTools.pens.boundsPen import BoundsPen as FTBoundsPen

from robofab.world import RGlyph

def getBounds(g):
    pen = RFBoundsPen(None)
    g.draw(pen)
    print "RoboFab:", pen.bounds

    pen = FTBoundsPen(None)
    g.draw(pen)
    print "RoboFab:", pen.bounds

g = RGlyph()
g.appendAnchor("name", (100, 100))
getBounds(g)

g = RGlyph()
pen = g.getPen()
pen.moveTo((100, 100))
pen.endPath()
getBounds(g)

g = RGlyph()
pen = g.getPen()
pen.moveTo((100, 100))
pen.lineTo((200, 200))
pen.endPath()
getBounds(g)

[adrientetar]

tbh this change hasn't much to do with ufo2 vs. ufo3, it solely results from the split of ufoLib from robofab (it's true that the ufoLib package uses ufo3 internally, but it could use ufo2 that it wouldn't change anything – this PR does dispatch calls to the adapterPens that were moved to ufoLib along the split. Had ufoLib been split in ufo2 times, we wouldn't need to do such dispatch). FWIW.

[anthrotype]

I think branching booleanOperations into a UFO2 and UFO3 version just because of the boundsPen is a bit too much. I suggest adding an "ignoreSinglePoints" argument to the bounds pen's constructor like in https://github.com/anthrotype/fonttools/commit/dc2ef2e341e4ab868801b4b3ff5a867114f182e4

[adrientetar]

I have pushed a new commit so to use robofab by default and ufoLib + fontTools with the additional arg otherwise. @anthrotype Thanks, will you push that commit to mainline? Also, anyone know why fontTools does calculate bounds with the anchors included? cc @behdad

[behdad]

What are the glyph bounds used for anyway?

[adrientetar]

@behdad I see in "optimization ideas" in the comments that contour-bounds could be used for fast-rejection of overlaps. Otherwise it doesn't seem to be used but is provided to the user, would the user need to calculate bounds? idk.

[anthrotype]

BooleanContour objects have a bounds property: https://github.com/trufont/booleanOperations/blob/patch-1/Lib/booleanOperations/booleanGlyph.py#L99

the docstring of boleanOperationManager.py explains

- Contours should only be sent here if they actually overlap.
  This can be checked easily using contour bounds.

@adrientetar beat me.

[behdad]

Exactly. My point was: if bounds for a few glyphs are not tight (include anchor points), it shouldn't be horrible. Or does this apply to all anchor points (not only lone points encoded in the glyph outline as well?)

[adrientetar]

Or does this apply to all anchor points (not only lone points encoded in the glyph outline as well?)

I'm not sure I understand but basically the fontTools pen will include the glyph's anchors when calculating bounds, which the robofab one doesn't. But the robofab one ignores single-point contours as-well since they are expressed the same in the pen protocol.

One solution would be to make caller not pass anchors to the pen.

Anyway I just notice that the _get_bounds is on BooleanContour (not Glyph) so anchors shouldn't get drawn in the first place here?

[anthrotype]

the fontTools pen will include the glyph's anchors when calculating bounds

yes, if one uses the fonttools pen to draw a whole glyph (like in @typemytype example above). If one uses the draw method of a contour, anchors are not passed on to the pen (they have their own draw and drawPoints methods).

the _get_bounds is on BooleanContour (not Glyph) so anchors shouldn't get drawn in the first place here

exactly. For this particular instance, the only difference between using the fonttools' or robofab's BoundsPen is that the latter excludes single-point paths.

From the point of view of booleanOperations, one is only interested in the bounds of closed paths, and a single-point contour is open by definition, though it's not the only one. Maybe a more appropriate bounds pen for booleanOperations is one that excludes not only single points but any non-closed paths?

[adrientetar]

From the point of view of booleanOperations, one is only interested in the bounds of closed paths, and a single-point contour is open by definition, though it's not the only one. Maybe a more appropriate bounds pen for booleanOperations is one that excludes not only single points but any non-closed paths?

Good question. Maybe.

Anyway this PR should be clear of any blockers now. @typemytype, is it fine to merge now?

[adrientetar]

@typemytype: can this PR be merged? thanks.

[typemytype]

the bounds pens still returns differences, somehow a BooleanGlyph has to detect the pen is the fontTools one and enable the new ignoreSinglePoints attribute.... and this doest sound like good practice.

[adrientetar]

@typemytype _get_bounds is a method of BooleanContour, which doesn't have anchors (they are in BooleanGlyph).

[typemytype]

A boolean glyph can have anchors as it inherits anchors and other data from a source glyph

On Monday, 18 January 2016, Adrien Tétar notifications@github.com wrote:

@typemytype https://github.com/typemytype _get_bounds is a method of BooleanContour, which doesn't have anchors (they are in BooleanGlyph).

— Reply to this email directly or view it on GitHub https://github.com/typemytype/booleanOperations/pull/20#issuecomment-172497480 .

gr Frederik www.typemytype.com

[adrientetar]

@typemytype Agreed, that's what the code does. However I was pointing out that the _get_bounds method is on BooleanContour, not BooleanGlyph and so the contour object unlike its glyph parent does not store the anchors. So in turn, the BoundsPen will never be passed anchors given that it is called from a contour object, not a glyph object.

[typemytype]

Check, I thought a BooleanGlyph object also had a bounds attribute, my bad

[adrientetar]

:+1: