Merge lp:~widelands-dev/widelands/bug-1746481 into lp:widelands

[TiborB]

AI now considers a building needed for 'basic economy' finished only if fully occupied.
I left one DEBUG log in the code for a tester (contains word DEBUG).
There is some short gap between a building gets occupied and AI recognizes it. And this can cause inconsistency when a game is saved just during such period. But should not be very frequent....
After testing is done, I will remove NOCOM lines.

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 3135. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/337068986.
Appveyor build 2942. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1746481-2942.

[TiborB (tiborb95)]

Codecheck complains about printfs, but these are only for temporary DEBUG logs and will be removed before merge...

[R M (weedfreak)]

Testing this the AI was set up with

1: Initializing in the basic economy mode, required buildings:
     97 / empire_tavern               - target 1
     95 / empire_winery               - target 1
     84 / empire_foresters_house      - target 2
    117 / empire_marblemine           - target 1
     83 / empire_lumberjacks_house    - target 2
     90 / empire_sawmill              - target 2
    106 / empire_farm                 - target 1
     89 / empire_stonemasons_house    - target 1
     92 / empire_bakery               - target 1
     94 / empire_vineyard             - target 1

  which it then ignored, it built
     empire_tavern               - 1
     empire_winery               - 1
     empire_foresters_house      - 7
     empire_marblemine           - 0
     empire_lumberjacks_house    - 6
     empire_sawmill              - 3
     empire_farm                 -1
     empire_stonemasons_house    - 2
     empire_bakery               - target 3
     empire_vineyard             - target 2

After an hour it started building training sites despite having no mines
or mills for wheat. Eventually it built a marble mine, but 5 minutes
later it had still not announced the basic economy established. Loading
from a save game did not work as I did not have a zipped save, will try
that next.

Checking the terminal there ws no debug message for the mine so perhaps
I was not patient enough.

[TiborB (tiborb95)]

Everytime a site is occupied for the first time you should see this DEBUG log: https://bazaar.launchpad.net/~widelands-dev/widelands/bug-1746481/view/head:/src/ai/defaultai.cc#L3706

The problem could had been printf() in my code, I just pushed new branch which uses log() to print these logs. If this still not print these DEBUG logs something must be messed up.

From your prescription, only problem you found was that it built a trainingsite before the basic economy was achieved. Yes, there is genetic algorithm involved there that under some circumstances allows trainingsites before basic economy. This is question of course, if this should be allowed...

[R M (weedfreak)]

The gap between the building being finished and being recognised seems
to increase with game time, the first few are almost instantaneous as
the worker reaches the buildings flag, later it occurs just after
occupation.

This run the vineyard was not built until after 8 foresters so there was
no shovel left and thus no worker but the message DEBUG: empire_vineyard
first time occupied appeared, incorrectly, after a few minutes.

Loading save games has not, so far produced any problems, but it may
still be a problem later in game when the delay gets greater.

[TiborB (tiborb95)]

INDEED, I see a problem (in the code) now. I will fix and let you know...

[R M (weedfreak)]

I am not sure what code problem you have found but further testing shows
no problem later in game with saves, the marble mine does not produce a
debug message after 20 minutes and when a basic economy building is
built but not occupied the AI starts building another of that building,
so that was why there were 3 sawmills, 2 stonemasons, 2 bakeries and 2
vineyards etc. most were started when the worker was on the way.

On 06/02/18 13:12, TiborB wrote:
> INDEED, I see a problem (in the code) now. I will fix and let you know...

[TiborB (tiborb95)]

I omit to incorporate can_start_working() function to the check altogether, it simply cannot work properly....

[TiborB (tiborb95)]

Please try once more..

[R M (weedfreak)]

The woodcutter first occupied debug message is printed twice, most basic
economy buildings are built one more than the basic setting. The marble
mine, which is built twice, does not give a debug message and no message
for basic economy reached is given. The marble mines were the last basic
economy buildings built and I ran the game for an hour afterwards.

To be certain the vineyard was occupied I prohibited foresters for 600
so there was at least one shovel in stock.

[TiborB (tiborb95)]

Weird, the code look straightforward in this regard... I will have to test it myself.

No save&load was involved I presume.....

[TiborB (tiborb95)]

