fix infinite loop in cureLocalIntersections

[randomdude7332]

No description provided.

[mrgreywater]

Sorry for the late reply. Could you please add a Fixture / test-case to this PR to verify its function and guard against regression?

[mrgreywater]

Ref #89

[BaptisteCazaubieilh-TomTom]

Hey ! So I have this exact problem, your fix fixes it though it breaks some of our tests. I am not sure whether or not this is the right fix.

However, here is my situation with the nodes when going into the 'if' block within cureLocalIntersections:

A sequence in the do-while if block.

Expectations

Before remove:
p->prev --> p --> p->next --> p->next->next

After remove:
p->prev --> p->next->next

Steps

Remove p
Expected output: p->prev --> p->prev->next --> p->prev->next->next

Remove p->next
Expected output: p->prev --> p->prev->next->next

With real data

first iteration of the do - while triggering the if block:

Input: 0x1169c6228 --> 0x1169c6270 --> 0x1169c6348 --> 0x1169c61e0
Expected output:0x1169c6228 --> 0x1169c61e0

Steps:

Remove 0x1169c6270
Output = 0x1169c6228 --> 0x1169c6348 --> 0x1169c61e0 | OK

Remove 0x1169c6348
Output = 0x1169c6228 --> 0x1169c61e0 | OK

Second iteration of the do - while triggering the if block:

Input: 0x1169c61e0 --> 0x1169c6228-- > 0x1169c61e0 --> 0x1169c6228
Expected Output: 0x1169c61e0 --> 0x1169c6228

Steps:

Remove 0x1169c6228
Output: 0x1169c61e0 --> 0x1169c61e0 --> 0x1169c61e0 | NOT OK

Remove 0x1169c61e0
Output: 0x1169c61e0 --> 0x1169c61e0 | NOT OK

Observations

In the second iteration, there are only two nodes involves that points at each other. Obviously, they can not be both removed and this is why this ends up in a node pointing at itself from both sides.

I am not sure whether removing p->next before p is an actual solution but I hope that this can help bring a solution.

[BaptisteCazaubieilh-TomTom]

The problem can be reproduced as follows:

#include <vector>

#include <mapbox/earcut.hpp>
#include <gtest/gtest.h>

struct Point
{
  double x, y;
};

namespace mapbox
{
namespace util
{
template<>
struct nth<0, Point>
{
  inline static double get(const Point& t)
  {
    return t.x;
  };
};
template<>
struct nth<1, Point>
{
  inline static double get(const Point& t)
  {
    return t.y;
  };
};

} // namespace util
} // namespace mapbox

