-
Notifications
You must be signed in to change notification settings - Fork 825
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
Reduce landcover fading at mid-low zoom levels #3952
Conversation
Remove built-up-z11 color Less fade - 0.2 lighten instead of 0.4
I am not really fond of this idea of a compromise between fading and not fading colors. Most of the reasons why fading is a problem, in particular the problem of consistent design across zoom levels, do not vanish if you reduce the amount of fading. But if this change helps weaning us off the fading idea i would not mind. As i explained in #3670 (comment) i think a consensus to fully get rid of fading would be a prerequisite for #3670 because both ideas represent opposing strategies towards low zoom map design. |
This is meant as an intermediate step, since entirely removing the fading would be a much stronger change. I still intend to advocate for merging #3670 later: since we already combine urban land uses into one rendering at z12 and below, it makes the most sense to simplify the vegetation and rural landuse colors as well. Also note that the test images show the darker river-color. That's why I had been waiting to submit this PR until #3930 was merged. |
I still have not working testing environment, but the examples look very good for me - they are both readable and nicely balanced. I see problems with #3670, but I think they should be discussed there, not here. |
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 already gave my opinion above i think. My view is that #2654 moved us outside consensus in terms of color design in this style and i don't think this change would return us to a consensus position. I would not block this change if you think it helps us working towards a consensus position but i am not qualified to review it cartographically because i disagree with the idea of fading in the first place. I would like to see us working on a consensus with a uniform color scheme across zoom levels.
Yes, that is my understanding. My goal is still to merge #3670 But by accepting this PR, it will make it easier to get to unfaded colors. Right now there is a large difference between the "before" and "after" images in #3670 due to the current strong fading, and this distracts from the more subtle difference proposed there. Unfortunately, @kocio-pl is not able to review this technically and @matthijsmelissen has not been available recently, so I am not sure who is going to be able to do a full review of this PR. |
@jeisenbe, can I do a review of it or is that something that has to be done by a Collaborator? Id like to see it go through. As the current fading irritates me. |
Yes, anyone is welcome to do a review, and I would encourage contributors who have some experience with this style to make a review when they are interested in an issue, especially if they see a specific problem, but also if they have reviewed the PR carefully and think it should be approved. Ideally a reviewer would fetch and check out the branch (jeisenbe:reduce-fade), and check how the cartographical changes look in a couple of representative areas. It's also possible to do a partial review of just the code changes, or just the cartography changes based on test images. Either way, it's best to use the button to "review changes" under the tab "File changed", or if there is a comment about a particular line of code, it is helpful to select that line and comment there first. |
Anyone willing to review this PR? |
@kocio-pl are you willing to approve this PR? |
Nothing changed from #3952 (comment) - I still have no tools to test this technically, but I accept what is seen on visual examples you provided. |
Anyone else willing to review this? While I agree that it is not a long-term solution to low zoom landcover rendering, I think incremental changes like this might make it easier to get back to a consensus-based solution, since any change would not be as large or sudden. |
OK, I'm taking the comment by @kocio-pl as a positive cartographic review, @Adamant36 is "thumbs up", and @imagico is not in favor of this as a long-term solution, but not opposed either if it helps get us back towards consensus. Are there any other thoughts or reviews about this? I am considering merging this for the next release. |
While as indicated i have no serious objections to this change although i don't see it solving any problem i would like to point out that our principle of every PR being reviewed by a maintainer in addition to the person making the PR requires actual independent testing to be meaningful. The whole idea is to make sure someone independently looked at the change and its effect - testing it under circumstances other than those tested by the PR developer. This is not the case here. On a more general note - as we discussed in #3951 everyone constructively working towards a solution is a fundamental prerequisite for consensus based decision making to work. If @kocio-pl objects to #3670 but is not able to actively work towards a different route to consensus either using this PR as a step or using a different approach that is not going to work. |
Good point, so this PR still needs a formal review. Perhaps @Adamant36 would be able to test it? |
Sure. I was actually going to ask if I could test it even though I'm not a maintainer. |
From the minor testing so far I'd say its a major improvement. Not having the fading transition between z12 and z13 makes zooming seem much more fluid. Also, the docker colors make farmland much easier to see zoomed out and parks look slightly less prominent because they are darker and therefore slightly more inline with other green colors. |
Anyone can test and add a review. It's best to add a review rather than issue comments because then it's clear that it's been reviewed through the Github UI. |
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.
👍
Good call. I didn't know that was an option. |
Related to #3935 and #3936
Changes proposed in this pull request:
lighten 0.2
instead of 0.4.@built-up-z12
to#dddddd
- this maintains the current rendered color at z12@built-up-low-zoom
to#d0d0d0
- same as current@built-up-z12
colorExplanation
As discussed in issue #3936 and the WIP PR#3670, the current fading of the landcover layers at low and mid-low zoom levels makes it difficult to distinguish different types of landcover/landuse at z12 and below.
Roads, railroads, and waterways are much more prominent than landcover at z12 and below, especially at z10 and below. Water areas are not faded, and therefore increase in prominence relative to landcover
This PR reduces the fading. While this transition still makes it more difficult to distinguish between light landcovers (like farmland), this situation is partial improved. This change will also make the transition smoother between the current fading and a future goal of no fading at low zoom levels, but few landcover classes, as shown in #3670
Test rendering :
Before
z10 Luxembourg
z11
z12
After
z10
z11
z12
Also see more examples in #3936 (comment)