widelands-dev team mailing list archive

[SirVer <SirVer@xxxxxx>]

Review: Approve

code lgtm now. A couple of minor nits, feel free to merge after addressing these.

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-04 18:44:46 +0000
> @@ -664,14 +665,45 @@
>  	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;

prefer UniqueDistance ud = { dist, ...get_serial(), provider };

if anybody ever adds more fields to UniqueDistance, this code will fail to compile (which is good!) while your old code might slip through unnoticed.

> +		ud.distance = dist;
> +		ud.serial = supp.get_position(game)->serial();
> +		ud.provider_type = provider;
> +
> +		// 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::pair<UniqueDistance, Supply*>(ud, &m_supplies[i]));

nit: insert(std::make_pair(ud, &m_supplies[i])) is a bit more readable imho

> +
> +	}
> +
> +	// Now available supplies have been sorted by distance to requestor
> +	for (auto& supplypair : available_supplies) {
> +

remove empty line?

> +		Supply & supp = *supplypair.second;
> +
>  		Route * const route =
>  			best_route != &buf_route0 ? &buf_route0 : &buf_route1;
>  		// will be cleared by find_route()
> @@ -700,6 +738,7 @@
>  
>  	cost = best_cost;
>  	return best_supply;
> +

remove empty line?

>  }
>  
>  struct RequestSupplyPair {
> 
> === modified file 'src/economy/economy.h'
> --- src/economy/economy.h	2015-11-11 09:52:55 +0000
> +++ src/economy/economy.h	2016-01-04 18:44:46 +0000
> @@ -237,6 +238,24 @@
>  	static std::unique_ptr<Soldier> m_soldier_prototype;
>  	UI::UniqueWindow::Registry m_optionswindow_registry;
>  
> +	// This structs is to store distance from supply to request(or), but to allow unambiguous

nit: I prefer to have the following ordering after private: types, functions, variables. Your call.

> +	// sorting if distances are the same, we use also serial number of provider and type of provider (flag,
> +	// warehouse)
> +	struct UniqueDistance {
> +		uint32_t distance;
> +		uint32_t serial;
> +		SupplyProviders provider_type;
> +
> +	bool operator<(const UniqueDistance& other) const {
> +       return std::forward_as_tuple(distance, serial, provider_type)
> +			<
> +			std::forward_as_tuple(other.distance, other.serial, other.provider_type);
> +		}
> +	};
> +	// 'list' of unique providers
> +	std::map<UniqueDistance, Supply*> available_supplies;
> +
> +
>  	DISALLOW_COPY_AND_ASSIGN(Economy);
>  };
>  
> 
> === modified file 'src/economy/idleworkersupply.cc'
> --- src/economy/idleworkersupply.cc	2015-11-11 09:52:55 +0000
> +++ src/economy/idleworkersupply.cc	2016-01-04 18:44:46 +0000
> @@ -71,6 +71,11 @@
>  	return true;
>  }
>  
> +SupplyProviders IdleWorkerSupply::provider_type(Game &) const

nit: prefer to take mutable parameters (Game) as pointers - so that on the callsite it is immediately visible that game might be mutated.

> +{
> +	return SupplyProviders::kFlagOrRoad;
> +}
> +
>  bool IdleWorkerSupply::has_storage() const
>  {
>  	return m_worker.get_transfer();
> 
> === modified file 'src/logic/widelands_geometry.h'
> --- src/logic/widelands_geometry.h	2014-09-19 12:54:54 +0000
> +++ src/logic/widelands_geometry.h	2016-01-04 18:44:46 +0000
> @@ -55,6 +55,10 @@
>  		}
>  	};
>  
> +	uint32_t hash() const {

seems unused? remove.

> +		return  x << 16 | y;
> +	};
> +
>  	// Move the coords to the 'new_origin'.
>  	void reorigin(Coords new_origin, const Extent & extent);
>  


-- 
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.