widelands-dev team mailing list archive

[GunChleoc <fios@xxxxxxxxxxxxxxxxxxx>]

Review: Approve code

Code LGTM - only some minor nits.

Not tested.

Diff comments:

> === modified file 'src/ai/defaultai.cc'
> --- src/ai/defaultai.cc	2017-12-10 21:19:02 +0000
> +++ src/ai/defaultai.cc	2017-12-25 21:20:29 +0000
> @@ -1048,10 +1048,11 @@
>  }
>  
>  /**
> - * Checks ALL available buildable fields.
> + * Checks PART of available buildable fields.
>   *
> - * this shouldn't be used often, as it might hang the game for some 100
> - * milliseconds if the area the computer owns is big.
> + * this checks about 40-50 buildable fields. In big games the player can have thousends

In big games, the player can have thousands

> + * of them, so we rotate the buildable_fields container and check 35 fields, and in addition
> + * we look for medium&big fields and near border fields if needed.

medium & big

>   */
>  void DefaultAI::update_all_buildable_fields(const uint32_t gametime) {
>  	uint16_t i = 0;
> @@ -1059,35 +1060,111 @@
>  	// To be sure we have some info about enemies we might see
>  	update_player_stat(gametime);
>  
> +	// Generall we check fields as they are in the container, but we need also given

// Generally,

> +	// number of "special" fields. So if given number of fields are not found within
> +	// "regular" check, we must go on and look also on other fields...
> +	uint8_t non_small_needed = 4;
> +	uint8_t near_border_needed = 10;
> +
> +	const uint16_t minimal_fields_check = 35;

This can be a constexpr uint16_t kMinimalFieldsCheck

> +
>  	// we test 40 fields that were update more than 1 seconds ago
> -	while (!buildable_fields.empty() &&
> -	       (buildable_fields.front()->field_info_expiration - kFieldInfoExpiration + 1000) <=
> -	          gametime &&
> -	       i < 40) {
> +	while (!buildable_fields.empty() && i < std::min<uint16_t>(minimal_fields_check, buildable_fields.size())) {
>  		BuildableField& bf = *buildable_fields.front();
>  
> -		//  check whether we lost ownership of the node
> -		if (bf.coords.field->get_owned_by() != player_number()) {
> -			delete &bf;
> -			buildable_fields.pop_front();
> -			continue;
> -		}
> -
> -		//  check whether we can still construct regular buildings on the node
> -		if ((player_->get_buildcaps(bf.coords) & BUILDCAPS_SIZEMASK) == 0) {
> -			unusable_fields.push_back(bf.coords);
> -			delete &bf;
> -			buildable_fields.pop_front();
> -			continue;
> -		}
> -
> -		update_buildable_field(bf);
> +		if ((buildable_fields.front()->field_info_expiration - kFieldInfoExpiration + 1000) <=
> +		    gametime) {
> +
> +			//  check whether we lost ownership of the node
> +			if (bf.coords.field->get_owned_by() != player_number()) {
> +				delete &bf;
> +				buildable_fields.pop_front();
> +				continue;
> +			}
> +
> +			//  check whether we can still construct regular buildings on the node
> +			if ((player_->get_buildcaps(bf.coords) & BUILDCAPS_SIZEMASK) == 0) {
> +				unusable_fields.push_back(bf.coords);
> +				delete &bf;
> +				buildable_fields.pop_front();
> +				continue;
> +			}
> +
> +			update_buildable_field(bf);
> +			if (non_small_needed > 0) {
> +				int32_t const maxsize = player_->get_buildcaps(bf.coords) & BUILDCAPS_SIZEMASK;
> +				if (maxsize > 1) {
> +					non_small_needed -= 1;
> +				}
> +			}
> +			if (near_border_needed > 0) {
> +				if (bf.near_border) {
> +					near_border_needed -= 1;
> +				}
> +			}
> +		}
>  		bf.field_info_expiration = gametime + kFieldInfoExpiration;
>  		buildable_fields.push_back(&bf);
>  		buildable_fields.pop_front();
>  
>  		i += 1;
>  	}
> +
> +	// If needed we iterate once more and look for 'special' fields
> +	// starting in the middle of buildable_fields to skip fields tested lately
> +	// But not doing this if the count of buildable fields is too low
> +	// (no need to bother) or this is a new game (< 20 seconds)

I think the extra "new game" check complicates he code - can we just remove it? It should matter much for performance. We also have the expiration comparison below.

> +	if (buildable_fields.size() < minimal_fields_check * 3 || gametime < 20000) {
> +		return;
> +	}
> +
> +	for (uint32_t j = buildable_fields.size() / 2; j < buildable_fields.size(); j++) {
> +		// If we dont need to iterate (anymore) ...
> +		if (non_small_needed + near_border_needed == 0) {
> +			break;
> +		}
> +
> +		// Skip if the field is not ours or were updated lately

were -> was

> +		if (buildable_fields[j]->coords.field->get_owned_by() != player_number()) {
> +			continue;
> +		}
> +		// We are not interested in fields where info has expired less than 20s ago
> +		if (buildable_fields[j]->field_info_expiration > gametime - 20000) {
> +			continue;
> +		}
> +
> +		// Continue if field is blocked at the moment
> +		if (blocked_fields.is_blocked(buildable_fields[j]->coords)) {
> +			continue;
> +		}
> +
> +		// Few constants to keep the code cleaner
> +		const int32_t field_maxsize =
> +		   player_->get_buildcaps(buildable_fields[j]->coords) & BUILDCAPS_SIZEMASK;
> +		const bool field_near_border = buildable_fields[j]->near_border;
> +
> +		// Let decide if we need to update and for what reason
> +		const bool update_due_size = non_small_needed && field_maxsize > 1;
> +		const bool update_due_border = near_border_needed && field_near_border;
> +
> +		if (!(update_due_size || update_due_border)) {
> +			continue;
> +		}
> +
> +		// decreasing the counters
> +		if (update_due_size) {
> +			assert (non_small_needed > 0);
> +			non_small_needed -= 1;
> +		}
> +		if (update_due_border) {
> +			assert (near_border_needed > 0);
> +			near_border_needed -= 1;
> +		}
> +
> +		// and finnaly update the buildable field
> +		update_buildable_field(*buildable_fields[j]);
> +		buildable_fields[j]->field_info_expiration = gametime + kFieldInfoExpiration;
> +	}
>  }
>  
>  /**


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