Improve settler location AI

[ajhalme]

Basically just salvaging some ideas from #430 to improve the SettlerLocationAI with tile improvement metrics.

[ajhalme]

The more I read the SettlerLocationAI code, the less it seems like #430 has to add here.

Only thing that isn't really here is the potential food/production yield from a tile, but that's influenced by game rules on terrain improvement and player characteristics, etc. - an expensive calculation for not that much gain.

As such, I'm reducing this mainly to adding scaled contributions from the BFC outer ring to the existing tile scoring logic, as called for in #802.

Something like a quadratic penalty from distance could be an improvement over the current logic.

I believe this now closes #802. I don't think #430 has anything more to add either.

[stavrosfa]

Thanks, I only have one comment, as I wasn't really involved in any of this, so I can't really provide any usefull insight.

[stavrosfa]

Hmm, I have some reservations regarding this.

The BFC is not necessarily up to 2 tiles, as we have made this configurable in the rules

OpenCiv3/C7Engine/C7GameData/Rules.cs

Line 18 in 775869f

public int MaxRankOfWorkableTiles;

So I think it would be preferable if we added another method like

OpenCiv3/C7Engine/C7GameData/Tile.cs

Line 693 in 775869f

public List<Tile> GetTilesWithinRankDistance(int rank) {

excluding anything but the rank we want, to get the tiles

Unless this method is doing something extra which I am not immediatly getting.

[ajhalme]

Replaced GetBFCOuterRing(..) with GetTilesWithinRankDistance(..). Made the distance-based score adjustment linear so tiles up to maximum workable rank have stepwise diminishing influence over the target tile, with tiles beyond the max rank contributing nothing.

Turned out to be a bit of a hassle, as the implementation of the GetTilesWithinRankDistance makes some assumptions about the tile init that the existing test setup doesn't cover. I patched the tests with some extra setup to make the function work.

Also spotted an opportunity to do a bit of memoization to speed up the calculation: once scored, a tile's score can easily be reused when scoring neighboring tiles.

[stavrosfa]

It looks good to me. I just have a question, feel free to merge

[stavrosfa]

Why the null checks here? Did you come by an instance where any of these were null? If so I think we might have a bigger problem

[ajhalme]

I added the EngineStorage part in, as that was how I saw MaxRankOfWorkableTiles being used. Using this global state is problematic for a a number of reasons, but the null stuff here is just so the test suite can run the function.

There's an easy fix, though, as the Player object carries the game's rules. I expanded the test suite to generate the missing context and the tests are now happy. There's no longer any dependency on EngineStorage.

Indeed, to make testing of all kinds easier across the codebase, I feel that the use of EngineStorage should be removed where it isn't absolutely essential.

[stavrosfa]

Yeah, I saw that the Player carries the rules the other day, this seems super weird to me, but anyway.

Regarding the EngineStorage stuff, we could theoretically do the following:

GameData gameData = new GameData();
EngineStorage.InitializeGameDataForTests(gameData); <- this is an existing method in EngineStorage (and is I guess just a fancy way of doing EngineStorage.gameData = gameData; )

add the rules like you 've done here

I don't know what happens after a test has finished, I haven't really thought about it untill now, but at the very end we could also do this for some cleanup

EngineStorage.gameData = null;

That's all in case we want to keep the previous format, though I don't know how strongly you feel against EngineStorage.
If you want to elaborate on that, I 'd love to hear your thoughts and also how it can be improved.

[ajhalme]

Hmm, I think it's reasonable to have the rules in the Player object. The Game object is of course the natural home for the rules, but if the player is the main argument passed around to both engine functions and map/presentation related things, having the rules there is handy. Just like here in the test case. The player could know what game is being played, and then make use of game->rules, but the current setup is fine. I can even imagine player-specific rules or behaviours, where rules really are more naturally a property of the player.

I agree that something like what you are suggesting could work for test harnesses, but I would still refrain from using the EngineStorage global state as much as possible. It's much nicer to have static functions where all the inputs are defined in the arguments.

That said, games are one of those software domains where relying on global state sometimes is the least bad option available. I'm a big fan of C#'s interfaces and various dependency injection patterns, etc., but we make do with what's available.