From 46ebd5e717841386286997079170251c1ecb6c37 Mon Sep 17 00:00:00 2001 From: Louis Moureaux Date: Tue, 21 May 2024 01:59:35 +0200 Subject: [PATCH] Sort unit lists shown to the user This patch makes the user interface look more consistent by sorting units in two contexts: * In the city dialog * In the unit picker The sorting logic is an improved version of code formerly used to sort units in the city dialog (but that had been lost for some reasion). It tries to produce a sort order that looks natural and is stable in time. To do so: * Loaded units are always sorted right after their transport, so one can tell in which transport they are loaded; * The best defensive units go to the beginning of the list. These tend to stay in cities and not move around too much; * Tie breaker by unit type and nationality ensure similar units are grouped together; * Finally, the unit id is used as a last resort. I tested this logic in a few contexts and it seems to achieve its goal. With respect to the previous sorting function, I removed an order based on unit activities (fortified, fortifying, sentried) because it felt wrong to have a unit move in the list just because I changed its activity. Closes #1836. See #1838. --- client/CMakeLists.txt | 1 + client/citydlg.cpp | 77 ++--------------------------- client/text.cpp | 2 +- client/text.h | 2 +- client/unitselect.cpp | 24 ++++------ client/unitselect.h | 9 ++-- client/utils/unit_utils.cpp | 96 +++++++++++++++++++++++++++++++++++++ client/utils/unit_utils.h | 11 +++++ 8 files changed, 128 insertions(+), 94 deletions(-) create mode 100644 client/utils/unit_utils.cpp create mode 100644 client/utils/unit_utils.h diff --git a/client/CMakeLists.txt b/client/CMakeLists.txt index e735b6e846..c7a2b4744c 100644 --- a/client/CMakeLists.txt +++ b/client/CMakeLists.txt @@ -124,6 +124,7 @@ target_sources( utils/colorizer.cpp utils/improvement_seller.cpp utils/unit_quick_menu.cpp + utils/unit_utils.cpp views/view_cities.cpp views/view_cities_data.cpp views/view_economics.cpp diff --git a/client/citydlg.cpp b/client/citydlg.cpp index 8e56d80e2a..99a6789b65 100644 --- a/client/citydlg.cpp +++ b/client/citydlg.cpp @@ -26,6 +26,7 @@ // utility #include "fc_types.h" #include "fcintl.h" +#include "player.h" #include "support.h" // common #include "citizens.h" @@ -54,6 +55,7 @@ #include "unitlist.h" #include "utils/improvement_seller.h" #include "utils/unit_quick_menu.h" +#include "utils/unit_utils.h" #include "views/view_cities.h" // hIcon #include "views/view_map.h" #include "views/view_map_common.h" @@ -102,8 +104,7 @@ void unit_list_widget::set_units(unit_list *units) clear(); QSize icon_size; - unit_list_iterate(units, punit) - { + for (const auto *punit : sorted(units)) { auto *item = new QListWidgetItem(); item->setToolTip(unit_description(punit)); item->setData(Qt::UserRole, punit->id); @@ -113,7 +114,6 @@ void unit_list_widget::set_units(unit_list *units) item->setIcon(QIcon(pixmap)); addItem(item); } - unit_list_iterate_end; setGridSize(icon_size); setIconSize(icon_size); @@ -1763,77 +1763,6 @@ void city_dialog::dbl_click_p(QTableWidgetItem *item) city_set_queue(pcity, &queue); } -namespace /* anonymous */ { -/** - * Finds how deeply the unit is nested in transports. - */ -int transport_depth(const unit *unit) -{ - int depth = 0; - for (auto parent = unit->transporter; parent != nullptr; - parent = parent->transporter) { - depth++; - } - return depth; -} - -/** - * Comparison function to sort units as shown in the city dialog. - */ -int units_sort(const unit *const *plhs, const unit *const *prhs) -{ - if (plhs == prhs || *plhs == *prhs) { - return 0; - } - - auto lhs = *plhs; - auto rhs = *prhs; - - // Transports are shown before the units they transport. - if (lhs == rhs->transporter) { - return false; - } else if (lhs->transporter == rhs) { - return true; - } - - // When one unit is deeper or the two transporters are different, compare - // the parents instead. - int lhs_depth = transport_depth(lhs); - int rhs_depth = transport_depth(rhs); - if (lhs_depth > rhs_depth) { - return units_sort(&lhs->transporter, &rhs); - } else if (lhs_depth < rhs_depth) { - return units_sort(&lhs, &rhs->transporter); - } else if (lhs->transporter != rhs->transporter) { - return units_sort(&lhs->transporter, &rhs->transporter); - } - - // Put defensive units on the left - if (lhs->utype->defense_strength != rhs->utype->defense_strength) { - return rhs->utype->defense_strength - lhs->utype->defense_strength; - } - - // Put fortified units on the left, then fortifying units, then sentried - // units. - for (auto activity : - {ACTIVITY_FORTIFIED, ACTIVITY_FORTIFYING, ACTIVITY_SENTRY}) { - if (lhs->activity == activity && rhs->activity != activity) { - return false; - } else if (lhs->activity != activity && rhs->activity == activity) { - return true; - } - } - - // Order by unit type - if (lhs->utype != rhs->utype) { - return lhs->utype->item_number - rhs->utype->item_number; - } - - // Then unit id - return lhs->id - rhs->id; -} -} // anonymous namespace - /** Updates layouts for supported and present units in city */ diff --git a/client/text.cpp b/client/text.cpp index ff275aaa48..907e787624 100644 --- a/client/text.cpp +++ b/client/text.cpp @@ -548,7 +548,7 @@ const QString get_nearest_city_text(struct city *pcity, int sq_dist) FIXME: This function is not re-entrant because it returns a pointer to static data. */ -const QString unit_description(struct unit *punit) +const QString unit_description(const unit *punit) { int pcity_near_dist; struct player *owner = unit_owner(punit); diff --git a/client/text.h b/client/text.h index 91202e7c49..e9ee1aa605 100644 --- a/client/text.h +++ b/client/text.h @@ -24,7 +24,7 @@ struct player_spaceship; const QString get_tile_output_text(const struct tile *ptile); const QString popup_info_text(struct tile *ptile); const QString get_nearest_city_text(struct city *pcity, int sq_dist); -const QString unit_description(struct unit *punit); +const QString unit_description(const unit *punit); const QString get_airlift_text(const std::vector &units, const struct city *pdest); const QString science_dialog_text(); diff --git a/client/unitselect.cpp b/client/unitselect.cpp index a837340353..d4a2199672 100644 --- a/client/unitselect.cpp +++ b/client/unitselect.cpp @@ -26,6 +26,7 @@ #include "fonts.h" #include "page_game.h" #include "tileset/tilespec.h" +#include "utils/unit_utils.h" #include "views/view_map.h" #include "views/view_map_common.h" @@ -76,7 +77,7 @@ void units_select::create_pixmap() QPixmap *tmp_pix; QRect crop; QPixmap *unit_pixmap; - struct unit *punit; + const unit *punit; float isosize; delete pix; @@ -86,7 +87,7 @@ void units_select::create_pixmap() } update_units(); - if (unit_list.count() > 0) { + if (!unit_list.empty()) { if (!tileset_is_isometric(tileset)) { item_size.setWidth(tileset_unit_width(tileset)); item_size.setHeight(tileset_unit_width(tileset)); @@ -211,14 +212,12 @@ void units_select::mouseMoveEvent(QMouseEvent *event) */ void units_select::mousePressEvent(QMouseEvent *event) { - struct unit *punit; if (event->button() == Qt::LeftButton && highligh_num != -1) { update_units(); - if (highligh_num >= unit_list.count()) { + if (highligh_num >= unit_list.size()) { return; } - punit = unit_list.at(highligh_num); - unit_focus_set(punit); + unit_focus_set(unit_list.at(highligh_num)); } QMenu::mousePressEvent(event); } @@ -234,7 +233,6 @@ void units_select::paint(QPainter *painter, QPaintEvent *event) int *f_size; QPen pen; QString str, str2, unit_name; - struct unit *punit; int point_size = info_font.pointSize(); int pixel_size = info_font.pixelSize(); @@ -243,8 +241,8 @@ void units_select::paint(QPainter *painter, QPaintEvent *event) } else { f_size = &point_size; } - if (highligh_num != -1 && highligh_num < unit_list.count()) { - punit = unit_list.at(highligh_num); + if (highligh_num != -1 && highligh_num < unit_list.size()) { + auto punit = unit_list.at(highligh_num); // TRANS: HP - hit points unit_name = unit_name_translation(punit); str2 = QString(_("%1 HP:%2/%3")) @@ -271,7 +269,7 @@ void units_select::paint(QPainter *painter, QPaintEvent *event) painter->setPen(pen); painter->setFont(info_font); painter->drawText(10, h, str); - if (highligh_num != -1 && highligh_num < unit_list.count()) { + if (highligh_num != -1 && highligh_num < unit_list.size()) { painter->drawText(10, height() - 5 - h, unit_name); painter->drawText(10, height() - 5, str2); } @@ -332,18 +330,16 @@ void units_select::update_units() if (utile != nullptr) { punit_list = utile->units; if (punit_list != nullptr) { - unit_list_iterate(utile->units, punit) - { + for (auto *punit : sorted(utile->units)) { unit_count++; if (i > show_line * column_count) { unit_list.push_back(punit); } i++; } - unit_list_iterate_end; } } - if (unit_list.count() == 0) { + if (unit_list.empty()) { close(); } } diff --git a/client/unitselect.h b/client/unitselect.h index a3411add07..097ae43bf1 100644 --- a/client/unitselect.h +++ b/client/unitselect.h @@ -14,6 +14,8 @@ #include +#include + class QPixmap; class QSize; class QFont; @@ -27,10 +29,9 @@ class units_select : public QMenu { Q_OBJECT Q_DISABLE_COPY(units_select); QPixmap *pix; - QPixmap *h_pix; /** pixmap for highlighting */ - QSize item_size; /** size of each pixmap of unit */ - QList unit_list; /** storing units only for drawing, for rest units - * iterate utile->units */ + QPixmap *h_pix; /** pixmap for highlighting */ + QSize item_size; /** size of each pixmap of unit */ + std::vector unit_list; ///< units currently visible QFont ufont; QFont info_font; int column_count, row_count; diff --git a/client/utils/unit_utils.cpp b/client/utils/unit_utils.cpp new file mode 100644 index 0000000000..30307cfefd --- /dev/null +++ b/client/utils/unit_utils.cpp @@ -0,0 +1,96 @@ +// SPDX-FileCopyrightText: Louis Moureaux +// SPDX-License-Identifier: GPLv3-or-later + +#include "unit_utils.h" + +#include "player.h" +#include "unit.h" +#include "unitlist.h" + +#include + +namespace /* anonymous */ { +/** + * Finds how deeply the unit is nested in transports. + */ +int transport_depth(const unit *unit) +{ + int depth = 0; + for (auto parent = unit->transporter; parent != nullptr; + parent = parent->transporter) { + depth++; + } + return depth; +} + +/** + * Comparison function to sort units. Used in the city dialog and the unit + * selector. + */ +bool units_sort(const unit *lhs, const unit *rhs) +{ + if (lhs == rhs) { + return 0; + } + + // Transports are shown before the units they transport. + if (lhs == rhs->transporter) { + return true; + } else if (lhs->transporter == rhs) { + return false; + } + + // When one unit is deeper or the two transporters are different, compare + // the parents instead. + int lhs_depth = transport_depth(lhs); + int rhs_depth = transport_depth(rhs); + if (lhs_depth > rhs_depth) { + return units_sort(lhs->transporter, rhs); + } else if (lhs_depth < rhs_depth) { + return units_sort(lhs, rhs->transporter); + } else if (lhs->transporter != rhs->transporter) { + return units_sort(lhs->transporter, rhs->transporter); + } + + // Put defensive units on the left - these are least likely to move, having + // them first makes the list more stable + const auto lhs_def = lhs->utype->defense_strength * lhs->utype->hp; + const auto rhs_def = rhs->utype->defense_strength * rhs->utype->hp; + if (lhs_def != rhs_def) { + return lhs_def > rhs_def; + } + + // More veteran units further left + if (lhs->veteran != rhs->veteran) { + return lhs->veteran > rhs->veteran; + } + + // Reverse order by unit type - in most ruleset this means old units last + if (lhs->utype != rhs->utype) { + return lhs->utype->item_number > rhs->utype->item_number; + } + + // By nationality so units from the same player are grouped together + if (player_index(lhs->nationality) != player_index(rhs->nationality)) { + return player_index(lhs->nationality) < player_index(rhs->nationality); + } + + // Then unit id + return lhs->id < rhs->id; +} +} // anonymous namespace + +/** + * Returns a version of \c units sorted in the way the user would like to see + * them. + */ +std::vector sorted(const unit_list *units) +{ + std::vector vec; + unit_list_iterate(units, unit) { vec.push_back(unit); } + unit_list_iterate_end; + + std::sort(vec.begin(), vec.end(), units_sort); + + return vec; +} diff --git a/client/utils/unit_utils.h b/client/utils/unit_utils.h new file mode 100644 index 0000000000..4d6175a807 --- /dev/null +++ b/client/utils/unit_utils.h @@ -0,0 +1,11 @@ +// SPDX-FileCopyrightText: Louis Moureaux +// SPDX-License-Identifier: GPLv3-or-later + +#pragma once + +#include + +struct unit; +struct unit_list; + +std::vector sorted(const unit_list *units);