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

TTF compilation: off-by-one points and composite bounds when compiling with --round-instances vs. compiling without #593

Closed
madig opened this issue Oct 11, 2019 · 7 comments · Fixed by googlefonts/ufo2ft#356

Comments

@madig
Copy link
Collaborator

madig commented Oct 11, 2019

The glyf table only supports integer coordinates and bounds. Therefore, adding --round-instances when interpolating TTFs should make no difference because both instantiator, ufo2ft and fonttools should use the same rounding method. Using the switch however leads to several point coordinates and composite (not outline glyphs?) bounds being off by one.

RoundInstancesTest.zip

TTX diff (a and b are outline glyphs, c is a composite):

--- NoRoundInstances.ttx        2019-10-11 18:09:35.447933800 +0100
+++ RoundInstances.ttx  2019-10-11 18:09:35.442927000 +0100
[...]
@@ -172,8 +172,8 @@
         <pt x="329" y="446" on="0"/>
         <pt x="382" y="436" on="0"/>
-        <pt x="435" y="420" on="0"/>
+        <pt x="434" y="420" on="0"/>
         <pt x="469" y="401" on="0"/>
         <pt x="469" y="392" on="1"/>
-        <pt x="469" y="384" on="0"/>
+        <pt x="469" y="385" on="0"/>
         <pt x="436" y="365" on="0"/>
         <pt x="384" y="345" on="0"/>
@@ -181,12 +181,12 @@
         <pt x="275" y="317" on="0"/>
         <pt x="261" y="317" on="1"/>
-        <pt x="236" y="317" on="0"/>
-        <pt x="178" y="341" on="0"/>
+        <pt x="237" y="317" on="0"/>
+        <pt x="178" y="340" on="0"/>
         <pt x="136" y="372" on="0"/>
         <pt x="136" y="384" on="1"/>
-        <pt x="136" y="394" on="0"/>
+        <pt x="136" y="393" on="0"/>
         <pt x="176" y="415" on="0"/>
         <pt x="234" y="434" on="0"/>
-        <pt x="292" y="446" on="0"/>
+        <pt x="293" y="446" on="0"/>
       </contour>
       <contour>
@@ -202,22 +202,22 @@
       <contour>
         <pt x="300" y="615" on="1"/>
-        <pt x="329" y="615" on="0"/>
+        <pt x="330" y="615" on="0"/>
         <pt x="393" y="581" on="0"/>
         <pt x="447" y="526" on="0"/>
-        <pt x="481" y="460" on="0"/>
+        <pt x="481" y="461" on="0"/>
         <pt x="481" y="429" on="1"/>
         <pt x="481" y="398" on="0"/>
         <pt x="448" y="329" on="0"/>
-        <pt x="394" y="270" on="0"/>
+        <pt x="394" y="269" on="0"/>
         <pt x="331" y="232" on="0"/>
         <pt x="301" y="232" on="1"/>
         <pt x="271" y="232" on="0"/>
-        <pt x="202" y="271" on="0"/>
+        <pt x="203" y="270" on="0"/>
         <pt x="141" y="331" on="0"/>
         <pt x="102" y="401" on="0"/>
         <pt x="102" y="432" on="1"/>
         <pt x="102" y="463" on="0"/>
-        <pt x="140" y="527" on="0"/>
-        <pt x="201" y="581" on="0"/>
+        <pt x="141" y="527" on="0"/>
+        <pt x="201" y="582" on="0"/>
         <pt x="270" y="615" on="0"/>
       </contour>
@@ -225,5 +225,5 @@
     </TTGlyph>

-    <TTGlyph name="c" xMin="95" yMin="-3" xMax="474" yMax="379">
+    <TTGlyph name="c" xMin="95" yMin="-3" xMax="474" yMax="380">
       <component glyphName="b" x="-7" y="-235" flags="0x4"/>
     </TTGlyph>
@madig madig changed the title TTF compilation: off-by-one points and composite widths when compiling with --round-instances vs. compiling without TTF compilation: off-by-one points and composite bounds when compiling with --round-instances vs. compiling without Oct 14, 2019
@madig
Copy link
Collaborator Author

