From 2791a180ea96758abeff1e05abba054267b4efc0 Mon Sep 17 00:00:00 2001 From: Louis Moureaux Date: Fri, 25 Oct 2024 22:26:09 +0200 Subject: [PATCH 1/2] Fix a crash when there are many UWT units The Units view (F2) was crashing when there were more than MAX_NUM_UNITS (=250) units under UWT. This was the consequence of using a fixed-size array. Fix by switching to std::vector. Reported by Corbeau. --- client/views/view_units.cpp | 65 +++++++++++++++---------------------- client/views/view_units.h | 3 +- 2 files changed, 28 insertions(+), 40 deletions(-) diff --git a/client/views/view_units.cpp b/client/views/view_units.cpp index b3b32a4f59..d7eece6f90 100644 --- a/client/views/view_units.cpp +++ b/client/views/view_units.cpp @@ -355,10 +355,7 @@ void units_view::update_waiting() QFontMetrics fm(f); int h = fm.height() + 24; - struct unit_waiting_entry unit_entries[U_LAST]; - - int entries_used = 0; - get_units_waiting_data(unit_entries, &entries_used); + auto unit_entries = get_units_waiting_data(); max_row = ui.uwt_table->rowCount(); @@ -372,12 +369,12 @@ void units_view::update_waiting() ids_in_table[item->text()] = item; } - for (int i = 0; i < entries_used; i++) { - struct unit_waiting_entry *pentry = unit_entries + i; - const struct unit_type *putype = pentry->type; + for (int i = 0; i < unit_entries.size(); i++) { + const unit_waiting_entry &entry = unit_entries[i]; + const struct unit_type *putype = entry.type; cid id = cid_encode_unit(putype); - QString unit_id = QString("%1").arg(pentry->id); - QString unit_waittime = format_simple_duration(abs(pentry->timer)); + QString unit_id = QString("%1").arg(entry.id); + QString unit_waittime = format_simple_duration(abs(entry.timer)); if (!ids_in_table.contains(unit_id)) { // Create a new row for the unit @@ -406,7 +403,7 @@ void units_view::update_waiting() case 2: // # Location item->setTextAlignment(Qt::AlignLeft | Qt::AlignVCenter); - item->setText(QString(_("%1")).arg(pentry->city_name)); + item->setText(QString(_("%1")).arg(entry.city_name)); break; case 3: // # Time Left @@ -721,47 +718,37 @@ void get_units_view_data(struct unit_view_entry *entries, /** * Returns an array of units subject to unitwaittime. */ -void get_units_waiting_data(struct unit_waiting_entry *entries, - int *num_entries_used) +std::vector get_units_waiting_data() { - *num_entries_used = 0; - if (nullptr == client.conn.playing) { - return; + return {}; } - int pcity_near_dist = 0; // Init distance + auto entries = std::vector(); unit_list_iterate(client.conn.playing->units, punit) { + int pcity_near_dist = 0; // Init distance struct city *pcity_near = get_nearest_city(punit, &pcity_near_dist); if (!can_unit_move_now(punit) && punit->ssa_controller == SSA_NONE) { - - entries[*num_entries_used].type = punit->utype; - entries[*num_entries_used].city_name = - get_nearest_city_text(pcity_near, pcity_near_dist); - entries[*num_entries_used].timer = - time(nullptr) - punit->action_timestamp; - entries[*num_entries_used].id = punit->id; - - (*num_entries_used)++; - } - - // Skip unused unit types - if (*num_entries_used == 0) { - continue; + entries.push_back( + {.type = punit->utype, + .timer = time(nullptr) - punit->action_timestamp, + .city_name = get_nearest_city_text(pcity_near, pcity_near_dist), + .id = punit->id}); } } unit_list_iterate_end; - std::sort(entries, entries + *num_entries_used, - [](const auto &lhs, const auto &rhs) { - return QString::localeAwareCompare( - utype_name_translation(lhs.type), - utype_name_translation(rhs.type)) - < 0; - }); + std::sort( + entries.begin(), entries.end(), [](const auto &lhs, const auto &rhs) { + return QString::localeAwareCompare(utype_name_translation(lhs.type), + utype_name_translation(rhs.type)) + < 0; + }); + + return entries; } /************************************ @@ -782,7 +769,9 @@ void units_view_dialog_update(void *unused) if (queen()->game_tab_widget->currentIndex() == i) { w = queen()->game_tab_widget->widget(i); uv = reinterpret_cast(w); - uv->update_view(); + if (uv) { + uv->update_view(); + } } } queen()->updateSidebarTooltips(); diff --git a/client/views/view_units.h b/client/views/view_units.h index 26e8b9685f..608fa7cccc 100644 --- a/client/views/view_units.h +++ b/client/views/view_units.h @@ -42,8 +42,7 @@ struct unit_waiting_entry { void get_units_view_data(struct unit_view_entry *entries, int *num_entries_used); -void get_units_waiting_data(struct unit_waiting_entry *entries, - int *num_entries_used); +std::vector get_units_waiting_data(); void units_view_dialog_update(void *unused); From 8a6dc923ed9d7848b025f91463aecce678bf4f7e Mon Sep 17 00:00:00 2001 From: Louis Moureaux Date: Fri, 25 Oct 2024 22:30:38 +0200 Subject: [PATCH 2/2] Switch get_units_view_data() to use std::vector This keeps the interface consistent with get_units_waiting_data() and is generally safer. --- client/views/view_units.cpp | 73 +++++++++++++++++-------------------- client/views/view_units.h | 5 +-- 2 files changed, 35 insertions(+), 43 deletions(-) diff --git a/client/views/view_units.cpp b/client/views/view_units.cpp index d7eece6f90..9d388d3f9b 100644 --- a/client/views/view_units.cpp +++ b/client/views/view_units.cpp @@ -150,10 +150,7 @@ void units_view::update_units() QFontMetrics fm(f); int h = fm.height() + 24; - int entries_used = 0; // Position in the units array - struct unit_view_entry unit_entries[U_LAST]; - - get_units_view_data(unit_entries, &entries_used); + auto unit_entries = get_units_view_data(); // Variables for the nested loops int total_count = 0; // Sum of unit type @@ -185,9 +182,9 @@ void units_view::update_units() unittypes_in_table[key] = ui.units_table->item(r, 0); } - for (int i = 0; i < entries_used; i++) { - struct unit_view_entry *pentry = unit_entries + i; - const struct unit_type *putype = pentry->type; + for (int i = 0; i < unit_entries.size(); i++) { + const unit_view_entry &entry = unit_entries[i]; + const struct unit_type *putype = entry.type; cid id = cid_encode_unit(putype); auto existing_row_for_unittype = unittypes_in_table.find(id); @@ -223,7 +220,7 @@ void units_view::update_units() case 2: // Is Upgradable item->setTextAlignment(Qt::AlignVCenter | Qt::AlignHCenter); - if (pentry->upg) { + if (entry.upg) { item->setData(Qt::DisplayRole, "★"); upg_count++; } else { @@ -233,44 +230,44 @@ void units_view::update_units() case 3: // # In Progress item->setTextAlignment(Qt::AlignVCenter | Qt::AlignHCenter); - item->setData(Qt::DisplayRole, pentry->in_prod); - in_progress += pentry->in_prod; + item->setData(Qt::DisplayRole, entry.in_prod); + in_progress += entry.in_prod; break; case 4: // # Active item->setTextAlignment(Qt::AlignVCenter | Qt::AlignHCenter); - item->setData(Qt::DisplayRole, pentry->count); - total_count += pentry->count; + item->setData(Qt::DisplayRole, entry.count); + total_count += entry.count; break; case 5: // Shield upkeep item->setTextAlignment(Qt::AlignVCenter | Qt::AlignHCenter); - if (pentry->shield_cost == 0) { + if (entry.shield_cost == 0) { item->setText("-"); } else { - item->setData(Qt::DisplayRole, pentry->shield_cost); + item->setData(Qt::DisplayRole, entry.shield_cost); } - total_shield += pentry->shield_cost; + total_shield += entry.shield_cost; break; case 6: // Food upkeep item->setTextAlignment(Qt::AlignVCenter | Qt::AlignHCenter); - if (pentry->food_cost == 0) { + if (entry.food_cost == 0) { item->setText("-"); } else { - item->setData(Qt::DisplayRole, pentry->food_cost); + item->setData(Qt::DisplayRole, entry.food_cost); } - total_food += pentry->food_cost; + total_food += entry.food_cost; break; case 7: // Gold upkeep item->setTextAlignment(Qt::AlignVCenter | Qt::AlignHCenter); - if (pentry->gold_cost == 0) { + if (entry.gold_cost == 0) { item->setText("-"); } else { - item->setData(Qt::DisplayRole, pentry->gold_cost); + item->setData(Qt::DisplayRole, entry.gold_cost); } - total_gold += pentry->gold_cost; + total_gold += entry.gold_cost; break; } ui.units_table->setItem(current_row, j, item); @@ -641,10 +638,9 @@ struct unit *find_nearest_unit(const struct unit_type *utype, /** * Returns an array of units data. */ -void get_units_view_data(struct unit_view_entry *entries, - int *num_entries_used) +std::vector get_units_view_data() { - *num_entries_used = 0; + auto entries = std::vector(); players_iterate(pplayer) { @@ -693,26 +689,25 @@ void get_units_view_data(struct unit_view_entry *entries, continue; } - entries[*num_entries_used].type = unittype; - entries[*num_entries_used].count = count; - entries[*num_entries_used].in_prod = in_progress; - entries[*num_entries_used].upg = upgradable; - entries[*num_entries_used].gold_cost = gold_cost; - entries[*num_entries_used].food_cost = food_cost; - entries[*num_entries_used].shield_cost = shield_cost; - (*num_entries_used)++; + entries.push_back({.type = unittype, + .count = count, + .in_prod = in_progress, + .food_cost = food_cost, + .gold_cost = gold_cost, + .shield_cost = shield_cost, + .upg = upgradable}); } unit_type_iterate_end; } players_iterate_end; - std::sort(entries, entries + *num_entries_used, - [](const auto &lhs, const auto &rhs) { - return QString::localeAwareCompare( - utype_name_translation(lhs.type), - utype_name_translation(rhs.type)) - < 0; - }); + std::sort( + entries.begin(), entries.end(), [](const auto &lhs, const auto &rhs) { + return QString::localeAwareCompare(utype_name_translation(lhs.type), + utype_name_translation(rhs.type)) + < 0; + }); + return entries; } /** diff --git a/client/views/view_units.h b/client/views/view_units.h index 608fa7cccc..34d3f6669e 100644 --- a/client/views/view_units.h +++ b/client/views/view_units.h @@ -27,6 +27,7 @@ struct unit_view_entry { int count, in_prod, total_cost, food_cost, gold_cost, shield_cost; bool upg; }; +std::vector get_units_view_data(); /** * Structure of unit waiting data for the Units View. @@ -38,10 +39,6 @@ struct unit_waiting_entry { QString city_name; int id; }; - -void get_units_view_data(struct unit_view_entry *entries, - int *num_entries_used); - std::vector get_units_waiting_data(); void units_view_dialog_update(void *unused);