TEST(Test, EarcutInifinitLoop)
{
  using Vertices = std::vector<Point>;
  Vertices v = {
    {42.1820068359375, 4.1820726067227322886}, {42.1875, 4.1820726067227322886},
    {42.2808837890625, 4.1492006930992886282}, {42.38525390625, 4.0560562101787809297},
    {42.484130859375, 3.9355003860137967031},  {42.528076171875, 3.924539892198442903},
    {42.5555419921875, 3.9026184734373017271}, {42.5830078125, 3.8039650544202538995},
    {42.5775146484375, 3.7984839750369747691}, {42.5555419921875, 3.7875217118824227036},
    {42.5445556640625, 3.7930028608410482072}, {42.5445556640625, 3.7984839750369747691},
    {42.56103515625, 3.8204080831949407404},   {42.5445556640625, 3.7984839750369747691},
    {42.5445556640625, 3.7930028608410482072}, {42.5445556640625, 3.765596769419838008},
    {42.5225830078125, 3.6285534948454238879}, {42.550048828125, 3.4476246666468650659},
    {42.56103515625, 3.4037578795775886853},   {42.4676513671875, 3.3269862108134997669},
    {42.506103515625, 3.2666614116116146072},  {42.5225830078125, 3.1295462178004864739},
    {42.5885009765625, 3.0966358718415634144}, {42.56103515625, 2.981441678317485966},
    {42.56103515625, 2.9759559359447682603},   {42.4786376953125, 2.8881803785839963439},
    {42.34130859375, 2.789424777005988787},    {42.36328125, 2.6851737608572987526},
    {42.374267578125, 2.6303012095641613577},  {42.374267578125, 2.6248138208185074483},
    {42.308349609375, 2.6028640254083494909},  {42.2918701171875, 2.520548914249552741},
    {42.34130859375, 2.4107873635632364184},   {42.34130859375, 2.3998107149273102401},
    {42.3248291015625, 2.3888339781603642464}, {42.2918701171875, 2.520548914249552741},
    {42.308349609375, 2.6028640254083494909},  {42.3687744140625, 2.6248138208185074483},
    {42.374267578125, 2.6248138208185074483},  {42.3577880859375, 2.6851737608572987526},
    {42.34130859375, 2.7839381088009784726},   {42.34130859375, 2.789424777005988787},
    {42.56103515625, 2.9759559359447682603},   {42.5390625, 3.1021210008142987569},
    {42.5006103515625, 3.1240612316206619603}, {42.462158203125, 3.3215022897186035067},
    {42.462158203125, 3.3269862108134997669},  {42.5445556640625, 3.4476246666468650659},
    {42.51708984375, 3.5189038690258671593},   {42.4951171875, 3.5189038690258671593},
    {42.47314453125, 3.5463175009986707131},   {42.506103515625, 3.6011423201587349219},
    {42.5115966796875, 3.6340356301913807613}, {42.5445556640625, 3.8258890228283370938},
    {42.5665283203125, 3.8204080831949407404}, {42.5775146484375, 3.8039650544202538995},
    {42.550048828125, 3.9026184734373017271},  {42.4566650390625, 3.9300201571114752319},
    {42.38525390625, 4.0505767861334671665},   {42.38525390625, 4.0560562101787809297},
    {42.3468017578125, 4.0505767861334671665}, {42.3193359375, 4.077973534281738921},
    {42.2808837890625, 4.1437219075192599504}, {42.154541015625, 4.1437219075192599504},
    {42.07763671875, 4.1820726067227322886},
  };

  std::vector<Vertices> partVertices;
  partVertices.emplace_back(std::move(v));

  mapbox::earcut<std::uint32_t>(partVertices);
}

[mrgreywater]

Thanks for the sample, I'll take a look and maybe merge the PR if @mourner doesn't chime in. I'd prefer if the changes came upstream from earcut.js though.

[mourner]

I'd prefer if the changes came upstream from earcut.js though.

Yep — let me check if the test case above reproduces there and whether the fix affects other test cases.

[mourner]

Just tried that test case and it triangulates fine in JS (added in a branch here mapbox/earcut@4c4b306), so maybe that bug is C++-specific? Not sure, perhaps worth investigating.

[BaptisteCazaubieilh-TomTom]

Just tried that test case and it triangulates fine in JS (added in a branch here mapbox/earcut@4c4b306), so maybe that bug is C++-specific? Not sure, perhaps worth investigating.

I think I found the problem: it relates to double precision on a given platform. If I change the Point to structure with the following:

template<int N>
constexpr double get10PowN() {
  return 10. * get10PowN<N - 1>();
}
template<>
constexpr double get10PowN<1>() {
  return 10.;
}

double do_round(double val)
{
  constexpr double round_factor = get10PowN<std::numeric_limits<double>::digits10>();
  return std::round(val * round_factor) / round_factor;
}

struct Point
{
  Point(double _x, double _y) : x(do_round(_x)), y(do_round(_y))
  {
  }
  double x, y;
};

On my platform, round_factor is 15 and the tests passes. Any value above, and the test goes infinite loop.

[mourner]

Interesting! JS uses double precision but there's no issue there, so this must be something weird platform-specific... But doing an equivalent change in JS, all the tests still pass, so I think no harm landing this.

[bartoszkosiorek]

It seems to integrate this change the Fixture / test-case needs to be added to that PR.

[borkhuis]

I am running into this same (original) issue, where my application end up in an infinite loop in cureLocalIntersections. Is there any progress on this PR?
Could implementing this change cause any other issues?

[borkhuis]

