Pathfinding: reject unconnected buildings / construction sites in specific scenarios

[Noseey]

Related Issue:
Closes #1785

Changes Summary
This will not implement the algorithm mentioned in #164, but is an attempt to fix the immediate issues in #1785 by rejecting buildings that are not connected to anything / are isolated. This is implemented for the following cases:

• OrderConstructionMaterial
• FindWarehouseForJob
• FindClientForWare
• FindClientForCoin

[Flamefire]

Thanks for the attempt anyway.
I added 2 general notes on style in case you pursue this further. :)

[Noseey]

Thanks for the attempt anyway. I added 2 general notes on style in case you pursue this further. :)

I'm not giving up on the probem just yet. 😉 But the current solution still seems a bit too easy for my taste, let me know if I missed something substantial here. At least the large map from the issue seems to run fine now, only very minor stuttering upon posting a new flag.

[Flamefire]

2 issues I see:

• It breaks replays as the order and amount of checks, and hence RNG-calls change. See the failing CI
• Pathfinding is not symmetric in general. E.g. for wares the amount of waiting wares at a flag is taken into account which will be wrong if you swap the directions

An idea: I guess "unconnected" buildings/construction sites usually have simply no road from the building flag to anywhere. We could check for that (only required at the target) before running the full pathfinding.
There is also the idea (see TODO) to search only up to the buildings flag to shave off a (little) bit.

In any case this will break replays so it is hard to measure how much benefit this provides. Only empirically by running many longer replays on a large map ignoring the async, which provides at least an indicator.

[Noseey]

An idea: I guess "unconnected" buildings/construction sites usually have simply no road from the building flag to anywhere. We could check for that (only required at the target) before running the full pathfinding.

Yes, this goes in the direction of my first attempt visible in the first two commits. It resolved the issue with the map in the ticket, but I found it too specific as it fails to work as soon as some roads are attached to the construction sites. But it could be a last resort. So you would say that even with that solution of checking just the target flag, the replay compatibility is endangered? Could you point me to an example in the code where randomness is factored into the pathfinding in these scenarios?

[Flamefire]

So you would say that even with that solution of checking just the target flag, the replay compatibility is endangered? Could you point me to an example in the code where randomness is factored into the pathfinding in these scenarios?

Looks like i misremembered that. Only place I found is unrelated to road pathfinding:

s25client/libs/s25main/pathfinding/FreePathFinder.cpp

Lines 103 to 105 in e6489dc

	// Start at random dir (so different jobs may use different roads)
	const Direction startDir =
	randomRoute ? convertToDirection(gwb_.GetIdx(start) * gwb_.GetEvMgr().GetCurrentGF()) : Direction::West;

Given that I guess that checking the flag connections should not impact the replays.

it fails to work as soon as some roads are attached to the construction sites.

What situation would that be? Usually either the building is fully connected or not connected at all, isn't it?
And swapping start & goal just reverses the issue. Does that really help? And as mentioned for wares it can cause wrong results for the "time" estimates and then choose different warehouses to order wares where the roads are already full.

[Noseey]

And as mentioned for wares it can cause wrong results for the "time" estimates and then choose different warehouses to order wares where the roads are already full.

I think I know what you mean now, it is related to the AdditonalCosts::Carrier() in FindPath introducing a time-dependency to the pathfinding calculation, as the situation upon arrival on high cost road segments is dynamic and thus can be different when swapping start and goal? because the GetPunishmentPoints depends on the direction?

If that is the case, I guess the current solution is then out of question. There would be an intermediate solution by additionally calculating if a path exists from the goal to nearby warehouses (see the third and forth commit) before, which also resolves the problem with isolated buildings aswell (at least in my tests) but adds additional pathfinding overall, which is not really great.

What situation would that be? Usually either the building is fully connected or not connected at all, isn't it?

Yes, I thought it could be somehow integrated into the pathfinding itself without additional checks and have the benefit of covering also "non-connected networks", but given the restrictions it might really be best to just check the building flag itself, as it would still cover most user scenarios. I will update the PR.

Edit: I think also by only rejecting isolated buildings, there still is the timing dependency to the dynamic situation of high cost road segments, so I guess it could really break replay compatibility over longer periods of time when a high numbers of isolated buildings are involved. I would say your rightfully claimed it as an issue that takes places, no matter how this is fixed. Ignore this, since the call is blocking it shouldn't do anything.

[Flamefire]

I think I know what you mean now, it is related to the AdditonalCosts::Carrier() in FindPath because the GetPunishmentPoints depends on the direction?

Right: That is for making sure wares can go around a flag that has currently so many wares in the same direction that the other way is faster even if longer.

There would be an intermediate solution by additionally calculating if a path exists from the goal to nearby warehouses

We could only do that for the PathExists which doesn't care about costs/distances. But if we need costs or directions then there is no alternative.

What situation would that be? Usually either the building is fully connected or not connected at all, isn't it?

Yes, I thought it could be somehow integrated into the pathfinding itself without additional checks and have the benefit of covering also "non-connected networks",

When there is no path from A to B then there is also no path from B to A. So I don't really see a benefit of always swapping the direction. It only helps if the network of B has less nodes than that of A but you don't know that before, do you? So swapping might even make it worse.
That was my question: In which situations is the network from the warehouse smaller than the target buildings network? Is that much more common than the other way round?

best to just check the building flag itself, as it would still cover most user scenarios.

My intention is that unconnected buildings are usually e.g. military buildings where no gold or soldiers should go in/out and hence have no connection at all. And this is common enough to do additional checks before running the full pathfinding.

[Flamefire]

See inline comments.

Major point: Why not put the reachable check at the lowest level close to where the pathfinding is done? That explains the check better (closer to where it matters) and avoids having to catch all call sites.

[Flamefire]

Final round: One is about avoiding reads then not using the result the rest is just style/wording.

If I haven't missed anything (please check) you can apply them all via the PR UI (IIRC in the "Changes" tab add each to queue and accept in one go.

If you are happy with that mark the PR as ready so we can merge it.
Have you checked how much this change gains?

Thanks a lot!

[Noseey]

If you are happy with that mark the PR as ready so we can merge it. Have you checked how much this change gains?

Yes, I did check it against the large example map posted in the issue ticket to see if it resolves the seconds long freezes in the following scenarios:

1. when building material is stored in warehouses
2. when building material is created and dropped infront of the building
3. when a new road is connected

For 1) and 2) it resolved the freezes completely. For 3) it was decreased down to a fraction of a second, due to the still large road network.

[Flamefire]

Ok LGTM!
Mark as ready if this can be merged