From 4f4f4f97c7713e42b9bf4446b1ebfe489de88a00 Mon Sep 17 00:00:00 2001 From: Isaac Brodsky Date: Fri, 3 May 2019 13:44:27 -0700 Subject: [PATCH 1/4] Consistent pentagon local IJ coords Change failed directions so that more than one face cannot be crossed when unfolding a pentagon. --- CHANGELOG.md | 2 + src/apps/testapps/testH3Line.c | 8 ++++ src/apps/testapps/testH3ToLocalIj.c | 70 +++++++++++++++++++++++++++++ src/h3lib/lib/localij.c | 52 ++++++++++----------- 4 files changed, 103 insertions(+), 29 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 564aeaff4..0c8ded434 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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. ## [3.4.3] - 2019-05-02 ### Added diff --git a/src/apps/testapps/testH3Line.c b/src/apps/testapps/testH3Line.c index 210054976..0366568f2 100644 --- a/src/apps/testapps/testH3Line.c +++ b/src/apps/testapps/testH3Line.c @@ -107,4 +107,12 @@ SUITE(h3Line) { iterateAllIndexesAtResPartial(3, h3Line_kRing_assertions, 6); // Further resolutions aren't tested to save time. } + + TEST(h3Line_acrossMultipleFaces) { + H3Index start = 0x85285aa7fffffff; + H3Index end = 0x851d9b1bfffffff; + + int lineSz = H3_EXPORT(h3LineSize)(start, end); + t_assert(lineSz < 0, "Line not computable across multiple icosa faces"); + } } diff --git a/src/apps/testapps/testH3ToLocalIj.c b/src/apps/testapps/testH3ToLocalIj.c index 75a972552..64777d44f 100644 --- a/src/apps/testapps/testH3ToLocalIj.c +++ b/src/apps/testapps/testH3ToLocalIj.c @@ -339,4 +339,74 @@ SUITE(h3ToLocalIj) { t_assert(H3_EXPORT(experimentalH3ToLocalIj)(pent1, bc3, &ij) != 0, "found IJ (5)"); } + + /** + * Test that coming from the same direction outside the pentagon is handled + * the same as coming from the same direction inside the pentagon. + */ + TEST(onOffPentagonSame) { + for (int bc = 0; bc < NUM_BASE_CELLS; bc++) { + for (int res = 1; res <= MAX_H3_RES; res++) { + // K_AXES_DIGIT is the first internal direction, and it's also + // invalid for pentagons, so skip to next. + Direction startDir = K_AXES_DIGIT; + if (_isBaseCellPentagon(bc)) { + startDir++; + } + + for (Direction dir = startDir; dir < NUM_DIGITS; dir++) { + H3Index internalOrigin; + setH3Index(&internalOrigin, res, bc, dir); + + H3Index externalOrigin; + setH3Index(&externalOrigin, res, + _getBaseCellNeighbor(bc, dir), CENTER_DIGIT); + + for (Direction testDir = startDir; testDir < NUM_DIGITS; + testDir++) { + H3Index testIndex; + setH3Index(&testIndex, res, bc, testDir); + + CoordIJ internalIj; + int internalIjFailed = + H3_EXPORT(experimentalH3ToLocalIj)( + internalOrigin, testIndex, &internalIj); + CoordIJ externalIj; + int externalIjFailed = + H3_EXPORT(experimentalH3ToLocalIj)( + externalOrigin, testIndex, &externalIj); + + t_assert( + (bool)internalIjFailed == (bool)externalIjFailed, + "internal/external failed matches when getting IJ"); + + if (internalIjFailed) { + continue; + } + + H3Index internalIndex; + int internalIjFailed2 = + H3_EXPORT(experimentalLocalIjToH3)( + internalOrigin, &internalIj, &internalIndex); + H3Index externalIndex; + int externalIjFailed2 = + H3_EXPORT(experimentalLocalIjToH3)( + externalOrigin, &externalIj, &externalIndex); + + t_assert( + (bool)internalIjFailed2 == (bool)externalIjFailed2, + "internal/external failed matches when getting " + "index"); + + if (internalIjFailed2) { + continue; + } + + t_assert(internalIndex == externalIndex, + "internal/external index matches"); + } + } + } + } + } } diff --git a/src/h3lib/lib/localij.c b/src/h3lib/lib/localij.c index 38392d053..275609689 100644 --- a/src/h3lib/lib/localij.c +++ b/src/h3lib/lib/localij.c @@ -87,24 +87,28 @@ const int PENTAGON_ROTATIONS_REVERSE_POLAR[7][7] = { {0, 1, 1, 0, 1, 1, 1}, // 6 }; -// Simply prohibit many pentagon distortion cases rather than handling them. -const bool FAILED_DIRECTIONS_II[7][7] = { - {false, false, false, false, false, false, false}, // 0 - {false, false, false, false, false, false, false}, // 1 - {false, false, false, false, true, false, false}, // 2 - {false, false, false, false, false, false, true}, // 3 - {false, false, false, true, false, false, false}, // 4 - {false, false, true, false, false, false, false}, // 5 - {false, false, false, false, false, true, false}, // 6 -}; -const bool FAILED_DIRECTIONS_III[7][7] = { +/** + * Prohibited directions when unfolding a pentagon. + * + * Indexes by two directions, both relative to the pentagon base cell. The first + * is the direction of the origin index and the second is the direction of the + * index to unfold. Direction refers to the direction from base cell to base + * cell if the indexes are on different base cells, or the leading digit if + * 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 + * of a failure cases. Currently, the logic is any unfolding across more than + * one icosahedron face is not permitted. + */ +const bool FAILED_DIRECTIONS[7][7] = { {false, false, false, false, false, false, false}, // 0 {false, false, false, false, false, false, false}, // 1 - {false, false, false, false, false, true, false}, // 2 - {false, false, false, false, true, false, false}, // 3 - {false, false, true, false, false, false, false}, // 4 - {false, false, false, false, false, false, true}, // 5 - {false, false, false, true, false, false, false}, // 6 + {false, false, false, false, true, true, false}, // 2 + {false, false, false, false, true, false, true}, // 3 + {false, false, true, true, false, false, false}, // 4 + {false, false, true, false, false, false, true}, // 5 + {false, false, false, true, false, true, false}, // 6 }; /** @@ -183,13 +187,7 @@ int h3ToLocalIjk(H3Index origin, H3Index h3, CoordIJK* out) { if (originOnPent) { int originLeadingDigit = _h3LeadingNonZeroDigit(origin); - // TODO: This previously included the Class III-based checks - // as in the index-on-pentagon case below, but these were - // removed due to some failure cases. It is possible that we - // could restrict this error to a narrower set of cases. - // 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. return 3; @@ -200,10 +198,7 @@ int h3ToLocalIjk(H3Index origin, H3Index h3, CoordIJK* out) { } else if (indexOnPent) { int indexLeadingDigit = _h3LeadingNonZeroDigit(h3); - if ((isResClassIII(res) && - FAILED_DIRECTIONS_III[indexLeadingDigit][revDir]) || - (!isResClassIII(res) && - FAILED_DIRECTIONS_II[indexLeadingDigit][revDir])) { + if (FAILED_DIRECTIONS[indexLeadingDigit][revDir]) { // TODO this part of the pentagon might not be unfolded // correctly. return 4; @@ -248,8 +243,7 @@ int h3ToLocalIjk(H3Index origin, H3Index h3, CoordIJK* out) { int originLeadingDigit = _h3LeadingNonZeroDigit(origin); int indexLeadingDigit = _h3LeadingNonZeroDigit(h3); - if (FAILED_DIRECTIONS_III[originLeadingDigit][indexLeadingDigit] || - FAILED_DIRECTIONS_II[originLeadingDigit][indexLeadingDigit]) { + if (FAILED_DIRECTIONS[originLeadingDigit][indexLeadingDigit]) { // TODO this part of the pentagon might not be unfolded // correctly. return 5; From d9bcfbb8ab0ddf2debc73ccab42a3ed9837adff3 Mon Sep 17 00:00:00 2001 From: Isaac Brodsky Date: Thu, 16 May 2019 16:30:28 -0700 Subject: [PATCH 2/4] Adjust comments and changelog entry --- CHANGELOG.md | 4 ++-- src/h3lib/lib/localij.c | 11 ++++------- 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 20674f29d..997a81661 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,8 +7,8 @@ The public API of this library consists of the functions declared in file ## [Unreleased] ### Changed -- Local coordinate spaces cannot cross more than one icosahedron face. -- All dynamic internal memory allocations happen on the heap instead of the stack +- Local coordinate spaces cannot cross more than one icosahedron edge. (#234) +- All dynamic internal memory allocations happen on the heap instead of the stack. (#235) ## [3.4.3] - 2019-05-02 ### Added diff --git a/src/h3lib/lib/localij.c b/src/h3lib/lib/localij.c index 01db024d1..557cf177f 100644 --- a/src/h3lib/lib/localij.c +++ b/src/h3lib/lib/localij.c @@ -97,7 +97,7 @@ const int PENTAGON_ROTATIONS_REVERSE_POLAR[7][7] = { * 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 + * due to failure cases. It's possible this could be restricted to a narrower set * of a failure cases. Currently, the logic is any unfolding across more than * one icosahedron face is not permitted. */ @@ -188,8 +188,7 @@ 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 unfolded correctly. return 3; } @@ -199,8 +198,7 @@ int h3ToLocalIjk(H3Index origin, H3Index h3, CoordIJK* out) { int indexLeadingDigit = _h3LeadingNonZeroDigit(h3); if (FAILED_DIRECTIONS[indexLeadingDigit][revDir]) { - // TODO this part of the pentagon might not be unfolded - // correctly. + // TODO In this case this part of the pentagon might not be unfolded correctly. return 4; } @@ -244,8 +242,7 @@ int h3ToLocalIjk(H3Index origin, H3Index h3, CoordIJK* out) { int indexLeadingDigit = _h3LeadingNonZeroDigit(h3); if (FAILED_DIRECTIONS[originLeadingDigit][indexLeadingDigit]) { - // TODO this part of the pentagon might not be unfolded - // correctly. + // TODO In this case this part of the pentagon might not be unfolded correctly. return 5; } From b440ffb5ef13d5a68b633b85c667718b8afa4d4b Mon Sep 17 00:00:00 2001 From: Isaac Brodsky Date: Thu, 16 May 2019 16:32:01 -0700 Subject: [PATCH 3/4] formatting --- src/h3lib/lib/localij.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/src/h3lib/lib/localij.c b/src/h3lib/lib/localij.c index 557cf177f..df6281e07 100644 --- a/src/h3lib/lib/localij.c +++ b/src/h3lib/lib/localij.c @@ -97,9 +97,9 @@ const int PENTAGON_ROTATIONS_REVERSE_POLAR[7][7] = { * within the pentagon base cell. * * This previously included a Class II/Class III check but these were removed - * due to failure cases. It's possible this could be restricted to a narrower set - * of a failure cases. Currently, the logic is any unfolding across more than - * one icosahedron face is not permitted. + * due to failure cases. It's possible this could be restricted to a narrower + * set of a failure cases. Currently, the logic is any unfolding across more + * than one icosahedron face is not permitted. */ const bool FAILED_DIRECTIONS[7][7] = { {false, false, false, false, false, false, false}, // 0 @@ -188,7 +188,8 @@ int h3ToLocalIjk(H3Index origin, H3Index h3, CoordIJK* out) { int originLeadingDigit = _h3LeadingNonZeroDigit(origin); if (FAILED_DIRECTIONS[originLeadingDigit][dir]) { - // TODO In this case this part of the pentagon might not be unfolded correctly. + // TODO In this case this part of the pentagon might not be + // unfolded correctly. return 3; } @@ -198,7 +199,8 @@ int h3ToLocalIjk(H3Index origin, H3Index h3, CoordIJK* out) { int indexLeadingDigit = _h3LeadingNonZeroDigit(h3); if (FAILED_DIRECTIONS[indexLeadingDigit][revDir]) { - // TODO In this case this part of the pentagon might not be unfolded correctly. + // TODO In this case this part of the pentagon might not be + // unfolded correctly. return 4; } @@ -242,7 +244,8 @@ int h3ToLocalIjk(H3Index origin, H3Index h3, CoordIJK* out) { int indexLeadingDigit = _h3LeadingNonZeroDigit(h3); if (FAILED_DIRECTIONS[originLeadingDigit][indexLeadingDigit]) { - // TODO In this case this part of the pentagon might not be unfolded correctly. + // TODO In this case this part of the pentagon might not be unfolded + // correctly. return 5; } From 83998087d0cc3dd085058abb5d273abcbbe8c672 Mon Sep 17 00:00:00 2001 From: Isaac Brodsky Date: Fri, 17 May 2019 16:52:19 -0700 Subject: [PATCH 4/4] Adjust comment about pentagon unfolding --- src/h3lib/lib/localij.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/h3lib/lib/localij.c b/src/h3lib/lib/localij.c index df6281e07..70da2f2cc 100644 --- a/src/h3lib/lib/localij.c +++ b/src/h3lib/lib/localij.c @@ -188,8 +188,9 @@ int h3ToLocalIjk(H3Index origin, H3Index h3, CoordIJK* out) { int originLeadingDigit = _h3LeadingNonZeroDigit(origin); if (FAILED_DIRECTIONS[originLeadingDigit][dir]) { - // TODO In this case this part of the pentagon might not be - // unfolded correctly. + // TODO: We may be unfolding the pentagon incorrectly in this + // case; return an error code until this is guaranteed to be + // correct. return 3; } @@ -199,8 +200,9 @@ int h3ToLocalIjk(H3Index origin, H3Index h3, CoordIJK* out) { int indexLeadingDigit = _h3LeadingNonZeroDigit(h3); if (FAILED_DIRECTIONS[indexLeadingDigit][revDir]) { - // TODO In this case this part of the pentagon might not be - // unfolded correctly. + // TODO: We may be unfolding the pentagon incorrectly in this + // case; return an error code until this is guaranteed to be + // correct. return 4; } @@ -244,8 +246,8 @@ int h3ToLocalIjk(H3Index origin, H3Index h3, CoordIJK* out) { int indexLeadingDigit = _h3LeadingNonZeroDigit(h3); if (FAILED_DIRECTIONS[originLeadingDigit][indexLeadingDigit]) { - // TODO In this case this part of the pentagon might not be unfolded - // correctly. + // TODO: We may be unfolding the pentagon incorrectly in this case; + // return an error code until this is guaranteed to be correct. return 5; }