Ignore isolated fish resources for fisheries

[DevOpsOfChaos]

Summary

• remove fish resources from non-water and isolated one-node water during map resource setup
• keep connected/larger water with fish usable for fisheries
• keep fisher runtime checks simple by relying on normalized resource data
• move resource setup/normalization into MapLoader
• add regression coverage for isolated fish water versus connected water

Motivation

Fisheries could still produce fish from an isolated one-node water pond if that node had a fish resource. In the original behavior described in #1171, such isolated water should be exhausted without producing fish.

This change keeps larger/connected water usable, but removes fish resources from unusable water during map resource setup.

Implementation details

Resource setup is now handled by MapLoader::SetupResources(GameWorldBase&).

The setup step normalizes resources after map loading:

• converts mine resource types as before
• places/fixes water as before
• removes fish resources from:
  • non-water points
  • isolated one-node water points

The fisher runtime path remains simple and only checks whether fish resources exist.

The fish cleanup iterates row-by-row and uses a row-local shortcut: if the previous point in the same row was kept as fish-on-water, the current water fish point does not need the full neighbor-water scan. This shortcut is not carried across row boundaries.

Validation

• Built Test_integration locally with Visual Studio 2022 / Debug
• Ran Test_integration.exe --run_test=BuildingSuite/FisherIgnoresIsolatedFishWater --report_level=short
  • Result: passed, 11 assertions
• Ran Test_integration.exe --run_test=BuildingSuite --report_level=short
  • Result: passed, 1738 assertions
• Ran git diff --check upstream/master...HEAD

Fixes #1171

[DevOpsOfChaos]

Updated. I cleaned up the test as requested.

The extra nofFarmhand reference is now only kept where it is needed to call GetPointQuality, and the unnecessary unsigned casts are gone. I also fixed the small formatting/include-order cleanup before pushing.

Validation:

• Built Test_integration
• Ran BuildingSuite/FisherIgnoresIsolatedFishWater
• Ran BuildingSuite

[Flamefire]

Thinking about this again: Wouldn't it make more sense to "fix" the map during loading?
I.e. remove fish resources in <=1-water places. This would simplify the runtime check to "is there fish?" which then improves performance.
The terrain cannot change at runtime, can it? so we'd only need to do those checks once.

@Spikeone ?

[DevOpsOfChaos]

Thinking about this again: Wouldn't it make more sense to "fix" the map during loading? I.e. remove fish resources in <=1-water places. This would simplify the runtime check to "is there fish?" which then improves performance. The terrain cannot change at runtime, can it? so we'd only need to do those checks once.

@Spikeone ?

I reworked this in the direction you suggested.

The isolated/non-water fish cleanup now happens once in GameWorld::SetupResources(), so the fisher runtime path can stay simple and only check for fish resources again.

Connected water with fish is unchanged; isolated one-node water and non-water fish resources are normalized away during world resource setup.

Validation:

• built Test_integration
• ran BuildingSuite/FisherIgnoresIsolatedFishWater
• ran BuildingSuite
• git diff --check upstream/master...HEAD

[Flamefire]

Please no force-pushes during a review. If necessary a revert works too. This way I have to check more changes.
Doesn't matter much for this small PR though.

With the added method in GameWorld: I think GameWorld::SetupResources() belongs more to MapLoader. Because that is the only context where it makes sense to call it, doesn't it?
So if I'm not missing anything please move those methods. SetupResources can be a public method, and the others can be private.

[DevOpsOfChaos]

Please no force-pushes during a review. If necessary a revert works too. This way I have to check more changes. Doesn't matter much for this small PR though.

With the added method in GameWorld: I think GameWorld::SetupResources() belongs more to MapLoader. Because that is the only context where it makes sense to call it, doesn't it? So if I'm not missing anything please move those methods. SetupResources can be a public method, and the others can be private.

Sorry about the force-push. I’ll avoid force-pushing this PR from here on and use normal follow-up commits instead.

[DevOpsOfChaos]

I moved the resource setup logic from GameWorld into MapLoader as suggested. MapLoader::SetupResources(GameWorldBase&) now owns the setup step, with the resource-normalization helpers kept private there.

The fish cleanup behavior is unchanged: fish on non-water and isolated one-node water is removed, while connected/larger water keeps fish. The row-local optimization for consecutive fish water is still in place.

Validation:

• built Test_integration
• ran BuildingSuite/FisherIgnoresIsolatedFishWater: 11 assertions passed
• ran BuildingSuite: 1738 assertions passed
• git diff --check upstream/master...HEAD

[Flow86]

pipeline is failing - did this break exiting replays/savegames?

[Flamefire]

pipeline is failing - did this break exiting replays/savegames?

Some, yes: For replays starting at zero, i.e. with a map as the base it will now remove some fish so fishers may not go out or somewhere else instead.

Double-checking and will update the replays in this branch if required. Maybe in a way that won't break them in similar ways if possible

[Flamefire]

Ok, done. The async in those replays is avoided now, I also changed the test to include a replay started from a savegame to cover the 2 different cases that are exposed here: Starting a replay that was started from a save game would have worked

[DevOpsOfChaos]

Pushed a small follow-up fix for the CI warning.

The Linux clang build treated the intentionally unused named parameter in the autoplay MockGameState::SystemChat override as an error, and clang-tidy surfaced the same diagnostic. The follow-up commit only removes the unused parameter name and keeps the override signature unchanged.

[Flamefire]

And hopefully the last one: The Boost version we use on Jenkins doesn't handle output of std::unique_ptr yet