tilezen / vector-datasource

Tilezen vector tile service - OpenStreetMap data in several formats
https://www.nextzen.org/
Other
502 stars 119 forks source link

Clip buildings layer with larger region #197

Closed rmarianski closed 8 years ago

rmarianski commented 8 years ago

There are certain features in the buildings layer that can span multiple tiles at high zooms, which is problematic for rendering. Eg the highline in nyc

Instead of clipping on the tile boundary, we can extend the clip region a tile's worth in every direction, ie a 3x3 grid.

nvkelso commented 8 years ago

More from the Trello thread:

The way buildings are exported now is the feature is duplicated into each tile it touches. The proposal here is to start clipping them, but not to a the current tile – rather a 3x3 grid centered on the current tile. (Clipping them to the current tile alone was found to have issues a while ago.)

From @bcamper:

This graphics bug is a consequence of vertex packing. We're packing to signed shorts, which have a range of +/- 16384. Tile geometry is encoded in range +/- 4096, which means the attribute can hold a geometry spanning up to 4 tiles. The High Line... is bigger than that :/

It doesn't seem like a good thing to have geometries this far out of tile bounds. I will try to find a client-side solution to make us more robust to this, but I also think we should look at clipping anything beyond a certain range, like more than 100% outside the tile bounds (so give a full tile buffer area), @nathanielkelso @robm19?

Here's another view of the High Light in OSM (relation this time):

At zoom 18 the High Line geometry goes everywhere (thanks @meetar for reporting this):

screen_shot_2015-09-10_at_2 23 33_pm
nvkelso commented 8 years ago

Talking with @bcamper more about this today, it sounds like this is not a problem in Tangram ES because of implementation details, but we should still fix this for Tangram WebGL (but lower priority). Moving this to Compilation 2.1 milestone to load balance.

nvkelso commented 8 years ago

Turns out this is an issue for Tangram after all, pulling into v0.7 release.

bcamper commented 8 years ago

Thanks!

bcamper commented 8 years ago

We just haven't been noticing it in JS, but yeah, still an issue. I had a proof of concept that fixed this client-side, but it's an awful lot of work to put every tile through for the very rare cases where it is actually needed, so I think doing it upfront on the server is the way to go.

zerebubuth commented 8 years ago

Unfortunately, this isn't without visual side effects... :crying_cat_face: (near 18/40.74773/-74.00499)

high-line-z18-bugs

@bcamper, is there anything we can do to mitigate this?

bcamper commented 8 years ago

Hmm, interesting issue, this is happening because we exclude edges that fall on the boundary of the current tile, but these are actually lines that while still falling on a tile boundary, it's over a full tile away from the one being rendered :)

Discussing with @blair1618, there are a couple things we'd like to look at:

migurski commented 8 years ago

What about clipping, but serving LineString stroke geometries separately from Polygon fill geometries, and styling accordingly?

bcamper commented 8 years ago

My primary concerns with that would be:

In general this has felt to me like intelligence that is reasonable to handle on the client-side (low-effort to detect a tile boundary vs. the cost of multiple filtering passes and increased tile weight)... I'm open to changing the way we approach but in that case we should take a broader look at things like polygon UV generation which are not ideal already (@meetar and @zerebubuth did some spitballing on this in NYC a couple months ago).

bcamper commented 8 years ago

(I should add that there are definitely places where I think different geometry representations for the same logical feature are a good approach -- an example is the point geometries we added for label placements, because it was just not practical to derive good label placements client-side, especially given the limited knowledge each tile had when a polygon was split across multiple tiles.)

zerebubuth commented 8 years ago

Can we make these tiles available on a dev server somewhere so we can try them out?

Absolutely, if the PRs get :+1:ed by the end of the day, then they'll be in dev tomorrow-ish.

We might want to bite the bullet and try clipping all buildings to the current tile bounds, like we do for other geometry.

Yup, this is pretty easy to try out, it's a one-line config change.

