CartoDB / node-mapnik

Bindings to mapnik for node.js
https://github.com/mapnik/node-mapnik
BSD 3-Clause "New" or "Revised" License
3 stars 5 forks source link

[mapnik-3] Review map buffer-size behaviour #2

Closed rochoa closed 7 years ago

rochoa commented 7 years ago

In order to make compatible existing maps relying on Map's buffer-size, we need to change the way we handle the global buffer-size property.

With mapnik 3 we might need to detect what is the bigger buffer-size among all layers and apply it individually to all layers.

More information in https://github.com/mapnik/node-mapnik/issues/342.

dgaubert commented 7 years ago

I've been reviewing our stack to know how buffer-size is handled from windshaft-cartodb to node-mapnik.

Basically, we define a default value for buffer-size in windshaft-cartodb's config and it is passed to the windshaft's renderer-factory. Windshaft is responsible of telling to tilelive-mapnik which buffer-size is going to be used. Mapnik exposes two ways to define the buffer-size, the first one is at map level while creating a new map from the pool and, the second one is when it renders the map using the buffer-size passed through options in metatile-cache creation. Since this commit in node-mapnik, they get the buffer-size from the options instead of the map but if I'm not wrong both values are the same (64 in our case). Tilelive-mapnik sets the both buffer-size at map level and options when renders the map but at map level is just ignored.

I guess we had issues after that change in node-mapnik, so I think I'm missing something important. I'm going to do some checks to be sure of what I'm saying, but in the meanwhile, @rochoa could you shed some light on this??

rochoa commented 7 years ago

You can set the buffer-size property from CartoCSS.

Map {
  buffer-size: 128;
}

#layer {
  marker-fill: red;
  /* ... */
}

So after the changes introduced in https://github.com/mapnik/node-mapnik/commit/5b823e0a52010e29a5d0c44adc590f9a28f217be#commitcomment-8024534, the CartoCSS Map buffer-size property is no longer honored, so the user is no longer able to control the buffer size, which usually is a problem when you want to deal with labels.

dgaubert commented 7 years ago

OMG! :man_facepalming:

So, is Mapbox ignoring buffer-size from CartoCSS?.

dgaubert commented 7 years ago

I've been researching buffer-size behaviour in Mapnik 3.0.12:

I've been playing with both python and node bindings against Mapnik 3.0.12 and the following XML:

<?xml version="1.0" encoding="utf-8"?>
<!DOCTYPE Map[]>
<Map srs="+init=epsg:3857" buffer-size="32">
    <Parameters>
        <Parameter name="format">png8:m=h</Parameter>
        <Parameter name="interactivity_layer">world-borders</Parameter>
        <Parameter name="interactivity_fields">cartodb_id</Parameter>
    </Parameters>
    <Style name="world-borders-labels" filter-mode="first">
        <Rule>
            <TextSymbolizer face-name="DejaVu Sans Bold" size="50" clip="true" label-position-tolerance="0">
                <![CDATA[[name]]]>
            </TextSymbolizer>
        </Rule>
    </Style>
    <Style name="world-borders" filter-mode="first">
        <Rule>
            <PolygonSymbolizer fill="#374c70" fill-opacity="0.9" clip="true"/>
            <LineSymbolizer stroke-width="1" stroke="#ffffff" stroke-opacity="0.5" clip="true"/>
        </Rule>
    </Style>
    <Layer name="world-borders" srs="+init=epsg:3857" buffer-size="32">
        <StyleName>world-borders</StyleName>
        <StyleName>world-borders-labels</StyleName>
        <Datasource>
            <Parameter name="type">
                <![CDATA[postgis]]>
            </Parameter>
            <Parameter name="user">
                <![CDATA[carto_user]]>
            </Parameter>
            <Parameter name="password">
                <![CDATA[wadus]]>
            </Parameter>
            <Parameter name="host">
                <![CDATA[localhost]]>
            </Parameter>
            <Parameter name="port">
                <![CDATA[5432]]>
            </Parameter>
            <Parameter name="extent">
                <![CDATA[-20037508.3,-20037508.3,20037508.3,20037508.3]]>
            </Parameter>
            <Parameter name="row_limit">
                <![CDATA[65535]]>
            </Parameter>
            <Parameter name="simplify_geometries">
                <![CDATA[true]]>
            </Parameter>
            <Parameter name="use_overviews">
                <![CDATA[true]]>
            </Parameter>
            <Parameter name="persist_connection">
                <![CDATA[false]]>
            </Parameter>
            <Parameter name="srid">
                <![CDATA[3857]]>
            </Parameter>
            <Parameter name="table">
                <![CDATA[SELECT * FROM world_borders]]>
            </Parameter>
            <Parameter name="dbname">
                <![CDATA[carto_user_db]]>
            </Parameter>
            <Parameter name="geometry_field">
                <![CDATA[the_geom_webmercator]]>
            </Parameter>
        </Datasource>
    </Layer>
