Skip to content

Commit

Permalink
Sort unit lists shown to the user
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
lmoureaux committed May 21, 2024
1 parent 65aa567 commit 46ebd5e
Show file tree
Hide file tree
Showing 8 changed files with 128 additions and 94 deletions.
1 change: 1 addition & 0 deletions client/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
77 changes: 3 additions & 74 deletions client/citydlg.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
// utility
#include "fc_types.h"
#include "fcintl.h"
#include "player.h"
#include "support.h"
// common
#include "citizens.h"
Expand Down Expand Up @@ -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"
Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -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
*/
Expand Down
2 changes: 1 addition & 1 deletion client/text.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion client/text.h
Original file line number Diff line number Diff line change
Expand Up @@ -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<unit *> &units,
const struct city *pdest);
const QString science_dialog_text();
Expand Down
24 changes: 10 additions & 14 deletions client/unitselect.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down Expand Up @@ -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;
Expand All @@ -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));
Expand Down Expand Up @@ -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);
}
Expand All @@ -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();

Expand All @@ -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"))
Expand All @@ -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);
}
Expand Down Expand Up @@ -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();
}
}
Expand Down
9 changes: 5 additions & 4 deletions client/unitselect.h
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@

#include <QMenu>

#include <vector>

class QPixmap;
class QSize;
class QFont;
Expand All @@ -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 *> 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 *> unit_list; ///< units currently visible
QFont ufont;
QFont info_font;
int column_count, row_count;
Expand Down
96 changes: 96 additions & 0 deletions client/utils/unit_utils.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
// SPDX-FileCopyrightText: Louis Moureaux <m_louis30@yahoo.com>
// SPDX-License-Identifier: GPLv3-or-later

#include "unit_utils.h"

#include "player.h"
#include "unit.h"
#include "unitlist.h"

#include <algorithm>

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<unit *> sorted(const unit_list *units)
{
std::vector<unit *> vec;
unit_list_iterate(units, unit) { vec.push_back(unit); }
unit_list_iterate_end;

std::sort(vec.begin(), vec.end(), units_sort);

return vec;
}
11 changes: 11 additions & 0 deletions client/utils/unit_utils.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
// SPDX-FileCopyrightText: Louis Moureaux <m_louis30@yahoo.com>
// SPDX-License-Identifier: GPLv3-or-later

#pragma once

#include <vector>

struct unit;
struct unit_list;

std::vector<unit *> sorted(const unit_list *units);

0 comments on commit 46ebd5e

Please sign in to comment.