Parse and validate metadata for stretchable icons from SVGs #68

kkaefer · 2019-10-28T17:05:52Z

Implements metadata parsing from SVGs for stretchable icons.

kkaefer · 2019-10-29T15:38:58Z

Missing:

spritezero.generateLayout should parse the SVGs and add the metadata
content, stretch-x, and stretch-y coordinates need to be stretched by the pixel ratio

kkaefer · 2019-10-29T15:40:25Z

We also need a few more test SVGs from Illustrator. (I don't have access to Illustrator so can't generate them myself)

samanpwbb · 2019-10-29T18:20:51Z

lib/extract-svg-metadata.js

+const applyPlugins = require('svgo/lib/svgo/plugins');
+const computePathBounds = require('svg-boundings').path;
+
+const extractMapboxIconMetadata = {


We should consider add more test fixtures to identify the limit of which kinds of SVGs this code will handle, and consider throwing errors in cases where we get SVGs that we can't handle well (nested transforms, groups, different element types). I can forsee difficult to debug cases cropping up in the future if we don't get on top of this.

samanpwbb

Code looks pretty good but I think we should add more thorough test coverage and afterwards, see if might make sense to do some stricter validation of elements with mapbox-icon.

I will post some illustrator svgs for you that match the affinity svgs!

lib/extract-svg-metadata.js

index.js

samanpwbb · 2019-10-29T18:38:28Z

@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 · 2019-10-30T10:49:03Z

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 mapbox/mapbox-gl-js#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 · 2019-10-30T12:36:43Z

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 · 2019-10-30T16:03:29Z

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 · 2019-10-30T22:23:20Z

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 · 2019-10-31T12:28:02Z

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

samanpwbb · 2019-10-31T14:40:05Z

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 · 2019-10-31T15:31:47Z

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 · 2019-11-04T14:25:28Z

@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.

samanpwbb

Looks great.

One thing: This new extractMetadata function is doing a lot of work: traversing SVGs multiple times, converting all shapes to path, then simplifying all paths for every SVG.

Is there a chance we could see a performance hit when we add this to the API? There are optimizations we could make, for example, by adding a property to certain icons to denote whether they should be searched for metadata or skipped. Studio could surface a checkbox that enables/disables extraction. Probably best to get this merged, and circle back around to performance optimizations if they're needed.

kkaefer · 2019-11-05T08:28:14Z

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, #7 has probably a higher impact. In any case, sprite generation should be an infrequent operation.

kkaefer · 2019-11-27T10:21:21Z

This is ready to be merged pending the discussion in mapbox/mapbox-gl-js#8917 (comment)

kkaefer requested review from a team and removed request for a team October 29, 2019 15:13

samanpwbb self-assigned this Oct 29, 2019

samanpwbb mentioned this pull request Oct 29, 2019

RFC: Stretchable icons mapbox/mapbox-gl-js#8917

Closed

samanpwbb reviewed Oct 29, 2019

View reviewed changes

samanpwbb suggested changes Oct 29, 2019

View reviewed changes

lib/extract-svg-metadata.js Outdated Show resolved Hide resolved

index.js Show resolved Hide resolved

kkaefer force-pushed the parse-metadata branch from 673253d to 01b55f3 Compare October 30, 2019 13:28

kkaefer force-pushed the parse-metadata branch from 01b55f3 to c848928 Compare November 4, 2019 14:23

samanpwbb approved these changes Nov 4, 2019

View reviewed changes

kkaefer added 3 commits November 27, 2019 11:08

move sprite generation code to lib folder

7ca87a9

parse and validate metadata for stretchable icons from SVGs

541173d

rename mapbox-icon-* to mapbox-* and stretch-x to stretchX

af06f40

kkaefer force-pushed the parse-metadata branch from c848928 to af06f40 Compare November 27, 2019 10:20

kkaefer merged commit c70e462 into master Dec 11, 2019

kkaefer deleted the parse-metadata branch December 11, 2019 11:10

andrewharvey mentioned this pull request Jan 25, 2020

svgo should be more than just dev-dependency? #72

Closed

jaericson mentioned this pull request Apr 9, 2020

Adds stretchMetadata option to generateLayout functions #75

Merged

2 tasks

flother mentioned this pull request Oct 2, 2023

Support for optional properties (content, stretchX & stretchY) flother/spreet#53

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parse and validate metadata for stretchable icons from SVGs #68

Parse and validate metadata for stretchable icons from SVGs #68

kkaefer commented Oct 28, 2019

kkaefer commented Oct 29, 2019 •

edited

Loading

kkaefer commented Oct 29, 2019

samanpwbb Oct 29, 2019

samanpwbb left a comment

samanpwbb commented Oct 29, 2019

kkaefer commented Oct 30, 2019

kkaefer commented Oct 30, 2019

samanpwbb commented Oct 30, 2019

samanpwbb commented Oct 30, 2019 •

edited

Loading

kkaefer commented Oct 31, 2019

samanpwbb commented Oct 31, 2019

kkaefer commented Oct 31, 2019

kkaefer commented Nov 4, 2019

samanpwbb left a comment

kkaefer commented Nov 5, 2019

kkaefer commented Nov 27, 2019

Parse and validate metadata for stretchable icons from SVGs #68

Parse and validate metadata for stretchable icons from SVGs #68

Conversation

kkaefer commented Oct 28, 2019

kkaefer commented Oct 29, 2019 • edited Loading

kkaefer commented Oct 29, 2019

samanpwbb Oct 29, 2019

Choose a reason for hiding this comment

samanpwbb left a comment

Choose a reason for hiding this comment

samanpwbb commented Oct 29, 2019

kkaefer commented Oct 30, 2019

kkaefer commented Oct 30, 2019

samanpwbb commented Oct 30, 2019

samanpwbb commented Oct 30, 2019 • edited Loading

kkaefer commented Oct 31, 2019

samanpwbb commented Oct 31, 2019

kkaefer commented Oct 31, 2019

kkaefer commented Nov 4, 2019

samanpwbb left a comment

Choose a reason for hiding this comment

kkaefer commented Nov 5, 2019

kkaefer commented Nov 27, 2019

kkaefer commented Oct 29, 2019 •

edited

Loading

samanpwbb commented Oct 30, 2019 •

edited

Loading