</Map>

It's a map with two layers, one layer is world-borders setting font-size of labels to 50. This will cause a lot of cut labels if buffer-size is not tuned accurately. Changing buffer-size values from Map and Layer have the expected behaviour, getting more cut labels for lower values (< 64), and less cut labels for higher values (> 64). Here the .carto:

Untitled Map 23 (on 2017-01-09 at 17.36.47).carto.zip.

Comparing with Mapnik cdb-2.3.x.

Having the same map (attached above) on Windshaft-cartodb with Mapnik 3.x in my local environment and current CARTO that we have in production, the behaviour is slightly different. For lower values of buffer-size there are less cut labels with Mapnik 3.x than our Mapnik cdb-2.3.x, whereas for higher values of buffer-size cut labels are clearly less on both Mapniks:

Note: Metatiling in both environments is set to 2.

buffer-size: 64

mspnik-3 x

mapnik-2 3 x

buffer-size: 16

mapnik-3 x-bs-16

mapnik-2 3 x-bs-16

Note: Why do labels disappear for buffer-size: 16?

buffer-size: 256

mapnik-3 x-bs-256

mapnik-2 3 x-bs-256

I've dived in Mapnik and node-binding's source code and changelogs and they have done a lot of work with labels and text symbolizers, rendering options and so on, but I couldn't see anything related to explain these different behaviours between Mapnik 3.x and Mapnik 2.3.x. By the way, I think Mapnik 3.x improves a lot avoiding cut labels and the performance could be increased as the same map rendered with Mapnik 3.x would need lower values of buffer-size.

cc/ @rochoa

rochoa commented 7 years ago

What happens if you turn off metatiling (=1)?

dgaubert commented 7 years ago

Pretty much the same:

buffer-size: 64

mapnik-3 x-meta-1-bs-64

buffer-size: 16

mapnik-3 x-meta-1-bs-16

buffer-size: 256

mapnik-3 x-meta-1-bs-256

I think metatiling does not have any effect over labels and tile edges.

rochoa commented 7 years ago

Is that mapnik 3.x or 2.3.x?

It should, shouldn't it? I mean, if your label is all within the metatile query/render area, you have set buffer-size: 0, it should be affected.

Did you try with a points dataset? A polygons dataset is more difficult to debug/understand.

Imagine this situation:

 ---- ----
|    |    |
|    |    |
|    |  o |
|    |    |
 ---- ----
|    |    |
|    |    |
|    |    |
|    |    |
 ---- ----

Where the o is a point, and what you are seeing is a metatile (where metatile=2). If you try to render the label for that point with buffer-size: 0, my understanding is that with metatile=2 it will show up, and with metatile=1 it won't.

dgaubert commented 7 years ago

Is that mapnik 3.x or 2.3.x?

mapnik 3.x.

Points on mapnik 3.x

buffer-size: 0

mapnik-3 x-meta-1-bs-0-points

mapnik-3 x-meta-2-bs-0-points

buffer-size: 64

mapnik-3 x-meta-1-bs-64-points

mapnik-3 x-meta-2-bs-64-points

buffer-size: 128

mapnik-3 x-meta-1-bs-128-points

mapnik-3 x-meta-2-bs-128-points

