-
Notifications
You must be signed in to change notification settings - Fork 70
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
set-overlap-bits added #918
base: main
Are you sure you want to change the base?
Conversation
# rm overlaps | ||
skia_path.simplify() | ||
simplified_area = skia_path.area | ||
if not math.isclose(area, simplified_area, abs_tol=0.1): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's unusual to set abs_tol so much larger than rel_tol (default rel_tol is 1e-09), it'd take precedence most of the time, whereas in fact it's supposed to help with comparisons close 0 (where rel_tol tends to be too small), see https://docs.python.org/3/library/math.html#math.isclose
If you only care about comparing against an absolute value (and not do any relative comparisons) just do abs(a-b) <= abs_tol and you don't need to bother using isclose.
But actually, perhaps it would make more sense to use a tolernce that is relative to the font's unitsPerEm?
I don't know, something like font.info.unitsPerEm / 10_000
(you'd still get 0.1 for fonts with 1000 UPEM, but if would increase/decrease accordingly)
contour.draw(pen) | ||
area = skia_path.area | ||
# rm overlaps | ||
skia_path.simplify() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this can fail sometimes (pathops has some tricky unfixable bugs), I think it'd be better to default to setting the flag for all glyphs except those whose area does not change after simplifying
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
which means you'll have to guard against pathops errors here and pass (with a warning maybe) in case it occurs
elif fp.endswith(".designspace"): | ||
ds = DesignSpaceDocument.fromfile(fp) | ||
for src in ds.sources: | ||
ufos.append(Font(src.path)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doing this for each master and then for each individual glyph is not sufficient; you want to set the flag on all the masters (or if you like only on the default master, which is the one that matters and which will contribute the actual glyf table to the final VF) if any of them needs overlap. If you only set on a non-default master, but not on the default one, then it will just be ignored and not present in the VF.
|
||
def set_overlap_bits(ufo): | ||
# Skip setting bits if ufo | ||
if any(g.lib.get("public.truetype.overlap") for g in ufo): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
safe to skip if flags are present (might have been added manually), but perhaps you could also add a CLI flag to overwrite existing overlap flags, as they might have been mechanically generated and out of sync
This script will automatically set the glyph overlap bits for glyphs that have overlapping contours. Atm, it will only work on ufos since AFAIK .glyphs files do not have the ability to set these bits yet.
I originally wrote this as a ufo2ft filter. However, on David Berlow families, it was agonisingly slow. The pain was similar to installing win 95 using floppy disks.
I may be cheeky and throw this into the gftools builder for fonts that have less than 10 masters.