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

In-browser polygon tesselation #948

Closed
wants to merge 19 commits into from
Closed

In-browser polygon tesselation #948

wants to merge 19 commits into from

Conversation

mourner
Copy link
Member

@mourner mourner commented Jan 22, 2015

Closes #682. Not ready yet. To do:

  • implement holed polygon -> single ring polygon conversion algorithm as described in FIST paper
  • implement outer/inner rings classification
  • ditch libtess code completely
  • fix artifacts on some complex water polygons
  • make triangulation code a separate repo https://github.com/mapbox/earcut
  • fix fill antialiasing @ansis
  • fix fill patterns @ansis
  • try out indexed vertices approach and see if it improves performance @ansis
  • wait for Polygon winding order & holes vs outer rings mapnik-vector-tile#59 and ditch ring classification code to fix low-zoom water bleeding artifacts
  • make sure there are no other glaring artifacts browsing through the map
  • fix tests

@mourner mourner mentioned this pull request Jan 22, 2015
10 tasks
@mourner
Copy link
Member Author

mourner commented Jan 22, 2015

@ansis you can try out the indexed approach using earcut indexed branch. mapbox/earcut#5 Lets do it in a separate earcut-indexed branch.

Earcut returns {vertices: [...], indices: [...]} object. Vertices is a flat JS array. You can turn it into a typed one with e.g. new Int16Array(vertices).

@jfirebaugh jfirebaugh modified the milestone: v0.6.0 Jan 22, 2015
@jfirebaugh
Copy link
Contributor

@mourner Modulo fixing the tests, is this good to merge as-is, leaving investigation of the indexed approach as a follow-up ticket?

@mourner
Copy link
Member Author

mourner commented Jan 28, 2015

@jfirebaugh no, mapbox/mapnik-vector-tile#59 is a blocker — we need to ditch unreliable ring classification hack, otherwise low zoom levels (<7) are completely broken.

@jfirebaugh
Copy link
Contributor

Is that something we need to worry about for user supplied GeoJSON as well?

@mourner
Copy link
Member Author

mourner commented Jan 28, 2015

@jfirebaugh no, because GeoJSON spec explicitly defines the first ring as outer and other rings as holes for each polygon, and differentiates between polygons in a multipolygon.

@ansis
Copy link
Contributor

ansis commented Jan 30, 2015

@mourner It looks like -native already assumes winding order is clockwise for outer rings and counterclockwise for holes: https://github.com/mapbox/mapbox-gl-native/blob/47966a143176a337733fe63b4121d2d00c48359a/src/mbgl/renderer/fill_bucket.cpp#L98

Is fixing the order of rings all that we need? mapbox/mapnik-vector-tile#59 (comment)

Do we also need better clipping/unioning in the vector tiles?

@mourner
Copy link
Member Author

mourner commented Jan 30, 2015

@ansis we need to try this. It may produce incorrect ring classification sometimes due to degeneracies — points can belong to several rings at once. This problem is solved by doing a union in native, but we can't afford operations like this in JS because they're expensive (probably more expensive than triangulation).

Once we can classify rings reliably, cleaner geometries (union, etc. mapbox/mapnik-vector-tile#53) will be nice to solve minor artifacts on bad data, but not a blocker.

@mourner
Copy link
Member Author

mourner commented Feb 26, 2015

I'll probably squash the PR into 2 commits, the first one and the rest. Such a mess currently after all the conflicting rebases...

and reuse vertices for outline and fill instead of adding them twice
@jfirebaugh jfirebaugh added this to the 0.9 milestone Jun 11, 2015
@jfirebaugh jfirebaugh removed this from the 0.9 milestone Jun 25, 2015
@mourner
Copy link
Member Author

mourner commented Oct 8, 2015

-> #1606

@mourner mourner closed this Oct 8, 2015
@jfirebaugh jfirebaugh deleted the earcut branch October 17, 2015 00:31
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.

tesselate polygons instead of rendering with stencil buffer
3 participants