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

Update to use Wagyu for clipper #719

Merged
merged 23 commits into from
Apr 3, 2017
Merged

Update to use Wagyu for clipper #719

merged 23 commits into from
Apr 3, 2017

Conversation

flippmoke
Copy link
Member

No description provided.

@flippmoke flippmoke merged commit 50be2ed into master Apr 3, 2017
@flippmoke flippmoke deleted the wagyu branch April 3, 2017 19:33
@springmeyer
Copy link
Member

I've started reviewing the visual changes. Right off I see something that looks problematic - it appears as if valid polygons are being lost with wagyu or the changes related to integrating wagyu.

If you look at the change to the tile0-b.expected.png https://github.com/mapnik/node-mapnik/pull/719/files#diff-4f291e06e1d0518dba661cecc3460514 the old image has all of northern Norway:

While the new image is missing northern Norway:

(Run the "swipe" on the github diff page or open them both in a browser tab and flip back and forth and the difference is obvious)

The missing regions appear to be Troms and Finnmark - ref https://www.openstreetmap.org/#map=6/68.588/16.699

@springmeyer
Copy link
Member

springmeyer commented Apr 3, 2017

The nz-1b.png testcase (https://github.com/mapnik/node-mapnik/pull/719/files#diff-c9615730b7b1b3b34d32861700d806dd) is also very different. It looks like previous to this PR many islands were dropped and now they are not. @flippmoke - what changed here? If this is expected, please add an explanation in the CHANGELOG.md

before:

after:

assert.equal(validityReport.length, 25); // Dataset not expected to be OGC valid
assert.equal(validityReport2.length, 0); // Dataset not expected to be OGC valid
assert.equal(validityReport3.length, 26); // Dataset not expected to be OGC valid
assert.equal(validityReport4.length, 25); // Dataset not expected to be OGC valid
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@flippmoke can you offer an explanation for why these results are so different between clipper and wagyu?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@springmeyer they are completely different outputs, some of this is due to snap rounding, but in general the number of errors and warnings that boost geometry is valid reports here has completely changed. We have found that in the multipolygon case it is often wrong, this is why this has the not expected to be OGC valid is left there still. This is an old warning but none the less a problem that we have seen. Hopefully it will be fixed soon in some bug fixes that boost geometry is working on.

@@ -3095,7 +3095,7 @@ describe('mapnik.VectorTile ', function() {
"id":1,
"type":3,
"geometry":[
[[2784,1776],[1976,1776],[1976,992],[2784,992],[2784,1776]]
[[1976,1776],[1976,992],[2784,992],[2784,1776],[1976,1776]]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this reversed ring direction expected?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a reversed ring direction, just a shift in the starting position of the rings.

assert.deepEqual(vtile3.names(),["water","admin"]);
vtile3.render(map,new mapnik.Image(256,256),function(err,im) {
if (err) throw err;
assert.equal(0,compare_to_image(im,expected_file));
vtile4.composite([vtile2],{reencode:true,max_extent:world_clipping_extent}, function(err) {
if (err) throw err;
assert.equal(vtile4.getData().length,54522);
assert.equal(vtile4.getData().length,54576);
assert.deepEqual(vtile4.names(),["water","admin"]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are these buffer sizes consistently bigger with this PR because less data is dropped per #719 (comment)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wagyu tends to be able to keep more data then the angus clipper did in general for attempting to put valid geometry into the data. Therefore, it has been fairly consistent that more data has been added.

@flippmoke
Copy link
Member Author

As per the questions around: #719 (comment)

This is in some ways related to mapbox/mapnik-vector-tile#131. Originally when we first put in the Angus clipper we were finding that better outputs with less errors were occuring when we did CleanPolygon. However, this typically removed data that was somewhat valid.

Wagyu was designed to deal with all the problems that CleanPolygon was doing, but in a much better way. Therefore, I am not shocked at all to see this data reappear.

@flippmoke
Copy link
Member Author

#719 (comment)

I really hate it when I don't remember why we have a test, but we do... The data is not missing from the tile because tile0.mvt is used for lots of other rendering tests and you see a few in the diff even, but they do not change more then perhaps a pixel.

This is the funky case when we are testing the rendering from a vector tile using a negative buffer.

vtile.render(map, new mapnik.Image(256, 256), {buffer_size:-64}, function(err, vtile_image) {

This might be slightly different due to a slight change in a coordinate of this little region that perhaps made mapnik do something different, or perhaps the latest version of mapnik has a small change here?

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.

None yet

2 participants