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

Pass flags down when copying the data #305

Merged
merged 3 commits into from
Dec 8, 2022
Merged

Conversation

justvanrossum
Copy link
Contributor

So copies can also be "strict".

@justvanrossum
Copy link
Contributor Author

(Sorry, I don't understand the test failure)

justvanrossum added a commit to justvanrossum/fontmake that referenced this pull request Dec 8, 2022
This fixes googlefonts#961, and makes instantiator.py as strict as varLib when interpolating.

Depends on robotools/fontMath#305, though, as fontMath doesn't properly propagate its "strict" flag down to copies.

So this PR should probably also update its fontMath version dependency, once a new fontMath is out.
@anthrotype
Copy link
Member

maybe it's sufficient that you rename "py.test" -> "pytest" in

fontMath/tox.ini

Lines 13 to 16 in 678c071

# run the test suite against the package installed inside tox env
!cov: py.test {posargs}
# if `*-cov` factor in the tox env name, also collect test coverage
cov: coverage run --parallel-mode -m py.test {posargs}

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, pending CI goes green again

So copies can also be "strict".
@justvanrossum
Copy link
Contributor Author

Rebased on master, CI should now pass.

@anthrotype
Copy link
Member

thanks, feel free to merge. I don't know how releases work here, so I'd leave that to you or Ben

@codecov
Copy link

codecov bot commented Dec 8, 2022

Codecov Report

Merging #305 (d4b7b03) into master (4472818) will not change coverage.
The diff coverage is 50.00%.

❗ Current head d4b7b03 differs from pull request most recent head 53becc7. Consider uploading reports for the commit 53becc7 to get more accurate results

@@           Coverage Diff           @@
##           master     #305   +/-   ##
=======================================
  Coverage   89.00%   89.00%           
=======================================
  Files          12       12           
  Lines        2338     2338           
  Branches      301      301           
=======================================
  Hits         2081     2081           
  Misses        189      189           
  Partials       68       68           
Flag Coverage Δ
unittests 88.96% <50.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
Lib/fontMath/mathGlyph.py 81.40% <50.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@justvanrossum justvanrossum merged commit fbfa7f6 into master Dec 8, 2022
@anthrotype anthrotype deleted the justvanrossum-patch-1 branch December 8, 2022 15:41
@LettError
Copy link
Member

Should I ask what this was about?

@anthrotype
Copy link
Member

anthrotype commented Dec 8, 2022

MathGlyph has a strict parameter to behave more like varLib when interpolating, but its copy() method was not retaining that flag

@anthrotype
Copy link
Member

context is googlefonts/fontmake#961 and googlefonts/fontmake#962

@justvanrossum
Copy link
Contributor Author

...which in turn causes off-curve points that coincide with on-curve points to be deleted as part of the "filter redundant points" step, which breaks interpolation compatibility. See also googlefonts/fontmake#961

@justvanrossum
Copy link
Contributor Author

justvanrossum commented Dec 8, 2022

It was more about MathGlyph.copyWithoutMathSubObjects(): all math operation call that, so any resulting math glyph would not have the "strict" flag set, so when writing back to a "normal" glyph those off-curves would be filtered, regardless of the original "strict" setting.

@LettError
Copy link
Member

@justvanrossum @anthrotype: I worked on the early strict stuff. I did not look at MathGlyph.copyWithoutMathSubObjects or copy. Really sorry this caused trouble, thank you for sorting it out!

@anthrotype
Copy link
Member

no worries! 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.

3 participants