[godot4] [AI] Fix Issue 94 - Improve SettlerLocationAI

[benskywalker]

This PR should solve issue #94. I have added the BFC tile logic for AI settlers, and it is working.

I left some log statements in we'll probably want to take out once this is ready to merge.

I also added the structure for calculating the irrigation and mining bonus for terrain types. The only issue is it looks like biq.cs is not pulling that information in yet. That's not really my area - @QuintillusCFC , that data would be in the biq file somewhere, right? Terr.cs does have a section for these bonuses, but it isn't getting input yet.

I'm just opening the PR so we can see where we're at on this

[benskywalker]

Just pushed out some more improvements. We now have a way to calculate whether a city is coastal, which does apply to unit production (lake cities can not build galleys). I've also added the logic for land isthmuses, which is working for the purposes of calculating lakes (small lakes separated from the ocean by a non-tile isthmus do not count as part of the ocean anymore).

[pcen]

I was reading the IsLake implementation and it got me thinking how we deal with Tiles throughout the entire code base. I was going to suggest that searchedWater be a HashSet but I realised, as you've implemented it, that there's probably no difference in performance when there's only 21 tiles in the list/set. Since we can determine the index of a Tile given it's x,y coords, we could uniquely identify tiles by their index, and use bitsets in a lot of places. Especially for TileKnowledge, it would save a lot of memory allocations to use bitsets instead of HashSets of tiles

[benskywalker]

I'm sorry, I deleted my previous comment because I was definitely commenting out of my ignorance about bitsets. That sounds like a very good idea for efficiency, and it would save a lot of unnecessary references for simple stuff. Because of the memory saved, it might be feasible to calculate things like IsLake for all tiles at the start of the game, and then it could just be a single bit.

[QuintillusCFC]

I believe there are a couple logical errors such as in the isLake method, and not utilizing the outerRing or mining/irrigation improvements in the AI evaluation. Also noted a few readability suggestions and a few areas that solve the problems elegantly.

All in all this will be a nice improvement in the AI's ability to decide where to settle once merged, and some of the calculations like the ring and isCoastal on Tile.cs will also be useful for future improvements.

[QuintillusCFC]

Minor, but the tradeBonus should also be considered. It's disabled by default for volcanoes, and can be disabled or increased in mods as well. The commerce yield weight in the tile score is 2 which seems largely appropriate.

[QuintillusCFC]

Shouldn't the improvementScore be included in this? I would argue that it should have a lesser weight than the yieldScore, since in most cases the improvements won't exist when the tile is being settled, but greater than zero weight.

[QuintillusCFC]

In this block, improvement score is included, but isn't it still looping over the same tiles (t.neighbors.Values in both cases)?

[QuintillusCFC]

Definitely like that conceptually, and it will be useful in scenarios which allow settling on volcanoes and mountains, or which set different bonuses for tundra/plains/etc.

One gotcha that should be noted in a comment for now is that forests and jungles provide defensive bonuses, but are removed when a city is built on them - thus the bonus should not factor in here. We don't yet have a place where we capture that "disappears when city is built on them" attribute as it isn't moddable in Civ3 and is just an intrinsic property of the terrain.

[QuintillusCFC]

I like the idea of having checks for both freshwater sources and coasts. The initial bonus was to encourage the AI to settle on coasts instead of one tile off the coasts, which it does to a baffling degree in Civ3, preventing it from both building ships and as importantly, harbors which boost the coastal food yield.

I would advise leaving the water bonus in for now as it's likely to encourage the coastal pattern, and if it also encourages settling by lakes for now, that's not a bad thing as it guarantees fresh water (once irrigation is implemented). But with a comment about making this more nuanced in the future.

[QuintillusCFC]

Update: This can just use the new Coastal() method, right? And then have a comment about eventually checking for nearby freshwater (which IMO could be both less weighty and more complex, as it can be chained across cities so local fresh water sources aren't necessarily that important).

[QuintillusCFC]

Let's put this 20 magic number in a constant, maybe LAKE_THRESHOLD, to set it up for eventual moddability. One of the goals of C7 is to reduce the number of hard-coded limitations in Civ3, and while lake size isn't a huge point of consternation, it is ultimately hard-coded.

[QuintillusCFC]

Also minor, but this long block of if/else/foreach is a good example of why I recommend the style:

if (something) {
   //do something
}

rather than:

if (something)
{
    //do something
}

When there are a lot of checks like this, the extra lines from that leading { add up to making it harder to read the logic at a glance due to having to scroll more to see the whole picture, with each scroll adding a little bit of time and overhead re-focusing the eye on where the most-recently-read line is now. Not a big deal and I see we've been inconsistent about it, but that's the reason I eventually switched to that style from originally giving the opening curly brace its own line.

[QuintillusCFC]

One last style comment, I realized that i and contiguousWater are both counting the same thing - how many water tiles we have. They're just zero-indexed to account for the array and one-indexed because that's how humans think of counting things. We want to say, if we find a 21st water tile, it's not a lake.

I'd propose just having one counter variable, something like waterTileCount. Start it at 0 and increment it in the same place contiguousWater is incremented, and it will both grab the right tile from the array and read nicely. That way someone reading this code won't be trying to figure out the difference between two counters and why one is incremented early and one is incremented late - it'll be easier to verify that the logic works correctly.

[QuintillusCFC]

I'm pretty sure that this block needs to check the value of the contiguousWater variable (or what it eventually is named, see above). In the case that there are fewer than LAKE_THRESHOLD tiles examined, it's a lake, right? But if we don't already know that one of the other tiles is a lake, it won't pass the if check on line 149.

Maybe the change should be on line 149, checking if (lake || contiguousWater < LAKE_THRESHOLD)?

This method is also ripe for a test in C7GameDataTests. Not saying it needs to be added now, and we might not have enough convenience methods to make it easy to set up the needed data at this point, but the logic is just complex enough that it's a good example of where a test would be high-value.

[QuintillusCFC]

Nicely done. I doubt this would be called repeatedly during runtime, so I wouldn't go to a lot of extra effort to cache it, but it's never going to change and it would just be storing one variable, so the extra effort isn't a lot. Your call.

[QuintillusCFC]

I have mixed feelings on this solution to giving an invalid direction. Crashing the game is bad. We had a bug in early 2022 where a method would throw an uncaught exception if it got bad data, and it did sometimes, and the game would mysteriously crash. This doesn't look as likely to cause that issue but all it would take is someone passing NORTHEAST into this method because they are thinking of Civ4 tiles where NORTHEAST is diagonal.

It may, or may not, be better to instead log an error, but to return a value (a tuple of the same direction passed in, in both parts of the tuple?) rather than crash. Is the resulting bug in game logic worse than a crash? It varies, but IMO crashes are the quickest way to negative reviews and players quitting a game (or just never restarting it after the crash).

[QuintillusCFC]

I like this solution to the outer ring problem. Certainly more elegant than how I did it in my editor, in a language that (at the time) didn't have tuple support.

One minor suggestion for readability would be having each of the tiles on its own line here, e.g.:

(TileDirection direction1, TileDirection direction2) [] outerRingDirections = {
    (TileDirection.NORTHWEST, TileDirection.NORTH),
    (TileDirection.NORTHWEST, TileDirection.NORTHWEST)
    ...
}

That would make it easier to verify that the expected 12 are there. Which they are... but I did validate it just to make sure there weren't any erroneous tiles in the ring.

[Sean-Brown]

alternatively it could be a nullable boolean, i.e.

public bool? isLake = null;

public bool IsLake()
{
   if (isLake.HasValue) return isLake.Value;
   ...
}

[Sean-Brown]

Also alternatively it can be a computed property of this class

public bool IsLake
{
   get
   {
      if (!IsWater()) return false;
      ...
   }
}