widelands-dev team mailing list archive

[GunChleoc <fios@xxxxxxxxxxxxxxxxxxx>]

Review: Approve

I see your point - see my diff comment.

Diff comments:

> === modified file 'src/logic/productionsite.cc'
> --- src/logic/productionsite.cc	2015-11-11 09:53:54 +0000
> +++ src/logic/productionsite.cc	2015-11-22 19:17:25 +0000
> @@ -284,33 +284,57 @@
>  }
>  
>  /**
> - * Detect if the workers are experienced enough for an upgrade
> + * Detect if the workers are experienced enough for an target building
> + * Buildable workers are skipped, but upgraded ones (required be target site) are tested
>   * @param idx Index of the enhancement
>   */
>  bool ProductionSite::has_workers(DescriptionIndex targetSite, Game & /* game */)
>  {
>  	// bld holds the description of the building we want to have
>  	if (upcast(ProductionSiteDescr const, bld, owner().tribe().get_building_descr(targetSite))) {
> -		// if he has workers
> +
>  		if (bld->nr_working_positions()) {
> -			DescriptionIndex need = bld->working_positions()[0].first;
> -			for (unsigned int i = 0; i < descr().nr_working_positions(); ++i) {
> -				if (!working_positions()[i].worker) {
> -					return false; // no one is in this house
> -				} else {
> -					DescriptionIndex have = working_positions()[i].worker->descr().worker_index();
> -					if (owner().tribe().get_worker_descr(have)->can_act_as(need)) {
> -						return true; // he found a lead worker
> +
> +			// Iterating over workers positions in target building
> +			for (const auto& wp : bld->working_positions()) {
> +
> +				// If worker for this position is buildable, just skip him
> +				if (owner().tribe().get_worker_descr(wp.first)->is_buildable()){
> +					continue;
> +				}
> +
> +				// This position needs promoted worker, so trying to find out if there is such worker
> +				// currently available in this site
> +				const DescriptionIndex needed_worker = wp.first;
> +				bool worker_available =  false;
> +				for (unsigned int i = 0; i < descr().nr_working_positions(); ++i) {
> +					if (working_positions()[i].worker) {
> +						DescriptionIndex current_worker
> +							= working_positions()[i].worker->descr().worker_index();

Duh, of course - that's an important difference. So, ignore my comment.

I don't know if sticking working_positions()[i].worker into a variable makes things easier to read, because it's used twice. I'll leave that up to you.

> +						if (owner().tribe().get_worker_descr(current_worker)->can_act_as(needed_worker)) {
> +							worker_available = true; // We found a worker for the position
> +							break;
> +						}
>  					}
>  				}
> +				if (!worker_available) {
> +					// We dont have needed workers in the site :(
> +					return false;
> +				}
> +
>  			}
> -			return false;
> +
> +			//if we are here, all needs are satisfied
> +			return true;
> +
> +		} else {
> +			throw wexception("Building, index: %d, needs no workers!\n", targetSite);
>  		}
> -		return true;
> -	} else return true;
> +	} else {
> +		throw wexception("No such building, index: %d\n", targetSite);
> +	}
>  }
>  
> -
>  WaresQueue & ProductionSite::waresqueue(DescriptionIndex const wi) {
>  	for (WaresQueue * ip_queue : m_input_queues) {
>  		if (ip_queue->get_ware() == wi) {


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