Add FlattenComponentsFilter to flatten nested components #214
Conversation
| unnested = True | ||
| index = glyph.componentIndex(comp) | ||
| glyph.removeComponent(comp) | ||
| for unnested in reversed(unnested_tuples): |
There was a problem hiding this comment.
this unnested for loop variable is shadowing/redefining the other local variable called unnested a few lines before. Was that intentional?
There was a problem hiding this comment.
Oh, no that wasn’t intentional.
| index = glyph.componentIndex(comp) | ||
| glyph.removeComponent(comp) | ||
| for unnested in reversed(unnested_tuples): | ||
| base_comp = glyph.instantiateComponent() |
There was a problem hiding this comment.
maybe this instantiateComponent method is too specific to defcon's API. I see for example fontParts does not have this.
Does it not work if you insert a generic defcon.Component?
Or maybe wouldn't it be better (more portable) if you used the pen API to draw the components onto the glyph object? That is, you get the pen (or point pen) associated with the glyph and call the addComponent method.
I guess in this case the new "unnested" components will all be drawn at the end of the glyphs.components list, whereas here you're using insertComponent (instead of appendComponent) to try to keep the order?
Not sure it's even worth to keep the order.
There was a problem hiding this comment.
I'd still prefer to use a generic pen.addComponent instead of relying on defcon-specific instantiateComponent and insertComponent methods
|
I'm thinking whether "flattenComponents" could convey the meaning better than "unnestComponents". Maybe a native english speaker could comment. |
|
a few questions:
|
moyogo
force-pushed
the
unnested-components
branch
from
f3afd4f
to
c55657e
Compare
|
I don’t mind using
I don’t think there any custom parameter to control this. It’s the default in 2.4.4.
Some printers. |
|
yeah, I think I like You know this filter works already if you specify it in the font's lib.plist under |
moyogo
force-pushed
the
unnested-components
branch
from
c55657e
to
1b3361e
Compare
|
tests would be nice too |
|
The flattening was applied mostly to be able to open the .ttf in FLS. I removed it as is was causing some problems. I didn’t hear about any problems. |
moyogo
force-pushed
the
unnested-components
branch
from
1b3361e
to
8cfaeba
Compare
|
OK, I renamed things to flattenComponents, FlattenComponentFilter, etc., and added tests. |
| glyph.removeComponent(comp) | ||
| for flattened_tuple in flattened_tuples: | ||
| pen = glyph.getPen() | ||
| # baseGlyph, transformation = flattened_tuple |
| flattened = True | ||
| glyph.removeComponent(comp) | ||
| for flattened_tuple in flattened_tuples: | ||
| pen = glyph.getPen() |
There was a problem hiding this comment.
you can get the pen outside the for loops, the glyph doesn't change so neither does the pen
| [('a', (1, 0, 0, 1, 0, 0))] | ||
| ) | ||
| assert ( | ||
| [(a.name, a.x, a.y) for a in font['c'].anchors] == |
There was a problem hiding this comment.
what's the point of asserting the anchors here? The filter is not supposed to modify these, is it?
|
@schriftgestalt Thanks! I guess we don’t want this as default then. |
|
@anthrotype Since Glyphs 2.5+ won’t be doing this anymore, should I just remove the argument from the functions and just let users use |
hm.. yeah, that was the idea of allowing preprocessor to load custom filters from the font's lib.plist. |
moyogo
force-pushed
the
unnested-components
branch
from
f51c1d1
to
f0da1ba
Compare
moyogo
changed the title
This allows to follow Glyphs.app behaviour, but not by default.
One may want direct components instead of nested components because some TrueType implementations don’t handle nested components properly.