typemytype / booleanOperations

Boolean operations on paths
MIT License
38 stars 18 forks source link

WIP: Attempt to fix issue with segments disappearing #39

Open anthrotype opened 7 years ago

anthrotype commented 7 years ago

This is an attempt at fixing https://github.com/googlei18n/fontmake/issues/180

@typemytype could you please have a look?

it appears like when a curve segment is "flat", i.e. the offcurves are on the same line connecting the previous and current on-curve, and it happens to be the last segment in the output contour, sometimes it can end up being dropped altogether, as shown in the above mentioned fontmake issue.

The glyph in question, which prompted this bug, is the following:

<?xml version="1.0" encoding="UTF-8"?>
<glyph name="uniA73A" format="2">
  <advance width="797"/>
  <unicode hex="A73A"/>
  <outline>
    <contour>
      <point x="0" y="0" type="line"/>
      <point x="29" y="0" type="line"/>
      <point x="245" y="593" type="line" smooth="yes"/>
      <point x="255" y="620"/>
      <point x="267" y="652"/>
      <point x="277" y="682" type="curve"/>
      <point x="289" y="645"/>
      <point x="299" y="617"/>
      <point x="307" y="594" type="curve" smooth="yes"/>
      <point x="515" y="0" type="line"/>
      <point x="544" y="0" type="line"/>
      <point x="797" y="714" type="line"/>
      <point x="767" y="714" type="line"/>
      <point x="567" y="143" type="line" smooth="yes"/>
      <point x="554" y="107"/>
      <point x="542" y="73"/>
      <point x="530" y="40" type="curve"/>
      <point x="517" y="76"/>
      <point x="504" y="112"/>
      <point x="491" y="148" type="curve" smooth="yes"/>
      <point x="291" y="716" type="line"/>
      <point x="265" y="716" type="line"/>
    </contour>
    <contour>
      <point x="146" y="344" type="line"/>
      <point x="655" y="344" type="line"/>
      <point x="655" y="369" type="line"/>
      <point x="146" y="369" type="line"/>
    </contour>
  </outline>
</glyph>

The mergeFirstSegments logic in the (extremely complicated) OutputContour.reCurveSubSegments method seems to be responsible for popping up the last segment. https://github.com/typemytype/booleanOperations/blob/master/Lib/booleanOperations/flatten.py#L765-L803

If the segmentType of such "flat" curves is set to be "line" (because that's what they actually are), then they will be kept.

I hope that makes sense.

anthrotype commented 7 years ago

this other issue seems to be related to that mergeFirstSegments issue as well:

https://github.com/googlei18n/fontmake/issues/181#issuecomment-258485735

The patch in the current PR does not fix the issue with the latter glyph, so I'm no longer sure if it's the correct one.

Only if I comment out the following tow lines (i.e. completely disabling the mergeFirstSegments thing) I can get the segment to not disappear: https://github.com/typemytype/booleanOperations/blob/master/Lib/booleanOperations/flatten.py#L801-L802

I still can't understand what is the rationale for that piece of code.

BTW, this is the glyph in question:

<?xml version="1.0" encoding="UTF-8"?>
<glyph name="A" format="2">
  <advance width="500"/>
  <unicode hex="0041"/>
  <outline>
    <contour>
      <point x="296" y="-467" type="curve" smooth="yes"/>
      <point x="1542" y="-465" type="line"/>
      <point x="1396" y="-393" type="line"/>
      <point x="292" y="-395" type="line" smooth="yes"/>
      <point x="214" y="-395"/>
      <point x="181" y="-349"/>
      <point x="181" y="-269" type="curve" smooth="yes"/>
      <point x="181" y="806" type="line" smooth="yes"/>
      <point x="181" y="885"/>
      <point x="214" y="931"/>
      <point x="292" y="931" type="curve" smooth="yes"/>
      <point x="1531" y="931" type="line" smooth="yes"/>
      <point x="1607" y="931"/>
      <point x="1642" y="885"/>
      <point x="1642" y="806" type="curve" smooth="yes"/>
      <point x="1642" y="760" type="line"/>
      <point x="1731" y="760" type="line"/>
      <point x="1731" y="789" type="line" smooth="yes"/>
      <point x="1731" y="932"/>
      <point x="1663" y="1003"/>
      <point x="1526" y="1003" type="curve" smooth="yes"/>
      <point x="296" y="1003" type="line" smooth="yes"/>
      <point x="159" y="1003"/>
      <point x="91" y="932"/>
      <point x="91" y="789" type="curve" smooth="yes"/>
      <point x="91" y="-253" type="line" smooth="yes"/>
      <point x="91" y="-396"/>
      <point x="159" y="-467"/>
    </contour>
    <contour>
      <point x="1542" y="-465" type="curve" smooth="yes"/>
      <point x="1658" y="-465"/>
      <point x="1729" y="-381"/>
      <point x="1729" y="-282" type="curve" smooth="yes"/>
      <point x="1729" y="-177"/>
      <point x="1661" y="-90"/>
      <point x="1545" y="-90" type="curve" smooth="yes"/>
      <point x="1433" y="-90"/>
      <point x="1362" y="-171"/>
      <point x="1362" y="-278" type="curve" smooth="yes"/>
      <point x="1362" y="-385"/>
      <point x="1432" y="-465"/>
    </contour>
    <contour>
      <point x="1546" y="-398" type="curve" smooth="yes"/>
      <point x="1482" y="-398"/>
      <point x="1441" y="-348"/>
      <point x="1441" y="-281" type="curve" smooth="yes"/>
      <point x="1441" y="-208"/>
      <point x="1481" y="-157"/>
      <point x="1546" y="-157" type="curve" smooth="yes"/>
      <point x="1610" y="-157"/>
      <point x="1649" y="-210"/>
      <point x="1649" y="-283" type="curve" smooth="yes"/>
      <point x="1649" y="-350"/>
      <point x="1606" y="-398"/>
    </contour>
  </outline>
</glyph>
anthrotype commented 7 years ago

from the git log, I can see @readroberts also contributed to this section of the code where it merges the first and last segments when the last segment is a curve: cd59abbb8bb9cb5fe0756dcceaec41270f9c2d14

I wonder if he could also shed some light on this bug.

readroberts commented 7 years ago

I will look at this in the next few days - can't immediately remember what that function is for, although I certainly added it for cases that are otherwise bugs.

marekjez86 commented 7 years ago

@readroberts : is this fixed?

readroberts commented 7 years ago

No, I have not yet taken the time to review this. I expect to do so next week.