-
Notifications
You must be signed in to change notification settings - Fork 503
Polyfill modes RFC #519
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
Polyfill modes RFC #519
Conversation
consistency with the rest of the library, the `polyfill` functions should be able to use the same | ||
cell boundaries. | ||
|
||
* Very large polygons |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have suggestions here? E.g. maxPolyfillSizes
could return an array of sizes, and polyfillIncremental(batchNum, polygon, out)
could polyfill a batch? (How this would work in the lib is still a bit fuzzy, but I think it's possible)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was considering if it would be possible to do this in an iterator but haven't looked into this more. We would need some allocation done, but perhaps that could be made relative to the number of points in the polygon rather than the number of cells. A simple polygon producing a large number of cells is related to the problem seen in zachasme/h3-pg#56
* Spherical geometry consistency | ||
|
||
Most of the H3 library uses spherical geoemtry. For example, cell boundaries are spherical hexagons | ||
and pentagons. The `polyfill` function is different that it assumes Cartesian geometry. For |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this RFC refer to polyfill
or polygonToCells
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm inconsistent in the term used, but we should do a pass making that consistent. polyfill
might be useful as a term as the abstract concept of filling a polygon (regardless of inclusion mode, etc).
## Approaches | ||
|
||
*What are the various options to address this issue?* | ||
|
||
On an API level, we will need to expose the containment mode and spherical/Cartesian choice as |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we might decide in v4 that polyfill is spherical, full stop, unless it's much slower this way. But I'm not sure there's a benefit to exposing both modes to the user, and it would mean a bunch of redundant code to check point inclusion and line intersection in both ways.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it was raised offline that cartesian is useful when maps are drawn on top of web mercator maps.
Co-authored-by: Nick Rabinowitz <public@nickrabinowitz.com>
354d1b7
to
c2c5f0b
Compare
|
||
```js | ||
polygonToCells(polygon, {res: res, cartesian: true, containment: h3.Containment.CENTROID}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: In JS, if a new object's property exactly matches a variable that will fill in that property, you can just list the property/variable name and it'll do the right thing. Eg:
polygonToCells(polygon, {res: res, cartesian: true, containment: h3.Containment.CENTROID}) | |
polygonToCells(polygon, {res, cartesian: true, containment: h3.Containment.CENTROID}) |
| Bits | Meaning | ||
| ---------- | ------- | ||
| 1-2 (LSB) | If 0, containment mode centroid.<br>If 1, containment mode cover.<br>If 2, containment mode intersects.<br>3 is a reserved value. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd reserve more bits here, there are other possible approaches (e.g. > 50% containment). Got plenty of bits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As in make the flags 64 bits?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, as in make this bit section wider (say 3 bits instead of two) so that we don't constrain ourselves to 4 containment modes.
No description provided.