So you are right. Metatiling and labeling works as expected. Using points is easier to see what happens... :-(

dgaubert commented 7 years ago

Points on mapnik 2.3.x

Note: metatile 2

buffer-size: 0

mapnik-2 3 x-meta-2-bs-0-points

buffer-size: 64

mapnik-2 3 x-meta-2-bs-64-points

buffer-size: 128

mapnik-2 3 x-meta-2-bs-128-points

buffer-size: 256

mapnik-2 3 x-meta-2-bs-256-points

Clearly the behaviour in Mapnik 3.x is better...

dgaubert commented 7 years ago

More results:

buffer-size: 64 (metatile: 2)

mapnik-2 3 x-meta-2-bs-64-rochoa

mapnik-3 x-meta-2-bs-64-rochoa

Again Mapnik 3.x have less cut labels.

dgaubert commented 7 years ago

Actually we can override buffer-size parameter at layer level since Mapnik v2.2.0, see this PR.

So @rochoa, should we detect what is the bigger buffer-size among all layers and apply it individually to all layers?

[ update ]

For the record: https://github.com/mapnik/mapnik/blob/master/CHANGELOG.md#220

dgaubert commented 7 years ago

Nevermind, carto@0.16.3 raise error using buffer-size out of Map rule:

Error: style0:8:2 Map properties are not permitted in other rules
dgaubert commented 7 years ago

Actually, you can define Map { buffer-size: 128 } on every layer of the MapConfig but the generated XML is going to use the buffer-size of the last layer defined in MapConfig.

If buffer-size is defined somewhere of the CartoCSS the generated XML will use it and the buffer-size from render options will be ignored, otherwise Mapnik will use it from options. Te same behaviour for both versions cdb-2.3.x and 3.x.

Examples:

{
    layers: [{
        id: 'layer-1',
        type: 'cartodb',
        options: {
            cartocss: 'Map {  buffer-size: 128 } #layer { ... }',
            cartocss_version: '2.3.0',
            sql: 'SELECT * FROM world_borders',
        }
    }, {
        id: 'layer-2',
        type: 'cartodb',
        options: {
            cartocss: '#layer { ... }',
            cartocss_version: '2.3.0',
            interactivity: 'cartodb_id',
            sql: 'SELECT * FROM populated_places',
        }
    }],
    ...
}
<?xml version="1.0" encoding="utf-8"?>
<!DOCTYPE Map[]>
<Map srs="+init=epsg:3857" buffer-size="128">
...
</Map>
{
    layers: [{
        id: 'layer-1',
        type: 'cartodb',
        options: {
            cartocss: 'Map {  buffer-size: 128 } #layer { ... }',
            cartocss_version: '2.3.0',
            sql: 'SELECT * FROM world_borders',
        }
    }, {
        id: 'layer-2',
        type: 'cartodb',
        options: {
            cartocss: 'Map {  buffer-size: 32 } #layer { ... }',
            cartocss_version: '2.3.0',
            interactivity: 'cartodb_id',
            sql: 'SELECT * FROM populated_places',
        }
    }],
    ...
}
<?xml version="1.0" encoding="utf-8"?>
<!DOCTYPE Map[]>
<Map srs="+init=epsg:3857" buffer-size="32">
...
</Map>
{
    layers: [{
        id: 'layer-1',
        type: 'cartodb',
        options: {
            cartocss: '#layer { ... }',
            cartocss_version: '2.3.0',
            sql: 'SELECT * FROM world_borders',
        }
    }, {
        id: 'layer-2',
        type: 'cartodb',
        options: {
            cartocss: '#layer { ... }',
            cartocss_version: '2.3.0',
            interactivity: 'cartodb_id',
            sql: 'SELECT * FROM populated_places',
        }
    }],
    ...
}
<?xml version="1.0" encoding="utf-8"?>
<!DOCTYPE Map[]>
<Map srs="+init=epsg:3857">
...
</Map>

Note: both mapnik cdb-2.3.x and mapnik 3.x have same behaviour.

dgaubert commented 7 years ago

I did one more test between node-mapnik@3.5.14 and node-mapnik@1.4.15-cdb10. I've create a map with points and I've chosen the following tiles:

 z:7, x:63, y:48    z:7, x:64, y:48
 ----------------- -----------------
|                 |                 |
|                 |                 |
|                o|                 |
|                 |                 |
|                 |                 |
|                 |                 |
 ----------------- -----------------

I've styled the map showing just labels with the following CartoCSS:

#layer {
  ::labels {
    text-name: [name];
    text-face-name: "DejaVu Sans Bold";
    text-size: 40;
  }
}

The point placed on the left tile (x=63) is just a few pixels of the tile on the right (x=64). So the corresponding label for that point should be rendered for tile x=64 for values close to 0 of buffer-size.

I've done a simple script to render the tile (z:7, x:64, y:48) using both node-mapnik@3.5.14 and node-mapnik@1.4.15-cdb10 and playing with different values for buffer-size to check if there are differences between versions of node-mapnik (we already know that) and to see what differences are.

var fs = require('fs')
var mapnik = require('mapnik')

mapnik.register_default_input_plugins()
mapnik.register_system_fonts()

var map = new mapnik.Map(256, 256);

map.fromStringSync(fs.readFileSync('./map-point.xml', 'utf8'));

// z: 7, x: 64, y: 48
map.extent = [0, 5009377.125, 313086.0703125, 4696291.0546875]

var image = new mapnik.Image(map.width, map.height);

var options = {
     bufferSize: 0 // this paramenter is completely ignored in .render()
}

// min buffer-size for node-mapnik@1.4.15-cdb10 -> 65 
// min buffer-size for node-mapnik@3.5.14 -> 5
map.bufferSize = 65;

