From 924fb820588eb9e156aa3fb144c2c265111fe994 Mon Sep 17 00:00:00 2001 From: Chris Beck Date: Sun, 28 Dec 2014 13:15:42 -0500 Subject: [PATCH] partially fix 23076 (don't hold std::vector in recall dialog) After this commit 9e7dc5ba008298d823c13cff214f7477601b5efd which introduced reference counted unit pointers, the recall dialog began to fail assertions. The problem was that it was making private copies of all units, and when units get deleted this list got out of sync. I'm not totally sure exactly what mechanism was making it work before, but this commit which shares the units list between the different gui elements prevents the assertion failure. Note that there is still a bug, in that the unit preview pane at left still gets out of sync somehow... --- src/actions/attack.cpp | 40 ++++++++++++++++++++-------------------- src/actions/attack.hpp | 4 ++-- src/dialogs.cpp | 26 ++++++++++---------------- src/dialogs.hpp | 3 --- 4 files changed, 32 insertions(+), 41 deletions(-) diff --git a/src/actions/attack.cpp b/src/actions/attack.cpp index 6ce3d9e9b6ec..43f8c292586c 100644 --- a/src/actions/attack.cpp +++ b/src/actions/attack.cpp @@ -1528,22 +1528,22 @@ void attack_unit_and_advance(const map_location &attacker, const map_location &d } -unit get_advanced_unit(const unit &u, const std::string& advance_to) +unit_ptr get_advanced_unit(const unit &u, const std::string& advance_to) { const unit_type *new_type = unit_types.find(advance_to); if (!new_type) { throw game::game_error("Could not find the unit being advanced" " to: " + advance_to); } - unit new_unit(u); - new_unit.set_experience(new_unit.experience() - new_unit.max_experience()); - new_unit.advance_to(*new_type); - new_unit.heal_all(); - new_unit.set_state(unit::STATE_POISONED, false); - new_unit.set_state(unit::STATE_SLOWED, false); - new_unit.set_state(unit::STATE_PETRIFIED, false); - new_unit.set_user_end_turn(false); - new_unit.set_hidden(false); + unit_ptr new_unit(new unit(u)); + new_unit->set_experience(new_unit->experience() - new_unit->max_experience()); + new_unit->advance_to(*new_type); + new_unit->heal_all(); + new_unit->set_state(unit::STATE_POISONED, false); + new_unit->set_state(unit::STATE_SLOWED, false); + new_unit->set_state(unit::STATE_PETRIFIED, false); + new_unit->set_user_end_turn(false); + new_unit->set_hidden(false); return new_unit; } @@ -1551,11 +1551,11 @@ unit get_advanced_unit(const unit &u, const std::string& advance_to) /** * Returns the AMLA-advanced version of a unit (with traits and items retained). */ -unit get_amla_unit(const unit &u, const config &mod_option) +unit_ptr get_amla_unit(const unit &u, const config &mod_option) { - unit amla_unit(u); - amla_unit.set_experience(amla_unit.experience() - amla_unit.max_experience()); - amla_unit.add_modification("advance", mod_option); + unit_ptr amla_unit(new unit(u)); + amla_unit->set_experience(amla_unit->experience() - amla_unit->max_experience()); + amla_unit->add_modification("advance", mod_option); return amla_unit; } @@ -1593,19 +1593,19 @@ void advance_unit(map_location loc, const std::string &advance_to, // Create the advanced unit. bool use_amla = mod_option != NULL; - unit new_unit = use_amla ? get_amla_unit(*u, *mod_option) : + unit_ptr new_unit = use_amla ? get_amla_unit(*u, *mod_option) : get_advanced_unit(*u, advance_to); if ( !use_amla ) { - statistics::advance_unit(new_unit); - preferences::encountered_units().insert(new_unit.type_id()); - LOG_CF << "Added '" << new_unit.type_id() << "' to the encountered units.\n"; + statistics::advance_unit(*new_unit); + preferences::encountered_units().insert(new_unit->type_id()); + LOG_CF << "Added '" << new_unit->type_id() << "' to the encountered units.\n"; } - u = resources::units->replace(loc, new_unit).first; + u = resources::units->replace(loc, *new_unit).first; // Update fog/shroud. actions::shroud_clearer clearer; - clearer.clear_unit(loc, new_unit); + clearer.clear_unit(loc, *new_unit); // "post_advance" event. if(fire_event) diff --git a/src/actions/attack.hpp b/src/actions/attack.hpp index ab16c6d7c89d..99fd324342fb 100644 --- a/src/actions/attack.hpp +++ b/src/actions/attack.hpp @@ -230,12 +230,12 @@ void advance_unit_at(const advance_unit_params& params); /** * Returns the advanced version of a unit (with traits and items retained). */ -unit get_advanced_unit(const unit &u, const std::string &advance_to); +unit_ptr get_advanced_unit(const unit &u, const std::string &advance_to); /** * Returns the AMLA-advanced version of a unit (with traits and items retained). */ -unit get_amla_unit(const unit &u, const config &mod_option); +unit_ptr get_amla_unit(const unit &u, const config &mod_option); /** * Function which will advance the unit at @a loc to 'advance_to'. diff --git a/src/dialogs.cpp b/src/dialogs.cpp index 5ba2dda0f8cc..1482b1bc86b0 100644 --- a/src/dialogs.cpp +++ b/src/dialogs.cpp @@ -91,13 +91,13 @@ namespace class delete_recall_unit : public gui::dialog_button_action { public: - delete_recall_unit(display& disp, gui::filter_textbox& filter, const std::vector& units) : disp_(disp), filter_(filter), units_(units) {} + delete_recall_unit(display& disp, gui::filter_textbox& filter, std::vector& units) : disp_(disp), filter_(filter), units_(units) {} private: gui::dialog_button_action::RESULT button_pressed(int menu_selection); display& disp_; gui::filter_textbox& filter_; - const std::vector& units_; + std::vector& units_; }; template void dump(const T & units) @@ -144,6 +144,9 @@ gui::dialog_button_action::RESULT delete_recall_unit::button_pressed(int menu_se return gui::CONTINUE_DIALOG; } } + // Remove the item from our dialog's list + units_.erase(units_.begin() + index); + // Remove the item from filter_textbox memory filter_.delete_item(menu_selection); //add dismissal to the undo stack @@ -178,10 +181,10 @@ int advance_unit_dialog(const map_location &loc) std::vector lang_options; - std::vector sample_units; + std::vector sample_units; for(std::vector::const_iterator op = options.begin(); op != options.end(); ++op) { sample_units.push_back(::get_advanced_unit(*u, *op)); - const unit& type = sample_units.back(); + const unit& type = *sample_units.back(); #ifdef LOW_MEM lang_options.push_back(IMAGE_PREFIX @@ -198,7 +201,7 @@ int advance_unit_dialog(const map_location &loc) { if (mod["always_display"].to_bool()) always_display = true; sample_units.push_back(::get_amla_unit(*u, mod)); - const unit& type = sample_units.back(); + const unit& type = *sample_units.back(); if (!mod["image"].empty()) { lang_options.push_back(IMAGE_PREFIX + mod["image"].str() + COLUMN_SEPARATOR + mod["description"].str()); } else { @@ -313,7 +316,7 @@ void show_unit_list(display& gui) items.push_back(heading); std::vector locations_list; - std::vector units_list; + std::vector units_list; int selected = 0; @@ -395,7 +398,7 @@ void show_unit_list(display& gui) items.push_back(row.str()); locations_list.push_back(i->get_location()); - units_list.push_back(*i); + units_list.push_back(i.get_shared_ptr()); } { @@ -1394,15 +1397,6 @@ units_list_preview_pane::units_list_preview_pane(const std::vector &units, - const gui::filter_textbox* filter, TYPE type, bool on_left_side) : - unit_preview_pane(filter, type, on_left_side), - units_(units.size()) -{ - for (unsigned i = 0; i < units.size(); ++i) - units_[i] = unit_const_ptr (new unit(units[i])); -} - size_t units_list_preview_pane::size() const { return units_.size(); diff --git a/src/dialogs.hpp b/src/dialogs.hpp index d4feb92b6cce..cc588e6dc63b 100644 --- a/src/dialogs.hpp +++ b/src/dialogs.hpp @@ -119,9 +119,6 @@ class units_list_preview_pane : public dialogs::unit_preview_pane units_list_preview_pane(const std::vector &units, const gui::filter_textbox *filter = NULL, TYPE type = SHOW_ALL, bool left_side = true); - units_list_preview_pane(const std::vector &units, - const gui::filter_textbox *filter = NULL, - TYPE type = SHOW_ALL, bool left_side = true); private: size_t size() const;