I probably know what is going on. The problem is a gap when a building exists and is not occupied yet (scratched from the list)
AI when considering new buildings tests:
- it is on the list of (missing) basic buildings
- none is in construction

I have to rethink this

[GunChleoc (gunchleoc)]

You probably need 2 lists, one with the buildings to build, and one with the buildings built. The buildings built list can then be checked whether they're occupied and if so, be removed from the second list.

Or you can expand the first list with number of buildings built and only build new buildings if there is a difference. Then remove fro the list if all buildings are occupied.

Regarding printf, that function is insecure and should be replaced with sprintf in all cases - our custom-built log function will do that for you. You also accidentally added a long list of "#include <unistd.h>" to main.cc in the commit that fixed the printf.

[TiborB (tiborb95)]

Yes, single list will not do. And I have to pay attention to the game saving aspect as well.

I spent a lot of time with another branch - findpath_modification, and if this will be OK now, I can come back to this...

[GunChleoc (gunchleoc)]

Is this branch ready for review? We should get rid of the extra lines in src/main.cc in any case.

[TiborB (tiborb95)]

No, I have not touched it since the last discussion. I am still not sure how to approach this. We can cancel this merge proposal....

[GunChleoc (gunchleoc)]

OK, I have set it back to "Work in Progress"

=== modified file 'src/ai/ai_help_structs.h'
--- src/ai/ai_help_structs.h	2017-11-17 07:16:12 +0000
+++ src/ai/ai_help_structs.h	2018-02-06 19:41:40 +0000
@@ -436,6 +436,7 @@
 	uint32_t current_stats;
 
 	uint32_t basic_amount;  // basic amount for basic economy as defined in init.lua
+	bool never_occupied;  //this switches
 
 	std::vector<Widelands::DescriptionIndex> inputs;
 	std::vector<Widelands::DescriptionIndex> outputs;

=== modified file 'src/ai/defaultai.cc'
--- src/ai/defaultai.cc	2018-01-30 11:44:48 +0000
+++ src/ai/defaultai.cc	2018-02-06 19:41:40 +0000
@@ -3702,6 +3702,16 @@
 	// Get link to productionsite that should be checked
 	ProductionSiteObserver& site = productionsites.front();
 
