Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Changes in SVG backend to improve compatibility with Affinity designer#28504

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
tacaswell merged 1 commit intomatplotlib:mainfromLorenzoPeri17:main
Jul 22, 2024

Conversation

LorenzoPeri17
Copy link
Contributor

Separate font attributes && use translate(x y) in RendererSVG._draw_text_as_path

PR summary

Closes#20910

Two small changes in the SVG backend:

  • xlink:href tags' position is specified viatransform=translate(x y) rather than withx andy attributes
  • text tags separatefont attribute into its separate constituent properties

PR checklist

brianmcdonald reacted with heart emojigiusSacco reacted with rocket emoji
Copy link

@github-actionsgithub-actionsbot left a comment

Choose a reason for hiding this comment

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

Thank you for opening your first PR into Matplotlib!

If you have not heard from us in a week or so, please leave a new comment below and that should bring it to our attention. Most of our reviewers are volunteers and sometimes things fall through the cracks.

You can also join uson gitter for real-time discussion.

For details on testing, writing docs, and our review process, please seethe developer guide

We strive to be a welcoming and open project. Please follow ourCode of Conduct.

@ksunden
Copy link
Member

Did you mean to close this?

I noticed that tests were not passing, but were all failing in the same way, so not sure what the fix is, but it may be relatively easy to do, and we can do it within the same PR.

@LorenzoPeri17
Copy link
ContributorAuthor

Hi! Thanks for the message. I realized I read about the issue I was having inlib/matplotlib/testing/compare.py::convert so I figured I would close the PR because it was failing to do some debugging.
I have chased down one issue which was described inthis commit. I'm more than happy to iron out the remaining issues within this PR if that works best :)

@LorenzoPeri17LorenzoPeri17 changed the titleChanges in SV backend to improve compatibility with Affinity designerChanges in SVG backend to improve compatibility with Affinity designerJul 7, 2024
@@ -433,7 +432,7 @@ def test_mathtext_fallback_invalid():
@pytest.mark.parametrize(
"fallback,fontlist",
[("cm", ['DejaVu Sans', 'mpltest', 'STIXGeneral', 'cmr10', 'STIXGeneral']),
("stix", ['DejaVu Sans', 'mpltest', 'STIXGeneral'])])
("stix", ['DejaVu Sans', 'mpltest', 'STIXGeneral', 'STIXGeneral', 'STIXGeneral'])])
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need the additional 'STIXGeneral' entries here?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

I have slightly changed the logic for how thetspans inside atext tag are separated (backend_svg.py::_draw_text_as_text line 1224 onwards).
During some testing I found poor compatibility with specifying multiplex andy in the sametspan (from readingthe docs maybedx would be better if the old way was to be preferred?)
My new solution is cleaner code-wise and seems to resolve the compatibility issues. However, it causes moretspans to be generates, hence why the repetition in the tests.

@tacaswell
Copy link
Member

To check we need to regenerate the svg files to make keep them in sync with our compare code, not because rendered output (in compliant viewers) actually changed?

@tacaswelltacaswell added this to thev3.10.0 milestoneJul 8, 2024
@LorenzoPeri17
Copy link
ContributorAuthor

To check we need to regenerate the svg files to make keep them in sync with our compare code, not because rendered output (in compliant viewers) actually changed?

Exactly. That and because on the appveyor tests (and only there, azure and github were passung just fine) for some reason a few tests would fail with a very small RMS (~0.001) because of a few pixels in long ticklabels beingslightly off when rendering (see attached example)

axvspan_epoch_svg
axvspan_epoch-expected_svg

Falied diff
axvspan_epoch_svg-failed-diff

@tacaswell
Copy link
Member

The appeveyor failures look related and real.

@LorenzoPeri17
Copy link
ContributorAuthor

I had some time to debug the failing tests. It seems that the baseline images were specifying certain coordinates forxlink:href elements beyond 6 digits of precision, causing inkscape to render some text half a pizel off. I have changed this and I can't see any failure.
However, some tests seem to fail now for unrelated reasons. I assume this is because of changes to the workflows. I assume this is because of this fork being behind with respect tomain?

@tacaswell
Copy link
Member

Can you please rebase on main and squash all of the image updating into one commit?

@tacaswell
Copy link
Member

At some point we change from outputting a lot of digits to rounding to a smaller number of digits but did not bulk re-generate the test images because they were still passing.

@LorenzoPeri17
Copy link
ContributorAuthor

Should be done :)

@@ -266,8 +266,11 @@ def _get_fontconfig_fonts():
@lru_cache
def _get_macos_fonts():
"""Cache and list the font paths known to ``system_profiler SPFontsDataType``."""
d, = plistlib.loads(
subprocess.check_output(["system_profiler", "-xml", "SPFontsDataType"]))
try:
Copy link
Member

Choose a reason for hiding this comment

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

Was this change supposed to be included?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Apologies, that slipped in when squasing. Fixed now

@tacaswell
Copy link
Member

It looks like there is still a merge from main in here?

@LorenzoPeri17LorenzoPeri17force-pushed themain branch 3 times, most recently from1f1b6e0 to0e7f73dCompareJuly 18, 2024 21:00
@LorenzoPeri17
Copy link
ContributorAuthor

Is this OK now?

@tacaswell
Copy link
Member

no, still a rouge merge from main...is your local main not equal to upstream main?

…SVG._draw_text_as_pathfixed linting (flake8 test failing)Fixed checks in testing for svg.fonttype = nonefixed flake8 && failing testslintingmodified tspans && fix to svg testsUpdated baseline images for svg testsUpdate lib/matplotlib/testing/compare.pynicer regex to check if font syling in svg is specifiedCo-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>Apply suggestions from code reviewMore concise font testing in test_backend_svg.pyCo-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>Update lib/matplotlib/tests/test_mathtext.pyCo-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>Update backend_svg.pyinlining `style`restored test imagesupdate to baseline imagesFix to regex for testing/compare.py::compareforgot one baseline image
@LorenzoPeri17
Copy link
ContributorAuthor

Gotcha! It is done now, I didn't realize I had to rebase, not merge upstream main. I was doing it though the nice green button on the github page, but that only offers the option to do a merge.

@tacaswelltacaswell merged commitbcfecca intomatplotlib:mainJul 22, 2024
40 checks passed
@tacaswell
Copy link
Member

Thank you for your work on this@LorenzoPeri17 .

Congratulations on your first merged Matplotlib PR 🎉 I hope we hear from you again!

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@github-actionsgithub-actions[bot]github-actions[bot] left review comments

@tacaswelltacaswelltacaswell approved these changes

@timhoffmtimhoffmtimhoffm approved these changes

Assignees
No one assigned
Projects
Status: Waiting for author
Milestone
v3.10.0
Development

Successfully merging this pull request may close these issues.

[Bug]: Exported SVG files are no longer imported Affinity Designer correctly
4 participants
@LorenzoPeri17@ksunden@tacaswell@timhoffm
[8]ページ先頭
©2009-2025 Movatter.jp