HatScripts / circle-flags

A collection of 400+ minimal circular SVG country, state and language flags
https://hatscripts.github.io/circle-flags
MIT License
945 stars 249 forks source link

Update svgo.yml and disable problematic arcto optimization #4

Closed waldyrious closed 4 years ago

waldyrious commented 4 years ago

Recent versions of svgo gained the ability to compress svg files further, in particular by removing spaces in some sections of path data. See commit https://github.com/svg/svgo/commit/258fecfa6b4a037d98ad2614f5c9573728f6959d (first included in release v1.3.0 of svgo) for more details.

HatScripts commented 4 years ago

Thank you once again for your contributions! 👍

The reason I was hesitant to update to newer versions of svgo is that it was causing the SVG files' thumbnails to display incorrectly. I am using Windows 10 and tibold's svg-see (formerly svg-explorer-extension).

Here is how these icons re-optimized with svgo 1.3.2 look to me in explorer: 2020-06-10 05 26 15

And here is how the base branch icons look: 2020-06-10 05 15 28

There is already a bug report on the svg-see issues page from back in January regarding this, and the OP appears to have linked the cause of the bug to way the zeros are combined that you mentioned in #3.

HatScripts commented 4 years ago

So as of right now I'm not sure what the best course of action is. The latest release of svg-explorer-extension is only 9 days old, and that's what I'm currently using, but even with that I still have this issue. Maybe @User756675678's regex hotfix was never applied... Any suggestions?

waldyrious commented 4 years ago

Well, that certainly seems to be a bug in svg-see 🤔. The files display correctly in all the platforms I tried: chrome, firefox, safari, inkscape, macOS preview, vscode markdown preview. We could certainly require an older version of svgo, but that would make things more cumbersome for contributors only to accommodate a bug in a program that is no doubt useful, but not required for this project.

That said, I'm happy to hold off this PR for a while if you think there's a good chance svg-see will integrate the fix sometime soon 👍.

waldyrious commented 4 years ago

(As a side note, happy to see from the screenshot that you have some uncommitted/unpushed local changes! Looking forward to see them in the repo :))

HatScripts commented 4 years ago

Well, that certainly seems to be a bug in svg-see 🤔. The files display correctly in all the platforms I tried: chrome, firefox, safari, inkscape, macOS preview, vscode markdown preview.

I'll admit that originally I hadn't updated svgo for lazy/selfish reasons. But then... after you brought this further to my attention I found that issue on svg-see where the OP and project owner seem to agree that it isn't just a bug in svg-see, it's a bug inherited from qt, which across its entire codebase I think it's safe to estimate it's used by thousands of applications. I'm normally all for optimising filesize to the very last byte, but in this instance it seems to come at the cost of breaking the appearance of the icons in some apps. Quality over quantity, right?

We could certainly require an older version of svgo, but that would make things more cumbersome for contributors only to accommodate a bug in a program that is no doubt useful, but not required for this project.

You are right about updating svgo to the latest version. That would both benefit contributors as well as filesizes.

That said, I'm happy to hold off this PR for a while if you think there's a good chance svg-see will integrate the fix sometime soon 👍.

Until qt is able to correctly render the svgs is there possibly an option we can add to the svgo.yml to disable the 0 0 0 -> 000 feature? Would that be a fair compromise for now?

HatScripts commented 4 years ago

Unless of course it's already fixed in qt, in which case you're right that it must be an svg-see bug.

waldyrious commented 4 years ago

is there possibly an option we can add to the svgo.yml to disable the 0 0 0 -> 000 feature? Would that be a fair compromise for now?

It would, IMO, but I'm afraid there's no such option — at least from my reading of the commit that introduced the change.

Unless of course it's already fixed in qt, in which case you're right that it must be an svg-see bug.

I'm not aware of an easy way to test that. It's been too long since I worked with Qt.

That said, although there are indeed many applications built with Qt, I wouldn't assume it would affect that many downstream users, considering that all the other rendering engines I tested (including major browsers, which I'd expect would be the main platform where these images would be consumed) seem to cope with these just fine.

Maybe another idea I was planning on proposing in a separate issue could provide an alternative solution to this: what do you think of keeping both human-readable SVG files in a src/ directory, and the optimized output in a dist/ directory. That way you could continue to use svg-see for your work in this project, and we'd treat the contents of dist/ as a black box. And even if users happen to be affected by this rendering issue, they could just use the source files instead, which wouldn't be that bad as they're not that large anyway. Would that work for you?

