Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Add tolerance to the features_at_point() query in the PostGIS plugin #503

Closed
artemp opened this Issue · 13 comments

3 participants

@artemp
Owner

It is very difficult to get results for a GetFeatureInfo request because the features_at_point() has no tolerance parameter:

map.cpp
ds->features_at_point(mapnik::coord2d(x,y));

postgis.cpp:
Envelope box(pt.x,pt.y,pt.x,pt.y);
std::string table_with_bbox = populate_tokens(table_,FMAX,box);

@artemp
Owner

[springmeyer] going to try to take a look at this before releasing 0.7.1....

@artemp
Owner

[manelclos] tolerance_full_patch.diff contains previous partial patches (Layer getters/setters, python bindings).

I've tested with a postgis points layer and ogcserver and it works really well.

In short, I've added a features_at_box function in datasource.hpp. Implementation in input plugins is straightforward (perhaps the shape implementation can also use a box when querying). Map::query_map_point will use the function and get all the results in the box.

Also take the new params into account when loading the map (missing in save!), initializing the Layer class, copying a Layer and python bindings.

mapfile.xml:

ogcserver: common.py -> copy_layer
...
lyr.tolerance = obj.tolerance
lyr.toleranceunits = obj.toleranceunits
...

  • should query_point be also modified?
  • in order to implement "meters", how do you know what unit is the map using?
@artemp
Owner

[springmeyer] Great, thanks for the patch and you raise good questions Manel.

Regarding units, see #389, which I support (ignore my confusing old comment on that ticket!). My sense is that we should likely spec out all the places explicit units may benefit from being specified and come up with generic approach. So, I think for now it is likely best to add just tolerance parameter in assumed pixels and then later (for #389) come up with proper class to handle unit ENUMS and conversions. (right now you know the map is either in degrees if map.srs() proj.is_geographic and otherwise we assume meters).

Also, I think we may want to just add tolerance argument to features_at_point, or add tolerance attribute to mapnik::query and pass that to both features_at_point() and features().

If we plan to add this to trunk then making those changes are more feasible (so assigning this ticket to 0.8.0/trunk milestone), and see that already we are starting to extending mapnik::query in trunk in r1615. We should open a new ticket discussing long term what other attributes might need to get attached to mapnik::query object in addition to tolerance.

@artemp
Owner

[manelclos] What about setting Datasource tolerance/toleranceunits from layer.cpp? this way the properties will be visible to the Datasource instance and the features_at_point function won't need modification. Will this preserve binary compatibility and allow the patch to go in 0.7.2?

{{{
void layer::set_datasource(datasource_ptr const& ds)
{
ds_ = ds;
// Set tolerance here, or in any other function that makes sense
ds_.tolerance = this.tolerance;
ds_.toleranceunits = this.toleranceunits
}
}}}

@artemp
Owner

[springmeyer] actually going to deal with this, first in mapnik2, then will backport (and break abi if needed). thanks for your patience.

@artemp
Owner

[manelclos] Dane, if you decide which approach to use I can create a patch for trunk:
a) make features_at_point use a Box2D parameter

b) create features_at_box and change map to use _at_box instead of _at_point

c) set tolerance / tolerance units in the datasource and magically apply them on _at_point calls.

d) any other :)

I'll go for c) which I've not tested yet, but looks less intrusive.

@artemp
Owner

[springmeyer] I'm thinking, to start, in trunk:

  • change features_at_point to accept a second, optional argument of tolerance (in geographic units), which will default to zero

  • change map.query_point and map.query_map_point to accept an optional tolerance argument (in pixels), and skip (for now at least) having to add tolerance parameter to mapnik.Layer

  • Usually polygons should need no tolerance, but lines and points will, so exposing in map.query...() seems like the most flexible so that the calling application can determine how likely a hit should be. TOLERANCE can be passed then as a URL param to OGCServer which will push that parameter (if it exists) to the query_point function.

  • Then try to develop some automated tests of behavior, using different geometry types and map projections (data using meters and degrees at least)

After the above are complete, then I think we can backport this work from trunk to 0.7.2 (and it will be then more clear how to do that in the least invasive way).

You are welcome to work on the patch, but I can do. The thing that would be most useful would be tests, so ideally if you have time you could start on those first. Any form is okay, but best would be a new file like tests/python/query_tolerance_test.py

@artemp
Owner

[springmeyer] I have a patch for this (somewhere on my system) against mapnik trunk, but no time to test it properly, so pushing to mapnik 2.1, which will hopefully come very soon. sorry for the delay.

@artemp
Owner

[manelclos] What about ABI compatibility? can a simple "options" string parameter be added? Mostly in Python's fashion to be able to pass more options in the future without breaking ABI comp.

@springmeyer springmeyer was assigned
@manelclos

Hi, I need to implement this in order to update servers. Dane, can you share your patch?

@manelclos

Added tolerance parameter in the same pull request. The test uses a shape file, which already handles the tolerance. Default tolerance is 3 pixels (see map.cpp query_point). Because PostGIS uses a box on the query, tolerance is not taken into account. The patch allows PostGIS datasource to get the tolerance and use it in the box query.

@springmeyer
Owner

Looking good! Can you also start adding tests against other datasources, with variable projections? CSV datasource might be the easiest to use to test various projections and to ensure tolerance makes sense for each. SQLite and OGR plugins are also important, so we should make sure feature_at_point is working.

@springmeyer
Owner

closed by merging #1499. @manelclos - as mentioned above, more tests are needed. Can you open further pull requests when you have time for more tests?

@springmeyer springmeyer closed this
@PetrDlouhy PetrDlouhy referenced this issue from a commit in PetrDlouhy/mapnik
@springmeyer springmeyer update changelog after 86e805b - refs #503 and #1499 869eeb3
@PetrDlouhy PetrDlouhy referenced this issue from a commit in PetrDlouhy/mapnik
@springmeyer springmeyer new features_at_point tolerance should be optional - make it so in py…
…thon to avoid test failures after #1499/#503
7653d3b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.