gravitystorm / openstreetmap-carto

A general-purpose OpenStreetMap mapnik style, in CartoCSS
Other
1.54k stars 823 forks source link

Stream is barely visible on scrub #2098

Closed sommerluk closed 5 years ago

sommerluk commented 8 years ago

Stream is barely visible on scrub.

The waterway=stream is barely visible on the natural=scrub, especially when combined with intermittent=yes. Screenshot from http://www.openstreetmap.org/#map=19/5.34678/-3.98195 screenshot

Not sure what would be a good solution for this, if any…

jeisenbe commented 5 years ago

The new pattern uses a different color, to match the new scrub background color. It is supposed to be similar to the current pattern otherwise On Fri, Nov 23, 2018 at 8:33 PM Adamant36 notifications@github.com wrote:

Adamant36: I like the new pattern. Whats the advantage over the original though? Maybe the same thing could be done for the forest pattern also at some point.

jeisenbe commented 5 years ago

This was the command used to generate the new pattern with jsdotpattern:

http://www.imagico.de/map/jsdotpattern.php#x,512,jdp97762;g5,45,64,64;rx,250,2,64,64;rx,250,2,64,64;rx,250,2,64,64;rd,1,0,0,scrub2,1,5,12,0,jdp3379,8ece8f,b5e3b5;

Compare to the wood/forest pattern:

http://www.imagico.de/map/jsdotpattern.php#x,256,jdp6894;g,30,32,32;s,jdp33742;s,jdp81637;rx,250,2,32,32;s,jdp28824;s,jdp59702;s,jdp91550;s,jdp27774;rx,250,2,64,64;rd,1,0,0,tree%20pair,1,5,5,0,jdp52898,6b8d5e,add19e;

Actually it looks like the old pattern was only semi-random; this new pattern is more random, and perhaps the icons are a few pixels closer together on average.

I'm thinking of submitting a PR for just this change, and leaving some more time to work on heath and grass later, because this change looks good and resolves this issue.

Adamant36 commented 5 years ago

I was thinking the new scrub pattern was random the old forest pattern. It might be worth randomizing the forest pattern more at some point.

As far as the PR goes, you should do it for just this change. Heath is a different issue anyway. Your going to do it with scrub as %3 darker then heath (at #d1e0b4) right?

jeisenbe commented 5 years ago

Area in Hawaii with streams, intermittent streams on scrub near golf course:

Scrub background color #c8d7ab with scrub pattern in #b0be93: z15 new-scrub-z15-hawaii z16 new-scrub-hawaii-z16

Tomasz-W commented 5 years ago

As new scrub colour is based on a new heath colour, it's obvious for me that we should finish heath first, then a change for scrub would be easier (propably something like darken(@heath, 3%)

jeisenbe commented 5 years ago

The two landcovers should get their own color in the code, to make it easier to maintain and adapt in the future.

This issue is about making intermittent waterways more visible on scrub, with a better-looking scrub rendering as a secondary goal, so I think it can go first, unless we can all agree on heath soon. On Sat, Nov 24, 2018 at 5:35 PM Tomasz Wójcik notifications@github.com wrote:

As new scrub colour is based on a new heath colour, it's obvious for me that we should finish heath first, then a change for scrub would be easier (propably something like darken(@heath, 3%)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/gravitystorm/openstreetmap-carto/issues/2098#issuecomment-441352372, or mute the thread https://github.com/notifications/unsubscribe-auth/AoxshKDW2qkP9KiJYcOHNVyJhfs3DK3Hks5uyQTogaJpZM4H5bx3 .

Adamant36 commented 5 years ago

I'm not exactly sure what the pros and cons of using a variable to represent a color is, but I know they are used a lot in the landcover.mss file. So I don't why it would be an issue. Unless its particular to this case for some reason. That being said, if you can figure out the exact hex value for darken(@heath, 3%) then that's fine.

Also, it then has the disadvantage of being harder to tweak in small amounts if need be. While most people would know what the difference between darken(@heath, 3%) and darken(@heath, 2.5) might be its much harder and takes more work to interpret and tweak a hex value with by using the lch whatever thing or a color slider in a photo editing app (where you might not be able to do it with that fine of detail). Which is probably why the darken thing is used.

As I said though, as long its the exact same color, it doesn't really matter how its represented in there. Although, thinking about it, I remember a couple of issues where the hex value was used instead of the darken variable and the maintainers said to use the darken variable instead because it shows how the color was obtained. So....I think there's definitely more of a slant toward using the variable when it can be. Plus, there's plenty in the contributing file about how code should be consistent and use already existing standards. Which the variable thing clearly is. Even if you might personally prefer it the other way.

Tomasz-W commented 5 years ago

#d0e3b6+ 10% vertical dash pattern for heath + #c8d6ab for scrub

scrub1 scrub2 scrub3 scrub4 scrub5

jeisenbe commented 5 years ago

Ok, it looks like we agree that #c8d6ab will work for scrub. I can make a PR for this change alone, to resolve this issue. The we can work on heath later. On Sun, Nov 25, 2018 at 6:13 PM Tomasz Wójcik notifications@github.com wrote:

d0e3b6+ 10% vertical dash pattern for heath + #c8d6ab for scrub

[image: scrub1] https://user-images.githubusercontent.com/25656654/48977451-a04f9500-f09a-11e8-8d69-cd888cec69f5.jpg [image: scrub2] https://user-images.githubusercontent.com/25656654/48977452-a04f9500-f09a-11e8-889a-edc7351ade1c.jpg [image: scrub3] https://user-images.githubusercontent.com/25656654/48977453-a04f9500-f09a-11e8-93e1-fc99233ef30a.jpg [image: scrub4] https://user-images.githubusercontent.com/25656654/48977454-a04f9500-f09a-11e8-83ae-e17dcc3f89dd.jpg [image: scrub5] https://user-images.githubusercontent.com/25656654/48977455-a0e82b80-f09a-11e8-9bd4-55eac2cf7e85.jpg

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/gravitystorm/openstreetmap-carto/issues/2098#issuecomment-441426184, or mute the thread https://github.com/notifications/unsubscribe-auth/AoxshF3ddXnzELmsb-0w1nRhjHcWjwzWks5uyl9BgaJpZM4H5bx3 .

Adamant36 commented 5 years ago

Sounds good to me.