Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1746481 into lp:widelands
OK, I have set it back to "Work in Progress" -- https://code.launchpad.net/~widelands-dev/widelands/bug-1746481/+merge/337125 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1746481 into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1746481 into lp:widelands
The proposal to merge lp:~widelands-dev/widelands/bug-1746481 into lp:widelands has been updated. Status: Needs review => Work in progress For more details, see: https://code.launchpad.net/~widelands-dev/widelands/bug-1746481/+merge/337125 -- Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1746481 into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1746481 into lp:widelands
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 -- https://code.launchpad.net/~widelands-dev/widelands/bug-1746481/+merge/337125 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1746481 into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1746481 into lp:widelands
Is this branch ready for review? We should get rid of the extra lines in src/main.cc in any case. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1746481/+merge/337125 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1746481 into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1746481 into lp:widelands
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... -- https://code.launchpad.net/~widelands-dev/widelands/bug-1746481/+merge/337125 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1746481 into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1746481 into lp:widelands
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 " to main.cc in the commit that fixed the printf. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1746481/+merge/337125 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1746481 into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1746481 into lp:widelands
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 -- https://code.launchpad.net/~widelands-dev/widelands/bug-1746481/+merge/337125 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1746481 into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1746481 into lp:widelands
Please try once more.. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1746481/+merge/337125 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1746481 into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1746481 into lp:widelands
I omit to incorporate can_start_working() function to the check altogether, it simply cannot work properly -- https://code.launchpad.net/~widelands-dev/widelands/bug-1746481/+merge/337125 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1746481 into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1746481 into lp:widelands
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... -- https://code.launchpad.net/~widelands-dev/widelands/bug-1746481/+merge/337125 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1746481 into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1746481 into lp:widelands
INDEED, I see a problem (in the code) now. I will fix and let you know... -- https://code.launchpad.net/~widelands-dev/widelands/bug-1746481/+merge/337125 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1746481 into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1746481 into lp:widelands
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. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1746481/+merge/337125 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1746481 into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1746481 into lp:widelands
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... -- https://code.launchpad.net/~widelands-dev/widelands/bug-1746481/+merge/337125 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1746481 into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1746481 into lp:widelands
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. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1746481/+merge/337125 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1746481 into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
Re: [Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1746481 into lp:widelands
Codecheck complains about printfs, but these are only for temporary DEBUG logs and will be removed before merge... -- https://code.launchpad.net/~widelands-dev/widelands/bug-1746481/+merge/337125 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1746481 into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1746481 into lp:widelands
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. -- https://code.launchpad.net/~widelands-dev/widelands/bug-1746481/+merge/337125 Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-1746481 into lp:widelands. ___ Mailing list: https://launchpad.net/~widelands-dev Post to : widelands-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~widelands-dev More help : https://help.launchpad.net/ListHelp
[Widelands-dev] [Merge] lp:~widelands-dev/widelands/bug-1746481 into lp:widelands
TiborB has proposed merging lp:~widelands-dev/widelands/bug-1746481 into
lp:widelands.
Requested reviews:
Widelands Developers (widelands-dev)
Related bugs:
Bug #1746481 in widelands: "AI reports basic economy built to soon."
https://bugs.launchpad.net/widelands/+bug/1746481
For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/bug-1746481/+merge/337125
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.
--
Your team Widelands Developers is requested to review the proposed merge of
lp:~widelands-dev/widelands/bug-1746481 into lp:widelands.
=== modified file 'src/ai/ai_help_structs.h'
--- src/ai/ai_help_structs.h 2017-11-17 07:16:12 +
+++ src/ai/ai_help_structs.h 2018-02-03 22:34:53 +
@@ -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 inputs;
std::vector outputs;
=== modified file 'src/ai/defaultai.cc'
--- src/ai/defaultai.cc 2018-01-30 11:44:48 +
+++ src/ai/defaultai.cc 2018-02-03 22:34:53 +
@@ -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) {
+ printf ("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) {
+ printf ("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 = _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_bac
