Consistent pentagon local IJ coords #234
Conversation
Change failed directions so that more than one face cannot be crossed when unfolding a pentagon.
src/h3lib/lib/localij.c
Outdated
| * within the pentagon base cell. | ||
| * | ||
| * This previously included a Class II/Class III check but these were removed | ||
| * due failure cases. It's possible this could be restricted to a narrower set |
There was a problem hiding this comment.
Nit: due to failure cases.
CHANGELOG.md
Outdated
| @@ -6,6 +6,8 @@ The public API of this library consists of the functions declared in file | |||
| [h3api.h.in](./src/h3lib/include/h3api.h.in). | |||
|
|
|||
| ## [Unreleased] | |||
| ### Changed | |||
| - Local coordinate spaces cannot cross more than one icosahedron face. | |||
There was a problem hiding this comment.
This restriction only applies to pentagons, right?
src/h3lib/lib/localij.c
Outdated
| // https://github.com/uber/h3/issues/163 | ||
| if (FAILED_DIRECTIONS_III[originLeadingDigit][dir] || | ||
| FAILED_DIRECTIONS_II[originLeadingDigit][dir]) { | ||
| if (FAILED_DIRECTIONS[originLeadingDigit][dir]) { | ||
| // TODO this part of the pentagon might not be unfolded | ||
| // correctly. |
There was a problem hiding this comment.
Nit: Not a big deal, but reading this comment makes me think something is incorrect, rather than something might be incorrect and so we're throwing an error. Maybe TODO: this part of the pentagon might not be unfolded correctly, so throw an error for now?
src/h3lib/lib/localij.c
Outdated
| @@ -188,8 +188,8 @@ int h3ToLocalIjk(H3Index origin, H3Index h3, CoordIJK* out) { | |||
| int originLeadingDigit = _h3LeadingNonZeroDigit(origin); | |||
|
|
|||
| if (FAILED_DIRECTIONS[originLeadingDigit][dir]) { | |||
| // TODO this part of the pentagon might not be unfolded | |||
| // correctly. | |||
| // TODO In this case this part of the pentagon might not be | |||
There was a problem hiding this comment.
Nit: In this case this part... -> In case this part...
There was a problem hiding this comment.
No, I think it's correct as is - "in case" means you don't know whether something might occur, but you want to anticipate it; "in this case" means you are clear that in the current context something will occur, and you are handling it.
There was a problem hiding this comment.
But we don't know if it's actually unfolding incorrectly or not, so in case that would happen we're doing something. The current phrasing is very confusing.
There was a problem hiding this comment.
I requested this change, because every time I see this comment I think that return 3 is thought to be possibly incorrect, instead of being a correct guard against possible issues. Open to other options. Maybe
TODO: We may be unfolding the pentagon incorrectly in this case; return an error
code until this is guaranteed to be correct.
?
There was a problem hiding this comment.
That one is definitely clear.
There was a problem hiding this comment.
I'll update with the revised wording @nrabinowitz suggested
Consistent pentagon local IJ coords
Change failed directions so that more than one face cannot be crossed
when unfolding a pentagon. This should fix #184 by disallowing this kind of line.