waldyrious commented 4 years ago

Glad to hear you're working on a gallery page yourself! Would you mind moving your comment to #5 so we could discuss there and avoid polluting this thread?

HatScripts commented 4 years ago

It would, IMO, but I'm afraid there's no such option — at least from my reading of the commit that introduced the change.

Hmm, I see, that's rather disappointing. 🤔

Maybe another idea I was planning on proposing in a separate issue could provide an alternative solution to this: what do you think of keeping both human-readable SVG files in a src/ directory, and the optimized output in a dist/ directory. That way you could continue to use svg-see for your work in this project, and we'd treat the contents of dist/ as a black box. And even if users happen to be affected by this rendering issue, they could just use the source files instead, which wouldn't be that bad as they're not that large anyway. Would that work for you?

When you say human-readable SVG do you mean the direct output of Inkscape/Illustrator without any minification/optimisation applied by svgo? Are there other benefits to only working on the src directory? I personally chose to have my src/dist folders be one in the same so that I knew what I was seeing within Inkscape is what the end user would be seeing.

waldyrious commented 4 years ago

When you say human-readable SVG do you mean the direct output of Inkscape/Illustrator without any minification/optimisation applied by svgo?

Not exactly :) I meant something closer to Inkscape's "plain svg" output, with some cleanup from svgo on top, but not as aggressive. For example, keeping the pretty printing, relevant IDs for paths and groups, and keeping actual geometric shapes rather than converting everything to paths.

Are there other benefits to only working on the src directory?

Well, call me old-fashioned, but I actually enjoy hand-editing (and hand-optimizing) SVG files. Having them be human-readable would allow quick fixes to be made much more confidently and conveniently (maybe even by editing the file in the GitHub interface!), thus attracting contributors. Also, since they're text files anyway, changes such as those I did in #1 and #2 would be understandable from the git diff, and git blame also gets more useful as not all changes edit the same very long line. 😄

I personally chose to have my src/dist folders be one in the same so that I knew what I was seeing within Inkscape is what the end user would be seeing.

I understand, but I have been following svgo for long enough to trust that they only make non-destructive changes to the files (bugs in renderers notwithstanding).

waldyrious commented 4 years ago

Update: apparently there is a way to disable this optimization! A comment in svgo#1137 shows how this can be configured.

I can edit svgo.yaml accordingly, and re-do the optimzation in this PR, but I'd like to hear your thoughts about the question I asked in #3 before doing so.

HatScripts commented 4 years ago

Update: apparently there is a way to disable this optimization! A comment in svgo#1137 shows how this can be configured.

I can edit svgo.yaml accordingly, and re-do the optimzation in this PR, but I'd like to hear your thoughts about the question I asked in #3 before doing so.

Oh that's great, nice find!

As for the question you asked in #3, I'm not entirely sure why I decided to set options in svgo.yml that are already the svgo default.

Perhaps as a precaution for the defaults ever changing between svgo versions (seems unlikely), or perhaps so contributors who don't know the svgo defaults off by heart can still see at a glance what svgo is set to do.

I'm happy for you to remove the duplicates, but maybe we could add some sort of comment to the top of svgo.yml

# Default options: https://github.com/svg/svgo#what-it-can-do
waldyrious commented 4 years ago

I'm happy for you to remove the duplicates, but maybe we could add some sort of comment to the top of svgo.yml

# Default options: https://github.com/svg/svgo#what-it-can-do

Sure, will do. If the goal was to document svgo's default behavior, it would make sense to add all the options, not a subset as we currently have — but that would be even harder to keep track of, as new options are added to svgo or their default status is (for some reason) changed.

waldyrious commented 4 years ago

Alright, I've rebased the branch and done the changes discussed above. Turns out if we disable that particular optimization, the current output matches the one produced by the latest svgo, so there's no longer a huge diff touching all flag files! However, the flag of Cabo Verde still had the optimizations which I did as part of #1 and #2, so I manually reverted the 0 0 0000 changes so that it matches the rest of the files. Let me just edit the title of the PR and it should be ready to merge.

HatScripts commented 4 years ago

Fantastic work as always, Waldir. Merged! 😃

waldyrious commented 4 years ago

@HatScripts I'd still like to pursue the src/dist split proposed above. Is that something you'd be willing to consider? I can open an issue to track that.