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.

Sign up for GitHub

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

Jump to bottom

Move danger_area into landuse-overlay #3469

Merged
merged 2 commits into from
Oct 29, 2018
Merged

Move danger_area into landuse-overlay #3469

merged 2 commits into from
Oct 29, 2018

Conversation

jragusa
Copy link
Contributor

@jragusa jragusa commented Oct 21, 2018

Fixes #3454

Changes proposed in this pull request:
Render military=danger_area above other landuses by moving it into the landuse-overlay layer

Test rendering with links to the example places:
https://www.openstreetmap.org/way/307904554
Before
map

After
danger_area

@kocio-pl
Copy link
Collaborator

@matkoniecz @matthijsmelissen Could you check this code? I'm not sure if it does not change anything else beside moving to overlay.

@matkoniecz
Copy link
Contributor

One thing that is certainly worth testing - is labelling of such areas still working correctly? I see nothing obviously problematic but it should be checked.

@kocio-pl
Copy link
Collaborator

Could you make this additional check?

@jragusa
Copy link
Contributor Author

jragusa commented Oct 23, 2018

it's working from my side:
https://www.openstreetmap.org/way/424081505 (without landuse=military)
danger_area_1

matthijsmelissen
matthijsmelissen requested changes Oct 28, 2018
View reviewed changes
Copy link
Collaborator

@matthijsmelissen matthijsmelissen left a comment

Choose a reason for hiding this comment

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

Thanks for your pull request, and sorry for the delay in reviewing! I have two small comments.

project.mml Outdated
@@ -87,15 +87,17 @@ Layer:
table: |-
(SELECT
way, name, way_pixels, religion,
COALESCE(wetland, landuse, "natural") AS feature
COALESCE(wetland, landuse, military, "natural") AS feature
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we don't need military=danger_area in this layer. The layer landuse-overlay also works for lowzoom rendering.

We can even remove the landuse=military from this layer: the style for #landcover-low-zoom does not seem to use landuse=military.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

project.mml Outdated
@@ -538,9 +538,11 @@ Layer:
(SELECT
way,
landuse,
way_area/NULLIF(!pixel_width!::real*!pixel_height!::real,0) AS way_pixels
way_area/NULLIF(!pixel_width!::real*!pixel_height!::real,0) AS way_pixels,
military
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe swap these lines for clarity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@matthijsmelissen matthijsmelissen merged commit 0af802e into gravitystorm:master Oct 29, 2018
@matthijsmelissen
Copy link
Collaborator

Thanks!

@Tomasz-W Tomasz-W mentioned this pull request Oct 29, 2018
Open
@jragusa jragusa deleted the danger branch November 6, 2018 20:24
Nakaner added a commit to Nakaner/openstreetmap-carto that referenced this pull request Feb 14, 2019
@Nakaner
Update condition of index planet_osm_polygon_military
2987280 
This reflects changes to the SQL query of layer landuser-overlay
introduced by commit 52085d3 (pull
request gravitystorm#3469).
Nakaner added a commit to Nakaner/openstreetmap-carto that referenced this pull request Feb 14, 2019
@Nakaner
Update condition of index planet_osm_polygon_military
34cf72b 
This reflects changes to the SQL query of layer landuser-overlay
introduced by commit 52085d3 (pull
request gravitystorm#3469).
@Nakaner Nakaner mentioned this pull request Feb 14, 2019
Merged
Nakaner added a commit to Nakaner/openstreetmap-carto that referenced this pull request Feb 14, 2019
@Nakaner
Update condition of index planet_osm_polygon_military
f31c838 
This reflects changes to the SQL query of layer landuser-overlay
introduced by commit 52085d3 (pull
request gravitystorm#3469).
matthijsmelissen pushed a commit that referenced this pull request Feb 15, 2019
@Nakaner @matthijsmelissen
Update condition of index planet_osm_polygon_military (#3682)
5e7c9dd 
This reflects changes to the SQL query of layer landuser-overlay
introduced by commit 52085d3 (pull
request #3469).
matthijsmelissen
matthijsmelissen reviewed Feb 15, 2019
View reviewed changes
project.mml
way_area/NULLIF(!pixel_width!::real*!pixel_height!::real,0) AS way_pixels
FROM planet_osm_polygon
WHERE landuse = 'military'
OR military = 'danger_area'
AND building IS NULL
Copy link
Collaborator

Choose a reason for hiding this comment

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

In SQL, AND has precedence over OR, so this will be parsed as landuse = 'military' OR (military = 'danger_area' AND building IS NULL). The intended meaning is of course (landuse = 'military' OR military = 'danger_area') AND building IS NULL so we are missing a pair of brackets here.

@matthijsmelissen matthijsmelissen mentioned this pull request Feb 15, 2019
Merged
@pitdicker pitdicker mentioned this pull request Sep 4, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthijsmelissen matthijsmelissen matthijsmelissen requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jragusa @kocio-pl @matkoniecz @matthijsmelissen