Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Polygon and line symbolizers need a {polygon,line}-clip: false #34

Closed
andrewxhill opened this Issue Nov 20, 2012 · 12 comments

Comments

Projects
None yet
3 participants
Member

andrewxhill commented Nov 20, 2012

Without polygon-clip anything on deployed,

http://examples.cartodb.com/tiles/carto_tests/10/0/0.png?style=%23carto_tests+%7Bpolygon-fill%3Agreen%3B+polygon-opacity%3A0.510811186271%3B+line-color%3Agreen%3B+line-opacity%3A0.464914139497%3B+polygon-gamma%3A0.517551778162%3B+%7D&sql=WITH+hgrid+AS+%28SELECT+CDB_HexagonGrid%28ST_Expand%28CDB_XYZ_Extent%280%2C0%2C10%29%2CCDB_XYZ_Resolution%2810%29+%2A+%2816%29%29%2CCDB_XYZ_Resolution%2810%29+%2A+%2816%29+%29+as+cell%29+SELECT+hgrid.cell+as+the_geom_webmercator%2C+%27my+really+long+name%27%3A%3Atext+as+name+FROM+hgrid

Without polygon-clip on development (luisico-prod15)

http://luisico-prod15.cartodb.com/tiles/carto_tests/10/0/0.png?style=%23carto_tests+%7Bpolygon-fill%3Agreen%3B+polygon-opacity%3A0.510811186271%3B+line-color%3Agreen%3B+line-opacity%3A0.464914139497%3B+polygon-gamma%3A0.517551778162%3B+%7D&sql=WITH+hgrid+AS+%28SELECT+CDB_HexagonGrid%28ST_Expand%28CDB_XYZ_Extent%280%2C0%2C10%29%2CCDB_XYZ_Resolution%2810%29+%2A+%2816%29%29%2CCDB_XYZ_Resolution%2810%29+%2A+%2816%29+%29+as+cell%29+SELECT+hgrid.cell+as+the_geom_webmercator%2C+%27my+really+long+name%27%3A%3Atext+as+name+FROM+hgrid

With polygon-clip:false added to development

http://luisico-prod15.cartodb.com/tiles/carto_tests/10/0/0.png?sql=WITH%20hgrid%20AS%20(SELECT%20CDB_HexagonGrid(ST_Expand(CDB_XYZ_Extent(0,0,10),CDB_XYZ_Resolution(10)%20*%20(16)),CDB_XYZ_Resolution(10)%20*%20(16)%20)%20as%20cell)%20SELECT%20hgrid.cell%20as%20the_geom_webmercator,%20'my%20really%20long%20name'::text%20as%20name%20FROM%20hgrid&style=%23carto_tests%20%7Bpolygon-fill%3Agreen%3B%20polygon-opacity%3A0.510811186271%3B%20line-color%3Agreen%3B%20line-opacity%3A0.464914139497%3B%20polygon-clip%3A%20false%3B%20polygon-gamma%3A0.517551778162%3B%20%7D

@ghost ghost assigned strk Nov 20, 2012

Contributor

strk commented Nov 20, 2012

@strk strk closed this in e137dec Nov 20, 2012

While turning off clipping for lines/polygons should do no harm, I'm surprised this was needed. It may be that some other bug is at play where the clipping extent of the features is invalid - it would be good to have a reduced testcase. I am unable to reproduce this locally.

Contributor

strk commented Nov 27, 2012

The clipping is computed by tilelive-mapnik in the case above, I had troubles reproducing too, then figured you need a Z value != 0 for it to work. The input polygons are valid, we're using a buffer of 64. Metatiling is in effect (4x4) so I think what you see there is the clipping performed on the 64 pixel boundary of the 4x4 tiles picture.

Here's a simplified XML giving different results with 2.0 and 2.1. By adding clip=false in the symbolizers the result becomes the same.

<Map srs=\"+init=epsg:3857\" maximum-extent=\"-20037508.34,-20037508.34,20037508.34,20037508.34\">
<Style name=\"carto_tests\" filter-mode=\"first\" >
  <Rule>
    <PolygonSymbolizer fill=\"#ff0000\" />
    <LineSymbolizer stroke=\"#000000\" />
  </Rule>
