Skip to content
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

Replacing non-vector icons with SVG #2754

Merged
merged 3 commits into from
Aug 19, 2017
Merged

Conversation

giggls
Copy link

@giggls giggls commented Aug 15, 2017

As #2752 got accidentally closed here is a new pull request.

@kocio-pl
Copy link
Collaborator

kocio-pl commented Aug 15, 2017

You should update this code to resolve merging conflict (color values). You can also split the code, so both issues would be separated.

@giggls
Copy link
Author

giggls commented Aug 16, 2017

I do not consider those two different things. Both changes are required to make the @transportation-text color changeable.

@giggls
Copy link
Author

giggls commented Aug 16, 2017

OK, I once again updated my upstream branch holding the pull request. There are two commits now. I still consider them being one change though.

@imagico
Copy link
Collaborator

imagico commented Aug 16, 2017

I did not have the opportunity to actually test the change yet but it makes a lot of sense - the icon is modeled after wilderness_hut wich is in a bit of styling discrepancy with shelter and the old alpine_hut but that is a pre-existing problem.

@giggls
Copy link
Author

giggls commented Aug 16, 2017

I just figured out, that I need to draw more replacement icons.

I will update at least the ones rendereed in transportation-text color.

However I think we should get rid of all the remaining png icons. There are seven including alpine hut:
bus_station.n.16.png
chalet.p.16.png
guest_house.p.16.png
butcher.png
miniature_golf.p.20.png
golf.p.20.png

@kocio-pl
Copy link
Collaborator

I did not have the opportunity to actually test the change yet but it makes a lot of sense

What change are you talking about (other than recreating PNG image in SVG)?

@imagico
Copy link
Collaborator

imagico commented Aug 16, 2017

I am talking about the changes of this PR - as a general policy i will not approve changes i have not tested. But the icon and the idea of replacing hardcoded colors with a symbol are both fine IMO.

@matthijsmelissen
Copy link
Collaborator

However I think we should get rid of all the remaining png icons

Yes, that would be very welcome. Especially now we're so close to completion.

@kocio-pl kocio-pl changed the title Change rendering of alpine Hut to vector icon second try Replacing non-vector icons to SVG Aug 16, 2017
@giggls
Copy link
Author

giggls commented Aug 16, 2017

OK, here are the remaining vector replacements (old,new):

miniature_golf-oldminiature_golf-new
golf-oldgolf-new
chalet guesthouse-oldchalet guesthouse-new
bus_station-oldbus_station-new
butcher-oldbutcher-new

Differences from the style guide are as follows:
Golf and miniature golf icons are 20x20 instead of 14px to fit the original icon size.
guest_house uses an alpha-channel on the background house to closely resemble the original icon.

@kocio-pl kocio-pl changed the title Replacing non-vector icons to SVG Replacing non-vector icons with SVG Aug 16, 2017
@matthijsmelissen
Copy link
Collaborator

Nice! The chalet and guesthouse icon are (both in the old and new version) not really recognizable to me though.

@giggls
Copy link
Author

giggls commented Aug 16, 2017

Frankly I do not know the meaning of the triangle in the guest-house icon. I just traced it.

@kocio-pl
Copy link
Collaborator

Wow, really great work at very impressive speed! I was thinking about recreating bus station for months...

Please check that every icon:

  • is saved as a plain SVG instead of Inkscape SVG
  • has all the paths merged (Path>Union, if I remember correctly)

Problems I see:

  • mini golf shouldn't have lighter shade in it
  • guest house background is too strong
  • triangle in chalet should have light filling

@giggls giggls force-pushed the upstream branch 3 times, most recently from fd3a438 to cd6d350 Compare August 16, 2017 13:56
@giggls
Copy link
Author

giggls commented Aug 16, 2017

Paths can not be unionized if one of them has an alpha channel. However Mapnik seems to render them fine anyway. The lighter shade in the Minigolf Icon resembles the original icon as with guest house.