Related note: is the Highline really best classified as a building?!

I wouldn't like to venture an opinion that... seems like it might have some History behind it. :wink:

If you'd prefer a more traditionally large building, we could use LIGO, or the other LIGO, or SLAC, or the Robert-Bourassa dam, or the Bailly generating station, etc...

What about clipping, but serving LineString stroke geometries separately from Polygon fill geometries, and styling accordingly?

Yes, we could totally do that, and already do for water boundaries. Its downside is that it'll increase the size of tile, although hopefully some of that would be reclaimed by TopoJSON and/or compression.

bcamper commented 8 years ago

Water boundaries are a good example where the stroke you want to render is the coastline, which is logically a different feature; it's not coincident with the underlying polygons, because water bodies get split into many polygons but the seams between them are often not on tile boundaries.

zerebubuth commented 8 years ago

From my local machine, here's a test with single-tile clipping. It's not looking so great, but I figure that's because I need to tune something in the scene file?

high-line-z18-single-tile-clip

bcamper commented 8 years ago

Hm, you shouldn't have to do anything to enable the edge clipping (which is why you don't see them on landuse polys for instance). What's the scene file you're using there? Maybe something I'm forgetting.

On Wed, Jan 13, 2016 at 2:31 PM, Matt Amos notifications@github.com wrote:

From my local machine, here's a test with single-tile clipping. It's not looking so great, but I figure that's because I need to tune something in the scene file?

[image: high-line-z18-single-tile-clip] https://cloud.githubusercontent.com/assets/271360/12305360/49befd18-ba2c-11e5-9e56-6d595ad43e79.jpg

— Reply to this email directly or view it on GitHub https://github.com/mapzen/vector-datasource/issues/197#issuecomment-171407927 .

zerebubuth commented 8 years ago

This is the latest version of Tangram Skin and Bones. With minor modifications to point at my local tile server.

bcamper commented 8 years ago

Wonder if it's related to extrusion, what happens if you change this to false?

https://github.com/nvkelso/tangram-skin-and-bones/blob/gh-pages/scene.yaml#L1066

nvkelso commented 8 years ago

:arrow_up: @zerebubuth?

zerebubuth commented 8 years ago

Sadly, looks like extrusion isn't the culprit:

high-line-z18-single-tile-clip-no-extrusion

bcamper commented 8 years ago

Oh, hehe, the tile edges are set to explicitly draw (I believe this was inadvertently copy-pasted from one of Patricio's old styles):

https://github.com/nvkelso/tangram-skin-and-bones/blob/gh-pages/scene.yaml#L3259

tile_edges: true

Just drop that line :)

zerebubuth commented 8 years ago

Yup, that looks pretty good:

high-line-z18-single-tile-clip-no-tile-edges

@nvkelso shall we take this to dev and check that there aren't any other rendering artefacts there?

bcamper commented 8 years ago

Let's look in dev. Things I think could be an issue: UVs in Eraser Map (almost certainly), seams in 3D from building extrusion, either in Tangram JS or ES or both.

nvkelso commented 8 years ago

