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

optimized coordinate transform that skips proj4 failures #116

Closed
springmeyer opened this issue Jun 10, 2015 · 3 comments
Closed

optimized coordinate transform that skips proj4 failures #116

springmeyer opened this issue Jun 10, 2015 · 3 comments
Milestone

Comments

@springmeyer
Copy link
Contributor

Our vector tile processor here uses boost::geometry::transform and this strategy. The underlying strategy is sound however boost::geometry::transform is not the right tool for the job because:

  • We need to intelligently skip and coordinates that fail to reproject (!prj_trans_.backward())
  • For the fastest conversion of geometries we need to call vector.reserve(size)

Therefore we need:

  • knowledge of the final size of each geometry part before starting to transform them
  • knowledge of the previous and next coordinate in order to robustly skip invalid coordinates without creating unintended artifacts.
@springmeyer springmeyer added this to the v0.8.2 milestone Jun 10, 2015
@springmeyer
Copy link
Contributor Author

We need to intelligently skip and coordinates that fail to reproject (!prj_trans_.backward())

Writing unit tests for this in mapnik-vt I hit this unhandled condition: mapnik/mapnik#2901.

It turns out that inf at

boost::geometry::set<0>(p2, static_cast<p2_type>(x));
boost::geometry::set<1>(p2, static_cast<p2_type>(y));
will flip to -9223372036854775807 and this will trigger ClipperLib::AddPath to throw in RangeTest. So, finally we have a valid and likely way to reproduce #111

springmeyer pushed a commit that referenced this issue Jun 11, 2015
the valid range of mercator.

Refs #116

This should fail currently:

 - Against mapnik before mapnik/mapnik#2901 this fails with:

```
due to unexpected exception with message:
  Coordinate outside allowed range: -9223372036854775808 -9223372036854775808
  -9223372036854775808 -9223372036854775808
```

 - Against mapnik after mapnik/mapnik#2901 this fails with:

```
due to unexpected exception with message:
  Can't transformm geometry
```
@springmeyer
Copy link
Contributor Author

9dd2729 adds the new transformer. Due to the ability to call std::reserve perf is noticeably better:

$ ./build/Release/vtile-transform 
2192.54ms (cpu 2179.91ms)   | boost::geometry::transform
1511.22ms (cpu 1500.99ms)   | transform_visitor with reserve

@springmeyer
Copy link
Contributor Author

After f6fb9d4 we are now again handling pj_transform failures without throwing.

The goal is not to perfectly represent shapes with coordinates outside the valid bounds - to do that robustly would require clipping before reprojecting. While we did that before it would now be inconvenient. So the key here is to perfectly represent shapes with 100% valid coords and therefore to:

  1. not risk aborting rendering due to invalid ones.
  2. do a decent job at trying to render shapes with some invalid coords

As an example of what rendering a shape with invalid coords looks like see the following image (which is the testcase geometry visualized in studio a few ways):

screen shot 2015-06-10 at 7 48 36 pm

  • Red line == the result of the new skipping transform when reprojecting from epsg:4269 -> epsg:3857. This hits pj_transform and skips failures resulting in a partially collapsed shape. Not pretty but expected.
  • Blue line == the result of the new skipping transform when reprojecting from epsg:4326 -> epsg:3857. In this case there are no pj_transform failures because proj4 is not invoked and invalid bounds are clamped by https://github.com/mapnik/mapnik/blob/352586e9d7f9276fb27c10d5449863fe67450612/include/mapnik/well_known_srs.hpp#L67-L70. So, the shape is not collapsed which is nice. Since this result is nicer we could consider clamping for more projections in the future, but that is pretty low priority in my opinion.

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

No branches or pull requests

1 participant