Graham scan c++ won't work

Question

My code for the graham scan is not working, it is supposed to get the perimeter of the convex hull. It gets the input of n points, which can have decimals. The algorithm returns a value higher than the actual perimeter.

I am using what I understood from: http://en.wikipedia.org/wiki/Graham_scan

#include <iostream>
#include <cstdio>
#include <cmath>
#include <vector>
#include <algorithm>

using namespace std;

#define PI 3.14159265

int nodes;

double xmin=10000000, ymin=10000000, totes=0;

struct ppoint
{
    double x, y, angle;
    void anglemake()
    {
        angle=atan2(y-ymin, x-xmin)*180/PI;
        if(angle<0)
        {
            angle=360+angle;
        }
    }
} np;

The point structure, with a function to make the angle between it and the point with lowest y and x coordinates

vector<ppoint> ch, clist;

bool hp(ppoint i, ppoint j)
{
    return i.angle<j.angle;
}

double cp(ppoint a, ppoint b, ppoint c)
{
    return ((b.x-a.x)*(c.y-a.y))-((b.y-a.y)*(c.x-a.x));
}

The z-cross product function

double dist(ppoint i, ppoint j)
{double vd, hd;
    vd=(i.y-j.y)*(i.y-j.y);
    hd=(i.x-j.x)*(i.x-j.x);
    return sqrt(vd+hd);
}

Distance generator

int main()
{
    scanf("%d", &nodes);
    for(int i=0; i<nodes; i++)
    {
        scanf("%lf%lf", &np.x, &np.y);
        if(np.y<ymin || (np.y==ymin && np.x<xmin))
        {
           ymin=np.y;
            xmin=np.x;
        }
        ch.push_back(np);
    }

Gets the points

    for(int i=0; i<nodes; i++)
    {
        ch[i].anglemake();
    }
    sort(ch.begin(), ch.end(), hp);
    clist.push_back(ch[0]);
    clist.push_back(ch[1]);
    ch.push_back(ch[0]);

Sorts and starts Graham Scan

    for(int i=2; i<=nodes; i++)
    {
        while(cp(clist[clist.size()-2], clist[clist.size()-1], ch[i])<0)
        {
            clist.pop_back();
        }
        clist.push_back(ch[i]);
    }

Graham scan

    for(int i=0; i<nodes; i++)
    {
        totes+=dist(clist[i], clist[i+1]);
    }

Gets length of the perimeter

    printf("%.2lf\n", totes);
    return 0;
}

Related: [Graham scan issue at high amount of points](http://stackoverflow.com/questions/25190164/graham-scan-issue-at-high-amount-of-points/25204997#25204997). — dbc, Dec 15 '15 at 09:21

score 1 · Answer 1 · answered Jun 11 '13 at 20:12

1

Just for the interest, print out value of nodes and clist.size() before the dist summming.

At glance clist can have nodes+1 items only if pop_back never happens. and if it does you have undefined behavior.

answered Jun 11 '13 at 20:12

Balog Pal

16,195
2
23
37

I think that could be the case. If you replace `vec[i]` with `vec.at(i)`, the vector will validate the index and throw an exception, which is the smallest and least intrusive change. Other than that, before `c.pop_back()`, make sure the element exists with `assert(!c.empty())`. – Ulrich Eckhardt Jun 11 '13 at 20:21

score 0 · Answer 2 · answered Jun 11 '13 at 20:33

I think the problem is here:

for(int i=0; i<nodes; i++)
{
    totes+=dist(clist[i], clist[i+1]);
}

clist will only have the remaining number of points, not nodes + 1 which is the number of points you loaded plus one. Storing this number in the first place is a fault IMHO, because it starts with the number of points, then you add one to close the loop, then again you remove points to make the hull convex. Just use container.size() and everything is clear.

One more note: Use a checked C++ standard library implementation for debugging. These would have warned you of undefined behaviour like accessing a vector beyond its range. C++ is a language that allows you to shoot yourself in the foot in to many ways, all in the name of performance. This is nice and well, unless when debugging, which is when you want the best diagnostics available.

Graham scan c++ won't work

2 Answers2