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

[WIP] Integrate wagyu to validate MVT polygons #356

Closed
wants to merge 51 commits into from

Conversation

@Komzpa

This comment has been minimized.

Copy link
Member

commented Dec 21, 2018

is copying the only way to ship wagyu?

@Algunenano Algunenano changed the title [WIP] Integrade wagyu to validate MVT polygons [WIP] Integrate wagyu to validate MVT polygons Dec 21, 2018

@Algunenano

This comment has been minimized.

Copy link
Member Author

commented Dec 21, 2018

is copying the only way to ship wagyu?

I expect this to be a conflict. I'm writing an explanatory email to postgis-devel with my reasoning.

@flippmoke

This comment has been minimized.

Copy link

commented Dec 21, 2018

With some effort it might be possible in wagyu to make it not dependent on any specific geometry implementation, much like boost geometry for example. If anyone was interested in doing this I will definitely be willing to help.

@Komzpa

This comment has been minimized.

Copy link
Member

commented Dec 22, 2018

@flippmoke PostGIS has its own lwgeom geometry implementation. Thing is, PostGIS is in C and wagyu looks like templated C++, so I'm not sure how to build a more direct bridge here. There is GEOS in C++, but C++ devs around PostGIS volunteers are rarely available now.

I would say that one way may be to understand what wagyu is doing to the contours to make them valid and try porting only that part of it, building on top of LWGEOM's abstractions. Documentation would help - link to https://github.com/mapbox/wagyu/blob/master/docs/intersections_chains.md on documentation looks dead, even a TBD with pointer to some code would be good - I'm not good at following templated C++ for now.

Is following integer grid a must for wagyu's underlying algorithms? Can it operate on doubles?

@Algunenano

This comment has been minimized.

Copy link
Member Author

commented Dec 22, 2018

Is following integer grid a must for wagyu's underlying algorithms? Can it operate on doubles?

Yes, it can. In my "bridge" file you can change just by modifying this line

The thing is that for MVT if you are using doubles for validation you need to grid any resulting points after the process is done, which can lead again to an invalid geometry. It's rare but I've seen it happening in our current MVT implementation with st_makevalid.

@flippmoke

This comment has been minimized.

Copy link

commented Jan 2, 2019

Can it operate on doubles?

Wagyu is not designed to operate on doubles. It will operate, but it has issues around several sections of the code - mapbox/wagyu#76. My current thought is that it would likely be best to introduce scaling for doubles before they are used in wagyu and then de-scale once it is complete. This is destructive to original values, but wagyu by its nature is destructive to original geometry state due to the snap rounding that occurs during its operation. This was a design choice that was acceptable during our creation of it because everything will be an integer coordinate and lossey once it is put into vector tiles.

Algunenano added 2 commits Dec 27, 2018
MVTGeom: Uniformize tests to match behaviour or GEOS and Wagyu
Wagyu sorts the polygon points so I've modified the tests so that the output is the same
for both backends. When impossible, after checking that both results were equally valid
I've used St_Area instead to the text itself

@Algunenano Algunenano force-pushed the Algunenano:libwagyu branch from 65ddd4a to 366e6f0 Jan 8, 2019

@Algunenano

This comment has been minimized.

Copy link
Member Author

commented Jan 9, 2019

I've changed the API of the adaptor to receive a geometry and a bounding box instead of 2 geometries. This is so, instead of asking wagyu to clip the 2 geometries (which was slow when most of the polygon was outside the tile), it checks the bounding box per linear_ring and uses quick_lr_clip if necessary. Thanks @flippmoke for the tip; those cases are up to 10x faster now.

I've also added a handle for interruptions in 9126fd9. It's not super nice since it involves adding some code in the library itself (in the most expensive loops) but I don't see any other way around it.

In the process I detected a bug in the GEOS path that made some ST_AsMVTGeom occasionally output float numbers. I'll probably push the fix in a different ticket, but a proper fix requires a big hit in performance (basically re-running validation, which is already the slowest part).

The only thing pending now is to create a new .travis build to check the --with-wagyu flag.

Current comparison (trunk vs wagyu): 20190104_trunk_vs_20190109_wagyu_interrupt.pdf

Updated: 20190111_geos_vs_20190111_wagyu.pdf

Algunenano added 10 commits Jan 9, 2019
Travis: Do not build twice the coverage run
Coverage for wagyu has been moved to its run
postgis_full_version: Uniformize log level on errors
Not having raster or topology installed in a database is normal
and shouldn't raise a NOTICE

@Algunenano Algunenano force-pushed the Algunenano:libwagyu branch from 4b5ba31 to c00631c Jan 21, 2019

@Algunenano Algunenano force-pushed the Algunenano:libwagyu branch from 0b2e777 to ba03394 Jan 22, 2019

@strk strk closed this in 5508a4f Feb 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.