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

building outlines are inconsistent #68

Closed
pnorman opened this issue Jun 13, 2013 · 32 comments · Fixed by #490 or #1153
Closed

building outlines are inconsistent #68

pnorman opened this issue Jun 13, 2013 · 32 comments · Fixed by #490 or #1153

Comments

@pnorman
Copy link
Collaborator

pnorman commented Jun 13, 2013

Buildings either have or don't have an outline, with no reason apparent to the user. It is not obvious what tags cause it without detailed inspection

@gravitystorm
Copy link
Owner

It's not obvious to me either! I'd never noticed before.

From looking at the stylesheet in more detail, it makes a distinction between "light" buildings (presumably "light" means "less important") and standard buildings. building=residential is light, but building=yes is standard.

From a personal point of view I'd like to encourage less blanket use of building=yes, which dominates buildings at 92.4%. But from a cartography stand point, I'm not certain what's best. The lack of outline on (adjoining) terraced houses doesn't look great at high zoom.

@ingalls
Copy link

ingalls commented Jun 13, 2013

+1, the lighter buildings drive me nuts!

@ian29
Copy link
Contributor

ian29 commented Jun 13, 2013

From a data-agnostic cartographic standpoint, the building outlines would ideally fade in so as to add more definition at higher zoom levels (ie what @gravitystorm mentions above) and less clutter at lower zoom levels.

If buildings with different tags need to be differentiated, their visual rendering should be more substantially different than outline/no-outline.

@vincentdephily
Copy link

Looking at the code, the outline-or-no-outline is determined by the sql query, with ('residential','house','garage','garages','detached','terrace','apartments') getting no outline. That's certainly the first thing to fix. Insofar as we want a "light building" type, I propose to replace the current list with this one (looking at the fist few pages in taginfo:

garage,garages,roof,service,shed,cabin,storage_tank,tank,shelter

I am not offering a patch myself because I find it odd that that list is maintained in the sql query rather than the mss file, where it would be easyer to maintain. Is there a reason for this other than a historical one ?

@vincentdephily
Copy link

The second thing we might want to fix is the list of what is not a building. In addition to 'no,planned' I suggest 'entrance,collapsed,construction,ruins,ruin'.

@vincentdephily
Copy link

Last bit of weirdness I'd like to address is the case of supermarket, station, and place_of_worship which are "visible" from zoom 10 onward and treated specially. However much I squint my eyes at zoom 11, buildings are too small to see. Even huge american supermarkets would probably be only a few pixels large at this zoom, and drowned in the other features, mainly roads.

So I think we should get rid of the buildings-lz query altogether (speeding up rendering as we go), and transfer the settings such as the supermarket's pink color (awfully ugly but that's a different issue) into the mss for #buildings

@dieterdreist
Copy link

all buildings should have outlines because otherwise they merge visually into a huge blob in the rendering

@cquest
Copy link

cquest commented Aug 23, 2013

@vincentdephily it is more efficient to have "light/non-light" building being handle as the query level, this simplifies the final XML and thus mapnik load a lot.

@vincentdephily
Copy link

@cquest Thanks for the explanation. Is there a cartocss best-practices document available ?

@dieterdreist True, but there probably is still value in distinguishing "lesser" buildings ? Perhaps instead of removing the outline we can make it lighter ?

@mrwojo
Copy link
Contributor

mrwojo commented Oct 18, 2013

Buildings should all be rendered the same, IMO, except perhaps if the tagging indicates a less-than-real building state (e.g., building=no or building=construction). Other than that I agree with what @vincentdephily has said.

@dieterdreist
Copy link

2013/10/18 mrwojo [email protected]

Buildings should all be rendered the same, IMO

IMO not

@vincentdephily
Copy link

I like having "lesser" (sheds etc) buildings rendered differently too. But as @dieterdreist said, they should probably all have outlines, for clarity. I wanted to send a patch for that, but haven't found the time to do the inital setup for artistic testing.

