Settler re-picks a destination when its chosen spot becomes unreachable (#213)

[Billytifft]

Summary

#213 — Settler AI stuck forever when destination is unreachable. The
original crash ("Could not get next part of path" exception) was already gone —
TryToMoveAlongPath repaths and fails gracefully with Result.Error. But the
settler was still effectively stuck: candidate selection re-picked the
unreachable destination (e.g. a rival unit parked on it), so the settler
re-evaluated the same impossible task every turn and never built.

Instead of blanket-rejecting foreign-occupied tiles at selection time (the
blocker may move before the settler arrives), the settler now only gives up a
tile when it actually tries to reach it and fails: on Result.Error it marks
that tile unreachable for the current AI pass, picks a new destination, and
repaths; if nothing is left it degrades to JOIN_CITY (disband) rather than
looping. The exclusion is scoped to the current SettlerAIData, so a tile
whose blocker moves away is eligible again later.

Changes

• C7Engine/AI/UnitAI/SettlerAI.cs — on a failed move, exclude the unreachable
  destination and re-pick (FindNewDestination); fall back to JOIN_CITY if
  nothing is left (If Settler AI can't complete the next step in its path, it gives up and stays where it is forever #213)
• C7Engine/AI/UnitAI/SettlerLocationAI.cs — selection no longer rejects
  foreign-occupied tiles; optional excludedTiles set threaded through
  candidate scoring (If Settler AI can't complete the next step in its path, it gives up and stays where it is forever #213)
• C7Engine/C7GameData/AIData/SettlerAIData.cs — per-AI-pass set of
  unreachable destinations (If Settler AI can't complete the next step in its path, it gives up and stays where it is forever #213)
• EngineTests/AI/UnitAI/SettlerDestinationBlockedTest.cs — 4 tests: selection
  stays unrestricted but honors an exclusion set; retargets to a reachable
  alternative; degrades to JOIN_CITY when none remains; a failed move still
  fails gracefully (If Settler AI can't complete the next step in its path, it gives up and stays where it is forever #213)

Verification

• dotnet build — 0 errors
• dotnet test — 78 passed, 0 failed
• dotnet format ... whitespace --verify-no-changes — clean

[stavrosfa]

It seems to me that the description of the PR does not match the changes.

Something like "Don't send settlers on tiles that are currently occupied by foreign units" seems more accurate to me.

There is nothing that checks if the settler goes through a city. For example, what about cities that are empty? As you state in the description of this PR, this is already adressed.

Also, having the algorithm reject outright tiles that are currently occupied (a unit passing through), but then the settler arrives in 5 turns there, seems like a bad choice.
It makes more sense to me to reject these tiles, for example when the settler is right next to them. If it takes 5 turns to get there, and the tile is still occupied by a random fortified unit or whatever, then we can exclude them and repath accordingly.

I also want to point out (something very very obvious, but nevertheless) that there is a difference between an occupied tile and an owned tile.

This PR could probably serve as a temp fix for the bug with the settlers, but it should be accompanied by a TODO to make this better in the future.

[Billytifft]

The PR name came from the related issues. I can update it.

[Billytifft]

Comments addressed, including the PR title.

[ajhalme]

having the algorithm reject outright tiles that are currently occupied (a unit passing through), but then the settler arrives in 5 turns there, seems like a bad choice.
It makes more sense to me to reject these tiles, for example when the settler is right next to them. If it takes 5 turns to get there, and the tile is still occupied by a random fortified unit or whatever, then we can exclude them and repath accordingly.

I agree with this approach. In general, what is here proposed to be categorically rejected isn't an illegal approach, or even a bad one. The AI already sends Settlers out with an escort that can take out the odd Warrior that blocks a narrow peninsula, say.

In any case, I'd approach this via target tile scoring adjustment that accounts for nearby enemy presence or existence of (nearby) owned tiles. Even the option of declaring a war could ultimately feature in the scoring.

[stavrosfa]

To add to that, we should also perhaps think if we want all the AI to be able to know about nearby enemy presence.

We might want to have the easiest AI (chieftain) to not be able to know about an inactive (from their perspective) tile with a unit on it. And at the same time, a Sid difficulty AI to see everything.

So yeah, hardcoding this here might not be the best move.

We also already do this, in a different context humans/AI in general, where if you send a unit to go to a (either fog of war or inactive) tile with the go-to mechanic, it will accept it even if on that tile there is a foreign unit, and that's by design. But the AI will never do this, since it knows, even if it can't "see" the tile, that the move is illegal.
It's the same with, for example, sea tiles that are unexplored yet. We allow the human to go in any unexplored tile too. Check out ResolveIntent in MapUnit.cs for it.

This is true to the original game too.

[Billytifft]

Reworked the fix.

A foreign unit on a tile no longer rejects it. The unit may move away before the settler arrives, so selection keeps that tile eligible.

The settler gives up a tile only when it tries to reach it and fails. TryToMoveAlongPath returns Result.Error, SettlerAI marks the tile unreachable for the current AI pass, picks a new destination, and repaths. If no tile is left, it falls back to JOIN_CITY instead of looping forever.

The exclusion list lives in SettlerAIData. A new AI pass clears it, so a tile whose blocker moves away is eligible again.

I added a TODO on FindNewDestination for the long-term fix: path-checking at selection time.

[ajhalme]

The settler AI stuff looks fine, and seems to improve on the current infinitely stuck behaviour.

--

I'd drop all the material relating to #371. There's some duplication in the tests in RivalCityRoutingTest.cs, and I don't think the treatment of combat-time routing is quite correct either. It looks like the test is trying to nail down behaviour relating to the case where AI units should route around enemy cities on their way to a city deeper in the enemy empire. I mean I guess we can add tests for that, but I feel like the larger scale AI behaviour should be affirmed first.

This patch doesn't contain any fixes for #371, which may be correct, but what we really want is a fix for players not respecting right of passage. I'd look into to that as a target, in a separate PR or issue.

In general, no point in conflating fixes / test coverage patches for two issues that are only superficially related.

[ajhalme]

Thanks, this is much more compact and to the point now - good stuff!