Skip to content
This repository has been archived by the owner on Aug 8, 2024. It is now read-only.

schemas: add glyph.fbs and plist.fbs #3

Merged
merged 7 commits into from
Dec 1, 2021
Merged

schemas: add glyph.fbs and plist.fbs #3

merged 7 commits into from
Dec 1, 2021

Conversation

anthrotype
Copy link
Member

The Glyph type mimics the properties of a defcon.Glyph or ufoLib2.Glyph (rather than literally following the UFO spec), to aid interoperability.
E.g. there's no Advance table with a width and height attributes, but the latter are directly attributes of Glyph itself. The unicodes are list of integers, not hex strings (as in the UFO XML serialization).
Plist is weird because apparently one can't have vector of unions or unions of anything other than a table (structs or literals not allowed) in languages other than c++, so one must add further levels of indirection.

The Glyph type mimics the properties of a ufoLib2.Glyph (rather than literally following the UFO spec), to aid interoperability.
schemas/ufo/glyph.fbs Outdated Show resolved Hide resolved
Copy link
Contributor

@behdad behdad left a comment

Choose a reason for hiding this comment

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

LGTM! We should add a build system (which one!) and add tests.

schemas/ufo/glyph.fbs Outdated Show resolved Hide resolved
schemas/ufo/glyph.fbs Show resolved Hide resolved
}

struct Transform {
values: [float:6];
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally a union with xScale, xyScale, yxScale, yScale, xOffset, yOffset. But union can't take struct?

I feel like Transform should also live in base.fbs. Perhaps in a toplevel base.fbs even, not in ufo/base.fbs even.

Copy link
Member Author

@anthrotype anthrotype Dec 1, 2021

Choose a reason for hiding this comment

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

why union? We can have Transform struct use names for each field instead of a fixed-size array, but in defcon/ufoLib2 APIs it is a 6-tuple, not a dictionary.

Yes I can move to base.fbs if you prefer, though it's only used in glyph.

Copy link
Contributor

Choose a reason for hiding this comment

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

why union?

Such that we can use it both as named or array. For now maybe make it named.

Yes I can move to base.fbs if you prefer, though it's only used in glyph.

Sure keep it here for now. Thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you prefer like this?

namespace FlatFont;

struct Transform {
  xScale: float;
  xyScale: float;
  yxScale: float;
  yScale: float;
  xOffset: float;
  yOffset: float;
}

Note that there's been a lot of confusions about these symbolic names for the 2x3 affine fields. In FontTools as well as UFO specification and a lot of the lib that derive from those, the names of xyScale and yxScale are swapped compared to similar structs found in FreeType and Cairo, while they actually mean the same thing, see:
https://github.com/fonttools/fonttools/blob/445108f735b22a5ca37f669808d47906d024fe24/Lib/fontTools/ttLib/tables/otData.py#L1633-L1648

Would be best to just avoid the names and use a fixed length array of 6 floats IMO.

Copy link
Contributor

Choose a reason for hiding this comment

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

Even if you use 6 floats the order needs to be specified, so you are not actually solving the problem.

I prefer the spelled out.

Also, set the default for xScale and yScale to 1.0. How about that?

Copy link
Member Author

Choose a reason for hiding this comment

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

(I mean't one cannot set defaults on structs)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah let's forget about struct and make it table, with defaults, which would make it stored more optimized.

The naming should be xyScale first and yxScale after indeed.

Copy link
Member Author

Choose a reason for hiding this comment

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

The naming should be xyScale first and yxScale after indeed.

funny you say that.. in COLRv1 Affine2x3 we ended up following the FreeType/Cairo naming and have yxScale before xyScale.
(Of course the actual ordering is the same, only the label differ, which adds to the confusion).

Copy link
Member Author

Choose a reason for hiding this comment

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

we call "yxScale" the "y-part of the x basis vector" and xyScale "the x-part of the y basis vector" (following 3blue1brown's definitions) /cc @rsheeter

Copy link
Contributor

Choose a reason for hiding this comment

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

funny you say that..

Not funny actually that groupthink prevailed...

we call "yxScale" the "y-part of the x basis vector"

That's English, which is not universal. It should be "xyScale", because math / computer code is:

x' = xxScale * x + xyScale * y
y' = yxScale * x + yyScale * y

@behdad
Copy link
Contributor

behdad commented Dec 1, 2021

LGTM recent changes. Thanks!

@anthrotype
Copy link
Member Author

anthrotype commented Dec 1, 2021

I changed Transform to a table with defaults, moved both Transform and Guideline to schemas/ufo/misc.fbs (for all residual stuff).
I also moved plist.fbs one level up to schemas/ directory (and FlatFont namespace) because it is not ufo-only (designspace also uses plist for the lib element).
Note that all schemas/ufo/*.fbs share the same "namespace FlatFont.Ufo" now. I'm not sure if we need to give each *.fbs file its unique scope (e.g. FontInfo.Ufo.Glyph, etc.) or if they can/should share the same namespace.
For the last commit were I have updated the rust generated code, I only rebuilt the fontinfo.fbs (and misc.fbs which is included by the latter). It exploded from one single .rs file to a directory tree with various submodules, one for each table I think.
Cargo build passes so it seems to be working, but please double check I did it correctly.

For some reasons, I noticed that the order in which I pass the *.fbs to the flatc --rust compiler changes the output of the generated top-level mod.rs file: i.e. I have to say flatc --rust -o ufoff/src/flatbuffers_generated schemas/ufo/{misc,fontinfo}.fbs to produce the expected result, if I change the order and pass fontinfo.fbs and then misc.fbs, the latter seems to overwrite the fontinfo related stuff in the top-level mod.rs... I have no idea why.

@behdad
Copy link
Contributor

behdad commented Dec 1, 2021

Note that all schemas/ufo/*.fbs share the same "namespace FlatFont.Ufo" now. I'm not sure if we need to give each *.fbs file its unique scope (e.g. FontInfo.Ufo.Glyph, etc.) or if they can/should share the same namespace.

No I think the namespace should follow the directory structure indeed.

@anthrotype anthrotype merged commit 6898614 into main Dec 1, 2021
@anthrotype anthrotype deleted the glyph-fbs branch December 1, 2021 18:21
@anthrotype
Copy link
Member Author

anthrotype commented Dec 1, 2021

The ufoff README instructions on how to regenerate the rust flatbuffers is outdated now that we added namespaces, it should read something like

flatc --rust -o ufoff/src/flatbuffers_generated schemas/**/*.fbs

however the order of the arguments seems to matter in an unclear way, as I noted above. Doing the glob will list files alphabetically on my shell, and misc comes after fontinfo, which wipes the fontinfo stuff from the top-level mod.rs...

I'll leave it to you to fix, enough fontbuffers for today.

@anthrotype
Copy link
Member Author

it seems like we hit this google/flatbuffers#6800 -- but I don't understand what the workaround is

@rsheeter
Copy link
Contributor

rsheeter commented Dec 2, 2021

@behdad could you elaborate on the objection to the naming y part of x basis? - it was the sanest scheme I came across. Most others seemed to simply ignore what the fields mean entirely.

@behdad
Copy link
Contributor

behdad commented Dec 2, 2021

@behdad could you elaborate on the objection to the naming y part of x basis? - it was the sanest scheme I came across. Most others seemed to simply ignore what the fields mean entirely.

As I said, it should be named in math, /not/ English, order:

x' = xxScale * x + xyScale * y
y' = yxScale * x + yyScale * y

@rsheeter
Copy link
Contributor

rsheeter commented Dec 2, 2021

But the math doesn't care what names you use?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants