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

Increase strength of tertiary/residential/unclassified casing [WIP] #2454

Closed

Conversation

matthijsmelissen
Copy link
Collaborator

@matthijsmelissen matthijsmelissen commented Nov 20, 2016

White roads (tertiary/residential/unclassified) should contrast sufficiently with features like landcover and buildings, with which they are frequently surrounded. To increase the contrast, this pull request makes the casing of these roads darker.

Resolves #2448 (this is a toned down version of the changes proposed there).

Before/after (note that Github/your browser might scale the images so best to open them individually and/or press ctrl+0):

12-old 12-new

13-old 13-new

14-old 14-new

15-old 15-new

16-old 16-new

@nebulon42
Copy link
Contributor

I have only looked at the examples here but I think the casing is too dark for its width on the first image. This leads to a jagged appearance. The second one is ok to me, could be lighter. On the last two I prefer lighter casing, could be a bit stronger than currently but not much.

I think the contrast is sufficient with residential and buildings. I'm not sure about other landcover, especially forest. Do you have another area in mind where you think the contrast is currently insufficient?

@matthijsmelissen
Copy link
Collaborator Author

the casing is too dark for its width on the first image

Are you sure you are watching the image on true size? I don't see this effect (except when watching downsized images)

@pnorman pnorman mentioned this pull request Nov 27, 2016
@nebulon42
Copy link
Contributor

Yes, I see this also with the full size image.
I think the previews that you show do not necessarily require a change. Do you have another area where this change would improve rendering significantly?

@kocio-pl
Copy link
Collaborator

kocio-pl commented Dec 6, 2016

I would also feel safer if we could see more different places.

@matthijsmelissen
Copy link
Collaborator Author

matthijsmelissen commented Dec 6, 2016

You can, just pull the branch :) No matter how many screenshots are posted, the best impression you always get by just browsing an area of the map you're familiar with.

@kocio-pl
Copy link
Collaborator

kocio-pl commented Dec 7, 2016

I'm not sure if it looks more readable on the farmland:
z12
Before
nalknb6e
After
9rgyyxtr

@pnorman
Copy link
Collaborator

pnorman commented Dec 8, 2016

I haven't checked it out locally yet, but I find the first two pairs of images about the same in readability, the next two better, and the last one slightly better.

@kocio-pl
Copy link
Collaborator

@nebulon42 Do you have any colors on your mind (proposed width seems OK to me)? It would make testing and deciding easier.

@matthijsmelissen
Copy link
Collaborator Author

I currently don't have time to work on this, but I'll get back to it when I have time again.

@pnorman
Copy link
Collaborator

pnorman commented Dec 12, 2016

I checked it out locally. I'm positive overall and would be fine with merging, but have a few comments

  • z14 to z15 seems a big big of a jump in casing strength
  • z17+ seems to be a big improvement

The biggest problem seems to be AA. Perhaps line-gamma can be adjusted, or the line made wider but the casing faded out.

@matthijsmelissen
Copy link
Collaborator Author

Will look at it.

@matthijsmelissen matthijsmelissen changed the title Increase strength of tertiary/residential/unclassified casing Increase strength of tertiary/residential/unclassified casing [WIP] Dec 12, 2016
@pnorman
Copy link
Collaborator

pnorman commented May 10, 2017

As this has cartography changes, this PR is currently held up by the feature freeze until 4.0.0 has been released.

@kocio-pl
Copy link
Collaborator

What is the current status of this code?

@matthijsmelissen
Copy link
Collaborator Author

I need to test if it still makes sense after the midzoom changes.

@matkoniecz
Copy link
Contributor

Sorry for not reviewing it earlier.

Are you planning to retest and rebase this PR?

@matthijsmelissen
Copy link
Collaborator Author

Are you planning to retest and rebase this PR?

Yes. It's not waiting for reviews, but for work on my side.

@ghost
Copy link

ghost commented May 22, 2018

I prefer before images xD

@kocio-pl
Copy link
Collaborator

@matthijsmelissen What is your current plan with this code?

@matkoniecz
Copy link
Contributor

I think that it would be better to officialy close it for now. @matthijsmelissen Please send a new PR in case of a restarted work!

@matkoniecz matkoniecz closed this Dec 7, 2018
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