I fiddled a bit more with guest house and chalet alpha channels:
chalet guesthouse-oldchalet guesthouse-new

@sommerluk
Copy link
Collaborator

Thanks for this PR and your work!

@kocio-pl
Copy link
Collaborator

More problems I see:

  • Both golf icons are darker, is it the same green as in raster icons?
  • In chalet icon house shape seems to be too high
  • Bus station icon should have white background instead of being translucent:
    yg8ye2et

@giggls
Copy link
Author

giggls commented Aug 17, 2017

  • The golf icons are not darker in themselves. It's just the same green as used for its name in the style. Apparently the text has been darker than the raster icons. Thus the difference. As with the other vector icons the color comes from the style not from the icon itself.
  • I adjusted the house sizes to be the same in Both icons which is the reason why it is a little bit higher than in the raster icon. I found it somewhat more logical for the houses to be the same size.
  • AFAIK there is no single vector icon with a white background. All of them are monochrome.

@giggls
Copy link
Author

giggls commented Aug 17, 2017

BTW three out of six icons I made up are just taken more or less verbatim from Bryan Quinion icon collection which has been in use for Mapnik as long as I remember and where the raster versions obviously also originated from.

So my question is: What is more important, the fact that the icons are looking exactly like the old ones or the consistency compared to other vector icons?

@kocio-pl
Copy link
Collaborator

kocio-pl commented Aug 17, 2017

For me personally the most important thing is that the icon should be easy to recognize, but it's nice if it's close to what we already know and is consistent with the rest of the icons we use in this style.

I have no problem with most of your decisions beside white background for bus station, because it looks bad for me without a solid white and is hard to be recognized on busy background, like the one I have shown.

@giggls
Copy link
Author

giggls commented Aug 17, 2017

Unfortunately as far as I understand carto this will not work this way. All svg object colors are replaced by the color given inside the style file thus it is not possible to use multiple colors.in an icon colored by Mapnik. The only way I know of would be to use the original colors from the svg itself which I would at least consider somewhat ugly because this way the colors would again come from the svg instead of the style. Maybe it is also possible to disable opacity in carto-css. I will have a look at this tomorrow,

@matthijsmelissen
Copy link
Collaborator

That's true if we define the color in the stylefile, but im not sure if were still doing that?

@kocio-pl
Copy link
Collaborator

As far as I know we use stylefile only for icons, for pattern symbols we have to use colored images. Changing color in SVG file is still easy to do text edit (from technical point of view it's pure code) and we have to live with different Mapnik limitations, while icon which is hard to read is much worse problem.

@giggls
Copy link
Author

giggls commented Aug 18, 2017

OK, changed the pull request. bus_station.svg is now rendered as_is without color-fill from style itself.
bus_station

@kocio-pl
Copy link
Collaborator

Thanks, now it looks good:
iuqlsc4z

Meanwhile I've been looking at chalet and I think that triangle is a symbol of mountains and I would make the house icon smaller, because typical Swiss chalet is low building.

@giggls
Copy link
Author

giggls commented Aug 18, 2017

The best way to piss off people willing to contribute is fiddling on very small details nobody will ever notice for days :(

I did one final change for my pull request now and I am definitely unwilling to do more.

chalet guesthouse-oldchalet

So take it or leave it.

Sven

@kocio-pl
Copy link
Collaborator

Sorry for this. I don't like to make review longer than necessary, but we try to talk about all details beforehand, because they will not be touched for years probably. I'm happy with the results, so for me it's "take it". Thanks for a design and for tuning it!

@kocio-pl kocio-pl merged commit da94578 into gravitystorm:master Aug 19, 2017
@kocio-pl
Copy link
Collaborator

Thanks once again!

@sommerluk
Copy link
Collaborator

@giggls Thanks a lot!

@nebulon42 nebulon42 mentioned this pull request Aug 19, 2017
66 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants