Skip to content

SVG-related improvements and fixes #569

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

Merged
merged 4 commits into from
Mar 7, 2023
Merged

SVG-related improvements and fixes #569

merged 4 commits into from
Mar 7, 2023

Conversation

Connum
Copy link
Contributor

@Connum Connum commented Feb 21, 2023

This PR supersedes draft #568

Description

Motivation and Context

The library failed several tests in the unicode test tool, despite technically supporting the underlying features.
Exporting SVG paths was possible, but the paths were flipped upside down due to inverted Y-axes in SVG vs font paths. Importing SVG paths was not supported.

How Has This Been Tested?

We now pass 69 tests instead of 37 in the unicode test tool, see report: https://connum.github.io/opentypejs-reports/reports/test-connum_svg-optimize-7e4de2c2ce9e822161b61349cf2a7ec55c7fd05e.html
We didn't have any tests for path.js so I added several to test the basic and new features.
I played around with loading, exporting and modifying SVG paths in existing fonts.

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
    Will break code that relies on SVG data being upside down, but that behaviour was wrong in the first place

Checklist:

  • I did npm run test and all tests passed green (including code styling checks).
  • I have added tests to cover my changes.
  • My change requires a change to the documentation.
  • I have updated the README accordingly.
  • I have read the CONTRIBUTING document.

@Connum Connum added enhancement editing Issues related to editing of glyphs, regarded as out of scope for now labels Feb 21, 2023
@Connum Connum added this to the Release 2.0.0 milestone Feb 21, 2023
@Connum Connum requested a review from ILOVEPIE February 21, 2023 23:51
Copy link
Contributor

@ILOVEPIE ILOVEPIE left a comment

Choose a reason for hiding this comment

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

I'd like to see someone else's opinion on this, the changes I've suggested are relatively minor, this is however a breaking change PR, so we should probably lump it into 2.0.0

@Connum
Copy link
Contributor Author

Connum commented Feb 23, 2023

this is however a breaking change PR, so we should probably lump it into 2.0.0

I was hesitant whether to mark it as breaking or not. Having upside down output is obviously not what anyone would expect, on the other hand it's been like that for years, so there's probably code out there that relies on it and flips it itself.

Copy link
Contributor

@yne yne left a comment

Choose a reason for hiding this comment

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

Very nice addition, but we shall clarify ASAP if the next release is going to be 2.0.0, in which case some function argument could be completely migrated (number => object).

@Connum Connum requested a review from yne February 23, 2023 20:47
@Connum
Copy link
Contributor Author

Connum commented Feb 27, 2023

up-to-date with the current master and ready for re-review

@ILOVEPIE
Copy link
Contributor

I was hesitant whether to mark it as breaking or not. Having upside down output is obviously not what anyone would expect, on the other hand it's been like that for years, so there's probably code out there that relies on it and flips it itself.

This is a breaking change as it changes some public facing apis, not just wither the SVG is upside down.

@Connum
Copy link
Contributor Author

Connum commented Feb 28, 2023

You can still pass a number instead of the options object and it will be handled accordingly, or am I missing something else?
We also have the platform agnostic names table change in the master already and had the version field bumped to 2.0.0 before it was removed with the overhauled build/release system. So I think we should be able to merge this?
If we really need a hotfix 1.x release before 2.0.0 is ready, we can still branch it off from an earlier point before all the new features that were added recently.

@yne
Copy link
Contributor

yne commented Feb 28, 2023

@Connum
I rebased-squashed your branch so it shall be mergable now,
please avoid merging master into your branch as it make the rebase more painful.

What @ILOVEPIE mean is that calling toPathData() without argument, did not flipY before your commit, but now toPathData() will flipY by default.

@Connum
Copy link
Contributor Author

Connum commented Mar 2, 2023

Can we please talk about the roadmap and how to proceed with this? I'm not that keen on resolving endless conflicts should this get merged after other PRs with big changes. 🙂 Also this SVG fix is very helpful when testing new features/enhancements/fixes with the Unicode test suite.

It was my understanding that we were going to release 2.0.0 next anyway (though I have to admit I'm not sure what that feeling is based on, as we never explicitly talked about it before) but if that's not the plan, we could at least have a dedicated 2.0.0 branch? Then I'll also scrap the partial backwards compatibility.

@yne
Copy link
Contributor

yne commented Mar 3, 2023

@ILOVEPIE can we process?

Copy link
Contributor

@ILOVEPIE ILOVEPIE left a comment

Choose a reason for hiding this comment

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

There is one comment I had regarding performance/readability, but it looks good.

@Connum Connum requested review from ILOVEPIE and yne March 6, 2023 13:52
Copy link
Contributor

@ILOVEPIE ILOVEPIE left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@Connum Connum merged commit d1deda1 into master Mar 7, 2023
@Connum Connum deleted the optimize-svg branch March 7, 2023 05:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
editing Issues related to editing of glyphs, regarded as out of scope for now enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support using a plain SVG file as a glyph Path.toPathData and Path.toSVG - X Axis is flipped
3 participants