DefaultAI::calculate_strength assert fails

[Benedikt Straub]

In latest trunk (bzr8445/Debug on Ubuntu 17.04), I very often get this crash:

widelands: /home/benedikt/widelands-debug/frisians/src/ai/defaultai_warfare.cc:917: int32_t DefaultAI::calculate_strength(const std::vector<Widelands::Soldier*>&): Assertion `final <= 25000 * soldiers.size()' failed.
Abgebrochen (Speicherabzug geschrieben)

In the attached savegame, a crash is almost sure to happen within ten minutes.

[Benedikt Straub (nordfriese)]

I forgot – I played as frisians here, using the code from trunk, but --datadir=frisians/data.

[TiborB (tiborb95)]

Obviously frisian soldiers has different military numbers and the magic number 25000 is exceeded. Either it will be increased, or soldiers military numbers modified to be in line with other tribes...

[Benedikt Straub (nordfriese)]

A lot of time and effort was made to balance Frisian soldier stats, so I´m not too keen to change them again.
Also, I noticed in the strength calculation that a defence increase per level of 8% is assumed – Frisians have 15% per level.
A level 10 frisian soldier with maximum health has a score of 31905.59441 (with the wrongly assumed defence 26322.11538).
So, the assert will need to be changed to a maximum of >=32000, though I don´t understand this check is necessary at all…

[TiborB (tiborb95)]

OK, no need to change frisian stats if you think that they are not unfair.
The number is only a sanity check to make sure that algorithm does not return nonsense. Feel free to increase it to 32000, I have no problem with it. I can do it myself if you want...

[Benedikt Straub (nordfriese)]

No, I´ll just change it to 32000 and upload it to the frisians branch. This bug only affects the Frisians, so there´s no need for a new branch here.

[TiborB (tiborb95)]

good idea....

[Benedikt Straub (nordfriese)]

I changed the number to 32000 and also fixed the error in defence calculation, so the increase per level is now fetched by function call like for evade, attack and health. The change is uploaded to the frisians branch.

[TiborB (tiborb95)]

looks good...

[GunChleoc (gunchleoc)]

Can you add your changes to the defence calculation to the branch that I just attached?

I do not like just increasing the number, because the next time someone adds a new tribe with even bigger values, this will break again. So, I am grabbing the actual values off the SolderDescr.

The AI also has an assumption somewhere that each tribe has exactly 1 well type - we should try to get rid of all these assumptions to make the AI more flexible. Tibor, can you just keep this in the back of your head whenever you work on the AI, so that we won't add more of those assumption? I'll try to remember it during code reviews too.

[TiborB (tiborb95)]

AI has a lot of such presumptions - one ranger, one lumberjack, one port, one shipyard, one quary, one well and so on...

All these are a separate categories with own code path.

I am bit afraid of new special buildings, because next time it can be well which also pollinates clover and sentry that grows mushrooms.

I dont mind adding and tweaking "normal" productionsites (pure producers and pure supporters I mean) of course

[GunChleoc (gunchleoc)]

Maybe we should reduce those restrictions to restrictions that are also in TribeDescr, if possible? for example, TribeDescr has a function port() which will give the DescriptionIndex for the tribe's Port building, so this is clearly unique in the engine.

[TiborB (tiborb95)]

The actual problem is not these asserts, but other AI code that could not cope with multiple types of ports or other of these buildings. Even the logic "pick random port (type) from available ports" has to be coded somewhere. Not mentioning situation when the ports would have differing features....

[GunChleoc (gunchleoc)]

By all means, keep the asserts in where you need them to make sure that the Ai won't break.

Maybe we just need some documentation somewhere, to know which AI hint can only be present for 1 mapobject per tribe?

[TiborB (tiborb95)]

Can be, but this still does not cover all possible changes to buildings that can backfire.

[GunChleoc (gunchleoc)]

Fixed in build20-rc1