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

Rewrite buildings #1153

Merged

Conversation

matthijsmelissen
Copy link
Collaborator

pnorman and others added 9 commits October 15, 2014 17:27
Refactor buildings code

Replace the old buildings SQL and MSS. This involves resulting changes to landcover stylings to handle landcover which was previously in buildings.mss.

Stops rendering supermarkets in a crazy pink to fix gravitystorm#520. Superceeds gravitystorm#550.

Moves the rendering of train station areas to landcover. Fixes gravitystorm#327. Fixes gravitystorm#389

Removes outline differences based on a distinction that no one fully understands. Superceeds gravitystorm#533. Fixes gravitystorm#68

Rebased in 6b2a4de by math1985 <[email protected]>
This adds the landcover labelling, which is significant
Conflicts:
	landcover.mss
	project.mml
	project.yaml
…across all levels

even the largest building in the world by footprint would be barely visible on z10 z11 so old building_major layer was not useful
now major buildings (place_of_worship for start) are displayed specially across all zoom levels with visible buildings
separate layer is necessary to ensure that building line of major building will be rendered on top of building line of touching building
note that major buildings will be rendered on top of not major buildings, including major buildings with lower layer tag
note that now aeroway terminal without building tag now will not be displayed
…gs_rewrite_new

Conflicts:
	project.mml
	project.yaml
This should improve performance.
@matthijsmelissen
Copy link
Collaborator Author

Not yet ready for merge.

@pnorman
Copy link
Collaborator

pnorman commented Dec 10, 2014

how about this for mss?

#buildings[zoom >= 12],
#buildings-major[zoom >= 8][zoom < 12] {
  polygon-fill: @building-fill;
  polygon-clip: false;
  [zoom >= 15] {
    line-color: @building-line;
    line-width: .75;
    line-clip: false;
  }
  [aeroway = 'terminal'] {
    polygon-fill: @building-aeroway-fill;
    polygon-clip: false;
    [zoom >= 15] {
      line-width: .75;
      line-clip: false;
      line-color: @building-aeroway-line;
    }
  }
  [amenity = 'place_of_worship'] {
    polygon-fill: @building-major-fill;
    polygon-clip: false;
    [zoom >= 15] {
      line-width: .75;
      line-clip: false;
      line-color: @building-major-line;
    }
  }
}

You'll then need SQL that returns just important buildings at low zoom and all buildings at high zoom, and aeroway and amenity columns at all zooms.

Do you think we need to render aeroway=terminal and amenity=place_of_worship without a building tag like a building? It makes the SQL a pain.

@matkoniecz
Copy link
Contributor

Do you think we need to render aeroway=terminal and amenity=place_of_worship without a building tag like a building? It makes the SQL a pain.

No, IMHO there is no reason to do this: http://wiki.openstreetmap.org/wiki/Tag:aeroway%3Dterminal mentions that building tag should be also present and defines it as "a building at an airport where (...)". Rendering PoW not on a building unlike buildings have even its own ticket.

@pnorman
Copy link
Collaborator

pnorman commented Dec 10, 2014

4880 aeroway=terminal ways, of which 1509 have no building tag and 2 have building=no. I can't be sure without sampling that 30% has a significant portion of buildings, but I suspect so.

This is consistent with the wiki, which defines aeroway=terminal by itself as a building.

@matthijsmelissen
Copy link
Collaborator Author

No, the wiki specifies to 'Add building=yes [...] as appropriate'.

@gravitystorm gravitystorm removed their assignment Dec 10, 2014
@gravitystorm
Copy link
Owner

I'm unassigning - @math1985 please reassign when ready for merging!

@matthijsmelissen
Copy link
Collaborator Author

how about this for mss?

That doesn't work, as pointed out by @mkoniecz in pnorman#5:

separate layer is necessary to ensure that building line of major building will be rendered on top of building line of touching building

A separate attachment would work too, but would involve looping over all buildings twice.

@matthijsmelissen
Copy link
Collaborator Author

We received a lot of comments, both here and on talk. To summarize:

It has been pointed out that the new colours might not work well with:

  • barracks
  • power=generator
  • amenity=allotments
  • residential and living streets
  • pitches
    All of these issues are relatively minor, and we really need to push this issue forward, so I would propose to postpone these issues to post-merge.

Some people propose adding a major/minor building distinction for e.g. garages. I don't think a three-tier system for buildings would work well graphically, so I don't really see this as a viable idea for the moment.

There is a lot of debate about colouring places of worship darker. I counted three opinions in support and four opinions against. Among the collaborators there are also different opinions. I'm not sure what to do here.

Some people say that the buildings might be a bit darker, especially on zoom 14-.

Some people dislike the lighter colour in general, and mention that it does not look great against industrial / construction / residential background.

Some people would prefer a more simple shade of gray, such as #d6d1c8.

On low zoom levels, buildings have no outline, so they need to be a bit
darker in order to have the same visual effect.
@matthijsmelissen
Copy link
Collaborator Author

I accepted the colour proposed by @vholten . I don't think this colour is really too close to living_street, and if it is, I'd propose changing the living_street colour.

@matthijsmelissen
Copy link
Collaborator Author

I agree buildings are too faint on the levels where they have no outline (14-). I made them a bit darken on these zoom levels. Thank you @imagico for the suggestion.

osm_b222fb

@matthijsmelissen
Copy link
Collaborator Author

As far as I'm concerned, this is ready to be merged. I'm sure there will be some comments, but I don't think we can make everybody happy. Of courses, further changes are still possible.

The only open issue is whether or not to render places of worship special. I think @gravitystorm can make the final call on that one.

@talllguy
Copy link

talllguy commented Jan 3, 2015

The new buildings look great. Great work collaborators!!

@talllguy
Copy link

talllguy commented Jan 3, 2015

screen shot 2015-01-03 at 11 27 45 am
You can see it is coming in as things update.

@daganzdaanda
Copy link

Thanks @ all, this turned out pretty well!
I'm getting used to it quickly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment