widelands-dev team mailing list archive

[Benedikt Straub <benedikt-straub@xxxxxx>]

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

Diff comments:

> 
> === modified file 'src/ai/defaultai.cc'
> --- src/ai/defaultai.cc	2019-04-20 05:44:37 +0000
> +++ src/ai/defaultai.cc	2019-05-15 06:30:23 +0000
> @@ -2454,6 +2458,12 @@
>  
>  		if (!bo.buildable(*player_)) {
>  			bo.new_building = BuildingNecessity::kNotNeeded;
> +			// TODO(Hessenfarmer): Add the buildings if they are allowed again
> +			// This line removes buildings from basic econmy if they are not allowed for the player
> +			// this should only happen by scripting.

Please take care of this TODO here – if a productionpath is prohibited until some turning point, it should still be considered more important then.

> +			if (bo.basic_amount) {
> +				persistent_data->remaining_basic_buildings.erase(bo.id);
> +			}
>  		} else if (bo.type == BuildingObserver::Type::kProductionsite ||
>  		           bo.type == BuildingObserver::Type::kMine) {
>  
> @@ -3752,6 +3763,8 @@
>  		// if we are within grace time, it is OK, just go on
>  		if (eco->dismantle_grace_time > gametime &&
>  		    eco->dismantle_grace_time != std::numeric_limits<uint32_t>::max()) {
> +			;

Please get rid of empty ifs:
if (eco->dismantle_grace_time == std::numeric_limits<uint32_t>::max()) {
  // The long if body
} else if eco->dismantle_grace_time <= gametime) {
  last_attempt_ = true;
  checkradius += 2;
}
// No else loop needed

> +
>  			// if grace time is not set, this is probably first time without a warehouse and we must
>  			// set it
>  		} else if (eco->dismantle_grace_time == std::numeric_limits<uint32_t>::max()) {
> @@ -4483,7 +4500,8 @@
>  	    site.site->can_start_working() &&
>  	    check_building_necessity(*site.bo, PerfEvaluation::kForDismantle, gametime) ==
>  	       BuildingNecessity::kNotNeeded &&
> -	    gametime - site.bo->last_dismantle_time > 5 * 60 * 1000 &&
> +	    gametime - site.bo->last_dismantle_time >
> +	        (std::abs(management_data.get_military_number_at(169)) / 5 + 1) * 60 * 1000 &&

This needs a static_cast, you mustn´t compare signed and unsigned

>  
>  	    site.bo->current_stats > site.site->get_statistics_percent() &&  // underperformer
>  	    (game().get_gametime() - site.unoccupied_till) > 10 * 60 * 1000) {
> @@ -4935,11 +4952,11 @@
>  		if (!basic_economy_established) {
>  			return BuildingNecessity::kForbidden;
>  		}
> -		const uint16_t min_roads_count = 50 + std::abs(management_data.get_military_number_at(33));
> -		if (roads.size() < min_roads_count) {
> +		const uint16_t min_roads_count = 40 + std::abs(management_data.get_military_number_at(33))/2;

Whitespaces before and after / please

> +		if (roads.size() < min_roads_count * (1 + bo.total_count())) {

static_cast needed to avoid signed-unsigned comparison

>  			return BuildingNecessity::kForbidden;
>  		}
> -		bo.primary_priority = (roads.size() - min_roads_count) *
> +		bo.primary_priority += (roads.size() - min_roads_count * (1 + bo.total_count())) *
>  		                      (2 + std::abs(management_data.get_military_number_at(143)) / 5);
>  		return BuildingNecessity::kNeeded;
>  	}
> @@ -5516,7 +5531,7 @@
>  			inputs[0] = (bo.total_count() <= 1) ?
>  			               std::abs(management_data.get_military_number_at(110)) / 10 :
>  			               0;
> -			inputs[1] = -2 * bo.total_count();
> +			inputs[1] = -4 * bo.total_count() + 2 * bo.total_count() + bo.total_count() / 2;

What is the point of such a calculation?
inputs[1] = bo.total_count() * -3 / 2;

>  			inputs[2] =
>  			   (bo.total_count() == 0) ? std::abs(management_data.get_military_number_at(0)) / 10 : 0;
>  			inputs[3] = (gametime >= 25 * 60 * 1000 && bo.inputs.empty()) ?


-- 
https://code.launchpad.net/~widelands-dev/widelands/AI-fixes/+merge/367309
Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/AI-fixes.