It could be coded a bit more elegantly(see my comments), but apart from that it
LGTM. However, I did not test it because I dont know a straight forward way to
get a warehous with 500+ workers.
Diff comments:
> === modified file 'src/logic/map_objects/tribes/warehouse.cc'
> --- src/logic/map_objects/tribes/warehouse.cc 2016-08-04 15:49:05 +0000
> +++ src/logic/map_objects/tribes/warehouse.cc 2016-11-07 19:19:08 +0000
> @@ -528,13 +529,24 @@
> if (upcast(Game, game, &egbase)) {
> const WareList& workers = get_workers();
> for (DescriptionIndex id = 0; id < workers.get_nrwareids();
> ++id) {
> - const uint32_t stock = workers.stock(id);
> + Quantity stock = workers.stock(id);
> // Separate behaviour for the case of loading the game
> // (which does save/destroy/reload) and simply
> destroying ingame
> if (game->is_loaded()) {
> // This game is really running
> - for (uint32_t i = 0; i < stock; ++i) {
> - launch_worker(*game, id,
> Requirements()).start_task_leavebuilding(*game, true);
> + Quantity worker_counter = 0;
I dont see why we need "worker_counter" and "i". I suggest to initialize
"worker_counter" within for() and get rid of "i"
> + for (Quantity i = 0; i < stock; ++i,
> ++worker_counter) {
> + // Make sure that we won't flood the
> map with carriers etc.
> + if (worker_counter < kFleeingUnitsCap) {
This condition - which is part of the exit condition - should be in the for
statement and not in the loop. E.g. sth like
for (Quantity worker_counter = 0; worker_counter < stock && worker_counter <
kFleeingUnitsCap; ++worker_counter).
If implemented like this we dont need the break statement.
> + launch_worker(*game, id,
> Requirements()).start_task_leavebuilding(*game, true);
> + } else {
> + break;
> + }
> + }
> + // Remove the remaining stock in case we hit
> the cap
> + stock = workers.stock(id);
> + if (stock > 0) {
> + remove_workers(id, stock);
> }
> assert(!incorporated_workers_.count(id) ||
> incorporated_workers_[id].empty());
> } else {
If we had a method remove_all_workers we wouldnt need to differntiate between
load-game and in-game. We could just remove all workers after the
if(game->is_loaded()) condition. So IMHO the cleanest solution would be to
implement this method, but I understand if you dont want to do this. Its not
nessecary to fix this bug.
--
https://code.launchpad.net/~widelands-dev/widelands/bug-1624216-horsepocalypse/+merge/310221
Your team Widelands Developers is requested to review the proposed merge of
lp:~widelands-dev/widelands/bug-1624216-horsepocalypse into lp:widelands.
_______________________________________________
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help : https://help.launchpad.net/ListHelp
