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.

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

[VARC] Variable Composites table #3395

Open
wants to merge 114 commits into
base: main
Choose a base branch
from
Open

[VARC] Variable Composites table #3395

wants to merge 114 commits into from

Conversation

behdad
Copy link
Member

@behdad behdad commented Dec 14, 2023

@behdad
Copy link
Member Author

behdad commented Dec 16, 2023

This is mostly steady now. I'll go ahead and write a spec for it.

What is left to do is:

  • Rip out the glyf1 VarComposite glyphs,
  • Hook VARC up in ttGlyphSet so we can draw.

@behdad
Copy link
Member Author

behdad commented Dec 16, 2023

I should also somehow figure out how to do the VARC glyph loading lazy.

@behdad
Copy link
Member Author

behdad commented Dec 16, 2023

TODO:

  • Make VARC table lazy-load glyphs.
  • Subsetter
  • Instancer
  • scaleUpem
  • tests

@behdad
Copy link
Member Author

behdad commented Dec 17, 2023

I can use some directions here. The multiVarStore and varStore share some similar code. Should I try to subclass them from a common class?

It will be more pronounced when adding subsetting to multiVarStore.

@behdad
Copy link
Member Author

behdad commented Dec 17, 2023

I can use some directions here. The multiVarStore and varStore share some similar code. Should I try to subclass them from a common class?

It will be more pronounced when adding subsetting to multiVarStore.

I managed to share some code by monkey-patching for now.

@behdad behdad marked this pull request as ready for review December 18, 2023 01:22
@behdad behdad force-pushed the varc-table branch 2 times, most recently from 87dc659 to c8a8b47 Compare December 18, 2023 01:40
@behdad
Copy link
Member Author

behdad commented Dec 18, 2023

This is ready for review. Thanks in advance @anthrotype @justvanrossum

@behdad
Copy link
Member Author

behdad commented Dec 18, 2023

This is ready for review. Thanks in advance @anthrotype @justvanrossum

It's terse but here's something of a spec for this:
https://github.com/harfbuzz/boring-expansion-spec/blob/main/varc.md

@behdad
Copy link
Member Author

behdad commented Dec 18, 2023

I probably should move some of the new code from otTables to a side module.

@behdad behdad force-pushed the varc-table branch 8 times, most recently from 8e75d94 to 2e057ba Compare December 20, 2023 18:50
@behdad
Copy link
Member Author

behdad commented Mar 16, 2024

HarfBuzz counterpart WIP: harfbuzz/harfbuzz#4578

Now can render fonts generated with this fonttools branch.

@behdad behdad force-pushed the varc-table branch 3 times, most recently from 45e579e to b7e8ac6 Compare March 20, 2024 23:03
@behdad
Copy link
Member Author

behdad commented Mar 22, 2024

This is ready. Has tests too.

@@ -22,6 +22,8 @@

DELTAS_ARE_ZERO = 0x80
DELTAS_ARE_WORDS = 0x40
DELTAS_ARE_LONGS = 0xC0
Copy link
Member

Choose a reason for hiding this comment

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

de/compiling TupleVariation with this new DELTAS_ARE_LONGS flag doesn't seem to be covered by tests

Copy link
Member Author

Choose a reason for hiding this comment

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

You caught me!

I'll see if I can make one.

Copy link
Member

Choose a reason for hiding this comment

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

You caught me!

codecov did! :)

@justvanrossum
Copy link
Collaborator

Suggest to apply this patch to make a var composite referencing its own GID work correctly with the pen protocol:

diff --git a/Lib/fontTools/ttLib/ttGlyphSet.py b/Lib/fontTools/ttLib/ttGlyphSet.py
index 8b276aa37..5d373cfb9 100644
--- a/Lib/fontTools/ttLib/ttGlyphSet.py
+++ b/Lib/fontTools/ttLib/ttGlyphSet.py
@@ -344,11 +344,17 @@ class _TTGlyphVARC(_TTGlyph):
             reset = comp.flags & VarComponentFlags.RESET_UNSPECIFIED_AXES
             with self.glyphSet.glyphSet.pushLocation(location, reset):
                 with self.glyphSet.pushLocation(location, reset):
-                    try:
-                        pen.addVarComponent(
-                            comp.glyphName, transform, self.glyphSet.rawLocation
-                        )
-                    except AttributeError:
+                    shouldDecompose = self.name == comp.glyphName
+
+                    if not shouldDecompose:
+                        try:
+                            pen.addVarComponent(
+                                comp.glyphName, transform, self.glyphSet.rawLocation
+                            )
+                        except AttributeError:
+                            shouldDecompose = True
+
+                    if shouldDecompose:
                         t = transform.toTransform()
                         compGlyphSet = (
                             self.glyphSet

Without this, it will emit a self-referencing component which (as far as I can see) can't be resolved in another way.

@behdad, I can't find a reference to the prescribed behavior in the proposal doc.

Others: what was new to me is that a VARC Composite may reference its own GID, which will then bypass VARC and go straight to glyf or CFF2. This is a handy way to mix outlines and (variable) components.

@behdad
Copy link
Member Author

behdad commented Apr 22, 2024

@justvanrossum Thanks. Can you push that change to the branch?

@behdad, I can't find a reference to the prescribed behavior in the proposal doc.

Here:

The component then is recursively loaded from the VARC table, if it is present in this table; otherwise, falls back to glyf/gvar, CFF2, or any other mechanism to get raw outlines. An exception to the recursive rule is that if a component refers to the glyphID of the current glyph, instead of recursing (which would result in infinite recursion), the glyph outline for the component is loaded directly instead, as if the glyphID had no VARC entry.

@justvanrossum
Copy link
Collaborator

Can you push that change to the branch?

Will do, thanks!

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

Successfully merging this pull request may close these issues.

None yet

4 participants