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

VarComposites #2958

Merged
merged 104 commits into from Feb 7, 2023
Merged

VarComposites #2958

merged 104 commits into from Feb 7, 2023

Conversation

behdad
Copy link
Member

@behdad behdad commented Jan 17, 2023

I've started implementing these. decompile() seems to work so far.

@behdad behdad marked this pull request as draft January 17, 2023 17:09
@justvanrossum
Copy link
Collaborator

You should probably run black.

@behdad
Copy link
Member Author

behdad commented Jan 17, 2023

Thanks. Will do every few commits.

@behdad
Copy link
Member Author

behdad commented Jan 17, 2023

@justvanrossum I have a question for you:

Currently I'm setting all transformation components on VarComponent, and flags controls which ones are compiled. This is a bit uneasy to work with since if client modifies the transformation components they also need to modify the flags. We cannot just compile transformation components smartly since they should all match across masters when compiling a variable font...

An alternative would be to NOT set attribute for those components that should not be compiled into the font. That would be inconvenient to use when working with the component though.

Can you advise which one you prefer?

@justvanrossum
Copy link
Collaborator

Currently I'm setting all transformation components on VarComponent,

Sounds good.

and flags controls which ones are compiled

Ideally, the flags would be computed autimatically based on the transformation data.

We cannot just compile transformation components smartly since they should all match across masters when compiling a variable font...

Couldn't a VF compiler take that into account and adjust accordingly?

@behdad
Copy link
Member Author

behdad commented Jan 17, 2023

We cannot just compile transformation components smartly since they should all match across masters when compiling a variable font...

Couldn't a VF compiler take that into account and adjust accordingly?

It can. A bit more work but doable indeed. Which then means, the glyf compiler should calculate flags based on presence of attributes only, and cannot drop components that have default value.

@behdad
Copy link
Member Author

behdad commented Jan 17, 2023

@justvanrossum This branch now has compile/decompile/toXML/fromXML.

It's a bit crude as axisIndices are retained as integers not tags. To convert to tags I need to pass fvar down to glyf compile/decompile, which is a bit inconvenient.

@justvanrossum
Copy link
Collaborator

the glyf compiler should calculate flags based on presence of attributes only, and cannot drop components that have default value

I don't follow. Ideally (I think), all fields would be present as attributes, but only those that don't have the default value get compiled into the binary.

@behdad
Copy link
Member Author

behdad commented Jan 17, 2023

Ideally (I think), all fields would be present as attributes, but only those that don't have the default value get compiled into the binary.

The problem is that the same flag controls whether variations for the transform components is encoded. So there are cases that eg. a translateX of 0 should be encoded, because it has variations. So the glyf-table compiler cannot just discard translateX of 0 automatically.

@justvanrossum
Copy link
Collaborator

Ah yes, now I get it. The variations in gvar have to match up.

@behdad
Copy link
Member Author

behdad commented Jan 17, 2023

Currently Glyph.decompile only takes the glyf table. Not sure how to pass fvar to it without changing API drastically.

@behdad
Copy link
Member Author

behdad commented Jan 17, 2023

Ah yes, now I get it. The variations in gvar have to match up.

Oh. If .flags is missing we can calculate it!

@justvanrossum
Copy link
Collaborator

Currently Glyph.decompile only takes the glyf table. Not sure how to pass fvar to it without changing API drastically.

Maybe table__g_l_y_f.compile() and table__g_l_y_f.decompile() can assign a reference to the ttFont for later use. It's a bit ugly, but then again: circular refs stopped being a big deal a long time ago, and I'm sure the API would have looked different if it were designed today... Or use a weakref.

@behdad
Copy link
Member Author

behdad commented Jan 17, 2023

circular refs

I'll try that. The cff table does it just fine.

@behdad
Copy link
Member Author

behdad commented Jan 17, 2023

Currently Glyph.decompile only takes the glyf table. Not sure how to pass fvar to it without changing API drastically.

Maybe table__g_l_y_f.compile() and table__g_l_y_f.decompile() can assign a reference to the ttFont for later use. It's a bit ugly, but then again: circular refs stopped being a big deal a long time ago, and I'm sure the API would have looked different if it were designed today... Or use a weakref.

Or I can just save the axisTags. That's all I need.

@justvanrossum
Copy link
Collaborator

Or I can just save the axisTags. That's all I need.

Ah yes. As long as they're refreshed on each compile/decompile call, that should be fine.

@behdad
Copy link
Member Author

behdad commented Jan 17, 2023

All done.

@behdad

This comment was marked as outdated.

@behdad
Copy link
Member Author

behdad commented Jan 17, 2023

I still have to handle the following flag:

