-
Notifications
You must be signed in to change notification settings - Fork 11
Description
I believe there might be two bugs in the kerning approach.
In lines https://github.com/wwwtyro/candygraph/blob/master/src/assets/font.ts#L78-L82 the kern table is constructed as an Int8 array of length numGlyphs ** 2. While the variable name maxid suggests that it represents the maximum glyph ID it's really just the length. For Arial with the ASCII char set, the difference is that glyphs.length is 95 while the maximum glyph.id is 126 (corresponding to ~).
I believe this leads to multiple issues. For one, this.kernTable[kern.first * this.maxid + kern.second] = scale * kern.amount; can be out of bounds. E.g., if there were a kerning between 124 and 126 you'd be accessing kernTable[11906] but the table's length is only 9025. In practice this might not happen as kerning is usually limited to alphabetical chars I believe. Second, we can potentially access the wrong kerning. Say there's a kerning between [33, 31] and [32, 126]. Both glyph combinations lead to the same kerning table index: 3166. Again this is likely not going to happen in practice but it's still possible. Finally, a bunch of entries of the kernTable are never used because the minimum glyph.id is often 32 (the white space). So anything 32 * 95 === 3040 entries are unused.
I think both issues here can be addressed by using the minimum glyph ID, the (real) maximum glyph ID, and the ID extent.
The other issue I noticed is that kernTable is an instance of Int8Array. Since you multiple the kern amount by scale, the kerning amount is relative and most likely going to be a float number below 1. So effectively kerning is disabled because numbers in ]0, 1[ are stored as 0. Would there be any harm in using Float32Array?
this.minId = Number.POSITIVE_INFINITY;
this.maxId = Number.NEGATIVE_INFINITY;
for (const { id } of this.glyphs) {
this.minId = Math.min(minId, id);
this.maxId = Math.max(maxId, id);
}
this.idExtent = this.maxId - this.minId;
this.kernTable = new Float32Array(this.idExtent ** 2);
for (const kern of json.kernings) {
const firstIdx = kern.first - this.minId;
const secondIdx = kern.second - this.minId;
this.kernTable[firstIdx * this.idExtent + secondIdx] = scale * kern.amount;
}
/** @internal */
public kern(first: number, second: number) {
const firstIdx = first - this.minId;
const secondIdx = second - this.minId;
return this.kernTable[firstIdx * this.idExtent + secondIdx];
}Again, I don't think the issues outlined here are severe by any means but since I came across it I thought I might file a issue to get your thoughts :)