Skip to content
This repository has been archived by the owner on Dec 21, 2019. It is now read-only.

WIP: Implement qcurve handling in tfont #15

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

WIP: Implement qcurve handling in tfont #15

wants to merge 9 commits into from

Conversation

madig
Copy link
Contributor

@madig madig commented Dec 1, 2018

This makes the text tool work.

Todo: look at what's missing. Intersection to make ruler work.

TestFont.ufo was cribbed from defcon. Had to change public.backgrounds A to not be all-off-curves, otherwise UFOConverter would be stuck in an infinite loop trying to shuffle points around ;)

bounds = arrayTools.unionRect(
bounds, bezierTools.calcQuadraticBounds(first_anchor, p1, p2)
)
first_anchor = p2
Copy link
Member

Choose a reason for hiding this comment

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

That's a lot of intermediate functions

Copy link
Member

Choose a reason for hiding this comment

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

prob. fine for now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What would speak against reusing fontTools functionality?

Copy link
Member

Choose a reason for hiding this comment

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

Simplicity & speed, basically

@@ -257,6 +257,8 @@ def __repr__(self):
def bounds(self):
points = self.points
pts_len = len(points)
Copy link
Member

Choose a reason for hiding this comment

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

Here you can do type = self._points[self._end].type instead of using len.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why not self.type?

@adrientetar
Copy link
Member

There is code for intersection here: https://github.com/trufont/trufont/blob/wx/src/trufont/util/bezierMath.py#L99

@madig
Copy link
Contributor Author

madig commented Dec 1, 2018

Neat! Any reason to have a bezierMath.py in Trufont and tfont?

@madig
Copy link
Contributor Author

madig commented Dec 1, 2018

Ugh. The TruFont function doesn't seem to work (aside from pc0 having a - instead of a + in it) or I'm confused. My trigonometry-fu is very weak. I found https://stackoverflow.com/questions/27664298/calculating-intersection-point-of-quadratic-bezier-curve but the math in there seems to work differently. E.g. in fontTools' solveQuadratic function, it computes DD = b * b - 4.0 * a * c which turns DD negative with the example curve and line and therefore returns no root, whereas the JS example computes DD = b * b - 4.0 * c and leaves out the / a. in the root calculation. Debugging...

@madig
Copy link
Contributor Author

madig commented Dec 1, 2018

Ok, so this is a very rough implementation that works. I don't know why fontTools' solveQuadratic works differently. I'd have to work out the math to avoid the _lerp(). The previous way of computing s0 and s1 looked better.

@madig
Copy link
Contributor Author

madig commented Dec 1, 2018

And for some reason, depending on ufoLib2 does not pull in fontTools[ufo]. Ugh.

@madig
Copy link
Contributor Author

madig commented Dec 1, 2018

Still occasionally get

  File "/home/nikolaus/Entwicklung/tfont/src/tfont/util/bezierMath.py", line 132, in qcurveIntersections
    pc1 = (nx * bx + ny * by) / pc0
ZeroDivisionError: float division by zero

Just catch that and return []?

@madig
Copy link
Contributor Author

madig commented Dec 2, 2018

@madig
Copy link
Contributor Author

madig commented Dec 2, 2018

I gave up and simply adapted curveIntersections' math for quadratics. It werks! 🎉

@madig
Copy link
Contributor Author

madig commented Dec 2, 2018

splitSegment is tricky I think, as we don't know which subsegment of the qcurve segment the t belongs to 🤔

@madig
Copy link
Contributor Author

madig commented Dec 5, 2018

I suppose I could change the return value of (q)curveIntersections to include the starting point of the curve the t belongs to, then it's just iterating over the implied subsegments until you hit the right starting point and splitting the subsegment.

@adrientetar
Copy link
Member

Or the index of the segment. then it can be accessed directly https://github.com/trufont/trufont/blob/wx/src/trufont/objects/factories.py#L104

@madig
Copy link
Contributor Author

madig commented Dec 22, 2018

What is the reason cmp is turned off for Point?

@adrientetar
Copy link
Member

What is the reason cmp is turned off for Point?

Comparison is based on identity so two different points with the same coordinates wouldn't hash to the same value.

@madig
Copy link
Contributor Author

madig commented Jan 29, 2019

I find this maddeningly tricky while it looks like there must be an easy solution.

@madig
Copy link
Contributor Author

madig commented Feb 22, 2019

@adrientetar So this mostly works, please advise on the segment list bookkeeping and what to return as newSegment.

I have code like

points_inserted = len(points_to_insert) - 1
segments.insert(index, newSegment)
new_segments = []
index_start = start
for index, point in enumerate(points[start : start + points_inserted]):
    if point.type is not None:
        new_segments.append(Segment(points, index_start, index))
        index_start += index + 1
segments[index:index+1] = new_segments
newSegment = new_segments[0]
for seg in segments[index + 1 :]:
    seg._start += points_inserted
    seg._end += points_inserted

But that likes to delete everything except one point under certain circumstances.

Even with the sledgehammer method in the PR, sometimes I can literally draw a cutting line through a glyph, deleting everything below it, if I cut a glyph just right...

@madig
Copy link
Contributor Author

madig commented Feb 23, 2019

Adding the fs dependency directly is ugly, but specifying fonttools[ufo] instead does not work...

Edit: alternative approach: always depend on fontools[ufo,lxml].

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

Successfully merging this pull request may close these issues.

2 participants