-
Notifications
You must be signed in to change notification settings - Fork 2.4k
update icon: llvm (plain, line) #2264
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
Conversation
b7eb814
to
526781b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmmm, I don't agree with this one. While I agree that the icon could benefit from some simplification I think going all black is too much simplification, making it less recognizable... but let's see what the other maintainers think too :)
Edit:
Good that you fixed the artifact though. That should probably be fixed in the original icon too.
Some context: The original icon is made by converting a png to svg with inkscape. The process is not ideal so there are a few artifacts. I think it could be improved upon now though, since I know there are better png to svg converters out there now than when we made this icon :)
😆 As I said, controversial (see image below). For me, as "in a font" the current plain one is just too intricate. Have a look at it sized down: But that is just for ME and font usage, which might or might not be important for you. |
@Finii |
Sidenote: The old |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great work 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe the suggested version is close enough to the original. Most of the lines should be kept and just made mono colored imo. I added a suggestion as to how it could look in a previous comment, I think if some of the nodes are removed on that version we'll have both a pretty nice looking and optimized plain version
@Finii What do you think about my suggested version? |
Ok, if you explicitly ask me ;-) At first glance the icon looked two color, but in fact it is monochrome. The outline of the outline 😬 is rather thin in some places, down to non-existing. So the impression is that the transparent 'line' of the outside is line-art style, but the actual outside 'line' is not line-art anymore and just a suggestion. This is not quite what I would call 'simple'. If we compare to the original logo material (https://llvm.org/img/LLVMWyvernBig.png) you turned the actual outline (originally black) into transparent and filled the internal areas; but to keep the line characteristic on a transparent backdrop you added this small suggestion of a outline-outline. I understand the reasons for the extra outline and also for its slimness, but I wonder if there is a better solution to the problem to transform the two-color-on-transparent filled-area line style into a one-color line style. One solution could be to non-uniformely use the original outline, sometimes as line-art, sometimes just combine it with the area (to keep the original apprearance), so that the outside of the logo is not line art, but only the inside is drawn with a line-art pen. Hmm, if that makes sense. So just considering the black line in the original icon, it is converted into a white/transparent line if it is surrounded by filled areas on both sides, but if it is only neighbored on one side by an area and on the other side there is 'the nothing' it gets combined to the area. Also interesting is the approach taken by the Clang Powertools to get a monochrome icon from the old dragon: Original This is a somewhat loose interpretation, note the neck, and the number of spikes everywhere. Whatever. Back to the llvm icon, I have the feeling that your suggested one is too intricate, and the thinner than thin outline - which is needed to keep the outer line a line when the line is transmogrified into 'transparent' - can even be hard to render small. I'm not sure.
Maybe I can find some time to spare on this today. |
Already in this very fast threshold-to-black-and-white image one can glimpse that the outlying lines are thinner (and should than vanish by combining with the inner areas) while the inside lines really appear more fat more substantial and would be converted to white/transparent lines in a monochrome version. 🤔 Edit: The outside line really appears often to be just 1/2 the width of the inside lines Arrows on outside line |
The design problem reminds me of that one spiky fish icon, that I solved in a comparable manner, lets see if I can find it... Ah, it was OpenBSD, here: lukas-w/font-logos#93, but apparently did not materialize? |
Something like this, this is a rough sketch needing cleanup: (This is a combination of the complete-outline and just the inner line-art lines: More detailed look at the problems, I produced them with gimp and imported individually into inkscape... Edit edit edit ... ;-) And here the Your version for comparison: ¯\_(ツ)_/¯ Tell me what to do :-D Size-wise not much different: |
@Finii damn you're good! There are some places where the transparent lines are inset (in line with the black outline) instead of being outside the black outline. And also some places where the transparent line doesn't reach the outline of the dragon. See this example: Notice how the transparent lines at the bottom kinda cut into the thicker part of the tail. I think it would look better if the transparent lines here were shifted outwards a bit so it instead cut through the thinner parts of the tail. A rough sketch of what I suggest is something like this: As for the top left red arrow, you can see that the transparent line doesn't fully reach the edge of the dragon. There are more places where this is the case, so this is just an example. Another thing I think looks a bit weird is the tip of the fingers on the wings, like here: I get the idea of adding a thicker part to make the tips visible at smaller sizes, but I think it's too detrimental to the detail to be worth it. Looking at your version did give me an idea though. So kinda reverse of what it is now. That way the tips of the wings should be visible at smaller sizes while still making the larger sizes look detailed. I think overall it just needs cleaner lines and outlining of the wings instead of filling them :) It's very difficult to do this changes when the path is unified, so it would be nice if you uploaded the ungrouped/un-unified version too next time :) |
I'm personally very happy with the last suggestion, but I haven't looked very closely at the result, so let me know if you find any flaws |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line version needs to be added to the font. Let's also add the color as fill to the line and plain versions :)
#5A90B6
Hi there, I'm Devicons' Peek Bot and I just peeked at the icons that you wanted to add using icomoon.io. Here are the SVGs as intepreted by Icomoon when we upload the files: Here are the zoomed-in screenshots of the added icons as SVGs: Here are the icons that will be generated by Icomoon: Here are the zoomed-in screenshots of the added icons as icons: Here are the colored versions: The maintainers will now check for:
In case of font issues, it might be caused by Icomoon not accepting strokes in the SVGs. Check this doc for more details and fix the issues as instructed by Icomoon and update this PR once you are done. Thank you for contributing to Devicon! I hope that your icons are accepted into the repository. Note: If the images don't show up, it has been autodeleted by Imgur after 6 months due to our API choice. Cheers, |
In the discussion of PR devicons#2264 @Snailedlt developed a line version of the icon. As the original icon it had thinner outside lines than lines within the icon. This has been fixed by me through Selection->Grow in Gimp that I applied only on the outside of the icon. Authored-by: Jørgen Kalsnes Hagen @Snailedlt Co-authored-by: Fini Jastrow <ulf.fini.jastrow@desy.de> Signed-off-by: Fini Jastrow <ulf.fini.jastrow@desy.de>
ef801ca
to
65964f5
Compare
Hi! I'm the
Check our CONTRIBUTING guide for more details regarding these errors. Please address these issues. When you update this PR, I will check your SVGs again. Thanks for your help, |
According to the specs having a unspecified fill color is ... expected? 🤔 Edit: Of course if you say the color is expected I change the svgs, thats easy. I just wonder, it somewhat contradicts with how I read the contrib info; and then, maybe the contrib info needs to be changed also. |
It's more of a convention we've used since it makes it easier for some third-party tools. It's not really needed for our internal workflow. I don't even remember why we re-added the fill for many icons at this point 😅 |
[why] While the plain icon is plainer than the original icon, it is still quite complex with very thin strokes. [how] In the discussion of PR devicons#2264 @Snailedlt developed a more detailed plain version of the icon. Taking that I just removed a three-node smudge at the dragon's left hand and did the Break-Apart & Union dance in Inkscape to clean the paths up. Authored-by: Jørgen Kalsnes Hagen @Snailedlt Co-authored-by: Fini Jastrow <ulf.fini.jastrow@desy.de> Signed-off-by: Fini Jastrow <ulf.fini.jastrow@desy.de>
In the discussion of PR devicons#2264 @Snailedlt developed a line version of the icon. As the original icon it had thinner outside lines than lines within the icon. This has been fixed by me through Selection->Grow in Gimp that I applied only on the outside of the icon. Authored-by: Jørgen Kalsnes Hagen @Snailedlt Co-authored-by: Fini Jastrow <ulf.fini.jastrow@desy.de> Signed-off-by: Fini Jastrow <ulf.fini.jastrow@desy.de>
65964f5
to
f30c820
Compare
Hi! I'm the
Check our CONTRIBUTING guide for more details regarding these errors. Please address these issues. When you update this PR, I will check your SVGs again. Thanks for your help, |
Dear SVG Checker Bot, it would probably be more helpful if you hint how to sort the $ jq --slurp --indent 4 '.[] | sort_by(.name)' < devicon.json > devicon_sorted.json
$ mv devicon_sorted.json devicon.json I will add a commit with the result of this. Lets see if you are content then. |
It seems not only that the current devicons.json is not sorted, it also has un-unified indent levels and other 'defects' obviously from manual manipulation without ever running it through a unifying process. Which is of course not needed, but maybe it's more nice to have all indents identical etc pp. The commands used to create this commit: jq -s --indent 4 '.[] | sort_by(.name)' < devicon.json > A mv A devicons.json Signed-off-by: Fini Jastrow <ulf.fini.jastrow@desy.de>
Hi there, I'm Devicons' Peek Bot and I just peeked at the icons that you wanted to add using icomoon.io. Here are the SVGs as intepreted by Icomoon when we upload the files: Here are the zoomed-in screenshots of the added icons as SVGs: Here are the icons that will be generated by Icomoon: Here are the zoomed-in screenshots of the added icons as icons: Here are the colored versions: The maintainers will now check for:
In case of font issues, it might be caused by Icomoon not accepting strokes in the SVGs. Check this doc for more details and fix the issues as instructed by Icomoon and update this PR once you are done. Thank you for contributing to Devicon! I hope that your icons are accepted into the repository. Note: If the images don't show up, it has been autodeleted by Imgur after 6 months due to our API choice. Cheers, |
Hi there, I'm Devicons' Peek Bot and I just peeked at the icons that you wanted to add using icomoon.io. Here are the SVGs as intepreted by Icomoon when we upload the files: Here are the zoomed-in screenshots of the added icons as SVGs: Here are the icons that will be generated by Icomoon: Here are the zoomed-in screenshots of the added icons as icons: Here are the colored versions: The maintainers will now check for:
In case of font issues, it might be caused by Icomoon not accepting strokes in the SVGs. Check this doc for more details and fix the issues as instructed by Icomoon and update this PR once you are done. Thank you for contributing to Devicon! I hope that your icons are accepted into the repository. Note: If the images don't show up, it has been autodeleted by Imgur after 6 months due to our API choice. Cheers, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! ✔️
Waiting for another review, since this PR also modifies the devicon.json order, which I think should be fine, but I'm not sure if it'll cause issues with the build bot.
@ReenigneArcher @canaleal
One of you should review this PR as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The order of the json file shouldn't hurt anything, and since everything is supposed to be alphabetical I think this is a good change.
In the future I want to add a PR check that makes sure the json file is properly formatted and properly sorted which can run for every PR.
For the icons, any idea how to fix this? The viewbox is the right size, but it displays as the wrong size.
Approving since both of the things I discussed would be future improvements.
@ReenigneArcher yeah, adding a check for formatting and sorting would be nice! I'm not sure why you have that sizing issue, in my GitHub preview it looks fine: |
@canaleal I know you already started the release process, but do you think we can merge this as well, since there were some issues with the font? |
[why]
While the plain icon is plainer than the original icon, it is still quite complex with very thin strokes.
[how]
Just take the outermost outlines of the plain icon and crate a solid icon from it.
Note
This can be controversial.
For me the plain icon is not really plain, it has lots and lots of detail and consists of 2600 nodes:
The simple plain icon proposed here drops all the details and uses just the outline (260 nodes)
If one needs an icon with all the glory details the
-original
should be a good fit, the plain should be a simple(r) version, I believe. But anyhow, this is very opinionated and I do not want to mess with your ideas here. For a small text icon the current plain icon is far too complex and all the small detail hairlines can not be seen anyhow.Double check these details before you open a PR
Features
This PR closes NONE
Notes
@Snailedlt as per ryanoasis/nerd-fonts#1691 (comment)