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

OpenType font spacing differs after update of FreeType from 2.9.1 to 2.10.0 (Overpass font) #28335

Closed
ForLoveOfCats opened this issue Apr 23, 2019 · 38 comments

Comments

@ForLoveOfCats
Copy link
Contributor

Godot versions:
Godot Mono 3.1.0 Beta 11 and Godot Mono 3.1.1 RC1

OS/device including version:
Manjaro Linux x64 w/Intel Sandybridge Mobile iGPU running Mesa 19.0.2-1

Issue description:
Upon upgrading from Godot Mono 3.1.0 Beta 11 to Godot Mono 3.1.1 RC1 my game's buttons went from this:
image
to this:
image

Switching between GLES2 and GLES3 does not effect either version. For the sake of completeness I will mention that these are both Mono builds but that should (hopefully) not be related to this issue.

Here is a reproduction project: ButtonSizeBug.zip
Godot Mono 3.1.0 Beta 11:
image
Godot Mono 3.1.1 RC1:
image

@ForLoveOfCats
Copy link
Contributor Author

ForLoveOfCats commented Apr 23, 2019

I possible I would like to attempt to fix this myself as I am trying to become more familiar with Godot's codebase and contribute more.

Edit: The hardware I am stuck with ATM is not up to compiling Godot quite a few times in order to find the root commit

@hpvb
Copy link
Member

hpvb commented Apr 23, 2019

Can you please check the difference between 3.1.0-stable and 3.1.1-rc1?

@ForLoveOfCats
Copy link
Contributor Author

Issue is not present in Godot Mono 3.1.0 Stable
image

@hpvb
Copy link
Member

hpvb commented Apr 23, 2019

OK, so this is a difference between 3.1.0-stable and 3.1.1-rc1?

@ForLoveOfCats
Copy link
Contributor Author

ForLoveOfCats commented Apr 23, 2019

The issue is not present in 3.1.0 Stable but is present in 3.1.1 RC1

@hpvb hpvb added this to the 3.1 milestone Apr 23, 2019
@hpvb hpvb self-assigned this Apr 23, 2019
@hpvb
Copy link
Member

hpvb commented Apr 23, 2019

If you want to look at this yourself use git bisect between 3.1.0-stable and the tip of the 3.1 branch. You should find it with about 7 compilations currently. Then we know what backport caused it and we can see if there's a fix for it already or revert it in 3.1 before release of 3.1.1-stable.

@ForLoveOfCats
Copy link
Contributor Author

Oof I'm stuck on this under powered laptop for the time being... While I would like to try to fix this myself that just isn't practical. Editing my original comment about doing this myself.

@akien-mga akien-mga changed the title Button shorter after upgrading 3.1.0 Beta 11 -> 3.1.1 RC1 Button shorter after upgrading 3.1-stable -> 3.1.1 RC1 Apr 23, 2019
@akien-mga
Copy link
Member

akien-mga commented Apr 23, 2019

I don't see any difference with the attached project between 3.1-stable and current 3.1 HEAD (476e179) (non Mono build).

Edit: I do get the issue in 3.1.1-rc1 though. Testing a release_debug build locally to see if it's related.

@akien-mga
Copy link
Member

Tested so far, all non-Mono on X11 64-bit:

  • 3.1-stable (release_debug, LTO): OK
  • 3.1.1-rc1 aka 39f1a11 (release_debug, LTO): bug
  • 39f1a11 local build (debug, no LTO): OK
  • 4764e17 local build (debug, no LTO): OK
  • 4764e17 local build (release_debug, no LTO): OK
  • 4764e17 local build (release_debug, LTO): OK

So not sure what's left to test, but something is wrong only in the official 3.1.1-rc1 build.

@hpvb
Copy link
Member

hpvb commented Apr 23, 2019

This is very puzzling as no changes were made to the build containers at all.

@akien-mga
Copy link
Member

This is probably due to the freetype version change.

@hpvb
Copy link
Member

hpvb commented Apr 23, 2019

I'll test with the freetype version reverted.

akien-mga added a commit that referenced this issue Apr 25, 2019
This reverts commit 9e2cf9e.

It caused this regression: 28335.
Fixes #28335.
@akien-mga
Copy link
Member

I can confirm that the update of the builtin freetype version is the problem. I've reverted it in the 3.1 branch, so the bug is fixed there, but we still need to figure out the issue for the master branch (especially as it means that linking against system freetype 2.10.0+ may trigger the issue again for 3.1.x or even 3.0.x).

@akien-mga akien-mga modified the milestones: 3.1, 3.2 Apr 25, 2019
@akien-mga akien-mga changed the title Button shorter after upgrading 3.1-stable -> 3.1.1 RC1 Button shorter after update of FreeType from 2.9.1 to 2.10.0 Apr 25, 2019
@volzhs
Copy link
Contributor

