Fall back to local glyph rendering if glyph PBF is unavailable #4564
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4564 +/- ##
==========================================
+ Coverage 87.74% 87.97% +0.22%
==========================================
Files 247 247
Lines 33494 33510 +16
Branches 2414 2375 -39
==========================================
+ Hits 29390 29481 +91
+ Misses 3118 3058 -60
+ Partials 986 971 -15 ☔ View full report in Codecov by Sentry. |
1ec5
force-pushed
the
glyph-missing-warn-4563
branch
from
96f830f to
8d37a1a
Compare
1ec5
force-pushed
the
glyph-missing-warn-4563
branch
from
8d37a1a to
a56d0f8
Compare
| @@ -108,7 +109,22 @@ export class GlyphManager { | |||
| entry.requests[range] = promise; | |||
| } | |||
|
|
|||
| const response = await entry.requests[range]; | |||
| let response; | |||
There was a problem hiding this comment.
Can we move this block to a different method maybe? This method is already too long I think
There was a problem hiding this comment.
Also, looking at the code above this part, can we now handle the case where url is not defined? I know it's not optimal, but there are situations where you just want the text to be render without finding a glyph server. You'd need to define localIdeographFontFamily anyway, which doesn't have a default, so you'd need to know what you are doing...
There was a problem hiding this comment.
Ultimately, I would love to make glyph PBFs completely optional. However, the style specification currently states that glyphs is required as long as any layer has a text-field property. Would relaxing that requirement constitute a breaking change in either the style spec or GL JS?
There was a problem hiding this comment.
I don't think it will be a breaking change as changing a required field to optional doesn't sound like a breaking change (unless you have invalid styles, which I'm not sure are interesting).
Relaxing it would make a far better developer experience, IMHO, as I find these glyphs requirement non-friendly...
There was a problem hiding this comment.
But this should be brought in the style spec repo first...
|
Looks good, thanks! |
| @@ -129,13 +145,13 @@ export class GlyphManager { | |||
| /\p{Ideo}|\p{sc=Hang}|\p{sc=Hira}|\p{sc=Kana}/u.test(String.fromCodePoint(id)); | |||
| } | |||
|
|
|||
| _tinySDF(entry: Entry, stack: string, id: number): StyleGlyph { | |||
| _tinySDF(entry: Entry, stack: string, id: number, force: boolean): StyleGlyph { | |||
| const fontFamily = this.localIdeographFontFamily; | |||
There was a problem hiding this comment.
I’m wary of choosing a font for non-ideographic text based on an option called localIdeographFontFamily. The choice should be up to the style author, in the stylesheet, and it should vary from layer to layer, just as in CSS. The iOS implementation has long attempted to match the text-font property to a local or system font. I think GL JS should attempt some matching along these lines.
Unfortunately, many non-Mapbox-hosted fontstacks are named according to the styles for which they were created, obscuring the PostScript names of the fonts from which they were derived. For example, OpenHistoricalMap has “OpenHistorical Bold”, and OSM Americana has “Americana-Bold”. These fontstacks are a pastiche of various fonts from different families. The style authors can’t simply append Open Sans, Unifont, or Noto Sans to the array, because text-font isn’t a fallback list. It simply joins the array into a comma-separated list and includes it in the request URL, so the request would fail and we’d be right back to where we started.
In order to typeset these layers in the designer’s choice of fonts, we could interpret text-font as a cascading fallback list of local font names or keywords (similar to CSS) if the glyph PBF request fails. We could short-circuit that request if glyphs is absent. Or we could introduce a new layout property, such as text-font-family or text-font-names, to represent a cascading fallback list. In the latter case, text-font would remain for backwards compatibility or where a fontstack is preferred for some reason.
There was a problem hiding this comment.
I think fallback is better than introducing a new field which would create confusion.
If the style and the server are not aligned, presenting something is better than not presenting anything, IMHO.
|
Ping. Any plans for merging this? |
|
I think @1ec5 mentioned there is work that he is planning to do before this can be merged as far as I remember... |
|
Yes, the to-do list looks like:
These changes are straightforward but may require a proposal to change the style spec. I haven’t gotten around to that part yet. |
|
@1ec5 any updates on this? |
If a glyph PBF is unavailable, try to fall back to local glyph rendering, and keep rendering the map without crashing.
Fixes #3302 and fixes #4563.
Launch Checklist
CHANGELOG.mdunder the## mainsection.