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

WIP: Attempt to fix issue with segments disappearing #39

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

anthrotype
Copy link
Collaborator

This is an attempt at fixing googlefonts/fontmake#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
Copy link
Collaborator Author

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

googlefonts/fontmake#181 (comment)

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 anthrotype changed the title flatten: convert 'flat' curves to lines if off-curves are aligned with on-curves WIP: Attempt to fix issue with segments disappearing Nov 4, 2016
@anthrotype
Copy link
Collaborator Author

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: cd59abb

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

@readroberts
Copy link

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
Copy link

@readroberts : is this fixed?

@readroberts
Copy link

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

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