Improve A* Pathfinding Efficiency by Addressing Final Segment Penalty

[wichern]

Issue

The A* cost function (noFlag::GetPunishmentPoints) adds a 500 point penalty to segments without carrier. This includes the segment from a building's flag to itself, which will never have a carrier. If the building is the destination of the pathfinding, the algorithm cannot find a better segment.
As a result, the large penalty causes A* to unnecessarily search a wide area around the final segment, even though no better options exist.

Solutions

I see two approaches to resolve this:

1. Clean solution

Modify noFlag::GetPunishmentPoints to handle this special case:

if(dir == Direction::NorthWest)
{
    noBase* no = world->GetNO(world->GetNeighbour(pos, Direction::NorthWest));
    if(no->GetType() == NodalObjectType::Buildingsite || no->GetType() == NodalObjectType::Building)
    {
        points -= 500;
    }
}

This solution directly addresses the issue but changes the behavior of FindPath when a max cost value is specified, making it not replay-compatible.

2. Dirty/Stable Solution (Implemented)

Make the A* algorithm aware of the issue and re-add the 500 points at the end of the run to make sure that FindPath returns consistent cost values. While less elegant, this approach preserves replay compatibility and is implemented in this change.

Impact

This fix significantly improves performance. Running games without a UI or using Test_autoplay is twice as fast, as pathfinding is a major component of game logic.

I prefer the clean implementation. The dirty one serves as proof-of-concept.

[Flamefire]

Awesome finding!

If I understand this correctly this affects only the case where a path for a ware is searched that has a building as its target.
Every possible path will have the segment from the flag to the building as its last part.
This part has an additional penalty of 500 points which is much higher than the heuristic using only the (semi-euclidean) distance which completely throws off the path finding to search in a radius of 500 more until using that.

Your "clean" solution detects paths from flag to buildings and deduces the 500 points from the score.
However any path to a building that is not the target building is always invalid (except for the harbor building)

Your "dirty" solution detects the path from flag to target building and deduces the 500 points from the score and readds it at the end. I don't really like that as it really litters the generic (road)pathfinder with a special case relying on non-local knowledge. (the 500 is a magic number here defined elsewhere)

I think we can improve on that: When the target is a building we search for a route to the flag instead. This avoids the penalty in addition to even considering any other route for an additional, very minor performance bonus. I guess every target for a ware is a building anyway.
We might need to deduce the cost for the segment from flag to building from the max cost at least for backwards compatibility. I guess for being most general it also makes sense, but I'd be willing to completely omit that cost as it shouldn't matter anywhere.

However we have a similar issue for harbor buildings: The algorithm will still add the 500 points additional penalty there. However I guess this is just an offset to the ship costs that effectively shouldn't matter. We could fix that by changing the GetPunishmentPoints function as you suggested as that fixes both cases if we wanted.

So IMO the best solution is to handle the final segment in RoadPathFinder::FindPath by finding a path to the buildings flag instead. If the source is already the building flag we need to return early to avoid source==target.
If this really breaks replays we can handle the (constant!) cost of that final segment there by determining it once, which is trivial.
But IIRC the cost returned is just used to compare different paths against each other so removing a value that is equal for all alternatives shouldn't change the outcome.
We could handle the case for harbors but I'd say the additional check in each flag computation for this rare case is not worth it as the seafaring distance cost of 2 * world->CalcHarborDistance(GetHarborPosID(), harbor_building->GetHarborPosID()) + 10 is basically just changed to 2 * world->CalcHarborDistance(GetHarborPosID(), harbor_building->GetHarborPosID()) + 510 which might be fine. That constant is a heuristic anyway to account for wait time for ships.

What do you think?
Again: Great finding! I banged my head against the pathfinding algorithm so many times to make it faster without seeing that fundamental issue.

[wichern]

Your summary is mostly accurate, but there's one clarification: the actual cost value does matter for replay compatibility. It's difficult to pinpoint the exact source of the hash mismatch in Test_replay, though. My current hypothesis is that it comes from GamePlayer::FindClientForCoin(), which subtracts the cost value (length) from another heuristic.

I believe keeping the GetPunishmentPoints() method unchanged could be misleading for developers. It implies that the penalty applies only to roads without carriers, whereas it also adds a penalty for harbors and has the issue with the final segments when used incorrectly. Limiting the search to the flag position of a building feels like a workaround rather than addressing the root issue.