I have been looking into this a bit further, trying to find what could be causing this problem. I added debugging to see what the pointers are doing when it fails and when it works (after switching the two lines.

Below is an example of failed run. The first number is the iteration, the second one is the number of times the if section is executed, the third number is the number of items in the list. Next all pointers are shown (100 means that the number cannot be counted).

The REMOVE lines are printed around the removeNode calls:

1 0 6 start: 0xaaaae4e0de78 a: 0xaaaae4e0de30 p: 0xaaaae4e0de78 p->next: 0xaaaae4e0dec0 b: 0xaaaae4e0df50 b->next: 0xaaaae4e0df98
2 0 6 start: 0xaaaae4e0de78 a: 0xaaaae4e0de78 p: 0xaaaae4e0dec0 p->next: 0xaaaae4e0df50 b: 0xaaaae4e0df98 b->next: 0xaaaae4e0db60
>>REMOVE before:  p: prev->prev: 0xaaaae4e0de30 prev: 0xaaaae4e0de78 p: 0xaaaae4e0dec0 next: 0xaaaae4e0df50 next->next: 0xaaaae4e0df98
>>REMOVE p:       b: prev->prev: 0xaaaae4e0de78 prev: 0xaaaae4e0df50 b: 0xaaaae4e0df98 next: 0xaaaae4e0db60 next->next: 0xaaaae4e0de30
>>REMOVE p->next: b: prev->prev: 0xaaaae4e0de30 prev: 0xaaaae4e0de78 b: 0xaaaae4e0df98 next: 0xaaaae4e0db60 next->next: 0xaaaae4e0de30
3 1 4 start: 0xaaaae4e0df98 a: 0xaaaae4e0df98 p: 0xaaaae4e0db60 p->next: 0xaaaae4e0de30 b: 0xaaaae4e0de78 b->next: 0xaaaae4e0df98
>>REMOVE before:  p: prev->prev: 0xaaaae4e0de78 prev: 0xaaaae4e0df98 p: 0xaaaae4e0db60 next: 0xaaaae4e0de30 next->next: 0xaaaae4e0de78
>>REMOVE p:       b: prev->prev: 0xaaaae4e0df98 prev: 0xaaaae4e0de30 b: 0xaaaae4e0de78 next: 0xaaaae4e0df98 next->next: 0xaaaae4e0de30
>>REMOVE p->next: b: prev->prev: 0xaaaae4e0de78 prev: 0xaaaae4e0df98 b: 0xaaaae4e0de78 next: 0xaaaae4e0df98 next->next: 0xaaaae4e0de78
4 2 2 start: 0xaaaae4e0de78 a: 0xaaaae4e0de78 p: 0xaaaae4e0df98 p->next: 0xaaaae4e0de78 b: 0xaaaae4e0df98 b->next: 0xaaaae4e0de78
>>REMOVE before:  p: prev->prev: 0xaaaae4e0df98 prev: 0xaaaae4e0de78 p: 0xaaaae4e0df98 next: 0xaaaae4e0de78 next->next: 0xaaaae4e0df98
>>REMOVE p:       b: prev->prev: 0xaaaae4e0de78 prev: 0xaaaae4e0de78 b: 0xaaaae4e0df98 next: 0xaaaae4e0de78 next->next: 0xaaaae4e0de78
>>REMOVE p->next: b: prev->prev: 0xaaaae4e0de78 prev: 0xaaaae4e0de78 b: 0xaaaae4e0df98 next: 0xaaaae4e0de78 next->next: 0xaaaae4e0de78
5 3 100 start: 0xaaaae4e0df98 a: 0xaaaae4e0de78 p: 0xaaaae4e0de78 p->next: 0xaaaae4e0de78 b: 0xaaaae4e0de78 b->next: 0xaaaae4e0de78
6 3 100 start: 0xaaaae4e0df98 a: 0xaaaae4e0de78 p: 0xaaaae4e0de78 p->next: 0xaaaae4e0de78 b: 0xaaaae4e0de78 b->next: 0xaaaae4e0de78
7 3 100 start: 0xaaaae4e0df98 a: 0xaaaae4e0de78 p: 0xaaaae4e0de78 p->next: 0xaaaae4e0de78 b: 0xaaaae4e0de78 b->next: 0xaaaae4e0de78

After the third remove-cycle the administration looks corrupted: p, p->next and p->prev all point to the same element, but start point to a different unconnected element. Which means that the while loop will continue forever. This can also be seen in the last REMOVE p line: b points to one element, but all next and prev pointers point to another element.

Now for a run with the order of the removeNode line changed:

1 0 6 start: 0xaaaabcbefe18 a: 0xaaaabcbefdd0 p: 0xaaaabcbefe18 p->next: 0xaaaabcbefe60 b: 0xaaaabcbefef0 b->next: 0xaaaabcbeff38
2 0 6 start: 0xaaaabcbefe18 a: 0xaaaabcbefe18 p: 0xaaaabcbefe60 p->next: 0xaaaabcbefef0 b: 0xaaaabcbeff38 b->next: 0xaaaabcbefb00
>>REMOVE before:  p: prev->prev: 0xaaaabcbefdd0 prev: 0xaaaabcbefe18 p: 0xaaaabcbefe60 next: 0xaaaabcbefef0 next->next: 0xaaaabcbeff38
>>REMOVE p->next: b: prev->prev: 0xaaaabcbefe18 prev: 0xaaaabcbefe60 b: 0xaaaabcbeff38 next: 0xaaaabcbefb00 next->next: 0xaaaabcbefdd0
>>REMOVE p:       b: prev->prev: 0xaaaabcbefdd0 prev: 0xaaaabcbefe18 b: 0xaaaabcbeff38 next: 0xaaaabcbefb00 next->next: 0xaaaabcbefdd0
3 1 4 start: 0xaaaabcbeff38 a: 0xaaaabcbeff38 p: 0xaaaabcbefb00 p->next: 0xaaaabcbefdd0 b: 0xaaaabcbefe18 b->next: 0xaaaabcbeff38
>>REMOVE before:  p: prev->prev: 0xaaaabcbefe18 prev: 0xaaaabcbeff38 p: 0xaaaabcbefb00 next: 0xaaaabcbefdd0 next->next: 0xaaaabcbefe18
>>REMOVE p->next: b: prev->prev: 0xaaaabcbeff38 prev: 0xaaaabcbefb00 b: 0xaaaabcbefe18 next: 0xaaaabcbeff38 next->next: 0xaaaabcbefb00
>>REMOVE p:       b: prev->prev: 0xaaaabcbefe18 prev: 0xaaaabcbeff38 b: 0xaaaabcbefe18 next: 0xaaaabcbeff38 next->next: 0xaaaabcbefe18
4 2 2 start: 0xaaaabcbefe18 a: 0xaaaabcbefe18 p: 0xaaaabcbeff38 p->next: 0xaaaabcbefe18 b: 0xaaaabcbeff38 b->next: 0xaaaabcbefe18
>>REMOVE before:  p: prev->prev: 0xaaaabcbeff38 prev: 0xaaaabcbefe18 p: 0xaaaabcbeff38 next: 0xaaaabcbefe18 next->next: 0xaaaabcbeff38
>>REMOVE p->next: b: prev->prev: 0xaaaabcbeff38 prev: 0xaaaabcbeff38 b: 0xaaaabcbeff38 next: 0xaaaabcbeff38 next->next: 0xaaaabcbeff38
>>REMOVE p:       b: prev->prev: 0xaaaabcbeff38 prev: 0xaaaabcbeff38 b: 0xaaaabcbeff38 next: 0xaaaabcbeff38 next->next: 0xaaaabcbeff38

And then the function exits, as it should: b, b->prev and b->next all point to the same element, which means that the exit condition (b->next != b) is false.

So it looks like this is working (at least for this data-set).

Another option could be to only continue the while loop when p != p->next, which means that there are at least two elements left in the list (or do both).

Looking at the last Remove cycle in both examples, the count of elements in the list is 2, which (I think) does not have to be executed, as there cannot be a selfintersection (if I understand the function correctly). So could changing the while condition to this also work:

    } while ((p != start) && (p != p->next) && (p != p->next->next));