The definition of a "lesser building" is subjective, do people agrre with my proposed list ? As for "non-buildings", it should be less controversial.

@mrwojo
Copy link
Contributor

mrwojo commented Oct 18, 2013

There's an argument to be made that residential buildings are a simpler group that benefits from unique styling because, unlike more public structures, building=* is often all they have to describe their function. building=shelter, for instance, can be used with amenity=shelter, which shows a rain icon that's larger than the shelters I've seen on the map.

We don't need to couple this fix with a new style for a different group of buildings. Let's just give houses their outlines and then continue the other parts of this discussion in other issues.

vincentdephily added a commit to vincentdephily/openstreetmap-carto that referenced this issue Oct 21, 2013
As discussed in issue gravitystorm#68, remove various kinds of residential values, and add a few other values.
vincentdephily added a commit to vincentdephily/openstreetmap-carto that referenced this issue Oct 21, 2013
As discussed in issue gravitystorm#68, remove station, supermarket, and add a few values.

I refrained from adding common values such as '', '*', 'undefined', as they should be corrected in the database itself.
@pauliuszaleckas
Copy link

Big ACK for @vincentdephily patches.
I really hate that residential is "light" and shed is darker building.

@HolgerJeromin
Copy link
Contributor

And please get rid of the special supermarket pink. :-)

@Rovastar
Copy link
Contributor

+1 to all the above.
And who on earth had the idea for that pink for supermarkets!?

@pnorman
Copy link
Collaborator Author

pnorman commented Nov 28, 2013

I like having "lesser" (sheds etc) buildings rendered differently too.

From a personal point of view I'd like to encourage less blanket use of building=yes,

What's a good way to visually distinguish building=<something appropriate> from building=yes/building=<something unknown>? We want to "reward" people using building=house, building=garage, etc. instead of a generic or typoed tag. I'm just not sure how to do it, and frankly, the current style doesn't succeed well at it.

@vincentdephily
Copy link

The only weapon we have against typoed tags is using a big whitelist (instead of the small blacklist that we currently use). I'm generally in favor of big whitelists (see #192), it does have pros and cons.

To encourage people away from "yes" we could style it differently from everything else, but that'd go counter to the current usage of "yes". I don't think that the standard map style is the right medium to discourage "yes" (yet).

The way to encourage many values is different rendering per value, like we currently do for "light" buildings. But it's easy to go overboard with that. I'd rather evaluate each new building category in a different issue, and keep this issue to the point.

My 2 commits address the definition of the "light" and "non" buildings, but not the styling of light buildings. I had a commit for that too, not sure where it went. I'll try to redo it this weekend, so we can merge this and move on to more finnicky bikesheding :p

@mozzbozz
Copy link

Well, I noticed this as well. It looks very unclear to the user why there are two buildings next to each other and one is rendered "light" and one not. Especially because the "light" rendering wiht "building=house" is transparent and therefore meadows or something can still be seen "through" the building. Example on map: http://www.openstreetmap.org/?mlat=47.64157&mlon=9.16827#map=19/47.64157/9.16827 in my POV it looks more than ugly if there is an transparency set.

@Circeus
Copy link

Circeus commented Feb 19, 2014

Can I ask about the status of this and the offered commits? The fact that a building=apartment connected to a building=roof causes only the latter to have an outline is just embarrassing.

If you want to encourage people away from building=yes, I'm all for making THAT have no outline. I'm sure people would get on it with lightning speed! "Fixing the rendering" has historically been a major encouragement.

@gravitystorm
Copy link
Owner

"offered commits"? This is just a discussion, not a pull request.

@vincentdephily
Copy link

