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

Add Shading for places defined as areas #2806

Closed
wants to merge 5 commits into from

Conversation

stevenLAD
Copy link

Add shading for places defined as areas. Currently only applies to
Villages and Hamlets but can be extended. Opacity is low to expose
other land use within villages.

stevenLAD added 3 commits September 3, 2017 13:17
Create features and text-polys to use in shading and naming places that
are defined as areas.

Replaces Pull 2800 which did not create features.

Subsequent Pulls in new branches will implement these features
Null edit in Atom that removed excess spaces.

To prepare file for edit so that code changes are clearer
Add shading for places defined as areas. Currently only applies to
Villages and Hamlets but can be extended. Opacity is low to expose
other land use within villages.
@HolgerJeromin
Copy link
Contributor

HolgerJeromin commented Sep 3, 2017

Can you provide screenshots?
Opacity/transparency is normaly not used as it has ugly results when stacked

@stevenLAD
Copy link
Author

As place areas are a superset of land uses, I used opacity to not obscure other land uses based on its use in Danger Areas http://www.openstreetmap.org/#map=13/51.2117/-1.6796. However without opacity, i do not believe that it will obscure other land uses cf anywhere in the Netherlands where landuse=residential is used over large areas encompassing many other land uses.

I am not fixed on this (opacity) and bow to your experience as to its use, as well as to the actual shading tone used - my goal was to add shading.

@kocio-pl
Copy link
Collaborator

kocio-pl commented Sep 4, 2017

Could you post some renderings to show how it would look like?

@stevenLAD
Copy link
Author

I do not have any renderings, and have not yet worked out how to install the rendering engine. My mods are based on existing code and images. Perhaps someone with more experience and better facilities could try? I will look at installing later but it will be some time from now.

@kocio-pl
Copy link
Collaborator

kocio-pl commented Sep 4, 2017

@matkoniecz
Copy link
Contributor

Is it intentional to display

[zoom >= 13] { polygon-fill: @residential; }

over entire area of place, including areas that are not a residential areas?

Testing changes is a part of making pull request and it is usually the most time-consuming part.

@kocio-pl
Copy link
Collaborator

kocio-pl commented Sep 4, 2017

I think of place key as a toponym, so the only relevant part for rendering should be a name label.

@matkoniecz
Copy link
Contributor

I think of place key as a toponym, so the only relevant part for rendering should be a name label.

Yeah, I also I think the same. Typical settlement includes many different landuses, not only residential area.

@stevenLAD
Copy link
Author

If a place is a toponym, then why would key values be village, town suburb etc - there appears to be some inconsistency there.

@kocio-pl
Copy link
Collaborator

kocio-pl commented Sep 4, 2017

Probably because the name and location might be the only thing we know about such place, not which landuses are there. If we know them, they can be added too.

@stevenLAD
Copy link
Author

A toponym is related to topography. Kocio-pl - Your comment highlights the need to shade places; we may know location and extent, but not necessarily land uses or name. Nothing precludes adding land uses later, hence the proposal to use opacity, or even replace with land uses. At present, landuse=residential is widely abused to highlight villages and towns, and this proposal would remove that need.

This subject has been the subject of discussion since.......and even the wiki references it as a bug!

@dieterdreist
Copy link

dieterdreist commented Sep 4, 2017 via email

@stevenLAD
Copy link
Author

Etymology apart, the name applies to a place=village, hamlet etc. These are not points but extents or areas and OSM allows their definition as such. Each area is a superset of landuses. This pull was intended to give shading that covers the entire area without obscuring any areas (landuses that may or may not exist) within.

If only name is shown, then the map looks like the cycle map - there is something somewhere without an extent.

To cover this in many areas landuse=residential has been overused to mark the extent including items that are not residential. Going forward, this would allow initial definition of an area that could be subsequently enhanced with more granular and accurate landuse areas.

@stevenLAD
Copy link
Author

stevenLAD commented Sep 4, 2017

To illustrate my point, below are 3 areas of Angola, all at zoom 13. The first uses residential areas and place points.
http://www.openstreetmap.org/#map=13/-10.6879/14.3366

The next 2 are views of another area using villages, the first in the standard layer
http://www.openstreetmap.org/#map=13/-9.9566/14.8158

and this one in the cycle map layer
http://www.openstreetmap.org/#map=13/-9.9566/14.8158&layers=C

