Don't cache server requested glyphs in the local glyph ranges #8657
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
asheemmamoowala
force-pushed
the
fix-local-glyphs
branch
from
cd7f5bd to
abc1b6c
Compare
ansis
reviewed
Aug 19, 2019
| @@ -64,6 +64,7 @@ class GlyphManager { | |||
|
|
|||
| glyph = this._tinySDF(entry, stack, id); | |||
| if (glyph) { | |||
| entry.glyphs[id] = glyph; | |||
There was a problem hiding this comment.
What does this line change? Were we previously not caching generated glyphs?
There was a problem hiding this comment.
Were we previously not caching generated glyphs?
That's right. Locally generated glyphs were not being cached before this PR in gl-js. Native was already doing it correctly. I considered creating a separate stack for local fonts - so that each local font would only have one cache entry. The current approach can lead to one entry per font stack (in the style definition), but is already an improvement over re-generating each local glyph per tile. Those changes could probably be addressed in a separate PR.
ansis
approved these changes
Aug 19, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #8653.
Font glyphs from the server are requested in packed in ranges of 256 characters. In some rare cases a single range may contain characters that can be locally generated (using tinySDF) and that need to be fetched from the server.
The issue described in #8653 stems from a character from such an overlapping range causing the remote glyphs being cached for even those characters that should be locally generated. This would be a non-issue if both local generation and the server request were resolving to the same font.
The fix is to check each character in the fetched range and skip it if it is should be generated locally. An additional improvement in this PR is to cache the locally generated glyphs so that subsequent tiles don't need to re-generate them.
I also found a bug in
Map#setStylewhile debugging this issue that uses the wrongdefaultfont family for local glyph generation. It should use the value resolved in the map constructor.Launch Checklist
cc @chloekraw @tmpsantos