googlefonts / fontations

Reading and writing font files
Apache License 2.0
391 stars 24 forks source link

Traversing a glyf table does nothing #694

Open simoncozens opened 11 months ago

simoncozens commented 11 months ago

I have some code which serialises a font, and it returns glyf: {} because of this:

https://github.com/googlefonts/fontations/blob/d4651c5e31e81dbddac66163de66ab7fdf84488c/read-fonts/generated/generated_glyf.rs#L40-L44

simoncozens commented 11 months ago

OK, I sort of understand why this is, because glyf is random-access, and you can't read it without reference to loca, and so on. But having it return zero fields on traversal is just plain wrong. This may be an occasion where the SomeTable trait should not be auto-generated, but should instead be implemented by hand.

cmyr commented 11 months ago

Your reasoning is correct. FWIW traversal currently only exists as a mechanism to generate reasonable impls of Debug, and it definitely has some warts/missing impls. It is possible to tell codegen to use a custom impl for a specific field (with the traverse_with attribute) but this does not currently exist for top-level tables.

In any case, this is a limitation of the current way we've done the glyf table, where it's just a dumb bag of bytes. If you're set on using traversal, you could consider having a GlyfLoca type that glues the two together, and then you can impl the traverse traits based on that?

simoncozens commented 11 months ago

FWIW traversal currently only exists as a mechanism to generate reasonable impls of Debug

Well, that's the thing about a library: people see that bits of the API are available and use them in ways they "shouldn't" and expect them to work... ;-)

In any case, this is a limitation of the current way we've done the glyf table, where it's just a dumb bag of bytes.

Right. Which is a fine thing to do with a top-level table that depends entirely on another top-level table for its interpretation. OpenType isn't your fault.

But in such a case, I think you should either:

  1. Properly traverse the table somehow, or
  2. Panic or don't implement the SomeTable trait or in some other way fail to implement traversal.

Either one of those is fine. But saying "Yes, we offer traversal of the glyf table, here it is: {}" is just plain ol' mean to end-users.

cmyr commented 11 months ago

(oops, thought I'd replied, sorry!)

So you're totally right, here, and this is a situation I've been hemming & hawing about for the past year.

Basically: traversal's main justification is that it allows us to implement useful versions of Debug (and potentially also traits like PartialEq). It was also used in the otexplorer project, which was a very useful debugging tool while I was doing the initial development.

In theory I would like to step back and try to figure out a more principled approach to this, which would be more generally useful.

In practice, it's hard to justify this work right now, since we have so much more to do to just get fontc to a place where it's useful.

The problem with glyf in particular is that it... doesn't have fields, in the sense that the spec does not show it having a header, and it is essentially a bag of bytes.

I think in general it might make sense to special-case glyf and loca and just always bundle them up together, which would make life easier; we sort of do this in write-fonts, where we have a unified GlyfLocaBuilder.

so if you're motivated to continue working down this path, your best bet would probably be to write your own combined GlyfLoca table, and then implement the traversal code manually for that. Once you get into the glyph data, though, traversal is going to fall on its face, since all it sees there is a bunch of untyped bytes. :(