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

vhea and glyf fixes #305

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

vhea and glyf fixes #305

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c4a8820
Fix `VHEA` table structure
Mar 26, 2023
7c7e6ac
Fix loading non-existent data from `glyf` table
Apr 7, 2023
6b9538a
Setting zero coordinates for empty glyphs (space)
Apr 7, 2023
30acdf8
Handling `maxp`, `post, `vhea` versions as Version16Dot16
Apr 10, 2023
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
16 changes: 16 additions & 0 deletions src/glyph/Path.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,14 @@ export default class Path {
}
}

if (this.commands.length === 0) {
// No content, put 0 instead of Infinity
cbox.minX = 0;
cbox.minY = 0;
cbox.maxX = 0;
cbox.maxY = 0;
}

this._cbox = Object.freeze(cbox);
}

Expand Down Expand Up @@ -172,6 +180,14 @@ export default class Path {
}
}

if (this.commands.length === 0) {
// No content, put 0 instead of Infinity
bbox.minX = 0;
bbox.minY = 0;
bbox.maxX = 0;
bbox.maxY = 0;
}

return this._bbox = Object.freeze(bbox);
}

Expand Down
10 changes: 9 additions & 1 deletion src/glyph/TTFGlyph.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,8 +74,16 @@ export default class TTFGlyph extends Glyph {
return this.path.cbox;
}

let glyfPos = this._font.loca.offsets[this.id];
let nextPos = this._font.loca.offsets[this.id + 1];

// No data for this glyph (space?)
if (glyfPos === nextPos) {
return super._getCBox();
}

let stream = this._font._getTableStream('glyf');
stream.pos += this._font.loca.offsets[this.id];
stream.pos += glyfPos;
let glyph = GlyfHeader.decode(stream);

let cbox = new BBox(glyph.xMin, glyph.yMin, glyph.xMax, glyph.yMax);
Expand Down
3 changes: 2 additions & 1 deletion src/tables/vhea.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@ import * as r from 'restructure';

// Vertical Header Table
export default new r.Struct({
version: r.uint16, // Version number of the Vertical Header Table
majorVersion: r.uint16, // Major version number of the Vertical Header Table
minorVersion: r.uint16, // Minor version number of the Vertical Header Table
Copy link
Contributor

@Pomax Pomax Apr 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Note that the vhea version field is a 16dot16 number, which is not the same as two 16 bit ints.

The first number is a uint16, but the second number uses a very different format, in which only values 0x0000, 0x1000, ..., 0x9000 are valid, and represent minor versions 0 through 9. When parsed as uint16, the result needs to be bitshifted right by 12 to get the actual value.

Copy link
Author

Choose a reason for hiding this comment

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

Good day! 👋 Thank you for the clarification. As I see it, the maxp table uses the same approach (16dot16) to versioning. In this library, maxp describes the version as a simple 32-bit number with no additional major/minor divisions, because version, as I understand it, is not used anywhere else.

fontkit/src/tables/maxp.js

Line 5 in a5fe0a1

version: r.int32,
Should I apply the same approach here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Correct, there are three tables (maxp, post, vhea) that use 16dot16 for historical reasons, and thankfully no future table will ever be allowed to use it again. If the maxp table uses a 32 int, that may need fixing too, as there are technically two versions for the maxp table (0.5 and 1.0).

Copy link
Contributor

@Pomax Pomax Apr 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Looking at https://github.com/foliojs/fontkit/tree/master/src/tables/post.js, it seems like that table, too, is not doing the right thing: as per the OpenType docs, "In earlier versions, these fields were documented as using a Fixed value, but had minor version numbers that did not follow the definition of the Fixed type" so if you want to make your code do "the same" as the post table, using the r.fixed32 as versioning field is probably the right way to go, unless restructure adds a 16dot16 type.

Hopefully @devongovett can have a quick look here.

Copy link
Member

Choose a reason for hiding this comment

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

oh that's fun. You might have to implement a custom type for that. Here is how Fixed is implemented. You'd need to do something similar to handle this type.

I would also not switch this to two separate fields as that might be considered a breaking change (if someone was using the version field before).

Copy link
Author

Choose a reason for hiding this comment

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

I made a small encoder/decoder that should handle the given type and applied it to all 3 tables. A small note regarding the Base class of the restructure library: it is not exported, so we had to abandon inheritance and add the fromBuffer and toBuffer methods directly to Version16Dot16. The test results did not change in any way (at least no degradation was noticed).

ascent: r.int16, // The vertical typographic ascender for this font
descent: r.int16, // The vertical typographic descender for this font
lineGap: r.int16, // The vertical typographic line gap for this font
Expand Down