widelands-dev team mailing list archive

[GunChleoc <fios@xxxxxxxxxxxxxxxxxxx>]

Some code review comments

Diff comments:

> === modified file 'src/economy/roadbase.cc'
> --- src/economy/roadbase.cc	2019-03-12 12:11:23 +0000
> +++ src/economy/roadbase.cc	2019-03-12 12:11:50 +0000
> @@ -69,6 +71,68 @@
>  	return *flags_[FlagStart];
>  }
>  
> +// This returns true if and only if this is a road that covers the specified edge and
> +// both triangles adjacent to that edge are unwalkable
> +bool RoadBase::is_bridge(const EditorGameBase& egbase, const FCoords& field, uint8_t dir) const {

How is the performance for this function? It might be worth it to remember it in the map and only recalculate on terrain changes, like we do for seafaring checks.

> +	if (descr().type() != MapObjectType::ROAD) {
> +		// waterways can't be bridges...
> +		return false;
> +	}
> +
> +	const Map& map = egbase.map();
> +
> +	FCoords iterate = map.get_fcoords(path_.get_start());
> +	const Path::StepVector::size_type nr_steps = path_.get_nsteps();
> +	bool found = false;
> +	for (Path::StepVector::size_type i = 0; i <= nr_steps; ++i) {
> +		if (iterate == field) {
> +			if ((i < nr_steps && path_[i] == dir) || (i > 0 && path_[i - 1] == get_reverse_dir(dir))) {
> +				found = true;
> +				break;
> +			}
> +			return false;
> +		}
> +		if (i < nr_steps) {
> +			map.get_neighbour(iterate, path_[i], &iterate);
> +		}
> +	}
> +	if (!found) {
> +		return false;
> +	}
> +
> +	FCoords fr, fd;
> +	switch (dir) {
> +		case WALK_SW:
> +			fd = field;
> +			map.get_ln(field, &fr);
> +			break;
> +		case WALK_SE:
> +			fd = field;
> +			fr = field;
> +			break;
> +		case WALK_NW:
> +			map.get_tln(field, &fd);
> +			fr = fd;
> +			break;
> +		case WALK_NE:
> +			map.get_trn(field, &fd);
> +			map.get_tln(field, &fr);
> +			break;
> +		case WALK_W:
> +			map.get_tln(field, &fd);
> +			map.get_ln(field, &fr);
> +			break;
> +		case WALK_E:
> +			map.get_trn(field, &fd);
> +			fr = field;
> +			break;
> +		default:
> +			NEVER_HERE();
> +	}
> +	return (egbase.world().terrain_descr(fd.field->terrain_d()).get_is() & TerrainDescription::Is::kUnwalkable) &&
> +			(egbase.world().terrain_descr(fr.field->terrain_r()).get_is() & TerrainDescription::Is::kUnwalkable);
> +}
> +
>  /**
>   * Return the cost of getting from fromflag to the other flag.
>   */
> @@ -107,15 +171,25 @@
>  		// mark the road that leads up to this field
>  		if (steps > 0) {
>  			const Direction dir = get_reverse_dir(path_[steps - 1]);
> -			if (dir == WALK_SW || dir == WALK_SE || dir == WALK_E)
> -				egbase.set_road(curf, dir, type_);
> +			if (dir == WALK_SW || dir == WALK_SE || dir == WALK_E) {
> +				uint8_t type = type_;
> +				if (is_bridge(egbase, curf, dir)) {
> +					type += RoadType::kBridgeNormal - RoadType::kNormal;

What about busy bridges?

> +				}
> +				egbase.set_road(curf, dir, type);
> +			}
>  		}
>  
>  		// mark the road that leads away from this field
>  		if (steps < path_.get_nsteps()) {
>  			const Direction dir = path_[steps];
> -			if (dir == WALK_SW || dir == WALK_SE || dir == WALK_E)
> -				egbase.set_road(curf, dir, type_);
> +			if (dir == WALK_SW || dir == WALK_SE || dir == WALK_E) {
> +				uint8_t type = type_;
> +				if (is_bridge(egbase, curf, dir)) {
> +					type += RoadType::kBridgeNormal - RoadType::kNormal;
> +				}
> +				egbase.set_road(curf, dir, type);
> +			}
>  
>  			map.get_neighbour(curf, dir, &curf);
>  		}
> 
> === modified file 'src/logic/map_objects/tribes/road_textures.cc'
> --- src/logic/map_objects/tribes/road_textures.cc	2019-03-12 12:11:23 +0000
> +++ src/logic/map_objects/tribes/road_textures.cc	2019-03-12 12:11:50 +0000
> @@ -17,10 +17,25 @@
>   *
>   */
>  
> +#include "base/wexception.h"
>  #include "logic/map_objects/tribes/road_textures.h"
>  
>  #include <memory>
>  
> +const Image& RoadTextures::get_texture(
> +		const Widelands::RoadType type, const Widelands::Coords& coords, int direction) const {
> +	switch (type) {
> +		case Widelands::RoadType::kNormal:
> +			return get_normal_texture(coords, direction);
> +		case Widelands::RoadType::kBusy:
> +			return get_busy_texture(coords, direction);
> +		case Widelands::RoadType::kWaterway:
> +			return get_waterway_texture(coords, direction);

Call the type and the function "...bridge..." for consistency

> +		default:
> +			NEVER_HERE();
> +	}
> +}
> +
>  const Image& RoadTextures::get_normal_texture(const Widelands::Coords& coords,
>                                                int direction) const {
>  	return *normal_textures_.at((coords.x + coords.y + direction) % normal_textures_.size());
> 
> === modified file 'src/wui/interactive_player.cc'
> --- src/wui/interactive_player.cc	2019-03-12 12:11:23 +0000
> +++ src/wui/interactive_player.cc	2019-03-12 12:11:50 +0000
> @@ -353,6 +353,21 @@
>  					}
>  				}
>  			}
> +			if (f->road_e == Widelands::RoadType::kBridgeNormal || f->road_e == Widelands::RoadType::kBridgeBusy) {
> +				dst->blit_animation(f->rendertarget_pixel, scale, f->owner->tribe().bridge_animation(
> +						Widelands::WALK_E, f->road_e == Widelands::RoadType::kBridgeBusy),
> +						f->vision == 1 ? 0 : gametime, f->owner->get_playercolor());

Use the vision as a function parameter and pull out a common function with the spectator

> +			}
> +			if (f->road_sw == Widelands::RoadType::kBridgeNormal || f->road_sw == Widelands::RoadType::kBridgeBusy) {
> +				dst->blit_animation(f->rendertarget_pixel, scale, f->owner->tribe().bridge_animation(
> +						Widelands::WALK_SW, f->road_sw == Widelands::RoadType::kBridgeBusy),
> +						f->vision == 1 ? 0 : gametime, f->owner->get_playercolor());
> +			}
> +			if (f->road_se == Widelands::RoadType::kBridgeNormal || f->road_se == Widelands::RoadType::kBridgeBusy) {
> +				dst->blit_animation(f->rendertarget_pixel, scale, f->owner->tribe().bridge_animation(
> +						Widelands::WALK_SE, f->road_se == Widelands::RoadType::kBridgeBusy),
> +						f->vision == 1 ? 0 : gametime, f->owner->get_playercolor());
> +			}
>  
>  			draw_border_markers(*f, scale, *fields_to_draw, dst);
>  


-- 
https://code.launchpad.net/~widelands-dev/widelands/bridges/+merge/364318
Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bridges into lp:widelands.