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.
_______________________________________________
Mailing list: https://launchpad.net/~widelands-dev
Post to : [email protected]
Unsubscribe : https://launchpad.net/~widelands-dev
More help : https://help.launchpad.net/ListHelp
