I think we have some room here to get rid of code duplication.
Diff comments:
>
> === modified file 'src/graphic/gl/workarea_program.cc'
> --- src/graphic/gl/workarea_program.cc 2019-04-25 21:48:17 +0000
> +++ src/graphic/gl/workarea_program.cc 2019-04-28 16:58:57 +0000
> @@ -104,31 +113,43 @@
> // Down triangle.
> if (field.bln_index != FieldsToDraw::kInvalidIndex) {
> RGBAColor color(0, 0, 0, 0);
> - for (const std::map<Widelands::TCoords<>, uint8_t>&
> wa_map : workarea) {
> - const auto it =
> -
> wa_map.find(Widelands::TCoords<>(field.fcoords, Widelands::TriangleIndex::D));
> - if (it != wa_map.end()) {
> - color = apply_color(color,
> workarea_colors[it->second]);
> + for (const WorkareasEntry& wa_map : workarea) {
> + for (const WorkareaPreviewData& data : wa_map) {
> + if (data.coords ==
> Widelands::TCoords<>(field.fcoords, Widelands::TriangleIndex::D)) {
> + RGBAColor color_to_apply =
> workarea_colors[data.index];
> + if (data.use_special_coloring) {
> + color_to_apply =
> apply_color_special(color_to_apply, RGBAColor(data.special_coloring));
> + }
> + color = apply_color(color,
> color_to_apply);
> + }
> }
> }
> - add_vertex(fields_to_draw.at(current_index), color);
> - add_vertex(fields_to_draw.at(field.bln_index), color);
> - add_vertex(fields_to_draw.at(field.brn_index), color);
> + if (color.a > 0) {
> + add_vertex(fields_to_draw.at(current_index),
> color);
> + add_vertex(fields_to_draw.at(field.bln_index),
> color);
> + add_vertex(fields_to_draw.at(field.brn_index),
> color);
> + }
> }
>
> // Right triangle.
> if (field.rn_index != FieldsToDraw::kInvalidIndex) {
> RGBAColor color(0, 0, 0, 0);
> - for (const std::map<Widelands::TCoords<>, uint8_t>&
> wa_map : workarea) {
> - const auto it =
> -
> wa_map.find(Widelands::TCoords<>(field.fcoords, Widelands::TriangleIndex::R));
> - if (it != wa_map.end()) {
> - color = apply_color(color,
> workarea_colors[it->second]);
> + for (const WorkareasEntry& wa_map : workarea) {
You could pull out a common function here that takes the TriangleIndex as
parameters. It could be a lambda function, because it's not being used anywhere
else, like this:
auto do_something = [this](const Field& field, Widelands::TriangleIndex
triangle_index) { // do something];
> + for (const WorkareaPreviewData& data : wa_map) {
> + if (data.coords ==
> Widelands::TCoords<>(field.fcoords, Widelands::TriangleIndex::R)) {
> + RGBAColor color_to_apply =
> workarea_colors[data.index];
> + if (data.use_special_coloring) {
> + color_to_apply =
> apply_color_special(color_to_apply, RGBAColor(data.special_coloring));
> + }
> + color = apply_color(color,
> color_to_apply);
> + }
> }
> }
> - add_vertex(fields_to_draw.at(current_index), color);
> - add_vertex(fields_to_draw.at(field.brn_index), color);
> - add_vertex(fields_to_draw.at(field.rn_index), color);
> + if (color.a > 0) {
> + add_vertex(fields_to_draw.at(current_index),
> color);
> + add_vertex(fields_to_draw.at(field.brn_index),
> color);
> + add_vertex(fields_to_draw.at(field.rn_index),
> color);
> + }
> }
> }
>
>
> === modified file 'src/logic/map_objects/tribes/tribes.cc'
> --- src/logic/map_objects/tribes/tribes.cc 2019-02-23 11:00:49 +0000
> +++ src/logic/map_objects/tribes/tribes.cc 2019-04-28 16:58:57 +0000
> @@ -425,4 +425,13 @@
> }
> }
>
> +uint32_t Tribes::get_largest_workarea() const {
I think that we should calculate it only once during postload() and store it
into a variable.
> + uint32_t largest = 0;
> + for (DescriptionIndex di = 0; di < buildings_->size(); ++di) {
> + for (const auto& pair : buildings_->get(di).workarea_info()) {
> + largest = std::max(largest, pair.first);
> + }
> + }
> + return largest;
> +}
> } // namespace Widelands
>
> === modified file 'src/logic/widelands_geometry.h'
> --- src/logic/widelands_geometry.h 2019-03-11 14:45:04 +0000
> +++ src/logic/widelands_geometry.h 2019-04-28 16:58:57 +0000
> @@ -157,6 +156,25 @@
> };
> } // namespace Widelands
>
> -using Workareas = std::set<std::map<Widelands::TCoords<>, uint8_t>>;
> +struct WorkareaPreviewData {
> + WorkareaPreviewData(Widelands::TCoords<> c, uint8_t i)
> + : coords(c), index(i), use_special_coloring(false),
> special_coloring(0) {
> + }
> + WorkareaPreviewData(Widelands::TCoords<> c, uint8_t i, uint32_t col)
> + : coords(c), index(i), use_special_coloring(true),
> special_coloring(col) {
> + }
> + WorkareaPreviewData()
> + : coords(Widelands::TCoords<>(Widelands::Coords::null(),
> Widelands::TriangleIndex::D)),
> + index(0), use_special_coloring(false), special_coloring(0) {
> + }
> + WorkareaPreviewData& operator=(const WorkareaPreviewData&) = default;
> +
> + Widelands::TCoords<> coords;
> + uint8_t index;
> + bool use_special_coloring;
What makes a coloring special, the overlap? I think we should rename this.
> + uint32_t special_coloring;
> +};
> +using WorkareasEntry = std::vector<WorkareaPreviewData>;
> +using Workareas = std::vector<WorkareasEntry>;
>
> #endif // end of include guard: WL_LOGIC_WIDELANDS_GEOMETRY_H
>
> === modified file 'src/wui/fieldaction.cc'
> --- src/wui/fieldaction.cc 2019-02-23 11:00:49 +0000
> +++ src/wui/fieldaction.cc 2019-04-28 16:58:57 +0000
> @@ -683,17 +695,99 @@
>
> void FieldActionWindow::building_icon_mouse_out(Widelands::DescriptionIndex)
> {
> if (showing_workarea_preview_) {
> - ibase().hide_workarea(node_);
> + ibase().hide_workarea(node_, false);
> + clear_overlapping_workareas();
> showing_workarea_preview_ = false;
> }
> }
>
> void FieldActionWindow::building_icon_mouse_in(const
> Widelands::DescriptionIndex idx) {
> if (!showing_workarea_preview_) {
> - const WorkareaInfo& workarea_info =
> -
> player_->tribe().get_building_descr(Widelands::DescriptionIndex(idx))->workarea_info();
> + assert(overlapping_workareas_.empty());
> + const Widelands::BuildingDescr& descr =
> *player_->tribe().get_building_descr(idx);
> + const WorkareaInfo& workarea_info = descr.workarea_info();
> ibase().show_workarea(workarea_info, node_);
> showing_workarea_preview_ = true;
> +
> + const Widelands::Map& map = ibase().egbase().map();
> + uint32_t workarea_radius = 0;
> + for (const auto& pair : workarea_info) {
> + workarea_radius = std::max(workarea_radius, pair.first);
> + }
> + if (workarea_radius == 0) {
> + return;
> + }
> + std::set<Widelands::TCoords<>> main_region =
> map.triangles_in_region(
> + map.to_set(Widelands::Area<>(node_,
> workarea_radius)));
> +
> + Widelands::MapRegion<Widelands::Area<Widelands::FCoords>>
> mr(map, Widelands::Area<Widelands::FCoords>(
> + node_, workarea_radius +
> ibase().egbase().tribes().get_largest_workarea()));
> + do {
> + if (player_->vision(map.get_index(mr.location())) > 1) {
> + if (Widelands::BaseImmovable* imm =
> mr.location().field->get_immovable()) {
> + const Widelands::MapObjectType imm_type
> = imm->descr().type();
> + if (imm_type <
> Widelands::MapObjectType::BUILDING) {
> + // We are not interested in
> trees and pebbles
> + continue;
> + }
> + const Widelands::BuildingDescr* d =
> nullptr;
> + if (descr.type() ==
> Widelands::MapObjectType::PRODUCTIONSITE) {
> + if (imm->get_owner() !=
> player_) {
> + continue;
> + }
> + if (imm_type !=
> Widelands::MapObjectType::PRODUCTIONSITE) {
Why not just check for imm_type == Widelands::MapObjectType::CONSTRUCTIONSITE
here and lose the inner if?
> + if (imm_type !=
> Widelands::MapObjectType::CONSTRUCTIONSITE) {
> + continue;
> + }
> +
> upcast(Widelands::ConstructionSite, cs, imm);
> + d =
> cs->get_info().becomes;
> + if (d->type() !=
> Widelands::MapObjectType::PRODUCTIONSITE) {
I think we can assume that this is not false, because we checked for
descr.type() == Widelands::MapObjectType::PRODUCTIONSITE above. So, we can skip
the whole upcast and becomes check and shift it below where we assign the
building description that we'll use for the workarea.
> + continue;
> + }
> + }
> + } else if (descr.type() ==
> Widelands::MapObjectType::WAREHOUSE ||
> + descr.type() ==
> Widelands::MapObjectType::MILITARYSITE) {
> + if (imm_type !=
> Widelands::MapObjectType::MILITARYSITE &&
Use same simplification as above. We could even do the check whether it's a
constructionsite below where we assign the building description that we'll use
for the workarea.
> + imm_type !=
> Widelands::MapObjectType::WAREHOUSE) {
> + if (imm_type !=
> Widelands::MapObjectType::CONSTRUCTIONSITE) {
> + continue;
> + }
> +
> upcast(Widelands::ConstructionSite, cs, imm);
> + d =
> cs->get_info().becomes;
> + if (d->type() !=
> Widelands::MapObjectType::WAREHOUSE &&
> +
> d->type() != Widelands::MapObjectType::MILITARYSITE) {
> + continue;
> + }
> + }
> + }
> + upcast(Widelands::Building, bld, imm);
> + if (bld->get_position() !=
> mr.location()) {
> + // Don't count big buildings
> more than once
> + continue;
> + }
> + if (!d) {
> + d = &bld->descr();
> + }
> + const WorkareaInfo& wa =
> d->workarea_info();
> + uint32_t wa_radius = 0;
> + for (const auto& pair : wa) {
> + wa_radius = std::max(wa_radius,
> pair.first);
> + }
> + if (wa_radius == 0) {
> + continue;
> + }
> + if (map.calc_distance(node_,
> mr.location()) <= workarea_radius + wa_radius) {
> + std::map<Widelands::TCoords<>,
> uint32_t> colors;
> + for (const
> Widelands::TCoords<>& t : map.triangles_in_region(
> +
> map.to_set(Widelands::Area<>(mr.location(), wa_radius)))) {
> + colors[t] =
> main_region.count(t) ? 0xbf7f0000 : 0x3fffffff;
> + }
> + ibase().show_workarea(wa,
> mr.location(), colors);
> +
> overlapping_workareas_.insert(mr.location());
> + }
> + }
> + }
> + } while (mr.advance(map));
> }
> }
>
>
> === modified file 'src/wui/interactive_base.cc'
> --- src/wui/interactive_base.cc 2019-04-25 06:31:33 +0000
> +++ src/wui/interactive_base.cc 2019-04-28 16:58:57 +0000
> @@ -196,14 +197,19 @@
> bool InteractiveBase::has_workarea_preview(const Widelands::Coords& coords,
> const Widelands::Map* map) const {
> if (!map) {
> - return workarea_previews_.count(coords) == 1;
> + for (const auto& it : workarea_previews_) {
> + if (it->coords == coords) {
> + return true;
> + }
> + }
> + return false;
> }
> - for (const auto& pair : workarea_previews_) {
> + for (const auto& it : workarea_previews_) {
"it" is a misleading name, because this is not an iterator, but a <key, entry>
pair.
> uint32_t radius = 0;
> - for (const auto& p : *pair.second) {
> - radius = std::max(radius, p.first);
> + for (const auto& wa : *it->info) {
> + radius = std::max(radius, wa.first);
> }
> - if (map->calc_distance(coords, pair.first) <= radius) {
> + if (map->calc_distance(coords, it->coords) <= radius) {
> return true;
> }
> }
> @@ -350,73 +364,100 @@
> }
> }
>
> -Workareas InteractiveBase::get_workarea_overlays(const Widelands::Map& map)
> const {
> - Workareas result_set;
> - for (const auto& wa_pair : workarea_previews_) {
> - std::map<Coords, uint8_t> intermediate_result;
> - const Coords& coords = wa_pair.first;
> - const WorkareaInfo* workarea_info = wa_pair.second;
> - intermediate_result[coords] = 0;
> - WorkareaInfo::size_type wa_index;
> - switch (workarea_info->size()) {
> - case 0:
> - continue; // no workarea
> - case 1:
> - wa_index = 5;
> - break;
> - case 2:
> - wa_index = 3;
> - break;
> - case 3:
> - wa_index = 0;
> - break;
> - default:
> - throw wexception(
> - "Encountered unexpected WorkareaInfo size %i",
> static_cast<int>(workarea_info->size()));
> - }
> -
> - Widelands::HollowArea<> hollow_area(Widelands::Area<>(coords,
> 0), 0);
> -
> - // Iterate through the work areas, from building to its
> enhancement
> - WorkareaInfo::const_iterator it = workarea_info->begin();
> - for (; it != workarea_info->end(); ++it) {
> - hollow_area.radius = it->first;
> - Widelands::MapHollowRegion<> mr(map, hollow_area);
> - do {
> - intermediate_result[mr.location()] = wa_index;
> - } while (mr.advance(map));
> - wa_index++;
> - hollow_area.hole_radius = hollow_area.radius;
> - }
> -
> - std::map<TCoords<>, uint8_t> result;
> - for (const auto& pair : intermediate_result) {
> - Coords c;
> - map.get_brn(pair.first, &c);
> - const auto brn = intermediate_result.find(c);
> - if (brn == intermediate_result.end()) {
> - continue;
> - }
> - map.get_bln(pair.first, &c);
> - const auto bln = intermediate_result.find(c);
> - map.get_rn(pair.first, &c);
> - const auto rn = intermediate_result.find(c);
> - if (bln != intermediate_result.end()) {
> - result[TCoords<>(pair.first,
> Widelands::TriangleIndex::D)] =
> - workarea_max(pair.second, brn->second,
> bln->second);
> - }
> - if (rn != intermediate_result.end()) {
> - result[TCoords<>(pair.first,
> Widelands::TriangleIndex::R)] =
> - workarea_max(pair.second, brn->second,
> rn->second);
> - }
> - }
> - result_set.emplace(result);
> - }
> - return result_set;
> -}
> -
> -void InteractiveBase::hide_workarea(const Widelands::Coords& coords) {
> - workarea_previews_.erase(coords);
> +Workareas InteractiveBase::get_workarea_overlays(const Widelands::Map& map) {
> + if (!workareas_cache_) {
> + workareas_cache_.reset(new Workareas());
> + for (const auto& it : workarea_previews_) {
> + workareas_cache_->push_back(get_workarea_overlay(map,
> *it));
> + }
> + }
> + return Workareas(*workareas_cache_);
> +}
> +
> +// static
Did you intend to make this static? If not, get rid of the comment.
> +WorkareasEntry InteractiveBase::get_workarea_overlay(const Widelands::Map&
> map, const WorkareaPreview& workarea) {
> + std::map<Coords, uint8_t> intermediate_result;
> + const Coords& coords = workarea.coords;
> + const WorkareaInfo* workarea_info = workarea.info;
> + intermediate_result[coords] = 0;
> + WorkareaInfo::size_type wa_index;
> + switch (workarea_info->size()) {
> + case 0:
> + return WorkareasEntry(); // no workarea
> + case 1:
> + wa_index = 5;
> + break;
> + case 2:
> + wa_index = 3;
> + break;
> + case 3:
> + wa_index = 0;
> + break;
> + default:
> + throw wexception(
> + "Encountered unexpected WorkareaInfo size %i",
> static_cast<int>(workarea_info->size()));
> + }
> +
> + Widelands::HollowArea<> hollow_area(Widelands::Area<>(coords, 0), 0);
> +
> + // Iterate through the work areas, from building to its enhancement
> + WorkareaInfo::const_iterator it = workarea_info->begin();
> + for (; it != workarea_info->end(); ++it) {
> + hollow_area.radius = it->first;
> + Widelands::MapHollowRegion<> mr(map, hollow_area);
> + do {
> + intermediate_result[mr.location()] = wa_index;
> + } while (mr.advance(map));
> + wa_index++;
> + hollow_area.hole_radius = hollow_area.radius;
> + }
> +
> + WorkareasEntry result;
> + for (const auto& pair : intermediate_result) {
> + Coords c;
> + map.get_brn(pair.first, &c);
> + const auto brn = intermediate_result.find(c);
> + if (brn == intermediate_result.end()) {
> + continue;
> + }
> + map.get_bln(pair.first, &c);
> + const auto bln = intermediate_result.find(c);
> + map.get_rn(pair.first, &c);
> + const auto rn = intermediate_result.find(c);
> + if (bln != intermediate_result.end()) {
> + TCoords<> tc(pair.first, Widelands::TriangleIndex::D);
> + WorkareaPreviewData wd(tc, workarea_max(pair.second,
> brn->second, bln->second));
> + for (const auto& p : workarea.data) {
> + if (p.first == tc) {
> + wd = WorkareaPreviewData(tc, wd.index,
> p.second);
> + break;
> + }
> + }
> + result.push_back(wd);
> + }
> + if (rn != intermediate_result.end()) {
> + TCoords<> tc(pair.first, Widelands::TriangleIndex::R);
> + WorkareaPreviewData wd(tc, workarea_max(pair.second,
> brn->second, rn->second));
> + for (const auto& p : workarea.data) {
> + if (p.first == tc) {
> + wd = WorkareaPreviewData(tc, wd.index,
> p.second);
> + break;
> + }
> + }
> + result.push_back(wd);
> + }
> + }
> + return result;
> +}
> +
> +void InteractiveBase::hide_workarea(const Widelands::Coords& coords, bool
> is_additional) {
> + for (auto it = workarea_previews_.begin(); it !=
> workarea_previews_.end(); ++it) {
> + if (it->get()->coords == coords && (is_additional ^
> it->get()->data.empty())) {
> + workarea_previews_.erase(it);
> + workareas_cache_.reset(nullptr);
> + return;
> + }
> + }
> }
>
> /**
--
https://code.launchpad.net/~widelands-dev/widelands/overlapping_workareas/+merge/366623
Your team Widelands Developers is requested to review the proposed merge of
lp:~widelands-dev/widelands/overlapping_workareas 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
