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

Buildings code rewrite #3426

Merged
merged 13 commits into from
Oct 19, 2018
Merged

Buildings code rewrite #3426

merged 13 commits into from
Oct 19, 2018

Conversation

meased
Copy link
Contributor

@meased meased commented Oct 5, 2018 •
edited
Loading

(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:

     (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;
      }
    }
  }

@jeisenbe
Copy link
Collaborator

jeisenbe commented Oct 5, 2018 via email

@kocio-pl
Copy link
Collaborator

kocio-pl commented Oct 5, 2018

I like the idea of these complex changes. Did you however test the performance? We use SQL a lot to achieve reasonable speed.

@kocio-pl
Copy link
Collaborator

kocio-pl commented Oct 5, 2018

Could you also test minor buildings darker, like half the way between the proposed color and standard buildings?

@meased
Copy link
Contributor Author

meased commented Oct 5, 2018

Minor buildings no change:
0perc

Minor buildings 4% lighter:
4perc

Minor buildings 7% lighter (same as screen shots above):
7perc

@meased
Copy link
Contributor Author

meased commented Oct 5, 2018

Major buildings no change:
maj2_20perc
maj_20perc

Major buildings 15% darker:
maj2_15perc
maj_15perc

Major buildings 10% darker (same as screenshots above):
maj2_10perc
maj_10perc

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.

@meased
Copy link
Contributor Author

meased commented Oct 5, 2018

Did you however test the performance?

No.

Anyone know how to run benchmarks in docker?

@jeisenbe
Copy link
Collaborator

jeisenbe commented Oct 5, 2018 via email

@meased
Copy link
Contributor Author

meased commented Oct 5, 2018

That's not that complicated. Worth a test rendering at least.

@kocio-pl
Copy link
Collaborator

kocio-pl commented Oct 6, 2018

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?

@matthijsmelissen
Copy link
Collaborator

See also #68 and #490.

I'm not sure if I really see the advantage of rendering minor buildings lighter.

@mboeringa
Copy link

I'm not sure if I really see the advantage of rendering minor buildings lighter.

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.

@Tomasz-W
Copy link

Tomasz-W commented Oct 6, 2018 •
edited
Loading

@meased Can you test lighter outline for minor buildings? It looks a little bit like some holes in ground currently.

@mboeringa
Copy link

@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.

@meased
Copy link
Contributor Author

meased commented Oct 6, 2018

I think that is mainly a consequence of the relatively dark grey of the landuse=residential

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:
base_minor

@kocio-pl
Copy link
Collaborator

kocio-pl commented Oct 6, 2018 •
edited
Loading

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.

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 building=yes (only about 50 mln out of 300 mln of all the buildings have any other value!). I don't see how could we attract people to more precise tagging other than showing them a bit different as a "minor" super-class.

taghistory 55
taghistory 56

@meased I see that you have added carpark type, but omitted collapsed - was there any reason for this? I have tried to stay above 10k limit, but we don't have to stick to it, there are also some other types that make sense for me, what do you think about it?

taghistory 57

@jragusa
Copy link
Contributor

jragusa commented Oct 7, 2018

Please also consider underground #552 and roof #2475 buildings to get a consistent rendering.

@meased
Copy link
Contributor Author

meased commented Oct 7, 2018

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'] {

@meased
Copy link
Contributor Author

meased commented Oct 7, 2018 •
edited
Loading

Playing with @jeisenbe's idea of variable major building darkness:

14
a14
15
a15
16
a16
17
a17

14
c14
15
c15
16
c16
17
c17

@Tomasz-W
Copy link

Tomasz-W commented Oct 7, 2018 •
edited
Loading

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.
Just to be clear: I very like the idea proposed in this PR, it just need some tuning IMO.

HolgerJeromin
HolgerJeromin reviewed Oct 7, 2018
View reviewed changes
buildings.mss Outdated
[building = 'stable'],
[building = 'cowshed'],
[building = 'greenhouse'],
[building = 'ger'],
Copy link
Contributor

Choose a reason for hiding this comment

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

What is that?

I miss =hut and perhaps =cabin in the list.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You mean ger? See https://en.wikipedia.org/wiki/Yurt

