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.
_______________________________________________
Mailing list: https://launchpad.net/~widelands-dev
Post to : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help : https://help.launchpad.net/ListHelp