volzhs commented Jun 26, 2019

freetype

Overpass font freetype 2.9 freetype 2.10
ascent 16 12
descent 5 4
NotoSansUI font freetype 2.9 freetype 2.10
ascent 18 18
descent 5 5

I tested freetype 2.9 and 2.10
the ascent and descent value are different with specific fonts.
as a result, the overpass font looks stuck to the top.

I looked over various values inside FT_Face and changes log.
but couldn't find a clue to fix it.

@akien-mga would it be better to revert freetype 2.10?

@Zireael07
Copy link
Contributor

IIRC freetype 2.10 has been reverted?

@akien-mga
Copy link
Member

would it be better to revert freetype 2.10?

No, we need to find a way to configure things to work the same on 2.9 and 2.10. Distros for example will typically unbundle freetype and build against their system version, and we can't freeze on 2.9 forever.

If we can't find a config option to fix this, we should probably contact upstream FreeType as this would be a compatibility breakage in 2.10.

@volzhs
Copy link
Contributor

volzhs commented Jun 26, 2019

dug up for several hours more but still no clue.
no time for investigating this issue.

@akien-mga
Copy link
Member

@hpvb OK with defaulting all stable branches to using builtin freetype on Linux for the time being?

I've already done that on master as we now need libpng16 (which Ubuntu < 18.10 and matching Debian still don't have out of the box), and if we use builtin libpng then we also need to make freetype and zlib builtin.

I think we should do the same on stable branches to be on the safe side, and so that users making custom builds don't get hit by this issue. Doesn't change much for distros who will have to opt-in for zlib, libpng and freetype, like they already have to do for all other vendored deps.

@akien-mga akien-mga assigned akien-mga and unassigned hpvb Jul 2, 2019
@akien-mga
Copy link
Member

OK with defaulting all stable branches to using builtin freetype on Linux for the time being?

Done that for all stable branches via cherry-picks of #29998.

@Anutrix
Copy link
Contributor

Anutrix commented Jul 3, 2019

These might possibly useless/unrelated info.
Maybe these?
golang/freetype#34 (comment)


2018-08-08  Alexei Podtelezhnikov  <[email protected]>

[pcf] Massive unsigning (part 1).

Unofficial specifications hesitate to use unsigned 32-bit integers. Negative values caused a lot of trouble in the past and it is safer and easier to treat some properties as unsigned.

 * src/pcf/pcf.h (PCF_AccelRec): Use unsigned values for `fontAscent',
`fontDescent', and `maxOverlap'.
* src/pcf/pcfread.c (pcf_load_font, pcf_get_accel): Updated.
* src/pcf/pcfdrivr.c (PCF_Glyph_Load, PCF_Size_Select,
PCF_Size_Request): Updated.

These?

- height is defined as (ascent - descent). Is this correct?

@akien-mga
Copy link
Member

Good finds, thanks. I'd suggest to step through Error DynamicFontAtSize::_load() with a debugger and compare values for the different metrics when loading a project in the master branch (using FreeType 2.10.0) and in the 3.1 branch (using FreeType 2.9.1).

@Anutrix
Copy link
Contributor

Anutrix commented Jul 3, 2019

Info so far:
Branch - master : 3.1
metrics.ascender Ratio - 3 : 4
metrics.descender Ratio - 4 : 5

Whenever this if block executes, ascender and decender values(not sure about other values) set in 3.1 and master:

error = FT_Open_Face(library, &fargs, 0, &face);

Update: These ratios were co-incidence.

@Anutrix
Copy link
Contributor

Anutrix commented Jul 5, 2019

Finally found the different values.
The face->ascender and face->descender in 2.9.1 is HHead values.
The face->ascender and face->descender in 2.10.0 is Typo values.
The following image should make it apparent.
tempsnip
I have manually confirmed the values by using debug messages in source files.
Notice that it is same in Notosans and hence doesn't issues in either.
First point in Miscellaneous section of CHANGELOG.

Here is the same Overpass-SemiBold.ttf with just that checkbox "Really use Typo metrics" unchecked and saved. This has same button height in both 3.1 and master.
Full Test Project(because the one in OP has path issues): ButtonSizeBug2.zip.

@volzhs
Copy link
Contributor

volzhs commented Jul 5, 2019

so you modified ttf file instead of code?

@volzhs
Copy link
Contributor

volzhs commented Jul 5, 2019

@akien-mga freetype released new 2.10.1 version.
it's better to test with the 2.10.1 version to see if solve this issue.
I like to do it myself, but I don't have much time these days.
I know you are also busy these days too. 😃