+	if (site.bo->never_occupied && persistent_data->remaining_basic_buildings.count(site.bo->id) > 0 && site.site->can_start_working()) {
+		log ("DEBUG: %s first time occupied\n", site.bo->name); // NOCOM
+		if (persistent_data->remaining_basic_buildings[site.bo->id] > 1) {
+			--persistent_data->remaining_basic_buildings[site.bo->id];
+		} else {
+			persistent_data->remaining_basic_buildings.erase(site.bo->id);
+		}
+		site.bo->never_occupied = false;
+	}
+
 	// Inform if we are above ai type limit.
 	if (site.bo->total_count() > site.bo->cnt_limit_by_aimode) {
 		log("AI check_productionsites: Too many %s: %d, ai limit: %d\n", site.bo->name,
@@ -4182,6 +4192,16 @@
 	// Get link to productionsite that should be checked
 	ProductionSiteObserver& site = mines_.front();
 
+	if (site.bo->never_occupied && persistent_data->remaining_basic_buildings.count(site.bo->id) > 0 && site.site->can_start_working()) {
+		log ("DEBUG: %s first time occupied\n", site.bo->name); // NOCOM
+		if (persistent_data->remaining_basic_buildings[site.bo->id] > 1) {
+			--persistent_data->remaining_basic_buildings[site.bo->id];
+		} else {
+			persistent_data->remaining_basic_buildings.erase(site.bo->id);
+		}
+		site.bo->never_occupied = false;
+	}
+
 	const bool connected_to_wh = !site.site->get_economy()->warehouses().empty();
 
 	// First we dismantle mines that are marked as such, generally we wait till all wares all gone
@@ -5748,18 +5768,6 @@
 		++bo.cnt_built;
 		const uint32_t gametime = game().get_gametime();
 		bo.last_building_built = gametime;
-		// erase building from remaining_basic_buildings, unless we are loading a saved game
-		if (!found_on_load && persistent_data->remaining_basic_buildings.count(bo.id) > 0) {
-			if (persistent_data->remaining_basic_buildings[bo.id] > 1) {
-				--persistent_data->remaining_basic_buildings[bo.id];
-			} else {
-				persistent_data->remaining_basic_buildings.erase(bo.id);
-			}
-		}
-		// Remaining basic buildings map contain either no entry for the building, or the number is
-		// nonzero
-		assert(persistent_data->remaining_basic_buildings.count(bo.id) == 0 ||
-		       persistent_data->remaining_basic_buildings[bo.id] > 0);
 
 		if (bo.type == BuildingObserver::Type::kProductionsite) {
 			productionsites.push_back(ProductionSiteObserver());
@@ -5786,6 +5794,14 @@
 
 			for (uint32_t i = 0; i < bo.inputs.size(); ++i)
 				++wares.at(bo.inputs.at(i)).consumers;
+
+			if (!found_on_load) {
+				// If this is brand new building, setting as never occupied
+				bo.never_occupied = true;
+			} else {
+				// otherwise just find out
+				bo.never_occupied = !productionsites.back().site->can_start_working();
+			}
 		} else if (bo.type == BuildingObserver::Type::kMine) {
 			mines_.push_back(ProductionSiteObserver());
 			mines_.back().site = &dynamic_cast<ProductionSite&>(b);
@@ -5810,6 +5826,14 @@
 
 			set_inputs_to_zero(mines_.back());
 
+
+			// If this is brand new building, setting as never occupied
+			if (!found_on_load) {
+				bo.never_occupied = false;
+			} else {
+				// otherwise just find out
+				bo.never_occupied = productionsites.back().site->can_start_working();
+			}
 		} else if (bo.type == BuildingObserver::Type::kMilitarysite) {
 			militarysites.push_back(MilitarySiteObserver());
 			militarysites.back().site = &dynamic_cast<MilitarySite&>(b);
@@ -5829,6 +5853,13 @@
 			trainingsites.push_back(TrainingSiteObserver());
 			trainingsites.back().site = &dynamic_cast<TrainingSite&>(b);
 			trainingsites.back().bo = &bo;
+			// If this is brand new building, setting as never occupied
+			if (!found_on_load) {
+				bo.never_occupied = false;
+			} else {
+				// otherwise just find out
+				bo.never_occupied = productionsites.back().site->can_start_working();
+			}
 
 		} else if (bo.type == BuildingObserver::Type::kWarehouse) {
 			++numof_warehouses_;

=== modified file 'src/ai/defaultai_warfare.cc'
--- src/ai/defaultai_warfare.cc	2017-11-13 09:10:04 +0000
+++ src/ai/defaultai_warfare.cc	2018-02-06 19:41:40 +0000
@@ -575,6 +575,17 @@
 	TrainingSite* ts = trainingsites.front().site;
 	TrainingSiteObserver& tso = trainingsites.front();
 
+	// In case this trainingsite is among basic buildings
+	if (tso.bo->never_occupied && persistent_data->remaining_basic_buildings.count(tso.bo->id) > 0 && tso.site->can_start_working()) {
+		printf ("DEBUG: %s first time occupied - erasing from basic economy list\n", tso.bo->name); //NOCOM
+		if (persistent_data->remaining_basic_buildings[tso.bo->id] > 1) {
+			--persistent_data->remaining_basic_buildings[tso.bo->id];
+		} else {
+			persistent_data->remaining_basic_buildings.erase(tso.bo->id);
+		}
+		tso.bo->never_occupied = false;
+	}
+
 	// Inform if we are above ai type limit.
 	if (tso.bo->total_count() > tso.bo->cnt_limit_by_aimode) {
 		log("AI check_trainingsites: AI player %d: count of %s exceeds an AI limit %d: actual count: "

=== modified file 'src/main.cc'
--- src/main.cc	2017-06-16 06:03:23 +0000
+++ src/main.cc	2018-02-06 19:41:40 +0000
@@ -1,3 +1,12 @@
+#include <unistd.h>
+#include <unistd.h>
+#include <unistd.h>
+#include <unistd.h>
+#include <unistd.h>
+#include <unistd.h>
+#include <unistd.h>
+#include <unistd.h>
+#include <unistd.h>
 /*
  * Copyright (C) 2002-2017 by the Widelands Development Team
  *