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

Fix viewBox in exported SVG glyph #3395

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@skef
Copy link

skef commented Jan 4, 2019

Export a glyph as an SVG, like J from tests/fonts/CaslonMM.sfd in the repository, and you get something like J_CaslonMM.svg in this zip: svgs.zip . Open that in a browser and you see something like this:

jsvg_browser

And the bottom continues to be cut off when you resize the window. So what's going on?

The relevant export code starts this way:

int _ExportSVG(FILE *svg,SplineChar *sc,int layer) {
    char *end;
    int em_size;
    DBounds b;

    SplineCharLayerFindBounds(sc,layer,&b);
    em_size = sc->parent->ascent+sc->parent->descent;
    if ( b.minx>0 ) b.minx=0;
    if ( b.miny>-sc->parent->descent ) b.miny = -sc->parent->descent;
    if ( b.maxy<em_size ) b.maxy = em_size;

    locale_t tmplocale; locale_t oldlocale; // Declare temporary locale storage.
    switch_to_c_locale(&tmplocale, &oldlocale); // Switch to the C locale temporarily and cache the old locale.
    fprintf(svg, "<?xml version=\"1.0\" standalone=\"no\"?>\n" );
    fprintf(svg, "<!DOCTYPE svg PUBLIC \"-//W3C//DTD SVG 1.1//EN\" \"http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd\" >\n" );
    fprintf(svg, "<svg xmlns=\"http://www.w3.org/2000/svg\" xmlns:xlink=\"http://www.w3.org/1999/xlink\" version=\"1.1\" viewBox=\"%d %d %d %d\">\n",
            (int) floor(b.minx), (int) floor(b.miny),
            (int) ceil(sc->width), (int) ceil(em_size));
    fprintf(svg, "  <g transform=\"matrix(1 0 0 -1 0 %d)\">\n",
            sc->parent->ascent );

In the FontForge geometric space descent (in the typography sense) corresponds to decreasing y, and the origin is typically near the lower left of a glyph. In SVG space descent corresponds to increasing y, and by convention the origin is usually placed in the upper left of a picture, so that x and y values are generally positive. That's what the transform line does. It has the effect of multiplying all embedded y values by negative one and adding the font's ascent value, putting the y origin at what that line is in FontForge space. This is all good.

The intent of the viewBox code immediately before is also sensible. The height (the fourth value) is set to the calculated em_size and the width (the third) is set to the calculated glyph width. The first value is also set sensibly, given that some glyphs have negative x values and the transform doesn't alter those. However, the second value makes no sense. The transformation has already moved y=0 to the proper location so the start of the viewBox should just be 0.

After that change, which is all this pull request is, if you export that character as an SVG you get the other file in the zip, which looks like this in a browser:

jsvggood_browser

The remaining worry is whether the viewBox is being used as some kind of record for importing or some other use. The answer is "no". Try importing either char svg into a font and you get the same effect. And by inspection viewBox is parsed twice in svg.c, once in SVGuseTransform and once in SVGParseSVG, but neither the x nor the y value is used.

Types of changes

What types of changes does your code introduce? Check all the boxes that apply:

  • Bug fix (non-breaking change which fixes an issue)

Description

  • Describe your changes in detail.

Final checklist

Go over all the following points and check all the boxes that apply.
If you're unsure about any of these, don't hesitate to ask. We're here to help! Various areas of the codebase have been worked on by different people in recent years, so if you are unfamiliar with the general area you're working in, please feel free to chat with people who have experience in that area. See the list here.

  • My code follows the code style of this project found here.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING guidelines.

Your pull request will be tested via Travis CI to automatically indicate that your changes do not prevent compilation. FontForge is a big program, so Travis can easily take over 20 minutes to confirm your changes are buildable. Please be patient. More details about using Travis can be found here.

If it reports back that there are problems, you can log into the Travis system and check the log report for your pull request to see what the problem was. If no error is shown, just re-run the Travis test for your pull-request (that failed) to see a fresh report since the last report may be for someone else that did a later pull request, or for mainline code. If you add new code to fix your issue/problem, then take note that you need to check the next pull request in the Travis system. Travis issue numbers are different from GitHub issue numbers.

Skef Iterum

@skef skef force-pushed the skef:svgcoord branch from 331c0e5 to ac71f5b Jan 4, 2019

@skef

This comment has been minimized.

Copy link

skef commented Jan 4, 2019

Ugh, pushed too soon. I forgot sc->width corresponds to the advance width, which isn't a good measure of the glyph width.

I pushed a better version of _ExportSVG that calculates the horizontal values of viewBox intelligently. Here is a zip with the export of f from AmbrosiaItalic.sfd, also in the test directory: widthtest.zip . Viewing this in a browser probably won't help much, but opening it in Inkscape (for example) should show the boundaries.

@ctrlcctrlv

This comment has been minimized.

Copy link

ctrlcctrlv commented Jan 5, 2019

This seems to work by only changing the horizontal values, never the height, therefore roundtrip exports to an SVG editor, having edits applied, and reimported to FontForge seem to work fine for me. FontForge (for some reason) has never supported setting the bearings used for kerning based on SVG data.

Are there any situations you are aware of where round-trip SVG imports won't work (or will require e.g. rescaling) with this new viewBox code, @skef?

@skef

This comment has been minimized.

Copy link

skef commented Jan 5, 2019

viewBox is only parsed twice in the code; once in svg.c:SVGuseTransform() and once in svg.c:ParseSVG(). In neither case are the first two values stored or used. This push doesn't alter the fourth (height) value. So the question is whether the (new version of the push) width changes will make a difference.

In the case of SVGuseTransform() the answer is straightforwardly "no" because the code only uses that value (swid) if there is also a width attribute and the export code doesn't add a width. The story in SVGParseSVG() is similar: because there is no width or height attribute both will be set to 1, which means width>height will be false, which means st.transform[0] and [3] will be initialized with sheight (the fourth value) rather than swidth (the third).

Speaking more generally, the SVG import code appears to be intended to work with a variety of code styles (within limits) and just uses the viewBox for scaling hints.

@skef

This comment has been minimized.

Copy link

skef commented Jan 5, 2019

It did occur to me that it could be handy to encode the Y value corresponding to FontForge space 0 and the X value of the advance width in some kind of structured comment in the export, so that those could be recovered during a round-trip. But that seemed like it would require discussion outside the scope of this little change.

@ctrlcctrlv

This comment has been minimized.

Copy link

ctrlcctrlv commented Jan 5, 2019

If we were to add the X value of the advance and Y value of the start of the descender, it would be more logical to use <sodipodi:guide> tags instead, which are the tags used by Inkscape, and due to their long history in the format (over 12 years), are something of a de-facto standard among other SVG editors/tools as well.

@skef

This comment has been minimized.

Copy link

skef commented Jan 11, 2019

Closes #3271

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