A* bug (A star search algorithm)

Question

I have started to study algorithms and software development and, as a small self evaluation project I have decided to write the A* search algorithm in C++. It uses Qt and OpenGL for the visual part (but that is not important).

Using mostly this source: A* Pathfinding for Beginners

I have write a small app, however I am have found a bug that I cant fix. It appears that for some reason the parent of a node close to the wall is set to the wall.(?) And the parent of the wall is set to the the start point(?) because of the way I am storing the info.

I have used a stateMatrix[][] where

1 = entrance green;
2 = exit;
3 = wall and;
4 = path;

I have also used matrix to represent openNodes and closedNode. The closedNodes matrix is bool matrix the openNode matrix also stores some info:

The openNodes instructions are:

openNodes[100][100][6];
0 - bool open or closed
1 - F
2 - G
3 - H
4 - parentX
5 - parentY

I know that there are better ways to code this but I have not yet got to this lesson ;)

Here is the code of the astar file:

#include <math.h>
#include "apath.h"

aPath::aPath()
{
    gridSize = 100;

    int i, j, k;

    for(i = 0; i < gridSize; i++)
        for(j = 0; j < gridSize; j++)
        {
            stateMatrix[i][j] = 0;
            for(int k = 0; k < 6; k++) openNodes[i][j][k] = 0;
            closedNodes[i][j] = 0;
        }

    targetX = targetY =
            openX = openY = entranceX = entranceY = 0;
}

void aPath::start()
{
    bool testOK = false;
    int G = 0;

    openNodes[entranceX][entranceY][0] = 1;
    openNodes[entranceX][entranceY][2] = 14;
    openNodes[entranceX][entranceY][3] = euclidean(entranceX,
                                                   entranceY);
    openNodes[entranceX][entranceY][1] =
            openNodes[entranceX][entranceY][2] +
            openNodes[entranceX][entranceY][3];
    openNodes[entranceX][entranceY][4] = entranceX;
    openNodes[entranceX][entranceY][5] = entranceY;

    int i, j, x, y;

    while(closedNodes[targetX][targetY] == 0)
    {
        searchLessOpen();
        closedNodes[openX][openY] = 1;
        openNodes[openX][openY][0] = 0;
        //Check the 8 squares around
        for(i = openX - 1; i <= openX + 1; i++)
            for(j = openY - 1; j <= openY + 1; j++)
            {
                //check if the square is in the limits,
                //is not a wall and is not in the closed list
                if((i  >= 0) && (j >= 0)  &&
                        (i < gridSize) && (j < gridSize) &&
                        (stateMatrix[i][j] != 3) &&
                        (closedNodes[i][j] == 0))
                {
                    //G calculus. If it is in the edge it costs more
                    x = i - openX + 1;
                    y = j - openY + 1;
                    if((x == 0 && y == 0) ||
                            (x == 0 && y == 2) ||
                            (x == 2 && y == 0) ||
                            (x == 2 && y == 2))
                    {
                        G = 14;
                    }
                    else G = 10;

                    //check if node is already open
                    if(openNodes[i][j][0] == 0)
                    {
                        openNodes[i][j][0] = 1;
                        openNodes[i][j][2] = G;
                        openNodes[i][j][3] = euclidean(i,j);
                        openNodes[i][j][1] = openNodes[i][j][2] + openNodes[i][j][3];
                        openNodes[i][j][4] = openX;
                        openNodes[i][j][5] = openY;
                    }
                    else //if node is open, check if this path is better
                    {
                        if(G < openNodes[i][j][2])
                        {
                            openNodes[i][j][2] = G;
                            openNodes[i][j][1] = openNodes[i][j][2] + openNodes[i][j][3];
                            openNodes[i][j][4] = openX;
                            openNodes[i][j][5] = openY;
                        }
                    }
                }
            }
    }

    reconstruct();
}

void aPath::reconstruct()
{
    bool test = false;

    int x = openNodes[targetX][targetY][4];
    int y = openNodes[targetX][targetY][5];

    do
    {
        stateMatrix[x][y] = 4;
        x = openNodes[x][y][4];
        y = openNodes[x][y][5];

        if(x == entranceX && y == entranceY) test = true;

    } while(test == false);
}

int aPath::euclidean(int currentX, int currentY)
{
    int dx = targetX - currentX;
    int dy = targetY - currentY;

    return 10*sqrt((dx*dx)+(dy*dy));
}

void aPath::searchLessOpen()
{
    int F = 1000000;
    int i, j;

    for(i = 0; i < gridSize; i++)
        for(j = 0; j < gridSize; j++)
        {
            if(openNodes[i][j][0] == 1)
            {
                if(openNodes[i][j][1] <= F)
                {
                    F = openNodes[i][j][1];
                    openX = i;
                    openY = j;
                }
            }
        }
}

Does anyone know what I am doing wrong?

Thanks. Edit: Here are some pictures:

Answer 1

In aPath::start(), you have:

openNodes[entranceX][entranceY][0] = 1;
openNodes[entranceX][entranceY][2] = 14;
openNodes[entranceX][entranceY][3] = euclidean(entranceX,
                                               entranceY);
openNodes[entranceX][entranceY][3] =
        openNodes[entranceX][entranceY][2] +
        openNodes[entranceX][entranceY][3];
openNodes[entranceX][entranceY][4] = entranceX;
openNodes[entranceX][entranceY][5] = entranceY;

Why is there no value for subscript [1]? And why do you assign two different values to subscript [3]? Also, to be honest, the entranceX and entranceY names are too long for the job they're doing; they make the code less readable (though I'm sure you were told to use good meaningful names). For these array indexes, I'd probably use just x and y.

At the code:

    //Check the 8 squares around
    for(i = openX - 1; i <= openX + 1; i++)
        for(j = openY - 1; j <= openY + 1; j++)
        {

I would probably ensure that neither i nor j took on invalid values with code such as:

    //Check the 8 squares around (openX, openY)
    int minX = max(openX - 1, 0);
    int maxX = min(openX + 1, gridsize);
    int minY = max(openY - 1, 0);
    int maxY = min(openY + 1, gridsize);
    for (i = minX; i <= maxX; i++)
        for (j = minY; j <= maxY; j++)
        {

I am not sure whether you need to explicitly check for the case where i == openX && j == openY (the current cell); it is not one of the 8 cells around the current cell (because it is the current cell), but the other conditions may already deal with that. If not:

            if (i == openX && j == openY)
                continue;

I note that we have no definitions of openX and openY or a number of other non-local variables. This makes it hard to work out whether they are class member variables or global variables of some sort. We also can't see how they're initialized, nor the documentation on what they represent.

Most plausible source of trouble

In aPath::SearchLessOpen(), you have:

        if(openNodes[i][j][0] == 1)
        {
            if(openNodes[i][j][6] <= F)
            {
                F = openNodes[i][j][7];

You indicated in your description that the subscripts on openNodes in the last place ranged over 0..5; your code, though, is accessing subscripts 6 and 7. This could easily lead to the sort of confusion you describe - you are accessing data out of bounds. I think this might easily be the root of your trouble. When you access openNodes[i][j][6], this is technically undefined behaviour, but the most likely result is that it is reading the same data as if you'd written openNodes[i][j+1][0] (when j < gridsize - 1). Similarly, openNodes[i][j][7] is equivalent to accessing openNodes[i][j+1][1], with the same caveats.