I recommend updating the noFlag::GetPunishmentPoints function and accept that replays have become incompatible. There might be more optimizations possible in the future, but every optimization has the potential to break replays, because of the changed cost value and because it could lead to different routes being found.

We could add another parameter to GetPunishmentPoints() (goal) in order to determine that a SHIP_TRANSPORT_PENALTY should be added.

GetPunishmentPoints(dir, goal)
  points = |wares(dir)| * PATHFINDING_PENALTY_WARES
  if(humanCarrier in dir)
    if(busy(humanCarrier) and not donkey in dir)
      points += PATHFINDING_PENALTY_CARRIER_BUSY
  else if(no carriers in dir)
    if(isBuildingEntry(dir))
      if(dir reaches goal)
        return 0
      assert(isHarbor(dir))
      points += PATHFINDING_PENALTY_SHIPPING
  return points

In addition to this change, your suggested change to reduce the search scope in RoadPathFinder::FindPath() to the destination building's flag could also be applied.

[Flamefire]

My current hypothesis is that it comes from GamePlayer::FindClientForCoin(), which subtracts the cost value (length) from another heuristic.

Even there we just have a comparison against other buildings and the 500 points offset applies to all of them so it shouldn't change their relative order.
I'm curious where that "offset" really does matter. I'll check once I got some time as that should "stabilize" replays. I suspect some under/overflow issue.

I believe keeping the GetPunishmentPoints() method unchanged could be misleading for developers. It implies that the penalty applies only to roads without carriers, whereas it also adds a penalty for harbors and has the issue with the final segments when used incorrectly.

The "used incorrectly" part should not happen and could rather be asserted than checked. As for the "misleading" part you are right although we could just accept and document it as I'm not sure it is worth the cost of 2 additional checks and extra parameter.
Alternatively: We already check for a path over a building:

s25client/libs/s25main/pathfinding/RoadPathFinder.cpp

Lines 204 to 210 in da659fc

	if(dir == Direction::NorthWest && neighbour != &goal)
	{
	// Flags and harbors are allowed
	const GO_Type got = neighbour->GetGOT();
	if(got != GO_Type::Flag && got != GO_Type::NobHarborbuilding)
	continue;
	}

We could add skipping the additional costs there but that makes that implementation less general in case the addCosts passed to it was changed at some point, so not great either.

So I'd rather add assert(!isBuildingEntry(dir) || isHarbor(dir)) to the "no-carrierscode path and document that the technically wrong penalty for entering a harbor is acceptable to avoid an extra check with pointer dereferencing. And/or name the constantPATHFINDING_PENALTY_NO_CARRIER_OR_SHIPPING`

However we rarely run into this codepath and access the "isHarbor" in the pathfinding right before the costs. So this check is likely cheap in practice. Hence we could do it cleanly:

GetPunishmentPoints(dir, goal)
  points = |wares(dir)| * PATHFINDING_PENALTY_WARES
  if(humanCarrier in dir)
    if(busy(humanCarrier) and not donkey in dir)
      points += PATHFINDING_PENALTY_CARRIER_BUSY
  else if(no carriers in dir)
    if(isBuildingEntry(dir))
      assert(isHarbor(dir))
      points += PATHFINDING_PENALTY_START_SHIPPING
  else
      points += PATHFINDING_PENALTY_NO_CARRIER
  return points

We can use the (non-local) fact that roads always go from flag to building so the building is route->F2 which avoids another access. It still is routes[dir]->GetGOT(), i.e. 2 accesses.
We need to measure if this makes a difference and if it is really negligible we can do this "cleanest" solution.

What do you think?

[Flamefire]

I'm curious where that "offset" really does matter. I'll check once I got some time as that should "stabilize" replays. I suspect some under/overflow issue.

I found a difference:
In GF 3631 some boards can go to one of 2 building sites:

ID     Points Distance  Estimate Len          Score
7150 9810    14         9803      725/225  9448/9698
7349 9780    10         9775      63/563    9499/9749

Columns are Game object ID, Distribution points (more=higher priority), cartesian distance from ware to goal, Estimate is Points - Distance / 2 as the score is calculated this way but using the Len as calculated by the pathfinder. Hence it is an upper bound to the final score. First value for Len an Score are with adding the final 500.

