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

Use hardwired baseline to match MapLibre behavior #20

Merged
merged 1 commit into from
Feb 21, 2023

Conversation

ChrisLoer
Copy link
Collaborator

node-fontnik always encoded glyph "top" metrics relative to the typeface's ascender, but it didn't actually tell the client what the ascender/descender was, so there was no way for Mapbox GL (and by extension MapLibre) to know what baseline to use. Mapbox effectively calibrated the baseline for its shaping code on DIN Pro, which has an ascender of 25 at 24pts (the font is proprietary, but you can see stats here).

You can see historical discussion at mapbox/mapbox-gl-js#191

There is a (open-source) PR to node-fontnik to expose the data at mapbox/node-fontnik#160, but it hasn't merged.

Post-fork, Mapbox GL implemented ascender/descender awareness -- I won't link to the PR here to avoid contamination concerns. I was actually a reviewer on the PR but have forgotten most of the details and don't think I should look at the code now. However, I think (??) one of the concerns was that in order to correctly handle verticalizing glyphs (for CJK labels that support vertical alignment) you need to know the actual baseline to know how to do your rotation.

What I haven't figured out is why Mapbox (including me while I was there) never just modified node-fontnik to do the simplest thing imaginable -- just use the "top" metric directly from the font without an ascender modification! My best guess is that we talked a lot about doing it the "right" way (including ascender metadata), but never looked for a fast fix because most of the fonts we used were close enough to the DIN Pro ascender that they looked OK? But honestly I think I may be missing/forgetting something.

At any rate, this PR does that simple thing -- it just returns the glyph's "top" metric, with an offset of 25 to account for the implicit DIN-Pro derived ascender baked into the renderer.

Here's a map before the change -- you can see that Atlas Grotesk on its own isn't properly centered in the collision box, and you can see when it's put next to Noto Sans Arabic, the baselines are way off:

Screenshot 2023-02-20 at 3 07 14 PM

And here's the map with the fonts re-generated using this change:

Screenshot 2023-02-20 at 3 07 47 PM

This change will affect all fonts generated with font-maker, so it would be good to test with a wide range to see if we're getting the better alignment we expect.

@bdon

@bdon
Copy link
Collaborator

bdon commented Feb 21, 2023

A few other relevant threads:
mapbox/tiny-sdf#44
maplibre/maplibre-gl-js#1005

The constant in https://github.com/maplibre/maplibre-gl-js/pull/1005/files seems to be 27 instead of 25, is this just an invisible difference?

@bdon bdon merged commit 34eb07e into maplibre:main Feb 21, 2023
@ChrisLoer
Copy link
Collaborator Author

The constant in https://github.com/maplibre/maplibre-gl-js/pull/1005/files seems to be 27 instead of 25, is this just an invisible difference?

In that context, TinySDF is trying to make glyphs that will have an implicit baseline that's as close as possible to server-generated glyphs, but it doesn't know what the implicit baseline of the server generated glyphs will be, so it guesses something in between DIN Pro (the main Mapbox streets latin font) and Arial Unicode (the Mapbox streets fallback font). So if you have text with both of those fonts mixed with TinySDF text, you'll only be off by ~2px in either direction.

In the font-maker context, I've been focused on just how the glyph lines up relative to MapLibre's idea of where it's being placed -- that is, if it's place to the right of an anchor, does it look centered vertically. I think font-maker derived font stacks are unlikely to be combined with fonts coming from say node-fontnik, but they are probably likely to be combined with TinySDF glyphs, in which case we'd be 2px off (probably not too bad?).

@wipfli
Copy link

wipfli commented May 19, 2023

Thanks for working on this @ChrisLoer. I re-generated my fonts with the lastest online version of font-maker, and changed MapLibre GL JS to use the TinySDF code path for the letter a while using the normal pre-generated fonts for the other latin letters.

The result is here:

image

While it is possible to read the labels, a 2 pixel baseline error really make the map look broken to me personally.

If I tuned the hard-coded offset in MapLibre GL JS, glyph_manager.ts to

- const topAdjustment = 27;
+ const topAdjustment = 25;

then it looks a bit better:

image

@wipfli
Copy link

wipfli commented May 19, 2023

Is there a way to settle on one convention and then stick to it for all pre-generated and TinySDF generated glyphs?

@ChrisLoer
Copy link
Collaborator Author

Is there a way to settle on one convention and then stick to it for all pre-generated and TinySDF generated glyphs?

Yeah, I think so. The issue is just that you don't know what font servers MapLibre is going to be talking to, so if you change its baseline assumption you'll be changing lots of existing maps' behaviors.

1 similar comment
@ChrisLoer
Copy link
Collaborator Author

Is there a way to settle on one convention and then stick to it for all pre-generated and TinySDF generated glyphs?

Yeah, I think so. The issue is just that you don't know what font servers MapLibre is going to be talking to, so if you change its baseline assumption you'll be changing lots of existing maps' behaviors.

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 this pull request may close these issues.

3 participants