Fix DFKai-SB and KaiU font rendering

JimBobSquarePants commented 1 year ago

Prerequisites

[x] I have written a descriptive pull-request title
[x] I have verified that there are no overlapping pull-requests open
[x] I have verified that I am following matches the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules :cop:.
[x] I have provided test coverage for my change (where applicable)

Description

Fixes #337

There's a lot going on here.

The font in question requires hinting in order to be displayed properly (Thanks @behdad !!) but we were not loading the instructions for composite glyphs from the font. As such, I ended up doing a significant refactoring (massively simplifying) of our glyph loading mechanism to make it possible to do so. There is a lot less cloning going on now so far fewer allocations will take place.

TrueTypeInterpreter changes are based on Freetype's implementation and all tests visual and unit remain unaffected.

Here's the correctly rendered font.

Questions.

We do not hint by default as it creates overhead and legacy fonts such as Arial can have minor errors at some font sizes yet this font specifically requires hinting. Should we do as FreeType do and explicitly force hinting if the font is in a known list?

https://github.com/behdad/freetype/blob/08f66322e32cc882733dfae408e040f5057ee2ac/src/truetype/ttobjs.c#L181

codecov[bot] commented 1 year ago

Codecov Report

Merging #340 (a84df2c) into main (b2396dc) will increase coverage by 0%. The diff coverage is 77%.

@@          Coverage Diff          @@
##            main    #340   +/-   ##
=====================================
  Coverage     83%     83%           
=====================================
  Files        227     225    -2     
  Lines      12859   12807   -52     
  Branches    1839    1837    -2     
=====================================
- Hits       10772   10757   -15     
+ Misses      1652    1613   -39     
- Partials     435     437    +2

Flag	Coverage Δ
unittests	`83% <77%> (+<1%)`	:arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...nts/Tables/AdvancedTypographic/GPos/AnchorTable.cs	`33% <0%> (ø)`
...abors.Fonts/Tables/TrueType/Glyphs/ControlPoint.cs	`40% <40%> (ø)`
...nts/Tables/TrueType/Hinting/TrueTypeInterpreter.cs	`55% <48%> (+4%)`	:arrow_up:
src/SixLabors.Fonts/Bounds.cs	`86% <88%> (-4%)`	:arrow_down:
src/SixLabors.Fonts/Tables/Woff/Woff2Utils.cs	`78% <93%> (+<1%)`	:arrow_up:
...nts/Tables/TrueType/Glyphs/CompositeGlyphLoader.cs	`98% <94%> (-2%)`	:arrow_down:
src/SixLabors.Fonts/StreamFontMetrics.TrueType.cs	`97% <100%> (ø)`
...s.Fonts/Tables/TrueType/Glyphs/EmptyGlyphLoader.cs	`100% <100%> (ø)`
...Labors.Fonts/Tables/TrueType/Glyphs/GlyphVector.cs	`94% <100%> (-1%)`	:arrow_down:
....Fonts/Tables/TrueType/Glyphs/SimpleGlyphLoader.cs	`96% <100%> (-1%)`	:arrow_down:
... and 2 more

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

JimBobSquarePants commented 1 year ago

code as it stand look good to me.

The question on overriding the hinting I feel is a little thorny. It feels wrong each font shaper/renderer having to maintain magic lists of special case fonts all over the place... having a global resource tracking these, arguably broken, fonts might be a useful tool to maintain (something that all the libraries can benefit from).. or users can see if they are using specific fonts need specific override settings.

We should then provide a mechanism at the FontCollection level to declare these sorts of override (forcing hinting on/off etc) forcing some specific shapings in other situations maybe too.

These sorts of font issues should be a thing that developers are made aware of, which is why I think an online tool/resource tracking these things and what the overrides shoud be is real answer here. It will end up being a never ending loop of 'I have a boken font, please add the overrides to lib x & lib y etc'.... it should be something that a user of Fonts can self discover and apply the fix in thier code themselves... which will be espcially important for those that are using it in applications where their users are supplying thier own fonts.

If the tracker/online tool is built right it could also be used as a source of metadata needed to auto produce a nuget that can apply all its defaults even... but built/released independently to Font as and when fonts with issues are discovered and configured with their fixes.

Yep. Greate initiative!!

I've created https://github.com/SixLabors/docs/issues/60 for now so we at least document them.

SixLabors / Fonts