The old code and the one correcting the final "distance" yields the 2nd building while the new code without correction yields the first. This is clearly wrong as with or without the correction the 2nd has a higher score.

TLDR: We also need to adjust the max check to check after deducing the 500 points:

The issue is caused by the path of the 2nd building getting restricted such that the final score will be higher than the current best score. I.e. Score = Points - Len/2 => Len = (Points - BestScore) * 2 -1 I.e. 163 or 663. But you check against the max before removing the 500 points penalty so it works for the case with adding 500 to the final value because it just so happens that the actual length would be exactly 500 points higher. I.e. in one case we check 563 against 663 and in the non-corrected case 563 against 163 which fails although it should not and it doesn't find a path to the 2nd building.

[wichern]

Hi @Flamefire ,

I don't completely understand what you mean by the code correcting the final distance.
Is this the one in this pull request?

I made a check with the original implementation (master) where I only changed the penalty to 501. The test failed at GF 10441. When changed to 499 the test failed at GF 18537.

[wichern]

I suspect that the calculation in GamePlayer::FindClientForWare() has to be changed.

As you wrote, the formula is like this:
score = points - (path_costs / 2)

When points are the same for two buildings, the one with the shorter route will be choosen. But when they are unequal, the path costs will have an increasing impact depending on how far the possible destinations are.
e.g. when the path costs are low, the algorithm will likely prefer the building with more points. If the path costs are high, it will more likely prefer the one that has less path costs and "ignore" the points.

I think it would be better to normalize the path costs first and then divide the points by them. Then, different costs values with not change the result of this algorithm. However, this would require to search all pathes and not limit by max length.

$$costs_{normalized} = \frac{costs- min(C)}{max(C) - min(C)}$$
$$score = \frac{points}{1 + costs_{normalized}}$$
$C$ being the set of path costs to all possible clients.

[wichern]

This exceeds the scope of this pull request, but it may be worth doing a breadth-first-search from the ware to find the N closest possible clients and then decide. That could reduce the search effort caused by repeatedly calling A*. What do you think?

[Flamefire]

I don't completely understand what you mean by the code correcting the final distance.
Is this the one in this pull request?

Yes. I tried to remove if(reachingDestViaAttachedFlag) *length += 500; but didn't fix if((cost + add_cost) > max) being before add_cost -= 500; which led to the described behavior. First I thought this was already wrong as-is but it wasn't.

However with the fix it worked. I made a PR to your branch to discuss the changes and some comments on what I tried.

it will more likely prefer the one that has less path costs and "ignore" the points.

Might be fine as the points get reduced with arriving/ordered wares so others get preferred for later orders. But yeah, it's a heuristic only...

I think it would be better to normalize the path costs first and then divide the points by them. Then, different costs values with not change the result of this algorithm. However, this would require to search all pathes and not limit by max length.

Maybe. But I think pathfinding is so slow that it is better to have a suboptimal result instead of doing the search more often. Especially as no-one will likely notice this slight change in practice. And the current heuristic isn't too bad:

1. For buildings with equal "needs" the shorter distance is preferred
2. For buildings in equal distance the bigger "needs" is preferred.

If either is much higher than the other it skews the results, yes. But at least it is cheap to calculate and so far "good enough", isn't it?

may be worth doing a breadth-first-search from the ware to find the N closest possible clients and then decide. That could reduce the search effort caused by repeatedly calling A*

Possible. I also though about using e.g. Dijkstra. But in the end it might not be so easy: For small N repeated A* could be faster already. Additionally we can avoid A* invocations by limiting to the previous best value. Could make this more effective by sorting the possible clients by Cartesian distance ascending.
And BFS & Dijkstra require extra storage too. For our A* we intrusively store state in the nodes for better performance/avoid allocations. That could be more expansive than the pruned A* especially as they likely check more of the road network than they need.
So my intuition is that it is not worth it, performance-wise and also the effort for implementing and maintaining another pathfinder that needs to match the current one. We could try that of course

[wichern]

Thanks for your PR. Your solution seems best to me. Replays are still compatible and the required fix is installed. 👍

[Flamefire]

Thank you for coming up with this fix and PR!