Add linter to check svg path dimensions #3107
Conversation
|
It seems that, if we want this check working, we need to scale all failed icons in some automated way before include it. |
PeterShaggyNoble
added
the
meta
|
Thanks for putting this together, @davidjb, looks like it would be a good addition to our linting process 👍 I think we'd need to work on the wording of the error message thrown, though, as, the vast majority of time, these issues creep in at the optimisation stage of icon creation process which can lead to confusion if someone has taken care to ensure that the dimensions are exactly right. I know, the first time it happened to me, I was confused as to what had happened until one of the other team members pointed it out to me. I also think we'd need to up the precision checked to 6 as, while 3 is our ideal maximum precision, there are cases where a higher precision is needed in order to exactly match the source. We currently, by my quick count, have:
(Although, separate to this, we should review all icons with a precision higher than 5, as well as the outliers with a lower number of decimal places - 23 have a precision of just 1 and 5 have no decimal places in any of their path commands.) I don't know that automatically scaling the path if it fails the linting is a viable solution here, @mondeja. As a I mentioned above, this issue usually arises at the optimisation stage and there may be other imperfections or inconsistencies that were created at the same time. Or they may creep in when automatically resizing the path. |
|
@PeterShaggyNoble Regarding the wording, I agree -- my first PR was a carefully-sized path, and was surprised to see With the PR, please use whatever makes sense and modify accordingly, it's essentially a PoC to discuss and cherry-pick. |
|
We can extract the maximum number of floating points of paths coordinates to set the accuracy accordingly, so if an icon has 10 decimal points in their coordinates, the error couldn't be higher than 0.0000000001. The real problem here is to update manually all the icons that doesn't fit this lint rule, it's a vast amount of work. |
There was a problem hiding this comment.
Nice! Very cool @davidjb 😃 I'm fine with the dependency, and I have a couple of small nitpick changes requested below (even though it's a PoC, don't want to forget about these). But overall I like this very much 👍
I think we'd need to work on the wording of the error message thrown, though, as, the vast majority of time, these issues creep in at the optimisation stage of icon creation process which can lead to confusion if someone has taken care to ensure that the dimensions are exactly right. I know, the first time it happened to me, I was confused as to what had happened until one of the other team members pointed it out to me.
Though I'm not entirely against this, I think a better compromise, as @davidjb suggested, is to put it in the Contributing Guidelines (possibly linking that in the error message)
I also think we'd need to up the precision checked to 6 as, while 3 is our ideal maximum precision, there are cases where a higher precision is needed in order to exactly match the source.
Depending on the difficulty of implementing this I like @mondeja suggestion of just taking whatever the precision of the icon is.
Not sure if we should adopt the minimum/maximum error threshold, can you explain why you think that is useful @mondeja?
The real problem here is to update manually all the icons that doesn't fit this lint rule, it's a vast amount of work.
Yep, that is a real problem 😞 Perhaps its an idea to manually ignore the existing icons (somehow?) for now and get this in as quickly as possible so that lists doesn't keep growing...
|
@ericcornelissen Changed made, thanks for the suggestions. Next step might be to also eslint or similar config for linting the linter js file 😄
This will be hard to determine given that when a path gets loaded into Javascript, you're subject to floating point calculations and thus rounding errors. As an example, loading the path from A text analysis of the path is an option but that doesn't necessarily equate to what the bounding box's precision would or should be. A curve or arc between points with one precision will produce path of a higher precision (eg particularly so with angle calculations). This might or might not matter given the icon; I don't know how Maybe a solution is to set a common maximum precision (precision of 5 sees the lint errors jump to 532 😓) or certain icons could include a metadata field for the desired precision if their icons are that precise. Either way, apologies for the work creation scheme -- hopefully this helps more than it hinders. |
Given how uncommon it is for the JS of this project to be changed, I don't want to bother with it. I think it's quite nice to keep as few dependencies as necessary, it makes
That is just the effect of floating point numbers, I think. But it's good that you bring it up as it tells us that we should definitely put a limit on the maximum precision we can possibly consider.
Alright, that sounds to me like we shouldn't do it dynamically, at least for now. What about the following (@davidjb @PeterShaggyNoble @mondeja): keep the precision of this analysis at 3 (maybe 4?) for now and find a solution for the problems with existing icons. I think, currently, it's not such a big deal if an icon exceeds the 24x24 box by less than 0.001. |
Yes, I agree, we can start from that point, but it's not difficult to achieve the extraction of numbers inside the path with a function like this (using regular expressions), and extract the maximum number of floating points with something like this. So, the problem of excesive floating point numbers would be not a problem if we can control the bounding box path processor. This is one of the problems for which I've started some weeks ago in the development of svg-path-bbox, but it's not mature yet. However, I think that it's better start from 0.001 and go further when first failing icons would be scaled. |
|
Just had a interesting discovery when editing an SVG in Inkscape -- the UI reports a node at |
|
@PeterShaggyNoble do you agree with this comment? If so I think we should start looking into a solution to fix/ignore the issues with existing icons 😄 |
|
Yup, I think a fixed precision of 3 or 4 should be OK. |
|
What are the opinions with regards to hardcoding the icons that currently generate an error and ignoring any errors they produce. This is the least work, we should just not forget to remove them from the list of hardcoded icons when they're updated 😅 Perhaps we can do this based on the exact path value (or full SVG) so that an update to one of the icons is certainly subject to this linting step? If someone is willing to manually resize over 290 icons they are of course welcome to 🙃 |
He says, knowing I'm just about crazy enough to take something like that on! 😆 I have been thinking of a full icon review lately! 🙈 |
Hahaha 🤣 It would be awesome if you could do it! (I would say this list takes priority over a "generic"/"general" icon review.) Still, unless you plan to do it in less than 1 week I don't think it hurts to implement a temporary solution to save ourselves some time reviewing new and existing PRs 😉 |
ericcornelissen
added
the
in discussion
|
I'll create a new "tracking" issue for them in a bit so we can get some help with them, work through them as we can. |
|
Okay 👍 @davidjb, are you interested in implementing some filtering mechanism for the existing icons, or would you prefer us to implement that? |
ericcornelissen
removed
the
in discussion
I think, for better future proofing and so that we don't have to remember to remove an icon from the list as we work through updating them, that this is the best option - if the path/SVG exactly matches one in the list then ignore it. If you let me know what format you need that list in, I can very quickly generate it from the list of names I've already done up. |
|
@ericcornelissen For the filtering, it sounds like you've got ideas in mind as to how you'd like to manage the errant icons so it seems simpler if you add a method which works best for you as maintainers. I can't think of anything better than @PeterShaggyNoble's comparison strategy, sort of perhaps using SHA hashes to simplify storage rather than the full string path. The simplest option is a JS object in the .svglint file with that data, annotated in some way (comments etc) of identifying which hash/path corresponds to which icon so you can know when to remove them. |
|
Would this format do the trick, @davidjb? |
That's certainly something worth considering 🤔 (using any hashing mechanism for that matter: SHA, MD5 or simpler)
I would propose putting them in a separate JSON file and loading it, but that's a small detail. I think commenting them would be unnecessary, when a particular SVG is updated no SVG should match the hash any more, and once #3169 is finished we can simply remove the entire JSON file (and the loading logic).
If we decide to go with SHA (or MD5, since I'm not worried about collisions in this dataset) hashes the values should be hashes though. Don't see why we would compute the hash every time the CI is ran. |
Aren't the additions in this PR already accessing the path's data?
Yup, it would definitely be easier but my (admittedly minor) concern would be that the #3169 review could take a good while and, in the meantime, we could in the future, for some reason we can't foresee now, decide to change the requirements for or formatting of the markup of our files - using the path alone gives us a little bit of extra future proofing.
Agreed. Should have said in that in my last comment: let me know which hashing function is easiest to work with that we want to use and I'll generate the complete file. |
Jup 😅 Nevermind my earlier comment
That is also a good point
I would prefer something that is super quick to compute, as we are going to be doing it at least 390 times every time |
|
Hmm ... which is the lesser of 2 evils here? The larger overhead involved in loading in a file with unhashed data or having to unhash all 290 paths for each build? |
|
Pushed the change as suggested by @ericcornelissen for the existing reporter, and also manually linted the .js file to add semicolons and use double quotes (muscle memory from other projects I'm working on). That should be it - let me know if there's any other changes required. |
This comment has been minimized.
This comment has been minimized.
Float precision is set at 3 which is the default for svgo in .svgo.yml; precision can be raised over time. This adds an ignore file with the current paths of non-conforming icons. This also changes the name of the icon title linter as well so it reads more nicely than "custom".
This comment has been minimized.
This comment has been minimized.
|
This all looks good to me - great work, @davidjb 👏 Thinking on it some more over the weekend, I think we should leave the precision at The only addition I can see that hasn't been made yet is this suggestion from @ericcornelissen:
|
I think I agree, but we can always review that at a later point in time 👍
Good call, forgot about that 😅 Do you have any ideas regarding this @PeterShaggyNoble? How about adding a sub-bullet to th Check the icon section's "Be scaled to fit the viewbox, while preserving the icon's original proportions." bullet stating something along the lines of "We check this up to 3 decimal points of precisions; as a result the optimization may affect this." |
|
I think that line is best placed in the "Optimize the Icon" section as that's the stage where this usually crops up. Maybe something along the lines of:
|
Add a note on visual imperfections and viewbox problems due to optimizing.
|
That's a good idea @PeterShaggyNoble, I took the liberty of adding it to this Pull Request (as well as rewriting it just slightly) 👌 |
Co-authored-by: Peter Noble <PeterShaggyNoble@users.noreply.github.com>
|
And with that I merge this Pull Request. Thanks so very much for your work on this @davidjb, this is a truly amazing contribution! 🎊 We will be forcing Pull Requests to be up-to-date with the base branch (i.e. |
|
Coming back around to this one: would we be better off checking on line 69 whether the width or the height is I also wonder if it would be possible to build on what was done here to check whether the path is centred on the canvas? Maybe within a precision of |
|
If I recall correctly, the 0x0 dimensions were the library reporting an invalid path - a quick glance at their source (https://github.com/icons8/svg-path-bounding-box/blob/master/lib/Path/BoundingBoxView.js#L10-L11) indicates that the dimensions start as NaN and get logical OR'd with 0. So both dimensions would be 0 in the case of an invalid path. As for checking if the path was centred, sounds like an idea. Calculate the centre of the path (e.g half way between min/max points in X & Y), round it off and compare that to the centre of the canvas (12x12). Sounded pretty quick since that data's already in the library...ahem.. #3250 |
This adds a tolerance of +/- 0.001 to either the X or Y dimensions, adjusting the existing ignore list and size linter to fit the new structure of the ignore file. See simple-icons#3107 (comment)
This adds a tolerance of +/- 0.001 to either the X or Y dimensions, adjusting the existing ignore list and size linter to fit the new structure of the ignore file. See simple-icons#3107 (comment)
This adds a tolerance of +/- 0.001 to either the X or Y dimensions, adjusting the existing ignore list and size linter to fit the new structure of the ignore file. See simple-icons#3107 (comment)
This adds a tolerance of +/- 0.001 to either the X or Y dimensions, adjusting the existing ignore list and size linter to fit the new structure of the ignore file. See #3107 (comment)
Issue: #3065 (comment)
Checklist
_data/simple-icons.jsonviewboxis0 0 24 24Description
This adds a custom linter to
svglintto pull out the path, run it through a bounding box checker and test that either the width or height is 24. The math checks to 3 decimal places, to avoid rounding errors in floating point comparisons (eg24vs24.000000000001), which follows how svgo does things and is configured in.svgo.yml.The library (https://github.com/icons8/svg-path-bounding-box) is admittedly faily old and may warrant review but it works well and is simple enough. Others I tried (eg
svg-path-boundschoked on icon paths, erroring on certain icons' curves). There are definitely others on NPM, including onesvg-path-bboxunder active development which I didn't try, and there is the source library wheresvg-path-bounding-boxgets its algorithm from beingcanvg, but the overhead to set up an object seemed excessive, adding even more dependencies. At any rate, I manually compared a dozen or so icons in the now-failing lint output to the path's dimensions visible in Inkscape and the bounds were exact, to the 3 decimal places Inkscape shows at least.Example lint output:
291 icons to fix 😢 but at least linting can catch it 😎