madig commented Oct 14, 2019

If I insert the following into ufo2ft's setupTable_glyf before glyph.draw(pen):

            if not isinstance(glyph, StubGlyph):
                for contour in glyph:
                    for point in contour:
                        point.x = otRound(point.x)
                        point.y = otRound(point.y)
                for component in glyph.components:
                    t = component.transformation
                    component.transformation = t.__class__(
                        *[*t[:4], otRound(t[4]), otRound(t[5])]
                    )

... the bounds issue seems to be fixed. The points issue remains. Maybe I need to doctor the points before they reach cu2qu.

@madig
Copy link
Collaborator Author

madig commented Oct 14, 2019

Todo:

@anthrotype
Copy link
Member

The component's x and y offsets need to be rounded to integer when the glyf component is being created in the ttGlyphPen, so that the bounds of composite glyphs that contain shifted components are computed the same with/without rounded coordinates. That's easy to fix in fonttools, and I think you're already on it.

As for the 1-point differences in the off-curve points, that's something we can't do much about. Cu2qu works internally with floats. There's lot of operations on floating points that make so that, if cu2qu is passed two UFO instances where one was rounded beforehand (thus contains ints) and another where coordinates are left as floats, then the result will inevitably be slightly different, imperceptibly so when looking at the converted, still unrounded float coordinates; but sometimes off-by-one when these are rounded for compiling a TrueType glyf table.

The current default behaviour of fontmake is to not apply rounding to UFO instances' geometry, unless --round-instances option is passed. I believe it's too late to change that. If we do, then all TTF generated so far may change, even if not visibly.

I suggest we close this as wont-fix.

@anthrotype
Copy link
Member

actually, maybe the change to unconditionally round the component's offsets in ttGlyphPen is not a good idea, and keeping them as floats like the rest of glyphs coordinates (until they are finally rounded by glyf compile method) is better. We just need to document that --round-instances may slightly change the generated output TTFs.

@anthrotype
Copy link
Member

We are not discussing here whether rounding is necessary or not for TrueType, since of course glyf must only contain integer coordinates and components' offsets. What the --round-instances option controls is when the rounding occurs: i.e. before the conversion to quadratic or at the very end upon compiling the glyf table. The latter is the current default and I am not willing to change that. If you opt in to using --round-instances option, then you are accepting that the TTFs generated from such rounded UFO instances may be slightly different (imperceptibly but enough to notice in a TTX diff) than those generated from UFOs without the option.

@madig
Copy link
Collaborator Author

madig commented Oct 14, 2019

Ok... will add --round-instances everywhere then.

@madig madig closed this as completed Oct 14, 2019
@anthrotype anthrotype reopened this Nov 26, 2019
anthrotype added a commit to anthrotype/fonttools that referenced this issue Nov 26, 2019
…sets

googlefonts/fontmake#593

This test currently fails. The compositeGlyph.xMax is set to 281, but it should be 282.
@anthrotype anthrotype reopened this Nov 26, 2019
@anthrotype
Copy link
Member

I'll keep this open while I investigate this issue further. I suspect we also need to change to the way glyph bounding boxes are calculated in ufo2ft.outlineCompiler when UFO contain float coordinates.

anthrotype added a commit to anthrotype/ufo2ft that referenced this issue Nov 27, 2019
Fixes googlefonts/fontmake#593

The input UFO glyphs may contain float coordinates. Instead of computing the
bounding boxes from the UFO glyphs, calculcate them based on the compiled TTGlyph
or CFF CharString objects which have already been rounded off to integer.
This way, the hmtx.lsb will correctly match the glyph xMin, even in presence of
an input UFO with float coordinates.
simoncozens pushed a commit to simoncozens/fonttools that referenced this issue Mar 10, 2020
…sets

googlefonts/fontmake#593

This test currently fails. The compositeGlyph.xMax is set to 281, but it should be 282.
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 a pull request may close this issue.

2 participants