map.render(image, options, function (err, image) {
    if (err) return console.error(err)

    var view = image.view(0, 0, map.width, map.height)
    var buffer = view.encodeSync('png')

    fs.writeFileSync('test-point-node-ded02.png', buffer)
    console.log('Done')
});

Note: I've follow the way to render an image that tilelive-mapnik does, but using sync versions of the API.

Conclusions

rochoa commented 7 years ago

My proposal is:

  1. Set map options as per: Math.max.apply(null, layers.map(layer => layer.Map.bufferSize));. This will fix the issue with the last layer defining the buffer size for all layers, that's the current behavior. layer.Map.bufferSize is what the user can define in CartoCSS layer with something like Map { buffer-size: 64; } #layer {...}. If there is no defined Map { buffer-size: 64; } in any layer, use the value from _uri.query.bufferSize as we are doing now.

  2. If buffer-size is defined at layer level, use that value per layer. As per your comments, that should override the Map value, giving more granular control to the end user.

What do you think?

dgaubert commented 7 years ago

one problem is, mapnik allows you to set buffer-size per layer at XML level:

<Map srs="+init=epsg:3857">
    ...
    <Layer name="laayer-1" srs="+init=epsg:3857" buffer-size="64">
    ...
    </Layer>
</Map>

But the parser of carto raises an error if you try to set the buffer-size to one layer like this:

#layer { buffer-size: 64 }

millstone raises the following error:

Error: style0:x:y Map properties are not permitted in other rules

So, user cannot set the buffer-size at layer level.

rochoa commented 7 years ago

millstone? or grainstore? millstone should not know anything about cartocss correctness.

What cartocss-reference version are you using when that happens?

dgaubert commented 7 years ago

millstone? or grainstore? millstone should not know anything about cartocss correctness.

Sorry, I meant grainstore.

What cartocss-reference version are you using when that happens?

If I recall correctly, it happens in both versions that we are currently using:

On the other hand, if we want to get maximum value of buffer-size and apply it to the map we have to modify how tilelive-mapnik initialises it or transform the CartoCSS removing all Map.bufferSize definitions and saving in query options the max value. Tilelive-manik first sets the bufferSize to the mapnik.Map and then loads the XML, overriding it with the last definition of Map { buffer-size: .. } among CartoCSS of the layers.

dgaubert commented 7 years ago

I suspect this is how it worked always. Regarding buffer-size there is no difference of behaviour between mapnik@3 and mapnik@1.4.15-cdb10 at API level. The only difference that I found is that internally mapnik@1.4.15-cdb10 doesn't work fine and it needs higher values of buffer-size to render labels.

rochoa commented 7 years ago

But that's what carto package version you use to parse/validate the CartoCSS using a cartocss-reference. By cartocss-reference I meant what version from https://github.com/mapnik/mapnik-reference. If not specified, it will use the one associated with your mapnik version, IIRC.

dgaubert commented 7 years ago

It happens in both versions of manik-reference, 2.3.0 and 3.0.6. More specifically the error comes from:

dgaubert commented 7 years ago

I've just seen that carto is in charge of getting the last definition of buffer-size and setting it to the map attribute. In grainstore, styles are passed tocarto just as they are, e.g:

{
    srs: '+init=epsg:3857',
    format: 'png',
    Layer: [{
        id: 'layer0',
        name: 'layer0',
        srs: '+init=epsg:3857',
        Datasource: {
            type: 'postgis',
            geometry_field: 'the_geom_webmercator',
            extent: '-20037508.3,-20037508.3,20037508.3,20037508.3',
            srid: 3857,
            max_size: 10,
            table: 'SELECT ST_MakePoint(0,0)',
            dbname: 'my_database'
        }
    }],
    Stylesheet: [{
        id: 'style0',
        data: 'Map {  buffer-size: 64} #layer0 {  marker-fill: #FF6600;  marker-opacity: 1;  marker-width: 16;  marker-line-color: white;  marker-line-width: 3;  marker-line-opacity: 0.9;  marker-placement: point;  marker-type: ellipse;  marker-allow-overlap: true;}'
    }]
}

This MML object is passed to carto to be rendered and carto sets the buffer-size using the last definition among all styles defined in Stylesheet array. This is the current behaviour that we have in production and it's the same that carto@0.16.3 has (version that supports Mapnik 3.x).

In spite of the wasted time researching this, I think that we don't have to apply any change to make compatible existing maps, as we keep the way that we handle the global buffer-size.

What do you think @rochoa ???

rochoa commented 7 years ago

Let's skip the proposed improvements about set map and layer buffer sizes, because as your mentioning we have the same behavior we had before 👍 .