</Style>
<Layer name=\"carto_tests\"
  srs=\"+init=epsg:3857\">
    <StyleName>carto_tests</StyleName>
    <Datasource>
       <Parameter name=\"type\"><![CDATA[postgis]]></Parameter>
       <Parameter name=\"host\"><![CDATA[127.0.0.1]]></Parameter>
       <Parameter name=\"port\"><![CDATA[5432]]></Parameter>
       <Parameter name=\"extent\"><![CDATA[-20005048.4188,-20005048.4188,20005048.4188,20005048.4188]]></Parameter>
       <Parameter name=\"simplify\"><![CDATA[true]]></Parameter>
       <Parameter name=\"geometry_field\"><![CDATA[geom]]></Parameter>
       <Parameter name=\"srid\"><![CDATA[3857]]></Parameter>
       <Parameter name=\"table\"><![CDATA[(SELECT (ST_Dump('0106000020110F0000040000000103000000010000000700000000000080D51473C136B4051591137341000000F0211573C1388840B00C137341000000D0BA1573C1388840B00C13734100000040071673C136B4051591137341000000D0BA1573C134E0CA7915147341000000F0211573C134E0CA791514734100000080D51473C136B40515911373410103000000010000000700000000000080D51473C1320C90DE99147341000000F0211573C134E0CA7915147341000000D0BA1573C134E0CA791514734100000040071673C1320C90DE99147341000000D0BA1573C12F3855431E157341000000F0211573C12F3855431E15734100000080D51473C1320C90DE991473410103000000010000000700000000000030F01373C134E0CA7915147341000000A03C1473C136B405159113734100000080D51473C136B4051591137341000000F0211573C134E0CA791514734100000080D51473C1320C90DE99147341000000A03C1473C1320C90DE9914734100000030F01373C134E0CA79151473410103000000010000000700000000000030F01373C12F3855431E157341000000A03C1473C1320C90DE9914734100000080D51473C1320C90DE99147341000000F0211573C12F3855431E15734100000080D51473C12D641AA8A2157341000000A03C1473C12D641AA8A215734100000030F01373C12F3855431E157341'::geometry)).geom) as cdbq]]></Parameter>
       <Parameter name=\"dbname\"><![CDATA[template_postgis]]></Parameter>
    </Datasource>
  </Layer>
</Map>

Thanks, I can replicate now. The problem is in grainstore not in Mapnik clipping. You should revert your changes.

I can see that the problem occurs in this sample XML because of the layer extent is smaller than the actual extent of the layer's geometries (-20013172.0,20000971.0158,-20004611.0,20011562.5064). The clipping extent is ending up wrong because you are setting a layer extent that is wrong (the clipping extent is based on the layer extent). I told you that your global extents were wrong 6 months ago! Vizzuality#7 (comment).

Really should you avoid hardcoding the global mercator extents and rather pre-calculate the extent field values based on querying the data (after it has been imported and indexed). This should lead to better performance rendering blank tiles that are outside of the layers extent. If you are thinking to just leave it blank (and let mapnik call ST_Extent internally)... well, don't because then that overhead will be in incurred per rendering thread, as you can likely imagine.

Contributor

strk commented Nov 28, 2012

I see your point. Interesting, thank you for the analisys. I'll look at that change again, but why then mapnik-2.0.0 doesn't clip ? The grainstore version is the same... Didn't the default line-clip and polygon-clip change between 2.0 and 2.1 ?

Mapnik 2.0.0 does not clip because it did not have any clipping functionality yet at all. This lack of clipping was the primary reason we did not perform better than mapserver by a larger margin in the FOSS4G WMS performance benchmark in denver.

Contributor

strk commented Nov 29, 2012

Ok, thanks, so this tickets till makes sense (we do need clip=off for the migration...)

Up to you. But I do not think you should set clip:false for lines/poly symbolization.

Contributor

strk commented Nov 29, 2012

Uhm, ok I think I see your point. The problem is probably only with markers
(and, ouch, shields and points and labels ? we tested NONE of these).
Anyway, less clipping won't hurt anyone.
Users can always drop the clip:false after checking the map visually

I'll have to write a migration guide once all of this is over, it's really being a long trip

Contributor

strk commented Nov 29, 2012

Oh Dane, I think you set clip:false by default lately, right ? was that in 2.1.x branch already ?

It is a pull request currently: mapnik/mapnik#1608. Only for shield, text, and markers because others like point and building never got support for clipping. building because its quite custom and point because it is not going to get much love since we plan to merge it with markers.

Please test that pull request if you are interested. It could potentially go into 2.1.1 and master. I need to mull it over for a few more days.

In terms of a migration guide, I've written (in case you've not seen): http://mapbox.com/tilemill/docs/upgrade/ and for developers: https://github.com/mapnik/mapnik/wiki/API-changes-between-v2.0-and-v2.1.

Contributor

strk commented Nov 30, 2012

Thank, I'm on my way to test that pull.
It won't be as easy as before now that we migrated all machines to mapnik-2.1, but will do my best.

Thanks for the migration guide!

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