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

2.3.x.twkb #2

Merged
merged 15 commits into from
Jul 13, 2015
Merged

2.3.x.twkb #2

merged 15 commits into from
Jul 13, 2015

Conversation

pramsey
Copy link

@pramsey pramsey commented Jul 8, 2015

This PR replaces #1.

Standard Parameters

  • table: Either the name of a table to map from or a SQL query to attempt to map (required)
  • geometry_table: In the case of a SQL query in the table parameter, the table in the query that is the source of the geometry
  • geometry_field: The name of the column in the table or SQL query that will return the geometry
  • key_field: The name of the column in the table or SQL query that contains a unique key
  • autodetect_key_field: Try and figure out the keyfield from metadata tables. (default: false)
  • cursor_size: If non-zero, the maximum number of rows to fetch while processing a query. For memory constrained situations, this limits the amount of data held in memory during query processing. (default: 0)
  • row_limit: If non-zero, apply a this LIMIT to the query, effectively placing a maximum on the number of features to be mapped. (default: 0)
  • srid: If non-zero, the EPSG SRID to use for the geometries coming from this query, notwithstanding the SRID that is actually on them. (default: 0)
  • host, port, dbname, user, password: PostgreSQL database connection parameters (required)
  • connect_timeout: Database connection timeout. (default: 4)
  • pool_max_size: How big to make the connection pool. (default: 10)
  • persist_connection: Keep connections open between requests? (default: true)
  • estimate_extent: Try and estimate extent of data? (default: false)
  • extent_from_subquery: If trying to estimate extent, and SQL is subquery, estimate on that? (default: false)
  • max_async_connection: If > 1, turn on the asyncronous connection feature. (default: 1)
  • simplify_geometries: Use PostGIS ST_Simplify call on the geometry to make input data smaller. (default: false)
  • simplify_dp_ratio: Use a PostGIS simplification factor of this proportion of the ground size of a pixel. For example, if pixels are 40m square for this rendering, and the ratio is set to 1/20, then ST_Simplify(geom, 2) would be used. (default: 1/20)

2.3.x.twkb Parameters

  • simplify_snap_ratio: Use PostGIS ST_SnapToGrid call to make the input data smaller. This is applied to geometry before any ST_Simplify is called, so should use a tolerance smaller than the simplify tolerance. Tolerance is expressed as a proportion of a pixel, as with simplify_dp_ratio. (default: 1/40)

Features below all require PostGIS 2.2+, as they use features that do not appear in earlier PostGIS versions.

  • twkb_encoding: Use TWKB to encode geometries for transport from the database to the renderer instead of standard WKB? (default: false)
  • simplify_dp_preserve: Set the preserve option in ST_Simplify when in simplify_geometries mode? This will ensure that features that get simplified down to nothing aren't dropped but are retained in point-like form. Useful for rendering to avoid gaps where small feature "disappear". (default: false)
  • simplify_clip_resolution: In non-zero, sets the map scale at which geometries start getting clipped to the rendering window. (default: 0.0)

SQL Tokens

SQL tokens are strings that are replaced in the input SQL before the SQL is sent to the database for execution. This is information that Mapnik knows as the result of the request, and can be useful in forming complex SQL queries.

  • !bbox!
  • !scale_denominator!
  • !pixel_width!
  • !pixel_height!

@pramsey pramsey mentioned this pull request Jul 8, 2015
@rafatower
Copy link

LGTM.

A couple of questions:

  • would you add tests for the format decoder and/or tolerance caclulcations?
  • is it feasible/reasonable to plug the twkb_reader as an external dependency? maybe a reference implementation so that we kind of promote its usage?

@pramsey
Copy link
Author

pramsey commented Jul 10, 2015

  • It might be possible to add some online tests to the python suite, though it looks like getting those tests to turn over is quite tricky (you need postgis and postgres and mapnik installed "just so" to get the postgis tests to turn over).
    • Hopefully I can figure out the simple C++ unit tests and copy some of the postgis-side tests over to the mapnik side.
  • The reason there's no "libwkb" and so on is that you need a model to deserialize to, and for any given practical application of a reader, the application is going to have its own model, whether it be the OGR model or the mapnik model or the postgis liblwgeom model. So everyone writes their own serializers/deserializers into/out of their models. What will drive adoption is implementations in useful places, like the postgis/mapnik combo and the postgis/openlayers combo. I could stick an implementation in mapserver too if we were interested in promoting things that way.

@pramsey
Copy link
Author

pramsey commented Jul 10, 2015

The cpp_tests unfortunately can only test things in mapnik proper, not in the input plugins. So question is: how key is having units tests for you? Will require moving the twkb handing from the postgis plugin to mapnik core first. Having already run all geometry types through it, I'm not convinced it's worthwhile (my code is perfect, first try! :)

@rafatower
Copy link

Fair enough. Regarding tests, I was just wondering, it's an effort/benefit judgement call. Since your code is perfect I guess there's little value in adding them ;) Thanks for the detailed responses.

@rochoa
Copy link

rochoa commented Jul 13, 2015

LGTM 👍

Thanks for taking the time to document all the params here.
About key_field and autodetect_key_field, what happens if key_field is not provided and autodetect_key_field is set to false?

Talking about tests: I'm OK with going on, we will have time to fix whatever we detect, probably in this case it applies the better done than perfect.

@pramsey
Copy link
Author

pramsey commented Jul 13, 2015

Regarding key_field, it looks like if you don't define one or ask for one, the only downside is that you'll get a feature collection back with a "unique" id that isn't actually unique, that is just the row number in the current collection. So if you're using mapnik features in python I guess and maybe doing something with them more involved than just drawing them on the screen, you'll get useless identifiers. It's very easy to end up in this state, it seems, since the default for autodetect_key_field is false, not true.

pramsey added a commit that referenced this pull request Jul 13, 2015
@pramsey pramsey merged commit 065214e into 2.3.x.cartodb Jul 13, 2015
rafatower pushed a commit that referenced this pull request May 6, 2016
@jgoizueta jgoizueta mentioned this pull request Jan 16, 2017
2 tasks
rochoa pushed a commit that referenced this pull request Apr 6, 2017
Algunenano added a commit that referenced this pull request Apr 9, 2019
Extract from ASAN:
```
$ ./node_modules/.bin/mocha -u tdd -t 5000 --exit test/image.svg.test.js --grep "should not error with async in non-strict mode on svg with validation and parse errors"

  mapnik.Image SVG
=================================================================
==4125==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x625003943900 at pc 0x7f3ffbd21206 bp 0x7f3ff7489950 sp 0x7f3ff7489948
READ of size 8 at 0x625003943900 thread T7
    #0 0x7f3ffbd21205 in agg::rasterizer_cells_aa<agg::cell_aa>::sort_cells() /usr/include/mapnik/agg/agg_rasterizer_cells_aa.h:680:20
    #1 0x7f3ffbd1fe0b in agg::rasterizer_scanline_aa<agg::rasterizer_sl_clip<agg::ras_conv_int> >::rewind_scanlines() /usr/include/mapnik/agg/agg_rasterizer_scanline_aa.h:467:19
    #2 0x7f3ffbd1007c in void agg::render_scanlines<agg::rasterizer_scanline_aa<agg::rasterizer_sl_clip<agg::ras_conv_int> >, agg::scanline_u8, agg::renderer_scanline_aa_solid<agg::renderer_base<agg::pixfmt_alpha_blend_rgba<agg::blender_rgba_pre<agg::rgba8T<agg::linear>, agg::order_rgba>, agg::row_ptr_cache<unsigned char>, unsigned int> > > >(agg::rasterizer_scanline_aa<agg::rasterizer_sl_clip<agg::ras_conv_int> >&, agg::scanline_u8&, agg::renderer_scanline_aa_solid<agg::renderer_base<agg::pixfmt_alpha_blend_rgba<agg::blender_rgba_pre<agg::rgba8T<agg::linear>, agg::order_rgba>, agg::row_ptr_cache<unsigned char>, unsigned int> > >&) /usr/include/mapnik/agg/agg_renderer_scanline.h:464:16
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants