widelands-dev team mailing list archive

[Janosch Peters <janosch.peters@xxxxxxxxx>]

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.