Merge lp://staging/~widelands-dev/widelands/AI-fixes into lp://staging/widelands

From AI code point of view - The changes can be OK, it is hard to say just looking at the code. I presume that you made sure the magic numbers does not repeat - dont forget they are used also in other AI files...
I will do some training once it is approved and merged.
I did not review changes to LUA code..
So for my part it is OK

Found a typo in a comment.

The Lua code does not need reviewing because it will be removed anyway, but we should do a bit of testing.

I gave up on the scenario after connecting 20 farms or so. Multiplayer game ran fine for > 7 hours gametime.

So, this can go in once the branch has been cleaned up :)

@Tibor: of course I made sure the numbers are not used elsewhere. Shall I add more comments to explain my changes?

@GunChleoc: will fix the typo ofcourse. I never did any thing in the scenario but watching the AI's doing their job. If you do so you would see significant progress. And you can see that my changes work as desirred. But this was meant for Nordfriese in particular to see whether my changes would solve his issues in the scenario which led him to start working (and making good progress for his own scenario specific AI). However I already had the impression he might be a little bit sadistic with the player to connect all these farms. ;-)

@ both: there is still a line commented out (5710ff in defaultai.cc) I wanted to have a value here that depended on stocklevel vs economy target, but the Code didn't work. I got a crash. So I commented this out as my Skills are to bad for realizing the cause if you know why this would crash you could fix it or give me a hint.

I'll have a look.

Also, I got this:

../src/ai/defaultai.cc: In member function ‘bool DefaultAI::check_productionsites(uint32_t)’:
../src/ai/defaultai.cc:4474:95: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
       1000 < gametime && site.site->can_start_working() && get_stocklevel(*site.bo, gametime) >
                                                            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^
       (std::abs(management_data.get_military_number_at(168)) / 5)) {
       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../src/ai/defaultai.cc:4502:46: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
      gametime - site.bo->last_dismantle_time >
      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^
          (std::abs(management_data.get_military_number_at(169)) / 10 + 1) * 60 * 1000 &&
          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

I think I found the problem:

(bo.outputs.size() == 1) means that there is 1 entry at index 0.

(tribe_->get_ware_descr(bo.outputs.at(1)) would only be available if bo.outputs.size() > 1

Unlike Lua, C++ starts counting from 0. Actually, Lua is quite exceptional here, since starting from 0 is the usual way of doing arrays.

Tested it now with the 3rd frisian scenario.

The AI behaved quite well, just some nits:
– It builds mainly blockhouses and small milsites. IMHO it should build preferably bigger milsites near the enemy´s border, even if it´s unguarded.
– It´s still quite confused by the inability to build woodcutters, quarries and mines. It builds useless buildings like taverns (that can be solved by just forbidding everything) and it neglects important buildings like warehouses (are very important there because that´s where the script creates wares you cannot produce yourself).
– There is much free space as a consequence of not having foresters, but space-consuming buildings (reedyard etc) are cramped closely together; but that´s a more general AI-problem.

– I get some compiler warnings:
/home/benedikt/wl/AI-fixes/src/ai/defaultai.cc:2461: Use "TODO(username): <msg>".
/home/benedikt/wl/AI-fixes/src/ai/defaultai.cc:2463: Trailing whitespace at end of line
[1007/1455] Building CXX object src/ai/CMakeFiles/ai.dir/defaultai.cc.o
../src/ai/defaultai.cc: In member function ‘bool DefaultAI::check_productionsites(uint32_t)’:
../src/ai/defaultai.cc:4502:46: warning: comparison of integer expressions of different signedness: ‘uint32_t’ {aka ‘unsigned int’} and ‘int’ [-Wsign-compare]
      gametime - site.bo->last_dismantle_time >
      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^
          (std::abs(management_data.get_military_number_at(169)) / 10 + 1) * 60 * 1000 &&
          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

> But this was meant for Nordfriese in particular to see whether my changes would solve his issues in the scenario which led him to start working (and making good progress for his own scenario specific AI). However I already had the impression he might be a little bit sadistic with the player to connect all these farms. ;-)

Hey ;) The point of this objective is force the player to be almost idle until the enemy is standing right at his gates. And to force him to create a good road network under this pressure, which can be very important for the battles later.
Btw, my own AI is not scenario-specific – it´s a template for every(!) scenario that wants an AI specific to it

Continuous integration builds have changed state:

Travis build 4946. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/531247546.
Appveyor build 4727. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_AI_fixes-4727.

