-
Notifications
You must be signed in to change notification settings - Fork 484
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
feat: add SVG table support #672
Conversation
Is this to implement apple-style emoji fonts? I'll pull in connum on this one as I'm a little out of the loop but this looks good to go. |
One issue I do notice for this is the lack of integration with the drawing code. |
Hello @ILOVEPIE! It is to parse and make color fonts like Colortube or Fattern where SVG documents are embedded directly into a font file instead of being converted into glyphs. |
The purpose of the library is to parse, generate and draw fonts. This PR is incomplete imho. |
Ok, I see your point and will try to find time to implement drawing as part of this PR. |
IMHO it's OK to implement parsing and writing first and drawing with an additional PR. We have other features where parsing works but writing doesn't, so here we even have parsing and writing in one go. |
But what I am missing is a method to retrieve the SVG document in plaintext, using |
440881a
to
3f7da0f
Compare
Nice! However, rendering of the Colortube-lWAd.otf is working in the font inspector and the two top canvases on the main page, but not the glyph inspector (or the glyphs on the main page) for me. Can you look into this? Two optimizations we should implement for performance:
We can implement the second one independently from this PR at a later time, but it would be great if we could have the lazy loading from the start. |
There also seem to be scaling issues when rendering SVG data in the following fonts from https://github.com/unicode-org/text-rendering-tests/tree/main/fonts |
I've added the ability to draw SVGs. Before drawing SVG images must be prepared by calling I've exported @Connum, you wanted to have a getter |
When getting the |
For example, NotoColorEmoji-SVG.otf is freezing the browser for a considerable amount of time. |
Thank you @Connum for the feedback! It's difficult to prepare images lazily, because |
I'm currently working on COLR/CPAL font rendering (so far we only support parsing and writing the tables), where I will also implement lazy loading of the layers. I hope to have this ready in the next days, then you could take some inspiration from it. |
It will be interesting to see your lazy loading implementation. But before I've read your message I've already found a way to implement it. It is now possible to add I haven't solved the scaling problem with the fonts you mentioned above yet. I'll look at it later. |
5c18882
to
eb091ce
Compare
I hope I'll finish the cpal/colr overhaul today or tomorrow. Maybe it will help you fix the performance issue. |
Haven't created a PR yet, but you can already take a look at my current work in progress here: regarding the lazy loading, especially: |
e01d5af
to
ba13f13
Compare
Thank you, @Connum , I've added |
We should definitely support all of the different methods of doing emoji that have been implemented into fonts, although I will say that only Apple supports the SVG table currently. |
I'll have a PR for CPAL/COLR v0 rendering ready by (hopefully) tomorrow. v1 is quite complex though. |
Scaling issues have been resolved. Is there anything anything else you'd like me to improve? |
Can you please resolve the conflicts with the current master in order to do a rebase? Then we can have another look at it! Thanks! |
9fb94d7
to
4efaee0
Compare
The conflicts are resolved. I took inspiration from your work and made three changes:
|
Thank you! I already spotted a few things, but I'll do a full review soon, hopefully in the course of this week. |
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.
FInished review with a few requested changes.
Could you also please add a TODO comment and console message to
Line 454 in 946f255
Glyph.prototype.toSVG = function(options) { |
similar to
Line 636 in 946f255
if (this._layers && this._layers.length) { |
Because we can't simply output the whole SVG document as it is, we should first sanitize it to make sure it doesn't contain any malicious scripts or features not supported in SVG fonts. (By the way, the latter about unsupported features would also be true for rendering in general, even on canvases, but we don't have to do that in this PR! See https://learn.microsoft.com/en-us/typography/opentype/spec/svg#svg-specification)
Thanks for the detailed review! I'll try to address all the issues you mentioned by the end of next week. |
TODO and warning are added to |
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.
Looking good! I'll add the licenses for the three new test fonts to the LICENSE file in a follow-up PR, so we don't have to take another round.
Thanks again for your work and your patience. I'd love to see more contributions in the future! |
Thank you for your time and attention. It was a pleasure to collaborate with you on this task! |
Description
This PR adds support for 'SVG ' table.
Motivation and Context
Fixes #106, #193
How Has This Been Tested?
I added tests to
test/svg.js
file. These tests parse and encode Colortube font.Screenshots (if appropriate):
Types of changes
Checklist:
npm run test
and all tests passed green (including code styling checks).