I did propose 2 commits here 4 months ago (maybe not as a proper merge request, sorry if that's the case), but as they were an incomplete solution I didn't push on. Sorry for the delay, osm-carto is a side side side project for me. However I'm going to an OSM hack day on the oth of march and I will finish this up then (yet no hard feelings if somebody beats me to the punch :p).

matthijsmelissen added a commit to matthijsmelissen/openstreetmap-carto that referenced this issue Apr 21, 2014
- The building types residential, house, detached, terrace and
  apartments are no longer light.
- The building types roof, service, shed, shelter, cabin,
  storage_tank, tank, support, glasshouse, mobile_home, kiosk
  silo, canopy and tent are now light.

This commit is based on a comment by @vincentdephily:
gravitystorm#434 (comment)

This closes gravitystorm#68 and supersedes gravitystorm#434.
@pnorman
Copy link
Collaborator Author

pnorman commented May 22, 2014

Reopening this because the reasoning behind the differences is fairly complex, and does not obviously convey something to the viewer - given the debate in #490 and other tickets about what light means, I'm not entirely clear myself!

@pnorman pnorman reopened this May 22, 2014
pnorman added a commit to pnorman/openstreetmap-carto that referenced this issue May 22, 2014
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
@matthijsmelissen
Copy link
Collaborator

I have a slight preference for getting rid of the distinction altogether.

@EdLoach
Copy link

EdLoach commented May 23, 2014

I found this when looking to see why the line had appeared around houses again - there is nothing special about them and the map looked better without the lines - so building with lines are something more interesting, such as shops, offices and the like. Please remove it again.

@matthijsmelissen
Copy link
Collaborator

The previous rendering distinguished buildings tagged as building=yes and building=residential, which did not make much sense and looked really ugly. It also did not accomplish what you mention (buildings with lines are more interesting) because building=yes is not interesting. Our first priority was to get rid of this meaningless distinction.

@pnorman is currently working on a new version of the building rendering. Is there a already Github issue for his changes, by the way?

@EdLoach
Copy link

EdLoach commented May 23, 2014

But if I accidentally leave the tag as building=yes (e.g. from using JOSM terrace plugin) then having a building with a line around in a residential area stands out and makes things easier to fix. If we're trying to encourage people not to use building=yes then rendering houses differently helps. An alternative would not to render building=yes at all until it is tagged more accurately, but I feel that would lead to people guessing. I realise I could use something like http://www.itoworld.com/map/122 but that doesn't negate the fact that houses now just look worse.

@matkoniecz
Copy link
Contributor

I would say that map now looks better.

I wonder whatever rendering building=yes 'worse' (transparent or something) to encourage better tagging would be a good idea.

I am worried that people would start tagging sheds etc as building=yes to render them as less important.

@pnorman
Copy link
Collaborator Author

pnorman commented May 23, 2014

@pnorman is currently working on a new version of the building rendering. Is there a already Github issue for his changes, by the way?

No, benchmarking it, then it'll probably take an hour or more to write up a pull request

pnorman added a commit to pnorman/openstreetmap-carto that referenced this issue May 25, 2014
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
@vincentdephily
Copy link

87% of the buildings in the db are buildings=yes. So for better or worse, we have to render "yes" and we have to assume that those building belong to the most common group, which is normal (non-light) buildings. I know we'd like to discourage building=yes, but it's better to use a carrot than a stick.

The original code had an incoherent definition of what a "light building" was, but I believe we've mostly fixed that and have a useful list now. As I see it those light buildings are one whitelist of specially-rendered buildings, but we also have other whitelists like churches and train stations.

One of the original annoyance of light buildings is their lack of outline even at high zoom. #568 is my suggested fix.

Performance-wise, #568 should be a tiny fraction faster, because lower zooms don't render light buildings anymore.

@matthijsmelissen
Copy link
Collaborator

Closed by #490.

matthijsmelissen pushed a commit to matthijsmelissen/openstreetmap-carto that referenced this issue Oct 1, 2014
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
pnorman added a commit that referenced this issue Oct 16, 2014
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 #520. Superceeds #550.

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

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

Rebased in 6b2a4de by math1985 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet