Update FastAStarAlgorithm.cs

[Smith81D]

The proposed changes fix some problems with the algorithm.

The first is rather simple:

Utility.InRange(start, goal, AreaSize) checks a "less or equal" relation for the distance, so it returns "true" if "start" and "goal" are exactly "AreaSize" fields apart. But since the coordinate system used here covers only the values [0 ... AreaSize - 1], this case is actually out of bounds. Propose to change this to Utility.InRange(start, goal, AreaSize - 1) in two places.

The second is more suble and concerns the following lines:

m_xOffset = (start.X + goal.X - AreaSize) / 2;
m_yOffset = (start.Y + goal.Y - AreaSize) / 2;

These offsets define the transformation between the local coordinate system that the algorithm uses, and the global coordinate system of the map. This transformation is pathological for the edge case "goal.X = start.X + AreaSize - 1" (or a similar relation for the Y coordinate), where the start and goal point have the maximum distance in the local coordinate system. For then we get

m_xOffset = (start.X + (start.X + AreaSize - 1) - AreaSize) / 2 = (2*start.X - 1) / 2 ;

Since integer division discards extra digits, this rounds to "m_xOffset = start.X - 1" and implies that the shifted coordinates of start and goal are

start.X - m_xOffset = 1
goal.X - m_xOffset = AreaSize

The later of these is out of bounds.

To fix this, it is proposed to add the ceiling function:

m_xOffset = (int)Math.Ceiling( (start.X + goal.X - AreaSize) / 2.0 ); m_yOffset = (int)Math.Ceiling( (start.Y + goal.Y - AreaSize) / 2.0 );

This ensures that the coordinate transformation is always well behaved and for the edge case gives:

start.X - m_xOffset = 0
goal.X - m_xOffset = AreaSize - 1

The third issue is an unnecessary termination condition:

if ( count == 0 )
break;

This causes the algorithm to terminate with failure whenever any expanded node has no successors, even if there are other nodes remaining on the open list. This can prevent the algorithm from finding a path out of enclosed spaces, even if an obvious short path exists.

[kamronbatman]

This is shortcutting the for loop below it. So deleting it is a functional no-op.

[Smith81D]

That would be the case, if it were "continue" instead of "break". With "break" we are exiting the while loop. This is equal to claiming that the open list ist empty (the condition fo the while loop), which isn't necessarily the case at this point.

[kamronbatman]

So it should be continue? At which case, yes it is redundant vs just leaving the for-loop.

[Smith81D]

I think it should be removed entirely.

The point is the following: The AStar algorithm keeps a list of grid points ("nodes") called "open list", which it considers promising steps towards the goal. From these it repeatedly choses the most promising one to "expand", i.e. to remove from the list and then to check for the possibility of adding its neighbors to the open list. There should, in principle, be three ways the algorithm can terminate:

1. The expansion of the node encounters the goal, in which case the algorithm has succeeded.
2. The number of expanded nodes reaches some maximum value which was set as an upper limit, in which case the algorithm has failed.
3. There are no more nodes left on the open list, i.e. all expansions have either run into barriers or the edge of the grid, in case of which the algorithm has failed.

The outermost while-loop here while (m_OpenList != -1){ ... } tests condition number 3).

What the if-statement does, is terminate the algorithm with failure if the expansion of any node fails, i.e. is blocked from all directions, even if there are other nodes left on the open list. This is not what should happen. The correct thing to do is to move on to the next best node on the open list and expand that one.

So yes, if you will, replacing "break" by "continue" would be correct. But then the statement would be redundant and could be dropped.

The problem with having the break statement in is that, if some node expands into a "pocket" surrounded by barriers, and the node inside the pocket gets chosen next for expansion, then the algorithm will terminate, even if there is a simple way to go around the obstacle.

In some tests that I ran, this happened for instance when trying to exit a cluttered building through a narrow door. Deleting the if-statement fixed the problem and allowed the algorithm to succeed even in such cases.

[kamronbatman]

Can we find a better way to do this without floats and ceiling? 🤔

[Smith81D]

From my experiments with this algorithm, the cost of this operation is negligible (it even falls outside of the outermost loop). But one possible alternative is to use Utility.InRange(start, goal, AreaSize - 2) instead of Utility.InRange(start, goal, AreaSize - 1) as suggested above. Then the problematic edge case is avoided.

[kamronbatman]

I think there is confusion about the AreaSize. It's actually a square with radius of 18. This means it is 37. The reason 38 is used is to account for the square you are currently on. For that reason, goal.X - m_xOffset = AreaSize being out of bounds should be ok?

[Smith81D]

I think there is confusion about the AreaSize. It's actually a square with radius of 18. This means it is 37. The reason 38 is used is to account for the square you are currently on. For that reason, goal.X - m_xOffset = AreaSize being out of bounds should be ok?

Yes, "AreaSize" is a misnomer. The meaning of this variable is the linear size of each dimension of the grid that is used. The x and y coordinates both run from 0 to AreaSize - 1.

The position of the mobile that calls the algorithm isn't treated in any special way. It falls somewhere in this coordinate system. Basically, this is a coordinate system with a midpoint that coincides with the center of mass of the start and goal points. For the case goal.X - m_xOffset = AreaSize, one of the points is necessarily outside of the coordinate system.

Further subtleties arise because we have integer coordinates, so we must round in one way or another.