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

Nature reserve boundaries revision #3574

Merged
merged 4 commits into from
Dec 27, 2018

Conversation

vholten
Copy link
Contributor

@vholten vholten commented Dec 15, 2018

Related to #3538, #2978

Changes proposed in this pull request:

  • Make nature reserve boundaries less prominent at z8 and z9
  • On all zoom levels: Reduce the "glowing" effect that occurs on boundaries that are shared between two adjacent nature reserves

Test renderings of Northern California

z8 before:
z8osm

z8 after:
z8tuned

z9 before:
z9osm

z9 after:
z9tuned

z10 before:
z10osm

z10 after:
z10tuned

@Tomasz-W
Copy link

Tomasz-W commented Dec 15, 2018

@vholten Can you try with 1.3 width on z8 and 1.6 on z9? When I was checking test images on my mobile phone, these borders were quite hard to see.

@vholten
Copy link
Contributor Author

vholten commented Dec 15, 2018

OK, here are the test renders with 1.3 width on z8 and 1.6 on z9. The differences are very small however. To tune the prominence of the borders at these zoom levels, it is better to change the opacity (currently 0.2 in this PR at z8 and z9).

z8, width 1.3:
z8new13

z9, width 1.6:
z9new16

@Tomasz-W
Copy link

Thanks! Please test also this place where these areas are not only an outiline over bare ground
https://www.openstreetmap.org/#map=8/48.897/20.599

@Adamant36
Copy link
Contributor

Thanks for doing this @vholten. It looks a lot better.

@Tomasz-W
Copy link

I think they are a little bit too thick on z10 also (e.g. https://www.openstreetmap.org//#map=10/51.4326/13.3690), so I propose a scale with:

  • 1.2 on z8
  • 1.5 on z9
  • 1.8 on z10

@matkoniecz
Copy link
Contributor

@Tomasz-W Can you give some test rendering for this proposed scale?

@vholten
Copy link
Contributor Author

vholten commented Dec 17, 2018

@matkoniecz I plan to show test renderings for this PR and for the scale proposed by @Tomasz-W in the area of https://www.openstreetmap.org/#map=8/48.897/20.599 within the next couple of days.

@vholten
Copy link
Contributor Author

vholten commented Dec 20, 2018

I have done some tests in the region suggested by @Tomasz-W and based on the results I think that the borders should be made a bit stronger than in my first proposal on z8 and z9 (opacity of 0.25 instead of 0.2).
On z10 I have tested thinner borders (width of 1.8 instead of 2).

(All test renders also use #3553 and #3563.)

z8

z8 before:
z8borders

z8 first proposal (opacity 0.2):
z8pr

z8 proposed stronger borders (opacity 0.25):
z8pr-op25

z9

z9 before:
z9borders

z9 first proposal (opacity 0.2):
z9pr

z9 proposed stronger borders (opacity 0.25):
z9pr-op25

z10

z10 before:
z10borders

z10 first proposal (thickness 2.0):
z10pr

z10 thinner borders (thickness 1.8):
z10pr-w1 8

@kocio-pl
Copy link
Collaborator

When you update the code I guess it will be ready to merge.

@vholten
Copy link
Contributor Author

vholten commented Dec 25, 2018

I've updated the code with the proposed changes. These are:

  • Slightly more prominent borders on on z8 and z9 (opacity of 0.25 instead of 0.20)
  • Thinner borders on z10 (width of 1.8 instead of 2.0)

I've also updated the test renderings of Northern California in the first post.

What do people think about making borders thinner on z11 as well?

@kocio-pl
Copy link
Collaborator

It's hard to say, please show some test rendering.

@vholten
Copy link
Contributor Author

vholten commented Dec 26, 2018

z11 current PR (https://www.openstreetmap.org/#map=11/49.5525/20.1495):
z11before

z11 thinner borders (width of 1.8 instead of 2.0)
z11-w1 8

@kocio-pl
Copy link
Collaborator

Works for me.

@vholten
Copy link
Contributor Author

vholten commented Dec 26, 2018

I've updated the code, I guess it's ready to be merged.

@kocio-pl kocio-pl merged commit 572df92 into gravitystorm:master Dec 27, 2018
@kocio-pl
Copy link
Collaborator

Thanks! Further tuning is still possible, but this helps a bit anyway at the moment.

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