search

mapbox / spritezero

small opinionated sprites
Other
173 stars 62 forks source link

Parse and validate metadata for stretchable icons from SVGs #68

Closed kkaefer closed 4 years ago

kkaefer commented 4 years ago

Implements metadata parsing from SVGs for stretchable icons.

kkaefer commented 4 years ago

Missing:

kkaefer commented 4 years ago
samanpwbb commented 4 years ago

@kkaefer here's a fixture I just made in illustrator. I think I did it right:

<?xml version="1.0" encoding="utf-8"?>
<!-- Generator: Adobe Illustrator 23.1.1, SVG Export Plug-In . SVG Version: 6.00 Build 0)  -->
<svg version="1.1" id="Layer_1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" x="0px" y="0px"
     width="18px" height="18px" viewBox="0 0 18 18" style="enable-background:new 0 0 18 18;" xml:space="preserve">
<rect x="3" y="3" style="fill:#FFFFFF;stroke:#000000;stroke-width:2;stroke-miterlimit:10;" width="12" height="12"/>
<rect x="4" y="4" style="fill:#00FFEE;" width="10" height="4"/>
<rect id="mapbox-icon-content" x="4" y="8" style="fill:none;" width="10" height="6"/>
<rect id="mapbox-icon-stretch-y" x="13" y="8" style="fill:none;" width="1" height="6"/>
<rect id="mapbox-icon-stretch-x" x="4" y="13" style="fill:none;" width="10" height="1"/>
</svg>
kkaefer commented 4 years ago

add more thorough test coverage

What kind of test coverage are you referring to? There are already a lot of tests for validation; do you think we should add more SVGs? In that case, I think we'd need a larger variety of icons designed for being stretched that we can test this with. Where do you propose we'd get these from?

Questions from https://github.com/mapbox/mapbox-gl-js/issues/8917:

What do we do when one of the keyword ids like mapbox-icon-content is inside a group with a transform? Skew, rotate, or scale transforms could potentially change the bounding box.

The code resolves transforms prior to computing the bounding boxes. I tested translate transforms, but I'll also check rotation and skewing.

What happens when a keyword id is on a path or some other element rather than a rect? Should that throw an error, or should our service try to derive a bounding box anyway?

The code converts all shapes to paths, and computes the bounding box of the path. We could change it to be restricted to just <rect> elements, but one application is that you can currently draw lines for stretch-x and stretch-y, which more closely mirrors the fact that we only care about one axis.

kkaefer commented 4 years ago

The code resolves transforms prior to computing the bounding boxes. I tested translate transforms, but I'll also check rotation and skewing.

Turns out svgo skips transform folding if elements have an id attribute. It does cleanup null transforms though.

samanpwbb commented 4 years ago

do you think we should add more SVGs?

Yep that’s what I meant - in my experience SVGO doesn’t always work exactly the way I expect it to. I can make a few more fixtures for you!

samanpwbb commented 4 years ago

Here are some weird SVGs, food for thought.

A shield where each element is individually rotated. Results in a shield that is rotated 45 degrees.

<?xml version="1.0" encoding="utf-8"?>
<!-- Generator: Adobe Illustrator 23.1.1, SVG Export Plug-In . SVG Version: 6.00 Build 0)  -->
<svg version="1.1" id="Layer_1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" x="0px" y="0px"
     width="18px" height="18px" viewBox="0 0 18 18" style="enable-background:new 0 0 18 18;" xml:space="preserve">
<g>
    <rect x="4.5" y="4.5" transform="matrix(0.7071 -0.7071 0.7071 0.7071 -3.7279 9)" style="fill:#FFFFFF;stroke:#000000;stroke-width:2;stroke-miterlimit:10;" width="8.9" height="8.9"/>
    <rect x="9.1" y="3.7" transform="matrix(0.7071 -0.7071 0.7071 0.7071 -2.1473 9.6547)" style="fill:#00FFEE;" width="3" height="7.5"/>
    <rect id="mapbox-icon-content" x="5.7" y="6.3" transform="matrix(0.7071 -0.7071 0.7071 0.7071 -4.7817 8.5635)" style="fill:none;" width="4.5" height="7.5"/>
    <rect id="mapbox-icon-stretch-y" x="8.1" y="12.1" transform="matrix(0.7071 -0.7071 0.7071 0.7071 -5.7637 10.9344)" style="fill:none;" width="4.5" height="0.7"/>
    <rect id="mapbox-icon-stretch-x" x="6.3" y="7.6" transform="matrix(0.7071 -0.7071 0.7071 0.7071 -6.0988 8.0179)" style="fill:none;" width="0.7" height="7.5"/>
</g>
</svg>

A shield inside two groups, one with a rotation, one with a translate. Results in a shield that is rotated 45 degrees.

<?xml version="1.0" encoding="utf-8"?>
<!-- Generator: Adobe Illustrator 23.1.1, SVG Export Plug-In . SVG Version: 6.00 Build 0)  -->
<svg version="1.1" id="Layer_1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" x="0px" y="0px"
     width="18px" height="18px" viewBox="0 0 18 18" style="enable-background:new 0 0 18 18;" xml:space="preserve">
<g transform="rotate(45)">
    <g transform="translate(4 -8)">
        <rect x="5" y="5" style="fill:#FFFFFF;stroke:#000000;stroke-width:2;stroke-miterlimit:10;" width="8" height="8"/>
        <rect x="6" y="6" style="fill:#00FFEE;" width="6" height="2"/>
        <rect id="mapbox-icon-content" x="6" y="8" style="fill:none;" width="6" height="4"/>
        <rect id="mapbox-icon-stretch-y" x="11" y="8" style="fill:none;" width="1" height="4"/>
        <rect id="mapbox-icon-stretch-x" x="6" y="11" style="fill:none;" width="6" height="1"/>
    </g>
</g>
</svg>

A shield inside two groups, one with a rotation, one with the reverse rotation. Results in a normal-looking shield:

<?xml version="1.0" encoding="utf-8"?>
<!-- Generator: Adobe Illustrator 23.1.1, SVG Export Plug-In . SVG Version: 6.00 Build 0)  -->
<svg version="1.1" id="Layer_1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" x="0px" y="0px"
     width="18px" height="18px" viewBox="0 0 18 18" style="enable-background:new 0 0 18 18;" xml:space="preserve">
<g transform="rotate(45)">
    <g transform="rotate(-45)">
        <rect x="5" y="5" style="fill:#FFFFFF;stroke:#000000;stroke-width:2;stroke-miterlimit:10;" width="8" height="8"/>
        <rect x="6" y="6" style="fill:#00FFEE;" width="6" height="2"/>
        <rect id="mapbox-icon-content" x="6" y="8" style="fill:none;" width="6" height="4"/>
        <rect id="mapbox-icon-stretch-y" x="11" y="8" style="fill:none;" width="1" height="4"/>
        <rect id="mapbox-icon-stretch-x" x="6" y="11" style="fill:none;" width="6" height="1"/>
    </g>
</g>
</svg>
kkaefer commented 4 years ago

@samanpwbb is that the verbatim output from Illustrator or did you modify them after the export?

samanpwbb commented 4 years ago

did you modify them after the export?

The last two are modified, first is directly from illustrator. Do you think these are edge cases that we shouldn’t worry about?

kkaefer commented 4 years ago

Ok, I was wondering whether Illustrator will actually put transforms into exported SVGs, but it looks like it does in some circumstances. Affinity seems to resolve them prior to exporting.

kkaefer commented 4 years ago

@samanpwbb Thanks for the SVGs. I incorportated them into the test suite and changed our parsing so that transforms, including multiple nested transforms on groups, are resolved correctly. The SVGs' stretch zones don't really make a lot of sense given that the entire icon is rotated.

kkaefer commented 4 years ago

Is there a chance we could see a performance hit when we add this to the API?

Yeah, it can take a few millisecond to process an icon. If we're looking for optimizations, https://github.com/mapbox/spritezero/issues/7 has probably a higher impact. In any case, sprite generation should be an infrequent operation.

kkaefer commented 4 years ago

This is ready to be merged pending the discussion in https://github.com/mapbox/mapbox-gl-js/issues/8917#issuecomment-558992126