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.

Sign up for GitHub

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

Don't cache server requested glyphs in the local glyph ranges #8657

Merged
merged 5 commits into from
Aug 20, 2019
Merged

Don't cache server requested glyphs in the local glyph ranges #8657

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5302fe9
Use correct value for localIdeographFontFamily in Map#setStyle
Aug 19, 2019
99aa019
Ignore local glyph ranges when caching server requested glyphs
Aug 19, 2019
477e509
Cache locally generated glyphs in GlyphManager
Aug 19, 2019
12b2db8
fix flow
Aug 19, 2019
abc1b6c
fix lint
Aug 19, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 16 additions & 7 deletions src/render/glyph_manager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ class GlyphManager {

glyph = this._tinySDF(entry, stack, id);
if (glyph) {
entry.glyphs[id] = glyph;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this line change? Were we previously not caching generated glyphs?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

callback(null, {stack, id, glyph});
return;
}
Expand All @@ -81,7 +82,9 @@ class GlyphManager {
(err, response: ?{[number]: StyleGlyph | null}) => {
if (response) {
for (const id in response) {
entry.glyphs[+id] = response[+id];
if (!this._doesCharSupportLocalGlyph(+id)) {
entry.glyphs[+id] = response[+id];
}
}
}
for (const cb of requests) {
Expand Down Expand Up @@ -118,17 +121,23 @@ class GlyphManager {
});
}

_doesCharSupportLocalGlyph(id: number): boolean {
/* eslint-disable new-cap */
return !!this.localIdeographFontFamily &&
(isChar['CJK Unified Ideographs'](id) ||
isChar['Hangul Syllables'](id) ||
isChar['Hiragana'](id) ||
isChar['Katakana'](id));
/* eslint-enable new-cap */
}

_tinySDF(entry: Entry, stack: string, id: number): ?StyleGlyph {
const family = this.localIdeographFontFamily;
if (!family) {
return;
}
/* eslint-disable new-cap */
if (!isChar['CJK Unified Ideographs'](id) &&
!isChar['Hangul Syllables'](id) &&
!isChar['Hiragana'](id) &&
!isChar['Katakana'](id)
) { /* eslint-enable new-cap */

if (!this._doesCharSupportLocalGlyph(id)) {
return;
}

Expand Down
2 changes: 1 addition & 1 deletion src/ui/map.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -965,7 +965,7 @@ class Map extends Camera {
* @see [Change a map's style](https://www.mapbox.com/mapbox-gl-js/example/setstyle/)
*/
setStyle(style: StyleSpecification | string | null, options?: {diff?: boolean} & StyleOptions) {
options = extend({}, { localIdeographFontFamily: defaultOptions.localIdeographFontFamily}, options);
options = extend({}, { localIdeographFontFamily: this._localIdeographFontFamily}, options);

if ((options.diff !== false && options.localIdeographFontFamily === this._localIdeographFontFamily) && this.style && style) {
this._diffStyle(style, options);
Expand Down
60 changes: 60 additions & 0 deletions test/unit/render/glyph_manager.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,41 @@ test('GlyphManager requests remote CJK PBF', (t) => {
});
});

test('GlyphManager does not cache CJK chars that should be rendered locally', (t) => {
t.stub(GlyphManager, 'loadGlyphRange').callsFake((stack, range, urlTemplate, transform, callback) => {
const overlappingGlyphs = {};
const start = range * 256;
const end = start + 256;
for (let i = start, j = 0; i < end; i++, j++) {
overlappingGlyphs[i] = glyphs[j];
}
setImmediate(() => callback(null, overlappingGlyphs));
});
t.stub(GlyphManager, 'TinySDF').value(class {
// Return empty 30x30 bitmap (24 fontsize + 3 * 2 buffer)
draw() {
return new Uint8ClampedArray(900);
}
});
const manager = new GlyphManager((url) => ({url}), 'sans-serif');
manager.setURL('https://localhost/fonts/v1/{fontstack}/{range}.pbf');

//Request char that overlaps Katakana range
manager.getGlyphs({'Arial Unicode MS': [0x3005]}, (err, glyphs) => {
t.ifError(err);
t.notEqual(glyphs['Arial Unicode MS'][0x3005], null);
//Request char from Katakana range (te)
manager.getGlyphs({'Arial Unicode MS': [0x30C6]}, (err, glyphs) => {
t.ifError(err);
const glyph = glyphs['Arial Unicode MS'][0x30c6];
//Ensure that te is locally generated.
t.equal(glyph.bitmap.height, 30);
t.equal(glyph.bitmap.width, 30);
t.end();
});
});
});

test('GlyphManager generates CJK PBF locally', (t) => {
t.stub(GlyphManager, 'TinySDF').value(class {
// Return empty 30x30 bitmap (24 fontsize + 3 * 2 buffer)
Expand Down Expand Up @@ -98,3 +133,28 @@ test('GlyphManager generates Hiragana PBF locally', (t) => {
t.end();
});
});

test('GlyphManager caches locally generated glyphs', (t) => {
let drawCallCount = 0;
t.stub(GlyphManager, 'TinySDF').value(class {
// Return empty 30x30 bitmap (24 fontsize + 3 * 2 buffer)
draw() {
drawCallCount++;
return new Uint8ClampedArray(900);
}
});

const manager = new GlyphManager((url) => ({url}), 'sans-serif');
manager.setURL('https://localhost/fonts/v1/{fontstack}/{range}.pbf');

// Katakana letter te
manager.getGlyphs({'Arial Unicode MS': [0x30c6]}, (err, glyphs) => {
t.ifError(err);
t.equal(glyphs['Arial Unicode MS'][0x30c6].metrics.advance, 24);
manager.getGlyphs({'Arial Unicode MS': [0x30c6]}, () => {
t.equal(drawCallCount, 1);
t.end();
});
});
});