@nordfriese
about warehouses - they are in proportion to productionsites, I dont think AI can have any idea it needs more warehouses in this specific case...

@hessenfarmer

"@Tibor: of course I made sure the numbers are not used elsewhere. Shall I add more comments to explain my changes?"
No, I am fine, I also believe that AI training will settle down these changes. I intend to run 100 iterations as soon as the branch is in trunk..

 > The point of this objective is force the player to be almost idle until the enemy is standing right at his gates. And to force him to create a good road network under this pressure, which can be very important for the battles later.

I get what you're trying to do, but hooking up all these farms is pretty repetitive. Maybe you can come up with some more varied stuff to do while still having some time pressure?

@ Nordfriese:
fixed the codecheck warnings.
We should try to forbid useless buildings like the tavern and see what's the effect.
Regarding warehouses the problem was not the number but the distribution. as the script delivers only to one warehouse a time, this is putting the road network under some pressure. therefore I forced at least one second carrrier.
As the gametime evolves I made the experience, that the AI starts to build bigger milsites, however they are normally based on enemy force which is neglectable in this scenario. so the closer you get to an enemy the bigger the chance of big military buildings.

@ Nordfriese:
If you want to test this further let us know, if you are finished we could merge this.

I still can't get the Ai to build a second carrier producer.

Generally AI can decide:
- it does not need second carrier producer
- it needs it but gives it too low priority

So for further debugging you should know which case it is...

Continuous integration builds have changed state:

Travis build 4962. State: errored. Details: https://travis-ci.org/widelands/widelands/builds/531521221.
Appveyor build 4743. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_AI_fixes-4743.

Finally I got the AI to properly build second carrier recruiters depending on size of road network. The issue was that we need to fake a preciousness for this building (similar to barracks). Did some other changes as well to prevent the Ai from dismantling too many productionsites too often.

oops I missed to fix the typo which GunChleoc found will be done in final polish

I have one objection, see comment in code

I´m through with the code review, a couple of nits. Will do more testing soon…

@ Tibor:
oops this was a left over of some trials. I don't think we need this anymore will fix this tonight when I am back at my dev machine.
@ Nordfriese
Thanks for the review. Will fix these issues as well. However I replied to some comments below them.

How about we implement preciousness for workers, just like we do for wares? That way, we would not need an ugly hack and could play with how precious they are without having to recompile.

@GunChleoc:
I had the same thought but went the easy route for the moment being. Currently we only have 2 buildings recruiting "workers": the barracks and the second carrier recruiter. Both of them are limited for AI (barracks to 1, second carrier to 2 hardcoded) For these reasons I doubt if any effort in this way might be worth the pain.

Continuous integration builds have changed state:

Travis build 4978. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/532671043.
Appveyor build 4759. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_AI_fixes-4759.

Tested, and it performs well. Code LGTM.
Good job :)

Ok I will remove the fri03 stuff and merge it then. will be tonight though

removed the lua stuff should be good to go now

@bunnybot merge

@hessenfarmer Hard-coding stuff in the AI has caused us problems before, e.g. when the Frisians were first introduced. I am fine with merging this now, but can you open a new bug, please?

@GunChleoc:
I will do so, however the 2 buildings (barracks, second carrier) are so special, we would need a lot of code to come to the same results (1 barracks, 2 carrier). We also have a lot of other hardcoded stuff regarding these two. But basically the AI just needs to ensure it builds a barracks but not to early to not run in a deadlock and not to late. more the less the same with second carrier. The problem with these buildings is that their demand is not steady, but has big spikes. By this we can't use the decision algorithms for normal buildings producing wares. And therefore just having a preciousness for the soldier and the second carrier would not do the trick. What I can think for is e.g. having them assigned a Normal Ai limit in lua instead of hardcoding the values as it is now.

@ GunChleoc:
I forgot to mention that I am pretty sure no matter what precautions we might take a new tribe would lead us to have some changes in Ai as well, as me and my crystal bowl can't foresee every issue that might arise. ;-)
In fact I was led to the current improvements only by the need of Nordfriese and his very special scenarios.

Of course, there will always be things that we haven't thought of. And having too many hard-coded things already does not count as an argument in favor of adding even more of them.

The additional code needed wouldn't be that much, it's just a matter of adding 1 more property to the worker descr objects + AI hints, which I could do easily.

For my understanding it is not only that but we should discuss this in the bug report.