widelands-dev team mailing list archive

[GunChleoc <fios@xxxxxxxxxxxxxxxxxxx>]

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.