widelands-dev team mailing list archive

[SirVer <SirVer@xxxxxx>]

Review: Approve

there are still NOCOM in the diff which need removing, otherwise lgtm.

Diff comments:

> === modified file 'src/economy/economy.cc'
> --- src/economy/economy.cc	2015-11-11 09:52:55 +0000
> +++ src/economy/economy.cc	2016-01-06 20:05:29 +0000
> @@ -664,14 +665,42 @@
>  	Route  * best_route  = nullptr;
>  	int32_t  best_cost   = -1;
>  	Flag & target_flag = req.target_flag();
> +	Map & map = game.map();
> +
> +	available_supplies.clear();
>  
>  	for (size_t i = 0; i < m_supplies.get_nrsupplies(); ++i) {
>  		Supply & supp = m_supplies[i];
>  
> -		// Check requirements
> +		// Just skip if supply does not provide required ware
>  		if (!supp.nr_supplies(game, req))
>  			continue;
>  
> +		const SupplyProviders provider = supp.provider_type(&game);
> +
> +		// We generally ignore disponible wares on ship as it is not possible to reliably
> +		// calculate route (transportation time)
> +		if (provider == SupplyProviders::kShip) {
> +			continue;
> +		}
> +
> +		const Widelands::Coords provider_position = supp.get_position(game)->base_flag().get_position();
> +
> +		const uint32_t dist = map.calc_distance(target_flag.get_position(), provider_position);
> +
> +		UniqueDistance ud = {dist, supp.get_position(game)->serial(), provider};

can also inline if you want, since you do not use it again:

available_supplies.insert(std::make_pair(UniqueDistance{dist, supp.get_position(game)->serial(), provider}, &m_supplies[i]));

your call.

> +
> +		// std::map quarantees uniqueness, practically it means that if more wares are on the same flag, only
> +		// first one will be inserted into available_supplies
> +		available_supplies.insert(std::make_pair(ud, &m_supplies[i]));
> +		//available_supplies.insert(std::pair<UniqueDistance, Supply*>(ud, &m_supplies[i])); NOCOM remove

remove?

> +
> +	}
> +
> +	// Now available supplies have been sorted by distance to requestor
> +	for (auto& supplypair : available_supplies) {
> +		Supply & supp = *supplypair.second;
> +
>  		Route * const route =
>  			best_route != &buf_route0 ? &buf_route0 : &buf_route1;
>  		// will be cleared by find_route()


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