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