(Outdated: Let's leave this at 3 tiles and get on dev so more people can experiment with it?)

nvkelso commented 8 years ago

Fixed in: https://github.com/nvkelso/tangram-skin-and-bones/commit/25f1e386a0247dc3c61656219fde5c20df8513ee

bcamper commented 8 years ago

@nvkelso is that in EM too?

nvkelso commented 8 years ago

Wasn't present in EM.

zerebubuth commented 8 years ago

I've added #486 to track the u, v issues, which I think we can solve separately.

I've left the tile clip at 1 for now, as I think that setting it to 3 would stop the tile_edges: false working?

bcamper commented 8 years ago

Yes that's right, with current Tangram behavior the tile clip at 3 would cause an issue, but we think we could make it work with some minor changes. This could be a preferable temporary fix depending on how Eraser Map looks. Happy to try a boundary of 1 tile first to see how bad the damage is though :)

nvkelso commented 8 years ago

We should also think of other clients like D3 and Mapbox Studio – I suspect a larger tile clipping strategy would work better for them / show less obvious bugs for styles there?

zerebubuth commented 8 years ago

Mapbox Studio, I think, won't render anything outside the extent of the tile, so the 3x clip would probably work for not introducing rendering artefacts. I'm not sure how D3 works and whether it clips / masks the draw area per tile - if not, then it wouldn't make a difference whether the clip is 3x or 1x.

bcamper commented 8 years ago

Right, and for Mapbox we might need to start buffering everything (or have an option for that anyway). I'm not a fan of buffering everything, it feels wasteful to me (adds tile weight and overdraw for something that seems reasonably resolvable client-side), but I do want to maintain compatibility where possible. This needs more research though, I suggest we keep it out of this immediate issue and just do something that works for EM for now?

nvkelso commented 8 years ago

Yes, let's take to dev.

nvkelso commented 8 years ago

Looks good to me in Eraser Map & Skin and Bones.

@bcamper for final Tangram sign-off.

I reviewed the follow tiles (zoom 16 and 19), previewing the geoms in QGIS for prod versus dev.

screen shot 2016-01-20 at 15 55 01 screen shot 2016-01-20 at 15 56 29
nvkelso commented 8 years ago

Hmmm, in both Skin & Bones and Eraser Map I'm seeing seams in the rendered map for the Highline at tile bounds. I thought we had fixed that with the tile_edge change? What else do we need to do to fix this? Was there a Tangram fix we're relying on?

zerebubuth commented 8 years ago

Our options were:

  1. Do no clipping, have the whole building. This was causing issues when very large buildings overflowed the tile local coordinate system.
  2. Clip to tile boundaries, which has special support in Tangram. This causes issues because it resets the u, v texture coordinates at tile boundaries, so building textures look funky. #486 would fix this, but needs work both in the tiles and Tangram.
  3. Clip to 3x tile boundaries, which has no special support in Tangram (AFAIK - @bcamper would know better). This causes tile boundary artefacts with very large buildings, and will have texture issues too, but it avoids boundary artefacts and the texture issue for most buildings.

We initially did (2), but the texture / tile edge artefacts meant we switched to (3). At the moment (AFAIK) none of the currently possible options is completely free from issues.

nvkelso commented 8 years ago

I'm still fine with (3), and talking with Brett last night it sounds like Tangram needs to be rev'd to support this. So @bcamper to add new issues and relate them here. But we're fine on the tiles end.

bcamper commented 8 years ago

Yes, we are good to go with tiles. I think this 3-tile-clipping was just the least-worst option, because 1) it only generates artifacts for very large features, and 2) the artifacts can be fixed with a small (if somewhat hacky) Tangram change (which I will document). We can later explore going back to 1-tile clipping (and the buffering discussion will become part of that too).

On Thu, Jan 21, 2016 at 8:57 AM, Nathaniel V. KELSO < notifications@github.com> wrote:

I'm still fine with (3), and talking with Brett last night it sounds like Tangram needs to be rev'd to support this. So @bcamper https://github.com/bcamper to add new issues and relate them here. But we're fine on the tiles end.

— Reply to this email directly or view it on GitHub https://github.com/mapzen/vector-datasource/issues/197#issuecomment-173633638 .

bcamper commented 8 years ago

Fix for JS in https://github.com/tangrams/tangram/pull/237, will need to add to ES as well.

matteblair commented 8 years ago

Will add in ES - hang tight. Update: https://github.com/tangrams/tangram-es/pull/487

matteblair commented 8 years ago

Fix merged in ES, all systems go.

nvkelso commented 8 years ago

Thank you!

On Jan 21, 2016, at 14:32, Matt Blair notifications@github.com wrote:

Fix merged in ES, all systems go.

― Reply to this email directly or view it on GitHub.