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

Reduce landcover fading at mid-low zoom levels #3952

Merged
merged 2 commits into from
Feb 13, 2020

Conversation

jeisenbe
Copy link
Collaborator

Related to #3935 and #3936

Changes proposed in this pull request:

  • Only fade from z11 and only 1 level - reduces the 3 level fading to only one level, using lighten 0.2 instead of 0.4.
  • Remove built-up-z11 color - this is no longer needed, since z11 is now the same as z10 and below
  • Change @built-up-z12 to #dddddd - this maintains the current rendered color at z12
  • Change @built-up-low-zoom to #d0d0d0 - same as current @built-up-z12 color

Explanation

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
z10-luxembourg-before

z11
z11-luxembourg-before

z12
z12-n-luxembourg-before

After
z10
z10-luxembourg-reduce-fade

z11
z11-luxembourg-reduce-fade

z12
z12-luxembourg-reduce-fade

Also see more examples in #3936 (comment)

Remove built-up-z11 color

Less fade - 0.2 lighten instead of 0.4
@imagico
Copy link
Collaborator

imagico commented Oct 26, 2019

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.

@jeisenbe
Copy link
Collaborator Author

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.

@kocio-pl
Copy link
Collaborator

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.

Copy link
Collaborator

@imagico imagico left a 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.

@jeisenbe
Copy link
Collaborator Author

it helps us working towards a consensus position

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.

@imagico imagico added the consensus needed Indicates the lack of consensus among maintainers blocks a PR/issue label Nov 15, 2019
@Adamant36
Copy link
Contributor

@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.

@jeisenbe
Copy link
Collaborator Author

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.

@jeisenbe
Copy link
Collaborator Author

Anyone willing to review this PR?

@jeisenbe
Copy link
Collaborator Author

@kocio-pl are you willing to approve this PR?

@kocio-pl
Copy link
Collaborator

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.

@jeisenbe
Copy link
Collaborator Author

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.

@jeisenbe jeisenbe mentioned this pull request Jan 26, 2020
@jeisenbe
Copy link
Collaborator Author

jeisenbe commented Feb 7, 2020

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.

@imagico
Copy link
Collaborator

imagico commented Feb 7, 2020

I'm taking the comment by @kocio-pl as a positive cartographic review

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.

@jeisenbe
Copy link
Collaborator Author

jeisenbe commented Feb 9, 2020

Good point, so this PR still needs a formal review. Perhaps @Adamant36 would be able to test it?

@Adamant36
Copy link
Contributor

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.

@Adamant36
Copy link
Contributor

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.

z12
z11

@pnorman
Copy link
Collaborator

pnorman commented Feb 12, 2020

Sure. I was actually going to ask if I could test it even though I'm not a maintainer.

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.

Copy link
Contributor

@Adamant36 Adamant36 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@Adamant36
Copy link
Contributor

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.

Good call. I didn't know that was an option.

@pnorman pnorman merged commit 2d92114 into gravitystorm:master Feb 13, 2020
@jeisenbe jeisenbe deleted the reduce-fade branch February 13, 2020 05:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
consensus needed Indicates the lack of consensus among maintainers blocks a PR/issue landcover
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants