mapbox / mapnik-vector-tile

Mapnik implemention of Mapbox Vector Tile specification
BSD 3-Clause "New" or "Revised" License
553 stars 117 forks source link

Updated the way that painted is determined within vector tiles #150

Closed flippmoke closed 9 years ago

flippmoke commented 9 years ago

@springmeyer

springmeyer commented 9 years ago

@flippmoke - can you please write a brief description of the motivation of the change and what it does?

Overall looks good. One thing I'm not totally comfortable with is the success = success | encode_geometry(poly, current_feature, start_x, start_y); syntax. I would prefer:

if (encode_geometry(poly, current_feature, start_x, start_y))
{
    success = true;
}

which I find easier to read.

flippmoke commented 9 years ago

This is a change so that painted is decoupled from the number of points (even in vectors) that are added to the protobuff. This replaces the passing of unsigned ints with the count of vertices with the passing of a boolean showing success of encoding a geometry or set of geometries.

The purpose of the painted is really to determine if the tile is empty or not prior to selecting data for the tile.

This change also increases the likelyhood that polygons are marked as painted the reasoning behind this is that it is very likely that some logic will distort the data such that it will make a tile empty and it will not be marked as painted.

CleanPolygon https://github.com/mapbox/mapnik-vector-tile/blob/7e480521b42084cb5f9ca78ca6b52a7d62a96acb/src/vector_tile_processor.ipp#L1028

CleanPolygon is likely to remove spikes and figures with no areas because they could have repeated points. It also does a simplification along points to remove collinear points. This can remove geometries. To solve this problem all geometries are treated as painted if they collide with the extent of the original geometry even if clipping is outside that extent.

area_threshold https://github.com/mapbox/mapnik-vector-tile/blob/7e480521b42084cb5f9ca78ca6b52a7d62a96acb/src/vector_tile_processor.ipp#L1030-L1032

The area threshold can remove a geometry if its area is too small, but should still be marking the tile as painted. The fix is the same as CleanPolygon.

simplification https://github.com/mapbox/mapnik-vector-tile/blob/7e480521b42084cb5f9ca78ca6b52a7d62a96acb/src/vector_tile_processor.ipp#L1295-L1296

Prior to clipping we apply simplification, this can possibly through spikes remove a geometry from being in a tile pyramid during rendering therefore, this can possibly falsely report a tile as not painted. This should be fixed in this because we take the extent of the geometry to determine if it is in the initial clipping set therefore, it should be painted still. Simplification is unlikely to change extent.

AddPath https://github.com/mapbox/mapnik-vector-tile/blob/7e480521b42084cb5f9ca78ca6b52a7d62a96acb/src/vector_tile_processor.ipp#L1295-L1296

It is possible that there is not sufficient conditions for the clipper to add a specific geometry. This is rare but involves collinear points with little to no area in the polygon. If this occurs it could remove geometry. The fix is the same used for CleanPolygon.

flippmoke commented 9 years ago

@springmeyer updated to use if statements

springmeyer commented 9 years ago

Great, LGTM to merge once travis is green.