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

chore(Arrow): add test cases for multipolygon with holes #2782

Merged
merged 3 commits into from
Nov 15, 2023

Conversation

lixun910
Copy link
Collaborator

@lixun910 lixun910 commented Nov 10, 2023

I am getting stuck on using the valueOffsets in an Arrow column to identify the holes (inner rings) within a multi-polygon, like this: [[extrior ring1], [interior ring1]], [exterior ring2]]. I think, like wkb or shapefile, we need offset values for 1) 'polygons', 2) 'parts within polygon', and 3) 'rings within parts'

Will create another PR to do earcut on arrow geometries directly:

  • polygonIndices and primitivePolygonIndices are not used together to render polygon (binary) with holes in deck.gl currently, so we pass geomOffset as a workaround. This issue needs to be fixed in deck.gl (utils/iterable-utils: getAccessorFromBuffer())

Signed-off-by: Xun Li <lixun910@gmail.com>
@lixun910 lixun910 marked this pull request as draft November 10, 2023 01:09
@kylebarron
Copy link
Collaborator

I wrote up some text on how to understand geoarrow polygons in https://observablehq.com/@kylebarron/geoarrow-and-geoparquet-in-deck-gl.

You can also see how I'm using earcut in https://github.com/geoarrow/deck.gl-layers/blob/eb157f1ef9b393f6348da950b2f058aa2291d467/src/earcut.ts#L26-L60. Does that help?

@lixun910
Copy link
Collaborator Author

Thanks, @kylebarron! I did read your post and code. It works on simple polygon, polygon with holes and multipolygon. However, it seems not working with multipolygon with holes. See the committed multipolygon_hole.arrow file and the expected GeoJson in the test. Let me know if I miss something. Thanks again!

@kylebarron
Copy link
Collaborator

I handle multipolygons by running earcut on each underlying polygon. So I'm rendering polygons individually, and only in picking do I reconstitute what the original multipolygons are.

@lixun910
Copy link
Collaborator Author

@kylebarron See the sandbox code that replicates the issue: https://codesandbox.io/s/test-multipolygons-w-holes-gygchx?file=/src/index.tsx (Please see the comment and console.log in the code. I am using your code in deck.gl-layers). Let me know if this makes sense. Thank you!

It seems we can't get the holes from multipolygon structure from current logic:
Screenshot 2023-11-11 at 9 35 15 PM

The geojson version and the GDAL/Arrow version are correct:

Screenshot 2023-11-11 at 9 37 37 PM

@lixun910
Copy link
Collaborator Author

Attached data:

  • mp_hole.geojson
  • multipolygon_hole.arrow (created using ogr2ogr)

data.zip

Signed-off-by: Xun Li <lixun910@gmail.com>
@lixun910
Copy link
Collaborator Author

@kylebarron Never mind. My apologies: the Arrow file generated by ogr2ogr is not correct. After using ogr2ogr holes.arrow mp_hole.geojson -f Arrow -lco COMPRESSION=NONE -sql "SELECT id, name FROM mp_hole", everything works fine now. Thanks for your help!

@lixun910 lixun910 marked this pull request as ready for review November 12, 2023 05:40
@lixun910 lixun910 changed the title chore(Arrow): add test cases for multipolygon with holes (WIP) chore(Arrow): add test cases for multipolygon with holes Nov 13, 2023
@lixun910
Copy link
Collaborator Author

Hi @ibgreen can you help to approve this pr? Thanks!

// TODO why deck.gl's tessellatePolygon performance is not good when using geometryIndicies
// even when there is no hole in any polygon
// NOTE: polygonIndices and primitivePolygonIndices are not used together to render polygon (binary) with holes
// in deck.gl currently, so we pass geomOffset as a work around. This issue needs to be fixed in deck.gl
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am confused about this.

  • What is the correct thing to pass? An array with zero elements?
  • What is in the work around? geomOffsets can we name this better or comment on it? Why does that work for deck?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

From the doc (https://deck.gl/docs/api-reference/layers/geojson-layer#polygons), we should pass polygonIndices and primitivePolygonIndices together to deal with the case that polygon has any holes.

Correct me if I am wrong: when I read the code in deck.gl, it seems this only happens in carto-vector-tile-loader (see here), not in solid-polygon-layer with binary geometry format. They are simply used for polygon and polygonOutline indicies (see here)

Thanks, @ibgreen!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I created another PR #2785 to add triangle indices using earcut, so we can pass geomOffsets for both polygonIndices and primitivePolygonIndices along with the triangles props for solid-polygon-layer with binary geometry format.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Correct me if I am wrong: when I read the code in deck.gl, it seems this only happens in carto-vector-tile-loader (see here),

@felixpalmer I see your name on those lines in carto module. Is that something that could/should be upstreamed to the generic / non-carto module?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@lixun910 FWIW - when I ask for clarification, I am usually not asking for additional detail in the PR comments, but for additional detail in the source code comments, so that things are easier for me to understand when I come back to this code in 6 months.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The reason we have the triangulation in the carto module is so that we can perform it in our worker. As I commented on the deck.gl repo, SolidPolygonLayer should support triangulation with binary data automatically.

My experience is that triangulation can be a very costly operation, so if it can be performed in the worker thread in loaders.gl that will likely give the best performance. Once you put it into deck, it is being done on the main thread and will lead to jank

Signed-off-by: Xun Li <lixun910@gmail.com>
@lixun910 lixun910 merged commit c56deab into master Nov 15, 2023
4 checks passed
@lixun910 lixun910 deleted the xli/add-tests-multipolyhole-arrow branch November 15, 2023 00:44
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.

4 participants