googlefonts / ufo2ft

A bridge from UFOs to FontTools objects (and therefore, OTFs and TTFs).
MIT License
151 stars 43 forks source link

Use atan instead of tan to calculate triangle legs #704

Closed yanone closed 1 year ago

yanone commented 1 year ago

As discussed in https://github.com/googlefonts/ufo2ft/issues/703, this PR fixes the triangle leg calculation formula from tan() to atan().

yanone commented 1 year ago

I found a third instance where the wrong formula is used, but I haven't dug into what that code bit does in reality. Changed it anyway because it looked just as wrong.

The failing tests are now a result of the changed formula. 206 is the correct value for -12°, not 213.

yanone commented 1 year ago

Sorry, I need to adjust the tests then...

yanone commented 1 year ago

Okay, I'm done

anthrotype commented 1 year ago

thanks. please hold on, I'd like to double check

anthrotype commented 1 year ago

wait a second, i needed to refresh my trigonometry.. The tangent of an angle is "rise over run", i.e.

tan(angle) = rise / run

hence, the angle whose tangent was rise/run is given by the inverse tangent or "arc tangent" (atan):

angle = atan(tan(angle)) = atan(rise / run)

[assume angles are all in randians]

I hope we agree so far.

Now, say we know the angle and the run, and we want to compute the rise, then we do

rise = tan(angle) * run

Similarly, if we know the angle and the rise and want to compute the run:

run = rise / tan(angle)

Ok, with this in mind, let's look at the current code and your proposed changes.

The current code in openTypeHheaCaretSlopeRunFallback computes the caret slope run from the italicAngle and the slope rise (the latter in turn falls back to 1000 when not set directly and italicAngle != 0). So in the simplest case where only italicAngle != 0 is defined, and no explicit caret slope run/rise is set, then the fallback slopeRise = 1000; and the slopeRun = tan(-italicAngle) * slopeRise

ok.. this is not what I was expecting according to the formulas above where I said run = rise / tan(angle)..

let's step back a little. The italicAngle is relative to the positive Y vertical axis and goes counter-clockwise; a negative angle leans to the right or forward (e.g. -12 the canonical italic angle).

In python math functions, whenever an angle in radians is specified, this angle is assumed to be relative to the positive X _horizontal_axis, that's the 'origin' for angles; positive angles proceed counter-clockwise from there.

So we mean slightly different things when we say 'italicAngle' in the context of post and the math methods. The confusion is compounded by the fact that in a right triangle the relation between the two non-square angles, call them a and b, is such that tan(a) == 1.0 / tan(b) (or, because of floating points.. math.isclose(math.tan(a), 1.0 / math.tan(b))).

So the current code is doing slopeRun = tan(-italicAngle) * slopeRise but what it actually means is

slopeRun = slopeRise / tan(pi/2 + italicAngle)

So AFAIU the current code produces a correct result, however it could be rewritten (and commented) in a way that makes more sense trigonometrically speaking.

Does this make any sense?

anthrotype commented 1 year ago

(wrong button, sorry)

yanone commented 1 year ago

Look, I think this is quite simple: I got my maff wrong. The calculation in ufo2ft was correct all along and I need pull back my PR. I confirmed the calculation in two 2D editors, both of which measure 12° for 1000/213, not 1000/206.

I apologize for the confusion I caused.

anthrotype commented 1 year ago

No worries! Thanks for taking care of checking this :)