@meased
Copy link
Contributor Author

meased commented Oct 14, 2018

More testing (at 7% lighter):

https://www.openstreetmap.org/#map=17/45.58231/-122.73927
sj_b
sj_a

https://www.openstreetmap.org/#map=17/45.15308/-122.84857
wb_b
wb_a

https://www.openstreetmap.org/#map=18/45.46846/-122.73448
hv_b
hv_a

https://www.openstreetmap.org/#map=17/45.50819/-122.64908
la_b
la_a

aly_b
aly_a

@meased meased changed the title Buildings code rewrite, major/normal/minor style changes Lighten minor buildings, buildings code rewrite Oct 14, 2018
@meased
Copy link
Contributor Author

meased commented Oct 14, 2018

@Tomasz-W wanted to see 5% lighter:

0%
la1_0p

5%
la1_5p

7%
la1_7p

0%
la2_0p

5%
la2_5p

7%
la2_7p

@Tomasz-W
Copy link

Tomasz-W commented Oct 14, 2018 •
edited
Loading

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%.

@meased
Copy link
Contributor Author

meased commented Oct 14, 2018

6% version of the last image:
la2_6p

@jeisenbe
Copy link
Collaborator

jeisenbe commented Oct 14, 2018 via email

@Adamant36
Copy link
Contributor

Adamant36 commented Oct 15, 2018 •
edited
Loading

@meased, can you try a couple of tests with 6% and the new yellowish societal amenities color?

@kocio-pl
Copy link
Collaborator

I think it's important to test it on different backgrounds where one can find buildings (for example landuse=garages or landuse=commercial), not just the bare ground. They will look different there, so it's too early for precise shade tuning.

@kocio-pl
Copy link
Collaborator

@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.

@meased
Copy link
Contributor Author

meased commented Oct 17, 2018

My proposition is to start with just the code restructuring as a separate PR

I think I'll do this.

@meased
Copy link
Contributor Author

meased commented Oct 17, 2018

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 meased changed the title Lighten minor buildings, buildings code rewrite Buildings code rewrite Oct 17, 2018
@kocio-pl
Copy link
Collaborator

Great! Since there is a change from SQL code to CSS code, there might be some performance difference. @rrzefox Could you test this difference?

@kocio-pl
Copy link
Collaborator

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.

@kocio-pl kocio-pl merged commit 6ac2d9c into gravitystorm:master Oct 19, 2018
@kocio-pl
Copy link
Collaborator

Now we can restart working on new buildings categorization.

@kocio-pl
Copy link
Collaborator

BTW: making major buildings lighter would also fix #3071.

@jragusa jragusa mentioned this pull request Nov 27, 2018
Open
@kocio-pl
Copy link
Collaborator

kocio-pl commented Jan 3, 2019

@meased What about further tuning? Do you plan to get back to the proposed minor and major buildings changes?

@meased
Copy link
Contributor Author

meased commented Jan 4, 2019

The problem here is that buildings need to simultaneously serve as a background for our lightest foreground (I believe that is gastromony #C77400) and as a foreground to our darkest landusage (which I believe is religous #d0d0d0, but residential is also a problem), all while having three brightnesses of minor, normal, and major which are all distinguishable from each other.

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.

@meased meased deleted the buildings branch January 4, 2019 21:59
@matthijsmelissen
Copy link
Collaborator

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?

@meased
Copy link
Contributor Author

meased commented Jan 4, 2019

what's your username on openstreetmap.org?

cowdog

@Tomasz-W
Copy link

Tomasz-W commented Jan 5, 2019

Please remember about discussed half-transparency for some buildings (#552 (comment)).

@meased meased mentioned this pull request Jan 21, 2019
Open
@meased meased mentioned this pull request Jan 28, 2019
Merged
@jeisenbe jeisenbe mentioned this pull request Feb 12, 2019
Closed
@jc86035 jc86035 mentioned this pull request Apr 11, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HolgerJeromin HolgerJeromin HolgerJeromin left review comments

@kocio-pl kocio-pl kocio-pl left review comments

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

Successfully merging this pull request may close these issues.