Buildings code rewrite #3426
Buildings code rewrite #3426
Conversation
|
Great work on the code!
I think it might be better with the major buildings a little more
prominent.
Could we try a color half-way between the old and new major building colors?
…On Fri, Oct 5, 2018 at 11:34 AM meased ***@***.***> wrote:
Fixes #2532
<#2532>
This PR simplifies the buildings SQL query and moves all styling decisions
out of SQL and into CSS (as the software gods intended 😉).
Visual changes:
- Major buildings are not as dark.
- Adds a minor building category (lighter).
SQL
Before, normal buildings and major building were queried separately with
each query specifically omitting the results of the other. This made it
difficult to redefine the categories (and very difficult to add a third
category).
The SQL query now queries all buildings together, along with any fields
needed to make styling decisions:
(SELECT
way,
building,
amenity,
aeroway,
aerialway,
tags->'public_transport' as public_transport
FROM planet_osm_polygon
WHERE building IS NOT NULL
AND building != 'no'
AND way_area > 1*!pixel_width!::real*!pixel_height!::real
ORDER BY COALESCE(layer,0), way_area DESC
) AS buildings
CSS
Now that the buildings class has access to all the fields needed, the CSS
can be written in one block. As you can see, it is now very easy to
reclassify buildings and add/remove the minor class:
#buildings {
[zoom >= 13] {
polygon-fill: @building-low-zoom;
polygon-clip: false;
line-width: 0;
[zoom >= 15] {
polygon-fill: @building-fill;
line-color: @building-line;
line-width: .75;
line-clip: false;
}
[amenity = 'place_of_worship'],
[aeroway = 'terminal'],
[aerialway = 'station'],
[building = 'train_station'],
[public_transport = 'station'] {
polygon-fill: @building-major-fill;
line-color: @building-major-line;
}
[building = 'garage'],
[building = 'garages'],
[building = 'carport'],
[building = 'shed'],
[building = 'greenhouse'],
[building = 'farm_auxiliary'],
[building = 'construction'],
[building = 'service'],
[building = 'ger'],
[building = 'ruins'] {
polygon-fill: @building-minor-fill;
line-color: @building-minor-line;
}
}
}
Test Renderings
https://www.openstreetmap.org/#map=18/45.59116/-122.74872
(Images line up so you can put them in browser tabs and switch tabs back
and forth.)
Current style on top.
[image: sj_14b]
<https://user-images.githubusercontent.com/14190496/46512926-21ad4880-c80b-11e8-9bd5-ce79a5a91950.png>
[image: sj_14a]
<https://user-images.githubusercontent.com/14190496/46512928-24a83900-c80b-11e8-9f8e-42587ae7b11d.png>
[image: sj_15b]
<https://user-images.githubusercontent.com/14190496/46512932-27a32980-c80b-11e8-8553-52a324fbff8a.png>
[image: sj_15a]
<https://user-images.githubusercontent.com/14190496/46512934-2a058380-c80b-11e8-8ed8-916e98be4b78.png>
[image: sj_16b]
<https://user-images.githubusercontent.com/14190496/46512936-2bcf4700-c80b-11e8-827a-5b0b0a33ada3.png>
[image: sj_16a]
<https://user-images.githubusercontent.com/14190496/46512937-2d990a80-c80b-11e8-9f73-165189c816d4.png>
[image: sj_17b]
<https://user-images.githubusercontent.com/14190496/46512939-2ffb6480-c80b-11e8-9501-9f829564db6a.png>
[image: sj_17a]
<https://user-images.githubusercontent.com/14190496/46512945-31c52800-c80b-11e8-81c8-7ef24abcc77d.png>
[image: sj_18b]
<https://user-images.githubusercontent.com/14190496/46512947-32f65500-c80b-11e8-927e-dd44db95486e.png>
[image: sj_18a]
<https://user-images.githubusercontent.com/14190496/46512950-34c01880-c80b-11e8-8054-73d2066c7941.png>
https://www.openstreetmap.org/#map=16/45.5879/-122.5936
[image: pa_16b]
<https://user-images.githubusercontent.com/14190496/46513013-7ea8fe80-c80b-11e8-9c50-b90f7d78de4c.png>
[image: pa_16a]
<https://user-images.githubusercontent.com/14190496/46513014-7fda2b80-c80b-11e8-9e0e-80d8893805b4.png>
[image: pa_17b]
<https://user-images.githubusercontent.com/14190496/46513017-810b5880-c80b-11e8-8d43-5f445e98550f.png>
[image: pa_17a]
<https://user-images.githubusercontent.com/14190496/46513020-82d51c00-c80b-11e8-9e2c-d4ff7d16dffc.png>
------------------------------
You can view, comment on, or merge this pull request online at:
#3426
Commit Summary
- Standard text halo for fitness_center and fitness_station
- Merge branch 'master' of github.com:meased/openstreetmap-carto
- Merge https://github.com/gravitystorm/openstreetmap-carto
- xMerge https://github.com/gravitystorm/openstreetmap-carto
- Merge https://github.com/gravitystorm/openstreetmap-carto
- Rewrite buildings query, support for minor buildings
- Adjust colors
- Add comment
- Original building color
File Changes
- *M* buildings.mss
<https://github.com/gravitystorm/openstreetmap-carto/pull/3426/files#diff-0>
(39)
- *M* project.mml
<https://github.com/gravitystorm/openstreetmap-carto/pull/3426/files#diff-1>
(31)
Patch Links:
- https://github.com/gravitystorm/openstreetmap-carto/pull/3426.patch
- https://github.com/gravitystorm/openstreetmap-carto/pull/3426.diff
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#3426>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AoxshM8MWXdk5DzEWYOHKTqYaib97KWCks5uhsVHgaJpZM4XJdL7>
.
|
|
I like the idea of these complex changes. Did you however test the performance? We use SQL a lot to achieve reasonable speed. |
|
Could you also test minor buildings darker, like half the way between the proposed color and standard buildings? |
|
Minor buildings no change: Minor buildings 4% lighter: Minor buildings 7% lighter (same as screen shots above): |
|
Major buildings no change: Major buildings 15% darker: Major buildings 10% darker (same as screenshots above): As you can see, I was more concerned with fixing the dreadful "gastronomy in the airport" problem than I was with spotting churches in neighbourhoods. |
No. Anyone know how to run benchmarks in docker? |
|
Would it make sense to lighten major buildings at higher zoom levels? At
mid-zoom it is very helpful that the airport terminal building is obvious,
and that’s when it is useful for places of worship etc.
But at zoom 18 there is no reason for the major building to stay so dark.
Would that be too complicated ?
…On Sat, Oct 6, 2018 at 6:45 AM meased ***@***.***> wrote:
Did you however test the performance?
No.
Anyone know how to run benchmarks in docker?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#3426 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AoxshCBb_Z9SgjZ3WC1j5qpWh9E_4cxRks5uh9LngaJpZM4XJdL7>
.
|
|
That's not that complicated. Worth a test rendering at least. |
|
I think both darker versions (4% for minor and 15% for major) work better. Since buildings are very popular map feature, we need much more testing in different areas (backgrounds). I like the idea of minor buildings instead of problems with ruins icon, especially for castles (see #331). Unfortunately we have no performance framework (see #1287). However we can at least try to deploy it on some real server - @rrzefox, could you measure the effect of replacing some SQL code with Carto CSS here? |
That surprises me. To me it makes utter sense to render relatively insignificant non-living quarters like small garden sheds and garages lighter. It might actually stimulate good tagging practices in that people actually start to distinguish them in tagging, and add building=shed instead of just generic building=yes. |
|
@meased Can you test lighter outline for minor buildings? It looks a little bit like some holes in ground currently. |
I think that is mainly a consequence of the relatively dark grey of the landuse=residential, which appears just a bit darker than the minor buildings. So either slightly lightening up of landuse=residential, or slightly darkening the minor buildings fill, might help. |
Exactly. I choose landuse=residential and airports for examples because they represent the worst cases for how light you can go on one end and how dark you can go on the other. There is not much space to play with in the middle. The minor buildings above would have looked better on bare background: |
I also believe it's more pleasant, less cluttering and more useful at the same time, so it's a triple win for me. There are a lot of auxiliary buildings, but still most of them is tagged with
@meased I see that you have added
|
|
I got the original minor buildings list from #2532. I was avoiding underground (because some people don't want it rendered at all) and roof (because #2475 is different than just rendering them lighter). But until those other issues are sorted out I don't see why they wouldn't qualify as minor buildings in the meantime. @kocio-pl: Strangly, collapsed isn't on the wiki page. I included it anyway because the usage is very high. The list of minor buildings is now: [building = 'roof'],
[building = 'construction'],
[building = 'damaged'],
[building = 'collapsed'],
[building = 'underground'],
[building = 'service'],
[building = 'slurry_tank'],
[building = 'shed'],
[building = 'garage'],
[building = 'garages'],
[building = 'farm_auxiliary'],
[building = 'carport'],
[building = 'barn'],
[building = 'stable'],
[building = 'cowshed'],
[building = 'greenhouse'],
[building = 'ger'],
[building = 'ruins'] { |
|
Playing with @jeisenbe's idea of variable major building darkness: 14 14 |
|
About test renderings above: I find darkness of major buildings on high zoom levels definetely too low. When I looked at them with "fresh eye" I had to search for major buildings, but they should be distinguishable intuitively. |
buildings.mss
Outdated
| [building = 'stable'], | ||
| [building = 'cowshed'], | ||
| [building = 'greenhouse'], | ||
| [building = 'ger'], |
There was a problem hiding this comment.
What is that?
I miss =hut and perhaps =cabin in the list.
There was a problem hiding this comment.
You mean ger? See https://en.wikipedia.org/wiki/Yurt
meased
changed the title
|
@Tomasz-W wanted to see 5% lighter: 0% 5% 7% 0% 5% 7% |
|
Thanks! As 5% is too dark to distinguish them fastly, but 7% is minimally too light (too prominent) on residential areas, I would go with middle one 6%. |
|
6% version of the last image: |
|
Looks good on residential landuse.
Can we try an example with minor buildings on Religious landuse?
…On Mon, Oct 15, 2018 at 2:19 AM meased ***@***.***> wrote:
6% version of the last image:
[image: la2_6p]
<https://user-images.githubusercontent.com/14190496/46919842-3ff90e00-cf9a-11e8-9d71-87f017a21491.png>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3426 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AoxshNMFTgzx5RQ88T8qrGWGs94PD4yuks5uk3IjgaJpZM4XJdL7>
.
|
|
@meased, can you try a couple of tests with 6% and the new yellowish societal amenities color? |
|
I think it's important to test it on different backgrounds where one can find buildings (for example |
|
@meased My proposition is to start with just the code restructuring as a separate PR, so testing would be easier and less error prone, and next making another PR based on this which will be focused only on making minor category and visual testing the colors. |
I think I'll do this. |
|
Done. This PR is now just a rewrite of the buildings SQL and style sheet. There should be no visual differences. I have tested this in Kosmtik by flipping the overlay of the standard map on and off and I can't see any difference on any zoom level. |
meased
changed the title
|
Great! Since there is a change from SQL code to CSS code, there might be some performance difference. @rrzefox Could you test this difference? |
|
Well, this statement was not accurate - it's rather simpler both SQL and CSS code, so there should be no problems with performance. I will just wait with this until v4.16.0 is out (I plan to do this 19.10) and probably merge this PR soon after that. Then we can start testing minor buildings PR. |
|
Now we can restart working on new buildings categorization. |
|
BTW: making major buildings lighter would also fix #3071. |
|
@meased What about further tuning? Do you plan to get back to the proposed minor and major buildings changes? |
|
The problem here is that buildings need to simultaneously serve as a background for our lightest foreground (I believe that is gastromony I'm not convinced that all of these criteria can be met, hence the inactivity. Regardless, I could start a PR so that these problems could be investigated. |
|
That's the kind of thinking we need! Feel free to propose dropping/altering any of these criteria if you think it would make sense. By the way, just curious, what's your username on openstreetmap.org? |
|
|
Please remember about discussed half-transparency for some buildings (#552 (comment)). |
(Most likely) a prerequisite for #2532
This PR simplifies the buildings SQL query and moves all styling decisions out of SQL and into CSS (as the software gods intended 😉).
No visual changes.
SQL
Before, normal buildings and major building were queried separately with each query specifically omitting the results of the other. This made it difficult to redefine the categories (and very difficult to add a third category).
The SQL query now queries all buildings together, along with any fields needed to make styling decisions:
CSS
Now that the buildings class has access to all the fields needed, the CSS can be written in one block. As you can see, it is now very easy to reclassify buildings and add/remove the minor class: