AI does not build fisheries (again)

[one-three-three-seven]

Seems like this old bug is back: https://github.com/Return-To-The-Roots/s25client/issues/578

Tested on the map "X - Das letzte Tor".

With version 0.8.2: AI builds lots of fisheries. With the current version: AI does not build a single fishery.

Tested with "Inexhaustible fish" enabled and disabled, makes no difference.

[RM87]

Might that have something to do with it? Both large numbers(positive and negative) do grow away from zero every 400GF)

[Flamefire]

I just checked that a bit: Functions involved are FindBestPositionDiminishingResource and CalcResourceValue

They seem to be calculating wrong values, i.e. there shouldn't be negative values. Hidden in FindBestPositionDiminishingResource there seems to be a function which updates the resource map in the given radius. To solve this (and greatly enhance the AI) one would need to extract that into smallish, testable functions and test them thoroughly. The code is so complex and confusing that I'm unable to tell if it does the right thing (it obviously doesn't judging from the results)

So a good, black-box testcase would be: Call the UpdateResourceMapInRadius function (extracted from above) for different points and radius values without changing the underlying data. The value per point should be equal no matter where the update was started from (and above zero). With the current implementation one could get the expected value per point with CalcResourceValue(const MapPoint pt, AIResource res)

However the problem seems to be much bigger: The resource maps seem to have been abused at some point so there is a mixture of functions (potentially) reading and writing to it with inconsistent value calculations. So this needs to be checked and refactored first so that there is only 1 true, valid and tested usage.

@jhkl who originally wrote the code is gone AFAIK so its gonna be hard...

Update: #1355 introduced f9972e789abe04afbfc8493a5147214f6a1d8280 which will make this whole thing possible to test and fix. Gonna tackle that soonish

[desofity]

So as I understood correctly AIResourceMap's is used to find best place for the building to get desired resource. In case of Fish it was in my opinion implemented wrong: in debug window numbers were only on the water. That's way even if the algorithm confirmed some place is suited for fishery, AI still can't build, because it is a water.

So in void AIResourceMap::init() I switched code

if(res == AIResource::Fish)
{
    isValid =
      aii.gwb.IsOfTerrain(pt, [](const TerrainDesc& desc) { return desc.kind == TerrainKind::Water; });
}

to

if(res == AIResource::Fish)
{
    isValid = aii.gwb.IsOfTerrain(pt, [](const TerrainDesc& desc) { return desc.Is(ETerrain::Buildable); });
}

and ResourceMap for fish now seems to be much better, AI even started to build fisheries!

But, as @Flamefire mentioned there is still a bug with inconsistent value calculations, which results in negative or large positive numbers on ResourceMap. Although I noticed, this values adjusts to normal at some moments and then starting go negative/large again.

Also I think resource rating should decrease if there is an existing fishery nearby (same logic is done with Wood and Woodcutters for example).

When I figure out what causes this inconsistency I'll make a PR.

[r-or]

Came to the same conclusion. I "fixed" it in the same way and didnt't check the github known issues :roll_eyes: For your point about resource rating to decrease if there's a fishery around, this is handled in another place, in findBestPosition.

Anyway I agree the whole resource map calculation is buggy, the building quality is wrong as well, and I believe for wood also.