I agree. Don't overlook the second NOCOM ;)
Diff comments:
>
> === modified file 'src/ai/defaultai.cc'
> --- src/ai/defaultai.cc 2016-02-22 08:50:04 +0000
> +++ src/ai/defaultai.cc 2016-02-26 09:06:57 +0000
> @@ -3114,74 +3118,130 @@
>
> Map& map = game().map();
>
> - // first we try to upgrade
> - // Upgrading policy
> - // a) if there are two buildings and none enhanced and there are workers
> - // available, one is to be enhanced
> - // b) if there are two buildings
> - // statistics percents are decisive
> - // c) yet there are buildings that might be upgraded, even when
> - // there is no second buiding of the kind (flag upgrade_substitutes)
> + // The code here is bit complicated
> + // a) Either this site is pending for upgrade, if ready, order the
> upgrade
> + // b) other site of type is pending for upgrade
> + // c) if none of above, we can consider upgrade of this one
>
> const DescriptionIndex enhancement = site.site->descr().enhancement();
> - if (connected_to_wh && enhancement != INVALID_INDEX &&
> - // if upgrade does not subsitute, we need to have two buildings
> at least
> - ((site.bo->cnt_built - site.bo->unoccupied_count > 1 &&
> site.bo->upgrade_extends)
> - ||
> - site.bo->upgrade_substitutes) &&
> - gametime > 45 * 60 * 1000 &&
> - gametime > site.built_time + 20 * 60 * 1000) {
> -
> - // Only enhance buildings that are allowed (scenario mode)
> - // do not do decisions too fast
> - if (player_->is_building_type_allowed(enhancement)) {
> -
> - const BuildingDescr& bld =
> *tribe_->get_building_descr(enhancement);
> - BuildingObserver& en_bo =
> get_building_observer(bld.name().c_str());
> - bool doing_upgrade = false;
> -
> - if (gametime - en_bo.construction_decision_time >= 10 *
> 60 * 1000 &&
> - (en_bo.cnt_under_construction +
> en_bo.unoccupied_count) == 0) {
> -
> - // don't upgrade without workers
> - if (site.site->has_workers(enhancement,
> game())) {
> -
> - // forcing first upgrade
> - if (en_bo.total_count() == 0) {
> +
> + bool considering_upgrade = enhancement != INVALID_INDEX;
> +
> + // First we check for rare case when input wares are set to 0 but AI is
> not aware that
> + // the site is pending for upgrade - one possible cause is this is a
> freshly loaded game
> + if (!site.upgrade_pending) {
> + bool resetting_wares = false;
> + for (auto& queue : site.site->warequeues()) {
> + if (queue->get_max_fill() == 0) {
> + resetting_wares = true;
> +
> game().send_player_set_ware_max_fill(*site.site, queue->get_ware(),
> queue->get_max_size());
> + }
> + }
> + if (resetting_wares) {
> + log(" %d: AI: input queues were reset to max for %s
> (game just loaded?)\n",
> + player_number(),
> + site.bo->name);
> + return true;
> + }
> + }
> +
> + if (site.upgrade_pending) {
> + // The site is in process of emptying its input queues
> + // Counting remaining wares in the site now
> + int32_t left_wares = 0;
> + for (auto& queue : site.site->warequeues()) {
> + left_wares += queue->get_filled();
> + }
> + // Do nothing when some wares are left, but do not wait more
> then 4 minutes
> + if (site.bo->construction_decision_time + 4 * 60 * 1000 >
> gametime && left_wares > 0) {
> + return false;
> + }
> + assert (site.bo->cnt_upgrade_pending == 1);
> + assert(enhancement != INVALID_INDEX);
> + game().send_player_enhance_building(*site.site, enhancement);
> + considering_upgrade = false; // NOCOM(#codereview): I don't
> think we need to set this here, because we return anyway.
> + return true;
> + } else if (site.bo->cnt_upgrade_pending > 0) {
> + // some other site of this type is in pending for upgrade
> + assert (site.bo->cnt_upgrade_pending == 1);
> + return false;
> + }
> + assert (site.bo->cnt_upgrade_pending == 0);
> +
> + // Of course type of productionsite must be allowed
> + if (considering_upgrade &&
> !player_->is_building_type_allowed(enhancement)) {
> + considering_upgrade = false;
> + }
> +
> + // Site must be connected to warehouse
> + if (considering_upgrade && !connected_to_wh) {
> + considering_upgrade = false;
> + }
> +
> + // if upgraded building produces other wares we are willing to upgrade
> it sooner
> + if (considering_upgrade) {
> + if (site.bo->upgrade_extends) {
> + // if upgrade produces new outputs, we force first
> upgrade
> + if (gametime < site.built_time + 10 * 60 * 1000) {
> + considering_upgrade = false; //
> NOCOM(codereview): I don't understand - how does 'false' force an upgrade
> here?
Sounds good to me :)
> + }
> + } else {
> + if (gametime < 45 * 60 * 1000 || gametime <
> site.built_time + 20 * 60 * 1000) {
> + considering_upgrade = false;
> + }
> + }
> + }
> +
> + // No upgrade without proper workers
> + if (considering_upgrade && !site.site->has_workers(enhancement,
> game())) {
> + considering_upgrade = false;
> + }
> +
> + if (considering_upgrade) {
> +
> + const BuildingDescr& bld =
> *tribe_->get_building_descr(enhancement);
> + BuildingObserver& en_bo =
> get_building_observer(bld.name().c_str());
> + bool doing_upgrade = false;
> +
> + // 10 minutes is a time to productions statics to settle
> + if ((en_bo.last_building_built == kNever || gametime -
> en_bo.last_building_built >= 10 * 60 * 1000) &&
> + (en_bo.cnt_under_construction + en_bo.unoccupied_count) ==
> 0) {
> +
> + // forcing first upgrade
> + if (en_bo.total_count() == 0) {
> + doing_upgrade = true;
> + }
> +
> + if (en_bo.total_count() == 1) {
> + if (en_bo.current_stats > 55) {
> + doing_upgrade = true;
> + }
> + }
> +
> + if (en_bo.total_count() > 1) {
> + if (en_bo.current_stats > 75) {
> doing_upgrade = true;
> - }
> -
> - if (en_bo.total_count() == 1) {
> - //if the upgrade itself can be
> upgraded futher, we are more willing to upgrade 2nd building
> - if (en_bo.upgrade_extends ||
> en_bo.upgrade_substitutes) {
> - if (en_bo.current_stats
> > 30) {
> - doing_upgrade =
> true;
> - }
> - } else if (en_bo.current_stats
> > 50) {
> - doing_upgrade = true;
> - }
> - }
> -
> - if (en_bo.total_count() > 1) {
> - if (en_bo.current_stats > 80) {
> - doing_upgrade =
> true;
> - }
> - }
> -
> - // Dont forget about limitation of
> number of buildings
> - if (en_bo.cnt_limit_by_aimode <=
> en_bo.total_count() - en_bo.unconnected_count) {
> - doing_upgrade = false;
> - }
> }
> }
>
> - // Enhance if enhanced building is useful
> - // additional: we dont want to lose the old building
> - if (doing_upgrade) {
> - game().send_player_enhance_building(*site.site,
> enhancement);
> - en_bo.construction_decision_time = gametime;
> - return true;
> - }
> + // Don't forget about limitation of number of buildings
> + if (en_bo.aimode_limit_status() !=
> AiModeBuildings::kAnotherAllowed) {
> + doing_upgrade = false;
> + }
> + }
> +
> + // Here we just restrict input wares to 0 and set flag
> 'upgrade_pending' to true
> + if (doing_upgrade) {
> +
> + // reducing input queues
> + for (auto& queue : site.site->warequeues()) {
> +
> game().send_player_set_ware_max_fill(*site.site, queue->get_ware(), 0);
> + }
> + site.bo->construction_decision_time = gametime;
> + en_bo.construction_decision_time = gametime;
> + site.upgrade_pending = true;
> + site.bo->cnt_upgrade_pending += 1;
> + return true;
> }
> }
>
--
https://code.launchpad.net/~widelands-dev/widelands/ai_upgrading_reworked/+merge/287229
Your team Widelands Developers is requested to review the proposed merge of
lp:~widelands-dev/widelands/ai_upgrading_reworked into lp:widelands.
_______________________________________________
Mailing list: https://launchpad.net/~widelands-dev
Post to : [email protected]
Unsubscribe : https://launchpad.net/~widelands-dev
More help : https://help.launchpad.net/ListHelp
