-
Notifications
You must be signed in to change notification settings - Fork 64
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
Re-render shields after fonts load #991
Conversation
I could not avoid some kind of font pre-load in order to make this all work. At least this version seems slightly less kludgy (or at least centralizes the kludge into one spot in the code...) |
Oh, this is less than ideal:
|
Oh ugh is that a maplibre error? |
Yes, it should be simple enough to do It might have a slight performance hit as it needs to re-layout the sprites, but will only happen once, max. I suppose it could be possible to check if the dimensions are unchanged but that's probably overkill. I note that we are careful to only replace images that we know we added, to avoid unintentionally clobbering anything. |
Try adding this in setShields(): const fontSpec = '1em ' + shieldSpec.options.shieldFont;
if (!document.fonts.check(fontSpec)) {
this._fontsLoaded = false;
document.fonts.load(fontSpec).then(() => this.onFontsLoaded());
} else {
this._fontsLoaded = true;
} I think that's the font loading entrypoint we need. This will avoid depending on document.fonts.ready as a sort of global state, making this less brittle when embedded. And avoid depending on all of the document's fonts being ready, in case the document wants other fonts unrelated to shield sprites. Broad overview of what this means: The CSS stylesheet installs the fonts into Alternative to CSS, the Although it also supports adding the fonts with an ArrayBuffer, which sort of allows more precise control over loading. But I think loading the fonts asynchronously and recovering the fallback sprites is acceptable (and allows the font to safely fail for any reason). The thing about triggering font loading with a hidden text is that (I don't think) there's no way to ask specifically, which web fonts did we use for this DOM element. So you can only use Another way could be this: Promise.all(Array.from(document.fonts, f => f.load())).then(() => … fonts loaded); But I think asking specifically for the value of It looks like this: > shieldFont=`"Noto Sans Condensed", "Noto Sans Armenian Condensed", sans-serif-condensed, "Arial Narrow", sans-serif`
> await document.fonts.load('1em ' + shieldFont)
(2) [FontFace, FontFace]
0: FontFace {family: 'Noto Sans Condensed', status: "loaded"}
1: FontFace {family: 'Noto Sans Armenian Condensed', status: "loaded"} So it gives us all the fonts we asked for. |
This works delightfully. I had to do some gymnastics because setShields can be invoked asynchronously (from URLShieldLoader), so it's not guaranteed that the Map is available before the font name. |
Hmm I wonder if this works okay with the taginfo shield generator. |
Does taginfo not run on pull requests? Seems like it does, shouldn't this PR's check be failing? if (!document.fonts.check(fontSpec)) {
^
ReferenceError: document is not defined Ah, I see. I suppose this needs to be encapsulated into headless_graphics and document_graphics. |
class DOMGraphicsFactory implements GraphicsFactory {
static loadFonts(shieldFont: string) {
…
}
} (or just a free function in that module) to perform the equivalent, synchronous check or Promise of font loading. |
I don't think that should matter, modulo having to supply a certain argument to a certain function. If the shield renderer has a font name and a document, it can proceed to ask the document to load that font. When the font is ready, it needs to ask the map to replace images and redraw itself, but that won't happen if there isn't a map yet, because the map is the driver of asking for sprites in the first place. |
Taginfo graphics appear to be working normally, e.g.: |
To make a long story short, taginfo generation doesn't have a problem because the taginfo script only draws shapes without text. Also, the test in |
this.map.addImage(spriteID, image); | ||
} else { | ||
console.log(`add ${spriteID}`); | ||
this.map.addImage(spriteID, image, { pixelRatio: pixelRatio }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The last parameter to addImage()
accepts some other options, of which sdf
may be the most relevant to a client of this library. Should we plumb through an entire options object?
This PR updates the shield renderer to re-generate all shields that were previous rendered when fonts loads. This ensures that a font update is replicated into prior generated shields but doesn't block the loading of the map.
I tested this by inserting delays after the font load and it seems to update the shields without glitching.
Follow-on from #987