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

feat(GeoArrow): getBinaryGeometriesFromArrow enhancement #2785

Merged
merged 11 commits into from
Nov 15, 2023

Conversation

lixun910
Copy link
Collaborator

@lixun910 lixun910 commented Nov 13, 2023

  • get triangle indices for polygons
  • add option chunkIndex to specify which chunk to get binary geometries from (for progressive rendering)
  • add option meanCenters to get mean centers for binary geometries (for polygon filtering)

ToDo:

  • add test cases after pr #2782

@lixun910 lixun910 marked this pull request as draft November 13, 2023 23:01
}
k++;
}
const triangleIndices = earcut(slicedFlatCoords, holeIndices, nDim);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note to self: OK for now, but

  • I am a little hesitant about adding code to perform earcut into every single geospatial table loader.
  • Ideally we would have one common worker that could perform earcut on the binary output of other loaders.
  • The main problem is that we would need that worker to work on batches - which is a bit fiddly as we will need to build an adapter between asyncIterators and ping-pong worker messaging.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, Ib! I agree. I think maybe we should do the polygon tesselation for geoarrow in deck.gl: e.g.
export function triangulate(polygons: BinaryPolygonFeatures) in carto-vector-tile-loader (see here), so we call it in solid-polygon-layer when do polygon tesselating on binary geometry. What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We definitely want to do the tesselation on a worker if we can, and since deck.gl does not really handle async attribute generation yet I think it is reasonable to do it here in loaders.gl.

I just wish we didn't need to import this code into every loader, but that is something we can optimize later.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I haven't looked into it yet, but I was thinking it should be possible for my layers when resolving the data input. deck supports that being async because it allows remote file input, so it should be possible to persist earcut's output in the layer state. And maybe also parallelize operations well through a worker farm.

I was thinking of trying to make an isolated helper worker that does earcut on a contiguous polygon array (i.e. my PolygonData type, or arrow.Data<arrow.List<arrow.List<arrow.FixedSizeList<arrow.Float64>>>>). If you follow the .data attributes, you should be able to access the underlying ArrayBuffers and transfer those. Maybe that wouldn't work in a transferable if those ArrayBuffers are shared among multiple columns, but I don't know if ArrowJS does that

Signed-off-by: Xun Li <lixun910@gmail.com>
Signed-off-by: Xun Li <lixun910@gmail.com>
Signed-off-by: Xun Li <lixun910@gmail.com>
Signed-off-by: Xun Li <lixun910@gmail.com>
Signed-off-by: Xun Li <lixun910@gmail.com>
Signed-off-by: Xun Li <lixun910@gmail.com>
Signed-off-by: Xun Li <lixun910@gmail.com>
Signed-off-by: Xun Li <lixun910@gmail.com>
Signed-off-by: Xun Li <lixun910@gmail.com>
@lixun910 lixun910 force-pushed the xli/improve-arrow-to-binary branch from a1e907b to 9dad8b0 Compare November 15, 2023 01:46
Signed-off-by: Xun Li <lixun910@gmail.com>
@lixun910 lixun910 marked this pull request as ready for review November 15, 2023 01:50
Signed-off-by: Xun Li <lixun910@gmail.com>
@lixun910 lixun910 merged commit 992d90e into master Nov 15, 2023
4 checks passed
@lixun910 lixun910 deleted the xli/improve-arrow-to-binary branch November 15, 2023 02:56
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.

3 participants