@Anutrix
Copy link
Contributor

Anutrix commented Jul 5, 2019

so you modified ttf file instead of code?

The changes are on Freetype's side so I couldn't do anything.
Note(just bragging): I changed Freetype's code about hundred times to print debugging face->ascender values at different times while testing.
Note: I don't know anything about how fonts work.

@Anutrix
Copy link
Contributor

Anutrix commented Jul 5, 2019

@akien-mga freetype released new 2.10.1 version.
it's better to test with the 2.10.1 version to see if solve this issue.
I like to do it myself, but I don't have much time these days.
I know you are also busy these days too. 😃

I tested Godot with Freetype 2.10.1 stable source today. Issue persists just like 2.10.0 . Still not sure if this is their bug or feature.

@Zireael07
Copy link
Contributor

Probably a compat-breaking feature, judging from their changelog notes?

@volzhs
Copy link
Contributor

volzhs commented Jul 5, 2019

@Zireael07 the problem is font is not vertically center aligned comparing to freetype 2.9

@akien-mga
Copy link
Member

Nice work @Anutrix.

So in the end it's an issue specific to OpenType fonts using the Really use typo metrics flag, and it's documented in the FreeType changelog:

  • The logic for computing the global ascender, descender, and
    height of OpenType fonts has been slightly adjusted for
    consistency.

    • If the useTypoMetrics' flag (i.e., bit 7 in the fsSelection'
      field) in the OS/2' table is set, use the sTypo' fields in
      `OS/2' unconditionally.
    • Otherwise use the metrics data from the `hhea' table (if not
      zero).
    • Otherwise use the sTypo' fields from the OS/2' table (if not
      zero).
    • Otherwise use the usWin' data from the OS/2' table as a last
      resort.

Is there anything we can do at the engine side to mitigate breaking compat for those fonts, or should we simply document this as a change in 3.2 and how to adapt fonts whose behavior changed?

@volzhs
Copy link
Contributor

volzhs commented Jul 5, 2019

I had looked in various way to solve it on our side.
but couldn't find it.

image
there is a workaround for those font in the font settings.

@Anutrix
Copy link
Contributor

Anutrix commented Jul 5, 2019

Disclaimer: I don't much about fonts. Just started reading on it recently.
I would like someone to test all fonts having this issue. According to what I have read, the values of the 3 versions of ascents/descents should be really close. Hence, only few fonts seem to have this issue.
Eg. Overpass 750 vs 982
Reporting the issues to respective font developers to be good IMHO.

@akien-mga akien-mga changed the title Button shorter after update of FreeType from 2.9.1 to 2.10.0 OpenType font spacing differs after update of FreeType from 2.9.1 to 2.10.0 (Overpass font) Jul 11, 2019
@akien-mga
Copy link
Member

As per @Anutrix's debugging, it seems to be an expected consequence of changes in FreeType's handling of some OpenType font metrics, which introduce this visual difference for fonts with invalid metrics.

So that's an issue for font providers to handle on their end (or negotiate with FreeType devs if they feel this 2.10 change was incorrect). From our end, I think we should just document this in the 3.2 changelog (cc @Calinou), ideally with a workaround for affected users (e.g. how to fix the font, or adding extra spacing as shown by @volzhs).

@apodtele
Copy link

This is the change that you question:
https://git.savannah.gnu.org/cgit/freetype/freetype2.git/commit/?id=f72b00746c64e13625cf8371f031411fbd0d6161

And the long discussion about it:
https://marc.info/?t=154508255300008&r=1&w=2

There is no reason for FreeType to disobey the font flags.

@akien-mga
Copy link
Member

Thanks for digging this up :)

We can close this as "not a bug" once this FreeType change is mentioned in our release notes (cc @Calinou). This is strictly something for font providers to handle so that their font metrics work as expected by the OpenType spec and FreeType implementation.

@Calinou
Copy link
Member

Calinou commented Jul 17, 2019

@akien-mga I added a mention of this change in the changelog.

@akien-mga
Copy link
Member

Thanks, closing then.

@madig
Copy link

madig commented Nov 23, 2019

The typo metrics have some semantics attached to them, so it is the responsibility of the text layout engine to properly handle them: https://docs.microsoft.com/en-us/typography/opentype/spec/recom#tad. The appearance of text being "shifted up" usually comes from the typo metrics providing a more accurate but tighter bounding box, while the head metrics usually carry the line gap at the top for compatibility reasons. Take that away and you get a "shift upwards".

The UseTypoMetrics flag really signals that the layout code must to take over. Trying to make vertical metrics the same across platforms is impossible, it is much better to expose controls to the designer to fully control line-spacing.

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

No branches or pull requests

9 participants