If ScaleY is missing: take value from ScaleX (to be discussed here: BlackFoundryCom/variable-components-spec#2)

It gets messy.

@justvanrossum
Copy link
Collaborator

It gets messy.

I bet :( Feel free to simplify this in the spec — we can be pragmatic about this.

@behdad

This comment was marked as outdated.

@behdad

This comment was marked as outdated.

@behdad behdad marked this pull request as ready for review January 17, 2023 22:35
@behdad
Copy link
Member Author

behdad commented Jan 17, 2023

I still have to handle the following flag:

If ScaleY is missing: take value from ScaleX (to be discussed here: BlackFoundryCom/variable-components-spec#2)

It gets messy.

I made the compiler handle this flag automatically.

@behdad behdad force-pushed the varc branch 2 times, most recently from e73d134 to 19a6552 Compare January 18, 2023 04:28
@behdad
Copy link
Member Author

behdad commented Jan 18, 2023

This is now works in:

  • ttx
  • WOFF
  • WOFF2
  • ttGlyphSet
  • scaleUpem
  • subset
  • merge
  • varLib.instancer when not instancing VarComposite axes with variations.
  • varLib.interpolatable

@behdad

This comment was marked as outdated.

behdad added a commit that referenced this pull request Feb 4, 2023
And add tests.

See thread starting at:
#2958 (comment)
@behdad
Copy link
Member Author

behdad commented Feb 4, 2023

Added tests and fixed.

Err. Actually fixing.

behdad added a commit that referenced this pull request Feb 4, 2023
And add tests.

See thread starting at:
#2958 (comment)
And add tests.

See thread starting at:
#2958 (comment)
@anthrotype
Copy link
Member

I see you added some generic xml-roundtripping tests for varComponents in the unit tests for scaleUpem module.. that's not ideal, althoughj some tests is always better than no tests. The glyf table module has unit test module of its own, in Tests/ttLib/tables/glyf_test.py, that's where I'd expect to find things like those.

@behdad
Copy link
Member Author

behdad commented Feb 6, 2023

I see you added some generic xml-roundtripping tests for varComponents in the unit tests for scaleUpem module.. that's not ideal, althoughj some tests is always better than no tests. The glyf table module has unit test module of its own, in Tests/ttLib/tables/glyf_test.py, that's where I'd expect to find things like those.

Ah thanks. I was wrongly looking for ttx tests and not finding them. Will move there.

@behdad
Copy link
Member Author

behdad commented Feb 6, 2023

This seems to be in good shape again.

@justvanrossum
Copy link
Collaborator

justvanrossum commented Feb 7, 2023

This seems to be in good shape again.

Looks good to me i general, and the pen.addVarComponent() api is now usable for glyph extraction uses cases.

I do worry that we don't have a solution for glyph.recalcBounds() for variable components. The factoring between _g_l_y_f.py and ttGlyphSet.py makes it seemingly difficult to implement. At the same time, ttGlyphSet._setCoordinates() does too much work in some cases, where we don't need the lsb - xMin offset (because is is supposed to be zero anyway). I have a vague idea of adding a new class "DrawingContext" to _g_l_y_f.py, that would contain the common code for recalcBounds and ttGlyphSet. Needs sketching out.

@behdad
Copy link
Member Author

behdad commented Feb 7, 2023

I do worry that we don't have a solution for glyph.recalcBounds() for variable components. The factoring between _g_l_y_f.py and ttGlyphSet.py makes it seemingly difficult to implement. At the same time, ttGlyphSet._setCoordinates() does too much work in some cases, where we don't need the lsb - xMin offset (because is is supposed to be zero anyway). I have a vague idea of adding a new class "DrawingContext" to _g_l_y_f.py, that would contain the common code for recalcBounds and ttGlyphSet. Needs sketching out.

We can do that in a followup PR. Sad thing is we don't have the font around; I suppose we can add a glyf.font link.

@behdad
Copy link
Member Author

behdad commented Feb 7, 2023

@anthrotype @justvanrossum Can we move towards merging this?

Copy link
Member

@anthrotype anthrotype left a comment

Choose a reason for hiding this comment

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

LGTM

a follow up PR can deal with recalcBounds for varComposites

@anthrotype
Copy link
Member

We can do that in a followup PR.

let's also file an issue tracking implementation of recalcBouds for varComposites

@behdad behdad merged commit 58bc16e into main Feb 7, 2023
@behdad behdad deleted the varc branch February 7, 2023 17:28
@behdad behdad mentioned this pull request Feb 7, 2023
@behdad
Copy link
Member Author

behdad commented Feb 7, 2023

We can do that in a followup PR.

let's also file an issue tracking implementation of recalcBouds for varComposites

#2986

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

3 participants