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

add possibility of specifying hsl colours to shield generation script #2000

Closed

Conversation

nebulon42
Copy link
Contributor

I wrote a small Python method to parse RGB e.g. rgb(150, 120, 130) or HSL e.g hsl(210, 20%, 50%) colour definitions. I think the shield generation script would benefit from it, since it makes playing with colours a lot easier.

If you don't think so too, please close this pull request.

P.S.: PR 2000, wow. :)

@nebulon42 nebulon42 force-pushed the shields-color-functions branch from f758642 to 3ade7bf Compare December 8, 2015 20:39
@pnorman
Copy link
Collaborator

pnorman commented Dec 8, 2015

The output of preprocessed code shouldn't really be edited by hand, so I don't see hsl or rgb has an advantage over hex.

@nebulon42
Copy link
Contributor Author

Others might want to use the script in a different way and I found hsl handy when experimenting. But as I said it's just a suggestion and I don't mind if you don't see the added value.

@pnorman
Copy link
Collaborator

pnorman commented Dec 8, 2015

if this PR is changing shield output, it should also contain changes to all the shields, right?

@nebulon42
Copy link
Contributor Author

Currently it does not change anything visually as any hex value will be returned without changes, see https://github.com/gravitystorm/openstreetmap-carto/pull/2000/files#diff-d6c0cb01945928a60a33f9441ed95cdeR136
Additionally, rgb and hsl are always converted to hex in the SVG.

@pnorman
Copy link
Collaborator

pnorman commented Dec 8, 2015

It's not a question of visual changes, it's a question of the files being up to date. If I run the script and copy/paste the output into the relevant files, git diff should show no changes.

@nebulon42
Copy link
Contributor Author

If I run the script and copy/paste the output into the relevant files, git diff should show no changes.

This is the case.

@pnorman
Copy link
Collaborator

pnorman commented Dec 10, 2015

To the script stroke_fill and such are just strings, and svg has HSL support

Can't you just do config['motorway']['fill'] = 'hsl(0,100%, 50%'?

@nebulon42
Copy link
Contributor Author

svg has HSL support

SVG 2 supports HSL, SVG 1.1 only supports RGB and Hex notation. I'm not sure how widespread SVG 2 support is, but I've tried this before and neither my built-in image preview program nor Inkscape where able to display this correctly. I doubt that Mapnik supports SVG 2, but I might be wrong. A quick search didn't bring something up.

@pnorman
Copy link
Collaborator

pnorman commented Dec 12, 2015

SVG 2 supports HSL, SVG 1.1 only supports RGB and Hex notation

Good point.

I'll have a closer look at the code, also I'm not sure how this best interacts with #1873

@nebulon42
Copy link
Contributor Author

I think I will withdraw this for now, because I want to enhance it with Lch. The function can always be found at https://github.com/gmgeo/osmic/blob/master/tools/export.py#L532-L558.

Another good thing would be to bring Lch to carto. @pnorman if you need help, let me know.

@nebulon42 nebulon42 closed this Dec 13, 2015
@nebulon42 nebulon42 deleted the shields-color-functions branch December 13, 2015 16:03
@matthijsmelissen
Copy link
Collaborator

Another good thing would be to bring Lch to carto

+1, is it supported in Mapnik?

@pnorman
Copy link
Collaborator

pnorman commented Dec 14, 2015

Another good thing would be to bring Lch to carto

+1, is it supported in Mapnik?

No, and doesn't need to be.

Carto takes colour functions and turns them into hex codes or rgba.

This is good because it lets us use new functions without a Mapnik upgrade, which is harder.

I'm working on some fairly complex carto changes for work, so I'm working on small changes to get familiar with the carto changes, and one of them is perceptual colourspaces (mapbox/carto#354)

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.

3 participants