dfellis / h3-node

H3 binding to Node using N-API
MIT License
30 stars 5 forks source link

How to tackle the `h3SetToMultiPolygon` implementation #20

Closed dfellis closed 5 years ago

dfellis commented 5 years ago

So before h3-node can reach parity with h3-js, I have to implement the h3SetToMultiPolygon function from it, which has the interesting distinction of being the only H3 function that is only available to h3-js users, since a critical component of it is written in the binding instead of at the C level.

There are roughly three ways that I can tackle this, in order of difficulty:

  1. Expose the h3SetToLinkedGeo and destroyLinkedPolyon functions and then mostly-copy-paste the h3-js Javascript implementation.
  2. Implement the readMultiPolygon function in C using N-API to construct the Array-of-Arrays-of-Arrays-of-Doubles.
  3. Implement the readMultiPolygon function in C in the core H3 library with a new struct type to construct the Array-of-Arrays-of-Arrays-of-Doubles and then port h3-js and h3-node to use this form, instead.

The third option is the most preferable, because that will unlock this functionality for the Python, Go, and Java bindings, as well as, of course, the direct C users and any third-party bindings (though none of the third party bindings have prioritized 100% parity that I can see so far). But I also don't know when I will be able to get time to do it, because the other bindings have all been relatively simple and I could squeeze in a few here or there in my schedule, besides polyfill (but since I wrote that originally for the C, JS, and Python H3s I knew what to do), while this will probably require literal full working days of time in larger contiguous blocks so I can keep the problem in my head.

The first option is basically punting the can down the road, and the second option seems like an evolutionary cul-de-sac since it would be extra effort to do but would also be thrown away like the first option once the third is done.

@isaacbrodsky @nrabinowitz what are your thoughts on this problem?

nrabinowitz commented 5 years ago

I'm not sure what you mean by "only available in h3-js" - we ported the normalization logic to C, so all bindings should have parity now (except maybe Go?).

The main issue here is that the caller doesn't know how much memory to allocate until the LinkedGeo structure has been created. So it's not clear to me how 2 or 3 would work if you need the binding to allocate the memory. You could have the binding pass in just an empty MultiPolygon struct, then have C allocate the memory and require the binding to call a C-level destroyMultiPolygon function, which is no worse than what we have now, though h3-js at least would still need to walk the output to get it into normal JS arrays. Strongly suggest we use the existing GeoPolygon struct as the basis of this if we go that direction.

Overall, it's not a big win for h3-js to go this way, since you'd have two transformations (C LinkedGeo to C MultiPolygon, then MultiPolygon to JS arrays) instead of one.

dfellis commented 5 years ago

So h3-py doesn't have h3SetToMultiPolygon, though I do see h3-java does. I wasn't aware because I've only been following the Python and Javascript bindings.

I see that the Java implementation does still make the final GeoJSON-like MultiPolygon in the binding and not in C, but the amount of work is less than it used to be.