which one speaks the clearest to the user about the extent and existence of population? I think that the first one, but is all of that landuse really residential?

while it is possible to achieve the result using the first approach, it pollutes the landuse database which could become a valuable asset.

@kocio-pl
Copy link
Collaborator

kocio-pl commented Sep 5, 2017

Rendering examples are really needed to see how it differs from landuse=residential. I still feel that we have different tools for showing boundaries and landuses and place area is not any of them, it's just more precise if the place we are naming takes more space than just a point - but actually rendering that area may clash with landuses and boundaries.

Copy link
Collaborator

@pnorman pnorman left a comment

Choose a reason for hiding this comment

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

I'm not sure of the difference between this and #2804. I'm not in favour of rendering village/hamlets as residential, and I'm skeptical if they should be rendered like built up (vmap). I'd love to replace vmap, but it's proven to be a hard problem.

landcover.mss Outdated
@@ -766,7 +785,7 @@
b/line-color: @tourism;
b/line-opacity: 0.3;
b/line-join: round;
b/line-cap: round;
b/line-cap: round;
Copy link
Collaborator

Choose a reason for hiding this comment

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

whicespace noise

Copy link
Author

@stevenLAD stevenLAD Sep 8, 2017

Choose a reason for hiding this comment

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

This has already been removed as part of a whitespace cleanup which I understood had been merged

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, but it shouldn't be in this PR as well.

landcover.mss Outdated
@@ -474,7 +493,7 @@
[way_pixels >= 4] { polygon-gamma: 0.75; }
[way_pixels >= 64] { polygon-gamma: 0.3; }
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

whitespace noise

Copy link
Author

@stevenLAD stevenLAD Sep 8, 2017

Choose a reason for hiding this comment

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

This has already been removed as part of a whitespace cleanup which I understood had been merged

landcover.mss Outdated
[feature = 'place_hamlet'] {
[zoom >= 10] {
polygon-fill: @built-up-lower-lowzoom;
polygon-opacity: 0.3;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't use opacity.

Copy link
Author

@stevenLAD stevenLAD Sep 8, 2017

Choose a reason for hiding this comment

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

As already said - the intention in using opacity was to not obfuscate finer detail. This can be removed if preferred.
Please remove if required or advise how I should proceed to remove so as not to cloud the pull. Thanks

Copy link
Collaborator

Choose a reason for hiding this comment

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

Given my basic concerns with rendering places like built up areas I'm not sure what changes you should make, except you need to find a way without opacity.

@Nakaner
Copy link
Contributor

Nakaner commented Sep 7, 2017

The SQL query of the landcover layer does currently only order features by ST_Area, i.e. larger polygons are drawn below smaller polygons. If a place=village polygon is covers all the area of a village (keep in mind that villages can have 1000 inhabitants and more!) and the landuse in this area is mapped in small pieces, your place polygon will be rendered below all those small landuse polygons.

@stevenLAD
Copy link
Author

That was my intention. The extent of the village should not prime over factual and accurate landuses as the village extent would normally be a superset of these with eventual common boundaries and was intended particularly for lower zooms. The shading should be adapted accordingly so as to deconfuscate ( a lighter shade of grey would appear ideal, but is not included).

@pnorman
Copy link
Collaborator

pnorman commented Sep 9, 2017

Rendering examples are really needed to see how it differs from landuse=residential.

Yes, we really need a few images selected to show the changes in order to meaningfully discuss this.

I still feel that we have different tools for showing boundaries and landuses and place area is not any of them,

👍

Remove opacity and lighten shading to differentiate from residential
landuse
@stevenLAD
Copy link
Author

Above commit removes opacity as requested and generally agreed. It also lightens the shading so that residential areas, if defined, will be easily distinguishable. The same colour is used as villages and hamlets will be majority residential so it appears to be consistent

@kocio-pl
Copy link
Collaborator

What about rendering examples? It's very important to show real examples of the output and test if it's really working.

If you don't have testing environment and don't know where to start, try these instructions:

https://github.com/gravitystorm/openstreetmap-carto/blob/master/DOCKER.md
https://ircama.github.io/osm-carto-tutorials/

@matkoniecz
Copy link
Contributor

We really need a few images selected to show the changes in order to meaningfully discuss this.

Once you have some please open a new PR or request reopening this one.

@matkoniecz matkoniecz closed this Nov 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants