-
Notifications
You must be signed in to change notification settings - Fork 819
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
Revised min zoom visibility for some elements #2139
Conversation
Revised min zoom visibility for some elements
Revert "Revised min zoom visibility for some elements"
If you change the starting zoom level of showing certain features, especially if this changes a lot - up to four zoom levels in your case, you need to demonstrate that this actually leads to good results in a large variety of settings (including different latitudes - this is the biggest problem in that regard with Mercator maps). For place_of_worship for example this seems doubtful - see the following areas with high density of this kind of feature: http://www.openstreetmap.org/#map=16/41.0104/28.9646 |
@@ -32,11 +32,13 @@ | |||
} | |||
} | |||
|
|||
// ircama: revised zoom and text size |
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.
These comments need removal
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.
@Ircama: There's no need to leave comments like this in the code. "git blame" can be used to show when, why and who made the last change, if we ever need to know it.
Can you provide some examples of places (in form of before/after images) where it improves situation and in regions where it will likely make situation worse? |
See also message from Travis from https://travis-ci.org/gravitystorm/openstreetmap-carto/builds/134040029 (linked in box below in "continuous-integration/travis-ci/pr — The Travis CI build failed ").
|
Yes, apologize. I have a Windows installation of Tilemill/openstreetmap-carto/git and realized that, after issuing
I also need to run
(or use an editor to convert This even if I have
if I forget to convert Windows line feeds to UNIX, |
…xtension-zooms # Conflicts: # amenity-points.mss # water.mss
…topo-extension-zooms # Conflicts: # project.mml
tourism=alpine_hut and tourism=viewpoint are now shown at zoom >= 14 instead of 15 and 16 respectively. viewpoint can be applied to chalet and place_of_worship in order to have them rendered at zoom >= 14: - tourism=chalet & tourism=viewpoint - amenity=place_of_worship & tourism=viewpoint - amenity=shelter & tourism=viewpoint
Notice that the rendering at lower zoom for chalet, place_of_worship and shelter is only applied when *tourism=viewpointé (which should be appropriate in the mountains, where those tags are orientation references):
This should allow compatibility with some populated areas or cities where these tags are sometimes present. |
sent from a phone
I don't understand the idea behind this, wouldn't you want to look at the landmark key rather than the viewpoint tag for this? I don't think in the case of place of worship it would be a good idea to lower their visibility in lower zoom levels, these are deliberately shown early and it should remain like this. It doesn't matter whether you can look from the place of worship, and while they are usually landmarks that can be seen from a distance, they are also quite important in general and merit early rendering from a cultural, historical, urbanist point of view |
In the mountains, in alpine unpopulated zones, a church is usually a reference point that should be visible at lower zoom levels. This is a usually adopted practice in standard topographic maps. In order to allow this, a church should be visible from great distances and useful for understanding places. As when using tourism=viewpoint this condition should be true, I would consider to only reduce zooms of these monuments when associated with this condition.
Agreed for populated zones. A church in a city for instance shall not get tourism=viewpoint. |
sent from a phone
there seems to be a misunderstanding, tourism=viewpoint is about a spot from where you can enjoy a good view, not a feature that can be seen from far |
Well, if in a place you can enjoy a good view, this does mean that it can be seen from far; at the end it should be considered a reference. We could add both possibilities anyway: [amenity=place_of_worship][landmark]: 27 points globally at the moment Adding both options should not impact the existing rendering. |
sent from a phone
I see now you want to show places of worship earlier not later, sorry, no objection to this. both options are currently so few that it doesn't really matter anyway ;-) I would prefer to tie this to buildings (e.g. building=church/chapel/mosque/...), not places of worship , which might in some occasions also be sacred places without any visible structures |
I would consider this appropriate: the idea is to enable an appropriate rendering for those features, avoiding to be invasive.
OK, I'll investigate this |
@@ -36,7 +36,10 @@ | |||
[zoom >= 14][way_pixels > 3000], | |||
[zoom >= 17] { | |||
text-name: "[name]"; | |||
text-size: 11; | |||
text-size: 8; |
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.
don't use text this small, it poses readability problems, particularly with CJK glyphs (e.g. Japanese)
broadly speaking, there's three things in this PR
I can see doing the last of these, I'm not sure about the other changes |
Zoom reduction when using tourism=viewpoint or landmark for the following features: - amenity=place_of_worship - man_made=campanile - building=shrine - building=synagogue - building=temple - building=mosque - building=church - building=chapel - building=cathedral Besides, all the above features are rendered now, including symbol and text. Also the text for the following features is now rendered according to tourism=viewpoint and landmark: - man_made=cross - historic=wayside_cross - building=transformer_tower - man_made=chimney - man_made=communications_tower - man_made=tower - man_made=water_tower Peaks (natural=peak) with cross (man_made=cross) are appropriately rendered with a cross starting from zoom level 13. addressing.mss: text-size set to 9 rather than 8
I can prepare some examples if needed
Now all those features should be rendered. Nevertheless, managing landmark tag needs hstore (at least, I used hstore so far; if there is some other method, please advise). Change listZoom reduction when using tourism=viewpoint or landmark for the following features:
Besides, all the above features are rendered now, including symbol and text. Also the text for the following features is now rendered according to tourism=viewpoint and landmark:
Peaks (natural=peak) with cross (man_made=cross) are appropriately rendered with a cross starting from zoom level 13. addressing.mss: text-size set to 9 rather than 8 |
You cannot use the |
@@ -36,7 +36,10 @@ | |||
[zoom >= 14][way_pixels > 3000], | |||
[zoom >= 17] { | |||
text-name: "[name]"; | |||
text-size: 11; | |||
text-size: 9; |
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.
this line never applies - the restrictions above require zoom >= 14
You've added in a lot of code about certain building types which is not changing min zoom for some elements. That belongs in a different PR. |
I understand that introducing elements that were not rendered at all has involved some coding. I would ask you to consider keeping all changes in a single PR just in this case. It is all about zooming and the new elements are of similar categories, part of the same zooming strategy. If you anyway consider necessary to separate the coding, I will do that. |
Do you think this kind of commenting convention could be acceptable? It would allow to temporarily disable hstore references in project.yaml and restore them in a moment as soon as hstore is available: Before:
After:
Before:
After:
Features can be developed and tested in advance on a dev env but have to be published with these disabling comments. To process project.yaml after hstore in order to enable them:
|
No - just don't develop anything that requires a different method of data loading. |
OK, I'll remove any reference or comment related to landmark from this branch |
There are too many things in this PR that are minimally related and can be considered independently.
I think the most effective route might be to close this PR and then you can create a new branch from master and not bundle unrelated changes together. A PR needs to have a clear scope, and expanding it to new changes makes review impossible. |
sent from a phone
I am inclined to see this as bad tagging practice, particularly for features like place of worship or buildings, which typically will cover a bigger area than the actual viewpoint (more likely a spot or part of an area of those mentioned before). A viewpoint is a place from which you can look in the distance, it is unlikely that from the whole of a building you will have a good view into the distance |
well, it generally depends on the construction strategy of the building and mappers shall decide case by case. A church in the Alps for instance is likely a viewpoint, often built on a prominent part of the surrounding territory, e.g., on a cliff, on the top of a hill, on a ridge. Mappers should tag these buildings as viewpoints if e.g. their surrounding courtyards are excellent observation places by design. And map rendering shall reflect this by showing the monument at a lower zoom level. I am with you that sometimes a building or a place of worship which is not a viewpoint (and shall not be tagged as such in these cases) can still be a landmark, for instance for its facade or campanile that can be a reference for anyone in the valley. Generally a campanile would be a landmark (it is typically so by design) and not a viewpoint (access is generally not allowed). Anyway the landmark tag cannot be rendered at the moment (I checked and there is a remarkable ongoing work) so we would need to complete the rendering as soon as tags like this can be exploited. |
2016-06-16 2:02 GMT+02:00 Ircama notifications@github.com:
no, the viewpoint is typically in front of the church. If you look at usage This mapstyle is rendering commonly mapped objects. Adding complex rules IMHO your general idea is good (render some landmarks ealier). My |
I'm learning now that churches like those ones linked here below will not be viewpoint references themselves, but maybe some surrounding area close to them: http://www.buonviaggioitalia.it/wp-content/uploads/2015/05/2015-04-14-2629.jpg http://www.hafling-meran2000.eu/bilder/stkathrein-meran-hafling-suedtirol.jpg http://www.golivefvg.com/wp-content/uploads/2012/09/milani_c1-162-2455-650x433.jpg :-) ok, no problem. Provided that there are many underrepresented features in OSM, that there might be a relation between unused tags and not rendered ones and that IMHO the current style is actually poor in rendering mountainous unpopulated places, your suggestion is viable. |
@Ircama This pull request still does a lot more than what is stated in the title ('revised min zoom visibility'). Would you be able to split this up even more? Thanks! |
@math1985: sure, will do |
This branch supersedes branch https://github.com/Ircama/openstreetmap-carto/tree/topo-extension-zooms for the specifical relation to peaks (natural=peak) with cross (man_made=cross), which are appropriately rendered with a cross starting from zoom level 13. Vs. previous branch, the marker-transform feature has been replaced by specifig icons. Related PR will replace gravitystorm#2139 for the feature "peaks with cross".
This branch supersedes branch https://github.com/Ircama/openstreetmap-carto/tree/topo-extension-zooms specifically for the text size to be used for lakes (natural=water or landuse=reservoir/basin). Related PR will replace gravitystorm#2139 for the feature "Revised text size for lakes".
This branch supersedes branch https://github.com/Ircama/openstreetmap-carto/tree/topo-extension-zooms specifically for the rendering of religious buildings and related text: amenity=place_of_worship man_made=campanile building=shrine building=synagogue building=temple building=mosque building=church building=chapel building=cathedral Related PR will replace gravitystorm#2139 for the rendering of religious features.
This branch supersedes branch https://github.com/Ircama/openstreetmap-carto/tree/topo-extension-zooms for the specifical relation to peaks (natural=peak) with cross (man_made=cross), which are appropriately rendered with a cross starting from zoom level 13. Vs. previous branch, the marker-transform feature has been replaced by specifig icons. Related PR will replace gravitystorm#2139 for the feature "peaks with cross".
This branch supersedes branch https://github.com/Ircama/openstreetmap-carto/tree/topo-extension-zooms specifically for the rendering of religious buildings and related text: amenity=place_of_worship man_made=campanile building=shrine building=synagogue building=temple building=mosque building=church building=chapel building=cathedral Related PR will replace gravitystorm#2139 for the rendering of religious features. Correct ('man_made_' || (CASE WHEN man_made IN ('tower', 'campanile') into ('man_made_' || (CASE WHEN man_made IN ('campanile')
No description provided.