Skip to content
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

fix infinite loop in cureLocalIntersections #90

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

randomdude7332
Copy link

No description provided.

@mrgreywater
Copy link
Collaborator

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
Copy link
Collaborator

Ref #89

@BaptisteCazaubieilh-TomTom
Copy link

BaptisteCazaubieilh-TomTom commented Apr 26, 2023

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
Copy link
Collaborator

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
Copy link
Member

mourner commented Apr 27, 2023

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
Copy link
Member

mourner commented Apr 27, 2023

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
Copy link
Member

mourner commented Apr 27, 2023

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
Copy link

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

@borkhuis
Copy link

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
Copy link

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));

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants