widelands-dev team mailing list archive

[GunChleoc <fios@xxxxxxxxxxxxxxxxxxx>]

Review: Approve code

I still want to do some testing, but code LGTM - just a few minor nits :)

Diff comments:

> 
> === modified file 'src/logic/map_objects/tribes/ship.cc'
> --- src/logic/map_objects/tribes/ship.cc	2016-02-08 08:01:21 +0000
> +++ src/logic/map_objects/tribes/ship.cc	2016-02-08 21:01:32 +0000
> @@ -49,6 +49,59 @@
>  
>  namespace Widelands {
>  
> +namespace  {
> +
> +/// Returns true if 'coord' is not occupied or onwned by 'player_number' and

Typo: onwned => owned

> +/// nothing stands there.
> +bool can_support_port(const PlayerNumber player_number, const FCoords& coord) {
> +	const PlayerNumber owner = coord.field->get_owned_by();
> +	if (owner != neutral() && owner != player_number) {
> +		return false;
> +	}
> +	BaseImmovable* baim = coord.field->get_immovable();
> +	if (baim != nullptr && baim->descr().type() >= MapObjectType::FLAG) {
> +		return false;
> +	}
> +	return true;
> +}
> +
> +/// Returns true if a ship owned by 'player_number' can land and errect a port at 'coord'.

Typo: errect => erect

> +bool can_build_port_here(const PlayerNumber player_number, const Map& map, const FCoords& coord) {

const PlayerNumber& player_number or PlayerNumber player_number?

PlayerNumber is an uint8_t really, so I guess both versions would be correct?.

> +	if (!can_support_port(player_number, coord)) {
> +		return false;
> +	}
> +
> +	// All fields of the port + their neighboring fields (for the border) must
> +	// be conquerable without military influence.
> +	Widelands::FCoords c[4];  //  Big buildings occupy 4 locations.
> +	c[0] = coord;
> +	map.get_ln(coord, &c[1]);
> +	map.get_tln(coord, &c[2]);
> +	map.get_trn(coord, &c[3]);
> +	for (int i = 0; i < 4; ++i) {
> +		MapRegion<Area<FCoords>> area(map, Area<FCoords>(c[i], 1));
> +		do {
> +			if (!can_support_port(player_number, area.location())) {
> +				return false;
> +			}
> +		} while (area.advance(map));
> +	}
> +
> +	// Also all areas around the flag must be conquerable and must not contain
> +	// another flag already.
> +	FCoords flag_position;
> +	map.get_brn(coord, &flag_position);
> +	MapRegion<Area<FCoords>> area(map, Area<FCoords>(flag_position, 1));
> +	do {
> +		if (!can_support_port(player_number, area.location())) {
> +			return false;
> +		}
> +	} while (area.advance(map));
> +	return true;
> +}
> +
> +}  // namespace
> +
>  ShipDescr::ShipDescr(const std::string& init_descname, const LuaTable& table)
>  	:
>  	BobDescr(init_descname, MapObjectType::SHIP, MapObjectDescr::OwnerType::kTribe, table) {
> === added file 'test/maps/port_space.wmf/elemental'
> --- test/maps/port_space.wmf/elemental	1970-01-01 00:00:00 +0000
> +++ test/maps/port_space.wmf/elemental	2016-02-08 21:01:32 +0000
> @@ -0,0 +1,12 @@
> +# Automatically created by Widelands bzr7801[trunk] (Debug)
> +
> +[global]
> +packet_version="1"
> +map_w="64"
> +map_h="64"
> +nr_players="2"
> +name="port_space"
> +author="Unknown"
> +descr="No description defined"
> +hint=
> +tags="artifacts,seafaring"

The "artifacts" tag should probably go?



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