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

Switch to using alphabetic baseline for local glyphs. #10390

Merged
merged 1 commit into from Feb 19, 2021
Merged

Conversation

ChrisLoer
Copy link
Contributor

@ChrisLoer ChrisLoer commented Feb 17, 2021

This PR integrates the TinySDF changes at mapbox/tiny-sdf#30 which switch to drawing glyphs on the "alphabetic" baseline instead of the "middle" baseline. This gives better visual alignment between fonts. The new version of TinySDF will have to be published before this PR builds successfully.

Here's an overlay of the Meiryo and YaHei fonts, showing how the alphabetic baseline matches expectations (Meiryo "broke" earlier assumption because it has a very large font descender):

Meiryo vs YaHei baselines

TinySDF's "top" metric now means "distance from alphabetic baseline to top of glyph" which seems pretty intuitive but notably different from what https://github.com/mapbox/sdf-glyph-foundry generates. This PR adjusts to bring the two metrics mostly into line.

Here are some cross browser/font/platform test results (it would be great to figure out a good way to automate this):

Chrome MacOS Meiryo : Server Arial
Chrome MacOS Meiryo : Server Arial
Chrome macOS Meiryo:Meiryo
Chrome macOS Meiryo:Meiryo
Chrome MacOS san-serif all
Chrome MacOS san-serif all
Chrome MacOS sans-serif: server Arial
Chrome MacOS sans-serif: server Arial
Chrome Windows 10 : sans-serif all
Chrome Windows 10 : sans-serif all
Chrome Windows 10 sans-serif : Server Arial
Chrome Windows 10 sans-serif : Server Arial
Edge Windows 10 sans-serif : Server Arial
Edge Windows 10 sans-serif : Server Arial
Edge Windows 10 sans-serif all
Edge Windows 10 sans-serif all
Firefox MacOS san-serif : Server Arial
Firefox MacOS san-serif : Server Arial
Firefox MacOS sans-serif all
Firefox MacOS sans-serif all
Firefox Windows 10 sans-serif : Server Arial Unicode
Firefox Windows 10 sans-serif : Server Arial Unicode
Firefox Windows 10 sans-serif all
Firefox Windows 10 sans-serif all

For an example of what was broken before, see this customer screenshot using Meiryo / Server Arial:

image

Launch Checklist

  • briefly describe the changes in this PR
  • include before/after visuals or gifs if this PR includes visual changes
  • write tests for all new functionality
  • document any changes to public APIs
  • post benchmark scores
  • manually test the debug page
  • tagged @mapbox/map-design-team @mapbox/static-apis if this PR includes style spec API or visual changes
  • tagged @mapbox/gl-native if this PR includes shader changes or needs a native port
  • apply changelog label ('bug', 'feature', 'docs', etc) or use the label 'skip changelog'
  • add an entry inside this element for inclusion in the mapbox-gl-js changelog:
<changelog>Switch to using alphabetic baseline for locally-rendered glyphs to avoid misalignment on fonts with large ascenders/descenders.</changelog>

@mourner @asheemmamoowala @tsuz

@ChrisLoer
Copy link
Contributor Author

I didn't re-run my manual TinySDF performance benchmarks, but I did a sanity check on a glyph-heavy Tokyo scene to rule out major perf regressions on Chrome or Firefox. There's no particular reason to expect this to be a performance impacting change (the size of the glyph and the canvas is still the same, just drawing at different positions and returning a different "top" metric.

As for changelog, I'm not sure this needs a changelog? We could create a public-facing issue about the baseline discrepancy for Meiryo, and record this as a fix for that issue. I don't think this really counts as a whole new feature, more just a tweak to a recently-released feature.

@mourner
Copy link
Member

mourner commented Feb 18, 2021

@ChrisLoer we don't have to make an issue for every changelog entry — I think it's OK to just say something like "Fix an edge case with glyph alignment when using local font rendering with certain fonts".

@ChrisLoer ChrisLoer merged commit b66fb77 into main Feb 19, 2021
@ChrisLoer ChrisLoer deleted the tinysdf_baseline branch February 19, 2021 02:53
@karimnaaji karimnaaji added this to the v2.2 milestone Mar 1, 2021
@tsuz tsuz restored the tinysdf_baseline branch May 10, 2021 23:14
@mourner mourner deleted the tinysdf_baseline branch February 16, 2022 08:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants